interactive-story/planning/reports/M4-IMPLEMENTATION-REPORT.md

# M4 Implementation Review Report

**Milestone:** M4 — Named Save Points / Checkpoints
**Reviewed:** 2026-09-03
**Tree reviewed:** `e08d49c`, working tree clean, nothing staged or untracked
**Reviewer note:** this document is the input to M4's acceptance decision. It does
not accept M4, and it does not authorize M5.

---

# A. Executive Result

```text
PASS WITH CORRECTIVE WORK REQUIRED
```

M4's Definition of Done is met and demonstrated:

> The user can create a named Save Point, continue, restart, restore it, and
> continue differently without losing later history.

Every clause of that sentence was exercised against a running server across a
genuine operating-system process restart, and the load-bearing architectural
constraint holds under inspection rather than merely by assertion: **restore is
head movement**, and the only head-field assignment M4 introduced anywhere in the
backend lives in `head.py`, not in the checkpoint router.

The corrective work is **not** in the head model. Three findings are recorded in
§R, none of which changes what the feature does:

1. `GET /checkpoints` is an N+1 that fetches whole `Action` rows including prose —
   53 SELECTs for 25 Save Points against 4 for the comparable branch endpoint,
   in a codebase that keeps a regression test specifically about this class of
   mistake (**medium**, M4 corrective);
2. deleting a branch silently deletes Save Points that name it, and the branch
   panel's confirmation does not say so — M4 added a consequence to an existing
   destructive action without updating its warning (**medium**, M4 corrective);
3. the automated D11/L03 tests restart a client, not a process, so the shipped
   suite is weaker than the claim it is named for (**low**, M4 corrective).

## Qualifications, stated up front

- **The browser smoke test was NOT PERFORMED, for M4 and still for M3.** No
  usable browser exists in this environment (§M). Every browser-facing claim in
  this report is source inspection plus an HTTP-level equivalent, and is labelled
  as such. This is now an open condition on two consecutive milestones.
- **There are no frontend tests at all**, so the Save Point panel has zero
  automated coverage. That is inherited (M8 owns it), not M4's regression, but it
  means §L is inspection only.
- The world state is used as **deterministic instrumentation** throughout, exactly
  as `test_head_cursor.py` uses it. Nothing here endorses the RPG state model; M5
  replaces it, and §U reports what M5 will have to move.

---

# B. Repository and Provenance

| Fact | Value |
| --- | --- |
| Branch | `m3-nondestructive-history` |
| M4 started from | `3c8e91f` — *Docs: correct post-M3 status and Ollama configuration* |
| M4 commits | **one**: `e08d49c` — *M4: add durable named Save Points* |
| Current HEAD | `e08d49c` |
| Signature | **Good signature**, RSA key `02C9BF7D8A4A77DF7A8905617D8AE19DB5C68569` (JesseMarkowitz) |
| Working tree | **clean** before this report; no staged, unstaged or untracked M4 files |
| Upstream ancestry | `git merge-base --is-ancestor d72f7c1b… HEAD` → **intact** |
| LICENSE | **unchanged by M4** (`git diff 3c8e91f..HEAD -- LICENSE` is empty) |

**There is no staged-but-uncommitted M4 work.** Unlike M3 — whose report had to
disclose that the milestone was staged and unsigned-commit-blocked (archived M3
report §B.1) — M4 is committed and signed. Every test result in this report was
run against that commit, with a clean tree, so *committed HEAD*, *staged* and
*unstaged* are the same tree and the distinction is not load-bearing here.

The only change made **by this reporting pass** is the report-path rotation in
§V, which touches no application code.

---

# C. Change Inventory

`git diff --stat 3c8e91f..e08d49c` — 20 files, **+2272 / −26**.

## C.1 By subsystem

| Subsystem | Files | Net | What changed |
| --- | --- | --- | --- |
| Schema | `models.py`, `migrations.py` | +65 | `Checkpoint` model; migration 80 (index) |
| Wire schema | `schemas.py` | +66 | `CheckpointOut` / `CheckpointCreate` / `CheckpointRename`, `CHECKPOINT_NAME_MAX` |
| History | `head.py` | +34 | **one** new function, `move_to_node` |
| API | `routers/adventures/checkpoints.py` (new), `__init__.py` | +260 | five endpoints |
| Export/import | `bundle.py` | +124 | export block, planner, writer |
| Frontend | `SavePointPanel.jsx` (new), `index.jsx`, `api.js`, `play.css` | +352 | panel, toolbar button, client, styles |
| Tests | `test_save_points.py` (new), `test_tree_migration.py` | +1140 | 42 new tests; one fixture repair |
| Docs | 6 `.md` files | +231 | recorded during implementation |

**Files added (3):** `backend/app/routers/adventures/checkpoints.py`,
`backend/tests/test_save_points.py`,
`frontend/src/pages/Play/panels/SavePointPanel.jsx`.
**Files deleted:** none.

`test_tree_migration.py` was modified for a real reason, not cosmetics: its two
fixtures drop tables by hand, and `checkpoints` references `branches` and
`adventures`, so SQLite refused the drop and 24 tests errored. `checkpoints` was
added to the front of both drop lists. This is a fixture that must be extended
whenever a referencing table is added, and the added comment now says so.

## C.2 The §3 questions, answered from source

| # | Question | Answer |
| --- | --- | --- |
| 1 | What record represents a Save Point? | One row in `checkpoints` (`models.py:244`) |
| 2 | What fields? | `id`, `adventure_id`, `name` VARCHAR(120), `note` TEXT, `branch_id`, `depth`, `created_at`, `updated_at` |
| 3 | Coordinate or copy? | **Coordinate.** No transcript, state, prompt, memory, summary or branch content is stored. Verified column-by-column in §O |
| 4 | How is the position captured? | `head.node_at(db, adventure, adventure.head_depth)` — the resolved node **at the active head**; its own `branch_id`/`depth` are stored |
| 5 | How is it resolved on restore? | `_node_at()` — a direct query for the **live** action at `(branch_id, depth)`, not scoped to any path |
| 6 | What moves the head? | `head.move_to_node` → `head.move_to` (M3, unchanged) |
| 7 | Does restore call M3's mechanism? | **Yes.** §F traces it |
| 8 | Any separate state/history restoration? | **No.** §F.2 proves it by exhaustive grep of the diff |
| 9 | Does restore create a branch? | **No.** Measured in §G.2: branch count unchanged across restore |
| 10 | What creates the branch? | The **first write below the restored head**, through M3's `fork_if_behind_head` |

