Skip to content

fix(analyzer): stop PhoneRecognizer leaking a match's region into later matches - #2270

Draft
feiiiiii5 wants to merge 1 commit into
data-privacy-stack:mainfrom
feiiiiii5:fix/phone-recognizer-region-leak
Draft

feiiiiii5 wants to merge 1 commit into
data-privacy-stack:mainfrom
feiiiiii5:fix/phone-recognizer-region-leak

Conversation

@feiiiiii5

@feiiiiii5 feiiiiii5 commented Sep 18, 2026

Copy link
Copy Markdown

Problem

PhoneRecognizer.analyze assigned the region detected for one match back to region, the variable the outer loop is iterating, so it leaked into every later match in that pass and produced an analysis_explanation naming a region that never matched the number.

Reproduces with the default recognizer, no configuration — on main @ f251c513:

"My international number is +44 1234 567890, and my US one is (415) 555-0132"
  span=(27,42) score=0.4  'Recognized as GB region phone number'   +44 1234 567890   correct
  span=(61,75) score=0.4  'Recognized as GB region phone number'   (415) 555-0132    wrong

Swapping the two numbers makes both labels correct, so the behaviour is order-dependent — which is also why the suite does not catch it: every multi-number case in test_when_phone_with_textual_explanation_then_succeed puts the national-format number first.

Mechanism: phonenumbers.parse() is called without a default_region, so national-format input always raises NumberParseException and lands in the except branch, which reports whatever region currently holds. After any international number in the same pass, that is the previous match's region.

Full write-up with both orders: #2268

Change

Keep the detected value in a per-match name and never overwrite the loop variable; the except branch then reports the region the match was actually found under.

                 try:
                     parsed_number = phonenumbers.parse(text[match.start : match.end])
-                    region = phonenumbers.region_code_for_number(parsed_number)
-                    results += [
-                        self._get_recognizer_result(match, text, region, nlp_artifacts)
-                    ]
+                    matched_region = phonenumbers.region_code_for_number(parsed_number)
                 except NumberParseException:
-                    results += [
-                        self._get_recognizer_result(match, text, region, nlp_artifacts)
-                    ]
+                    matched_region = region
+                results += [
+                    self._get_recognizer_result(
+                        match, text, matched_region, nlp_artifacts
+                    )
+                ]

The two identical results += [...] blocks collapse into one as a consequence of the rename, which is why the diff is 7 lines rather than 2. Detection logic, PhoneNumberMatcher arguments, scores and dedup are untouched.

Verification

