docs(adr): propose a reusable RPG scaffolding tier between Engine and rulesets#15
Conversation
… rulesets ADR-0008 + PLAN-0008: a new SharpMud.Ruleset.Rpg package for the combat/ encounter scaffolding that's already flavor-agnostic in samples/Classic today, plus a minimal SharpMud.Ruleset.Basic leaf for a real "few lines in Program.cs, run a basic game" quick-start - modeled loosely on EF Core's Core/Relational/Provider shape, informed by a research pass against WheelMUD (which has no precedent for this specific split, but does validate a DI-based dice-rolling abstraction over the existing IRandomSource). Classic stays an unpackaged sample, rebuilt on the new scaffolding instead of owning it directly. Status: Proposed - not yet implemented. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54fb372f94
ℹ️ 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".
- PLAN-0008's docs task said to flip ADR-0008 to Accepted after implementation lands, contradicting the plan's own header and docs/plans/README.md's lifecycle rule that a plan can't move to In Progress until its ADR is already Accepted. Reworded as a final consistency check, not the acceptance step. - engine-vs-ruleset.md's forward-reference said Classic would be rebuilt on top of both Ruleset.Rpg and Ruleset.Basic - per ADR-0008/ PLAN-0008, Basic and Classic are siblings on Rpg, neither depending on the other. Fixed the wording before it sends an implementer down the wrong dependency path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ae8ba2fdc
ℹ️ 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".
CombatManager doesn't just touch StatsBehavior - its constructor also hard-codes a concrete Thing hubRoom as the respawn destination on defeat, a Classic-specific world/respawn policy that would otherwise leak into the supposedly-generic Rpg package verbatim. Separately, CombatantBehavior's EF Core mapping (CombatantBehaviorConfiguration) only gets applied today because ClassicBehaviorMappingContributor scans its own assembly - moving the behavior without moving/adding an equivalent contributor in the new package means a saved world containing it hits an unmapped TPH discriminator subtype. Both now scoped into the ADR's package description and the plan's tasks/test plan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4a1e6fd66
ℹ️ 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".
…ping, docs, dice tests - ADR's Positive Consequences claimed the only real work was decoupling StatsBehavior, ignoring the hubRoom/persistence-mapping/dice-abstraction work the same ADR describes elsewhere - reworded to not minimize it. - Plan only moved CombatantBehavior's EF mapping into Rpg but never gave Basic's own new stats behavior a mapping/contributor/round-trip test, same unmapped-TPH-subtype risk already caught once. - Plan's docs tasks covered internal subsystem docs but never the actual quick-start guidance the ADR's headline goal promises - added a README/docsite getting-started + Rpg package-consumption docs task. - Test plan treated the new dice abstraction as incidental CombatResolver coverage; it's new public scaffolding behavior and needs its own direct tests (dice-count/sides/modifier math, invalid-input validation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53f18b114a
ℹ️ 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".
|
@codex review |
…ubRoom blocker - Neither the ADR nor plan defined a package-level DI registration entry point for SharpMud.Ruleset.Rpg itself - every consumer would have hand-rolled the same fragile multi-line wiring (ICombatResolver/ICombatManager-as-ITickable/mapping contributor/dice service) Classic's Program.cs does manually today, directly undermining the "few lines" goal. Added AddSharpMudRpgRuleset(...) (or equivalent) to both docs. - Basic's task list covered stats/world/DI options but never gave it an IPlayerFactory - Hosting's PlayerLogin/LoginFlow constructor-inject one, so without it a fresh CLI/Telnet player can't be created at all and the quick-start fails at first login. - docs/architecture.md (owns the solution layout/dependency summary) was missing from the docs task and critical-files list alongside engine-vs-ruleset.md. - Open Items/blockers only named the StatsBehavior decoupling as blocking; the hubRoom/respawn-destination seam is equally blocking for moving CombatManager into a generic package and belongs there too, not just as a nested task. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53f18b114a
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e9933cab6
ℹ️ 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".
…Persistence dependency - AddSharpMudRuleset(...) is single-callback - a second call (Rpg's own commands vs. Basic/Classic's) would silently clobber the first via DI's single-registration-wins resolution. Added an explicit task to define how Rpg/Basic/consumer command registration composes, rather than leaving it implicit. - Basic's default IWorldBuilder only promised "enough to walk around," which doesn't satisfy the Goal's "fight something" claim. Added a requirement for at least one CombatantBehavior-carrying NPC, plus verification that attack/flee actually resolve against it. - CombatantBehavior's mapping contributor lives in SharpMud.Persistence, so Ruleset.Rpg necessarily takes a dependency on it - no persistence-free path to the combat scaffolding. Called this out explicitly in the ADR's Decision Outcome and Open Items rather than leaving it an implicit consequence of a different task. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fb0c06d2d
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Drop the plan's now-stale deferred wheelmud-findings.md cleanup bullet (already done in this PR), add the fightable-NPC commitment to the ADR's Basic Decision Outcome, and track both the respawn-seam and command-registration-composition blockers that Decision Outcome/task list already implied but Open Items/Open questions omitted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
📝 |
…rrect stale plugin-loading claim Add the AttackCommand/FleeCommand composition seam to ADR Open Items and the Rpg->Persistence dependency to Negative Consequences; require explicit IPlayerFactory test coverage and carve out the intentional HP-reset fix as an expected behavioral change in the plan's test/verification sections; and correct wheelmud-findings.md's stale claim that rulesets load via reflection/attribute-scanning (actual direction is manual DI extension methods) plus its outdated ADR-0002 status. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Re: #15 (comment) — Fixed in 5cfcff4 — updated wheelmud-findings.md's ADR-0002 status annotation from Proposed to Accepted, matching the ADR file itself. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cfcff48cd
ℹ️ 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".
…ame kill/attack alias in test coverage Correct the two remaining assembly-scan/reflection references (wheelmud-findings.md §8, SPEC.md's Open Items) now that ruleset loading is manual DI extension methods, reword ADR-0008's "five types" to count-free wording, and make the plan's DI/composition test explicitly require both AttackCommand's kill verb and attack alias, not just "attack/flee" generically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Two spots still said "five types" after the earlier cleanup pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Proposed): a newSharpMud.Ruleset.Rpgpackage for combat/encounter scaffolding that's already flavor-agnostic insamples/SharpMud.Samples.Classictoday (CombatantBehavior,ICombatResolver/CombatResolver,ICombatManager/CombatManager,AttackCommand/FleeCommand— verified none of these referenceRace/CharacterClass), plus a minimalSharpMud.Ruleset.Basicleaf package for a genuine "few lines inProgram.cs, run a basic game" quick-startCore/Relational/Providershape; informed by a research pass against WheelMUD, which has no precedent for this specific rules-tier split (its only ruleset,WarriorRogueMage, was never split further) but does validate a DI-based dice-rolling abstraction over the existingIRandomSource(WheelMUD's ownDiceServiceis a static singleton — not adopted as such)Not Startedengine-vs-ruleset.md's Open Items (no current-state rewrite — nothing's implemented yet)Test plan
StatustoAccepted🤖 Generated with Claude Code