Skip to content

feat: add regex-based PII detection and sanitization module - #182

Closed
travisbreaks wants to merge 1 commit into
MemPalace:developfrom
travisbreaks:feat/pii-guard
Closed

feat: add regex-based PII detection and sanitization module#182
travisbreaks wants to merge 1 commit into
MemPalace:developfrom
travisbreaks:feat/pii-guard

Conversation

@travisbreaks

Copy link
Copy Markdown
Contributor

Summary

Addresses #118 (scoped-down first pass: regex-only, zero new dependencies).

Adds mempalace/pii_guard.py with a PIIGuard class that provides:

sanitize(text) -> (sanitized_text, mapping)

Replaces PII with deterministic tokens. Same PII value always produces the same token.

from mempalace.pii_guard import PIIGuard

guard = PIIGuard()
sanitized, mapping = guard.sanitize("Email me at alice@example.com")
# sanitized: "Email me at [EMAIL_a1b2c3]"
# mapping: {"[EMAIL_a1b2c3]": "alice@example.com"}

restore(sanitized_text, mapping) -> original_text

Round-trip restoration using the mapping.

Detected PII types

Type Examples
EMAIL alice@example.com
PHONE 555-123-4567, (555) 123-4567, +1 555.123.4567
SSN 123-45-6789
CREDIT_CARD Visa, Mastercard, Discover
IP_ADDRESS 192.168.1.100
DATE_OF_BIRTH DOB: 03/15/1990, born 1990-03-15

Configuration

  • enabled_types: limit which PII types to detect
  • custom_patterns: add your own regex patterns alongside built-ins
  • has_pii(): quick boolean check
  • summary(): grouped counts by PII type

Design decisions

  • Zero new dependencies: uses only stdlib re and hashlib, respecting CONTRIBUTING.md's "ChromaDB + PyYAML only" policy
  • Deterministic tokens: enables consistent replacement across repeated occurrences
  • Overlap handling: prevents double-replacement when PII patterns overlap
  • Pluggable interface: future NER-based detection (spaCy, presidio) can extend this without changing the API

What this doesn't do (future PRs)

  • NER-based name/org detection (would require new dependency discussion)
  • MCP tool integration (mempalace_sanitize / mempalace_restore)
  • Encrypted-at-rest mapping storage
  • Automatic sanitization in the mine pipeline

Test plan

  • pytest tests/ -v passes (124 tests, 23 new)
  • ruff check and ruff format --check pass
  • New tests cover:
    • Detection of all 6 PII types
    • Multiple format variations (6 phone formats, 3 CC types, 3 DOB formats)
    • False positive resistance on clean text
    • Sanitize/restore round-trip fidelity
    • Deterministic token generation
    • Type filtering via enabled_types
    • Custom patterns alongside built-ins
    • Overlap handling
    • Edge cases (empty strings, PII at boundaries)

🤖 Generated with Claude Code

Addresses MemPalace#118 (scoped-down first pass).

Adds `mempalace/pii_guard.py` with:
- `PIIGuard` class with `sanitize()` / `restore()` round-trip interface
- Deterministic token replacement (same PII value always maps to same token)
- Regex detection for: email, phone (US formats), SSN, credit card
  (Visa/MC/Discover), IPv4 addresses, dates of birth
- Configurable: enable/disable PII types, add custom regex patterns
- Overlap handling: prevents double-replacement on adjacent PII
- `has_pii()` quick check and `summary()` for reporting
- Zero new dependencies (stdlib `re` + `hashlib` only)

This is the foundation layer. Future work (NER-based name detection,
MCP integration, encrypted mapping storage) can build on this interface
without changing the API.

23 tests covering detection, sanitization, restoration, configuration,
and edge cases.

Co-Authored-By: Tadao <tadao@travisfixes.com>
@bgauryy

bgauryy commented Apr 8, 2026

Copy link
Copy Markdown

PR Review: feat: add regex-based PII detection and sanitization module

Executive Summary

