parththakkar106andClaude Opus 4.8
dab1807118
The shared per-adventure script_state ("scoreboard" scripts write to) was
never reverted by undo, and retry re-ran the output hook on top of the already-
mutated state, double-applying its changes (e.g. "+10 gold" became +20).
Each action now snapshots script_state as it was immediately before its own
hooks ran (new Action.state_before column, migration 25):
- undo restores the turn's first-action snapshot, prunes memories that
summarized the removed actions, and takes the turn lock against races.
- retry restores the AI action's snapshot before regenerating.
Story-card mutations are not reverted (documented limit). Adds the project's
first test suite: unit + full HTTP integration through the real scripting
engine (14 tests). See plan/11-state-revert-and-retry-fix.md.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
82 lines
3.5 KiB
Markdown
82 lines
3.5 KiB
Markdown
# Plan: undo/retry state revert (+ concurrency lock)
|
|
|
|
Fixes three linked issues around `script_state` (the shared per-adventure
|
|
"scoreboard" scripts write to) and undo/retry.
|
|
|
|
## Background
|
|
|
|
- `script_state` is one shared dict on `Adventure` (`models.py:90`), mutated in
|
|
exactly ONE place: `pipeline.py:89` (`self.adventure.script_state = state`).
|
|
- Today, undo (`adventures.py:445`) and retry (`:422`) delete *actions* but never
|
|
touch `script_state`, so state never rolls back.
|
|
|
|
## Issue 1 — retry double-applies state (pre-existing bug)
|
|
|
|
Retry deletes the last AI action and regenerates. The output hook already mutated
|
|
`script_state` on the first attempt; regenerating runs it again, stacking the change
|
|
(e.g. "add 10 gold" → 20 gold after one retry). Same root cause as undo not
|
|
reverting.
|
|
|
|
## Issue 4 — undo has no concurrency guard
|
|
|
|
Turns take `acquire_turn_lock` (`:189`); undo does not, so undo can race a turn
|
|
that is still streaming.
|
|
|
|
## Issue 2 — Memory Bank leftovers (smaller than expected)
|
|
|
|
`run_post_turn` already clamps `memory_cursor`/`summary_cursor` down to the current
|
|
action count (`memorybank.py:178-182`), so there is NO cursor stall. The only
|
|
remainder: a `Memory` created from a turn that was later undone stays behind, its
|
|
`source_start/source_end` now pointing past the end of the story.
|
|
|
|
---
|
|
|
|
## The fix
|
|
|
|
### 1. Snapshot state per turn (Issue 1 + enables undo revert)
|
|
|
|
- Add column `state_before: JSON nullable` to `Action` (`models.py`).
|
|
- Migration: append `(25, "ALTER TABLE actions ADD COLUMN state_before JSON")`
|
|
to `migrations.py`. `JSON` is valid on both SQLite and Postgres.
|
|
- In `run_player_turn` / `generate_turn`, when the FIRST action of a turn is
|
|
created, stash `copy.deepcopy(adventure.script_state)` onto it — captured
|
|
*before* any hook runs. (Player action for do/say/story; the AI action for a
|
|
bare `continue`.)
|
|
- Fix retry directly: before regenerating, restore
|
|
`adventure.script_state` from the deleted AI action's `state_before` so the
|
|
output hook starts from the pre-turn scoreboard instead of the mutated one.
|
|
|
|
### 2. Revert on undo (Issue depends on #1)
|
|
|
|
- In `undo_turn`, after deleting the popped actions, set
|
|
`adventure.script_state = <deleted player/first action>.state_before` (fall back
|
|
to `{}` if null, i.e. pre-migration turns), then commit.
|
|
- Only wire this into undo + retry — NOT the arbitrary
|
|
`delete_action` endpoint (`:830`); mid-history state revert is undefined.
|
|
|
|
### 3. Lock undo (Issue 4)
|
|
|
|
- Wrap `undo_turn` body in `acquire_turn_lock(adventure_id)` /
|
|
`_active_turns.discard(...)` in a `finally`. It's synchronous (not SSE), so no
|
|
`with_turn_lock` wrapper needed — just acquire and discard.
|
|
|
|
### 4. Clean up dangling memories (Issue 2, optional)
|
|
|
|
- In `undo_turn` after deleting actions, delete any `Memory` whose `source_start`
|
|
is >= the new story-action count (i.e. summarized a turn that no longer exists).
|
|
Cursors already self-heal, so this is polish, not correctness.
|
|
|
|
## Known limits (document, don't fix)
|
|
|
|
- Story cards a script created (`_apply_cards`, `pipeline.py:87`) are NOT reverted —
|
|
only text + script_state roll back.
|
|
- Pre-migration turns have `state_before = NULL` → undo falls back to `{}`.
|
|
- Demo turn cap is not refunded on undo (intentional).
|
|
|
|
## Test checklist
|
|
|
|
- Script that increments a counter: play → undo → counter back to prior value.
|
|
- Same script: play → retry → counter changes once, not twice.
|
|
- Undo during an active stream returns 409, doesn't corrupt state.
|
|
- Undo a turn old enough to have been summarized: no orphaned memory left.
|