Repository navigation
Support cantrip selection in character management - #96
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCharacter creation and update schemas now support ChangesCantrip persistence
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Health of changed files: 4.8 (unchanged) 📋 At a glance Files & modules (2)
✅ Health gate: passed 📌 Before you merge
🔎 More signals (3)🗺️ Change map flowchart LR
subgraph PR ["Changed in this PR (1 with dependents)"]
f_src_server_consolidated_character_manage_ts[".../consolidated/character-manage.ts 🔥"]:::changed
end
f_scripts_seed_bastion_ts["scripts/seed-bastion.ts"]
f_src_server_consolidated_character_manage_ts --> f_scripts_seed_bastion_ts
f_src_server_consolidated_batch_manage_ts[".../consolidated/batch-manage.ts"]
f_src_server_consolidated_character_manage_ts --> f_src_server_consolidated_batch_manage_ts
f_src_server_consolidated_index_ts[".../consolidated/index.ts"]
f_src_server_consolidated_character_manage_ts --> f_src_server_consolidated_index_ts
f_src_server_consolidated_npc_manage_ts[".../consolidated/npc-manage.ts"]
f_src_server_consolidated_character_manage_ts --> f_src_server_consolidated_npc_manage_ts
t_tests_server_consolidated_character_origin_test_ts(["✅ .../consolidated/character-origin.test.ts"]):::guard
t_tests_server_consolidated_character_origin_test_ts -.-> f_src_server_consolidated_character_manage_ts
classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d
Solid arrows: code that imports the changed files (4 direct dependents, from the last indexed snapshot). Dashed: history/tests. 🔥 Hotspots touched (2)
💀 Dead code (10 findings)
7 more
📊 See the full report for this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cc877e914
ℹ️ 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".
| characterType: CharacterTypeSchema.optional().default('pc'), | ||
| factionId: z.string().optional(), | ||
| behavior: z.string().optional(), | ||
| cantripsKnown: z.array(z.string()).optional().default([]), |
There was a problem hiding this comment.
When character_manage is invoked through the MCP server, withSession first parses arguments with CharacterManageTool.inputSchema, which does not declare cantripsKnown; Zod therefore strips the field before the router reaches this action-specific schema. Consequently, both create and update selections are silently ignored in production even though the direct-handler test passes. Add cantripsKnown to the shared registered input schema as well.
Useful? React with 👍 / 👎.
| character.cantripsKnown = args.cantripsKnown?.length | ||
| ? [...new Set(args.cantripsKnown)] | ||
| : provisioningResult.cantripsGranted || []; |
There was a problem hiding this comment.
When a caster is created with provisioning enabled and explicit cantrips, the provisioning call receives customSpells but not customCantrips, so provisioningResult.cantripsGranted still contains the class defaults. These lines persist the explicit selection, but the same response reports the defaults under _provisioning.cantripsGranted, giving downstream consumers a contradictory provisioning receipt. Pass the selection as customCantrips so the result and persisted character agree.
Useful? React with 👍 / 👎.
What changed
cantripsKnowntocharacter_managecreate and update schemasWhy
The hosted spellbook editor needs an authoritative engine write path for player-selected cantrips. The engine already persisted cantrips, but its consolidated API silently dropped create/update selections.
Validation
npm run buildnpx vitest run tests/server/consolidated/character-manage.test.ts --pool=forks(71 passed)Closes the engine-side dependency for rpg-mcp-live issue #254.
Summary by CodeRabbit
New Features
Bug Fixes