Aspect Value
PR Goal Add a standalone PIIGuard class for regex-based PII detection, sanitization, and round-trip restoration
Files Changed 2 (both new: mempalace/pii_guard.py, tests/test_pii_guard.py)
Risk Level 🟡 MEDIUM — standalone module with no callers yet, but contains a data-loss bug in its core round-trip logic
Review Effort 2/5 — small, focused PR with clear scope
Recommendation 🔄 REQUEST_CHANGES

Affected Areas: mempalace/pii_guard.py (new module), tests/test_pii_guard.py (new tests)

Business Impact: PII sanitization protects user data during mining/storage. A collision bug in the token system can silently corrupt text, undermining the safety guarantee this module is supposed to provide.

Flow Changes: None yet — module is not wired into any existing pipeline. Future integration with miner.py / convo_miner.py expected.

Ratings

Aspect Score
Correctness 3/5
Security 3/5
Performance 4/5
Maintainability 4/5

PR Health


High Priority Issues

🐛 #1: Token collision causes silent data loss in round-trip

Location: mempalace/pii_guard.py_make_token() | Confidence: ✅ HIGH

_make_token truncates SHA-256 to 6 hex characters (24 bits ≈ 16.7M possible values). Two different PII values of the same type can produce identical tokens. When this happens, sanitize() writes both originals into the mapping dict under the same key — the second silently overwrites the first. restore() then returns corrupted text with one PII value replacing the other.

For a module whose core promise is safe round-trip sanitization, this is a data integrity defect.

 def _make_token(pii_type: str, value: str) -> str:
-    digest = hashlib.sha256(value.encode()).hexdigest()[:6]
+    digest = hashlib.sha256(value.encode()).hexdigest()[:16]
     return f"[{pii_type}_{digest}]"

Alternatively, detect collisions in sanitize() and append a counter suffix when a collision occurs.


🐛 #2: Overlap resolution keeps rightmost match, not longest

Location: mempalace/pii_guard.pydetect() overlap filtering | Confidence: ✅ HIGH

The comment says "Remove overlapping matches (keep the first/longest one found)" but the algorithm sorts descending by start index and iterates in that order. This means the match with the highest start position wins — not the longest or the most specific. When an SSN like 123-45-6789 partially overlaps with a PHONE pattern, the winner is determined by position, not specificity or length.

-        matches.sort(key=lambda m: m.start, reverse=True)
-
-        # Remove overlapping matches (keep the first/longest one found)
-        filtered = []
-        used_ranges = []
-        for match in matches:
+        # Remove overlapping matches — keep the longest span
+        matches.sort(key=lambda m: (m.end - m.start), reverse=True)
+        filtered = []
+        used_ranges = []
+        for match in matches:
             overlaps = any(
                 match.start < ur_end and match.end > ur_start for ur_start, ur_end in used_ranges
             )
             if not overlaps:
                 filtered.append(match)
                 used_ranges.append((match.start, match.end))
+
+        # Re-sort descending by position for correct replacement order
+        filtered.sort(key=lambda m: m.start, reverse=True)

Medium Priority Issues

#3: Credit card regex too permissive — matches 13-digit sequences

Location: mempalace/pii_guard.pyPII_PATTERNS["CREDIT_CARD"] | Confidence: ⚠️ MED

The trailing \d{1,4} allows matches as short as 13 digits (e.g. 4111 1111 1111 1). Additionally, Amex cards use a 4-6-5 grouping (15 digits total) which doesn't match the assumed 4-4-4-N layout. The PR description claims Amex support.

-    "CREDIT_CARD": re.compile(
-        r"\b(?:4\d{3}|5[1-5]\d{2}|3[47]\d{2}|6(?:011|5\d{2}))"
-        r"[\s-]?\d{4}[\s-]?\d{4}[\s-]?\d{1,4}\b"
-    ),
+    "CREDIT_CARD": re.compile(
+        r"\b(?:"
+        r"(?:4\d{3}|5[1-5]\d{2}|6(?:011|5\d{2}))[\s-]?\d{4}[\s-]?\d{4}[\s-]?\d{4}"  # 16-digit: Visa/MC/Discover
+        r"|"
+        r"3[47]\d{2}[\s-]?\d{6}[\s-]?\d{5}"  # 15-digit: Amex (4-6-5)
+        r")\b"
+    ),

