diff --git a/backend/app/routers/adventures.py b/backend/app/routers/adventures.py index f5edd9f..93da26f 100644 --- a/backend/app/routers/adventures.py +++ b/backend/app/routers/adventures.py @@ -4,7 +4,7 @@ import threading from fastapi import APIRouter, Body, Depends, HTTPException, Request from fastapi.responses import StreamingResponse -from sqlalchemy import func +from sqlalchemy import func, select from sqlalchemy.orm import Session, load_only, undefer from sqlalchemy.orm.attributes import set_committed_value @@ -1874,7 +1874,7 @@ def action_context( def list_memories( adventure_id: int, db: Session = Depends(get_db), user: models.User = CurrentUser ): - get_adventure_or_404(adventure_id, db, user) + adventure = get_adventure_or_404(adventure_id, db, user) # A query naming its columns, not a walk of `adventure.memories`. The walk # is what retrieval used to do, and it is the reason a turn cost megabytes: # a relationship load takes whole entities, so it picks up whatever the @@ -1885,13 +1885,32 @@ def list_memories( # the story being played, the drawer manages the bank. Hiding a branch's # memories from the drawer would mean memories nobody can find to delete, # in a phase whose rule is that nothing is ever removed automatically. - return ( + rows = ( db.query(models.Memory) .options(load_only(*MEMORY_LIST_COLUMNS)) .filter(models.Memory.adventure_id == adventure_id) .order_by(models.Memory.id) .all() ) + # ...but listing them alike would be its own lie. A memory on a branch this + # story never travelled is never retrieved, so showing it beside one that is + # tells the player the model remembers something it cannot see. The ids come + # from **the predicate retrieval itself uses**, deliberately: two spellings + # of "on this path" would eventually disagree, and the failure would be a + # badge that says the opposite of what the model gets. + on_path = { + row[0] for row in db.execute( + select(models.Memory.id).where( + models.Memory.adventure_id == adventure_id, + lineage.path_of(db, adventure).clause( + models.Memory, unanchored=True + ), + ) + ) + } + for row in rows: + row.on_path = row.id in on_path + return rows @router.post("/{adventure_id}/memories", response_model=schemas.MemoryOut, status_code=201) diff --git a/backend/app/schemas.py b/backend/app/schemas.py index 0412b64..e43882f 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -289,6 +289,12 @@ class MemoryOut(ORMModel): last_used_at: datetime | None source_start: int | None source_end: int | None + # Whether this memory is on the story currently being read (Phase 14, SP7). + # The drawer lists the whole bank so nothing becomes impossible to find and + # delete, but a memory belonging to another branch will never be retrieved + # into context — and a list that showed the two alike would be telling the + # player the model knows something it cannot see. + on_path: bool = True created_at: datetime diff --git a/backend/tests/test_branch_management.py b/backend/tests/test_branch_management.py index ed85f69..c4c975a 100644 --- a/backend/tests/test_branch_management.py +++ b/backend/tests/test_branch_management.py @@ -31,9 +31,10 @@ os.environ.pop("DATABASE_URL", None) import pytest from fastapi import Depends from fastapi.testclient import TestClient +from sqlalchemy import select from app import auth, limits, models, schemas -from app.context import cursors +from app.context import cursors, lineage from app.database import Base, SessionLocal, engine, get_db from app.main import app from app.providers import PromptParts @@ -346,6 +347,88 @@ def test_deleting_an_unknown_branch_is_a_404(client): assert _delete(client, forked + 9999).status_code == 404 +# --------------------------------------------------------- the bank vs a path + +def _memories(client) -> list[dict]: + r = client.get(f"/api/adventures/{client.adv_id}/memories") + assert r.status_code == 200, r.text + return r.json() + + +def _add_memory(client, text): + r = client.post(f"/api/adventures/{client.adv_id}/memories", json={"text": text}) + assert r.status_code == 201, r.text + return r.json()["id"] + + +def test_the_drawer_keeps_every_memory_but_says_which_are_off_the_path(client): + """Both halves of the split, in one test, because either alone is a bug. + + Listing only the path's memories would leave the rest impossible to find + and delete, in a phase whose rule is that nothing is removed automatically. + Listing them all *alike* would tell the player the model remembers + something that is never retrieved on this branch. + """ + root, forked = _forked(client) + on_the_fork = _add_memory(client, "Took the other door.") + _switch(client, root) + on_the_root = _add_memory(client, "Went the long way instead.") + + listed = {m["id"]: m for m in _memories(client)} + assert set(listed) == {on_the_fork, on_the_root}, "the whole bank, always" + assert listed[on_the_root]["on_path"] is True + assert listed[on_the_fork]["on_path"] is False, "written on a branch we left" + + # And it follows the reader rather than being a property of the memory — + # but **asymmetrically**, which is the part worth pinning. A fork borrows + # its parent's story, so a memory written on the parent is on the fork's + # path too. The reverse is not true: the parent never went down the fork. + _switch(client, forked) + listed = {m["id"]: m for m in _memories(client)} + assert listed[on_the_fork]["on_path"] is True + assert listed[on_the_root]["on_path"] is True, "an ancestor's memory is shared" + + +def test_the_off_path_flag_agrees_with_what_retrieval_can_see(client): + """The flag has to come from retrieval's own predicate, not a second + spelling of it. Two spellings would drift, and the failure mode is a badge + claiming the opposite of what the model is actually given.""" + root, forked = _forked(client) + _add_memory(client, "Took the other door.") + _switch(client, root) + _add_memory(client, "Went the long way instead.") + + db = SessionLocal() + try: + adventure = db.get(models.Adventure, client.adv_id) + visible = { + row[0] for row in db.execute( + select(models.Memory.id).where( + models.Memory.adventure_id == adventure.id, + lineage.path_of(db, adventure).clause( + models.Memory, unanchored=True + ), + ) + ) + } + finally: + db.close() + + flagged = {m["id"] for m in _memories(client) if m["on_path"]} + assert flagged == visible + + +def test_a_memory_from_a_deleted_branch_is_gone_from_the_drawer(client): + """Not merely off-path — the row goes with the branch, through the cascade.""" + root, forked = _forked(client) + doomed = _add_memory(client, "Took the other door.") + _switch(client, root) + assert doomed in {m["id"] for m in _memories(client)} + + assert _delete(client, forked).status_code == 204 + assert doomed not in {m["id"] for m in _memories(client)} + + # ------------------------------------------------------------------- backup def test_a_bundle_carries_the_name_a_player_chose(client): diff --git a/frontend/src/index.css b/frontend/src/index.css index cc40de0..9f933e5 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -1362,6 +1362,17 @@ button:disabled { opacity: 0.45; cursor: default; transform: none; box-shadow: n border-radius: 999px; padding: 0 8px; } +/* A memory belonging to another branch. Set back rather than hidden: it is + still findable and still deletable, but it is never sent to the AI while + this branch is being read, and a row that looked identical to a live one + would be claiming the opposite. The dashed edge is the tell that carries + even when the badge scrolls out of view. */ +.memory-row.off-path { + opacity: 0.6; + border-style: dashed; + background: transparent; +} +.memory-badge.off-path { color: var(--text-dim); } .script-report { margin-top: 14px; border-top: 1px solid var(--border); padding-top: 8px; } .script-error { color: var(--danger); font-size: 0.85rem; margin-bottom: 4px; white-space: pre-wrap; } diff --git a/frontend/src/pages/Play.jsx b/frontend/src/pages/Play.jsx index e1ec388..3010457 100644 --- a/frontend/src/pages/Play.jsx +++ b/frontend/src/pages/Play.jsx @@ -337,7 +337,8 @@ function MemoryRow({ memory, onChange, onDelete }) { } return ( -
+
{editText !== null ? (
{memory.pinned && 📌 pinned} {memory.forgotten && forgotten} + {/* Kept in the list so it stays possible to find and delete, but + said plainly: this one belongs to a branch the story being read + never went down, and it is never retrieved. Pinning does not + override it — the path clause runs before pinning is + considered. */} + {memory.on_path === false && ( + + another branch + + )} {!memory.embedded && !memory.forgotten && ( not embedded yet )} diff --git a/plan/14-phase-story-tree.md b/plan/14-phase-story-tree.md index 5300798..4d0d085 100644 --- a/plan/14-phase-story-tree.md +++ b/plan/14-phase-story-tree.md @@ -767,10 +767,31 @@ never been looked at is whether the *screen* re-reads it. On `tools/branch_fixtu switching between the two branches moves the World State drawer from **hp 60 to hp 95** live, redraws the bar, swaps the story to the other take (`hp -5`, not `hp -40`) and repoints Insights at the other path — "History: 5 of 5 actions", carrying the scratch and -not the beating. The Memory Bank deliberately does *not* change: the drawer is -adventure-wide so a memory can always be found and deleted, and it is retrieval that is -path-scoped (`test_memory_nodes.py`). That split is worth stating out loud, because -"memories did not change when I switched" reads as a bug and is the design. +not the beating. + +**The memory drawer says which memories the model can actually see.** Retrieval has been +path-scoped since SP3, so a memory on a branch this story never travelled is never sent. +The drawer still lists the whole bank — hiding rows would leave memories impossible to +find and delete, in a phase whose rule is that nothing is removed automatically — but +listing them *alike* was its own lie: it told the player the model remembers something it +cannot see. `MemoryOut.on_path` marks them, and the row is set back and labelled *another +branch*. The flag is computed from **the predicate retrieval itself uses** +(`path_of(...).clause(Memory, unanchored=True)`), not a second spelling of it, because two +spellings drift and the failure would be a badge claiming the opposite of what the model +gets. Pinning does not override it — the path clause runs before pinning is considered. + +**The relationship is asymmetric, and a test now says so.** A fork borrows its ancestors, +so a memory written on the parent is on the fork's path too; the reverse is never true. +Worth knowing before anyone "fixes" it into a symmetric check. + +**One known sharp edge, inherited rather than introduced.** A *hand-written* memory has a +branch but no depth, and `unanchored=True` keeps it whatever the lineage cap says — so +one typed on the parent follows you onto a fork whose events it may not describe. That is +SP3's deliberate choice (`test_a_hand_written_memory_is_not_lost_at_the_first_fork`): the +alternative is a memory vanishing at the first fork after it was typed. Auto-summarized +memories carry a depth and *are* capped at the fork, so they behave as expected. If this +ever bites, the fix is to anchor a hand-written memory at the head depth when it is +created rather than to change the clause. **The scroll path was driven, and it holds.** Three prepends on the 602-action fixture, 60 actions and ~16,200 px each. The same DOM node stayed at viewport top 792 → 787 — a diff --git a/plan/STATUS.md b/plan/STATUS.md index ef4ff5a..208b6b9 100644 --- a/plan/STATUS.md +++ b/plan/STATUS.md @@ -166,6 +166,16 @@ That is now four bugs on this frontend found by exercising it rather than by tes two of them in paths that had just shipped. The pattern is not subtle any more: **this frontend has no test runner, so anything not driven by hand is unverified.** +**The memory drawer now says what the model can see.** Retrieval has been path-scoped +since SP3, but the drawer listed every memory alike — so on a fork you read "Fell down +the cellar stairs" and reasonably concluded the AI knew it, when that memory is never +retrieved on that branch. Rows off the current path are set back and labelled *another +branch*, still fully editable and deletable. The flag comes from the predicate retrieval +itself uses, so the badge cannot drift from the behaviour. Note the asymmetry before +changing it: a fork borrows its ancestors, so a parent's memory *is* on the fork's path; +the reverse never is. And a hand-written memory has no depth, so it survives the fork cap +by design — see SP7's entry in `plan/14` for the one case where that surprises. + **The scroll path is finally driven.** 602-action fixture, three prepends of ~16,200 px each: the same DOM node held viewport top 792 → 787, and the view stayed 48,174 px from the bottom. PR #2's fix holds. Measuring note worth keeping — the fixture's prose repeats,