fix(character): correct off-by-one in spell-slot conversion by Mnehmos · Pull Request #54 · Mnehmos/mnehmos.rpg.mcp

Mnehmos · 2026-04-18T05:59:04Z

Summary

convertSpellSlotsToObject read slots[1] for level1, slots[2] for level2, etc., treating the array as 1-indexed. But getSpellSlots returns a zero-indexed array (slots[0] = level-1 slot count). This made half-casters look slot-less and shifted every full caster's reported slots one level too low.

Effect (live session evidence)

Paladin L4: should have 3 level-1 slots → was reporting level1: 0/0. Tried to cast Bless → "No level 1+ spell slots available."
Wizard L4: should have 4/3 → was reporting 3/0.
Cleric L4: same as wizard.

Fix

Index the array from 0. No other touch points — the array shape was always zero-indexed (verified in src/data/class-starting-data.ts).

Test plan

New regression test asserts correct level1/level2/level3 max for wizard/cleric/paladin/ranger at L1/L2/L4/L5.
character-manage suite: 39/39 pass (was 38/38).
Full suite: 1890 passed, 6 skipped, 0 failed.

Closes #44

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Corrected spell-slot indexing so slot counts map properly to levels 1–9 and ensured spell slots are recomputed and returned when a character levels up.
Tests
- Added regression tests covering full- and half-caster classes across levels 1–9 and verifying spell-slot values before and after level-up transitions.

coderabbitai · 2026-04-18T05:59:18Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0d7c930d-bb17-45d1-8392-7b5b19951fb8

📥 Commits

Reviewing files that changed from the base of the PR and between b55e453 and 528d5b6.

📒 Files selected for processing (2)

src/server/consolidated/character-manage.ts
tests/server/consolidated/character-manage.test.ts

✅ Files skipped from review due to trivial changes (1)

tests/server/consolidated/character-manage.test.ts

📝 Walkthrough

Walkthrough

Recomputes and returns spell slot data when a spellcaster levels up by calling getSpellSlots(className, targetLevel) and converting the result; adds tests validating spell-slot seeding and level-up behavior for full- and half-casters across levels 1–9.

Changes

Cohort / File(s)	Summary
Character manage logic `src/server/consolidated/character-manage.ts`	On level-up for spellcasters, call `getSpellSlots(className, targetLevel)` and `convertSpellSlotsToObject(...)` to recompute `updates.spellSlots`; include `spellSlots` in the level-up response.
Regression tests `tests/server/consolidated/character-manage.test.ts`	Added tests that assert create-time seeding and level-up recomputation of spell slots for Wizard, Cleric (full-casters) and Paladin (half-caster) across levels 1–9 and specific level-up transitions (e.g., Wizard 4→5, Paladin 1→2).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Paladin spell slots not seeded on character_manage create #44: Paladin spell slots not seeded on character_manage create — Fixes the off-by-one/missing seed behavior causing paladin (and other casters) to get zeroed slot fields on create.
Spell slot array off-by-one: all casters get 0 slots at creation #31 — Related report of convertSpellSlotsToObject reading incorrect array index; this change addresses that same mapping concern.

Possibly related PRs

fix(character): insert character row before provisioning to satisfy inventory FK #55 — Modifies/relies on convertSpellSlotsToObject behavior; strongly related to the slot-indexing and creation/level-up flow adjustments in this PR.

Poem

🐇 I hopped through indexes, fixed each little one,
Recomputed slots till the spellwork was done.
Wizards, clerics, paladins cheer —
Level-ups now bring slots near! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(character): correct off-by-one in spell-slot conversion' directly describes the core bug fix—an off-by-one indexing error in spell slot conversion that caused incorrect slot counts.
Linked Issues check	✅ Passed	The PR implements spell-slot table lookup on character create and level_up for both full-casters (wizard, cleric) and half-casters (paladin, ranger), with comprehensive regression tests covering levels 1–9 for all three classes, meeting all acceptance criteria from issue `#44`.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the spell-slot off-by-one bug and adding corresponding regression tests; no unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/44-paladin-spell-slots

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/server/consolidated/character-manage.ts (1)

152-155: ⚠️ Potential issue | 🟡 Minor

Fix the stale slot-indexing docblock.

Line 152–155 still documents a shifted array format ([0, slots1, slots2, ...]), which now contradicts Line 159–170 and the getSpellSlots contract. This is misleading and can reintroduce the same bug later.

📝 Suggested doc fix

-/**
- * Convert spell slots from array format [0, slots1, slots2, ...] to object format
- * The array format from provisionStartingEquipment has index 0 as cantrips (unused here),
- * and indices 1-9 as spell levels 1-9.
- */
+/**
+ * Convert spell slots from array format [level1, level2, ..., level9]
+ * to object format { level1..level9 }.
+ * `getSpellSlots` is zero-indexed: slots[0] = level1, slots[1] = level2, etc.
+ */

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/consolidated/character-manage.ts` around lines 152 - 155, The
docblock above getSpellSlots incorrectly describes the spell-slot array as
starting with a dummy 0 (e.g., "[0, slots1, slots2,...]"); update that comment
to match the actual contract used by getSpellSlots/provisionStartingEquipment
(i.e., the array indexes correspond directly to spell levels or explicitly state
which index maps to cantrips vs level 1–9), remove the "shifted" wording and
clearly state which index contains cantrips (if any) and that indices 1–9 map to
levels 1–9 so the documentation matches the implementation and avoids future
off-by-one assumptions.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/server/consolidated/character-manage.test.ts`:
- Around line 103-133: The test currently samples a few levels but misses full
regression coverage; expand the test in character-manage.test.ts to iterate
Paladin, Wizard, and Cleric across levels 1..9 (verify
parsed.spellSlots.level1/level2/level3 max/current against expected per class
table) and add separate assertions exercising the level_up code path by calling
handleCharacterManage with action: 'level_up' (use same name/class and
incremented level) then re-parse and assert spellSlots updated correctly; locate
and modify the loop and cases near handleCharacterManage and the expectations to
generate programmatic cases for levels 1-9 and include a level_up invocation for
at least one representative character per class.

---

Outside diff comments:
In `@src/server/consolidated/character-manage.ts`:
- Around line 152-155: The docblock above getSpellSlots incorrectly describes
the spell-slot array as starting with a dummy 0 (e.g., "[0, slots1,
slots2,...]"); update that comment to match the actual contract used by
getSpellSlots/provisionStartingEquipment (i.e., the array indexes correspond
directly to spell levels or explicitly state which index maps to cantrips vs
level 1–9), remove the "shifted" wording and clearly state which index contains
cantrips (if any) and that indices 1–9 map to levels 1–9 so the documentation
matches the implementation and avoids future off-by-one assumptions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 861fa891-9d32-49ce-891b-8cae70bff0e6

📥 Commits

Reviewing files that changed from the base of the PR and between 2d9ddb9 and ca546ec.

📒 Files selected for processing (2)

src/server/consolidated/character-manage.ts
tests/server/consolidated/character-manage.test.ts

Reviewers (PR #54) asked for L1-L9 coverage and a level_up assertion. Investigating revealed handleLevelUp never recomputed spell slots, so a wizard hitting L5 wouldn't gain their first 3rd-level slot, and a paladin hitting L2 stayed at zero. Same bug class as #44. - handleLevelUp: when the class is a spellcaster, recompute slots via getSpellSlots + convertSpellSlotsToObject and persist them. - Tests: parameterized create checks for Wizard/Cleric/Paladin at every level 1-9 against PHB tables (full and half caster). - Tests: level_up Wizard L4 -> L5 grows level1=4 / level2=3 / level3=2 and Paladin L1 -> L2 picks up two level-1 slots. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

…lot bug Reviewers (PR #55) flagged that this branch reorders character creation so provisioning succeeds, but it still uses the off-by-one convertSpellSlotsToObject helper. Merged standalone, every wizard would persist with level1=3 (should be 4) and every paladin with level1=0 (should be 3). Apply the same zero-indexing fix that lives in PR #54 here so #55 is correct on its own merge boundary. Tests assert wizard L4 = 4/3 and paladin L4 = 3 after the reorder. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/server/consolidated/character-manage.ts`:
- Around line 435-436: The code uses a non-existent fallback property char.class
causing a type error; change the logic to rely only on char.characterClass
(e.g., replace the declaration of className and the isSpellcaster check so they
use char.characterClass directly) and remove any reference to char.class; ensure
callers like the const className assignment and the isSpellcaster(className)
call reference only characterClass to match the Character | NPC types.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 649a9c37-ec78-4754-bd05-d52c74a61691

📥 Commits

Reviewing files that changed from the base of the PR and between ca546ec and 130c9be.

📒 Files selected for processing (2)

src/server/consolidated/character-manage.ts
tests/server/consolidated/character-manage.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

tests/server/consolidated/character-manage.test.ts

CI on PR #54 failed with TS2339 because Character only exposes characterClass — char.class doesn't exist on the type. The defensive fallback was unnecessary; the schema is consistent. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

…ot indexing (#55) * fix(character): insert character row before provisioning to satisfy inventory FK handleCreate built the provisioning result first, then assembled the character record, then inserted it. The provisioner writes to inventory_items immediately, and that table's character_id column has a FOREIGN KEY → characters(id). Since the character row didn't exist yet, every inventory insert failed and surfaced as "Failed to grant <Item>: FOREIGN KEY constraint failed" — every PC spawned with an empty inventory. Fix: - Insert the character row first (with whatever args supplied for spell fields, defaulted otherwise). - Run provisioning second — FK now resolves, inventory grants succeed. - Update the character row with knownSpells/cantripsKnown/spellSlots/ pactMagicSlots from the provisioning result. Adds a regression test that creates one character per major class and asserts _provisioning.errors stays unset and equipmentGranted is non-empty. Closes #45 Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * fix(character): bundle spell-slot index fix so #55 doesn't ship the slot bug Reviewers (PR #55) flagged that this branch reorders character creation so provisioning succeeds, but it still uses the off-by-one convertSpellSlotsToObject helper. Merged standalone, every wizard would persist with level1=3 (should be 4) and every paladin with level1=0 (should be 3). Apply the same zero-indexing fix that lives in PR #54 here so #55 is correct on its own merge boundary. Tests assert wizard L4 = 4/3 and paladin L4 = 3 after the reorder. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>

Reviewers (PR #54) asked for L1-L9 coverage and a level_up assertion. Investigating revealed handleLevelUp never recomputed spell slots, so a wizard hitting L5 wouldn't gain their first 3rd-level slot, and a paladin hitting L2 stayed at zero. Same bug class as #44. - handleLevelUp: when the class is a spellcaster, recompute slots via getSpellSlots + convertSpellSlotsToObject and persist them. - Tests: parameterized create checks for Wizard/Cleric/Paladin at every level 1-9 against PHB tables (full and half caster). - Tests: level_up Wizard L4 -> L5 grows level1=4 / level2=3 / level3=2 and Paladin L1 -> L2 picks up two level-1 slots. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

CI on PR #54 failed with TS2339 because Character only exposes characterClass — char.class doesn't exist on the type. The defensive fallback was unnecessary; the schema is consistent. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

coderabbitai Bot reviewed Apr 18, 2026

View reviewed changes

Comment thread tests/server/consolidated/character-manage.test.ts Outdated

coderabbitai Bot reviewed Apr 19, 2026

View reviewed changes

Comment thread src/server/consolidated/character-manage.ts Outdated

Mnehmos and others added 2 commits April 20, 2026 07:28

Mnehmos force-pushed the fix/44-paladin-spell-slots branch from b55e453 to 528d5b6 Compare April 20, 2026 14:28

Mnehmos merged commit 135218e into main Apr 20, 2026
4 checks passed

Mnehmos deleted the fix/44-paladin-spell-slots branch April 20, 2026 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(character): correct off-by-one in spell-slot conversion#54

fix(character): correct off-by-one in spell-slot conversion#54
Mnehmos merged 2 commits intomainfrom
fix/44-paladin-spell-slots

Mnehmos commented Apr 18, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 18, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Mnehmos commented Apr 18, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Effect (live session evidence)

Fix

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Mnehmos commented Apr 18, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 18, 2026 •

edited

Loading