Fix the magic shield crash and report a melee with nothing left to strike

CHANGELOG.md

@@ -21,7 +21,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
     - [`MemberView.encounter_rate_feet`][osrlib.crawl.views.MemberView.encounter_rate_feet] gives how far a member can move in a round, after encumbrance and *haste*.
     - [`EncounterGroupView.lit`][osrlib.crawl.views.EncounterGroupView.lit] says whether a light a member carries reaches the group, and [`EncounterGroupView.seen_by`][osrlib.crawl.views.EncounterGroupView.seen_by] lists the members who can see it. The session refuses nothing because of light or sight, so use these fields to decide what your game offers and how it describes the fight.
 - The new event [`MemberMovedEvent`][osrlib.crawl.events.MemberMovedEvent] (`battle.member.moved`) reports each member who moves by themselves, closing on a group or falling back alone (#209). Its fields are `character_id`, `move`, `advance_feet`, `group_id`, and `distance_feet`. It reports the member's place right after the move, before the party line moves back, so use it to narrate the move and read the player view after the round to draw where everyone stands.
-- [`DeclarationFizzledEvent`][osrlib.core.events.DeclarationFizzledEvent] has a new code, `battle.melee.fizzled`, and a new optional field, `group_id` (#209). The session reports it when a declared melee attack can't resolve because of where the member stands when the party's melee step comes, for example when the monsters win initiative and run before the member's close. Its `reason` is `combat.attack.out_of_reach` or `battle.declaration.not_in_front_rank`, so tell the player why the member didn't swing. The field defaults to `None`.
+- [`DeclarationFizzledEvent`][osrlib.core.events.DeclarationFizzledEvent] has a new code, `battle.melee.fizzled`, and a new optional field, `group_id` (#209). The session reports it when a declared melee attack can't resolve when the party's melee step comes, for example when the monsters win initiative and run before the member's close. Its `reason` is `combat.attack.out_of_reach` or `battle.declaration.not_in_front_rank`, so tell the player why the member didn't swing. The field defaults to `None`.
+    - The reason is `battle.declaration.no_target` when the group has no living monster the member can see left to strike, for example because the party's missiles or spells killed it earlier in the round. The session checks this before it checks where the member stands, so the member gets it whether or not they closed. A slow attack gets the same check at the end of the round.
+    - The reason is `combat.attack.out_of_reach` when the group ran out of the battle before the attack came up, even for a member who stood within 5 feet of it when the round was accepted.
 - Two new refusal codes for a move set on an action that can't take one (#209). [`ResolveBattleRound`][osrlib.crawl.commands.ResolveBattleRound] refuses a `cast` that sets any `move` with `battle.declaration.cast_cannot_move`, because a caster can't move in the round they cast. It refuses a `turn_undead`, a `use_item`, or a `hold` that sets any `move`, and an `attack` with a `fighting_withdrawal` or a `retreat`, with `battle.declaration.move_not_combinable`. Each has the member's id as its `character` param. An attack that closes, by a member who can't move, gets the existing `battle.declaration.cannot_move`.
 
 ### Changed
@@ -43,6 +45,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
     - The replay plays out differently when the log contains a `close`, an `attack` that set `move="close"`, which 2.2.0 ignored, a fighting withdrawal or a retreat that one member made alone, or a monster group that closed on the party. A battle plays as before when no member moves and no group closes.
 - [`RemoveMember`][osrlib.crawl.commands.RemoveMember] without `give_to` now refuses to remove a member who carries an item the adventure depends on, so the party can't lose a quest item by dropping the member who held it. The adventure depends on an item when a gate on a door or transition, a trigger that can still fire, or any clause of a quest that isn't completed names its catalog id. The session refuses with `party.remove.carries_quest_item`, and the rejection's `character` and `item` params give the member and the item. Name a member in `give_to` and the item moves to them, so the removal goes through.
 
+### Fixed
+
+- An attack that ignores the defender's shield no longer raises `AttributeError` when the shield is magic. The bug was present in 2.2.0. Before, an attack on a retreating member who carried a magic shield, or an attack from behind on such a member, crashed the round. Now the attack is made against the armour class the defender would have without the shield, so a magic shield loses its enchantment along with the +1 every shield gives. A cursed shield that makes armour class worse still counts, because the rule takes away a shield's bonus and not its penalty. [`AttackContext.behind_target`][osrlib.core.combat.AttackContext.behind_target] and [`AttackContext.defender_retreating`][osrlib.core.combat.AttackContext.defender_retreating] describe the rule.
+
 ## [2.2.0] - 2026-10-30
 
 ### Added

docs/adaptations.md

@@ -232,6 +232,10 @@ Damage resolves in a fixed order:
 
 Halving for a successful save-for-half also rounds down. A back-stab adds +4 to hit and doubles damage, both from the thief tag's params, when the caller says the target is unaware and attacked from behind. Locked by `test_combat.py::TestDamagePipeline`.
 
+### An ignored shield loses its bonus and keeps its penalty
+
+The Other Combat Issues page says of attacking from behind, "AC bonuses from shields are negated", and the Combat page says a retreating combatant's opponent "ignores any AC bonus due to the combatant's shield". When `AttackContext` sets `behind_target` or `defender_retreating`, the attack is made against the armour class the defender would have without the shield. For a magic shield, that takes away the enchantment along with the +1 every shield gives. A cursed shield that makes armour class worse, like the cursed shield -2 or the cursed shield that sets AC 9 [10], gives no bonus to negate, so it still counts. That's osrlib's reading. Locked by `test_ignored_shield.py`.
+
 ### The WIS save modifier applies to magical non-breath saves only
 
 The WIS modifier applies to saving throws against magic, except saves against breath attacks. That's how osrlib reads "does not normally include saves against breath attacks". Any further referee discretion is a modifier the caller supplies. Locked by `test_combat.py::TestSavingThrows`.
@@ -1108,9 +1112,10 @@ The session judges a melee attack before the round starts, but the round can cha
 - That happens when a member the round expected to step back can no longer move and still stands in the place. It also happens when the monsters win initiative and the group runs before the member's close.
 - An attack that closes from beyond 5' to within 5' stays a melee attack wherever the member ends up. When the close falls short, the attack fizzles instead of turning a dagger's stab into a throw the player never declared.
 - A slow melee attack faces the same check again when it resolves at the end of the round.
-- Against a group that runs, what decides it is where the member stood when the round was accepted: a member who already stood within 5' of the group still strikes it, wherever it has run since, and a member who was closing on it fizzles. The SRD gives the opponent its attack against a retreating combatant, and osrlib reads only a member already within 5' as that opponent.
+- Against a group that runs, what decides it is where the member stood when the round was accepted: a member who already stood within 5' of the group still strikes it, wherever it has run since, and a member who was closing on it fizzles. A group that runs out of the battle altogether leaves nothing to strike, so even a member who stood within 5' of it fizzles with `combat.attack.out_of_reach`. The SRD gives the opponent its attack against a retreating combatant, and osrlib reads only a member already within 5' as that opponent.
+- An attack whose group has no living monster the member can see left to strike fizzles too, with the reason `battle.declaration.no_target`. That's the code the session refuses such an attack with at the start of the round. It happens when the party's missiles or spells, or an earlier member's melee, kill every monster in the group. The session checks for a target before it checks the member's position, as it does at the start of the round, so the member gets `no_target` whether or not they closed, and wherever they stand.
 
-Locked by `test_move_and_attack.py::TestTheMeleeStep` and `TestTheMeleeFizzleEvent`.
+Locked by `test_move_and_attack.py::TestTheMeleeStep` and `TestTheMeleeFizzleEvent`, and by `test_melee_no_target.py`.
 
 #### Light and sight are reported in the view only
 

src/osrlib/core/character.py

@@ -492,7 +492,7 @@ def _armour_parts(self, *, ascending: bool) -> tuple[int, int]:
         the +1 an ordinary shield gives. A worn item with an armour class bonus that is always
         active, like a ring of protection, adds to the bonus too.
         """
-        from osrlib.core.items import ArmourTemplate, MagicItemInstance, magic_item_template
+        from osrlib.core.items import ArmourTemplate, MagicItemInstance, _shield_ac_parts, magic_item_template
         from osrlib.data import load_equipment
 
         base = 10 if ascending else 9
@@ -509,21 +509,10 @@ def _armour_parts(self, *, ascending: bool) -> tuple[int, int]:
         elif worn is not None and isinstance(worn.template, ArmourTemplate) and worn.template.ac is not None:
             printed_ascending = worn.template.ac_ascending
             base = printed_ascending if ascending and printed_ascending is not None else worn.template.ac
-        bonus = 0
-        shield = self.inventory.shield
-        if isinstance(shield, MagicItemInstance):
-            template = magic_item_template(shield)
-            if template.ac_set is not None and template.ac_set_ascending is not None:
-                # The cursed shield's AC 9 [10] sets the wearer's base outright.
-                base = template.ac_set_ascending if ascending else template.ac_set
-            else:
-                mundane_shield = load_equipment().get("shield")
-                mundane_bonus = (getattr(mundane_shield, "ac_bonus", None) or 0) if mundane_shield else 0
-                bonus += mundane_bonus + template.ac_bonus
-        elif (
-            shield is not None and isinstance(shield.template, ArmourTemplate) and shield.template.ac_bonus is not None
-        ):
-            bonus += shield.template.ac_bonus
+        # The cursed shield's AC 9 [10] sets the wearer's base outright.
+        shield_base, bonus = _shield_ac_parts(self.inventory.shield, ascending=ascending)
+        if shield_base is not None:
+            base = shield_base
         for ring in self.inventory.rings:
             template = magic_item_template(ring)
             if template.always_active:

src/osrlib/core/combat.py

@@ -172,6 +172,7 @@
     MissileRanges,
     WeaponQuality,
     WeaponTemplate,
+    _shield_ac_parts,
     equipped_item_modifiers,
     magic_item_template,
 )
@@ -379,8 +380,12 @@ class AttackContext(BaseModel):
     behind_target: bool = False
     """Whether the attacker strikes from behind.
 
-    The defender's shield doesn't count. With `target_unaware`, a thief attacker gets the
-    back-stab bonus to hit and to damage, and the attack's
+    The defender's shield doesn't count, as the OSE SRD's "AC bonuses from shields are
+    negated" says. The attack is made against the armour class the defender would have
+    without the shield, so a magic shield loses its enchantment along with the +1 that every
+    shield gives. A cursed shield that makes the defender's armour class worse still counts,
+    because the rule takes away a shield's bonus and not its penalty. With `target_unaware`,
+    a thief attacker gets the back-stab bonus to hit and to damage, and the attack's
     [`AttackRolledEvent`][osrlib.core.events.AttackRolledEvent] has `back_stab` set.
     """
 
@@ -395,7 +400,9 @@ class AttackContext(BaseModel):
     defender_retreating: bool = False
     """Whether the defender is retreating.
 
-    The attacker gets +2 to hit, and the defender's shield doesn't count. A
+    The attacker gets +2 to hit, and the defender's shield doesn't count, as it doesn't with
+    `behind_target`. The attack is made against the armour class the defender would have
+    without the shield, a magic shield's enchantment included, but never a better one. A
     [`GameSession`][osrlib.crawl.session.GameSession] sets it for you in two cases:
 
     - On an attack against a monster group that's fleeing.
@@ -1356,12 +1363,12 @@ def _defender_descending_ac(defender: Any, context: AttackContext, *, missile: b
     ac = getattr(defender, "armour_class", None)
     if ac is None:
         return None
-    # Shield AC is ignored against a retreating defender and attacks from behind.
+    # A retreating defender, or one attacked from behind, loses the shield's bonus, a magic
+    # shield's enchantment included. A cursed shield's penalty has no bonus to lose, so it stays.
     if context.defender_retreating or context.behind_target:
         inventory = getattr(defender, "inventory", None)
-        shield = getattr(inventory, "shield", None) if inventory is not None else None
-        if shield is not None and shield.template.ac_bonus is not None:
-            ac += shield.template.ac_bonus
+        _, shield_bonus = _shield_ac_parts(getattr(inventory, "shield", None), ascending=False)
+        ac += max(shield_bonus, 0)
     if context.attacker_large:
         params = _class_ability_params(defender, "defensive_bonus")
         if params is not None:
@@ -1412,7 +1419,8 @@ def attack_roll(
     - The context's `situational_modifier`.
 
     The function adjusts the defender's armour class too. The defender's shield doesn't
-    count against an attack from behind, or while the defender retreats. The context's
+    count against an attack from behind, or while the defender retreats, and a magic
+    shield's enchantment doesn't count either. The context's
     `defender_ally_ac_bonus`, an armour class bonus that an ally gives the defender, does
     count.
 

src/osrlib/core/events.py

@@ -1150,13 +1150,15 @@ class DeclarationFizzledEvent(Event):
     caused the failure. For example, the group a staff of striking was aimed at moved out of melee reach, or the
     group a wand was aimed at left the battle.
 
-    A melee attack fizzles because of where the member stands when the party's melee step comes. The session judges
-    a melee attack before the round starts, as if every move the round declares had happened. When the attack comes
-    up, it resolves only if the member stands in the group's front rank within 5' of it. Something earlier in the
-    round can change that. For example, the monsters win initiative and the group runs before the member's `close`,
-    or a member the session expected to step back can no longer move. Then the attack fizzles with the code
-    `battle.melee.fizzled`. The one exception is a group that has started to flee: a member who stood within 5' of
-    it when the round was accepted still gets the attack.
+    A melee attack fizzles when the party's melee step comes and the attack has nothing left to strike, or the
+    member stands in the wrong place. The session judges a melee attack before the round starts, as if every move the
+    round declares had happened. When the attack comes up, it resolves only if its group still has a living monster
+    the member can see to strike, and the member stands in the group's front rank within 5' of it. Something earlier
+    in the round can change either. For example, the party's missiles or spells kill every monster in the group, the
+    monsters win initiative and the group runs before the member's `close`, or a member the session expected to step
+    back can no longer move. Then the attack fizzles with the code `battle.melee.fizzled`. A group that has started to
+    flee is an exception to the position check: a member who stood within 5' of it when the round was accepted still
+    gets the attack, if the group has a monster left to strike and hasn't run out of the battle.
 
     The event's `actor_id` is the member who declared the action, `item_id` is the device, `group_id` is the group a
     melee attack was aimed at, and `reason` is the rejection code of the check that failed.
@@ -1203,12 +1205,16 @@ class DeclarationFizzledEvent(Event):
     cause. Some codes you may see:
 
     - `combat.attack.out_of_reach`: the group a staff of striking was aimed at moved out of reach, or the member
-      who declared a melee attack is farther than 5' from its group when the attack comes up.
+      who declared a melee attack is farther than 5' from its group when the attack comes up. A melee attack gets
+      it too when its group ran out of the battle, wherever the member stood.
     - `battle.declaration.not_in_front_rank`: the member who declared a melee attack is within 5' of its group
       but not in its front rank when the attack comes up. A melee attack gets this code only when the member is
       within 5', and `combat.attack.out_of_reach` otherwise.
     - `battle.declaration.unknown_group`: the group a wand was aimed at left the battle.
-    - `battle.declaration.no_target`: no living monster is left in the battle for a turning.
+    - `battle.declaration.no_target`: no living monster is left in the battle for a turning, or no living monster
+      the member can see is left in the group a melee attack was aimed at, for example because the party's missiles
+      or spells killed them earlier in the round. A melee attack gets this code wherever the member stands, whether
+      or not the member closed, because the session checks for a target before it checks the member's position.
 
     Match on it as you match on the code of a [`Rejection`][osrlib.core.validation.Rejection].
     [The rejection code reference][rejection-codes] explains each code.

src/osrlib/core/items.py

@@ -2254,6 +2254,28 @@ def magic_item_template(instance: MagicItemInstance) -> MagicItemTemplate:
     return load_magic_items().get(instance.template_id)
 
 
+def _shield_ac_parts(shield: AnyInstance | None, *, ascending: bool) -> tuple[int | None, int]:
+    """Return the armour class a shield sets, or None, and the bonus the shield adds to armour class.
+
+    A mundane shield adds its +1. A magic shield adds its enchantment on top of the +1 an ordinary
+    shield gives, so a cursed shield's negative enchantment can make the sum a penalty. The cursed
+    shield that prints AC 9 [10] sets the wearer's armour class outright and adds nothing.
+    `Character._armour_parts` and the attack roll both read a shield here, so they agree on what it
+    gives.
+    """
+    from osrlib.data import load_equipment
+
+    if isinstance(shield, MagicItemInstance):
+        template = magic_item_template(shield)
+        if template.ac_set is not None and template.ac_set_ascending is not None:
+            return (template.ac_set_ascending if ascending else template.ac_set), 0
+        mundane = load_equipment().get("shield")
+        return None, (getattr(mundane, "ac_bonus", None) or 0) + template.ac_bonus
+    if shield is not None and isinstance(shield.template, ArmourTemplate) and shield.template.ac_bonus is not None:
+        return None, shield.template.ac_bonus
+    return None, 0
+
+
 def equipped_item_modifiers(inventory: Inventory) -> list[ModifierSpec]:
     """Return the stat modifiers a character's equipped magic items grant.