feat: add authoritative skill legality and additive S.D.C.#64
Merged
Conversation
added 10 commits
July 26, 2026 17:24
📝 WalkthroughWalkthroughThe change introduces typed skill catalogs and O.C.C. legality, authoritative skill-selection payloads, shared skill assembly, additive sourced Physical S.D.C. derivation, backend validation and rolling, and builder support for repeatable labeled selections and S.D.C. previews. ChangesSkill legality and catalog
Derived S.D.C. and persistence
Builder integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Builder
participant RulesAssembler
participant ConvexBackend
participant CharacterSheet
participant SdcRoller
Builder->>RulesAssembler: Submit authoritative skill selections
RulesAssembler-->>Builder: Preview assembled skills and S.D.C. breakdown
Builder->>ConvexBackend: Persist selections
ConvexBackend->>RulesAssembler: Validate and derive character
RulesAssembler-->>ConvexBackend: Effective skills and S.D.C. formulas
ConvexBackend->>SdcRoller: Roll all derived S.D.C. components
SdcRoller-->>ConvexBackend: Persist final S.D.C. total
ConvexBackend->>CharacterSheet: Return derived sheet
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
StreamDemon
marked this pull request as ready for review
July 26, 2026 12:04
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
packages/backend/convex/schema.ts (1)
5-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkill-category enum duplicated between rules and backend.
This 18-literal union re-encodes
skillCategorySchemafrompackages/rules/src/schema/skills.tsverbatim. Sincepackages/backendalready depends on@riftforge/rulesforgetOcc/deriveSheet, consider exporting the raw category string list from the rules package and building both the Zod enum and this Convex validator from it, so a future category addition/rename can't silently drift between the two packages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/convex/schema.ts` around lines 5 - 24, Remove the duplicated category literals from skillCategoryValidator and centralize the raw category list in the rules package alongside skillCategorySchema. Export that shared list, build the rules Zod enum and backend Convex validator from it, and update package exports/imports so both consumers use the same source.packages/rules/src/engine/combat.ts (1)
71-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider making
rngrequired now that the signature is being rewritten. TheMath.randomdefault lets a caller obtain a non-deterministic roll from the pure rules engine; the backend already passes its own source. Making the parameter explicit removes that footgun (recognising this matches the existingrollDice/rollHitPointsconvention, so it may be a deliberate house style).As per coding guidelines, "Engine functions must return raw book amounts and remain pure and deterministic; dice rolls and elapsed time must be supplied as inputs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rules/src/engine/combat.ts` around lines 71 - 79, Make the rng parameter required in rollPhysicalSdc instead of defaulting to Math.random, matching the explicit-rng convention used by rollDice and rollHitPoints. Update all callers to pass their random source so the rules engine remains deterministic and returns raw book amounts.Source: Coding guidelines
packages/rules/src/engine/skills.ts (1)
111-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIndexes are built twice at load.
buildSkillCatalogalready callsbuildSkillIndexes(catalog.skills)internally (Line 76); returning them (or exposing an internal helper) would avoid the second full pass and keep a single source of truth for the id/name maps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rules/src/engine/skills.ts` around lines 111 - 113, Update buildSkillCatalog and the skillCatalog initialization so the indexes produced during catalog construction are returned or exposed and reused for skillById and skillByName. Remove the second buildSkillIndexes(skillCatalog.skills) call, keeping a single source of truth for both maps.packages/rules/src/engine/builder.ts (1)
166-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
constraints[].fromCategoryasSkillCategoryto drop the cast.occRelatedSkillsSchemaalready narrows it to the enum, so theas SkillCategoryat Line 405 is only needed because the plan interface widens it back tostring. Narrowing the interface also makesperCategorylookups checked.♻️ Proposed change
- constraints: { fromCategory: string; min: number }[]; + constraints: { fromCategory: SkillCategory; min: number }[];- const category = constraint.fromCategory as SkillCategory; - const have = perCategory[category] ?? 0; + const have = perCategory[constraint.fromCategory] ?? 0;Also applies to: 404-406
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rules/src/engine/builder.ts` at line 166, Update the plan interface’s constraints type so fromCategory uses SkillCategory instead of string, matching occRelatedSkillsSchema. Then remove the unnecessary as SkillCategory cast around the perCategory lookup near the related skill-building logic, preserving typed key checking.packages/rules/tests/builder.test.ts (1)
504-508: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrder-sensitive assertion. The expected array encodes the recursion/
Setinsertion order ofaddOccPrerequisites; a harmless reordering there would fail this test for the wrong reason. Prefer asserting membership (e.g.expect(new Set(ids)).toEqual(new Set([...]))ortoContainper id) unless the ordering is itself a contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rules/tests/builder.test.ts` around lines 504 - 508, Update the assertion for assembled.skills in the addOccPrerequisites test to verify skill membership without depending on insertion order. Compare Sets or assert each expected skillId is contained, while preserving validation that all expected skills are present.packages/rules/src/engine/occ.ts (1)
190-209: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffUnbounded backtracking is fine today but has no guard as content grows. Branching is
candidates.lengthper slot and depth is the sum of allconstraint.minvalues, so a future O.C.C. with several large "any" categories and higher minimums makes this exponential at module load. A memo on(slotIndex, spent, usedBySkill-signature)or a simple visited-state cap would keep the fail-fast cheap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rules/src/engine/occ.ts` around lines 190 - 209, Bound the recursive search in canAssignMinimums with a visited-state memo or state-count cap keyed by slotIndex, spent, and the usedBySkill allocation signature. Ensure repeated states are skipped or the search fails fast once the cap is reached, while preserving successful minimum assignment behavior and the existing error when no assignment is possible.packages/rules/src/schema/skills.ts (1)
63-64: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider rejecting a
categorythat is repeated inadditionalCategories. A duplicate heading would double-count the skill inskillsForPolicyconsumers that concatenate per-category results (e.g.relatedSkillPlanvariants). A refinement here keeps the contradiction out at content-load time.♻️ Suggested refinement
.superRefine((skill, ctx) => { + const extra = skill.additionalCategories ?? []; + if (extra.includes(skill.category) || new Set(extra).size !== extra.length) { + ctx.addIssue({ + code: "custom", + path: ["additionalCategories"], + message: "additionalCategories must be distinct and exclude the primary category.", + }); + } if (Also applies to: 90-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rules/src/schema/skills.ts` around lines 63 - 64, Update the schema containing category and additionalCategories to reject entries in additionalCategories that duplicate the primary category, using a Zod refinement or equivalent validation. Preserve the existing optional-array behavior and ensure duplicate headings are rejected during content loading, including the corresponding schema definition around the additional category fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/builder/steps/occ-skills.tsx`:
- Around line 141-146: The choose-N label editor in the TextInput within the
slot rendering flow should not remove a pick on every keystroke when the label
is temporarily empty. Update the onInput/setLabel interaction to retain the
current slot while editing, and only remove or commit an empty label on
blur/commit (or otherwise defer empty-value handling), preserving later pick
positions.
In `@apps/web/src/builder/steps/related-skills.tsx`:
- Around line 111-132: Replace the referentially keyed For rendering in the
selected specialization rows with index-keyed rendering using Index, so label
edits do not remount the active TextInput. Preserve the existing pick label
binding, onLabel(pickIndex, ...), and onRemoveRepeat behavior while deriving
each row from its stable selection index; verify typing keeps focus and the
caret in the browser.
In `@apps/web/src/components/sheet-view.tsx`:
- Around line 456-464: Replace the hardcoded “RUE” prefix in the StatRow
rendering at apps/web/src/components/sheet-view.tsx#L456-L464 with
contribution.source.book, preserving the page citation format. Update the
corresponding source-string assertion at
apps/web/tests/character-sheet.test.ts#L46-L51 to expect the dynamic book label.
In `@docs/rules/PAGE_MAP.md`:
- Around line 42-44: The category index table in PAGE_MAP.md is missing the
catalog headings Cowboy, Horsemanship, Military, and Pilot Related. Update the
page-anchored list to include each heading with its corresponding page ranges
from skills.json, while preserving the existing ordering and formatting.
In `@packages/rules/src/engine/builder.ts`:
- Around line 420-432: Update the skill-pick validation and assembly flow around
assembleSkills() so hand-to-hand-basic cannot be selected from the Physical
secondary or related pools unless its hthType value is also propagated. Prefer
excluding hand-to-hand-basic from those eligible pools, preserving existing
handling for all other skills; otherwise wire its selection through to hthType
consistently.
---
Nitpick comments:
In `@packages/backend/convex/schema.ts`:
- Around line 5-24: Remove the duplicated category literals from
skillCategoryValidator and centralize the raw category list in the rules package
alongside skillCategorySchema. Export that shared list, build the rules Zod enum
and backend Convex validator from it, and update package exports/imports so both
consumers use the same source.
In `@packages/rules/src/engine/builder.ts`:
- Line 166: Update the plan interface’s constraints type so fromCategory uses
SkillCategory instead of string, matching occRelatedSkillsSchema. Then remove
the unnecessary as SkillCategory cast around the perCategory lookup near the
related skill-building logic, preserving typed key checking.
In `@packages/rules/src/engine/combat.ts`:
- Around line 71-79: Make the rng parameter required in rollPhysicalSdc instead
of defaulting to Math.random, matching the explicit-rng convention used by
rollDice and rollHitPoints. Update all callers to pass their random source so
the rules engine remains deterministic and returns raw book amounts.
In `@packages/rules/src/engine/occ.ts`:
- Around line 190-209: Bound the recursive search in canAssignMinimums with a
visited-state memo or state-count cap keyed by slotIndex, spent, and the
usedBySkill allocation signature. Ensure repeated states are skipped or the
search fails fast once the cap is reached, while preserving successful minimum
assignment behavior and the existing error when no assignment is possible.
In `@packages/rules/src/engine/skills.ts`:
- Around line 111-113: Update buildSkillCatalog and the skillCatalog
initialization so the indexes produced during catalog construction are returned
or exposed and reused for skillById and skillByName. Remove the second
buildSkillIndexes(skillCatalog.skills) call, keeping a single source of truth
for both maps.
In `@packages/rules/src/schema/skills.ts`:
- Around line 63-64: Update the schema containing category and
additionalCategories to reject entries in additionalCategories that duplicate
the primary category, using a Zod refinement or equivalent validation. Preserve
the existing optional-array behavior and ensure duplicate headings are rejected
during content loading, including the corresponding schema definition around the
additional category fields.
In `@packages/rules/tests/builder.test.ts`:
- Around line 504-508: Update the assertion for assembled.skills in the
addOccPrerequisites test to verify skill membership without depending on
insertion order. Compare Sets or assert each expected skillId is contained,
while preserving validation that all expected skills are present.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 890d8d9b-45d0-41b8-9572-669ac2227729
📒 Files selected for processing (29)
.codex/superpowers/plans/2026-07-26-skill-legality-sdc-foundation.mdREADME.mdapps/web/src/builder/steps/occ-skills.tsxapps/web/src/builder/steps/related-skills.tsxapps/web/src/builder/store.tsapps/web/src/components/sheet-view.tsxapps/web/tests/builder.test.tsapps/web/tests/character-sheet.test.tsdocs/rules/PAGE_MAP.mdpackages/backend/convex/characters.tspackages/backend/convex/schema.tspackages/backend/tests/characters.test.tspackages/backend/tests/combat.test.tspackages/backend/tests/healing-cast.test.tspackages/rules/src/content/occ/ley-line-walker.jsonpackages/rules/src/content/skills/skills.jsonpackages/rules/src/engine/builder.tspackages/rules/src/engine/character.tspackages/rules/src/engine/combat.tspackages/rules/src/engine/occ.tspackages/rules/src/engine/skills.tspackages/rules/src/schema/character.tspackages/rules/src/schema/occ.tspackages/rules/src/schema/skills.tspackages/rules/tests/builder.test.tspackages/rules/tests/character.test.tspackages/rules/tests/combat.test.tspackages/rules/tests/occ.test.tspackages/rules/tests/skills.test.ts
This was referenced Jul 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
2D6+12base plus every applicable O.C.C./Physical-skill formula, expose a sourced contribution breakdown, and roll the same formulas server-side.Why
Issue #61 is the rules foundation required before the next O.C.C. can land. Previously, legality lived partly in prose, flattened bonuses were client-authored, prerequisites could dangle silently, Secondary Skills borrowed incomplete selection behavior, and S.D.C. always used the bare base formula.
Source authority
The implementation was checked against rendered Rifts Ultimate Edition pages:
docs/rules/PAGE_MAP.mdrecords the corrected page coverage.Compatibility and boundaries
Verification
vp check packages/rulesand 396 rules testsvp check packages/backendand 134 backend testsvp check apps/weband 63 web testsvp checkand 593 root testsgit diff --checkCloses #61
Closes #22
Closes #25
Closes #26
Summary by cubic
Adds a complete, typed skill catalog with an authoritative assembler for O.C.C., O.C.C.-Related, Secondary, and Hand-to-Hand selections, replacing client-authored bonuses with source-validated decisions. Physical S.D.C. is now additive (p.287 base plus all O.C.C./Physical-skill contributions) with a source breakdown and server-side rolls.
New Features
@riftforge/rules.Migration
skillSelections(occChoices, related, secondary, optional hthId); do not send legacyskillsorhthType.Closes #61
Closes #22
Closes #25
Closes #26
Written for commit 2d3076e. Summary will update on new commits.