🔒 #4: Deterministic tokens are trivially brute-forceable

Location: mempalace/pii_guard.py_make_token() | Confidence: ⚠️ MED

The design uses bare SHA-256(value)[:6] for token generation. Since there's no secret key, an attacker with the sanitized text can reverse common PII values (emails, SSNs) via dictionary lookup in seconds. This is acceptable if the threat model only covers casual exposure, but should be explicitly documented. For stronger guarantees, use hmac.new(secret_key, value, hashlib.sha256) instead.

At minimum, add a docstring/comment stating the threat model: "Tokens resist casual inspection but are not cryptographically secure against targeted reversal."


🎨 #5: Missing Amex credit card test

Location: tests/test_pii_guard.pytest_detect_credit_card | Confidence: ✅ HIGH

The PR description and regex explicitly include Amex (3[47]\d{2}) but the test only covers Visa, Mastercard, and Discover. Adding an Amex test will also surface the regex issue from #3.

         cards = [
             "4111 1111 1111 1111",  # Visa
             "5500-0000-0000-0004",  # Mastercard
             "6011 1111 1111 1117",  # Discover
+            "3782 822463 10005",    # Amex (4-6-5 grouping)
         ]

Low Priority Issues

🎨 #6: Loose type hints reduce IDE support

Location: mempalace/pii_guard.pyPIIGuard class | Confidence: ✅ HIGH

Several return types and field annotations use bare list, dict, tuple, set instead of parameterized generics. This weakens static analysis and IDE autocompletion.

-    enabled_types: set = field(default_factory=lambda: set(PII_PATTERNS.keys()))
-    custom_patterns: dict = field(default_factory=dict)
+    enabled_types: set[str] = field(default_factory=lambda: set(PII_PATTERNS.keys()))
+    custom_patterns: dict[str, re.Pattern] = field(default_factory=dict)

-    def detect(self, text: str) -> list:
+    def detect(self, text: str) -> list[PIIMatch]:

-    def sanitize(self, text: str) -> tuple:
+    def sanitize(self, text: str) -> tuple[str, dict[str, str]]:

🏗️ #7: No pipeline integration (follow-up)

Location: N/A | Confidence: ✅ HIGH

The module is entirely standalone — not imported or called by miner.py, convo_miner.py, or mcp_server.py. This is acceptable per the PR's "first pass" scope, but should have a tracked follow-up to wire it into the mining pipeline so it actually protects user data.


Created by Octocode MCP https://octocode.ai 🔍🐙

@web3guru888 web3guru888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of #182feat: add regex-based PII detection and sanitization module

Scope: +392/−0 · 2 file(s)

  • mempalace/pii_guard.py (added: +178/−0)
  • tests/test_pii_guard.py (added: +214/−0)

Suggestions

  • 📋 PR checklist: 2/3 completed — 1 item(s) still unchecked

Strengths

  • ✅ Includes test coverage

🟢 Approved — clean, well-structured PR. Good work @travisbreaks!


🏛️ Reviewed by MemPalace-AGI · Autonomous research system with perfect memory · Showcase: Truth Palace of Atlantis

@bensig
bensig changed the base branch from main to develop April 11, 2026 22:23
@bensig

bensig commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

hey @travisbreaks — thanks for this, the PIIGuard design is clean.

we're going to hold off though — v4 is bringing local NLP providers (#507) which will enable NER-based PII detection that catches things regex can't (names in context, addresses, etc). we don't want to ship a regex-only implementation now and replace it shortly after.

the deterministic token mapping pattern is something we'll likely reuse in the NLP-based version. appreciate the work.

@bensig bensig closed this Apr 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants