fix(ledger): normalize every load rejection to ValueError at the ledger's own entrance
SavingsLedger.load unpacked the payload with `**`, so a valid-JSON but non-object book ([], "x", 3, null) escaped as a raw TypeError — a failure mode no caller catching ValueError would see. The run path was already covered: valuereport.load_ledger caught the TypeError and re-raised it as ValueError, and `run.py --goals` goes through that function. The leak reached only callers outside that one path, which is why the suite stayed green. The fix moves the normalization DOWN into ledger.py, where the public boundary is, and deletes the now-dead patch in valuereport.load_ledger. One except clause now covers the whole boundary: unparsable bytes (JSONDecodeError), non-object top level (explicit check), wrong-shaped object (ValidationError). Load-bearing (§11): the new TestLoadHasOneFailureType went RED before the fix with exactly the TypeError it exists to forbid — pytest.raises(ValueError) does not swallow it. Detach point named in the class docstring: drop the isinstance check and the array/string cases raise TypeError again. Found by cross-checking MAF's 7dab2df; queued in STATE as post 2b, approved by the operator this session. 604 -> 612 passed, ruff + mypy --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQu2xxwedckjU56byu1aUG
This commit is contained in:
parent
3529299335
commit
2c1317bdb5
3 changed files with 62 additions and 12 deletions
|
|
@ -138,8 +138,21 @@ class SavingsLedger:
|
||||||
|
|
||||||
@classmethod
|
@classmethod
|
||||||
def load(cls, path: Path) -> SavingsLedger:
|
def load(cls, path: Path) -> SavingsLedger:
|
||||||
"""Load a persisted ledger, schema-validated fail-fast (§10-style)."""
|
"""Load a persisted ledger, schema-validated fail-fast (§10-style).
|
||||||
parsed = _LedgerFile(**json.loads(path.read_text(encoding="utf-8")))
|
|
||||||
|
ONE failure type for the whole boundary: unparsable bytes, a non-object
|
||||||
|
top level (an array or a bare string would otherwise surface as a raw
|
||||||
|
``TypeError`` from ``**``) and a wrong-shaped object all raise
|
||||||
|
``ValueError``. The normalization lives HERE, at the public entrance,
|
||||||
|
not in one caller — a book is refused the same way whoever opens it.
|
||||||
|
"""
|
||||||
|
payload = json.loads(path.read_text(encoding="utf-8"))
|
||||||
|
if not isinstance(payload, dict):
|
||||||
|
raise ValueError(
|
||||||
|
f"ledger {path} is not a JSON object: top level is "
|
||||||
|
f"{type(payload).__name__} — a book is an object with 'entries'"
|
||||||
|
)
|
||||||
|
parsed = _LedgerFile(**payload)
|
||||||
ledger = cls()
|
ledger = cls()
|
||||||
for entry in parsed.entries:
|
for entry in parsed.entries:
|
||||||
ledger._entries.setdefault(entry.key, entry)
|
ledger._entries.setdefault(entry.key, entry)
|
||||||
|
|
|
||||||
|
|
@ -292,19 +292,16 @@ def _project_value(
|
||||||
def load_ledger(ledger_path: Path | None) -> SavingsLedger:
|
def load_ledger(ledger_path: Path | None) -> SavingsLedger:
|
||||||
"""Load the book fail-fast (§10) — a wrong-SHAPE ledger never masquerades as empty.
|
"""Load the book fail-fast (§10) — a wrong-SHAPE ledger never masquerades as empty.
|
||||||
|
|
||||||
``None`` means no book was supplied (an empty one). A path that exists but
|
``None`` means no book was supplied (an empty one); anything else is opened
|
||||||
holds valid JSON of the wrong shape (an array, a string, ``{"entries": {}}``)
|
by ``SavingsLedger.load``, which refuses malformed bytes, a non-object top
|
||||||
raises: reading it as an empty book would silently report every realized
|
level and a wrong-shaped object alike as ``ValueError``. Reading any of them
|
||||||
saving as unmarked. ``SavingsLedger.load`` unpacks the payload, so a non-object
|
as an empty book would silently report every realized saving as unmarked.
|
||||||
top level surfaces as ``TypeError`` — normalized here to ``ValueError`` so the
|
This function adds ONLY the None case — the failure-type normalization lives
|
||||||
caller has ONE failure type to catch.
|
at the ledger's own entrance, so every caller gets it, not just this path.
|
||||||
"""
|
"""
|
||||||
if ledger_path is None:
|
if ledger_path is None:
|
||||||
return SavingsLedger()
|
return SavingsLedger()
|
||||||
try:
|
return SavingsLedger.load(ledger_path)
|
||||||
return SavingsLedger.load(ledger_path)
|
|
||||||
except TypeError as exc: # non-object JSON: `**` needs a mapping
|
|
||||||
raise ValueError(f"ledger {ledger_path} is not a JSON object: {exc}") from exc
|
|
||||||
|
|
||||||
|
|
||||||
def build_value_report(
|
def build_value_report(
|
||||||
|
|
|
||||||
|
|
@ -169,3 +169,43 @@ class TestDeterministicPersistence:
|
||||||
path.write_text(text, encoding="utf-8")
|
path.write_text(text, encoding="utf-8")
|
||||||
with pytest.raises(ValidationError):
|
with pytest.raises(ValidationError):
|
||||||
SavingsLedger.load(path)
|
SavingsLedger.load(path)
|
||||||
|
|
||||||
|
|
||||||
|
class TestLoadHasOneFailureType:
|
||||||
|
"""LOAD-BEARING (§10-style, §11): every rejected ledger file raises ``ValueError``.
|
||||||
|
|
||||||
|
``load`` is a PUBLIC boundary, and its rejections must be catchable as one
|
||||||
|
type. A non-object top level (an array, a bare string, a number) reached
|
||||||
|
``_LedgerFile(**payload)`` and escaped as a raw ``TypeError`` — a failure
|
||||||
|
mode no caller catching ``ValueError`` would see. The run path patched
|
||||||
|
around it one level up (``valuereport.load_ledger``), so only callers
|
||||||
|
outside that single path met the leak.
|
||||||
|
|
||||||
|
Detach point: remove the top-level object check in ``load`` → the array and
|
||||||
|
string cases raise ``TypeError`` again → RED (``pytest.raises(ValueError)``
|
||||||
|
does not swallow a ``TypeError``).
|
||||||
|
"""
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("payload", ["[]", '["a"]', '"x"', "3", "true", "null"])
|
||||||
|
def test_a_non_object_top_level_is_a_value_error(self, tmp_path: Path, payload: str) -> None:
|
||||||
|
path = tmp_path / "ledger.json"
|
||||||
|
path.write_text(payload, encoding="utf-8")
|
||||||
|
with pytest.raises(ValueError) as exc:
|
||||||
|
SavingsLedger.load(path)
|
||||||
|
assert str(path) in str(exc.value) # the refusal names the offending file
|
||||||
|
|
||||||
|
def test_malformed_json_is_the_same_failure_type(self, tmp_path: Path) -> None:
|
||||||
|
# json.JSONDecodeError IS a ValueError — asserted so the one-type
|
||||||
|
# contract covers unparsable bytes too, not just wrong shapes.
|
||||||
|
path = tmp_path / "ledger.json"
|
||||||
|
path.write_text("{not json", encoding="utf-8")
|
||||||
|
with pytest.raises(ValueError):
|
||||||
|
SavingsLedger.load(path)
|
||||||
|
|
||||||
|
def test_a_wrong_shaped_object_is_the_same_failure_type(self, tmp_path: Path) -> None:
|
||||||
|
# pydantic's ValidationError is a ValueError subclass; the caller needs
|
||||||
|
# ONE except clause for the whole boundary.
|
||||||
|
path = tmp_path / "ledger.json"
|
||||||
|
path.write_text('{"entries": {}}', encoding="utf-8")
|
||||||
|
with pytest.raises(ValueError):
|
||||||
|
SavingsLedger.load(path)
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue