fix: address review comments left unaddressed on #186

[RonShakutai]

Follow-up to #186. @omri374 left a second review round on Aug 17; #186 was merged on Aug 18 without those comments being answered. My mistake — this addresses all four.

1. add_alias() rejected a valid alias on LICENSE

The rollback guard asked where the target's name resolves and compared that to where the new alias landed. Same question for every node but one: the hierarchy declares LICENSE both as a canonical leaf under EMPLOYMENT and as an alias of PROFESSIONAL_LICENSE, so canonicalize("LICENSE") == "PROFESSIONAL_LICENSE".

h.add_alias("LICENSE", "PROF_LIC")
# ValueError: Alias 'PROF_LIC' already resolves to 'LICENSE'

PROF_LIC was brand new — the mapping being reported as a conflict was the one the call created one line earlier, exactly as described in review. The guard now derives the target from the node's path in the tree, so another node claiming the same name can't confuse it.

Swept all 126 canonical entities: 1 rejected a fresh alias before, 0 now.

2. add_alias() silently stole aliases from other entities

Found while fixing (1). add_alias("LOCATION", "EMAIL") was accepted and quietly re-pointed EMAIL from EMAIL_ADDRESS to LOCATION — invalidating any corpus annotated with that label, succeeding or failing purely on dict ordering. Pre-existing, not from #186, but the fix above already required reading the alias's prior owner, so it's a two-line check. Now refused before the hierarchy is touched.

3. Rejected add_alias() logged a spurious shadow warning

The speculative rebuild ran the shadow check while the doomed alias was still attached, so a failed call warned about a state discarded microseconds later, worded as though definitions.py were at fault. _rebuild() now takes warn=False for probe rebuilds; construction still warns.

4. _to_l1() / _to_l0() removed

Deleted rather than taught about aliases, per the review suggestion. _to_l1 was a copy of the branch projection that never learned alias resolution, leaving it disagreeing with to_branch() on 434/570 labels — including LOC, bucketed under a LOC branch of its own instead of LOCATION, the exact mis-bucketing #186 exists to remove. Callerless, so nothing miscounted; deleting it means a future import can't resurrect the divergence.

CHANGELOG correction

The #186 entry described PER as behaving like LOC/ORG. It didn't — PER was a depth-4 leaf under PERSON > NAME. Verified against 1663688 (the commit before #186); the review's table is correct on all four rows:

claim in #186	actual before	now
canonicalize("PER") was "PER"	'NAME'	'PERSON'
PER no longer in all_canonical_entities	already absent	absent
get_depth("PER") was 3	raised EntityNotMappedError	2
to_branch("PER") "still returns PERSON"	'PER' — it did change	'PERSON'

That last row is now called out as a behaviour change rather than filed under "unchanged".

Verification

9 tests added. 7 fail on 2c61890; the other 2 are controls that must keep passing (construction still warns; to_branch resolves aliases). 737 passed, 2 skipped; ruff clean.

[Copilot]

Pull request overview

This PR is a follow-up to #186 to correct edge cases in EntityHierarchy.add_alias() introduced alongside branch-level aliases, remove unused projection helpers from base_evaluator, and update the changelog to accurately describe the PER behavior change.

Changes:

• Fix add_alias() rollback/guard logic to use structural node paths, prevent alias “re-homing”, and silence shadow warnings during speculative rebuilds.
• Remove _to_l1() / _to_l0() from presidio_evaluator.evaluation.base_evaluator.
• Add regression tests for the above and adjust CHANGELOG.md to correct the prior PER description.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
tests/entity_mapping/test_entity_hierarchy.py	Adds regression tests covering add_alias() guard edge cases, warning behavior, and helper removals.
presidio_evaluator/evaluation/base_evaluator.py	Removes unused _to_l1() / _to_l0() helpers (and the now-unused EntityHierarchy import).
presidio_evaluator/entity_mapping/hierarchy.py	Fixes add_alias() guard semantics and adds path-based canonical target derivation + warning suppression in probe rebuilds.
CHANGELOG.md	Documents the fixes/removals and corrects the prior PER behavior-change description.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[omri374]

This setdefault runs before the new ownership guard, so a rejected add_alias on a branch node still mutates the tree:

h = EntityHierarchy()
before = copy.deepcopy(h.hierarchy)

h.add_alias("EMPLOYMENT", "EMAIL")
# ValueError: Alias 'EMAIL' already resolves to 'EMAIL_ADDRESS',
#             so it cannot be added to 'EMPLOYMENT'

h.hierarchy == before                              # False
h.hierarchy["PII"]["EMPLOYMENT"]["_aliases"]       # []  <- left by the rejected call

[omri374]

The method this now guards still contains the bug this PR fixes. _warn_on_shadowed_branch_aliases asks where the branch's name resolves:

expected = self.raw_to_canonical.get(self.normalize(branch_key))

That's the same name-based lookup add_alias just replaced with _canonical_name_for_path(path), and it goes wrong on the same shape, a branch whose name is also an alias of something else:

hier = copy.deepcopy(HIERARCHY)
hier["PII"]["EMPLOYMENT"]["LICENSE"] = {"_aliases": ["WORK_LIC"], "SUB_LIC": []}

h = EntityHierarchy(hierarchy=hier)
# WARNING  Branch alias 'WORK_LIC' on 'LICENSE' is shadowed by 'LICENSE'
#          and will never resolve to 'PROFESSIONAL_LICENSE'.

h.canonicalize("WORK_LIC")   # 'LICENSE'  <- landed exactly where declared

The alias resolves correctly; the warning says it never will, and points at PROFESSIONAL_LICENSE, which has nothing to do with this branch. Note the warning is what tells maintainers a static collision exists, so a false positive here trains people to ignore the one signal for real collisions.

The shipped hierarchy emits 0 warnings today, so nothing is noisy right now. This only fires once someone adds a branch in that shape. _collect_branch_aliases would need to carry the path down for the fix, since _canonical_name_for_path needs it.

[omri374]

The warn=False change isn't covered — reverting both call sites to self._rebuild() leaves the suite green:

$ sed -i 's/self\._rebuild(warn=False)/self._rebuild()/g' hierarchy.py
$ pytest tests/entity_mapping -q
501 passed

test_rejected_add_alias_does_not_warn uses add_alias("LOCATION", "CITY"), which the new ownership guard rejects at line 152 — before line 166 ever runs. No rebuild, so no warning, with or without warn=False. It passes for a reason unrelated to what it's named after.

This is really the same observation as the dead-guard comment: nothing can currently reach the probe rebuild and produce a shadow warning, which is why the flag is untestable through the public API. If the second guard goes, warn=False can go with it. If it stays as a backstop, the honest test is a direct _rebuild(warn=False) call asserting no warning, rather than one routed through add_alias.

Its sibling test_construction_still_warns_about_static_shadowing is a genuine control and does fail if _rebuild's default flips to False — worth keeping either way.

---

Generated by Claude Code

[omri374]

LICENSE is a bug unrelated to this (I opened #199), so let's not use it as an example.

[omri374]

This can't fire any more. The check at line 152 already covers it.

Line 152 refuses when the alias name is already owned by someone other than the target, so there are only two ways past it: nobody owns the name (previously is None), or the target already owns it
(previously == expected).

If the target already owns it, adding it again changes nothing, so resolved == expected. If nobody owns it, this call adds the only occurrence, on the target, so again resolved == expected. Either way this
condition is false.

Being shadowed by a descendant means that descendant already owns the name,
which is the case line 152 rejects. Both checks ask the same question, one
before the write and one after.

The example in the comment above shows it. add_alias("LOCATION", "CITY") stops at line 152 with already resolves to 'ADDRESS' and never gets here.

Every node name in the tree is in raw_to_canonical, deep leaves included, so no descendant can shadow you without line 152 seeing it first.

So this block, its message, the rollback and the second _rebuild are dead, and warn=False only exists to keep that rollback quiet.

Delete it, or keep it as a backstop with a note that nothing reaches it. As written, the comment above reads like this is the main guard.