fix(combat): accept compound dice notation like "1d6+4+2d6" by Mnehmos · Pull Request #51 · Mnehmos/mnehmos.rpg.mcp

Mnehmos · 2026-04-18T05:48:09Z

Summary

CombatRNG.roll and rollDamageDetailed previously matched a single-term regex, rejecting standard compound expressions (sneak attack 1d6+4+2d6, divine smite 1d8+3+2d8).
Added a shared parseDiceTerms helper that splits on +/- preserving signs and evaluates each term as either dice or a scalar.
Supports shorthand dN and mixed dice/subtraction (2d6+1d4-2).

Test plan

5 new RNG tests (sneak attack, smite, mixed +/-, shorthand, detailed-roll structure)
Full suite: 1894 passed, 6 skipped, 0 failed
tsc --noEmit clean

Closes #42

🤖 Generated with Claude Code

Summary by CodeRabbit

New Features
- Dice notation now supports chained expressions with multiple dice terms and modifiers (e.g., 1d6+4+2d6, 2d6+1d4-2, d20+5).
- Damage reporting now returns detailed breakdowns: per-die rolls, subtotal of dice, summed modifiers, and final total.
Bug Fixes
- Invalid or malformed operator sequences in dice notation are now rejected with an error.
Tests
- Expanded test coverage for compound notation, shorthand dice, and regression cases.

CombatRNG.roll and rollDamageDetailed previously matched a single-term regex (NdS+M), rejecting standard compound expressions used by sneak attack (1d6+4+2d6) and divine smite (1d8+3+2d8). Adds a parseDiceTerms helper that splits on +/- preserving signs and evaluates each term as either dice or a scalar. Supports shorthand "dN" and mixed dice/subtraction (2d6+1d4-2). Both roll() and rollDamageDetailed() now share the parser. Closes #42 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-04-18T05:48:25Z

📝 Walkthrough

Walkthrough

Extended the combat RNG to parse and evaluate compound dice expressions containing multiple dice terms and integer scalars joined by +/- (e.g., 1d6+4+2d6, 2d6+1d4-2, d20). Added a strict term parser and updated roll functions to accumulate signed term results.

Changes

Cohort / File(s)	Summary
Dice Parser & RNG `src/engine/combat/rng.ts`	Added exported `parseDiceTerms(notation: string)` and internal `DiceTerm` handling; expanded `CombatRNG.roll()` and `rollDamageDetailed()` to handle multi-term `NdS`/`dS` dice and scalar chains with `+`/`-`, enforce strict grammar, and accumulate dice totals and modifiers.
Tests `tests/combat/rng.test.ts`	Added tests for compound expressions (`1d6+4+2d6`, `1d8+3+2d8`, `2d6+1d4-2`), shorthand (`d20`), `rollDamageDetailed()` structure checks, and regression cases ensuring malformed operator sequences throw `Invalid dice notation`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰🎲
I twitch my nose at numbered fate,
I parse the rolls, I count the weight.
From 1d6+4+2d6 to d20 bright,
Each signed small hop now sums just right.
Hooray — the rabbit crowns the fight!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: accepting compound dice notation with multiple terms like '1d6+4+2d6'.
Linked Issues check	✅ Passed	The implementation addresses all coding requirements from issue `#42`: multi-term dice parsing (1d6+4+2d6, 1d8+3+2d8), mixed +/- operations (2d6+1d4-2), and strict grammar validation for malformed inputs.
Out of Scope Changes check	✅ Passed	Changes are limited to src/engine/combat/rng.ts and tests/combat/rng.test.ts, directly implementing the dice parsing requirements. The PR updates combat dice rolling and adds comprehensive tests without unrelated modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ 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/42-dice-notation-compound

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

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c6bcbb5d0

ℹ️ 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".

chatgpt-codex-connector · 2026-04-18T05:50:11Z

+        throw new Error(`Invalid dice notation: ${notation}`);
+    }
+    // Split on +/- while preserving the sign with each term.
+    const tokens = stripped.match(/[+-]?[^+-]+/g);


Reject malformed operator sequences in dice notation

Using stripped.match(/[+-]?[^+-]+/g) tokenizes only valid chunks and ignores unmatched separator characters, so malformed inputs like 1d6++2, 1d6--2, or 1d6- are now accepted and evaluated instead of throwing Invalid dice notation. This is a regression from the previous anchored parser and can silently change intended combat outcomes when users mistype formulas, because typos are normalized into different valid rolls.

Useful? React with 👍 / 👎.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/combat/rng.test.ts (1)

58-97: Add invalid-compound regression cases for parser strictness.

This block validates happy paths well, but it should also pin malformed chains so the parser can’t silently accept bad expressions (e.g., trailing/consecutive operators).

✅ Suggested test additions

+        it('should reject malformed compound expressions', () => {
+            const rng = new CombatRNG('compound-invalid');
+            expect(() => rng.roll('1d6+')).toThrow('Invalid dice notation');
+            expect(() => rng.roll('1d6+-2')).toThrow('Invalid dice notation');
+            expect(() => rng.roll('1d6--2')).toThrow('Invalid dice notation');
+        });

🤖 Prompt for AI Agents

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

In `@tests/combat/rng.test.ts` around lines 58 - 97, Add regression tests for
malformed compound expressions so the parser fails fast: in
tests/combat/rng.test.ts add cases that call new
CombatRNG('invalid-...').roll(...) and rollDamageDetailed(...) with inputs like
"1d6+", "+1d6", "1d6++2d6" (and other trailing/consecutive/leading-operator
patterns) and assert the parser rejects them (expect a thrown error or a
rejected parse result consistent with existing error behavior of CombatRNG.roll
and CombatRNG.rollDamageDetailed). Refer to CombatRNG.roll and
CombatRNG.rollDamageDetailed to implement each assertion and mirror the style of
existing tests for error expectations.

🤖 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/engine/combat/rng.ts`:
- Around line 349-352: The tokenization currently uses stripped.match(...)
without rejecting malformed operator chains; before calling stripped.match, add
a strict pre-check on stripped (used with notation and tokens) that throws for
invalid operator sequences such as any adjacent operator pair or a trailing
operator (e.g. if (/[+\-]{2,}/.test(stripped) || /[+\-]$/.test(stripped)) throw
new Error(`Invalid dice notation: ${notation}`)); keep allowing a single leading
sign but reject inputs like "1d6+", "1d6+-2", or "1d6--2" so tokens =
stripped.match(...) only runs after this validation.

---

Nitpick comments:
In `@tests/combat/rng.test.ts`:
- Around line 58-97: Add regression tests for malformed compound expressions so
the parser fails fast: in tests/combat/rng.test.ts add cases that call new
CombatRNG('invalid-...').roll(...) and rollDamageDetailed(...) with inputs like
"1d6+", "+1d6", "1d6++2d6" (and other trailing/consecutive/leading-operator
patterns) and assert the parser rejects them (expect a thrown error or a
rejected parse result consistent with existing error behavior of CombatRNG.roll
and CombatRNG.rollDamageDetailed). Refer to CombatRNG.roll and
CombatRNG.rollDamageDetailed to implement each assertion and mirror the style of
existing tests for error expectations.

🪄 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: de018991-59c2-449c-bacc-02db84407b0c

📥 Commits

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

📒 Files selected for processing (2)

src/engine/combat/rng.ts
tests/combat/rng.test.ts

Reviewers (PR #51) flagged that the original tokenization regex silently accepted inputs like "1d6++2", "1d6--2", "1d6+", or trailing operators — the regex matched valid chunks and skipped junk. That could change combat outcomes when callers fat-fingered an expression. Adds a strict grammar guard before tokenization: <term> ( [+-] <term> )* with at most one leading sign. Adds a parameterized test for 10 malformed inputs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

coderabbitai

🧹 Nitpick comments (1)

src/engine/combat/rng.ts (1)
287-291: Signed roll values may affect downstream display formatting.

When dice terms are subtracted (e.g., 2d6-1d4), the rolls array will contain negative values. The caller in engine.ts:577 uses dmgResult.rolls.join('+'), which would produce output like (3+-2+5) for mixed signs.

This is semantically correct for the new compound notation support, but the display logic in engine.ts may need adjustment if subtracted dice terms are expected to be common.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/engine/combat/rng.ts` around lines 287 - 291, The rolls array currently
receives signed numeric values (term.sign * r) which causes join('+') in
engine.ts to produce strings like "3+-2"; change the roll representation so sign
and magnitude are preserved separately: instead of pushing signed numbers into
rolls within the loop that calls rollDie(term.sides), push either an
object/tuple {value: r, sign: term.sign} (or [r, term.sign]) or push the
absolute value into rolls and store term.sign in a parallel signs array; update
consumers (e.g., the code referenced at engine.ts:577 that does
dmgResult.rolls.join('+')) to format each entry as (sign === -1 ? '-' : '') +
value when building the display string. Ensure the symbol rollDie and the loop
that uses term.sign/term.count/term.sides are the ones modified so numeric
totals (diceTotal) remain computed with sign while display values are formatted
from the new signed-aware structure.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@src/engine/combat/rng.ts`:
- Around line 287-291: The rolls array currently receives signed numeric values
(term.sign * r) which causes join('+') in engine.ts to produce strings like
"3+-2"; change the roll representation so sign and magnitude are preserved
separately: instead of pushing signed numbers into rolls within the loop that
calls rollDie(term.sides), push either an object/tuple {value: r, sign:
term.sign} (or [r, term.sign]) or push the absolute value into rolls and store
term.sign in a parallel signs array; update consumers (e.g., the code referenced
at engine.ts:577 that does dmgResult.rolls.join('+')) to format each entry as
(sign === -1 ? '-' : '') + value when building the display string. Ensure the
symbol rollDie and the loop that uses term.sign/term.count/term.sides are the
ones modified so numeric totals (diceTotal) remain computed with sign while
display values are formatted from the new signed-aware structure.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0aef05fd-291f-4bb9-ac41-13476da7a9bd

📥 Commits

Reviewing files that changed from the base of the PR and between 8c6bcbb and adbd33e.

📒 Files selected for processing (2)

src/engine/combat/rng.ts
tests/combat/rng.test.ts

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

tests/combat/rng.test.ts

* fix(combat): accept compound dice notation like "1d6+4+2d6" CombatRNG.roll and rollDamageDetailed previously matched a single-term regex (NdS+M), rejecting standard compound expressions used by sneak attack (1d6+4+2d6) and divine smite (1d8+3+2d8). Adds a parseDiceTerms helper that splits on +/- preserving signs and evaluates each term as either dice or a scalar. Supports shorthand "dN" and mixed dice/subtraction (2d6+1d4-2). Both roll() and rollDamageDetailed() now share the parser. Closes Mnehmos#42 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(combat): reject malformed operator chains in compound dice notation Reviewers (PR Mnehmos#51) flagged that the original tokenization regex silently accepted inputs like "1d6++2", "1d6--2", "1d6+", or trailing operators — the regex matched valid chunks and skipped junk. That could change combat outcomes when callers fat-fingered an expression. Adds a strict grammar guard before tokenization: <term> ( [+-] <term> )* with at most one leading sign. Adds a parameterized test for 10 malformed inputs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector Bot reviewed Apr 18, 2026

View reviewed changes

coderabbitai Bot reviewed Apr 18, 2026

View reviewed changes

Comment thread src/engine/combat/rng.ts Resolved

coderabbitai Bot reviewed Apr 19, 2026

View reviewed changes

Mnehmos merged commit 94a565a into main Apr 20, 2026
4 checks passed

Mnehmos deleted the fix/42-dice-notation-compound branch April 20, 2026 14:15

This was referenced May 25, 2026

fix: backend bug sweep — session/travel/inventory (#34 #35 #36 #37) blackeagle54/mnehmos.rpg.mcp#1

Merged

fix: backend triage sweep #2 — #29 weapon type + #40 regression blackeagle54/mnehmos.rpg.mcp#3

Merged

Navigation Menu

Repository navigation

fix(combat): accept compound dice notation like "1d6+4+2d6" - #51

fix(combat): accept compound dice notation like "1d6+4+2d6"#51
Mnehmos merged 2 commits into
mainfrom
fix/42-dice-notation-compound

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant