Answer the review, and keep the opening node's bank

Nine findings from a review of the phase-14 stack. The one about a retry
withdrawing a memory is not a bug — a memory anchored to a node describes that
node, and it goes when the node goes. The root is the exception, and it is the
only one: migration 62 parked every memory written before memories had
coordinates on depth 0, so withdrawing the opening node would retire a whole
bank nobody attached there. A memory with no source range covers no story and
now stays; a summary that genuinely ends there is still withdrawn.

The rest are repairs.

* The adventure list quoted whichever attempt was written last rather than the
  one the story tells, so switching back left the index disagreeing with the
  page.
* A v1 import gave a typed memory no depth, rebuilding the NULL the migration
  exists to remove — invisible until the imported adventure forked.
* The action cap counted a v1 file's turns, and a turn expands into a row per
  saved attempt, so a file inside the cap could write a multiple of it.
* Forking a live node on a borrowed ancestor promoted a sibling on a branch the
  caller never named. It is a branch switch, and now says so.
* Switching attempts left the state, status and memory panels reading the
  previous take: the story does not change length, so nothing keyed on its
  length noticed. Same class as the branch-switch bug this phase already fixed.
* A retry after switching back numbered the new attempt into the middle of the
  group instead of the end.
* The cursor backfill numbered every action in the table once per adventure;
  correlated to the adventure being updated, it is an index lookup instead.
* Renaming a branch answered own_actions=0.

And one behaviour change recorded rather than repaired: script-visible history
and actionCount no longer count blank-text rows. That is the right shape and
there is no reading compatible with both, so plan/14 says so.

409 tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015H5qiyiR7gtFQaoDphHZ3g

backend/app/attempts.py

@@ -150,7 +150,17 @@ def add_attempt(
     replacement.branch_id = previous.branch_id
     replacement.depth = previous.depth
     replacement.live = True
-    replacement.variant_index = previous.variant_index + 1
+    # The end of the group, not one past `previous` — which is only the same
+    # thing when `previous` is the newest take. Switch a three-take turn back to
+    # take 1 and retry, and `previous.variant_index + 1` collides with take 2;
+    # `renumber` then breaks the tie by id and files the new attempt *between*
+    # takes 2 and 3, so the pager walks the takes in an order they were not made
+    # in. `group` is 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,
+    )
     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

backend/app/bundle.py

@@ -545,6 +545,16 @@ def _write_memories(
         # last action it summarises, which on one branch is that node's depth.
         if memory.depth is None and memory.source_end is not None:
             memory.depth = memory.source_end
+        # A v1 memory that summarises nothing — one the player typed — has no
+        # depth to derive, and leaving it NULL here would rebuild by import the
+        # exact state migration 62 exists to end: `Path._entry_clause` compares
+        # `depth <= max_depth`, which a NULL fails, so the memory would vanish
+        # from every branch the moment the imported adventure was forked. The
+        # root is the same answer the migration gives, and for the same reason
+        # — 0 is at or before every fork point, so it is visible from every
+        # path this adventure can grow.
+        if memory.depth is None:
+            memory.depth = lineage.ROOT_DEPTH
         db.add(memory)
 
 

backend/app/context/lineage.py

@@ -41,6 +41,14 @@ from .. import models
 # separately so a read never has to import the write half.
 NO_DEPTH = -1
 
+# The opening node of an adventure. Depth 0 exists only on the root branch — a
+# fork starts its own nodes after the depth it forked at — so this names one
+# node per adventure, not one per branch. It is also where migration 62 parked
+# every memory written before memories had coordinates, which is why the two
+# places that can retire a memory (`memorybank.forget_node`, and a v1 import
+# with no depth to read) both have to say something about it.
+ROOT_DEPTH = 0
+
 
 def entries_of(branch: models.Branch) -> list[tuple[int, int | None]]:
     """`branch.lineage` as (branch_id, max_depth) pairs, newest first.

backend/app/memorybank.py

@@ -171,6 +171,9 @@ def forget_node(db: Session, adventure: models.Adventure, action: models.Action)
     the node before it, which is a depth whether or not anything still sits
     there.
 
+    The opening node is the one exception, because migration 62 parked the
+    whole pre-coordinate bank on it — see the comment on `lineage.ROOT_DEPTH`.
+
     Returns how many memories were withdrawn.
     """
     if action.branch_id is None or action.depth is None:
@@ -184,6 +187,17 @@ def forget_node(db: Session, adventure: models.Adventure, action: models.Action)
         )
         .all()
     )
