Edit the take you are reading, not the one it replaced

The pager parks a turn on take 2 of 4 and the row draws that take's words,
but the row itself is keyed by the live node — the take the story tells. The
edit button seeded from that node and saved back to it, so opening the editor
on a 2/4 turn showed 4/4's text and saving overwrote 4/4. The take being read
was never reachable.

It has an id of its own, carried on the preview, and the edit endpoint takes
any row by id whether or not the path runs through it. So an edit opened over
a preview carries the take's id, the transcript matches the editor to the row
through the preview rather than the node, and the saved text goes back into
the preview because there is no row in `actions` to put it in. The pager
caches the take list it fetched, so it is told to drop it — otherwise
stepping away and back showed the words from before the edit. Leaving the
take at all drops a half-typed edit with it.

The fork button had the same seed and now takes its text from the screen too.
Its id stays the live node's: a take branches just above the turn, and the
server only accepts a turn that is on the path.

The tests are on the promise the fix leans on — a take is an ordinary row to
the edit endpoint, addressed by its own id, and the group listing says so
afterwards. That already held; nothing in the backend changed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/tests/test_take_edit.py

@@ -0,0 +1,197 @@
+"""Phase 14 SP9 — editing the take you are actually reading.
+
+The pager can park a turn on take 2 of 4. The transcript row it sits in is
+still keyed by the *live* take, because that is the row the story tells and the
+one the window carries; the take being read is only in the client's hand, by
+its own node id.
+
+So "edit this" has two ids to choose from, and the page shipped choosing the
+wrong one: it opened the editor on the live take's text and saved over it, from
+a row that was showing take 2. That is a client bug and it is fixed in
+Play.jsx, but the fix rests on something only the server can promise —
+
+  a take is an ordinary row to the edit endpoint, addressed by its own id,
+  whether or not it is the one the path runs through
+
+— and on the group listing telling the truth about it afterwards. Both are
+asserted here so the client's fix cannot be quietly undermined.
+
+    python -m pytest tests/test_take_edit.py -v
+"""
+import os
+import tempfile
+
+_tmp = tempfile.NamedTemporaryFile(suffix=".db", delete=False)
+_tmp.close()
+os.environ["AIDND_DB_PATH"] = _tmp.name
+os.environ.pop("AIDND_DATABASE_URL", None)
+os.environ.pop("DATABASE_URL", None)
+
+import pytest
+from fastapi import Depends
+from fastapi.testclient import TestClient
+
+from app import auth, limits, models
+from app.database import Base, SessionLocal, engine, get_db
+from app.main import app
+from app.providers import PromptParts
+from app.routers import adventures
+
+
+class ScriptedProvider:
+    replies: list = []
+    calls = 0
+
+    def __init__(self, *a, **k):
+        pass
+
+    async def generate(self, parts: PromptParts, *, temperature, max_tokens):
+        index = min(ScriptedProvider.calls, len(ScriptedProvider.replies) - 1)
+        ScriptedProvider.calls += 1
+        yield ("text", ScriptedProvider.replies[index])
+
+
+@pytest.fixture()
+def client(monkeypatch):
+    Base.metadata.create_all(bind=engine)
+    setup = SessionLocal()
+    user = models.User(is_guest=False, email="takeedit@example.com")
+    setup.add(user)
+    setup.flush()
+    setup.add(models.Settings(user_id=user.id, api_key="enc:dummy", model="test-model"))
+    scenario = models.Scenario(user_id=user.id, title="S")
+    setup.add(scenario)
+    setup.flush()
+    adv = models.Adventure(
+        user_id=user.id, title="Vault", scenario_id=scenario.id,
+        script_state={}, world_state={},
+    )
+    setup.add(adv)
+    setup.flush()
+    setup.add(models.Action(adventure_id=adv.id, index=0, type="start", text="You begin."))
+    setup.commit()
+    adv_id, user_id = adv.id, user.id
+    setup.close()
+
+    ScriptedProvider.replies = [f"Take {n}." for n in range(1, 40)]
+    ScriptedProvider.calls = 0
+    monkeypatch.setattr(adventures, "OpenAICompatibleProvider", ScriptedProvider)
+    monkeypatch.setattr(auth, "resolve_provider_config", lambda s: auth.ProviderConfig(
+        "http://fake", "k", "test-model", False))
+    monkeypatch.setattr(limits, "rate_limit", lambda *a, **k: None)
+    monkeypatch.setattr(limits, "check_row_cap", lambda *a, **k: None)
+
+    def _current_user(db=Depends(get_db)):
+        return db.get(models.User, user_id)
+
+    app.dependency_overrides[auth.get_current_user] = _current_user
+    c = TestClient(app)
+    c.adv_id = adv_id
+    try:
+        yield c
+    finally:
+        app.dependency_overrides.clear()
+        adventures._active_turns.clear()
+        Base.metadata.drop_all(bind=engine)
+
+
+def _play(client, text="look around"):
+    r = client.post(f"/api/adventures/{client.adv_id}/actions",
+                    json={"type": "do", "text": text})
+    assert r.status_code == 200, r.text
+
+
+def _retry(client):
+    r = client.post(f"/api/adventures/{client.adv_id}/retry")
+    assert r.status_code == 200, r.text
+
+
+def _takes(client, action_id):
+    r = client.get(f"/api/adventures/{client.adv_id}/actions/{action_id}/variants")
+    assert r.status_code == 200, r.text
+    return r.json()
+
+
+def _edit(client, action_id, text):
+    return client.patch(f"/api/adventures/{client.adv_id}/actions/{action_id}",
+                        json={"text": text})
+
+
+def _live_ai(client):
+    """The AI row the story currently tells, as the transcript reports it."""
+    adv = client.get(f"/api/adventures/{client.adv_id}").json()
+    return [a for a in adv["actions"] if a["type"] == "ai"][-1]
+
+
+def _four_takes(client):
+    """One turn, played four times. Returns (row the page holds, take list)."""
+    _play(client)
+    for _ in range(3):
+        _retry(client)
+    row = _live_ai(client)
+    assert row["take_count"] == 4
+    return row, _takes(client, row["id"])
+
+
+def test_the_pager_reads_four_distinct_takes(client):
+    """The premise: 2/4 and 4/4 are different rows with different words."""
+    row, takes = _four_takes(client)
+    assert [t["text"] for t in takes] == ["Take 1.", "Take 2.", "Take 3.", "Take 4."]
+    assert takes[3]["id"] == row["id"], "the newest take is the one the story tells"
+    assert takes[1]["id"] != row["id"], "2/4 is not the row the transcript is keyed by"
+
+
+def test_editing_a_take_that_is_not_live_edits_that_take(client):
+    """The bug, at the level the client's fix depends on.
+
+    Saving against take 2's own id must land on take 2 — not be refused for
+    being off the path, and not be redirected onto the live row.
+    """
+    row, takes = _four_takes(client)
+    second = takes[1]
+
+    r = _edit(client, second["id"], "Take 2, rewritten.")
+    assert r.status_code == 200, r.text
+    assert r.json()["text"] == "Take 2, rewritten."
+
+    after = _takes(client, row["id"])
+    assert [t["text"] for t in after] == [
+        "Take 1.", "Take 2, rewritten.", "Take 3.", "Take 4.",
+    ], "one take changed, and only the one addressed"
+
+
+def test_editing_a_take_leaves_the_live_one_alone(client):
+    """What the page did instead: the reader saw 2/4 and 4/4 was overwritten."""
+    row, takes = _four_takes(client)
+
+    _edit(client, takes[1]["id"], "Take 2, rewritten.")
+
+    assert _live_ai(client)["text"] == "Take 4.", "the story still tells what it told"
+
+
+def test_a_take_edit_survives_paging_away_and_back(client):
+    """The listing is the pager's only source, so the edit has to be in it.
+
+    (The client caches this list per message; the fix drops that cache after an
+    edit. If the server ever started answering from a copy of its own, stepping
+    away and back would show the words before the edit and nobody would see it
+    here — hence the round trip.)
+    """
+    row, takes = _four_takes(client)
+    _edit(client, takes[1]["id"], "Take 2, rewritten.")
+
+    # Addressed by the live row, as the pager does, and by the edited take
+    # itself, as a client holding that id would.
+    assert _takes(client, row["id"])[1]["text"] == "Take 2, rewritten."
+    assert _takes(client, takes[1]["id"])[1]["text"] == "Take 2, rewritten."
+
+
+def test_an_edited_take_is_still_the_take_it_was(client):
+    """Editing text is not switching, forking, or reordering."""
+    row, takes = _four_takes(client)
+    _edit(client, takes[1]["id"], "Take 2, rewritten.")
+
+    after = _takes(client, row["id"])
+    assert [t["id"] for t in after] == [t["id"] for t in takes], "same rows, same order"
+    assert [t["active"] for t in after] == [False, False, False, True]
+    assert _live_ai(client)["take_index"] == 3, "still 4/4 on screen"

frontend/src/pages/Play.jsx

@@ -971,9 +971,15 @@ function WorldStateDrawer({ advId, refreshKey }) {
 // A take on this branch is a leaf by construction: whatever was played after
 // this turn was played after the take that is live, so a take that is not live
 // has nothing under it and the transcript simply ends there.
-function TakePager({ advId, action, busy, preview, onPreview, onSwitchedBranch, onError }) {
+function TakePager({
+  advId, action, busy, preview, takesKey, onPreview, onSwitchedBranch, onError,
+}) {
   const [takes, setTakes] = useState(null)
   const [loading, setLoading] = useState(false)
+  // The cached list is only as good as the text in it. Editing a take
+  // rewrites one of those rows, so the page says so and the list is fetched
+  // again on the next step.
+  useEffect(() => { setTakes(null) }, [takesKey])
   const count = action.take_count
   const live = action.take_index
   const current = preview ? preview.index : live
@@ -1513,6 +1519,10 @@ export default function Play() {
   // decision. It becomes one when something is written below it, and that is
   // what `after_id` carries.
   const [preview, setPreview] = useState(null)
+  // Bumped when a take's stored text changes under the pagers, which cache the
+  // list they fetched. Nothing else invalidates it: a take is added by playing
+  // a turn, and that re-reads the whole window anyway.
+  const [takesKey, setTakesKey] = useState(0)
   // The transcript is a window on the story, not the whole of it: the page
   // load brings the newest page and older ones arrive as the reader scrolls
   // up. `total` is the story's real length, for the "N earlier" line.
@@ -1821,8 +1831,16 @@ export default function Play() {
     return () => window.removeEventListener('keydown', onKey)
   })
 
+  // A take-edit belongs to the preview that opened it. Anything that leaves
+  // that take — playing a turn, switching branch — takes the box with it, so
+  // the pending edit goes too rather than being saved onto a take nobody is
+  // looking at any more.
+  useEffect(() => {
+    if (editing?.take && preview?.takeId !== editing.id) setEditing(null)
+  }, [editing, preview])
+
   async function saveEdit() {
-    const { id: actionId, text, fork } = editing
+    const { id: actionId, text, fork, take } = editing
     setEditing(null)
     if (fork) {
       // Not an edit at all: the turn is played again with this text, and what
@@ -1832,7 +1850,17 @@ export default function Play() {
     }
     try {
       const updated = await api.updateAction(id, actionId, text)
-      setActions((prev) => prev.map((a) => (a.id === actionId ? updated : a)))
+      // A take that is only being read is not in `actions` — the row there is
+      // the live one — so the new text goes back into the preview, which is
+      // what that row is drawing. The pager holds the take list it fetched, so
+      // it is told to drop it: stepping away and back would otherwise show the
+      // words before the edit.
+      if (take) {
+        setPreview((p) => (p && p.takeId === actionId ? { ...p, text: updated.text } : p))
+        setTakesKey((k) => k + 1)
+      } else {
+        setActions((prev) => prev.map((a) => (a.id === actionId ? updated : a)))
+      }
     } catch (err) {
       setToast({ text: err.message, isError: true })
     }
@@ -1955,7 +1983,14 @@ export default function Play() {
             // message without making it active (earlier turns only).
             const previewing = preview?.actionId === action.id ? preview : null
 
-            return editing?.id === action.id ? (
+            // The editor stands in for the row it was opened from. That row is
+            // keyed by the live node, so an edit on a take the pager is parked
+            // on carries the take's id instead and is matched through the
+            // preview.
+            const editingHere = editing
+              && (editing.take ? previewing?.takeId === editing.id : editing.id === action.id)
+
+            return editingHere ? (
               <div key={action.id} className="action-edit">
                 <AutoTextarea
                   autoFocus
@@ -1988,6 +2023,7 @@ export default function Play() {
                     action={action}
                     busy={busy}
                     preview={previewing}
+                    takesKey={takesKey}
                     onPreview={setPreview}
                     onSwitchedBranch={adoptWindow}
                     onError={(message) => setToast({ text: message, isError: true })}
@@ -1998,13 +2034,29 @@ export default function Play() {
                         <button title="View the exact prompt that produced this"
                           onClick={() => inspect(action.id)}>🔍</button>
                       )}
+                      {/* Edits the take that is *on screen*, which is not the
+                          live one while the pager is parked on another. The
+                          row is keyed by the live node's id, so seeding from
+                          `action` here opened the editor on take 4/4's text
+                          while 2/4 was being read — and saved over it. A take
+                          is an ordinary row to the edit endpoint, on the path
+                          or not, so its own id is all this needs. */}
                       <button title="Edit"
-                        onClick={() => setEditing({ id: action.id, text: action.text })}>✎</button>
+                        onClick={() => setEditing(previewing
+                          ? { id: previewing.takeId, text: previewing.text, take: true }
+                          : { id: action.id, text: action.text })}>✎</button>
                       {/* Play this turn again, differently. On the AI's turn
                           that is a regeneration; on your own it opens the text
                           so you can say something else. Either way the story
                           that followed the old take is kept, on the line it
-                          was written on. */}
+                          was written on.
+
+                          The id stays the live node's even while another take
+                          is being read: adding a take branches just above the
+                          turn, and the server only accepts a turn that is on
+                          the path. Only the seeded text follows the screen, so
+                          varying the take you are reading starts from its
+                          words. */}
                       {action.type !== 'start' && (
                         <button
                           title={action.type === 'ai'
@@ -2012,7 +2064,11 @@ export default function Play() {
                             : 'Say this differently, and keep both'}
                           onClick={() => (action.type === 'ai'
                             ? addTake(action.id, '')
-                            : setEditing({ id: action.id, text: action.text, fork: true }))}
+                            : setEditing({
+                              id: action.id,
+                              text: previewing ? previewing.text : action.text,
+                              fork: true,
+                            }))}
                         >⑂</button>
                       )}
                       <button title="Delete" onClick={() => removeAction(action.id)}>✕</button>