From 8f705111a2391418d4e05e43f16dd0aaee1943d3 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 17 Jul 2026 12:49:08 -0400 Subject: [PATCH 01/15] Design Slice 3: security role model + moderation commands (ADR-0005) Deep design dive (docs/adr/0005-...): SecurityRole flags enum (full WheelMUD 12-value set), a hand-rolled Decorator pattern (RoleGuardedCommand/MuteGuardedCommand wrapping ICommand) instead of an attribute+reflection port or DecoWeaver (which only intercepts DI-container registration - sharp-mud's commands go through a custom ICommandRegistry, not IServiceCollection). ICommandRegistry.Register is replaced with two intentional entry points, RegisterOpen and RegisterWithRole, so no command can be registered without declaring its access level. Slice 3's command set: Boot/Mute/Unmute/Announce (MinorAdmin), Ban/Unban/RoleGrant/RoleRevoke (FullAdmin) - Unban added beyond WheelMUD's own list since an irreversible ban is an operability trap. SHARPMUD_INITIAL_ADMIN env var bootstraps the first FullAdmin, since RoleGrant itself requires FullAdmin to run. PLAN-0005 tracks execution; not yet implemented. Forward-references only added to SPEC.md/commands.md/accounts-auth.md per design.md - no code, no "current state" doc claims until tasks/implement.md actually builds this. Co-Authored-By: Claude Sonnet 5 --- SPEC.md | 10 +- docs/accounts-auth.md | 11 +- ...rity-role-model-and-moderation-commands.md | 288 ++++++++++++++++++ docs/adr/README.md | 1 + docs/commands.md | 9 +- .../0001-wheelmud-reconciliation-roadmap.md | 3 +- ...rity-role-model-and-moderation-commands.md | 200 ++++++++++++ docs/plans/README.md | 1 + 8 files changed, 515 insertions(+), 8 deletions(-) create mode 100644 docs/adr/0005-security-role-model-and-moderation-commands.md create mode 100644 docs/plans/0005-security-role-model-and-moderation-commands.md diff --git a/SPEC.md b/SPEC.md index b8b76dd..046ab3d 100644 --- a/SPEC.md +++ b/SPEC.md @@ -179,9 +179,13 @@ configuration and open items. Explicitly out of scope for v1, to revisit later: -- **Moderation/admin tooling**: permission levels (player/builder/admin), - mute/kick/ban, admin commands, audit logging. Known future need, not - designed yet. +- **Moderation/admin tooling**: permission levels, mute/kick/ban, admin + commands — designed, see + [ADR-0005](docs/adr/0005-security-role-model-and-moderation-commands.md) + and + [PLAN-0005](docs/plans/0005-security-role-model-and-moderation-commands.md); + not yet implemented. Audit logging remains undesigned, tracked as an + open item in PLAN-0005. - **Soft-code/scripting engine**: revisit once data/config-driven NPC and room behavior proves insufficient. - **Procedural frontier generation algorithm**: choice of generation approach diff --git a/docs/accounts-auth.md b/docs/accounts-auth.md index 9428fea..2d0739c 100644 --- a/docs/accounts-auth.md +++ b/docs/accounts-auth.md @@ -147,8 +147,15 @@ silently skipped). "non-empty" is enforced. - Password reset flow — with no email/OAuth identity backing the account, there's no "forgot password" recovery path; not designed. Likely - admin-assisted reset only (ties into deferred moderation tooling, see - `SPEC.md`). + admin-assisted reset only (ties into the moderation tooling designed in + [ADR-0005](adr/0005-security-role-model-and-moderation-commands.md), not + yet implemented). +- Ban enforcement — designed in + [ADR-0005](adr/0005-security-role-model-and-moderation-commands.md)/ + [PLAN-0005](plans/0005-security-role-model-and-moderation-commands.md) + (a banned `PlayerBehavior.IsBanned` rejects login at password + verification), not yet implemented — today, login has no concept of a + banned user at all. - The password itself travels in cleartext over Telnet (only the on-screen *display* is suppressed) — no transport encryption exists yet; SSH (see [networking.md](networking.md)) would be the natural place this gets diff --git a/docs/adr/0005-security-role-model-and-moderation-commands.md b/docs/adr/0005-security-role-model-and-moderation-commands.md new file mode 100644 index 0000000..b25c7db --- /dev/null +++ b/docs/adr/0005-security-role-model-and-moderation-commands.md @@ -0,0 +1,288 @@ +# [ADR-0005] Security Role Model + Moderation Commands + +**Status:** Accepted + +**Date:** 2026-07-17 + +**Decision Makers:** solo (design dive conducted with the user) + +## Context + +Per ADR-0001, this is Slice 3 of the WheelMUD reconciliation roadmap. +Verified by direct research (grep across `src/`/`tests/`, full read of +`docs/commands.md`, `docs/accounts-auth.md`, `ICommand.cs`, +`ICommandRegistry.cs`, `CommandGuards.cs`, `PlayerBehavior.cs`): sharp-mud +has **no authorization concept at all** today. Every command any connected +player types is unconditionally resolved and executed — +`CommandGuards.cs` has exactly one guard (`RequireArgsAsync`, an +arg-presence check), nothing role-related. `SPEC.md`'s Deferred/Open Items +lists "Moderation/admin tooling... Known future need, not designed yet." + +WheelMUD's own mechanism (`Core/Attributes/ActionSecurityAttribute.cs` + +`Actions/Admin/`, 16 files): a `[Flags] enum SecurityRole` (`mobile` / +`item` / `room` / `tutorialPlayer` / `player` / `helper` / `married` / +`minorBuilder` / `fullBuilder` / `minorAdmin` / `fullAdmin` / `all`, +12 real values), an `[ActionSecurity(SecurityRole.x)]` class attribute on +each Action, reflected off at registration time into a `Command +.SecurityRole` field, gated at dispatch by a bitwise-AND check +(`command.SecurityRole & user.SecurityRoles`) in `CommandGuard.cs`. +`Actions/Admin/` (`Announce`, `Ban`, `Boot`, `Buff`, `Clone`, `Control`, +`Find`, `GoTo`, `Jail`, `Locate`, `Mute`, `Relinquish`, `RoleGrant`, +`RoleRevoke`, `Spawn`, `Unmute`) are ordinary Actions decorated with +high-tier roles — no separate mechanism from any other command. + +## Decision Drivers + +- No new `Thing` subtype (`design-decisions.md` rule 2) — role/mute/ban + state lives on `PlayerBehavior`, the existing identity `Behavior`. +- This repo has twice already (ADR-0002, ADR-0004) chosen a leaner + reimplementation over a faithful WheelMUD structural port when the + extra structure doesn't earn its keep here — evaluated directly against + WheelMUD's attribute+reflection mechanism as part of this dive. +- The user has a compile-time decorator-generation package + (`LayeredCraft.DecoWeaver`) and asked whether it fit. Verified against + its own docs (`https://decoweaver.layeredcraft.dev/`, + `https://github.com/LayeredCraft/decoweaver`): it only intercepts + `Microsoft.Extensions.DependencyInjection` registration calls + (`AddScoped()` etc. via C# 11 interceptors). + sharp-mud's commands are registered directly into a custom + `ICommandRegistry` (`BuiltinCommands.RegisterAll`/ + `ClassicCommands.RegisterAll` call `registry.Register(...)`), never + through `IServiceCollection` — DecoWeaver cannot see them without first + re-plumbing command registration through the DI container, a real, + separate architecture change with no driver of its own right now + (`code-of-conduct.md`: don't bundle an unrelated change into this one). +- A missing guard on a security boundary is a materially worse failure + mode than a missing guard on a UX check (the existing `RequireArgsAsync` + precedent) — an admin command silently callable by any player is a real + vulnerability, not a confusing error message. This pushed the decision + toward something structurally harder to forget than "paste a guard call + at the top of `ExecuteAsync`." +- `docs/persistence.md`/`WearableBehaviorConfiguration.cs`'s existing + precedent: a genuine bitmask (`Slot`) is a **plain** `[Flags] enum` with + EF Core's default int mapping, not a `LayeredCraft.OptimizedEnums` + instance — `OptimizedEnum` in this repo is for small non-combinable + state machines (`Race`, `CharacterClass`, `ConnectionState`), not + bitwise-combinable flag sets. `SecurityRole` follows the plain-enum + precedent. +- Bootstrap problem, surfaced during the dive: if granting a role itself + requires `FullAdmin`, and every new character defaults to `Player`, + nothing in-game can ever produce the first admin. + +## Considered Options + +1. **Explicit per-command guard call**, mirroring the existing + `CommandGuards.RequireArgsAsync` pattern — each gated command's + `ExecuteAsync` starts with `if (await CommandGuards.RequireRoleAsync(...)) + return;`. +2. **`ICommand.RequiredRole` property**, checked centrally by `SessionLoop` + before `ExecuteAsync` ever runs — every `ICommand` implementation must + supply it (compile error otherwise). +3. **Attribute + reflection**, a close port of WheelMUD's + `[ActionSecurity(...)]` + registration-time reflection. +4. **Compile-time decorator via `LayeredCraft.DecoWeaver`** — requires + first re-plumbing command registration through + `Microsoft.Extensions.DependencyInjection`. +5. **Hand-rolled Decorator pattern**: `RoleGuardedCommand : ICommand` wraps + an inner `ICommand`, checks the actor's roles against a required mask, + and delegates or rejects. `ICommandRegistry`'s single generic + `Register(ICommand)` is replaced with exactly two intentional entry + points, `RegisterOpen(ICommand)` and `RegisterWithRole(ICommand, + SecurityRole)` — the latter always applies the wrapper, so there is no + third, accidental way to register a command without declaring its + access level. + +## Decision Outcome + +Chosen option: **"5 — hand-rolled Decorator pattern with an intentional +two-method registry API,"** arrived at through the dive: option 1 was +rejected once framed as a security boundary specifically (forgettable by +construction); option 2 works but forces every existing `ICommand` +(~14 classes, including harmless ones like `look`) to declare a role it +doesn't need, just to satisfy the interface; option 3 repeats this repo's +now-established pattern of not faithfully porting WheelMUD's structure +when it doesn't earn its keep; option 4 doesn't fit sharp-mud's actual +command-registration model without an unrelated prerequisite change. The +Decorator pattern is the *actual* thing the user was reaching for with +DecoWeaver — DecoWeaver is one implementation strategy for it (compile-time +codegen over DI-registered services), not the pattern itself; hand-writing +it here gets the real benefit (composable, independently-testable +cross-cutting command wrappers — the next one, e.g. a cooldown check, is +just another wrapper of the same shape) without DecoWeaver's registration +-model requirement. + +**Mechanism**, in full: +- `SecurityRole` (`SharpMud.Engine.Commands`): a plain `[Flags] enum`, + full WheelMUD 12-value set ported (PascalCase: `None`, `Mobile`, `Item`, + `Room`, `TutorialPlayer`, `Player`, `Helper`, `Married`, `MinorBuilder`, + `FullBuilder`, `MinorAdmin`, `FullAdmin`, `All`) — adopted in full even + though several values (`Mobile`/`Item`/`Room`: sharp-mud has no + non-player command issuer today; `Married`: no marriage system; + `TutorialPlayer`/builder tiers: not yet used) are inert now, so future + slices (world-building/OLC is Slice 4, explicitly bundled with this one + in ADR-0001) already have a role to reach for instead of extending the + enum again. +- `PlayerBehavior` gains `SecurityRole Roles` (persisted — unlike + `ConnectionState`/`Session`, a role assignment must survive a restart or + it's useless; default `Player` for new characters), plus `bool IsMuted` + and `bool IsBanned` (also persisted; separate from `Roles` — a + restriction, not a capability). +- `RoleGuardedCommand` checks `(actor.Roles & requiredRole) != + SecurityRole.None` (any-of semantics, matching WheelMUD's own bitwise + check) before delegating to the inner command. The same Decorator shape + is reused for mute enforcement (`MuteGuardedCommand`, wrapping `say`/ + `emote`, checking the *target*'s `IsMuted` rather than the actor's role) + — validating the pattern's reusability for a cross-cutting concern that + isn't role-based at all. +- `ICommandRegistry.Register(ICommand)` is removed from the public + interface; `RegisterOpen(ICommand)` and `RegisterWithRole(ICommand, + SecurityRole)` are the only ways in. Every existing command's + registration call site changes from `Register` to `RegisterOpen` + (mechanical, no behavior change for them). +- Ban is enforced in `LoginFlow` at successful password verification + (`IsBanned` → reject, distinct message). + +**Command set for this slice** — the subset of WheelMUD's 16 Admin +actions that map onto systems sharp-mud already has; the rest need +prerequisite infrastructure this slice doesn't build (Find/Locate/GoTo/ +Control need world/NPC lookup and puppeting; Clone/Spawn need item/NPC +creation tooling; Jail needs a cell-room concept; Buff needs a generic +stat-modification system; Relinquish is builder-role-specific and tied to +Slice 4): + +| Command | Required role | Notes | +|---|---|---| +| `Boot` | `MinorAdmin` | Disconnects a currently-online target. | +| `Mute` / `Unmute` | `MinorAdmin` | Sets/clears `IsMuted`; enforced on `say`/`emote` via `MuteGuardedCommand`. | +| `Announce` | `MinorAdmin` | Broadcasts to every connected session. | +| `Ban` / `Unban` | `FullAdmin` | Sets/clears `IsBanned`, enforced in `LoginFlow`. `Unban` has no WheelMUD equivalent — added here because a ban with no in-game reversal is an operability trap, not because WheelMUD has one. | +| `RoleGrant` / `RoleRevoke` | `FullAdmin` | Mutates a target's `Roles`. Gated at `FullAdmin` specifically so a `MinorAdmin` can never self-escalate. | + +Day-to-day moderation (`Boot`/`Mute`/`Unmute`/`Announce`) sits at +`MinorAdmin`; harder-to-reverse or privilege-affecting actions +(`Ban`/`Unban`/`RoleGrant`/`RoleRevoke`) require `FullAdmin`. Target lookup +(online or not) mirrors `LoginFlow.FindAndAttachExistingAsync`'s existing +live-then-repository pattern; an offline target is loaded, mutated, and +saved without being attached into the live world tree (no need to, unlike +login). + +**Bootstrap**: `HostOptions` gains `SHARPMUD_INITIAL_ADMIN` (env var, +matching the existing `SHARPMUD_MODE`/`SHARPMUD_TELNET_PORT`/ +`SHARPMUD_DB_PATH` precedent). On boot, if a character with that username +exists, the host ensures it has `FullAdmin` — idempotent, safe to leave +set permanently or unset after first use. Solves "how does the first +admin ever get `FullAdmin`" without an in-game path that would otherwise +be a chicken-and-egg dead end. + +### Positive Consequences + +- Closes a real, previously-total gap: no command in this codebase was + ever gated by anything before this. +- The Decorator mechanism is reusable for the *next* cross-cutting + command concern (a cooldown, a "must not be in combat" check) without + inventing a new approach each time — validated within this same slice + by reusing it for mute enforcement, not just role-gating. +- `RegisterOpen`/`RegisterWithRole` replacing a single generic `Register` + means every command registration is a legible, intentional statement of + its own access level — there's no third, silent way in. +- Adopting the full WheelMUD role set now means Slice 4 (world-building, + already flagged in ADR-0001 as needing security gating) has + `MinorBuilder`/`FullBuilder` ready rather than needing another enum + change. + +### Negative Consequences + +- `RegisterWithRole` still depends on the person registering a command + choosing the right entry point — not literally impossible to misuse + (e.g. calling `RegisterOpen` on something that should have been gated), + just much harder to do by accident than a scattered guard call, since + the two entry points sit side by side at every registration call site. +- Most of the 12-value `SecurityRole` set (`Mobile`/`Item`/`Room`/ + `TutorialPlayer`/`Married`) has no consumer yet — accepted as the cost + of not re-deriving the enum later, but it is real unused surface area + today. +- `SHARPMUD_INITIAL_ADMIN` is a standing env var that, if left set and a + malicious actor ever created a character with that exact username on a + fresh world, would hand them `FullAdmin` — acceptable for a + solo/small-group server at this stage, worth revisiting if/when this + goes to a wider public deployment (`docs/deployment.md`). + +## Pros and Cons of the Options + +### Option 1: Explicit per-command guard call + +- Good, because it requires no interface or registry changes at all. +- Good, because it matches an already-established repo pattern + (`RequireArgsAsync`). +- Bad, because forgetting it is silent and the failure mode is a real + security hole, not a UX inconvenience — the exact risk profile that + makes this the wrong pattern to reuse here. + +### Option 2: `ICommand.RequiredRole` property + +- Good, because it's structurally impossible to omit — a compile error, + not a runtime gap. +- Good, because the check lives in exactly one place (`SessionLoop`'s + dispatch), not scattered per-command. +- Bad, because it forces every existing `ICommand` (including commands + that need no gating at all) to declare a role, touching ~14 files for + no functional benefit to those commands. + +### Option 3: Attribute + reflection (WheelMUD-faithful) + +- Good, because it's a direct, low-judgment translation of prior art. +- Bad, because this repo has already twice (ADR-0002, ADR-0004) rejected + faithfully porting WheelMUD's structure in favor of something leaner + fit to sharp-mud's actual needs, and the same reasoning applies here: + reflection-based registration-time discovery solves a + plugin-discoverability problem sharp-mud's fixed, hand-wired + `RegisterAll` calls don't have. + +### Option 4: Compile-time decorator via DecoWeaver + +- Good, because it's genuinely zero-runtime-overhead and would give + broader decoration for free if the app already used DI-container-based + registration everywhere. +- Bad, because sharp-mud's commands aren't DI-container-registered today, + and re-plumbing that is a separate, unrelated architecture change with + no driver of its own yet. + +### Option 5: Hand-rolled Decorator pattern (chosen) + +- Good, because it needs zero `ICommand` interface changes and zero + registration-model changes — a genuinely additive change. +- Good, because it's the actual design pattern the user was reaching for, + independent of any specific codegen tool, and it's proven reusable + within this same slice (mute enforcement, not just role-gating). +- Good, because the `RegisterOpen`/`RegisterWithRole` split closes almost + all of option 1's "forgettable" risk without option 2's forced, + unnecessary interface changes. +- Bad, because enforcement is still a registration-time discipline, not a + compile-time guarantee — mitigated, not eliminated, by there being only + two, clearly-named ways to register anything. + +## Links + +- [ADR-0001](0001-wheelmud-reconciliation-roadmap.md) — WheelMUD + Reconciliation Roadmap (this is Slice 3; Slice 4 world-building is + explicitly bundled with this one and will consume the same mechanism). +- [PLAN-0005](../plans/0005-security-role-model-and-moderation-commands.md) + — execution plan for this decision. +- `SPEC.md` — "Moderation/admin tooling" Deferred/Open Item this ADR + resolves (not yet implemented — see the plan). +- `docs/accounts-auth.md` — the existing forward-reference to "deferred + moderation tooling" this ADR makes concrete; ban enforcement lands in + `LoginFlow`. +- `docs/commands.md` — command pipeline (`ICommand`/`ICommandRegistry`) + this ADR extends. +- `docs/persistence.md` / `WearableBehaviorConfiguration.cs` — the + existing plain-`[Flags]`-enum-with-default-EF-mapping precedent + `SecurityRole` follows (as opposed to `LayeredCraft.OptimizedEnums`). +- `WheelMUD/src/Core/Attributes/ActionSecurityAttribute.cs`, + `WheelMUD/src/Core/ManagerSystems/CommandManager.cs`, + `WheelMUD/src/Core/CommandSystem/CommandGuard.cs`, + `WheelMUD/src/Core/Behaviors/UserControlledBehavior.cs`, + `WheelMUD/src/Actions/Admin/` — source researched for this decision. +- `https://decoweaver.layeredcraft.dev/`, + `https://github.com/LayeredCraft/decoweaver` — DecoWeaver documentation + consulted and ruled out for this slice (registration-model mismatch). diff --git a/docs/adr/README.md b/docs/adr/README.md index df0ee0f..681673f 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -64,3 +64,4 @@ the mechanics: numbering, status, and the index. | [0002](0002-telnet-protocol-negotiation.md) | Telnet Protocol Negotiation (IAC/Q-Method core + NAWS) | Accepted | | [0003](0003-allow-appsettingsjson-for-non-secret-config.md) | Allow `appsettings.json` for Non-Secret Configuration | Accepted | | [0004](0004-session-state-machine-and-reconnect.md) | Session State Machine + Linkdead Reconnect | Accepted | +| [0005](0005-security-role-model-and-moderation-commands.md) | Security Role Model + Moderation Commands | Accepted | diff --git a/docs/commands.md b/docs/commands.md index 48396a8..fd123f2 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -124,9 +124,14 @@ actually implemented as of the inventory/items build-order phase: - **Character**: `score`/`stats` (display derived stats — see [character.md](character.md)) is **not implemented yet**. - **Meta** ✅: `help`, `quit`. -- **Builder/admin verbs** (`@dig`, `@describe`, etc.) explicitly excluded — +- **Builder/OLC verbs** (`@dig`, `@describe`, etc.) explicitly excluded — those belong to the deferred in-game building phase (see - [world-model.md](world-model.md)). + [world-model.md](world-model.md)). **Moderation/admin verbs** + (`boot`/`mute`/`unmute`/`announce`/`ban`/`unban`/`rolegrant`/ + `rolerevoke`) are a separate thing — designed in + [ADR-0005](adr/0005-security-role-model-and-moderation-commands.md), not + yet implemented (see + [PLAN-0005](plans/0005-security-role-model-and-moderation-commands.md)). ## Open Items diff --git a/docs/plans/0001-wheelmud-reconciliation-roadmap.md b/docs/plans/0001-wheelmud-reconciliation-roadmap.md index abc713c..e738f8d 100644 --- a/docs/plans/0001-wheelmud-reconciliation-roadmap.md +++ b/docs/plans/0001-wheelmud-reconciliation-roadmap.md @@ -29,7 +29,8 @@ reconciliation effort stands. Accepted, PLAN-0004 Done. See [PLAN-0004](0004-session-state-machine-and-reconnect.md). - [ ] **Slice 3 — Permission/security-role model + moderation commands.** - Not yet designed. + ADR-0005 Accepted, PLAN-0005 Not Started. See + [PLAN-0005](0005-security-role-model-and-moderation-commands.md). - [ ] **Slice 4 — World-building/OLC command surface.** Not yet designed; bundles with Slice 3. - [ ] **Slice 5 — Help system.** Not yet designed. diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md new file mode 100644 index 0000000..f6aecc8 --- /dev/null +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -0,0 +1,200 @@ +# [PLAN-0005] Security Role Model + Moderation Commands + +**Implements:** [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md) + +**Status:** Not Started + +**Last updated:** 2026-07-17 + +## Goal + +A `MinorAdmin`/`FullAdmin`-gated set of moderation commands +(`boot`/`mute`/`unmute`/`announce`/`ban`/`unban`/`rolegrant`/`rolerevoke`) +works end-to-end over real Telnet, no command can be registered without an +explicit access-level declaration, mute/ban are enforced against the +right targets, and a `SHARPMUD_INITIAL_ADMIN` env var bootstraps the first +`FullAdmin` without an in-game path. + +## Scope + +Per ADR-0005's Decision Outcome. In scope: `SecurityRole` enum, +`RoleGuardedCommand`/`MuteGuardedCommand` decorators, the +`RegisterOpen`/`RegisterWithRole` registry split (and updating every +existing registration call site), `PlayerBehavior.Roles`/`IsMuted`/ +`IsBanned`, the 8 moderation commands above, `LoginFlow` ban enforcement, +`SHARPMUD_INITIAL_ADMIN` bootstrap. + +Explicitly deferred (per ADR-0005): `Find`/`Locate`/`GoTo`/`Control` +(need world/NPC lookup + puppeting), `Clone`/`Spawn` (need item/NPC +creation tooling), `Jail` (needs a cell-room concept), `Buff` (needs a +generic stat-modification system), `Relinquish` (Slice 4/builder-specific). +Audit logging of moderation actions — not in ADR-0005's scope, flagged as +an open item below. + +## Tasks + +### `SecurityRole` + registry + +- [ ] New `src/SharpMud.Engine/Commands/SecurityRole.cs` — plain + `[Flags] enum SecurityRole : uint` (`None`, `Mobile`, `Item`, `Room`, + `TutorialPlayer`, `Player`, `Helper`, `Married`, `MinorBuilder`, + `FullBuilder`, `MinorAdmin`, `FullAdmin`, `All`), XML doc comments + per `documentation.md`'s new-public-member rule. +- [ ] New `src/SharpMud.Engine/Commands/RoleGuardedCommand.cs` — wraps an + inner `ICommand`, checks `(actor.Roles & requiredRole) != + SecurityRole.None` before delegating; generic rejection message. +- [ ] New `src/SharpMud.Engine/Commands/MuteGuardedCommand.cs` — wraps an + inner `ICommand`, checks the *actor's own* `IsMuted` (not a target's + — this gates the muted player's own `say`/`emote`, not something + they're doing to someone else) before delegating. +- [ ] `ICommandRegistry.cs` / `CommandRegistry.cs`: remove `Register + (ICommand)` from the public surface; add `RegisterOpen(ICommand)` + and `RegisterWithRole(ICommand, SecurityRole)` (the latter wraps in + `RoleGuardedCommand` before storing). +- [ ] Update every existing registration call site (`BuiltinCommands + .RegisterAll`, `ClassicCommands.RegisterAll`) from `Register` to + `RegisterOpen` — mechanical, no behavior change. + +### `PlayerBehavior` + persistence + +- [ ] `PlayerBehavior.cs`: add `SecurityRole Roles { get; private set; } = + SecurityRole.Player`, `bool IsMuted { get; private set; }`, `bool + IsBanned { get; private set; }`, plus mutation methods + (`GrantRole`/`RevokeRole`, `Mute`/`Unmute`, `Ban`/`Unban`) rather than + public setters, matching `ConnectionState`'s existing + transition-method style. +- [ ] `PlayerBehaviorConfiguration.cs`: map `Roles` with the plain-enum + default EF conversion (matching `WearableBehaviorConfiguration`'s + `Slot` precedent — no custom value converter needed); map `IsMuted`/ + `IsBanned` as plain persisted columns (NOT `Ignore`d — unlike + `ConnectionState`, these must survive a restart). + +### Moderation commands (`src/SharpMud.Engine/Commands/Builtin/Admin/`) + +- [ ] `BootCommand` (`MinorAdmin`) — disconnects a currently-online target + by username; "not online" message if not found live. +- [ ] `MuteCommand`/`UnmuteCommand` (`MinorAdmin`) — sets/clears + `IsMuted` on a target (online-or-not, mirrors `LoginFlow`'s + live-then-repository lookup), saves immediately. +- [ ] `AnnounceCommand` (`MinorAdmin`) — broadcasts to every session in + `world.AllWithBehavior()` with a live session + (`WhoCommand`'s iteration pattern). +- [ ] `BanCommand`/`UnbanCommand` (`FullAdmin`) — sets/clears `IsBanned`, + online-or-not lookup, saves immediately. +- [ ] `RoleGrantCommand`/`RoleRevokeCommand` (`FullAdmin`) — mutates a + target's `Roles`, online-or-not lookup, saves immediately; validate + the role name argument against `SecurityRole`'s named values. +- [ ] Register all 8 via `RegisterWithRole` in a new + `AdminCommands.RegisterAll(registry)` (mirrors `BuiltinCommands`/ + `ClassicCommands`'s shape), called from `Program.cs`. Wrap `say`/ + `emote`'s existing registrations in `MuteGuardedCommand` at the same + call site. + +### Login-flow + bootstrap + +- [ ] `LoginFlow.LoginExistingAsync`: after password verification + succeeds, check `IsBanned` → reject with a distinct message before + the `ConnectionState` branch. +- [ ] `HostOptions.cs`: add `string? InitialAdminUsername`, parsed from + `SHARPMUD_INITIAL_ADMIN`. +- [ ] `Program.cs`: after world load/build, if `InitialAdminUsername` is + set and that username's character exists (live or via repository), + idempotently ensure it has `FullAdmin` (grant + save if not already + present). + +### Docs + +- [ ] `docs/commands.md`: describe the new admin command set as current + state, link ADR-0005. +- [ ] `docs/accounts-auth.md`: describe ban enforcement in the login flow + as current state, update the existing "deferred moderation tooling" + forward-reference to point at ADR-0005/this plan. +- [ ] `SPEC.md`: update the "Moderation/admin tooling" Deferred/Open Item + to reflect what's actually implemented vs. still deferred (Find/ + GoTo/Control/Clone/Spawn/Jail/Buff/Relinquish, audit logging). +- [ ] `docs/adr/README.md` / `docs/plans/README.md`: index rows for + ADR-0005/PLAN-0005. +- [ ] `docs/plans/0001-wheelmud-reconciliation-roadmap.md`: check off + Slice 3. + +## Critical files + +New: +- `src/SharpMud.Engine/Commands/SecurityRole.cs` +- `src/SharpMud.Engine/Commands/RoleGuardedCommand.cs` +- `src/SharpMud.Engine/Commands/MuteGuardedCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/BootCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/MuteCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/UnmuteCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/AnnounceCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/BanCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/UnbanCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/RoleGrantCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/RoleRevokeCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/AdminCommands.cs` +- Matching test files under `tests/SharpMud.Engine.Tests/Commands/...` + +Modified: +- `src/SharpMud.Engine/Commands/ICommandRegistry.cs`, + `CommandRegistry.cs` +- `src/SharpMud.Engine/Commands/BuiltinCommands.cs` (or wherever + `RegisterAll` lives), `src/SharpMud.Ruleset.Classic/ClassicCommands.cs` +- `src/SharpMud.Engine/Behaviors/PlayerBehavior.cs` +- `src/SharpMud.Persistence/Configurations/PlayerBehaviorConfiguration.cs` +- `src/SharpMud.Host/LoginFlow.cs`, `HostOptions.cs`, `Program.cs` +- `docs/commands.md`, `docs/accounts-auth.md`, `SPEC.md`, + `docs/adr/README.md`, `docs/plans/README.md`, + `docs/plans/0001-wheelmud-reconciliation-roadmap.md` + +## Test plan + +- Unit: `RoleGuardedCommand` — actor with the required role reaches the + inner command; actor without it doesn't, gets the rejection message. + Cover the any-of/bitwise semantics (actor has one of several required + flags). +- Unit: `MuteGuardedCommand` — muted actor blocked, unmuted actor passes + through to the inner command. +- Unit: `CommandRegistry` — `RegisterOpen` resolves unconditionally; + `RegisterWithRole` resolves to a `RoleGuardedCommand` wrapping the given + command with the given role. +- Unit: each of the 8 admin commands — happy path (role holder, valid + target) and the online/offline target-lookup branches. +- Unit: `LoginFlow` — banned user rejected at password verification with + the correct message, not silently falling through. +- Unit: `PlayerBehavior.GrantRole`/`RevokeRole`/`Mute`/`Unmute`/`Ban`/ + `Unban` — state mutates as expected. +- Unit: `HostOptions.Parse` — `SHARPMUD_INITIAL_ADMIN` parses correctly, + absent env var leaves it null. + +## Verification + +Real manual check over Telnet (this repo's established pattern for +session/persistence-facing changes): + +1. Boot with `SHARPMUD_INITIAL_ADMIN=` set, create that + character, confirm `Roles` includes `FullAdmin` (e.g. via a debug + `roles` self-check, or by successfully running a `FullAdmin`-gated + command). +2. As that admin, `rolegrant minoradmin` on a second + character; confirm the second character can now run `boot`/`mute`/ + `announce` but not `ban`/`rolegrant`. +3. `mute `; confirm that player's `say`/`emote` are blocked with a + clear message, `unmute` restores them. +4. `ban `; confirm that player can no longer log in (distinct + message, not the generic "incorrect" one); `unban` restores login. +5. `boot `; confirm their session is disconnected immediately. +6. `announce `; confirm every currently-connected session + receives it. +7. Confirm a non-admin attempting any of the above gets a clear rejection, + not a crash or a silent no-op. +8. Restart the server; confirm `Roles`/`IsMuted`/`IsBanned` all survived + (unlike `ConnectionState`, which is intentionally runtime-only). + +## Open questions / blockers + +- Audit logging of moderation actions (who banned/muted whom, when) is + explicitly not in ADR-0005's scope — flagged here as a likely near-term + follow-up once this lands, not designed yet. +- Exact rejection message wording for gated commands is a placeholder + ("You don't have permission to do that.") — not a considered final + string. diff --git a/docs/plans/README.md b/docs/plans/README.md index 4ef1919..aae3dcd 100644 --- a/docs/plans/README.md +++ b/docs/plans/README.md @@ -59,3 +59,4 @@ isn't settled yet — don't do that. | [0001](0001-wheelmud-reconciliation-roadmap.md) | [ADR-0001](../adr/0001-wheelmud-reconciliation-roadmap.md) | In Progress | | [0002](0002-telnet-protocol-negotiation.md) | [ADR-0002](../adr/0002-telnet-protocol-negotiation.md) | Done | | [0004](0004-session-state-machine-and-reconnect.md) | [ADR-0004](../adr/0004-session-state-machine-and-reconnect.md) | Done | +| [0005](0005-security-role-model-and-moderation-commands.md) | [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md) | Not Started | From 08a35b9279bb98fd03c03119f52fd629218730f1 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 17 Jul 2026 14:01:47 -0400 Subject: [PATCH 02/15] Address PR review feedback on Slice 3 design (ADR-0005/PLAN-0005) - Role accumulation: GrantRole(FullAdmin) now also grants MinorAdmin + Player (FullBuilder -> MinorBuilder), matching WheelMUD's actual behavior. Without this, a FullAdmin-only user failed every MinorAdmin-gated command since they're independent bits with no inherent relationship - the bootstrap admin couldn't have run day-to-day moderation commands. - Bootstrap timing: SHARPMUD_INITIAL_ADMIN is now checked in two places, not one - at boot (existing-world restart) AND at character-creation time in LoginFlow.MaybeCreateAsync (fresh-server case). A boot-time-only check was a no-op on a genuinely fresh server, since the target character doesn't exist yet at boot. - Repository access: the 6 admin commands needing offline target lookup (Mute/Unmute/Ban/Unban/RoleGrant/RoleRevoke) get IThingRepository via their own constructor, matching the existing ClassicCommands.RegisterAll pattern - not a CommandContext change. - Fixed ADR wording: MuteGuardedCommand checks the actor's own IsMuted (the speaker being blocked), not a "target's" - the plan already had this right, only the ADR prose was wrong. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 43 +++++-- ...rity-role-model-and-moderation-commands.md | 112 +++++++++++++----- 2 files changed, 115 insertions(+), 40 deletions(-) diff --git a/docs/adr/0005-security-role-model-and-moderation-commands.md b/docs/adr/0005-security-role-model-and-moderation-commands.md index b25c7db..8b3425d 100644 --- a/docs/adr/0005-security-role-model-and-moderation-commands.md +++ b/docs/adr/0005-security-role-model-and-moderation-commands.md @@ -127,13 +127,29 @@ just another wrapper of the same shape) without DecoWeaver's registration it's useless; default `Player` for new characters), plus `bool IsMuted` and `bool IsBanned` (also persisted; separate from `Roles` — a restriction, not a capability). +- **Roles accumulate at grant time, matching WheelMUD's own behavior** + (confirmed during research: WheelMUD's `UserControlledBehavior + .SecurityRoles` is a bitwise-OR accumulation, e.g. "a promoted builder + keeps `player | minorBuilder | fullBuilder`," not just the newest tier's + bit alone). `PlayerBehavior.GrantRole(SecurityRole role)` ORs in the + requested role *and* every tier it implies: `FullAdmin` implies + `MinorAdmin` implies `Player`; `FullBuilder` implies `MinorBuilder`. + This matters because `RoleGuardedCommand`'s check stays a simple + bitwise AND with no hierarchy logic of its own — without accumulation, + a user granted only `FullAdmin` would fail every `MinorAdmin`-gated + command (`boot`/`mute`/`unmute`/`announce`), since `FullAdmin` and + `MinorAdmin` are independent bits with no inherent relationship. Caught + during PR review (the bootstrap admin would otherwise be unable to run + day-to-day moderation commands) — see `SHARPMUD_INITIAL_ADMIN` below, + which relies on this same accumulation. - `RoleGuardedCommand` checks `(actor.Roles & requiredRole) != SecurityRole.None` (any-of semantics, matching WheelMUD's own bitwise check) before delegating to the inner command. The same Decorator shape is reused for mute enforcement (`MuteGuardedCommand`, wrapping `say`/ - `emote`, checking the *target*'s `IsMuted` rather than the actor's role) - — validating the pattern's reusability for a cross-cutting concern that - isn't role-based at all. + `emote`, checking the *actor's own* `IsMuted` — it's the speaker being + blocked from speaking, not anything about a target) — validating the + pattern's reusability for a cross-cutting concern that isn't role-based + at all. - `ICommandRegistry.Register(ICommand)` is removed from the public interface; `RegisterOpen(ICommand)` and `RegisterWithRole(ICommand, SecurityRole)` are the only ways in. Every existing command's @@ -168,11 +184,22 @@ login). **Bootstrap**: `HostOptions` gains `SHARPMUD_INITIAL_ADMIN` (env var, matching the existing `SHARPMUD_MODE`/`SHARPMUD_TELNET_PORT`/ -`SHARPMUD_DB_PATH` precedent). On boot, if a character with that username -exists, the host ensures it has `FullAdmin` — idempotent, safe to leave -set permanently or unset after first use. Solves "how does the first -admin ever get `FullAdmin`" without an in-game path that would otherwise -be a chicken-and-egg dead end. +`SHARPMUD_DB_PATH` precedent). The grant is checked in **two** places, not +just one — caught during PR review that checking only once at boot is a +no-op on a genuinely fresh server, since the target character doesn't +exist yet at boot time and only gets created later through the normal +login flow: +1. At boot (after world load), if a character with that username already + exists (a restart of an existing world) — the original case. +2. At the moment a character with that username is actually created + (`LoginFlow.MaybeCreateAsync`), covering the fresh-server case. + +Both paths are idempotent and go through the same `GrantRole(FullAdmin)` +call (which accumulates `MinorAdmin`/`Player` too, per the accumulation +rule above) — safe to leave `SHARPMUD_INITIAL_ADMIN` set permanently, or +unset after first use. Solves "how does the first admin ever get +`FullAdmin`" without an in-game path that would otherwise be a +chicken-and-egg dead end. ### Positive Consequences diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index f6aecc8..4b36cc2 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -62,7 +62,11 @@ an open item below. IsBanned { get; private set; }`, plus mutation methods (`GrantRole`/`RevokeRole`, `Mute`/`Unmute`, `Ban`/`Unban`) rather than public setters, matching `ConnectionState`'s existing - transition-method style. + transition-method style. `GrantRole` accumulates implied lower tiers + (`FullAdmin` → also `MinorAdmin` + `Player`; `FullBuilder` → also + `MinorBuilder`) per ADR-0005's accumulation rule — a plain + `Roles |= role` is not enough on its own, `RevokeRole` only clears + the exact bit(s) passed in (no cascading revoke of implied tiers). - [ ] `PlayerBehaviorConfiguration.cs`: map `Roles` with the plain-enum default EF conversion (matching `WearableBehaviorConfiguration`'s `Slot` precedent — no custom value converter needed); map `IsMuted`/ @@ -71,22 +75,37 @@ an open item below. ### Moderation commands (`src/SharpMud.Engine/Commands/Builtin/Admin/`) -- [ ] `BootCommand` (`MinorAdmin`) — disconnects a currently-online target - by username; "not online" message if not found live. -- [ ] `MuteCommand`/`UnmuteCommand` (`MinorAdmin`) — sets/clears - `IsMuted` on a target (online-or-not, mirrors `LoginFlow`'s - live-then-repository lookup), saves immediately. -- [ ] `AnnounceCommand` (`MinorAdmin`) — broadcasts to every session in - `world.AllWithBehavior()` with a live session - (`WhoCommand`'s iteration pattern). -- [ ] `BanCommand`/`UnbanCommand` (`FullAdmin`) — sets/clears `IsBanned`, - online-or-not lookup, saves immediately. -- [ ] `RoleGrantCommand`/`RoleRevokeCommand` (`FullAdmin`) — mutates a - target's `Roles`, online-or-not lookup, saves immediately; validate +`Mute`/`Unmute`/`Ban`/`Unban`/`RoleGrant`/`RoleRevoke` need `IThingRepository` +for offline target lookup + immediate saves — `CommandContext` only carries +`World`/`Session`, not the repository, and today the repository is only +available inside `SessionLoop`. Rather than extending `CommandContext` +(bigger blast radius, touches every command's context), these six commands +take `IThingRepository` via their own constructor — the same shape +`ClassicCommands.RegisterAll` already uses for `combatManager`/`random`. +`Boot`/`Announce` don't need it (online-only / broadcast-only). + +- [ ] `BootCommand` (`MinorAdmin`, no repository dependency) — disconnects + a currently-online target by username; "not online" message if not + found live. +- [ ] `MuteCommand`/`UnmuteCommand` (`MinorAdmin`, `IThingRepository`) — + sets/clears `IsMuted` on a target (online-or-not, mirrors + `LoginFlow`'s live-then-repository lookup), saves immediately. +- [ ] `AnnounceCommand` (`MinorAdmin`, no repository dependency) — + broadcasts to every session in `world.AllWithBehavior()` + with a live session (`WhoCommand`'s iteration pattern). +- [ ] `BanCommand`/`UnbanCommand` (`FullAdmin`, `IThingRepository`) — + sets/clears `IsBanned`, online-or-not lookup, saves immediately. +- [ ] `RoleGrantCommand`/`RoleRevokeCommand` (`FullAdmin`, + `IThingRepository`) — mutates a target's `Roles` via + `GrantRole`/`RevokeRole` (accumulation happens inside `GrantRole` + itself, not here), online-or-not lookup, saves immediately; validate the role name argument against `SecurityRole`'s named values. - [ ] Register all 8 via `RegisterWithRole` in a new - `AdminCommands.RegisterAll(registry)` (mirrors `BuiltinCommands`/ - `ClassicCommands`'s shape), called from `Program.cs`. Wrap `say`/ + `AdminCommands.RegisterAll(registry, repository)` (mirrors + `BuiltinCommands`/`ClassicCommands`'s shape — `ClassicCommands + .RegisterAll` already takes extra constructed dependencies the same + way), called from `Program.cs` alongside the existing `RegisterAll` + calls, passing the already-constructed `repository`. Wrap `say`/ `emote`'s existing registrations in `MuteGuardedCommand` at the same call site. @@ -97,10 +116,21 @@ an open item below. the `ConnectionState` branch. - [ ] `HostOptions.cs`: add `string? InitialAdminUsername`, parsed from `SHARPMUD_INITIAL_ADMIN`. -- [ ] `Program.cs`: after world load/build, if `InitialAdminUsername` is - set and that username's character exists (live or via repository), - idempotently ensure it has `FullAdmin` (grant + save if not already - present). +- [ ] Bootstrap the grant in **two** places, not just one — a boot-time-only + check is a no-op on a genuinely fresh server, since the target + character doesn't exist yet at boot and only gets created later + through the normal login flow (caught in PR review): + - [ ] `Program.cs`: after world load/build, if `InitialAdminUsername` + is set and that username's character already exists (live or + via repository — the "restart of an existing world" case), + idempotently `GrantRole(FullAdmin)` + save if not already + present. + - [ ] `LoginFlow.MaybeCreateAsync`: after a new character is created + and saved, if its username matches `InitialAdminUsername`, + `GrantRole(FullAdmin)` + save again (the fresh-server case). + Both paths call the same `PlayerBehavior.GrantRole(SecurityRole + .FullAdmin)`, so both get the accumulation behavior (also granting + `MinorAdmin`/`Player`) for free. ### Docs @@ -162,33 +192,51 @@ Modified: - Unit: `LoginFlow` — banned user rejected at password verification with the correct message, not silently falling through. - Unit: `PlayerBehavior.GrantRole`/`RevokeRole`/`Mute`/`Unmute`/`Ban`/ - `Unban` — state mutates as expected. + `Unban` — state mutates as expected. Specifically cover accumulation: + `GrantRole(FullAdmin)` results in `Roles` containing `FullAdmin`, + `MinorAdmin`, *and* `Player`; `GrantRole(FullBuilder)` results in + `FullBuilder` + `MinorBuilder`; `RevokeRole` only clears the exact bit + passed, not implied lower tiers. +- Unit: a `FullAdmin`-only actor (post-accumulation) successfully passes a + `MinorAdmin`-gated `RoleGuardedCommand` — the regression test for the + bootstrap-admin-can't-moderate gap caught in PR review. - Unit: `HostOptions.Parse` — `SHARPMUD_INITIAL_ADMIN` parses correctly, absent env var leaves it null. +- Unit: bootstrap grants `FullAdmin` via both paths independently — the + `Program.cs` existing-character path, and `LoginFlow.MaybeCreateAsync`'s + newly-created-character path — since a boot-time-only check was the + fresh-server gap caught in PR review. ## Verification Real manual check over Telnet (this repo's established pattern for session/persistence-facing changes): -1. Boot with `SHARPMUD_INITIAL_ADMIN=` set, create that - character, confirm `Roles` includes `FullAdmin` (e.g. via a debug - `roles` self-check, or by successfully running a `FullAdmin`-gated - command). -2. As that admin, `rolegrant minoradmin` on a second +1. **Fresh-server case** (the gap caught in PR review): boot a brand-new + world with `SHARPMUD_INITIAL_ADMIN=` set, and only *then* + create that character over Telnet — confirm the grant happens at + creation time, not just at boot. Confirm the resulting admin can run + *both* a `FullAdmin`-gated command (e.g. `ban`) *and* a + `MinorAdmin`-gated one (e.g. `boot`) without a separate grant — + validates the accumulation fix. +2. **Restart case**: restart the server (same world, same + `SHARPMUD_INITIAL_ADMIN`); confirm the existing admin's roles are + unaffected (idempotent, no duplicate grants/errors). +3. As the admin, `rolegrant minoradmin` on a second character; confirm the second character can now run `boot`/`mute`/ `announce` but not `ban`/`rolegrant`. -3. `mute `; confirm that player's `say`/`emote` are blocked with a +4. `mute `; confirm that player's `say`/`emote` are blocked with a clear message, `unmute` restores them. -4. `ban `; confirm that player can no longer log in (distinct +5. `ban `; confirm that player can no longer log in (distinct message, not the generic "incorrect" one); `unban` restores login. -5. `boot `; confirm their session is disconnected immediately. -6. `announce `; confirm every currently-connected session +6. `boot `; confirm their session is disconnected immediately. +7. `announce `; confirm every currently-connected session receives it. -7. Confirm a non-admin attempting any of the above gets a clear rejection, +8. Confirm a non-admin attempting any of the above gets a clear rejection, not a crash or a silent no-op. -8. Restart the server; confirm `Roles`/`IsMuted`/`IsBanned` all survived - (unlike `ConnectionState`, which is intentionally runtime-only). +9. Restart the server again; confirm `Roles`/`IsMuted`/`IsBanned` all + survived (unlike `ConnectionState`, which is intentionally + runtime-only). ## Open questions / blockers From 1770819220b6b5b23c508a8f85b98bb4c0e014a4 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 17 Jul 2026 14:31:33 -0400 Subject: [PATCH 03/15] Correct false codebase claim in ADR-0005 about a [Flags] precedent The ADR cited WearableBehaviorConfiguration/Slot as an existing plain [Flags] bitmask precedent. EquipSlot (the actual type behind Slot) is a plain enum, not [Flags] - there is no existing [Flags] enum anywhere in this repo today. The "plain enum over OptimizedEnum" part of the citation was accurate and is kept; SecurityRole is correctly described as the first genuinely bitwise-combinable [Flags] enum in the codebase, not a second instance of an existing pattern. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/docs/adr/0005-security-role-model-and-moderation-commands.md b/docs/adr/0005-security-role-model-and-moderation-commands.md index 8b3425d..3a0604e 100644 --- a/docs/adr/0005-security-role-model-and-moderation-commands.md +++ b/docs/adr/0005-security-role-model-and-moderation-commands.md @@ -59,12 +59,16 @@ high-tier roles — no separate mechanism from any other command. toward something structurally harder to forget than "paste a guard call at the top of `ExecuteAsync`." - `docs/persistence.md`/`WearableBehaviorConfiguration.cs`'s existing - precedent: a genuine bitmask (`Slot`) is a **plain** `[Flags] enum` with - EF Core's default int mapping, not a `LayeredCraft.OptimizedEnums` - instance — `OptimizedEnum` in this repo is for small non-combinable - state machines (`Race`, `CharacterClass`, `ConnectionState`), not - bitwise-combinable flag sets. `SecurityRole` follows the plain-enum - precedent. + precedent: `WearableBehavior.Slot` (backed by `EquipSlot`) is a **plain** + C# enum with EF Core's default int mapping, not a + `LayeredCraft.OptimizedEnums` instance — confirmed, `OptimizedEnum` in + this repo is for small non-combinable state machines (`Race`, + `CharacterClass`, `ConnectionState`). Corrected during PR review: + `EquipSlot` is **not** itself `[Flags]` (it's an ordinary enum, not a + bitmask), and there is no existing `[Flags]` enum anywhere in this repo + today — `SecurityRole` follows `Slot`'s "plain enum over `OptimizedEnum`" + precedent, but is the first genuinely bitwise-combinable `[Flags]` enum + in the codebase, not a second instance of an existing pattern. - Bootstrap problem, surfaced during the dive: if granting a role itself requires `FullAdmin`, and every new character defaults to `Player`, nothing in-game can ever produce the first admin. @@ -303,8 +307,9 @@ chicken-and-egg dead end. - `docs/commands.md` — command pipeline (`ICommand`/`ICommandRegistry`) this ADR extends. - `docs/persistence.md` / `WearableBehaviorConfiguration.cs` — the - existing plain-`[Flags]`-enum-with-default-EF-mapping precedent - `SecurityRole` follows (as opposed to `LayeredCraft.OptimizedEnums`). + existing plain-enum-with-default-EF-mapping precedent `SecurityRole` + follows for "plain enum, not `OptimizedEnum`" (`EquipSlot` itself is not + `[Flags]` — corrected during PR review). - `WheelMUD/src/Core/Attributes/ActionSecurityAttribute.cs`, `WheelMUD/src/Core/ManagerSystems/CommandManager.cs`, `WheelMUD/src/Core/CommandSystem/CommandGuard.cs`, From da4eab082cd74f5b4ed539a4ee0be5bbdf38f61e Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 17 Jul 2026 14:41:30 -0400 Subject: [PATCH 04/15] Enforce the role hierarchy invariant symmetrically on revoke too RevokeRole could clear a lower tier (e.g. MinorAdmin) while a higher tier that implies it (FullAdmin) remained set, breaking "FullAdmin implies MinorAdmin" for that user going forward - the same class of bug as the earlier grant-side gap, just on the way out instead of the way in. RevokeRole now rejects a revoke of a tier still implied by another currently-held role, naming the blocking tier, rather than silently applying an inconsistent state. Revoking the top tier itself (FullAdmin) still works normally and correctly leaves the tiers it implied (MinorAdmin/Player) intact - a demotion, not a cascading reset. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 10 +++- ...rity-role-model-and-moderation-commands.md | 49 +++++++++++++++---- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/docs/adr/0005-security-role-model-and-moderation-commands.md b/docs/adr/0005-security-role-model-and-moderation-commands.md index 3a0604e..3fb5d4c 100644 --- a/docs/adr/0005-security-role-model-and-moderation-commands.md +++ b/docs/adr/0005-security-role-model-and-moderation-commands.md @@ -145,7 +145,15 @@ just another wrapper of the same shape) without DecoWeaver's registration `MinorAdmin` are independent bits with no inherent relationship. Caught during PR review (the bootstrap admin would otherwise be unable to run day-to-day moderation commands) — see `SHARPMUD_INITIAL_ADMIN` below, - which relies on this same accumulation. + which relies on this same accumulation. The same invariant has to hold + on the way out, not just the way in — also caught during PR review: + `PlayerBehavior.RevokeRole(SecurityRole role)` rejects (rather than + silently applying) a revoke of a tier still implied by a higher one the + target currently holds — e.g. revoking `MinorAdmin` from someone who + still has `FullAdmin` would otherwise leave `FullAdmin` set with + `MinorAdmin` cleared, breaking "`FullAdmin` implies `MinorAdmin`" for + that user going forward. The rejection names the blocking higher tier + so the admin knows to revoke that one first (or instead). - `RoleGuardedCommand` checks `(actor.Roles & requiredRole) != SecurityRole.None` (any-of semantics, matching WheelMUD's own bitwise check) before delegating to the inner command. The same Decorator shape diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index 4b36cc2..30bcb49 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -62,11 +62,30 @@ an open item below. IsBanned { get; private set; }`, plus mutation methods (`GrantRole`/`RevokeRole`, `Mute`/`Unmute`, `Ban`/`Unban`) rather than public setters, matching `ConnectionState`'s existing - transition-method style. `GrantRole` accumulates implied lower tiers - (`FullAdmin` → also `MinorAdmin` + `Player`; `FullBuilder` → also - `MinorBuilder`) per ADR-0005's accumulation rule — a plain - `Roles |= role` is not enough on its own, `RevokeRole` only clears - the exact bit(s) passed in (no cascading revoke of implied tiers). + transition-method style. + - [ ] `GrantRole(SecurityRole role)`: ORs in `role` *and* every tier + it implies (`FullAdmin` → also `MinorAdmin` + `Player`; + `FullBuilder` → also `MinorBuilder`) per ADR-0005's + accumulation rule — a plain `Roles |= role` is not enough on + its own. + - [ ] A static `SecurityRole.Implies(SecurityRole role)` (or + equivalent lookup) expressing the same hierarchy + (`FullAdmin`→`MinorAdmin`→`Player`, `FullBuilder`→ + `MinorBuilder`) — used by both `GrantRole` (to accumulate + downward) and `RevokeRole` (to check upward, see next) so the + hierarchy is defined once, not duplicated between the two + directions. + - [ ] `RevokeRole(SecurityRole role)`: **enforces the same + invariant symmetrically** (caught in PR review — clearing + only the exact bit passed in can leave a higher tier set with + a lower tier it implies cleared, e.g. `FullAdmin` present but + `MinorAdmin` cleared after revoking just `MinorAdmin`). Before + clearing, check whether any *other* currently-held role + implies `role`; if so, throw/reject (surfaced by + `RoleRevokeCommand` as a message naming the blocking higher + tier — "still has FullAdmin, which includes MinorAdmin — + revoke FullAdmin instead") rather than silently applying an + inconsistent state. - [ ] `PlayerBehaviorConfiguration.cs`: map `Roles` with the plain-enum default EF conversion (matching `WearableBehaviorConfiguration`'s `Slot` precedent — no custom value converter needed); map `IsMuted`/ @@ -97,9 +116,13 @@ take `IThingRepository` via their own constructor — the same shape sets/clears `IsBanned`, online-or-not lookup, saves immediately. - [ ] `RoleGrantCommand`/`RoleRevokeCommand` (`FullAdmin`, `IThingRepository`) — mutates a target's `Roles` via - `GrantRole`/`RevokeRole` (accumulation happens inside `GrantRole` - itself, not here), online-or-not lookup, saves immediately; validate - the role name argument against `SecurityRole`'s named values. + `GrantRole`/`RevokeRole` (accumulation/hierarchy-invariant + enforcement happens inside `PlayerBehavior` itself, not here), + online-or-not lookup, saves immediately; validate the role name + argument against `SecurityRole`'s named values. `RoleRevokeCommand` + catches `RevokeRole`'s rejection (target still holds a higher tier + that implies the requested one) and surfaces it as a clear message + naming the blocking tier, not a crash. - [ ] Register all 8 via `RegisterWithRole` in a new `AdminCommands.RegisterAll(registry, repository)` (mirrors `BuiltinCommands`/`ClassicCommands`'s shape — `ClassicCommands @@ -195,11 +218,17 @@ Modified: `Unban` — state mutates as expected. Specifically cover accumulation: `GrantRole(FullAdmin)` results in `Roles` containing `FullAdmin`, `MinorAdmin`, *and* `Player`; `GrantRole(FullBuilder)` results in - `FullBuilder` + `MinorBuilder`; `RevokeRole` only clears the exact bit - passed, not implied lower tiers. + `FullBuilder` + `MinorBuilder`. - Unit: a `FullAdmin`-only actor (post-accumulation) successfully passes a `MinorAdmin`-gated `RoleGuardedCommand` — the regression test for the bootstrap-admin-can't-moderate gap caught in PR review. +- Unit: `RevokeRole(MinorAdmin)` on an actor who also holds `FullAdmin` + is rejected (`Roles` unchanged, exception/rejection surfaced) — the + regression test for the revoke-side hierarchy gap caught in PR review. + `RevokeRole(FullAdmin)` on that same actor succeeds and leaves + `MinorAdmin`/`Player` intact (demotion, not a full reset — revoking the + top tier doesn't cascade-clear what it implied). `RevokeRole(MinorAdmin)` + on an actor who does *not* also hold `FullAdmin` succeeds normally. - Unit: `HostOptions.Parse` — `SHARPMUD_INITIAL_ADMIN` parses correctly, absent env var leaves it null. - Unit: bootstrap grants `FullAdmin` via both paths independently — the From aee0ab9292be525feb3d0af905d6108ce8af15cf Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 17 Jul 2026 14:53:19 -0400 Subject: [PATCH 05/15] Reject All/None as rolegrant/rolerevoke targets Validating a role name against "is this a real SecurityRole name" would also accept All (every current AND future flag - not a real assignable tier, a severe over-grant) and None (a meaningless no-op sentinel), since both are literally named enum members. Restricted to an explicit allowlist of individually-grantable roles instead, rejecting All/None with a clear message. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index 30bcb49..9cc0cc0 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -118,10 +118,17 @@ take `IThingRepository` via their own constructor — the same shape `IThingRepository`) — mutates a target's `Roles` via `GrantRole`/`RevokeRole` (accumulation/hierarchy-invariant enforcement happens inside `PlayerBehavior` itself, not here), - online-or-not lookup, saves immediately; validate the role name - argument against `SecurityRole`'s named values. `RoleRevokeCommand` - catches `RevokeRole`'s rejection (target still holds a higher tier - that implies the requested one) and surfaces it as a clear message + online-or-not lookup, saves immediately. Validate the role name + argument against an **explicit allowlist of individually-grantable + roles — not just "is this a real `SecurityRole` name."** Caught in + PR review: a plain `Enum.TryParse` would also accept + `All` (every current *and future* flag — not a real assignable + tier, a severe over-grant) and `None` (a meaningless no-op + sentinel), since both are literally named enum members. Reject + either with a clear message rather than silently persisting them. + `RoleRevokeCommand` catches `RevokeRole`'s rejection (target still + holds a higher tier that implies the requested one) and surfaces it + as a clear message naming the blocking tier, not a crash. - [ ] Register all 8 via `RegisterWithRole` in a new `AdminCommands.RegisterAll(registry, repository)` (mirrors @@ -212,6 +219,11 @@ Modified: command with the given role. - Unit: each of the 8 admin commands — happy path (role holder, valid target) and the online/offline target-lookup branches. +- Unit: `RoleGrantCommand`/`RoleRevokeCommand` — `all` and `none` (any + casing) are rejected with a clear message and never reach + `GrantRole`/`RevokeRole`; every other individually-grantable role name + is accepted. The regression test for the `All`/sentinel-value gap + caught in PR review. - Unit: `LoginFlow` — banned user rejected at password verification with the correct message, not silently falling through. - Unit: `PlayerBehavior.GrantRole`/`RevokeRole`/`Mute`/`Unmute`/`Ban`/ From ff502de0c0df82d2d444566b6cacb0d1f68815d5 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 18 Jul 2026 08:56:18 -0400 Subject: [PATCH 06/15] Specify explicit power-of-two values for every SecurityRole member The ADR/plan only listed member names with no values. If implemented literally, C# auto-numbers unnumbered enum members sequentially (0, 1, 2, 3...) which are not distinct bits - Room would silently auto-number to 3, equal to Mobile | Item combined, breaking the bitwise-AND role check into granting unrelated permissions on overlapping bits. Added explicit 1 << n values to every member in both documents, and defined All as the union of the individual flags rather than a separate hardcoded value so it can't drift out of sync if a flag is added later. Added a dedicated "no two roles share a bit" regression test to the test plan. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 21 ++++++++++++---- ...rity-role-model-and-moderation-commands.md | 24 ++++++++++++++++--- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/docs/adr/0005-security-role-model-and-moderation-commands.md b/docs/adr/0005-security-role-model-and-moderation-commands.md index 3fb5d4c..f885b57 100644 --- a/docs/adr/0005-security-role-model-and-moderation-commands.md +++ b/docs/adr/0005-security-role-model-and-moderation-commands.md @@ -117,11 +117,22 @@ just another wrapper of the same shape) without DecoWeaver's registration **Mechanism**, in full: - `SecurityRole` (`SharpMud.Engine.Commands`): a plain `[Flags] enum`, - full WheelMUD 12-value set ported (PascalCase: `None`, `Mobile`, `Item`, - `Room`, `TutorialPlayer`, `Player`, `Helper`, `Married`, `MinorBuilder`, - `FullBuilder`, `MinorAdmin`, `FullAdmin`, `All`) — adopted in full even - though several values (`Mobile`/`Item`/`Room`: sharp-mud has no - non-player command issuer today; `Married`: no marriage system; + full WheelMUD 12-value set ported (PascalCase: `None = 0`, `Mobile = 1 + << 0`, `Item = 1 << 1`, `Room = 1 << 2`, `TutorialPlayer = 1 << 3`, + `Player = 1 << 4`, `Helper = 1 << 5`, `Married = 1 << 6`, `MinorBuilder + = 1 << 7`, `FullBuilder = 1 << 8`, `MinorAdmin = 1 << 9`, `FullAdmin = + 1 << 10`, `All = Mobile | Item | Room | TutorialPlayer | Player | + Helper | Married | MinorBuilder | FullBuilder | MinorAdmin | + FullAdmin`). **Explicit values are load-bearing, not stylistic** — + caught in PR review: without them, C# auto-numbers enum members + sequentially (`0, 1, 2, 3...`), which are *not* distinct bits (`Room` + would auto-number to `3`, silently equal to `Mobile | Item` combined, + and the `RoleGuardedCommand` bitwise-AND check would grant unrelated + permissions on overlapping bits). `All` is defined as the union of the + individual flags, not a separate hardcoded value (e.g. `uint.MaxValue`) + — so it can't drift out of sync if a flag is ever added later. Adopted + in full even though several values (`Mobile`/`Item`/`Room`: sharp-mud + has no non-player command issuer today; `Married`: no marriage system; `TutorialPlayer`/builder tiers: not yet used) are inert now, so future slices (world-building/OLC is Slice 4, explicitly bundled with this one in ADR-0001) already have a role to reach for instead of extending the diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index 9cc0cc0..f7c674a 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -36,9 +36,20 @@ an open item below. ### `SecurityRole` + registry - [ ] New `src/SharpMud.Engine/Commands/SecurityRole.cs` — plain - `[Flags] enum SecurityRole : uint` (`None`, `Mobile`, `Item`, `Room`, - `TutorialPlayer`, `Player`, `Helper`, `Married`, `MinorBuilder`, - `FullBuilder`, `MinorAdmin`, `FullAdmin`, `All`), XML doc comments + `[Flags] enum SecurityRole : uint` with **explicit power-of-two + values on every member** (`None = 0`, `Mobile = 1 << 0`, `Item = 1 + << 1`, `Room = 1 << 2`, `TutorialPlayer = 1 << 3`, `Player = 1 << + 4`, `Helper = 1 << 5`, `Married = 1 << 6`, `MinorBuilder = 1 << 7`, + `FullBuilder = 1 << 8`, `MinorAdmin = 1 << 9`, `FullAdmin = 1 << + 10`, `All = Mobile | Item | Room | TutorialPlayer | Player | Helper + | Married | MinorBuilder | FullBuilder | MinorAdmin | FullAdmin`). + **Do not rely on C#'s default sequential auto-numbering** — caught + in PR review: unnumbered members would auto-assign `0, 1, 2, 3...`, + which are not distinct bits (e.g. `Room` would silently equal + `Mobile | Item` combined), breaking `RoleGuardedCommand`'s bitwise + -AND check into granting unrelated permissions on overlapping bits. + `All` is a union expression, not a separate hardcoded value, so it + can't drift out of sync if a flag is added later. XML doc comments per `documentation.md`'s new-public-member rule. - [ ] New `src/SharpMud.Engine/Commands/RoleGuardedCommand.cs` — wraps an inner `ICommand`, checks `(actor.Roles & requiredRole) != @@ -208,6 +219,13 @@ Modified: ## Test plan +- Unit: `SecurityRole` — every named member (excluding `None`/`All`) is a + distinct power of two, and no two members share a bit + (`Enum.GetValues()` pairwise-AND'd should all be `None` + except `All`/self-comparisons). `All` equals the OR of every individual + flag. The regression test for the undefined-values gap caught in PR + review — this is the one test that would have caught it immediately if + the enum were ever implemented with auto-numbered members. - Unit: `RoleGuardedCommand` — actor with the required role reaches the inner command; actor without it doesn't, gets the rejection message. Cover the any-of/bitwise semantics (actor has one of several required From 3427721213cc94c7ad400aeacb277a05cced417b Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 18 Jul 2026 14:29:17 -0400 Subject: [PATCH 07/15] RevokeRole returns a failure reason instead of throwing "Revoking MinorAdmin while FullAdmin is held" is a normal, directly-user-triggerable business-rule outcome (an admin ran rolerevoke), not a bug or an invariant violation - per coding-standards.md's Error Handling section, that must be a return value the caller checks, not a thrown exception. Changed RevokeRole's signature to string? (null on success, a message naming the blocking tier on failure), mirroring the existing MoveRequest.CancelReason nullable-reason idiom rather than a new pattern. RoleRevokeCommand checks the return value; never wraps the call in try/catch. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 42 ++++++++++++------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index f7c674a..b2ad514 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -92,11 +92,21 @@ an open item below. a lower tier it implies cleared, e.g. `FullAdmin` present but `MinorAdmin` cleared after revoking just `MinorAdmin`). Before clearing, check whether any *other* currently-held role - implies `role`; if so, throw/reject (surfaced by - `RoleRevokeCommand` as a message naming the blocking higher - tier — "still has FullAdmin, which includes MinorAdmin — - revoke FullAdmin instead") rather than silently applying an - inconsistent state. + implies `role`; if so, **return a failure, don't throw** — + per `coding-standards.md`'s Error Handling section, this is a + normal, directly-user-triggerable business-rule outcome (an + admin typed a `rolerevoke` that doesn't make sense given the + target's current roles), not a bug or an invariant violation, + so it must be a return value the caller checks (caught in PR + review — the original wording said "throw/reject," which + contradicts the standard). Signature: + `string? RevokeRole(SecurityRole role)` — `null` on success, + a message naming the blocking higher tier on failure ("still + has FullAdmin, which includes MinorAdmin — revoke FullAdmin + instead"), mirroring the existing `MoveRequest.CancelReason` + nullable-reason idiom rather than inventing a new pattern. + `RoleRevokeCommand` relays a non-null return straight to the + admin; never wraps this call in a try/catch. - [ ] `PlayerBehaviorConfiguration.cs`: map `Roles` with the plain-enum default EF conversion (matching `WearableBehaviorConfiguration`'s `Slot` precedent — no custom value converter needed); map `IsMuted`/ @@ -137,10 +147,9 @@ take `IThingRepository` via their own constructor — the same shape tier, a severe over-grant) and `None` (a meaningless no-op sentinel), since both are literally named enum members. Reject either with a clear message rather than silently persisting them. - `RoleRevokeCommand` catches `RevokeRole`'s rejection (target still - holds a higher tier that implies the requested one) and surfaces it - as a clear message - naming the blocking tier, not a crash. + `RoleRevokeCommand` checks `RevokeRole`'s `string?` return (not a + caught exception — see the `PlayerBehavior` task above) and relays + a non-null failure straight to the admin as the rejection message. - [ ] Register all 8 via `RegisterWithRole` in a new `AdminCommands.RegisterAll(registry, repository)` (mirrors `BuiltinCommands`/`ClassicCommands`'s shape — `ClassicCommands @@ -253,12 +262,15 @@ Modified: `MinorAdmin`-gated `RoleGuardedCommand` — the regression test for the bootstrap-admin-can't-moderate gap caught in PR review. - Unit: `RevokeRole(MinorAdmin)` on an actor who also holds `FullAdmin` - is rejected (`Roles` unchanged, exception/rejection surfaced) — the - regression test for the revoke-side hierarchy gap caught in PR review. - `RevokeRole(FullAdmin)` on that same actor succeeds and leaves - `MinorAdmin`/`Player` intact (demotion, not a full reset — revoking the - top tier doesn't cascade-clear what it implied). `RevokeRole(MinorAdmin)` - on an actor who does *not* also hold `FullAdmin` succeeds normally. + returns a non-null failure message and leaves `Roles` unchanged — the + regression test for the revoke-side hierarchy gap caught in PR review + (and, separately, that the failure is a return value, not a thrown + exception, per `coding-standards.md`'s Error Handling section). + `RevokeRole(FullAdmin)` on that same actor returns `null` (success) and + leaves `MinorAdmin`/`Player` intact (demotion, not a full reset — + revoking the top tier doesn't cascade-clear what it implied). + `RevokeRole(MinorAdmin)` on an actor who does *not* also hold + `FullAdmin` returns `null` (success) normally. - Unit: `HostOptions.Parse` — `SHARPMUD_INITIAL_ADMIN` parses correctly, absent env var leaves it null. - Unit: bootstrap grants `FullAdmin` via both paths independently — the From 9bc37e103a5c04fe1c7043eff399c396ef78ebe4 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 18 Jul 2026 14:37:41 -0400 Subject: [PATCH 08/15] Add SHARPMUD_INITIAL_ADMIN to deployment.md's docs task docs/deployment.md's Runtime Configuration table documents every HostOptions env var, and SHARPMUD_INITIAL_ADMIN was missing from both the plan's docs checklist and Critical files list - the only bootstrap path to a FullAdmin in a fresh deployment, easy to miss and leave a container with no discoverable way to administer it. Co-Authored-By: Claude Sonnet 5 --- ...0005-security-role-model-and-moderation-commands.md | 4 ++++ ...0005-security-role-model-and-moderation-commands.md | 10 ++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/docs/adr/0005-security-role-model-and-moderation-commands.md b/docs/adr/0005-security-role-model-and-moderation-commands.md index f885b57..dfa9e16 100644 --- a/docs/adr/0005-security-role-model-and-moderation-commands.md +++ b/docs/adr/0005-security-role-model-and-moderation-commands.md @@ -325,6 +325,10 @@ chicken-and-egg dead end. `LoginFlow`. - `docs/commands.md` — command pipeline (`ICommand`/`ICommandRegistry`) this ADR extends. +- `docs/deployment.md` — Runtime Configuration table `SHARPMUD_INITIAL_ADMIN` + joins (caught in PR review — it's the only bootstrap path to a + `FullAdmin` in a fresh deployment, and this table is the documented + list of every `HostOptions` env var). - `docs/persistence.md` / `WearableBehaviorConfiguration.cs` — the existing plain-enum-with-default-EF-mapping precedent `SecurityRole` follows for "plain enum, not `OptimizedEnum`" (`EquipSlot` itself is not diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index b2ad514..3d09f98 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -189,6 +189,12 @@ take `IThingRepository` via their own constructor — the same shape - [ ] `docs/accounts-auth.md`: describe ban enforcement in the login flow as current state, update the existing "deferred moderation tooling" forward-reference to point at ADR-0005/this plan. +- [ ] `docs/deployment.md`: add `SHARPMUD_INITIAL_ADMIN` to the Runtime + Configuration table (caught in PR review — this table is the + documented list of every `HostOptions` env var, and + `SHARPMUD_INITIAL_ADMIN` is the *only* bootstrap path to a + `FullAdmin` in a fresh deployment; omitting it here means deploying + a container with no discoverable way to administer it). - [ ] `SPEC.md`: update the "Moderation/admin tooling" Deferred/Open Item to reflect what's actually implemented vs. still deferred (Find/ GoTo/Control/Clone/Spawn/Jail/Buff/Relinquish, audit logging). @@ -222,8 +228,8 @@ Modified: - `src/SharpMud.Engine/Behaviors/PlayerBehavior.cs` - `src/SharpMud.Persistence/Configurations/PlayerBehaviorConfiguration.cs` - `src/SharpMud.Host/LoginFlow.cs`, `HostOptions.cs`, `Program.cs` -- `docs/commands.md`, `docs/accounts-auth.md`, `SPEC.md`, - `docs/adr/README.md`, `docs/plans/README.md`, +- `docs/commands.md`, `docs/accounts-auth.md`, `docs/deployment.md`, + `SPEC.md`, `docs/adr/README.md`, `docs/plans/README.md`, `docs/plans/0001-wheelmud-reconciliation-roadmap.md` ## Test plan From d50aefcd249c383aa176ca334c17ea2586c037aa Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sun, 19 Jul 2026 08:40:49 -0400 Subject: [PATCH 09/15] Self-review: fix 4 gaps found via a full pr-review.md pass - HelpCommand would list every admin command's Verb/Aliases to every player (RoleGuardedCommand passes them through unchanged) - added a task to filter registry.Commands by the actor's roles, and a RequiredRole property on RoleGuardedCommand for it to check against. - "Online" was ambiguous for Boot/Announce, and the cited precedent (WhoCommand's iteration) doesn't actually do what the plan said - WhoCommand has no liveness filter at all, and since ADR-0004 that means Linkdead (disconnected-but-not-swept) players too. Made explicit: both need a ConnectionState == Playing check on top of the AllWithBehavior scan, not a blind copy of WhoCommand's loop. - RevokeRole's plan wording cited a nonexistent "MoveRequest.CancelReason" precedent - corrected to reference the actual type (UseExitEvent) and its real shape (a property set via .Cancel(), not a direct method return), while keeping the simpler string? return design. - No guard against self-lockout: an admin banning themselves has no in-game recovery, and a sole FullAdmin revoking their own tier has no path back without another FullAdmin present. Added explicit self-targeting rejection to BanCommand and RoleRevokeCommand. Added matching test-plan coverage for all four. Found via a full tasks/pr-review.md pass against this PR before further reviewer feedback, not from an external round. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 85 ++++++++++++++++--- 1 file changed, 75 insertions(+), 10 deletions(-) diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index 3d09f98..fcd9885 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -54,6 +54,8 @@ an open item below. - [ ] New `src/SharpMud.Engine/Commands/RoleGuardedCommand.cs` — wraps an inner `ICommand`, checks `(actor.Roles & requiredRole) != SecurityRole.None` before delegating; generic rejection message. + Exposes `public SecurityRole RequiredRole { get; }` — needed by + `HelpCommand`'s filtering below, not just internally. - [ ] New `src/SharpMud.Engine/Commands/MuteGuardedCommand.cs` — wraps an inner `ICommand`, checks the *actor's own* `IsMuted` (not a target's — this gates the muted player's own `say`/`emote`, not something @@ -65,6 +67,15 @@ an open item below. - [ ] Update every existing registration call site (`BuiltinCommands .RegisterAll`, `ClassicCommands.RegisterAll`) from `Register` to `RegisterOpen` — mechanical, no behavior change. +- [ ] `HelpCommand.cs`: filter `registry.Commands` by the actor's roles + before listing them — caught in self-review. `RoleGuardedCommand` + passes `Verb`/`Aliases` straight through from the wrapped command, so + without this, `help` lists every admin command (`ban`, `boot`, + `rolegrant`, ...) to every player. Not an exploit (the gate still + blocks execution) but a real, unpolished info leak — skip a command + in the listing if it's a `RoleGuardedCommand` and `(ctx.Actor`'s + `Roles & command.RequiredRole) == SecurityRole.None`; anything not + role-guarded still lists unconditionally. ### `PlayerBehavior` + persistence @@ -103,8 +114,16 @@ an open item below. `string? RevokeRole(SecurityRole role)` — `null` on success, a message naming the blocking higher tier on failure ("still has FullAdmin, which includes MinorAdmin — revoke FullAdmin - instead"), mirroring the existing `MoveRequest.CancelReason` - nullable-reason idiom rather than inventing a new pattern. + instead"). Corrected during self-review: earlier wording cited + this as mirroring "`MoveRequest.CancelReason`," but no + `MoveRequest` type exists — the actual precedent + (`UseExitEvent.CancelReason`, `src/SharpMud.Engine/Core + /Events.cs`) is a *property* set via `.Cancel(reason)` on a + published cancellable event, a different shape (pub/sub + event-object mutation, not a direct method return). A plain + nullable-string return is simpler and doesn't need that + machinery here — it just needs to be a return value, per + `coding-standards.md`, not a citation to a nonexistent type. `RoleRevokeCommand` relays a non-null return straight to the admin; never wraps this call in a try/catch. - [ ] `PlayerBehaviorConfiguration.cs`: map `Roles` with the plain-enum @@ -124,17 +143,42 @@ take `IThingRepository` via their own constructor — the same shape `ClassicCommands.RegisterAll` already uses for `combatManager`/`random`. `Boot`/`Announce` don't need it (online-only / broadcast-only). +**"Online" means `ConnectionState == Playing`, not just "registered in +`World`"** — caught in self-review. `WhoCommand`'s iteration +(`world.AllWithBehavior()`, no further filter) was +originally cited here as the pattern to copy, but `WhoCommand` itself has +no liveness filter at all, and since ADR-0004 that call also returns +**`Linkdead`** players (disconnected but not yet swept) — `WhoCommand` +mislabeling those as "online" is a separate, pre-existing gap, out of +scope for this slice to fix. `BootCommand`/`AnnounceCommand` must *not* +copy that omission: both need an explicit `ConnectionState == Playing` +check (equivalently, `Session is { IsConnected: true }`) on top of the +`AllWithBehavior()` scan — without it, `Boot` would call +`DisconnectAsync` on an already-dead session for a `Linkdead` target +(harmless but wrong "not online" reporting), and `Announce` risks writing +to a stale/disconnected session outright. + - [ ] `BootCommand` (`MinorAdmin`, no repository dependency) — disconnects - a currently-online target by username; "not online" message if not - found live. + a currently-online (`ConnectionState == Playing`) target by + username; "not online" message if not found live or found only + `Linkdead`. - [ ] `MuteCommand`/`UnmuteCommand` (`MinorAdmin`, `IThingRepository`) — sets/clears `IsMuted` on a target (online-or-not, mirrors `LoginFlow`'s live-then-repository lookup), saves immediately. - [ ] `AnnounceCommand` (`MinorAdmin`, no repository dependency) — - broadcasts to every session in `world.AllWithBehavior()` - with a live session (`WhoCommand`'s iteration pattern). -- [ ] `BanCommand`/`UnbanCommand` (`FullAdmin`, `IThingRepository`) — - sets/clears `IsBanned`, online-or-not lookup, saves immediately. + broadcasts to every `world.AllWithBehavior()` entry + whose `ConnectionState == Playing` — explicitly **not** every entry + `WhoCommand`-style, since that would include stale `Linkdead` + sessions. +- [ ] `BanCommand` (`FullAdmin`, `IThingRepository`) — sets `IsBanned`, + online-or-not lookup, saves immediately. **Rejects self-targeting** + (caught in self-review: `Ban` has no in-game recovery — + `SHARPMUD_INITIAL_ADMIN` only re-grants roles, it doesn't clear + `IsBanned` — so an admin banning themselves is locked out short of a + manual DB edit; `boot`/`mute` self-targeting is left alone, both are + harmless and trivially reversible by the same admin). `UnbanCommand` + needs no such guard (undoing your own ban isn't reachable — you + can't be logged in while banned). - [ ] `RoleGrantCommand`/`RoleRevokeCommand` (`FullAdmin`, `IThingRepository`) — mutates a target's `Roles` via `GrantRole`/`RevokeRole` (accumulation/hierarchy-invariant @@ -147,7 +191,13 @@ take `IThingRepository` via their own constructor — the same shape tier, a severe over-grant) and `None` (a meaningless no-op sentinel), since both are literally named enum members. Reject either with a clear message rather than silently persisting them. - `RoleRevokeCommand` checks `RevokeRole`'s `string?` return (not a + **`RoleRevokeCommand` rejects revoking your own `FullAdmin`** (caught + in self-review — same class of lockout risk as self-`Ban`: a sole + `FullAdmin` revoking their own tier has no in-game path back without + another `FullAdmin` already present to re-grant it). Revoking any + other role from yourself, or revoking `FullAdmin` from someone + *else*, is unaffected. `RoleRevokeCommand` checks `RevokeRole`'s + `string?` return (not a caught exception — see the `PlayerBehavior` task above) and relays a non-null failure straight to the admin as the rejection message. - [ ] Register all 8 via `RegisterWithRole` in a new @@ -222,7 +272,7 @@ New: Modified: - `src/SharpMud.Engine/Commands/ICommandRegistry.cs`, - `CommandRegistry.cs` + `CommandRegistry.cs`, `Builtin/HelpCommand.cs` - `src/SharpMud.Engine/Commands/BuiltinCommands.cs` (or wherever `RegisterAll` lives), `src/SharpMud.Ruleset.Classic/ClassicCommands.cs` - `src/SharpMud.Engine/Behaviors/PlayerBehavior.cs` @@ -252,6 +302,21 @@ Modified: command with the given role. - Unit: each of the 8 admin commands — happy path (role holder, valid target) and the online/offline target-lookup branches. +- Unit: `BootCommand`/`AnnounceCommand` — a `Linkdead` player is treated as + not-online (`Boot` reports "not online," `Announce` doesn't attempt a + write to their stale session), only `ConnectionState == Playing` + targets/recipients count. The regression test for the online/live + ambiguity caught in self-review. +- Unit: `HelpCommand` — a role-gated command is omitted from the listing + for an actor without the required role, and included for one with it; + non-gated commands always list regardless of role. The regression test + for the admin-command-visibility gap caught in self-review. +- Unit: `BanCommand` — self-targeting is rejected with a clear message, + `IsBanned` unchanged; targeting another player still works normally. +- Unit: `RoleRevokeCommand` — revoking your own `FullAdmin` is rejected; + revoking a different role from yourself, or `FullAdmin` from someone + else, still works normally. Both are the regression test for the + self-lockout risk caught in self-review. - Unit: `RoleGrantCommand`/`RoleRevokeCommand` — `all` and `none` (any casing) are rejected with a clear message and never reach `GrantRole`/`RevokeRole`; every other individually-grantable role name From 37aae9dbc4bf514a0e349dad8f6799a748f9c1d1 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sun, 19 Jul 2026 10:31:06 -0400 Subject: [PATCH 10/15] Add SHARPMUD_INITIAL_ADMIN to Program.cs's env dictionary task Program.cs builds a fixed dictionary of only the three existing env vars before calling HostOptions.Parse(args, env) - not a full environment pass-through. Adding the parse logic to HostOptions.cs alone isn't enough; SHARPMUD_INITIAL_ADMIN needs its own dictionary entry or the real host never sees it even with the env var set. Co-Authored-By: Claude Sonnet 5 --- .../0005-security-role-model-and-moderation-commands.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index fcd9885..31f8b85 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -216,6 +216,15 @@ to a stale/disconnected session outright. the `ConnectionState` branch. - [ ] `HostOptions.cs`: add `string? InitialAdminUsername`, parsed from `SHARPMUD_INITIAL_ADMIN`. +- [ ] `Program.cs`: add `["SHARPMUD_INITIAL_ADMIN"] = + Environment.GetEnvironmentVariable("SHARPMUD_INITIAL_ADMIN")` to the + `env` dictionary built before `HostOptions.Parse(args, env)` — caught + in PR review. `Program.cs` builds a **fixed** dictionary of only the + three existing env vars (`SHARPMUD_MODE`/`SHARPMUD_TELNET_PORT`/ + `SHARPMUD_DB_PATH`), not a full environment pass-through, so adding + the parse logic to `HostOptions.cs` alone isn't enough — + `SHARPMUD_INITIAL_ADMIN` needs its own entry here or the real host + never sees it, even with the env var actually set in production. - [ ] Bootstrap the grant in **two** places, not just one — a boot-time-only check is a no-op on a genuinely fresh server, since the target character doesn't exist yet at boot and only gets created later From d4979668adcb06b60531677ebaecb82364f2203b Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sun, 19 Jul 2026 10:47:47 -0400 Subject: [PATCH 11/15] Fix online-check and thread InitialAdminUsername through the real call chain - "Online" was ConnectionState == Playing alone, which is wrong: ConnectionState is Ignore'd (not persisted), so it defaults to Playing on any freshly-constructed PlayerBehavior - including a player reloaded from the repository with no live session. LoginFlow.FindAndAttachExistingAsync already registers such a player into World before password verification even completes, so this isn't hypothetical. Switched to the exact combined check LoginFlow.LoginExistingAsync already established for this exact purpose: ConnectionState == Playing && Session is {IsConnected: true} - not a narrower, newly-invented check. - The plan never specified how InitialAdminUsername actually reaches LoginFlow.MaybeCreateAsync. Verified the real Telnet call chain (HostRunner.RunTelnetAsync -> HandleConnectionAsync -> LoginFlow.RunAsync) and added explicit tasks: TelnetHostContext gains the field, LoginFlow.RunAsync/MaybeCreateAsync gain the parameter, HostRunner and Program.cs thread it through. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 84 +++++++++++++++---- 1 file changed, 66 insertions(+), 18 deletions(-) diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index 31f8b85..0cee6bf 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -143,33 +143,47 @@ take `IThingRepository` via their own constructor — the same shape `ClassicCommands.RegisterAll` already uses for `combatManager`/`random`. `Boot`/`Announce` don't need it (online-only / broadcast-only). -**"Online" means `ConnectionState == Playing`, not just "registered in -`World`"** — caught in self-review. `WhoCommand`'s iteration +**"Online" means `ConnectionState == Playing` *and* `Session is { +IsConnected: true }` — both, not either alone.** `WhoCommand`'s iteration (`world.AllWithBehavior()`, no further filter) was originally cited here as the pattern to copy, but `WhoCommand` itself has no liveness filter at all, and since ADR-0004 that call also returns **`Linkdead`** players (disconnected but not yet swept) — `WhoCommand` mislabeling those as "online" is a separate, pre-existing gap, out of scope for this slice to fix. `BootCommand`/`AnnounceCommand` must *not* -copy that omission: both need an explicit `ConnectionState == Playing` -check (equivalently, `Session is { IsConnected: true }`) on top of the -`AllWithBehavior()` scan — without it, `Boot` would call -`DisconnectAsync` on an already-dead session for a `Linkdead` target -(harmless but wrong "not online" reporting), and `Announce` risks writing -to a stale/disconnected session outright. +copy that omission. + +An earlier version of this note said `ConnectionState == Playing` alone +was sufficient — **wrong, caught in PR review**: `ConnectionState` is +`Ignore`d by `PlayerBehaviorConfiguration` (runtime-only, not persisted), +so it defaults back to `Playing` on any freshly-constructed +`PlayerBehavior` — including a player just reloaded from the repository +with no live session at all. This isn't hypothetical: +`LoginFlow.FindAndAttachExistingAsync` already registers a +repository-loaded player into `World` *before password verification even +runs* — during that window (and after a server restart, before that +player reconnects) the Thing sits in `World` with `ConnectionState +.Playing` and `Session == null`. Checking `ConnectionState` alone would +make `Boot`/`Announce` treat that player as online and attempt a +null-session disconnect/write. The fix: use the exact same combined check +`LoginFlow.LoginExistingAsync` already established for this +(`playerBehavior.ConnectionState == ConnectionState.Playing && +playerBehavior.Session is { IsConnected: true }`) — don't invent a +narrower one. - [ ] `BootCommand` (`MinorAdmin`, no repository dependency) — disconnects - a currently-online (`ConnectionState == Playing`) target by - username; "not online" message if not found live or found only - `Linkdead`. + a currently-online (`ConnectionState == Playing && Session is { + IsConnected: true }`) target by username; "not online" message + otherwise (not found at all, found only `Linkdead`, or found but + with a null/disconnected `Session`). - [ ] `MuteCommand`/`UnmuteCommand` (`MinorAdmin`, `IThingRepository`) — sets/clears `IsMuted` on a target (online-or-not, mirrors `LoginFlow`'s live-then-repository lookup), saves immediately. - [ ] `AnnounceCommand` (`MinorAdmin`, no repository dependency) — broadcasts to every `world.AllWithBehavior()` entry - whose `ConnectionState == Playing` — explicitly **not** every entry - `WhoCommand`-style, since that would include stale `Linkdead` - sessions. + whose `ConnectionState == Playing && Session is { IsConnected: true + }` — explicitly **not** every entry `WhoCommand`-style, and + explicitly **not** `ConnectionState` alone (see above). - [ ] `BanCommand` (`FullAdmin`, `IThingRepository`) — sets `IsBanned`, online-or-not lookup, saves immediately. **Rejects self-targeting** (caught in self-review: `Ban` has no in-game recovery — @@ -237,6 +251,28 @@ to a stale/disconnected session outright. - [ ] `LoginFlow.MaybeCreateAsync`: after a new character is created and saved, if its username matches `InitialAdminUsername`, `GrantRole(FullAdmin)` + save again (the fresh-server case). + **This needs `InitialAdminUsername` threaded all the way down + to `MaybeCreateAsync` — not hidden global state** (caught in + PR review): the actual Telnet call chain is + `HostRunner.RunTelnetAsync` → `HandleConnectionAsync` → + `LoginFlow.RunAsync(session, context.World, context.Repository, + context.StartingRoom, ct)` → `MaybeCreateAsync`, and neither + `TelnetHostContext` nor `LoginFlow.RunAsync`'s signature + carries `HostOptions`/`InitialAdminUsername` today. Concretely: + - [ ] `TelnetHostContext` (`src/SharpMud.Host + /TelnetHostContext.cs`) gains a `string? + InitialAdminUsername` field. + - [ ] `LoginFlow.RunAsync`/`MaybeCreateAsync` gain an + `InitialAdminUsername` parameter (already at/near the + 4-param limit — a small parameter object may be + warranted here too, matching `TelnetHostContext`'s own + precedent per `coding-standards.md`'s 4-param rule, + rather than pushing a 5th positional parameter through). + - [ ] `HostRunner.HandleConnectionAsync` passes + `context.InitialAdminUsername` through to + `LoginFlow.RunAsync`. + - [ ] `Program.cs`'s `TelnetHostContext` construction passes + `hostOptions.InitialAdminUsername`. Both paths call the same `PlayerBehavior.GrantRole(SecurityRole .FullAdmin)`, so both get the accumulation behavior (also granting `MinorAdmin`/`Player`) for free. @@ -286,7 +322,8 @@ Modified: `RegisterAll` lives), `src/SharpMud.Ruleset.Classic/ClassicCommands.cs` - `src/SharpMud.Engine/Behaviors/PlayerBehavior.cs` - `src/SharpMud.Persistence/Configurations/PlayerBehaviorConfiguration.cs` -- `src/SharpMud.Host/LoginFlow.cs`, `HostOptions.cs`, `Program.cs` +- `src/SharpMud.Host/LoginFlow.cs`, `HostOptions.cs`, `Program.cs`, + `HostRunner.cs`, `TelnetHostContext.cs` - `docs/commands.md`, `docs/accounts-auth.md`, `docs/deployment.md`, `SPEC.md`, `docs/adr/README.md`, `docs/plans/README.md`, `docs/plans/0001-wheelmud-reconciliation-roadmap.md` @@ -313,9 +350,13 @@ Modified: target) and the online/offline target-lookup branches. - Unit: `BootCommand`/`AnnounceCommand` — a `Linkdead` player is treated as not-online (`Boot` reports "not online," `Announce` doesn't attempt a - write to their stale session), only `ConnectionState == Playing` - targets/recipients count. The regression test for the online/live - ambiguity caught in self-review. + write to their stale session); only `ConnectionState == Playing && + Session is { IsConnected: true }` targets/recipients count. Specifically + cover a `Playing`-but-`Session == null` player (the repository-reload + case — `ConnectionState` defaults to `Playing` because it isn't + persisted) being correctly treated as *not* online — the regression test + for both the online/live ambiguity caught in self-review and the + reload-defaults-to-Playing gap caught in PR review. - Unit: `HelpCommand` — a role-gated command is omitted from the listing for an actor without the required role, and included for one with it; non-gated commands always list regardless of role. The regression test @@ -357,6 +398,13 @@ Modified: `Program.cs` existing-character path, and `LoginFlow.MaybeCreateAsync`'s newly-created-character path — since a boot-time-only check was the fresh-server gap caught in PR review. +- Unit: `LoginFlow.MaybeCreateAsync` (or its `RunAsync` entry point) + actually receives and uses `InitialAdminUsername` — a character created + with a username matching it gets `FullAdmin`; a character created with a + non-matching (or null/absent) `InitialAdminUsername` does not. The + regression test for the parameter-threading gap caught in PR review + (`TelnetHostContext`/`LoginFlow.RunAsync` never carried this value + before). ## Verification From 767d3c5ccf00a8d10b5c03ad2f42e53002dddfcd Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sun, 19 Jul 2026 15:23:19 -0400 Subject: [PATCH 12/15] Fix boot/Linkdead interaction gap caught in PR review BootCommand's DisconnectAsync ran in the admin's SessionLoop, not the target's, so explicitQuit never got set on the target's own loop and they could reconnect within the grace window as if merely Linkdead. Add PlayerBehavior.WasBooted (transient, MarkBooted()) as the signal that crosses the SessionLoop-instance boundary, plus test-plan and manual-verification coverage for the regression. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 55 +++++++++++++++++-- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index 0cee6bf..0ce180e 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -77,7 +77,7 @@ an open item below. `Roles & command.RequiredRole) == SecurityRole.None`; anything not role-guarded still lists unconditionally. -### `PlayerBehavior` + persistence +### `PlayerBehavior` + persistence + `SessionLoop` - [ ] `PlayerBehavior.cs`: add `SecurityRole Roles { get; private set; } = SecurityRole.Player`, `bool IsMuted { get; private set; }`, `bool @@ -85,6 +85,30 @@ an open item below. (`GrantRole`/`RevokeRole`, `Mute`/`Unmute`, `Ban`/`Unban`) rather than public setters, matching `ConnectionState`'s existing transition-method style. + - [ ] Also add `bool WasBooted { get; private set; }` (transient, + like `Session`/`ConnectionState` — `Ignore`d in + `PlayerBehaviorConfiguration`) and a `MarkBooted()` method. + **Real gap caught in PR review**: `BootCommand` runs inside + the *admin's* `SessionLoop`, calling `DisconnectAsync` on the + *target's* session — but `SessionLoop`'s `explicitQuit` flag + is a local variable scoped to each connection's own + `RunAsync` call. The target's own loop never sees an + admin-triggered disconnect as a "quit," so today it would + take the `Linkdead` branch (per ADR-0004) — the booted player + could just reconnect within the grace window and resume + exactly where they were, making `boot` a no-op as a + moderation tool. `WasBooted` is the signal that crosses that + boundary: `BootCommand` sets it on the target's + `PlayerBehavior` *before* calling `DisconnectAsync`; the + target's own `SessionLoop.RunAsync` (a completely separate + call stack) checks it in its `finally` block. + - [ ] `SessionLoop.cs`: in the `finally` block, treat `WasBooted` + exactly like `explicitQuit` — both mean "this disconnect was + intentional, skip `Linkdead` and remove immediately" (same + save-then-remove ordering `explicitQuit` already uses, not + `EnterLinkdead`'s mutate-before-save ordering). Concretely: + replace the bare `explicitQuit` checks in both branches with + `explicitQuit || (playerBehavior?.WasBooted ?? false)`. - [ ] `GrantRole(SecurityRole role)`: ORs in `role` *and* every tier it implies (`FullAdmin` → also `MinorAdmin` + `Player`; `FullBuilder` → also `MinorBuilder`) per ADR-0005's @@ -130,7 +154,9 @@ an open item below. default EF conversion (matching `WearableBehaviorConfiguration`'s `Slot` precedent — no custom value converter needed); map `IsMuted`/ `IsBanned` as plain persisted columns (NOT `Ignore`d — unlike - `ConnectionState`, these must survive a restart). + `ConnectionState`, these must survive a restart). `Ignore(x => + x.WasBooted)` alongside `Session`/`ConnectionState` — transient, + same category, never meaningful across a restart. ### Moderation commands (`src/SharpMud.Engine/Commands/Builtin/Admin/`) @@ -175,7 +201,14 @@ narrower one. a currently-online (`ConnectionState == Playing && Session is { IsConnected: true }`) target by username; "not online" message otherwise (not found at all, found only `Linkdead`, or found but - with a null/disconnected `Session`). + with a null/disconnected `Session`). **Calls + `target.FindBehavior()!.MarkBooted()` before** + `target's session.DisconnectAsync(...)` — without this the boot is + cosmetic (caught in PR review; see the `PlayerBehavior`/ + `SessionLoop` task above for why). Writes a message to the target's + session first (mirrors `QuitCommand`'s "Goodbye!" before + disconnecting), e.g. "You have been disconnected by an + administrator." - [ ] `MuteCommand`/`UnmuteCommand` (`MinorAdmin`, `IThingRepository`) — sets/clears `IsMuted` on a target (online-or-not, mirrors `LoginFlow`'s live-then-repository lookup), saves immediately. @@ -323,7 +356,7 @@ Modified: - `src/SharpMud.Engine/Behaviors/PlayerBehavior.cs` - `src/SharpMud.Persistence/Configurations/PlayerBehaviorConfiguration.cs` - `src/SharpMud.Host/LoginFlow.cs`, `HostOptions.cs`, `Program.cs`, - `HostRunner.cs`, `TelnetHostContext.cs` + `HostRunner.cs`, `TelnetHostContext.cs`, `SessionLoop.cs` - `docs/commands.md`, `docs/accounts-auth.md`, `docs/deployment.md`, `SPEC.md`, `docs/adr/README.md`, `docs/plans/README.md`, `docs/plans/0001-wheelmud-reconciliation-roadmap.md` @@ -357,6 +390,12 @@ Modified: persisted) being correctly treated as *not* online — the regression test for both the online/live ambiguity caught in self-review and the reload-defaults-to-Playing gap caught in PR review. +- Unit: `SessionLoop` — a session ending with `WasBooted` set (but not + `explicitQuit`) takes the same immediate-removal path `explicitQuit` + takes, not `EnterLinkdead`. The regression test for the + boot-is-cosmetic gap caught in PR review: without this, a booted player + would just resume via the `Linkdead` reconnect path, making `boot` a + no-op as a moderation tool. - Unit: `HelpCommand` — a role-gated command is omitted from the listing for an actor without the required role, and included for one with it; non-gated commands always list regardless of role. The regression test @@ -428,7 +467,13 @@ session/persistence-facing changes): clear message, `unmute` restores them. 5. `ban `; confirm that player can no longer log in (distinct message, not the generic "incorrect" one); `unban` restores login. -6. `boot `; confirm their session is disconnected immediately. +6. `boot `; confirm their session is disconnected immediately, then + **immediately reconnect as that player** (within the `Linkdead` grace + window) and confirm login goes through the normal username/password + prompt from scratch, not a "Welcome back" resume — the regression check + for `WasBooted` actually crossing the `SessionLoop` boundary (PR review + gap: without it, `boot` would be a no-op since the target could just + reconnect and resume where they were). 7. `announce `; confirm every currently-connected session receives it. 8. Confirm a non-admin attempting any of the above gets a clear rejection, From ac529bbd3aacf1bd25cf1d1a71b87ada6c069ebb Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Mon, 20 Jul 2026 07:11:23 -0400 Subject: [PATCH 13/15] Move Slice 3 WheelMUD source dive into research doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-0005's Context inlined the full ActionSecurityAttribute/Actions.Admin source dive instead of recording it under docs/research/ per the documented design process. Added §11 to wheelmud-findings.md with the dive plus adopted/not-adopted notes, and pointed the ADR at it. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 14 +++++----- docs/research/wheelmud-findings.md | 28 +++++++++++++++++++ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/docs/adr/0005-security-role-model-and-moderation-commands.md b/docs/adr/0005-security-role-model-and-moderation-commands.md index dfa9e16..278b4b0 100644 --- a/docs/adr/0005-security-role-model-and-moderation-commands.md +++ b/docs/adr/0005-security-role-model-and-moderation-commands.md @@ -18,13 +18,13 @@ player types is unconditionally resolved and executed — arg-presence check), nothing role-related. `SPEC.md`'s Deferred/Open Items lists "Moderation/admin tooling... Known future need, not designed yet." -WheelMUD's own mechanism (`Core/Attributes/ActionSecurityAttribute.cs` + -`Actions/Admin/`, 16 files): a `[Flags] enum SecurityRole` (`mobile` / -`item` / `room` / `tutorialPlayer` / `player` / `helper` / `married` / -`minorBuilder` / `fullBuilder` / `minorAdmin` / `fullAdmin` / `all`, -12 real values), an `[ActionSecurity(SecurityRole.x)]` class attribute on -each Action, reflected off at registration time into a `Command -.SecurityRole` field, gated at dispatch by a bitwise-AND check +WheelMUD's own mechanism, full source dive recorded in +[wheelmud-findings.md §11](../research/wheelmud-findings.md#11-security-roles--moderation--actionsecurityattribute-actionsadmin): +a `[Flags] enum SecurityRole` (`mobile`/`item`/`room`/`tutorialPlayer`/ +`player`/`helper`/`married`/`minorBuilder`/`fullBuilder`/`minorAdmin`/ +`fullAdmin`/`all`, 12 real values), an `[ActionSecurity(SecurityRole.x)]` +class attribute on each Action, reflected off at registration time into a +`Command.SecurityRole` field, gated at dispatch by a bitwise-AND check (`command.SecurityRole & user.SecurityRoles`) in `CommandGuard.cs`. `Actions/Admin/` (`Announce`, `Ban`, `Boot`, `Buff`, `Clone`, `Control`, `Find`, `GoTo`, `Jail`, `Locate`, `Mute`, `Relinquish`, `RoleGrant`, diff --git a/docs/research/wheelmud-findings.md b/docs/research/wheelmud-findings.md index d979c27..aa3e3a9 100644 --- a/docs/research/wheelmud-findings.md +++ b/docs/research/wheelmud-findings.md @@ -379,6 +379,34 @@ don't require adding new delegate pairs to a growing interface. one sequential `await` chain per connection. MCCP/MXP/TermType remain unreviewed beyond the survey in §Networking above. +## 11. Security roles / moderation — `ActionSecurityAttribute`, `Actions/Admin/` + +Source dive conducted for [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md) +(Slice 3 of the reconciliation roadmap). `Core/Attributes/ActionSecurityAttribute.cs` ++ `Actions/Admin/` (16 files): a `[Flags] enum SecurityRole` (`mobile` / +`item` / `room` / `tutorialPlayer` / `player` / `helper` / `married` / +`minorBuilder` / `fullBuilder` / `minorAdmin` / `fullAdmin` / `all`, 12 real +values), an `[ActionSecurity(SecurityRole.x)]` class attribute on each +Action, reflected off at registration time into a `Command.SecurityRole` +field, gated at dispatch by a bitwise-AND check +(`command.SecurityRole & user.SecurityRoles`) in `CommandGuard.cs`. +`Actions/Admin/` (`Announce`, `Ban`, `Boot`, `Buff`, `Clone`, `Control`, +`Find`, `GoTo`, `Jail`, `Locate`, `Mute`, `Relinquish`, `RoleGrant`, +`RoleRevoke`, `Spawn`, `Unmute`) are ordinary Actions decorated with +high-tier roles — no separate mechanism from any other command. + +**Adopted**: the `[Flags] SecurityRole` bitmask and the bitwise-AND gating +check. **Not adopted**: attribute+reflection dispatch — sharp-mud gates via +a hand-rolled Decorator (`RoleGuardedCommand`/`MuteGuardedCommand` wrapping +an inner `ICommand` at registration time) instead, since sharp-mud's +`ICommandRegistry.Register(ICommand)` model has no DI-container +registration step for an attribute-scanning approach (or +`LayeredCraft.DecoWeaver`, which only intercepts +`IServiceCollection` registrations) to hook into. Full rationale, the +grant/revoke hierarchy-accumulation rule, and the command set actually +built are in [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md)/ +[PLAN-0005](../plans/0005-security-role-model-and-moderation-commands.md). + Going forward, further reconciliation against WheelMUD (moderation/admin tooling, session reconnect, world-building commands, and more) is tracked as a sequenced roadmap in [ADR-0001](../adr/0001-wheelmud-reconciliation-roadmap.md) From ab221bd944fb50441428cec71504f09589a9674f Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Mon, 20 Jul 2026 07:39:41 -0400 Subject: [PATCH 14/15] ci(github): add dependabot, release-drafter, and PR title check Bring in devops-templates workflows for automated dependency updates, grouped major/minor-patch dependabot PRs, and conventional-commit enforcement on PR titles. Co-Authored-By: Claude Sonnet 5 --- .github/dependabot.yml | 25 +++++++ .github/release-drafter.yml | 95 ++++++++++++++++++++++++++ .github/workflows/pr-title-check.yaml | 13 ++++ .github/workflows/release-drafter.yaml | 20 ++++++ 4 files changed, 153 insertions(+) create mode 100644 .github/dependabot.yml create mode 100644 .github/release-drafter.yml create mode 100644 .github/workflows/pr-title-check.yaml create mode 100644 .github/workflows/release-drafter.yaml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..e2379a0 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,25 @@ +version: 2 +updates: + - package-ecosystem: "nuget" + directory: "/" + schedule: + interval: "weekly" + day: "wednesday" + open-pull-requests-limit: 25 + cooldown: + default-days: 7 + commit-message: + prefix: "chore" + include: "scope" + groups: + dotnet-minor-patch: + update-types: + - "minor" + - "patch" + patterns: + - "*" + dotnet-major: + update-types: + - "major" + patterns: + - "*" diff --git a/.github/release-drafter.yml b/.github/release-drafter.yml new file mode 100644 index 0000000..a686049 --- /dev/null +++ b/.github/release-drafter.yml @@ -0,0 +1,95 @@ +name-template: 'v$RESOLVED_VERSION' +tag-template: 'v$RESOLVED_VERSION' +commitish: main + +version-resolver: + major: + labels: + - 'breaking-change' + minor: + labels: + - 'type: feat' + patch: + labels: + - 'type: fix' + - 'type: docs' + - 'type: refactor' + - 'type: test' + - 'type: chore' + - 'type: ci' + - 'type: revert' + default: patch + +autolabeler: + - label: 'type: feat' + title: + - '/^feat(\(.+\))?(!)?:/i' + - label: 'type: fix' + title: + - '/^fix(\(.+\))?(!)?:/i' + - label: 'type: docs' + title: + - '/^docs(\(.+\))?(!)?:/i' + - label: 'type: refactor' + title: + - '/^refactor(\(.+\))?(!)?:/i' + - label: 'type: test' + title: + - '/^test(\(.+\))?(!)?:/i' + - label: 'type: chore' + title: + - '/^chore(\(.+\))?(!)?:/i' + - label: 'type: ci' + title: + - '/^ci(\(.+\))?(!)?:/i' + - label: 'type: revert' + title: + - '/^revert(\(.+\))?(!)?:/i' + - label: 'breaking-change' + title: + - '/^(feat|fix|docs|refactor|test|chore|ci|revert)(\([^)]*\))?!:/i' + +categories: + - title: '⚠️ Breaking Changes' + labels: + - 'breaking-change' + - title: '🚀 Features' + labels: + - 'type: feat' + - title: '🐛 Bug Fixes' + labels: + - 'type: fix' + - title: '📚 Documentation' + labels: + - 'type: docs' + - title: '🔄 Refactoring' + labels: + - 'type: refactor' + - title: '✅ Tests' + labels: + - 'type: test' + - title: '🔧 Maintenance' + labels: + - 'type: chore' + - 'type: ci' + - 'type: revert' + +include-labels: + - 'type: feat' + - 'type: fix' + - 'type: docs' + - 'type: refactor' + - 'type: test' + - 'type: chore' + - 'type: ci' + - 'type: revert' + - 'breaking-change' + +exclude-labels: + - 'skip-changelog' + - 'internal' + +template: | + ## Changes + + $CHANGES \ No newline at end of file diff --git a/.github/workflows/pr-title-check.yaml b/.github/workflows/pr-title-check.yaml new file mode 100644 index 0000000..6180520 --- /dev/null +++ b/.github/workflows/pr-title-check.yaml @@ -0,0 +1,13 @@ +name: PR Title Check + +on: + pull_request: + types: [opened, edited, synchronize, reopened] + +permissions: + pull-requests: read + statuses: write + +jobs: + validate: + uses: LayeredCraft/devops-templates/.github/workflows/pr-title-check.yml@v10.1 \ No newline at end of file diff --git a/.github/workflows/release-drafter.yaml b/.github/workflows/release-drafter.yaml new file mode 100644 index 0000000..7db3a98 --- /dev/null +++ b/.github/workflows/release-drafter.yaml @@ -0,0 +1,20 @@ +name: Release Drafter + +on: + push: + branches: + - main + pull_request: + types: [opened, edited, synchronize, reopened, ready_for_review] + workflow_dispatch: + +permissions: + contents: write + pull-requests: write + +jobs: + draft: + uses: LayeredCraft/devops-templates/.github/workflows/release-drafter.yml@v10.1 + with: + event_name: ${{ github.event_name }} + pr_draft: ${{ github.event.pull_request.draft == true }} \ No newline at end of file From f2d5499d923131fd42c5b70842a98ce6c2c6b412 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Mon, 20 Jul 2026 07:51:48 -0400 Subject: [PATCH 15/15] docs(plan): disconnect live targets on ban, fix research doc section order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ban only set IsBanned/saved; SessionLoop never re-checks the flag mid-session, so an already-connected banned player kept issuing commands until an admin separately ran boot. BanCommand's task now mirrors BootCommand's live-disconnect handling. Also moved wheelmud-findings.md's new §11 above the closing Decisions section per docs/research/'s convention, folding its adopted/not-adopted summary into that section instead of appending after it. Co-Authored-By: Claude Sonnet 5 --- ...rity-role-model-and-moderation-commands.md | 9 ++- docs/research/wheelmud-findings.md | 57 ++++++++++--------- 2 files changed, 38 insertions(+), 28 deletions(-) diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md index 0ce180e..2894efd 100644 --- a/docs/plans/0005-security-role-model-and-moderation-commands.md +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -218,7 +218,14 @@ narrower one. }` — explicitly **not** every entry `WhoCommand`-style, and explicitly **not** `ConnectionState` alone (see above). - [ ] `BanCommand` (`FullAdmin`, `IThingRepository`) — sets `IsBanned`, - online-or-not lookup, saves immediately. **Rejects self-targeting** + online-or-not lookup, saves immediately. **If the target is + currently online (`ConnectionState == Playing && Session is { + IsConnected: true }`), also disconnects them the same way + `BootCommand` does** — `MarkBooted()` then session write + `Disconn + ectAsync(...)` (caught in PR review: `SessionLoop` never re-checks + `IsBanned` mid-session, so without this an already-connected banned + player keeps issuing commands until an admin separately remembers + to `boot` them). **Rejects self-targeting** (caught in self-review: `Ban` has no in-game recovery — `SHARPMUD_INITIAL_ADMIN` only re-grants roles, it doesn't clear `IsBanned` — so an admin banning themselves is locked out short of a diff --git a/docs/research/wheelmud-findings.md b/docs/research/wheelmud-findings.md index aa3e3a9..e86732a 100644 --- a/docs/research/wheelmud-findings.md +++ b/docs/research/wheelmud-findings.md @@ -335,6 +335,24 @@ to regress here. --- +## 11. Security roles / moderation — `ActionSecurityAttribute`, `Actions/Admin/` + +Source dive conducted for [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md) +(Slice 3 of the reconciliation roadmap). `Core/Attributes/ActionSecurityAttribute.cs` ++ `Actions/Admin/` (16 files): a `[Flags] enum SecurityRole` (`mobile` / +`item` / `room` / `tutorialPlayer` / `player` / `helper` / `married` / +`minorBuilder` / `fullBuilder` / `minorAdmin` / `fullAdmin` / `all`, 12 real +values), an `[ActionSecurity(SecurityRole.x)]` class attribute on each +Action, reflected off at registration time into a `Command.SecurityRole` +field, gated at dispatch by a bitwise-AND check +(`command.SecurityRole & user.SecurityRoles`) in `CommandGuard.cs`. +`Actions/Admin/` (`Announce`, `Ban`, `Boot`, `Buff`, `Clone`, `Control`, +`Find`, `GoTo`, `Jail`, `Locate`, `Mute`, `Relinquish`, `RoleGrant`, +`RoleRevoke`, `Spawn`, `Unmute`) are ordinary Actions decorated with +high-tier roles — no separate mechanism from any other command. + +--- + ## Decisions for sharp-mud Two real forks, resolved as follows (see the "Engine vs. Ruleset" architecture @@ -379,33 +397,18 @@ don't require adding new delegate pairs to a growing interface. one sequential `await` chain per connection. MCCP/MXP/TermType remain unreviewed beyond the survey in §Networking above. -## 11. Security roles / moderation — `ActionSecurityAttribute`, `Actions/Admin/` - -Source dive conducted for [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md) -(Slice 3 of the reconciliation roadmap). `Core/Attributes/ActionSecurityAttribute.cs` -+ `Actions/Admin/` (16 files): a `[Flags] enum SecurityRole` (`mobile` / -`item` / `room` / `tutorialPlayer` / `player` / `helper` / `married` / -`minorBuilder` / `fullBuilder` / `minorAdmin` / `fullAdmin` / `all`, 12 real -values), an `[ActionSecurity(SecurityRole.x)]` class attribute on each -Action, reflected off at registration time into a `Command.SecurityRole` -field, gated at dispatch by a bitwise-AND check -(`command.SecurityRole & user.SecurityRoles`) in `CommandGuard.cs`. -`Actions/Admin/` (`Announce`, `Ban`, `Boot`, `Buff`, `Clone`, `Control`, -`Find`, `GoTo`, `Jail`, `Locate`, `Mute`, `Relinquish`, `RoleGrant`, -`RoleRevoke`, `Spawn`, `Unmute`) are ordinary Actions decorated with -high-tier roles — no separate mechanism from any other command. - -**Adopted**: the `[Flags] SecurityRole` bitmask and the bitwise-AND gating -check. **Not adopted**: attribute+reflection dispatch — sharp-mud gates via -a hand-rolled Decorator (`RoleGuardedCommand`/`MuteGuardedCommand` wrapping -an inner `ICommand` at registration time) instead, since sharp-mud's -`ICommandRegistry.Register(ICommand)` model has no DI-container -registration step for an attribute-scanning approach (or -`LayeredCraft.DecoWeaver`, which only intercepts -`IServiceCollection` registrations) to hook into. Full rationale, the -grant/revoke hierarchy-accumulation rule, and the command set actually -built are in [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md)/ -[PLAN-0005](../plans/0005-security-role-model-and-moderation-commands.md). +4. **The `[Flags] SecurityRole` bitmask and its bitwise-AND gating check are + adopted from §11's `ActionSecurityAttribute`/`Actions/Admin/` dive; + attribute+reflection dispatch is not.** sharp-mud gates via a + hand-rolled Decorator (`RoleGuardedCommand`/`MuteGuardedCommand` + wrapping an inner `ICommand` at registration time) instead, since + sharp-mud's `ICommandRegistry.Register(ICommand)` model has no + DI-container registration step for an attribute-scanning approach (or + `LayeredCraft.DecoWeaver`, which only intercepts `IServiceCollection` + registrations) to hook into. Full rationale, the grant/revoke + hierarchy-accumulation rule, and the command set actually built are in + [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md)/ + [PLAN-0005](../plans/0005-security-role-model-and-moderation-commands.md). Going forward, further reconciliation against WheelMUD (moderation/admin tooling, session reconnect, world-building commands, and more) is tracked as