From b8e96294a6ab0cc3e53eee8e6314b3e65bf6cc65 Mon Sep 17 00:00:00 2001 From: ctan Date: Sun, 20 Sep 2026 17:02:39 +0200 Subject: [PATCH] fix(loaders): non-list features and blank/non-string entries fail cleanly features was popped and only re-added when it was a list, so a bare string (or dict/null) loaded with zero feats - silent data loss. Now a present non-list features raises CombatantLoadError, and the list resolution adopts an accept-string-or-error contract: non-string or blank entries raise instead of being silently dropped. --- src/pf1e_simulator/loaders/combatant.py | 35 ++++++++++++++----- tests/test_combatant_loader.py | 46 ++++++++++++++++++------- 2 files changed, 60 insertions(+), 21 deletions(-) diff --git a/src/pf1e_simulator/loaders/combatant.py b/src/pf1e_simulator/loaders/combatant.py index 55aa41f..9716113 100644 --- a/src/pf1e_simulator/loaders/combatant.py +++ b/src/pf1e_simulator/loaders/combatant.py @@ -187,13 +187,24 @@ class CombatantLoadError(Exception): self.reason = reason -def _resolve_features(raw_features: list[Json], level: int) -> list[AbilitySpec]: - """Resolve a list of feat name strings to AbilitySpec objects.""" - return [ - _resolve_feat(item, level) - for item in raw_features - if isinstance(item, str) and item.strip() - ] +def _resolve_features( + raw_features: list[Json], level: int, path: Path +) -> list[AbilitySpec]: + """Resolve a list of feat name strings to AbilitySpec objects. + + Data contract (accept-string-or-error): every entry must be a + non-blank string. A non-string entry (``42``, ``None``, a nested + list) or a blank/whitespace-only string is a data error and raises + ``CombatantLoadError`` -- nothing is silently dropped, so a feat can + never disappear from a sheet without the loader failing loudly. + """ + specs: list[AbilitySpec] = [] + for item in raw_features: + if not isinstance(item, str) or not item.strip(): + msg = f"features entries must be non-empty feat name strings, got {item!r}" + raise CombatantLoadError(path, msg) + specs.append(_resolve_feat(item, level)) + return specs def _merge_resource_grants( @@ -287,13 +298,19 @@ def load_combatant(path: Path) -> Combatant: _auto_attack_ids(data, monster_id) - # Resolve features (list of strings → list of AbilitySpec) + # Resolve features (list of strings → list of AbilitySpec). A present + # but non-list value (e.g. a bare string) used to be popped and dropped + # silently, so the combatant lost every feat; it is now a data error. raw_level = data.get("level") level = raw_level if isinstance(raw_level, int) and not isinstance(raw_level, bool) else 1 + had_features = "features" in data raw_features = data.pop("features", None) + if had_features and not isinstance(raw_features, list): + msg = f"features must be a list of feat name strings, got {raw_features!r}" + raise CombatantLoadError(path, msg) validate_payload: dict[str, object] = dict(data) if isinstance(raw_features, list): - specs = _resolve_features(raw_features, level) + specs = _resolve_features(raw_features, level, path) validate_payload["features"] = specs granted = _merge_resource_grants(data.get("resources"), specs) if granted: diff --git a/tests/test_combatant_loader.py b/tests/test_combatant_loader.py index 0aa3cc2..4b1ebe6 100644 --- a/tests/test_combatant_loader.py +++ b/tests/test_combatant_loader.py @@ -76,22 +76,24 @@ class TestResolveFeat: class TestResolveFeatures: - def test_filters_empty_and_whitespace(self) -> None: - result = _resolve_features(["Power Attack", "", " ", "Dodge"], 1) - assert len(result) == 2 - assert result[0] is POWER_ATTACK - assert result[1] is DODGE + def test_blank_entry_raises(self, tmp_path: Path) -> None: + with pytest.raises(CombatantLoadError, match="non-empty feat name strings"): + _resolve_features(["Power Attack", "", " ", "Dodge"], 1, tmp_path) - def test_filters_non_strings(self) -> None: + def test_non_string_entry_raises(self, tmp_path: Path) -> None: raw: list[Json] = ["Iron Will", 42, None, True] - result = _resolve_features(raw, 1) - assert len(result) == 1 + with pytest.raises(CombatantLoadError, match="non-empty feat name strings"): + _resolve_features(raw, 1, tmp_path) - def test_empty_list(self) -> None: - assert _resolve_features([], 1) == [] + def test_valid_strings_resolve(self, tmp_path: Path) -> None: + result = _resolve_features(["Power Attack", "Dodge"], 1, tmp_path) + assert result == [POWER_ATTACK, DODGE] - def test_all_unknown(self) -> None: - result = _resolve_features(["Feat A", "Feat B"], 1) + def test_empty_list(self, tmp_path: Path) -> None: + assert _resolve_features([], 1, tmp_path) == [] + + def test_all_unknown(self, tmp_path: Path) -> None: + result = _resolve_features(["Feat A", "Feat B"], 1, tmp_path) assert len(result) == 2 assert all(f.effects == () for f in result) @@ -159,6 +161,26 @@ class TestFeatures: c = load_combatant(_write(tmp_path, data)) assert c.features == [] + def test_string_features_raises(self, tmp_path: Path) -> None: + data = _minimal_payload(features="Toughness") + with pytest.raises(CombatantLoadError, match="features must be a list"): + load_combatant(_write(tmp_path, data)) + + def test_dict_features_raises(self, tmp_path: Path) -> None: + data = _minimal_payload(features={"a": "Toughness"}) + with pytest.raises(CombatantLoadError, match="features must be a list"): + load_combatant(_write(tmp_path, data)) + + def test_null_features_raises(self, tmp_path: Path) -> None: + data = _minimal_payload(features=None) + with pytest.raises(CombatantLoadError, match="features must be a list"): + load_combatant(_write(tmp_path, data)) + + def test_non_string_entry_raises(self, tmp_path: Path) -> None: + data = _minimal_payload(features=["Toughness", 42]) + with pytest.raises(CombatantLoadError, match="non-empty feat name strings"): + load_combatant(_write(tmp_path, data)) + class TestSpells: def test_spells_passed_through(self, tmp_path: Path) -> None: