Files
interactive-story/planning/archive/milestone-reports/M5-IMPLEMENTATION-REPORT.md
T
JesseMarkowitzandClaude Opus 5 a6e9c7a32b M6: branch-safe context, summaries and long-term story memory
Aligns the inherited AI-DnD memory and context foundation with the history,
authority and state model M3-M5 established. Long stories now reach the narrator
through a bounded, lineage-safe, inspectable context rather than a growing
transcript.

This commit includes the corrective work that followed the independent review in
planning/reports/M6-IMPLEMENTATION-REPORT.md. The first implementation reported
E03 as passing and it was not; the report records that history rather than
hiding it.

What was already correct, and was kept rather than rebuilt

  Memory lineage. Memories already carried (branch_id, depth) and retrieval
  already filtered through the capped-path clause; the ten-step negative control
  was measured passing against b7005e6 before any change here. M6 adds the
  regression tests that pin it, plus provenance and authority on the result.

Summary lineage — both halves

  A summary is a row carrying the coordinate of the last node it covers, and
  eligibility is the same head-capped lineage clause memories use. That alone
  was not enough: generation was seeded from adventures.story_summary, a
  campaign-global column with no lineage, so after a divergence the summariser
  was handed the abandoned line's prose and asked to update it. The row it
  produced was correctly anchored and therefore looked safe while its sentences
  described a story the reader had left.

  Generation is now seeded from summaries.current — the same question the
  context builder asks — so the input and the output are scoped by one rule.
  adventures.story_summary remains a reader-facing mirror for the Plot panel and
  the export bundle, kept in step when a summary is written and when the head
  moves, and nothing authoritative reads it.

Retrieval redundancy

  With a real embedding model, four near-identical memories crowded out the one
  distinctive clue, which survived only because the default memory_top_k is 5.
  Retrieval now drops a candidate that repeats one already chosen, never across
  authority classes, at a threshold measured against the configured embedding
  model. The clue is retrieved at top_k 5, 4 and 3. Ranking itself is unchanged;
  the further factors CONTEXT-AND-MEMORY §20 contemplates remain unimplemented
  and are recorded as such.

Memory authority, budgeting, observability

  Memory.authority is accepted_story or heuristic, classified by the application
  and marked in the prompt; retrieval never writes state. The reply is reserved
  out of the context budget, and an impossible configuration fails clearly
  instead of overflowing. Each derived pass records ok/idle/failed per campaign,
  served by GET /adventures/{id}/derived and shown in Insights, so the M2
  failure — a dead memory bank with a green suite — is visible if it recurs.
  Provider-wiring tests mock no factory.

Also: two pre-existing test-suite leaks fixed; two fixtures that stored one
vector in every memory now use distinct ones, so lineage assertions stay
readable alongside redundancy suppression.

Planning: CONTEXT-AND-MEMORY, TECHNICAL-DESIGN, DATA-MODEL, V1-ACCEPTANCE-TESTS,
BUILD-MILESTONES, VERSION and planning/README updated to describe what exists,
including that a valid E03 test must regenerate a summary after diverging. The
M5 report was rotated to planning/archive/milestone-reports/. No new ADR — every
choice implements a decision the package had already settled.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PWU4gTfLYY6Qq9U7aa9Qw2
2026-09-06 03:00:33 -04:00

957 lines
46 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# M5 Implementation Review — Typed Narrative State
**Reviewed:** 2026-09-03 / 2026-09-04
**Branch:** `m5-narrative-state` **Base commit:** `62a997f` (M4 closeout)
**Subject of review:** the staged, uncommitted M5 working tree
**Verdict:** PASS WITH CORRECTIVE WORK REQUIRED
---
## A. Scope
This reviews the M5 milestone: replacing the RPG relative-delta world-state
mechanism with genre-neutral typed narrative state per ADR 010, and making that
state the authoritative thing the narrator is told and the reader is shown.
It is a review, not a closeout and not implementation. No application code was
changed. Defects found are recorded here rather than fixed, and the working tree
is exactly as the implementation left it.
M6 was not started.
## B. Method and independence
The M5 implementation summary was treated as a claim, not as evidence. Every
acceptance statement below rests on something run during this review:
- the full backend suite, re-run from scratch;
- purpose-built probes that drive the real FastAPI application over its real
HTTP surface against a real SQLite file, with a scripted narrator;
- a real Firefox 154.0.1 driven over WebDriver against the real built frontend
served by the real backend;
- the real model endpoint on the trusted LAN, for the tests that are skipped
without one;
- a real `docker build`.
Where a check could only be done with a mock, it is labelled as such. The
atomicity check (L01) specifically was **not** done with mocks — the failure was
induced at the storage layer.
## C. Repository state
```
branch m5-narrative-state
HEAD 62a997f
staged 45 files changed, 4979 insertions(+), 364 deletions(-)
12 added, 33 modified
unstaged / untracked none
```
The tree was in this state at the start of the review and is in this state at
the end of it. No secrets, databases, or browser profiles are staged. Upstream
ancestry is intact and `LICENSE` is unmodified.
The new package is `backend/app/narrative/` — `model.py`, `events.py`,
`validate.py`, `apply.py`, `extract.py`, `render.py`, `store.py` — plus
`backend/tests/test_narrative_state.py` and `test_narrative_realistic.py`.
## D. What M5 changed
The relative-delta protocol (`{"player.hp": -15}`) is gone from the prompt path.
In its place is a 14-event allowlist of explicit, **absolute** narrative events
(`create_entity`, `set_entity_attribute`, `set_possession`,
`set_current_location`, `add_fact`, `invalidate_fact`, `open_story_thread`, …)
carried in a fenced block the extraction pass strips out of the prose.
State is stored twice on purpose, per DATA-MODEL §17: validated events in
`state_events` for audit, and a whole-document snapshot per position in
`actions.narrative_state_after` for restore. The live document sits in
`adventures.narrative_state`.
The legacy world-state module still exists and is still exercised by its own
tests, but it no longer reaches the assembled prompt. That demotion is verified
(§I below), not assumed.
## E. ADR 010 conformance
Conformant, and the central ambiguity ADR 010 exists to remove is genuinely
removed. There is no code path where one numeric value can be read as either an
absolute or a delta: `apply.py` is an explicit `if/elif` chain over event types,
each writing a named field, with no arithmetic on prior values.
The state document is genre-neutral. A scan of the new package for RPG and
fantasy vocabulary (`dungeon`, `hp`, `mana`, `quest`, `loot`, `potion`, …)
returns nothing but one citation of AI-DnD in a module docstring explaining what
ADR 010 replaced.
## F. Validation and the security boundary
`validate.py` is five layers — envelope, allowlist, schema, referential,
semantic — and the allowlist is checked before anything else touches the
payload. This is the right order: an unknown event type is rejected before its
fields are read.
**H05 holds.** Hostile proposals were fed through the real turn pipeline:
`execute_shell`, an `eval` event carrying `__import__('os').system(...)`, an
event keyed `event_type` instead of `type` to test envelope confusion, an entity
named `__class__`, and a story-thread id of `../../etc/passwd`. No marker file
was created, no event was accepted, and the proposal was recorded with its
rejection reasons. Nothing in the event path reaches the filesystem, a
subprocess, or an attribute lookup on a Python object.
Cross-campaign references are refused: an event naming an entity that exists in
a *different* campaign is rejected as `unknown_reference` rather than resolved.
## G. Head, lineage, and restore
**Restore does not replay the event log.** This was proven, not inferred: the
`state_events` table was renamed away mid-campaign, and undo and redo both still
returned 200 and moved the state correctly. Restore is a snapshot row lookup, as
ADR 012 requires. Query counts for undo were also measured at two depths and did
not grow.
**Lineage isolation holds (E01, E04).** State established on a line that is then
abandoned does not appear in the new line's state document, and does not appear
in the next turn's assembled prompt.
## H. Atomicity (L01)
Tested by induced failure, not by mocks: the `state_events` table was renamed
away underneath a live state-writing turn.
```
before head (1,2) mara.cloak = red
state_events taken away
turn OperationalError: no such table: state_events
after head (1,3) mara.cloak = red
accepted narration rows for the failed turn: 0
previous story reachable: yes
recovery after the table returns: next turn 200, state advances normally
```
No half-written state, no accepted narration without it, no inaccessible
history. The head advanced by exactly one, onto the player's own submitted text
— which the L01 note explicitly defines as correct under A05, not as a
half-advanced head. **L01 PASS.**
## I. Context assembly
The narrator is given the state document rendered as prose, not as JSON, and the
legacy world-state block never appears. Verified by inspecting stored
`context_snapshot` rows from real turns.
One defect here, and it is the mechanism behind Finding 4: the **history**
section replays each past narrator turn's raw text *including its ```state
fence*. So the prompt simultaneously instructs the model that prose must not
contain protocol, and shows it several past examples where prose does.
## J. Manual corrections (C04)
A correction lands, is stored with `manual_correction` authority, is shown in
the inspector marked as the reader's own, and is visible in the audit view.
But it is only half-applied to the narrator's context. With the fact
`mara-knows / "knows where the key was found"` withdrawn by an explicit
correction:
```
state section contains the withdrawn fact: False ← correct
full prompt contains the withdrawn assertion: True ← in the history replay
any statement anywhere that it was corrected: False
```
The state section drops it; the history section replays the original `add_fact`
verbatim; and nothing in the prompt tells the model the assertion was
withdrawn. The narrator is left holding the contradicted claim with no
indication it is contradicted. **C04 PARTIAL.**
## K. Export / import
Round-tripping a campaign preserves the state document, campaign canon, save
points, head depth (including an undone head, so I07 holds), every per-node
snapshot (7 of 7), and correction *authority* — an imported corrected fact is
still marked `manual_correction`.
It does not preserve the audit trail. `state_events` and `state_proposals` are
not in the bundle, so an imported campaign reports **0 audit events**. The
correction's effect survives; the record of who made it and why does not. This
is the round-trip half of C04's "auditable" condition.
## L. Migration from pre-M5
A genuine pre-M5 database was constructed (M5 tables dropped, columns removed,
`user_version` stamped back) and then migrated. Migration itself is clean: the
campaign opens, all rows and save points survive, and new M5 turns play
normally.
The defect is at the seam. Pre-M5 nodes have no snapshot, and
`attempts.restore_state` treats a NULL snapshot as "leave the live state alone".
So restoring to an old Save Point moves the transcript back without moving the
state:
```
restore to the pre-M5 Save Point at depth 2 -> head (1,2)
state left standing describes depth 6 (the M5 turn's entities)
```
The reader is at depth 2 and the state panel describes depth 6. §9 of the M5
brief forbids exactly this.
## M. Performance
One regression, in the action-list read path.
`paging.py:ACTION_LIST_COLUMNS` does not include `models.Action.state_changes`,
so every row lazy-loads it individually:
```
11 actions on the page -> 24 queries, 13 fetching state_changes one row at a time
51 actions on the page -> 64 queries, 53 fetching state_changes one row at a time
```
The comment directly above that list warns against this exact mistake for
`world_delta`, in these words: *"Omitting it saves no bytes. It converts one
bulk read into one lazy load per row."* The new column was added to the model
without being added to the list.
## N. Frontend
`npm run lint` exits 0. Every warning it prints is pre-existing at `62a997f` in
files M5 did not touch. `npm run build` succeeds. `docker build` succeeds and
the image contains `backend/app/narrative/`; M5 added no dependency and needed
no packaging change.
The stale-panel bug reported during implementation was fixed in `saveEdit` by
bumping `stateKey`. That is the correct fix for that mutation, and the panel's
key (`actions.length` + `stateKey`) does now cover every state-moving mutation:
undo/redo, take switching, save-point restore, correction, delete. It is
per-mutation rather than a single shared invalidation boundary, so a future
mutation must remember to bump it — worth noting, but not a defect today.
## O. Real-browser verification
Firefox 154.0.1, headless, real backend, fresh database. **16 of 16 checks
passed**: inspector starts empty; shows what a turn established; groups by
category; shows possession with its owner; follows a later turn, Undo, Redo, and
a Save Point restore; a manual correction lands, is marked as the reader's own,
survives another turn, and is explained in the audit view; a narrator edit
re-derives state; edited prose carries no protocol; no console errors.
That suite edits the **last** narrator turn, so it does not exercise the case in
Finding 1. A second browser probe was written for that case, and it reproduces
the defect visibly — see below.
## P. Realistic-model behaviour
Run against one small local model (`qwen2.5:3b-instruct`) on the trusted LAN. All
3 otherwise-skipped tests pass. Over 6 turns:
```
accepted 3 | partially_accepted 1 | unparseable 2
events rejected 1 (unknown_reference)
```
Two of six turns produced a state block the pipeline could not parse. The
pipeline degrades correctly — the prose still lands, the state is left alone,
and the proposal is recorded `unparseable` — which is why the tests pass. But a
33% unparseable rate on a 3B model is a real datum for the prompt work ahead.
This is **one** model. It is not evidence about models in general, and no
broader claim is made from it.
## Q. Test suite
```
764 passed, 3 skipped, 1 warning (197s)
```
All three skips are `test_narrative_realistic.py`, skipped for missing
`AIDND_TEST_ENDPOINT` / `AIDND_TEST_MODEL`. With those set, all three pass (§P).
There are no other skips and no xfails.
Rollback assertions were **moved, not deleted**. `test_worldstate_integration.py`
went from 6 tests to 4 and was rewritten as a demotion test, with the removed
tests and the reason for each recorded in the module docstring. Their subject
matter is now covered by `test_narrative_state.py` (67 tests).
Two gaps:
- `test_state_revert.py` — the dedicated unit test for
`attempts.restore_state` / `snapshot_outcome` — has **zero** references to
`narrative_state`. It still tests only the legacy column. The NULL-snapshot
rule that produces the migration defect in §L lives in exactly the code this
file covers, and was not extended to the column M5 made load-bearing.
- No test covers editing a turn that has later **visible** story. That is the
hole Finding 1 sits in.
## R. Genre neutrality (J01–J03)
The backend is neutral. The new package is clean, and a science-fiction campaign
plays through the same events with no fantasy vocabulary anywhere in the path.
Remaining hits in the backend are either citations of AI Dungeon as the design
being followed, the frozen wire-format string `ai-dnd-adventure-v2` (which must
not change), or the demoted legacy world-state module's own examples.
The frontend is not. `frontend/src/pages/ScenarioEditor.jsx` is still routed at
`scenarios/:id` and still tells the user, in user-facing prose, that the app
"will track them each turn — HP, mana, a raised alarm, quest objectives", with a
JSON placeholder of `player.hp`, `npcs.gwen.stats.trust`, and `milestones`. That
is the surface ADR 010 replaced, still describing the replaced design to the
user. J03 asks that genre be configuration; this screen hard-codes one genre's
vocabulary as the explanation of how state works.
## S. Findings
### Finding 1 — Editing a turn with later visible story breaks the state/head invariant — **SERIOUS**
`STORY-BRANCH-SEMANTICS.md` §14A refuses an in-place edit only when descending
story is **off screen**. An edit with a *visible* future is therefore permitted,
and `_reevaluate_state()` in `routers/adventures/actions.py` then calls
`narrative.store.set_current()` with the edited node's re-derived state — while
the head stays at the tip.
The comment there asserts the case cannot arise: *"If the head sits further
along, the refusal above already ran — this node has no off-screen future."*
That is incorrect. The refusal covers off-screen futures only.
Reproduced in a real browser, on a four-turn campaign, by editing the first
narrator turn through the UI's own ✎ control:
```
state panel BEFORE : CHARACTERS Aldric, Mara | LOCATIONS the Crooked Lantern
ITEMS the silver key | POSSESSIONS held by Mara
FACTS Mara knows where the key was found
OPEN STORY THREADS Reach the Old Abbey
state panel AFTER : "Nothing established yet."
transcript : all four turns still on screen
head : still (1,7), the tip
```
The database shows the invariant broken directly — the head's own snapshot is
intact while the live column it is supposed to agree with is empty:
```
live narrative_state entities 0 facts 0 threads 0
snapshot at depth 7 (head) entities 4 facts 1 threads 1
snapshots at depths 2..6 entities 4 ← stale: describe prose that no longer exists
```
Three consequences: the reader sees a full transcript over an empty state panel;
the next turn's prompt is built from the rewound state; and Undo/Redo restores
the stale downstream snapshots, resurrecting state whose originating prose was
edited away. An earlier API-level probe showed the milder form of the same bug —
after editing a "red cloak" turn to "green", undoing back restored `cloak: red`
while the visible prose said green, the prose/state disagreement §15 forbids.
**D10 is not satisfied.** Its M5 pass condition is *"downstream state is
re-evaluated"*. Downstream state is not re-evaluated; live state is rewound and
downstream snapshots are left stale. Separately, §15 lists five requirements for
this workflow and only 1–3 are implemented: there is no new continuation and the
original narration is overwritten in place, so *"old version/future remains
retained"* also fails. The UI offers ✎ (edits in place, no fork) and ⑂ (forks,
but regenerates rather than using the corrected text) — **no available path
satisfies §15 fully.**
### Finding 2 — N+1 on the action list — **MODERATE**
`state_changes` is missing from `ACTION_LIST_COLUMNS`; 51 rows cost 53 extra
queries. Details and the warning comment that predicted it: §M.
### Finding 3 — Restoring to a pre-M5 node leaves M5 state standing — **MODERATE**
NULL snapshot is treated as "leave the live state alone", so head and state
disagree at any migrated position. Details: §L. Same broken invariant as
Finding 1, different cause.
### Finding 4 — A withdrawn fact survives in the prompt's history replay — **MODERATE**
The state section drops it, the history replay keeps it verbatim, and nothing
says it was corrected. Details: §I, §J.
### Finding 5 — Export/import loses the audit trail — **MINOR**
`state_events` and `state_proposals` are not bundled; an imported campaign shows
0 audit events. Effects survive, records do not. Details: §K.
### Finding 6 — Extraction strips legitimate prose — **MINOR**
The fence stripper removes content it should leave alone:
```
CUT a bracketed aside that merely mentions the state block
CUT a story containing a legitimate ```json fence
in : 'She typed it out:\n```json\n{"name": "Mara"}\n```\nThen closed the terminal.'
out: 'She typed it out:\n\nThen closed the terminal.'
KEPT a story containing a ```python fence
```
A story about programmers loses its code. The `python` fence surviving shows the
rule is specifically over-broad on `json`.
### Finding 7 — The RPG schema editor still describes the replaced design — **MINOR**
Details: §R.
### Finding 8 — Snapshot/restore unit tests were not extended to the new column — **MINOR**
Details: §Q.
## T. Acceptance criteria
| ID | Criterion | Result | Basis |
|----|-----------|--------|-------|
| C01 | Campaign canon preserved | PASS | canon survives turns, restore, and round-trip |
| C02 | Possession state | PASS | `set_possession` moves ownership; shown with owner in UI |
| C03 | Character knowledge not invented | PASS | `knows()` gated on recorded facts; unknown refs rejected |
| C04 | Manual state correction | **PARTIAL** | reflected in state and auditable live; but the withdrawn fact survives in the history replay (F4) and the audit is lost on import (F5) |
| C06 | Structured state matches accepted narration | PASS | absolute events only, no delta/absolute ambiguity; malformed proposals rejected, recorded, prose still lands |
| D01 | Undo one turn | PASS | snapshot restore, verified in browser and API |
| D04 | Redo | PASS | as above |
| D05 | Redo invalidated by new continuation | PASS | abandoned line isolated (E01/E04 probes) |
| D06 | Retry narrator response | PASS | unchanged from M3/M4, re-run green |
| D07 | Select prior retry take | PASS | take switch restores that take's state |
| D08 | Retry does not delete prior take | PASS | suite + browser |
| D09 | Edit earlier user input | PASS | within the same mechanism as D10; no visible-future case in the fixtures |
| D10 | Edit narrator output | **FAIL** | M5's own pass condition — downstream state re-evaluation — is not met; state is rewound, downstream snapshots left stale (F1); retention condition also unmet |
| D11 | Named checkpoint | PASS | M4 machinery, re-verified in browser |
| D12 | Restore checkpoint | PASS | state moves with the restore |
| D13 | Restore does not delete later history | PASS | later rows retained and reachable |
| D14 | Delete checkpoint | PASS | suite |
| E01 | Abandoned future cannot affect active state | PASS | probe: no leak into state or prompt |
| E04 | Scene state is lineage-safe | PASS | same probe |
| H05 | Invalid state event rejected | PASS | hostile payloads produced no filesystem effect and no accepted events |
| I01 | Export campaign | PASS | bundle carries state, canon, checkpoints, head |
| I02 | Import exported campaign | PASS | 201, state and authority intact |
| I03 | Branch/disposable history export | PASS | snapshots 7 of 7 |
| I04 | Checkpoint export | PASS | save points round-trip |
| I07 | Export/import preserves an undone active head | PASS | `can_redo` preserved in the copy |
| J01 | Science-fiction campaign | PASS | plays through the same events, no genre coupling |
| J02 | Generic entity support | PASS | `entity_type` is free-form |
| J03 | Genre profiles are configuration | **PARTIAL** | backend neutral; the scenario editor still hard-codes HP/mana as the explanation of state (F7) |
| L01 | Atomic turn commit | PASS | induced storage failure, not mocks — §H |
| L02 | State reconstruction | PASS for undo/redo; breaks after an edit (F1) |
| L03 | Checkpoint reconstruction after restart | PASS | M4's real-process-boundary test reads the M5 state document |
## U. Documentation
`planning/` was updated in step with the code, and `DATA-MODEL.md` §17 correctly
describes the hybrid that was actually built. Two corrections are needed:
- `STORY-BRANCH-SEMANTICS.md` §14A should no longer say the refusal is "replaced
by" M5's re-evaluation. It was not replaced; it was retained for off-screen
futures, and the visible-future case it does not cover is unhandled.
- The incorrect comment in `_reevaluate_state()` quoted in Finding 1 should be
removed rather than reworded — it asserts an invariant the code does not hold.
## V. Planning-document recommendations
1. **The state-document shape deserves ratification.** ADR 010 settled the
event protocol; it did not settle the document those events write into
(`entities` / `facts` / `possessions` / `threads` / `scene`, each with
authority and provenance). That shape is now load-bearing for the prompt, the
UI, export, and migration. It should be written down as an ADR rather than
left as an implementation detail of `narrative/model.py`.
2. **§15 of the M5 brief needs re-specifying before it can be implemented.** It
requires a corrected narrator turn to become authoritative *and* the original
to be retained. Retaining the original means forking; the current ✎ does not
fork and the current ⑂ does not use the typed text. The missing operation —
"fork here, using this exact text, and re-derive forward" — should be
specified explicitly.
3. **D10 should record its remaining scope**, the way it already records its M3
and M5 split, rather than being left looking complete.
## W. M6 readiness
M5's core is sound: the protocol is unambiguous, the security boundary holds,
restore is snapshot-based and does not replay, lineage isolation is intact, and
atomicity survives an induced storage failure. That is the hard part, and it
works.
M6 should not start on top of Finding 1. The broken invariant is
`transcript position == head == authoritative state`, and every later feature
that reads state at a position inherits it. Findings 2, 3, and 4 are each
contained enough to fix alongside it. Findings 5–8 can ride along or be filed.
Recommended sequence: fix Findings 1 and 3 together, since both are the same
invariant reached by different routes; then 2 and 4; then close out M5.
---
## Evidence appendix
| Check | Command / harness | Result |
|---|---|---|
| Backend suite | `.venv/bin/python -m pytest tests/ -q` | 764 passed, 3 skipped |
| Realistic model | same, with endpoint and model set | 3 passed (674s) |
| Frontend lint | `npm run lint` | exit 0, warnings pre-existing |
| Frontend build | `npm run build` | success |
| Container | `docker build` | success; image contains `app/narrative/` |
| Browser | Firefox 154.0.1 headless over WebDriver | 16/16 |
| Browser (edit case) | second probe, first narrator turn | defect reproduced |
| Atomicity | `state_events` renamed away mid-turn | no half-commit |
| No-replay | `state_events` renamed away, then undo/redo | both 200 |
| Hostile events | 5 payloads through the real turn path | 0 accepted, no filesystem effect |
| N+1 | query counter over the action list | 51 rows -> 53 extra queries |
| Migration | pre-M5 DB rebuilt, stamped, migrated | restore leaves state stale |
| Export/import | real bundle round-trip | state yes, audit no |
---
---
# ADDENDUM — M5 Corrective Pass
**Date:** 2026-09-04
**Branch:** `m5-narrative-state` **Base commit:** `62a997f`
**Status of the review above:** unchanged. Nothing in it has been edited or
withdrawn. This addendum records what was done about it.
**Recommendation:** M5 CORRECTED — READY FOR CLOSEOUT / M6
## A. Repository and staging state
Recorded before any change was made:
```
branch m5-narrative-state
HEAD 62a997f364e5387e4cc1dcc20f5a6618dee5f267
staged 45 files changed, 4979 insertions(+), 364 deletions(-)
12 added, 33 modified, nothing unstaged, nothing untracked
```
The staged M5 implementation was never reset, discarded, squashed or recreated.
The corrective work is added on top of it, and the whole is now staged together:
```
staged 57 files changed, 6809 insertions(+), 487 deletions(-)
15 added, 41 modified, 1 renamed (the M4 report, into planning/archive)
nothing unstaged, nothing untracked
```
LICENSE is untouched, the AI-DnD base commit `d72f7c1b` is still an ancestor of
HEAD, and no database, secret, token or browser profile is staged. No commit was
created: this repository's commits are signed by its owner, so the message is
prepared at `.git/M5_CORRECTIVE_MSG` instead (§P).
## B. Disposition of Findings 1–8
| # | Finding | Disposition |
|---|---------|-------------|
| 1 | Narrator edit breaks the state/head invariant (**serious**) | **Fixed.** Narrator editing rebuilt on §§14-15 fork semantics. §C, §D, §E. |
| 2 | N+1 on the action list | **Fixed.** `state_changes` added to the bulk read; 51 rows now cost a constant query count. §G. |
| 3 | Restoring to a pre-M5 node leaves M5 state standing | **Fixed.** Migration 88 backfills; a missing snapshot restores the empty document. §F. |
| 4 | A withdrawn fact survives in the prompt's history replay | **Fixed.** History carries prose only; withdrawals are named explicitly. §H. |
| 5 | Export/import loses the audit trail | **Deferred to M9**, deliberately, and recorded in C04 and BUILD-MILESTONES. §K. |
| 6 | Extraction strips legitimate prose | **Fixed**, and a second defect found while fixing it. §I. |
| 7 | Scenario editor describes the replaced design | **Copy corrected; screen deferred to M8.** §K. |
| 8 | Snapshot/restore tests never covered the new column | **Fixed.** `test_state_revert.py` now covers it first-class. §J. |
## C. The narrator-edit implementation
The operation is no longer a write to the row being corrected. Nothing on the
line being left is written to at all.
```text
before after
parent parent
└── original narrator ├── original narrator ──> old future [retained]
└── old future └── corrected narrator [active]
```
`_edit_narration` in `backend/app/routers/adventures/actions.py` maps one step
to each clause of §15:
1. **Return to the state before the narration** — the preceding node's
`narrative_state_after`, one row read, not a replay.
2. **Treat the edited text as the accepted output** — stored verbatim with only
the protocol block stripped. No model is called.
3. **Re-evaluate the implied state** — the same extraction, allowlist, schema,
reference and canon validation a generated turn faces.
4. **Create a new active continuation** — a new node, and the head on it.
5. **Retain the original narration and its future** — untouched.
Two shapes, chosen by whether anything was written after the turn:
- **At the tip:** the turn's attempts are still leaves, so the correction joins
them as another take (`attempts.hand_over_the_prompt` + `attempts.add_attempt`)
and the original stays beside it in the pager. No branch, and the head does
not move.
- **With story below it**, visible or not: `lineage.branch_of` →
`tree.branch_at(depth - 1)` → `head.mark_superseded` → `tree.place_action`.
The departed line keeps its node, its future and its live flag.
**No new history mechanism was introduced.** This is the ⑂ path already in
`takes.py` with the reader's text in place of a generated reply, using the same
`tree`, `head`, `attempts` and `lineage` primitives M3 and M4 provide.
Two consequences worth stating:
- The §14A refusal is **gone for narrator turns** — the case it refused is now
handled rather than blocked, because a fork writes nothing to the off-screen
line. It remains for a player's own input (§13), which M5 did not change.
- Editing a take the story is **not** telling stays a plain in-place edit. Such
a take has no continuation of its own, so correcting its words contradicts
nothing. This preserved two existing behaviours the first draft of the fix had
broken.
The frontend follows: `saveEdit` detects that the server answered with a
different node and re-reads the window, because the shape of the story changed
and only the server can say what the transcript is now.
## D. How retention was proven
Not by inspection — by identity. Every test below captures the original row's id
and text and the ids of every row below it *before* the edit, and asserts they
are all still present and unchanged afterwards.
- `test_editing_a_narrator_turn_with_visible_descendants_forks`
- `test_the_old_narration_and_its_future_leave_the_active_transcript`
- `test_editing_a_narrator_turn_with_an_undone_future_keeps_it`
- `test_editing_a_narrator_turn_a_divergence_left_behind_keeps_that_line`
- `test_an_edit_is_safe_when_a_future_is_off_screen`
- `test_editing_the_latest_narrator_turn_still_works` (the original take retained
and no longer live, with no branch created)
In the browser, the same thing read straight out of SQLite: the original
narrator row still holds its own words, every row of the old future still
exists, and the correction is a different node id on a different branch id.
## E. How live-state/head consistency was proven
A helper asserts the invariant directly against the database rather than through
the API, resolving the head node **through the lineage** — after a fork the head
branch owns one node and inherits the rest of the path:
```python
node = head.node_at(db, adventure, adventure.head_depth)
return normalize(adventure.narrative_state) == normalize(node.narrative_state_after)
```
It is asserted after every narrator edit, and at **every position visited** while
undoing and redoing across an edit
(`test_undo_and_redo_after_an_edit_stay_on_the_corrected_lineage`), which is
where the review found the worst symptom — Undo restoring a snapshot from a line
the reader was no longer on, so the prose said green and the state said red.
That test additionally asserts that no attribute from the abandoned line ever
reappears.
The same equality is checked in the browser run (check 8) and across Undo/Redo
there (check 9).
## F. How pre-M5 positions are handled
Two halves, so that the stored data is explicit rather than relying on a
fallback:
- **Migration 88** — no DDL, a data pass. `_backfill_narrative_snapshots` writes
the empty narrative document onto every action whose `narrative_state_after`
is NULL. One statement, no row loop: the document is identical for every row,
so it is encoded once with the same `compression.pack` and `narrative.model`
the runtime uses, and bound as a single parameter.
- **`attempts.restore_state`** — a missing narrative snapshot now restores the
empty document. This covers a node arriving from an older export, which the
migration never sees.
The legacy RPG column deliberately keeps the opposite rule: a NULL
`world_state_after` is still left alone, because nothing consults those numbers
and blanking a running campaign's would help no one. The difference is now
documented in the function rather than implicit.
The empty document is the honest answer. A pre-M5 position established nothing
in the narrative-state system because that system did not exist yet; retaining
another position's state is a claim about a story that had not been told.
`backend/tests/test_pre_m5_compatibility.py` builds a genuine pre-M5 database
(M5 tables dropped, columns removed, `user_version` rewound to 80), migrates it,
and runs the six required steps:
```
the migration backfills every existing action PASS
the migrated campaign opens and keeps its history PASS
restoring a pre-M5 Save Point leaves no later state standing PASS
undo/redo across the pre-M5 boundary stay coherent PASS
a pre-M5 campaign can be continued normally PASS
```
Restoring the old Save Point now yields `head depth 2` with an empty document
and `live state == head snapshot`; redoing forward brings the M5 state back with
the position it belongs to. State restoration remains a bounded row read — no
head movement was turned into replay-from-root.
## G. Action-list query counts
Measured with a statement counter over the real endpoint, before and after:
| page | before | after |
|------|--------|-------|
| 11 actions | 24 queries, 13 fetching `state_changes` one row at a time | **13 queries** |
| 51 actions | 64 queries, 53 fetching `state_changes` one row at a time | **13 queries** |
Constant with page size. The fix is one line: `models.Action.state_changes`
joins `ACTION_LIST_COLUMNS`, which is what `models.py` already said it was for
("`world_delta` has an M5 counterpart in `state_changes` for the bulk read").
It is a small per-turn column of the same order as `world_delta`, not the
deferred snapshot — no large unrelated column was pulled in, and
`narrative_state_after` is still asserted *not* to be fetched in bulk.
`test_the_action_list_does_not_cost_a_query_per_action` asserts by measurement
rather than by inspecting the column tuple, so a future column consumed during
serialization is caught the same way. It was confirmed to fail without the fix
(58 SELECTs for 52 rows) and pass with it.
## H. Manual corrections and the narrator's prompt
Two changes, neither of which touches the stored historical records:
- **Replayed history is prose only.** `_history_text` no longer reconstructs the
protocol block into past turns. That reconstruction put a second, older
account of the world into the same prompt as the authoritative one with
nothing marking which governed, and handed back a fact the reader had
explicitly withdrawn as an accepted event. The format instruction survives in
`EMIT_RULE` (with a worked example) and `EMIT_REMINDER` (placed last).
- **A withdrawn fact is named.** `render.for_prompt` adds a section after the
facts that stand:
```text
No longer true — do not treat these as established:
Mara knows where the key was found — Mara never learned where the silver key was found.
```
Silence was the problem: dropping the fact left the narration that first
asserted it as the only account in the prompt, and prose reads as current
truth.
Evidence, from the review's own Mara example run through the real turn pipeline:
```
history section contains "```state": no
contains "add_fact": no
contains "mara-knows": no
state section under "Established": the withdrawn fact is absent
under "No longer true": named, with the reader's reason
```
Nothing was deleted or rewritten: the invalidated fact keeps its status, reason
and provenance in the document, and the `state_events` audit is untouched.
## I. Fence-stripping behaviour
The extractor now removes the application's own protocol payload and nothing
else. `state` is our label and is taken unconditionally; `json` and unlabelled
fences are taken only when their contents are this protocol — judged both by
parsing into a proposal *and* by plainly reading as one.
| input | result |
|---|---|
| ```` ```state ```` block | stripped |
| ```` ```state ```` block that does not parse | stripped, raw kept for the audit |
| ```` ```json ```` containing `{"name": "Mara"}` | **kept** — it is the story |
| ```` ```json ```` containing a real proposal | stripped |
| ```` ```json ```` containing a *malformed* proposal | stripped |
| ```` ```python ```` block | kept |
| unlabelled fence with non-proposal JSON | kept |
| dangling ```` ```json ```` that is story | kept |
| dangling ```` ```json ```` that is a truncated proposal | stripped |
| trailing bracketed aside mentioning "state block" | kept |
| the prompt's own reminder, parroted back | stripped |
The malformed-proposal row is a defect the corrective pass introduced and then
caught: narrowing the rule to "must parse" meant a small model that mangled its
own JSON had the wreckage shown to the reader. **The realistic-model run found
it, not the unit tests** — a reminder of why that run exists. The rule became
"parses as a proposal, or plainly reads as protocol", and
`test_a_malformed_proposal_in_a_json_fence_never_reaches_the_reader` pins it.
## J. Test results
```
794 passed, 3 skipped, 1 warning (backend, 206s)
```
Up from 764 passed at review time — 30 net new tests. The 3 skips are the
realistic-model tests, skipped for missing `AIDND_TEST_ENDPOINT` /
`AIDND_TEST_MODEL`; with those set they run (§L).
New and rewritten:
- `test_narrative_state.py` — seven D10 regressions, two correction-to-prompt
regressions, nine fence-safety tests.
- `test_pre_m5_compatibility.py` — new module, five migration regressions.
- `test_state_revert.py` — seven new tests covering `narrative_state`
first-class (Finding 8): the snapshot is recorded, deep-copied both ways, and
written empty when there is none; restore puts it back, does not alias it, and
restores the empty document when the node has no snapshot, while the legacy
column keeps the opposite rule.
- `test_egress.py` — the query-count regression and a guard that
`narrative_state_after` stays out of the bulk read.
- `test_head_cursor.py`, `test_change_visibility.py`, `test_retry_variants.py` —
four tests rewritten. Each asserted behaviour this pass deliberately replaces
(the §14A refusal for narrator turns; the protocol replay; edit-as-rewrite).
None was deleted: each now asserts the stronger property that replaced it.
Targeted runs, all green: narrator edit with visible descendants; latest-turn
edit; retained original future; live-state == head-snapshot; pre-M5 Save Point
restore; Undo/Redo across old and new positions; manual correction → next
prompt; fence stripping; action-list query count; atomic state commit; lineage
isolation.
## K. Deferred, deliberately
- **M9 — export/recovery (Finding 5).** `state_events` / `state_proposals` are
not carried in a bundle, so an imported campaign keeps a correction's effect
(`manual_correction` authority survives) but reports zero audit events. C04
therefore passes for a live campaign and not across a round trip; this is
written into C04's result and into BUILD-MILESTONES rather than left implicit.
Not fixed here: bundle format work is M9's, and the corrective gate did not
require it.
- **M8 — browser UX (Finding 7).** The scenario editor still exposes the legacy
RPG stat schema. Its copy claimed the app "will track them each turn — HP,
mana, a raised alarm, quest objectives", which M5 made false; that sentence is
corrected to say what is now true. The screen itself is M8's to redesign.
- **§13 — editing player input.** Still an in-place edit guarded by the
off-screen refusal. Bringing it onto the §§14-15 footing was outside M5's
scope; recorded in §14A and BUILD-MILESTONES.
- **A player's own input may still contain a protocol fence**, since extraction
applies to narrator output. Observed while building the browser harness, not a
review finding, and not acted on here.
## L. Realistic-model results
Run against the same single small local model the review used, on the trusted
LAN. All 3 otherwise-skipped tests pass (805s).
```
review run corrective run
turns 6 6
accepted 3 6
partially_accepted 1 0
unparseable 2 0
events rejected 1 (unknown_ref) 0
assembled prompt 9382 chars 9922 chars
```
**This is one stochastic run of six turns against one 3B model, and no causal
claim is made from it.** The prompt did change materially — replayed history no
longer carries protocol blocks, and withdrawn facts are now stated — so an
effect is plausible in either direction, but six turns cannot separate that from
run-to-run variance. What the run does establish is that the corrective changes
did not degrade emission against a real model, and that the pipeline still
degrades safely: no event was rejected and no block was unparseable.
The run also earned its keep by catching a real defect the unit tests missed —
the malformed-proposal leak described in §I.
## M. Frontend, container
```
npm run lint exit 0 (7 warnings, all pre-existing at 62a997f, in files M5 did not touch)
npm run build success
docker build success
```
## N. Real-browser results
Firefox 154.0.1, headless, over WebDriver, against the real built frontend
served by the real backend on a fresh database.
**The standard M5 checks: 16/16 pass** — inspector empty then populated, grouped
by category, possession with owner, a later turn moving it, Undo, Redo, Save
Point restore, a manual correction landing and marked as the reader's own and
surviving another turn, the audit view, a narrator edit re-deriving state, no
protocol in the prose, no console errors.
**The previously failing case: 16/16 pass.** Four turns establishing state, then
the *earliest* narrator turn corrected through the UI's own ✎ control with all
later story on screen — the exact case the review reproduced as broken:
```
1-2 several turns established several facts PASS
3 an Edit control on the earliest narrator turn PASS
5 the corrected prose is what the story now tells PASS
5b the original narration is off the active transcript PASS
5c the old future is off the active transcript too PASS
6 the original narrator row still exists, with its own words PASS
6b every row of the old future still exists PASS
6c the correction is a different node on a different branch PASS
7 the panel shows what the corrected turn established PASS
7b nothing from the abandoned future is still shown PASS
8 live narrative_state equals the active-head snapshot PASS
9 Undo and Redo never resurrect the abandoned line's state PASS
10 the next prompt carries the corrected narration PASS
10b and not the abandoned future PASS
10c and no protocol block in the replayed history PASS
11 no console errors PASS
```
No console errors in either run.
One harness correction worth recording, because the first run's failures were
mine and not the product's: the probe initially picked the first Edit control on
the page, which belongs to the opening *player* action. Editing a player turn
correctly takes the unchanged in-place path, so nothing forked. Addressing the
earliest **narrator** row — `.action:not(.player)` carrying the expected words —
was the fix, and every check then passed.
## O. Acceptance status
| ID | Before | After | Basis |
|----|--------|-------|-------|
| **D10** | FAIL | **PASS** | All three pass conditions demonstrated: corrected narration authoritative, state re-evaluated, old version and future retained. §C, §D, §E, §N. |
| **C04** | PARTIAL | **PASS for the live campaign** | The withdrawn fact no longer returns through history and the withdrawal is stated explicitly. Auditability across export/import remains open and is recorded as M9 debt. §H, §K. |
| **L02** | PASS for undo/redo; broke after an edit | **PASS** | The invariant is asserted at every position visited while undoing and redoing across an edit, and after restoring to a migrated pre-M5 position. §E, §F. |
| **J03** | PARTIAL | **PARTIAL, narrowed** | Backend unchanged and neutral. The false user-facing claim is corrected; the legacy schema screen itself is M8's. §K. |
| L01, E01, E04, H05, C01–C03, C06, D01–D09, D11–D14, I01–I04, I07, J01–J02, L03 | PASS | **PASS** | Re-run green in the full suite. |
## P. Planning documents changed
- `STORY-BRANCH-SEMANTICS.md` — §§14-15 gain a "How this is built" section with
the fork diagram, the two shapes, and the invariant. §14A is retitled
*Superseded for Narrator Output*: it no longer says the finished behaviour is
pending, and records why the refusal is gone for narrator turns and what still
uses it.
- `V1-ACCEPTANCE-TESTS.md` — D10's milestone-ownership note replaced by a PASS
result naming the evidence for each of its three unchanged pass conditions.
C04 gains a result recording what is fixed and what is deferred. **Neither
criterion's pass conditions were altered.**
- `planning/DECISIONS/013-authoritative-narrative-state-document.md` — new ADR
recording the document shape, the authority/provenance fields, the
events → document → snapshot pipeline, what each store is for, the invariant,
and the consequences. It records what M5 built and invents nothing beyond it.
- `BUILD-MILESTONES.md` — status line moved to M1-M5 complete with M6 next; the
§14-15 note records that the corrective pass delivered it; a new "M5 — Outcome"
section records what shipped and the debt assigned to M8, M9 and §13.
- `README.md` — the play-loop feature bullet now says what a narrator correction
does.
- `planning/VERSION.md` — one line recording ADR 013.
- `planning/reports/M4-IMPLEMENTATION-REPORT.md` → `planning/archive/milestone-reports/`
(report rotation; a clean rename).
## Q. Verification summary
| Check | Result |
|---|---|
| Backend suite | **794 passed, 3 skipped** (206s) |
| Realistic model, one small local model | **3 passed** (805s); 6/6 turns accepted, 0 unparseable |
| Frontend lint | exit 0, warnings pre-existing |
| Frontend build | success |
| Docker build | success |
| Browser — standard M5 checks | **16/16** |
| Browser — the previously failing edit case | **16/16** |
| Action-list queries, 51 rows | 64 → **13** |
| Pre-M5 Save Point restore | head, transcript and state agree |
## R. Is M5 safe to close, and is M6 safe to begin?
**Yes to both.**
The invariant M6 would have inherited is now held everywhere it was broken, and
held by assertion rather than by argument: after a narrator edit, at every
position visited across Undo and Redo, and after restoring to a migrated pre-M5
position. The two failures the review called the same invariant reached by
different routes are fixed by the same rule rather than by two special cases.
Nothing deferred blocks M6. Export of the audit trail is M9's own subject
matter; the scenario editor is an M8 screen; §13 is an existing, guarded
behaviour that M5 did not change and M6 does not build on.
**M5 CORRECTED — READY FOR CLOSEOUT / M6**