From a8c7e37f8ede6d3ef358d6362d6cc54951ea3ead Mon Sep 17 00:00:00 2001 From: ctan Date: Sun, 20 Sep 2026 17:13:09 +0200 Subject: [PATCH] fix(loaders): resource pools validated consistently before grant merge _merge_resource_grants silently dropped non-int pool values when features were present (a "5" pool vanished, leaving only the feat grant) while Pydantic coerced the same value when features were absent. _validated_resources now runs the JSON resources through TypeAdapter(dict[str, int]) -- the same lax coercion as Combatant.resources -- so both paths coerce-or-fail identically, and grants are added to the coerced ints. --- src/pf1e_simulator/loaders/combatant.py | 93 ++++++++++++++++--------- tests/test_combatant_loader.py | 40 +++++++++++ 2 files changed, 100 insertions(+), 33 deletions(-) diff --git a/src/pf1e_simulator/loaders/combatant.py b/src/pf1e_simulator/loaders/combatant.py index 4b013b3..b0e0337 100644 --- a/src/pf1e_simulator/loaders/combatant.py +++ b/src/pf1e_simulator/loaders/combatant.py @@ -226,25 +226,38 @@ def _resolve_features( return specs +_RESOURCE_POOLS_ADAPTER: TypeAdapter[dict[str, int]] = TypeAdapter(dict[str, int]) + + +def _validated_resources(data: dict[str, Json], path: Path) -> dict[str, int] | None: + """Coerce the JSON ``resources`` pools with the model's own lax rules. + + The merge below used to filter out non-int values (a string "5" pool + silently vanished when features were present, but Pydantic coerced it + when they were not). Validating once here with the same lax rules as + ``Combatant.resources`` makes both paths agree: coerce what the model + would coerce, fail with CombatantLoadError on what it would reject. + """ + if "resources" not in data: + return None + try: + return _RESOURCE_POOLS_ADAPTER.validate_python(data["resources"]) + except ValidationError as exc: + msg = f"resources must be an object mapping pool names to integers: {exc}" + raise CombatantLoadError(path, msg) from exc + + def _merge_resource_grants( - raw_resources: Json, specs: list[AbilitySpec] + raw_resources: dict[str, int] | None, specs: list[AbilitySpec] ) -> dict[str, int]: - """Merge the JSON resource pool with per-day grants from resolved features. + """Merge the validated JSON resource pool with per-day grants from features. Traits like Exalted of the Society ("You may channel energy one additional time per day") store their benefit in ``AbilitySpec.resource_grants`` because the engine keeps resource pools (``Combatant.resources`` -> ``CombatantState.resources_left``), not stat modifiers, for per-day uses. """ - merged: dict[str, int] = {} - if isinstance(raw_resources, dict): - merged.update( - { - key: value - for key, value in raw_resources.items() - if isinstance(value, int) and not isinstance(value, bool) - } - ) + merged: dict[str, int] = dict(raw_resources) if raw_resources is not None else {} for spec in specs: for resource_name, amount in spec.resource_grants: merged[resource_name] = merged.get(resource_name, 0) + amount @@ -279,6 +292,41 @@ def _auto_attack_ids(data: dict[str, Json], monster_id: str) -> None: attack["id"] = f"{monster_id}:{index}" +def _prepare_payload(data: dict[str, Json], path: Path) -> dict[str, object]: + """Validate/convert the loader-owned keys before Pydantic validation. + + ``features`` (list of feat name strings) is resolved to ``AbilitySpec`` + objects; ``spell_slots`` string keys are converted to ints; ``level`` + and ``resources`` are coerced with the same lax rules the model uses. + A present but non-list ``features`` (e.g. a bare string) used to be + popped and dropped silently, so the combatant lost every feat; it is + now a data error. + """ + level = _parse_level(data.get("level", 1), path) + resources = _validated_resources(data, path) + 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 resources is not None: + validate_payload["resources"] = resources + if isinstance(raw_features, list): + specs = _resolve_features(raw_features, level, path) + validate_payload["features"] = specs + granted = _merge_resource_grants(resources, specs) + if granted: + validate_payload["resources"] = granted + + # spells stays as list[str] — Pydantic validates it directly + + raw_slots = data.pop("spell_slots", None) + if isinstance(raw_slots, dict): + validate_payload["spell_slots"] = _parse_spell_slot_keys(raw_slots, path) + return validate_payload + + def load_combatant(path: Path) -> Combatant: """Load a combatant JSON file (PC or monster) into a ``Combatant``. @@ -317,28 +365,7 @@ def load_combatant(path: Path) -> Combatant: _auto_attack_ids(data, monster_id) - # 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. - level = _parse_level(data.get("level", 1), path) - 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, path) - validate_payload["features"] = specs - granted = _merge_resource_grants(data.get("resources"), specs) - if granted: - validate_payload["resources"] = granted - - # spells stays as list[str] — Pydantic validates it directly - - raw_slots = data.pop("spell_slots", None) - if isinstance(raw_slots, dict): - validate_payload["spell_slots"] = _parse_spell_slot_keys(raw_slots, path) + validate_payload = _prepare_payload(data, path) try: return Combatant.model_validate(validate_payload) diff --git a/tests/test_combatant_loader.py b/tests/test_combatant_loader.py index d71af16..e166f81 100644 --- a/tests/test_combatant_loader.py +++ b/tests/test_combatant_loader.py @@ -221,6 +221,46 @@ class TestSpells: assert c.spells == [] +class TestResourcePools: + def test_string_pool_coerced_without_features(self, tmp_path: Path) -> None: + data = _minimal_payload(resources={"channel_energy": "5"}) + c = load_combatant(_write(tmp_path, data)) + assert c.resources == {"channel_energy": 5} + + def test_string_pool_coerced_with_granting_feature(self, tmp_path: Path) -> None: + data = _minimal_payload( + resources={"channel_energy": "5"}, + features=["Exalted of the Society (cleric, pathfinder society)"], + ) + c = load_combatant(_write(tmp_path, data)) + assert c.resources == {"channel_energy": 6} + + def test_non_numeric_pool_value_raises_with_feature(self, tmp_path: Path) -> None: + data = _minimal_payload( + resources={"channel_energy": "lots"}, features=["Iron Will"] + ) + with pytest.raises(CombatantLoadError, match="resources must be an object"): + load_combatant(_write(tmp_path, data)) + + def test_non_numeric_pool_value_raises_without_feature(self, tmp_path: Path) -> None: + data = _minimal_payload(resources={"channel_energy": "lots"}) + with pytest.raises(CombatantLoadError, match="resources must be an object"): + load_combatant(_write(tmp_path, data)) + + def test_null_resources_raises(self, tmp_path: Path) -> None: + data = _minimal_payload(resources=None) + with pytest.raises(CombatantLoadError, match="resources must be an object"): + load_combatant(_write(tmp_path, data)) + + def test_grant_added_to_int_pool(self, tmp_path: Path) -> None: + data = _minimal_payload( + resources={"channel_energy": 2}, + features=["Exalted of the Society (cleric, pathfinder society)"], + ) + c = load_combatant(_write(tmp_path, data)) + assert c.resources == {"channel_energy": 3} + + class TestSpellSlots: def test_numeric_string_keys_converted(self, tmp_path: Path) -> None: data = _minimal_payload(spell_slots={"1": 4, "2": 3})