fix(engine): Champion expanded crit-range + War Caster concentration advantage#1038
Merged
Merged
Conversation
…advantage
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.
![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: 6233fff71e
ℹ️ 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".
Comment on lines
+1485
to
+1489
| 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) |
There was a problem hiding this comment.
This derives crit_min only in _apply_srd_class_defaults, but the normal level_up() path grants Champion subclass features in its own feature loop and never re-runs this block after adding Improved Critical or Superior Critical. A Fighter who reaches level 3/15 via level_up(..., subclass="Champion") will have the feature listed but keep crit_min == 20, so their natural 19/18 attacks still do not crit; only characters seeded directly at that level get the new behavior.
Useful? React with 👍 / 👎.
This was referenced Jun 19, 2026
100yenadmin
added a commit
that referenced
this pull request
Jun 19, 2026
…ty (#1038) + scorer timeout-guard (#1039) (#1041) Test-proven code checkpoint on rc2. Still NOT a GA — the GLM re-measure under the current ruler is deferred (scorer hangs on combat-sprint transcripts, #1040). Story above bar at depth (old ruler), mech the real gap, satisfaction green. Co-authored-by: Eva <arncalso@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two additive SRD-fidelity fixes the Angry-DM lens docks (found via the combat-sprint); both default to today's behavior (old snapshots round-trip under
extra="forbid").dice.py roll()gains optionalcrit_min:int=20(crit = natural >= crit_min);Character.crit_min:int=20;_apply_srd_class_defaultssets 19/18 for Improved/Superior Critical;attack()threadsattacker.crit_min. The deadcrit_source()"expanded_crit_range" branch now lights up. A Champion crits on a natural 19; a plain fighter does not._auto_concentration_save, theconcentration_savetool) passadvantage=Truewhen the character has the War Caster feat (read fromch.feats). 5e: concentration saves are damage-triggered, so the feat applies to both sites.Tests: 6 new (dice crit_min, Champion-19, Superior-18, War Caster advantage at both sites). Full engine suite 3042 passed, 0 failed (single-process). (c) ranged-in-melee left as-is — design-correct theater-of-mind, the gate WARN is right.
🤖 Generated with Claude Code