fix(character): expose SRD subclass options + apply choice-level features (#624) - #742
Merged
Merged
Conversation
…ures (closes #624) A Wizard reaching level 3 could not pick an Arcane Tradition: the engine treated subclass as a free-text string, applied only the generic "Wizard Subclass" placeholder, and exposed NO valid options for the level-up surface to render. The optimizer persona bailed at turn 2 (cannot build). Additive fix: - data/srd/subclasses.json: curated SRD 5.2.4 subclass table — per class, the subclass-choice level (3), the canonical SRD subclass (one per class), a brief feature preview, the choice-level features it grants, and a loose-name alias map. - srd_tables: subclass_level / subclass_options / subclass_group_label / resolve_subclass / subclass_features_at accessors (engine OWNS the legal options). - level_up + create-at-level path: normalize a chosen subclass to its canonical SRD name ('Evocation' -> 'Evoker'; unknown/world-canon names pass through) and grant its choice-level features (Evocation Savant, Sculpt Spells) — not just the placeholder. - build_options: each option that levels INTO a class's subclass level now carries a `subclass` block (options + previews + required flag) for the surface to render. - screen-character LevelUpModal: presents the engine-exposed options as a pickable list with feature previews, keeping the named free-text input as a fallback for world-canon traditions the SRD table doesn't enumerate. Engine SOLE-WRITER + additive invariants preserved: the data is SRD-only, old snapshots round-trip (display path never normalizes; only a class-signature re-patch does), and an unknown subclass still passes through verbatim. Tests: 6 new engine tests (test_class_features.py) + 1 viewer bridge test + updated levelup-picker guard; 3 pre-existing tests updated for the now-canonical subclass normalization (Rolan the Evoker now actually gains his L3 features). Full engine suite 1740 passed; viewer 452 passed/6 skipped; fast_gate 188 passed.
📝 WalkthroughWalkthroughThis PR implements SRD subclass selection at the appropriate class level (e.g., Wizard at level 3). It adds subclass metadata, resolves user-provided names to canonical SRD names, grants subclass-specific features when characters reach the selection level, surfaces subclass choices in the build-options response, and updates the frontend to render engine-provided subclass options. ChangesSRD Subclass Selection and Feature Application
Sequence DiagramsequenceDiagram
participant Player
participant LevelUpModal
participant build_options
participant level_up
participant srd_tables
participant Database
Player->>LevelUpModal: Reach subclass-selection level
LevelUpModal->>build_options: Request options for class/level pair
build_options->>srd_tables: Check if level is subclass-selection level
srd_tables-->>build_options: Yes, return subclass level info
build_options->>srd_tables: Fetch subclass options and labels
srd_tables->>Database: Load subclasses.json data
Database-->>srd_tables: Subclass metadata with features
srd_tables-->>build_options: Options list with group label
build_options-->>LevelUpModal: Build options including subclass block
LevelUpModal->>Player: Display subclass picker with options
Player->>LevelUpModal: Select subclass (e.g., "Evoker")
LevelUpModal->>level_up: Confirm with subclass choice
level_up->>srd_tables: Resolve subclass name to canonical form
srd_tables-->>level_up: Canonical name + features
level_up->>Database: Save character with subclass and features applied
Database-->>level_up: Character updated
level_up-->>LevelUpModal: Confirmation
LevelUpModal-->>Player: Character leveled with subclass features
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
servers/engine/server.py (1)
4772-4781:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the subclass choice server-side and stop late rewrites.
build_optionsnow marks this choice as required, butlevel_upstill lets a caller reach the subclass level with nosubclass, and it will also overwrite an existing subclass on later levels. That leaves invalid sheets in both directions: a Wizard can still hit level 3 with only the generic placeholder, and a later call can switchEvoker -> Illusionistwithout reconciling the old level-3 features.Suggested guard
if subclass: subclass = srd_tables.resolve_subclass(cname, subclass) or subclass + subclass_level = srd_tables.subclass_level(cname) + chosen_subclass = existing.subclass if existing else None + if subclass_level == new_class_level and not (chosen_subclass or subclass): + raise ValueError(f"{class_name} level {new_class_level} requires a subclass choice") + if chosen_subclass and subclass and chosen_subclass != subclass: + raise ValueError( + f"{class_name} subclass is already set to {chosen_subclass!r}; " + "changing it here would leave stale subclass features on the sheet" + )Also applies to: 4811-4817
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/engine/server.py` around lines 4772 - 4781, level_up currently allows reaching and later changing subclass choices; enforce server-side that when build_options marks the subclass choice required you must reject/raise when no subclass is provided and avoid overwriting an already-set subclass on subsequent levels. In the level_up logic around the srd_tables.resolve_subclass(...) and the block that updates existing (variables: subclass, existing, srd_tables.resolve_subclass), validate that subclass is present when the choice is required (throw/return an error) and only assign existing.subclass if it is currently empty (i.e., set it once at the subclass level) — do the same change in the second similar block (the other level_up occurrence around lines 4811-4817) so late rewrites are prevented and missing required choices are rejected.
🧹 Nitpick comments (2)
servers/engine/tests/test_class_features.py (2)
128-128: 💤 Low valueAdd defensive assertion before
next()for clearer test failures.If
planner["options"]does not contain a wizard entry, thenext()call will raiseStopIterationwith a generic message. Adding an assertion first (or a default argument tonext()) would produce a clearer diagnostic if the wizard option is missing.♻️ More defensive lookup
+ class_names = {o["class_name"] for o in planner["options"]} + assert "wizard" in class_names, "build_options must include wizard at level 2" wiz_opt = next(o for o in planner["options"] if o["class_name"] == "wizard")or
- wiz_opt = next(o for o in planner["options"] if o["class_name"] == "wizard") + wiz_opt = next((o for o in planner["options"] if o["class_name"] == "wizard"), None) + assert wiz_opt is not None, "build_options must include wizard at level 2"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/engine/tests/test_class_features.py` at line 128, The test currently uses wiz_opt = next(o for o in planner["options"] if o["class_name"] == "wizard") which can raise StopIteration with a generic message; add a defensive check before calling next() (e.g., assert any(o.get("class_name") == "wizard" for o in planner["options"]), "wizard option missing in planner['options']") or supply a default to next() and raise a clearer AssertionError afterwards so that missing wizard entries produce an explicit, informative test failure referencing planner["options"] and wiz_opt.
78-79: 💤 Low valueConsider tightening the feature-preview assertion to verify both expected features.
The current
any(...or...)pattern passes if at least one of the two features appears in any feature name. To align with later tests (lines 101, 103-104, 115-116) that verify both features are granted, consider checking that both "Evocation Savant" and "Sculpt Spells" are present in the features list.♻️ More precise assertion
- assert any("Evocation Savant" in f["name"] or "Sculpt Spells" in f["name"] - for f in evoker["features"]) + feature_names = {f["name"] for f in evoker["features"]} + assert "Evocation Savant" in feature_names and "Sculpt Spells" in feature_names🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/engine/tests/test_class_features.py` around lines 78 - 79, The assertion for evoker features currently uses any(... or ...) and can pass if only one of the expected features is present; update the assertion to verify both "Evocation Savant" and "Sculpt Spells" are present in evoker["features"]. Locate the assertion in test_class_features.py that references evoker and its "features" list and replace the any(... or ...) check with a check that both feature names exist (e.g., by collecting feature["name"] values and asserting both names are in that set or using all(...) over two membership checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@servers/engine/server.py`:
- Around line 4772-4781: level_up currently allows reaching and later changing
subclass choices; enforce server-side that when build_options marks the subclass
choice required you must reject/raise when no subclass is provided and avoid
overwriting an already-set subclass on subsequent levels. In the level_up logic
around the srd_tables.resolve_subclass(...) and the block that updates existing
(variables: subclass, existing, srd_tables.resolve_subclass), validate that
subclass is present when the choice is required (throw/return an error) and only
assign existing.subclass if it is currently empty (i.e., set it once at the
subclass level) — do the same change in the second similar block (the other
level_up occurrence around lines 4811-4817) so late rewrites are prevented and
missing required choices are rejected.
---
Nitpick comments:
In `@servers/engine/tests/test_class_features.py`:
- Line 128: The test currently uses wiz_opt = next(o for o in planner["options"]
if o["class_name"] == "wizard") which can raise StopIteration with a generic
message; add a defensive check before calling next() (e.g., assert
any(o.get("class_name") == "wizard" for o in planner["options"]), "wizard option
missing in planner['options']") or supply a default to next() and raise a
clearer AssertionError afterwards so that missing wizard entries produce an
explicit, informative test failure referencing planner["options"] and wiz_opt.
- Around line 78-79: The assertion for evoker features currently uses any(... or
...) and can pass if only one of the expected features is present; update the
assertion to verify both "Evocation Savant" and "Sculpt Spells" are present in
evoker["features"]. Locate the assertion in test_class_features.py that
references evoker and its "features" list and replace the any(... or ...) check
with a check that both feature names exist (e.g., by collecting feature["name"]
values and asserting both names are in that set or using all(...) over two
membership checks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb818f98-635e-432a-9e96-b04303c5189d
📒 Files selected for processing (9)
data/srd/subclasses.jsonservers/engine/server.pyservers/engine/srd_tables.pyservers/engine/tests/test_class_features.pyservers/engine/tests/test_codex_provider_wrapper.pyservers/engine/tests/test_qa_fixes.pyviewer/openworlds/screen-character.jsxviewer/tests/test_build_options_bridge.pyviewer/tests/test_levelup_picker.py
This was referenced Jun 10, 2026
Closed
Closed
Closed
100yenadmin
added a commit
that referenced
this pull request
Jun 10, 2026
…backfill) (#759) The #742 subclass-options block only fired when next_class_level == the subclass-choice level exactly. A character ALREADY PAST that level with the subclass still unset (the pendingSubclass case — e.g. an L5 wizard with no Arcane Tradition leveling to L6) got NO options block, so the picker fell back to free text (rc2 audit: vm2-optimizer + vm2-veteran bugs.ndjson). - _subclass_block_for: also fire when next_class_level > slvl AND the subclass is unset (backfill semantics — a missed choice is offered at the next level-up). Subclass already set => unchanged (no block past the level). - level_up: a subclass SET FOR THE FIRST TIME past the choice level grants its choice-level features (Evocation Savant + Sculpt Spells), not nothing. - Viewer needs no change: screen-character.jsx renders the option list whenever option.subclass exists (free text only as fallback), and subclassDue is already true via hero.pendingSubclass. 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.
Issue
Closes #624 — Wizard subclass (Arcane Tradition) absent at L3 — min-maxer cannot build (crit) + the optimizer finding "Subclass selection is a free-text field — no list of options or feature previews."
Root cause (validated against code)
The level-up / character flow treated subclass as an opaque free-text string:
srd_tableshad no subclass enumeration —class_data('wizard')carries no subclass list, so the surface had nothing to render and fell back to a blind text box.level_up(subclass='Evocation')stored the string but applied only the generic"Wizard Subclass"placeholder fromclass_features.json— no actual Evocation features.build_optionsreturned no subclass block, so the/characterpicker (LevelUpModal) could only offer a free-text input.Reproduced before fixing: a Wizard leveled 2→3 with
subclass='Evocation'gotfeatures=['…','Wizard Subclass']—Sculpt Spells/Evocation Savantabsent.Fix (additive)
data/srd/subclasses.json(new) — curated SRD 5.2.4 table: per class the subclass-choice level (3), the canonical SRD subclass (one per class, per the SRD subset), a brief feature preview, the choice-level features it grants, and a loose-name alias map.srd_tables—subclass_level/subclass_options/subclass_group_label/resolve_subclass/subclass_features_at. The engine owns the legal options.level_up+ create-at-level path — normalize a chosen subclass to canonical (Evocation→Evoker; unknown/world-canon names pass through verbatim) and grant its choice-level features.build_options— each option leveling into a class's subclass level carries asubclassblock (options+descpreviews +required+group_label).screen-characterLevelUpModal— presents the engine-exposed options as a pickable list with feature previews; keeps the named free-text input as a fallback for world-canon traditions the SRD table doesn't enumerate.Invariants preserved
dointent → DM →level_up)._private/untouched.Tests
test_class_features.py): subclass-level table; wizard options exposed w/ preview; alias resolution; level-up applies Evoker features; create-at-L3 applies them;build_optionsexposes the choice.test_build_options_bridge.py): thesubclassblock surfaces through/build-optionsfor a wizard at L2→L3.test_levelup_picker.pyguard updated: options come from the engine planner (option.subclass), never JSX-hardcoded; named input retained.test_qa_fixes,test_codex_provider_wrapper, charsheet stays display-verbatim). Rolan the Evoker now actually gains his L3 features at seed time.Verification
qa/fast_gate.sh→ 188 passed ✅Honest scope note: this lands the SRD choice-level features per #624's "full fix" intent. Later-level subclass features remain represented by the existing generic
"Subclass Feature"placeholders inclass_features.json(out of scope here; not regressed).Summary by CodeRabbit
New Features
Improvements