diff --git a/planning/BUILD-MILESTONES.md b/planning/BUILD-MILESTONES.md index 542d267..8ed9ed7 100644 --- a/planning/BUILD-MILESTONES.md +++ b/planning/BUILD-MILESTONES.md @@ -250,7 +250,7 @@ History operations are non-destructive, Redo works, divergence preserves old fut ## Status: COMPLETE -Accepted 2026-09-03. Evidence: `planning/reports/M3-IMPLEMENTATION-REPORT.md`, +Accepted 2026-09-03. Evidence: `planning/archive/milestone-reports/M3-IMPLEMENTATION-REPORT.md`, which is M3's primary evidence record — no separate baseline report was produced, so that document carries the raw counts and runtime observations as well as the review. The architecture is recorded in **ADR 012**. diff --git a/planning/PROJECT-SOURCES.md b/planning/PROJECT-SOURCES.md index ca05ba4..2ab3a9b 100644 --- a/planning/PROJECT-SOURCES.md +++ b/planning/PROJECT-SOURCES.md @@ -82,7 +82,7 @@ in `planning/archive/decisions/`. One file, and it changes as development progresses: ```text -planning/reports/M3-IMPLEMENTATION-REPORT.md +planning/reports/M4-IMPLEMENTATION-REPORT.md ``` M3 is the most recently completed milestone, and M4 is the next to be briefed. diff --git a/planning/README.md b/planning/README.md index d78c652..7d53e45 100644 --- a/planning/README.md +++ b/planning/README.md @@ -96,7 +96,7 @@ Two standing qualifications: 10. `BROWSER-UX-SPEC.md` 11. `V1-ACCEPTANCE-TESTS.md` 12. `DECISIONS/` — all of them; they are short. -13. `reports/M3-IMPLEMENTATION-REPORT.md`, for what the last milestone actually +13. `reports/M4-IMPLEMENTATION-REPORT.md`, for what the last milestone actually left behind. Nothing in `planning/archive/` unless sent there. ## Architectural decisions @@ -127,15 +127,12 @@ work until Phase 0 closes — which Phase 0 satisfied on 2026-09-01. It is in `reports/` holds the report for the milestone most recently completed, because that is the one the next milestone's planning has to consult: -- `reports/M3-IMPLEMENTATION-REPORT.md` — M3's review **and** its primary - evidence record; no separate M3 baseline report was produced. M4 needs its - §W (closeout) and §M/§W.4 (the open browser smoke test). +- `reports/M4-IMPLEMENTATION-REPORT.md` — M4's review and its evidence record. -Completed earlier milestones are in `archive/milestone-reports/`. When M4's -report lands, M3's moves there too: a milestone report is useful during the -immediate next milestone and historical afterwards. **M3's report has not moved -yet**, because M4's does not exist — the rotation belongs to M4's closeout, not -to its implementation. +Completed earlier milestones are in `archive/milestone-reports/`, which M3's +report joined when M4's landed: a milestone report is useful during the +immediate next milestone and historical afterwards. M3's is now at +`archive/milestone-reports/M3-IMPLEMENTATION-REPORT.md`, unedited. ## The decision this package rests on @@ -233,7 +230,7 @@ Milestone M2 COMPLETE (2026-09-02) | v Milestone M3 COMPLETE (2026-09-03) - non-destructive undo/redo, reports/M3-IMPLEMENTATION-REPORT.md + non-destructive undo/redo, archive/milestone-reports/M3-*.md active-head export and ADR 012 | v @@ -253,8 +250,9 @@ action; M5 does not begin before that report is written and accepted. Two conditions remain open. The **browser smoke test has still not been performed** — now for M3 and for M4 — because no session so far has had a usable -browser. See `reports/M3-IMPLEMENTATION-REPORT.md` §M and §W.4, and the M4 status -block in `BUILD-MILESTONES.md`. And **no M4 review exists**: the status block was +browser. See `archive/milestone-reports/M3-IMPLEMENTATION-REPORT.md` §M and §W.4, +`reports/M4-IMPLEMENTATION-REPORT.md` §M, and the M4 status block in +`BUILD-MILESTONES.md`. And **no M4 review exists**: the status block was written by the implementation and records what it built, which is not the same as a reviewer having read it. diff --git a/planning/VERSION.md b/planning/VERSION.md index 96e8dc1..0cb2cb8 100644 --- a/planning/VERSION.md +++ b/planning/VERSION.md @@ -77,7 +77,7 @@ tell authoritative material from evidence at a glance. ## v2.3 — Post-M3 Closeout (2026-09-03) M3 replaced destructive Undo with a stored active head. Its review is -`reports/M3-IMPLEMENTATION-REPORT.md`, which is also M3's primary evidence +`archive/milestone-reports/M3-IMPLEMENTATION-REPORT.md`, which is also M3's primary evidence record — no separate baseline report was produced — and whose §W records this closeout. diff --git a/planning/archive/README.md b/planning/archive/README.md index 23dc245..822814e 100644 --- a/planning/archive/README.md +++ b/planning/archive/README.md @@ -41,7 +41,11 @@ Git history. ### `milestone-reports/` — completed milestone evidence `M1-BASELINE-REPORT.md`, `M1-IMPLEMENTATION-REPORT.md`, -`M2-BASELINE-REPORT.md`, `M2-IMPLEMENTATION-REPORT.md`. +`M2-BASELINE-REPORT.md`, `M2-IMPLEMENTATION-REPORT.md`, +`M3-IMPLEMENTATION-REPORT.md`. + +M3's report is both its review and its primary evidence record; no separate M3 +baseline report was produced. It arrived here when M4's report landed. Every architectural conclusion these reports reached has already been applied to the active planning documents and the ADRs — see `planning/VERSION.md`, which diff --git a/planning/reports/M3-IMPLEMENTATION-REPORT.md b/planning/archive/milestone-reports/M3-IMPLEMENTATION-REPORT.md similarity index 100% rename from planning/reports/M3-IMPLEMENTATION-REPORT.md rename to planning/archive/milestone-reports/M3-IMPLEMENTATION-REPORT.md diff --git a/planning/reports/M4-IMPLEMENTATION-REPORT.md b/planning/reports/M4-IMPLEMENTATION-REPORT.md new file mode 100644 index 0000000..81cc783 --- /dev/null +++ b/planning/reports/M4-IMPLEMENTATION-REPORT.md @@ -0,0 +1,1043 @@ +# 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 = &"' +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.*