## C.3 The `(branch, depth)` decision

The brief permitted a coordinate rather than `DATA-MODEL.md` §8's `turn_id`. The
implementation took that permission, and review finds the choice is not merely
acceptable but **the more correct pointer**, for a reason that is demonstrable
rather than argued:

A coordinate holds every attempt at a turn, exactly one of them live. Retrying a
turn makes a new live sibling at the same coordinate. Measured (§H.4):

```text
save point at (branch 4, depth 3)
live row at that coordinate BEFORE retry:  id 17  "Beat 9."
rows at that coordinate AFTER  retry:      id 17 (live=False), id 18 (live=True)
live row AFTER retry:                      id 18  "Beat 10."
Save Point still resolves: True — and now names the new live take
```

A `turn_id` pointer would have pinned row 17 — a take the story no longer tells.
The coordinate followed the story. §T recommends `DATA-MODEL.md` keep the record
of this.

---

# D. Save Point Persistence Model

```python
class Checkpoint(Base):
    __tablename__ = "checkpoints"
    id           INTEGER PK
    adventure_id INTEGER NOT NULL  FK adventures(id) ON DELETE CASCADE
    name         VARCHAR(120) NOT NULL
    note         TEXT NOT NULL
    branch_id    INTEGER NOT NULL  FK branches(id)   ON DELETE CASCADE
    depth        INTEGER NOT NULL
    created_at   DATETIME NOT NULL
    updated_at   DATETIME NOT NULL   -- onupdate; bumped by rename only
```

Confirmed by live introspection after migration (§O), not from the model file.

**`branch_id` is the node's own branch, not the branch being read at creation.**
Those differ whenever the head rests in a shared prefix. Storing the node's branch
is what keeps the pointer meaningful after the reader forks away, and it is why
`_node_at` can be a direct coordinate lookup with no lineage reasoning.

**Nothing derived is stored.** `turn`, `on_path` and `resolved` in the API
response are computed per request; only name, note and coordinate persist.

---

# E. API Review

All five endpoints are campaign-scoped under `/api/adventures/{adventure_id}`,
and all resolve the adventure through the existing `current_adventure` ownership
dependency before the handler body runs.

| Method | Path | Request | Success | Notes |
| --- | --- | --- | --- | --- |
| GET | `/checkpoints` | — | 200 `[CheckpointOut]` | newest-first |
| POST | `/checkpoints` | `{name, note?}` | **201** `CheckpointOut` | position is *not* a field |
| PATCH | `/checkpoints/{id}` | `{name?, note?}` | 200 `CheckpointOut` | label only |
| DELETE | `/checkpoints/{id}` | — | **204** | pointer only |
| POST | `/checkpoints/{id}/restore` | — | 200 `ActionPage` | takes the turn lock |

## E.1 Validation and error handling — measured

| Case | Result | Correct? |
| --- | --- | --- |
| blank name `"   "` | **400** "A Save Point needs a name." | yes |
| missing `name` field | **422** (Pydantic) | yes |
| name > 120 chars | **422** | yes |
| name stored | trimmed (`"  X  "` → `"X"`) | yes |
| rename to blank | **400**, original name intact | yes |
| unknown checkpoint id | **404** | yes |
| unknown adventure id | **404** | yes |
| double DELETE | 204 then **404** | yes |
| coordinate no longer live | **409** + head does not move | yes |
| empty story (head at NO_DEPTH) | **400** "no turn here to save yet" | yes |

## E.2 Cross-campaign — measured

A Save Point belonging to campaign A, addressed through campaign B:

```text
POST   /adventures/{B}/checkpoints/{A_sp}/restore  -> 404
PATCH  /adventures/{B}/checkpoints/{A_sp}          -> 404
DELETE /adventures/{B}/checkpoints/{A_sp}          -> 404
GET    /adventures/{B}/checkpoints                 -> []
```

`_get_or_404` matches `checkpoint.adventure_id` against the adventure in the
path, so a foreign id is *not found* rather than *found and acted on*. This is
the right shape: it cannot restore the wrong story, and it does not leak whether
the id exists elsewhere.

## E.3 CREATE captures the active head — measured

The distinction that only exists because M3 stopped Undo deleting:

```text
head after 5 turns:                    (branch 1, depth 10)
head after 2 undos:                    (branch 1, depth  6)
Save Point created here:               branch_id 1, depth 6, turn 7
captured the ACTIVE HEAD, not the tip: True
```

## E.4 LIST shape

Returns `id, adventure_id, name, note, branch_id, depth, turn, on_path,
resolved, created_at, updated_at`. `turn` (= `depth + 1`) is what the panel
shows; `on_path` and `resolved` drive the two states the panel must distinguish.

Two observations for the reviewer:

- **Ordering is newest-created-first, not story order.** The implementation's
  stated reason is sound — depths on lines that have parted company are not
  comparable, so story order would draw a sequence no reading passes through.
  `BROWSER-UX-SPEC.md` §25 does not specify an order. **No defect**, but it is an
  undocumented product decision; §T recommends recording it.
- `branch_id` **is** exposed. It is not used by the panel for any
  branch-management purpose (the panel shows "on a path you left" instead), so
  this does not leak branch complexity into the UI, but it is more than the UI
  needs.

---

# F. Restore / Active-Head Integration

**This is the load-bearing review item, and it holds.**

## F.1 The trace

```text
POST /adventures/{id}/checkpoints/{cp}/restore     checkpoints.py:217
  -> _get_or_404(db, adventure, cp)                checkpoints.py:81   ownership
  -> turns.acquire_turn_lock(adventure_id)         turns.py            same lock as undo/redo
  -> _node_at(db, adventure, branch_id, depth)     checkpoints.py:38   coordinate -> live Action
       -> None  => HTTPException(409); head unmoved
  -> head.move_to_node(db, adventure, node)        head.py:335         THE MOVER
       -> lineage.path_of(...).uncapped().contains(node)   is it on this line?
       -> (only if not) adventure.head_branch_id = node.branch_id      head.py:346
       -> head.move_to(db, adventure, node.depth)          head.py:302  M3, UNCHANGED
            -> attempts.restore_state(adventure, node_at(...))          M3, UNCHANGED
  -> adventure.updated_at; db.commit()
  -> current_window(db, adventure)                 paging.py           the same window undo/redo return
```

`head.move_to` and `attempts.restore_state` are M3 code, untouched by M4
(`git diff 3c8e91f..HEAD -- backend/app/head.py` adds a function and changes no
existing line; `attempts.py` is not in the M4 diff at all).

## F.2 Did M4 build a second mechanism? — exhaustive grep of the diff

Every added backend line matching each forbidden category:

```text
writes head fields:      2 hits
    head.py:346   adventure.head_branch_id = node.branch_id
    checkpoints.py  node = head.node_at(db, adventure, adventure.head_depth)   [a READ]
forks / prunes / recomputes:  0 hits
    (branch_at|tree.fork|forget_node|redo_target|restore_state|refresh_head|mark_superseded)
deletes rows:            1 hit
    checkpoints.py  db.delete(checkpoint)      [the pointer row itself]
```

Independently confirmed: `grep "head_branch_id = \|head_depth = "` against
`checkpoints.py` returns **nothing**. The single assignment lives at
`head.py:346`.

So M4 introduced **no** separate code that sets head fields in a router,
reconstructs state, filters the transcript, prunes memories, calculates Redo, or
creates branches.

## F.3 The one architectural call M4 had to make

`move_to_node` moves the **branch** half of the head only when the coordinate is
not on the uncapped path being read. ADR 012 defines the head as a
`(branch, depth)` coordinate but does not say what restoring to a coordinate on a
*departed* line should do.

Both directions were verified, and both matter:

- **Conditional is required for correctness.** After a divergence, restoring to a
  Save Point in the shared prefix must leave the reader on the *new* line. Test
  `test_restore_onto_an_inherited_position_keeps_the_line_being_read` asserts the
  head branch is unchanged and Redo walks into the new continuation. Moving
  unconditionally would silently hand back the abandoned story.
- **The move is required for §19.** A Save Point survives divergence, so one can
  name a position on a displaced line, which no depth movement reaches. Test
  `test_a_save_point_on_a_line_the_story_left_still_restores` restores it and
  lands on `(old_branch, old_depth)` with the old line's state.

**Review judgement: this needs no new ADR.** It applies ADR 012's mechanism to
both halves of a coordinate ADR 012 already defines. `TECHNICAL-DESIGN.md` §8.8
records it, which is the right home. §T concurs with the implementation's own
decision not to write ADR 013.

## F.4 Behaviour equivalence, measured

`test_restore_is_the_same_head_movement_undo_makes` asserts that arriving at a
position by Restore and arriving by Undo produce an identical
`(head, world state, transcript)` triple. It passes.

---

# G. Non-Destructive Restore and Divergence

The §6 scenario, run with objective identifiers.

## G.1 Restore keeps everything

```text
5 turns played; head (branch 1, depth 10); 11 rows
Save Point "Before entering the abbey" -> branch 1, depth 6

row ids BEFORE restore: [1,2,3,4,5,6,7,8,9,10,11]
row ids AFTER  restore: [1,2,3,4,5,6,7,8,9,10,11]
RESTORE DELETED ZERO ROWS: True          <- identity, not a count

head after restore:      (branch 1, depth 6)     == the Save Point
world state:             gold 30  (was 50 at the tip)
transcript ends at:      "Beat 3."
can_redo after restore:  True
```

Redoing twice returns to the tip with **transcript identical** and **gold
identical** to before the restore.

## G.2 Divergence is caused by the write, not by Restore

```text
branches BEFORE the divergent write: [1]      <- after restore; restore created nothing
branches AFTER  the divergent write: [1, 2]
FORK CAUSED BY THE WRITE: True

can_redo after divergence:    False
POST /redo after divergence:  400
all original row ids present: True
```

The resulting tree, showing the displaced future retained on branch 1 while the
active line continues on branch 2:

```text
id  br depth live  text
 1   1   0   True  The road forks
 2   1   1   True  > You turn 0.
 …
 7   1   6   True  Beat 3.          <- the Save Point's coordinate
 8   1   7   True  > You turn 3.    ┐
 9   1   8   True  Beat 4.          │ displaced future, retained on branch 1
10   1   9   True  > You turn 4.    │
11   1  10   True  Beat 5.          ┘
12   2   7   True  > You go aroun   ┐ new continuation on branch 2
13   2   8   True  Beat 6.          ┘
```

## G.3 The Save Point survives its own divergence

After the fork, the Save Point still reports `(branch_id 1, depth 6)` —
unchanged — and restoring it again returns to that position on the *new* line.
`STORY-BRANCH-SEMANTICS.md` §19 and §21 are satisfied.

---

# H. Save Point Durability

| Operation | Save Point survives? | Evidence |
| --- | --- | --- |
| ordinary continuation | yes | §G, and every test in the module |
| Undo | yes | `test_restore_after_undo_and_redo_activity_lands_where_the_name_says` |
| Redo | yes | same |
| **Retry** | yes, **and follows the new live take** | §H.4 below |
| Restore | yes | `test_restoring_the_same_save_point_repeatedly_is_stable` |
| restore + divergence | yes, coordinate unchanged | `test_d13_the_save_point_still_names_the_same_position_after_divergence` |
| rename | yes, coordinate unchanged | `test_rename_changes_the_label_and_not_the_coordinate` |
| **OS process restart** | yes | §H.5 — real two-process run |
| branch deletion | **NO — cascaded away** | §R-2; by design, but unwarned |

## H.1–H.3 Positions and multiplicity

| Case | Result |
| --- | --- |
| Save Point at campaign opening/root | restores to the opening; gold 0 |
| Save Point at the current tip | restore is a no-op; head/state/transcript identical |
| created while head is behind the retained tip | saves the **undone** position (§E.3) |
| several Save Points at different positions | each restores to its own transcript and state |
| two Save Points at one position | allowed; both restore correctly |
| repeated restore of one Save Point | stable across 3 consecutive restores |

**No uniqueness is imposed on names or positions.** Nothing in the product
requirements asks for it. §T recommends this be recorded rather than left
implicit.

## H.4 Retry — the strongest evidence for the coordinate design

```text
save point at (branch 4, depth 3)
live row at that coordinate BEFORE retry: id 17 "Beat 9."   live=True
rows at that coordinate AFTER  retry:     id 17 "Beat 9."   live=False
                                          id 18 "Beat 10."  live=True
Save Point resolves: True     restore: 200, head (4, 3)
```

## H.5 Restart — a real process boundary

The shipped tests restart a *client*, not a process (§R-3). For this review the
sequence was re-run over HTTP against `uvicorn`, with the server **killed** and a
**second process** started on the same database file:

```text
[server process 1]  save point "Before entering the abbey" at depth 5
                    state at save point: gold 6
                    state at tip:        gold 28      (14 rows)
--- process 1 killed; port confirmed dead (probe 000); process 2 started ---
[server process 2]  PASS  D11 Save Point survives an OS process restart
                    PASS  D11 name survived
                    PASS  D11 coordinate survived  (branch 1, depth 5)
                    PASS  story survived the restart
                    PASS  L03 transcript restored after restart  (6 turns)
                    PASS  L03 STATE restored after restart  gold 28 -> 6
                    PASS  restore deleted zero rows   14 == 14
                    PASS  later history still present 14 == 14
                    PASS  Redo available before divergence
                    9/9 passed
```

## H.6 Undocumented limits found

- name capped at **120** characters (422 beyond); not stated in any planning doc.
- `note` is capped by the shared `Prose` type at 50 000 characters.
- no cap on the **number** of Save Points per campaign. With the §R-1 N+1, a
  campaign with many Save Points makes the list endpoint linearly slower. Not a
  present danger; recorded.

---

# I. Export / Import (I04)

## I.1 Representation

Bundle format is **unchanged**: `ai-dnd-adventure-v2`, no version bump. A new
top-level `checkpoints` array:

```json
{
  "name": "Before entering the abbey",
  "note": "door was locked",
  "branch": 0,
  "depth": 4,
  "createdAt": "2026-09-03T22:53:52.611529"
}
```

`branch` is the file-local branch index, as every other branch reference in the
bundle is; `depth` needs no translation. This follows the module's stated rule —
a bundle carries what was *chosen* — and a named position is not recoverable from
the turns, so it belongs in the file. No bump is correct: an absent key is
unambiguous, which is the same reasoning `headDepth`, the persona block and the
branch disposition already rely on.

## I.2 Round trip — measured

Source: three Save Points across **two** branches, head deliberately left behind
the tip.

```text
source head: (branch 2, depth 6)   [behind the tip]
source save points: ('On the new road', 2, 6) ('In the cloister', 1, 8)
                    ('Before entering the abbey', 1, 4)

import -> 201
imported save points: ('On the new road', 4, 6) ('In the cloister', 3, 8)
                      ('Before entering the abbey', 3, 4)      all resolved=True
distinct branches among them:      2
row ids differ from source:        True   (fresh rows)
branch ids remapped 1->3, 2->4:    True
imported story == source story:    True
imported opens undone (can_redo):  True
```

Restoring each imported Save Point in the new campaign lands on a **different**
head with a **different** state, proving the coordinates were remapped correctly
rather than collapsed:

```text
restore 'On the new road'              -> 200, head (4,6), 7 turns, gold 30
restore 'In the cloister'              -> 200, head (3,8), 9 turns, gold 40
restore 'Before entering the abbey'    -> 200, head (3,4), 5 turns, gold 20
```

## I.3 The head stays authoritative

Save Points are written by `_write_checkpoints`, which runs **after**
`_point_the_head` and only inserts rows. The imported head came from `headDepth`,
and the campaign opened undone. `test_i04_an_import_opens_where_the_bundle_was_read_not_at_a_save_point`
is the clean assertion (`saved["depth"] < head_depth`), because in the ad-hoc run
above the head depth and one Save Point's depth coincided by construction.

## I.4 Malformed and pre-M4 files

```text
pre-M4 bundle (no `checkpoints` key):  201, save points [], story intact, head honoured
malformed checkpoints:                 201, survivors: ['Real one']
    dropped: depth 9999 (off the end), blank name, branch 77 (no such branch),
             missing depth, a bare string
```

Dropping rather than refusing is a deliberate asymmetry with the head depth, and
review finds it correct: a misplaced head corrupts every read in the file, while
a bookmark pointing outside the story affects only itself. Rejecting a whole
campaign to protect one bookmark would lose the story to save the pointer.

**I04: PASS.**

---

# J. State Reconstruction (L03)

Restore restores state because `head.move_to` calls `attempts.restore_state` with
the node at the destination — the M3 contract, unchanged. M4 added no state code.

Measured, across the real process restart of §H.5:

```text
state at the Save Point (before advancing): gold 6
state at the tip (4 further turns):         gold 28
--- process killed and restarted ---
after restore:                              gold 6      exact
transcript:                                 6 turns     exact
rows:                                       14 == 14    nothing deleted
```

The in-suite equivalents (`test_l03_*`) also pass. **L03: PASS**, on the
process-boundary evidence.

The world state is instrumentation here. What is actually demonstrated is that
*the state belonging to a position returns when the head returns to it*, which is
a property of the snapshot contract, not of the RPG schema. See §U.

---

# K. Context / Memory Lineage

M4 wrote no memory or context code. The claim under review is that head-capped
behaviour follows *automatically* through a Save Point restore. It does.

## K.1 Eligibility narrows and widens — measured

```text
memory written at the new line's head; visible:  ['MEM-NEW-LINE']
after restoring behind it, visible:              []            rows still: 1
after redo, visible:                             ['MEM-NEW-LINE']  rows still: 1
```

Nothing was pruned, deleted or re-embedded — the row count is 1 throughout. The
memory left and rejoined the story because `lineage.Path` caps at the head.

## K.2 E-series through a Save Point restore

| Test | Result | Evidence |
| --- | --- | --- |
| **E01** abandoned future cannot affect active state | PASS | `test_e01_a_memory_past_a_restored_head_stops_being_retrievable`; §G.1 shows state comes back to the restored position |
| **E01** old-future memory absent on a new continuation | PASS | `test_e01_an_old_futures_memory_stays_out_of_a_new_continuation` — still absent after the new line grows *past* the old depth |
| **E04** transcript holds only the active lineage | PASS | `test_e04_the_transcript_after_a_restore_holds_only_the_active_lineage` — displaced rows present in the tree, absent from the read |

