From 05bd07eb7b97b082af1d630f715b16d092c85cd1 Mon Sep 17 00:00:00 2001 From: Chris Guidry Date: Tue, 21 Apr 2026 20:59:40 -0400 Subject: [PATCH] Heal list-shaped skills/tools and keep wikilinks out of narration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things I ran into in one onboarding session, bundled together since neither's big enough to stand alone: - The DM's opening salvo crashed `update_character` with "list indices must be integers or slices, not str" — the LLM had written `proficiencies.skills` as a flat list of names instead of a name→"proficient"/"expertise" dict, then came back with `proficiencies.skills.survival = "proficient"`-style dot-path updates that can't walk into a list. Tightened the schema so the wrong shape gets rejected with a concrete dict example, added coercion in `coerce_character` so existing bad yamls heal on load (mirrors the `resources` / `equipment` pattern already there), and swapped `_set_nested`'s bare `TypeError` for a returned DM-readable message so any future "can't dot-path into a list" scenario surfaces as a rejection instead of a stack trace. - DM was narrating `[[Name]]` wikilinks in the prose it sends the player. Added a line to `prompts/dm-system.md` spelling out that wikilinks belong in tool calls only, never in narration. New tests live in their own `tests/test_character_proficiencies.py` rather than growing `tests/test_character.py` past its loq baseline. Co-Authored-By: Claude Opus 4.7 (1M context) --- prompts/dm-system.md | 7 ++ src/storied/character/data.py | 36 +++++-- src/storied/character/schema.py | 49 ++++++++- tests/test_character_proficiencies.py | 145 ++++++++++++++++++++++++++ 4 files changed, 226 insertions(+), 11 deletions(-) create mode 100644 tests/test_character_proficiencies.py diff --git a/prompts/dm-system.md b/prompts/dm-system.md index 0c29e31..67331fa 100644 --- a/prompts/dm-system.md +++ b/prompts/dm-system.md @@ -590,6 +590,13 @@ Use `[[Name]]` syntax when referencing saved entities in session state: This helps load relevant context automatically when resuming sessions. +**Wikilinks are bookkeeping, not narration.** They belong in tool calls +— `set_scene` fields (`situation`, `present`, `threads`), `establish` +descriptions, `mark` events, `add_note` entries. They must **never** +appear in the prose you send back to the player. The player sees a +story, not a wiki. Write "Vera Blackwater" or "the Rusty Anchor" in +narration; the double brackets are for your own records. + ## Ending Sessions When the player indicates they want to stop playing (saying "quit", "exit", "I need to go", "let's stop here", etc.), use the `end_session` tool: diff --git a/src/storied/character/data.py b/src/storied/character/data.py index 3aaa5de..7f32c0b 100644 --- a/src/storied/character/data.py +++ b/src/storied/character/data.py @@ -144,7 +144,9 @@ def update_character(player_id: str, updates: dict) -> str: changes = [] for key, value in updates.items(): - _set_nested(data, key, value) + error = _set_nested(data, key, value) + if error: + return f"Update rejected — {error}. Character on disk is unchanged." changes.append(f"{key} = {value}") # Clamp HP to 0..max @@ -172,28 +174,42 @@ def update_character(player_id: str, updates: dict) -> str: return "Character updated: " + ", ".join(changes) -def _set_nested(data: dict, key: str, value) -> None: +def _set_nested(data: dict, key: str, value) -> str | None: """Set a nested value using dot notation. - Supports list indices like 'classes.0.level'. + Supports list indices like 'classes.0.level'. Returns None on success, + or a DM-readable error string if the path can't be walked (e.g. trying + to dot-path into a field that currently holds a list — typically + because a previous write stored the wrong shape there). """ parts = key.split(".") - current = data + current: object = data - for part in parts[:-1]: - # Try as integer for list index + for i, part in enumerate(parts[:-1]): if part.isdigit() and isinstance(current, list): current = current[int(part)] continue - if part not in current: - current[part] = {} - current = current[part] + if isinstance(current, dict): + if part not in current: + current[part] = {} + current = current[part] + continue + walked = ".".join(parts[: i + 1]) + return ( + f"can't descend into '{walked}' — it's a " + f"{type(current).__name__}, not a dict. Set the whole field " + f"at '{walked}' to the right shape instead" + ) last = parts[-1] if last.isdigit() and isinstance(current, list): current[int(last)] = value - else: + return None + if isinstance(current, dict): current[last] = value + return None + walked = ".".join(parts[:-1]) or "(root)" + return f"can't set '{key}' — '{walked}' is a {type(current).__name__}, not a dict" def create_character( diff --git a/src/storied/character/schema.py b/src/storied/character/schema.py index 2755614..172c8ef 100644 --- a/src/storied/character/schema.py +++ b/src/storied/character/schema.py @@ -96,6 +96,45 @@ class MagicItems(BaseModel): carried: list[str] = Field(default_factory=list) +class Proficiencies(BaseModel): + """Proficiency buckets on the character sheet. + + `skills` and `tools` are dicts mapping name → "proficient" | "expertise" + so the DM can dot-path into them (`proficiencies.skills.stealth`) and + `is_proficient_in` / `has_expertise_in` can answer at roll time. + """ + + model_config = ConfigDict(extra="allow") + saves: list[str] = Field(default_factory=list) + skills: dict[str, str] = Field(default_factory=dict) + tools: dict[str, str] = Field(default_factory=dict) + weapons: list[str] = Field(default_factory=list) + armor: list[str] = Field(default_factory=list) + languages: list[str] = Field(default_factory=list) + + @field_validator("skills", mode="before") + @classmethod + def _skills_must_be_dict(cls, v: Any) -> Any: + if isinstance(v, list): + raise ValueError( + "must be a dict of name → 'proficient' | 'expertise', " + "not a list. Example: " + "{'stealth': 'expertise', 'perception': 'proficient'}" + ) + return v + + @field_validator("tools", mode="before") + @classmethod + def _tools_must_be_dict(cls, v: Any) -> Any: + if isinstance(v, list): + raise ValueError( + "must be a dict of name → 'proficient' | 'expertise', " + "not a list. Example: " + "{'thieves_tools': 'proficient'}" + ) + return v + + # --- Top-level character model ---------------------------------------------- @@ -112,7 +151,7 @@ class Character(BaseModel): identity: dict[str, Any] = Field(default_factory=dict) abilities: dict[str, int] = Field(default_factory=dict) - proficiencies: dict[str, Any] = Field(default_factory=dict) + proficiencies: Proficiencies = Field(default_factory=Proficiencies) state: State defenses: dict[str, Any] = Field(default_factory=dict) conditions: list[str] = Field(default_factory=list) @@ -211,4 +250,12 @@ def coerce_character(data: dict[str, Any]) -> dict[str, Any]: "carried": [str(x) for x in magic_items], } + # proficiencies.skills / .tools: list-of-names → dict of name → "proficient" + proficiencies = data.get("proficiencies") + if isinstance(proficiencies, dict): + for bucket in ("skills", "tools"): + current = proficiencies.get(bucket) + if isinstance(current, list): + proficiencies[bucket] = {str(name): "proficient" for name in current} + return data diff --git a/tests/test_character_proficiencies.py b/tests/test_character_proficiencies.py new file mode 100644 index 0000000..263df3a --- /dev/null +++ b/tests/test_character_proficiencies.py @@ -0,0 +1,145 @@ +# pyright: reportOptionalSubscript=false, reportOptionalMemberAccess=false +# pyright: reportReturnType=false +"""Schema validation and coercion for `proficiencies.skills` / `.tools`. + +Both buckets are dicts mapping name → "proficient" | "expertise" so the DM +can dot-path into them (`proficiencies.skills.stealth = "proficient"`). +Older sessions stored them as flat lists of names; we reject that shape at +write time and heal it at read time. +""" + +from pathlib import Path + +import pytest +import yaml + +from storied.character import ( + create_character, + load_character, + update_character, +) + + +@pytest.fixture +def player_dir(tmp_path: Path) -> Path: + (tmp_path / "players" / "test-player").mkdir(parents=True) + return tmp_path + + +@pytest.fixture +def mira(player_dir: Path) -> dict: + """Minimal level-3 Rogue for write-time validation tests.""" + create_character( + player_id="test-player", + name="Mira", + race="Human", + char_class="Rogue", + level=3, + abilities={ + "strength": 11, + "dexterity": 18, + "constitution": 14, + "intelligence": 15, + "wisdom": 14, + "charisma": 18, + }, + hp_max=24, + ac=16, + ) + return load_character("test-player") + + +class TestProficienciesValidation: + def test_skills_as_list_is_rejected(self, mira: dict, player_dir: Path): + before = load_character("test-player") + result = update_character( + "test-player", + {"proficiencies.skills": ["stealth", "perception"]}, + ) + assert "rejected" in result.lower() + assert "skills" in result + assert "dict" in result.lower() + after = load_character("test-player") + assert after["proficiencies"]["skills"] == before["proficiencies"]["skills"] + + def test_tools_as_list_is_rejected(self, mira: dict, player_dir: Path): + result = update_character( + "test-player", + {"proficiencies.tools": ["thieves_tools"]}, + ) + assert "rejected" in result.lower() + assert "tools" in result + + def test_dot_path_into_list_returns_readable_error( + self, mira: dict, player_dir: Path + ): + """Dot-pathing into a field that's legitimately a list (like `saves`) + returns a readable error, not a bare TypeError.""" + result = update_character( + "test-player", + {"proficiencies.saves.dexterity": "proficient"}, + ) + assert "rejected" in result.lower() + assert "proficiencies.saves" in result + assert "list" in result.lower() + + +class TestProficienciesCoercion: + def test_load_coerces_skills_list_to_dict(self, player_dir: Path): + path = player_dir / "players" / "test-player" / "character.yaml" + path.write_text( + yaml.dump( + { + "identity": {"name": "Damaged"}, + "abilities": { + "strength": 10, + "dexterity": 10, + "constitution": 10, + "intelligence": 10, + "wisdom": 10, + "charisma": 10, + }, + "state": {"hp": {"max": 20, "current": 20, "temp": 0}}, + "proficiencies": { + "skills": ["stealth", "perception"], + "tools": ["thieves_tools"], + }, + } + ) + ) + data = load_character("test-player") + assert data["proficiencies"]["skills"] == { + "stealth": "proficient", + "perception": "proficient", + } + assert data["proficiencies"]["tools"] == {"thieves_tools": "proficient"} + + def test_coerced_skills_support_dot_path_updates(self, player_dir: Path): + """End-to-end: a character with list-shaped skills on disk can still + accept `proficiencies.skills.X` updates after coercion.""" + path = player_dir / "players" / "test-player" / "character.yaml" + path.write_text( + yaml.dump( + { + "identity": {"name": "Damaged"}, + "abilities": { + "strength": 10, + "dexterity": 10, + "constitution": 10, + "intelligence": 10, + "wisdom": 10, + "charisma": 10, + }, + "state": {"hp": {"max": 20, "current": 20, "temp": 0}}, + "proficiencies": {"skills": ["stealth"]}, + } + ) + ) + result = update_character( + "test-player", + {"proficiencies.skills.survival": "proficient"}, + ) + assert "rejected" not in result.lower() + data = load_character("test-player") + assert data["proficiencies"]["skills"]["stealth"] == "proficient" + assert data["proficiencies"]["skills"]["survival"] == "proficient" -- 2.51.2