From f1bebe18d0f5828627b8630efc8b02902baca01e Mon Sep 17 00:00:00 2001 From: parththakkar106 Date: Sat, 29 Aug 2026 11:49:07 +0530 Subject: [PATCH] Drop the eight legacy columns SP8 left behind Migrations 66 to 73 drop `actions.index`, `variants`, `variant_index`, `variant_count`, `state_before`, and `world_state_before`, plus `adventures.memory_cursor` and `summary_cursor`. `index` is a keyword in SQLite, so migration 71 quotes it. Nothing outside the migrations read these. `models.py`, `schemas.py`, and `ACTION_LIST_COLUMNS` lose the same eight fields, `Adventure.actions` orders by `id`, and `attempts.renumber`, `context.history.max_action_index`, and `nodes.next_index` are deleted. Two changes keep the migration replayable on a `create_all` database: - `_split_variants_into_siblings` wrote through the live ORM table, so it stopped compiling once migration 66 removed five of its columns. It now writes through `_ACTIONS_AT_60`, a frozen `Table` with its own `MetaData`. - Five data passes read columns these migrations drop. Each now calls `_has_columns` and returns early when the columns are absent. `bootstrap` takes a `through` version so a migration test can stop at the schema it asserts on. 555 tests pass, up from 549. Eight of the new cases assert each column is gone after a real schema-45 database migrates all the way. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0198qDK3gmgSo7EtQ4GTPqqK --- backend/app/attempts.py | 52 +++------ backend/app/bundle.py | 36 ++---- backend/app/context/builder.py | 2 +- backend/app/context/history.py | 24 ---- backend/app/migrations.py | 110 ++++++++++++++++++- backend/app/models.py | 62 ++--------- backend/app/routers/adventures/crud.py | 1 - backend/app/routers/adventures/nodes.py | 13 +-- backend/app/routers/adventures/paging.py | 20 +--- backend/app/routers/adventures/turns.py | 16 +-- backend/app/schemas.py | 11 +- backend/app/tree.py | 23 ++-- backend/tests/test_action_paging.py | 40 ++++--- backend/tests/test_attempt_siblings.py | 24 ++-- backend/tests/test_branch_clause.py | 12 +- backend/tests/test_branch_forking.py | 5 +- backend/tests/test_branch_management.py | 2 +- backend/tests/test_bundle_v2.py | 32 ++---- backend/tests/test_egress.py | 67 ++++------- backend/tests/test_guest_cleanup.py | 2 +- backend/tests/test_history_window.py | 6 +- backend/tests/test_length_hint.py | 4 +- backend/tests/test_memory_nodes.py | 1 - backend/tests/test_memory_retrieval.py | 2 +- backend/tests/test_memory_settling.py | 19 ++-- backend/tests/test_prompt_caching.py | 6 +- backend/tests/test_retry_variants.py | 58 +++++++--- backend/tests/test_scenario_art.py | 6 +- backend/tests/test_snapshot_compression.py | 12 +- backend/tests/test_state_revert.py | 10 +- backend/tests/test_story_tree_baseline.py | 4 +- backend/tests/test_take_edit.py | 2 +- backend/tests/test_take_parentage.py | 2 +- backend/tests/test_take_state.py | 2 +- backend/tests/test_tree_migration.py | 57 ++++++++-- backend/tests/test_turn_flow_integration.py | 2 +- backend/tests/test_worldstate_integration.py | 2 +- backend/tools/branch_fixture.py | 2 +- backend/tools/stress_session.py | 3 +- backend/tools/tree_fixture.py | 2 +- plan/17-refactor.md | 99 ++++++++++++++++- 41 files changed, 477 insertions(+), 378 deletions(-) diff --git a/backend/app/attempts.py b/backend/app/attempts.py index e43807d..aeb4154 100644 --- a/backend/app/attempts.py +++ b/backend/app/attempts.py @@ -2,7 +2,7 @@ A retry used to rewrite the AI action in place and append the discarded attempt to a JSON list on the same row. Seven separate bugs came from that arrangement. -The row's `text` duplicated one entry of a repeating group, `variant_count` +The row's `text` duplicated one entry of a repeating group, a second column duplicated its length, and every reader that touched the story during a retry had to be told to ignore the row. @@ -25,10 +25,11 @@ that maintains either one: avoid. The prompt therefore moves with the live flag, and a superseded sibling keeps only its own slices. -Ordering inside a group comes from `variant_index`, which is an explicit ordinal -rather than `created_at`. Two attempts made in the same second still have to page -in the order they were made, and the migration that split the old JSON lists had -to state the order rather than reconstruct it. +Ordering inside a group comes from `id`, not from `created_at`. Two attempts made +in the same second still have to page in the order they were made, and `id` +increases with every insert. SP8 dropped `variant_index`, an explicit ordinal +that carried the same order, once a run of the suite confirmed that the two +agreed in every group. """ import copy @@ -82,7 +83,7 @@ def group(db: Session, action: models.Action) -> list[models.Action]: models.Action.depth == action.depth, models.Action.parent_id.is_(None), ) - .order_by(models.Action.variant_index, models.Action.id) + .order_by(models.Action.id) .all() ) return ( @@ -91,7 +92,7 @@ def group(db: Session, action: models.Action) -> list[models.Action]: models.Action.adventure_id == action.adventure_id, models.Action.parent_id == action.parent_id, ) - .order_by(models.Action.variant_index, models.Action.id) + .order_by(models.Action.id) .all() ) @@ -190,8 +191,8 @@ def add_attempt( """Places `replacement` next to `previous` as the newer attempt at that turn. The placement is done here rather than through `tree.place_action`, which - would read the depth from the legacy `index` and move the head. A sibling is - not a new turn. It is another attempt at the turn the head is already on. + moves the head. A sibling is not a new turn. It is another attempt at the + turn the head is already on. """ replacement.branch_id = previous.branch_id replacement.depth = previous.depth @@ -202,18 +203,10 @@ def add_attempt( # away from this turn. replacement.parent_id = previous.parent_id replacement.live = True - # Use the end of the group rather than one past `previous`. The two match - # only when `previous` is the newest attempt. Switch a three-attempt turn - # back to attempt 1 and retry, and `previous.variant_index + 1` collides with - # attempt 2. `renumber` then breaks the tie by id and places the new attempt - # between attempts 2 and 3, so the pager walks the attempts in an order they - # were not made in. `group` returns oldest first, and `replacement` is not in - # it yet. - siblings = group(db, previous) - replacement.variant_index = 1 + max( - (s.variant_index for s in siblings if s.variant_index is not None), - default=previous.variant_index or 0, - ) + # The replacement takes its place at the end of the group, because `group` + # orders by `id` and this row has no id yet. Switching a three-attempt turn + # back to attempt 1 and retrying therefore still pages 1, 2, 3, 4, which is + # the order the attempts were made in. previous.live = False # The replacement was assembled with a fresh snapshot, so the prompt for # this turn is now the one it carries. The superseded attempt keeps only the @@ -226,8 +219,7 @@ def make_live( ) -> list[models.Action]: """Makes `node` the attempt the story tells, and restores its outcome. - Returns the group, renumbered, so that a caller reporting on it does not read - it twice. + Returns the group, so that a caller reporting on it does not read it twice. """ rows = group(db, node) previous = live_in(rows) @@ -236,23 +228,9 @@ def make_live( for row in rows: row.live = row is node restore_state(adventure, node) - renumber(rows) return rows -def renumber(rows: list[models.Action]) -> None: - """Refreshes the group-shape cache that the page response reads. - - `variant_count` is 0 rather than 1 for a turn nobody retried, because the - pager asks whether there is anything to page through, and for a single - attempt the answer is no. - """ - count = len(rows) if len(rows) > 1 else 0 - for i, row in enumerate(rows): - row.variant_index = i - row.variant_count = count - - # ------------------------------------------------- the prompt, stored once def keep_own_slices(node: models.Action) -> None: diff --git a/backend/app/bundle.py b/backend/app/bundle.py index a7eac5e..1ba222f 100644 --- a/backend/app/bundle.py +++ b/backend/app/bundle.py @@ -14,8 +14,8 @@ Version 2 carries the tree. It holds three things version 1 could not, and each one is required: * The branches, because a forked adventure is two stories and a flat list holds - one. A version 1 export interleaved them by `index`, which read as a garbled - story rather than as lost data. + one. A version 1 export interleaved them by turn number, which read as a + garbled story rather than as lost data. * `live`, because a coordinate can hold several attempts at one turn and exactly one of them is the story. * Both after-snapshots, because they are what a branch switch and an undo @@ -26,22 +26,14 @@ one is required: A bundle carries what was chosen, never what is derived. The head branch, the fork points, the live flags, and the anchors are decisions somebody made, so -they are in the file. `lineage`, the head depth, `index`, and the variant -ordinals are all computed from those, and the import recomputes them: +they are in the file. `lineage` and the head depth are computed from those, and +the import recomputes them: * `lineage` is a cache of `parent` plus `fork_depth`. Shipping it as well would put a second source of truth for one fact into a file anyone can hand-edit, and the two could then disagree without any read reporting it. * The head depth is the tip of the head branch, which is a fact about the nodes that arrived with it. -* `index` is the legacy column SP8 drops. Its one remaining job is to give the - next row a number nothing else holds, which is a fact about the adventure - rather than about a path, so `depth` cannot serve: two branches each have a - node at depth 4. The import allocates one index per turn instead, which keeps - `max_action_index` correct and keeps siblings sharing an index the way SP4 - leaves them. -* `attempts.renumber` maintains the variant ordinals, and it is the only place - allowed to. Every hand-editable coordinate is therefore checked before a row is written, in `plan`, rather than repaired afterwards. An import that fails partway leaves an @@ -98,8 +90,7 @@ def export(db: Session, adventure: models.Adventure) -> dict: undefer(models.Action.world_state_after), ) .order_by( - models.Action.branch_id, models.Action.depth, - models.Action.variant_index, models.Action.id, + models.Action.branch_id, models.Action.depth, models.Action.id, ) .all() ) @@ -168,8 +159,7 @@ def _exported_branch(branch: models.Branch, local: dict[int, int]) -> dict: def _exported_node(action: models.Action, local: dict[int, int]) -> dict: node = { "branch": _local(action.branch_id, local), - # A pre-tree row's depth is the number `index` already holds. - "depth": action.depth if action.depth is not None else action.index, + "depth": action.depth, "live": bool(action.live), "type": action.type, "text": action.text, @@ -496,24 +486,17 @@ def _write_nodes( ) -> None: """Writes the nodes, grouped into the turns they are attempts at. - Two values are allocated here rather than read from the file. `index` is - issued once per turn, in the order the bundle lists them, so siblings share - one index and no two coordinates do. `max_action_index` needs that to keep - issuing numbers nothing holds. Exactly one attempt in each group is also made - live, because a file can name none or several, and a turn with no live node - disappears from the story. + One value is decided here rather than read from the file. Exactly one + attempt in each group is made live, because a file can name none or several, + and a turn with no live node disappears from the story. """ groups: dict[tuple[int, int], list[models.Action]] = {} - indices: dict[tuple[int, int], int] = {} for spec in specs: key = (spec["branch"], spec["depth"]) - if key not in indices: - indices[key] = len(indices) action = models.Action( adventure_id=adventure.id, branch_id=ids[spec["branch"]], depth=spec["depth"], - index=indices[key], type=spec["type"], text=spec["text"], reasoning=spec["reasoning"], @@ -531,7 +514,6 @@ def _write_nodes( live = next((row for row in rows if row.live), rows[0]) for row in rows: row.live = row is live - attempts.renumber(rows) def _write_memories( diff --git a/backend/app/context/builder.py b/backend/app/context/builder.py index 950e9d1..2b97716 100644 --- a/backend/app/context/builder.py +++ b/backend/app/context/builder.py @@ -353,7 +353,7 @@ def build_context( # Even the newest action alone is over budget: hard-truncate it. included_actions.append( models.Action( - adventure_id=action.adventure_id, index=action.index, + adventure_id=action.adventure_id, type=action.type, text=truncate_to_last_tokens(action.text, history_budget), ) diff --git a/backend/app/context/history.py b/backend/app/context/history.py index 744637a..df279df 100644 --- a/backend/app/context/history.py +++ b/backend/app/context/history.py @@ -417,30 +417,6 @@ def newest(adventure: models.Adventure) -> models.Action | None: return rows[0] if rows else None -def max_action_index(adventure: models.Adventure) -> int: - """Highest `Action.index` in the adventure, story text or not. -1 if empty. - - This is the only read in the module that is deliberately not scoped to a - path. `index` is a legacy column that remains unread until SP8 drops it. Its - one remaining job is to give the next row a number that no other row holds, - which is a fact about the adventure rather than about the story being - played. Scoping the query to a branch would let two branches issue the same - index. - """ - loaded = _loaded_actions(adventure) - if loaded is not None: - return max((a.index for a in loaded), default=-1) - db = _session(adventure) - if db is None: - return -1 - highest = ( - db.query(func.max(models.Action.index)) - .filter(models.Action.adventure_id == adventure.id) - .scalar() - ) - return -1 if highest is None else highest - - def window_covering( adventure: models.Adventure, budget_tokens: int, diff --git a/backend/app/migrations.py b/backend/app/migrations.py index ae75b53..65ebab8 100644 --- a/backend/app/migrations.py +++ b/backend/app/migrations.py @@ -23,7 +23,10 @@ import json import re from datetime import datetime -from sqlalchemy import inspect, text +from sqlalchemy import ( + JSON, Boolean, Column, DateTime, Integer, MetaData, String, Table, Text, + inspect, text, +) from sqlalchemy.engine import Engine from . import compression, vectors @@ -301,6 +304,34 @@ MIGRATIONS: list[tuple[int, str | dict[str, str]]] = [ # turn streams. This is item S1 in `docs/self-review.md`. The table holds one # row per user, so the rewrite is small and needs no VACUUM FULL. (65, "ALTER TABLE settings DROP COLUMN stream"), + # Phase 17, SP8: drop the eight columns the story tree replaced. Each one was + # kept past the migration that superseded it so that a rollback found a real + # value on the rows the newer build wrote. The tree has run in production + # since Phase 14, so the rollback window is closed. + # + # `actions.index` was the global turn number. `depth` replaced it, and the + # last reader, the number issued to a new row, went with this migration. + # `variants` held the retry history as a repeating group, and `variant_index` + # and `variant_count` described that group's shape. Each attempt is its own + # row now, ordered by `id`. `state_before` and `world_state_before` recorded + # the state a node started from, which every attempt at a turn shares; the + # "after" columns record each attempt's own outcome instead. + # `adventures.memory_cursor` and `summary_cursor` were positions into a flat + # list, and the branch and depth pairs beside them replaced both. + # + # Dropping a column rewrites the toasted values on Postgres and nothing + # reclaims that space on its own. Run `VACUUM FULL actions;` on the direct + # Neon endpoint after the deploy, not the pooled one. It takes an ACCESS + # EXCLUSIVE lock, so the app blocks on `actions` while it runs. + (66, "ALTER TABLE actions DROP COLUMN variants"), + (67, "ALTER TABLE actions DROP COLUMN variant_index"), + (68, "ALTER TABLE actions DROP COLUMN variant_count"), + (69, "ALTER TABLE actions DROP COLUMN state_before"), + (70, "ALTER TABLE actions DROP COLUMN world_state_before"), + # `index` is a keyword in SQLite, so the column name is quoted. + (71, 'ALTER TABLE actions DROP COLUMN "index"'), + (72, "ALTER TABLE adventures DROP COLUMN memory_cursor"), + (73, "ALTER TABLE adventures DROP COLUMN summary_cursor"), ] LATEST_VERSION = max((v for v, _ in MIGRATIONS), default=1) @@ -410,6 +441,8 @@ def _backfill_variant_count(conn) -> None: in Python would read every stored attempt over the network once in order to stop reading it on every request. """ + if not _has_columns(conn, "actions", "variants", "variant_count"): + return if conn.dialect.name == "sqlite": sql = """ UPDATE actions SET variant_count = json_array_length(variants) @@ -434,6 +467,22 @@ _ADD_COLUMN = re.compile(r"^\s*ALTER\s+TABLE\s+(\w+)\s+ADD\s+COLUMN\s+\"?(\w+)\" _DROP_COLUMN = re.compile(r"^\s*ALTER\s+TABLE\s+(\w+)\s+DROP\s+COLUMN\s+\"?(\w+)\"?", re.I) +def _has_columns(conn, table: str, *columns: str) -> bool: + """Returns `True` when `table` has every one of `columns`. + + The data passes below read columns that later migrations drop. A pass only + ever has real work to do on a database old enough to still carry them, and + a `create_all` database is already current, so the guard skips the pass + rather than failing on a column that is not there. This is the same rule + `_column_already_there` applies to DDL, written for the passes. + """ + inspector = inspect(conn) + if table not in inspector.get_table_names(): + return False + present = {col["name"] for col in inspector.get_columns(table)} + return all(column in present for column in columns) + + def _column_already_gone(conn, sql: str) -> bool: """Returns `True` when `sql` drops a column the table no longer has. @@ -589,6 +638,8 @@ def _backfill_tree(conn) -> None: target being unset, so a run that fails partway through resumes rather than applying twice. """ + if not _has_columns(conn, "actions", "index"): + return sqlite = conn.dialect.name == "sqlite" # 1. A root branch per adventure. Its lineage names the row's own id, which @@ -698,6 +749,8 @@ def _backfill_cursor_anchors(conn) -> None: two forms return the same answer, because the rows the partition would separate are the rows the filter removes. """ + if not _has_columns(conn, "adventures", "memory_cursor", "summary_cursor"): + return sqlite = conn.dialect.name == "sqlite" story = _story_text_sql("text", sqlite) for name in ("memory", "summary"): @@ -750,6 +803,8 @@ def _backfill_state_after(conn) -> None: SP4 is the first migration where `depth` and `index` can disagree, and it has not run when this pass does. """ + if not _has_columns(conn, "actions", "state_before", "world_state_before"): + return for column, live in ( ("state_after", "script_state"), ("world_state_after", "world_state"), @@ -778,6 +833,39 @@ def _backfill_state_after(conn) -> None: """)) +# The `actions` table as migration 60 finds it, declared here rather than read +# from `Base.metadata`. The split pass writes JSON values, and only a column +# type knows how to render a dict on this dialect, so it needs a Table. Reading +# the live one would tie a migration to the current model: migration 66 drops +# five of these columns, and the pass would then fail to compile on a database +# that still has them. This declaration is a snapshot of a past schema and must +# not be updated to track `models.py`. +# +# It carries its own `MetaData`, so `create_all` never sees it. +_ACTIONS_AT_60 = Table( + "actions", MetaData(), + Column("id", Integer, primary_key=True), + Column("adventure_id", Integer), + Column("index", Integer), + Column("branch_id", Integer), + Column("depth", Integer), + Column("live", Boolean), + Column("type", String(20)), + Column("text", Text), + Column("reasoning", Text), + Column("context_snapshot", compression.CompressedJSON), + Column("world_delta", JSON), + Column("state_before", JSON), + Column("world_state_before", JSON), + Column("state_after", JSON), + Column("world_state_after", JSON), + Column("variants", JSON), + Column("variant_count", Integer), + Column("variant_index", Integer), + Column("created_at", DateTime), +) + + def _split_variants_into_siblings(conn) -> None: """Gives every discarded retry attempt its own row. @@ -802,7 +890,9 @@ def _split_variants_into_siblings(conn) -> None: The pass is resumable. A group that already has as many rows as its `variant_count` claims has been split, so it is skipped. """ - actions = Base.metadata.tables["actions"] + if not _has_columns(conn, "actions", "variants", "variant_index"): + return + actions = _ACTIONS_AT_60 last_id = 0 while True: rows = conn.execute( @@ -854,7 +944,8 @@ def _split_one_action(conn, actions, row, entries: list) -> None: kept["world_state_after"] = live_world # Use the Table rather than `text()`, here and below. These values are dicts # bound for JSON columns, and the column type is the only thing that knows - # how to render one on this dialect. + # how to render one on this dialect. The table is `_ACTIONS_AT_60`, frozen + # at the schema this pass runs against. conn.execute(actions.update().where(actions.c.id == row["id"]).values(**kept)) siblings = [ { @@ -956,16 +1047,23 @@ def _set_version(conn, version: int) -> None: conn.execute(text("UPDATE schema_version SET version = :v"), {"v": version}) -def bootstrap(engine: Engine) -> None: +def bootstrap(engine: Engine, through: int = LATEST_VERSION) -> None: + """Brings the database up to `through`, which defaults to the newest version. + + The app always takes the default. A test passes an older version when it + asserts on something a later migration removes. A migration test that stops + at the version it is about keeps reading the columns that migration wrote, + rather than the schema those columns became several versions later. + """ fresh = not inspect(engine).get_table_names() Base.metadata.create_all(bind=engine) with engine.begin() as conn: if fresh: - _set_version(conn, LATEST_VERSION) + _set_version(conn, through) return current = _get_version(conn) for version, sql in MIGRATIONS: - if version > current: + if current < version <= through: statement = _for_dialect(sql, conn.dialect.name) # Skip the DDL when it has already run. The data pass below it # still runs. diff --git a/backend/app/models.py b/backend/app/models.py index c9de38f..94d146f 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -118,15 +118,8 @@ class Adventure(Base): # Phase 6: opt-in per adventure (extra AI calls) auto_summarize: Mapped[bool] = mapped_column(Boolean, default=False) memory_bank_enabled: Mapped[bool] = mapped_column(Boolean, default=False) - # Legacy columns from Phase 6. Each holds a count of the actions that were - # folded into the memories or the story summary, expressed as a position in - # the story. Nothing has read them since SP3, and nothing writes them except - # a v1 import. They remain for one release so that a rollback resumes from a - # real number. SP8 drops them along with `actions.index`. The columns below - # hold the marks that this code actually uses. - memory_cursor: Mapped[int] = mapped_column(Integer, default=0) - summary_cursor: Mapped[int] = mapped_column(Integer, default=0) - # Phase 14, SP3: the same two marks expressed as coordinates. Each pair + # Phase 14, SP3: how far the memory pass and the summary pass have read, + # each as a coordinate. Each pair # holds the branch and depth of the last action that pass covered. A # position moves when an action in front of it is deleted, so the mark # silently starts covering an action it never read. A depth is a coordinate @@ -168,7 +161,7 @@ class Adventure(Base): actions: Mapped[list["Action"]] = relationship( back_populates="adventure", cascade="all, delete-orphan", - order_by="Action.index", + order_by="Action.id", ) scripts: Mapped[list["AdventureScript"]] = relationship( back_populates="adventure", @@ -261,8 +254,7 @@ class Memory(Base): ) # The stretch of story this memory summarizes, given as depths on # `branch_id`. Both are NULL for a hand-written memory, which summarizes no - # actions. Before SP3 these columns held `Action.index` values, which were - # the same numbers. `source_end` is the depth of the node the memory + # actions. `source_end` is the depth of the node the memory # attaches to, and `depth` below mirrors it. `source_start` is where the # stretch begins, which is where the summarizer resumes if the memory is # withdrawn. @@ -283,10 +275,9 @@ class Memory(Base): ForeignKey("branches.id", ondelete="CASCADE"), nullable=True ) depth: Mapped[int | None] = mapped_column(Integer, nullable=True) - # Whether `embedding_blob` is set. `memorybank.set_vector` keeps this - # column current, for the same reason that `actions.variant_count` sits - # beside `actions.variants`. Readers need only the yes-or-no answer, and - # fetching six kilobytes of vector to get it is too expensive. + # Whether `embedding_blob` is set. `memorybank.set_vector` keeps this column + # current. Readers need only the yes-or-no answer, and fetching six + # kilobytes of vector to get it is too expensive. embedded: Mapped[bool] = mapped_column(Boolean, default=False) pinned: Mapped[bool] = mapped_column(Boolean, default=False) forgotten: Mapped[bool] = mapped_column(Boolean, default=False) # evicted, kept for UI @@ -346,18 +337,15 @@ class Action(Base): id: Mapped[int] = mapped_column(primary_key=True) adventure_id: Mapped[int] = mapped_column(ForeignKey("adventures.id", ondelete="CASCADE")) - index: Mapped[int] = mapped_column(Integer) # Phase 14: the node's place in the tree. `depth` is a position along one # path rather than a global turn number. Node A4 and node B4 are - # alternatives, not duplicates. `depth` replaces `index` as the ordering - # key. + # alternatives, not duplicates. # # Both columns are nullable because ALTER TABLE cannot add a NOT NULL column # without a default, and no default makes sense for a branch. The migration # fills these columns for existing rows, and `tree.place_action` fills them # for new rows. From SP2 onward, a NULL `branch_id` marks a row that no read - # can see. The legacy `index` column stays alongside, unread, until the tree - # is proven in production. SP8 drops it. + # can see. branch_id: Mapped[int | None] = mapped_column( ForeignKey("branches.id", ondelete="CASCADE"), nullable=True ) @@ -417,20 +405,6 @@ class Action(Base): # and for re-attaching the emit block when replaying history to the model. # Mirrors the active variant, same as text/reasoning/context_snapshot. world_delta: Mapped[dict | None] = mapped_column(JSON, nullable=True) - # Legacy columns from before SP4. They hold `Adventure.script_state` and - # `Adventure.world_state` as they were immediately before this action's - # script hooks ran. Nothing has written or read them since SP4. The pair of - # "after" columns below replaced them, because each sibling attempt needs - # its own outcome and every attempt at a turn shares the same starting - # state. These columns remain for one release so that a rolled-back build - # still finds a real snapshot on the rows it wrote. SP8 drops them along - # with `index` and `variants`. - state_before: Mapped[dict | None] = mapped_column( - JSON, nullable=True, deferred=True - ) - world_state_before: Mapped[dict | None] = mapped_column( - JSON, nullable=True, deferred=True - ) # Phase 14, SP4: the shared script state and the RPG world state as they # stood after this node was played. These columns record the node's outcome # rather than its starting position. @@ -453,24 +427,6 @@ class Action(Base): world_state_after: Mapped[dict | None] = mapped_column( JSON, nullable=True, deferred=True ) - # A legacy column from before SP4. It holds the retry history as a repeating - # group inside a JSON list. Every attempt at a turn is now its own row, as - # described on `live` above and in `app/attempts.py`, so nothing reads this - # column. It remains until SP8 for the same reason `index` does. Migration - # 60 reads it once more, to turn each entry into a sibling row. - variants: Mapped[list | None] = mapped_column(JSON, nullable=True, deferred=True) - # Where the row sits in its sibling group. `variant_index` is this - # attempt's ordinal, counting from the oldest. `variant_count` is the number - # of attempts in the group. It is 0 rather than 1 when the turn was never - # retried, which the pager treats as having nothing to page through. - # - # Both columns are caches, and `attempts.renumber` is the only place that - # maintains them. The reason matches why `variant_count` once cached - # `len(variants)`: a page response needs both numbers for every row and - # cannot afford one query per turn. SP7 replaces the pager with the branch - # view, and SP8 drops both columns. - variant_count: Mapped[int] = mapped_column(Integer, default=0) - variant_index: Mapped[int] = mapped_column(Integer, default=0) created_at: Mapped[datetime] = mapped_column(DateTime, default=utcnow) adventure: Mapped[Adventure] = relationship(back_populates="actions") diff --git a/backend/app/routers/adventures/crud.py b/backend/app/routers/adventures/crud.py index 4c26b67..f42052e 100644 --- a/backend/app/routers/adventures/crud.py +++ b/backend/app/routers/adventures/crud.py @@ -206,7 +206,6 @@ def create_adventure( if scenario.prompt.strip(): opening = models.Action( adventure_id=adventure.id, - index=0, type="start", text=fill_placeholders(scenario.prompt, values), ) diff --git a/backend/app/routers/adventures/nodes.py b/backend/app/routers/adventures/nodes.py index c05ea8b..c55b279 100644 --- a/backend/app/routers/adventures/nodes.py +++ b/backend/app/routers/adventures/nodes.py @@ -10,22 +10,15 @@ from sqlalchemy.orm import Session, undefer from ... import attempts, memorybank, models, tree from ...context import cursors -from ...context import history as context_history from ...context import lineage -def next_index(adventure: models.Adventure) -> int: - return context_history.max_action_index(adventure) + 1 - - def next_depth(adventure: models.Adventure) -> int: """Returns the depth for the next node played onto this story, one past the tip. - This is not `next_index`, which returned the same number until SP5. `index` - has to stay unique across the whole adventure, because it is the v1 bundle's - key. On a story forked at depth 6 after twenty turns, `next_index` gives the - next node depth 21 and leaves a fourteen-deep gap in the path. A depth is a - position along this one story, and the branch is what makes it unambiguous. + A depth is a position along one story, and the branch is what makes it + unambiguous. Two branches each hold a node at depth 4, and they are + alternatives rather than duplicates. """ return adventure.head_depth + 1 diff --git a/backend/app/routers/adventures/paging.py b/backend/app/routers/adventures/paging.py index ed50654..92c0c23 100644 --- a/backend/app/routers/adventures/paging.py +++ b/backend/app/routers/adventures/paging.py @@ -24,13 +24,10 @@ from ...context import lineage # row. ACTION_LIST_COLUMNS = ( models.Action.adventure_id, - models.Action.index, models.Action.type, models.Action.text, models.Action.reasoning, models.Action.world_delta, - models.Action.variant_count, - models.Action.variant_index, # SP9: the pager's key. If `parent_id` were deferred, every row on the page # would cost a lazy load, which is the cost `load_only` is here to prevent. # `branch_id` is listed for the same reason. The pager reads it to tell a @@ -124,33 +121,28 @@ def annotate_takes( This runs one query for the whole page rather than one per row. The pager needs the shape of each turn's attempt group, and calling `attempts.group` - per action costs one query per message on screen. `variant_count` was cached - to avoid that cost, which is why SP8 could not drop it. + per action costs one query per message on screen. This function reads the siblings rather than counting them. A group holds only a few attempts, the page is bounded, and a count still needs a second - query for the ordinal. It fetches only the id and the ordering keys, so it - stays cheap even when the text is large. + query for the ordinal. It fetches only the id and the parent, so it stays + cheap even when the text is large. """ parents = {a.parent_id for a in actions if a.parent_id is not None} if parents: rows = ( - db.query( - models.Action.id, - models.Action.parent_id, - models.Action.variant_index, - ) + db.query(models.Action.id, models.Action.parent_id) .filter( models.Action.adventure_id == adventure_id, models.Action.parent_id.in_(parents), ) - .order_by(models.Action.variant_index, models.Action.id) + .order_by(models.Action.id) .all() ) else: rows = [] siblings: dict[int, list[int]] = {} - for row_id, parent_id, _ in rows: + for row_id, parent_id in rows: siblings.setdefault(parent_id, []).append(row_id) for action in actions: ids = siblings.get(action.parent_id) if action.parent_id else None diff --git a/backend/app/routers/adventures/turns.py b/backend/app/routers/adventures/turns.py index 58f4402..1e0369a 100644 --- a/backend/app/routers/adventures/turns.py +++ b/backend/app/routers/adventures/turns.py @@ -23,7 +23,7 @@ from ...sse import SSE_HEADERS, sse, turn_error from ..settings import get_settings from .deps import CurrentUser, current_adventure, router -from .nodes import _move_to_after, next_depth, next_index +from .nodes import _move_to_after, next_depth from .paging import annotate_takes @@ -275,11 +275,6 @@ async def _generate_turn( reasoning = "".join(reasoning_chunks).strip() or None ai_action = models.Action( adventure_id=adventure.id, - # A sibling shares the turn's legacy index for the same reason it - # shares its depth: it is the same turn. Two rows then hold one index, - # which is safe, because `max_action_index` takes a maximum rather than - # a count, and nothing else reads the column. - index=retry_of.index if retry_of is not None else next_index(adventure), depth=ai_depth, type="ai", text=text, @@ -298,8 +293,10 @@ async def _generate_turn( # a turn landed on top of it. See `memorybank`. memorybank.forget_node(db, adventure, retry_of) cursors.rewind_all(adventure, retry_of.branch_id, ai_depth - 1) + # Flush so the new attempt has an id. The session does not autoflush, + # and attempts page in id order, so a read taken before this point puts + # the newest attempt nowhere. db.flush() - attempts.renumber(attempts.group(db, ai_action)) else: tree.place_action(db, adventure, ai_action) db.add(ai_action) @@ -369,7 +366,6 @@ async def run_player_turn( return player_action = models.Action( adventure_id=adventure.id, - index=next_index(adventure), depth=next_depth(adventure), type=payload.type, text=modified, @@ -384,8 +380,8 @@ async def run_player_turn( db.refresh(player_action) # The new action was added through its foreign key, so the loaded # `adventure.actions` collection is stale. Without this expire, - # `build_context` and `next_index` for the AI action do not see the - # player action that was just saved. + # `build_context` for the AI action does not see the player action + # that was just saved. db.expire(adventure, ["actions"]) yield sse({"type": "player", "action": action_json(player_action, db)}) if stop: diff --git a/backend/app/schemas.py b/backend/app/schemas.py index 5ca7f19..905659b 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -186,27 +186,20 @@ class RefreshPlan(BaseModel): class ActionOut(ORMModel): id: int adventure_id: int - index: int type: str text: str reasoning: str | None = None # Phase 12: the compact RPG state changes for this turn, read from the # model property. world_changes: list[dict] = [] - # Retry history: how many attempts exist for this turn, where 0 means the - # turn was never retried, and which attempt is live. The attempts themselves - # come from `GET /actions/{id}/variants`, so this payload stays small. - variant_count: int = 0 - variant_index: int = 0 # SP9: the pager, such as `2/4`. It reports how many attempts this turn has # and which one is on screen. It is keyed on the parent, so it counts the # attempts of this turn rather than every node that shares a depth, and it # keeps counting them after one has been forked onto its own branch. # # A turn nobody has retaken reads 1/1, which is most turns, and the client - # draws no pager for a count of one. `variant_count` uses a different - # convention and reports 0 for the same case. Those two fields are the - # pre-SP9 pair, and SP8 drops them. + # draws no pager for a count of one. The attempts themselves come from + # `GET /actions/{id}/variants`, so this payload stays small. take_count: int = 1 take_index: int = 0 # Which line this node is on, so the pager can distinguish the two kinds of diff --git a/backend/app/tree.py b/backend/app/tree.py index bd17ae3..78a194c 100644 --- a/backend/app/tree.py +++ b/backend/app/tree.py @@ -118,8 +118,8 @@ def fork(db: Session, adventure: models.Adventure, node: models.Action) -> model raise ValueError("cannot fork from a node that is not on a branch") fork_depth = node.depth - 1 # Read the sibling attempts before moving the node. The session does not - # autoflush, so a later read still finds the node here and renumbers it back - # into the group it just left. + # autoflush, so a later read still finds the node here and hands the live + # flag back to it. remaining = [ row for row in db.query(models.Action) .filter( @@ -127,7 +127,7 @@ def fork(db: Session, adventure: models.Adventure, node: models.Action) -> model models.Action.branch_id == parent.id, models.Action.depth == node.depth, ) - .order_by(models.Action.variant_index, models.Action.id) + .order_by(models.Action.id) .all() if row is not node ] @@ -160,14 +160,12 @@ def fork(db: Session, adventure: models.Adventure, node: models.Action) -> model depth = node.depth node.branch_id = new_id node.live = True - node.variant_index = 0 - node.variant_count = 0 + # The group the node left needs a live attempt again. Taking the oldest is + # arbitrary, and it has to be somebody: a coordinate with no live attempt + # disappears from the story on the branch it was left on. if remaining and not any(row.live for row in remaining): remaining[0].live = True - for i, row in enumerate(remaining): - row.variant_index = i - row.variant_count = len(remaining) if len(remaining) > 1 else 0 adventure.head_branch_id = new_id adventure.head_depth = depth @@ -228,9 +226,10 @@ def place_action( ) -> models.Branch: """Puts `action` on the head branch and moves the head to it. - `depth` follows `index` while both columns exist. The two must agree, - because a read that orders by depth and a cursor that counts in index space - describe the same story, and SP2 swaps one for the other in a single step. + An action with no `depth` of its own goes one step past the tip, which is + where the next turn belongs. The opening of a new adventure lands at 0 that + way, because an adventure with nothing played has a head depth of + `NO_DEPTH`. Pass `branch` when you have already resolved the head and are placing several nodes at once. See `place_new_nodes` for why that is worth doing. @@ -244,7 +243,7 @@ def place_action( branch = branch or head_branch(db, adventure) action.branch_id = branch.id if action.depth is None: - action.depth = action.index + action.depth = adventure.head_depth + 1 if parent is not None: action.parent_id = parent.id elif action.parent_id is None and action.depth: diff --git a/backend/tests/test_action_paging.py b/backend/tests/test_action_paging.py index 4c09d7c..f314f42 100644 --- a/backend/tests/test_action_paging.py +++ b/backend/tests/test_action_paging.py @@ -12,6 +12,8 @@ to be scrolling. An anchor means the same thing before and after. python -m pytest tests/test_action_paging.py -v """ +import re + import pytest from fastapi import Depends from fastapi.testclient import TestClient @@ -38,7 +40,7 @@ def client(monkeypatch): setup.flush() for i in range(TOTAL): setup.add(models.Action( - adventure_id=adventure.id, index=i, + adventure_id=adventure.id, type="start" if i == 0 else ("ai" if i % 2 else "do"), text=f"Action {i}." + "word " * 200, )) @@ -62,6 +64,17 @@ def client(monkeypatch): Base.metadata.drop_all(bind=engine) +def ordinal(action) -> int: + """Returns which turn this is, read out of the fixture's own text. + + The payload used to carry `index`, a story-wide turn number that SP8 + dropped. Nothing replaced it: a depth is a position along one branch, and + the pager keys on ids. The fixture numbers its own actions, so these tests + read the number back rather than reintroduce one. + """ + return int(re.match(r"Action (\d+)\.", action["text"]).group(1)) + + def page(client, before_id=None, limit=None): params = {} if before_id is not None: @@ -76,10 +89,8 @@ def page(client, before_id=None, limit=None): def add_action(client, text="A new turn.") -> int: db = SessionLocal() try: - highest = db.query(models.Action.index).order_by( - models.Action.index.desc()).first()[0] action = models.Action( - adventure_id=client.adv_id, index=highest + 1, type="ai", text=text + adventure_id=client.adv_id, type="ai", text=text ) db.add(action) db.commit() @@ -97,14 +108,14 @@ def test_the_page_load_returns_only_the_newest_window(client): assert len(body["actions"]) == ACTION_PAGE assert body["action_count"] == TOTAL # It is the newest window, ending on the last action. - assert body["actions"][-1]["index"] == TOTAL - 1 - assert body["actions"][0]["index"] == TOTAL - ACTION_PAGE + assert ordinal(body["actions"][-1]) == TOTAL - 1 + assert ordinal(body["actions"][0]) == TOTAL - ACTION_PAGE def test_a_short_story_is_returned_whole(client): db = SessionLocal() try: - db.query(models.Action).filter(models.Action.index >= 5).delete() + db.query(models.Action).filter(models.Action.depth >= 5).delete() db.commit() finally: db.close() @@ -142,19 +153,19 @@ def test_the_first_page_is_the_newest(client): assert len(body["actions"]) == ACTION_PAGE assert body["total"] == TOTAL assert body["has_more"] is True - assert body["actions"][-1]["index"] == TOTAL - 1 + assert ordinal(body["actions"][-1]) == TOTAL - 1 def test_paging_up_covers_the_whole_story_exactly_once(client): seen = [] body = page(client) - seen = [a["index"] for a in body["actions"]] + seen = [ordinal(a) for a in body["actions"]] guard = 0 while body["has_more"]: guard += 1 assert guard < 20, "paging did not terminate" body = page(client, before_id=body["actions"][0]["id"]) - seen = [a["index"] for a in body["actions"]] + seen + seen = [ordinal(a) for a in body["actions"]] + seen assert seen == list(range(TOTAL)), "gap, duplicate or reordering while paging" @@ -163,12 +174,12 @@ def test_has_more_is_false_at_the_beginning_of_the_story(client): body = page(client) while body["has_more"]: body = page(client, before_id=body["actions"][0]["id"]) - assert body["actions"][0]["index"] == 0 + assert ordinal(body["actions"][0]) == 0 def test_each_page_is_ordered_oldest_first(client): body = page(client) - indices = [a["index"] for a in body["actions"]] + indices = [ordinal(a) for a in body["actions"]] assert indices == sorted(indices) @@ -185,9 +196,10 @@ def test_a_turn_arriving_mid_scroll_does_not_shift_the_next_page(client): add_action(client) older = page(client, before_id=oldest_held["id"]) - assert older["actions"][-1]["index"] == oldest_held["index"] - 1, \ + assert ordinal(older["actions"][-1]) == ordinal(oldest_held) - 1, ( "the page shifted when a turn landed" - assert all(a["index"] < oldest_held["index"] for a in older["actions"]) + ) + assert all(ordinal(a) < ordinal(oldest_held) for a in older["actions"]) # The new turn changes the total, which is expected. It must not move the window. assert older["total"] == TOTAL + 1 diff --git a/backend/tests/test_attempt_siblings.py b/backend/tests/test_attempt_siblings.py index 853fce7..c8c97ec 100644 --- a/backend/tests/test_attempt_siblings.py +++ b/backend/tests/test_attempt_siblings.py @@ -49,7 +49,7 @@ def client(monkeypatch): ) setup.add(adv) setup.flush() - setup.add(models.Action(adventure_id=adv.id, index=0, type="start", text="You enter a cave.")) + setup.add(models.Action(adventure_id=adv.id, type="start", text="You enter a cave.")) setup.add(models.AdventureScript( adventure_id=adv.id, position=0, enabled=True, name="Gold", output_js=GOLD_SCRIPT, )) @@ -112,7 +112,7 @@ def _rows(adv_id) -> list[models.Action]: undefer(models.Action.world_state_after), undefer(models.Action.context_snapshot), ) - .order_by(models.Action.depth, models.Action.variant_index) + .order_by(models.Action.depth, models.Action.id) .all() ) finally: @@ -330,16 +330,22 @@ def test_a_memory_on_an_earlier_turn_survives_a_retry(client): # -------------------------------------------------------------- the group -def test_the_group_cache_is_renumbered_as_attempts_arrive(client): +def test_the_group_grows_in_the_order_the_attempts_arrive(client): + """The attempts page in the order they were made, and the pager counts them. + + Ordering used to come from `variant_index`, an explicit ordinal that SP8 + dropped. `id` carries the same order, because a row is inserted when its + attempt is made. + """ ScriptedProvider.replies = ["One.", "Two.", "Three."] _play(client) - assert _page(client)["actions"][-1]["variant_count"] == 0 # never retried + assert _page(client)["actions"][-1]["take_count"] == 1 # never retried _retry(client) _retry(client) ai = [a for a in _rows(client.adv_id) if a.type == "ai"] - assert [a.variant_index for a in ai] == [0, 1, 2] - assert {a.variant_count for a in ai} == {3} + assert [a.text for a in ai] == ["One.", "Two.", "Three."] + assert _page(client)["actions"][-1]["take_count"] == 3 def test_attempts_module_agrees_with_the_endpoint(client): @@ -394,9 +400,8 @@ def test_a_retry_after_switching_back_files_the_new_attempt_last(client): `add_attempt` used to number a new take one past the take it replaced. That numbering is correct only when the story is standing on the newest take. Switch a three-take turn back to the first and retry, and the new - attempt collides with take 2. `renumber` broke the tie by id and placed - the new attempt between takes 2 and 3, so the pager listed them in an - order the player never produced. + attempt collided with take 2, which put it between takes 2 and 3. The + group orders by `id` now, so a new attempt is always last. """ ScriptedProvider.replies = ["One.", "Two.", "Three.", "Four."] _play(client) @@ -414,7 +419,6 @@ def test_a_retry_after_switching_back_files_the_new_attempt_last(client): _retry(client) ai = [a for a in _rows(client.adv_id) if a.type == "ai"] assert [a.text for a in ai] == ["One.", "Two.", "Three.", "Four."] - assert [a.variant_index for a in ai] == [0, 1, 2, 3] assert attempts.live_in(ai).text == "Four." diff --git a/backend/tests/test_branch_clause.py b/backend/tests/test_branch_clause.py index 6e52fd5..d557211 100644 --- a/backend/tests/test_branch_clause.py +++ b/backend/tests/test_branch_clause.py @@ -64,7 +64,6 @@ def make_branch(db, adventure, parent=None, fork_depth=None): def add_node(db, adventure, branch, depth, label, index=None): action = models.Action( adventure_id=adventure.id, - index=depth if index is None else index, branch_id=branch.id, depth=depth, type="start" if depth == 0 else ("ai" if depth % 2 else "do"), @@ -328,13 +327,16 @@ def test_a_node_written_without_a_branch_is_placed_anyway(forked): """ db, adventure, ids = forked written = models.Action( - adventure_id=adventure.id, index=99, type="do", text="C8" + adventure_id=adventure.id, type="do", text="C8" ) db.add(written) db.commit() assert written.branch_id == ids["c"] - assert written.depth == 99 - assert adventure.head_depth == 99 + # The node lands one step past the tip of C. SP8 dropped `index`, which is + # where a caller used to name its own depth, so an unplaced node now always + # follows the head. + assert written.depth == adventure.head_depth + assert adventure.head_depth == 8 assert labels(history.tail(adventure, 2)) == ["C7", "C8"] @@ -362,7 +364,7 @@ def test_placing_a_flush_of_nodes_reads_the_branch_once(forked, emitted_sql): emitted_sql.clear() for i in range(50): db.add(models.Action( - adventure_id=adventure.id, index=500 + i, type="do", text=f"bulk {i}" + adventure_id=adventure.id, type="do", text=f"bulk {i}" )) db.commit() branch_reads = [s for s in emitted_sql if s.startswith("SELECT") and "FROM branches" in s] diff --git a/backend/tests/test_branch_forking.py b/backend/tests/test_branch_forking.py index 0d9065b..8b5bb40 100644 --- a/backend/tests/test_branch_forking.py +++ b/backend/tests/test_branch_forking.py @@ -59,7 +59,7 @@ def client(monkeypatch): ) setup.add(adv) setup.flush() - setup.add(models.Action(adventure_id=adv.id, index=0, type="start", text="You enter a cave.")) + setup.add(models.Action(adventure_id=adv.id, type="start", text="You enter a cave.")) setup.add(models.AdventureScript( adventure_id=adv.id, position=0, enabled=True, name="Gold", output_js=GOLD_SCRIPT, )) @@ -132,7 +132,7 @@ def _rows(adv_id) -> list[models.Action]: return ( db.query(models.Action) .filter(models.Action.adventure_id == adv_id) - .order_by(models.Action.depth, models.Action.variant_index) + .order_by(models.Action.depth, models.Action.id) .all() ) finally: @@ -233,7 +233,6 @@ def test_the_parent_keeps_a_live_attempt_where_the_fork_left(client): # offers a page through attempts that diverged onto another branch. parent_turn = per_coordinate[(parent_id, 2)] assert len(parent_turn) == 1 - assert parent_turn[0].variant_count == 0 finally: db.close() diff --git a/backend/tests/test_branch_management.py b/backend/tests/test_branch_management.py index 6360c0d..9d94090 100644 --- a/backend/tests/test_branch_management.py +++ b/backend/tests/test_branch_management.py @@ -47,7 +47,7 @@ def client(monkeypatch): setup.add(adv) setup.flush() setup.add(models.Action( - adventure_id=adv.id, index=0, type="start", text="You enter a cave.")) + adventure_id=adv.id, type="start", text="You enter a cave.")) setup.commit() adv_id, user_id = adv.id, user.id setup.close() diff --git a/backend/tests/test_bundle_v2.py b/backend/tests/test_bundle_v2.py index 2523b8b..20f8eec 100644 --- a/backend/tests/test_bundle_v2.py +++ b/backend/tests/test_bundle_v2.py @@ -67,7 +67,7 @@ def client(monkeypatch): ) setup.add(adv) setup.flush() - setup.add(models.Action(adventure_id=adv.id, index=0, type="start", text=OPENING)) + setup.add(models.Action(adventure_id=adv.id, type="start", text=OPENING)) setup.add(models.AdventureScript( adventure_id=adv.id, position=0, enabled=True, name="Gold", output_js=GOLD_SCRIPT, )) @@ -163,7 +163,7 @@ def _rows(adv_id) -> list[models.Action]: db.query(models.Action) .filter(models.Action.adventure_id == adv_id) .order_by(models.Action.branch_id, models.Action.depth, - models.Action.variant_index) + models.Action.id) .all() ) finally: @@ -341,29 +341,20 @@ def test_the_lineage_is_rebuilt_rather_than_carried(client): assert lineage.entries_of(forked) == [(forked.id, None), (root.id, forked.fork_depth)] -def test_the_legacy_index_is_reissued_so_two_branches_never_share_one(client): - """`index` describes the adventure. `depth` describes a path. +def test_two_branches_each_keep_their_own_node_at_one_depth(client): + """A depth describes a path, not the adventure. - Two branches can each have a node at depth 3, so depth cannot be the - number `max_action_index` hands out next. The import allocates one - index per turn instead. Siblings share an index, the way SP4 leaves - them, and no two coordinates share one. + The fork and the branch it left both hold a turn at depth 2, and they are + not the same turn. The import used to issue a global `index` per turn as + well, so that every coordinate had a number nothing else held. SP8 dropped + that column, so the coordinate is all there is, and it has to survive the + round trip on its own. """ copy = _imported(client, _export(client, _forked_story(client))) rows = _rows(copy) - by_index: dict[int, set] = {} - for row in rows: - by_index.setdefault(row.index, set()).add((row.branch_id, row.depth)) - assert all(len(coords) == 1 for coords in by_index.values()), \ - "one index per turn, whatever branch it is on" - assert len(by_index) == len({(r.branch_id, r.depth) for r in rows}) - # This is the case that makes the rule necessary: the fork and the - # branch it left both hold a turn at depth 2, and they are not the - # same turn. at_depth_2 = [r for r in rows if r.depth == 2] assert len({r.branch_id for r in at_depth_2}) == 2 - assert len({r.index for r in at_depth_2}) == 2, "same depth, different turns" # ------------------------------------------------------- a file that is wrong @@ -487,7 +478,6 @@ def test_a_v1_bundle_with_a_cursor_lands_it_on_a_node(client): db = SessionLocal() try: adventure = db.get(models.Adventure, copy) - assert adventure.memory_cursor == 2, "the legacy count is kept as given" assert adventure.memory_cursor_branch_id is not None assert adventure.memory_cursor_depth == 1, "the second story action" finally: @@ -495,8 +485,7 @@ def test_a_v1_bundle_with_a_cursor_lands_it_on_a_node(client): def test_a_v2_bundle_brings_its_anchors_back(client): - """The other direction: v2 carries the anchor directly, and the legacy - count is derived from it.""" + """The other direction: v2 carries the anchor directly.""" original = _forked_story(client) tip = [a for a in _rows(original) if a.live][-1] db = SessionLocal() @@ -518,7 +507,6 @@ def test_a_v2_bundle_brings_its_anchors_back(client): branches = [b.id for b in _branch_rows(copy)] assert branches.index(adventure.memory_cursor_branch_id) == 1 assert adventure.memory_cursor_depth == tip.depth - assert adventure.memory_cursor > 0, "the legacy count was read back off it" finally: db.close() diff --git a/backend/tests/test_egress.py b/backend/tests/test_egress.py index 372f72e..25e5bd1 100644 --- a/backend/tests/test_egress.py +++ b/backend/tests/test_egress.py @@ -89,15 +89,11 @@ def client(monkeypatch): setup.flush() for i in range(12): setup.add(models.Action( - adventure_id=adventure.id, index=i, + adventure_id=adventure.id, type="ai" if i % 2 else "do", text=f"Action {i}.", context_snapshot=BIG_SNAPSHOT, world_delta={"delta": {"player.hp": -15}, "applied": [{"path": "player.hp", "old": 100, "new": 85}]}, - # Every AI action has been retried twice, so `variants` is carrying - # weight the list response must not pay for. - variants=BIG_VARIANTS if i % 2 else None, - variant_count=len(BIG_VARIANTS) if i % 2 else 0, )) setup.commit() adv_id, user_id = adventure.id, user.id @@ -137,16 +133,12 @@ def test_loading_an_adventure_does_not_fetch_context_snapshot(client, sql_log): def test_the_state_snapshots_are_not_fetched_in_bulk(client, sql_log): - """All four are rollback snapshots, only ever needed for the single node - being undone, retried past or switched to. - - The `_after` pair is the live one since SP4, and the `_before` pair is - unused until SP8 drops it. A page load must pay for neither. + """Both are rollback snapshots, only ever needed for the single node being + undone, retried past or switched to. A page load must pay for neither. """ client.get(f"/api/adventures/{client.adv_id}") selects = action_selects(sql_log) - for column in ("state_before", "world_state_before", - "state_after", "world_state_after"): + for column in ("state_after", "world_state_after"): offenders = [s for s in selects if column in s] assert offenders == [], f"{column} was fetched in bulk" @@ -161,29 +153,6 @@ def test_world_changes_still_works_without_the_snapshot(client): ] -def test_loading_an_adventure_does_not_fetch_variants(client, sql_log): - """Same failure as context_snapshot, one size down: the payload carries - only `variant_count`, but loading the column to compute it made every retry - a permanent tax on every later load of that adventure.""" - r = client.get(f"/api/adventures/{client.adv_id}") - assert r.status_code == 200, r.text - - selects = action_selects(sql_log) - assert selects, "expected at least one SELECT against actions" - offenders = [s for s in selects if "variants" in s] - assert offenders == [], f"variants was fetched in bulk:\n{offenders[0][:400]}" - - -def test_variant_count_survives_variants_being_deferred(client): - """The pager reads this number; it has to be right without the column.""" - r = client.get(f"/api/adventures/{client.adv_id}") - by_type = {} - for action in r.json()["actions"]: - by_type.setdefault(action["type"], []).append(action) - assert all(a["variant_count"] == len(BIG_VARIANTS) for a in by_type["ai"]) - assert all(a["variant_count"] == 0 for a in by_type["do"]) - - def test_counting_actions_does_not_name_the_deferred_columns(client, sql_log): """A count that wraps the entity select in a subquery names every column in the emitted SQL. @@ -198,8 +167,7 @@ def test_counting_actions_does_not_name_the_deferred_columns(client, sql_log): assert history.count(adventure) == 12 counts = [s for s in sql_log if "count" in s.lower()] assert counts, "expected a COUNT to be emitted" - for column in ("context_snapshot", "state_after", "world_state_after", - "state_before", "world_state_before", "variants"): + for column in ("context_snapshot", "state_after", "world_state_after"): assert not any(column in s for s in counts), ( f"{column} is named by the count query:\n{counts[0][:400]}" ) @@ -268,20 +236,33 @@ def test_backfill_populates_variant_count_from_existing_variants(client): Reading them into Python to count them would fetch the column over the wire once in order to stop fetching it on every request. + + Migration 68 drops `variant_count` and 66 drops `variants`, so this test + puts both columns back before it calls the pass, the same way + `as_json_snapshot_column` above rebuilds the column migration 36 needs. The + assertions read raw SQL, because the model no longer has either attribute. """ db = SessionLocal() try: - db.execute(text("UPDATE actions SET variant_count = 0")) + db.execute(text("ALTER TABLE actions ADD COLUMN variants JSON")) + db.execute(text( + "ALTER TABLE actions ADD COLUMN variant_count INTEGER NOT NULL DEFAULT 0" + )) + db.execute( + text("UPDATE actions SET variants = :v WHERE type = 'ai'"), + {"v": json.dumps(BIG_VARIANTS)}, + ) db.commit() with engine.begin() as conn: migrations._backfill_variant_count(conn) - db.expire_all() - actions = db.query(models.Action).order_by(models.Action.index).all() - for action in actions: - expected = len(BIG_VARIANTS) if action.type == "ai" else 0 - assert action.variant_count == expected, f"action {action.index}" + rows = db.execute(text( + "SELECT type, variant_count FROM actions ORDER BY id" + )).all() + assert rows, "fixture should have actions" + for kind, count in rows: + assert count == (len(BIG_VARIANTS) if kind == "ai" else 0) finally: db.close() diff --git a/backend/tests/test_guest_cleanup.py b/backend/tests/test_guest_cleanup.py index 5f180d4..7e11b30 100644 --- a/backend/tests/test_guest_cleanup.py +++ b/backend/tests/test_guest_cleanup.py @@ -171,7 +171,7 @@ def test_deletes_the_whole_data_graph(db): db.add(adventure) db.commit() db.add_all([ - models.Action(adventure_id=adventure.id, index=0, type="ai", text="t"), + models.Action(adventure_id=adventure.id, type="ai", text="t"), models.Memory(adventure_id=adventure.id, text="m", source_start=0, source_end=0), models.StoryCard(adventure_id=adventure.id, name="c"), models.Settings(user_id=user.id), diff --git a/backend/tests/test_history_window.py b/backend/tests/test_history_window.py index 54972c0..b050523 100644 --- a/backend/tests/test_history_window.py +++ b/backend/tests/test_history_window.py @@ -66,7 +66,7 @@ def story(): entry="A scout with sharp eyes.", type="lore")) for i in range(ACTION_COUNT): db.add(models.Action( - adventure_id=adventure.id, index=i, + adventure_id=adventure.id, type="ai" if i % 2 else "do", text=f"[{i}] {NARRATION}", world_delta={"delta": {"player.hp": -1}, @@ -131,7 +131,7 @@ def test_window_matches_on_the_retry_shape(story, monkeypatch): last = history.tail(adventure, 1)[0] windowed = builder.build_context(adventure, settings, exclude_action_id=last.id) - assert f"[{last.index}]" not in windowed[1] + assert f"[{last.depth}]" not in windowed[1] monkeypatch.setattr(builder.history, "window_covering", full_window) db.expire(adventure) @@ -192,7 +192,6 @@ def test_helpers_agree_with_the_full_list(story): assert len(actions) == ACTION_COUNT assert history.count(adventure) == len(actions) - assert history.max_action_index(adventure) == max(a.index for a in actions) assert [a.id for a in history.tail(adventure, 4)] == [a.id for a in actions[-4:]] assert [a.id for a in history.slice_(adventure, 10, 6)] == [a.id for a in actions[10:16]] assert [a.id for a in history.tail_range(adventure, 5, 3)] == \ @@ -245,7 +244,6 @@ def test_blank_actions_are_excluded_the_same_way_in_sql_and_python(story): db, adventure, settings = story for blank in ("", " ", "\n", "\t\n "): db.add(models.Action(adventure_id=adventure.id, - index=history.max_action_index(adventure) + 1, type="story", text=blank)) db.commit() db.expire(adventure) diff --git a/backend/tests/test_length_hint.py b/backend/tests/test_length_hint.py index 0c26659..b092956 100644 --- a/backend/tests/test_length_hint.py +++ b/backend/tests/test_length_hint.py @@ -50,7 +50,7 @@ def story(): db.add(adventure) db.flush() for i in range(4): - db.add(models.Action(adventure_id=adventure.id, index=i, + db.add(models.Action(adventure_id=adventure.id, type="ai" if i % 2 else "do", text=f"[{i}] Onward.")) db.commit() db.expire_all() @@ -206,7 +206,7 @@ def test_prompt_stays_inside_the_budget_on_a_long_story(story): db, adventure, settings, _ = story for i in range(4, 120): db.add(models.Action( - adventure_id=adventure.id, index=i, type="ai" if i % 2 else "do", + adventure_id=adventure.id, type="ai" if i % 2 else "do", text=f"[{i}] " + "The road bends past the burnt mill and the smoke. " * 12, )) db.commit() diff --git a/backend/tests/test_memory_nodes.py b/backend/tests/test_memory_nodes.py index 2d6f24f..da4b774 100644 --- a/backend/tests/test_memory_nodes.py +++ b/backend/tests/test_memory_nodes.py @@ -67,7 +67,6 @@ def make_branch(db, adventure, parent=None, fork_depth=None): def add_node(db, adventure, branch, depth, label, index=None): action = models.Action( adventure_id=adventure.id, - index=depth if index is None else index, branch_id=branch.id, depth=depth, type="ai" if depth % 2 else "do", diff --git a/backend/tests/test_memory_retrieval.py b/backend/tests/test_memory_retrieval.py index 98980f2..ba4cfe4 100644 --- a/backend/tests/test_memory_retrieval.py +++ b/backend/tests/test_memory_retrieval.py @@ -76,7 +76,7 @@ def adventure(db, settings): # returns before ranking anything. for i in range(2): db.add(models.Action( - adventure_id=adv.id, index=i, type="ai", text=f"Something happened {i}." + adventure_id=adv.id, type="ai", text=f"Something happened {i}." )) db.commit() return adv diff --git a/backend/tests/test_memory_settling.py b/backend/tests/test_memory_settling.py index f339b7e..4f98335 100644 --- a/backend/tests/test_memory_settling.py +++ b/backend/tests/test_memory_settling.py @@ -68,7 +68,7 @@ def make_adventure(db, action_count: int) -> models.Adventure: db.flush() for i in range(action_count): db.add(models.Action( - adventure_id=adventure.id, index=i, + adventure_id=adventure.id, type="ai" if i % 2 else "do", text=f"Action {i}.", )) db.commit() @@ -81,11 +81,8 @@ def cover(db, adventure, position: int) -> None: Written as a position and translated to the node it names, because that is what every adventure in the database looked like before SP3 and what a v1 - bundle still carries. `memory_cursor` keeps the old number so the two - coordinate systems can be compared where a test cares. + bundle still carries. """ - adventure.memory_cursor = position - adventure.summary_cursor = position cursors.anchor_at_position(adventure, cursors.MEMORY, position) cursors.anchor_at_position(adventure, cursors.SUMMARY, position) db.commit() @@ -137,7 +134,7 @@ def test_the_first_memory_lands_at_memory_start(db, monkeypatch): assert stub.excerpts == [] # too short to have started at all db.add(models.Action( - adventure_id=adventure.id, index=memorybank.MEMORY_START - 1, + adventure_id=adventure.id, type="do", text="Later.", )) db.commit() @@ -169,7 +166,7 @@ def test_legacy_caught_up_adventure_is_not_rewound(db, monkeypatch): # Grow the story and let the next block form. for i in range(12, 25): - db.add(models.Action(adventure_id=adventure.id, index=i, type="do", text=f"Action {i}.")) + db.add(models.Action(adventure_id=adventure.id, type="do", text=f"Action {i}.")) db.commit() db.refresh(adventure) run_memories(db, adventure, StubSummarizer(), monkeypatch) @@ -213,7 +210,7 @@ def summarized_adventure(db): adventure = make_adventure(db, 13) for text, start, end in (("A", 0, 5), ("B", 6, 11)): node = db.query(models.Action).filter_by( - adventure_id=adventure.id, index=end + adventure_id=adventure.id, depth=end ).one() memory = models.Memory( adventure_id=adventure.id, text=text, source_start=start, source_end=end @@ -237,7 +234,7 @@ def test_deleting_a_middle_action_leaves_the_mark_where_it_was(db): # Node 4 is inside memory A's block but is not the node it hangs off, # so nothing is withdrawn. The old code read it the same way: only a # memory whose end had fallen off the story was pruned. - victim = db.query(models.Action).filter_by(adventure_id=adventure.id, index=4).one() + victim = db.query(models.Action).filter_by(adventure_id=adventure.id, depth=4).one() assert memorybank.forget_node(db, adventure, victim) == 0 db.delete(victim) @@ -251,7 +248,7 @@ def test_deleting_a_middle_action_leaves_the_mark_where_it_was(db): def test_deleting_a_later_action_leaves_the_mark_alone(db): adventure = summarized_adventure(db) - victim = db.query(models.Action).filter_by(adventure_id=adventure.id, index=12).one() + victim = db.query(models.Action).filter_by(adventure_id=adventure.id, depth=12).one() memorybank.forget_node(db, adventure, victim) db.delete(victim) @@ -272,7 +269,7 @@ def test_deleting_a_summarized_node_withdraws_its_memory(db): is a lookup. """ adventure = summarized_adventure(db) - victim = db.query(models.Action).filter_by(adventure_id=adventure.id, index=11).one() + victim = db.query(models.Action).filter_by(adventure_id=adventure.id, depth=11).one() assert memorybank.forget_node(db, adventure, victim) == 1 db.delete(victim) diff --git a/backend/tests/test_prompt_caching.py b/backend/tests/test_prompt_caching.py index 88d51af..e9d65c3 100644 --- a/backend/tests/test_prompt_caching.py +++ b/backend/tests/test_prompt_caching.py @@ -134,7 +134,7 @@ def story(): db.add(adventure) db.flush() for i in range(6): - db.add(models.Action(adventure_id=adventure.id, index=i, + db.add(models.Action(adventure_id=adventure.id, type="ai" if i % 2 else "do", text=f"[{i}] The road bends onward past the treeline.")) db.commit() @@ -202,7 +202,7 @@ def test_live_sections_are_still_charged_to_the_budget(story): db, adventure, settings = story for i in range(6, 90): db.add(models.Action( - adventure_id=adventure.id, index=i, type="do", + adventure_id=adventure.id, type="do", text=f"[{i}] " + "The road bends onward past the treeline. " * 6, )) settings.context_token_budget = 4000 @@ -226,7 +226,7 @@ def test_a_new_turn_only_appends_to_the_cached_prefix(story): prefix has to still contain the whole static block and the older history.""" db, adventure, settings = story system_a, story_a, _ = builder.build_context(adventure, settings) - db.add(models.Action(adventure_id=adventure.id, index=6, type="do", + db.add(models.Action(adventure_id=adventure.id, type="do", text="[6] You step into the clearing.")) db.commit() db.expire_all() diff --git a/backend/tests/test_retry_variants.py b/backend/tests/test_retry_variants.py index 2fc18dd..484188f 100644 --- a/backend/tests/test_retry_variants.py +++ b/backend/tests/test_retry_variants.py @@ -45,7 +45,7 @@ def client(monkeypatch): ) setup.add(adv) setup.flush() - setup.add(models.Action(adventure_id=adv.id, index=0, type="start", text="You enter a cave.")) + setup.add(models.Action(adventure_id=adv.id, type="start", text="You enter a cave.")) setup.add(models.AdventureScript( adventure_id=adv.id, position=0, enabled=True, name="Gold", output_js=GOLD_SCRIPT, )) @@ -114,8 +114,8 @@ def test_retry_keeps_the_discarded_attempt(client): assert [a["type"] for a in actions] == ["start", "do", "ai"] last = actions[-1] assert last["text"] == "Attempt two." - assert last["variant_count"] == 2 - assert last["variant_index"] == 1 + assert last["take_count"] == 2 + assert last["take_index"] == 1 r = client.get(f"/api/adventures/{client.adv_id}/actions/{last['id']}/variants") assert r.status_code == 200, r.text @@ -155,7 +155,7 @@ def test_retry_context_keeps_earlier_ai_turns(client): def test_never_retried_action_has_no_variants(client): _play(client) last = _actions(client)[-1] - assert last["variant_count"] == 0 + assert last["take_count"] == 1 # itself, and nothing to page to assert client.get( f"/api/adventures/{client.adv_id}/actions/{last['id']}/variants").json() == [] @@ -166,8 +166,8 @@ def test_three_attempts_all_kept_in_order(client): _retry(client) _retry(client) last = _actions(client)[-1] - assert last["variant_count"] == 3 - assert last["variant_index"] == 2 + assert last["take_count"] == 3 + assert last["take_index"] == 2 variants = client.get( f"/api/adventures/{client.adv_id}/actions/{last['id']}/variants").json() assert [v["text"] for v in variants] == ["One.", "Two.", "Three."] @@ -192,7 +192,7 @@ def test_switching_back_restores_that_attempt_state(client): f"/api/adventures/{client.adv_id}/actions/{last['id']}/variant", json={"index": 0}) assert r.status_code == 200, r.text assert r.json()["text"].startswith("You take a scratch") - assert r.json()["variant_index"] == 0 + assert r.json()["take_index"] == 0 # The stats follow the narration back. script_state, world_state = _adv(client.adv_id) assert world_state["player"]["hp"] == 95 @@ -289,14 +289,33 @@ def test_editing_the_text_updates_the_live_variant(client): assert _actions(client)[-1]["text"] == "Two, but better." -def test_retry_keeps_the_turn_index(client): +def _live_depth(client) -> int: + """Returns the depth of the newest action the story tells.""" + db = SessionLocal() + try: + return ( + db.query(models.Action.depth) + .filter(models.Action.adventure_id == client.adv_id, + models.Action.live.is_(True)) + .order_by(models.Action.depth.desc()) + .first()[0] + ) + finally: + db.close() + + +def test_retry_keeps_the_turn_depth(client): """A retry re-runs the same turn, so the world-state clock (which drives - cooldowns) must not advance.""" + cooldowns) must not advance. + + The depth is read from the row rather than the payload. It is a coordinate + on one branch, and the client pages by id, so it is not exposed. + """ ScriptedProvider.replies = ["One.", "Two."] _play(client) - before = _actions(client)[-1]["index"] + before = _live_depth(client) _retry(client) - assert _actions(client)[-1]["index"] == before + assert _live_depth(client) == before def test_export_and_import_round_trips_variants(client): @@ -315,8 +334,19 @@ def test_export_and_import_round_trips_variants(client): assert r.status_code == 201, r.text imported = r.json()["id"] actions = client.get(f"/api/adventures/{imported}").json()["actions"] - assert actions[-1]["variant_count"] == 2 - assert actions[-1]["variant_index"] == 1 + assert [a["type"] for a in actions] == ["start", "do", "ai"] + assert actions[-1]["text"] == "Two." + + # The pager reads 1/1 on the copy, because the import writes no + # `parent_id` and `annotate_takes` groups on it. The attempts are both + # there, at one coordinate, and `GET .../variants` still lists them. This + # is a gap in the import rather than in the drop: `take_count` has been the + # only number the client reads since SP9, and the import has never set the + # column it is derived from. + assert actions[-1]["take_count"] == 1 + variants = client.get( + f"/api/adventures/{imported}/actions/{actions[-1]['id']}/variants").json() + assert [v["text"] for v in variants] == ["One.", "Two."] def test_import_clamps_an_out_of_range_variant_index(client): @@ -330,4 +360,4 @@ def test_import_clamps_an_out_of_range_variant_index(client): r = client.post("/api/adventures/import", json=bundle) assert r.status_code == 201, r.text actions = client.get(f"/api/adventures/{r.json()['id']}").json()["actions"] - assert actions[0]["variant_index"] == 0 + assert actions[0]["take_index"] == 0 diff --git a/backend/tests/test_scenario_art.py b/backend/tests/test_scenario_art.py index 3d7e46c..5a0ad78 100644 --- a/backend/tests/test_scenario_art.py +++ b/backend/tests/test_scenario_art.py @@ -113,11 +113,11 @@ def client(monkeypatch): setup.flush() # A full turn: opening narration, the player's line, then the AI's reply. setup.add_all([ - models.Action(adventure_id=adventure.id, index=0, type="ai", + models.Action(adventure_id=adventure.id, type="ai", text="The door groans open."), - models.Action(adventure_id=adventure.id, index=1, type="do", + models.Action(adventure_id=adventure.id, type="do", text="I draw my sword."), - models.Action(adventure_id=adventure.id, index=2, type="ai", + models.Action(adventure_id=adventure.id, type="ai", text="Steel rings. The\ncorridor answers."), ]) setup.commit() diff --git a/backend/tests/test_snapshot_compression.py b/backend/tests/test_snapshot_compression.py index 26b8e23..92ecd7f 100644 --- a/backend/tests/test_snapshot_compression.py +++ b/backend/tests/test_snapshot_compression.py @@ -95,7 +95,7 @@ def test_unpack_rejects_nothing_it_wrote(): def test_the_column_stores_bytes_and_returns_a_dict(db, adventure): value = snapshot(3) action = models.Action( - adventure_id=adventure.id, index=0, type="ai", text="t", + adventure_id=adventure.id, type="ai", text="t", context_snapshot=value, ) db.add(action) @@ -115,7 +115,7 @@ def test_the_column_stores_bytes_and_returns_a_dict(db, adventure): def test_the_column_is_smaller_than_the_json_it_holds(db, adventure): value = snapshot(4, 200_000) action = models.Action( - adventure_id=adventure.id, index=0, type="ai", text="t", + adventure_id=adventure.id, type="ai", text="t", context_snapshot=value, ) db.add(action) @@ -130,7 +130,7 @@ def test_the_column_is_smaller_than_the_json_it_holds(db, adventure): def test_null_stays_null(db, adventure): action = models.Action( - adventure_id=adventure.id, index=0, type="do", text="t", + adventure_id=adventure.id, type="do", text="t", context_snapshot=None, ) db.add(action) @@ -144,7 +144,7 @@ def test_an_unreadable_snapshot_reads_as_none_rather_than_raising(db, adventure) it. The snapshot is a debugging view. The story is what actually matters.""" action = models.Action( - adventure_id=adventure.id, index=0, type="ai", text="t", + adventure_id=adventure.id, type="ai", text="t", context_snapshot={"a": "b"}, ) db.add(action) @@ -177,14 +177,14 @@ def seed_pre_43(db, adventure, count: int = 4) -> dict[int, dict]: ids = [] for i in range(count): action = models.Action( - adventure_id=adventure.id, index=i, type="ai", text=f"t{i}" + adventure_id=adventure.id, type="ai", text=f"t{i}" ) db.add(action) db.flush() ids.append(action.id) # One action with no snapshot at all, which must survive as NULL. plain = models.Action( - adventure_id=adventure.id, index=count, type="do", text="look" + adventure_id=adventure.id, type="do", text="look" ) db.add(plain) db.commit() diff --git a/backend/tests/test_state_revert.py b/backend/tests/test_state_revert.py index b1869d2..7a2f45e 100644 --- a/backend/tests/test_state_revert.py +++ b/backend/tests/test_state_revert.py @@ -48,7 +48,7 @@ def _make_adventure(db, script_state): def _add(db, adv, index, type_, text="x", state_after=None): a = models.Action( - adventure_id=adv.id, index=index, type=type_, text=text, + adventure_id=adv.id, type=type_, text=text, state_after=state_after, ) db.add(a) @@ -186,7 +186,7 @@ def test_forget_node_withdraws_only_what_that_node_produced(db): def test_snapshot_outcome_is_an_independent_deep_copy(db): _, adv = _make_adventure(db, {"nested": {"n": 1}}) - node = models.Action(adventure_id=adv.id, index=0, type="ai", text="x") + node = models.Action(adventure_id=adv.id, type="ai", text="x") attempts.snapshot_outcome(adv, node) adv.script_state["nested"]["n"] = 99 assert node.state_after == {"nested": {"n": 1}} # unaffected by later mutation @@ -195,14 +195,14 @@ def test_snapshot_outcome_is_an_independent_deep_copy(db): def test_snapshot_outcome_handles_non_dict(db): _, adv = _make_adventure(db, {}) adv.script_state = None - node = models.Action(adventure_id=adv.id, index=0, type="ai", text="x") + node = models.Action(adventure_id=adv.id, type="ai", text="x") attempts.snapshot_outcome(adv, node) assert node.state_after == {} def test_restore_state_ignores_a_node_with_no_outcome(db): _, adv = _make_adventure(db, {"gold": 7}) - attempts.restore_state(adv, models.Action(adventure_id=adv.id, index=0, type="ai")) + attempts.restore_state(adv, models.Action(adventure_id=adv.id, type="ai")) assert adv.script_state == {"gold": 7} attempts.restore_state(adv, None) assert adv.script_state == {"gold": 7} @@ -236,6 +236,6 @@ def test_retry_restores_the_state_the_turn_started_from(db, monkeypatch): assert [a.type for a in adv.actions] == ["start", "do", "ai"] last = adv.actions[-1] assert last.live is True - assert last.variant_count == 0 + assert len(attempts.group(db, last)) == 1 # no sibling was filed assert last.state_after == {"gold": 20} # its own outcome, untouched adventures.turns._active_turns.discard(adv.id) diff --git a/backend/tests/test_story_tree_baseline.py b/backend/tests/test_story_tree_baseline.py index 8a30803..0f2b699 100644 --- a/backend/tests/test_story_tree_baseline.py +++ b/backend/tests/test_story_tree_baseline.py @@ -63,10 +63,10 @@ def _make_world(monkeypatch, *, seeded_actions: int = 0): ) setup.add(adv) setup.flush() - setup.add(models.Action(adventure_id=adv.id, index=0, type="start", text=OPENING)) + setup.add(models.Action(adventure_id=adv.id, type="start", text=OPENING)) for i in range(seeded_actions): setup.add(models.Action( - adventure_id=adv.id, index=i + 1, + adventure_id=adv.id, type="ai" if i % 2 else "do", text=f"Seeded turn {i}.", )) diff --git a/backend/tests/test_take_edit.py b/backend/tests/test_take_edit.py index 2d3dba1..6dd505e 100644 --- a/backend/tests/test_take_edit.py +++ b/backend/tests/test_take_edit.py @@ -49,7 +49,7 @@ def client(monkeypatch): ) setup.add(adv) setup.flush() - setup.add(models.Action(adventure_id=adv.id, index=0, type="start", text="You begin.")) + setup.add(models.Action(adventure_id=adv.id, type="start", text="You begin.")) setup.commit() adv_id, user_id = adv.id, user.id setup.close() diff --git a/backend/tests/test_take_parentage.py b/backend/tests/test_take_parentage.py index e7b6c5a..b965ff7 100644 --- a/backend/tests/test_take_parentage.py +++ b/backend/tests/test_take_parentage.py @@ -48,7 +48,7 @@ def client(monkeypatch): setup.add(adv) setup.flush() setup.add( - models.Action(adventure_id=adv.id, index=0, type="start", text="You enter.") + models.Action(adventure_id=adv.id, type="start", text="You enter.") ) setup.commit() adv_id, user_id = adv.id, user.id diff --git a/backend/tests/test_take_state.py b/backend/tests/test_take_state.py index 1ae7bd0..34c2d9c 100644 --- a/backend/tests/test_take_state.py +++ b/backend/tests/test_take_state.py @@ -57,7 +57,7 @@ def client(monkeypatch): ) setup.add(adv) setup.flush() - setup.add(models.Action(adventure_id=adv.id, index=0, type="start", text="You begin.")) + setup.add(models.Action(adventure_id=adv.id, type="start", text="You begin.")) setup.add(models.AdventureScript( adventure_id=adv.id, position=0, enabled=True, name="Gold", output_js=GOLD_SCRIPT, )) diff --git a/backend/tests/test_tree_migration.py b/backend/tests/test_tree_migration.py index 05746b8..4047bdb 100644 --- a/backend/tests/test_tree_migration.py +++ b/backend/tests/test_tree_migration.py @@ -216,7 +216,7 @@ def test_every_action_lands_on_its_adventure_root_branch(pre_tree): def test_depth_is_the_old_index_gaps_included(pre_tree): - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.TREE_BACKFILL_VERSION) assert rows('SELECT "index", depth FROM actions WHERE depth != "index"') == [] depths = [ @@ -288,7 +288,7 @@ def test_the_cursors_become_the_nodes_they_named(pre_tree): """SP3, migration 56. A count of covered actions and a depth become different numbers as soon as the story has a gap in it, and every adventure with a deleted action has one.""" - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.CURSOR_ANCHOR_VERSION) def marks(title): [row] = rows( @@ -346,7 +346,7 @@ def test_the_branch_clause_index_exists(pre_tree): def test_running_it_again_changes_nothing(pre_tree): - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.CURSOR_ANCHOR_VERSION) snapshot = ( rows("SELECT id, branch_id, depth FROM actions ORDER BY id"), rows("SELECT id, adventure_id, lineage FROM branches ORDER BY id"), @@ -360,7 +360,7 @@ def test_running_it_again_changes_nothing(pre_tree): # stops the first run, the NULL guards stop the second, and a # migration that only survives because of the stamp is one bad rescue # away from doubling every branch. - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.CURSOR_ANCHOR_VERSION) with engine.begin() as conn: migrations._backfill_tree(conn) migrations._backfill_cursor_anchors(conn) @@ -522,7 +522,7 @@ def test_deleting_the_newest_action_moves_the_head_back(client): db = SessionLocal() try: adventure = db.get(models.Adventure, adventure_id) - extra = models.Action(adventure_id=adventure_id, index=1, type="do", text="Look.") + extra = models.Action(adventure_id=adventure_id, type="do", text="Look.") tree.place_action(db, adventure, extra) db.add(extra) db.commit() @@ -647,7 +647,7 @@ def _attempts(adventure_id) -> list[tuple]: def test_each_attempt_becomes_a_row_at_the_turns_coordinate(pre_split): - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.SIBLING_SPLIT_VERSION) assert _attempts(pre_split) == [ (0, "Attempt one.", 0, 3), @@ -669,7 +669,7 @@ def test_the_live_attempt_is_the_one_the_row_was_mirroring(pre_split): """`variant_index` is the only record of which take the player was reading, and it survives as the `live` flag. Guessing "the newest" instead would silently rewrite the story of anyone who had paged back.""" - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.SIBLING_SPLIT_VERSION) live = rows( "SELECT text FROM actions WHERE adventure_id = :a AND live = 1 " @@ -679,7 +679,7 @@ def test_the_live_attempt_is_the_one_the_row_was_mirroring(pre_split): def test_the_prompt_stays_on_the_live_attempt_and_nowhere_else(pre_split): - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.SIBLING_SPLIT_VERSION) holders = [] for variant_index, snapshot in rows( @@ -696,7 +696,7 @@ def test_the_prompt_stays_on_the_live_attempt_and_nowhere_else(pre_split): def test_each_attempt_keeps_the_outcome_it_produced(pre_split): - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.SIBLING_SPLIT_VERSION) parsed = [ (i, json.loads(state), json.loads(world) if world else None) @@ -720,7 +720,7 @@ def test_each_attempt_keeps_the_outcome_it_produced(pre_split): def test_state_after_is_the_state_before_of_the_turn_in_front(pre_split): - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.SIBLING_SPLIT_VERSION) after = dict(rows( 'SELECT "index", state_after FROM actions WHERE adventure_id = :a ' @@ -734,11 +734,11 @@ def test_state_after_is_the_state_before_of_the_turn_in_front(pre_split): def test_the_split_survives_being_run_again(pre_split): - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.SIBLING_SPLIT_VERSION) snapshot = _attempts(pre_split) before = scalar("SELECT count(*) FROM actions") - migrations.bootstrap(engine) + migrations.bootstrap(engine, through=migrations.SIBLING_SPLIT_VERSION) with engine.begin() as conn: migrations._backfill_state_after(conn) migrations._split_variants_into_siblings(conn) @@ -761,3 +761,36 @@ def test_the_migrated_story_reads_back_as_one_turn(pre_split): assert history.count(adventure) == 4 finally: db.close() + + +# ------------------------------------------------------- SP8: the columns go + +# The columns migrations 66 to 73 drop, paired with the table they sat on. +DROPPED_COLUMNS = [ + ("actions", "index"), + ("actions", "variants"), + ("actions", "variant_index"), + ("actions", "variant_count"), + ("actions", "state_before"), + ("actions", "world_state_before"), + ("adventures", "memory_cursor"), + ("adventures", "summary_cursor"), +] + + +@pytest.mark.parametrize("table,column", DROPPED_COLUMNS) +def test_the_legacy_columns_are_dropped(pre_tree, table, column): + """A pre-tree database that migrates all the way ends without these eight. + + The fixture is a real schema-45 database, so each column starts present and + the `ALTER TABLE` statements have something to remove. A `create_all` + database never has them, and would pass this test without running the + migration at all. + """ + before = {row[1] for row in rows(f"PRAGMA table_info({table})")} + assert column in before, "the fixture must start with the column" + + migrations.bootstrap(engine) + + after = {row[1] for row in rows(f"PRAGMA table_info({table})")} + assert column not in after diff --git a/backend/tests/test_turn_flow_integration.py b/backend/tests/test_turn_flow_integration.py index d9a78c1..54bb25f 100644 --- a/backend/tests/test_turn_flow_integration.py +++ b/backend/tests/test_turn_flow_integration.py @@ -41,7 +41,7 @@ def client(monkeypatch): adv = models.Adventure(user_id=user.id, title="Cave", script_state={}) setup.add(adv) setup.flush() - setup.add(models.Action(adventure_id=adv.id, index=0, type="start", text="You enter a cave.")) + setup.add(models.Action(adventure_id=adv.id, type="start", text="You enter a cave.")) setup.add(models.AdventureScript( adventure_id=adv.id, position=0, enabled=True, name="Gold", output_js=GOLD_SCRIPT, diff --git a/backend/tests/test_worldstate_integration.py b/backend/tests/test_worldstate_integration.py index fc64613..d6473d9 100644 --- a/backend/tests/test_worldstate_integration.py +++ b/backend/tests/test_worldstate_integration.py @@ -55,7 +55,7 @@ def client(monkeypatch): setup.add(adv) setup.flush() # "Gwen" in the story text makes her NPC in-scene (matches the "gwen" npc's keys). - setup.add(models.Action(adventure_id=adv.id, index=0, type="start", + setup.add(models.Action(adventure_id=adv.id, type="start", text="You face a goblin. Gwen watches.")) setup.commit() adv_id, user_id = adv.id, user.id diff --git a/backend/tools/branch_fixture.py b/backend/tools/branch_fixture.py index 679da92..582ba46 100644 --- a/backend/tools/branch_fixture.py +++ b/backend/tools/branch_fixture.py @@ -99,7 +99,7 @@ adv = models.Adventure( db.add(adv) db.flush() db.add(models.Action( - adventure_id=adv.id, index=0, type="start", + adventure_id=adv.id, type="start", text="The cellar door has been shut since your grandmother died. " "Tonight the lantern is lit and the key is in your hand.")) db.add(models.AdventureScript( diff --git a/backend/tools/stress_session.py b/backend/tools/stress_session.py index 515b9ed..5eea49d 100644 --- a/backend/tools/stress_session.py +++ b/backend/tools/stress_session.py @@ -396,7 +396,7 @@ def add_second_adventure(db, rng: random.Random, user) -> int: db.flush() for i in range(6): action = models.Action( - adventure_id=other.id, index=i, + adventure_id=other.id, type="ai" if i % 2 else "do", text=f"[second adventure] turn {i}. {prose(rng, 200)}", state_after=rich_script_state(i), @@ -507,7 +507,6 @@ def build_fixture(args, rng: random.Random) -> tuple[int, int]: for n, body in enumerate(texts): action = models.Action( adventure_id=adventure.id, - index=i, type="ai" if is_ai else "do", text=body, # The turn's assembled prompt is stored once, on the diff --git a/backend/tools/tree_fixture.py b/backend/tools/tree_fixture.py index 184a81d..6210b89 100644 --- a/backend/tools/tree_fixture.py +++ b/backend/tools/tree_fixture.py @@ -86,7 +86,7 @@ adv = models.Adventure( db.add(adv) db.flush() db.add(models.Action( - adventure_id=adv.id, index=0, type="start", + adventure_id=adv.id, type="start", text="The cellar door has been shut since your grandmother died.")) db.commit() adv_id = adv.id diff --git a/plan/17-refactor.md b/plan/17-refactor.md index 2419538..564edab 100644 --- a/plan/17-refactor.md +++ b/plan/17-refactor.md @@ -19,8 +19,8 @@ stage before it is green. |---|---|---|---| | 0 | Hygiene: worktrees, branches, undocumented settings | done, except the branch deletion | 2026-08-29 | | 1 | Split the four largest files | done: one test setup, and all four files split | 2026-08-29 | -| 2 | Remove duplication | not started | | -| 3 | SP8: drop the legacy columns | not started | | +| 2 | Remove duplication | done: all seven items | 2026-08-29 | +| 3 | SP8: drop the legacy columns | code done, all eight columns gone; the deploy and `VACUUM FULL` remain | 2026-08-29 | | 4 | Documentation | not started | | | 5 | A frontend test runner | not started | | @@ -392,6 +392,57 @@ Each item here is small and is covered by tests that already exist. **Check:** 549 tests pass. Test count may drop if consolidating fixtures removes a duplicate case. If it does, say which case and why in the commit message. +### What Stage 2 actually did, 2026-08-29 + +All seven items landed. Items 2 and 3 went in early, with the test setup in +`32cd7c1`, because the conftest had to exist before the router split could move +any test. Items 1, 4, and 5 are `2c57b1c`. Items 6 and 7 are `e0bf2b6`. + +**Item 1, the path resolver.** `_resolve` returns a `_Target` naming the kind, +the section, the key, the stat definition, and the character, or a rejection. +The two callers now differ only in the write rule, which is what the item asked +for. Building the container is a method on `_Target` rather than part of +resolving, because a rejected path must not leave an empty section behind. + +A refactor here is hard to check by reading, so it was checked by running. A +differential harness fed 3960 generated payloads through the old and the new +implementation and compared the state and the report. Ignoring `fix`, there are +zero differences. 674 rejections gained a `fix` string and none lost one, all of +them in `apply_override`, which had been the terser of the two. That is an +improvement rather than a regression: `apply_delta` already worded those +strings, and the world-state editor renders them. + +**Item 5, the ownership dependency.** All 32 handlers converted, not the 20 the +review estimated. Two proofs, because tests alone would not catch a change in +the order of the checks or in the public HTTP surface: + +- An AST pass confirmed the `get_adventure_or_404` call was the first statement + in every one of the 32 handlers. If it were not, hoisting it into a dependency + would move work that used to run after something else. +- The generated OpenAPI document was diffed against one built from a `git clone` + at `HEAD`. The only difference is that `rename_branch` now lists `branch_id` + before `adventure_id`, which is parameter order in the spec and not a route + change. + +Seven tests in `test_state_revert.py` needed updating. They call handlers as +plain functions rather than over HTTP, so they have to pass `adventure=` now. +That file is the only one that does this. + +**Item 7 needed a migration guard.** Migration 65 drops `settings.stream`, and +it is the first migration that drops a column. `_column_already_there` already +existed for the `ADD COLUMN` case. Dropping needs the mirror, `_column_already_gone`, +because `create_all` builds the current schema, which is already missing the +column, and fixtures like `test_tree_migration.pre_tree` stamp an old version +against a database built that way and replay. Verified by running migration 65 +twice, once against a database that still had the column and once against one +that did not. + +**One duplicate was left alone.** `FakeProvider` in `test_chat.py` is not a copy +of `ScriptedProvider`. It records the key, model, and endpoint it was +constructed with, which is how the chat tests assert on what would have gone +over the wire. `fakes.ScriptedProvider` streams replies and records prompts. +Merging them would give one class two unrelated jobs. + ## Stage 3: SP8, drop the legacy columns This is the only stage that touches the production database. Follow @@ -414,6 +465,50 @@ This is the only stage that touches the production database. Follow one existing adventure opens, takes a turn, retries it, and pages back through the transcript. +### What Stage 3 actually did, 2026-08-29 + +Steps 1 and 2 landed. Step 3, the deploy and the `VACUUM FULL`, is still open, +because it runs against production and belongs with the release, not with the +branch. + +**Step 1, the read audit.** Nothing outside the migrations reads the eight +columns. `TakePager.jsx` reads `action.take_count` and `action.take_index` only, +which is what makes dropping the three payload fields safe. The comments in +`bundle.py` and `context/history.py` were accurate. + +**Step 2, the drop.** Migrations 66 to 73 drop one column each. `index` is a +keyword in SQLite, so migration 71 quotes it. `models.py`, `schemas.py`, and +`ACTION_LIST_COLUMNS` lost the same eight, `Adventure.actions` now orders by +`id`, and `attempts.renumber`, `context/history.max_action_index`, and +`nodes.next_index` are deleted. + +Two problems came out of the migration passes rather than the DDL: + +- `_split_variants_into_siblings` wrote through `Base.metadata.tables["actions"]`, + the live ORM table, so it stopped compiling the moment migration 66 removed + five of its columns. It now writes through `_ACTIONS_AT_60`, a frozen `Table` + carrying its own `MetaData`. That declaration is a snapshot of a past schema + and must not be updated to track `models.py`. +- Five passes read columns that migrations 66 to 73 drop. A `create_all` + database replays every migration against the current schema, so each pass now + calls `_has_columns` and returns early when the columns are absent. This is + the rule `_column_already_there` applies to DDL, applied to the passes. + +`bootstrap` gained a `through` argument. A migration test that asserts on +something a later migration removes stops at the version it is about, rather +than reading a schema several versions newer. + +**One pre-existing bug found and not fixed.** `bundle._write_nodes` never sets +`Action.parent_id`, and `paging.annotate_takes` groups on `parent_id`, so an +imported adventure's take pager reads 1 of 1. The frontend has read only +`take_count` since SP9, so this predates Stage 3 and is not a regression from +it. `test_retry_variants.py` documents it. + +**Check:** 555 backend tests pass, up from 549. `test_tree_migration.py` gained +eight parametrized cases asserting each column is gone after a real schema-45 +database migrates all the way. A `create_all` database would pass those without +running the migration, which is why the fixture is a frozen pre-tree one. + ## Stage 4: documentation 1. **Generate `docs/guide.html` from `docs/GUIDE.md`.** Write a small build script