fix(combat): turn-anchor Guiding Bolt advantage (SRD duration bug) + Battle Master maneuver cue#1033
Conversation
…aneuver cue
Two combat-fidelity fixes, both additive (engine is the sole writer; every new
field defaults to today's behavior; old snapshots round-trip; models stay
extra=forbid).
FIX A (engine bug, hot combat path) — Guiding Bolt's "the next attack roll
against the target before the END OF YOUR NEXT TURN has Advantage" was stored as
a fixed round counter (rounds_remaining=1). next_turn ticks round-scale effects
at the START of a new round (turn_index wraps to 0), so a marker materialized in
round 1 was expired at the very start of round 2 — BEFORE the next attacker
acted — losing the advantage SRD 5.2 owes it. The same-round path was tested and
fine; the CROSS-round case was the gap. Root cause: the window is TURN-ANCHORED
to the caster's next turn, not a fixed round count.
Fix (SRD-exact, mirrors the existing repeat_save exemption pattern):
- models.ActiveEffect: add `expires_end_of_turn_of: Optional[str] = None`,
holding the CASTER's character_id. None == not turn-anchored (every old
snapshot/effect unchanged).
- combat.tick_round_effects: exempt turn-anchored markers from the round-counter
tick (so the round-boundary tick can't expire them), exactly mirroring the
repeat_save `surviving.append(eff); continue` block.
- server.attack (rider materialization): when grants_advantage is True, set
expires_end_of_turn_of = the rider's source_id (the spellcaster — the
PendingOnHitRider already carries the caster id at cast_spell time, matched as
attack()'s attacker == caster; no new field needed). Bump the marker's
rounds_remaining by +1 so the CAST turn's own end does not expire it.
- server.next_turn: after advancing, for each turn-anchored marker whose caster
is `previous` (the combatant whose turn just ended, captured BEFORE turn_index
advanced), decrement rounds_remaining and expire at 0 — i.e. "the end of your
next turn". ORPHAN GUARD: also expire any marker whose caster is no longer an
active combatant (caster died/removed), so it can't leak. The consume-on-attack
path in attack() is untouched; the new expiry only matters when no one attacked.
DEVIATION from the literal spec step 4 (documented): the spec said expire on the
FIRST caster-turn-end (== `previous`). That breaks the existing same-round test
(test_guiding_bolt_marker_auto_grants...): the cast happens DURING the caster's
turn, so the first next_turn-off-the-caster is the cast turn itself — expiring
there kills the marker before a same-round attacker can use it. SRD "your NEXT
turn" means the turn AFTER the cast turn, so the marker is given a +1
rounds_remaining grace and ticked at each caster-turn-end (cast turn: 2->1; next
turn: 1->0 -> expire). This preserves the same-round test and fixes the
cross-round bug. No existing test/guard weakened.
FIX B (missing cue only — NOT a mechanics bug; the maneuver-damage paths are
correct + tested) — _turn_brief emitted a tactical `suggested_when` cue for
second_wind/action_surge/channel_divinity but NOT for superiority_dice, so the
DM was never steered to the attack(maneuver=) path and burned the die via a bare
use_resource (correct-but-pointless).
- server._turn_brief: add a `superiority_dice` branch steering the DM to declare
the maneuver ON the attack (attack(maneuver=..., maneuver_resource=...)).
- server.use_resource: advisory-only `warning` when a die pool is spent in active
combat with no maneuver= (does NOT block — the spend still succeeds).
TESTS (all single-process, no xdist):
- test_effect_durations.py:
* test_guiding_bolt_marker_survives_into_casters_next_turn — the cross-round
bug: fighter acts before the cleric in round 2; the marker survives the
round-2-start tick and the fighter attacks with advantage
(advantage_source == 'Guiding Bolt'), then it's consumed. PROVEN to FAIL on
pre-fix code (marker expired at the round boundary -> advantage False).
* test_guiding_bolt_marker_expires_at_end_of_casters_next_turn_if_unused — when
unused, the marker expires at the end of the caster's round-2 turn and does
NOT leak into round 3.
- test_class_resources.py:
* test_turn_brief_superiority_dice_suggests_maneuver_on_attack
* test_use_resource_bare_superiority_die_in_combat_warns
* test_use_resource_bare_superiority_die_out_of_combat_no_warning (advisory inert
out of combat)
Verified: test_effect_durations + test_class_resources + test_combat = 249
passed; affected adjacent suites (action_economy, effect_riders, finesse_sneak,
multiattack_parser, world_clock_rest_seams, zones, combat_depth_enforcement,
qa_gate) all green.
… (not just on remove) Adversarial review found a real leak: death (_die) sets dead=True but does NOT pop c.combat.order, so a dead-but-unremoved caster (the common 0-HP monster case) defeated the orphan guard — the turn-anchored advantage marker leaked for the rest of the fight and granted unearned advantage to later attacks on the marked foe. Build active_ids from LIVING combatants (mirrors the advance loop's own not-dead skip). Proven RED->GREEN by reverting the one-line guard. + test_guiding_bolt_marker_swept_when_caster_dies_midcombat.
|
Warning Review limit reached
More reviews will be available in 3 hours, 8 minutes, and 55 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ccfe510af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| active_ids = { | ||
| cb.character_id | ||
| for cb in order | ||
| if (h := c.characters.get(cb.character_id)) is not None and not h.dead | ||
| } |
There was a problem hiding this comment.
When a Guiding Bolt caster is killed before an ally's next attack in the same initiative cycle (for example: caster hits, enemy kills the caster, then an ally acts), this living-only active_ids makes caster_gone true on the enemy's next_turn, so the non-concentration rider is removed before the ally can use the advantage. Guiding Bolt's rider lasts until the end of the caster's next turn or the next attack and is not concentration-based, so death shouldn't cancel it immediately; expire it when the dead caster's initiative slot would have passed instead of sweeping it as soon as the caster is dead.
Useful? React with 👍 / 👎.
… annotation (#1034) (#1035) Checkpoint marking the Guiding Bolt SRD duration fix (found by running the combat-sprint, proven RED->GREEN, adversarially reviewed) + the ruler-version annotation. Still NOT a GA — mech remains below the 4.5 bar; story BG-caliber + satisfaction green. Co-authored-by: Eva <arncalso@gmail.com>
1 participant
Why
Running the combat-sprint (the surface that actually exercises combat — not a code-read) scored mech 3.3, and the Angry-DM found two HIGH defects. Root-caused against source + the real transcript (
qa/transcripts/cs-mechread.jsonl):A — REAL engine bug: Guiding Bolt advantage expires one round-boundary too early
SRD 5.2 Guiding Bolt grants advantage on the next attack "before the end of your next turn" — a turn-anchored window. The engine stored it as a fixed
rounds_remaining=1round-counter, whichnext_turnticks to 0 at the start of the new round — before the next attacker acts — so the qualifying attack resolvedadvantage:false. The existing test only covered the same-round case, so it shipped green.Fix (additive, mirrors the
repeat_saveexemption): addActiveEffect.expires_end_of_turn_of(caster id, defaultsNone); exempt such markers from thetick_round_effectsround-decrement; bumprounds_remaining +1at materialization; tick/expire them innext_turnonly at the caster's turn-end. Orphan guard sweeps the marker if the caster dies (death setsdead=Truebut doesn't pop the order — caught by adversarial review;active_idsis built from living combatants) or is removed.B — NOT an engine bug, a missing cue: Battle Master superiority die spent for no bonus damage
The engine's maneuver-damage linkage is correct + tested (two paths). The DM called
use_resourcewithoutmaneuver=, burning the die as a plain point (working as designed). The gap:_turn_briefcuessecond_wind/action_surge/channel_divinitybut notsuperiority_dice. Fix: add thesuggested_whencue (the #A3 nova-cue precedent) + an advisorywarningon a bare in-combat die-pool spend. No mechanics change.Validation
tests/test_effect_durations.py+tests/test_class_resources.py+tests/test_combat.py→ 250 passed (single-process).Invariants
Additive (new field defaults
None, old snapshots round-trip underextra="forbid"); engine = sole writer; engine rolls & DM is told; no existing guard weakened (test diffs are additions-only).🤖 Generated with Claude Code