Skip to content

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

Open
asteier2026 wants to merge 7 commits into
mainfrom
asteier2026/bugfix/sens-disp-low-risk-coercion
Open

fix: coerce low combined_risk_level to medium instead of rejecting#210
asteier2026 wants to merge 7 commits into
mainfrom
asteier2026/bugfix/sens-disp-low-risk-coercion

Conversation

@asteier2026

@asteier2026 asteier2026 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The LLM occasionally outputs combined_risk_level='low' with a non-leave_as_is
    protection_method_suggestion (e.g. suppress_inference, generalize)
  • Previously EntityDispositionSchema._validate_protection_consistency raised a ValidationError, which
    caused the entire record to be skipped (UPDATE - sometimes the record isn't skipped, but the entity silently gets dropped from the sensitivity disposition)
  • Now combined_risk_level is promoted to medium — preserving the model's intent to protect the entity
    rather than suppressing it. This is more appropriate for weaker models that apply protection methods
    without fully accounting for contextual risk
  • Updated the corresponding test from asserting a ValidationError to asserting that
    combined_risk_level is promoted and the original protection_method_suggestion is retained

UPDATE - the sensitivity disposition prompt is overloaded and doesn't do as well on gpt-oss-120b. We will do another PR to fix that, but above is a quick fix for now

@asteier2026
asteier2026 requested a review from a team as a code owner July 2, 2026 17:30
@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR changes how EntityDispositionSchema handles the inconsistent combination of combined_risk_level='low' with an active protection method: instead of raising a ValidationError (which could silently drop the entity), the validator now promotes combined_risk_level to 'medium' and emits a logging.warning, preserving the model's protection intent.

  • rewrite.py: Imports logging, adds a module-level logger, and replaces the raise ValueError branch with a field mutation (self.combined_risk_level = CombinedRiskLevel.medium.value) plus a structured warning log. The existing high-risk / leave_as_is error path is unchanged.
  • test_schemas.py: Replaces the single pytest.raises(ValidationError) case with a @pytest.mark.parametrize test over all four active protection methods (replace, generalize, remove, suppress_inference), asserting both the promoted risk level and the serialization type from model_dump().

Confidence Score: 5/5

  • This PR is safe to merge. The change is contained to a single validation branch in one schema class, adds a warning log for observability, and is covered by a parametrized test across all affected protection methods.
  • The coercion logic is straightforward — the field mutation uses .value correctly for use_enum_values=True, the second validation branch (high risk + leave_as_is) is untouched, and StrictEntityDispositionSchema is unaffected because it rejects low at field-parse time before the validator runs. All three previously flagged concerns from prior review rounds have been addressed.
  • No files require special attention.

Important Files Changed

Filename Overview
src/anonymizer/engine/schemas/rewrite.py Replaces the ValueError with a warning log + field coercion in _validate_protection_consistency; assignment uses .value to stay consistent with use_enum_values=True; second validation branch (high + leave_as_is) is unaffected.
tests/engine/test_schemas.py Old single-case ValidationError test replaced with a parametrized test covering all four active protection methods; also validates serialization type consistency via model_dump().

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[LLM output: combined_risk_level='low'\nprotection_method_suggestion ≠ leave_as_is] --> B{_validate_protection_consistency}
    B --> C[Log warning]
    C --> D[Promote combined_risk_level → 'medium'\nKeep protection_method_suggestion unchanged]
    D --> E{combined_risk_level='high'\n+ leave_as_is?}
    E -- No --> F[Return valid EntityDispositionSchema\nwith medium risk + active protection]
    E -- Yes --> G[raise ValueError]

    H[Normal input: consistent risk+method] --> B
    B --> E
Loading

Reviews (7): Last reviewed commit: "fix: suppress type: ignore for use_enum_..." | Re-trigger Greptile

Comment thread src/anonymizer/engine/schemas/rewrite.py Outdated
Comment thread tests/engine/test_schemas.py Outdated
Comment thread src/anonymizer/engine/schemas/rewrite.py Outdated
@asteier2026 asteier2026 changed the title bugfix: coerce low combined_risk_level to leave_as_is instead of rejecting bugfix: coerce low combined_risk_level to medium instead of rejecting Jul 2, 2026
Comment thread src/anonymizer/engine/schemas/rewrite.py Outdated
@asteier2026 asteier2026 changed the title bugfix: coerce low combined_risk_level to medium instead of rejecting fix: coerce low combined_risk_level to medium instead of rejecting Jul 29, 2026
asteier2026 and others added 5 commits July 29, 2026 11:29
…cting

The LLM occasionally outputs combined_risk_level='low' with a non-leave_as_is
protection_method_suggestion. Previously this caused a ValidationError that
dropped the entire record. Now the protection_method_suggestion is silently
coerced to leave_as_is, consistent with the semantics of low combined risk.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: asteier2026 <asteier@nvidia.com>
…protection

When the LLM assigns combined_risk_level='low' alongside a non-leave_as_is
protection method, trust the protection intent over the risk label. Promoting
the risk level to medium preserves the model's decision to protect the entity
rather than silently discarding it.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: asteier2026 <asteier@nvidia.com>
…nsistency

Silent coercion made it impossible to detect in production whether the
inconsistency was a rare blip or a systematic regression from a prompt
or model change. The warning names the entity and both conflicting field
values so it is actionable in logs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: asteier2026 <asteier@nvidia.com>
…hods

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: asteier2026 <asteier@nvidia.com>
Replace entity_value with entity_label in the logger.warning call so
raw PII strings are not written to log aggregators.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@asteier2026
asteier2026 force-pushed the asteier2026/bugfix/sens-disp-low-risk-coercion branch from 0fb61f0 to e5a4b05 Compare July 29, 2026 18:29

@lipikaramaswamy lipikaramaswamy 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.

One non-blocking serialization consistency suggestion.

)
# Trust the protection intent over the risk label; promote risk to medium
# rather than suppressing the protection.
self.combined_risk_level = CombinedRiskLevel.medium

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?

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.

@asteier2026

Copy link
Copy Markdown
Contributor Author

Note that this branch has a different version of rewrite_generation.py than what is being updated in PR 208 - it doesn't have the prereplace logic. How do we handle that?

asteier2026 and others added 2 commits August 11, 2026 14:15
…erialization

Post-validation assignment of CombinedRiskLevel.medium was storing the enum
object instead of a plain string, inconsistent with use_enum_values=True. Use
CombinedRiskLevel.medium.value so model_dump() returns a plain string on both
the coerced and non-coerced paths. Adds assertion to the existing test.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ment

Assigning CombinedRiskLevel.medium.value (str) to a CombinedRiskLevel-typed
field after validation requires a type: ignore[assignment] since the static
type doesn't reflect Pydantic's use_enum_values=True runtime behavior.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants