JesseMarkowitzandClaude Opus 5
ba737de9b4
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
215 lines
9.0 KiB
Markdown
215 lines
9.0 KiB
Markdown
# 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.
|