feat(engine): world-building/OLC command surface (dig/tunnel/describe)#20
Conversation
Closes Slice 4 of the WheelMUD reconciliation roadmap (ADR-0009): adds dig/tunnel/describe, gated at SecurityRole.MinorBuilder via the same RegisterWithRole mechanism ADR-0005 introduced for moderation commands. Extracts a shared RoomConnector.Connect helper (Engine) from the two world-boot builders (BasicWorldBuilder, HubWorldBuilder), which each had their own identical copy of the two-way exit-wiring logic - one shared implementation instead of three. Fixes a real bug found during manual Telnet verification: dig/tunnel didn't check whether the current room already had an exit in the requested direction, so digging into an already-occupied direction silently created an unreachable, shadowed duplicate exit rather than rejecting. Added BuilderCommandHelpers.HasExit plus regression tests. Documents the new commands - and backfills the previously-undocumented Slice 3 moderation commands - in the public docsite, and records a world-loading scale concern surfaced during design discussion as tracked future work (PLAN-0001 Slice 11) rather than solving it speculatively. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ncipollina
left a comment
There was a problem hiding this comment.
💬 Deep-dive review complete — build clean, 235/235 tests pass locally (matches PR's own claim), and ADR-0009/PLAN-0009/docs/persistence.md's factual claims all verified against the actual code. 4 findings posted inline, none blocking (🛑 not warranted): two Thing.Add return + non-atomic dual-tree save in the multi-area tunnel case, one
Checks area.Add(room)'s bool return in DigCommand instead of assuming success - Thing.Add publishes a cancelable AddChildEvent, and proceeding regardless would register/connect/save a room that was never actually attached to the tree if anything ever vetoed it. Matches MoveCommand's existing precedent of checking Remove's return the same way. Dedups the "already an exit ... from here" rejection message (was identical, hand-copied in both DigCommand and TunnelCommand) into a shared BuilderCommandHelpers.OccupiedDirectionMessage. Fixes a factual error in the new docsite page's SecurityRole "Implies" table: MinorBuilder/FullBuilder's accumulation was documented as "-"/ "MinorBuilder" when SecurityRoleExtensions.ImpliedRoles actually gives "Player"/"MinorBuilder, Player" - would have told a reader granting MinorBuilder doesn't also grant Player, which is false. Defers (tracked in persistence.md Open Items + PLAN-0009, not fixed here) TunnelCommand's two SaveTreeAsync calls not being atomic together across a multi-area tunnel - currently unreachable since every existing world has exactly one area, and fixing it properly needs a repository- level multi-root atomic save, a bigger change than this PR's scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Re: #20 (review) — all 4 inline findings triaged and replied to individually. 3 fixed in a449627 (DigCommand's unchecked Add return, the duplicated occupied-direction message, the docsite Implies table error); the TunnelCommand dual-save atomicity gap is deferred with a tracked note in docs/persistence.md and PLAN-0009 rather than fixed here (repository-API change, currently unreachable). All 4 review threads resolved. |
📋 Summary
Closes Slice 4 of the WheelMUD reconciliation roadmap (ADR-0009): adds
dig/tunnel/describeworld-building commands, gated atSecurityRole.MinorBuildervia the sameRegisterWithRolemechanism ADR-0005 introduced for moderation commands. Also documents the new commands (and backfills the previously-undocumented Slice 3 moderation commands) in the public docsite, and records a world-loading scale concern surfaced during design discussion as tracked future work rather than fixing it speculatively.📝 Changes
New commands (
src/SharpMud.Engine/Commands/Builtin/Builder/)DigCommand— creates a new room and wires a two-way exit from the builder's current room to it.TunnelCommand— wires a two-way exit between the current room and an already-existing room, found by exact name.DescribeCommand— sets the current room's description.BuilderCommandHelpers— direction parsing, room-by-name lookup, exit-collision check, tree-root lookup for saving.BuilderCommands.RegisterAll— registers all three viaRegisterWithRole(_, SecurityRole.MinorBuilder). Wired intosamples/SharpMud.Samples.Classic/Program.csalongside the existingAdminCommands.RegisterAll.Shared exit-wiring dedup
SharpMud.Engine.Behaviors.RoomConnector.Connect— the two-way exit-wiring logic, extracted fromBasicWorldBuilderandHubWorldBuilder, which each had their own identical private copy. Both now call the shared helper; no behavior change (confirmed by the pre-existingBasicWorldBuilderTestsstill passing unchanged).Bug fix found during manual verification
dig/tunneldidn't check whether the current room already had an exit in the requested direction. SinceMoveCommandresolves exits viaFirstOrDefault, digging into an already-occupied direction silently created an unreachable, shadowed duplicate exit instead of rejecting. Fixed viaBuilderCommandHelpers.HasExit, checked byDigCommand(origin) andTunnelCommand(origin and destination's opposite direction). Regression tests added for all three collision cases.Docs
docs/adr/0009-...md/docs/plans/0009-...md(new), indexed in both READMEs andPLAN-0001.docs/commands.md,docs/world-model.md,SPEC.mdupdated to describe the new commands as current state.SPEC.md/docs/persistence.md/PLAN-0001— added an unnumbered "Slice 11" tracking a world-loading scale concern (ThingRepositoryloads the entire world into memory on every load) surfaced while discussing this PR's room-lookup approach — deliberately left undesigned, not solved here.docsite/docs/moderation-and-world-building.md(public, consumer-facing) coveringSecurityRole/RegisterWithRoleand both command sets that use it; cross-linked fromcustomizing.md.🧪 Validation
dotnet buildclean,dotnet test— 235/235 passing (includes new tests forRoomConnector,DigCommand,TunnelCommand,DescribeCommand,BuilderCommandsregistration).SharpMud.Samples.Classicwith a scratch SQLite DB andSHARPMUD_INITIAL_ADMINbootstrap — created a character, grantedMinorBuilder, exerciseddig/tunnel/describehappy paths and all rejection paths (invalid direction, missing args, no/ambiguous room match, self-tunnel, occupied-direction on both origin and destination sides). Restarted the server and confirmed persistence via direct SQLite inspection (Things/Behaviorstables) — every new room, description, and exit (correct bidirectionalExitDestinationId, no duplicates) survived alongside the pre-existing hub content.dig's origin andtunnel's origin/destination), ambiguous/missing room names, self-tunnel, multi-area save correctness (TunnelCommandsaves both the origin's and destination's tree roots when they differ).docsitebuilds clean (zensical build→ "No issues found") with the new page in nav.💬 Notes for Reviewers
BasicWorldBuilder's starter world.