Merge main into the Phase 17 refactor
`main` changed the four files this branch split into packages, so all four came back as modify/delete conflicts. Each change is ported to the module that now holds the code, unchanged in behavior: - `delete_action` moves to `routers/adventures/actions.py`. It reaches the turn lock as `turns.acquire_turn_lock` and `turns._active_turns`, which is the rule this branch set: the package root no longer re-exports the names a test rebinds. - `EMIT_RULE` moves to `worldstate/parse.py`, and `_describe_stat` and `render_reference` to `worldstate/render.py`. - The chip-wrap rules move to `styles/schema-editor.css`, and `StateChangeChips` to `pages/Play/reports.jsx`. `index.css` keeps this branch's import list. `test_delete_state.py` needed four edits to run here. It drops the eight-line database prologue, because `conftest.py` does that once now. It imports the shared `ScriptedProvider` from `fakes.py` instead of carrying a copy. It patches `adventures.turns.OpenAICompatibleProvider` and reaches the lock the same way. And it no longer passes `index=0` when it builds the start action, because migration 71 drops that column. 564 tests pass. `npm run lint` and `npm run build` are clean, and the CSS bundle is 56.71 kB, which is the pre-merge size plus main's new rules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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.<stat>", '
|
||||
'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.<stat>", '
|
||||
'"world.<stat>", "npc.<id>.<stat>" (use the id in parentheses, e.g. npc.gwen.trust, '
|
||||
'not the display name); "flags.<name>": true or false to toggle an on/off state; and '
|
||||
'"milestones.<id>": true when an objective is completed. Some stats marked (free text) in '
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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"
|
||||
@@ -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():
|
||||
|
||||
Reference in New Issue
Block a user