diff --git a/services/_shared/personas/code_reviewer/dispatch.py b/services/_shared/personas/code_reviewer/dispatch.py index 32e14599..8efa40ff 100644 --- a/services/_shared/personas/code_reviewer/dispatch.py +++ b/services/_shared/personas/code_reviewer/dispatch.py @@ -48,7 +48,7 @@ review_reasoner_diff, ) from voice_pack import VoiceSelection, entitled_voice -from review_types import EFFORTS +from review_types import EFFORTS, SEVERITIES from personas.code_reviewer.dedup import ( dedup_findings, finding_key, parse_rule, prior_keys_from_comments, rule_marker, @@ -768,12 +768,14 @@ def _consolidated_agent_prompt(evaluation: CodeReviewEvaluation) -> str: return block -# Derived from the shared vocabulary so a new effort level can never -# silently drop its chip (the Severity-partition-assert drift class). -_EFFORT_LABELS = {e: e.replace("-", " ") for e in EFFORTS} - # Closed caveman-inspired chrome chips (FLINT-density scanability). # Identifiers and check *names* stay plain ASCII; Markings surface uses these. +# +# These two are hand-written, so they can drift from the shared vocabulary. +# `_chip_vocabulary_drift()` is what stops that, and it is asserted in tests +# against SEVERITIES/EFFORTS - a level added to review_types but not given a +# chip here would otherwise fall through `.get()` to the bare identifier, and +# nothing would say so. _SEVERITY_CHIP: dict[str, str] = { "critical": "💀 critical", "high": "🔥 high", @@ -786,6 +788,21 @@ def _consolidated_agent_prompt(evaluation: CodeReviewEvaluation) -> str: } +def _chip_vocabulary_drift() -> dict[str, frozenset[str]]: + """Vocabulary levels with no chip. Empty dict means no drift. + + Returned rather than asserted at import so a drifted chip table degrades + to the bare identifier in production (ugly) instead of refusing to import + dispatch (fatal). The test is where it must fail loudly. + """ + return { + k: v for k, v in { + "severity": SEVERITIES - set(_SEVERITY_CHIP), + "effort": EFFORTS - set(_EFFORT_CHIP), + }.items() if v + } + + def _severity_chip(severity: str) -> str: return _SEVERITY_CHIP.get(severity, severity) diff --git a/services/webhook/tests/test_code_reviewer_dispatch.py b/services/webhook/tests/test_code_reviewer_dispatch.py index 795c14a4..7b94b367 100644 --- a/services/webhook/tests/test_code_reviewer_dispatch.py +++ b/services/webhook/tests/test_code_reviewer_dispatch.py @@ -1950,11 +1950,28 @@ def test_agent_prompt_blocks_are_fence_breakout_safe(): assert "````" in summary -def test_effort_labels_derive_from_shared_vocabulary(): - """A new effort level can never silently drop its chip - labels derive - from review_types.EFFORTS.""" - from review_types import EFFORTS - assert frozenset(cr_dispatch._EFFORT_LABELS) == EFFORTS +def test_chips_cover_the_whole_shared_vocabulary(): + """A level added to review_types must not silently lose its chip. + + This guard used to assert against `_EFFORT_LABELS`, a derived dict that + NOTHING used - so it could never drift and the test could never fail. The + dicts actually rendered, `_SEVERITY_CHIP` and `_EFFORT_CHIP`, are + hand-written and had no guard at all: a new severity would have fallen + through `.get()` to the bare identifier with nothing to say so. The guard + was watching a decoy. + """ + assert cr_dispatch._chip_vocabulary_drift() == {} + + # And it can actually fail - a guard nobody has seen go red is a guess. + from review_types import SEVERITIES + original = dict(cr_dispatch._SEVERITY_CHIP) + try: + cr_dispatch._SEVERITY_CHIP.pop(next(iter(SEVERITIES))) + assert cr_dispatch._chip_vocabulary_drift() != {} + finally: + cr_dispatch._SEVERITY_CHIP.clear() + cr_dispatch._SEVERITY_CHIP.update(original) + assert cr_dispatch._chip_vocabulary_drift() == {}