Refine Rolemaster support plan

TASKS.md

@@ -21,24 +21,39 @@ The initial scope is only roll definition, validation, execution, logging, and U
   - Recursive follow-up rolls are high-ended only.
 - Standard percentile rolls do not open-end and do not use a fumble range.
 - Initiative rolls are simple `2d10 + x` rolls with no open-ended behavior.
-- The modifier `x` should be an integer and may be negative if we want Rolemaster penalties to be expressible. Current generic dice validation only allows non-negative modifiers, so this must be revisited.
+- The modifier `x` should be an integer.
+- Negative modifiers are supported for Rolemaster only.
+- D6 and D&D 5e keep their current non-negative modifier rules unless separately re-scoped in a future change.
 - Existing D6 and D&D 5e behavior must remain unchanged.
 
-## Open Questions To Resolve Before Implementation
+## Clarifications
 
-- Should the Rolemaster modifier allow negative values everywhere, or only for Rolemaster skills?
+- The Rolemaster modifier should allow negative values only for Rolemaster.
-- What is the allowed fumble range for open-ended percentile skills:
+- The standard fumble range for open-ended percentile skills is `0-5`, but can be configured individually for each skill.
-  - `0-5`
+- The fumble range has to be guaranteed to be less than 96.
-  - `0-10`
+- Roll breakdown text format for low-ended subtraction: `12 (03) -97 -44 +15 = -114`
-  - unrestricted `0-100`
+- Initiative should be represented as a dedicated Rolemaster roll type, because UI copy and validation differ
-- Should a low-ended first roll that is also `96+` ever be possible through configuration overlap:
+- Skill groups need to support Rolemaster defaults for all of these fields, to stay consistent with current prototype behavior
-  - recommended answer: no, validation should prevent impossible overlap by constraining fumble range below `96`
+- Rolemaster uses expression parsing with canonical syntax:
-- How should roll breakdown text read for low-ended subtraction:
+  - initiative: `2d10+48`
-  - recommended format: `12 - (97 + 44) + 15 = -114`
+  - standard percentile: `d100+4`
-- Should initiative be represented as a dedicated Rolemaster roll type, or as a generic fixed expression with Rolemaster labeling:
+  - open-ended percentile: `d100!+85`
-  - recommended answer: dedicated type, because UI copy and validation differ
+  - negative modifiers are valid only for Rolemaster, for example `d100-15`
-- Do skill groups need to support Rolemaster defaults for all of these fields:
+
-  - recommended answer: yes, to stay consistent with current prototype behavior
+## Architecture Guardrails
+
+- Keep minimal API endpoint handlers thin and continue to place gameplay validation and roll execution in services.
+- Keep Blazor components focused on form state, rendering, and API orchestration; do not duplicate Rolemaster rules in component code.
+- Keep request/response DTOs separate from domain/persistence models.
+- Preserve the current lazy log-detail loading flow so Rolemaster detail stays out of hot list payloads.
+- Prefer additive changes to contracts and storage over replacing current D6 / D&D shapes outright.
+
+## Backward Compatibility
+
+- Existing D6 and D&D 5e API request and response fields must remain valid throughout the Rolemaster rollout.
+- Existing persisted skills, skill groups, and roll log rows must continue to load without migration-time data loss.
+- Any richer die metadata must deserialize safely for old rows that only contain the current `RollDieResult` shape.
+- Payload budgets for roster, log page, roll mutation responses, and lazy-loaded roll detail must remain explicitly guarded by tests.
 
 ## Design Direction
 
@@ -66,7 +81,7 @@ The current model is D6-shaped:
 - `WildDice`
 - `AllowFumble`
 
-That shape is not a clean fit for Rolemaster. The Rolemaster extension should move toward a ruleset-aware skill definition that can express roll behavior directly instead of encoding everything into a free-form expression string.
+That shape is not a clean fit for Rolemaster. The Rolemaster extension should still preserve expression parsing, but it needs a ruleset-aware internal definition so the app can validate, execute, and display Rolemaster rolls safely.
 
 Recommended model additions:
 - `SkillRollKind` enum
@@ -74,18 +89,19 @@ Recommended model additions:
   - `RolemasterInitiative`
   - `RolemasterPercentile`
   - `RolemasterOpenEndedPercentile`
-- `RollModifier` integer
 - `RolemasterFumbleRange` nullable integer
-- keep `DiceRollDefinition` only if needed for legacy display or migration compatibility
+- keep `DiceRollDefinition` as the canonical persisted expression across rulesets
+- derive `SkillRollKind` and `RollModifier` from parsed expressions unless persisting them provides a concrete implementation benefit
 
 Recommended direction:
 - Keep existing fields working for D6 and D&D 5e.
-- Add new fields for Rolemaster rather than overloading `WildDice` / `AllowFumble`.
+- Preserve `DiceRollDefinition` as the authoritative user-facing input for D6, D&D 5e, and Rolemaster.
-- Centralize skill validation into a single ruleset-aware validator that returns a canonical internal representation.
+- Add only the extra Rolemaster fields that cannot be encoded cleanly in the expression itself, specifically the fumble range.
+- Centralize parsing and validation into a single ruleset-aware validator that returns a canonical internal representation.
 
 Tasks:
 - Define a ruleset-aware skill definition model in domain/contracts.
-- Decide whether to preserve `DiceRollDefinition` as persisted state or derive display text from structured fields.
+- Keep `DiceRollDefinition` as persisted state and define canonical formatting rules for Rolemaster expressions.
 - Add mapping helpers so UI and API get a stable, ruleset-aware DTO.
 - Ensure skill groups can store the same Rolemaster defaults as skills.
 
@@ -115,15 +131,22 @@ Validation rules to add:
   - fumble range must be below `96`
 - reject D6-only options for Rolemaster skills
 - reject Rolemaster-only fields for D6 / D&D 5e skills
+- reject negative modifiers for D6 / D&D 5e expressions
+- accept negative modifiers for Rolemaster expressions only
 
 Tasks:
 - Replace or extend `ValidateSkillDefinition`.
-- Add a Rolemaster-focused parser/validator path.
+- Extend `DiceRules.ParseExpression` with a Rolemaster-focused parser/validator path.
-- Decide whether `DiceRules.ParseExpression` should remain only for free-form rulesets or become a lower-level helper.
+- Add canonical Rolemaster expression parsing rules:
+  - `2d10+48` and `2d10-15` for initiative
+  - `d100+4` and `d100-20` for standard percentile
+  - `d100!+85` and `d100!-15` for open-ended percentile
+- Keep Rolemaster fumble range validation separate from the expression because it is configured independently.
 - Add canonical display formatting for Rolemaster skills, for example:
   - `Initiative: 2d10+15`
   - `Percentile: 1d100+48`
   - `Open-ended percentile: OE 1d100+48, fumble <= 5`
+- Add explicit regression tests that D6 / D&D still reject negative modifiers after Rolemaster support lands.
 
 Affected areas:
 - `RpgRoller/Services/DiceRules.cs`
@@ -160,13 +183,15 @@ Open-ended algorithm:
 Tasks:
 - Extend `ComputeRoll` dispatch.
 - Add Rolemaster-specific roll methods.
+- Parse Rolemaster expressions into an internal roll definition before execution rather than branching on raw strings in multiple places.
 - Decide whether the current `RollDieResult` DTO is sufficient.
   - likely not sufficient, because it only has `Crit`, `Fumble`, `Wild`, `Removed`, `Added`
 - Recommended DTO extension:
   - `Sequence` or `Order`
   - `Phase` / `Kind` such as `initial`, `open-ended-high`, `open-ended-low-subtract`
   - `SignedContribution` or equivalent metadata for subtraction
-- Update log summary generation so Rolemaster rolls remain understandable in compact log entries.
+- Update log summary generation so Rolemaster rolls remain understandable in compact log entries without bloating list payloads.
+- Keep lazy detail expansion as the place where the full Rolemaster breakdown and rich die metadata are loaded.
 
 Affected areas:
 - `RpgRoller/Services/GameService.cs`
@@ -186,6 +211,7 @@ Tasks:
 - Decide migration defaults for legacy rows.
 - Update `SqliteSchemaUpgrader` coverage expectations if schema assumptions change.
 - Verify serialized roll log payloads remain readable for old entries after DTO extension.
+- Ensure additive schema changes allow mixed old/new data during migration and startup upgrade.
 
 Data migration recommendations:
 - Legacy D6 / D&D 5e skills should retain current behavior with deterministic defaults.
@@ -210,13 +236,16 @@ Tasks:
 
 Recommended frontend model changes:
 - Introduce ruleset-aware form state rather than raw D6 toggles.
-- Replace "Expression" with a Rolemaster-specific editor when the selected campaign ruleset is `rolemaster`.
+- Keep "Expression" as the primary input, but provide Rolemaster-specific help text, examples, and validation when the selected campaign ruleset is `rolemaster`.
 - Add a "Roll type" selector with options:
   - `Initiative`
   - `Percentile`
   - `Open-ended percentile`
-- Show modifier input for all Rolemaster roll types.
+- Keep the selector and expression input synchronized so users can either choose a type or see the parsed/canonical expression clearly.
 - Show fumble range input only for open-ended percentile.
+- Use progressive disclosure so only the fields relevant to the chosen Rolemaster roll type are shown.
+- Add inline validation on blur and submit, visible required indicators, and `aria-live` / alert semantics for validation errors.
+- Preserve the existing app visual language instead of introducing a separate Rolemaster-specific theme.
 
 Affected areas:
 - `RpgRoller/Contracts/ApiContracts.cs`
@@ -239,6 +268,7 @@ Tasks:
 - Add tooltip or title text that explains each die's role in the chain.
 - Update compact log summaries so they surface open-ended behavior.
 - Ensure private/public visibility behavior remains unchanged.
+- Keep roll list rows dense and readable; reserve multi-line breakdown detail for expanded rows or detail fetches.
 
 Recommended display goals:
 - A normal percentile roll should read as simple and uncluttered.
@@ -260,6 +290,8 @@ Service-level tests to add:
 - skill creation validation for each Rolemaster roll type
 - rejection of invalid fumble ranges
 - rejection of D6-only options on Rolemaster skills
+- rejection of negative modifiers on D6 / D&D skills
+- acceptance of negative modifiers on Rolemaster expressions
 - initiative roll math
 - standard percentile roll math
 - open-ended high roll with one extra roll
@@ -274,11 +306,14 @@ API tests to add:
 - create/update Rolemaster skills
 - roll each Rolemaster skill type via API
 - verify log page and detail endpoints for Rolemaster rolls
+- verify old D6 / D&D contracts still round-trip without requiring Rolemaster-only fields
 
 Frontend/regression checks:
 - Rolemaster ruleset appears in campaign creation UI
 - Rolemaster skill form shows correct conditional fields
 - roll results render correctly for open-ended cases
+- validation errors are announced and visible for invalid Rolemaster expressions / fumble ranges
+- the compact log remains readable and detail expansion shows the full Rolemaster breakdown
 
 Coverage-sensitive files likely needing tests:
 - `RpgRoller.Tests/Services/DiceRulesTests.cs`
@@ -286,6 +321,7 @@ Coverage-sensitive files likely needing tests:
 - new dedicated `Rolemaster` service tests
 - `RpgRoller.Tests/Api/CampaignApiTests.cs`
 - `RpgRoller.Tests/HostingCoverageTests.cs`
+- `RpgRoller.Tests/PayloadBudgetTests.cs`
 
 ### 9. Documentation updates
 
@@ -307,11 +343,11 @@ Recommended implementation order:
 #### Iteration 1: ruleset plumbing
 - Add `Rolemaster` ruleset enum/id/display support.
 - Add campaign creation support.
-- Add parser/validator scaffolding for Rolemaster roll types.
+- Add parser/validator scaffolding for Rolemaster expressions and Rolemaster-only negative modifier support.
 - Add baseline tests for ruleset mapping and validation.
 
 #### Iteration 2: structured skill model
-- Add persisted Rolemaster fields for skills and skill groups.
+- Preserve `DiceRollDefinition` as the canonical persisted expression and add only the extra persisted Rolemaster fields the expression cannot represent.
 - Add migration and schema tests.
 - Update API contracts and service mapping.
 - Keep existing D6 / D&D behavior green.
@@ -320,16 +356,18 @@ Recommended implementation order:
 - Implement initiative and standard percentile execution.
 - Implement open-ended percentile execution with recursive high-end chaining and low-end subtraction.
 - Extend die-result metadata and breakdown formatting.
-- Add focused service tests for roll math.
+- Add focused service tests for roll math and payload/detail serialization compatibility.
 
 #### Iteration 4: frontend editing
 - Update campaign ruleset selection UI.
 - Replace D6-only form branching with ruleset-aware skill editing.
 - Support Rolemaster skill groups and skills in create/edit flows.
+- Add accessible inline validation and progressive disclosure to the Rolemaster editor.
 - Verify with ephemeral Playwright.
 
 #### Iteration 5: log/detail polish and docs
 - Improve roll chip rendering and compact summaries for Rolemaster.
+- Re-run payload-budget checks and confirm lazy detail loading still holds.
 - Update README/OpenAPI/docs.
 - Run full local CI and coverage checks.
 
@@ -337,9 +375,10 @@ Recommended implementation order:
 
 - The current skill contract is tightly coupled to D6-era fields, so a shallow patch will create long-term complexity.
 - The current `RollDieResult` type is too generic for clear Rolemaster open-ended auditing.
-- Allowing negative Rolemaster modifiers may require carefully scoping validation changes so D6/D&D rules do not loosen unintentionally.
+- Allowing negative Rolemaster modifiers requires carefully scoping validation changes so D6/D&D rules do not loosen unintentionally.
 - Migration design needs care to avoid breaking legacy SQLite databases.
 - UI complexity will grow if ruleset-specific fields are bolted onto the current `IsD6` logic instead of replacing it with a ruleset-aware form model.
+- Richer Rolemaster die metadata could regress payload sizes or list readability if detail is not kept behind the existing lazy-load flow.
 
 ## Recommended Acceptance Criteria