fix: stop treating bare digit runs as SSN/PHONE by default (#158)

[sidmohan0]

Closes #158.

The bug

Dogfooding the Claude Code hook in real agent sessions, browser/MCP tool output like {"tabId": <9-digit>, "tabGroupId": <10-digit>} triggered SSN/PHONE warnings on nearly every tool call. Any bare 9-digit integer matched the SSN pattern and any bare 10-digit matched PHONE, so sessions touching tab ids, row ids, epoch timestamps, or ticket numbers got a constant stream of advisory noise — which trains users to ignore the firewall entirely, the one failure mode a security tool can't survive.

The fix

strict_numeric (default True) on scan()/redact():

• SSN requires a dash or space delimiter (NNN-NN-NNNN / NNN NN NNNN). Space delimiters are newly supported.
• PHONE requires a separator, parentheses, or a +country prefix.
• Delimited/formatted numbers still match exactly as before; the pre-existing 000/666 area, 00 group, and 0000 serial checks are unchanged.
• strict_numeric=False restores undelimited matching (v4.4.0 parity) as an opt-in.

Threaded through both agent adapters (they run strict). The hook README and plugin README document an ^\d{9}$|^\d{10}$ allowlist pattern as belt-and-braces.

The exact #158 payload now yields zero findings; a delimited SSN still matches.

Scope

This is a behavior change shipped as a patch with a prominent CHANGELOG note — flagging in case you'd rather signal it as 4.8.0. It deliberately reverses the v4.4.0 bare-9-digit SSN parity that was restored earlier (that parity is exactly what #158 is complaining about); parity is preserved as strict_numeric=False.

Broader structural validation (SSA area/group ranges, NANP area/exchange must start 2-9) is deferred to the v5 validator layer (DFPY-110) — it would reject the invalid placeholder values several test fixtures use, which is a larger change than a hotfix warrants.

Test plan

• New tests/test_numeric_precision.py (12 tests: bare-not-matched, delimited-still-matched, opt-in parity, the False positives: numeric IDs in structured tool output flagged as SSN/PHONE #158 JSON payload)
• Updated fixtures that encoded the old bare-digit behavior (corpus ssn-no-dashes/phone-plain-digits/passport-log, regex parametrize flips, DE-VAT parity test, allowlist-timestamp test) — dropping only bare-numeric expectations, preserving all else (e.g. PERSON)
• Full suite: 640 passed (3 skipped spacy-import failures are pre-existing/environmental)
• pre-commit clean
• CI green

[sidmohan0]

Agent review

Critical

• datafog.redact() likely raises TypeError on every call. datafog/__init__.py redact() wrapper now always forwards strict_numeric=strict_numeric to _redact(...) (the imported engine.redact). The diff stats for datafog/engine.py (+14 -1) are fully accounted for by the _regex_entities, scan(), and scan_and_redact() hunks alone -- there is no hunk touching a redact() function definition in engine.py. That means engine.redact() (the function actually invoked as _redact) was never given a strict_numeric parameter, so the wrapper call _redact(..., strict_numeric=strict_numeric) should fail with TypeError: redact() got an unexpected keyword argument 'strict_numeric' on every invocation, including the default case, since strict_numeric defaults to True and is unconditionally passed.
  • This would not be caught by the "640 passed" run if the existing suite only exercises scan_and_redact() (which was updated) rather than the top-level datafog.redact(). Please add a direct test for datafog.redact(...) (entities=None, default args) and confirm engine.redact() signature was updated to accept (or explicitly ignore) strict_numeric.
  • If engine.redact() genuinely does not need strict_numeric (e.g. it never re-scans internally), the fix is to not forward it from the wrapper rather than pass an argument the callee cannot accept.

High

• Default recall reduction for a PII redaction tool, shipped as a patch (4.7.0 -> 4.7.1). Bare/undelimited SSNs and phone numbers are extremely common in real-world exports, logs, and CSVs (e.g. a plain digit-string SSN field, phone columns stored as digits-only). With strict_numeric=True as the new default, datafog.redact()/scan() will silently stop flagging these unless callers explicitly opt in with strict_numeric=False. For consumers who pin patch ranges (~=4.7.0, ^4.7.0) and rely on this library for compliance-relevant redaction, a silent recall drop delivered as a patch release is a meaningful risk -- they will not see it unless they read the CHANGELOG. This tension is already flagged in the PR description; given the compliance angle specifically, consider shipping as a minor (4.8.0) release and calling it out in the GitHub Release notes (not just CHANGELOG.MD) so automated upgrades do not quietly reduce detection recall.

Medium

• Large, unrelated import-reformatting churn across roughly 15 files (datafog/__init___lean.py, __init___original.py, client.py, core.py, main.py, services/*.py, several tests/*.py, etc.). Multi-line parenthesized imports (one name per line, trailing comma -- the actual style Black produces) are being collapsed into a different style, and several single-name imports are rewritten using backslash continuations (e.g. from x import \ in donut_processor.py, text_service.py, test_runtime_dependency_safety.py). Black never emits backslash continuations for imports (it is a hard rule), so this reformatting was produced by a different tool/config than whatever normally formats this repo. This is unrelated to the SSN/PHONE fix, bloats the diff, and will likely get flipped back (or fail a black --check/isort --check step) the next time the actual pre-commit config runs. Worth reverting this churn and re-running the canonical formatter so the diff stays focused on the behavior change.

Low

• regex_annotator.py: the SSN delimiter character class matches any whitespace character (tab, newline, form feed, etc.), not just the literal space described in the PR/CHANGELOG ("space delimiter"). Consider restricting the class to dash-or-literal-space only, since as written a tab- or newline-separated digit run would also match as an SSN.
• Nice touch keeping every SSN/PHONE literal in tests/test_numeric_precision.py split across string concatenations so the file does not trip the project own Claude Code hook -- worth applying the same convention in test_regex_annotator.py / test_de_pii_regex.py, which still inline the literals directly.

The core regex changes themselves look correct: the mandatory-separator PHONE branch and dash/space-delimited SSN branch (verified via VERBOSE-mode whitespace stripping) correctly gate on delimiters in strict mode, and the strict_numeric=False opt-out branches restore exact v4.4.0 bare-digit behavior. The new tests/test_numeric_precision.py suite directly covers the #158 regression and the opt-in escape hatch.

[sidmohan0]

Agent review

No new commits since the last review. The PR head is still 1502e0a4 (the same SHA already reviewed) -- diffing it against itself confirms zero changes since that comment was posted. All prior findings are therefore still open; restating them here so they are not lost, plus confirming one via the diff itself.

Critical (unresolved from last review)

• datafog.redact() in datafog/__init__.py unconditionally forwards strict_numeric=strict_numeric to _redact(...). In datafog/engine.py, the only functions in this diff that gained a strict_numeric parameter are _regex_entities, scan(), and scan_and_redact() -- there is no hunk touching a top-level redact() definition. If _redact resolves to engine.redact (a function distinct from scan_and_redact), every call to datafog.redact(...) -- including the default, most common case -- should raise TypeError: redact() got an unexpected keyword argument 'strict_numeric'. This would not be caught by the "640 passed" suite unless a test calls the top-level datafog.redact() directly (as opposed to engine.scan_and_redact(), which was updated in this diff). Please add a direct datafog.redact(text, ...) test with default args and confirm engine.redact()'s signature actually accepts (or explicitly drops) strict_numeric before merge -- as written this looks like it would break the public redact API entirely.

High (unresolved)

• Shipping a recall-reducing default (strict_numeric=True) as a patch release (4.7.0 -> 4.7.1) is risky for a PII redaction tool: undelimited SSNs/phone numbers (common in raw exports, CSV columns, logs) will silently stop being flagged for anyone who upgrades within a ~=4.7/^4.7 pin and does not read the CHANGELOG. Given the compliance angle already flagged in the PR description, consider a minor bump (4.8.0) and calling this out in the GitHub Release notes, not just CHANGELOG.MD.

Medium (unresolved)

• Large, unrelated import-reformatting churn across roughly 15 files (client.py, core.py, main.py, services/*.py, several tests/*.py, donut_processor.py, etc.), including backslash-continuation imports (e.g. from x import \) that Black never produces for import statements. This is orthogonal to the SSN/PHONE fix, meaningfully inflates the diff, and will likely get reverted by the next black --check/isort --check run under this repo's actual pre-commit config. Worth reverting this churn and re-running the canonical formatter so the diff stays scoped to the behavior change.

Low (unresolved)

• regex_annotator.py: the SSN strict delimiter class [-\s] matches any whitespace character (tab, newline, form feed), not just the literal space the PR/CHANGELOG describe as the new delimiter. Consider narrowing to a dash-or-literal-space class if only those two are intended.
• tests/test_numeric_precision.py carefully splits every SSN/PHONE literal so this file does not trip the project's own Claude Code hook -- test_regex_annotator.py and test_de_pii_regex.py still inline the literals directly and could adopt the same convention.

The regex logic itself (mandatory-separator PHONE branch, dash/space-delimited SSN branch, strict_numeric=False opt-out restoring v4.4.0 parity) still looks correct and remains well covered by the new precision test suite. The blocking issue is the Critical item above -- until it is confirmed/fixed, this should not merge.

[sidmohan0]

Verdict: NOT_READY

Independently checked gh pr diff 162 against the reviewer's summary and confirmed the core blocking concern: datafog/engine.py only gains a strict_numeric parameter on _regex_entities, scan(), and scan_and_redact() — there is no matching hunk touching a top-level redact() in engine.py — while datafog/__init__.py's public redact() now unconditionally forwards strict_numeric=strict_numeric into _redact(...). If _redact resolves to engine.redact, every call to the public datafog.redact() entrypoint (the default call path, not just an edge case) would raise TypeError: redact() got an unexpected keyword argument 'strict_numeric'. This is a credible, unresolved CRITICAL/HIGH-severity break in the package's primary API surface.

The head commit (1502e0a4) is unchanged since this was first flagged, so the issue remains open. Also still outstanding: shipping a recall-reducing detection default as a patch release (4.7.0→4.7.1) for a PII-redaction tool, and ~15 files of unrelated import-reformatting churn bundled into a bugfix PR. The core regex/test changes for #158 itself look sound, but the plausible TypeError alone is enough to block merge pending human verification.

[sidmohan0]

@orchestrator re-review