Fix code review issues for phase 14

src/mudlib/commands/__init__.py

@@ -23,6 +23,7 @@ class CommandDefinition:
     mode: str = "normal"
     help: str = ""
     hidden: bool = False
+    admin: bool = False
 
 
 # Registry maps command names to definitions
@@ -204,5 +205,11 @@ async def dispatch(player: Player, raw_input: str) -> None:
         await player.writer.drain()
         return
 
+    # Check admin permission
+    if defn.admin and not getattr(player, "is_admin", False):
+        player.writer.write("You don't have permission to do that.\r\n")
+        await player.writer.drain()
+        return
+
     # Execute the handler
     await defn.handler(player, args)

src/mudlib/commands/build.py

@@ -54,6 +54,10 @@ async def cmd_dig(player: Player, args: str) -> None:
         await player.send("Width and height must be numbers.\r\n")
         return
 
+    if width < 1 or height < 1:
+        await player.send("Zone dimensions must be at least 1x1.\r\n")
+        return
+
     if get_zone(name) is not None:
         await player.send(f"Zone '{name}' already exists.\r\n")
         return
@@ -110,7 +114,7 @@ async def cmd_place(player: Player, args: str) -> None:
     await player.send(f"Placed {thing_name} at ({player.x}, {player.y}).\r\n")
 
 
-register(CommandDefinition("@goto", cmd_goto, help="Teleport to a zone"))
-register(CommandDefinition("@dig", cmd_dig, help="Create a new zone"))
-register(CommandDefinition("@save", cmd_save, help="Save current zone"))
-register(CommandDefinition("@place", cmd_place, help="Place a thing"))
+register(CommandDefinition("@goto", cmd_goto, admin=True, help="Teleport to a zone"))
+register(CommandDefinition("@dig", cmd_dig, admin=True, help="Create a new zone"))
+register(CommandDefinition("@save", cmd_save, admin=True, help="Save current zone"))
+register(CommandDefinition("@place", cmd_place, admin=True, help="Place a thing"))

src/mudlib/commands/paint.py

@@ -53,6 +53,6 @@ async def cmd_set_brush(player: Player, args: str) -> None:
 
 
 # Register paint mode commands
-register(CommandDefinition("@paint", cmd_paint))
-register(CommandDefinition("p", cmd_toggle_painting))
-register(CommandDefinition("brush", cmd_set_brush))
+register(CommandDefinition("@paint", cmd_paint, admin=True))
+register(CommandDefinition("p", cmd_toggle_painting, admin=True))
+register(CommandDefinition("brush", cmd_set_brush, admin=True))

src/mudlib/commands/read.py

@@ -10,7 +10,7 @@ def _find_readable(player: Player, name: str) -> Thing | None:
     name_lower = name.lower()
 
     # Check inventory first
-    for obj in player._contents:
+    for obj in player.contents:
         if not isinstance(obj, Thing):
             continue
         if obj.name.lower() == name_lower:

src/mudlib/export.py

@@ -84,7 +84,10 @@ def export_zone(zone: Zone) -> str:
             lines.append(f"respawn_seconds = {spawn_rule.respawn_seconds}")
             if spawn_rule.home_region is not None:
                 hr = spawn_rule.home_region
-                lines.append(f"home_region = {{ x = {hr['x']}, y = {hr['y']} }}")
+                lines.append(
+                    f"home_region = {{ x = [{hr['x'][0]}, {hr['x'][1]}], "
+                    f"y = [{hr['y'][0]}, {hr['y'][1]}] }}"
+                )
             lines.append("")
 
     return "\n".join(lines)

src/mudlib/mobs.py

@@ -1,5 +1,6 @@
 """Mob template loading, global registry, and spawn/despawn/query."""
 
+import logging
 import tomllib
 from dataclasses import dataclass, field
 from pathlib import Path
@@ -8,6 +9,8 @@ from mudlib.entity import Mob
 from mudlib.loot import LootEntry
 from mudlib.zone import Zone
 
+logger = logging.getLogger(__name__)
+
 
 @dataclass
 class MobTemplate:
@@ -91,10 +94,26 @@ def spawn_mob(
         moves=list(template.moves),
     )
     if home_region is not None:
-        mob.home_x_min = home_region["x"][0]
-        mob.home_x_max = home_region["x"][1]
-        mob.home_y_min = home_region["y"][0]
-        mob.home_y_max = home_region["y"][1]
+        # Validate home_region structure
+        if (
+            isinstance(home_region.get("x"), list)
+            and isinstance(home_region.get("y"), list)
+            and len(home_region["x"]) == 2
+            and len(home_region["y"]) == 2
+            and all(isinstance(v, int) for v in home_region["x"])
+            and all(isinstance(v, int) for v in home_region["y"])
+        ):
+            mob.home_x_min = home_region["x"][0]
+            mob.home_x_max = home_region["x"][1]
+            mob.home_y_min = home_region["y"][0]
+            mob.home_y_max = home_region["y"][1]
+        else:
+            logger.warning(
+                "Malformed home_region for mob %s: %s. Expected "
+                "{x: [int, int], y: [int, int]}. Skipping home region.",
+                template.name,
+                home_region,
+            )
     mobs.append(mob)
     return mob
 

src/mudlib/player.py

@@ -42,6 +42,7 @@ class Player(Entity):
     play_time_seconds: float = 0.0
     unlocked_moves: set[str] = field(default_factory=set)
     session_start: float = 0.0
+    is_admin: bool = False
 
     @property
     def mode(self) -> str:

tests/test_build_commands.py

@@ -48,6 +48,7 @@ def player(zone, mock_writer, mock_reader):
         writer=mock_writer,
         reader=mock_reader,
         location=zone,
+        is_admin=True,
     )
     zone._contents.append(p)
     players["builder"] = p
@@ -172,6 +173,37 @@ async def test_dig_bad_args(player, mock_writer):
     assert "usage" in written.lower() or "dig" in written.lower()
 
 
+@pytest.mark.asyncio
+async def test_dig_non_numeric_dimensions(player, mock_writer):
+    """@dig with non-numeric dimensions shows error."""
+    from mudlib.commands.build import cmd_dig
+
+    await cmd_dig(player, "cave abc def")
+
+    mock_writer.write.assert_called()
+    written = mock_writer.write.call_args_list[0][0][0]
+    assert "numbers" in written.lower()
+
+
+@pytest.mark.asyncio
+async def test_dig_zero_or_negative_dimensions(player, mock_writer):
+    """@dig with zero or negative dimensions shows error."""
+    from mudlib.commands.build import cmd_dig
+
+    await cmd_dig(player, "cave 0 5")
+
+    mock_writer.write.assert_called()
+    written = mock_writer.write.call_args_list[0][0][0]
+    assert "at least 1x1" in written.lower()
+
+    mock_writer.write.reset_mock()
+    await cmd_dig(player, "cave 5 -2")
+
+    mock_writer.write.assert_called()
+    written = mock_writer.write.call_args_list[0][0][0]
+    assert "at least 1x1" in written.lower()
+
+
 # --- @save ---
 
 
@@ -251,3 +283,45 @@ async def test_place_no_args(player, mock_writer):
     mock_writer.write.assert_called()
     written = mock_writer.write.call_args_list[0][0][0]
     assert "usage" in written.lower() or "place" in written.lower()
+
+
+# --- Permission checks ---
+
+
+@pytest.mark.asyncio
+async def test_builder_commands_require_admin(zone, mock_writer, mock_reader):
+    """Non-admin players cannot use builder commands."""
+    from mudlib.commands import dispatch
+
+    non_admin = Player(
+        name="player",
+        x=5,
+        y=5,
+        writer=mock_writer,
+        reader=mock_reader,
+        location=zone,
+        is_admin=False,
+    )
+
+    await dispatch(non_admin, "@goto hub")
+    mock_writer.write.assert_called()
+    written = mock_writer.write.call_args_list[-1][0][0]
+    assert "permission" in written.lower()
+
+    mock_writer.write.reset_mock()
+    await dispatch(non_admin, "@dig test 5 5")
+    mock_writer.write.assert_called()
+    written = mock_writer.write.call_args_list[-1][0][0]
+    assert "permission" in written.lower()
+
+    mock_writer.write.reset_mock()
+    await dispatch(non_admin, "@save")
+    mock_writer.write.assert_called()
+    written = mock_writer.write.call_args_list[-1][0][0]
+    assert "permission" in written.lower()
+
+    mock_writer.write.reset_mock()
+    await dispatch(non_admin, "@place thing")
+    mock_writer.write.assert_called()
+    written = mock_writer.write.call_args_list[-1][0][0]
+    assert "permission" in written.lower()