feat: spell preparation for dnd_5e_srd#111
Merged
Merged
Conversation
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #b7e895
Actionable Suggestions - 4
-
cli/src/display.rs - 1
- Incorrect cap display when None · Line 154-157
-
apps/ex_ttrpg_dev/lib/characters.ex - 1
- Hardcoded system-specific IDs in library · Line 193-333
-
apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/ranger.toml - 1
- Incorrect Ranger spellcasting mode · Line 6-6
-
apps/ttrpg_dev_cli/lib/cli/spell_prep.ex - 1
- Spell ID Uniqueness Validation · Line 67-68
Additional Suggestions - 1
-
apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/paladin.toml - 1
-
Incorrect spell prep calculation · Line 87-87The half_level mapping steps appear to implement ceil(level/2) instead of floor(level/2), which is required for Paladin spell preparation in D&D 5e. This would cause incorrect spell preparation counts, e.g., level 1 Paladins getting 1 instead of 0.
Code suggestion
@@ -87,1 +87,1 @@ - steps = [[1, 1], [4, 2], [6, 3], [8, 4], [10, 5], [12, 6], [14, 7], [16, 8], [18, 9], [20, 10]] + steps = [[1,0],[3,1],[5,2],[7,3],[9,4],[11,5],[13,6],[15,7],[17,8],[19,9]]
-
Review Details
-
Files reviewed - 19 · Commit Range:
56784b9..841e148- apps/ex_ttrpg_dev/lib/characters.ex
- apps/ex_ttrpg_dev/lib/characters/character.ex
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/bard.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/cleric.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/druid.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/paladin.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/ranger.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/sorcerer.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/warlock.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/wizard.toml
- apps/ex_ttrpg_dev/test/rule_system/graph_test.exs
- apps/ex_ttrpg_dev/test/rule_systems_test.exs
- apps/ttrpg_dev_cli/lib/cli/serializer.ex
- apps/ttrpg_dev_cli/lib/cli/server.ex
- apps/ttrpg_dev_cli/lib/cli/spell_prep.ex
- cli/Cargo.lock
- cli/src/commands/characters.rs
- cli/src/display.rs
- cli/src/protocol.rs
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at dev.6y28p@passmail.net.
Documentation & Help
AI Code Review powered by
Comment on lines
+154
to
+157
| Some("prepared") => { | ||
| let prepared = r.prepared_spells.len(); | ||
| let cap = r.cap.unwrap_or(0); | ||
| println!("Prepared spells: {prepared}/{cap}"); |
There was a problem hiding this comment.
Incorrect cap display when None
In the 'prepared' mode, using unwrap_or(0) for cap may display incorrect information if cap is None, implying a limit of 0 when it might mean unlimited. Consider handling None by omitting the '/cap' part.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| Some("prepared") => { | |
| let prepared = r.prepared_spells.len(); | |
| let cap = r.cap.unwrap_or(0); | |
| println!("Prepared spells: {prepared}/{cap}"); | |
| Some("prepared") => { | |
| let prepared = r.prepared_spells.len(); | |
| if let Some(cap) = r.cap { | |
| println!("Prepared spells: {prepared}/{cap}"); | |
| } else { | |
| println!("Prepared spells: {prepared}"); | |
| } |
Code Review Run #b7e895
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| hit_die = "d10" | ||
| armor_proficiencies = ["Light Armor", "Medium Armor", "Shield"] | ||
| weapon_proficiencies = ["Simple Weapons", "Martial Weapons"] | ||
| preparation_mode = "all" |
There was a problem hiding this comment.
Incorrect Ranger spellcasting mode
Rangers in D&D 5e are prepared spellcasters, selecting spells from their spell list each day, not spontaneous casters who know all spells. This should match Cleric, Druid, Paladin, and Wizard with "prepared".
Code suggestion
Check the AI-generated fix before applying
Suggested change
| preparation_mode = "all" | |
| preparation_mode = "prepared" |
Code Review Run #b7e895
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
QMalcolm
force-pushed
the
qmalcolm--feat-spell-preparation
branch
from
841e148 to
25eb923
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Introduces a dedicated prepared_spells list on Character for classes that use the prepare-from-pool model (Cleric, Druid, Paladin, Wizard). Only player-chosen spells are stored here — always-prepared spells contributed by subclass features are computed at read time and not persisted. The field defaults to [] so existing saved characters deserialise without migration.
…lcasting classes
Bard, Ranger, Sorcerer, and Warlock are preparation_mode = "all" — their
prepared list is simply their full known-spells list, no player management
required.
Cleric, Druid, Paladin, and Wizard are preparation_mode = "prepared" with a
preparation_pool ("class_spells" or "spellbook") and a preparation_cap formula
node: wisdom/intelligence modifier + character level (full casters) or charisma
modifier + half paladin level (Paladin). The cap nodes use the existing formula
and mapping DAG infrastructure.
Paladin's half_level mapping is added as an intermediate node (floor(level/2),
minimum 1 at level 1) to keep the preparation_cap formula readable. Charisma is
used for Paladin per PHB — the issue description incorrectly cited Wisdom.
…aration_spells
Three new public functions on Characters for spell preparation mechanics:
- preparation_cap/3: evaluates the class's preparation_cap formula node
against the character's current state; clamps result to minimum of 1.
- always_prepared_spells/3: reads always_prepared list from the active
subclass metadata and filters to spells within the character's current
max_spell_level. Computed at read time — not persisted — so subclass
changes are reflected automatically.
- eligible_preparation_spells/3: returns spells available for preparation
based on the class's preparation_pool ("class_spells" or "spellbook").
"class_spells" filters the system's full spell list by class membership
and max_spell_level; "spellbook" filters spells known via progression
decisions.
"spell" as a concept type and "classes"/"always_prepared" as metadata
field names are library conventions for spell preparation, analogous to
how "character_progression" is a convention for character advancement.
…odule Server was approaching CodeScene code health thresholds: 1265 lines, 75+ functions. Extracting the serialization layer into a dedicated Serializer module keeps both modules well under the limits and separates protocol-dispatch concerns from data-shaping concerns. All public entry points (serialize_character, serialize_building_choices, serialize_choices_list, etc.) are unchanged in behaviour; callers updated to use Serializer.* prefix. resolve_character stays private in Server as it is used by multiple handlers and does not belong in the serialization layer. refactor(cli/server): extract serialization helpers into Serializer module Server was approaching CodeScene code health thresholds: 1265 lines, 75+ functions. Extracting the serialization layer into a dedicated Serializer module keeps both modules well under the limits and separates protocol-dispatch concerns from data-shaping concerns. All public entry points (serialize_character, serialize_building_choices, serialize_choices_list, etc.) are unchanged in behaviour; callers updated to use Serializer.* prefix. resolve_character stays private in Server as it is used by multiple handlers and does not belong in the serialization layer.
Implements the server-side protocol for spell preparation queries and updates. SpellPrep module (extracted separately) contains the core logic; server.ex adds protocol docs and the combined handler clause. - characters.spells: returns preparation mode, cap, eligible pool, and current prepared_spells for the character's spellcasting class - characters.prepare: validates and persists a new prepared_spells list, returning the updated state with always_prepared spells included
Implements the Rust-side of spell preparation: - SpellsResponse and PrepareResult protocol types - display::print_spells renders preparation mode, cap, always-prepared spells, and current prepared list - characters spells <slug>: queries and displays current spell prep state - characters prepare <slug> <spell_id>...: submits a new prepared list and prints the result Protocol tests cover both types including the null-mode (no spellcasting class) case. display::print_spells handles all three modes: nil (no class), "all" (sorcerer/bard/etc), and "prepared" (cleric/druid/etc).
Adds prepared_spells to the serialize_character/4 output so that characters with a prepared-mode spellcasting class show their currently prepared spells when viewed with `characters show`. Spells are rendered using the system's spell display template (falling back to raw name), matching the label format used elsewhere in the sheet.
…Oath of Devotion Life Domain clerics and Oath of Devotion paladins have fixed subclass spell lists that are always prepared without counting against the preparation cap. These lists are now declared via always_prepared in each subclass concept so SpellPrep.query/2 surfaces them correctly. Circle of the Land is skipped — its bonus spells are terrain-dependent and there is no terrain sub-choice in the system yet. School of Evocation has no always-prepared spells in the SRD.
…tivate config Replace the flat [inventory]/[inventory_item_schema] TOML format with [inventory_type.<id>] sections, where each type declares its own schema, activation verb, deactivation verb, activation field, add_on_progression hooks, and optional preparation config. Add spell inventory type to dnd_5e_srd/inventory_rules.toml with full preparation config (mode_field, pool_field, cap_field, always_prepared, auto_activate_when, and pool strategies for class_spells and spellbook). Cantrips enter spell inventory with auto_activate=true and excludes_from_cap=true; spells_known progressions enter without auto-activation. InventoryRules module gains per-type structs (TypeConfig, ProgressionConfig, PoolConfig, PreparationConfig) and new query functions: type_config/2, type_schema/2, default_fields/2 (now type-aware), type_for_activate_command/2, type_for_progression/2, preparation_types/1, progression_config/3. InventoryItem.new/4 and set_field/4 now look up the per-type schema rather than a global schema. The old [inventory]/[inventory_item_schema] format is not supported; all test fixtures are updated to the new format.
Adds two new public functions to Characters: - activate/4: prepares items of a typed inventory based on the preparation config in InventoryRules. Dispatches to add_remove or toggle_field management based on the pool config. Validates the eligible pool and preparation cap before applying changes. - add_to_typed_inventory/4: called when a character_progression decision is resolved. Looks up the inventory type that lists the progression in add_on_progression, then adds the concept as an inventory item. Initial activation is determined by: per-progression auto_activate flag (cantrips), the auto_activate_when class condition (Bard all-mode), or false by default. The old preparation_cap/3, always_prepared_spells/3, and eligible_preparation_spells/3 are kept for now since spell_prep.ex still calls them; they will be removed alongside spell_prep.ex in the CLI refactor commit.
Remove the three hardcoded-to-dnd_5e_srd public functions (preparation_cap/3, always_prepared_spells/3, eligible_preparation_spells/3) and their private helpers from characters.ex, replacing them with the generic preparation_state/3 driven by PreparationConfig. Drop spell_prep.ex and wire two server commands generically: - characters.spells calls Characters.preparation_state/3, reading prepared items from inventory rather than the removed Character.prepared_spells. - characters.prepare replaced by characters.activate, dispatching via type_for_activate_command lookup on the verb. Response is the inventory. Remove prepared_spells from Character struct, to_json_map, and from_json!. Old JSON files with the key are forward-compatible (field silently ignored). Serializer drops the prepared_spells key from serialize_character output.
When a selection-type progression is resolved (cantrips, spells_known, etc.), call Characters.add_to_typed_inventory/4 after recording the decision. This automatically places the selected concept into the appropriate typed inventory with the correct initial activation state: - auto_activate progressions (cantrips) → prepared: true - auto_activate_when class condition (Bard) → prepared: true - default → prepared: false Equipment choices already handled via gen_character!/2; this covers the ongoing-play path through resolve_choice.
Replaces the spell-prep-specific `handle_characters_prepare` and `PrepareResult` type with a generic `handle_activate` that sends `characters.activate` with a `verb` and `items` payload. Any inventory type declared in TOML with a matching `activate_command` is now reachable from the CLI without new Rust code. Also removes `prepared_spells` from `CharacterData` and `display.rs` — prepared spell state is now read from the typed spell inventory via `characters spells`.
QMalcolm
force-pushed
the
qmalcolm--feat-spell-preparation
branch
from
25eb923 to
46a8abf
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Author
|
/review |
Code Review Agent Run #479d80Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
Code Review Agent Run #7d3b86
Actionable Suggestions - 3
-
apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/paladin.toml - 1
- Incorrect paladin spell prep calculation · Line 87-87
-
apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/bard.toml - 1
- Wrong spell preparation mode for bard · Line 6-6
-
cli/src/commands/characters.rs - 1
- Incorrect activation count logic · Line 719-729
Review Details
-
Files reviewed - 25 · Commit Range:
21dbf8b..2008de5- apps/ex_ttrpg_dev/lib/characters.ex
- apps/ex_ttrpg_dev/lib/characters/inventory_item.ex
- apps/ex_ttrpg_dev/lib/rule_system/inventory_rules.ex
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/bard.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/cleric.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/druid.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/paladin.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/ranger.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/sorcerer.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/warlock.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/wizard.toml
- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/inventory_rules.toml
- apps/ex_ttrpg_dev/test/characters/character_test.exs
- apps/ex_ttrpg_dev/test/characters/inventory_item_test.exs
- apps/ex_ttrpg_dev/test/characters_test.exs
- apps/ex_ttrpg_dev/test/rule_system/graph_test.exs
- apps/ex_ttrpg_dev/test/rule_system/inventory_rules_test.exs
- apps/ex_ttrpg_dev/test/rule_system/loader_test.exs
- apps/ex_ttrpg_dev/test/rule_systems_test.exs
- apps/ttrpg_dev_cli/lib/cli/serializer.ex
- apps/ttrpg_dev_cli/lib/cli/server.ex
- cli/Cargo.lock
- cli/src/commands/characters.rs
- cli/src/display.rs
- cli/src/protocol.rs
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at dev.6y28p@passmail.net.
Documentation & Help
AI Code Review powered by
| [class.paladin.half_level] | ||
| type = "mapping" | ||
| input = "character_trait('character_level').level" | ||
| steps = [[1, 1], [4, 2], [6, 3], [8, 4], [10, 5], [12, 6], [14, 7], [16, 8], [18, 9], [20, 10]] |
There was a problem hiding this comment.
Incorrect paladin spell prep calculation
The half_level mapping steps incorrectly start with [1, 1], but D&D 5e paladin spell preparation uses floor(level/2), so level 1 should map to 0. This causes overestimation of prepared spells at low levels.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| steps = [[1, 1], [4, 2], [6, 3], [8, 4], [10, 5], [12, 6], [14, 7], [16, 8], [18, 9], [20, 10]] | |
| steps = [[1, 0], [2, 1], [4, 2], [6, 3], [8, 4], [10, 5], [12, 6], [14, 7], [16, 8], [18, 9], [20, 10]] |
Code Review Run #7d3b86
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| hit_die = "d8" | ||
| armor_proficiencies = ["Light Armor"] | ||
| weapon_proficiencies = ["Simple Weapons", "Hand Crossbow", "Longsword", "Rapier", "Shortsword"] | ||
| preparation_mode = "all" |
There was a problem hiding this comment.
Wrong spell preparation mode for bard
The added preparation_mode = "all" treats bards like sorcerers who know all spells without preparation, but bards actually prepare spells like wizards. This will incorrectly auto-prepare all known spells for bards instead of requiring manual preparation.
Code Review Run #7d3b86
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
+719
to
+729
| let activated = r | ||
| .inventory | ||
| .iter() | ||
| .filter(|item| { | ||
| item.fields | ||
| .as_object() | ||
| .map(|m| m.values().any(|v| v == &serde_json::Value::Bool(true))) | ||
| .unwrap_or(false) | ||
| }) | ||
| .count(); | ||
| println!("{activated} item(s) active."); |
There was a problem hiding this comment.
Incorrect activation count logic
The filter counts all inventory items with any boolean field set to true, incorrectly including items activated by other commands (e.g., equipped equipment with 'equipped': true). For spell preparation, it should only count items where 'prepared' is true. The message should also say 'prepared' instead of 'active' to match the action.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| let activated = r | |
| .inventory | |
| .iter() | |
| .filter(|item| { | |
| item.fields | |
| .as_object() | |
| .map(|m| m.values().any(|v| v == &serde_json::Value::Bool(true))) | |
| .unwrap_or(false) | |
| }) | |
| .count(); | |
| println!("{activated} item(s) active."); | |
| let activated = r | |
| .inventory | |
| .iter() | |
| .filter(|item| { | |
| item.fields | |
| .as_object() | |
| .and_then(|m| m.get("prepared")) | |
| .map(|v| v == &serde_json::Value::Bool(true)) | |
| .unwrap_or(false) | |
| }) | |
| .count(); | |
| println!("{activated} item(s) prepared."); |
Code Review Run #7d3b86
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
QMalcolm
commented
Apr 4, 2026
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| pub(crate) struct SpellsResponse { |
Collaborator
Author
There was a problem hiding this comment.
Why is SpellsResponse a thing? A spell is a system specific concept. Is this to handle the spell inventory? Shouldn't it just be a InventoryResponse?
floor(level/2) at level 1 is 0, not 1. The previous first step [1, 1] overestimated the preparation cap by 1 for level 1 paladins. Added [2, 1] as the second step so the mapping correctly returns 1 for levels 2-3.
The previous count logic filtered inventory items with any boolean-true field, which incorrectly counted items from other inventory types (e.g. equipped gear when preparing spells). Since handle_activate is generic and does not know which field name corresponds to the verb, a count is not meaningful here. Print 'Done.' instead; callers can use 'characters spells' or 'characters inventory' for full state.
SpellsResponse tied a protocol type to a system-specific concept (spells). The struct represents the preparation state of any typed inventory (mode, cap, eligible pool, prepared items, always-prepared) — not spells specifically. PreparationStateResponse reflects that generality.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…onse eligible_spells, prepared_spells, and always_prepared encoded a spell-specific concept in the protocol layer. Renamed to eligible_items, prepared_items, and always_active — accurate for any preparation-managed inventory type, not only spells.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review Agent Run #0b27eb
Actionable Suggestions - 2
-
cli/src/display.rs - 1
- Inconsistent display labels after field rename · Line 139-166
-
cli/src/protocol.rs - 1
- API Key Mismatch · Line 223-223
Review Details
-
Files reviewed - 5 · Commit Range:
46a8abf..c81821a- apps/ex_ttrpg_dev/priv/system_configs/dnd_5e_srd/concepts/class/paladin.toml
- cli/src/commands/characters.rs
- cli/src/display.rs
- cli/src/protocol.rs
- apps/ttrpg_dev_cli/lib/cli/server.ex
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at dev.6y28p@passmail.net.
Documentation & Help
AI Code Review powered by
Comment on lines
+139
to
+166
| Some("all") => { | ||
| println!( | ||
| "All {} spell(s) prepared (no manual preparation needed).", | ||
| r.prepared_items.len() | ||
| ); | ||
| if !r.always_active.is_empty() { | ||
| println!("Always prepared: {}", r.always_active.join(", ")); | ||
| } | ||
| if !r.prepared_items.is_empty() { | ||
| println!("Prepared: {}", r.prepared_items.join(", ")); | ||
| } | ||
| } | ||
| Some("prepared") => { | ||
| let prepared = r.prepared_items.len(); | ||
| let cap = r.cap.unwrap_or(0); | ||
| println!("Prepared spells: {prepared}/{cap}"); | ||
| if !r.always_active.is_empty() { | ||
| println!("Always prepared: {}", r.always_active.join(", ")); | ||
| } | ||
| if !r.prepared_items.is_empty() { | ||
| println!("Prepared: {}", r.prepared_items.join(", ")); | ||
| } else { | ||
| println!("Prepared: (none)"); | ||
| } | ||
| println!("Eligible pool: {} spell(s)", r.eligible_items.len()); | ||
| } | ||
| Some(mode) => { | ||
| println!("Preparation mode: {mode}"); |
There was a problem hiding this comment.
Inconsistent display labels after field rename
The display output still refers to 'spells' in user-facing messages, but the code now operates on 'items' (which may include non-spells). Similarly, 'Always prepared:' doesn't match the always_active field semantics. This could confuse users expecting spell-specific output.
Code suggestion
Check the AI-generated fix before applying
- "All {} spell(s) prepared (no manual preparation needed).",
+ "All {} item(s) prepared (no manual preparation needed).",
r.prepared_items.len()
);
if !r.always_active.is_empty() {
- println!("Always prepared: {}", r.always_active.join(", "));
+ println!("Always active: {}", r.always_active.join(", "));
}
if !r.prepared_items.is_empty() {
println!("Prepared: {}", r.prepared_items.join(", "));
}
}
Some("prepared") => {
let prepared = r.prepared_items.len();
let cap = r.cap.unwrap_or(0);
- println!("Prepared spells: {prepared}/{cap}");
+ println!("Prepared items: {prepared}/{cap}");
if !r.always_active.is_empty() {
- println!("Always prepared: {}", r.always_active.join(", "));
+ println!("Always active: {}", r.always_active.join(", "));
}
if !r.prepared_items.is_empty() {
println!("Prepared: {}", r.prepared_items.join(", "));
} else {
println!("Prepared: (none)");
}
- println!("Eligible pool: {} spell(s)", r.eligible_items.len());
+ println!("Eligible pool: {} item(s)", r.eligible_items.len());
}
Some(mode) => {
println!("Preparation mode: {mode}");
}
Code Review Run #0b27eb
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| pub(crate) preparation_mode: Option<String>, | ||
| pub(crate) cap: Option<i64>, | ||
| #[serde(default)] | ||
| pub(crate) eligible_items: Vec<String>, |
There was a problem hiding this comment.
API Key Mismatch
The field rename from 'always_prepared' to 'always_active' changes the expected JSON key, but the Elixir backend in characters.ex still returns a map with 'always_prepared'. This will cause serde deserialization to fail or produce incorrect empty vectors, as the CLI expects 'always_active' but receives 'always_prepared' from the engine.
Code Review Run #0b27eb
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
apply_activation for add_remove management was removing all items of the given concept_type from inventory, then replacing with only the newly activated item_ids. Items outside the eligible pool — primarily cantrips (level 0), which are excluded from the add_remove eligible pool by the level >= 1 filter — were silently deleted every time a Cleric, Druid, or Paladin prepared leveled spells. Fix: thread the computed eligible list through to apply_activation and only remove items whose concept_id is in that set. Items outside the eligible pool (cantrips, equipment in a mixed-type inventory, etc.) are preserved unchanged. The toggle_field clause receives _eligible to keep the call site uniform.
The only existing activate/4 test that reached apply_activation used the toggle_field (Wizard spellbook) path. The add_remove path — used by Cleric, Druid, and Paladin — was entirely untested, which is why the cantrip wipe bug went unnoticed. Add a test that builds a Cleric character with a sacred_flame cantrip already in inventory, prepares bless via add_remove, and asserts: - spell inventory contains exactly sacred_flame and bless (cantrip preserved, bless added, cure_wounds absent from inventory entirely) - all retained spell items have prepared: true Also adds the class_spells pool to the shared spell_inv_rules fixture so the add_remove management path is available to all activate/4 tests.
…remove) The previous flat_map silently dropped failed item creations with _ -> []. A player could request N spells but have fewer created without any error returned, leaving inventory in a partially-prepared state with no indication of what went wrong. Replace with Enum.reduce_while so the first creation failure short-circuits and the error is returned from apply_activation to activate/4's caller.
prep_always_prepared/3, find_subclass_choice/4, and filter_within_level/3 had no test coverage despite always-prepared being a visible feature for Life Domain (Cleric) and Oath of Devotion (Paladin). Add a test with a Cleric + Life Domain subclass decision where the subclass declares three always-prepared spells: bless and cure_wounds (level 1) and hold_person (level 2). With max_spell_level capped at 1, only bless and cure_wounds should appear in always_prepared — confirming both subclass resolution and level filtering work correctly. Also adds the missing always_prepared section to the shared spell_inv_rules fixture so the subclass choice and metadata key are wired up for all preparation_state/3 tests.
Without deduplication, passing ["bless", "bless"] counted 2 against the cap and created two identical inventory items via add_remove. Enum.uniq is applied after pool computation (which is unaffected by duplicates) and before the eligibility and cap checks so both validations see the true unique count.
…s handler
The handler takes [{type_id, _} | _] from preparation_types/1, silently
using only the first. Adding support for multiple types requires a
protocol change on both sides (Elixir response shape and Rust
PreparationStateResponse struct). dnd_5e_srd has one preparation type
so the current behaviour is correct; the comment records what would need
to change if a second type were added.
…de_not_prepared The previous message "spells for this class are not manually prepared" hardcoded spell-specific language in a function shared by all inventory types. Binding the mode value and using it in a generic message makes the error accurate for any type (e.g. equipment with mode "all") and more informative — the player sees which mode blocked the activation.
The single test "returns error, nil mode, and structured state across cases" packed three unrelated assertions together. A failure in any one gave no indication of which scenario broke. Split into three focused tests: - "returns error for unknown inventory type" - "returns mode nil when character has no class with preparation config" - "returns full preparation state for a character with a prepared-mode class" The DAG setup is moved into the third test only, where it is actually needed. The first two need nothing beyond spell_system() and minimal_character([]).
…tory The resolve_choice handler was updated to call apply_inventory_addition!/4 which delegates to Characters.add_to_typed_inventory/4, but there was no test proving the wiring is correct at the server level. Add a describe block that builds a Wizard character (deterministic class) via the build flow and then resolves spell choices through characters.resolve_choice: - Resolving a cantrip choice adds the spell to inventory with prepared: true (cantrips are auto-activated via add_on_progression) - Resolving a spells_known choice adds the spell to inventory with prepared: false (manual preparation required) Both tests then read characters.inventory to confirm the item and its prepared field, exercising the full resolve_choice → inventory path.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
apply_activation for management: "toggle_field" was mapping all items of the given concept type and setting their activation field based on item_ids. Items outside the eligible pool (e.g. cantrips, excluded from the spellbook pool because level 0 is not in 1..max_level) can never appear in item_ids, so they were always written to false. The add_remove path handled this correctly via an explicit other_items pass that preserves non-eligible items. toggle_field now does the same by building an eligible_set and only rewriting items whose concept_id is a member of it. Adds a regression test: wizard with a cantrip (prepared: true) and a spells_known spell (prepared: false); after activate(["magic_missile"]) the cantrip must remain true. Also extracts @fighter_effect and @spend_xp_progression module attributes in characters_test.exs to eliminate copy-pasted inline maps across six similar test functions.
require_prepared_mode_for_activate was hardcoding the string "prepared" in library source, which is a system-specific metadata value. This violates the library/system boundary: if a rule system used a different string to mean "manually activate spells", the library would break. Adds activation_mode to PreparationConfig (struct + TOML parser) and threads it through to require_prepared_mode_for_activate/2, which now compares the character's mode against the configured value rather than the literal "prepared". Adds activation_mode = "prepared" to dnd_5e_srd inventory_rules.toml. Updates test fixtures in characters_test and inventory_rules_test to include the new field.
… time
"add_remove" and "toggle_field" are library-defined behavioral variants,
not arbitrary system data. Storing them as opaque strings meant dispatch
was stringly-typed with a silent _ -> [] fallthrough that would swallow
misconfigured management values without any error.
parse_management/1 now validates and converts to an atom at parse time,
returning {:error, {:unknown_pool_management, other}} for unknown values.
parse_pool_config/1 and parse_pools/1 extract the pool parsing path and
thread errors upward properly (previously parse_preparation always
returned {:ok, _}).
compute_eligible_pool and apply_activation now dispatch on atoms; the
_ -> [] fallthrough in compute_eligible_pool is removed since unknown
strategies are rejected at load time.
prep_always_prepared previously found the character's subclass decision for the relevant class and read always_prepared_metadata_key from that one concept's metadata. This only supported one source (subclass) and required two config fields (always_prepared_subclass_choice + always_prepared_metadata_key) to identify that single concept. The general pattern is: any active concept can contribute to the always-prepared list by having the configured metadata key present. This naturally handles subclasses (already active via active_concepts), racial traits, feats, or any other concept the character has resolved. Replaces the find_subclass_choice lookup with an active_concepts scan: each active concept is checked for the metadata key and any lists found are flat-mapped and level-filtered. Removes always_prepared_subclass_choice from PreparationConfig struct, parser, and TOML. Test fixtures that use the subclass path need the class concept_metadata to declare its "choices" key so that active_concepts follows the subclass decision — the real dnd_5e_srd class TOML does this; the minimal test maps were missing it. Adds a test for always_prepared accumulating from a non-subclass concept (race with always_prepared metadata key).
The field was parsed and stored but never read by any library code. It was intended to mark progression items (e.g. cantrips) as exempt from the preparation cap, but the cap-exemption is already enforced by the eligible-pool level filter: add_remove_eligible and toggle_field_eligible both gate on level in 1..max_level, so level-0 items (cantrips) cannot appear in item_ids and therefore cannot consume cap regardless. A field that is parsed, stored, and has no observable effect on behaviour is worse than not having it. If a future system needs explicit cap exemption beyond what the level filter provides, it can be added then with a real implementation.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cap is None only when the cap node doesn't exist in the resolved system at all — a TOML config error, not a legitimate zero. unwrap_or(0) was silently displaying "Prepared spells: N/0", which looks like a valid state. "?" makes the misconfiguration visible.
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.
Summary
prepared_spellsfield toCharacter, persisted to JSONpreparation_mode,preparation_pool, andpreparation_capmetadata to Cleric, Druid, Paladin, and Wizard TOML configsalways_preparedspell lists for Life Domain (Cleric) and Oath of Devotion (Paladin) subclassescharacters.spellsandcharacters.prepareserver protocol commands (now via genericcharacters.activate)characters spells <slug>andcharacters prepare <slug> <spell...>CLI commandscharacters show)Inventory generalisation (part of this branch)
The spell prep implementation was immediately followed by a refactor that generalises the inventory system:
inventory_rules.tomlredesigned to declare per-type schemas, activation verbs, andadd_on_progressionconfigCharacters.activate/4andCharacters.add_to_typed_inventory/4replace the spell-prep-specific functions — all code paths are config-driven, no system-specific strings in library or CLIcharacters.activateserver handler replacescharacters.prepare— any TOML-declared inventory type with a matchingactivate_commandis reachable without new server codehandle_activatein the Rust CLI replaceshandle_characters_prepare— adding a new inventory type requires only a TOML section, no new RustSpellPrepmodule deleted;prepared_spellsremoved fromCharacterand migrated into the typed spell inventoryCircle of the Land is left without an
always_preparedlist — its bonus spells are terrain-dependent and no terrain sub-choice exists yet. School of Evocation has no always-prepared spells in the SRD.Closes #93.
Test plan
mix test --umbrellapassescargo testpasses (unit tests)characters build dnd_5e_srd→ build a Cleric (Life Domain) →characters spells <slug>shows prepared mode, cap, eligible poolcharacters prepare <slug> bless cure_wounds→ confirms active count;characters spellsreflects the changecharacters show <slug>includes prepared spells sectioncharacters spellsoutput for Life Domain and Oath of Devotion charactersprepared_spellsmigrates correctly to typed spell inventorySummary by Bito
This PR implements spell preparation mechanics for D&D 5e SRD spellcasting classes, allowing characters to prepare spells up to their class-specific caps from eligible pools, with support for always-prepared spells for certain subclasses. It also refactors the inventory system to be config-driven, enabling easy addition of new inventory types without code changes. The implementation includes CLI commands, server protocol updates, and comprehensive tests. The latest changes rename protocol fields to generic terms and adjust class configurations for consistency.
Detailed Changes