feat(pii): add v3 replace_pii config and tabular replacer

[nina-xu]

Summary

Suggested reading order for review

The finished design is one cohesive cutover (new engine + delete NER/data_editor). Intermediate “split PRs” are not independently shippable on main. Read the current tree in this order — the rest of this doc follows the same sequence:

1. Contract — config/replace_pii.py → entities.py (EntitySpec) → models.py
2. Plan lifecycle — planning/validation.py → planning/io.py → preflight/checks/pii.py
3. Detection — detection/ → patterns/ → planning/discovery.py
4. Entity handlers — entity_handlers.py (thin adapters for generate/skip/pattern rejection)
5. Replacement — replacement/ (scope → instances → personas → standalone → free_text → apply)
6. LLM seams — llm/ (PiiEnhancer)
7. Entry / cutover — replacer.py, sdk/library_builder.py, deletions, docs
8. Tests last — tests/pii_replacer/ (mirrors packages) + golden plans

Implementation walkthrough (same order, with file paths):
pii-repl-v3-behavior-impl-notes.md

Release plan: we'll get this PR reviewed and ready for merging, but we won't merge just yet. We'll then review #665 , merge that into this branch; add the LLM mode into this branch. At that point we'll do a final check on the huge branch and merge & release.

---

What users experience

When replace_pii is enabled (the default), the pipeline still runs PII replacement on the training dataframe before training. The mechanism is new:

1. Resolve a plan — auto-discover, load a YAML file, or use an inline plan.
2. Validate the plan against the dataframe (and, for user plans, also during --validate / early preflight).
3. Apply synthetic replacements, then propagate those values into free-text columns.
4. Write pii_replacement_plan.yaml into the run directory so the discovered (or used) plan can be edited and reused.

Turning it off is unchanged in spirit: replace_pii: null, --no-replace-pii, or .with_replace_pii(enable=False).

---

Config shape (replace_pii)

Old step/NER/classify YAML (PiiReplacerConfig) is gone. The new top-level object is ReplacePiiConfig:

Field	Behavior
replacement_plan	"auto_discovery" (default), a path to a plan YAML, or an inline plan (config file only)
replacement.locale / seed	Locale and RNG seed for generation
person.backend	managed (default), faker, or internal-only pgm
person.managed_assets_path / sdg_pgms_src	Asset / PGM source locations
schema_version	1 — forward-compat gate for config shape (not plan YAML)
llm_enhancement	Reserved; true is a preflight error and raises ParameterError from the PiiEnhancer seams when discovery/apply run
llm	Reserved LLM settings (unused while enhancement is unimplemented)

There is no discovery.replace_group_key (removed). Whether a group key may be replaced is governed by protected columns (below), not a discovery toggle.

A plan has:

• scope: record | group | dataframe — how widely one original value keeps the same synthetic value (applies to personas and standalone identifiers/phones/cards/etc.)
• persona_backed_columns: named personas; columns filled from one synthetic identity; optional match_persona_by (gender / ethnic_background) columns that are read only
• standalone_columns_to_replace: IDs, cards, free text, etc., replaced independently

Each column entry is column_name + entity_type + optional patterns (formats for writing values).

Important rule: the engine follows entity type, not YAML section. Putting an ID under a persona does not make it persona-sourced; putting a name only under standalone means it will not share a synthetic person with other name columns. Preflight/apply emit warnings for those mismatches; they do not rewrite the plan.

Protected columns (structural — never replaced)

Some columns define training structure and must stay intact:

Column	When protected
data.group_training_examples_by	Only in time-series mode (time_series.is_timeseries)
data.order_training_examples_by	Whenever set (any mode)
time_series.timestamp_column	Only in time-series mode

• Auto-discovery: protected columns are stripped from the plan before emit (with a user warning if anything was dropped).
• User-supplied plans: listing a protected column under columns_to_replace is a validation error (pii_plan_protected_column) — the plan is not silently rewritten.
• Non–time-series grouping: if you set group_training_examples_by outside TS mode, that column is not protected. Discovery may plan it as unique_identifier (unless other heuristics skip it, e.g. contiguous integer sequences). Scope can still be group so replacements stay consistent within each group.

---

Discovery (auto plan)

With replacement_plan: auto_discovery, discovery:

• Requires a column-name match (regex + fuzzy, threshold 0.86) before assigning any replaceable entity. Value evidence alone never plans replacement.
• Where a simple content check exists, also requires it (email/phone/SSN/card/IP regexes ≥85% dominant coverage; credential-like API keys; street house-number evidence; parseable DOB). Name-only is enough for entities without a practical content gate (e.g. some unique_identifier / national_id shapes).
• No cardinality gate — low distinct-count columns are still planned when name (+ content) match.
• Persona naming from role prefixes when possible (patient_first_name → persona patient, provider_email → provider); unprefixed columns use person_1, person_2, …. Same role → same pool; duplicate entity label or disagreeing name parts → patient_2 / provider_2 + warning.
• Name consistency before merging first_name / last_name / full_name onto one persona (~85% agreement over ≥3 comparable rows); otherwise split + warn.
• Puts entity-driven columns (IDs, cards, IPs, DOB, SSN, national_id, non-pgm phones) in standalone; marks long prose columns as free_text when eligible.
• Sets scope to group only if group_training_examples_by is set and that column exists in the dataframe; otherwise dataframe (+ warning if the key was set but missing).
• Marks some name-matched columns as identified, not replaced (apply_path: identify_only): address parts city / state / zipcode (name-matched) and generic temporals date / datetime / time / duration (value-based — no name match needed, so oddly named date columns still qualify). These are logged, omitted from the emitted plan, and excluded from the free-text scan so their values are neither replaced nor propagated, because these columns should not contain any free-text PII.
• Free-text scan (heuristics mode): only when at least one structured replaceable column exists (persona-backed or standalone entity). Identify-only columns (temporals, city/state/zip) alone do not enable the scan. If nothing structured was found, text-like columns are listed in a user warning and omitted.
• Infers patterns from observed value shapes (names/emails as persona placeholders; DOB as strftime; IDs/phones as character templates). IPv4/IPv6, SSN, national_id, street_address, free_text get no templates.
• Skips contiguous sequential integer columns when they are planned as unique_identifier; gapped numeric IDs can still be planned. Numeric columns headed ssn / national_id keep that entity (they are not collapsed to unique_identifier and are not sequential-skipped).
• Requires house-number-like evidence for street_address.
• May probe numeric columns for compact YMD DOBs / long digit IDs when headers suggest it.

No LLM or NER model is used. Enabling llm_enhancement fails at the PiiEnhancer seams (not a silent alternate path).

---

Replacement (exact apply behavior)

Persona-sourced (from one synthetic identity when listed under a persona): names, email, street address. Phone is persona-sourced only under pgm; under managed/faker it is rebuilt from the column’s format like other identifiers.

Entity-driven (scoped original→synthetic maps; standalone path always): unique IDs, cards, API keys, IPs, DOB (age-preserving perturbation keeping format), SSN, national_id, and non-pgm phones. scope is honored for these maps (dataframe / group / record). Under scope: record, one map is built per row — fine for typical samples; a warning fires above 25k rows.

Standalone persona entities (name/email/etc. listed only under standalone): replaced with real Faker entity values (not character-class noise), without sharing a persona across columns. Placement advisories still warn.

Identified, not replaced (city / state / zipcode / generic temporals): pass through unchanged. If such a label somehow reaches apply (e.g. a hand-written plan), its standalone map is empty and the column is left as-is. entity_type: date in a user plan is a validation error (pii_plan_entity_type_invalid) — use date_of_birth or drop the column.

Patterns (user plans): patterns are optional. Missing/empty lists are normal — persona columns use default persona formatting; standalone columns fall back to Faker/shape-preserving generation (or value-inferred DOB format). Wrong-family patterns (e.g. full_name with %Y/%m/%d) are rejected at validation as pii_plan_pattern_invalid, not applied.

Free text: does not replace the whole cell; substitutes values already replaced from persona-backed instances for that row (plus name tokens for partial mentions like “Dr. Smith”), and scoped standalone pairs for that row. Matching is case-insensitive; the synthetic is reshaped to the matched token’s case (upper / lower / title / else as stored). Word-boundary matching; punctuation around structured name columns is stripped for token aliases. Pairs are row-/instance-local (no merging every persona in a group into every note). A warning fires if a row exceeds 500 substitution pairs.

Persona backends:

• managed — sample datasets/{locale}.parquet under NSS_MANAGED_ASSETS_PATH or ~/.data-designer/managed-assets; missing assets fall back to Faker with a warning. Column stats report the effective backend after fallback.
• faker — Faker personas; locale must be supported. Auto-discovery omits ethnic_background matchers under faker; hand-written ethnic matchers are ignored with an advisory.
• pgm — local sdg_pgms checkout; no fallback; en_US only; only backend that supplies persona phone numbers.

match_persona_by conditions which persona is drawn (e.g. sex→gender for name agreement). Condition columns are never replaced.

Non-unique DataFrame indices are reset to a positional index (runtime warning) before apply.

After apply, column stats for the evaluation report record transform methods (personas / Faker / PGM, pattern, perturbation, propagation).

---

Validation and preflight

User-supplied plans are checked in early preflight (PiiPlanValidityCheck) so --validate catches bad plans without running replacement. Errors include unknown/duplicate columns, missing entity_type, entity_type: date, patterns that don’t match values or don’t apply to that entity, group scope without a group key, persona/matcher conflicts, and protected columns listed for replacement.

Late full preflight skips pii.plan_validity (already checked early on the full input) — it focuses on post-transform concerns (token budget, etc.).

Auto-discovered plans are validated when replacement runs (after protected columns are stripped), not via the user-plan preflight check. Auto validate_plan failures are reported as discovery/internal errors (message prefixed), except clearly user-actionable group-scope issues.

Separate config checks (early + late):

• Faker locale invalid → error
• Managed assets missing → warning (apply falls back to Faker)
• PGM source / locale → error
• llm_enhancement: true → error
• Legacy replace_pii.globals / steps → always rejected (even under unknown_fields: ignore)

---

What was removed

• Entire NER stack, GLiNER/regex entity extractors, and NemoPII / data_editor (including Jinja/Faker template execution).
• Old replace_pii step/classify/NER config.
• Expression-based data actions (GenExpression, GenFaker, etc.) that depended on that engine.
• Dead fragment.py and NER metadata on field features.
• discovery.replace_group_key (replaced by protected-column rules).
• Cardinality accept/reject gate for entity typing (low_card_max).
• Binary primary/secondary persona bucketing (replaced by column-derived role keys).

Typed ActionExecutor actions remain but are not on the default PII path.

---

Net behavioral change vs main

Before	After
NER + optional LLM classification; transform steps / Jinja	Heuristic discovery + declarative plan + programmatic replace
Large entity vocabulary / NER models	Closed entity set above
GPU/model deps for detection	CPU heuristics only
Opaque transforms	Editable pii_replacement_plan.yaml round-trip

Default runs still replace PII automatically; the plan file in the run dir is the main new control surface for audit and override. Always review it before production / regulated use.

Auto-discovery MVP expectations

Works best on English, US-ish, well-named tabular schemas. Expect to hand-author plans for narrative-heavy data, international IDs/phones, opaque tokens, and oddly named columns (name match is required). Free text is not a redaction NER — it only propagates persona (and per-row standalone) substitutions.

---

Testing Plan

Paths below assume datasets live in /root/datasets and configs in /root/configs. Artifacts land under safe-synthesizer-artifacts/<config>---<dataset>/<timestamp>/ (plan as pii_replacement_plan.yaml, transformed data as dataset/transformed_training.csv when PII changed anything).

For PII-only checks, prefer the SDK TabularPiiReplacer snippets (no GPU/train). Use CLI --validate for plan/preflight, and a tiny TinyLlama run only when you need end-to-end artifact wiring.

Dataset map

Dataset	Why use it	Behaviors exercised
/root/datasets/telco_churn.csv	Flat table: CustomerID, Name, Gender, city/state/zip	Auto-discovery; full_name + match_persona_by gender; ID patterns; dataframe scope; unprefixed → person_N
/root/datasets/patient_events.csv	Multi-row patients	Group scope; role personas (patient, provider); name-part agreement; DOB; free-text propagation
/root/datasets/bike_sales.csv	Split name parts + address + phone + DOB	Persona-sourced names; street (needs house #); phone entity-driven under managed/faker
/root/datasets/crm/Contact.csv	CRM person record	Email + phone patterns; street address
/root/datasets/pii_dataset_no_label.csv or personal_bios.csv	Structured PII + long bio	Free-text propagation (case-insensitive)
/root/datasets/call_transcripts.csv / clinc_oos.csv	Mostly free text	No structured PII → free-text scan skipped
Time-series config + grouped data	Group + timestamp + order-by	Protected columns

Suggested matrix

Priority	Scenario	Dataset
P0	Auto-discovery + plan emit	telco_churn, patient_events
P0	Group consistency	patient_events
P0	Free-text propagation + case fold	pii_dataset_no_label, patient_events
P0	--validate bad / protected plan	telco + hand-written YAML
P0	Role personas + name agreement	patient_events
P1	Plan round-trip edit	patient_events
P1	Record-scope / managed fallback / TS protected	synthetic / telco / TS config
P2	No structured PII → skip free text	call_transcripts, clinc_oos
P2	Golden discovery fixtures	tests/pii_replacer/test_discovery_golden.py

What to inspect after each run

1. pii_replacement_plan.yaml — scope, role/person_N personas, matchers, patterns, free_text, protected omissions
2. Logs — placement advisories, managed-fallback, free-text skip, name-agreement / role-collision warnings
3. Transformed vs original data — consistency within scope
4. Evaluation column stats / transform methods

Pre-Review Checklist

Ensure that the following pass:

• mise run format && mise run check or via prek validation.
• mise run test passes locally
• mise run test:e2e passes locally
• mise run test:ci-container passes locally (recommended)
• GPU CI status check passes -- comment /sync on this PR to trigger a run (auto-triggers on ready-for-review)

Pre-Merge Checklist

• New or updated tests for any fix or new behavior
• Updated documentation for new features and behaviors, including docstrings for API docs.

Other Notes

• Closes [PII] Better column name fuzzy matching for MVP #630 , [PII] Align the prototype with the new config design #631 , [PII] testing on datasets #632

Summary by CodeRabbit

• New Features

  • Added heuristic tabular PII discovery using column names, values, and formats.
  • Added replacement plans with automatic discovery, YAML import/export, scopes, and validation.
  • Added synthetic persona replacement with consistent identities, format preservation, locale support, and Faker fallback.
  • Added propagation of structured replacements into free-text fields.
  • Added detailed replacement statistics and preflight diagnostics.

• Bug Fixes

  • Improved handling of missing values, protected columns, invalid plans, and unsupported settings.

• Documentation

  • Updated PII replacement guides, configuration references, troubleshooting, and environment settings.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The PR replaces the legacy NER-based PII workflow with heuristic tabular discovery, declarative replacement plans, persona generation, scoped mappings, free-text propagation, and new preflight validation. It also removes obsolete NER and transformation modules.

Changes

Tabular PII replacement

Layer / File(s)	Summary
Replacement configuration and migration src/nemo_safe_synthesizer/config/..., src/nemo_safe_synthesizer/cli/..., src/nemo_safe_synthesizer/data_processing/...	Replaced the legacy configuration with declarative replacement plans, persona settings, managed assets, and migration errors for legacy keys.
Heuristic detection and typed contracts src/nemo_safe_synthesizer/pii_replacer/detection/..., src/nemo_safe_synthesizer/pii_replacer/entities.py, src/nemo_safe_synthesizer/pii_replacer/models.py	Added column-name, value-pattern, scope, persona, and free-text detection with typed discovery and replacement models.
Pattern inference and persona handlers src/nemo_safe_synthesizer/pii_replacer/patterns/..., src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py	Added persona, temporal, identifier, phone, and card pattern handling with entity-specific generation rules.
Plan discovery and validation src/nemo_safe_synthesizer/pii_replacer/planning/..., src/nemo_safe_synthesizer/preflight/checks/pii.py	Added automatic, inline, and file-backed plan resolution, YAML persistence, protected-column checks, pattern validation, and preflight reporting.
Scoped replacement execution src/nemo_safe_synthesizer/pii_replacer/replacement/..., src/nemo_safe_synthesizer/pii_replacer/replacer.py	Added persona synthesis, scoped standalone mappings, free-text propagation, replacement execution, statistics, and the TabularPiiReplacer entry point.
Runtime integration and regression coverage src/nemo_safe_synthesizer/sdk/..., src/nemo_safe_synthesizer/preflight/..., tests/pii_replacer/..., docs/...	Updated SDK integration, preflight stages, rendering, documentation, fixtures, golden plans, and tests for the new replacement flow.

Estimated code review effort: 5 (Critical) | ~120 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR implements structure-aware matching and expanded role keywords for #630, but it does not implement the requested small LLM evaluation.	Implement the small LLM evaluation or split that objective into a follow-up issue with explicit scope.
Out of Scope Changes check	⚠️ Warning	The PR includes a full replacement-system rewrite, legacy NER removal, configuration migration, and broad documentation changes beyond #630's fuzzy-matching objectives.	Split the broader replacement-system rewrite into separate scoped pull requests, or link issues that cover those additional objectives.
Docstring Coverage	⚠️ Warning	Docstring coverage is 52.75% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main changes: a new v3 replace_pii configuration and tabular replacer.

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch nina-xu/pii-repl-v3-alt-config

---

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 89.44374% with 334 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...e_synthesizer/pii_replacer/replacement/personas.py	70.76%	69 Missing ⚠️
...esizer/pii_replacer/detection/value_recognizers.py	81.95%	48 Missing ⚠️
...o_safe_synthesizer/pii_replacer/entity_handlers.py	76.31%	45 Missing ⚠️
...hesizer/pii_replacer/detection/persona_grouping.py	87.09%	28 Missing ⚠️
src/nemo_safe_synthesizer/pii_replacer/models.py	86.06%	28 Missing ⚠️
...safe_synthesizer/pii_replacer/replacement/apply.py	90.52%	18 Missing ⚠️
...synthesizer/pii_replacer/replacement/standalone.py	83.00%	17 Missing ⚠️
...nthesizer/pii_replacer/patterns/value_templates.py	92.52%	16 Missing ⚠️
...hesizer/pii_replacer/patterns/persona_templates.py	94.55%	11 Missing ⚠️
..._synthesizer/pii_replacer/replacement/free_text.py	90.72%	9 Missing ⚠️
... and 15 more

📢 Thoughts on this report? Let us know!

[nina-xu]

@greptile-apps

[greptile-apps]

Greptile Summary

The PR replaces the legacy NER/Jinja PII stack with heuristic, plan-driven tabular discovery and deterministic persona-aware replacement.

• Adds declarative replacement-plan discovery, validation, serialization, and preflight integration.
• Adds scoped persona, standalone-value, and free-text replacement paths with configurable backends.
• Removes the legacy NER, GLiNER, data-editor, and expression-based replacement implementation.
• Aligns discovery and validation pattern evidence, resolving the previously reported high-cardinality sampling failure.

Confidence Score: 5/5

The PR appears safe to merge with respect to the previously reported sampling issue.

No blocking failure remains; discovery and validation now use the same deterministic evidence slice for identifier, phone, card, API-key, and date patterns, and focused regression coverage exercises the original late-format scenario.

Important Files Changed

Filename	Overview
src/nemo_safe_synthesizer/pii_replacer/planning/validation.py	Validates replacement plans and now checks patterned values using the same deterministic evidence slice as discovery.
src/nemo_safe_synthesizer/pii_replacer/patterns/temporal.py	Defines the shared seeded pattern-evidence sampling contract used by discovery and validation.
tests/pii_replacer/planning/test_discovery.py	Adds regression coverage proving late secondary identifier formats no longer cause discovered-plan validation failures.
src/nemo_safe_synthesizer/pii_replacer/planning/discovery.py	Orchestrates heuristic structured discovery, pattern attachment, free-text eligibility, and plan construction.
src/nemo_safe_synthesizer/pii_replacer/replacer.py	Introduces the main tabular PII replacement orchestration and artifact lifecycle.

Sequence Diagram

sequenceDiagram
    participant User
    participant Config
    participant Preflight
    participant Discovery
    participant Validation
    participant Replacement
    participant Artifacts
    User->>Config: Configure replace_pii
    Config->>Preflight: Resolve user plan or auto mode
    Preflight->>Validation: Validate user-supplied plan
    Config->>Discovery: Discover plan from dataframe
    Discovery->>Validation: Validate discovered plan
    Validation->>Replacement: Apply scoped replacements
    Replacement->>Replacement: Propagate replacements into free text
    Replacement->>Artifacts: Emit transformed data and plan YAML

Loading

_{Reviews (4): Last reviewed commit: "architectural refactoring and style guid..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 20

🧹 Nitpick comments (15)

src/nemo_safe_synthesizer/config/pii_replacement.py (1)

91-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add descriptions to all user-facing Pydantic fields.

The new plan and configuration fields omit required Field(description=...) metadata. This removes field documentation from generated configuration help and API references.

• src/nemo_safe_synthesizer/config/pii_replacement.py#L91-L92: Add descriptions for persona matching fields.
• src/nemo_safe_synthesizer/config/pii_replacement.py#L103-L111: Add descriptions for column-plan fields.
• src/nemo_safe_synthesizer/config/pii_replacement.py#L132-L147: Add descriptions for persona-set and plan fields.
• src/nemo_safe_synthesizer/config/pii_replacement.py#L153-L161: Add descriptions for LLM and replacement-setting fields.
• src/nemo_safe_synthesizer/config/pii_replacement.py#L216-L226: Add descriptions for top-level replacement configuration fields.

As per coding guidelines, “Pydantic model fields must include Field(description=...).”

Source: Coding guidelines

src/nemo_safe_synthesizer/pii_replacer/core.py (1)

2205-2213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep one Luhn implementation.

_luhn_valid duplicates _luhn_ok (lines 284-295). Both are used in this module: _luhn_ok classifies card values during detection, and _luhn_valid verifies generated card values. Two checksum implementations can diverge, which would let detection and generation disagree about the same number.

Delete one and call the survivor from both call sites.

src/nemo_safe_synthesizer/pii_replacer/discovery.py (1)

240-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a docstring to discover_plan.

discover_plan is the entry point that plan.py calls to build a plan. It has no docstring, so the generated API reference documents nothing about the arguments, the returned scope, or the effect of config.llm_enhancement.

Add a Google-style docstring that states what group_key selects, how the scope is derived, and that the returned plan is not yet validated.

As per coding guidelines: "Use Google-style docstrings for public, nontrivial, or non-obvious functions and classes".

Source: Coding guidelines

src/nemo_safe_synthesizer/pii_replacer/persona.py (1)

122-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document that assign mutates the instances in place.

assign is public and writes synthetic_person and synthetic_person_source into every dict of the caller's list. It returns None. Add a Google-style docstring that names the keys it sets and states the in-place side effect, because the API reference is generated from these docstrings.

As per coding guidelines: "document arguments, returns, raises, side effects, thread safety, and idempotency as applicable".

Source: Coding guidelines

src/nemo_safe_synthesizer/pii_replacer/replacement.py (3)

102-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Bind loop variables in _append_instance so ruff B023 does not fail mise run check.

Ruff flags field_cols, col_set, match_persona_by, and patterns_by_label as unbound loop variables. The closure is only called inside the same iteration, so the behavior is correct today. The lint failure is still real, and the pattern breaks if the call ever moves out of the loop. Bind the values as default arguments, or move the helper out of the loop and pass the per-persona state explicitly.

♻️ Proposed binding

         def _append_instance(
             match: tuple,
             row: pd.Series,
             originals: dict,
             row_indices: list,
+            *,
+            col_set: PersonaColumnSet = col_set,
+            field_cols: dict[str, str] = field_cols,
+            match_persona_by: list[dict[str, str]] = match_persona_by,
+            patterns_by_label: dict[str, list[str]] = patterns_by_label,
         ) -> None:

Source: Linters/SAST tools

---

476-481: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move import re to module scope.

_present imports re on every call, and it runs once per row, per free-text column, per pair. The import is cheap after the first call, but the local import contradicts the repository import convention (standard library first, at module level). Add import re at the top of the module and remove the local import.

The ast-grep ReDoS hint on this line is a false positive: re.escape(needle) removes all pattern metacharacters, and the lookarounds are literals.

Source: Linters/SAST tools

---

346-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing Google-style docstrings on the new public replacement APIs. These three functions form the public replacement path, and none documents its contract. The shared root cause is that the new entry points were added without the docstrings the repository requires for public and nontrivial APIs; the API reference is generated from them.

• src/nemo_safe_synthesizer/pii_replacer/replacement.py#L346-L356: document the keys of the returned dict[str, Any] (replaced_df, structured_cols, free_text_applied, standalone_cols, changed_summary, free_text_entities) and the group_key requirement for group scope.
• src/nemo_safe_synthesizer/pii_replacer/replacement.py#L542-L549: document the returned tuple, the extra keys it adds (instances, standalone_maps, persona_backend_effective), and the persona_engine override.
• src/nemo_safe_synthesizer/pii_replacer/replacer.py#L70-L77: document the argument, the mutated attributes (result, resolved_plan, elapsed_time), the plan-file side effect, and the raised ParameterError.

As per path instructions: "Public APIs and nontrivial functions need Google-style docstrings."

Source: Path instructions

src/nemo_safe_synthesizer/pii_replacer/replacer.py (1)

66-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Annotate the public attributes.

result, elapsed_time, and resolved_plan form the public surface of TabularPiiReplacer. library_builder.process_data reads replacer.result and replacer.elapsed_time, and it needs assert replacer.result is not None to narrow the type. Explicit annotations let the type checker see the optional types instead of inferring None.

♻️ Proposed annotations

-        self.result = None
-        self.elapsed_time = 0.0
-        self.resolved_plan = None
+        self.result: TransformResult | None = None
+        self.elapsed_time: float = 0.0
+        self.resolved_plan: PiiReplacementPlan | None = None

src/nemo_safe_synthesizer/preflight/checks/environment.py (1)

490-494: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Update the class docstring to match the new gate.

The gate now reads replace_pii.llm_enhancement. The docstring above still says "When classification is enabled" and refers to "PII column classification", which no longer exists in the replacement path. Reword the docstring to reference replace_pii.llm_enhancement and LLM-assisted replacement.

Note the resulting interaction: PiiReplacementConfigCheck reports pii_llm_not_implemented as an error whenever llm_enhancement=True, so every finding from this check now accompanies a failing preflight run. That is acceptable for this release, but the docstring should say so.

docs/product-overview/pii_replacement.md (1)

75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Apply the Markdown emphasis and code-span conventions.

The changed text uses decorative bold for not and entity type. Line 205 uses double backticks for examples. Use plain text or single-backtick code spans.

As per coding guidelines, Markdown body text must not use decorative bold, and inline code uses single backticks.

Also applies to: 139-139, 205-205

Source: Coding guidelines

docs/user-guide/running.md (1)

655-657: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fix markdownlint MD046 violations for nested code blocks.

Static analysis flags fenced code blocks at Line 655-657, Line 667-673, and Line 682-692 as violations of MD046 (code-block-style). Each fenced block sits inside a numbered list item (1. Obtain an NGC API key..., 2. Download locales..., 3. Install parquet files...). The linter expects an indented code block in this context. Convert each fenced block to an indented block, or confirm the project's markdownlint config permits fenced blocks inside list items before merging.

Also applies to: 667-673, 682-692

Source: Linters/SAST tools

tests/pii_replacer/test_tabular_pii.py (2)

103-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Isolate PERSON_RANDOM_SEED for the whole module.

config_from_replace_pii falls back to the PERSON_RANDOM_SEED environment variable when replacement.seed is None (src/nemo_safe_synthesizer/pii_replacer/core.py:138-152). Most tests in this file build ReplacePiiConfig() without a seed, so their engine seed comes from the ambient environment. Only test_config_from_replace_pii_maps_user_fields clears that variable, and this test sets it to 99 for the process until monkeypatch teardown. If a developer shell or CI job exports PERSON_RANDOM_SEED, statistical assertions such as changed.mean() > 0.9 at Line 1854 run against a different draw.

Add an autouse fixture that removes the variable, and keep the explicit setenv inside the one test that verifies the fallback.

♻️ Proposed module-level isolation fixture

+@pytest.fixture(autouse=True)
+def _isolate_person_random_seed(monkeypatch):
+    """Keep the engine seed independent of the ambient environment."""
+    monkeypatch.delenv("PERSON_RANDOM_SEED", raising=False)
+
+
 `@pytest.fixture`
 def patient_df() -> pd.DataFrame:

As per path instructions: "Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements."

Source: Path instructions

---

54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use the fixture_ prefix for the dataset fixtures.

tests/TESTING.md states that dataset fixtures use the fixture_ prefix. The dataset fixtures in this file (patient_df, dob_df, phone_df, contact_df, middle_name_df, numbered_email_df) do not follow it. Rename them so fixture discovery stays consistent with the rest of the suite.

As per coding guidelines: "Use the fixture_ prefix for dataset and tokenizer fixtures".

Also applies to: 808-809, 1145-1146, 1539-1540, 1671-1672, 1819-1820

Source: Coding guidelines

tests/sdk/test_builder.py (1)

72-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert that the inline replacement_plan survives builder resolution.

The test now passes an inline replacement_plan through with_replace_pii, but the only assertion is replacement.locale == "en_US". The inline-plan branch of ReplacePiiConfig._resolve_replacement_plan and from_config_source is executed and then discarded. If plan resolution dropped or flattened persona_backed_columns, this test would still pass. Add an assertion on the resolved plan.

♻️ Proposed assertion on the resolved plan

     assert builder._nss_config is not None
     assert builder._nss_config.replace_pii is not None
     assert builder._nss_config.replace_pii.replacement.locale == "en_US"
+    plan = builder._nss_config.replace_pii.inline_plan
+    assert plan is not None
+    assert [spec.column_name for spec in plan.persona_backed_columns[0].columns_to_replace] == ["name"]

tests/smoke/test_pii_replacement_cpu.py (1)

42-55: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Sensitive Data Exposure (CWE-359)

Reachability: Internal

Assert the free-text propagation that the plan declares.

The plan marks notes as free_text, but the test does not verify that each note contains its row’s synthetic first name and no longer contains "Alice".

Proposed assertion on the propagated note text

     assert replacer.result is not None
     out = replacer.result.transformed_df
     assert "Alice" not in out["first_name"].tolist()
     assert out.loc[out["patient_id"] == "p1", "first_name"].nunique() == 1
+    for first_name, note in zip(out["first_name"], out["notes"], strict=True):
+        assert first_name in note, (first_name, note)
+        assert "Alice" not in note

Source: Path instructions

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e558206c-30fa-498d-8067-eedbc79097d0

📥 Commits

Reviewing files that changed from the base of the PR and between e449de4 and bb2cfbb.

📒 Files selected for processing (136)

• .github/workflows/config/.secrets.baseline
• AGENTS.md
• design.md
• docs/dev-notes/posts/introducing-nemo-safe-synthesizer.md
• docs/developer-guide/architecture.md
• docs/developer-guide/configuration_management.md
• docs/product-overview/pii_replacement.md
• docs/product-overview/pipeline.md
• docs/tutorials/time-series-financial-transactions.ipynb
• docs/user-guide/configuration.md
• docs/user-guide/docker.md
• docs/user-guide/environment.md
• docs/user-guide/evaluating-data.md
• docs/user-guide/getting-started.md
• docs/user-guide/running.md
• docs/user-guide/troubleshooting.md
• src/nemo_safe_synthesizer/artifacts/base/fields.py
• src/nemo_safe_synthesizer/cli/run.py
• src/nemo_safe_synthesizer/cli/settings.py
• src/nemo_safe_synthesizer/cli/utils.py
• src/nemo_safe_synthesizer/config/__init__.py
• src/nemo_safe_synthesizer/config/parameters.py
• src/nemo_safe_synthesizer/config/patch.py
• src/nemo_safe_synthesizer/config/pii_replacement.py
• src/nemo_safe_synthesizer/config/replace_pii.py
• src/nemo_safe_synthesizer/config/unknown_fields.py
• src/nemo_safe_synthesizer/configurator/pydantic_click_options.py
• src/nemo_safe_synthesizer/data_processing/actions/data_actions.py
• src/nemo_safe_synthesizer/data_processing/actions/utils.py
• src/nemo_safe_synthesizer/data_processing/records/fragment.py
• src/nemo_safe_synthesizer/defaults.py
• src/nemo_safe_synthesizer/evaluation/assets/jinja/components/training_columns.j2
• src/nemo_safe_synthesizer/evaluation/assets/text/multi_modal_tooltips.py
• src/nemo_safe_synthesizer/pii_replacer/__init__.py
• src/nemo_safe_synthesizer/pii_replacer/core.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/__init__.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/edit.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/environment.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/filters.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/transform_test_utils.py
• src/nemo_safe_synthesizer/pii_replacer/discovery.py
• src/nemo_safe_synthesizer/pii_replacer/nemo_pii.py
• src/nemo_safe_synthesizer/pii_replacer/ner/__init__.py
• src/nemo_safe_synthesizer/pii_replacer/ner/const.py
• src/nemo_safe_synthesizer/pii_replacer/ner/custom.py
• src/nemo_safe_synthesizer/pii_replacer/ner/datetime.py
• src/nemo_safe_synthesizer/pii_replacer/ner/entity.py
• src/nemo_safe_synthesizer/pii_replacer/ner/factory.py
• src/nemo_safe_synthesizer/pii_replacer/ner/fasttext.py
• src/nemo_safe_synthesizer/pii_replacer/ner/helpers.py
• src/nemo_safe_synthesizer/pii_replacer/ner/labels.py
• src/nemo_safe_synthesizer/pii_replacer/ner/metadata.py
• src/nemo_safe_synthesizer/pii_replacer/ner/model.py
• src/nemo_safe_synthesizer/pii_replacer/ner/models.py
• src/nemo_safe_synthesizer/pii_replacer/ner/ner.py
• src/nemo_safe_synthesizer/pii_replacer/ner/ner_mp.py
• src/nemo_safe_synthesizer/pii_replacer/ner/nlp.py
• src/nemo_safe_synthesizer/pii_replacer/ner/person_name.py
• src/nemo_safe_synthesizer/pii_replacer/ner/pipeline.py
• src/nemo_safe_synthesizer/pii_replacer/ner/predictor.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regex.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/__init__.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/aba_routing_number.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/age.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/credit_card.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/domain_name.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/email.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/facebook.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/generic_key.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/github.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/google.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/google_olc.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/iban.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/imei.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/ip_address.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/jwt.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/lat_lon.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/md5.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/race_ethnicity.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/sendgrid.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/sex_gender.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/sha256.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/sha512.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/slack.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/square.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/stripe.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/swift.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/twilio.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/url.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/us_phone.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/us_ssn.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/us_zipcode.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/uuid.py
• src/nemo_safe_synthesizer/pii_replacer/ner/report/__init__.py
• src/nemo_safe_synthesizer/pii_replacer/ner/report/base.py
• src/nemo_safe_synthesizer/pii_replacer/ner/report/metadata.py
• src/nemo_safe_synthesizer/pii_replacer/ner/report/results.py
• src/nemo_safe_synthesizer/pii_replacer/ner/utils.py
• src/nemo_safe_synthesizer/pii_replacer/persona.py
• src/nemo_safe_synthesizer/pii_replacer/plan.py
• src/nemo_safe_synthesizer/pii_replacer/replacement.py
• src/nemo_safe_synthesizer/pii_replacer/replacer.py
• src/nemo_safe_synthesizer/pii_replacer/transform_result.py
• src/nemo_safe_synthesizer/preflight/checks/__init__.py
• src/nemo_safe_synthesizer/preflight/checks/environment.py
• src/nemo_safe_synthesizer/preflight/checks/pii.py
• src/nemo_safe_synthesizer/sdk/config_builder.py
• src/nemo_safe_synthesizer/sdk/library_builder.py
• src/nemo_safe_synthesizer/utils.py
• tests/TESTING.md
• tests/cli/test_run.py
• tests/cli/test_settings.py
• tests/cli/test_utils.py
• tests/config/test_nss_config.py
• tests/config/test_parameters.py
• tests/config/test_patch.py
• tests/configurator/test_pydantic_click_options.py
• tests/conftest.py
• tests/evaluation/components/test_pii_replay.py
• tests/evaluation/conftest.py
• tests/evaluation/test_render.py
• tests/evaluation/test_render_assets.py
• tests/nss_pii_replacer_test.py
• tests/pii_replacer/test_detect.py
• tests/pii_replacer/test_edit.py
• tests/pii_replacer/test_filters.py
• tests/pii_replacer/test_nemo_pii.py
• tests/pii_replacer/test_tabular_pii.py
• tests/preflight/conftest.py
• tests/preflight/test_preflight.py
• tests/sdk/test_builder.py
• tests/sdk/test_config_builder.py
• tests/sdk/test_process_data.py
• tests/sdk/test_process_data_pii_regression.py
• tests/smoke/test_pii_replacement_cpu.py

💤 Files with no reviewable changes (72)

• src/nemo_safe_synthesizer/pii_replacer/data_editor/init.py
• src/nemo_safe_synthesizer/artifacts/base/fields.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/google.py
• src/nemo_safe_synthesizer/pii_replacer/ner/helpers.py
• src/nemo_safe_synthesizer/config/replace_pii.py
• src/nemo_safe_synthesizer/pii_replacer/ner/const.py
• src/nemo_safe_synthesizer/pii_replacer/ner/ner_mp.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/credit_card.py
• src/nemo_safe_synthesizer/pii_replacer/ner/labels.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/aba_routing_number.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/init.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/filters.py
• src/nemo_safe_synthesizer/pii_replacer/ner/report/base.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py
• src/nemo_safe_synthesizer/pii_replacer/ner/nlp.py
• src/nemo_safe_synthesizer/data_processing/records/fragment.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/edit.py
• src/nemo_safe_synthesizer/pii_replacer/nemo_pii.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/us_zipcode.py
• src/nemo_safe_synthesizer/pii_replacer/ner/custom.py
• src/nemo_safe_synthesizer/pii_replacer/ner/metadata.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/md5.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/sha512.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regex.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/lat_lon.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/twilio.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/slack.py
• src/nemo_safe_synthesizer/data_processing/actions/data_actions.py
• src/nemo_safe_synthesizer/pii_replacer/ner/factory.py
• src/nemo_safe_synthesizer/pii_replacer/ner/datetime.py
• tests/pii_replacer/test_edit.py
• src/nemo_safe_synthesizer/pii_replacer/ner/person_name.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/jwt.py
• src/nemo_safe_synthesizer/pii_replacer/ner/ner.py
• src/nemo_safe_synthesizer/pii_replacer/ner/report/metadata.py
• tests/nss_pii_replacer_test.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/email.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/age.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/generic_key.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/race_ethnicity.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/transform_test_utils.py
• src/nemo_safe_synthesizer/pii_replacer/ner/entity.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/github.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/square.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/sha256.py
• tests/pii_replacer/test_filters.py
• src/nemo_safe_synthesizer/pii_replacer/ner/report/init.py
• src/nemo_safe_synthesizer/pii_replacer/ner/init.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/sendgrid.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/us_ssn.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/sex_gender.py
• src/nemo_safe_synthesizer/pii_replacer/ner/utils.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/uuid.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/imei.py
• src/nemo_safe_synthesizer/pii_replacer/ner/report/results.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/ip_address.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/google_olc.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/domain_name.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/iban.py
• tests/pii_replacer/test_detect.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/stripe.py
• src/nemo_safe_synthesizer/pii_replacer/ner/predictor.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/url.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/us_phone.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/facebook.py
• src/nemo_safe_synthesizer/pii_replacer/ner/regexes/swift.py
• tests/pii_replacer/test_nemo_pii.py
• src/nemo_safe_synthesizer/pii_replacer/ner/pipeline.py
• src/nemo_safe_synthesizer/pii_replacer/ner/models.py
• src/nemo_safe_synthesizer/pii_replacer/ner/model.py
• src/nemo_safe_synthesizer/pii_replacer/data_editor/environment.py
• src/nemo_safe_synthesizer/pii_replacer/ner/fasttext.py

[binaryaaron]

thanks @nina-xu - as a note for the rest of the team, i'd suggest we continue to let this PR bake for a while so we can properly dogfood and review it (+/- 10K loc and a whole new design), so no one needs to "approve" it right now.

we can group reviews comments into a few buckets as well - pii config design/logic/semantics, structural implementation/code quality/maintainability, usability, and testing?

[kendrickb-nvidia]

@binaryaaron @zywind @nina-xu If we want this to be a longer-lived branch, then we should merge this into feature/pii-replacement branch (or some other branch not main), right? And the bulk of the reviewing should happen on these PRs into feature/pii-replacement branch.

[greptile-apps]

Too many files changed for review (189 files, 100 file limit).

Bypass the limit by tagging @greptile-apps to review.

[coderabbitai]

Actionable comments posted: 17

🧹 Nitpick comments (17)

src/nemo_safe_synthesizer/pii_replacer/replacement/free_text.py (1)

228-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Annotate the returned substituter. build_text_substituter has no return annotation, and the docstring states it returns a callable or None. Add -> Callable[[object], object] | None so callers get checked types.

As per path instructions: "use typed contracts and Protocols, native Python 3.11-compatible annotations" (STYLE_GUIDE.md).

Source: Path instructions

src/nemo_safe_synthesizer/pii_replacer/replacement/instances.py (3)

154-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Bind the loop variables in the closure. Ruff reports B023 for field_cols, col_set, match_persona_by, and patterns_by_label at lines 160-170. The current calls happen inside the same iteration, so behavior is correct today, but the closure is one refactor away from capturing the wrong col_set. Pass the values as default arguments, or move _append_instance to a module-level helper that takes them explicitly.

Source: Linters/SAST tools

---

196-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Deduplicate the record and default scope branches. The case "record" and case _ arms differ only in whether repeated signatures are collapsed. The case _ body also re-annotates sig_rows and sig_first, which the case "group" arm already annotated in the same function scope. Extract one helper that takes a "collapse duplicates" flag, or drop the duplicate annotations.

---

26-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Untyped parameters in new library helpers. Several new helpers omit annotations on parameters and return values, so ty cannot check the call sites and callers cannot see the expected interface. The docstrings already name the concrete shapes, so the annotations are mechanical.

• src/nemo_safe_synthesizer/pii_replacer/replacement/instances.py#L26-L34: annotate _instance_is_person dictionaries and every _make_instance parameter (tuple, pd.Series, entities.Config, dict[str, list[str]] | None).
• src/nemo_safe_synthesizer/pii_replacer/patterns/value_templates.py#L55-L55: annotate shape_fn as Callable[[str], str], and give rng and values types in pattern_preserving_token, infer_value_pattern, generate_from_pattern, and conform_to_template.
• src/nemo_safe_synthesizer/pii_replacer/replacement/free_text.py#L228-L228: add the Callable[[object], object] | None return annotation to build_text_substituter.

As per path instructions: "use typed contracts and Protocols, native Python 3.11-compatible annotations" (STYLE_GUIDE.md).

Source: Path instructions

tests/pii_replacer/replacement/test_personas.py (1)

199-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Set an explicit replacement seed for this statistical assertion. The test omits replacement.seed; the resolver uses PERSON_RANDOM_SEED or 42. This leaves the assertion dependent on ambient CI environment state.

tests/pii_replacer/detection/test_column_names.py (2)

28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated discovery setup into a fixture.

Ten call sites repeat discover_plan(df, None, config_from_replace_pii(PiiReplacerConfig()), PiiReplacerConfig()). A small module fixture or helper (for example fixture_discover returning a callable that takes df and an optional group_key) keeps the behavior under test visible and removes the duplicated wiring. tests/TESTING.md asks for focused fixture_-prefixed fixtures for shared setup.

Also applies to: 47-47, 59-59, 80-80, 96-96, 123-123, 164-164

Source: Path instructions

---

212-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log assertions treat message prose as a contract. Three new assertions require full warning or info phrasing, so a harmless rewording of a log message fails the suite without any behavior regression. Assert only the contract-relevant tokens: column name, candidate labels, and chosen label.

• tests/pii_replacer/detection/test_column_names.py#L212-L221: drop the "Review the replacement plan" and "bug report" conditions; keep the column name and the label names.
• tests/pii_replacer/detection/test_column_names.py#L239-L242: replace the "chose 'sex'" condition with an assertion on the returned demo_label plus the candidate label names in the message.
• tests/pii_replacer/detection/test_value_recognizers.py#L149-L149: assert "misc_col" and a short stable token instead of the full "Identified temporal column 'misc_col'" phrase.

Source: Path instructions

tests/pii_replacer/detection/test_value_recognizers.py (1)

115-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the unused caplog fixture or assert on the captured records.

Both tests request caplog and call caplog.set_level(logging.WARNING), but neither asserts anything about the records. The scaffolding suggests an intended warning assertion that is missing. Either assert the expected warning (which would add real coverage for the skip decision) or drop the fixture and the logging import.

Also applies to: 152-163

src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py (1)

197-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move or drop the stale keyword comment.

This comment describes the curated FUZZY_KEYWORDS spellings, but those now live in entities.py as EntitySpec.fuzzy_keywords. In this file it reads as documentation for fuzzy_match_label, which does something else.

♻️ Proposed cleanup

-# Curated, specific keyword spellings per label for the fuzzy backstop. These are
-# deliberately NOT generic single words (no bare "name"/"date"/"id"), so a typo'd
-# variant matches but unrelated columns (e.g. "event_name", "event_date") do not.
 def fuzzy_match_label(col: str, patterns: Mapping[str, list[str]], threshold: float) -> str | None:

src/nemo_safe_synthesizer/pii_replacer/detection/free_text.py (1)

69-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log the skip reason that _free_text_eligibility already returns.

_free_text_eligibility computes a reason per column, but line 69 discards it. The summary log then lists skipped columns without saying whether the dtype or the field classification excluded them. Users who ask why a text column was not scanned get no answer from the logs.

♻️ Proposed change to keep the reason

-        eligible, _reason = _free_text_eligibility(col, df[col])
+        eligible, reason = _free_text_eligibility(col, df[col])
         if eligible:
             text_fields.append(col)
         else:
-            not_scanned.append(col)
+            not_scanned.append(f"{col} ({reason})")

src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py (1)

43-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Export IpAddressHandler from __all__.

IpAddressHandler is a public handler with the same role as CreditCardHandler and DateOfBirthHandler, and _HANDLERS registers it for ipv4 and ipv6. It is missing from __all__, so the public surface is inconsistent.

♻️ Proposed fix

 __all__ = [
     "CreditCardHandler",
     "DateOfBirthHandler",
     "DefaultHandler",
     "EntityHandler",
+    "IpAddressHandler",
     "get_handler",
 ]

As per path instructions for src/**/*.py, which require __all__ for public APIs.

Source: Path instructions

src/nemo_safe_synthesizer/pii_replacer/patterns/persona_templates.py (1)

204-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the missing return annotation on split_title.

split_title is public and returns tuple[str | None, str]. The function has no return annotation, so callers such as persona_written in replacement/personas.py lose type information. The repository requires type-correct code checked by ty.

♻️ Proposed annotation

-def split_title(value: str):
+def split_title(value: str) -> tuple[str | None, str]:

Source: Coding guidelines

src/nemo_safe_synthesizer/pii_replacer/replacement/apply.py (1)

267-273: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Compute cells_changed only for columns the plan touched.

_cells_changed runs an element-wise Python map over the original and replaced series for every column in the frame, including columns no replacement can reach. On wide frames this dominates the replacement cost and produces no extra information, because only structured_cols, standalone_cols, and free_text_applied can change.

♻️ Proposed narrowing

-    changed_summary = [{"column": c, "cells_changed": _cells_changed(c)} for c in column_order]
+    touched = structured_cols | set(standalone_cols) | set(free_text_applied)
+    changed_summary = [{"column": c, "cells_changed": _cells_changed(c)} for c in column_order if c in touched]
     changed_summary = [d for d in changed_summary if d["cells_changed"]]

src/nemo_safe_synthesizer/pii_replacer/replacement/demographics.py (2)

59-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Widen the missing-value guard to pandas NA scalars.

The guard only recognizes None and float NaN. instances.py calls norm_sex(row[cond["sex"]]) with a raw cell value, so a nullable-dtype column yields pd.NA and a datetime column yields pd.NaT. Neither is a float, so _norm_cat converts them to "na" and "nat" and the fuzzy scan runs on a sentinel string.

🛡️ Proposed guard

-    if value is None or (isinstance(value, float) and pd.isna(value)):
+    if value is None or (not isinstance(value, str) and pd.isna(value) is True):
         return None

---

288-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Export persona_match_map and give its parameter a real type.

replacement/instances.py imports persona_match_map, but __all__ (lines 17-23) omits it, so the module's declared public surface does not match its actual use. The parameter is also annotated as a bare list, and the body indexes entries as untyped mappings. The style guide asks for typed contracts instead of nested dictionary exchanges.

Add "persona_match_map" to __all__ and annotate the parameter with the plan's match_persona_by entry model.

Source: Path instructions

src/nemo_safe_synthesizer/pii_replacer/replacement/personas.py (1)

80-102: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift

Do not install a global sudachipy stub, and replace the setattr calls.

Two problems in this block:

1. Lines 83-99 insert a fake sudachipy module into sys.modules for the whole process and never remove it. Any other code in the same process that imports sudachipy afterwards silently receives a stub whose Dictionary.create() returns None. sys.path.insert at line 82 is also permanent. The PGM backend is internal-only, but the side effect is process-wide and outlives the call.

2. Ruff flags lines 96-97 (B010): setattr with a constant attribute name. Use direct assignment.

If the stub must stay, restore sys.modules and sys.path in a try/finally block, which the coding guidelines require for resource cleanup.

♻️ Minimal change for the `setattr` calls

-            _dict_mod = _types.ModuleType("sudachipy.dictionary")
-            setattr(_dict_mod, "Dictionary", _StubDict)
-            setattr(_stub, "dictionary", _dict_mod)
+            _dict_mod = _types.ModuleType("sudachipy.dictionary")
+            _dict_mod.Dictionary = _StubDict
+            _stub.dictionary = _dict_mod

Sources: Coding guidelines, Linters/SAST tools

tests/sdk/test_builder.py (1)

87-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Builder tests assert existence instead of the configured replacement values. Both tests supply a specific replace_pii payload and then check only that replace_pii is not None or that one field survived. A regression that dropped the plan, the seed, or the llm_enhancement flag during resolution would still pass.

• tests/sdk/test_builder.py#L87-L89: assert the resolved persona-backed plan contains the name column with entity first_name, and assert replacement.seed == 42.
• tests/sdk/test_builder.py#L255-L268: assert llm_enhancement is False and that the replacement plan resolved to auto discovery from the YAML string.

Source: Path instructions

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e6c7e3a4-e3f9-4c65-87e7-5b1f46db2eea

📥 Commits

Reviewing files that changed from the base of the PR and between 9c263ae and 97cc468.

📒 Files selected for processing (67)

• docs/developer-guide/architecture.md
• docs/product-overview/pii_replacement.md
• docs/user-guide/configuration.md
• docs/user-guide/running.md
• docs/user-guide/troubleshooting.md
• src/nemo_safe_synthesizer/config/__init__.py
• src/nemo_safe_synthesizer/config/parameters.py
• src/nemo_safe_synthesizer/config/patch.py
• src/nemo_safe_synthesizer/config/replace_pii.py
• src/nemo_safe_synthesizer/pii_replacer/detection/__init__.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py
• src/nemo_safe_synthesizer/pii_replacer/detection/free_text.py
• src/nemo_safe_synthesizer/pii_replacer/detection/persona_grouping.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/llm/noop.py
• src/nemo_safe_synthesizer/pii_replacer/llm/not_implemented.py
• src/nemo_safe_synthesizer/pii_replacer/llm/protocol.py
• src/nemo_safe_synthesizer/pii_replacer/models.py
• src/nemo_safe_synthesizer/pii_replacer/patterns/__init__.py
• src/nemo_safe_synthesizer/pii_replacer/patterns/evidence.py
• src/nemo_safe_synthesizer/pii_replacer/patterns/persona_templates.py
• src/nemo_safe_synthesizer/pii_replacer/patterns/temporal.py
• src/nemo_safe_synthesizer/pii_replacer/patterns/value_templates.py
• src/nemo_safe_synthesizer/pii_replacer/planning/discovery.py
• src/nemo_safe_synthesizer/pii_replacer/planning/io.py
• src/nemo_safe_synthesizer/pii_replacer/planning/validation.py
• src/nemo_safe_synthesizer/pii_replacer/replacement/apply.py
• src/nemo_safe_synthesizer/pii_replacer/replacement/demographics.py
• src/nemo_safe_synthesizer/pii_replacer/replacement/free_text.py
• src/nemo_safe_synthesizer/pii_replacer/replacement/instances.py
• src/nemo_safe_synthesizer/pii_replacer/replacement/personas.py
• src/nemo_safe_synthesizer/pii_replacer/replacement/scope.py
• src/nemo_safe_synthesizer/pii_replacer/replacement/standalone.py
• src/nemo_safe_synthesizer/pii_replacer/replacer.py
• src/nemo_safe_synthesizer/preflight/checks/pii.py
• src/nemo_safe_synthesizer/sdk/config_builder.py
• tests/config/test_nss_config.py
• tests/config/test_parameters.py
• tests/config/test_patch.py
• tests/conftest.py
• tests/pii_replacer/detection/test_column_names.py
• tests/pii_replacer/detection/test_free_text_detect.py
• tests/pii_replacer/detection/test_persona_grouping.py
• tests/pii_replacer/detection/test_value_recognizers.py
• tests/pii_replacer/golden/patient_events.plan.yaml
• tests/pii_replacer/helpers.py
• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/patterns/test_value_templates.py
• tests/pii_replacer/planning/test_discovery.py
• tests/pii_replacer/planning/test_validation.py
• tests/pii_replacer/replacement/test_free_text.py
• tests/pii_replacer/replacement/test_personas.py
• tests/pii_replacer/replacement/test_phone.py
• tests/pii_replacer/replacement/test_scope.py
• tests/pii_replacer/replacement/test_scope_consistency.py
• tests/pii_replacer/replacement/test_standalone.py
• tests/pii_replacer/test_config.py
• tests/pii_replacer/test_discovery_golden.py
• tests/pii_replacer/test_preflight.py
• tests/preflight/conftest.py
• tests/sdk/test_builder.py
• tests/sdk/test_config_builder.py
• tests/sdk/test_process_data.py
• tests/sdk/test_process_data_pii_regression.py
• tests/smoke/test_pii_replacement_cpu.py

🚧 Files skipped from review as they are similar to previous changes (30)

• tests/smoke/test_pii_replacement_cpu.py
• src/nemo_safe_synthesizer/pii_replacer/detection/init.py
• src/nemo_safe_synthesizer/pii_replacer/llm/noop.py
• tests/pii_replacer/test_discovery_golden.py
• tests/pii_replacer/detection/test_free_text_detect.py
• docs/user-guide/configuration.md
• tests/conftest.py
• src/nemo_safe_synthesizer/pii_replacer/llm/protocol.py
• tests/preflight/conftest.py
• docs/user-guide/troubleshooting.md
• src/nemo_safe_synthesizer/config/patch.py
• tests/pii_replacer/helpers.py
• tests/pii_replacer/replacement/test_free_text.py
• tests/sdk/test_config_builder.py
• src/nemo_safe_synthesizer/preflight/checks/pii.py
• tests/config/test_patch.py
• tests/pii_replacer/detection/test_persona_grouping.py
• tests/pii_replacer/test_preflight.py
• src/nemo_safe_synthesizer/pii_replacer/llm/not_implemented.py
• tests/pii_replacer/planning/test_discovery.py
• src/nemo_safe_synthesizer/pii_replacer/models.py
• src/nemo_safe_synthesizer/pii_replacer/planning/io.py
• docs/product-overview/pii_replacement.md
• src/nemo_safe_synthesizer/pii_replacer/detection/persona_grouping.py
• src/nemo_safe_synthesizer/pii_replacer/replacement/standalone.py
• tests/pii_replacer/planning/test_validation.py
• src/nemo_safe_synthesizer/pii_replacer/replacer.py
• tests/pii_replacer/golden/patient_events.plan.yaml
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• tests/pii_replacer/test_config.py

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect markdownlint config and the flagged block to confirm the MD046 finding.
set -euo pipefail

fd -H '.markdownlint*' -x cat
sed -n '555,625p' docs/user-guide/running.md | cat -A | sed -n '1,80p' | head -80

Repository: NVIDIA-NeMo/Safe-Synthesizer

Length of output: 2491

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- markdownlint references ---'
rg -n -i 'markdownlint|mise run .*lint|lint.*docs' . --glob '!uv.lock' --glob '!*.lock' | head -120

printf '%s\n' '--- available local tools ---'
command -v markdownlint || true
command -v markdownlint-cli2 || true
command -v mise || true

printf '%s\n' '--- exact block and file status ---'
sed -n '560,625p' docs/user-guide/running.md | cat -n
git status --short -- docs/user-guide/running.md .markdownlint.json .markdownlint.yaml .markdownlint.yml

Repository: NVIDIA-NeMo/Safe-Synthesizer

Length of output: 3568

---

🏁 Script executed:

#!/bin/bash
set -u

printf '%s\n' '--- markdownlint result ---'
markdownlint-cli2 docs/user-guide/running.md
status=$?
printf 'markdownlint-cli2 exit status: %s\n' "$status"
exit 0

Repository: NVIDIA-NeMo/Safe-Synthesizer

Length of output: 13916

---

Handle MD046 for MkDocs tab blocks. This block already matches the surrounding tab syntax, but markdownlint reports MD046 for all similarly indented tab blocks. Update the markdownlint configuration to account for MkDocs tabs instead of changing this block alone.

🧰 Tools

🪛 markdownlint-cli2 (0.23.2)

[warning] 611-611: Code block style
Expected: fenced; Actual: indented

(MD046, code-block-style)

Source: Linters/SAST tools

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

ngc registry resource download-version command syntax version suffix

💡 Result:

The command syntax for downloading an NGC resource using the NGC CLI is: ngc registry resource download-version /[/]: [options] [1][2][3] In this syntax, the version suffix is specified by appending a colon (:) followed by the specific version number to the resource name [1][2][3]. For example: ngc registry resource download-version my-org/my-team/my-resource:1.0 [2][4] Key details: - Organization (org) and team (team) are part of the target identifier, where the team is optional [1][3]. - The version is a mandatory suffix after the colon [1][2][3]. - You can specify a destination directory using the --dest option [5][2][6]. For further help or to see specific options available for your version of the NGC CLI, you can run the following command: ngc registry resource download-version -h [5][7]

Citations:

• 1: https://archive.docs.nvidia.com/ace/resource-downloader-container/1.2/ngc-resource-downloader.html
• 2: https://docs.nvidia.com/brev/guides/inference-deployment/ngc-developer-guide
• 3: https://docs.nvidia.com/dgx/ngc-registry-cli-user-guide/index-bak.html
• 4: https://github.com/NVIDIA/nv-cloud-function-helpers/blob/main/skills/nvcf-ngc-cli-skill/references/registry.md
• 5: https://docs.ngc.nvidia.com/cli/cmd_registry.html
• 6: https://docs.nvidia.com/dgx/pdf/ngc-registry-cli-user-guide.pdf
• 7: https://docs.ngc.nvidia.com/cli/cmd.html

---

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- documented commands ---'
rg -n -C 3 'download-version|nemotron-personas-dataset-en_us|NGC package' docs/user-guide/running.md
printf '%s\n' '--- repository examples ---'
rg -n 'ngc registry resource download-version' . --glob '*.md' --glob '*.yaml' --glob '*.yml' --glob '*.sh' || true

Repository: NVIDIA-NeMo/Safe-Synthesizer

Length of output: 1605

---

Add a version suffix to both NGC commands. download-version requires <org>/<team>/<resource>:<version>; the current commands can fail with a usage error.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject a blank replacement_plan string here.

An empty or whitespace-only string passes this validator. plan_path then returns a falsy value, so resolve_plan in src/nemo_safe_synthesizer/pii_replacer/planning/io.py (Lines 185-190) treats the config as having no plan source and raises the generic "replacement_plan must be auto_discovery, a path, or an inline plan" error. The user sees a message that does not name the real problem. Reject blank strings at config validation so the error points at the field.

🛠️ Proposed fix

         if isinstance(value, Path):
             return str(value)
-        if isinstance(value, str | PiiReplacementPlan):
+        if isinstance(value, str):
+            if not value.strip():
+                raise ParameterError(
+                    f"replacement_plan must be {AUTO_DISCOVERY!r}, a path to a plan file, or an inline plan; "
+                    "got an empty string"
+                )
+            return value
+        if isinstance(value, PiiReplacementPlan):
             return value

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	@field_validator("replacement_plan", mode="before")
	@classmethod
	def _resolve_replacement_plan(cls, value: object) -> object:
	"""Resolve the plan/string union here so errors describe the plan, not the union.
	Left to the union, a malformed inline plan reports the plan's own errors
	*and* "input should be a valid string", which reads as though a file path
	was expected. Validating a mapping as a plan up front keeps the report to
	the fields the user actually got wrong.
	"""
	if isinstance(value, Mapping):
	try:
	return PiiReplacementPlan.model_validate(value)
	except ValidationError as exc:
	details = "; ".join(
	f"{'.'.join(str(part) for part in error['loc'])}: {error['msg']}" for error in exc.errors()
	)
	raise ParameterError(f"invalid inline replacement plan ({details})") from exc
	if isinstance(value, Path):
	return str(value)
	if isinstance(value, str | PiiReplacementPlan):
	return value
	raise ParameterError(
	f"replacement_plan must be {AUTO_DISCOVERY!r}, a path to a plan file, or an inline plan; "
	f"got {type(value).__name__}"
	)
	@field_validator("replacement_plan", mode="before")
	@classmethod
	def _resolve_replacement_plan(cls, value: object) -> object:
	"""Resolve the plan/string union here so errors describe the plan, not the union.
	Left to the union, a malformed inline plan reports the plan's own errors
	*and* "input should be a valid string", which reads as though a file path
	was expected. Validating a mapping as a plan up front keeps the report to
	the fields the user actually got wrong.
	"""
	if isinstance(value, Mapping):
	try:
	return PiiReplacementPlan.model_validate(value)
	except ValidationError as exc:
	details = "; ".join(
	f"{'.'.join(str(part) for part in error['loc'])}: {error['msg']}" for error in exc.errors()
	)
	raise ParameterError(f"invalid inline replacement plan ({details})") from exc
	if isinstance(value, Path):
	return str(value)
	if isinstance(value, str):
	if not value.strip():
	raise ParameterError(
	f"replacement_plan must be {AUTO_DISCOVERY!r}, a path to a plan file, or an inline plan; "
	"got an empty string"
	)
	return value
	if isinstance(value, PiiReplacementPlan):
	return value
	raise ParameterError(
	f"replacement_plan must be {AUTO_DISCOVERY!r}, a path to a plan file, or an inline plan; "
	f"got {type(value).__name__}"
	)

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

A non-numeric PERSON_RANDOM_SEED raises a bare ValueError.

int(os.environ.get("PERSON_RANDOM_SEED", "42") or "42") handles an empty value but not a malformed one. With PERSON_RANDOM_SEED=auto, the run fails with an unhandled ValueError instead of a project error that names the variable. The same expression is repeated at lines 114-116, so both call sites share the defect.

Extract one helper that parses the variable and raises ParameterError with the offending value, then call it from both sites.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check whether the replacement entry point normalizes the dataframe index.
set -euo pipefail

rg -n -C4 'reset_index|index\.is_unique|def run_replacement' --glob 'src/nemo_safe_synthesizer/pii_replacer/**/*.py'

Repository: NVIDIA-NeMo/Safe-Synthesizer

Length of output: 166

---

🏁 Script executed:

set -euo pipefail

printf '%s\n' '--- candidate files ---'
git ls-files 'src/nemo_safe_synthesizer/pii_replacer/**'

printf '%s\n' '--- scope module outline ---'
ast-grep outline src/nemo_safe_synthesizer/pii_replacer/replacement/scope.py

printf '%s\n' '--- replacement call sites and index handling ---'
rg -n -C5 'ScopedValueMap|scope|original_df|reset_index|index' src/nemo_safe_synthesizer/pii_replacer

Repository: NVIDIA-NeMo/Safe-Synthesizer

Length of output: 50385

---

🏁 Script executed:

set -euo pipefail

printf '%s\n' '--- replacement entry point ---'
sed -n '60,155p' src/nemo_safe_synthesizer/pii_replacer/replacer.py

printf '%s\n' '--- scope map implementation ---'
sed -n '120,225p' src/nemo_safe_synthesizer/pii_replacer/replacement/scope.py

printf '%s\n' '--- record-scope consumers ---'
rg -n -C6 'build_scoped_col_map|unit_key\(|row_indices|standalone_maps' \
  src/nemo_safe_synthesizer/pii_replacer/replacement \
  src/nemo_safe_synthesizer/pii_replacer/replacer.py

Repository: NVIDIA-NeMo/Safe-Synthesizer

Length of output: 42776

---

🏁 Script executed:

set -euo pipefail

printf '%s\n' '--- all build_scoped_col_map callers ---'
rg -n -C3 'build_scoped_col_map\(' --glob '*.py' .

printf '%s\n' '--- standalone map entry point ---'
sed -n '244,315p' src/nemo_safe_synthesizer/pii_replacer/replacement/standalone.py

printf '%s\n' '--- record-scope tests and index fixtures ---'
rg -n -C4 'record.scope|scope.*record|duplicate|non.unique|index' tests src/nemo_safe_synthesizer/pii_replacer --glob '*.py' | head -n 240

printf '%s\n' '--- pandas duplicate-label behavior probe ---'
python3 - <<'PY'
try:
    import pandas as pd
except ImportError as exc:
    print(f"pandas unavailable: {exc}")
else:
    df = pd.DataFrame({"value": ["a", "b"]}, index=["same", "same"])
    print("index:", list(df.index))
    for idx in df.index:
        selected = df.loc[[idx], "value"]
        print(f"{idx!r}: selected={selected.tolist()!r}, unique_values={selected.dropna().unique().tolist()!r}")
    data = {idx: df.loc[[idx], "value"].dropna().unique().tolist() for idx in df.index}
    print("dict keys:", list(data.keys()))
    print("dict result:", data)
PY

Repository: NVIDIA-NeMo/Safe-Synthesizer

Length of output: 22392

---

🏁 Script executed:

set -euo pipefail

printf '%s\n' '--- public replacement APIs and duplicate-index boundary ---'
sed -n '270,340p' src/nemo_safe_synthesizer/pii_replacer/replacement/apply.py
sed -n '1,55p' src/nemo_safe_synthesizer/pii_replacer/replacement/__init__.py
rg -n -C5 'run_replacement\(|build_standalone_maps\(' \
  src tests --glob '*.py' | head -n 220

printf '%s\n' '--- exact transform_df normalization context ---'
sed -n '75,90p' src/nemo_safe_synthesizer/pii_replacer/replacer.py

Repository: NVIDIA-NeMo/Safe-Synthesizer

Length of output: 20147

---

🏁 Script executed:

set -euo pipefail

sed -n '340,380p' tests/pii_replacer/replacement/test_scope.py

Repository: NVIDIA-NeMo/Safe-Synthesizer

Length of output: 1635

---

Handle duplicate indexes in direct replacement APIs. TabularPiiReplacer.transform_df resets non-unique indexes, but direct calls to run_replacement or build_standalone_maps do not. In those calls, record scope still collapses duplicate labels. Enforce a unique-index precondition or iterate rows positionally in build_scoped_col_map.

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Align the test name and docstring with the plan it builds. The name and docstring describe standalone phones, but the call passes standalone=False, so the plan puts phone in a persona set. With the Faker backend the column still routes through the standalone map, which is exactly what line 75 asserts, so the coverage is real but the intent is unclear. Either pass standalone=True, or state in the docstring that a Faker-backed persona set falls through to the standalone path.

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Replace the hardcoded /tmp values that Ruff reports as errors.

Ruff reports S108 as an error on lines 140, 252, 269, and 329. Unless S108 is suppressed for tests/, lint fails in CI. The value is never read here, because every test uses the faker or managed backend, so a neutral placeholder works.

Set sdg_pgms_src from the tmp_path fixture, or use a clearly non-filesystem sentinel such as "unused-pgm-src", in all four places.

🧰 Tools

🪛 ast-grep (0.45.1)

[info] 139-139: Do not hardcode temporary file or directory names
Context: "/tmp"
Note: [CWE-377] Insecure Temporary File.

(hardcoded-tmp-file)

🪛 Ruff (0.16.1)

[error] 140-140: Probable insecure usage of temporary file or directory: "/tmp"

(S108)

Sources: Path instructions, Linters/SAST tools

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Suppress or allowlist the card-number literals so secret scanning does not fail.

OpenGrep reports three pii.credit-card-number-dashed errors on line 95. The values are the well-known public test numbers, so the finding is a false positive, but this PR also adds .github/workflows/config/.secrets.baseline, and an unsuppressed error can block CI.

Add the values to the baseline, or build them from parts in the test so no literal card-shaped string appears. Keep a short comment that records why the numbers are safe.

🧰 Tools

🪛 OpenGrep (1.26.0)

[ERROR] 95-95: Possible credit card number with dashes or spaces detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.

(coderabbit.pii.credit-card-number-dashed)

---

[ERROR] 95-95: Possible credit card number with dashes or spaces detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.

(coderabbit.pii.credit-card-number-dashed)

---

[ERROR] 95-95: Possible credit card number with dashes or spaces detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.

(coderabbit.pii.credit-card-number-dashed)

Source: Linters/SAST tools

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Assert which validation rule rejected schema_version: 2.

pytest.raises(ValidationError) also passes if the model rejects the payload for an unrelated reason, for example a required field that the partial dict omits. The test then no longer proves the forward-compatibility gate its docstring describes. Add match="schema_version" or assert exc_info.value.errors()[0]["loc"].

💚 Proposed fix

-    with pytest.raises(ValidationError):
+    with pytest.raises(ValidationError, match="schema_version"):
         PiiReplacerConfig.model_validate({"schema_version": 2})

As per path instructions: "Use pytest.raises(..., match=...) for validation errors".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	def test_pii_schema_version_refuses_an_unknown_value():
	"""schema_version is the forward-compat gate: only the current release's value is accepted."""
	with pytest.raises(ValidationError):
	PiiReplacerConfig.model_validate({"schema_version": 2})
	def test_pii_schema_version_refuses_an_unknown_value():
	"""schema_version is the forward-compat gate: only the current release's value is accepted."""
	with pytest.raises(ValidationError, match="schema_version"):
	PiiReplacerConfig.model_validate({"schema_version": 2})

Source: Path instructions

[zywind]

The typed plan -> validate -> apply flow and the centralized entity registry
are good architectural directions. At the current head, however, I reproduced
two paths that silently retain planned PII: null group keys and readable but
malformed managed-persona assets. I also reproduced group identity instability
because discovery computes structural grain but does not use it, and explicit
persona formats being overridden at apply time.

The remaining comments separate smaller contract/test gaps from architectural
decisions. The PR's release plan already calls for the LLM follow-up before the
final merge, and #630, #631, and #632 remain open, so I am treating those tracked
items as follow-up rather than duplicating them as inline findings.

Agent-assisted review: generated by @zywind's Agent and reviewed by @zywind.

[zywind]

🎯 Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift | 🧭 Human decision required

Use structural grain when allocating persona fields.

scoped_column_stats labels columns as group-constant or record-varying, but
detect_structured_columns never reads stats. In a grouped frame with one
constant full_name and 20 varying email values per group, discovery placed
both fields in person_1; apply then emitted 20 different synthetic names within
each group. Carry a typed structural-grain field into ColumnEvidence and use it
when deciding persona membership and instance identity. Please also rename the
internal scope stat (for example, to grain) so it is not confused with the
plan's record / group / dataframe replacement scope.

[zywind]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win | 👤 Author review required

Honor the plan's persona formatting patterns.

own or pats[0] always prefers a format inferred from the source value. As a
result, a hand-authored {LAST}, {First} pattern applied to John Smith emitted
Jane Doe rather than DOE, Jane. Treat supplied patterns as the allowed output
formats, using the source shape only to choose among that list when necessary,
and add an edited round-trip plan regression.

[zywind]

🎯 Security & Privacy | 🔴 Critical | ⚡ Quick win | 🤖 Agent-fix candidate

Reject null group keys before group-scoped replacement.

groupby(..., dropna=True) omits the null bucket here and during persona
extraction, while the apply mask cannot select a null key. Through the public
TabularPiiReplacer path, a group-scoped explicit plan therefore left both a
name and an identifier unchanged on the null-key row. Reject null group keys at
this public boundary before extraction/map construction, and cover both persona
and standalone replacements in the regression test.

[zywind]

🎯 Security & Privacy | 🔴 Critical | ⚡ Quick win | 🤖 Agent-fix candidate

Validate managed persona assets before accepting them.

Any readable parquet is accepted as a managed persona source. A file containing
only sex is sampled successfully, but it has no first_name or last_name, so
a planned full_name remains unchanged without an error or fallback. Validate
the required managed schema immediately after loading and route malformed assets
through the existing warning plus Faker fallback. Add a regression asserting
that planned persona-sourced PII is transformed.

[zywind]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win | 📌 Follow-up acceptable

Reject unsupported scalar full_name values during validation.

An explicit plan assigning guardian_name as full_name validates, but this
apply-time discovery heuristic rejects Alice Smith and Bob Jones; that row
stays raw while an ordinary Jane Doe row is replaced. First-class repeated
personas can remain future work. For the current scalar contract, reject value
shapes that apply will not handle and tell the user to pre-split them instead of
silently passing the original PII through.

[zywind]

🎯 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win | 🤖 Agent-fix candidate

Exercise multi-match warnings against the real registry.

The warn-and-continue behavior is covered only with monkeypatched pattern maps,
so changes to the real registry can bypass the agreed warning contract. Add
cases such as physician_gender and doctor_id using ENTITY_NAME_PATTERNS and
DEMO_LABEL_PATTERNS, and assert that the warning contains every candidate and
the chosen label. This tests the policy without hard-coding synthetic collisions
that production patterns never exercise.

[zywind]

🎯 Stability & Availability | 🟠 Major | 🏗️ Heavy lift | 🧭 Human decision required

Resolve the managed-locale dependency on Faker.

Preflight checks Faker locale support only for the Faker backend, but managed
apply still constructs Faker for persona rendering and standalone maps. The
documented managed locales en_SG, hi_Deva_IN, and hi_Latn_IN are not
supported by the installed Faker version, so they pass preflight and then fail
with Faker's raw locale error. Decide whether those managed locales remain part
of the contract: either decouple managed rendering/randomness from Faker, or
validate every locale passed to Faker and update the documented support matrix.

[zywind]

🎯 Maintainability & Code Quality | 🟡 Minor | 🏗️ Heavy lift | 🧭 Human decision required

Break the handler/detection dependency cycle.

entity_handlers imports skip_reason_named_column from detection, while
persona_grouping locally imports get_handler to avoid the reverse import at
module load. That keeps entity behavior split across the handler and detection
layers and makes the new handler seam shallower than intended. Move skip policy
behind the handler interface, or into a lower-level evidence/policy module that
both layers can depend on, so detection can consume handlers without a cycle.

[coderabbitai]

Actionable comments posted: 3

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 88fb5ed8-e80f-48bc-88fa-d9dcd92fe01e

📥 Commits

Reviewing files that changed from the base of the PR and between 97cc468 and 30dc1fb.

📒 Files selected for processing (11)

• src/nemo_safe_synthesizer/evaluation/assets/jinja/components/training_columns.j2
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py
• src/nemo_safe_synthesizer/pii_replacer/detection/persona_grouping.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/patterns/temporal.py
• src/nemo_safe_synthesizer/pii_replacer/replacement/personas.py
• src/nemo_safe_synthesizer/pii_replacer/replacer.py
• tests/pii_replacer/detection/test_column_names.py
• tests/pii_replacer/patterns/test_temporal.py

🚧 Files skipped from review as they are similar to previous changes (5)

• src/nemo_safe_synthesizer/evaluation/assets/jinja/components/training_columns.j2
• src/nemo_safe_synthesizer/pii_replacer/detection/persona_grouping.py
• src/nemo_safe_synthesizer/pii_replacer/replacer.py
• src/nemo_safe_synthesizer/pii_replacer/patterns/temporal.py
• src/nemo_safe_synthesizer/pii_replacer/replacement/personas.py

📜 Review details

⏰ Context from checks skipped due to timeout. (4)

• GitHub Check: Unit Tests (3.11)
• GitHub Check: Unit Tests (3.14)
• GitHub Check: Unit Tests (3.12)
• GitHub Check: Unit Tests (3.13)

🧰 Additional context used

📓 Path-based instructions (12)

**/*.{md,markdown,py}

📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)

**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

tests/**

📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)

tests/**: Mirror src/ directory structure in tests/ directory for test organization
Auto-mark tests by directory: tests/e2e/ → e2e, tests/smoke/ → smoke, otherwise default to unit

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py

**/*.{py,pyi}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.{py,pyi}: Keep shared Python package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
Use the repository's pinned Ruff tasks for Python formatting, import sorting, and linting rather than invoking unpinned tools directly.
Run the repository's pinned ty type checker and maintain type-correct Python code.

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Include SPDX copyright headers in all source files, except files explicitly listed in .copyrightignore.

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

tests/**/*.py

📄 CodeRabbit inference engine (CONTRIBUTING.md)

All existing tests must pass before submitting a pull request; new features must include tests and bug fixes must include regression tests.

tests/**/*.py: __init__.py files must never be added under tests/.
Use tmp_path fixture for file operations, never write to the repo tree
Mock only external boundaries, not internal implementation details
Use @pytest.mark.parametrize for testing multiple input combinations rather than copy-pasting similar tests

tests/**/*.py: Every test should have exactly one of the category markers: unit, smoke, e2e.
Optional dependencies: use pytest.importorskip to gate on packages that require specific extras.
Dataset/tokenizer fixtures use the fixture_ prefix; CLI helpers use descriptive names (mock_workdir).
Faker: seed with fake.seed_instance(seed) and random.seed(seed) for reproducibility.
Naming: fixture names use fixture_ prefix consistently (e.g., fixture_iris_dataset).
print() is allowed in tests (ruff T201 is suppressed for tests/). Use it freely for debug output in test functions.
Importing from another file under tests/, such as tests/cli/helpers.py does not work due to how pytest operates.
Tests mirror source structure: tests/training/, tests/generation/, etc.

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py

---

⚙️ CodeRabbit configuration file

Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py

**/*.{py,sh,yaml,yml,toml,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Use mise tasks with the repository's pinned tool versions for formatting, checking, and testing before submitting changes.

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use uv for everything -- never pip or raw python. Python 3.11–3.14 with modern syntax (X | Y, list[str], Self).

**/*.py: Use American English spelling: "initialize" not "initialise", "recognize" not "recognise", "color" not "colour".
Never print() for operational output. Approved alternatives: click.echo() for CLI output, sys.stdout.write() for raw output in tools.
X | Y not Optional[X] or Union[X, Y]
list[str] not List[str], dict[str, int] not Dict[str, int]
Self for fluent method returns
Protocol for structural subtyping when you need duck-typing boundaries
Avoid Any -- prefer object, generics, or Protocol
Prefer match/case for dispatch on types or tagged values. Not a blanket rule -- if/elif is fine for simple boolean predicates.
Comprehensions over imperative loops where intent is clearer. No multiple for clauses -- optimize for readability, not conciseness (per Google Python Style Guide sec 2.7).
PascalCase classes, snake_case functions/variables, UPPER_SNAKE_CASE constants, leading _ for private
Order of imports: 1) stdlib, 2) third-party, 3) local (enforced by ruff I001/I002)
from __future__ import annotations -- add to every module.
Google style is mandatory.
A docstring is mandatory for every function that has one or more of: being part of the public API, nontrivial size, or non-obvious logic.

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

