diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index d5e7608..0b3e97a 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -209,7 +209,7 @@ visible from within. ## Tests ```bash -cd backend && .venv/bin/python -m pytest tests/ -q # 680 tests +cd backend && .venv/bin/python -m pytest tests/ -q # 698 tests cd frontend && npm run lint && npm run build ``` @@ -226,8 +226,15 @@ is lost from the union, or if a new HTTP client is added without the shared verification context. M4 added `test_save_points.py`, which fails if restoring a Save Point starts -deleting history, stops going through the active head, forks on its own, or lets -a Save Point on one campaign be restored through another. +deleting history, stops going through the active head, forks on its own, lets a +Save Point on one campaign be restored through another, or lets deleting a branch +take a Save Point with it. It also fails if listing Save Points goes back to one +query per Save Point, or starts fetching narration to render the list. + +`test_process_restart.py` is the durability guard: it starts the application as a +real subprocess, kills it, and starts a second one against the same database. A +Save Point that survived only because a Python object was still alive would pass +an in-process test and fail a user's restart. M2 added two more. `test_endpoint_policy.py` fails if the set of reachable addresses widens, or if either place the rule is applied stops applying it — diff --git a/README.md b/README.md index d2f2043..cf764b1 100644 --- a/README.md +++ b/README.md @@ -74,8 +74,10 @@ that isn't the live one starts a new branch. is what starts a new line while the old one is kept. A Save Point is a name for a position and holds no copy of the story, so restoring it is the same movement Undo makes (`backend/app/routers/adventures/checkpoints.py`, - `backend/app/head.py`). They last until you delete them, and deleting one - deletes no story. + `backend/app/head.py`). They last until *you* delete them: deleting one deletes + no story, and deleting a branch a Save Point is kept on is refused until you + remove the Save Point yourself, so nothing takes a named moment away behind + your back. - **Import and export.** AI Dungeon-compatible scenario format; JSON for everything else. An adventure exports as `ai-dnd-adventure-v2`, which carries the whole tree: every branch, every take, the fork points, which branches the story has left behind, the Save Points and the position it is being read at — all of them chosen rather than computed, which is @@ -98,8 +100,9 @@ that isn't the live one starts a new branch. None yet. The inherited screenshots showed upstream's UI — a Scripts tab, Log in and Sign up, a guest banner, scripting demo scenarios — none of which this fork has since M2, so they were -removed rather than left standing as a picture of a product that no longer exists. New ones -are taken when the browser smoke test M3 still owes is run. +removed rather than left standing as a picture of a product that no longer exists. The M4 +closeout drove the real application in a real browser, so the screens exist and work; taking +presentable screenshots of them is a job for the UI pass in M8. ## Quick start @@ -221,7 +224,7 @@ development, Vite proxies `/api` to FastAPI. ## Tests -680 backend tests: unit tests plus full HTTP integration through the real turn engine, with +698 backend tests: unit tests plus full HTTP integration through the real turn engine, with the model provider mocked. They run with no route to the Internet, which is a requirement rather than a convenience — an offline claim proved on a machine that has been online once proves nothing. diff --git a/backend/app/models.py b/backend/app/models.py index e607b27..8aff36f 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -265,11 +265,18 @@ class Checkpoint(Base): resting in a shared prefix, and the node's own branch is the one that still names the position after the reader has moved elsewhere. - Deleting a branch deletes its Save Points, by the same cascade that takes - its memories: the story the pointer names is gone with it. Nothing else - removes one. They are not cleaned up for going stale, for being behind the - head, or for pointing into a future the story has left - (`STORY-BRANCH-SEMANTICS.md` §19). + **Nothing removes a Save Point but the user.** They are not cleaned up for + going stale, for being behind the head, or for pointing into a future the + story has left (`STORY-BRANCH-SEMANTICS.md` §19). + + That includes deleting a branch. `branch_id` carries `ON DELETE CASCADE` as + referential integrity — a Save Point must never point at a branch that is + gone — but the branch endpoint refuses to delete a branch any Save Point + names, so the cascade does not fire through the application + (`routers/adventures/branches.py`, `STORY-BRANCH-SEMANTICS.md` §19.1). The + user deletes the Save Point first, which deletes no story, and then the + branch. Deleting the whole campaign does cascade, and should: that is what + the user asked for. """ __tablename__ = "checkpoints" diff --git a/backend/app/routers/adventures/branches.py b/backend/app/routers/adventures/branches.py index a09f73a..4f572f0 100644 --- a/backend/app/routers/adventures/branches.py +++ b/backend/app/routers/adventures/branches.py @@ -56,6 +56,19 @@ def list_branches( .group_by(models.Action.branch_id) .all() } + # M4 closeout: how many Save Points name a position on each line. Deleting a + # branch deletes them along with its story, and the panel has to be able to + # say so before the button is pressed (review §R B-2). One grouped query for + # the whole tree, like the one above it — never one per branch. + save_points = { + branch_id: count + for branch_id, count in db.query( + models.Checkpoint.branch_id, func.count(models.Checkpoint.id) + ) + .filter(models.Checkpoint.adventure_id == adventure.id) + .group_by(models.Checkpoint.branch_id) + .all() + } out = [] for branch in branches: count, tip = owned.get(branch.id, (0, None)) @@ -70,6 +83,7 @@ def list_branches( branch.fork_depth if branch.fork_depth is not None else tree.NO_DEPTH ), own_actions=count, + save_points=save_points.get(branch.id, 0), is_head=(branch.id == adventure.head_branch_id), name=branch.name, created_at=branch.created_at, @@ -151,15 +165,31 @@ def delete_branch( heavily retried adventure from growing without bound. That is why it ships with the view that first lets anyone create a fork rather than after it. - Two kinds of branch cannot be deleted. The root cannot, because it holds the - turns every other branch borrows, so deleting it deletes the whole story. The - branch currently being read cannot, and neither can any branch it was forked - from, because the cascade would remove the head under the player and leave - `head_branch_id` dangling. Switch branches first. + Three kinds of branch cannot be deleted. The root cannot, because it holds + the turns every other branch borrows, so deleting it deletes the whole story. + The branch currently being read cannot, and neither can any branch it was + forked from, because the cascade would remove the head under the player and + leave `head_branch_id` dangling. Switch branches first. + + The third is M4's: **a branch a Save Point names cannot be deleted while that + Save Point exists.** `STORY-BRANCH-SEMANTICS.md` §19 says a named checkpoint + remains until explicitly deleted, and §28 says a future cleanup feature must + retain paths referenced by checkpoints. A cascade that removed Save Points + along with a branch would break both, and would break them silently: the + story the user asked to delete is the visible thing, and the named moments + would go without ever being named in the request. So the deletion is refused, + the Save Points are listed, and the user decides — delete the Save Point + first, then the branch. Deleting a Save Point still deletes no story (§25), + so the recovery costs nothing but a click. + + The check covers the whole doomed subtree, not just this branch, because + deleting a branch takes everything forked from it. Nodes and memories are deleted by `ON DELETE CASCADE`, and descendants by the cascade on `branches.parent_branch_id`, so the delete is a single statement - however deep the subtree is. + however deep the subtree is. `checkpoints.branch_id` also carries a cascade, + as referential integrity — a Save Point must never point at a branch that is + gone — but the guard above means it does not fire through this endpoint. """ branch = get_branch_or_404(adventure, branch_id, db) if branch.parent_branch_id is None: @@ -178,6 +208,11 @@ def delete_branch( 400, "You are reading this branch, or one forked from it. Switch to " "another branch first.", ) + # Refused before the lock is taken: this is a decision about the request, not + # a race with a turn. + protecting = _save_points_protecting(db, adventure, branch) + if protecting: + raise HTTPException(409, _protected_message(protecting)) turns.acquire_turn_lock(adventure_id) try: # Collect the subtree before the delete, because afterwards there is no @@ -199,6 +234,55 @@ def delete_branch( finally: turns._active_turns.discard(adventure_id) +# How many Save Point names to spell out before the message starts summarising. +# Enough to be actionable, few enough to stay a sentence. +NAMED_IN_REFUSAL = 3 + + +def _save_points_protecting( + db: Session, adventure: models.Adventure, branch: models.Branch +) -> list[models.Checkpoint]: + """Returns the Save Points that deleting `branch` would destroy. + + The whole subtree, because deleting a branch takes everything forked from + it, and a check that looked only at this branch would let a Save Point on a + child be deleted without a word. + """ + doomed = _branch_subtree(db, adventure, branch) + return ( + db.query(models.Checkpoint) + .filter( + models.Checkpoint.adventure_id == adventure.id, + models.Checkpoint.branch_id.in_(doomed), + ) + .order_by(models.Checkpoint.created_at, models.Checkpoint.id) + .all() + ) + + +def _protected_message(protecting: list[models.Checkpoint]) -> str: + """Says which Save Points stand in the way, and what to do about it. + + Named rather than counted, because "2 Save Points" leaves the user hunting + for which ones. A long list is truncated so the message stays readable; the + Save Points panel shows the rest. + """ + names = [f"“{c.name}”" for c in protecting[:NAMED_IN_REFUSAL]] + listed = ", ".join(names) + extra = len(protecting) - len(names) + if extra > 0: + listed += f" and {extra} more" + subject = "a Save Point" if len(protecting) == 1 else "Save Points" + return ( + f"This branch, or a branch forked from it, is where {subject} " + f"{listed} {'is' if len(protecting) == 1 else 'are'} saved. Delete " + f"{'that Save Point' if len(protecting) == 1 else 'those Save Points'} " + f"first if you no longer need " + f"{'it' if len(protecting) == 1 else 'them'}, then delete the branch. " + f"Deleting a Save Point does not delete any story." + ) + + def _branch_subtree( db: Session, adventure: models.Adventure, root: models.Branch ) -> set[int]: diff --git a/backend/app/routers/adventures/checkpoints.py b/backend/app/routers/adventures/checkpoints.py index c9b99f9..a8c56ce 100644 --- a/backend/app/routers/adventures/checkpoints.py +++ b/backend/app/routers/adventures/checkpoints.py @@ -23,7 +23,10 @@ The user-facing word is "Save Point" and the internal one is `checkpoint` Save Point. """ +from dataclasses import dataclass + from fastapi import Depends, HTTPException +from sqlalchemy import and_, or_ from sqlalchemy.orm import Session from ... import head, models, schemas @@ -64,18 +67,92 @@ def _node_at( ) +@dataclass(frozen=True) +class _Coordinate: + """The shape `lineage.Path` reads, without loading a story row. + + `Path.contains` asks three things of a node: its branch, its depth, and + whether it is live. A coordinate already known to resolve has all three, so + the membership question can be put to the coordinate itself. That keeps the + single implementation of "is this on the path being read" in `lineage`, + where M3 put it, while costing no query and no prose. + """ + + branch_id: int + depth: int + live: bool = True + + +def _live_coordinates( + db: Session, adventure: models.Adventure, checkpoints: list[models.Checkpoint] +) -> set[tuple[int, int]]: + """Returns which of these Save Points' coordinates still name a live turn. + + One query for the whole list, selecting two integer columns. + + This replaces a resolution per Save Point (M4 review §R B-1), which cost one + query each and loaded whole `Action` entities — narration included — to + answer a question that is only ever "does a row exist here". `paging.py` + states the rule this now follows: a bulk read names the columns it needs, so + a new column costs nothing until someone adds it to the list. + + The clause is an OR of exact `(branch, depth)` pairs rather than + `branch IN (…) AND depth IN (…)`, which would match the cross product and + report a Save Point as resolved because *some other* Save Point's depth + exists on *this* one's branch. + """ + coordinates = {(c.branch_id, c.depth) for c in checkpoints} + if not coordinates: + return set() + rows = ( + db.query(models.Action.branch_id, models.Action.depth) + .filter( + models.Action.adventure_id == adventure.id, + models.Action.live.is_(True), + or_(*[ + and_(models.Action.branch_id == branch, models.Action.depth == depth) + for branch, depth in coordinates + ]), + ) + .all() + ) + return {(branch, depth) for branch, depth in rows} + + +def _render_all( + db: Session, adventure: models.Adventure, checkpoints: list[models.Checkpoint] +) -> list[schemas.CheckpointOut]: + """Reads Save Points out with the three facts the panel needs about them. + + Bounded work whatever the length of the list: one query for the coordinates + and one lineage for the campaign, both computed before the loop. Rendering + one Save Point and rendering fifty differ in Python, not in round trips. + """ + live = _live_coordinates(db, adventure, checkpoints) + # The path is a property of the campaign, not of any Save Point, so it is + # read once. Reading it per row was the other half of the N+1. + path = lineage.path_of(db, adventure).uncapped() + out = [] + for checkpoint in checkpoints: + coordinate = (checkpoint.branch_id, checkpoint.depth) + resolved = coordinate in live + rendered = schemas.CheckpointOut.model_validate(checkpoint) + # The same `depth + 1` the branch list counts with, so a moment number + # means the same thing in both places. + rendered.turn = checkpoint.depth + 1 + rendered.resolved = resolved + rendered.on_path = resolved and path.contains( + _Coordinate(checkpoint.branch_id, checkpoint.depth) + ) + out.append(rendered) + return out + + def _rendered( db: Session, adventure: models.Adventure, checkpoint: models.Checkpoint ) -> schemas.CheckpointOut: - """Reads one Save Point out with the three facts the panel needs about it.""" - node = _node_at(db, adventure, checkpoint.branch_id, checkpoint.depth) - out = schemas.CheckpointOut.model_validate(checkpoint) - # The same `depth + 1` the branch list counts with, so "turn 42" means the - # same thing in both places. - out.turn = checkpoint.depth + 1 - out.resolved = node is not None - out.on_path = node is not None and lineage.path_of(db, adventure).uncapped().contains(node) - return out + """Reads one Save Point out, through the same path the list uses.""" + return _render_all(db, adventure, [checkpoint])[0] def _get_or_404( @@ -123,7 +200,7 @@ def list_checkpoints( .order_by(models.Checkpoint.created_at.desc(), models.Checkpoint.id.desc()) .all() ) - return [_rendered(db, adventure, row) for row in rows] + return _render_all(db, adventure, rows) @router.post( @@ -132,6 +209,7 @@ def list_checkpoints( status_code=201, ) def create_checkpoint( + adventure_id: int, payload: schemas.CheckpointCreate, db: Session = Depends(get_db), adventure: models.Adventure = Depends(current_adventure), @@ -148,22 +226,37 @@ def create_checkpoint( head resting in a shared prefix sits on an ancestor's node, and the ancestor is the branch that still names that position after the reader has forked away from it. + + **Held under the campaign's turn lock** (M4 closeout, review §S C-5). "Save + where I am" has to name one committed position, and the head is exactly what + a turn in flight is about to move. Without the lock this endpoint could read + `head_depth` while a turn was mid-commit and store a coordinate for a + position the story had already left — a Save Point silently naming the wrong + moment, which no later operation could detect. It is the same lock Undo, + Redo and Restore take, for the same reason, and not a new mechanism. + + Rename and Delete deliberately do **not** take it: neither reads nor moves a + story position, so there is nothing for a turn in flight to race them over. """ name = _clean_name(payload.name) - node = head.node_at(db, adventure, adventure.head_depth) - if node is None: - raise HTTPException(400, "There is no turn here to save yet.") - checkpoint = models.Checkpoint( - adventure_id=adventure.id, - name=name, - note=payload.note or "", - branch_id=node.branch_id, - depth=node.depth, - ) - db.add(checkpoint) - db.commit() - db.refresh(checkpoint) - return _rendered(db, adventure, checkpoint) + turns.acquire_turn_lock(adventure_id) + try: + node = head.node_at(db, adventure, adventure.head_depth) + if node is None: + raise HTTPException(400, "There is no turn here to save yet.") + checkpoint = models.Checkpoint( + adventure_id=adventure.id, + name=name, + note=payload.note or "", + branch_id=node.branch_id, + depth=node.depth, + ) + db.add(checkpoint) + db.commit() + db.refresh(checkpoint) + return _rendered(db, adventure, checkpoint) + finally: + turns._active_turns.discard(adventure_id) @router.patch( diff --git a/backend/app/schemas.py b/backend/app/schemas.py index c5df8ee..e206105 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -259,6 +259,10 @@ class BranchOut(ORMModel): fork_depth: int | None = None depth: int own_actions: int = 0 + # M4: how many Save Points name a position on this line. Deleting the branch + # deletes them with its story, so the panel warns with a number rather than + # a vague caution. Zero for a line nobody has bookmarked, which is most. + save_points: int = 0 is_head: bool = False # NULL for a branch nobody has named. The client labels those from the fork # depth rather than the server inventing a name. See the column comment. diff --git a/backend/tests/_restart_server.py b/backend/tests/_restart_server.py new file mode 100644 index 0000000..0dfcfeb --- /dev/null +++ b/backend/tests/_restart_server.py @@ -0,0 +1,63 @@ +"""The storyteller, run as a real OS process for `test_process_restart.py`. + +Not a test module, and named so pytest does not collect it: it is the program +the test starts, twice, against one database file. + +The model is replaced with a deterministic fake before the app is imported, so +the process needs no Ollama, no network and no configuration. Everything else — +the engine, the migrations, the routers, the session lifecycle — is the real +application, which is the whole point of spawning a process at all. + + python _restart_server.py +""" +import itertools +import os +import sys +from pathlib import Path + +HERE = Path(__file__).resolve().parent +sys.path.insert(0, str(HERE.parent)) # backend/, so `app` imports +sys.path.insert(0, str(HERE)) # tests/, so `fakes` imports + +db_path, port = sys.argv[1], int(sys.argv[2]) +os.environ["AIDND_DB_PATH"] = db_path +# A developer's shell may point these at Postgres, and `app.database` prefers +# either over the SQLite path. The suite's conftest clears them for the same +# reason; a spawned process does not inherit that, so clear them here too. +os.environ.pop("AIDND_DATABASE_URL", None) +os.environ.pop("DATABASE_URL", None) + +from fakes import GOLD_PER_TURN, gold_reply # noqa: E402 + +_turn = itertools.count(1) + + +class DeterministicProvider: + """Banks one turn's worth of gold per reply, numbered so text is checkable. + + The same instrumentation `test_head_cursor.py` and `test_save_points.py` + use, for the same reason: it makes "the state at this position" a number the + test can assert rather than a paragraph it has to interpret. M5 replaces the + machinery underneath; what this measures is where the story is being read. + """ + + last_usage = None + + def __init__(self, *a, **k): + pass + + async def generate(self, parts, *, temperature, max_tokens): + yield ("text", gold_reply(f"Beat {next(_turn)}.")) + + +from app.routers.adventures import turns # noqa: E402 + +turns.OpenAICompatibleProvider = DeterministicProvider + +from app.main import app # noqa: E402 + +if __name__ == "__main__": + import uvicorn + + # Loopback only, as every supported start path does. + uvicorn.run(app, host="127.0.0.1", port=port, log_level="warning") diff --git a/backend/tests/test_process_restart.py b/backend/tests/test_process_restart.py new file mode 100644 index 0000000..63c926f --- /dev/null +++ b/backend/tests/test_process_restart.py @@ -0,0 +1,325 @@ +"""D11 and L03 across a genuine OS process boundary. + +The rest of the suite runs the app in-process through `TestClient`, which is the +right tool for almost everything and the wrong one for exactly one claim: +*durability*. A Save Point that survived only because a Python object was still +alive would pass a same-process test and fail a user's restart. `TestClient` +cannot tell those apart, so the M4 review recorded the shipped D11/L03 tests as +weaker than the acceptance items they were named for (§R B-3). + +This module closes that. It starts the real application as a **subprocess**, +plays a story over HTTP, kills the process, starts a **second** process against +the same database file, and only then asks whether the Save Point is still +there. Everything crossing the boundary crosses it as bytes on disk. + +Deterministic and local: the spawned server replaces the model with a scripted +provider (`_restart_server.py`), so there is no Ollama, no network and no +sleep-and-hope — readiness is probed, not waited for. + + python -m pytest tests/test_process_restart.py -v +""" +import json +import os +import socket +import subprocess +import sys +import tempfile +import time +import urllib.error +import urllib.request +from pathlib import Path + +import pytest + +from fakes import GOLD_PER_TURN + +HERE = Path(__file__).resolve().parent +SERVER = HERE / "_restart_server.py" + +# How long a spawned server may take to answer before the test gives up. The +# process imports the app and runs migrations on a fresh file, which is well +# under a second on this project; the ceiling is for a loaded machine. +STARTUP_TIMEOUT = 60.0 + + +def _free_port() -> int: + """Returns a port nothing is listening on. + + Bind, read, release. There is a race between releasing and the server + claiming it, which is why the caller probes for readiness rather than + assuming success — a lost race shows up as a startup timeout, not as a + silent pass. + """ + with socket.socket() as s: + s.bind(("127.0.0.1", 0)) + return s.getsockname()[1] + + +class Server: + """One storyteller process, and the HTTP calls the test makes against it.""" + + def __init__(self, db_path: str, port: int): + self.port = port + self.proc = subprocess.Popen( + [sys.executable, str(SERVER), db_path, str(port)], + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + # Never inherit the parent's database redirection; the child is told + # which file to open on its command line. + env={**os.environ, "AIDND_DB_PATH": db_path}, + ) + + # ------------------------------------------------------------ lifecycle + + def wait_until_ready(self) -> None: + deadline = time.monotonic() + STARTUP_TIMEOUT + while time.monotonic() < deadline: + if self.proc.poll() is not None: + raise AssertionError( + f"server exited early ({self.proc.returncode}):\n{self._output()}" + ) + try: + self.call("GET", "/settings") + return + except (urllib.error.URLError, ConnectionError, OSError): + time.sleep(0.05) + raise AssertionError(f"server never became ready:\n{self._output()}") + + def stop(self) -> None: + """Ends the process, and does not return until it is actually gone.""" + if self.proc.poll() is None: + self.proc.terminate() + try: + self.proc.wait(timeout=15) + except subprocess.TimeoutExpired: + self.proc.kill() + self.proc.wait(timeout=15) + if self.proc.stdout is not None: + self.proc.stdout.close() + + def _output(self) -> str: + if self.proc.stdout is None: + return "(no output captured)" + try: + return self.proc.stdout.read().decode(errors="replace")[-2000:] + except Exception: + return "(output unreadable)" + + def is_listening(self) -> bool: + try: + self.call("GET", "/settings") + return True + except Exception: + return False + + # ---------------------------------------------------------------- HTTP + + def call(self, method: str, path: str, payload=None, expect: int | None = None): + data = json.dumps(payload).encode() if payload is not None else None + request = urllib.request.Request( + f"http://127.0.0.1:{self.port}/api{path}", + data=data, + method=method, + headers={"Content-Type": "application/json"}, + ) + try: + with urllib.request.urlopen(request, timeout=60) as response: + body, status = response.read(), response.status + except urllib.error.HTTPError as exc: # a real answer, not a failure + body, status = exc.read(), exc.code + if expect is not None and status != expect: + raise AssertionError(f"{method} {path} -> {status}: {body[:400]!r}") + return json.loads(body) if body and status != 204 else None + + def play(self, adventure_id: int, text: str) -> None: + """Plays one turn through the streaming endpoint, to completion.""" + request = urllib.request.Request( + f"http://127.0.0.1:{self.port}/api/adventures/{adventure_id}/actions", + data=json.dumps({"type": "do", "text": text}).encode(), + method="POST", + headers={"Content-Type": "application/json"}, + ) + with urllib.request.urlopen(request, timeout=120) as response: + response.read() + + # ------------------------------------------------------------- reading + + def transcript(self, adventure_id: int) -> list[str]: + page = self.call("GET", f"/adventures/{adventure_id}", expect=200) + return [a["text"] for a in page["actions"]] + + def gold(self, adventure_id: int) -> int: + state = self.call("GET", f"/adventures/{adventure_id}/world-state", expect=200) + return state["state"]["player"]["gold"] + + def total_rows(self, adventure_id: int) -> int: + """Every row of the whole tree, head or no head. + + `action_count` is scoped to the path being read, so it falls when the + head moves back — which is the feature, not a deletion. The export + carries the entire tree whatever the head is doing, so it is what + "nothing was deleted" has to be measured against. + """ + bundle = self.call("GET", f"/adventures/{adventure_id}/export", expect=200) + return len(bundle["actions"]) + + +@pytest.fixture() +def workspace(): + """A database file, and whichever servers a test starts against it.""" + directory = tempfile.mkdtemp(prefix="m4-restart-") + db_path = os.path.join(directory, "campaign.db") + started: list[Server] = [] + + def start() -> Server: + server = Server(db_path, _free_port()) + started.append(server) + server.wait_until_ready() + return server + + try: + yield start + finally: + # Every child dies even if the test failed part way through, and each + # stop() waits, so a later test cannot inherit a live listener. + for server in started: + server.stop() + + +def _campaign_with_a_save_point(server: Server): + """Three turns, a Save Point on the third, then four more turns. + + Returns everything the second process has to be able to reproduce. + """ + scenario = server.call("POST", "/scenarios", { + "title": "Abbey", + "stat_schema": {"player": {"gold": {"initial": 0, "min": 0, "max": 9999}}}, + }, expect=201) + adventure = server.call("POST", "/adventures", { + "title": "The abbey", "scenario_id": scenario["id"], + }, expect=201) + adventure_id = adventure["id"] + + for n in range(3): + server.play(adventure_id, f"turn {n}") + at_save = { + "transcript": server.transcript(adventure_id), + "gold": server.gold(adventure_id), + } + save_point = server.call("POST", f"/adventures/{adventure_id}/checkpoints", + {"name": "Before entering the abbey"}, expect=201) + + for n in range(4): + server.play(adventure_id, f"later {n}") + at_tip = { + "transcript": server.transcript(adventure_id), + "gold": server.gold(adventure_id), + "rows": server.total_rows(adventure_id), + } + return adventure_id, save_point, at_save, at_tip + + +def test_d11_l03_a_save_point_survives_a_real_process_restart(workspace): + """D11 and L03 together, across a boundary a same-process test cannot cross. + + The first process writes the campaign and exits. The second process is a + different interpreter with an empty session, an empty identity map and no + memory of anything — everything it knows, it reads off the disk. + """ + first = workspace() + adventure_id, save_point, at_save, at_tip = _campaign_with_a_save_point(first) + + assert at_tip["gold"] == at_save["gold"] + 4 * GOLD_PER_TURN + assert len(at_tip["transcript"]) == len(at_save["transcript"]) + 8 + + # --- the boundary ----------------------------------------------------- + first.stop() + assert first.proc.poll() is not None, "the first server did not actually exit" + assert not first.is_listening(), "the first server is still answering" + + second = workspace() + assert second.proc.pid != first.proc.pid + + # --- D11: the Save Point is still there ------------------------------- + listed = second.call("GET", f"/adventures/{adventure_id}/checkpoints", expect=200) + assert [c["id"] for c in listed] == [save_point["id"]] + assert listed[0]["name"] == "Before entering the abbey" + assert (listed[0]["branch_id"], listed[0]["depth"]) == ( + save_point["branch_id"], save_point["depth"] + ) + assert listed[0]["resolved"] is True + # The story came back too, at the position the first process left it. + assert second.transcript(adventure_id) == at_tip["transcript"] + + rows_before_restore = second.total_rows(adventure_id) + assert rows_before_restore == at_tip["rows"] + + # --- L03: restoring reconstructs the historical position -------------- + page = second.call( + "POST", f"/adventures/{adventure_id}/checkpoints/{save_point['id']}/restore", + expect=200, + ) + assert [a["text"] for a in page["actions"]] == at_save["transcript"] + assert second.transcript(adventure_id) == at_save["transcript"] + assert second.gold(adventure_id) == at_save["gold"] + + # --- restore deleted nothing, and Redo is still available ------------- + assert second.total_rows(adventure_id) == rows_before_restore + assert page["can_redo"] is True + assert second.call("GET", f"/adventures/{adventure_id}", expect=200)["can_redo"] is True + + +def test_the_retained_continuation_is_reachable_after_a_restart(workspace): + """The later history is not merely present in the database after a restart — + it is still the continuation the story tells, walkable by ordinary Redo.""" + first = workspace() + adventure_id, save_point, at_save, at_tip = _campaign_with_a_save_point(first) + first.stop() + + second = workspace() + second.call("POST", + f"/adventures/{adventure_id}/checkpoints/{save_point['id']}/restore", + expect=200) + assert second.transcript(adventure_id) == at_save["transcript"] + + steps = 0 + while second.call("GET", f"/adventures/{adventure_id}", expect=200)["can_redo"]: + second.call("POST", f"/adventures/{adventure_id}/redo", expect=200) + steps += 1 + assert steps <= 10, "Redo never stopped" + + assert steps == 4 + assert second.transcript(adventure_id) == at_tip["transcript"] + assert second.gold(adventure_id) == at_tip["gold"] + assert second.total_rows(adventure_id) == at_tip["rows"] + + +def test_a_divergent_write_after_a_restart_keeps_the_displaced_future(workspace): + """D13's second half, across the boundary: the fork still happens on the + write rather than on the restore, and the displaced turns keep their rows.""" + first = workspace() + adventure_id, save_point, at_save, at_tip = _campaign_with_a_save_point(first) + first.stop() + + second = workspace() + second.call("POST", + f"/adventures/{adventure_id}/checkpoints/{save_point['id']}/restore", + expect=200) + branches_before = second.call("GET", f"/adventures/{adventure_id}/branches", + expect=200) + assert len(branches_before) == 1, "restore must not fork" + + second.play(adventure_id, "go around the back") + + branches_after = second.call("GET", f"/adventures/{adventure_id}/branches", + expect=200) + assert len(branches_after) == 2, "the write should have forked" + # Nothing was deleted to achieve it: the tree grew by the new turn only. + assert second.total_rows(adventure_id) == at_tip["rows"] + 2 + # Ordinary Redo no longer offers the displaced future. + assert second.call("GET", f"/adventures/{adventure_id}", expect=200)["can_redo"] is False + # And the Save Point still names the position it always did. + listed = second.call("GET", f"/adventures/{adventure_id}/checkpoints", expect=200) + assert (listed[0]["branch_id"], listed[0]["depth"]) == ( + save_point["branch_id"], save_point["depth"] + ) diff --git a/backend/tests/test_save_points.py b/backend/tests/test_save_points.py index 0ba190a..ed9d617 100644 --- a/backend/tests/test_save_points.py +++ b/backend/tests/test_save_points.py @@ -748,36 +748,169 @@ def test_a_save_point_whose_turn_is_gone_refuses_rather_than_approximating(clien assert _head(client.adv_id) == head_before -def test_deleting_a_branch_takes_its_save_points_with_it(client): - """Referential integrity, not cleanup. Nothing removes a Save Point for - going stale; this one goes because the story it named went.""" +def test_a_branch_a_save_point_names_cannot_be_deleted(client): + """`STORY-BRANCH-SEMANTICS.md` §19: a named Save Point remains until it is + explicitly deleted — and §28 says even a future cleanup feature must retain + paths a checkpoint references. + + So the branch delete is refused rather than taking the Save Point with it. + The alternative, a silent cascade, breaks §19 in the way that matters least + visibly: the story is the thing the user asked to delete, and the named + moments would go without ever being mentioned. + """ _turns(client, 2) _undo(client) _undo(client) ScriptedProvider.replies = ["A new road.\n```state\n{\"player.gold\": 1}\n```"] _play(client, "the other way") forked = _save(client, "On the new line") + doomed_branch = forked["branch_id"] - # Read somewhere the doomed branch is not load-bearing, then delete it. + root_id = _root_branch(client.adv_id) + assert doomed_branch != root_id + client.post(f"/api/adventures/{client.adv_id}/branches/{root_id}/switch") + + r = client.delete(f"/api/adventures/{client.adv_id}/branches/{doomed_branch}") + assert r.status_code == 409, r.text + detail = r.json()["detail"] + # The message names the Save Point, so the user does not have to hunt. + assert "On the new line" in detail + assert "does not delete any story" in detail + + # Nothing happened: the Save Point, the branch and the story all remain. + assert [c["id"] for c in _list(client)] == [forked["id"]] + assert _branch_count(client.adv_id) == 2 + assert any(a.branch_id == doomed_branch for a in _rows(client.adv_id)) + + +def test_deleting_the_save_point_then_lets_the_branch_go(client): + """The refusal has to be recoverable, or it is just a wall. Deleting the + Save Point deletes no story (§25), so the cost of the recovery is a click.""" + _turns(client, 2) + _undo(client) + _undo(client) + ScriptedProvider.replies = ["A new road.\n```state\n{\"player.gold\": 1}\n```"] + _play(client, "the other way") + forked = _save(client, "On the new line") + doomed_branch = forked["branch_id"] + root_id = _root_branch(client.adv_id) + client.post(f"/api/adventures/{client.adv_id}/branches/{root_id}/switch") + + assert client.delete( + f"/api/adventures/{client.adv_id}/branches/{doomed_branch}" + ).status_code == 409 + rows_with_story = len(_rows(client.adv_id)) + + # Delete the Save Point explicitly... + assert client.delete( + f"/api/adventures/{client.adv_id}/checkpoints/{forked['id']}" + ).status_code == 204 + # ...which took no story with it... + assert len(_rows(client.adv_id)) == rows_with_story + + # ...and now the branch can go. + r = client.delete(f"/api/adventures/{client.adv_id}/branches/{doomed_branch}") + assert r.status_code in (200, 204), r.text + assert _branch_count(client.adv_id) == 1 + + +def test_a_branch_no_save_point_names_still_deletes(client): + """The guard must not turn into a general refusal to delete branches.""" + _turns(client, 2) + _undo(client) + _undo(client) + ScriptedProvider.replies = ["A new road.\n```state\n{\"player.gold\": 1}\n```"] + _play(client, "the other way") + forked_branch = _head(client.adv_id)[0] + root_id = _root_branch(client.adv_id) + client.post(f"/api/adventures/{client.adv_id}/branches/{root_id}/switch") + + assert _list(client) == [] + r = client.delete(f"/api/adventures/{client.adv_id}/branches/{forked_branch}") + assert r.status_code in (200, 204), r.text + assert _branch_count(client.adv_id) == 1 + + +def test_a_save_point_on_a_descendant_also_protects_the_branch(client): + """Deleting a branch takes everything forked from it, so the check has to + cover the subtree. A guard that looked only at the named branch would let a + Save Point on a child be deleted without a word — the exact failure the + guard exists to prevent, one level down.""" + _turns(client, 4) + _undo(client) + _undo(client) + ScriptedProvider.replies = ["Second line.\n```state\n{\"player.gold\": 1}\n```"] + _play(client, "second line") + middle_branch = _head(client.adv_id)[0] + _undo(client) + ScriptedProvider.replies = ["Third line.\n```state\n{\"player.gold\": 1}\n```"] + _play(client, "third line") + deepest = _save(client, "Down on the deepest line") + assert deepest["branch_id"] != middle_branch + + root_id = _root_branch(client.adv_id) + client.post(f"/api/adventures/{client.adv_id}/branches/{root_id}/switch") + + # Deleting the *middle* branch would take the deepest one with it. + r = client.delete(f"/api/adventures/{client.adv_id}/branches/{middle_branch}") + assert r.status_code == 409, r.text + assert "Down on the deepest line" in r.json()["detail"] + assert [c["id"] for c in _list(client)] == [deepest["id"]] + + +def test_a_save_point_elsewhere_does_not_block_an_unrelated_branch(client): + """The guard is scoped to the subtree being deleted, not to the campaign.""" + _turns(client, 3) + elsewhere = _save(client, "Safe on the root") + _undo(client) + _undo(client) + ScriptedProvider.replies = ["A new road.\n```state\n{\"player.gold\": 1}\n```"] + _play(client, "the other way") + forked_branch = _head(client.adv_id)[0] + root_id = _root_branch(client.adv_id) + client.post(f"/api/adventures/{client.adv_id}/branches/{root_id}/switch") + + assert elsewhere["branch_id"] == root_id + r = client.delete(f"/api/adventures/{client.adv_id}/branches/{forked_branch}") + assert r.status_code in (200, 204), r.text + # The unrelated Save Point is untouched and still restores. + assert [c["id"] for c in _list(client)] == [elsewhere["id"]] + assert _restore(client, elsewhere["id"]).status_code == 200 + + +def test_the_refusal_names_several_save_points_without_running_on(client): + """A long list is truncated so the message stays a sentence someone reads.""" + _turns(client, 2) + _undo(client) + _undo(client) + ScriptedProvider.replies = ["A new road.\n```state\n{\"player.gold\": 1}\n```"] + _play(client, "the other way") + for n in range(5): + _save(client, f"Point {n}") + doomed_branch = _head(client.adv_id)[0] + root_id = _root_branch(client.adv_id) + client.post(f"/api/adventures/{client.adv_id}/branches/{root_id}/switch") + + detail = client.delete( + f"/api/adventures/{client.adv_id}/branches/{doomed_branch}" + ).json()["detail"] + assert "Point 0" in detail and "2 more" in detail + assert "Point 4" not in detail + assert len(_list(client)) == 5 + + +def _root_branch(adv_id) -> int: db = SessionLocal() try: - adv = db.get(models.Adventure, client.adv_id) - root = ( + return ( db.query(models.Branch) - .filter_by(adventure_id=client.adv_id, parent_branch_id=None) + .filter_by(adventure_id=adv_id, parent_branch_id=None) .one() + .id ) - doomed_id = forked["branch_id"] - assert doomed_id != root.id finally: db.close() - client.post(f"/api/adventures/{client.adv_id}/branches/{root.id}/switch") - r = client.delete(f"/api/adventures/{client.adv_id}/branches/{doomed_id}") - assert r.status_code in (200, 204), r.text - - assert [c["id"] for c in _list(client)] == [] - # --------------------------------------------- E-series: lineage and memory @@ -1129,3 +1262,328 @@ def test_an_m3_database_gains_the_save_point_table_and_keeps_its_story(): migrations.bootstrap(m3) assert "checkpoints" in inspect(m3).get_table_names() m3.dispose() + + +# ------------------------------------------- the cost of listing Save Points + +def _sql_during(work): + """Returns every SQL statement a block of work executed.""" + from sqlalchemy import event + seen: list[str] = [] + + def record(conn, cursor, statement, params, context, executemany): + seen.append(statement) + + event.listen(engine, "before_cursor_execute", record) + try: + work() + finally: + event.remove(engine, "before_cursor_execute", record) + return seen + + +def test_listing_save_points_costs_a_bounded_number_of_queries(client): + """M4 review §R B-1. The list resolved each Save Point on its own, so the + query count grew with the list: 53 SELECTs for 25 Save Points, against 4 for + the comparable branch panel. + + The assertion is on *growth*, not on an exact number, because a fixed budget + would be a number to edit rather than a rule to keep. Five times the Save + Points must not mean five times the queries. + """ + _turns(client, 5) + for n in range(5): + _save(client, f"Save Point {n}") + few = _sql_during(lambda: _list(client)) + + _turns(client, 20) + for n in range(20): + _save(client, f"Later Save Point {n}") + assert len(_list(client)) == 25 + many = _sql_during(lambda: _list(client)) + + # Five times the rows, and the query count does not move at all. + assert len(many) == len(few), ( + f"listing 25 Save Points cost {len(many)} queries where 5 cost {len(few)}" + ) + # And the whole thing is a handful, not a per-row walk. + assert len(many) <= 6, f"{len(many)} queries to list 25 Save Points" + + +def test_listing_save_points_does_not_read_story_prose(client): + """The other half of B-1. Resolving a coordinate asks whether a row exists; + it never needs the narration in it, and `paging.py` states the rule this + follows — a bulk read names the columns it needs. + + Enforced on the emitted SQL rather than on a byte count, because the failure + this guards against is a `SELECT` widening back to the whole entity, which a + small fixture would not make visible in bytes. + """ + _turns(client, 3) + for n in range(3): + _save(client, f"Save Point {n}") + + statements = _sql_during(lambda: _list(client)) + action_reads = [q for q in statements if "FROM actions" in q] + assert action_reads, "the list must still check that coordinates resolve" + for query in action_reads: + selected = query.split("FROM actions")[0] + for column in ("actions.text", "actions.reasoning", "actions.world_delta"): + assert column not in selected, f"{column} fetched to render the list:\n{query}" + + +def test_a_save_point_on_a_deleted_turn_is_still_reported_unresolved(client): + """The bulk resolution must not have quietly changed what `resolved` means. + + This is the negative control for the B-1 rewrite: one query for many + coordinates is only correct if a coordinate with no live row still comes + back false. + """ + _turns(client, 3) + alive = _save(client, "Still here") + _turns(client, 1) + doomed = _save(client, "About to vanish") + + db = SessionLocal() + try: + for row in db.query(models.Action).filter_by( + adventure_id=client.adv_id, + branch_id=doomed["branch_id"], + depth=doomed["depth"], + ): + db.delete(row) + db.commit() + finally: + db.close() + + by_id = {c["id"]: c for c in _list(client)} + assert by_id[doomed["id"]]["resolved"] is False + assert by_id[alive["id"]]["resolved"] is True + # One resolving and one not, in the same single query. + + +def test_the_bulk_resolution_does_not_confuse_coordinates_across_branches(client): + """A coordinate is a pair, and the bulk query must match it as a pair. + + Matching `branch IN (...) AND depth IN (...)` would take the cross product, + and a Save Point at a depth that exists on *another* branch would be + reported as resolved. This builds exactly that trap: two branches, and a + Save Point whose own coordinate is dead while the other branch has a live + row at the same depth. + """ + _turns(client, 4) + _undo(client) + _undo(client) + ScriptedProvider.replies = ["A new road.\n```state\n{\"player.gold\": 1}\n```"] + _play(client, "the other way") # forks; new branch has rows at 5,6 + on_new_line = _save(client, "On the new line") + + # A Save Point on the old line at the same depth, whose row we then remove. + db = SessionLocal() + try: + adventure = db.get(models.Adventure, client.adv_id) + old_branch = ( + db.query(models.Branch) + .filter_by(adventure_id=client.adv_id, parent_branch_id=None) + .one() + ) + stranded = models.Checkpoint( + adventure_id=client.adv_id, + name="Stranded on the old line", + branch_id=old_branch.id, + depth=on_new_line["depth"], + ) + db.add(stranded) + db.flush() + stranded_id = stranded.id + # Remove the old line's row at that depth, so this coordinate is dead + # while the *other* branch still has a live row at the same depth. + for row in db.query(models.Action).filter_by( + adventure_id=client.adv_id, + branch_id=old_branch.id, + depth=on_new_line["depth"], + ): + db.delete(row) + db.commit() + finally: + db.close() + + by_id = {c["id"]: c for c in _list(client)} + assert by_id[on_new_line["id"]]["resolved"] is True + assert by_id[stranded_id]["resolved"] is False, ( + "a dead coordinate was reported resolved because another branch has a " + "live row at the same depth" + ) + + +# --------------------------------- deleting a branch, and saying so first + +def test_the_branch_list_reports_how_many_save_points_each_line_carries(client): + """The number the delete warning is built from (M4 review §R B-2). + + Served as part of the branch list rather than from an endpoint of its own, + and as one grouped query rather than one per branch — the panel already + reads this list to draw itself. + """ + _turns(client, 2) + _undo(client) + _undo(client) + ScriptedProvider.replies = ["A new road.\n```state\n{\"player.gold\": 1}\n```"] + _play(client, "the other way") + on_new = _save(client, "On the new line") + + branches = client.get(f"/api/adventures/{client.adv_id}/branches").json() + by_id = {b["id"]: b for b in branches} + assert len(branches) == 2 + assert by_id[on_new["branch_id"]]["save_points"] == 1 + other = next(b for b in branches if b["id"] != on_new["branch_id"]) + assert other["save_points"] == 0 + + _save(client, "A second one here") + branches = client.get(f"/api/adventures/{client.adv_id}/branches").json() + assert {b["id"]: b["save_points"] for b in branches}[on_new["branch_id"]] == 2 + + +def test_the_save_point_count_matches_what_blocks_the_deletion(client): + """The number the panel disables its Delete button on has to be the same + number the server refuses on, or the UI and the rule disagree.""" + _turns(client, 4) + _undo(client) + _undo(client) + ScriptedProvider.replies = ["Second line.\n```state\n{\"player.gold\": 1}\n```"] + _play(client, "second line") + middle_branch = _head(client.adv_id)[0] + _undo(client) + ScriptedProvider.replies = ["Third line.\n```state\n{\"player.gold\": 1}\n```"] + _play(client, "third line") + _save(client, "Deep one") + + branches = client.get(f"/api/adventures/{client.adv_id}/branches").json() + counts = {b["id"]: b["save_points"] for b in branches} + # The count is per branch; the client sums it over the subtree, and the + # server refuses on the same subtree. + assert sum(counts.values()) == 1 + assert counts[middle_branch] == 0 + + root_id = _root_branch(client.adv_id) + client.post(f"/api/adventures/{client.adv_id}/branches/{root_id}/switch") + assert client.delete( + f"/api/adventures/{client.adv_id}/branches/{middle_branch}" + ).status_code == 409 + + +def test_both_branch_delete_surfaces_explain_the_save_point_rule(): + """The rule must be visible in every view the deletion is reachable from. + + A source-level assertion, because the project has no frontend test runner + (M8). `test_offline_assets.py` reads the frontend the same way, for the same + reason: the check is worth having now, and it is honest about what it is — + it proves the wiring is in the build, not that a user saw it. The browser + smoke test performed at closeout is what proves the rendering. + + Two files, because the branch list and the tree overlay each render their + own delete control, and a rule that held in one of them would not be a rule. + """ + from pathlib import Path + + repo = Path(__file__).resolve().parents[2] + views = { + "the branch panel": + repo / "frontend/src/pages/Play/panels/BranchPanel.jsx", + "the tree overlay": + repo / "frontend/src/BranchMap.jsx", + } + for where, path in views.items(): + source = path.read_text(encoding="utf-8") + assert "savePointsUnder" in source, ( + f"{where} does not count the Save Points that protect a branch" + ) + # The Delete control is disabled while Save Points protect the subtree, + # and says why rather than failing silently on the server. + assert "protecting > 0" in source, ( + f"{where} does not disable Delete while Save Points protect the branch" + ) + assert "deleting a Save Point deletes no story" in source, ( + f"{where} does not tell the user how to proceed" + ) + # The user-facing copy must not explain itself in schema terms. + for jargon in ("cascade", "foreign key", "foreign-key", "ON DELETE"): + assert jargon.lower() not in source.lower(), ( + f"{where} uses implementation jargon in user-facing copy: {jargon}" + ) + + +def test_creating_a_save_point_takes_the_campaigns_turn_lock(client): + """M4 review §S C-5. "Save where I am" has to name one committed position, + and the head is exactly what a turn in flight is about to move. + + Asserted by holding the lock and watching Create refuse, which is the same + contract Undo, Redo and Restore already answer with a 409. That proves it + participates in the serialization rather than merely being fast. + """ + _turns(client, 2) + adventures.turns.acquire_turn_lock(client.adv_id) + try: + r = client.post(f"/api/adventures/{client.adv_id}/checkpoints", + json={"name": "During a turn"}) + assert r.status_code == 409, r.text + # The same refusal the other position-moving operations give. + assert _undo(client).status_code == 409 + assert _restore(client, 1).status_code in (404, 409) + finally: + adventures.turns._active_turns.discard(client.adv_id) + + # Nothing was written while the lock was held... + assert _list(client) == [] + # ...and the endpoint works again once the turn is done. + assert _save(client, "After the turn")["name"] == "After the turn" + + +def test_creating_a_save_point_releases_the_lock_even_when_it_refuses(client): + """A refused create must not leave the campaign wedged. + + The empty-story refusal is raised from inside the locked section, so this is + the case that would strand the lock if the release were not in a `finally`. + """ + db = SessionLocal() + try: + adv = db.get(models.Adventure, client.adv_id) + adv.head_depth = lineage.NO_DEPTH + db.commit() + finally: + db.close() + + r = client.post(f"/api/adventures/{client.adv_id}/checkpoints", + json={"name": "Nowhere"}) + assert r.status_code == 400 + + # The lock is free: an ordinary turn still works. + assert client.adv_id not in adventures.turns._active_turns + _play(client, "carry on") + + +def test_rename_and_delete_do_not_need_the_turn_lock(client): + """Deliberate, and worth pinning down: neither reads nor moves a story + position, so neither can race a turn in flight. Renaming a Save Point while + a turn generates is a label edit, and refusing it would be a worse product + for no safety gained.""" + _turns(client, 2) + made = _save(client, "One") + + adventures.turns.acquire_turn_lock(client.adv_id) + try: + renamed = client.patch( + f"/api/adventures/{client.adv_id}/checkpoints/{made['id']}", + json={"name": "Renamed mid-turn"}, + ) + assert renamed.status_code == 200, renamed.text + assert renamed.json()["name"] == "Renamed mid-turn" + # And the coordinate did not move while a turn was in flight. + assert (renamed.json()["branch_id"], renamed.json()["depth"]) == ( + made["branch_id"], made["depth"] + ) + assert client.delete( + f"/api/adventures/{client.adv_id}/checkpoints/{made['id']}" + ).status_code == 204 + finally: + adventures.turns._active_turns.discard(client.adv_id) diff --git a/frontend/src/BranchMap.jsx b/frontend/src/BranchMap.jsx index 2dc4296..b2d5ca6 100644 --- a/frontend/src/BranchMap.jsx +++ b/frontend/src/BranchMap.jsx @@ -1,6 +1,6 @@ import { useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react' import { createPortal } from 'react-dom' -import { CORNER, PAD, ROW_H, branchLabel, headLineage, layoutTree, momentTicks } from './branches' +import { CORNER, PAD, ROW_H, branchLabel, headLineage, layoutTree, momentTicks, savePointsUnder } from './branches' // The story tree, drawn. // @@ -108,6 +108,9 @@ export function BranchMap({ branches, busyId, onSwitch, onRename, onDelete, onCl // would throw away the only copy of it. const save = async () => { if (await onRename(selected, renameText)) setRenameText(null) } const drop = async () => { if (await onDelete(selected)) setConfirming(false) } + // Save Points naming a moment on the selected branch, or on anything forked + // from it, keep it alive — the server refuses to delete them along with it. + const protecting = selected ? savePointsUnder(branches, selected.id) : 0 return createPortal(
@@ -239,7 +242,13 @@ export function BranchMap({ branches, busyId, onSwitch, onRename, onDelete, onCl {confirming ? (
- Delete this branch and everything forked from it? + {/* The same wording the list gives, because the same deletion + is reachable from both views and a rule that held in one of + them would not be a rule. */} + + Delete this branch and everything forked from it? The story on + other paths, and every Save Point, is unaffected. + @@ -265,10 +274,14 @@ export function BranchMap({ branches, busyId, onSwitch, onRename, onDelete, onCl the head stands on is offered, and says why it cannot go. */} {!isRoot && ( )}
diff --git a/frontend/src/branches.js b/frontend/src/branches.js index 5cfa04e..113e676 100644 --- a/frontend/src/branches.js +++ b/frontend/src/branches.js @@ -61,6 +61,41 @@ export function headLineage(branches) { return out } +// How many Save Points would go if this branch were deleted. +// +// Deleting a branch deletes everything forked from it, and a Save Point names a +// position on a line, so the Save Points on the whole doomed subtree go too. +// The count is summed over that subtree rather than over the one branch — a +// warning that said "1 Save Point" while three disappeared would be worse than +// no warning at all. +// +// Computed here because the panel already holds every branch and its parent, +// so the answer costs a walk rather than an endpoint. The server remains the +// authority on what is actually deleted; this only lets the button say so +// before it is pressed, the same division `headLineage` already uses. +export function savePointsUnder(branches, rootId) { + const children = new Map() + for (const b of branches) { + const key = b.parent_branch_id + if (!children.has(key)) children.set(key, []) + children.get(key).push(b) + } + const byId = new Map(branches.map((b) => [b.id, b])) + const seen = new Set() + const stack = [rootId] + let total = 0 + while (stack.length) { + const id = stack.pop() + // The guard is against a cycle, which the schema forbids and a walk should + // still never hang on. + if (seen.has(id)) continue + seen.add(id) + total += byId.get(id)?.save_points ?? 0 + for (const child of children.get(id) ?? []) stack.push(child.id) + } + return total +} + // ---------- Map geometry ---------- export const ROW_H = 64 // one branch, name above the lane and meta below diff --git a/frontend/src/pages/Play/panels/BranchPanel.jsx b/frontend/src/pages/Play/panels/BranchPanel.jsx index d7038d5..2d75cc1 100644 --- a/frontend/src/pages/Play/panels/BranchPanel.jsx +++ b/frontend/src/pages/Play/panels/BranchPanel.jsx @@ -11,7 +11,7 @@ import { useEffect, useState } from 'react' import { api } from '../../../api' import { BranchMap } from '../../../BranchMap' -import { branchLabel, headLineage, orderBranches } from '../../../branches' +import { branchLabel, headLineage, orderBranches, savePointsUnder } from '../../../branches' function BranchPanel({ advId, refreshKey, onSwitched, onTreeChanged, onError }) { const [branches, setBranches] = useState(null) @@ -88,6 +88,12 @@ function BranchPanel({ advId, refreshKey, onSwitched, onTreeChanged, onError }) // was forked from. The button said nothing about that and answered // with a toast; it now says so before it is pressed. const loadBearing = lineage.has(branch.id) + // A Save Point naming a moment on this branch, or on anything forked + // from it, keeps the branch alive: named moments last until the user + // deletes them, so the server refuses the deletion rather than taking + // them with it. Counted over the subtree, because deleting a branch + // takes its descendants. + const protecting = savePointsUnder(branches, branch.id) return (
@@ -117,11 +123,20 @@ function BranchPanel({ advId, refreshKey, onSwitched, onTreeChanged, onError }) {branch.own_actions} of its own {branch.parent_branch_id !== null && ` · forked at moment ${branch.fork_depth + 1}`} {` · ends at ${branch.depth + 1}`} + {protecting > 0 && + ` · ${protecting === 1 ? '1 Save Point' : `${protecting} Save Points`} kept here`}
{isConfirming ? (
- Delete this branch and everything forked from it? + {/* Only reachable when no Save Point points into this + subtree — the button is disabled otherwise, and the server + refuses regardless. So this says what the deletion costs + without hedging about Save Points that cannot be at risk. */} + + Delete this branch and everything forked from it? The story + on other paths, and every Save Point, is unaffected. + @@ -145,10 +160,15 @@ function BranchPanel({ advId, refreshKey, onSwitched, onTreeChanged, onError }) {/* The root holds the turns every other branch borrows, and the server refuses it — so it is not offered. */} {branch.parent_branch_id !== null && ( - )}
diff --git a/planning/BROWSER-UX-SPEC.md b/planning/BROWSER-UX-SPEC.md index bd33a03..bc13667 100644 --- a/planning/BROWSER-UX-SPEC.md +++ b/planning/BROWSER-UX-SPEC.md @@ -358,10 +358,16 @@ Each entry: ```text Before entering the abbey -Turn 42 +Moment 42 [Restore] [Rename] [Delete] ``` +**Moment, not Turn.** M4 closeout ruled in favour of the implemented product: +the branch panel and the tree overlay already count in moments (`forked at +moment 9`, `ends at moment 40`), so a Save Point list saying "Turn 42" would +make one screen use two words for one thing. This is a vocabulary alignment and +changes no behaviour; the number is unchanged. + ## 26. Restore Confirmation Restoring is non-destructive. diff --git a/planning/BUILD-MILESTONES.md b/planning/BUILD-MILESTONES.md index 8ed9ed7..51d0d48 100644 --- a/planning/BUILD-MILESTONES.md +++ b/planning/BUILD-MILESTONES.md @@ -1,6 +1,6 @@ # Adventure Storyteller — Production Build Milestones -**Status:** In implementation. M1, M2 and M3 complete and accepted (M1 and M2: 2026-09-02; M3: 2026-09-03); M4 — Named Save Points / Checkpoints — implemented 2026-09-03, awaiting review +**Status:** In implementation. M1-M4 complete and accepted (M1 and M2: 2026-09-02; M3 and M4: 2026-09-03); M5 — Genre-Neutral Authoritative Narrative State — next to brief **Base:** AI-DnD `d72f7c1bda0f34fccd84afb7a25c34eb01c901de` ## 1. Purpose @@ -274,14 +274,18 @@ review. The architecture is recorded in **ADR 012**. a turn says while story descends from it off screen — both ratified in `STORY-BRANCH-SEMANTICS.md` (§5, §10, §14A). -**Outstanding closeout condition:** the required **browser smoke test has not -been performed** — no session in which M3 was implemented or reviewed had a -browser available. The equivalent sequence was driven end-to-end against the -running application with real inference and a process restart, and every -server-side behaviour it covers passes; the DOM-level behaviour of the Redo -button, its disabled states and its keyboard shortcut remain unverified by -observation. This does not block M4, which touches none of that wiring, but it -remains an open M3 item until a human runs it. +**Closeout condition — CLOSED at M4 closeout (2026-09-03).** M3 was accepted with +one condition outstanding: the required **browser smoke test had not been +performed**, because no session in which M3 was implemented or reviewed had a +browser available, leaving the DOM-level behaviour of the Redo button and its +disabled states unverified by observation. + +That condition is now discharged. A real Firefox 154.0.1, driven through +geckodriver, exercised M3's controls in the rendered application: Undo enabled +and Redo disabled at the tip, two Undos moving the transcript back, Redo becoming +enabled and returning the original tip exactly, Retry and the take pager, and a +divergent write retiring Redo with no stale old-future text on screen. It passed. +Evidence: `planning/reports/M4-IMPLEMENTATION-REPORT.md` §W.7. **Debt carried forward, none of it blocking M4:** full narrator-edit state re-evaluation is deferred to M5 (`STORY-BRANCH-SEMANTICS.md` §14A records the @@ -351,12 +355,11 @@ M3's cost was reconciling them; a parallel checkpoint mover would recreate that divergence in a place where the two paths would silently disagree about what "restore" means. See ADR 012. -## Status: IMPLEMENTED — awaiting review +## Status: COMPLETE -Implementation landed 2026-09-03. **Not accepted**: the milestone report has not -been written and no reviewer has read the change. The Definition of Done above is -met by the code and the tests below; whether it is met by the *product* is what -the review is for. +Accepted 2026-09-03. Evidence: `planning/reports/M4-IMPLEMENTATION-REPORT.md`, +including its §W closeout addendum. The Definition of Done is met, and — for the +first time in this project — **verified in a real browser**. **What M4 delivered:** @@ -395,17 +398,77 @@ it. E-series lineage and memory isolation after restore and divergence, the edge cases in the brief, and the M3-database migration. -**Outstanding condition, carried from M3 and not resolved here:** the **browser -smoke test has still not been performed**, for M3 or for M4. No session has had a -usable browser. The M4 sequence was driven end-to-end over HTTP against a live -server with a real process restart, and every server-side behaviour it covers -passes; the DOM-level behaviour of the Save Point panel, its buttons and its -confirmations remains unverified by observation. +**Facts M5 inherits, and must not redesign:** -**Debt M4 carries forward:** none newly discovered in the head model. The Save -Point panel has no frontend test, because the project still has no frontend test -runner at all (M8). `POST /adventures/import` still returns every branch's rows -rather than a head-capped window (inherited, M3). +- a Save Point is a **durable story coordinate** — `(branch, depth)` — carrying + no copy of transcript, state, prompt, memory or summary; +- **restore is M3 head movement**, through the same `head.move_to` Undo and Redo + use; there is no second restore path and M5 must not add one; +- **state recovery stays snapshot/cached-position based**, never a replay of the + campaign (`TECHNICAL-DESIGN.md` §10.4). This is now load-bearing for Save + Points as well as Undo/Redo; +- **the first divergent write** after a restore creates the continuation; + restore itself never forks; +- **history a Save Point names cannot disappear** through an unrelated deletion + (§19.1); +- **M3/M4 history and Save Point semantics are infrastructure now.** M5 replaces + the state *model*; it does not revisit how the story is positioned. + +**Acceptance evidence:** D11-D14, I04 and L03 all pass. D11 and L03 are +discharged by automation that crosses a **genuine OS process boundary** — one +server process writes the campaign, is killed, and a second process reads it back +— rather than by recreating a client in one process. + +**The browser condition is closed, for M4 and retrospectively for M3.** A real +Firefox 154.0.1, driven through geckodriver over the W3C WebDriver protocol, +exercised the rendered DOM end to end: **44/44 checks passed**, covering M3's +Undo/Redo enable states and transcript movement, Retry and the take pager, M3 +divergence and the disappearance of Redo, and every M4 Save Point operation +including both confirmations and the branch-delete warning. No console errors. +This closes the outstanding M3 condition recorded above and the equivalent M4 +one. + +**The three review findings were fixed during closeout:** + +- **B-1** — the Save Point list was an N+1 that loaded whole `Action` rows, + narration included. It is now one bulk two-column coordinate query plus one + lineage: **53 SELECTs for 25 Save Points became 5**, and the query count no + longer moves with the length of the list. +- **B-2** — deleting a branch silently deleted the Save Points naming it. Fixed + as a **behaviour** defect, not a wording one: a branch a Save Point names can + no longer be deleted at all. The request is refused with the offending Save + Points named, the user deletes them explicitly (which deletes no story), and + the branch then goes. Both delete controls, in the branch list and in the tree + overlay, disable and explain rather than warning about a loss that no longer + happens. `STORY-BRANCH-SEMANTICS.md` **§19.1** records the rule; §28 already + required a future cleanup feature to retain checkpoint-referenced paths, and + this is that requirement applied to the deletion path that exists today. +- **B-3** — the D11/L03 automation now spawns real server processes. + +**Also fixed:** creating a Save Point takes the campaign's turn lock (review §S +C-5), so "save where I am" cannot read a head that a turn in flight is about to +move. Rename and Delete deliberately do not take it — neither reads nor moves a +story position. + +**Debt M4 carries forward:** the Save Point panel has no frontend test, because +the project still has no frontend test runner at all (M8) — the browser smoke +test above is a closeout procedure, not a suite. `POST /adventures/import` still +returns every branch's rows rather than a head-capped window (inherited, M3). + +## Note to M5 — the instrumentation is now larger than M3 estimated + +M4 added **60 tests that use inherited RPG world-state values as deterministic +instrumentation**, on top of the ~20 M3 flagged. M5 replaces that state model, +and must **move the instrumentation while preserving the behavioural +assertions**: what those tests measure is where the story is being read and what +state belongs to that position, which is exactly as true after M5 as before it. +Deleting them would delete the evidence for D11-D14, I04, L03 and the E-series. + +The existing constraint stands and is now load-bearing for Save Points as well as +Undo/Redo: **historical state must remain efficiently snapshot/cache +recoverable**, so that moving to a position never becomes proportional to +campaign length (`TECHNICAL-DESIGN.md` §10.4). Restore, Undo and Redo all pay +whatever that costs. --- diff --git a/planning/DATA-MODEL.md b/planning/DATA-MODEL.md index 436f0fb..b286cf0 100644 --- a/planning/DATA-MODEL.md +++ b/planning/DATA-MODEL.md @@ -230,6 +230,30 @@ It carries **no** copy of the transcript, the state, the prompt, a memory, a summary, or a branch's contents. Everything a restore produces comes from the retained history the coordinate points into. +The retry case is what settles the coordinate-versus-turn-id question, and M4 +closeout measured it rather than arguing it. A Save Point named a turn whose +live row was id 17; retrying that turn made id 17 dead and id 18 live at the +same coordinate; the Save Point resolved to id 18 and restored correctly. A row +id would have pinned a take the story no longer tells. **A Save Point names a +story position, not a particular take of it.** + +Three further properties of the implemented model, recorded so they are decided +rather than incidental: + +- **Names are not unique**, and nothing requires them to be. No product + requirement asks for uniqueness, and two names for one moment is a reasonable + thing for a player to want. +- **Several Save Points may name the same position.** Same reason. +- **Ordinary list presentation is newest-created first.** Story order is not + something the list can honestly claim: depths on lines that have parted + company are not comparable, so ordering by depth would draw a sequence that no + reading of the story passes through. When each was made is a fact about all of + them. + +None of this is genre-specific. A coordinate is a position in a story; what the +state at that position *contains* is M5's question, and changing it does not +change what a Save Point is. + Two consequences worth recording here: - **Restore reuses the campaign's one head-movement mechanism.** It resolves the @@ -238,11 +262,13 @@ Two consequences worth recording here: the path being read, which is what makes a Save Point on a departed line restorable at all — and what keeps a Save Point in a shared prefix from dragging the reader off the line they chose. See `TECHNICAL-DESIGN.md` §8.8. -- **A checkpoint is durable against everything but its own deletion and its - branch's.** No pass removes one for going stale, sitting behind the head, or - naming a line the story left (`STORY-BRANCH-SEMANTICS.md` §19). Deleting a - branch removes its checkpoints by cascade, as it removes its memories, because - the story they named is gone. +- **A checkpoint is durable against everything but its own explicit deletion.** + No pass removes one for going stale, sitting behind the head, or naming a line + the story left (`STORY-BRANCH-SEMANTICS.md` §19). Deleting a *branch* does not + remove one either: the deletion is refused while a checkpoint names any + position in the subtree, and the user deletes the checkpoint first + (§19.1). Deleting the whole campaign removes them, which is what deleting a + campaign means. ## 9. Narrative Entity diff --git a/planning/PROJECT-SOURCES.md b/planning/PROJECT-SOURCES.md index 2ab3a9b..3a8b305 100644 --- a/planning/PROJECT-SOURCES.md +++ b/planning/PROJECT-SOURCES.md @@ -85,13 +85,13 @@ One file, and it changes as development progresses: planning/reports/M4-IMPLEMENTATION-REPORT.md ``` -M3 is the most recently completed milestone, and M4 is the next to be briefed. -This report is M3's review *and* its primary evidence record — no separate M3 -baseline report was produced — so it is the only place some of what M3 left -behind is written down, including the browser smoke test M3 still owes. +M4 is the most recently completed milestone, and M5 is the next to be briefed. +This report is M4's review *and* its closeout record: its §W holds the corrective +work, the process-boundary automation, and the real-browser verification that +closed the condition M3 and M4 both carried. -**Replace it, do not accumulate.** When M4's report lands, remove this one from -the project Sources and upload M4's instead. The repository does the same thing: +**Replace it, do not accumulate.** When M5's report lands, remove this one from +the project Sources and upload M5's instead. The repository does the same thing: `planning/reports/` holds the current milestone's report and `planning/archive/milestone-reports/` holds the rest. diff --git a/planning/README.md b/planning/README.md index 7d53e45..83c0e4c 100644 --- a/planning/README.md +++ b/planning/README.md @@ -3,10 +3,10 @@ **This file is the index. Start here.** **Current state:** Phase 0 complete; AI-DnD forked as the production base; -milestones **M1, M2 and M3 implemented and accepted** (M3: 2026-09-03). -**M4 — named Save Points — is implemented (2026-09-03) and awaiting review.** -Its implementation report has not been written, and writing it is the current -action. Do not begin M5. +milestones **M1, M2, M3 and M4 implemented and accepted** (M3 and M4: +2026-09-03). +**Next:** **M5 — Genre-Neutral Authoritative Narrative State.** Its brief has not +been written yet, and writing it is the current action. **Package version:** see `VERSION.md`, which records what each revision changed and why. @@ -234,9 +234,9 @@ Milestone M3 COMPLETE (2026-09-03) active-head export and ADR 012 | v -Milestone M4 IMPLEMENTED 2026-09-03 — - named Save Points awaiting review; no report yet - | +Milestone M4 COMPLETE (2026-09-03) + named Save Points reports/M4-IMPLEMENTATION-REPORT.md + | browser verification: PASS (M3 + M4) v M5-M11, one at a time see BUILD-MILESTONES.md ``` @@ -245,16 +245,17 @@ M5-M11, one at a time see BUILD-MILESTONES.md **One milestone at a time. Do not begin a milestone before its brief exists.** -**M4 is implemented and unreviewed.** Its implementation report is the current -action; M5 does not begin before that report is written and accepted. +**No M5 brief has been prepared.** Writing one is the current action, informed by +the M4 report's §U readiness assessment and by the note `BUILD-MILESTONES.md` +attaches to M5 — in particular that M4 added 55 tests using the inherited +world-state values as deterministic instrumentation, which M5 must **move rather +than delete**. -Two conditions remain open. The **browser smoke test has still not been -performed** — now for M3 and for M4 — because no session so far has had a usable -browser. See `archive/milestone-reports/M3-IMPLEMENTATION-REPORT.md` §M and §W.4, -`reports/M4-IMPLEMENTATION-REPORT.md` §M, and the M4 status block in -`BUILD-MILESTONES.md`. And **no M4 review exists**: the status block was -written by the implementation and records what it built, which is not the same as -a reviewer having read it. +**No conditions remain open on M1-M4.** The browser smoke condition that M3 and +M4 both carried was satisfied at M4 closeout: a real Firefox exercised the +rendered DOM for both milestones' controls, 44/44 checks passing. The M3 report's +§M.2 and the M4 report's §M record the condition as it stood; the M4 report's §W +records it closed. ## What each milestone closeout corrected diff --git a/planning/STORY-BRANCH-SEMANTICS.md b/planning/STORY-BRANCH-SEMANTICS.md index 54ee137..bf22cc0 100644 --- a/planning/STORY-BRANCH-SEMANTICS.md +++ b/planning/STORY-BRANCH-SEMANTICS.md @@ -467,6 +467,39 @@ They should not be removed by: - branch divergence, - ordinary history cleanup. +### 19.1 A checkpoint protects the history it names + +Added at M4 closeout, from implementation evidence. + +§19's durability rule is only meaningful if something enforces it against the +operations that delete history. Deleting a branch deletes that branch and +everything forked from it, so a checkpoint naming a position anywhere in that +subtree would go with it — and go silently, because the story is what the user +asked to delete and the named moments are not mentioned in the request. + +Therefore: + +- **A branch cannot be deleted while a checkpoint names a position on it, or on + any branch forked from it.** The deletion is refused, not amended: nothing is + half-deleted, and no checkpoint is quietly relocated or dropped. +- **The refusal names the checkpoints standing in the way**, so the user knows + what to act on rather than being told only that something is in the way. +- **The user resolves it by deleting the checkpoint explicitly**, which is + §19's "until explicitly deleted" being honoured rather than worked around. + Deleting a checkpoint still deletes no story (§25), so the recovery costs the + user nothing they wanted to keep. +- **The scope is the subtree**, not the named branch alone. + +This is the same rule §28 already anticipates for a future cleanup feature — +*retain paths referenced by checkpoints* — applied to the one deletion path that +exists today. A cleanup or discarded-history feature added later must honour it +too, and must not acquire a way to delete a checkpoint as a side effect of +deleting history. + +Campaign deletion is not an exception to this and needs no rule: deleting a +campaign deletes everything in it, checkpoints included, and that is what the +user asked for. + ## 20. Checkpoint Restore Restoring a checkpoint: diff --git a/planning/TECHNICAL-DESIGN.md b/planning/TECHNICAL-DESIGN.md index 24983ee..faaf333 100644 --- a/planning/TECHNICAL-DESIGN.md +++ b/planning/TECHNICAL-DESIGN.md @@ -452,9 +452,17 @@ once they have. **Nothing removes a Save Point but the user.** There is no cleanup pass, and none is wanted: a Save Point pointing behind the head, or into a line the story left, -is doing its job. The one exception is referential and not a policy — deleting a -branch takes its Save Points with it, by the same cascade that takes its -memories, because the story they named went with it. +is doing its job. + +That rule is enforced against the one operation that could break it. Deleting a +branch deletes everything forked from it, so a Save Point naming a position in +that subtree would go too — silently, since the story is what the user asked to +delete. **The deletion is therefore refused while any Save Point names that +subtree**, and the refusal names them. The user deletes the Save Point +explicitly, which deletes no story, and then the branch. `STORY-BRANCH-SEMANTICS.md` +§19.1 states the rule; §28 already required a future cleanup feature to retain +paths a checkpoint references, and this is that requirement applied to the +deletion path that exists today. ## 9. Export / Import and Head Position diff --git a/planning/V1-ACCEPTANCE-TESTS.md b/planning/V1-ACCEPTANCE-TESTS.md index 7abc6ae..c0eb694 100644 --- a/planning/V1-ACCEPTANCE-TESTS.md +++ b/planning/V1-ACCEPTANCE-TESTS.md @@ -1,8 +1,28 @@ # Adventure Storyteller — V1 Acceptance Tests -**Status:** v1.2 planning/release contract — updated after Phase 0B, after M2 for -the security contract (H10 strengthened, H12 added), and after M3 for history -ownership and results (D03, D10, I07, L01) +**Status:** v1.3 planning/release contract — updated after Phase 0B, after M2 for +the security contract (H10 strengthened, H12 added), after M3 for history +ownership and results (D03, D10, I07, L01), and after M4 for Save Point results +(D11-D14, I04, L03, E-series) and the browser condition below + +> **Browser-level verification (M4 closeout, 2026-09-03).** The browser smoke +> condition that M3 and M4 both carried is **satisfied**. A real Firefox 154.0.1, +> driven through geckodriver over the W3C WebDriver protocol, exercised the +> rendered DOM: M3's Undo/Redo enable states, transcript movement, Retry and the +> take pager, divergence and the loss of Redo; and M4's full Save Point +> lifecycle including both confirmations and the branch-delete warning. 44/44 +> checks passed with no console errors, on two independent runs. No pass +> condition anywhere in this document was changed to achieve it. See +> `reports/M4-IMPLEMENTATION-REPORT.md` §W. +> +> **Three kinds of evidence are recorded separately below, and are not +> interchangeable.** *Automated* means a test in the repository's suite, which +> runs on every future change. *Live runtime* means a real server exercised over +> HTTP — stronger than a unit test about process boundaries, weaker than a +> browser about anything a user sees. *Browser* means the rendered DOM driven by +> a real browser, which is the only evidence that a control is visible, enabled +> and wired. Where a result cites more than one, the strongest is named last. + **Purpose:** Define black-box acceptance tests for finalist evaluation during Phase 0B and for the eventual v1 release. ## 1. Test Philosophy @@ -737,6 +757,14 @@ Before entering the abbey ### Pass Checkpoint persists across application restart. +### Result — PASS (M4 closeout, 2026-09-04) +*Automated:* `backend/tests/test_process_restart.py` starts the application as a +subprocess, writes the campaign, **terminates the process**, and starts a second +process against the same database — the Save Point, its name and its +`(branch, depth)` coordinate all survive. +*Browser:* the Save Point is still listed after a full page reload +(`reports/M4-IMPLEMENTATION-REPORT.md` §W.7 section E). + --- ## D12 — Restore Checkpoint @@ -751,6 +779,11 @@ Checkpoint persists across application restart. ### Pass Transcript/state return to checkpoint position. +### Result — PASS (M4 closeout, 2026-09-04) +*Automated:* `test_d12_restore_returns_the_transcript_and_the_state`. +*Browser:* the visible transcript and the state both move back, and the view +refreshes without a manual reload (§W.7 section F). + --- ## D13 — Restore Does Not Delete Later History @@ -760,6 +793,13 @@ Transcript/state return to checkpoint position. ### Pass Later story is retained as abandoned/disposable history. +### Result — PASS (M4 closeout, 2026-09-04) +*Automated:* measured on **row identity**, not on counts — the set of action row +ids before a restore equals the set after it. Ordinary Redo still walks the retained +continuation until a divergent write, and after that write the displaced rows are +still present while Redo reports nothing ahead. Four `test_d13_*` tests, and +confirmed in the browser with a database check behind it. + --- ## D14 — Delete Checkpoint @@ -773,10 +813,31 @@ Delete named checkpoint. - checkpoint pointer disappears, - referenced story turn/history remains intact. +### Result — PASS (M4 closeout, 2026-09-04) +*Automated:* the pointer row goes; the referenced turn, the later history and the +active head are all unchanged (`test_d14_delete_removes_the_pointer_and_no_story`). +*Browser:* the confirmation states that deleting the Save Point does not delete +the story, and the story remains afterwards (§W.7 section I). + +A Save Point is also the **only** thing that can remove itself: deleting a branch +whose history a Save Point names is refused rather than cascading +(`STORY-BRANCH-SEMANTICS.md` §19.1), verified automatically and in the browser. + --- # E. Branch and Lineage Safety +> **M4 result (2026-09-03).** E01 and E04 were re-exercised through a Save Point +> restore rather than only through Undo, and pass: a memory derived past a +> restored head stops being retrievable and becomes eligible again on Redo, +> without being deleted or re-embedded; after restore-plus-divergence the old +> future's memory stays ineligible even as the new line grows past its depth; and +> the transcript after a restore holds only the active lineage while the +> displaced rows remain in the tree. **E03** (summary lineage over a long story) +> remains **NOT PERFORMED** — it needs a long-run campaign and is owned by +> M6/M11, unchanged from M3. + + ## E01 — Abandoned Future Cannot Affect Active State **Priority:** REQUIRED FOR V1 @@ -1353,6 +1414,15 @@ Export preserves retained alternate/disposable history needed for recovery, unle ### Pass Named checkpoints survive export/import. +### Result — PASS (M4 closeout, 2026-09-04) +*Automated and live runtime.* A real round trip with three Save Points across two +branches: names, notes and +coordinates survive, branch references are remapped to the imported rows +(1→3, 2→4), and each restores to a distinct position and state in the new +campaign. Importing Save Points does **not** move the active head — the head +still comes from the bundle's `headDepth`. Bundles written before M4 carry no +`checkpoints` key, import cleanly, and create none. + --- ## I05 — Knowledge Provenance Export @@ -1581,6 +1651,14 @@ State at each position matches original accepted state. ### Pass Correct historical state is reconstructed. +### Result — PASS (M4 closeout, 2026-09-04) +*Automated, across a genuine OS process boundary:* the state at the Save Point +was recorded before the first process exited, and a second process restored +exactly that value after the campaign had been advanced past it +(`backend/tests/test_process_restart.py`). This replaced a same-process +`TestClient` restart, which could not distinguish durable state from a live +object. + --- ## L04 — Derived Data Can Be Rebuilt diff --git a/planning/VERSION.md b/planning/VERSION.md index 0cb2cb8..d5592d7 100644 --- a/planning/VERSION.md +++ b/planning/VERSION.md @@ -1,8 +1,93 @@ # Planning Package Version -- **Package:** Adventure Storyteller Planning Package v2.5 +- **Package:** Adventure Storyteller Planning Package v2.6 - **Revision date:** 2026-09-03 -- **Status:** Phase 0 complete; architecture selected; **Milestones M1, M2 and M3 implemented and accepted**; **M4 implemented 2026-09-03 and awaiting review.** +- **Status:** Phase 0 complete; architecture selected; **Milestones M1-M4 implemented and accepted**; M5 is next to brief. + +## v2.6 — M4 Closeout (2026-09-03) + +M4 is **accepted**. Its review returned *PASS WITH CORRECTIVE WORK REQUIRED*; the +corrective work is done, and the browser condition that M3 and M4 both carried is +closed. + +**The three review findings, fixed:** + +- **B-1 — the Save Point list was an N+1.** It resolved each Save Point with its + own query and loaded whole `Action` rows, narration included, to answer "does a + row exist here". It is now one bulk two-column coordinate query plus one + lineage computation: **53 SELECTs for 25 Save Points became 5**, and the count + no longer grows with the list. Guarded by three tests, including one proving + the coordinate is matched as a *pair* — an `IN`-list version would report a + Save Point resolved because another branch has a live row at the same depth. +- **B-2 — deleting a branch silently deleted its Save Points.** Fixed as a + **behaviour** defect rather than a missing warning, because + `STORY-BRANCH-SEMANTICS.md` §19 says a named checkpoint remains until + explicitly deleted and §28 already required future cleanup to retain + checkpoint-referenced paths. **A branch a Save Point names can no longer be + deleted.** The request is refused with the offending Save Points named; the + user deletes them explicitly, which deletes no story, and the branch then + goes. Both delete controls disable and explain. Recorded as a new + **§19.1**. +- **B-3 — the D11/L03 automation never left one process.** A new module spawns + real server processes, kills the first, and reads the campaign back with the + second. + +**Also fixed (review §S C-5):** creating a Save Point now takes the campaign's +turn lock, so "save where I am" cannot read a head a turn in flight is about to +move. Rename and Delete deliberately do not take it, and a test pins that +decision. + +**Real-browser verification — the first in this project.** A Firefox 154.0.1 +driven through geckodriver over the W3C WebDriver protocol exercised the rendered +DOM for **both** milestones: **44/44 checks passed**, no console errors. It +covered M3's Undo/Redo enable states, transcript movement, Retry and the take +pager, and divergence retiring Redo; and M4's whole Save Point lifecycle +including both confirmations and the new branch-delete warning. **The outstanding +M3 browser condition is therefore closed as well.** No dependency was added to +the repository: the WebDriver client for the run was written against stdlib HTTP. + +**Also corrected, found while fixing B-2:** `models.py`, `TECHNICAL-DESIGN.md` +§8.8 and `DATA-MODEL.md` §8 all described the cascade as the durability rule. +They now describe the refusal, and record that `checkpoints.branch_id`'s cascade +survives as referential integrity that the application no longer reaches. + +**Documents corrected by this closeout:** + +- `V1-ACCEPTANCE-TESTS.md` records results for **D11-D14, I04, L03** and the + E-series, and states that the browser-level condition is satisfied. **No pass + condition was weakened** — and D11/L03 now note that the automation crosses a + genuine OS process boundary, which is the standard later milestones should + meet. +- `DATA-MODEL.md` §8 records the coordinate as implemented, with the retry + measurement that settles coordinate-versus-turn-id, and three decisions that + were previously implicit: names are not unique, several Save Points may name + one position, and the list is newest-created first. +- `STORY-BRANCH-SEMANTICS.md` gains **§19.1** — a checkpoint protects the + history it names. This is the only behavioural specification change in the + closeout, and it strengthens §19 rather than weakening anything. +- `BROWSER-UX-SPEC.md` §25 rules for the implemented vocabulary: **Moment N**, + not *Turn N*, because the branch panel and tree overlay already count in + moments. Vocabulary only; no behaviour changes. +- `BUILD-MILESTONES.md` marks **M4 COMPLETE**, records the fixes and the browser + result, and warns M5 that the instrumentation to move is now **55 tests**. +- `README.md` records M1-M4 accepted and M5 as next to brief. +- `PROJECT-SOURCES.md` and `project-sources.txt` point at the current report; + `project-sources.txt` still named the archived M3 report and was corrected. + +**Report rotation** happened in the reporting pass that preceded this closeout: +`M3-IMPLEMENTATION-REPORT.md` moved to `archive/milestone-reports/` as a pure +rename, and `reports/` now holds M4's report, whose **§W** is this closeout's +evidence record. + +**No new ADR.** ADR 012 already decides the architecture, and the corrective work +forced no new architectural decision. `SPECIFICATION.md`, +`STORY-BRANCH-SEMANTICS.md`, `SECURITY-THREAT-MODEL.md`, `CONTEXT-AND-MEMORY.md`, +`IMPORTED-KNOWLEDGE-DESIGN.md` and ADRs 003, 005 and 012 are unchanged. + +**M5 readiness:** ready. Save Points store no state and no checkpoint code reads +any, so M5 can change what a snapshot contains without touching what a Save Point +is — provided it keeps state recoverable at a position without replay +(`TECHNICAL-DESIGN.md` §10.4). ## v2.5 — M4 Implementation (2026-09-03) diff --git a/planning/project-sources.txt b/planning/project-sources.txt index 7bdda56..987fcac 100644 --- a/planning/project-sources.txt +++ b/planning/project-sources.txt @@ -26,4 +26,4 @@ planning/DECISIONS/009-ai-dnd-production-base.md planning/DECISIONS/010-explicit-typed-narrative-state-events.md planning/DECISIONS/011-local-inference-endpoint-policy.md planning/DECISIONS/012-active-head-non-destructive-history.md -planning/reports/M3-IMPLEMENTATION-REPORT.md +planning/reports/M4-IMPLEMENTATION-REPORT.md diff --git a/planning/reports/M4-IMPLEMENTATION-REPORT.md b/planning/reports/M4-IMPLEMENTATION-REPORT.md index 81cc783..ccc9ecb 100644 --- a/planning/reports/M4-IMPLEMENTATION-REPORT.md +++ b/planning/reports/M4-IMPLEMENTATION-REPORT.md @@ -1040,4 +1040,279 @@ second consecutive milestone. What to do with that is the reviewer's call. --- +# W. M4 Closeout Addendum (2026-09-03 / 2026-09-04) + +**This addendum is current where it differs from the review above**, and §W.3 is +current where it differs from an earlier draft of this addendum: B-2 was first +addressed as a warning over a surviving cascade, then reclassified as a behaviour +defect and fixed by refusing the deletion. Only the final behaviour is described +below; the review's §R still records the finding as it was raised. Sections +A-V record what was true at commit `e08d49c`, before corrective work; nothing in +them has been rewritten. Where they say a finding is open or a test is +unperformed, this section says what happened next. + +## W.1 Starting state + +| Fact | Value | +| --- | --- | +| Closeout started from | `279a871` — *Planning: add the M4 implementation review report and rotate M3's* | +| Closeout dates | corrective work and browser verification 2026-09-03; B-2 reclassified and re-fixed 2026-09-04 | +| Reviewed implementation | `e08d49c` — signed, unchanged by this pass | +| Working tree at start | clean; the report and rotation were already committed | +| Upstream ancestry | intact | +| LICENSE | unchanged | + +## W.2 B-1 — the Save Point list N+1 is gone + +`GET /checkpoints` resolved each Save Point with its own query and loaded whole +`Action` entities to do it. It now takes one bulk query over two integer columns +plus one lineage computation, both before the loop. + +Measured on the same 25-Save-Point fixture the review used: + +| | before | after | +| --- | --- | --- | +| SQL statements per list | **53** | **5** | +| SELECTs against `actions` | 25 | **1** | +| columns in that SELECT | 11, including `text` | **2** (`branch_id`, `depth`) | +| narration fetched | yes | **no** | +| queries per Save Point | 2.1 | 0.2 | + +Three tests guard it. One asserts on *growth* rather than a fixed budget — five +times the Save Points must not mean more queries — because a fixed number is +something to edit rather than a rule to keep. One asserts the emitted SQL never +names `actions.text`, `actions.reasoning` or `actions.world_delta`. The third is +the one worth keeping: it builds a **cross-product trap**, a dead coordinate on +one branch while another branch has a live row at the same depth, and proves the +dead one still reports `resolved: false`. An `IN`-list implementation would fail +it; the OR-of-pairs passes. + +`_render_all` is now the single rendering path — create and rename call it +through `_rendered` with a list of one — so the list and the single-item +responses cannot drift. + +## W.3 B-2 — a branch a Save Point names cannot be deleted + +The review recorded this as a missing warning. On closeout it was reclassified as +a **behaviour defect**, because the authority is unambiguous: +`STORY-BRANCH-SEMANTICS.md` §19 says a named checkpoint remains until explicitly +deleted, and §28 already requires a future cleanup feature to *retain paths +referenced by checkpoints*. A cascade that removed Save Points along with a +branch violates both, and a warning would only have documented the violation. + +**The deletion is now refused.** `DELETE /branches/{id}` returns **409** when any +Save Point names a position on that branch or on anything forked from it — the +subtree, because deleting a branch takes its descendants, and a check scoped to +the named branch alone would let a Save Point on a child vanish silently. + +The refusal names what stands in the way rather than only counting it: + +```text +This branch, or a branch forked from it, is where a Save Point “On the new +line” is saved. Delete that Save Point first if you no longer need it, then +delete the branch. Deleting a Save Point does not delete any story. +``` + +Long lists truncate ("and 2 more") so the message stays a sentence someone reads. + +The recovery is the point, and it is cheap: deleting a Save Point deletes no +story (§25), so the user removes the pointer and the branch then goes. Verified +end to end in the browser (§W.7 section J). + +Both delete controls — the branch list and the tree overlay — now **disable** +Delete while Save Points protect the subtree and say why, and the branch row +reports `· 2 Save Points kept here`. The confirmation, now only reachable when +nothing is at risk, says plainly that the story on other paths and every Save +Point are unaffected. Two views changed, because the same deletion is reachable +from both and a rule holding in one of them would not be a rule. + +`checkpoints.branch_id` keeps `ON DELETE CASCADE` as referential integrity — a +Save Point must never point at a branch that is gone — but the guard means it +does not fire through the application. Campaign deletion still cascades, which is +what deleting a campaign means. + +Six tests cover the five required scenarios plus the truncation: deletion refused +and nothing lost; the Save Point intact after the refusal; explicit deletion then +allowing the branch delete; a branch no Save Point names still deleting; a Save +Point on a *descendant* also protecting; and an unrelated Save Point not blocking +anything. A source-level test asserts both views disable and explain. + +**Documents this corrected beyond the planned set:** `models.py`, +`TECHNICAL-DESIGN.md` §8.8 and `DATA-MODEL.md` §8 had all recorded the cascade as +the durability rule. They now record the refusal. + +## W.4 B-3 — the restart test now crosses a process boundary + +`backend/tests/test_process_restart.py` (3 tests, ~9 s) starts the real +application with `subprocess.Popen`, plays a story over HTTP, **kills the +process**, starts a second process against the same database file, and only then +asks its questions. Readiness is probed, never slept on; children are terminated +in a `finally` whether or not the test passes. + +It covers D11 (the Save Point, its name and its coordinate survive), L03 (the +state at the position returns), that restore deletes no rows, that Redo is +available before divergence, that the retained continuation is still walkable, +and that a divergent write after a restart still forks on the write rather than +the restore. + +The old same-process helper was **not** kept as a pretend equivalent. + +## W.5 C-5 — creating a Save Point is serialized + +Create now takes the campaign's existing turn lock, the same one Undo, Redo and +Restore take. No new lock was introduced. "Save where I am" has to name one +committed position, and the head is exactly what a turn in flight is about to +move. + +Rename and Delete deliberately **do not** take it: neither reads nor moves a +story position, and refusing a label edit during generation would be a worse +product for no safety gained. A test pins that decision so it reads as a choice +rather than an oversight, and another proves a refused create releases the lock. + +## W.6 Test and build results + +All against the closeout tree. + +| Suite | Result | +| --- | --- | +| **Full backend** | **698 passed**, 0 failed, 0 skipped (was 680) | +| **M4 targeted** (`test_save_points.py` + `test_process_restart.py`) | **60 passed** (57 + 3) | +| **M3 invariants** (8 modules) | **130 passed** | +| **Security / local-only** (incl. `test_egress.py`) | **93 passed** | +| **Frontend lint** | exit 0; 7 warnings, unchanged from baseline | +| **Frontend build** | exit 0 | +| **Docker build** | exit 0 | + +698 − 680 = 18 new tests: 3 process-restart, 4 for B-1, 7 for B-2 (the refusal +rule and its recovery), 3 for C-5, and one holding the branch-list count to the +same subtree the server refuses on. Two tests written earlier in this closeout +were **replaced**, not kept alongside: they asserted the cascade behaviour that +§W.3 removed, and leaving them would have pinned the defect. + +## W.7 Browser verification — PERFORMED, and it passes + +**This is the first real-browser verification in the project**, and it discharges +the condition M3 and M4 both carried. + +| | | +| --- | --- | +| Browser | **Mozilla Firefox 154.0.1**, headless | +| Driver | geckodriver 0.37.1, W3C WebDriver over HTTP | +| Client | written for the run against Python's stdlib — **no dependency added to the repository** | +| App | the real production-shaped server, SPA served same-origin, deterministic scripted model | +| Result | **47/47 checks passed**, no console errors; run twice on independent fresh databases | + +§M reported no usable browser, and that was true of the paths tried there: the +`firefox` snap wrapper fails with mount-namespace errors and hangs on a headless +screenshot. The binary **inside** the snap +(`/snap/firefox/current/usr/lib/firefox/firefox`) runs correctly under +geckodriver, which §M did not try. §M's conclusion is superseded; its account of +what was attempted stands. + +What the browser actually did, by section of the closeout brief: + +| Section | Verified in the DOM | +| --- | --- | +| **A — M3 history** | transcript renders; Undo enabled and Redo disabled at the tip; two Undos move the transcript back twice; Redo becomes enabled; two Redos return the original tip exactly | +| **B — retry / takes** | Retry produces an alternate take; the take pager appears; stepping between takes changes the visible narration; no branch vocabulary needed | +| **C — M3 divergence** | a new continuation from a moved-back head appears; Redo becomes unavailable; no stale old-future text in the active transcript; position and continuation survive a reload — with a database check confirming the displaced rows are still on disk | +| **D — creation** | Save Point created and named through the form; appears in the list; panel says **Save Point** with no branch/head/node/fork wording; position reads **Moment N** | +| **E — persistence** | still present after a full page reload | +| **F — restore** | the confirmation visibly explains later history is kept; transcript and state move back; the Save Point stays listed; Redo becomes available; zero rows deleted (database check); the view refreshes without a manual reload | +| **G — restore + redo** | Redo returns the original continuation; the Save Point survives | +| **H — restore + divergence** | the different continuation appears; Redo into the displaced future is gone; no old-future narration in the active view; the displaced rows retained on disk | +| **I — rename / delete** | rename changes the name and not the position; it still restores to the same moment; the delete warning says the story is not deleted; the row disappears with no stale list; the story remains | +| **J — branch deletion** | the branch row reports the Save Points kept on it; Delete is **disabled** and explains what to do; the server independently refuses with **409** naming the Save Point; nothing is deleted by the refusal; and the documented recovery works — deleting the Save Point frees the branch | +| **K — control state** | enable/disable states match the position; no material console errors | + +**No defect was found in the application by the browser run.** Four failures +occurred and all four were in the harness: a wrong SPA route (`/adventures/:id` +rather than `/play/:id`); a wait predicate that compared transcript *length* when +the empty-story placeholder is longer than the first turn; a fixture that tried +to delete the branch it was reading, which is refused by design; and a reload +assertion that sampled the transcript once instead of waiting for it to render. +The last was checked against the application before being called a harness bug — +after a reload the text is present at the first sample, so the race was the +test's. + +## W.8 Planning documents updated + +`V1-ACCEPTANCE-TESTS.md` (D11-D14, I04, L03, E-series results and the browser +condition; **no pass condition weakened**), `STORY-BRANCH-SEMANTICS.md` **§19.1** +(new — a checkpoint protects the history it names; the closeout's only +behavioural specification change, and it strengthens §19), +`DATA-MODEL.md` §8, `TECHNICAL-DESIGN.md` §8.8, `BROWSER-UX-SPEC.md` §25 (Moment, +not Turn), `BUILD-MILESTONES.md` (M4 COMPLETE, the facts M5 inherits, the M5 +instrumentation note), `planning/README.md`, `VERSION.md` (v2.6), +`PROJECT-SOURCES.md` and `project-sources.txt`, which still named the archived M3 +report. + +Unchanged, as §T recommended: `SPECIFICATION.md`, `STORY-BRANCH-SEMANTICS.md`, +`SECURITY-THREAT-MODEL.md`, `CONTEXT-AND-MEMORY.md`, +`IMPORTED-KNOWLEDGE-DESIGN.md`, ADR 003, ADR 005, ADR 012. **No ADR 013**: the +corrective work forced no architectural decision. §19.1 is a durability rule +inside an existing specification, not a new architecture — ADR 005 already says +history is preserved rather than overwritten, and refusing to delete a +checkpoint's history is that decision applied, not a departure from it. + +## W.9 The architecture is unchanged + +Worth stating plainly, because closeout touched the checkpoint module. A Save +Point is still `name + optional note + (branch, depth)`. Restore is still +`coordinate → head.move_to_node → head.move_to → attempts.restore_state`. The +corrective work touched **how the list is read** and **when create is allowed to +read the head** — never what a Save Point is or how restoring one moves the +story. Nothing was copied into a checkpoint, no checkpoint-specific Redo stack +exists, restore still does not fork, and the first divergent write is still what +creates the continuation. + +## W.10 Remaining debt + +Unchanged from §S and none of it M4's: no frontend test runner (M8) — the browser +run above is a closeout procedure, not a suite; `POST /adventures/import` returns +every branch's rows rather than a head-capped window (inherited, M3); the RPG +world-state instrumentation, now **60 tests**, moves at M5. + +## W.11 Result + +D11, D12, D13, D14, I04 and L03 all **PASS**. The E-series lineage results +through a Save Point restore all **PASS** (E03 remains NOT PERFORMED, owned by +M6/M11). The Definition of Done is met, and now met in a browser: + +> The user can create a named Save Point, continue, restart, restore it, and +> continue differently without losing later history. + +```text +M4 CLOSED — READY FOR M5 +``` + +M5 is next to brief. It has not been started. + +## W.12 What M5 inherits + +Recorded here because it is the only thing this report owes the next milestone. +These are constraints, not suggestions, and none of them is M5's to revisit. + +- **The inherited RPG world-state system is instrumentation, not the target + architecture.** 60 tests in this milestone use gold arithmetic to make "the + state at this position" a number a test can assert. M5 replaces the machinery + underneath and must **move the instrumentation while keeping the assertions** — + what they measure is where the story is being read and what state belongs to + that position, which is exactly as true after M5. +- **M5 implements ADR 010's genre-neutral typed narrative-state model.** That is + the target; the stat/band/cooldown protocol is what it replaces. +- **Per-position snapshots, or equivalent fast recovery, must survive the + replacement** (`TECHNICAL-DESIGN.md` §10.4). Undo, Redo and Save Point restore + all resolve a coordinate and read the state recorded there. If M5 makes state + reconstruction proportional to campaign length, it degrades all three at once. +- **M3/M4 history and Save Point semantics are infrastructure now.** The stored + head, the single movement mechanism, the capped lineage, fork-on-first-write, + and the Save Point coordinate are settled. M5 changes what state *contains*, + not how the story is positioned, and should not add a second restore path. +- **A Save Point's history is protected** (`STORY-BRANCH-SEMANTICS.md` §19.1). + Any state-cleanup or migration work M5 introduces must not acquire a way to + delete a checkpoint, or the history one names, as a side effect. + +--- + *End of report.*