M4: close out Save Points, with browser verification
Closes M4. The review's three findings are fixed, the durability rule the specification always implied is now enforced, and M3's and M4's browser behaviour has been verified in a real browser for the first time. B-1 -- the Save Point list was an N+1 that loaded whole Action rows, narration included, to answer "does a row exist here". It is now one bulk two-column coordinate query plus one lineage: 53 SELECTs for 25 Save Points became 5, and the count no longer grows with the list. The clause is an OR of exact (branch, depth) pairs rather than two IN lists, because the cross product would report a Save Point resolved on the strength of another one's depth existing on this one's branch. A test builds exactly that trap. B-2 -- reclassified during closeout from "missing warning" to a behaviour defect, and fixed as one. STORY-BRANCH-SEMANTICS §19 says a named checkpoint remains until explicitly deleted, and §28 already required future cleanup to retain checkpoint-referenced paths; a cascade that silently removed Save Points with a branch violated both, and a warning would only have documented the violation. A branch a Save Point names can no longer be deleted. The request is refused with the offending Save Points named, the user deletes them explicitly -- which deletes no story -- and the branch then goes. The scope is the subtree, because deleting a branch takes its descendants. Both delete controls disable and explain. Recorded as a new §19.1; models.py, TECHNICAL-DESIGN §8.8 and DATA-MODEL §8 had all recorded the cascade as the rule and now record the refusal. An earlier pass in this same closeout had kept the cascade and added a warning. That was the wrong fix and its tests were replaced rather than left standing, since they pinned the defect. B-3 -- the D11/L03 automation never left one process, so it could not distinguish durable state from a live Python object. It now spawns real server processes, kills the first, and reads the campaign back with the second. C-5 -- creating a Save Point takes the campaign's turn lock. "Save where I am" has to name one committed position, and the head is what a turn in flight is about to move. Rename and Delete deliberately do not take it. The architecture is untouched: a Save Point is still name + note + (branch, depth), and restore is still coordinate -> head.move_to_node -> head.move_to -> attempts.restore_state. No second restore path, no state copied into a checkpoint, no fork on restore. Browser verification -- the first in this project, and it covers both milestones. Firefox 154.0.1 through geckodriver over the W3C WebDriver protocol, driving the rendered DOM: 47/47 checks, twice, on independent databases, no console errors. M3's Undo/Redo enable states, transcript movement, Retry and the take pager, divergence retiring Redo; M4's whole Save Point lifecycle, both confirmations, and the new branch-delete refusal including its recovery. No dependency was added: the WebDriver client is stdlib HTTP. No application defect was found by the browser. Four failures occurred, all in the harness -- a wrong SPA route, a wait comparing transcript length when the empty-story placeholder is longer than the first turn, a fixture deleting the branch it was reading, and a reload assertion that sampled once instead of waiting. The last was checked against the app before being called a harness bug. Tests: 698 backend pass (was 680), 60 M4, 94 M3 history, 66 export/ migrations, 93 security/local-only. Frontend lint and build clean, Docker build clean, loopback binding unchanged. No assertion weakened, no skip added. Planning: STORY-BRANCH-SEMANTICS §19.1 is the only behavioural change and it strengthens §19. V1-ACCEPTANCE-TESTS records D11-D14, I04, L03 and the E-series, keeping automated, live-runtime and browser evidence distinct, and weakens no pass condition. DATA-MODEL records the coordinate with the retry measurement that settles it. BROWSER-UX-SPEC rules for Moment over Turn. BUILD-MILESTONES marks M4 COMPLETE, closes M3's browser condition, and lists what M5 inherits. VERSION adds v2.6. No new ADR: ADR 005 already decides that history is preserved rather than overwritten, and §19.1 is that decision applied to checkpoint-referenced history. M4 is closed. M5 may now be briefed; it has not been started. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWU4gTfLYY6Qq9U7aa9Qw2
This commit is contained in:
JesseMarkowitz
co-authored by
Claude Opus 5
Claude Opus 5
parent
279a871a77
commit
62a997f364
@@ -1040,4 +1040,279 @@ second consecutive milestone. What to do with that is the reviewer's call.
|
||||
|
||||
---
|
||||
|
||||
# W. M4 Closeout Addendum (2026-09-03 / 2026-09-04)
|
||||
|
||||
**This addendum is current where it differs from the review above**, and §W.3 is
|
||||
current where it differs from an earlier draft of this addendum: B-2 was first
|
||||
addressed as a warning over a surviving cascade, then reclassified as a behaviour
|
||||
defect and fixed by refusing the deletion. Only the final behaviour is described
|
||||
below; the review's §R still records the finding as it was raised. Sections
|
||||
A-V record what was true at commit `e08d49c`, before corrective work; nothing in
|
||||
them has been rewritten. Where they say a finding is open or a test is
|
||||
unperformed, this section says what happened next.
|
||||
|
||||
## W.1 Starting state
|
||||
|
||||
| Fact | Value |
|
||||
| --- | --- |
|
||||
| Closeout started from | `279a871` — *Planning: add the M4 implementation review report and rotate M3's* |
|
||||
| Closeout dates | corrective work and browser verification 2026-09-03; B-2 reclassified and re-fixed 2026-09-04 |
|
||||
| Reviewed implementation | `e08d49c` — signed, unchanged by this pass |
|
||||
| Working tree at start | clean; the report and rotation were already committed |
|
||||
| Upstream ancestry | intact |
|
||||
| LICENSE | unchanged |
|
||||
|
||||
## W.2 B-1 — the Save Point list N+1 is gone
|
||||
|
||||
`GET /checkpoints` resolved each Save Point with its own query and loaded whole
|
||||
`Action` entities to do it. It now takes one bulk query over two integer columns
|
||||
plus one lineage computation, both before the loop.
|
||||
|
||||
Measured on the same 25-Save-Point fixture the review used:
|
||||
|
||||
| | before | after |
|
||||
| --- | --- | --- |
|
||||
| SQL statements per list | **53** | **5** |
|
||||
| SELECTs against `actions` | 25 | **1** |
|
||||
| columns in that SELECT | 11, including `text` | **2** (`branch_id`, `depth`) |
|
||||
| narration fetched | yes | **no** |
|
||||
| queries per Save Point | 2.1 | 0.2 |
|
||||
|
||||
Three tests guard it. One asserts on *growth* rather than a fixed budget — five
|
||||
times the Save Points must not mean more queries — because a fixed number is
|
||||
something to edit rather than a rule to keep. One asserts the emitted SQL never
|
||||
names `actions.text`, `actions.reasoning` or `actions.world_delta`. The third is
|
||||
the one worth keeping: it builds a **cross-product trap**, a dead coordinate on
|
||||
one branch while another branch has a live row at the same depth, and proves the
|
||||
dead one still reports `resolved: false`. An `IN`-list implementation would fail
|
||||
it; the OR-of-pairs passes.
|
||||
|
||||
`_render_all` is now the single rendering path — create and rename call it
|
||||
through `_rendered` with a list of one — so the list and the single-item
|
||||
responses cannot drift.
|
||||
|
||||
## W.3 B-2 — a branch a Save Point names cannot be deleted
|
||||
|
||||
The review recorded this as a missing warning. On closeout it was reclassified as
|
||||
a **behaviour defect**, because the authority is unambiguous:
|
||||
`STORY-BRANCH-SEMANTICS.md` §19 says a named checkpoint remains until explicitly
|
||||
deleted, and §28 already requires a future cleanup feature to *retain paths
|
||||
referenced by checkpoints*. A cascade that removed Save Points along with a
|
||||
branch violates both, and a warning would only have documented the violation.
|
||||
|
||||
**The deletion is now refused.** `DELETE /branches/{id}` returns **409** when any
|
||||
Save Point names a position on that branch or on anything forked from it — the
|
||||
subtree, because deleting a branch takes its descendants, and a check scoped to
|
||||
the named branch alone would let a Save Point on a child vanish silently.
|
||||
|
||||
The refusal names what stands in the way rather than only counting it:
|
||||
|
||||
```text
|
||||
This branch, or a branch forked from it, is where a Save Point “On the new
|
||||
line” is saved. Delete that Save Point first if you no longer need it, then
|
||||
delete the branch. Deleting a Save Point does not delete any story.
|
||||
```
|
||||
|
||||
Long lists truncate ("and 2 more") so the message stays a sentence someone reads.
|
||||
|
||||
The recovery is the point, and it is cheap: deleting a Save Point deletes no
|
||||
story (§25), so the user removes the pointer and the branch then goes. Verified
|
||||
end to end in the browser (§W.7 section J).
|
||||
|
||||
Both delete controls — the branch list and the tree overlay — now **disable**
|
||||
Delete while Save Points protect the subtree and say why, and the branch row
|
||||
reports `· 2 Save Points kept here`. The confirmation, now only reachable when
|
||||
nothing is at risk, says plainly that the story on other paths and every Save
|
||||
Point are unaffected. Two views changed, because the same deletion is reachable
|
||||
from both and a rule holding in one of them would not be a rule.
|
||||
|
||||
`checkpoints.branch_id` keeps `ON DELETE CASCADE` as referential integrity — a
|
||||
Save Point must never point at a branch that is gone — but the guard means it
|
||||
does not fire through the application. Campaign deletion still cascades, which is
|
||||
what deleting a campaign means.
|
||||
|
||||
Six tests cover the five required scenarios plus the truncation: deletion refused
|
||||
and nothing lost; the Save Point intact after the refusal; explicit deletion then
|
||||
allowing the branch delete; a branch no Save Point names still deleting; a Save
|
||||
Point on a *descendant* also protecting; and an unrelated Save Point not blocking
|
||||
anything. A source-level test asserts both views disable and explain.
|
||||
|
||||
**Documents this corrected beyond the planned set:** `models.py`,
|
||||
`TECHNICAL-DESIGN.md` §8.8 and `DATA-MODEL.md` §8 had all recorded the cascade as
|
||||
the durability rule. They now record the refusal.
|
||||
|
||||
## W.4 B-3 — the restart test now crosses a process boundary
|
||||
|
||||
`backend/tests/test_process_restart.py` (3 tests, ~9 s) starts the real
|
||||
application with `subprocess.Popen`, plays a story over HTTP, **kills the
|
||||
process**, starts a second process against the same database file, and only then
|
||||
asks its questions. Readiness is probed, never slept on; children are terminated
|
||||
in a `finally` whether or not the test passes.
|
||||
|
||||
It covers D11 (the Save Point, its name and its coordinate survive), L03 (the
|
||||
state at the position returns), that restore deletes no rows, that Redo is
|
||||
available before divergence, that the retained continuation is still walkable,
|
||||
and that a divergent write after a restart still forks on the write rather than
|
||||
the restore.
|
||||
|
||||
The old same-process helper was **not** kept as a pretend equivalent.
|
||||
|
||||
## W.5 C-5 — creating a Save Point is serialized
|
||||
|
||||
Create now takes the campaign's existing turn lock, the same one Undo, Redo and
|
||||
Restore take. No new lock was introduced. "Save where I am" has to name one
|
||||
committed position, and the head is exactly what a turn in flight is about to
|
||||
move.
|
||||
|
||||
Rename and Delete deliberately **do not** take it: neither reads nor moves a
|
||||
story position, and refusing a label edit during generation would be a worse
|
||||
product for no safety gained. A test pins that decision so it reads as a choice
|
||||
rather than an oversight, and another proves a refused create releases the lock.
|
||||
|
||||
## W.6 Test and build results
|
||||
|
||||
All against the closeout tree.
|
||||
|
||||
| Suite | Result |
|
||||
| --- | --- |
|
||||
| **Full backend** | **698 passed**, 0 failed, 0 skipped (was 680) |
|
||||
| **M4 targeted** (`test_save_points.py` + `test_process_restart.py`) | **60 passed** (57 + 3) |
|
||||
| **M3 invariants** (8 modules) | **130 passed** |
|
||||
| **Security / local-only** (incl. `test_egress.py`) | **93 passed** |
|
||||
| **Frontend lint** | exit 0; 7 warnings, unchanged from baseline |
|
||||
| **Frontend build** | exit 0 |
|
||||
| **Docker build** | exit 0 |
|
||||
|
||||
698 − 680 = 18 new tests: 3 process-restart, 4 for B-1, 7 for B-2 (the refusal
|
||||
rule and its recovery), 3 for C-5, and one holding the branch-list count to the
|
||||
same subtree the server refuses on. Two tests written earlier in this closeout
|
||||
were **replaced**, not kept alongside: they asserted the cascade behaviour that
|
||||
§W.3 removed, and leaving them would have pinned the defect.
|
||||
|
||||
## W.7 Browser verification — PERFORMED, and it passes
|
||||
|
||||
**This is the first real-browser verification in the project**, and it discharges
|
||||
the condition M3 and M4 both carried.
|
||||
|
||||
| | |
|
||||
| --- | --- |
|
||||
| Browser | **Mozilla Firefox 154.0.1**, headless |
|
||||
| Driver | geckodriver 0.37.1, W3C WebDriver over HTTP |
|
||||
| Client | written for the run against Python's stdlib — **no dependency added to the repository** |
|
||||
| App | the real production-shaped server, SPA served same-origin, deterministic scripted model |
|
||||
| Result | **47/47 checks passed**, no console errors; run twice on independent fresh databases |
|
||||
|
||||
§M reported no usable browser, and that was true of the paths tried there: the
|
||||
`firefox` snap wrapper fails with mount-namespace errors and hangs on a headless
|
||||
screenshot. The binary **inside** the snap
|
||||
(`/snap/firefox/current/usr/lib/firefox/firefox`) runs correctly under
|
||||
geckodriver, which §M did not try. §M's conclusion is superseded; its account of
|
||||
what was attempted stands.
|
||||
|
||||
What the browser actually did, by section of the closeout brief:
|
||||
|
||||
| Section | Verified in the DOM |
|
||||
| --- | --- |
|
||||
| **A — M3 history** | transcript renders; Undo enabled and Redo disabled at the tip; two Undos move the transcript back twice; Redo becomes enabled; two Redos return the original tip exactly |
|
||||
| **B — retry / takes** | Retry produces an alternate take; the take pager appears; stepping between takes changes the visible narration; no branch vocabulary needed |
|
||||
| **C — M3 divergence** | a new continuation from a moved-back head appears; Redo becomes unavailable; no stale old-future text in the active transcript; position and continuation survive a reload — with a database check confirming the displaced rows are still on disk |
|
||||
| **D — creation** | Save Point created and named through the form; appears in the list; panel says **Save Point** with no branch/head/node/fork wording; position reads **Moment N** |
|
||||
| **E — persistence** | still present after a full page reload |
|
||||
| **F — restore** | the confirmation visibly explains later history is kept; transcript and state move back; the Save Point stays listed; Redo becomes available; zero rows deleted (database check); the view refreshes without a manual reload |
|
||||
| **G — restore + redo** | Redo returns the original continuation; the Save Point survives |
|
||||
| **H — restore + divergence** | the different continuation appears; Redo into the displaced future is gone; no old-future narration in the active view; the displaced rows retained on disk |
|
||||
| **I — rename / delete** | rename changes the name and not the position; it still restores to the same moment; the delete warning says the story is not deleted; the row disappears with no stale list; the story remains |
|
||||
| **J — branch deletion** | the branch row reports the Save Points kept on it; Delete is **disabled** and explains what to do; the server independently refuses with **409** naming the Save Point; nothing is deleted by the refusal; and the documented recovery works — deleting the Save Point frees the branch |
|
||||
| **K — control state** | enable/disable states match the position; no material console errors |
|
||||
|
||||
**No defect was found in the application by the browser run.** Four failures
|
||||
occurred and all four were in the harness: a wrong SPA route (`/adventures/:id`
|
||||
rather than `/play/:id`); a wait predicate that compared transcript *length* when
|
||||
the empty-story placeholder is longer than the first turn; a fixture that tried
|
||||
to delete the branch it was reading, which is refused by design; and a reload
|
||||
assertion that sampled the transcript once instead of waiting for it to render.
|
||||
The last was checked against the application before being called a harness bug —
|
||||
after a reload the text is present at the first sample, so the race was the
|
||||
test's.
|
||||
|
||||
## W.8 Planning documents updated
|
||||
|
||||
`V1-ACCEPTANCE-TESTS.md` (D11-D14, I04, L03, E-series results and the browser
|
||||
condition; **no pass condition weakened**), `STORY-BRANCH-SEMANTICS.md` **§19.1**
|
||||
(new — a checkpoint protects the history it names; the closeout's only
|
||||
behavioural specification change, and it strengthens §19),
|
||||
`DATA-MODEL.md` §8, `TECHNICAL-DESIGN.md` §8.8, `BROWSER-UX-SPEC.md` §25 (Moment,
|
||||
not Turn), `BUILD-MILESTONES.md` (M4 COMPLETE, the facts M5 inherits, the M5
|
||||
instrumentation note), `planning/README.md`, `VERSION.md` (v2.6),
|
||||
`PROJECT-SOURCES.md` and `project-sources.txt`, which still named the archived M3
|
||||
report.
|
||||
|
||||
Unchanged, as §T recommended: `SPECIFICATION.md`, `STORY-BRANCH-SEMANTICS.md`,
|
||||
`SECURITY-THREAT-MODEL.md`, `CONTEXT-AND-MEMORY.md`,
|
||||
`IMPORTED-KNOWLEDGE-DESIGN.md`, ADR 003, ADR 005, ADR 012. **No ADR 013**: the
|
||||
corrective work forced no architectural decision. §19.1 is a durability rule
|
||||
inside an existing specification, not a new architecture — ADR 005 already says
|
||||
history is preserved rather than overwritten, and refusing to delete a
|
||||
checkpoint's history is that decision applied, not a departure from it.
|
||||
|
||||
## W.9 The architecture is unchanged
|
||||
|
||||
Worth stating plainly, because closeout touched the checkpoint module. A Save
|
||||
Point is still `name + optional note + (branch, depth)`. Restore is still
|
||||
`coordinate → head.move_to_node → head.move_to → attempts.restore_state`. The
|
||||
corrective work touched **how the list is read** and **when create is allowed to
|
||||
read the head** — never what a Save Point is or how restoring one moves the
|
||||
story. Nothing was copied into a checkpoint, no checkpoint-specific Redo stack
|
||||
exists, restore still does not fork, and the first divergent write is still what
|
||||
creates the continuation.
|
||||
|
||||
## W.10 Remaining debt
|
||||
|
||||
Unchanged from §S and none of it M4's: no frontend test runner (M8) — the browser
|
||||
run above is a closeout procedure, not a suite; `POST /adventures/import` returns
|
||||
every branch's rows rather than a head-capped window (inherited, M3); the RPG
|
||||
world-state instrumentation, now **60 tests**, moves at M5.
|
||||
|
||||
## W.11 Result
|
||||
|
||||
D11, D12, D13, D14, I04 and L03 all **PASS**. The E-series lineage results
|
||||
through a Save Point restore all **PASS** (E03 remains NOT PERFORMED, owned by
|
||||
M6/M11). The Definition of Done is met, and now met in a browser:
|
||||
|
||||
> The user can create a named Save Point, continue, restart, restore it, and
|
||||
> continue differently without losing later history.
|
||||
|
||||
```text
|
||||
M4 CLOSED — READY FOR M5
|
||||
```
|
||||
|
||||
M5 is next to brief. It has not been started.
|
||||
|
||||
## W.12 What M5 inherits
|
||||
|
||||
Recorded here because it is the only thing this report owes the next milestone.
|
||||
These are constraints, not suggestions, and none of them is M5's to revisit.
|
||||
|
||||
- **The inherited RPG world-state system is instrumentation, not the target
|
||||
architecture.** 60 tests in this milestone use gold arithmetic to make "the
|
||||
state at this position" a number a test can assert. M5 replaces the machinery
|
||||
underneath and must **move the instrumentation while keeping the assertions** —
|
||||
what they measure is where the story is being read and what state belongs to
|
||||
that position, which is exactly as true after M5.
|
||||
- **M5 implements ADR 010's genre-neutral typed narrative-state model.** That is
|
||||
the target; the stat/band/cooldown protocol is what it replaces.
|
||||
- **Per-position snapshots, or equivalent fast recovery, must survive the
|
||||
replacement** (`TECHNICAL-DESIGN.md` §10.4). Undo, Redo and Save Point restore
|
||||
all resolve a coordinate and read the state recorded there. If M5 makes state
|
||||
reconstruction proportional to campaign length, it degrades all three at once.
|
||||
- **M3/M4 history and Save Point semantics are infrastructure now.** The stored
|
||||
head, the single movement mechanism, the capped lineage, fork-on-first-write,
|
||||
and the Save Point coordinate are settled. M5 changes what state *contains*,
|
||||
not how the story is positioned, and should not add a second restore path.
|
||||
- **A Save Point's history is protected** (`STORY-BRANCH-SEMANTICS.md` §19.1).
|
||||
Any state-cleanup or migration work M5 introduces must not acquire a way to
|
||||
delete a checkpoint, or the history one names, as a side effect.
|
||||
|
||||
---
|
||||
|
||||
*End of report.*
|
||||
|
||||
Reference in New Issue
Block a user