Repository navigation
fix(math): document registered check tool - #106
Conversation
|
✅ Health of changed files: 6.9 (unchanged) 📋 At a glance Files & modules (2)
✅ Health gate: passed 🔎 More signals (3)🗺️ Change map flowchart LR
subgraph PR ["Changed in this PR (1 with dependents)"]
f_src_server_consolidated_math_manage_ts[".../consolidated/math-manage.ts 🔥"]:::changed
end
f_src_server_consolidated_index_ts[".../consolidated/index.ts"]
f_src_server_consolidated_math_manage_ts --> f_src_server_consolidated_index_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 (1 direct dependent, from the last indexed snapshot). Dashed: history/tests. 🔥 Hotspots touched (2)
💀 Dead code (10 findings)
7 more
📊 See the full report for this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change updates RPG check routing guidance in ChangesRPG Check Routing
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This change only corrects guidance for invoking an existing check tool and removes references to nonexistent tools; no runtime behavior or production state changes. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 032fa8fe2d
ℹ️ 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".
| - Saving throws → Use the combat/effect tool that requires the save; there is no standalone roll_saving_throw tool on this MCP surface | ||
|
|
||
| These specialized tools look up character stats and apply bonuses automatically! | ||
| The DM chooses the appropriate skill and DC, and the stunt action rolls the d20 and applies the character's skill modifier automatically. Do not invent or call roll_skill_check, roll_ability_check, or roll_saving_throw; those tools are not registered here. |
There was a problem hiding this comment.
When a character is proficient or has expertise in the selected skill, this guidance produces an incorrect check total: getSkillModifier in improvisation-manage.ts derives only the associated ability modifier and handleStunt never reads the persisted skillProficiencies or expertise fields. Redirecting general skill checks here while claiming the character's skill modifier is applied will therefore silently omit proficiency and expertise bonuses; either add those bonuses to stunt resolution or avoid presenting it as the general skill-check replacement.
Useful? React with 👍 / 👎.
| - Skill checks → Use roll_skill_check (auto-applies proficiency) | ||
| - Ability checks → Use roll_ability_check (auto-applies modifier) | ||
| - Saving throws → Use roll_saving_throw (auto-applies save proficiency) | ||
| - Ability or skill checks → Use improvisation_manage { action: "stunt", actorId, skill, dc, effectType: "none" } |
There was a problem hiding this comment.
When the DM needs a raw Constitution ability check, this redirect cannot represent it: StuntSchema requires one of the 18 skills, and SKILL_TO_ABILITY maps none of them to Constitution, so passing constitution is rejected and choosing an allowed skill uses a different ability. The blanket ability-check guidance should point to a route that accepts an ability or be narrowed until stunt supports raw ability checks.
Useful? React with 👍 / 👎.
Risk
R1 (tool documentation only); no runtime state, auth, billing, secrets, or migration changes.
Scope
Correct
math_manageguidance so the DM is pointed at the tool that actually exists for non-combat ability and skill checks:improvisation_managewithaction: "stunt". Remove references to nonexistent standalone roll tools and clarify that saves are resolved by the applicable combat/effect tool.Acceptance criteria
roll_skill_check,roll_ability_check, orroll_saving_throwtools.Verification
npm test -- tests/server/consolidated/math-manage.test.ts tests/server/consolidated/improvisation-manage.test.ts(75 tests)npm test -- --reporter=basic(2,276 passed, 7 skipped)npm run buildgit diff --checkPersistence or migration
None.
Rollback
Revert the squash commit.
Summary by CodeRabbit
Bug Fixes
Tests