One venv with the project installed and the spaCy model present, Python 3.11; each command repeated on the pristine f251c513 tree as a control (file hashes compared so the "base" run provably loaded base code).

  • pytest tests/test_phone_recognizer.py tests/test_ph_mobile_number_recognizer.py tests/test_tr_phone_number_recognizer.py tests/test_recognizers_loader_utils.py tests/test_context_support.py -q
    • base with the new case: 1 failed, 436 passed, 2 skipped
    • this branch: 437 passed, 2 skipped
    • the single difference is the new case, so nothing that passed before now fails.
  • New case added to the existing parametrized test (the repo's own idiom for this recognizer) rather than a new file, asserting the international-first order yields GB then US. It fails on base with the exact mislabel from the issue, so it pins the behaviour rather than the implementation.
  • Both orders re-measured directly against the recognizer after the change: +44 first → GB, US; (415) first → US, GB. Spans and scores (0.4) are unchanged in every case, so the only thing the patch alters is the label text.
  • ruff check from the repository root with the version CI pins (requirements-ruff.txt → ruff 0.9.2): All checks passed. CI's lint job runs only ruff check (.github/workflows/ci.yml:38-39); ruff format would reformat tests/test_phone_recognizer.py on main as well, so I left the file's existing layout alone rather than mix a formatter sweep into this change.
  • Not run: the rest of presidio-analyzer/tests/ and the e2e suite. A full pytest tests collects modules that make real HuggingFace/Azure/stanza network calls in this environment, so I scoped to the files that exercise this recognizer plus the ones that import it, instead of reporting an unrun green.

Note on tracking: #2268 is the report for this defect. A second issue (#2269) describing the same root cause was filed from this same account about a minute later while two of my sessions were working in parallel; it has been closed as a duplicate so the report lives in one place.

Scope and limits

  • One root cause: the leaked loop variable. I deliberately did not add a fallback for the case where region_code_for_number returns an empty string (a parsed number with no assigned region) — today that renders as Recognized as region phone number, and whether it should fall back to the iterating region is a separate product decision. Happy to fold it in if you want it.
  • No behaviour change to what gets detected; the visible effect is the explanation string. Stating that plainly because it is a low-severity correctness bug, not a recall/precision one.
  • Expect a conflict with the open draft test(analyzer): strengthen GB phone regression coverage #2186 ("strengthen GB phone regression coverage"): it edits presidio-analyzer/tests/test_phone_recognizer.py too. Different concern — it is about DEFAULT_SUPPORTED_REGIONS using GB rather than UK — but whichever lands second needs a one-line rebase.
  • CHANGELOG.md entry added under Analyzer → Fixed, following the precedent of fix(analyzer): use valid region code GB instead of UK in PhoneRecognizer #2174 in this same recognizer.

Opening as draft: docs/development.md:70 asks for an issue before a PR and :73 for two maintainer approvals, so I would rather this sit visibly until someone confirms the direction and the empty-region question above.


Disclosure: prepared with an AI coding assistant. Every number above comes from a command run against main @ f251c513 and repeated on the unpatched tree with the identical command line; the recognizer output blocks were each produced by running that exact text.

…er matches

analyze() assigned region_code_for_number()'s result back to the region it was
iterating, so an international number that parsed cleanly overwrote the region
used to explain every later match in that pass. Hold it per match instead.
@feiiiiii5

Copy link
Copy Markdown
Author

Second verification pass on this same root cause from this account (two of my sessions worked it in
parallel; #2268 is the surviving report and this PR is the submission, so there is nothing new here to
review — only three measurements that bear on two sentences).

1. The empty-region case renders as None, not as an empty string. region_code_for_number returns
Python None for a number that parses but has no assigned region, so the f-string yields
Recognized as None region phone number rather than the Recognized as region phone number in the Scope
and limits
bullet. Measured with python-phonenumbers 9.0.34 (the locked version):

phonenumbers.region_code_for_number(phonenumbers.parse("+44 1234 567"))    -> None
phonenumbers.region_code_for_number(phonenumbers.parse("+1 555 1234"))     -> None

It is reachable through the documented leniency parameter — PhoneRecognizer(supported_regions=["GB"], leniency=0).analyze("+44 1234 567 or (212) 555-0187", ["PHONE_NUMBER"]) explains the first match as
None. That first-match None is not a leak (it is that number's own region_code_for_number result),
and the second match correctly becomes GB with this patch, so it is genuinely a separate decision — but
the sentence should say None, and ... or region on the try path would close it in the same line if a
maintainer wants one invariant ("the explanation always names a region that is attributable to this match",
no None in output). I could not produce it at the default leniency=1.

2. CHANGELOG.md conflicts with the current repo rule. AGENTS.md:28: "Do not edit CHANGELOG.md;
release entries are generated from merged PRs." The precedent cited in the notes, #2174, does contain a
CHANGELOG.md hunk — but it merged on 2026-07-21, before AGENTS.md landed (it came with the
review-instructions work in #2211). Worth either dropping the hunk or naming the conflict, since a
reviewer reading AGENTS.md today will stop on it.

3. Evidence you can fold in, all re-measured against main @ f251c513 with the same command line on
both trees (module path printed and asserted each time):

  • 5-case parametrized regression in test_phone_recognizer.py: 4 failed, 28 passed on base →
    32 passed on the fix (one case is a deliberate control that passes on base).
  • Mutation of the derived value back to the loop variable (match_region = region) → 11 red: the 4 new
    cases plus all 7 pre-existing test_when_phone_with_textual_explanation_then_succeed cases. That matches
    the 11 you got from the "loop region for every match" alternative, which is a useful cross-check: the
    shipped suite already mandates the number's-own-region semantics.
  • Whole tests/ directory both trees (13 model/Azure-dependent files excluded, same list on both legs):
    base 6 failed, 3287 passed, 3 skipped, branch 6 failed, 3292 passed, 3 skipped, and the 6 FAILED ids
    are identical (stanza / transformers engine-provider, GLiNER device, two recognizer-registry chunker
    configs — missing-model gaps in this sandbox). So "+5, nothing flipped" is provable, not implied.
  • Corpus check of the blast radius: 20 texts × 5 recognizer configs = 165 results, spans / entity types /
    scores identical, 20 textual_explanation strings differ, and an oracle that accepts a region only if it
    is either region_code_for_number(parse(fragment)) or a supported region whose PhoneNumberMatcher
    actually produced that span fails 20/165 on main and 0/165 on the fix (the 0 is without the or region
    guard; the None rows in point 1 are the residue). Digest unchanged under PYTHONHASHSEED 0 / 1 / 12345
    on both trees, so the deltas are not list(set(results)) ordering noise — RecognizerResult.__eq__ /
    __hash__ ignore analysis_explanation, so the surviving duplicate is the first inserted one.
  • Version range: presidio-analyzer/pyproject.toml constrains phonenumbers (>=9.0.28,<10.0.0); every
    number above was produced on the locked 9.0.34, so anyone checking the hardcoded spans on another
    in-range version should say so rather than assume.

Happy to hand over the test block or the corpus script if you want them in the Verification section rather
than re-deriving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant