fix(s51): close 2 review MINORs — non-UTF-8 skip + affected_codes parity
Both live in the tolerant inbox-read path and both contradicted the module's own "skipped, never raised" / loader-parity docstrings (S5.1 review: ALLOW, 2 MINOR, non-gating): - UnicodeDecodeError (a *.json hand-saved in Latin-1 with Norwegian æ/ø/å is invalid UTF-8) now SKIPPED in hitl._load_json_dict AND verdicts.load_verdicts_from_dir — was: crashed pending/route with a raw traceback (a ValueError subclass, caught by neither OSError nor JSONDecodeError). Fix symmetric across both readers. - hitl._inbox_verdict_ids now skips a non-iterable affected_codes exactly as load_verdicts_from_dir does (frozenset() raises TypeError) — was: marked the proposal judged on key-presence alone → a silent false-negative in the operator pending queue. hitl-only: verdicts is the correct reference. TDD: 3 RED-then-green (both UnicodeDecodeError twins + the parity gap) + 1 ground-truth pin locking the loader side so the parity cannot rot. Gate: pytest → 389 passed, 4 skipped (was 385); ruff check + format clean; mypy clean.
This commit is contained in:
parent
5807ac428c
commit
7e86896bc8
5 changed files with 131 additions and 4 deletions
|
|
@ -111,8 +111,11 @@ def _inbox_verdict_ids(verdict_dir: str) -> set[str]:
|
||||||
"""Collect the ``id`` of every inbox verdict file that ``load_verdicts_from_dir`` WOULD accept —
|
"""Collect the ``id`` of every inbox verdict file that ``load_verdicts_from_dir`` WOULD accept —
|
||||||
the same tolerant predicate mirrored inline: a dict carrying all ``_REQUIRED_VERDICT_KEYS``, a
|
the same tolerant predicate mirrored inline: a dict carrying all ``_REQUIRED_VERDICT_KEYS``, a
|
||||||
``decision`` in the binary vocabulary ``{approved, rejected}``, and a ``proposal_features`` dict
|
``decision`` in the binary vocabulary ``{approved, rejected}``, and a ``proposal_features`` dict
|
||||||
carrying the inner keys ``verdict_from_dict`` reads. A file that would be skipped or raise in the
|
carrying the inner keys ``verdict_from_dict`` reads AND an ``affected_codes`` that
|
||||||
real loader is skipped here too, so ``pending`` never treats it as a delivered dom."""
|
``frozenset(...)`` accepts (a non-iterable — ``null``/int/float/bool — makes the real loader's
|
||||||
|
``frozenset(pf['affected_codes'])`` raise ``TypeError``, so it is skipped here too). A file that
|
||||||
|
would be skipped or raise in the real loader is skipped here too, so ``pending`` never treats it as
|
||||||
|
a delivered dom."""
|
||||||
directory = Path(verdict_dir)
|
directory = Path(verdict_dir)
|
||||||
if not directory.is_dir():
|
if not directory.is_dir():
|
||||||
return set()
|
return set()
|
||||||
|
|
@ -126,6 +129,12 @@ def _inbox_verdict_ids(verdict_dir: str) -> set[str]:
|
||||||
features = data.get("proposal_features")
|
features = data.get("proposal_features")
|
||||||
if not isinstance(features, dict) or not _REQUIRED_FEATURE_KEYS <= features.keys():
|
if not isinstance(features, dict) or not _REQUIRED_FEATURE_KEYS <= features.keys():
|
||||||
continue
|
continue
|
||||||
|
# Mirror verdict_from_dict EXACTLY: frozenset(affected_codes) raises TypeError on a
|
||||||
|
# non-iterable (null/int/float/bool), which the real loader skips — so skip it here too.
|
||||||
|
try:
|
||||||
|
frozenset(features["affected_codes"])
|
||||||
|
except TypeError:
|
||||||
|
continue
|
||||||
ids.add(data["id"])
|
ids.add(data["id"])
|
||||||
return ids
|
return ids
|
||||||
|
|
||||||
|
|
@ -237,7 +246,9 @@ def _load_json_dict(file: Path) -> dict[str, Any] | None:
|
||||||
unreadable / non-JSON / non-object file is skipped by every reader here)."""
|
unreadable / non-JSON / non-object file is skipped by every reader here)."""
|
||||||
try:
|
try:
|
||||||
data = json.loads(file.read_text(encoding="utf-8"))
|
data = json.loads(file.read_text(encoding="utf-8"))
|
||||||
except (OSError, json.JSONDecodeError):
|
except (OSError, UnicodeDecodeError, json.JSONDecodeError):
|
||||||
|
# UnicodeDecodeError: a *.json file hand-saved in Latin-1 (Norwegian æ/ø/å) is invalid UTF-8 —
|
||||||
|
# skip it, not raise (a ValueError subclass, so neither OSError nor JSONDecodeError catches it).
|
||||||
return None
|
return None
|
||||||
return data if isinstance(data, dict) else None
|
return data if isinstance(data, dict) else None
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -212,7 +212,10 @@ def load_verdicts_from_dir(
|
||||||
for file in files:
|
for file in files:
|
||||||
try:
|
try:
|
||||||
data = json.loads(file.read_text(encoding="utf-8"))
|
data = json.loads(file.read_text(encoding="utf-8"))
|
||||||
except (OSError, json.JSONDecodeError):
|
except (OSError, UnicodeDecodeError, json.JSONDecodeError):
|
||||||
|
# UnicodeDecodeError: a *.json file hand-saved in Latin-1 (Norwegian æ/ø/å) is invalid
|
||||||
|
# UTF-8 — a per-file tolerant skip, not a raise (a ValueError subclass, caught by neither
|
||||||
|
# OSError nor JSONDecodeError). Kept in lockstep with hitl._load_json_dict.
|
||||||
continue
|
continue
|
||||||
if not isinstance(data, dict) or not _REQUIRED_VERDICT_KEYS <= data.keys():
|
if not isinstance(data, dict) or not _REQUIRED_VERDICT_KEYS <= data.keys():
|
||||||
continue
|
continue
|
||||||
|
|
|
||||||
|
|
@ -128,6 +128,36 @@ def test_pending_tolerant_to_missing_dir_and_foreign_files(tmp_path: Path) -> No
|
||||||
assert [p.run_id for p in result] == ["run-1"]
|
assert [p.run_id for p in result] == ["run-1"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_pending_tolerant_to_non_utf8_inbox_file(tmp_path: Path) -> None:
|
||||||
|
"""A hand-authored inbox verdict saved in Latin-1 (Norwegian ``æ/ø/å``) is valid JSON bytes but
|
||||||
|
INVALID UTF-8; it matches the ``*.json`` glob yet must be SKIPPED, never crash ``pending``/``route``
|
||||||
|
with a raw traceback — the module's "skipped, never raised" contract. Realistic in a Norwegian
|
||||||
|
domain. Its ``id`` equals the proposal's ``verdict_id``, so IF it were (wrongly) read it would clear
|
||||||
|
the queue → ``[]``; a clean skip keeps the proposal pending → ``["run-1"]`` uniquely proves it was
|
||||||
|
skipped, not read and not raised. Before the ``UnicodeDecodeError`` catch, this ERRORs (RED)."""
|
||||||
|
outbox = tmp_path / "outbox"
|
||||||
|
inbox = tmp_path / "inbox"
|
||||||
|
_write_proposal(outbox, "run-1", verdict_id="v1")
|
||||||
|
inbox.mkdir()
|
||||||
|
(inbox / "latin1.json").write_bytes(
|
||||||
|
json.dumps(
|
||||||
|
{
|
||||||
|
"id": "v1",
|
||||||
|
"decision": "approved",
|
||||||
|
"rationale": "godkjent på møtet",
|
||||||
|
"proposal_features": {
|
||||||
|
"affected_codes": ["05.2"],
|
||||||
|
"measure_type": "scope_reduction",
|
||||||
|
"claimed_saving_nok": 200.0,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
ensure_ascii=False,
|
||||||
|
).encode("latin-1")
|
||||||
|
)
|
||||||
|
|
||||||
|
assert [p.run_id for p in hitl.pending(str(outbox), str(inbox))] == ["run-1"]
|
||||||
|
|
||||||
|
|
||||||
def test_pending_skips_orphan_proposal_without_outcome(tmp_path: Path) -> None:
|
def test_pending_skips_orphan_proposal_without_outcome(tmp_path: Path) -> None:
|
||||||
"""A proposal file with no matching outcome (orphan from a half-written live run) is skipped."""
|
"""A proposal file with no matching outcome (orphan from a half-written live run) is skipped."""
|
||||||
outbox = tmp_path / "outbox"
|
outbox = tmp_path / "outbox"
|
||||||
|
|
|
||||||
|
|
@ -193,6 +193,36 @@ def test_inbox_idset_skips_malformed_features(tmp_path: Path) -> None:
|
||||||
assert [p.run_id for p in hitl.pending(str(outbox), str(inbox))] == ["run-1"]
|
assert [p.run_id for p in hitl.pending(str(outbox), str(inbox))] == ["run-1"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_inbox_idset_skips_non_iterable_affected_codes(tmp_path: Path) -> None:
|
||||||
|
"""PARITY (wrong-type): an inbox verdict with every required key present but ``affected_codes`` a
|
||||||
|
NON-ITERABLE (``null``) is skipped — the real loader's ``frozenset(pf['affected_codes'])``
|
||||||
|
(verdicts.py) raises ``TypeError`` and ``load_verdicts_from_dir`` drops it. So ``pending`` must NOT
|
||||||
|
count it as judged; otherwise ``hitl`` marks the proposal judged while the learning pipeline still
|
||||||
|
treats it pending — a silent false-negative in the operator queue, the exact failure the parity
|
||||||
|
docstring says it prevents. Detach the ``affected_codes`` iterability check (accept on key-presence
|
||||||
|
alone) → the file clears pending → RED. Ground-truth twin:
|
||||||
|
``test_verdicts.test_load_skips_non_iterable_affected_codes``."""
|
||||||
|
outbox = tmp_path / "outbox"
|
||||||
|
inbox = tmp_path / "inbox"
|
||||||
|
_write_proposal(outbox, "run-1", verdict_id="SHARED-VID")
|
||||||
|
_write_raw_verdict(
|
||||||
|
inbox,
|
||||||
|
"SHARED-VID",
|
||||||
|
{
|
||||||
|
"id": "SHARED-VID",
|
||||||
|
"decision": "approved",
|
||||||
|
"rationale": "ok",
|
||||||
|
"proposal_features": {
|
||||||
|
"affected_codes": None, # non-iterable → real loader's frozenset() raises → skips
|
||||||
|
"measure_type": "scope_reduction",
|
||||||
|
"claimed_saving_nok": 200.0,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
|
assert [p.run_id for p in hitl.pending(str(outbox), str(inbox))] == ["run-1"]
|
||||||
|
|
||||||
|
|
||||||
# --- transitive import-graph MAF-freedom probe (Step 5) -------------------------------------------
|
# --- transitive import-graph MAF-freedom probe (Step 5) -------------------------------------------
|
||||||
|
|
||||||
_SRC_DIR = Path(__file__).resolve().parents[1] / "src" / "portfolio_optimiser"
|
_SRC_DIR = Path(__file__).resolve().parents[1] / "src" / "portfolio_optimiser"
|
||||||
|
|
|
||||||
|
|
@ -7,6 +7,7 @@ genuine two-arg ``extend_instructions(source_id, instructions)`` GA signature
|
||||||
Critical Fase 1 risk. Pattern: tests/spikes/test_d_verdictstore.py + real SessionContext.
|
Critical Fase 1 risk. Pattern: tests/spikes/test_d_verdictstore.py + real SessionContext.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
import json
|
||||||
import logging
|
import logging
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
|
|
@ -205,6 +206,58 @@ def test_load_within_caps_is_unchanged(tmp_path) -> None:
|
||||||
assert {v.decision for v in loaded} == {"approved", "rejected"}
|
assert {v.decision for v in loaded} == {"approved", "rejected"}
|
||||||
|
|
||||||
|
|
||||||
|
def test_load_skips_non_utf8_file(tmp_path) -> None:
|
||||||
|
"""T-2.5d: a hand-authored inbox verdict saved in Latin-1 (Norwegian ``æ/ø/å``) is valid JSON bytes
|
||||||
|
but INVALID UTF-8; it matches the ``*.json`` glob yet must be SKIPPED, never raise — the loader's
|
||||||
|
per-file tolerant contract (the Step-7 loop relies on it). Detach the ``UnicodeDecodeError`` catch
|
||||||
|
→ ``read_text(encoding='utf-8')`` raises → RED. The valid verdict alongside still loads."""
|
||||||
|
(tmp_path / "latin1.json").write_bytes(
|
||||||
|
json.dumps(
|
||||||
|
{
|
||||||
|
"id": "X",
|
||||||
|
"decision": "approved",
|
||||||
|
"rationale": "godkjent på møtet",
|
||||||
|
"proposal_features": {
|
||||||
|
"affected_codes": ["05.2"],
|
||||||
|
"measure_type": "m",
|
||||||
|
"claimed_saving_nok": 200.0,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
ensure_ascii=False,
|
||||||
|
).encode("latin-1")
|
||||||
|
)
|
||||||
|
write_verdict(str(tmp_path), capture_verdict(_feats("Y"), "approved", "fine"))
|
||||||
|
|
||||||
|
loaded = load_verdicts_from_dir(str(tmp_path))
|
||||||
|
assert [v.decision for v in loaded] == ["approved"] # latin1 file skipped, the valid one loads
|
||||||
|
|
||||||
|
|
||||||
|
def test_load_skips_non_iterable_affected_codes(tmp_path) -> None:
|
||||||
|
"""Parity ground-truth: ``verdict_from_dict``'s ``frozenset(affected_codes)`` raises ``TypeError``
|
||||||
|
on a non-iterable (``null``), and ``load_verdicts_from_dir`` SKIPS it. This pins the reference the
|
||||||
|
hitl inbox predicate mirrors (``test_hitl_loadbearing.test_inbox_idset_skips_non_iterable_affected_codes``)
|
||||||
|
so the parity contract cannot silently rot on the loader side."""
|
||||||
|
(tmp_path / "bad.json").write_text(
|
||||||
|
json.dumps(
|
||||||
|
{
|
||||||
|
"id": "X",
|
||||||
|
"decision": "approved",
|
||||||
|
"rationale": "r",
|
||||||
|
"proposal_features": {
|
||||||
|
"affected_codes": None,
|
||||||
|
"measure_type": "m",
|
||||||
|
"claimed_saving_nok": 1.0,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
),
|
||||||
|
encoding="utf-8",
|
||||||
|
)
|
||||||
|
write_verdict(str(tmp_path), capture_verdict(_feats("Y"), "approved", "fine"))
|
||||||
|
|
||||||
|
loaded = load_verdicts_from_dir(str(tmp_path))
|
||||||
|
assert [v.decision for v in loaded] == ["approved"] # non-iterable affected_codes skipped
|
||||||
|
|
||||||
|
|
||||||
def test_no_inbox_json_uses_approved_with_adjustment() -> None:
|
def test_no_inbox_json_uses_approved_with_adjustment() -> None:
|
||||||
"""Assumption 3 (TDD guard): no shipped/test ``.json`` uses the promotion-only decision
|
"""Assumption 3 (TDD guard): no shipped/test ``.json`` uses the promotion-only decision
|
||||||
``approved_with_adjustment`` — the vocabulary SKIP would now silently drop it. It lives only in
|
``approved_with_adjustment`` — the vocabulary SKIP would now silently drop it. It lives only in
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue