ctan/pf1e-simulator
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(loaders): resource pools validated consistently before grant merge

Browse Source 
_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.
This commit is contained in:
Thien An
2026-09-20 17:13:09 +02:00
parent 9adc87c9e1
commit a8c7e37f8e
2 changed files with 100 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/pf1e_simulator/loaders/combatant.py
+60 -33
View File
@@ -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)
tests/test_combatant_loader.py
+40
View File
@@ -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})