## K.3 What could not be fully exercised — stated, not papered over

- **E03 / summaries over a long story.** The summary anchor is a `(branch, depth)`
  coordinate read through the same capped path, so it should behave; but no
  long-run campaign was played, and M3's report carried the same limitation to
  M6/M11. **Inherited from M3, not demonstrated by M4.**
- **Scene state (§9's "location/scene state").** There is no scene subsystem yet —
  it is M5 work. What exists is the world-state snapshot, which §J shows follows
  the head. **NOT APPLICABLE at M4**, and it must not be read as scene state
  being complete.
- **Real embedding-based retrieval.** The memory tests assert *eligibility* via
  the lineage clause, not ranking through a live embedding model. That is the
  same instrumentation M3 used.

---

# L. Browser UX — inspection only

**No browser was available (§M), so nothing in this section is observed
behaviour.** It is source inspection of `SavePointPanel.jsx` (209 lines),
`index.jsx` and `api.js`.

## L.1 Required surfaces exist

| `BROWSER-UX-SPEC.md` | Implemented | Where |
| --- | --- | --- |
| §4 Save Point in the control bar beside Undo/Redo/Retry | yes — `⚑ Save Point`, opens the panel | `index.jsx` toolbar |
| §23 label is "Save Point" | yes | throughout |
| §24 create dialog with a name, note optional | yes — form with name + optional note | panel |
| §25 list with Restore / Rename / Delete per entry | yes | panel |
| §26 restore confirmation explaining non-destructiveness | yes (wording differs, meaning matches) | panel |
| §27 delete confirmation clarifying no story is deleted | yes (wording differs, meaning matches) | panel |

## L.2 Terminology

Every occurrence of *branch*, *fork*, *node*, *head* and *lineage* in the panel is
in a **code comment or a CSS class name**. Zero appear in rendered text. A Save
Point on a departed line reads `· on a path you left`, and an unresolvable one
reads `· this moment is no longer in the story`.

**One deviation:** the spec's §25 sample shows `Turn 42`; the panel renders
`Moment 7`. "Moment" is the word the existing branch panel already uses
(`forked at moment N`), so this is internal consistency against a spec example.
**Not a defect**; §T asks the reviewer to rule on which word wins.

## L.3 Confirmation copy

| Spec meaning | Implemented text |
| --- | --- |
| "story will return… later history retained but no longer active" | "The story will return to this Save Point. Everything you wrote after it is kept — it just stops being where you are." |
| "deleting this save point does not delete story history" | "Delete this Save Point? Deleting it does not delete any of the story — only the name you gave this moment." |

Both required meanings are carried. The spec presents its text as an
explanation to convey, not a string to copy.

## L.4 UI-state audit

| Risk | Finding |
| --- | --- |
| stale list after create | handled — `setTick` refetches; form clears |
| stale list after rename/delete | handled — `run()` refetches on success |
| Restore does not refresh transcript/state | handled — `onRestored` → `adoptWindow`, the same callback the branch switch uses, which replaces the window and bumps `stateKey` |
| wrong enabled/disabled state | Restore is disabled when `!resolved`, with a `title` explaining why; row buttons disable on `busyId`; create disables on `saving` and on an empty name |
| editor closes on a refusal | handled — `run()` returns a boolean and rename keeps the editor open on failure |
| error path invisible | handled — errors route to the page toast via `onError` |
| double-click | mitigated by `busyId` / `saving`; not eliminated (React state is async), same exposure as the existing branch panel |
| modal does not close | the confirmations are inline, not modal; both close on success and on Cancel/Keep |

**Minor, non-blocking:** a successful restore triggers **two** list fetches — one
from `run()`'s `setTick`, one from `refreshKey` changing when `adoptWindow` bumps
`stateKey`. Harmless; recorded in §S.

---

# M. Browser Smoke Testing

## M.1 M4 browser smoke test — **NOT PERFORMED**

No usable browser exists in this environment. Re-checked at review time:

```text
DISPLAY / WAYLAND_DISPLAY   unset
google-chrome / chromium    NOT FOUND
firefox                     /usr/bin/firefox  (snap)
geckodriver                 /snap/bin/geckodriver
Xvfb / xvfb-run             NOT FOUND
selenium (py)               ModuleNotFoundError
playwright / puppeteer / cypress (node)   0 packages
```

Firefox was tried directly rather than assumed unusable. It prints
`*** You are running in headless mode.` and then emits snap mount-namespace
errors on every invocation and **hangs past 90 s on a trivial
`--screenshot about:blank`, producing no file**. `geckodriver` is present but has
no working browser to drive and no client library to drive it with.

**This test is NOT PERFORMED. It is not a PASS and must not be recorded as one.**
Unverified by observation: that the panel renders, that buttons enable and
disable correctly, that the confirmations appear, that Restore visibly moves the
transcript, and that the error toast is visible.

## M.2 M3 browser smoke test — **STILL NOT PERFORMED**

Reported separately and deliberately: this is M3's outstanding acceptance
condition, unchanged since the M3 report's §M.2 and §W.4. M4 did not perform it
and did not touch the Redo control's wiring.

**Two consecutive milestones now carry an unperformed browser requirement.** §R
records this as a standing acceptance risk rather than a defect in either
milestone's code.

## M.3 What was done instead

The §17 twenty-step sequence was driven over HTTP against a live `uvicorn`
process with a real process restart. **17/17 checks passed**, and the
D11/L03 subset was independently re-run for this review (§H.5, 9/9). This
exercises every endpoint the buttons call and every server-side behaviour the
sequence covers. It cannot exercise the DOM.

---

# N. Automated Test Results

All runs against `e08d49c`, clean tree.

| Suite | Command | Result |
| --- | --- | --- |
| **Full backend** | `pytest tests/ -q` | **680 passed**, 0 failed, 0 skipped, 1 warning, 167.85 s |
| **M4 targeted** | `pytest tests/test_save_points.py -q` | **42 passed**, 20.16 s |
| **M3 invariants** | 8 modules (head cursor, forking, branch mgmt, retry, take state, take edit, bundle v2, memory nodes) | **130 passed**, 54.73 s |
| **Security / local-only** | endpoint policy, TLS trust, local-only surface, offline assets, egress | **93 passed**, 9.23 s |
| **Frontend lint** | `npm run lint` | exit **0**; 7 warnings, byte-identical to the pre-M4 baseline |
| **Frontend build** | `npm run build` | exit **0** |
| **Docker build** | `docker build -t storyteller-m4-review .` | exit **0** |

Baseline before M4 was 638 passed. **680 − 638 = 42**, matching the M4 module
exactly: no existing test was deleted, and none was weakened to obtain green.
The one existing test file M4 modified (`test_tree_migration.py`) had a fixture
repaired, not an assertion changed — its 30 tests still pass.

The Docker build was run because M4 changed runtime schema behaviour (a new
table). It succeeds.

## N.1 M3 invariants specifically reconfirmed

| Invariant | Status |
| --- | --- |
| Undo deletes zero accepted rows | pass (`test_the_m3_invariant_undo_deletes_zero_accepted_turns`) |
| Redo exact round trip | pass (`test_l02_state_matches_the_position_in_both_directions`) |
| divergence preserves the old future | pass, and re-measured through Restore in §G.2 |
| Redo disappears after divergence | pass, and re-measured in §G.2 (400) |
| retry / take semantics | pass (`test_retry_variants`, `test_take_state`, `test_take_edit`) |
| active-head export/import | pass (`test_bundle_v2`, I07 in `test_head_cursor`) |
| memory isolation | pass (`test_memory_nodes`), re-measured through Restore in §K.1 |

---

# O. Migration / Backward Compatibility

## O.1 What M4 adds

**Migration 80**, the only one: `CREATE INDEX IF NOT EXISTS
ix_checkpoints_adventure ON checkpoints (adventure_id)`. The **table** is created
by `Base.metadata.create_all`, which `bootstrap` runs before the migration loop
on existing databases as well as fresh ones — the same route `memories` (v2) and
`branches` (v46) took. No backfill.

## O.2 A representative pre-M4 database — measured

A realistic M3 campaign was built (adventure, branch with lineage, 5 actions, a
memory), then its `checkpoints` table dropped and the stamp set to 79.

```text
BEFORE  stamp 79, checkpoints table present: False
AFTER   stamp 80  (LATEST_VERSION = 80), checkpoints present: True
columns  id INTEGER NOT NULL | adventure_id INTEGER NOT NULL | name VARCHAR(120) NOT NULL
         note TEXT NOT NULL  | branch_id INTEGER NOT NULL    | depth INTEGER NOT NULL
         created_at DATETIME NOT NULL | updated_at DATETIME NOT NULL
indexes  ix_checkpoints_adventure (adventure_id)
FKs      adventure_id -> adventures  ON DELETE CASCADE
         branch_id    -> branches    ON DELETE CASCADE

campaign untouched: ('An M3 campaign', head_depth 4, head_branch_id 1)
action rows 5 | memory rows 1 | phantom save points 0
second bootstrap: ok, stamp still 80          [idempotent]
```

An existing campaign with no Save Points **opens unchanged, with none invented**.

## O.3 Cascades and orphans — measured at the DB level

SQLite foreign keys are enforced (`database.py` sets `PRAGMA foreign_keys=ON` per
connection), so both cascades are real:

```text
DELETE branch     -> checkpoints on it: 1 -> 0
DELETE adventure  -> checkpoints in it: 1 -> 0
orphan count (checkpoints with no adventure): 0
NULL branch_id    -> IntegrityError: NOT NULL constraint failed
```

Via the API, deleting a campaign leaves **zero** orphaned checkpoint rows.
**Checkpoint rows cannot become orphaned.**

## O.4 Constraints on M5–M9

- The coordinate is `(branch_id, depth)` with a **NOT NULL** branch. If a later
  milestone introduces a position that is not on a branch, this schema will need
  a migration. Nothing planned through M9 requires that.
- The `checkpoints` table is **not** among the inert legacy tables awaiting the
  post-M5 cleanup migration, and should not be swept up in it.
- M5 changes what a snapshot *contains*, not where a position *is*. The
  coordinate is untouched by that. See §U.

---

# P. Security / Local-Only Regression

| Check | Result |
| --- | --- |
| new outbound calls | **none** — the M4 backend diff contains no `http://`, `https://`, `fetch(`, `httpx`, `urllib` or `requests` |
| telemetry / cloud dependency | none |
| new external assets | none; frontend build output has no new remote origin (`test_offline_assets` passes) |
| authentication / accounts | none; endpoints use the existing single-user ownership dependency |
| listener / binding changes | none |
| new credentials in the export | none — the `checkpoints` block carries `name`, `note`, `branch`, `depth`, `createdAt` and nothing else |
| endpoint policy / TLS | 93 security tests pass, unchanged |

## P.1 Save Point names as untrusted input

```text
name = <img src=x onerror=alert(1)>&"'
create: 201; stored verbatim; round-trips through the list unchanged
length cap 120 enforced (422)
```

The server stores names verbatim and does **not** HTML-escape them, which is
correct — escaping belongs to the renderer. The panel renders `{p.name}` and
`{p.note}` as JSX children, which React escapes; `grep` confirms **no
`dangerouslySetInnerHTML` and no `innerHTML`** anywhere in the panel. Names reach
the DOM as text.

Caveat, honestly stated: this is source inspection. Without a browser (§M) the
rendered escaping was not observed.

## P.2 Campaign scoping

A Save Point id from campaign A cannot restore, rename or delete through
campaign B — all three return 404 (§E.2). This is the one security-shaped
invariant M4 introduces, and it is covered by an automated test.

---

# Q. Acceptance Matrix

| ID | Requirement | Result | Evidence |
| --- | --- | --- | --- |
| **D11** | Named checkpoint persists across restart | **PASS** | §H.5 — real two-process restart; name and coordinate intact. `test_d11_a_named_save_point_survives_a_restart` |
| **D12** | Restore returns transcript **and** state | **PASS** | §G.1 — transcript to "Beat 3.", gold 50→30. `test_d12_restore_returns_the_transcript_and_the_state` |
| **D13** | Restore does not delete later history | **PASS** | §G.1 row-id identity `[1..11]` before and after; §G.2 Redo works pre-divergence, old future retained post-divergence, `POST /redo` → 400. Four `test_d13_*` tests |
| **D14** | Delete removes the pointer, not the story | **PASS** | `test_d14_delete_removes_the_pointer_and_no_story` — row ids, transcript and head all unchanged |
| **I04** | Save Points survive export/import | **PASS** | §I.2 — 3 Save Points across 2 branches, branch ids remapped 1→3/2→4, each restores to a distinct head and state |
| **L03** | Correct historical state after restart | **PASS** | §H.5 / §J — gold 28→6 across a real process boundary |
| **E01** | Abandoned future cannot affect active state | **PASS** | §K.1, §G.1 |
| **E01(m)** | Old-future memory absent on a new continuation | **PASS** | `test_e01_an_old_futures_memory_stays_out_of_a_new_continuation` |
| **E03** | Summary lineage over a long story | **NOT PERFORMED** | §K.3 — inherited M3 limitation, owned by M6/M11 |
| **E04** | Transcript holds only the active lineage | **PASS** | `test_e04_the_transcript_after_a_restore_holds_only_the_active_lineage` |
| **M4 browser smoke** | §17 twenty-step sequence in a browser | **NOT PERFORMED** | §M.1 — no usable browser |
| **M3 browser smoke** | M3's outstanding condition | **NOT PERFORMED** | §M.2 — unchanged since M3 |

## Q.1 Definition of Done

> The user can create a named Save Point, continue, restart, restore it, and
> continue differently without losing later history.

**MET**, clause by clause, at the API and server level:

| Clause | Demonstrated |
| --- | --- |
| create a named Save Point | §E.3 — at the active head, named |
| continue | §G.1 — 4 further turns |
| restart | §H.5 — server process killed and replaced |
| restore it | §H.5 — transcript and state exact |
| continue differently | §G.2 — divergent write forks |
| without losing later history | §G.1/§G.2 — row identity preserved throughout |

**Qualified only by §M**: the sentence describes something *a user does in a
browser*, and no browser executed it. The API-level demonstration is complete.

---

# R. Defects / Corrective Work

## R.A — BLOCKERS TO M4 ACCEPTANCE

**None found in the implementation.**

The single open item is procedural and is not M4's code:

| # | Item | Severity | Notes |
| --- | --- | --- | --- |
| A-1 | **The browser smoke test is unperformed for M4 and still for M3.** | acceptance risk | Whether this blocks is the reviewer's call, not this report's. It blocked nothing in code; §M states exactly what is unverified. Two consecutive milestones now carry it, and the trend is the concern rather than either instance. |

## R.B — CORRECTIVE WORK RECOMMENDED BEFORE M5

| # | Defect | Severity | Affected behaviour | Evidence | Owner |
| --- | --- | --- | --- | --- | --- |
| B-1 | **`GET /checkpoints` is an N+1 that fetches whole `Action` rows including prose.** `_rendered` calls `_node_at` (a full-entity query) plus `lineage.path_of` for **every** Save Point. | medium | Query count scales linearly with Save Points; each hits `actions` for 11 columns including `text`, all discarded except existence. | Measured: **53 SELECTs for 25 Save Points** (2.1 each), 25 against `actions`, `text` fetched. `GET /branches` costs **4** regardless of branch count. | **M4** |
| B-2 | **Deleting a branch silently deletes Save Points naming it, unwarned.** The cascade is by design (§O.3) and defensible, but `BranchPanel`'s confirmation says only *"Delete this branch and everything forked from it?"* | medium | A user deleting a branch loses named Save Points with no warning. M4 created this consequence; the copy predates it. | `grep` of `BranchPanel.jsx`: no mention of Save Points (or memories). Cascade measured in §O.3. | **M4** |
| B-3 | **The automated D11/L03 tests do not cross a process boundary.** `_restart()` closes the `TestClient` and opens a new one against the same engine in the same process. | low | The shipped suite is weaker than the acceptance item it is named for. D11/L03 pass here only because §H.5 was run by hand. | `test_save_points.py::_restart`; contrast §H.5. | **M4** |

B-1 deserves particular note: `backend/tests/test_egress.py` exists precisely
because this project has shipped two egress regressions from columns that were
fetched and discarded, and `paging.py` documents an explicit opt-in discipline
for exactly this. M4's list endpoint does not follow it. The response is only
5 224 bytes for 25 Save Points, so this is a query-count and I/O concern rather
than an egress blowout — but it is the codebase's own standard, unmet.

## R.C — NON-BLOCKING DEBT

| # | Item | Severity | Owner |
| --- | --- | --- | --- |
| C-1 | Save Point panel has **no frontend test**, because the project has no frontend test runner. | low | M8 (inherited) |
| C-2 | A successful restore triggers **two** list fetches (§L.4). | trivial | M4 or M8 |
| C-3 | `CheckpointOut` exposes `branch_id`, which the UI does not need. | trivial | M8 |
| C-4 | Panel renders **"Moment N"** where `BROWSER-UX-SPEC.md` §25 shows **"Turn N"** (§L.2). Internal consistency vs. spec example; needs a ruling, not a fix. | trivial | reviewer / M8 |
| C-5 | Create / rename / delete do **not** take the turn lock (restore does). Creating a Save Point while a turn is streaming could capture a head that is about to move. Not observed; narrow window. | low | M4 or M6 |
| C-6 | No cap on Save Points per campaign (§H.6); interacts with B-1. | low | M8 |
| C-7 | Inherited, unchanged: `POST /adventures/import` returns every branch's rows rather than a head-capped window. | low | inherited (M3) |

**No M4 defect has been assigned to M5.** B-1, B-2 and B-3 are M4's own.

---

# S. Non-Blocking Debt Carried Forward

M3's debt table was re-checked. Unchanged and still open: the RPG world-state
instrumentation in ~20 history tests (M5) — **M4 added 42 more tests using the
same instrumentation**, so M5's instrumentation move is now larger than M3's
report estimated; genre-neutral state must stay snapshot-recoverable per node
(M5); long-run summary isolation (M6/M11); no frontend tests (M8);
`Settings.model` defaults to `""` (M8); inert legacy tables awaiting the post-M5
cleanup (§O.4 notes `checkpoints` must not be swept into it).

---

# T. Planning-Document Recommendations

**No planning document was modified by this report**, other than the report-path
rotation in §V.

| Document | Recommendation | Justification |
| --- | --- | --- |
| `planning/SPECIFICATION.md` | **NO CHANGE RECOMMENDED** | M4 altered no product requirement. |
| `planning/STORY-BRANCH-SEMANTICS.md` | **NO CHANGE RECOMMENDED** | §18–25 specified checkpoint behaviour; the implementation matches, including §20 (restore does not fork), §23 (rename), §24 (no move) and §25 (delete). Nothing needed ratifying. |
| `planning/SECURITY-THREAT-MODEL.md` | **NO CHANGE RECOMMENDED** | No new path (§P). |
| `planning/TECHNICAL-DESIGN.md` | **NO CHANGE RECOMMENDED** for §8.8/§9.2, which were written during implementation and which this review confirms as accurate. | Verified against the code, not accepted on assertion. |
| `planning/DATA-MODEL.md` | **CHANGE RECOMMENDED (small)** | §8 records the coordinate choice; the **retry evidence in §C.3/§H.4 is stronger than the argument written there** and should be cited. Also add the two undocumented facts §H.6/§E.4 found: names are not unique, and the list order is by creation. |
| `planning/BUILD-MILESTONES.md` | **CHANGE RECOMMENDED** | M4's status block says "IMPLEMENTED — awaiting review". After review it needs the outcome, the three §R.B items, and a note to M5 that the instrumentation move now covers 42 more tests (§S). |
| `planning/V1-ACCEPTANCE-TESTS.md` | **CHANGE RECOMMENDED (small)** | Record D11–D14, I04, L03 results and their evidence pointers, as M1–M3 closeouts did. **Do not weaken any pass condition** — none needed weakening. Note that D11/L03 are satisfied by a real process boundary (§H.5), which is the standard future milestones should meet. |
| `planning/BROWSER-UX-SPEC.md` | **CHANGE RECOMMENDED (trivial, or rule against)** | §25's sample says "Turn 42"; the app says "Moment N" everywhere (§L.2, C-4). Either align the spec to the app's vocabulary or rule that "Turn" wins. |
| `planning/README.md` | **CHANGE RECOMMENDED at closeout, not now** | Must eventually record M4's disposition and the report rotation. This pass changed **only** the M3 report path (§V). It deliberately does **not** say M4 is accepted or M5 is authorized. |
| `planning/VERSION.md` | **CHANGE RECOMMENDED at closeout** | v2.5 records the implementation; a closeout entry should record the review outcome and any corrective work. |

## T.1 New ADR?

**No new ADR is recommended.** ADR 012 decides the architecture, and §F.3's
conditional branch move is that architecture applied to both halves of a
coordinate ADR 012 already defines — recorded in `TECHNICAL-DESIGN.md` §8.8,
which is the right home. A table and five endpoints are not an architectural
decision. This review concurs with the implementation's own restraint here.

---

# U. M5 Readiness Assessment

1. **Are Save Points stable enough for M5 to change state representation?**
   **Yes.** A Save Point stores a name and a `(branch, depth)` coordinate. It
   stores nothing about state, and no checkpoint code reads state.

2. **Does Restore depend only on a coordinate plus the standard snapshot
   contract?** **Yes.** The chain is coordinate → `head.move_to_node` →
   `head.move_to` → `attempts.restore_state`. M4 added no state code; the only
   state contract it relies on is M3's "each node carries what it left behind".

3. **Is anything in M4 accidentally coupled to the RPG world-state schema?**
   **No, in the product code** — `checkpoints.py`, the `Checkpoint` model and the
   bundle code contain no reference to `world_state`, stats, or the delta
   protocol. **Yes, in the tests**: `test_save_points.py` uses `GOLD_SCHEMA` and
   gold arithmetic as deterministic instrumentation, exactly as
   `test_head_cursor.py` does. M5 must **move the instrumentation and keep the
   assertions** — 42 tests here, on top of the ~20 M3 flagged.

4. **Can M5 change snapshots/events while preserving Save Point behaviour?**
   **Yes**, provided M5 honours the constraint `TECHNICAL-DESIGN.md` §10.4
   already carries from M3: state must remain recoverable at a position without
   replay. If M5 made state reconstruction proportional to campaign length, it
   would degrade Save Point restore and head movement together — one constraint,
   not two.

5. **Do any M4 defects block M5?** **No.** B-1, B-2 and B-3 are real and should be
   fixed, but none of them touches state representation, the coordinate, or head
   movement, and none would be made harder or easier by M5 landing first.

```text
M5 IS NOT BLOCKED BY M4's ARCHITECTURE.
M5 SHOULD NOT BEGIN until this report is reviewed and M4's disposition is
recorded — the project's own stop rule, not a technical finding.
```

---

# V. Final Repository State

## V.1 What this reporting pass changed

Two things, no application code:

1. **Report rotation.** `planning/reports/M3-IMPLEMENTATION-REPORT.md` →
   `planning/archive/milestone-reports/M3-IMPLEMENTATION-REPORT.md`
   (`git mv`; contents unedited).
2. **This report**, created at `planning/reports/M4-IMPLEMENTATION-REPORT.md`.

Six references to the old M3 path exist in active documents
(`planning/README.md` ×4, `BUILD-MILESTONES.md` ×1, `PROJECT-SOURCES.md` ×1,
`VERSION.md` ×1, `archive/README.md` index ×1). They are updated **only** where
the path changed — no status, no wording, no claim about M4's acceptance. Two
further occurrences are inside the M3 report itself, in quoted `git status`
output; those are historical contents and were **not** edited.

## V.2 Verification state

| Item | Value |
| --- | --- |
| M4 commit | `e08d49c`, **signed**, good signature |
| Tree at review | clean; all results above are from HEAD |
| Backend | **680 passed** |
| M4 targeted | **42 passed** |
| Frontend | lint exit 0 (7 pre-existing warnings), build exit 0 |
| Docker | build exit 0 |
| Upstream ancestry | intact |
| LICENSE | unchanged |

## V.3 Why M4 is not described as accepted

Because acceptance is a decision made by a reviewer reading this document, not by
the session that produced it. This report says the Definition of Done is met at
the API level, that the architecture holds under inspection, that three
corrective items exist, and that the browser requirement is unperformed for a
second consecutive milestone. What to do with that is the reviewer's call.

---

*End of report.*