+    if action.depth == lineage.ROOT_DEPTH:
+        # The opening node is special, and only for memories that describe no
+        # stretch of story. Migration 62 parked every memory written before
+        # memories had coordinates at depth 0 — that was the choice that took
+        # nothing away from anybody, but it also collected them all onto one
+        # node, so withdrawing that node would retire a player's whole bank in
+        # a single click. A memory with no `source_start` was typed (or
+        # migrated), describes nothing that can fall off the end, and so has
+        # nothing to be withdrawn *from*: it stays. A summary that genuinely
+        # ends here is still withdrawn, because the text it describes is going.
+        doomed = [m for m in doomed if m.source_start is not None]
     if not doomed:
         return 0
     starts = [m.source_start for m in doomed if m.source_start is not None]

backend/app/migrations.py

@@ -585,6 +585,17 @@ def _backfill_cursor_anchors(conn) -> None:
     Guarded on `_depth = -1` so a run that dies halfway resumes: every
     adventure this has already converted is skipped, and one it has not is
     indistinguishable from an untouched row.
+
+    **The row numbering is correlated, not ranked-then-filtered.** Numbering
+    every action in the table and picking one row out of the result reads the
+    whole of `actions` per adventure — the window function is what stops the
+    correlation being pushed down, so the planner has no way to make it cheaper
+    — and this runs inside the one transaction that holds the schema, at boot,
+    against a database with real stories in it. Restricting the scan to the
+    adventure being updated makes each pass an index lookup on
+    `actions.adventure_id` instead, and `PARTITION BY` is then a partition of
+    one. The two forms give the same answer for the same reason: the rows the
+    partition would have separated are exactly the rows the filter removes.
     """
     sqlite = conn.dialect.name == "sqlite"
     story = _story_text_sql("text", sqlite)
@@ -594,13 +605,14 @@ def _backfill_cursor_anchors(conn) -> None:
             SET {name}_cursor_branch_id = {_root_branch_of('adventures.id')},
                 {name}_cursor_depth = COALESCE(
                     (SELECT ranked.depth FROM (
-                        SELECT adventure_id, depth, ROW_NUMBER() OVER (
-                            PARTITION BY adventure_id ORDER BY depth, id
+                        SELECT depth, ROW_NUMBER() OVER (
+                            ORDER BY depth, id
                         ) AS rn
-                        FROM actions WHERE {story}
+                        FROM actions
+                        WHERE {story}
+                          AND actions.adventure_id = adventures.id
                      ) AS ranked
-                     WHERE ranked.adventure_id = adventures.id
-                       AND ranked.rn = adventures.{name}_cursor),
+                     WHERE ranked.rn = adventures.{name}_cursor),
                     (SELECT MAX(a.depth) FROM actions a
                       WHERE a.adventure_id = adventures.id
                         AND {_story_text_sql('a.text', sqlite)}),

backend/app/routers/adventures.py

@@ -203,6 +203,13 @@ def _latest_narration(db: Session, head_branches: dict[int, int | None]) -> dict
             models.Action.adventure_id.in_(list(head_branches)),
             models.Action.branch_id.in_(branch_ids),
             models.Action.type.in_(NARRATION_TYPES),
+            # Siblings share a depth and the newest of them has the highest id,
+            # so without this the snippet quotes whichever attempt was written
+            # last rather than the one the story tells. Switching back to an
+            # earlier take would leave the index screen quoting the discarded
+            # one — the story on the screen and the story in the list would
+            # disagree, and only the list would be wrong.
+            models.Action.live.is_(True),
         )
         .subquery()
     )
@@ -1150,14 +1157,17 @@ def rename_branch(
     adventure.updated_at = models.utcnow()
     db.commit()
     db.refresh(branch)
-    tip = (
-        db.query(func.max(models.Action.depth))
+    # Both numbers in the one pass, and counted the way `list_branches` counts
+    # them — live rows on this branch. A renamed branch is the same branch, so
+    # the row this hands back has to be the row the panel would have fetched.
+    tip, own = (
+        db.query(func.max(models.Action.depth), func.count(models.Action.id))
         .filter(
             models.Action.adventure_id == adventure.id,
             models.Action.branch_id == branch.id,
             models.Action.live.is_(True),
         )
-        .scalar()
+        .one()
     )
     return schemas.BranchOut(
         id=branch.id,
@@ -1166,7 +1176,7 @@ def rename_branch(
         depth=tip if tip is not None else (
             branch.fork_depth if branch.fork_depth is not None else tree.NO_DEPTH
         ),
-        own_actions=0,
+        own_actions=own,
         is_head=(branch.id == adventure.head_branch_id),
         name=branch.name,
         created_at=branch.created_at,
@@ -1342,8 +1352,26 @@ def fork_from_attempt(
     # attempt alone on its branch: a client that repeats the call — a double
     # click, a retried request — must get the same answer, not a complaint that
     # the turn it just forked has nothing to fork to.
-    if action.live and action.branch_id == adventure.head_branch_id:
+    if action.live:
+        # A live node already *is* what its coordinate says, so there is no
+        # attempt here to take. On the path being read that is simply a no-op,
+        # and it has to stay one: a client that repeats the call — a double
+        # click, a retried request — must get the same answer, not a complaint
+        # that the turn it just forked has nothing to fork to. Off the path it
+        # is a different line's story, and moving there is a branch switch.
+        #
+        # The membership test is the whole lineage, not `head_branch_id`. A
+        # head borrows its ancestors' turns, so a live node on an ancestor is
+        # already being read; forking it would move the live row off the parent
+        # and promote a sibling in its place, rewriting the story on a branch
+        # nobody asked about *and* on this one, which borrows that depth.
+        if lineage.path_of(db, adventure).contains(action):
             return current_window(db, adventure)
+        raise HTTPException(
+            400,
+            "That take is already the story on another branch. Switch to that "
+            "branch to read it.",
+        )
     if len(attempts.group(db, action)) < 2:
         raise HTTPException(
             400, "This turn has only one take, so there is nothing to fork to."
@@ -1482,6 +1510,18 @@ def import_adventure(
     # disagrees with itself is a 400 and not a half-imported adventure holding a
     # story with a hole in it.
     story = bundle.plan(payload, version)
+    # Counted again, on what will actually be written. The check above reads the
+    # file's own lists, and in a v1 file a turn is one entry carrying its retries
+    # in a `variants` array — which `plan()` expands into one row per attempt
+    # (SP4 made every attempt a node). So a file of 5,000 turns with ten takes
+    # each passes a 5,000-action cap and writes 50,000 rows, comfortably inside
+    # the 20 MB body limit. `plan()` is pure and the adventure does not exist
+    # yet, so this still costs nothing but the planning.
+    limits.check_bundle_lists(
+        actions=story["nodes"],
+        memories=story["memories"],
+        branches=story["branches"],
+    )
 
     # Raw-dict import bypasses the schemas — clamp strings headed for VARCHAR
     # columns (Postgres enforces the widths; see schemas.py).

backend/app/scripting/pipeline.py

@@ -28,6 +28,15 @@ class ScriptPipeline:
         # actions, and this is the documented history API a user script reads.
         # Handing a script the siblings of the turn it is running on would be
         # the same bug as building a prompt from them, only user-visible.
+        #
+        # `story_actions` also drops blank-text rows, which `adventure.actions`
+        # kept, so this array is shorter than it used to be for an adventure
+        # that has any — and `info.actionCount` counts the same way. That is
+        # deliberate: a row with no text is this app's bookkeeping, it has no
+        # counterpart in the AI Dungeon history a ported script was written
+        # against, and the prompt has never included one. A script keyed on
+        # "every N actions" will land on different turns than it did before
+        # phase 14; there is no reading of this that is compatible with both.
         return [
             {"text": a.text, "rawText": a.text, "type": a.type}
             for a in context_history.story_actions(self.adventure)

backend/tests/test_attempt_siblings.py

@@ -404,3 +404,53 @@ def test_export_carries_every_attempt_as_its_own_node(client):
     ai_rows = [a for a in rows if a.type == "ai"]
     assert [(a.text, a.live) for a in ai_rows] == [("One.", False), ("Two.", True)]
     assert len({(a.branch_id, a.depth) for a in ai_rows}) == 1
+
+
+def test_a_retry_after_switching_back_files_the_new_attempt_last(client):
+    """The group stays in the order the attempts were made.
+
+    `add_attempt` used to number a new take one past the take it replaced, which
+    is the end of the group only when the story is standing on the newest one.
+    Switch a three-take turn back to the first and retry, and the new attempt
+    collided with take 2 — `renumber` then broke the tie by id and filed it
+    *between* takes 2 and 3, so the pager walked them in an order nobody played.
+    """
+    ScriptedProvider.replies = ["One.", "Two.", "Three.", "Four."]
+    _play(client)
+    _retry(client)
+    _retry(client)
+    assert [a.text for a in _rows(client.adv_id) if a.type == "ai"] == [
+        "One.", "Two.", "Three."]
+
+    live = _page(client)["actions"][-1]
+    r = client.post(
+        f"/api/adventures/{client.adv_id}/actions/{live['id']}/variant",
+        json={"index": 0})
+    assert r.status_code == 200, r.text
+
+    _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."
+
+
+def test_the_adventure_list_quotes_the_take_the_story_tells(client):
+    """The index screen and the story have to agree.
+
+    Siblings share a depth and the newest of them has the highest id, so a
+    snippet ordered by `(depth, id)` alone quotes whichever attempt was written
+    last — which, after switching back, is the one the player threw away.
+    """
+    ScriptedProvider.replies = ["One.", "Two."]
+    _play(client)
+    _retry(client)
+    live = _page(client)["actions"][-1]
+    assert live["text"] == "Two."
+    client.post(f"/api/adventures/{client.adv_id}/actions/{live['id']}/variant",
+                json={"index": 0})
+
+    listed = client.get("/api/adventures").json()
+    row = [a for a in listed if a["id"] == client.adv_id][0]
+    assert "One." in row["snippet"]
+    assert "Two." not in row["snippet"]

backend/tests/test_branch_forking.py

@@ -321,10 +321,11 @@ def test_forking_a_turn_that_is_already_the_story_is_a_no_op(client):
     assert len(_branches(client)) == 1
 
 
-def test_forking_a_single_take_on_another_branch_is_refused(client):
-    """The only way to reach the refusal, and it names the wrong tool: a node
-    with no siblings is not a divergence, so what the caller wants is to switch
-    to the branch it is on."""
+def test_forking_a_live_node_on_another_branch_is_refused(client):
+    """A live node off the path is another line's story, not an attempt going
+    spare — so the refusal names the tool that would actually do it. It used to
+    answer "only one take", which was true of the group and no help at all: the
+    caller does not want another take, it wants the branch this one is on."""
     discarded = _divergent_story(client)
     _fork(client, discarded)
     parent_id = [b for b in _branches(client) if b["parent_branch_id"] is None][0]["id"]
@@ -333,7 +334,9 @@ def test_forking_a_single_take_on_another_branch_is_refused(client):
 
     r = _fork(client, stranded)
     assert r.status_code == 400
-    assert "one take" in r.json()["detail"]
+    assert "another branch" in r.json()["detail"]
+    # And refusing left the tree alone — the bug this guards is a fork that
+    # promotes a sibling on the branch it was called against.
     assert len(_branches(client)) == 2
 
 

backend/tests/test_branch_management.py

@@ -222,6 +222,24 @@ def test_a_name_longer_than_the_column_is_refused(client):
     assert _rename(client, forked, "x" * schemas.BRANCH_NAME_MAX).status_code == 200
 
 
+def test_a_rename_hands_back_the_row_the_listing_would_give(client):
+    """Renaming a branch does not change how many turns are on it.
+
+    `own_actions` was hard-coded to 0 in this response, which only stayed
+    invisible because the panel throws the body away and refetches. Anything
+    that trusted the reply would draw a branch that had just lost its turns.
+    """
+    root, forked = _forked(client)
+    listed = {b["id"]: b for b in _branches(client)}
+
+    renamed = _rename(client, forked, "the cellar").json()
+
+    assert renamed["own_actions"] == listed[forked]["own_actions"]
+    assert renamed["own_actions"] > 0, "the fixture put turns on this branch"
+    assert renamed["depth"] == listed[forked]["depth"]
+    assert renamed["is_head"] == listed[forked]["is_head"]
+
+
 def test_naming_a_branch_of_another_adventure_is_a_404(client):
     root, forked = _forked(client)
     db = SessionLocal()

backend/tests/test_bundle_v2.py

@@ -533,6 +533,75 @@ def test_a_v2_bundle_brings_its_anchors_back(client):
         db.close()
 
 
+def test_a_v1_memory_that_summarises_nothing_lands_on_the_root(client):
+    """The import has to answer the question migration 62 answered.
+
+    A v1 file has no depths, and a memory the player typed has no `sourceEnd`
+    to derive one from — so it used to come back with a NULL depth, which is the
+    exact state SP7 removed from the schema. `Path._entry_clause` compares
+    `depth <= max_depth` and a NULL fails it, so the memory would read fine
+    until the imported adventure was forked and then vanish from the new branch.
+    """
+    payload = {
+        "format": bundle.LEGACY_FORMAT, "title": "Old backup",
+        "memoryCursor": 0, "summaryCursor": 0,
+        "actions": [
+            {"index": 0, "type": "start", "text": OPENING},
+            {"index": 1, "type": "story", "text": "A corridor."},
+        ],
+        "memories": [
+            {"text": "Kira is the innkeeper's daughter"},          # typed
+            {"text": "The corridor, summarised", "sourceStart": 1, "sourceEnd": 1},
+        ],
+    }
+    copy = _imported(client, payload)
+
+    db = SessionLocal()
+    try:
+        rows = {m.text: m for m in db.query(models.Memory)
+                .filter(models.Memory.adventure_id == copy).all()}
+        assert rows["Kira is the innkeeper's daughter"].depth == 0, (
+            "a typed memory anchors at the root, which every branch can see"
+        )
+        assert rows["The corridor, summarised"].depth == 1, "derived from its range"
+        assert all(m.depth is not None for m in rows.values())
+        assert all(m.branch_id is not None for m in rows.values())
+    finally:
+        db.close()
+
+
+def test_the_action_cap_counts_the_rows_a_v1_file_expands_into(client, monkeypatch):
+    """The cap has to count what gets written, not what the file lists.
+
+    A v1 turn carries its retries in a `variants` array, and SP4 made every
+    attempt a row — so one entry can become ten. Counting entries lets a file
+    inside the cap write a multiple of it, and the body limit is no help: the
+    text is tiny, it is the row count that is the cost.
+    """
+    monkeypatch.setattr(auth, "MULTI_USER", True)
+    monkeypatch.setattr(limits, "MAX_ACTIONS_PER_ADVENTURE", 6)
+    monkeypatch.setattr(limits, "_BUNDLE_LIST_CAPS",
+                        {**limits._BUNDLE_LIST_CAPS, "actions": 6})
+    payload = {
+        "format": bundle.LEGACY_FORMAT, "title": "Small file, many rows",
+        "memoryCursor": 0, "summaryCursor": 0,
+        "actions": [{"index": 0, "type": "start", "text": OPENING}] + [
+            {
+                "index": i, "type": "ai", "text": "Take four.",
+                "variants": [{"text": f"Take {n}."} for n in range(4)],
+                "variantIndex": 3,
+            }
+            for i in range(1, 4)
+        ],
+    }
+    assert len(payload["actions"]) <= 6, "the file itself is inside the cap"
+    before = _adventure_count()
+
+    r = _import(client, payload)
+    assert r.status_code == 409, r.text
+    assert _adventure_count() == before, "and nothing was written"
+
+
 def test_an_unknown_format_is_refused(client):
     r = _import(client, {"format": "ai-dnd-adventure-v3", "title": "From the future"})
     assert r.status_code == 400, r.text

backend/tests/test_memory_nodes.py

@@ -452,3 +452,53 @@ def test_retrieving_from_a_deep_fork_costs_what_a_flat_story_costs(deeply_forked
         f"retrieval on a 20-fork story cost {forked_bytes:,} B against the "
         f"{flat_bytes:,} B a flat story of the same length cost"
     )
+
+
+# ------------------------------------------------------- the opening node
+
+def test_a_typed_memory_on_the_opening_node_survives_that_node_going(forked):
+    """The one place a node and its memories part company.
+
+    A memory anchored to a node is withdrawn with the node, which is the rule
+    and is deliberate: it described that turn, and the turn is leaving. But
+    migration 62 parked *every* memory written before memories had coordinates
+    on depth 0 — the only landing spot visible from every branch — so the
+    opening node carries a whole bank it never produced. Withdrawing it would
+    retire all of that in one click, for every adventure predating the tree.
+
+    A memory with no `source_start` covers no stretch of story, so nothing about
+    it can go stale. It stays.
+    """
+    db, adventure, settings, ids = forked
+    typed = models.Memory(
+        adventure_id=adventure.id, text="Kira is the innkeeper's daughter",
+        source_start=None, source_end=None,
+    )
+    typed.branch_id, typed.depth = ids["a"], 0
+    db.add(typed)
+    db.commit()
+    typed_id = typed.id
+
+    withdrawn = memorybank.forget_node(db, adventure, ids["nodes"]["A0"])
+    db.commit()
+
+    assert withdrawn == 0
+    assert db.get(models.Memory, typed_id) is not None
+
+
+def test_a_summary_of_the_opening_node_is_still_withdrawn(forked):
+    """The exception is about memories that describe nothing, not about depth 0.
+
+    A summary that genuinely ends on the opening node describes text that is
+    going, so it goes too — otherwise the root would collect exactly the
+    dangling rows `forget_node` replaced `prune_dangling_memories` to prevent.
+    """
+    db, adventure, settings, ids = forked
+    derived = add_memory(db, adventure, "the opening, summarised", ids["nodes"]["A0"])
+    derived_id = derived.id
+
+    withdrawn = memorybank.forget_node(db, adventure, ids["nodes"]["A0"])
+    db.commit()
+
+    assert withdrawn == 1
+    assert db.get(models.Memory, derived_id) is None

frontend/src/pages/Play.jsx

@@ -1943,6 +1943,15 @@ export default function Play() {
                         // rewriting this one, so the reply carries a new id.
                         setActions((prev) => prev.map(
                           (a) => (a.id === action.id ? updated : a)))
+                        // Switching takes is not only a change of text. The
+                        // server puts back that attempt's script and world
+                        // state, withdraws the memory that hung off the
+                        // coordinate, and rewinds both cursors — none of which
+                        // the panels can see, because they key on
+                        // `actions.length` and the story is the same length it
+                        // was. Same class of bug as a branch switch, which
+                        // `adoptWindow` already bumps this for.
+                        setStateKey((k) => k + 1)
                       }}
                       onError={(message) => setToast({ text: message, isError: true })}
                     />

plan/14-phase-story-tree.md

@@ -812,6 +812,17 @@ hand-driving *or* the harness and this took the first. The harness remains the t
 would catch a prepend regression without a person in the loop, and jsdom's lack of layout
 means it would not have settled the 5 px question either way.
 
+**One user-visible change nobody asked for, recorded here because a script
+author would otherwise find it by being surprised.** Moving `_history` onto
+`context_history.story_actions` fixed the branch bug it was there to fix, and
+carried a second change with it: `story_actions` drops blank-text rows, which
+`adventure.actions` did not. So a user script's `history` array and
+`info.actionCount` both got shorter for any adventure holding one. It is the
+right shape — a textless row is bookkeeping with no AI Dungeon counterpart, and
+the prompt never included one — but a script that fires "every N actions" now
+fires on different turns. Not compatible with both readings; this is the one
+chosen.
+
 ### SP8 — Drop the legacy columns
 
 Only once the tree is proven live. Migration drops `index`, `variants`, `variant_index`,