diff --git a/backend/app/attempts.py b/backend/app/attempts.py index eaf8ed1..a4b13ef 100644 --- a/backend/app/attempts.py +++ b/backend/app/attempts.py @@ -50,23 +50,66 @@ ATTEMPT_KEYS = ("world_state", "script", "raw_output") def group(db: Session, action: models.Action) -> list[models.Action]: """Every attempt at `action`'s turn, oldest first. - A node with no branch is a pre-tree row that no path contains; it is its - own only attempt, and saying so here saves every caller a special case. + Keyed on the **parent**, not on the coordinate (SP9). The two agree right up + until a take is forked onto its own branch: it keeps its parent but leaves + the (branch, depth) its siblings are still at, so a coordinate would report + it as the only take of its turn — `1/1` where the player is owed `1/3`. + + The parent also gets the nesting right without being asked. Takes under C1 + and takes under C2 share a depth and, until one of them forks, a branch; + only the parent separates them, which is what makes a pager under C2 read + `2/2` instead of counting C1's three as well. + + Two fallbacks, both meaning "this row predates the key being asked about": + a node with no branch is a pre-tree row no path contains, and a node with no + parent is a pre-SP9 row the backfill could not place. Both are their own + only attempt under the rule they were written with. """ if action.branch_id is None or action.depth is None: return [action] + if action.parent_id is None: + # Pre-SP9, and the coordinate is the key those rows were written under. + # Root nodes land here too and are genuinely alone: nothing is a take of + # the opening of a story. + return ( + db.query(models.Action) + .filter( + models.Action.adventure_id == action.adventure_id, + models.Action.branch_id == action.branch_id, + models.Action.depth == action.depth, + models.Action.parent_id.is_(None), + ) + .order_by(models.Action.variant_index, models.Action.id) + .all() + ) return ( db.query(models.Action) .filter( models.Action.adventure_id == action.adventure_id, - models.Action.branch_id == action.branch_id, - models.Action.depth == action.depth, + models.Action.parent_id == action.parent_id, ) .order_by(models.Action.variant_index, models.Action.id) .all() ) +def on_branch(rows: list[models.Action], node: models.Action) -> list[models.Action]: + """The takes in `rows` that sit on `node`'s own branch. + + `group` answers "which takes are of this turn", and since SP9 that spans + branches — a take forked onto its own line is still a take of the same turn, + which is the whole point of keying on the parent. + + Deleting is the one caller that must not follow it there. A take on another + branch is reachable through that branch and belongs to the story somebody is + telling on it; removing it because a turn was undone over here would delete + a line nobody asked about. Same parent *and* same branch is the coordinate, + which is what "every attempt at this turn" meant before a fork could move + one out of it. + """ + return [row for row in rows if row.branch_id == node.branch_id] + + def live_in(rows: list[models.Action]) -> models.Action | None: for row in rows: if row.live: @@ -149,6 +192,11 @@ def add_attempt( """ replacement.branch_id = previous.branch_id replacement.depth = previous.depth + # Copied, never resolved from the path: a take belongs to the turn it is a + # take *of*, and that is what `group` keys on. Resolving it here would ask + # what is live one depth back, which is the same node right now and stops + # being once this turn is forked away from. + replacement.parent_id = previous.parent_id replacement.live = True # 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 diff --git a/backend/app/migrations.py b/backend/app/migrations.py index 93962b5..bb06617 100644 --- a/backend/app/migrations.py +++ b/backend/app/migrations.py @@ -266,6 +266,16 @@ MIGRATIONS: list[tuple[int, str | dict[str, str]]] = [ # visible from today. Anchoring them at the tip instead would have emptied # them out of every branch forked earlier than they were typed. (62, "UPDATE memories SET depth = 0 WHERE depth IS NULL"), + # Phase 14, SP9 — the node a node was played after, so a turn's takes can be + # grouped by parent instead of by coordinate. A coordinate stops answering + # "which takes belong together" the moment one of them is forked onto its + # own branch: it leaves its siblings behind and reads 1/1 beside their 1/3. + # + # Nullable, and left NULL wherever the backfill cannot honestly place a row + # (see _backfill_parents). `attempts.group` falls back to the coordinate for + # those, which is the rule they were written under. + (63, "ALTER TABLE actions ADD COLUMN parent_id INTEGER REFERENCES actions(id) ON DELETE SET NULL"), + (64, "CREATE INDEX IF NOT EXISTS ix_actions_parent ON actions (parent_id)"), ] LATEST_VERSION = max((v for v, _ in MIGRATIONS), default=1) @@ -278,6 +288,7 @@ SNAPSHOT_COMPRESS_VERSION = 43 TREE_BACKFILL_VERSION = 52 CURSOR_ANCHOR_VERSION = 56 SIBLING_SPLIT_VERSION = 60 +PARENT_BACKFILL_VERSION = 64 # An adventure with no actions has no tip. -1 keeps "the next node goes at # head_depth + 1" true without a special case (mirrors tree.NO_DEPTH). @@ -324,6 +335,45 @@ def _backfill_world_delta(conn) -> None: conn.execute(text(sql)) +def _backfill_parents(conn) -> None: + """Point every node at the take it was played after. + + One pass, and deliberately only one: the live node one depth back on the + same branch. That is the whole of a linear story, which is the whole of + every adventure written before SP9 — forking reached the screen in SP7 and + the tree was found unusable before anyone forked with it. + + The rows left NULL are the first node of a forked branch, whose parent sits + on an ancestor branch and cannot be found without walking `lineage` per row. + `attempts.group` falls back to the coordinate for a NULL parent, which is + exactly the rule those rows were written under, so the fallback is not a + degraded answer for them — it is the original one. Every fork made from SP9 + on sets `parent_id` at write time and never relies on this. + + Correlated to the row being updated rather than numbering the table, so the + planner drives it off ix_actions_branch_depth. That is the lesson of + _backfill_cursor_anchors, which numbered every action once per adventure. + """ + conn.execute( + text( + """ + UPDATE actions SET parent_id = ( + SELECT prev.id FROM actions AS prev + WHERE prev.adventure_id = actions.adventure_id + AND prev.branch_id = actions.branch_id + AND prev.depth = actions.depth - 1 + AND prev.live = TRUE + ORDER BY prev.id + LIMIT 1 + ) + WHERE parent_id IS NULL + AND branch_id IS NOT NULL + AND depth IS NOT NULL + """ + ) + ) + + def _backfill_variant_count(conn) -> None: """Populate actions.variant_count from the existing variants list. @@ -890,6 +940,10 @@ def bootstrap(engine: Engine) -> None: if version == SIBLING_SPLIT_VERSION: _backfill_state_after(conn) _split_variants_into_siblings(conn) + # After 63 adds the column and 64 indexes it: the UPDATE is + # what the index is for, so it runs once both are in place. + if version == PARENT_BACKFILL_VERSION: + _backfill_parents(conn) current = version _set_version(conn, current) _encrypt_plaintext_api_keys(conn) diff --git a/backend/app/models.py b/backend/app/models.py index 9f1e7f2..78bbc9b 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -333,6 +333,24 @@ class Action(Base): ForeignKey("branches.id", ondelete="CASCADE"), nullable=True ) depth: Mapped[int | None] = mapped_column(Integer, nullable=True) + # Phase 14, SP9: the node this one was played after — the take that was + # live when it was written, not merely whatever sits at depth - 1 now. + # + # It exists for one question: which takes belong to the same turn. A + # coordinate cannot answer it, because a take that gets forked onto its own + # branch leaves the coordinate its siblings are still at and would read + # `1/1` next to their `1/3`. A parent does not move when a branch does. + # + # Read only to group takes — one indexed lookup, never a walk. Paths still + # resolve through `lineage`, which is why this column can be added without + # touching a single read of the story. + # + # NULL on a root node, and on every pre-SP9 row the migration could not + # place: `attempts.group` falls back to the coordinate there, which is what + # those rows were written under. + parent_id: Mapped[int | None] = mapped_column( + ForeignKey("actions.id", ondelete="SET NULL"), nullable=True, index=True + ) # Phase 14, SP4: whether this node is the one the story tells at its # coordinate. Retry no longer rewrites a row — it writes a *sibling* at the # same (branch, depth), so a coordinate can hold several attempts and diff --git a/backend/app/routers/adventures.py b/backend/app/routers/adventures.py index a42d66f..98bfad1 100644 --- a/backend/app/routers/adventures.py +++ b/backend/app/routers/adventures.py @@ -1047,7 +1047,10 @@ def delete_turn( rather than off one of its attempts. """ memorybank.forget_node(db, adventure, node) - for attempt in attempts.group(db, node): + # Scoped to this node's branch (SP9). The group spans branches now, and a + # take that was forked onto its own line is another branch's story — see + # `attempts.on_branch`. + for attempt in attempts.on_branch(attempts.group(db, node), node): db.delete(attempt) diff --git a/backend/app/tree.py b/backend/app/tree.py index 77fd308..29ba005 100644 --- a/backend/app/tree.py +++ b/backend/app/tree.py @@ -182,6 +182,7 @@ def place_action( adventure: models.Adventure, action: models.Action, branch: models.Branch | None = None, + parent: models.Action | None = None, ) -> models.Branch: """Put `action` on the head branch and move the head to it. @@ -191,17 +192,52 @@ def place_action( `branch` is the head, already resolved, for a caller placing several nodes at once — see `place_new_nodes` for why that is worth a parameter. + + `parent` is the take this node was played after (SP9), and is what groups a + turn's takes. Resolved from the path when the caller does not say, which is + the honest default: a node written now follows whatever the player is + reading now. A caller placing several nodes in one flush should chain it — + the second node's parent is the first, and the database has not seen either. """ branch = branch or head_branch(db, adventure) action.branch_id = branch.id if action.depth is None: action.depth = action.index + if parent is not None: + action.parent_id = parent.id + elif action.parent_id is None and action.depth: + action.parent_id = _preceding_id(db, adventure, branch, action.depth) adventure.head_branch_id = branch.id if action.depth is not None and action.depth > adventure.head_depth: adventure.head_depth = action.depth return branch +def _preceding_id( + db: Session, adventure: models.Adventure, branch: models.Branch, depth: int +) -> int | None: + """The id of the live node one step back along `branch`'s path. + + Asked of the whole lineage rather than of `branch` alone, because a branch + borrows the story before its fork point: the node in front of a forked + branch's first turn lives on an ancestor, and that is exactly the parent a + pager needs to find its siblings through. + """ + path = lineage.Path(lineage.entries_of(branch)) + return ( + db.query(models.Action.id) + .filter( + models.Action.adventure_id == adventure.id, + models.Action.depth == depth - 1, + models.Action.live.is_(True), + path.clause(), + ) + .order_by(models.Action.id) + .limit(1) + .scalar() + ) + + def place_memory( db: Session, adventure: models.Adventure, diff --git a/plan/14-phase-story-tree.md b/plan/14-phase-story-tree.md index 0b483ba..3847cb5 100644 --- a/plan/14-phase-story-tree.md +++ b/plan/14-phase-story-tree.md @@ -842,6 +842,66 @@ once SP7's tree view has replaced the pager. **Verify:** full suite; egress ceilings; a measured before/after size, aggregates only. +### SP9 — Takes, not chips: one pager, and a fork that makes a new take + +**Why this exists.** SP7 was driven by hand and the tree was unusable. Three things were +wrong, and only the third is a bug: + +* **A chip did two different things.** At the tip it switched; above the tip it only + *previewed*, and taking it needed a second button. One control, two meanings, and the + meaning depended on where the player was standing. +* **Nothing could fork but an AI turn that already had a second take.** + `POST /actions/{id}/fork` answers 400 when the turn has one take, so a player's own + message had no way to become anything else and "branch from here" did not exist. +* **Retry only worked on the newest action.** Mid-story there was no retry at all. + +**The model, in the player's words.** Every action can gain another *take*. On an AI node +that means regenerate; on the player's own it means type something else. Stepping between +takes is free — `3/3` to `1/3` is navigation, and the story below simply empties, because +that take has no children yet. **A branch is created when you write below a take that is +not the live one**, never before. + +That collapses SP5's fork and SP4's retry into one operation and deletes the +tip-versus-past distinction from the UI entirely. It survives only in the implementation, +where it decides whether a write needs a branch at all. + +**Takes are grouped by parent, not by coordinate.** This is the load-bearing change. +`attempts.group()` filters `branch_id == … AND depth == …`, and the player's own example +breaks it: + +``` +B ── C C1 C2 <- three takes, one parent (B) + │ └── D1' D2' <- two takes, parent C2 + └── D1 D2 D3 <- three takes, parent C1 +``` + +Standing on the C2 path at that depth must read `2/2`, not `5`. Coordinate grouping gets +that right by accident — writing under a non-live take forks, so the two sets land on +different branches. It gets `C` wrong: once C is forked onto its own branch it is alone +at its coordinate and reads `1/1`, losing C1 and C2 from a pager that must still say +`1/3`. + +**Decision: add `actions.parent_id`.** The alternative — making a branch's fork point a +*node* instead of a depth, so a promoted take never moves — was rejected. `lineage` exists +precisely so a read is an OR-clause per branch rather than a walk up parent pointers, and +re-pointing the fork at a node changes path resolution itself, which drags in `cursors`, +memory depths and both bundle formats. `parent_id` is read only to group a turn's takes: +one indexed lookup, never a walk, and nothing about how a path resolves changes. Per SP2's +sizing note an integer beside `depth` is cheap, and the backfill rewrites a heap measured +at 1.7 MB on 2026-08-18. + +**`variant_count` / `variant_index` are reprieved, not revived.** SP8 was going to drop +both; the pager needs the group's shape again. It needs it *per parent*, which is not what +either column caches, so SP8 still drops them and SP9 computes the shape from `parent_id`. + +**Verify:** the SP0 baseline passes unmodified — a linear story has one take per parent, +and none of this is reachable without a second one. Plus: `2/2` under one take while its +sibling holds `3/3`; a pager that still reads `1/3` after its take has been forked onto its +own branch; a write below a non-live take forks exactly once; a fork on a player's own +message generates a reply. + +**Owed:** a migration adding one column, and a `VACUUM FULL actions;` after it. + ## Standing constraints - **No production data is read at any point in this phase.** Migrations, the e2e