Skip to content

fix(anonymizer): stop REMOVE_INTERSECTIONS from leaving flagged text in clear - #2331

Merged
omri374 merged 2 commits into
data-privacy-stack:mainfrom
anishmehta24:fix/remove-intersections-sort-by-end
Oct 8, 2026
Merged

omri374 merged 2 commits into
data-privacy-stack:mainfrom
anishmehta24:fix/remove-intersections-sort-by-end

Conversation

@anishmehta24

@anishmehta24 anishmehta24 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Change Description

With ConflictResolutionStrategy.REMOVE_INTERSECTIONS, the trimming pass sorts results by start only. After a trim, two results can share a start, and when the longer one ends up first it is compared with the shorter one and trimmed to zero length. Its tail is then left unanonymized, and an empty placeholder is still inserted:

text = "aaaaaaaaaabbbbbcccccdddd"
results = [A(0, 10, 0.9), B(5, 15, 0.5), C(8, 20, 0.4)]
engine.anonymize(text, results, conflict_resolution=REMOVE_INTERSECTIONS).text
# main:  '<A><C><B>cccccdddd'   -> "ccccc" was flagged by C but is left in clear
# fixed: '<A><B><C>dddd'

The fix sorts by (start, end) in both places, so the shorter result is resolved first and the longer one is trimmed after it, and drops zero-length results (start < end instead of start <= end) so no empty placeholder is emitted.

Behavior change: for REMOVE_INTERSECTIONS only, results that were cut to zero length are now kept as non-empty spans after the higher-priority result, and zero-length results are no longer returned. In a randomized check (150k sets of 2-5 random overlapping results over a 30-character text), main left some flagged characters uncovered in 1,579 sets and returned 2,751 zero-length results; with the fix, none of either. Other strategies are unchanged.

Issue reference

No existing issue. Open PR #2287 touches the same block (new opt-in strategy), so one of the two will need a small rebase.

Checklist

  • I have reviewed the contribution guidelines
  • I agree to follow this project's Code of Conduct
  • I confirm that I have the right to submit this contribution and that it does not knowingly contain proprietary or confidential code.
  • My code includes unit tests
  • All unit tests and lint checks pass locally
  • My PR contains documentation updates / additions if required

Two new cases in test_when_remove_intersections_conflict_selected_then_all_conflicts_handled: one fails on main, the other fails if only the filter is reverted to <=. The REMOVE_INTERSECTIONS docstring now describes the trimming. presidio-anonymizer tests: 318 passed (test_ahds_surrogate.py skipped, dotenv not installed here). ruff check clean; ruff format --check flags two pre-existing lines in anonymizer_engine.py that this PR does not touch.

AI assistance (Claude Code) was used for this change.

Copilot AI balanced review requested due to automatic review settings October 7, 2026 00:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

omri374
omri374 previously approved these changes Oct 7, 2026

@omri374 omri374 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The fix is correct. The new test fails on main and passes here. Fuzzing 60k random result sets through the engine: on main, 201 sets left flagged characters uncovered and 442 returned zero-length results; on this branch there are no overlaps, no zero-length output and no uncovered flagged characters. ruff check is clean.

Inline comments cover one gap worth closing before merge (the < filter change has no test) and three non-blocking cleanups.

One item is outside the diff: presidio_anonymizer/entities/conflict_resolution_strategy.py holds the only user-facing description of REMOVE_INTERSECTIONS, and it does not say that lower-scored overlaps are trimmed or that results trimmed to nothing are dropped. Since the PR declares this as a behavior change, a sentence or two in that docstring would keep docs in step with the code (the documentation checkbox in the PR template is also unchecked).


Generated by Claude Code

Comment thread presidio-anonymizer/tests/test_conflict_resolution_strategy.py
Comment thread presidio-anonymizer/presidio_anonymizer/anonymizer_engine.py
Comment thread presidio-anonymizer/presidio_anonymizer/anonymizer_engine.py
Comment thread presidio-anonymizer/presidio_anonymizer/anonymizer_engine.py
Copilot AI balanced review requested due to automatic review settings October 7, 2026 08:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@anishmehta24

Copy link
Copy Markdown
Contributor Author

Thanks for the review. 36124f5 adds the fully-consumed test case and updates the REMOVE_INTERSECTIONS docstring to say lower-scored overlaps are trimmed and results trimmed to nothing are dropped. I left the three non-blocking cleanups (eager drop, sorting only on the branch that moves an element, one shared sort key) out to keep this PR to the fix. Happy to add them here or in a follow-up if you prefer.

@omri374 omri374 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@omri374
omri374 enabled auto-merge (squash) October 7, 2026 12:16
@anishmehta24

Copy link
Copy Markdown
Contributor Author

The red E2E (amd64) job is not from this change. That job's pip cache also restores e2e-tests/env, and the amd64 entry was saved by a runner with CPython 3.10.22 (run 37551204723). This run got 3.10.21, so the restored venv's bin/python points at an interpreter that is not there and python -m venv env fails with ENOENT. Another rerun that lands on a 3.10.22 runner should pass. Putting the Python version in the cache key (or using python -m venv --clear env) would stop it from recurring. Happy to send that as a separate PR.

@omri374
omri374 merged commit 2523c7b into data-privacy-stack:main Oct 8, 2026
67 of 69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants