From d05150151790dadf804c4946c0dc03ec95126e26 Mon Sep 17 00:00:00 2001 From: parththakkar106 Date: Tue, 18 Aug 2026 12:00:11 +0530 Subject: [PATCH] Score the plan against what the tree actually did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SP4 and SP5 are written down: what shipped, what it measured, and the handful of things worth not rediscovering. The bug table at the top is scored now that six of its seven rows are gone — and the seventh, the one-turn holdback, is gone for a different reason than the one written there, which is the correction that matters most. Editing a summarised action is still unfixed and now says so; no subphase is scheduled for it. The Open section closes. `retry_of.index` was the last item and SP4 answered it by reusing the retried node's depth. Two vacuums are owed, SP1's and SP4's, and neither has deployed. One run after the SP4 deploy settles both. SP8 grows two more columns to drop and a caveat about which ones are not dead yet. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015H5qiyiR7gtFQaoDphHZ3g --- plan/14-phase-story-tree.md | 174 ++++++++++++++++++++++++++++++++++-- plan/STATUS.md | 108 ++++++++++++++++------ 2 files changed, 252 insertions(+), 30 deletions(-) diff --git a/plan/14-phase-story-tree.md b/plan/14-phase-story-tree.md index 7365aba..415ed8c 100644 --- a/plan/14-phase-story-tree.md +++ b/plan/14-phase-story-tree.md @@ -25,6 +25,21 @@ mutable list**: It also resolves the 1NF violation: `variants` as a JSON repeating group becomes rows. +**Scored, 2026-08-18, with SP1–SP5 shipped.** Six of the seven are gone as described. +The seventh — the one-turn holdback — is gone too, but the reasoning above was wrong +about *why* it could go: siblings share a coordinate, so replacing what a turn says still +invalidates the memory covering it. What made it deletable is that the repair already +existed for undo and delete; see the trap note below. Two rows want a footnote: + +- **Editing a summarised action** is still unfixed. The table says editing makes a new + node; nothing in SP1–SP5 makes it do that, and no subphase is scheduled to. `PATCH + /actions/{id}` still writes over the text in place, and the memory covering it goes + stale exactly as before. What *has* changed is that the machinery to fix it now exists + — an edit could write a sibling and switch to it, which is a retry the player typed — + so it is a small change whenever it is wanted. +- **The 1NF violation** is resolved in the database. The `variants` array survives in + exactly one place: the v1 export bundle, which SP6 replaces. + ## Design decisions (settled 2026-08-16) - **Full branching, with UI.** Not the "tree schema, no branch picker" middle option — @@ -107,10 +122,10 @@ number — `A4` and `B4` are alternatives, not duplicates. ## Open -- **`retry_of.index` reuse** stops the world-state cooldown clock advancing on a re-run of - the same turn. Whatever replaces it must preserve that. (Carried into SP5 below.) - -Everything else that stood open here was decided on 2026-08-17 — see the next section. +Nothing. The last item — **`retry_of.index` reuse**, which stopped the world-state +cooldown clock advancing on a re-run of the same turn — was closed in SP4 by reusing the +retried node's *depth*, and pinned by a test in SP5. Everything else that stood open here +was decided on 2026-08-17; see the next section. --- @@ -161,6 +176,13 @@ even there only where the change is deliberate and named below. SP3 as `memorybank.settled_after`, which is the `- 1` in "how much story is past the mark"; that subtraction is the whole of it. + *Closed in SP4, but not for the stated reason.* Siblings share a coordinate and the + mark names the coordinate, so replacing what a turn says still invalidates the memory + covering it — retry mutating a row was never the whole of the problem. What let the + holdback go is that the right repair (`forget_node` plus a rewind) already existed for + undo and delete, and retry and a sibling switch now make it too. **If a mark still + needs correcting when the story changes, correct it; do not decline to make the mark.** + ## Subphases Each ships independently, on its own branch, green before the next starts. @@ -437,6 +459,68 @@ results, including that the gold script is not double-applied. `test_state_rever against after-snapshots. Migration test: attempt count preserved, the active attempt becomes the head. +**Done, 2026-08-18** (branch `sp4-sibling-nodes`). **347 tests green**, the 330 SP3 +finished with plus 13 in the new `test_attempt_siblings.py`, 8 in `test_tree_migration +.py`, and three holdback tests deleted. `app/attempts.py` is the new module; migrations +57–60 add `live`, `state_after`, `world_state_after`, derive the after-snapshots, and +split every `variants` list into rows. + +**The baseline test did not have to move, and neither did `test_retry_variants.py`.** +Both pass unmodified. SP4 was *permitted* to change the variant-count semantics and it +turned out nothing observable needed changing — which is the strongest form the pass +condition could have taken, and worth knowing before SP6 asks for the same licence. + +Seven things worth not rediscovering: + +- **A coordinate needs a `live` flag, and the branch clause is where it belongs.** + Siblings share `(branch_id, depth)`, so the lineage clause alone returns all of them + and the story tells itself twice. `Path.clause` adds `Action.live` for actions (and + only for actions — memories have no siblings to lose to), which means no read of the + story had to learn that retries exist. `app/attempts.py` is the only code that looks + past it. +- **The prompt has to move with the flag or retry becomes a storage multiplier.** A + `context_snapshot` is ~163 kB of prompt that every attempt at a turn shares, and the + old JSON list existed precisely to store it once. Giving each sibling row a copy would + have undone that. So the invariant is: the assembled prompt lives on the attempt the + story tells, and a superseded one keeps only its own slices (`ATTEMPT_KEYS` — the + world-state delta, the script report, the raw reply). Measured on the pre-tree + 600-action fixture, migrated: **700 rows for the same 600-turn story, and the prompt + archive byte-identical at 0.50 MB.** +- **The holdback was not quite unnecessary — it was the wrong repair.** The plan said + retry stops mutating rows so nothing goes stale. Not so: siblings share a coordinate, + and the mark and the memory both name the *coordinate*, so replacing what a turn says + still invalidates them. What made the holdback deletable is that the right repair + already existed — `forget_node` plus a rewind, which undo and delete have called since + SP3. Retry and a sibling switch now call it too, and the holdback (`settled_count`, + `settled_after`, `settled_story_actions`, `newest_settled`) is gone. **A memory can + now cover the newest action**, which it never could before. +- **`state_after` needed the same flush guard `place_action` has.** The migration + derives every existing row's outcome from the next row's `state_before`, and the tip's + from the adventure's live state — but a row written by a fixture or a script after + that has nobody to derive from, and the failure mode is undo silently leaving the + scoreboard where it was. `tree.stamp_outcome` runs from `before_flush` beside + `place_new_nodes`. It writes the truth as of the flush: a writer that changes no state + between two nodes leaves the same state behind both. +- **Switching attempts hands the caller a different row id**, because that is what an + attempt being a node *means*. The endpoints are addressed by any attempt at the turn + rather than by the live one, so a client holding a stale id still asks about the right + turn — but `Play.jsx` matched the reply against `updated.id` and had to be changed to + match against the action it asked about. One line, and SP7 removes the pager anyway. +- **Deleting a turn deletes its attempts.** A discarded attempt is only reachable + *through* its coordinate, so leaving it behind would leave a row nothing can name and + no read can see. `delete_turn` is that rule in one place, called by undo and by + delete-an-action, and it works whichever attempt's id the caller happens to hold. +- **Siblings share the legacy `index`.** They are takes on one turn, so two rows now + carry one index — which `max_action_index` (a maximum, not a count) survives, and + which is what lets the v1 export fold a group back into a `variants` array. Export is + now the only producer of that shape anywhere; nothing in the database holds one. + +Measured: index **1.8 kB**, page load **62.7 kB**, one turn **734.8 kB** — the first two +byte-identical to SP1's and SP3's, the turn up 1.0 kB (0.14 %) for the `live` column +across a 346-row read. Migrations 57–59 each rewrite every row of `actions` and 60 +inserts one per discarded attempt, so **this deploy owes a `VACUUM FULL actions;`** — +the SP1 one is still owed too, and one vacuum after this deploy settles both. + ### SP5 — Fork on continue Playing past a non-head sibling promotes it: a `branches` row with `parent_branch_id`, @@ -449,6 +533,74 @@ correct and capped at each `fork_depth`; both branches read independently; switc restores the right script/world state; the cooldown test from `test_worldstate.py` still holds across a retry. +**Done, 2026-08-18** (branch `sp5-fork-on-continue`). **365 tests green**, the 347 from +SP4 plus 18 in `test_branch_forking.py`. `tree.fork` is the whole of it; three endpoints +(`GET /branches`, `POST /branches/{id}/switch`, `POST /actions/{id}/fork`) are what SP7's +tree view will be drawn on. + +**Where the promotion happens, and why not where the plan said.** The plan put it on the +*next turn*: attempts stay leaves, and one becomes a branch when a turn is played past +it. Promoting the winner then means moving a row off the branch the reader is standing +on and leaving that branch to pick a new node for the depth — it disturbs a story nobody +asked to change. The same divergence, seen from the other side, promotes the attempt you +are *leaving for*: `POST /actions/{id}/fork` gives the discarded attempt a branch and +moves the head to it, and the line it leaves is untouched. Branch count is identical +either way — one per divergence somebody actually built on — and one of the two never +rewrites a story in place. Without it, the losing attempts would also be unreachable +forever, since only the tip can be switched: fork-on-continue alone is a one-way door. + +Six things worth not rediscovering: + +- **A fork must not move the derived work, and it was about to.** The first cut moved the + memories at the forked coordinate onto the new branch and re-anchored the cursors that + named it. Both are wrong, and for the same reason: a memory describes whichever attempt + was *live* at that coordinate, which is the one staying on the parent. The right answer + needs no code at all — the lineage caps the parent at `fork_depth`, one depth short of + it, so the memory is simply out of range from the fork, invisible to both the retrieval + clause and `Path.depth_on`. The block is summarized again, from the text this branch + actually tells. +- **Depth had to stop following `index`.** They agreed until now. `index` is + adventure-wide (the v1 bundle is keyed on it) so on a story forked at depth 6 after + twenty turns it hands the next node depth 21 and leaves a fourteen-deep hole in the + middle of a path — which every windowing estimate then has to work around. + `next_depth` is `head_depth + 1`; `place_action` still derives depth from index for + fixtures and imports, which is what that default is for. +- **Undo has to stop at the fork.** It reads the newest two nodes on the path and deletes + the turn they make up — and on a fresh branch the second of them is borrowed from the + parent, whose story also contains it. The guard is on the row's own `branch_id`, not on + the fork depth, because that is the fact that decides it. +- **The cooldown clock came out right for free.** It lives in `_meta.last_changed` inside + the world state, and the world state is restored from the tip's `world_state_after` on + every switch — so each branch carries its own clock without anything knowing there is + one. The carried-over open item (a retry must not advance it) is SP4's reused depth, + and both are pinned by tests. +- **The session does not autoflush**, and `fork` read the sibling group after moving the + node out of it — so the move had not been written and the node was renumbered straight + back into the group it had just left. Read the group first. (`autoflush=False` is + deliberate, in `database.py`; anything in this phase that mutates then queries the same + rows has to order itself by hand.) +- **`/fork` has to be idempotent before it is anything else**, because a fork leaves the + promoted attempt alone on its branch: a repeated call — a double click, a retried + request — would otherwise be told the turn it just forked has nothing to fork to. The + "already the story" case is answered before the shape of the turn is looked at. + +Measured, on a 40-turn story forked **twenty** times against the same story flat: +21 branches, 140 rows, an 80-action story, and a page load of **31,652 B against +31,433 B (1.007×)**. A branch costs **103 B** of id, parent, fork depth and cached +ancestry. No migration, no vacuum. + +**One gap, deliberately left for SP6.** A forked adventure has no honest `v1` export — +the format has one story and there are two — so export emits every branch's turns +interleaved by index, which reads as a mangled story rather than as lost data. SP6's v2 +bundle fixes it, and SP7 is where a player first gets any way to fork at all, so the +order those two ship in is the order that matters. + +**Also known, and not fixed here:** the two cursors are one pair on the adventure, so +switching branches makes the mark on the branch being left unreadable from the new one +(`Path.depth_on` answers "nothing covered", which is the safe direction — redo the work, +never skip it). Switching back and forth therefore re-summarizes. Per-branch cursors are +the fix if it ever matters; it costs AI calls, not correctness. + ### SP6 — Export/import v2 `ai-dnd-adventure-v2` carries branches and nodes; import accepts v1 and v2, mapping a v1 @@ -475,6 +627,16 @@ Only once the tree is proven live. Migration drops `index`, `variants`, `variant and `summary_cursor`** — unread since SP3, kept only so a rolled-back build resumes from a real number. They are on `adventures`, so dropping them costs no vacuum. +**And `actions.state_before` / `world_state_before`**, unwritten and unread since SP4 for +the same reason: a rolled-back build still finds a real snapshot on every row it wrote +itself. They are deferred JSON on `actions`, so they go in the same rewrite as `index` +and cost nothing extra. + +`variant_count` and `variant_index` are the two to check before dropping: SP4 left them +as a maintained cache of the sibling group's shape, because the pager reads both for +every row of a page and must not pay a query per turn to get them. They are dead only +once SP7's tree view has replaced the pager. + **Verify:** full suite; egress ceilings; a measured before/after size, aggregates only. ## Standing constraints @@ -484,4 +646,6 @@ a real number. They are on `adventures`, so dropping them costs no vacuum. fixture. If a real Postgres is ever needed for a write path, it is a throwaway database whose name contains `stress`/`scratch`, and it is asked for first. - **Any migration that rewrites `actions` is followed by `VACUUM FULL actions;`** on the - direct endpoint, not `-pooler`. SP1, SP4 and SP8 each rewrite every row. + direct endpoint, not `-pooler`. SP1, SP4 and SP8 each rewrite every row. **As of + 2026-08-18 two are owed** (SP1's and SP4's) and neither has been deployed; one vacuum + after the SP4 deploy settles both. diff --git a/plan/STATUS.md b/plan/STATUS.md index 8f38c32..28da522 100644 --- a/plan/STATUS.md +++ b/plan/STATUS.md @@ -78,34 +78,42 @@ needed; nothing requires reading a row of anyone's story. ## Pick up here -**`plan/14-phase-story-tree.md`, SP4 — variants become sibling nodes.** SP0 (the -regression contract and the `--rich` fixture), SP1 (schema, migration, and the writer that -keeps new rows on the tree), SP2 (the branch clause: every action read selects on -`(branch_id, depth)` through `app/context/lineage.py`) and SP3 (memories hang off nodes, -and both marks are `(branch_id, depth)` through `app/context/cursors.py`) are done and -green; **nothing is deployed yet**. SP4 is where retry stops rewriting a row and writes a -sibling leaf at the same depth instead, where `state_before`/`world_state_before` become -*after* snapshots, and where the legacy `variants` JSON is migrated into rows. It is also -the first subphase allowed to move the baseline test, and only for -`variant_count`/`variant_index` semantics. +**`plan/14-phase-story-tree.md`, SP6 — export/import v2.** SP0–SP5 are done and green +(**365 tests**); **nothing is deployed yet**. The tree is complete as a storage model: a +retry writes a sibling node, continuing from a discarded attempt forks a branch, and +`GET /branches`, `POST /branches/{id}/switch` and `POST /actions/{id}/fork` are the +endpoints SP7's tree view will be drawn on. What is left is the bundle format (SP6), the +frontend (SP7) and dropping the legacy columns (SP8). + +**Do SP6 before SP7, and the reason is a live gap.** A forked adventure has no honest v1 +export — the format has one story and there are two — so export currently emits every +branch's turns interleaved by `index`, which reads as a mangled story. Nobody can reach +that state through the product yet, because forking has no UI until SP7. That ordering is +the whole mitigation, so keep it. **The schema is live in code but not on production.** When this ships, the deploy needs one `VACUUM FULL actions;` on the direct (non-`-pooler`) endpoint afterwards — SP1's -migration rewrites every row, and SP4's does it again. SP3's own migrations touch -`adventures` only and need no vacuum. See the 144 MB lesson at the top of this file. +migration rewrites every row and SP4's rewrites it three times more, so **two vacuums are +owed and one run settles both**. SP3's and SP5's changes touch `adventures` only (SP5 adds +no migration at all) and need none. See the 144 MB lesson at the top of this file. -Three things to carry into it: +Three things to carry into SP6: -- **The holdback dies in SP4 and nowhere earlier.** `settled_story_actions` exists - because retry mutates a row in place; it survived SP3 as the `- 1` inside - `memorybank.settled_after`. Retry stops mutating in SP4, which is the only point at - which removing it does not reopen the bug it was written for. -- **Anything derived attaches to the node that produced it.** A memory now does - (`tree.attach_memory`), and so do both marks. A sibling leaf is a node, so whatever SP4 - derives per attempt hangs off the attempt — and `memorybank.forget_node` is what - withdraws it when the node goes. +- **The `variants` array now exists in exactly one place: the export bundle.** Nothing in + the database holds one. `export_adventure` folds each sibling group back into the shape + the v1 reader expects, and `_imported_turn` splits one back out into rows. Those two + functions are the whole v1 surface, and v2 replaces them. +- **A v2 bundle needs branches, `live`, and both after-snapshots.** `state_after` / + `world_state_after` are what a branch switch restores; a bundle that carried the + actions but not the outcomes would import a tree nobody could switch inside. + `limits.check_bundle_lists` has to learn about branches too. - **Weigh new columns in bytes.** `actions` is already the table that fills the disk. - `tests/test_egress.py` has byte ceilings now — they will tell you. + `tests/test_egress.py` has byte ceilings — they will tell you. + +And one known cost, not a bug: the two memory marks are a single pair on the adventure, +so switching branches makes the mark on the branch being left unreadable from the new one +and that ground is summarized again. `Path.depth_on` answers "nothing covered", which is +the safe direction. Per-branch cursors are the fix if it ever matters. **After any migration that rewrites `actions`:** one `VACUUM FULL actions;`. That is the lesson of the 144 MB above — a rewrite doubles the table and only a `VACUUM FULL` gives @@ -117,6 +125,56 @@ drive it before rewriting it. --- +## What happened on 2026-08-18, part two — the tree, SP4 and SP5 + +A retry stopped rewriting a row, and a story learned to go two ways at once. + +**SP4** (branch `sp4-sibling-nodes`, **347 tests green**). Every attempt at a turn is now +its own node at the same `(branch_id, depth)`, with a `live` flag naming the one the story +tells; `app/attempts.py` owns the group. The JSON repeating group on `actions.variants` is +read one last time — by migration 60, which writes it out as the rows it always described +— and then goes unread. The state snapshots turned around with it: an action carries what +it left *behind* (`state_after` / `world_state_after`) rather than what it started from, +because attempts at one turn share a starting position and differ exactly in their +outcome. **The SP0 baseline and `test_retry_variants.py` both pass unmodified**, which SP4 +was permitted to change and did not need to. + +**SP5** (branch `sp5-fork-on-continue`, **365 tests green**). Taking the story down an +attempt the line has already moved past gives that attempt a branch of its own, forked at +the depth just before it. One row inserted, one row moved, nothing copied. Measured on a +40-turn story forked twenty times against the same story flat: 21 branches, 140 rows, an +80-action story, page load **31,652 B against 31,433 B (1.007×)**, and a branch costs +**103 B** of cached ancestry. + +Four things to carry forward: + +- **The holdback was the wrong repair, not an unnecessary one.** The plan said retry would + stop mutating rows so nothing could go stale, and that is not quite true — siblings + share a coordinate and the mark names the coordinate, so replacing what a turn says + still invalidates the memory covering it. What made `settled_story_actions` deletable is + that the *right* repair already existed: `forget_node` plus a rewind, which undo and + delete have called since SP3. Retry and a sibling switch make it too. **If a mark still + needs correcting when the story changes, correct it — do not decline to make the mark.** +- **A fork must move nothing derived, and the first cut moved it all.** Memories at the + forked coordinate were being carried onto the new branch and the cursors re-anchored. + Both wrong, for one reason: a memory describes whichever attempt was *live* there, and + that one stays on the parent. The right answer needs no code — the lineage caps the + parent one depth short of it, so it is simply out of range from the fork, and the block + is summarized again from the text this branch actually tells. **When a coordinate system + already answers a question, adding bookkeeping to answer it again is how it gets two + answers.** +- **Storage arrangements have invariants too.** A `context_snapshot` is ~163 kB of prompt + that every attempt at a turn shares — the JSON list existed to store it once. Giving + each sibling row a copy would have made retry a permanent multiplier on the biggest + column in the database. So the prompt moves with the `live` flag and a superseded + attempt keeps only its own few hundred bytes. Migrating the real 600-action fixture: + **700 rows for the same 600-turn story, prompt archive byte-identical at 0.50 MB**, and + index/page-load/turn egress unmoved at 1.8 kB / 62.7 kB / 734.8 kB. +- **`autoflush=False` is set in `database.py`**, and it bit once: `tree.fork` read the + sibling group *after* moving the node out of it, so the move had not been written and + the node was renumbered straight back into the group it had just left. Anything in this + phase that mutates rows and then queries the same rows has to order itself by hand. + ## What happened on 2026-08-18 — the tree, SP3 The memory bank stopped counting. `memory_cursor` and `summary_cursor` were positions in @@ -445,7 +503,7 @@ the SQLite dev parity this codebase protects on purpose). ``` cd backend -.venv/Scripts/python.exe -m pytest tests/ # 330 tests (~55s) +.venv/Scripts/python.exe -m pytest tests/ # 365 tests (~100s) .venv/Scripts/python.exe -m tools.stress_session # egress report (SQLite) # Same harness against a real Postgres. The target must be a THROWAWAY database @@ -474,9 +532,9 @@ the fixture never lands in a commit. **A fixture to check correctness against, rather than bytes.** The measuring fixture leaves every column it does not weigh at its default, which turns out to be exactly the -set a story tree has to migrate — `state_before`/`world_state_before` NULL on all 600 +set a story tree has to migrate — the per-action state snapshots identical on all 600 rows, no RPG scenario, no adventure scripts, both cursors 0, and retry attempts whose -text is byte-identical with `variant_index` always 0. `--rich` fills in those and only +text is byte-identical with the first always live. `--rich` fills in those and only those: ```