feat: apply direct identifier replacements before rewrite LLM call - #208
feat: apply direct identifier replacements before rewrite LLM call#208asteier2026 wants to merge 11 commits into
Conversation
Direct identifiers are now substituted programmatically from the replacement map before the rewrite LLM sees the text, ensuring all occurrences are replaced consistently without relying on the LLM to apply a <replacement_map> block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: asteier2026 <asteier@nvidia.com>
Greptile SummaryThis PR moves direct-identifier replacement from the rewrite LLM to a deterministic pre-processing step. Before the LLM sees the text,
Confidence Score: 5/5
Important Files Changed
|
Sequential str.replace() calls could incorrectly replace a synthetic value that happened to match another entity's original string (e.g. Alice→Bob then Bob→Carlos making Alice appear as Carlos). A combined regex alternation matches all originals simultaneously, eliminating the cascade. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: asteier2026 <asteier@nvidia.com>
Moved the missing-map warning into _get_replace_pairs so the disposition is parsed in one place. _apply_direct_replacements no longer re-parses COL_SENSITIVITY_DISPOSITION when _get_replace_pairs returns an empty list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: asteier2026 <asteier@nvidia.com>
The LLM generating the replacement map occasionally normalises unusual Unicode whitespace (e.g. U+202F narrow no-break space) to a regular space in the original field. The exact-match lookup then misses the entity, triggering the unprotected warning. Add _normalize_ws and a second-pass lookup so that if the exact match fails, a whitespace-normalised comparison is tried. When a normalised match is found the disposition entity value (which reflects what is actually in the text) is used as the substitution key. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: asteier2026 <asteier@nvidia.com>
…cefully Any failure (malformed disposition, bad replacement map, regex error) previously caused the entire record to be skipped. Now the error is logged and the original text is passed through unchanged so the LLM rewrite step can still run. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: asteier2026 <asteier@nvidia.com>
The LLM generating the replacement map normalises unusual Unicode whitespace (e.g. U+202F narrow no-break space) to a regular space in the original field. _filter_replacement_map_to_input_entities was using an exact match against the detected entity values, so these entries were silently dropped from the map. Add a whitespace-normalized fallback: when the exact (original, label) pair is not in allowed_pairs, try a normalized comparison and, if it matches, rewrite original to the canonical detected value so all downstream lookups succeed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: asteier2026 <asteier@nvidia.com>
…matched _get_replace_pairs now returns (pairs, replace_values) so the caller can detect gaps. _apply_direct_replacements raises RuntimeError when any required entity has no replacement entry, covering both the missing-map and partial-map cases. Silently passing PII-containing text to the rewrite LLM is unsafe because replace entities are excluded from the disposition block and would receive no protection instructions. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
| pairs = _get_replace_pairs(row) | ||
| if pairs: | ||
| sorted_pairs = sorted(pairs, key=lambda p: len(p[0]), reverse=True) | ||
| pattern = re.compile("|".join(re.escape(original) for original, _ in sorted_pairs)) |
There was a problem hiding this comment.
This global regex does not preserve the span-aware matching semantics used by the Substitute workflow. For example, Ann → Maria transforms Ann met Anna into Maria met Mariaa, whereas _apply_replacement_map_to_text() replaces only detected entity spans.
Can the rewrite path reuse or adapt the existing span-aware replacement logic—including for tagged text—and add this case as a regression test?
There was a problem hiding this comment.
One additional thought for the span-aware approach: could the regression test also confirm that every detected span selected for replace was actually transformed? That would give us an explicit coverage check while preserving the existing boundary-aware semantics.
| unmatched = replace_values - matched | ||
| if unmatched: | ||
| logger.warning( | ||
| "Replace entities have no entry in the replacement map and will pass through unprotected: %s", |
There was a problem hiding this comment.
Small logging-safety suggestion: sorted(unmatched) includes raw detected entity values, so the PII being protected can be persisted in operational logs. If this becomes an exception, including the values there would have the same issue. Could we report only safe metadata such as the record ID, entity IDs or labels, and counts?
lipikaramaswamy
left a comment
There was a problem hiding this comment.
Thanks for moving direct replacement into deterministic preprocessing—the overall direction makes sense. I’m requesting changes for the two safety and correctness gaps discussed inline: incomplete replacement maps currently fail open after replace entities are omitted from the prompt, and the global regex can modify text outside detected entity spans. There is also a smaller logging-safety concern around including raw entity values. Fail-safe handling, span-aware replacement, and focused regression tests would put this in good shape for another look.
…e error logging Three reviewer-requested changes: - Plain text: reuses _apply_replacement_map_to_text (span-aware via character offsets from COL_FINAL_ENTITIES) so 'Ann' cannot corrupt 'Anna' - Tagged text: new _apply_tagged_text_replacements replaces only within tag wrappers per notation (xml/bracket/paren/sentinel), preventing substring contamination without needing remapped offsets - Error message: omits raw entity values; reports count and entity labels only (_get_replace_pairs now returns (original, synthetic, label) triples) - Tests: regression test for Ann/Anna substring guard, coverage check that all replace spans transform, updated existing tests with COL_TAG_NOTATION Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Made these changes:
|
| def _apply_tagged_text_replacements( | ||
| tagged_text: str, pairs: list[tuple[str, str, str]], tag_notation: str | ||
| ) -> str: | ||
| """Replace entity values in tagged text using tag-boundary-aware matching. | ||
|
|
||
| Matches each entity value only when it appears as the text content of its | ||
| corresponding tag wrapper, preventing substring corruption (e.g. 'Ann' inside | ||
| 'Anna' is safe because the tagged form '<first_name>Ann</first_name>' is bounded | ||
| by tag delimiters that 'Anna' does not share). | ||
| """ | ||
| for original, synthetic, label in sorted(pairs, key=lambda p: len(p[0]), reverse=True): | ||
| esc_o = re.escape(original) | ||
| esc_l = re.escape(label) | ||
| if tag_notation == "xml": | ||
| tagged_text = re.sub( | ||
| r"(<" + esc_l + r">)" + esc_o + r"(</" + esc_l + r">)", | ||
| lambda m, s=synthetic: m.group(1) + s + m.group(2), | ||
| tagged_text, | ||
| ) | ||
| elif tag_notation == "bracket": | ||
| tagged_text = re.sub( | ||
| r"\[\[" + esc_o + r"\|" + esc_l + r"\]\]", | ||
| lambda m, s=synthetic, l=label: "[[" + s + "|" + l + "]]", | ||
| tagged_text, | ||
| ) | ||
| elif tag_notation == "paren": | ||
| tagged_text = re.sub( | ||
| r"\(\(SENSITIVE:" + esc_l + r"\|" + esc_o + r"\)\)", | ||
| lambda m, s=synthetic, l=label: "((SENSITIVE:" + l + "|" + s + "))", | ||
| tagged_text, | ||
| ) | ||
| else: # sentinel | ||
| tagged_text = re.sub( | ||
| r"(<<SENSITIVE:" + esc_l + r">>)" + esc_o + r"(<</SENSITIVE:" + esc_l + r">>)", | ||
| lambda m, s=synthetic: m.group(1) + s + m.group(2), | ||
| tagged_text, | ||
| ) | ||
| return tagged_text |
There was a problem hiding this comment.
Sequential loop in
_apply_tagged_text_replacements can still cascade
The function iterates over pairs sequentially, so a synthetic value that matches another entity's original is re-replaced in a later iteration. For example, with Alice → "Bob" and Bob → "Carlos", iteration 1 rewrites [[Alice|first_name]] to [[Bob|first_name]], and iteration 2 then matches the freshly written [[Bob|first_name]] and replaces it with [[Carlos|first_name]] — Alice ends up as Carlos in COL_PREREPLACE_TAGGED_TEXT while plain text correctly has "Bob". The LLM then sees wrong synthetic values in the tagged text it is asked to rewrite.
The existing cascade test (test_apply_direct_replacements_no_cascade_when_synthetic_matches_another_original) passes silently because COL_TAGGED_TEXT is set to "Alice and Bob met." (plain, untagged text). The xml-mode regex (<first_name>)Alice(</first_name>) finds no matches in that string, so _apply_tagged_text_replacements makes no substitutions and the cascade is never exercised. There is also no assertion on COL_PREREPLACE_TAGGED_TEXT in that test.
The plain-text path uses a single-pass regex (re.compile("|".join(...)).sub(...)) to avoid this exact problem. The tagged-text path needs the same treatment — build a single combined regex per notation format that matches all tagged originals simultaneously, then look up the replacement in one pass.
There was a problem hiding this comment.
Just fixed this too
Sequential per-pair regex substitution in _apply_tagged_text_replacements allowed cascade (Alice→Bob then Bob→Carlos in the same tagged text). Rewrite as a single-pass lookup substitution matching full tagged spans, mirroring the plain-text path. Updates the cascade test to use real tagged text and assert on COL_PREREPLACE_TAGGED_TEXT. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…kflow.py Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Update 8/13: I think PR #246 supersedes this PR now
Summary
the replacement map before the rewrite LLM call, using a single-pass regex to prevent cascade
replacements (e.g. Alice→Bob→Carlos)
replacements
_get_replace_pairs to handle LLM-normalised Unicode whitespace (e.g. U+202F → U+0020) in entity values
dropping the record
Motivation
The rewrite LLM was inconsistently applying replacement map entries, especially for entities that
appear multiple times. Programmatic pre-replacement guarantees all occurrences are substituted before
the LLM sees the text. The whitespace fixes handle cases where the LLM normalises unusual Unicode
whitespace in entity values, which caused map lookups to silently miss.