Skip to content

fix: coerce low combined_risk_level to medium instead of rejecting #210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
asteier2026 wants to merge 7 commits into
base: main
Choose a base branch
from
+23 −8
Open

fix: coerce low combined_risk_level to medium instead of rejecting #210

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c100215
bugfix: coerce low combined_risk_level to leave_as_is instead of reje…
asteier2026 Jul 2, 2026
a4d8724
bugfix: promote combined_risk_level to medium instead of suppressing …
asteier2026 Jul 2, 2026
1d3d689
fix: log warning when combined_risk_level is promoted due to LLM inco…
asteier2026 Jul 2, 2026
6ee088a
test: parametrize low-risk coercion test over all four protection met…
asteier2026 Jul 2, 2026
e5a4b05
fix: redact entity_value from coercion warning log to avoid PII exposure
asteier2026 Jul 29, 2026
3ab0b7a
fix: assign .value in low-risk coercion to preserve use_enum_values s…
asteier2026 Aug 11, 2026
290e218
fix: suppress type: ignore for use_enum_values post-validation assign…
asteier2026 Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/anonymizer/engine/schemas/rewrite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,13 @@

from __future__ import annotations

import logging
from enum import Enum

from pydantic import BaseModel, ConfigDict, Field, ValidationInfo, model_validator

logger = logging.getLogger("anonymizer.schemas.rewrite")

# ---------------------------------------------------------------------------
# Domain
# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -150,10 +153,16 @@ def _validate_protection_consistency(self) -> EntityDispositionSchema:
self.combined_risk_level == CombinedRiskLevel.low
and self.protection_method_suggestion != ProtectionMethod.leave_as_is
):
raise ValueError(
f"Entity {self.id}: combined_risk_level='low' requires protection_method_suggestion='leave_as_is', "
f"got '{self.protection_method_suggestion}'"
logger.warning(
"Entity %d (label=%r): combined_risk_level='low' conflicts with "
"protection_method_suggestion=%r; promoting risk to 'medium'.",
self.id,
self.entity_label,
self.protection_method_suggestion,
)
# Trust the protection intent over the risk label; promote risk to medium
# rather than suppressing the protection.
self.combined_risk_level = CombinedRiskLevel.medium
Comment thread
asteier2026 marked this conversation as resolved.
Outdated

@lipikaramaswamy lipikaramaswamy Aug 10, 2026

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.

Non-blocking: because model_config uses use_enum_values=True, normally validated combined_risk_level values are stored and returned by model_dump() as plain strings. This assignment happens after validation, so the coerced path instead stores a CombinedRiskLevel enum object (although it compares equal to "medium"). Could we assign CombinedRiskLevel.medium.value here and add a model_dump() assertion so coerced and non-coerced records preserve the same serialization contract?

@asteier2026 asteier2026 Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Made suggested changs.

@lipikaramaswamy lipikaramaswamy Aug 14, 2026

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—the .value change looks good. CI is failing because ty requires # ty: ignore[invalid-assignment]. Also, since CombinedRiskLevel subclasses str, the isinstance(..., str) assertion passes for the old enum path too; could you change it to type(...) is str so the regression test actually distinguishes them?

if (
self.combined_risk_level == CombinedRiskLevel.high
and self.protection_method_suggestion == ProtectionMethod.leave_as_is
Expand Down
12 changes: 7 additions & 5 deletions tests/engine/test_schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,11 +244,13 @@ def mixed_disposition() -> SensitivityDispositionSchema:
# EntityDispositionSchema — protection consistency


def test_entity_disposition_invalid_low_risk_but_not_leave_as_is() -> None:
with pytest.raises(ValidationError, match="combined_risk_level='low'"):
EntityDispositionSchema.model_validate(
_make_entity(combined_risk_level="low", protection_method_suggestion="replace")
)
@pytest.mark.parametrize("method", ["replace", "generalize", "remove", "suppress_inference"])
def test_entity_disposition_low_risk_non_leave_as_is_promotes_risk_to_medium(method: str) -> None:
entity = EntityDispositionSchema.model_validate(
_make_entity(combined_risk_level="low", protection_method_suggestion=method)
)
assert entity.combined_risk_level == "medium"
assert entity.protection_method_suggestion == method


def test_entity_disposition_invalid_high_risk_but_leave_as_is() -> None:
Expand Down
Loading