Add Phase 0B local validation findings and recommendation
Validates the three finalists by clone, build, test run and live local Ollama inference, then answers the fork question with measurements rather than static review. Recommendation: fork AI-DnD, confidence high. The Phase 0A call holds, but it was wrong that AI-DnD's undo is non-destructive — retry preserves the replaced take, undo hard-deletes it. A follow-up spike fixed that in 3 files (+130/-31): undo now moves a head cursor, redo round-trips, writing below a moved-back head forks and keeps the abandoned line, branch-scoped memory isolation survives, suite 627/632 with all 5 failures asserting the deleted-row behaviour that was replaced. Findings that change the plan: - AI-DnD cannot take a turn air-gapped as shipped; tiktoken fetches its encoding from a CDN. Proven on an internal Docker network, proven fixed by vendoring the file. - ai-adventure needs zero code for Ollama — two config lines — and its turn/head/checkpoint schema is the target model to build to. - Open Dungeon has zero automated tests and a positional summary watermark, making its branch retrofit larger than Phase 0A costed. - The world-state referee takes relative deltas; a 3B model sent absolute values under full context, so a wounded player ended at full health. Validation cannot catch this, so prefer ai-adventure's typed-event vocabulary when generalising narrative state. - Export/import recomputes head depth, so a round-trip silently undoes an undo. Must be fixed alongside the undo work. Docs only; no production code. Working tree from the runs stays untracked under phase0b/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015gUPLuxLs8wypxZPEmccJu
This commit is contained in:
JesseMarkowitz
co-authored by
Claude Opus 5
Claude Opus 5
parent
f011362494
commit
ba737de9b4
@@ -0,0 +1,214 @@
|
||||
# Phase 0B — Spike: Non-Destructive Undo/Redo in AI-DnD
|
||||
|
||||
**Date:** 2026-09-01
|
||||
**Base:** AI-DnD `d72f7c1b`, disposable copy at `phase0b/spike/`
|
||||
**Verdict:** **Passed on every criterion.** The retrofit is small, centralized,
|
||||
and costs 5 of 632 tests — all of which assert the deleted-row behavior that was
|
||||
deliberately replaced.
|
||||
|
||||
## The question
|
||||
|
||||
Can undo move a head cursor backward without deleting rows, does writing below a
|
||||
moved-back head fork a branch, and does Redo work — without breaking
|
||||
branch-scoped memory isolation?
|
||||
|
||||
## What the change turned out to be
|
||||
|
||||
Three files, **+130 / -31 lines**, and roughly a fifth of the additions are
|
||||
comments and the new endpoint's docstring.
|
||||
|
||||
| File | Change |
|
||||
|---|---|
|
||||
| `app/context/lineage.py` | +26 / -3 |
|
||||
| `app/routers/adventures/takes.py` | +74 / -27 |
|
||||
| `app/routers/adventures/turns.py` | +30 / -1 |
|
||||
|
||||
### 1. Read path — cap the lineage at the head
|
||||
|
||||
The stored lineage records the newest branch entry **uncapped** (`max_depth =
|
||||
None`, meaning "through to the tip"), and older entries capped at their fork
|
||||
depths. `Path.clause` and `Path.contains` now read an uncapped entry as capped at
|
||||
`self.tip`, which is `adventure.head_depth`:
|
||||
|
||||
```python
|
||||
def _cap(self, max_depth: int | None) -> int | None:
|
||||
return self.tip if max_depth is None else max_depth
|
||||
```
|
||||
|
||||
This is the whole read-side change. It works because **every read already funnels
|
||||
through `lineage.path_of()`**, so one substitution moves the entire application —
|
||||
transcript paging, context assembly, `attempts.preceding`, and memory retrieval —
|
||||
onto a head that can sit behind the deepest node.
|
||||
|
||||
A `Path.uncapped()` helper was added for the two callers that must deliberately
|
||||
look past the head (redo, and the fork check).
|
||||
|
||||
`prefix_covering` already computed `top = self.tip if max_depth is None else
|
||||
max_depth`, so the windowing arithmetic was consistent with this reading before
|
||||
the change; nothing there needed touching.
|
||||
|
||||
### 2. Undo — move the head instead of deleting
|
||||
|
||||
`undo_turn` keeps its turn lock, its "nothing to undo" guard and its
|
||||
fork-boundary guard, keeps `attempts.preceding` + `attempts.restore_state` for
|
||||
the state rollback, and replaces the two `delete_turn` calls plus
|
||||
`tree.refresh_head` with one assignment:
|
||||
|
||||
```python
|
||||
adventure.head_depth = (first_removed.depth or 0) - 1
|
||||
```
|
||||
|
||||
No `db.delete`. No `memorybank.forget_node`.
|
||||
|
||||
### 3. Write path — fork when writing below the head
|
||||
|
||||
`turns.fork_if_behind_head` runs in `create_action` beside the existing
|
||||
`_move_to_after`. If any live node on the path sits deeper than the head, it calls
|
||||
the **already-existing** `tree.branch_at(db, adventure, adventure.head_depth)`,
|
||||
which creates an empty branch leaving the path at that depth and does not touch
|
||||
the branch being left. When the head is at the tip it does nothing, so a story
|
||||
that is never undone forks exactly as often as before.
|
||||
|
||||
### 4. Redo — walk the head forward
|
||||
|
||||
New `POST /adventures/{id}/redo`. Reads the path *uncapped*, takes the next node
|
||||
deeper than the head, and advances over the whole turn (player action + AI reply)
|
||||
rather than half of it. 400 when there is nothing to redo.
|
||||
|
||||
## Results
|
||||
|
||||
### Undo deletes nothing; redo round-trips exactly
|
||||
|
||||
Six actions across three turns, live Ollama:
|
||||
|
||||
```
|
||||
AFTER 3 TURNS head=(branch 1, depth 5) rows 1..6 all live
|
||||
visible: [1 story, 2 ai, 3 do, 4 ai, 5 do, 6 ai]
|
||||
|
||||
UNDO x1 rows 6 -> 6 DELETED=0 head=(1, 3)
|
||||
visible: [1 story, 2 ai, 3 do, 4 ai]
|
||||
|
||||
UNDO x2 more head=(1, -1)
|
||||
visible: [] rows still 6
|
||||
|
||||
REDO x3 head=(1, 5) rows 6
|
||||
visible: [1 story, 2 ai, 3 do, 4 ai, 5 do, 6 ai]
|
||||
```
|
||||
|
||||
Elsewhere in the run, **11 consecutive undos** were performed on a 23-action
|
||||
story with 0 rows deleted, which clears the specification's "at least five undo
|
||||
operations, unlimited preferred" comfortably.
|
||||
|
||||
### Writing below a moved-back head forks, and the abandoned line survives
|
||||
|
||||
```
|
||||
UNDO once, then write a different continuation:
|
||||
|
||||
1 story br1 d0 live 5 do br1 d4 live <- the abandoned tail,
|
||||
2 ai br1 d1 live 6 ai br1 d5 live still live, still on br1
|
||||
3 do br1 d2 live
|
||||
4 ai br1 d3 live 7 do br2 d4 live <- the new line
|
||||
8 ai br2 d5 live
|
||||
|
||||
branches: {id 1, parent null, fork_depth null, own_actions 6, is_head False}
|
||||
{id 2, parent 1, fork_depth 3, own_actions 2, is_head True}
|
||||
|
||||
visible on the new line: [1, 2, 3, 4, 7, 8]
|
||||
switch to branch 1 : [1, 2, 3, 4, 5, 6]
|
||||
```
|
||||
|
||||
This is `DECISIONS/005-branch-preserving-history.md` behavior: the abandoned
|
||||
future is retained as an alternate branch rather than erased, and it is reachable
|
||||
through the existing branch switcher with no new UI concept.
|
||||
|
||||
### Memory isolation still holds — the important regression check
|
||||
|
||||
A 23-action story with a real memory embedded via `nomic-embed-text` at depth 5.
|
||||
Probed with terms unique to the turns an undo hides, plus terms from the turns it
|
||||
keeps:
|
||||
|
||||
```
|
||||
AT TIP (control) head_depth=22 memories_used=1 (similarity 0.689)
|
||||
hidden-turn terms leaking: ['six bullets','warehouse nine','iron stairs',
|
||||
'ledger','docks','office desk','inside my coat']
|
||||
(correct — nothing is hidden at the tip)
|
||||
visible-turn terms present: ['rainy city','desk drawer']
|
||||
|
||||
11 undos, 0 rows deleted (actions still 23)
|
||||
|
||||
AFTER UNDO head_depth=3 memories_used=0
|
||||
hidden-turn terms leaking: NONE
|
||||
visible-turn terms present: ['rainy city','desk drawer']
|
||||
|
||||
REDO back to tip head_depth=22 memories_used=1 (similarity 0.689)
|
||||
```
|
||||
|
||||
The memory at depth 5 stops being retrieved when the head moves behind it and
|
||||
becomes eligible again on redo — **without being deleted**. That is the property
|
||||
the destructive undo bought by calling `forget_node`, recovered for free, because
|
||||
`Memory` rows carry `branch_id` and `depth` and retrieval already goes through
|
||||
the same capped clause.
|
||||
|
||||
> One false alarm worth recording: an early probe reported `revolver` leaking. It
|
||||
> was legitimate visible history — actions 22 and 23 sit at depths 2 and 3, at or
|
||||
> below the head. A second false alarm came from probing the context *after* the
|
||||
> script had already redone to the tip. Both were resolved by re-probing inside a
|
||||
> single script with an explicit control, which is why the table above reports the
|
||||
> control run alongside the result.
|
||||
|
||||
### Test suite: 627 passed, 5 failed
|
||||
|
||||
```
|
||||
5 failed, 627 passed in 184s (0.8% of the suite)
|
||||
|
||||
tests/test_attempt_siblings.py::test_undo_takes_every_attempt_with_it
|
||||
tests/test_branch_forking.py::test_undo_stops_at_the_fork
|
||||
tests/test_state_revert.py::test_undo_reverts_state_to_before_the_turn
|
||||
tests/test_state_revert.py::test_undo_of_bare_continue_uses_the_node_in_front
|
||||
tests/test_state_revert.py::test_undo_prunes_memory_covering_removed_actions
|
||||
```
|
||||
|
||||
Every one asserts that rows or memories were **deleted**:
|
||||
|
||||
- `assert [a.type for a in adv.actions] == ["start"]` (three of them)
|
||||
- `assert len(_rows(...)) == rows_before - 1`
|
||||
- `assert texts == {"k"}` — the pruned-memory set
|
||||
|
||||
Crucially, the *state* assertions inside those same tests still pass. In both
|
||||
`test_state_revert` cases, `assert adv.script_state == {"gold": 0}` succeeds and
|
||||
only the row-count line fails — state rollback is intact. And
|
||||
`test_undo_stops_at_the_fork` still enforces its real subject: the guard
|
||||
refusing to undo into a parent branch is untouched and still returns 400.
|
||||
|
||||
Also worth noting for the open question raised in the recommendation: the four
|
||||
memory suites — `test_memory_settling`, `test_memory_nodes`,
|
||||
`test_memory_retrieval`, `test_memory_rewrite` — and `test_history_window` all
|
||||
**passed unchanged**. The cursor arithmetic did not need re-deriving.
|
||||
|
||||
## Rough edges found, not fixed
|
||||
|
||||
1. **Undo can empty the story.** The original guard refuses when the newest node
|
||||
is `type == "start"`, but an adventure opened with a user-written `story`
|
||||
action has no `start` node, so undo walks to `head_depth = -1` and the
|
||||
transcript renders empty. Redo recovers it, but the floor should be the
|
||||
opening node rather than `-1`.
|
||||
2. **Retry and `add_take` were left on the old path.** The spike only routed the
|
||||
ordinary write through `fork_if_behind_head`. Retrying while the head is
|
||||
behind the tip is not yet defined and needs a decision — most likely the same
|
||||
fork.
|
||||
3. **No pruning story yet.** Abandoned turns now accumulate. That matches the
|
||||
specification ("retained but marked disposable; cleanup later"), but nothing
|
||||
marks them disposable and no cleanup exists.
|
||||
4. **Frontend untouched.** There is no Redo button and the branch panel does not
|
||||
distinguish a line abandoned by undo from one forked deliberately.
|
||||
|
||||
## Conclusion
|
||||
|
||||
The retrofit lands where the recommendation predicted: one chokepoint
|
||||
(`lineage.Path.clause`), one stored head that already existed
|
||||
(`adventures.head_depth`), and one branch primitive that already existed
|
||||
(`tree.branch_at`). The unknown was whether the depth cap would disturb the
|
||||
memory and summary cursors, and it does not.
|
||||
|
||||
The fork decision is sound. The remaining work on this axis is the four rough
|
||||
edges above plus named checkpoints, not a redesign.
|
||||
Reference in New Issue
Block a user