**/tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Testing gotchas: asyncio_mode = auto in pytest.ini -- async tests work without @pytest.mark.asyncio. The unit_test marker is deprecated; use unit.

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py

**/*

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*: Every source file requires an SPDX copyright header and mise run format handles this automatically.
Newline at end of file, no trailing whitespace (enforced by pre-commit)

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

---

⚙️ CodeRabbit configuration file

**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.

• Refactor suggestion: use for local maintainability problems introduced
  by the diff when they have clear future cost, such as duplicated setup,
  unclear boundaries, over-mocking, avoidable complexity, or opaque test
  helpers.
• Nitpick: avoid in chill mode. Do not emit formatting, import-order,
  wording, or style-only comments unless automated tools cannot catch the
  issue and it affects maintainability.

Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.

• Major: incorrect generation/training/evaluation behavior, broken
  CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
  cleanup and process-isolation bugs likely to fail CI or production
  runs.
• Minor: localized bugs, missing focused tests for changed behavior, or
  bad test patterns that weaken regression coverage.
• Trivial: small cleanup with no behavior impact. Usually suppress in
  chill mode.
• Info: context only. Avoid unless it helps reviewers understand risk.
  Safe-Synthesizer-specific review focus: - Data ...

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

**/*.{py,md,sh,yaml,yml}

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

Line length: 120 characters for code, comments, and docstrings (configured in ruff.toml)

Files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

src/**/*.py

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

src/**/*.py: Every directory under src/ that contains Python files must include an __init__.py file, even if empty.
Relative imports in src/ (from ..observability import get_logger), absolute imports in tests/ (from nemo_safe_synthesizer.observability import get_logger)

Files:

• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

---

⚙️ CodeRabbit configuration file

Review library code against STYLE_GUIDE.md. Focus on behavior, API contracts, error handling, resource cleanup, typing, logging, and user-facing failures. Public APIs and nontrivial functions need Google-style docstrings.

Files:

• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

src/nemo_safe_synthesizer/pii_replacer/**/*.py

⚙️ CodeRabbit configuration file

Treat PII replacement changes as high-risk. Check entity coverage, replacement determinism, leakage of original values, handling of empty or multilingual text, and compatibility with optional dependencies.

Files:

• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

🧠 Learnings (7)

📓 Common learnings

Learnt from: nina-xu
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 672
File: src/nemo_safe_synthesizer/pii_replacer/entities.py:586-603
Timestamp: 2026-08-12T19:04:29.192Z
Learning: For `src/nemo_safe_synthesizer/pii_replacer/entities.py` `unique_identifier` header detection, compact suffix headers such as `userid` and `orderid` are valid identifier candidates. Ambiguous suffix-only matches such as `valid` and `hybrid` require a dominant structured value pattern before the PII discovery pipeline adds them to the replacement plan.

Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer

Timestamp: 2026-08-12T19:30:40.415Z
Learning: Always use mise tasks or the wrapper scripts in `tools/` instead of running `ruff` or `ty` directly.

Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer

Timestamp: 2026-08-12T19:30:40.415Z
Learning: Do not commit unless the user asks for a commit or PR work.

Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer

Timestamp: 2026-08-12T19:30:40.415Z
Learning: When committing, all commits require DCO sign-off and GPG signing. Always use `git commit --signoff --gpg-sign` (or `-s -S`) -- never write the `Signed-off-by` trailer manually, and never pass `--no-gpg-sign`.

📚 Learning: 2026-05-27T22:20:37.354Z

Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.

Applied to files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py

📚 Learning: 2026-07-27T22:07:22.590Z

Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 673
File: tests/pii_replacer/test_edit.py:327-327
Timestamp: 2026-07-27T22:07:22.590Z
Learning: When tests read structured logging context from Python `logging.LogRecord` instances, don’t access `record.ctx` directly (it isn’t declared on `LogRecord` and will break static typing). Instead, use `getattr(record, "ctx", default)` (or an appropriate fallback) to safely handle cases where `ctx` may or may not be attached. This applies even if Ruff rule `B009` isn’t enabled in the repo.

Applied to files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py

📚 Learning: 2026-07-29T17:12:32.642Z

Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 654
File: tests/config/test_parameters.py:116-118
Timestamp: 2026-07-29T17:12:32.642Z
Learning: In Pydantic validation tests (e.g., models configured with `from_attributes`), when asserting failures from `model_validate(...)`, assert the structured error details (such as `ValidationError.errors()[0]["type"]`, e.g. `"model_attributes_type"`) rather than relying on the human-readable error message text. This keeps tests stable even if wording changes, while still verifying the correct validation rule is triggered.

Applied to files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py

📚 Learning: 2026-08-05T19:07:15.856Z

Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 679
File: tests/conftest.py:89-96
Timestamp: 2026-08-05T19:07:15.856Z
Learning: In the Safe Synthesizer test suite, treat CPU and CUDA installation profiles as the supported pytest collection profiles because they install PyTorch. A bare installation without PyTorch is an incomplete, unsupported profile. GPU-marked test modules may import PyTorch before tests/conftest.py::pytest_collection_modifyitems executes, so do not require that collection hook to prevent such imports.

Applied to files:

• tests/pii_replacer/patterns/test_temporal.py
• tests/pii_replacer/detection/test_column_names.py

📚 Learning: 2026-08-12T19:04:29.192Z

Learnt from: nina-xu
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 672
File: src/nemo_safe_synthesizer/pii_replacer/entities.py:586-603
Timestamp: 2026-08-12T19:04:29.192Z
Learning: For `src/nemo_safe_synthesizer/pii_replacer/entities.py` `unique_identifier` header detection, compact suffix headers such as `userid` and `orderid` are valid identifier candidates. Ambiguous suffix-only matches such as `valid` and `hybrid` require a dominant structured value pattern before the PII discovery pipeline adds them to the replacement plan.

Applied to files:

• tests/pii_replacer/detection/test_column_names.py
• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

📚 Learning: 2026-08-06T15:42:45.969Z

Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 703
File: src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py:356-362
Timestamp: 2026-08-06T15:42:45.969Z
Learning: In NeMo Safe Synthesizer Python code, do not flag diagnostics solely because they report raw DataFrame column names. This is an established convention used in preflight warnings and validation errors. Flag such reporting only when an explicit repository-wide logging or privacy policy requires redaction, or when the diagnostic includes cell values, records, or other sensitive data.

Applied to files:

• src/nemo_safe_synthesizer/pii_replacer/entities.py
• src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/value_recognizers.py
• src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

🪛 ast-grep (0.45.1)

src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py

[warning] 43-43: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(pattern, name, re.IGNORECASE)
Note: [CWE-1333] Inefficient Regular Expression Complexity.

(redos-non-literal-regex-python)

🪛 Ruff (0.16.1)

src/nemo_safe_synthesizer/pii_replacer/entities.py

[warning] 612-612: Consider iterable unpacking instead of concatenation

Replace with iterable unpacking

(RUF005)

🔇 Additional comments (6)

src/nemo_safe_synthesizer/pii_replacer/entities.py (1)

171-177: LGTM!

Also applies to: 191-201, 608-612, 713-784

src/nemo_safe_synthesizer/pii_replacer/entity_handlers.py (1)

8-425: LGTM!

Also applies to: 466-571

tests/pii_replacer/detection/test_column_names.py (1)

265-311: LGTM!

tests/pii_replacer/patterns/test_temporal.py (1)

85-100: LGTM!

src/nemo_safe_synthesizer/pii_replacer/detection/column_names.py (2)

13-13: LGTM!

---

41-44: 🩺 Stability & Availability

Use repository-controlled patterns for header_matches_patterns.

header_matches_patterns receives only repository-controlled EntitySpec.strong_name_patterns values. User-configured plan patterns do not reach this function, and the registry patterns are valid regular expressions.

			> Likely an incorrect or invalid review comment.

[kendrickb-nvidia]

Some higher-level comments from skimming the PR. Haven't done any detailed reading since I know there's still a lot of code iteration going on.

[kendrickb-nvidia]

question: Why is this diff here? Maybe an incorrect merge conflict resolution? Looks like possibly some other unexpected changes like pyproject.toml that are effectively reverting PRs from main.

If we do need to make changes to this secrets config, can we do it in a totally separate PR that just merges directly to main, and is not part of the feature branch changes?

[kendrickb-nvidia]

nit: Consider using a different name for the field, so it's clearer if using the old or new pii replacer config? If this is really the only and best name then we can keep it the same.

[kendrickb-nvidia]

nit: Here and in a lot of other places where we're passing Mapping/dict and other constructed structures around, we should consider using a dataclass, pydantic model, or a regular python class to describe and constrain what's being passed around. E.g. here, the keys should always be values of an enum for both mappings. Encoding that in the typing helps catch bugs and is often easier to read.

[kendrickb-nvidia]

nit: Another great place for a dataclass for the stats instead of a generic and untypable dict. I'll stop commenting on these now :)

[kendrickb-nvidia]

question: @binaryaaron and @zywind are we using Protocol a lot in NSS, do we want to? I'd prefer explicit subclassing unless there's some particular reason not to?

[kendrickb-nvidia]

question: Are other classes setup to do both discovery and replacement-time via one object? This seems a little strange and would be more logical to me to have 2 interfaces/protocols/superclasses that define standard methods each for Discover and Applier or something (with better names).

[kendrickb-nvidia]

suggestion: Can this be unified with existing datetime format inference in src/nemo_safe_synthesizer/data_processing/actions/dates.py (and maybe other places, that's one I know about)?

[kendrickb-nvidia]

comment: I find the conceptualization of the heuristics as the base and a singular PiiEnhancer a little strange. There are many possible ways to do this discovery and we have/are implementing a few right now: LLMbased, hueristics based. Treating those as equals (and with an explicit priority or resolution path) seems better. And perhaps fits splitting out ExactRegexHeuristic, FuzzyRegexHeuristic, LLMHeuristic and then having the weighting, priority be explicit across all three?

I'm not into the full details in this PR so maybe this doesn't fit and would be more awkward, but a possible abstraction to consider.

[kendrickb-nvidia]

question: Does this pattern match other steps in NSS and exhibit the abstractions we want? A "do_something" method that returns None and then separate methods that fetch the results (but thus return None or raise an exception if you haven't previously called transform_df or otherwise mean the typing status of the object is not as neatly defined).

We don't have to keep the same shape as the previous nemo_pii.py object, this is a great time to change it if we want to.

[binaryaaron]

I've taken a first pass with a few agent-powered sweeps for specific inline feedback. I'll take another pass at the broader structural revisions, likely as direct PRs to this branch early next week.

Agent-assisted review: generated by @binaryaaron's Agent and reviewed by @binaryaaron.

[binaryaaron]

🎯 Stability & Availability | 🟠 Major | ⚡ Quick win | 🤖 Agent-fix candidate

Define the all-null replay result.

PII Replay divides by zero when a planned entity column contains only null values. A direct current-head probe reaches a zero unique-value denominator and raises ZeroDivisionError. Please define the empty-entity result and add an all-null regression before division.

[binaryaaron]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win | 🤖 Agent-fix candidate

Count detections by occurrence.

_build_column_statistics() counts dropna().unique() values and reports that number as detected_counts. Three rows with one repeated name report two detections. Please count occurrences for the detection metric and keep cardinality as a separate typed statistic.

[binaryaaron]

🎯 Security & Privacy | 🟠 Major | 🏗️ Heavy lift | 👤 Author review required

Detect source PII embedded in free text.

Free-text PII Replay uses whole-cell Series.isin(). It reports no replay when the source entity is Ada and the generated text is Ada visited the clinic. Please compare detected spans or normalized substrings and add an embedded-entity regression.

[binaryaaron]

🎯 Stability & Availability | 🟠 Major | ⚡ Quick win | 🤖 Agent-fix candidate

Support replacement in categorical columns.

Replacement writes synthetic strings directly into categorical columns. A value outside the existing categories raises TypeError: Cannot setitem on a Categorical with a new category. Please normalize the dtype or extend the categories at the core replacement entry point and add a persona-backed categorical regression.

[binaryaaron]

🎯 Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift | 🧭 Human decision required

Define how legacy replacement plans load.

Saved configurations with replace_pii.globals or replace_pii.steps cannot resume. config.unknown_fields rejects those fields before the selected unknown-field policy can ignore them. Please define the supported saved-run window and add either a loader migration or a versioned compatibility error with tests.

[binaryaaron]

🎯 Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift | 🧭 Human decision required

Define compatibility for removed public and serialized types.

This branch removes public config symbols and serialized data-action types without a compatibility shim or migration test. Saved artifacts and Python callers can fail before they receive a versioned diagnostic. Please define the support window, then add the required re-exports, read-only aliases, or explicit migration errors.