From e6ca4dc6b19bfcc54cdb800934855418a9d3282a Mon Sep 17 00:00:00 2001 From: Jared Miller Date: Sat, 14 Feb 2026 12:12:23 -0500 Subject: [PATCH] Fix code review issues for phase 14 --- src/mudlib/commands/__init__.py | 7 ++++ src/mudlib/commands/build.py | 12 ++++-- src/mudlib/commands/paint.py | 6 +-- src/mudlib/commands/read.py | 2 +- src/mudlib/export.py | 5 ++- src/mudlib/mobs.py | 27 ++++++++++-- src/mudlib/player.py | 1 + tests/test_build_commands.py | 74 +++++++++++++++++++++++++++++++++ 8 files changed, 121 insertions(+), 13 deletions(-) diff --git a/src/mudlib/commands/__init__.py b/src/mudlib/commands/__init__.py index 0736323..0efd752 100644 --- a/src/mudlib/commands/__init__.py +++ b/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) diff --git a/src/mudlib/commands/build.py b/src/mudlib/commands/build.py index 7687020..8a332b8 100644 --- a/src/mudlib/commands/build.py +++ b/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")) diff --git a/src/mudlib/commands/paint.py b/src/mudlib/commands/paint.py index bf0230e..0814005 100644 --- a/src/mudlib/commands/paint.py +++ b/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)) diff --git a/src/mudlib/commands/read.py b/src/mudlib/commands/read.py index a345162..0291964 100644 --- a/src/mudlib/commands/read.py +++ b/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: diff --git a/src/mudlib/export.py b/src/mudlib/export.py index 13c3aaa..5a3348a 100644 --- a/src/mudlib/export.py +++ b/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) diff --git a/src/mudlib/mobs.py b/src/mudlib/mobs.py index b1b6ad3..7f91ce4 100644 --- a/src/mudlib/mobs.py +++ b/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 diff --git a/src/mudlib/player.py b/src/mudlib/player.py index 7eb219f..dcb3699 100644 --- a/src/mudlib/player.py +++ b/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: diff --git a/tests/test_build_commands.py b/tests/test_build_commands.py index 7e2151f..91b751c 100644 --- a/tests/test_build_commands.py +++ b/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()