perf(pii): replace resident RegexSet prefilter with byte-oriented candidate scan#119
Conversation
…filter Drop the always-resident combined SCREEN RegexSet (shared NFA + per-thread lazy-DFA caches) in favor of a single cheap byte pass that derives per-class candidate flags. Precise validation regexes stay LazyLock and now compile only when their class is actually flagged, so a workload only ever pays for the PII classes it sees. Each flag is a necessary condition of its precise regex, so redact_pii / has_likely_pii semantics are unchanged; the union is a superset of the old SCREEN set (pinned by prefilter_is_superset_of_legacy_screen). Adds per-class scan tests (incl. Devanagari/Japanese keywords), clean-text negatives, the bare-10-digit NANP guard, and the legacy-SCREEN parity oracle.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b289c732e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // E.164 `\+\d{7,15}` — a `+` and a 7+ digit run. | ||
| phone_e164: has_plus && max_digit_run >= 7, | ||
| // NANP — screen-entry necessary condition (internal separator). | ||
| phone_nanp: nanp_sep, |
There was a problem hiding this comment.
When content contains a separator-less country-code NANP number such as 12025551234, the old SCREEN matched the \d{11,} entry and then PHONE_NANP_RE redacted it; this new flag stays false because nanp_sep only records an internal separator, so collect_redactions_inner skips the phone regex and leaves the number unredacted. Keep this gate broad enough for the legacy long-digit SCREEN path while still excluding the documented bare 10-digit case.
Useful? React with 👍 / 👎.
| b"mynumber", | ||
| b"my number", |
There was a problem hiding this comment.
When the English keyword uses whitespace accepted by the precise regex, e.g. My\tNumber 123456789012 or a line break between the words, this keyword list does not set mynumber, so the My Number regex is never run and the 12 digits remain unredacted. The previous gate used My\s?Number, so the candidate scan needs to preserve that whitespace variant rather than only mynumber and my number.
Useful? React with 👍 / 👎.
| b'.' => has_dot = true, | ||
| b'-' => has_dash = true, | ||
| b'/' => has_slash = true, | ||
| b' ' | b'\t' => has_space = true, |
There was a problem hiding this comment.
When a formatted Aadhaar uses a newline or other \s separator, such as 2341\n2341\n2346, the legacy screen and AADHAAR_FMT_RE both match, but this scan only marks literal space/tab as has_space; without a dash and with only four-digit runs, aadhaar_fmt remains false and redaction is skipped. The candidate condition needs to cover the same whitespace class as the precise pattern.
Useful? React with 👍 / 👎.
| iban: bool, | ||
| cc: bool, | ||
| cnpj_bare: bool, | ||
| cpf_bare: bool, |
There was a problem hiding this comment.
This adds the byte prefilter directly to pii.rs, taking the implementation from 495 lines to 762 lines; the repository guideline asks source files stay under 500 lines and be split into focused modules before they grow past that point. Moving the candidate scan/types into a focused sibling module would keep this change within the documented structure.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
…odule
The byte prefilter dropped three PII cases the old resident RegexSet redacted,
because its candidate gates were narrower than the precise regexes they guard:
- Bare country-code NANP (`12025551234`): old SCREEN reached PHONE_NANP_RE via
the `\d{11,}` run. The new `phone_nanp` gate keyed only on an internal
separator, and the bare-CPF fallback's checksum rejects it — so it was left
unredacted. Gate now also fires on `max_digit_run >= 11` (a bare 10-digit run
still stays out of the phone path).
- English `My\s?Number` with a tab/newline separator: the keyword needle scan
only matched a literal-space `my number`. Now gates on both `my` and `number`
substrings, covering every `\s` variant.
- Newline-separated Aadhaar (`2341\n2341\n2346`): `has_space` tracked only
space/tab, but the precise `[\s-]` separator class includes `\n`/`\r`/etc.
Renamed to `has_ws` covering all ASCII whitespace.
Also splits the oversized `pii.rs` (762 -> 475 lines) into focused siblings
`pii/prefilter.rs` and `pii/normalize.rs`, per the <500-line repo guideline.
Adds regression tests for all three cases and extends the
`prefilter_is_superset_of_legacy_screen` corpus with the whitespace variants.
Addresses Codex review comments on PR #119.
|
Pushed 33e8ee7 addressing the Codex review comments. All three prefilter correctness flags were confirmed real regressions (verified empirically — the number was left unredacted under the new prefilter but redacted by the old resident RegexSet):
Regression tests added for all three cases, and the Heads-up on the red |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33e8ee7951
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| if matches!(b, b' ' | b'\t' | b'.' | b'-') && i > 0 && i + 1 < bytes.len() { |
There was a problem hiding this comment.
When a formatted NANP number uses another \s separator accepted by both the legacy screen and PHONE_NANP_RE, such as 415\n555\n0123, this gate never sets nanp_sep; the digit runs are only 3/3/4, so max_digit_run >= 11 is false as well and the phone regex is skipped, leaving the number unredacted. Include the same whitespace class here before deciding phone_nanp.
Useful? React with 👍 / 👎.
| #[derive(Default, Clone, Copy)] | ||
| pub(super) struct Candidates { |
There was a problem hiding this comment.
This introduces the new Candidates type directly in prefilter.rs, but the repository convention requires module type definitions to live in a dedicated types.rs; keeping the type in the implementation module makes the new split inconsistent with the documented module layout. Please move the type definition into an appropriate types.rs and import it from the prefilter implementation.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
1 participant
Why
OpenHuman resource profiling (openhuman
docs/resource-profiling-session-2026-07-21.md) identifiedpii::SCREEN— the always-resident 17-pattern multilingualRegexSetprefilter — as the dominant live Rust heap allocation family during normal memory capture: the combined NFA plus per-thread hybrid-DFA caches. The target deployment runs 100-1000 concurrent agents, so the per-thread caches multiply with thread count.What
Replaces
SCREENwithscan_candidates(): a single linear byte pass producing per-class boolean flags (digit-run stats, separator presence, NANP digit-sep-digit window, case-insensitive keyword probes including the non-Latin needles). Each flag gates whether that class's preciseLazyLock<Regex>is compiled and run, so:redact_pii/has_likely_piipublic API and semantics are unchanged (identical hits, priority, dedup order), including the documented "bare 10-digit NANP is never reached" behavior.Correctness
Every flag is a necessary condition of its class's precise regex, so the prefilter is a strict superset of the old screen: false positives possible (they just run a precise regex that then rejects), false negatives impossible. Pinned by
prefilter_is_superset_of_legacy_screen, which rebuilds the oldRegexSetat test time as an oracle over a mixed corpus.memory::store::safety60 passed / 0 failedMeasured (via the openhuman
library-profilebenchmark harness)memory-ingestandsubagentsscenarios: at par with baselines (within noise) — the single-process mock workloads have low PII density, so no regression on the common path.memory-ingest: old vs new essentially identical (Total 14.83 MB, t-gmax 1.12 MiB, t-end 0.44 MiB).Verbose
[pii]debug logging added on the scan paths; matched text is never logged.