diff --git a/backend/app/routers/adventures/actions.py b/backend/app/routers/adventures/actions.py index d14f3f2..7c46ce1 100644 --- a/backend/app/routers/adventures/actions.py +++ b/backend/app/routers/adventures/actions.py @@ -8,11 +8,12 @@ coordinate through `nodes.delete_turn`. from fastapi import Depends, HTTPException from sqlalchemy.orm import Session -from ... import models, schemas, tree +from ... import attempts, models, schemas, tree from ...database import get_db +from . import turns from .deps import CurrentUser, current_adventure, router -from .nodes import delete_turn +from .nodes import db_tip, delete_turn from .paging import ACTION_PAGE, action_window, annotate_takes @@ -72,15 +73,33 @@ def delete_action( action = db.get(models.Action, action_id) if action is None or action.adventure_id != adventure_id: raise HTTPException(404, "Action not found") - # This works like undo. The turn is deleted with all of its attempts, and - # whatever it produced is withdrawn. Nothing else is needed, because the - # marks are depths, and a depth does not move when an action before it is - # deleted. - delete_turn(db, adventure, action) - db.flush() - db.expire(adventure, ["actions"]) - # Deleting the newest action moves the tip. Deleting an action in the - # middle leaves a gap in the depths, which is intended. See - # `_backfill_tree`. - tree.refresh_head(db, adventure) - db.commit() + # The lock is held for the same reason undo holds it: this endpoint puts + # the shared state back, and a turn that is still generating is about to + # write it. + turns.acquire_turn_lock(adventure_id) + try: + # This works like undo. The turn is deleted with all of its attempts, + # and whatever it produced is withdrawn. The marks are depths, and a + # depth does not move when an action before it is deleted. + delete_turn(db, adventure, action) + db.flush() + db.expire(adventure, ["actions"]) + # Deleting the newest action moves the tip. Deleting an action in the + # middle leaves a gap in the depths, which is intended. See + # `_backfill_tree`. + tree.refresh_head(db, adventure) + # The script state and the world state belong to the adventure, not to + # the node, so deleting the node does not take back what it did to + # them. Put them back to what the story now ends with, which is the + # same restore a branch switch does. + # + # The world state carries the cooldown clock in `_meta.last_changed`, + # and that clock is a depth. Leaving it set marked the deleted turn's + # changes as having happened at the depth the next turn is played at, + # so the referee refused them as changed too recently — on a turn the + # story no longer contains. Deleting a middle action restores the tip's + # own outcome, which is the state the adventure is already in. + attempts.restore_state(adventure, db_tip(db, adventure)) + db.commit() + finally: + turns._active_turns.discard(adventure_id) diff --git a/backend/app/worldstate/parse.py b/backend/app/worldstate/parse.py index 775427e..6c91e75 100644 --- a/backend/app/worldstate/parse.py +++ b/backend/app/worldstate/parse.py @@ -23,8 +23,9 @@ EMIT_RULE = ( "while a large change — or reaching a stat's minimum or maximum — is reserved for a " "genuinely pivotal, defining moment (a passing remark shifts a relationship a little; a " "lasting act of loyalty or betrayal shifts it a lot). Do not move a value across most of " - 'its range in a single ordinary turn. Use the paths exactly as shown in the world state: ' - '"player.", ' + 'its range in a single ordinary turn. Every stat is listed by its exact path in the ' + 'stat guide and again beside its live value — copy a path from there rather than ' + 'building one out of a name. The shapes are "player.", ' '"world.", "npc.." (use the id in parentheses, e.g. npc.gwen.trust, ' 'not the display name); "flags.": true or false to toggle an on/off state; and ' '"milestones.": true when an objective is completed. Some stats marked (free text) in ' diff --git a/backend/app/worldstate/render.py b/backend/app/worldstate/render.py index 5fe8fc6..421a5bc 100644 --- a/backend/app/worldstate/render.py +++ b/backend/app/worldstate/render.py @@ -83,20 +83,31 @@ def render_state_section(world_state: dict, stat_schema: dict, def _describe_stat(name: str, d: dict) -> str | None: - """Returns one reference line for a stat. + """Returns one reference line for a stat, named by the path the AI writes. + + `name` is that path. The guide is the model's only complete list of what + exists, so a stat named any other way leaves it to work the path out from + the live values, and those cover only what is on screen this turn. The description and the band ladder are independent, and each is included only when present, so a stat may have either, both, or neither. """ + is_text = d.get("type") == "text" + # `EMIT_RULE` sends the model here to find out which stats take a whole + # value instead of a change, and it names this marker, so the two have to + # be written the same way. + label = f"{name} (free text)" if is_text else name bits: list[str] = [] desc = d.get("desc") if isinstance(desc, str) and desc.strip(): # Fragments are joined with "; " and end with a single ".", so remove # any trailing period the author put on the description. bits.append(desc.strip().rstrip(".")) - if d.get("type") == "text": - bits.append("free text") - return f"{name} — {'; '.join(bits)}." if bits else None + if is_text: + # Free text has no range and no bands, so the marker is the whole + # entry. It still earns a line without a description, because the + # marker is what stops the model sending a delta. + return f"{label} — {'; '.join(bits)}." if bits else f"{label}." lo, hi = d.get("min"), d.get("max") if isinstance(lo, (int, float)) and isinstance(hi, (int, float)): bits.append(f"range {lo}–{hi}") @@ -108,7 +119,7 @@ def _describe_stat(name: str, d: dict) -> str | None: ) if ladder: bits.append(f"bands: {ladder}") - return f"{name} — {'; '.join(bits)}." if bits else None + return f"{label} — {'; '.join(bits)}." if bits else None def render_reference(stat_schema: dict) -> str: @@ -122,7 +133,7 @@ def render_reference(stat_schema: dict) -> str: for section in STAT_SECTIONS: for name, d in (stat_schema.get(section) or {}).items(): if isinstance(d, dict): - row = _describe_stat(name, d) + row = _describe_stat(f"{section}.{name}", d) if row: lines.append(row) for npc_key, ndef in (stat_schema.get("npcs") or {}).items(): @@ -130,18 +141,27 @@ def render_reference(stat_schema: dict) -> str: continue name = npc_name(ndef, npc_key) desc = ndef.get("desc") - if isinstance(desc, str) and desc.strip(): - lines.append(f"NPC {name} ({npc_key}) — {desc.strip().rstrip('.')}.") + # The header is written even for an NPC with no description, because it + # is the only line that ties a display name to the id the AI has to + # address. The live values state it too, but only for the NPCs a scene + # has mentioned, so without this an NPC off screen can only be guessed + # at — and a guess is refused as a character or a stat that does not + # exist. + head = f"NPC {name} (npc.{npc_key})" + lines.append( + f"{head} — {desc.strip().rstrip('.')}." + if isinstance(desc, str) and desc.strip() else f"{head}." + ) for sname, sdef in (ndef.get("stats") or {}).items(): if isinstance(sdef, dict): - row = _describe_stat(f"{name} {sname}", sdef) + row = _describe_stat(f"npc.{npc_key}.{sname}", sdef) if row: lines.append(row) for name, d in (stat_schema.get("flags") or {}).items(): if isinstance(d, dict): desc = d.get("desc") if isinstance(desc, str) and desc.strip(): - lines.append(f"{name} (flag) — {desc.strip().rstrip('.')}.") + lines.append(f"flags.{name} — {desc.strip().rstrip('.')}.") if not lines: return "" return "Stat guide (fixed reference):\n" + "\n".join(f"- {ln}" for ln in lines) diff --git a/backend/tests/test_delete_state.py b/backend/tests/test_delete_state.py new file mode 100644 index 0000000..533d121 --- /dev/null +++ b/backend/tests/test_delete_state.py @@ -0,0 +1,214 @@ +"""Deleting a turn puts the shared state back. + +`script_state` and `world_state` belong to the adventure, not to the node +that changed them. Undo, retry, a take and a branch switch all restore them; +the delete endpoint did not. Deleting an AI turn removed the text and left +everything the turn did to the numbers standing. + +The visible symptom was the cooldown clock. It lives in +`world_state._meta.last_changed` and it holds a depth. A deleted turn left +its depth there, and the turn played in its place is played at that same +depth, so the referee refused the change as one that had happened this very +turn — on a turn the story no longer contains. Delete the AI reply because +you did not like the stat change it proposed, press Continue, and the same +change comes back refused as "changed too recently". + + python -m pytest tests/test_delete_state.py -v +""" +import pytest +from fastapi import Depends +from fastapi.testclient import TestClient + +from app import auth, limits, models +from app.database import Base, SessionLocal, engine, get_db +from app.main import app +from app.routers import adventures +from fakes import ScriptedProvider + +# `mana` carries a cooldown, so a clock that was not rolled back shows up as +# a refusal rather than as a number that is merely off. +SCHEMA = { + "player": { + "hp": {"min": 0, "max": 100, "initial": 100}, + "mana": {"min": 0, "max": 50, "initial": 50, "cooldown": 2}, + } +} + +# Ten gold a turn. A total that only ever climbs makes a missing rollback +# obvious: it is off by exactly one turn's worth. +GOLD_SCRIPT = """ +const modifier = (text) => { + state.gold = (state.gold || 0) + 10; + return { text }; +}; +modifier(text); +""" + +DRAIN = 'Drained.\n```state\n{"player.mana": -10}\n```' + + +@pytest.fixture() +def client(monkeypatch): + Base.metadata.create_all(bind=engine) + setup = SessionLocal() + user = models.User(is_guest=False, email="delstate@example.com") + setup.add(user) + setup.flush() + setup.add(models.Settings(user_id=user.id, api_key="enc:dummy", model="test-model")) + scenario = models.Scenario(user_id=user.id, title="S", stat_schema=SCHEMA) + setup.add(scenario) + setup.flush() + adv = models.Adventure( + user_id=user.id, title="Tower", scenario_id=scenario.id, + script_state={}, world_state={"player": {"hp": 100, "mana": 50}}, + ) + setup.add(adv) + setup.flush() + setup.add(models.Action(adventure_id=adv.id, type="start", text="You begin.")) + setup.add(models.AdventureScript( + adventure_id=adv.id, position=0, enabled=True, name="Gold", output_js=GOLD_SCRIPT, + )) + setup.commit() + adv_id, user_id = adv.id, user.id + setup.close() + + ScriptedProvider.replies = [DRAIN] + ScriptedProvider.calls = 0 + monkeypatch.setattr(adventures.turns, "OpenAICompatibleProvider", ScriptedProvider) + monkeypatch.setattr(auth, "resolve_provider_config", lambda s: auth.ProviderConfig( + "http://fake", "k", "test-model", False)) + monkeypatch.setattr(limits, "rate_limit", lambda *a, **k: None) + monkeypatch.setattr(limits, "check_row_cap", lambda *a, **k: None) + + def _current_user(db=Depends(get_db)): + return db.get(models.User, user_id) + + app.dependency_overrides[auth.get_current_user] = _current_user + c = TestClient(app) + c.adv_id = adv_id + try: + yield c + finally: + app.dependency_overrides.clear() + adventures.turns._active_turns.clear() + Base.metadata.drop_all(bind=engine) + + +# ------------------------------------------------------------------ helpers + +def _play(client, text="look around"): + r = client.post(f"/api/adventures/{client.adv_id}/actions", + json={"type": "do", "text": text}) + assert r.status_code == 200, r.text + + +def _continue(client): + r = client.post(f"/api/adventures/{client.adv_id}/actions", + json={"type": "continue", "text": ""}) + assert r.status_code == 200, r.text + + +def _delete(client, action_id): + return client.delete(f"/api/adventures/{client.adv_id}/actions/{action_id}") + + +def _state(adv_id): + db = SessionLocal() + try: + adv = db.get(models.Adventure, adv_id) + return adv.script_state, adv.world_state + finally: + db.close() + + +def _ai_rows(adv_id): + db = SessionLocal() + try: + return ( + db.query(models.Action) + .filter(models.Action.adventure_id == adv_id, models.Action.type == "ai") + .order_by(models.Action.depth, models.Action.id) + .all() + ) + finally: + db.close() + + +def _last_changes(adv_id): + db = SessionLocal() + try: + adv = db.get(models.Adventure, adv_id) + return adv.actions[-1].world_changes + finally: + db.close() + + +# ------------------------------------------------------- the reported bug + +def test_deleting_the_ai_turn_rewinds_the_world_state(client): + _play(client) + assert _state(client.adv_id)[1]["player"]["mana"] == 40 + + _delete(client, _ai_rows(client.adv_id)[-1].id) + + _, world = _state(client.adv_id) + assert world["player"]["mana"] == 50, "the drain went with the turn" + assert not (world.get("_meta") or {}).get("last_changed"), "and so did its clock" + + +def test_the_next_turn_is_not_refused_for_a_deleted_turn_s_cooldown(client): + """The bug as a player meets it: delete the reply, press Continue, and + the change it proposes is refused as one that already happened.""" + _play(client) + _delete(client, _ai_rows(client.adv_id)[-1].id) + + _continue(client) + + assert _state(client.adv_id)[1]["player"]["mana"] == 40, "the drain lands" + assert [c for c in _last_changes(client.adv_id) if c["kind"] == "rejected"] == [] + + +def test_deleting_the_ai_turn_rewinds_the_script_state(client): + """The same restore, on the other half of the shared state. Without it a + replayed turn stacks its script run on top of the deleted one's.""" + _play(client) + assert _state(client.adv_id)[0] == {"gold": 10} + + _delete(client, _ai_rows(client.adv_id)[-1].id) + assert _state(client.adv_id)[0] == {} + + _continue(client) + assert _state(client.adv_id)[0] == {"gold": 10}, "one turn of gold, not two" + + +# ------------------------------------------------- deleting further back + +def test_deleting_a_turn_the_story_moved_past_leaves_the_tip_alone(client): + """A restore reads the tip's own outcome, not the deleted node's + neighbour, so removing a turn from the middle of the story does not roll + the numbers back to that point. The text goes; the state stays.""" + _play(client) + _play(client, "press on") + before = _state(client.adv_id) + assert before[0] == {"gold": 20} + + first_ai = _ai_rows(client.adv_id)[0] + assert _delete(client, first_ai.id).status_code == 204 + + assert _state(client.adv_id) == before + + +def test_delete_is_blocked_while_a_turn_is_generating(client): + """The endpoint writes the shared state now, so it takes the same lock + undo takes rather than racing the turn that is about to write it.""" + _play(client) + action_id = _ai_rows(client.adv_id)[-1].id + + adventures.turns.acquire_turn_lock(client.adv_id) # a turn is "generating" + try: + assert _delete(client, action_id).status_code == 409 + # The refused delete must not have released someone else's lock. + assert client.adv_id in adventures.turns._active_turns + finally: + adventures.turns._active_turns.discard(client.adv_id) + assert len(_ai_rows(client.adv_id)) == 1, "and the turn is still there" diff --git a/backend/tests/test_worldstate.py b/backend/tests/test_worldstate.py index d347821..a9528b4 100644 --- a/backend/tests/test_worldstate.py +++ b/backend/tests/test_worldstate.py @@ -209,10 +209,48 @@ def test_reference_includes_desc_and_bands_independently(): assert "very weak" in guide and "range 0–100" in guide # A counter like day has no desc or bands, so it contributes nothing # here. Flags do show their desc. - assert "has_key (flag) — Holds the key." in guide + assert "flags.has_key — Holds the key." in guide # NPCs contribute their own description and per-NPC stat lines. - assert "NPC Gwen (gwen) — A loyal ranger." in guide - assert "Gwen trust" in guide and "The Drake ferocity" in guide + assert "NPC Gwen (npc.gwen) — A loyal ranger." in guide + assert "npc.gwen.trust" in guide and "npc.drake.ferocity" in guide + + +def test_reference_names_every_stat_by_its_path(): + """The guide is the model's only complete list of what exists, so it has + to name each stat the way a state block must name it. Naming a stat after + its owner's display name ("Trainer Milo active_status") left the model to + build the path itself, and the paths it built were refused as stats and + characters that do not exist.""" + guide = w.render_reference(SCHEMA) + assert "player.hp" in guide + assert "player.outfit" in guide + # The old wording, which read as prose rather than as an address. + assert "Gwen trust" not in guide + assert "The Drake ferocity" not in guide + + +def test_reference_names_an_npc_that_has_no_description(): + """The live values name an NPC only while a scene mentions them, so the + guide is the only place an off-screen NPC's id is stated. The Drake has no + `desc`, and used to reach the model as "The Drake ferocity" alone.""" + guide = w.render_reference(SCHEMA) + assert "NPC The Drake (npc.drake)." in guide + + +def test_reference_marks_free_text_stats_the_way_the_emit_rule_names_them(): + """`EMIT_RULE` tells the model that a stat "marked (free text) in the stat + guide" takes a whole value rather than a delta, so the guide has to carry + that marker literally.""" + assert "(free text)" in w.EMIT_RULE + guide = w.render_reference(SCHEMA) + assert "player.outfit (free text) — What the player is wearing." in guide + + +def test_reference_lists_a_free_text_stat_with_no_description(): + """The marker alone is the entry. Dropping the line would leave the model + sending a number for a stat that holds a string.""" + schema = {"player": {"holding": {"type": "text", "initial": ""}}} + assert "player.holding (free text)." in w.render_reference(schema) def test_unknown_paths_rejected_not_fatal(): diff --git a/frontend/src/pages/Play/reports.jsx b/frontend/src/pages/Play/reports.jsx index e569d59..8c04a98 100644 --- a/frontend/src/pages/Play/reports.jsx +++ b/frontend/src/pages/Play/reports.jsx @@ -53,9 +53,10 @@ function StateChangeChips({ changes }) { // neither an update nor a refusal, and "+0" read as the former. const blocked = c.clamped && d === 0 const dir = blocked ? 'refused' : typeof d === 'number' ? (d > 0 ? 'up' : d < 0 ? 'down' : 'flat') : 'flat' + const signed = !blocked && typeof d === 'number' const txt = blocked ? 'no change — at its limit' - : typeof d === 'number' ? (d > 0 ? `+${d}` : `${d}`) : `→ ${c.value}` + : signed ? (d > 0 ? `+${d}` : `${d}`) : `→ ${c.value}` const title = blocked ? c.fix || 'The story asked to change this and it is already at the limit the scenario allows.' : c.clamped @@ -63,7 +64,7 @@ function StateChangeChips({ changes }) { : undefined return ( - {nice(c.label)} {txt} + {nice(c.label)} {txt} {c.clamped && !blocked ? (limited) : null} ) diff --git a/frontend/src/styles/schema-editor.css b/frontend/src/styles/schema-editor.css index dacc093..c32779a 100644 --- a/frontend/src/styles/schema-editor.css +++ b/frontend/src/styles/schema-editor.css @@ -208,13 +208,19 @@ font-variant-numeric: tabular-nums; /* A chip must never be wider than the column it sits in: an NPC stat like "bandit leader aggression +10" overruns a phone, and the chip row is - inside the story text, which has nothing to scroll. The label may break - mid-word as a last resort; the value is glued to it by its own nowrap so - the sign and number never end up on separate lines. */ + inside the story text, which has nothing to scroll. Text may break + mid-word as a last resort, which is what keeps a chip's minimum width + small enough for the flex row to shrink it. */ max-width: 100%; overflow-wrap: anywhere; } -.chg-val { white-space: nowrap; } +.chg-val { overflow-wrap: anywhere; } +/* Only a signed number is held on one line, so the sign never ends up on a + line of its own. The other values a chip can carry are phrases — a refusal + reason, "no change — at its limit", or the whole new value of a free-text + stat — and holding those together set the chip's minimum width to the width + of the phrase, which is what pushed a chip out of the message on a phone. */ +.chg-num { white-space: nowrap; } .chg-up { color: var(--player, #4caf82); border-color: color-mix(in srgb, var(--player, #4caf82) 40%, var(--border)); } .chg-down { color: var(--danger, #e5484d); border-color: color-mix(in srgb, var(--danger, #e5484d) 40%, var(--border)); } .chg-flag { color: var(--accent-bright); }