From 6233fff71e6ff55bff4b05df85ce5a4430f4f41a Mon Sep 17 00:00:00 2001 From: Eva Date: Sat, 20 Jun 2026 02:42:43 +0700 Subject: [PATCH] fix(engine): Champion expanded crit-range + War Caster concentration advantage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two additive SRD-fidelity fixes, root-caused against source. Both new fields default to today's behavior; old snapshots round-trip under _StrictModel extra="forbid"; the engine is the sole writer, rolls the dice, and tells the DM. (a) Champion EXPANDED CRIT RANGE (crit on 19-20 / 18-20) Crit derivation read only a natural 20, so the Champion's Improved Critical / Superior Critical features granted nothing and combat.crit_source()'s "expanded_crit_range" branch was dead (unreachable — crit could only ever be a nat-20). * dice.roll() gains crit_min: int = 20; crit = (natural >= crit_min). Default 20 == today's behavior; every existing caller is unaffected (param optional). * Character.crit_min: int = 20 (additive). * _apply_srd_class_defaults derives crit_min from the granted features: Improved Critical -> 19, Superior Critical -> 18 (Superior implies Improved; takes the lower floor). A non-Champion stays 20. * attack() threads crit_min=attacker.crit_min into the d20 roll, so a Champion's nat-19 flags atk.crit and crit_source() resolves "expanded_crit_range". (b) War Caster CONCENTRATION-SAVE ADVANTAGE Both concentration-save sites rolled a flat 1d20+CON, so the War Caster feat ("advantage on CON saves to maintain concentration when you take damage") granted nothing. * _has_war_caster(ch) reads ch.feats at call time (case-insensitive). * _auto_concentration_save (on-damage path) and the concentration_save tool now pass advantage=_has_war_caster(ch). No feat == flat d20 (today's behavior); advantage is scoped to concentration saves only. (c) ranged-in-melee disadvantage: intentionally untouched (theater-of-mind, correctly handled by the existing nudge + behavioral-gate WARN). Tests (single-process, no xdist): * test_dice.py: crit_min defaults to 20; crit_min expands the range (19/18). * test_combat.py: Champion crits on a forced nat-19 with crit_source "expanded_crit_range"; a plain fighter's nat-19 is an ordinary hit; Superior Critical at L15 lowers crit_min to 18. * test_effect_riders.py: a War Caster rolls the concentration save (both the manual tool and the auto-on-damage path) with advantage; a non-War-Caster rolls flat. Pre-existing dice-stub test doubles widened to accept the optional crit_min kwarg (signature-only; no behavior change). Full engine suite: 3042 passed. --- servers/engine/dice.py | 9 +- servers/engine/models.py | 6 ++ servers/engine/server.py | 40 ++++++++- servers/engine/tests/test_combat.py | 86 +++++++++++++++++-- servers/engine/tests/test_dice.py | 21 +++++ servers/engine/tests/test_effect_durations.py | 2 +- servers/engine/tests/test_effect_riders.py | 65 ++++++++++++++ servers/engine/tests/test_spellcasting.py | 2 +- 8 files changed, 217 insertions(+), 14 deletions(-) diff --git a/servers/engine/dice.py b/servers/engine/dice.py index 9026c4f9..bbf8788b 100644 --- a/servers/engine/dice.py +++ b/servers/engine/dice.py @@ -42,9 +42,14 @@ def roll( advantage: bool = False, disadvantage: bool = False, seed: int | None = None, + crit_min: int = 20, ) -> DiceRoll: """Roll a dice expression. Advantage/disadvantage apply to a single d20 term - and cancel each other out (5e rule) if both are set.""" + and cancel each other out (5e rule) if both are set. + + crit_min is the lowest natural d20 face that counts as a critical hit + (default 20 = standard 5e). Champion fighters lower it via Improved/Superior + Critical (19/18); callers thread the attacker's crit_min through here.""" if advantage and disadvantage: advantage = disadvantage = False @@ -113,7 +118,7 @@ def roll( modifier += sign * val parts.append(term) - crit = bool(is_d20 and natural == 20) + crit = bool(is_d20 and natural is not None and natural >= crit_min) fumble = bool(is_d20 and natural == 1) detail = " ".join(parts) + f" = {total}" if is_d20 and advantage: diff --git a/servers/engine/models.py b/servers/engine/models.py index fe6f2be9..46a4f3ac 100644 --- a/servers/engine/models.py +++ b/servers/engine/models.py @@ -908,6 +908,12 @@ class Character(_StrictModel): # identical, non-martial classes are untouched, and the viewer ignores an absent key. fighting_style: str = "" extra_attacks: int = 0 # extra attacks per Attack action (Extra Attack feature) + # Lowest natural d20 face that scores a CRITICAL HIT on this character's attack rolls. + # 20 == standard 5e (today's behavior). LOWERED by the Champion fighter's Improved + # Critical (19) and Superior Critical (18) features; set in _apply_srd_class_defaults + # from the features list and threaded into the attack d20 roll. ADDITIVE: 20 == today's + # behavior; old snapshots round-trip byte-identical. + crit_min: int = 20 sneak_attack_dice: str = "" # e.g. "3d6" (rogue Sneak Attack), "" if none # A defensive REACTION that adds this many points to AC against ONE melee attack that # would otherwise hit (the Parry reaction — Bandit Captain +2, fallen consular +4, etc., diff --git a/servers/engine/server.py b/servers/engine/server.py index 1228f802..af77c5b4 100644 --- a/servers/engine/server.py +++ b/servers/engine/server.py @@ -1476,6 +1476,17 @@ def _apply_srd_class_defaults(ch, class_name: str, level: int, set_base_ac: bool ch.extra_attacks = max(ch.extra_attacks, int(f["extra_attacks"])) if f.get("sneak_attack_dice"): ch.sneak_attack_dice = f["sneak_attack_dice"] + # EXPANDED CRIT RANGE — the Champion fighter's Improved Critical (L3, natural 19–20) + # and Superior Critical (L15, natural 18–20). Derived from the granted features so it + # tracks level: Superior implies Improved, so it takes the lower (better) floor. The + # attack d20 roll threads ch.crit_min so a Champion's nat-19 actually crits and the + # crit_source() "expanded_crit_range" branch is reachable (it was dead — crit could only + # ever be a nat-20). ADDITIVE: a non-Champion keeps crit_min 20 (today's behavior). + feat_names_lower = {f.lower() for f in ch.features} + if "superior critical" in feat_names_lower: + ch.crit_min = min(ch.crit_min, 18) + elif "improved critical" in feat_names_lower: + ch.crit_min = min(ch.crit_min, 19) # Grant the class's default skill proficiencies if none were chosen, so skill # checks (incl. social_check) include the proficiency bonus instead of the DM # inventing a modifier on an empty sheet. The caller can pass an explicit @@ -4896,6 +4907,17 @@ def _roll_effect_bonus_dice(ch: Character, field: str) -> tuple[int, list[dict]] return total, rolls +def _has_war_caster(ch: Character) -> bool: + """True if ``ch`` has the War Caster feat — which grants ADVANTAGE on a CON saving + throw to maintain concentration when the character takes damage (SRD 5.2). Read off + the feats list at call time (the engine's sole source for chosen feats); case- + insensitive so "war caster" / "War Caster" both match. ADDITIVE: no War Caster == a + flat 1d20 save (today's behavior). The advantage applies ONLY to concentration saves + here (War Caster's other riders — opportunity-attack spells, somatic-while-handed — are + not yet modeled), so a non-concentration save is untouched.""" + return any((f or "").strip().lower() == "war caster" for f in (ch.feats or [])) + + def _auto_concentration_save(ch: Character, dc: int) -> dict | None: """F01-9 (audit 2026-06-11): when a concentrating creature TAKES damage, 5e checks concentration the instant the damage lands. The engine already computes the DC @@ -4909,7 +4931,10 @@ def _auto_concentration_save(ch: Character, dc: int) -> dict | None: None). combat.py stays dice-free; all dice are rolled here. Caller persists (sole-writer).""" if not dc or not getattr(ch, "concentration", None): return None - r = dice_mod.roll(f"1d20+{ch.saving_throw_bonus(Ability.CON)}") + # War Caster: advantage on the CON save to maintain concentration on taking damage. + r = dice_mod.roll( + f"1d20+{ch.saving_throw_bonus(Ability.CON)}", advantage=_has_war_caster(ch) + ) rider_bonus, rider_rolls = _roll_effect_bonus_dice(ch, "save_bonus_dice") total = r.total + rider_bonus maintained = total >= dc @@ -5143,7 +5168,13 @@ def attack( cadv, cdis = combat.attack_modifiers(attacker, target, is_ranged=is_ranged) adv = advantage or cadv dis = disadvantage or cdis - atk = dice_mod.roll(f"1d20+{attack_bonus}", advantage=adv, disadvantage=dis) + # crit_min threads the attacker's expanded crit range (Champion Improved/Superior + # Critical → 19/18; everyone else 20 = today's behavior) so a Champion's nat-19 + # actually flags atk.crit and crit_source() resolves "expanded_crit_range". + atk = dice_mod.roll( + f"1d20+{attack_bonus}", advantage=adv, disadvantage=dis, + crit_min=getattr(attacker, "crit_min", 20), + ) # NUMERIC RIDERS (SYN-06 / #780): fold the attacker's engine-tracked bonus dice # (Bless +1d4 / Bane -1d4) into the attack total — the engine ROLLS the rider it # advertises instead of tracking it as theater. Nat-20 auto-hit / nat-1 auto-miss @@ -5803,7 +5834,10 @@ def concentration_save(campaign_id: str, character_id: str, dc: int) -> dict: with campaign_lock(campaign_id): c = _require(campaign_id) ch = _char(c, character_id) - r = dice_mod.roll(f"1d20+{ch.saving_throw_bonus(Ability.CON)}") + # War Caster: advantage on the CON save to maintain concentration (SRD 5.2). + r = dice_mod.roll( + f"1d20+{ch.saving_throw_bonus(Ability.CON)}", advantage=_has_war_caster(ch) + ) # NUMERIC RIDERS (SYN-06 / #780): a concentration save is a saving throw — fold # the engine-tracked save bonus dice (Bless +1d4 / Bane -1d4) like saving_throw. rider_bonus, rider_rolls = _roll_effect_bonus_dice(ch, "save_bonus_dice") diff --git a/servers/engine/tests/test_combat.py b/servers/engine/tests/test_combat.py index c5efc134..fcca164c 100644 --- a/servers/engine/tests/test_combat.py +++ b/servers/engine/tests/test_combat.py @@ -22,7 +22,7 @@ def _ds(total: int, natural: int) -> DiceRoll: ) -def _fixed_roll(expression: str, advantage: bool = False, disadvantage: bool = False, seed: int | None = None) -> DiceRoll: +def _fixed_roll(expression: str, advantage: bool = False, disadvantage: bool = False, seed: int | None = None, crit_min: int = 20) -> DiceRoll: if expression.startswith("1d20"): return DiceRoll( expression=expression, @@ -1452,7 +1452,7 @@ def _crit_then_fixed(component_total: int = 5): seen: list[str] = [] - def _roll(expression, advantage=False, disadvantage=False, seed=None): + def _roll(expression, advantage=False, disadvantage=False, seed=None, crit_min=20): if expression.startswith("1d20"): return DiceRoll(expression=expression, total=30, rolls=[20], is_d20=True, natural=20, crit=True) seen.append(expression) @@ -1499,7 +1499,7 @@ def test_attack_multi_component_per_type_resistance_halves_only_matching(tmp_pat import server from dice import DiceRoll - def roll(expression, advantage=False, disadvantage=False, seed=None): + def roll(expression, advantage=False, disadvantage=False, seed=None, crit_min=20): if expression.startswith("1d20"): return DiceRoll(expression=expression, total=19, rolls=[15], is_d20=True, natural=15, crit=False) return DiceRoll(expression=expression, total=5, rolls=[5], detail="x") @@ -1532,7 +1532,7 @@ def test_attack_single_damage_dice_unchanged(tmp_path, monkeypatch): import server from dice import DiceRoll - def roll(expression, advantage=False, disadvantage=False, seed=None): + def roll(expression, advantage=False, disadvantage=False, seed=None, crit_min=20): if expression.startswith("1d20"): return DiceRoll(expression=expression, total=19, rolls=[15], is_d20=True, natural=15, crit=False) return DiceRoll(expression=expression, total=7, rolls=[7], detail="1d6+2=7") @@ -2125,7 +2125,7 @@ def _rigged_roller(d20_total: int, fixed: dict[str, int]): text, defaulting to 0 so an unexpected expr is loud rather than silently '6'.""" from dice import DiceRoll - def _roll(expr, advantage=False, disadvantage=False, seed=None): + def _roll(expr, advantage=False, disadvantage=False, seed=None, crit_min=20): e = expr.replace(" ", "").lower() if e.startswith("1d20"): return DiceRoll( @@ -2295,6 +2295,78 @@ def test_crit_source_attributes_the_right_reason(): assert combat.crit_source(True, 20, True, paralyzed) == "nat_20" +def _nat19_honoring_crit_min(natural: int = 19): + """A deterministic dice stub for server.attack: every 1d20 rolls `natural`, and — like + the real roller — flags crit iff natural >= the crit_min the caller threads in. Proves + server.attack passes the attacker's expanded crit range through to the roll.""" + def _roll(expression, advantage=False, disadvantage=False, seed=None, crit_min=20): + if expression.startswith("1d20"): + total = natural + 5 + return DiceRoll( + expression=expression, total=total, rolls=[natural], modifier=5, + detail=f"{expression}[{natural}] = {total}", is_d20=True, natural=natural, + crit=(natural >= crit_min), fumble=(natural == 1), + ) + return DiceRoll(expression=expression, total=6, rolls=[3], + detail=f"{expression}[3] = 6") + return _roll + + +def _attack_nat19_in_fresh_combat(server, label, *, subclass=None): + """Seat a level-3 fighter (optionally a Champion) alone vs a soft dummy and resolve ONE + attack on a forced natural 19. Order-independent: a single attack right after + start_combat resolves whether or not it's the attacker's turn (off-turn it's a + reaction), and no next_turn is called — so initiative randomness can't perturb it.""" + cid = server.create_campaign(label)["id"] + fighter = server.create_character( + cid, "Fighter", kind="player", class_name="fighter", subclass=subclass, + level=3, max_hp=28, armor_class=16, apply_srd_defaults=True, + )["id"] + dummy = server.create_character(cid, "Dummy", kind="monster", max_hp=200, armor_class=10)["id"] + server.start_combat(cid, [fighter, dummy]) + server.dice_mod.roll = _nat19_honoring_crit_min(19) + return cid, fighter, server.attack(cid, fighter, dummy, attack_bonus=5, + damage_dice="1d8+3", damage_type="slashing") + + +def test_champion_crits_on_natural_19_expanded_crit_range(tmp_path, monkeypatch): + """Fix (a): a Champion fighter with Improved Critical (crit_min lowered to 19) scores a + CRIT on a natural 19, and crit_source() attributes it to the expanded crit range — the + previously-dead branch. A plain fighter's natural 19 is an ordinary hit, not a crit.""" + monkeypatch.setenv("WORLDOS_STATE_DIR", str(tmp_path)) + import server + _orig_roll = server.dice_mod.roll + try: + cid_c, champ, champ_res = _attack_nat19_in_fresh_combat( + server, "Champion Crit", subclass="Champion") + assert server.get_character(cid_c, champ)["crit_min"] == 19 # SRD derivation + assert champ_res["crit"] is True + assert champ_res["crit_source"] == "expanded_crit_range" + + cid_p, plain, plain_res = _attack_nat19_in_fresh_combat(server, "Plain Fighter") + assert server.get_character(cid_p, plain)["crit_min"] == 20 # no expanded range + assert plain_res["hit"] is True # 19+5=24 vs AC 10 — a hit + assert plain_res["crit"] is False + assert plain_res["crit_source"] == "" + finally: + server.dice_mod.roll = _orig_roll + + +def test_superior_critical_lowers_crit_min_to_18(tmp_path, monkeypatch): + """Fix (a): a Champion at L15 has Superior Critical → crit_min 18 (crits on 18–20).""" + monkeypatch.setenv("WORLDOS_STATE_DIR", str(tmp_path)) + import server + + cid = server.create_campaign("Superior Crit")["id"] + champ = server.create_character( + cid, "Veteran", kind="player", class_name="fighter", subclass="Champion", + level=15, max_hp=120, armor_class=18, apply_srd_defaults=True, + ) + sheet = server.get_character(cid, champ["id"]) + assert "Superior Critical" in sheet["features"] + assert sheet["crit_min"] == 18 + + # --- #218: monster defensive reactions (Parry) --- def test_bestiary_parry_bonus_parses_the_ac_reaction(): import bestiary @@ -2315,7 +2387,7 @@ def test_bestiary_parry_bonus_parses_the_ac_reaction(): def _attack_roll_stub(d20_total: int, d20_natural: int = 14): """A dice_mod.roll stub: the 1d20 attack returns a controlled (total, natural); any other roll (damage) is small + deterministic so the attack resolves.""" - def _roll(expression, advantage=False, disadvantage=False, seed=None): + def _roll(expression, advantage=False, disadvantage=False, seed=None, crit_min=20): if expression.startswith("1d20"): return _ds(d20_total, d20_natural) return DiceRoll(expression=expression, total=4, rolls=[3], modifier=1, detail=f"{expression}[3] = 4") @@ -2475,7 +2547,7 @@ def _bm_roller(*, d20: int, crit: bool, fixed: dict[str, int]): seen: list[str] = [] - def _roll(expr, advantage=False, disadvantage=False, seed=None): + def _roll(expr, advantage=False, disadvantage=False, seed=None, crit_min=20): e = expr.replace(" ", "").lower() if e.startswith("1d20"): nat = 20 if crit else max(1, min(19, d20 - 5)) diff --git a/servers/engine/tests/test_dice.py b/servers/engine/tests/test_dice.py index 5f6f8ed4..72a844c1 100644 --- a/servers/engine/tests/test_dice.py +++ b/servers/engine/tests/test_dice.py @@ -64,6 +64,27 @@ def test_crit_and_fumble_flags(): assert saw_crit and saw_fumble +def test_crit_min_defaults_to_20(): + # ADDITIVE: omitting crit_min preserves today's behavior — only a natural 20 crits. + assert dice.roll("1d20", seed=5).natural == 20 and dice.roll("1d20", seed=5).crit + assert dice.roll("1d20", seed=6).natural == 19 and not dice.roll("1d20", seed=6).crit + + +def test_crit_min_expands_the_crit_range(): + # Champion Improved Critical (crit_min=19): a natural 19 crits, an 18 does not. + r19 = dice.roll("1d20+5", seed=6, crit_min=19) + assert r19.natural == 19 and r19.crit + r18 = dice.roll("1d20+5", seed=29, crit_min=19) + assert r18.natural == 18 and not r18.crit + # Superior Critical (crit_min=18): an 18 now crits too. + r18b = dice.roll("1d20+5", seed=29, crit_min=18) + assert r18b.natural == 18 and r18b.crit + r17 = dice.roll("1d20+5", seed=17, crit_min=18) + assert r17.natural == 17 and not r17.crit + # A natural 20 always crits regardless of crit_min. + assert dice.roll("1d20", seed=5, crit_min=18).crit + + def test_multi_dice_sum(): r = dice.roll("2d6+1d4+2", seed=10) assert 5 <= r.total <= 18 diff --git a/servers/engine/tests/test_effect_durations.py b/servers/engine/tests/test_effect_durations.py index 87e3b057..69f25fd0 100644 --- a/servers/engine/tests/test_effect_durations.py +++ b/servers/engine/tests/test_effect_durations.py @@ -160,7 +160,7 @@ def _d20_roll(natural: int): """A dice.roll stub that forces the d20 NATURAL face (so a spell attack is a deterministic hit/miss) while honoring the expression's flat modifier; non-d20 expressions (damage) roll a fixed mid value. Used via monkeypatch on the attack.""" - def _roll(expression: str, advantage: bool = False, disadvantage: bool = False, seed=None) -> DiceRoll: + def _roll(expression: str, advantage: bool = False, disadvantage: bool = False, seed=None, crit_min: int = 20) -> DiceRoll: if expression.startswith("1d20"): mod = 0 if "+" in expression: diff --git a/servers/engine/tests/test_effect_riders.py b/servers/engine/tests/test_effect_riders.py index 0537c0f5..100388ab 100644 --- a/servers/engine/tests/test_effect_riders.py +++ b/servers/engine/tests/test_effect_riders.py @@ -181,6 +181,71 @@ def test_blessed_concentration_save_includes_d4(tmp_path, monkeypatch): assert out["bonus_dice"][0]["source"] == "Bless" +# --- War Caster: advantage on the concentration CON save ---------------------- + +def _spy_advantage(monkeypatch): + """Record the `advantage` kwarg passed to each 1d20 roll, then delegate to the real + roller. Returns the list the spy appends (expression, advantage) onto.""" + import store + _orig = server.dice_mod.roll + seen: list = [] + + def _spy(expression, advantage=False, disadvantage=False, seed=None, crit_min=20): + if expression.startswith("1d20"): + seen.append((expression, advantage)) + return _orig(expression, advantage=advantage, disadvantage=disadvantage, + seed=seed, crit_min=crit_min) + + monkeypatch.setattr(server.dice_mod, "roll", _spy) + return seen, store + + +def test_war_caster_rolls_concentration_save_with_advantage(tmp_path, monkeypatch): + """Fix (b): a War Caster taking damage rolls the concentration CON save WITH advantage + (the manual concentration_save tool path). A non-War-Caster rolls a flat d20.""" + monkeypatch.setenv("WORLDOS_STATE_DIR", str(tmp_path)) + cid = server.create_campaign("WarCaster")["id"] + wiz = server.create_character(cid, "Caster", kind="player", class_name="Wizard", + level=5, max_hp=30, apply_srd_defaults=True)["id"] + plain = server.create_character(cid, "Plain", kind="player", class_name="Wizard", + level=5, max_hp=30, apply_srd_defaults=True)["id"] + + seen, store = _spy_advantage(monkeypatch) + c = store.load_campaign(cid) + c.characters[wiz].feats = ["War Caster"] + c.characters[wiz].concentration = "Bless" + c.characters[plain].concentration = "Bless" + store.save_campaign(c) + + seen.clear() + server.concentration_save(cid, wiz, dc=10) + assert seen and all(adv is True for _, adv in seen), seen # War Caster -> advantage + + seen.clear() + server.concentration_save(cid, plain, dc=10) + assert seen and all(adv is False for _, adv in seen), seen # no feat -> flat d20 + + +def test_auto_concentration_save_grants_war_caster_advantage(tmp_path, monkeypatch): + """Fix (b): the on-damage auto concentration save (_auto_concentration_save, the path + apply_damage drives) also rolls with advantage for a War Caster.""" + monkeypatch.setenv("WORLDOS_STATE_DIR", str(tmp_path)) + cid = server.create_campaign("WarCasterAuto")["id"] + wiz = server.create_character(cid, "Caster", kind="player", class_name="Wizard", + level=5, max_hp=40, apply_srd_defaults=True)["id"] + + seen, store = _spy_advantage(monkeypatch) + c = store.load_campaign(cid) + c.characters[wiz].feats = ["War Caster"] + c.characters[wiz].concentration = "Bless" + store.save_campaign(c) + + seen.clear() + out = server._auto_concentration_save(c.characters[wiz], dc=12) + assert out is not None and out["rolled"] is True + assert seen and all(adv is True for _, adv in seen), seen + + # --- concentration break releases the linked children (BOTH paths) ------------ def test_drop_concentration_frees_linked_children(tmp_path, monkeypatch): diff --git a/servers/engine/tests/test_spellcasting.py b/servers/engine/tests/test_spellcasting.py index 698a5330..f8bfa81e 100644 --- a/servers/engine/tests/test_spellcasting.py +++ b/servers/engine/tests/test_spellcasting.py @@ -574,7 +574,7 @@ def _fixed_roll(d20_natural: int, dmg_total: int): """A dice stub: every 1d20 save rolls the given natural (+ the expression's flat mod); every other expression (the shared AoE damage) returns dmg_total. Deterministic per-target save outcomes + a single known damage figure.""" - def _roll(expression, advantage=False, disadvantage=False, seed=None): + def _roll(expression, advantage=False, disadvantage=False, seed=None, crit_min=20): if expression.startswith("1d20"): mod = 0 if "+" in expression: