From 0633cb624e15e48dfc474e6550227ae3b9eb8ac1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 10:11:26 +0000 Subject: [PATCH] Run the new memory prompt back over an old bank MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prompt change only reaches memories written after it. plan/18 decided to leave the existing ones alone and let eviction age them out at memory_bank_capacity, on the grounds that re-summarizing would duplicate whatever was still in the bank because nothing deletes the old rows. That was wrong about the only option. A memory can be rewritten in place. The row carries more than its text — whether it is pinned, how often it has been retrieved, and the node it hangs off, which is what makes a fork inherit the right memories — and rewriting `text` keeps all of it. Deleting the bank and rewinding the cursor would lose that, and would trickle memories back at MAX_MEMORIES_PER_RUN per turn, so an adventure nobody is playing would never recover. tools/rewrite_memories.py does it. Without --write it makes no model calls and only reports the scope; --write rewrites, --embed re-embeds in the run rather than leaving it to the app's post-turn pass. It reads whichever database the app reads, so it works against the hosted Postgres as well as a local file. Two things it needed from the app. `summarize_block` is now the one place a memory prompt is assembled, and the post-turn pass calls it too — a backfill that built its own prompt would be writing memories with a prompt that never shipped, and nothing would report the drift. `source_block` reads a memory's block back out of the story, which nothing has ever had to do: it reads on the lineage of the branch the memory was written on, not the branch being played, because after a fork the same depths hold different actions on each side and a read through the adventure's path would summarize the wrong story silently. It also excludes the discarded attempts at a retried turn, and tolerates a block an action has since been deleted from. Left alone: a memory with no source range, which is hand-written or migrated by 62 and may be the player's own words; a memory whose actions are gone; and an adventure whose owner has no API key, because summarization spends the user's own key by construction and never the shared demo key. --api-key/--model/ --endpoint override that, the last of them aiming a run at claude_shim.py. The vector is cleared for every rewrite, because the stored one describes wording that no longer exists. Re-embedding always uses the owner's own embedding model, never --endpoint: a vector only means anything against the vectors it is ranked beside. 17 tests, 627 green. The fork case is the one that would fail quietly, so the test builds a fork whose depths hold different actions on each side. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Tqgupw5CZGjSZrUTNUd4fW --- backend/app/memorybank.py | 87 +++++- backend/tests/test_memory_rewrite.py | 377 ++++++++++++++++++++++++++ backend/tools/rewrite_memories.py | 273 +++++++++++++++++++ plan/18-persona-and-memory-quality.md | 89 +++++- plan/STATUS.md | 33 ++- 5 files changed, 841 insertions(+), 18 deletions(-) create mode 100644 backend/tests/test_memory_rewrite.py create mode 100644 backend/tools/rewrite_memories.py diff --git a/backend/app/memorybank.py b/backend/app/memorybank.py index e66e6d3..0012b62 100644 --- a/backend/app/memorybank.py +++ b/backend/app/memorybank.py @@ -30,7 +30,7 @@ from array import array from collections import OrderedDict from sqlalchemy import func, select, update -from sqlalchemy.orm import Session, object_session +from sqlalchemy.orm import Session, defer, object_session from . import models, tree, vectors from .context import ( @@ -53,6 +53,7 @@ MAX_EMBED_BATCH = 32 RETRIEVAL_WINDOW_TOKENS = 600 # recent story text used as the similarity query RETRIEVAL_WINDOW_ACTIONS = 4 # ...taken from this many of the newest actions SUMMARY_MAX_WORDS = 250 +MEMORY_EXCERPT_TOKENS = 2000 # of the block, when a block is longer than this # ---- The cast brief (Phase 18b) ---- # How many characters the brief names, and how much of each description it @@ -271,6 +272,51 @@ def forget_node(db: Session, adventure: models.Adventure, action: models.Action) return len(doomed) +def source_block(db: Session, memory: models.Memory) -> list[models.Action]: + """The actions a memory was written from, oldest first. + + The inverse of what `_create_due_memories` recorded. `source_start` and + `source_end` are depths, and `branch_id` says which path they are depths + on — that branch's own lineage, not the adventure's current path. A memory + written before a fork must still read back from the branch it was written + on, whichever branch the adventure has since moved to. + + Returns `[]` for a memory that describes no stretch of story. Those are + hand-written, or migrated from before memories had coordinates (see + `lineage.ROOT_DEPTH`), and there is no block to read. + + The range may come back shorter than `MEMORY_INTERVAL`. An action inside it + can have been deleted since, and a memory whose block is now partial still + describes the actions that remain. + """ + if memory.source_start is None or memory.source_end is None: + return [] + if memory.branch_id is None: + return [] # A pre-tree row: no path contains it. + branch = db.get(models.Branch, memory.branch_id) + if branch is None: + return [] + path = lineage.Path(lineage.entries_of(branch)) + rows = ( + db.query(models.Action) + .filter( + models.Action.adventure_id == memory.adventure_id, + # This clause excludes the sibling attempts at a retried turn, so + # the block holds the one text the story used. + path.clause(models.Action), + models.Action.depth >= memory.source_start, + models.Action.depth <= memory.source_end, + ) + # `id` breaks a tie on `depth`, as everywhere else that orders actions. + .order_by(models.Action.depth, models.Action.id) + # Reasoning traces are never part of an excerpt and can outweigh the + # narration on a reasoning model. + .options(defer(models.Action.reasoning)) + .all() + ) + return [a for a in rows if history.is_story_text(a.text)] + + # ---------- The cast brief ---------- def _cast_line(name: str, entry: str, *, protagonist: bool = False) -> str: @@ -529,6 +575,35 @@ async def run_post_turn(adventure_id: int) -> None: _running.discard(adventure_id) +async def summarize_block( + adventure: models.Adventure, + provider: OpenAICompatibleProvider, + block: list[models.Action], +) -> str: + """Writes one memory from one block of story. + + Both callers come through here, which is the point of the function. The + pass below writes a memory as the story reaches it; `tools/rewrite_memories` + rewrites one an older prompt produced. Assembling the prompt in two places + would mean a rewritten memory was written by a prompt that never shipped, + and nothing would report the difference. + + Raises `ProviderError`, which each caller handles its own way: the pass + below leaves the cursor alone and retries next turn, and the tool leaves the + old text in place and moves on. + """ + raw = "\n\n".join(a.text for a in block) + excerpt = truncate_to_last_tokens(raw, MEMORY_EXCERPT_TOKENS) + # Match the cast against the untruncated block. The excerpt is what the + # model reads, but a character named in the part that was trimmed is still + # one the memory may have to name. + brief = cast_brief(adventure, raw) + prompt = f"Story excerpt:\n\n{excerpt}\n\nMemory:" + return await provider.complete( + MEMORY_SYSTEM_PROMPT, f"{brief}\n\n{prompt}" if brief else prompt + ) + + async def _create_due_memories( adventure: models.Adventure, settings: models.Settings, db: Session ) -> None: @@ -548,16 +623,8 @@ async def _create_due_memories( block = history.after(adventure, anchor, MEMORY_INTERVAL) if len(block) < MEMORY_INTERVAL: return - excerpt = truncate_to_last_tokens("\n\n".join(a.text for a in block), 2000) - # Match the cast against the untruncated block. The excerpt is what the - # model reads, but a character named in the part that was trimmed is - # still one the memory may have to name. - brief = cast_brief(adventure, "\n\n".join(a.text for a in block)) - prompt = f"Story excerpt:\n\n{excerpt}\n\nMemory:" try: - text = await provider.complete( - MEMORY_SYSTEM_PROMPT, f"{brief}\n\n{prompt}" if brief else prompt - ) + text = await summarize_block(adventure, provider, block) except ProviderError: return # Logged on the debug page. The cursor is unchanged, so the # next turn retries this block. diff --git a/backend/tests/test_memory_rewrite.py b/backend/tests/test_memory_rewrite.py new file mode 100644 index 0000000..b8446b9 --- /dev/null +++ b/backend/tests/test_memory_rewrite.py @@ -0,0 +1,377 @@ +"""Phase 18b: rewriting memories an older prompt wrote. + +The prompt change only reaches memories written after it. A bank filled before +it keeps its unnamed, second-person entries and injects them into every turn +from then on, so there has to be a way to run the new prompt back over them. + +Two halves, and the first is the one that can be wrong quietly: + +* `memorybank.source_block` reads a memory's block back out of the story. + Nothing in the app has ever had to do that. It has to read on the branch the + memory was written on rather than the one the adventure is playing now, skip + the sibling attempts at a retried turn, and cope with a memory whose actions + have since been deleted. +* `tools/rewrite_memories` replaces the text and clears the vector, leaves a + hand-written memory alone, and writes nothing at all without `--write`. + + python -m pytest tests/test_memory_rewrite.py -v +""" +import argparse +import asyncio + +import pytest + +from app import memorybank, models, tree +from app.context import lineage +from app.database import Base, SessionLocal, engine +from tools import rewrite_memories + + +class StubSummarizer: + """Returns a numbered memory, and records what it was asked. + + The default text is what the new prompt asks for and the `OLD` text is what + the old one produced, so an assertion says which prompt wrote a memory + rather than counting calls. + """ + + OLD = "You entered the crypt {n}." + + def __init__(self, template="Kaelen entered the crypt {n}.", error=None): + self.calls: list[tuple[str, str]] = [] + self.template = template + self.error = error + + async def complete(self, system, user, **kwargs): + self.calls.append((system, user)) + if self.error is not None: + raise self.error + return self.template.format(n=len(self.calls)) + + +class StubEmbedder: + def __init__(self): + self.texts: list[str] = [] + + async def embed(self, texts): + self.texts.extend(texts) + return [[1.0, 0.0, 0.0] for _ in texts] + + +@pytest.fixture() +def db(): + Base.metadata.create_all(bind=engine) + session = SessionLocal() + try: + yield session + finally: + session.close() + Base.metadata.drop_all(bind=engine) + + +def make_adventure(db, *, actions=12, key="sk-test-key", email="rewrite@example.com"): + """An adventure whose bank was written by the old prompt.""" + user = models.User(is_guest=False, email=email) + db.add(user) + db.flush() + # A key with no `enc:` prefix is stored plaintext and read back as-is; see + # `security.decrypt_secret`. That keeps the fixture off the crypto path. + db.add(models.Settings(user_id=user.id, api_key=key, model="test-model", + embedding_model="text-embedding-3-small")) + adventure = models.Adventure( + user_id=user.id, title="Camp", script_state={}, auto_summarize=True, + memory="The player and Gwen are raiding a bandit camp.", + persona_name="Kaelen", persona_pronouns="he/him", + persona_desc="A half-elf ranger.", + ) + db.add(adventure) + db.flush() + db.add(models.StoryCard(adventure_id=adventure.id, name="Gwen", + keys="Gwen, her", type="character", + entry="A loyal ranger and the player's ally.")) + for i in range(actions): + db.add(models.Action(adventure_id=adventure.id, + type="ai" if i % 2 else "do", + text=f"You walk on. Action {i}.")) + db.commit() + db.refresh(adventure) + return adventure + + +def fill_bank(db, adventure, *, count=2): + """Runs the real pass, so the memories carry the coordinates the real ones + carry rather than coordinates this file made up.""" + stub = StubSummarizer(template=StubSummarizer.OLD) + original = memorybank.summary_provider + memorybank.summary_provider = lambda s: stub + try: + settings = (db.query(models.Settings) + .filter(models.Settings.user_id == adventure.user_id).one()) + asyncio.run(memorybank._create_due_memories(adventure, settings, db)) + finally: + memorybank.summary_provider = original + memories = (db.query(models.Memory) + .filter(models.Memory.adventure_id == adventure.id) + .order_by(models.Memory.id).all()) + assert len(memories) == count, f"expected {count} memories, got {len(memories)}" + for memory in memories: + memorybank.set_vector(memory, [1.0, 0.0, 0.0]) + db.commit() + return memories + + +def options(**overrides): + args = dict(write=False, adventure=None, limit=None, include_forgotten=False, + embed=False, endpoint=None, model=None, api_key=None) + args.update(overrides) + return argparse.Namespace(**args) + + +def run_tool(args) -> int: + return asyncio.run(rewrite_memories.main(args)) + + +# ------------------------------------------------------ reading the block back + +def test_the_block_is_the_actions_the_memory_covers(db): + adventure = make_adventure(db) + first, second = fill_bank(db, adventure) + block = memorybank.source_block(db, first) + assert [a.depth for a in block] == list( + range(first.source_start, first.source_end + 1)) + assert len(block) == memorybank.MEMORY_INTERVAL + assert [a.text for a in block] == [f"You walk on. Action {i}." for i in range(6)] + assert [a.text for a in memorybank.source_block(db, second)] == [ + f"You walk on. Action {i}." for i in range(6, 12)] + + +def test_a_hand_written_memory_has_no_block(db): + """It summarizes no actions, so there is nothing to rewrite it from — and + the player may have typed it.""" + adventure = make_adventure(db) + memory = models.Memory(adventure_id=adventure.id, text="Kaelen owes a debt.") + tree.place_memory(db, adventure, memory) + db.add(memory) + db.commit() + assert memorybank.source_block(db, memory) == [] + + +def test_a_deleted_action_shortens_the_block(db): + """A memory whose block is now partial still describes what remains.""" + adventure = make_adventure(db) + first, _ = fill_bank(db, adventure) + db.delete(memorybank.source_block(db, first)[2]) + db.commit() + block = memorybank.source_block(db, first) + assert len(block) == memorybank.MEMORY_INTERVAL - 1 + assert "Action 2." not in [a.text for a in block] + + +def test_a_retried_turn_contributes_only_its_live_attempt(db): + """Sibling attempts share a coordinate. The block holds the one the story + used, not both.""" + adventure = make_adventure(db) + first, _ = fill_bank(db, adventure) + kept = memorybank.source_block(db, first)[3] + db.add(models.Action(adventure_id=adventure.id, branch_id=kept.branch_id, + depth=kept.depth, live=False, type=kept.type, + text="A discarded attempt.")) + db.commit() + texts = [a.text for a in memorybank.source_block(db, first)] + assert "A discarded attempt." not in texts + assert len(texts) == memorybank.MEMORY_INTERVAL + + +def test_the_block_is_read_on_the_branch_the_memory_was_written_on(db): + """The adventure has moved to a fork since. A memory on the trunk still + reads back, and one left on the abandoned continuation reads that + continuation rather than the branch now being played.""" + adventure = make_adventure(db) + trunk_memory, _ = fill_bank(db, adventure) + trunk = tree.head_branch(db, adventure) + + fork = models.Branch(adventure_id=adventure.id, parent_branch_id=trunk.id, + fork_depth=5, lineage=[]) + db.add(fork) + db.flush() + fork.lineage = [[fork.id, None], [trunk.id, 5]] + for depth in (6, 7, 8, 9, 10, 11): + db.add(models.Action(adventure_id=adventure.id, branch_id=fork.id, + depth=depth, type="do", text=f"Fork action {depth}.")) + db.flush() + tip = (db.query(models.Action) + .filter(models.Action.branch_id == fork.id) + .order_by(models.Action.depth.desc()).first()) + fork_memory = models.Memory(adventure_id=adventure.id, text="On the fork.", + source_start=6, source_end=11) + tree.attach_memory(fork_memory, tip) + db.add(fork_memory) + adventure.head_branch_id = fork.id + adventure.head_depth = 11 + db.commit() + + # The trunk memory predates the fork and is inherited, so it reads the same + # actions from either branch. + assert [a.text for a in memorybank.source_block(db, trunk_memory)] == [ + f"You walk on. Action {i}." for i in range(6)] + # The abandoned continuation's memory covers depths 6..11 on the trunk. The + # fork covers the same depths with different actions, and the head is on the + # fork, so a read that used the adventure's path would return the fork's. + abandoned = (db.query(models.Memory) + .filter(models.Memory.branch_id == trunk.id, + models.Memory.source_start == 6).one()) + assert [a.text for a in memorybank.source_block(db, abandoned)] == [ + f"You walk on. Action {i}." for i in range(6, 12)] + assert [a.text for a in memorybank.source_block(db, fork_memory)] == [ + f"Fork action {i}." for i in range(6, 12)] + + +def test_the_rewrite_prompt_is_the_one_the_app_sends(db): + """`summarize_block` is the app's own prompt assembly. A rewrite that built + its own would be written by a prompt that never shipped.""" + adventure = make_adventure(db) + first, _ = fill_bank(db, adventure) + stub = StubSummarizer() + asyncio.run(memorybank.summarize_block( + adventure, stub, memorybank.source_block(db, first))) + system, user = stub.calls[0] + assert "third person" in system + assert user.index("Cast:") < user.index("Story excerpt:") + assert "Kaelen (he/him) — the protagonist" in user + + +# --------------------------------------------------------------------- the tool + +def test_without_write_nothing_changes(db, monkeypatch): + adventure = make_adventure(db) + first, _ = fill_bank(db, adventure) + stub = StubSummarizer() + monkeypatch.setattr(memorybank, "summary_provider", lambda s: stub) + assert run_tool(options()) == 0 + db.expire_all() + assert stub.calls == [], "a dry run must not call the model" + assert first.text == "You entered the crypt 1." + assert first.embedded is True + + +def test_write_replaces_the_text_and_clears_the_vector(db, monkeypatch): + adventure = make_adventure(db) + first, second = fill_bank(db, adventure) + stub = StubSummarizer() + monkeypatch.setattr(memorybank, "summary_provider", lambda s: stub) + assert run_tool(options(write=True)) == 0 + db.expire_all() + assert [first.text, second.text] == ["Kaelen entered the crypt 1.", "Kaelen entered the crypt 2."] + # The stored vector describes wording that no longer exists, so the memory + # leaves the ranked bank until something embeds the new text. + assert (first.embedded, first.embedding_blob) == (False, None) + + +def test_embed_puts_the_rewritten_memories_back_in_the_bank(db, monkeypatch): + adventure = make_adventure(db) + first, _ = fill_bank(db, adventure) + embedder = StubEmbedder() + summarizer = StubSummarizer() + monkeypatch.setattr(memorybank, "summary_provider", lambda s: summarizer) + monkeypatch.setattr(memorybank, "embedding_provider", lambda s: embedder) + assert run_tool(options(write=True, embed=True)) == 0 + db.expire_all() + assert embedder.texts == ["Kaelen entered the crypt 1.", "Kaelen entered the crypt 2."] + assert first.embedded is True + + +def test_a_hand_written_memory_is_left_alone(db, monkeypatch): + adventure = make_adventure(db) + fill_bank(db, adventure) + typed = models.Memory(adventure_id=adventure.id, text="Kaelen owes a debt.") + tree.place_memory(db, adventure, typed) + db.add(typed) + db.commit() + monkeypatch.setattr(memorybank, "summary_provider", lambda s: StubSummarizer()) + assert run_tool(options(write=True)) == 0 + db.expire_all() + assert typed.text == "Kaelen owes a debt." + + +def test_an_owner_with_no_api_key_is_skipped(db, monkeypatch): + """Summarization spends the user's own key and never the shared demo key.""" + adventure = make_adventure(db, key="") + first, _ = fill_bank(db, adventure) + stub = StubSummarizer() + monkeypatch.setattr(memorybank, "summary_provider", lambda s: stub) + assert run_tool(options(write=True)) == 0 + db.expire_all() + assert stub.calls == [] + assert first.text == "You entered the crypt 1." + + +def test_an_api_key_on_the_command_line_covers_that_owner(db, monkeypatch): + """The way to summarize for an owner who has no key of their own, and the + way to point a run at the local Claude shim instead of a paid endpoint.""" + adventure = make_adventure(db, key="") + first, _ = fill_bank(db, adventure) + built: list[tuple] = [] + + def build(*args, **kwargs): + built.append(args) + return StubSummarizer() + + monkeypatch.setattr("app.providers.OpenAICompatibleProvider", build) + assert run_tool(options(write=True, api_key="sk-cli", model="sonnet", + endpoint="http://127.0.0.1:8787/v1")) == 0 + db.expire_all() + assert first.text == "Kaelen entered the crypt 1." + assert built[0][:3] == ("http://127.0.0.1:8787/v1", "sk-cli", "sonnet") + + +def test_limit_stops_early(db, monkeypatch): + adventure = make_adventure(db) + first, second = fill_bank(db, adventure) + monkeypatch.setattr(memorybank, "summary_provider", lambda s: StubSummarizer()) + assert run_tool(options(write=True, limit=1)) == 0 + db.expire_all() + assert first.text == "Kaelen entered the crypt 1." + assert second.text == "You entered the crypt 2." + + +def test_a_provider_error_leaves_the_old_text_and_reports_it(db, monkeypatch): + from app.providers import ProviderError + adventure = make_adventure(db) + first, _ = fill_bank(db, adventure) + monkeypatch.setattr(memorybank, "summary_provider", + lambda s: StubSummarizer(error=ProviderError("nope"))) + assert run_tool(options(write=True)) == 1 + db.expire_all() + assert first.text == "You entered the crypt 1." + assert first.embedded is True + + +def test_only_the_named_adventure_is_touched(db, monkeypatch): + one = make_adventure(db) + two = make_adventure(db, email="other@example.com") + kept, _ = fill_bank(db, one) + changed, _ = fill_bank(db, two) + monkeypatch.setattr(memorybank, "summary_provider", lambda s: StubSummarizer()) + assert run_tool(options(write=True, adventure=[two.id])) == 0 + db.expire_all() + assert kept.text == "You entered the crypt 1." + assert changed.text == "Kaelen entered the crypt 1." + + +def test_an_unknown_adventure_id_is_an_error(db): + make_adventure(db) + assert run_tool(options(adventure=[9999])) == 2 + + +def test_an_evicted_memory_is_left_out_unless_asked_for(db, monkeypatch): + adventure = make_adventure(db) + first, second = fill_bank(db, adventure) + second.forgotten = True + db.commit() + monkeypatch.setattr(memorybank, "summary_provider", lambda s: StubSummarizer()) + assert run_tool(options(write=True)) == 0 + db.expire_all() + assert second.text == "You entered the crypt 2." + assert run_tool(options(write=True, include_forgotten=True)) == 0 + db.expire_all() + assert second.text.startswith("Kaelen entered") diff --git a/backend/tools/rewrite_memories.py b/backend/tools/rewrite_memories.py new file mode 100644 index 0000000..490237f --- /dev/null +++ b/backend/tools/rewrite_memories.py @@ -0,0 +1,273 @@ +"""Rewrite memories already in the bank with the current summarizer prompt. + +Phase 18b changed what the summarizer is told: a cast brief naming the +protagonist and the other characters, a rule to write in the third person, and +`MEMORY_MAX_WORDS`. Only memories written after that change get any of it. An +adventure played before it keeps a bank of "You entered the crypt" — unnamed, +in the second person, and occasionally a hundred-word paragraph — and those are +the memories that get injected into every turn from now on. + +This rewrites them in place, from the same actions they were written from. It is +a one-off backfill, not part of the app. + +**In place, rather than deleting and letting the app re-summarize.** A memory +row carries more than its text: whether it is pinned, how often it has been +retrieved, and the node it hangs off, which is what makes a fork inherit the +right memories. Rewriting `text` keeps all of that. Deleting the bank and +rewinding the cursor would lose it, and would then trickle memories back at +`MAX_MEMORIES_PER_RUN` per turn. + +**The prompt is not assembled here.** `memorybank.summarize_block` is what the +app itself calls, so a rewritten memory is written by the prompt that is +actually shipping rather than by a copy of it that can drift. + +Reading the block back is the one thing the app has never had to do. A memory +records `source_start`, `source_end` and `branch_id`; `memorybank.source_block` +turns those back into actions, on the branch the memory was written on rather +than whichever branch the adventure is on now. + +What it will not touch: + +- A memory with no source range: hand-written, or migrated from before memories + had coordinates. There is no block to rewrite it from, and the player may have + typed it. +- A memory whose actions have since been deleted. +- An adventure whose owner has no API key in Settings. Summarization spends the + user's own key by construction and never the shared demo key (see the comment + above `memorybank.summary_provider`), and this holds to that. Pass + `--endpoint`/`--model`/`--api-key` to summarize with something else. + +The vector is cleared for every memory it rewrites, because the stored one +describes the old wording. `--embed` re-embeds them here; without it the app's +own post-turn pass does it, `MAX_EMBED_BATCH` per turn, and until then those +memories are out of the ranked bank. **Stop the app before running with +`--embed`.** A running process caches vectors by memory id and expects to be the +one writing them (see `memorybank._vector_cache`), so a vector written from +outside it can sit behind a stale cached copy until it restarts. + +`--embed` always uses the embedding model and endpoint in the owner's Settings, +never `--endpoint`. A vector is only meaningful against the vectors it is ranked +beside, so re-embedding two memories with a different model would quietly put +two coordinate systems in one bank. `--endpoint` moves the summarizer alone. + +Usage, from `backend/`: + + python -m tools.rewrite_memories # what would change + python -m tools.rewrite_memories --write --limit 3 # try three of them + python -m tools.rewrite_memories --write --embed # the whole backfill + +Without `--write` it makes no model calls, spends nothing, and only reports the +scope. Every run reads the database the app reads: `AIDND_DB_PATH`, or +`DATABASE_URL` for a hosted Postgres. Take a copy of it first — the old text is +overwritten and is not kept anywhere. +""" +import argparse +import asyncio +import sys +from pathlib import Path + +from sqlalchemy import inspect as sa_inspect + + +def words(text: str) -> int: + return len(text.split()) + + +def one_line(text: str, width: int = 96) -> str: + flat = " ".join(text.split()) + return flat if len(flat) <= width else flat[: width - 1] + "…" + + +async def main(args) -> int: + from app import memorybank, models + from app.database import DB_PATH, DATABASE_URL, SessionLocal + from app.providers import OpenAICompatibleProvider, ProviderError + + db = SessionLocal() + print(f"database: {DATABASE_URL or DB_PATH}") + if not sa_inspect(db.get_bind()).has_table(models.Adventure.__tablename__): + # A mistyped path creates an empty SQLite file rather than failing, so + # say what is wrong instead of raising "no such table: adventures". + print("There are no tables here. Point AIDND_DB_PATH, or DATABASE_URL " + "for a hosted deploy, at the database the app uses.") + return 2 + + adventures = db.query(models.Adventure).order_by(models.Adventure.id) + if args.adventure: + adventures = adventures.filter(models.Adventure.id.in_(args.adventure)) + adventures = adventures.all() + if args.adventure and len(adventures) != len(set(args.adventure)): + found = {a.id for a in adventures} + missing = sorted(set(args.adventure) - found) + print(f"no such adventure: {', '.join(str(i) for i in missing)}") + return 2 + + # Settings are per user, and several adventures usually share one owner. + settings_by_user: dict[int, models.Settings | None] = {} + + def settings_for(user_id: int) -> models.Settings | None: + if user_id not in settings_by_user: + settings_by_user[user_id] = ( + db.query(models.Settings) + .filter(models.Settings.user_id == user_id) + .first() + ) + return settings_by_user[user_id] + + def provider_for(settings: models.Settings) -> OpenAICompatibleProvider: + """The adventure owner's own summarizer, unless the run overrides it.""" + if not (args.endpoint or args.model or args.api_key): + return memorybank.summary_provider(settings) + return OpenAICompatibleProvider( + args.endpoint or settings.endpoint_url, + args.api_key or settings.api_key_plain, + args.model or settings.summary_model or settings.model, + settings.api_mode, + settings.reasoning_max_tokens, + ) + + totals = {"rewritten": 0, "would rewrite": 0, "no source": 0, + "no story left": 0, "no key": 0, "failed": 0, "empty reply": 0} + rewritten: list[models.Memory] = [] + budget = args.limit + + for adventure in adventures: + memories = ( + db.query(models.Memory) + .filter(models.Memory.adventure_id == adventure.id) + .order_by(models.Memory.id) + ) + if not args.include_forgotten: + memories = memories.filter(models.Memory.forgotten.is_(False)) + memories = memories.all() + if not memories: + continue + + settings = settings_for(adventure.user_id) + # An adventure whose owner has no key is reported rather than skipped + # silently: it is the one reason a memory this tool can rewrite is left + # alone, and the operator can fix it with --api-key. + usable = settings is not None and bool( + args.api_key or args.endpoint or settings.api_key_plain + ) + print(f"\nadventure {adventure.id}: {adventure.title!r} " + f"— {len(memories)} memories" + + ("" if usable else " [owner has no API key in Settings]")) + + for memory in memories: + if memory.source_start is None or memory.source_end is None: + totals["no source"] += 1 + print(f" #{memory.id} skipped: hand-written, no source range") + continue + block = memorybank.source_block(db, memory) + if not block: + totals["no story left"] += 1 + print(f" #{memory.id} skipped: the actions it summarized are gone") + continue + if not usable: + totals["no key"] += 1 + continue + if not args.write: + totals["would rewrite"] += 1 + print(f" #{memory.id} would rewrite from {len(block)} actions " + f"({words(memory.text)}w): {one_line(memory.text, 70)}") + continue + if budget is not None and budget <= 0: + break + try: + text = (await memorybank.summarize_block( + adventure, provider_for(settings), block)).strip() + except ProviderError as exc: + totals["failed"] += 1 + print(f" #{memory.id} FAILED: {exc}") + continue + if not text: + totals["empty reply"] += 1 + print(f" #{memory.id} FAILED: the model returned nothing") + continue + print(f" #{memory.id} {words(memory.text)}w → {words(text)}w") + print(f" was: {one_line(memory.text)}") + print(f" now: {one_line(text)}") + memory.text = text + # The stored vector describes the wording that has just been + # replaced. Clearing it takes the memory out of the ranked bank + # (`retrieve_memories` selects on `embedded`) until something + # embeds the new text. + memorybank.set_vector(memory, None) + # Commit each one. A run that dies halfway keeps what it has done, + # and re-running it costs only the memories still to do. + db.commit() + rewritten.append(memory) + totals["rewritten"] += 1 + if budget is not None: + budget -= 1 + if budget is not None and budget <= 0 and args.write: + print("\n--limit reached.") + break + + embedded = 0 + if args.write and args.embed and rewritten: + by_adventure: dict[int, list[models.Memory]] = {} + for memory in rewritten: + by_adventure.setdefault(memory.adventure_id, []).append(memory) + print() + for adventure_id, group in by_adventure.items(): + adventure = db.get(models.Adventure, adventure_id) + settings = settings_for(adventure.user_id) + if settings is None or not settings.embedding_model.strip(): + print(f"adventure {adventure_id}: no embedding model in Settings; " + f"{len(group)} memories left for the app to embed") + continue + done = 0 + for start in range(0, len(group), memorybank.MAX_EMBED_BATCH): + batch = group[start:start + memorybank.MAX_EMBED_BATCH] + try: + new = await memorybank.embedding_provider(settings).embed( + [m.text for m in batch]) + except ProviderError as exc: + print(f"adventure {adventure_id}: embedding failed ({exc}); " + f"{len(group) - done} left for the app to embed") + break + for memory, vector in zip(batch, new): + memorybank.set_vector(memory, vector) + db.commit() + done += len(batch) + embedded += done + if done: + print(f"adventure {adventure_id}: embedded {done}") + + print("\n" + "-" * 60) + for label, count in totals.items(): + if count: + print(f"{label:>14}: {count}") + if not args.write: + print("\nNothing was written. Re-run with --write to rewrite these.") + elif totals["rewritten"] - embedded > 0: + left = totals["rewritten"] - embedded + print(f"\n{left} rewritten {'memory is' if left == 1 else 'memories are'}" + " unembedded, so they are out of the ranked bank until the app's " + f"post-turn pass embeds them ({memorybank.MAX_EMBED_BATCH} per " + "turn).") + db.close() + return 1 if totals["failed"] or totals["empty reply"] else 0 + + +if __name__ == "__main__": + parser = argparse.ArgumentParser(description=__doc__.split("\n")[0]) + parser.add_argument("--write", action="store_true", + help="rewrite. Without it, only report what would change.") + parser.add_argument("--adventure", type=int, action="append", + help="restrict to this adventure id; repeatable.") + parser.add_argument("--limit", type=int, + help="stop after rewriting this many memories.") + parser.add_argument("--include-forgotten", action="store_true", + help="also rewrite evicted memories, which are out of play.") + parser.add_argument("--embed", action="store_true", + help="re-embed here. Stop the app first; see the docstring.") + parser.add_argument("--endpoint", help="override the owner's endpoint URL.") + parser.add_argument("--model", help="override the owner's summary model.") + parser.add_argument("--api-key", help="override the owner's API key.") + args = parser.parse_args() + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + raise SystemExit(asyncio.run(main(args))) diff --git a/plan/18-persona-and-memory-quality.md b/plan/18-persona-and-memory-quality.md index 6152a7c..5ad4644 100644 --- a/plan/18-persona-and-memory-quality.md +++ b/plan/18-persona-and-memory-quality.md @@ -4,9 +4,10 @@ Two changes, in order. Phase 1 gives the adventure a persona. Phase 2 uses it, along with the cast, to fix the memories. Phase 1 is worth shipping on its own; Phase 2 depends on it and is much smaller once it lands. -**Both phases are built and green (610 backend tests). Phase 1 was driven in a +**Both phases are built and green (627 backend tests). Phase 1 was driven in a browser (21/21 checks). Phase 2 was run end to end against a real model, as a -controlled A/B on one story — see "Run with a real model".** +controlled A/B on one story — see "Run with a real model". A bank written under +the old prompt can be rewritten in place — see the last section.** **Last updated: 2026-08-31.** @@ -446,11 +447,12 @@ lines. A test holds that. ## Decided, from the sketch's open questions -**Existing memories: left alone.** A bank written under the old prompt will mix -"you" and named framing for a while. Re-summarizing costs one call per 6 actions -and duplicates whatever is still in the bank, because nothing deletes the old -rows. Eviction ages them out at `memory_bank_capacity` (80). Revisit only if the -mixture visibly hurts. +**Existing memories: left alone at first, and now rewritable on demand.** The +original answer was to let eviction age them out at `memory_bank_capacity` (80), +on the grounds that re-summarizing would duplicate whatever was still in the +bank, because nothing deletes the old rows. That reasoning was wrong about the +only option: a memory can be rewritten in place rather than written again. +See "Rewriting a bank written under the old prompt", at the end of this file. **Retrieval framing mismatch: watched, not fixed.** `retrieve_memories` still embeds the last 4 actions raw, in second person, while new memories are third @@ -545,3 +547,76 @@ correctly: the model sent `{"milestones.strongbox_found": false}`, `apply_delta` refused it ("a milestone is sticky, so only true is accepted"), the refusal reached the model through `render_refusals`, and the next turn sent `true`. The Phase 1 persona also held across all six generated turns. + +--- + +# Rewriting a bank written under the old prompt + +An adventure played before this change keeps a bank of unnamed, second-person +memories, and those are exactly the rows `memory_top_k` injects into every turn +from now on. Ageing them out only works for an adventure that is still being +played, and only after another 80 memories have been written. So there is a +backfill: `backend/tools/rewrite_memories.py`. + +``` +cd backend +python -m tools.rewrite_memories # what would change +python -m tools.rewrite_memories --write --limit 3 # try three of them +python -m tools.rewrite_memories --write --embed # the whole backfill +``` + +Without `--write` it makes no model calls and spends nothing. It reads whichever +database the app reads — `AIDND_DB_PATH`, or `DATABASE_URL` on a hosted deploy. + +**In place, not delete-and-regenerate.** The obvious alternative is to drop the +bank and rewind `cursors.MEMORY`, letting the post-turn pass write it again. +That loses everything the row carries besides its text: whether it is pinned, +how often it has been retrieved, and the node it hangs off, which is what makes +a fork inherit the right memories and no others. It would also trickle the bank +back at `MAX_MEMORIES_PER_RUN` per turn, so an adventure nobody is playing would +never recover. Rewriting `text` keeps the row and costs one call per memory. + +**One prompt assembly, not two.** `memorybank.summarize_block` is now the single +place a memory prompt is built, and both the post-turn pass and the tool call +it. A backfill that assembled its own prompt would be writing memories with a +prompt that never shipped, and nothing would report the difference. + +**Reading the block back is the one genuinely new part.** A memory records +`source_start`, `source_end` and `branch_id`, and `memorybank.source_block` +turns those back into actions. The subtlety is the branch: the read has to use +the lineage of *the branch the memory was written on*, not the branch the +adventure is playing now. After a fork, the same depths hold different actions +on each side, so a read through the adventure's current path would summarize the +wrong story and say nothing about it. `test_memory_rewrite.py` builds that fork +and holds the rule. + +**What it will not touch:** + +- A memory with no source range — hand-written, or migrated by 62 from before + memories had coordinates. There is no block to rewrite it from, and the player + may have typed it. +- A memory whose actions have since been deleted. +- An adventure whose owner has no API key in Settings, because summarization + spends the user's own key by construction and never the shared demo key. + `--api-key`, `--model` and `--endpoint` override that — the last of these + points a run at `tools/claude_shim.py`, so a backfill can spend a Claude + subscription instead of API credit. + +**The vector is cleared for every memory it rewrites**, because the stored one +describes wording that no longer exists. That takes the memory out of the ranked +bank until something embeds the new text: the app's own post-turn pass does it +`MAX_EMBED_BATCH` at a time, or `--embed` does it in the run. Re-embedding always +uses the owner's own embedding model and endpoint, never `--endpoint`, because a +vector is only meaningful against the vectors it is ranked beside. + +**Stop the app before running with `--embed`.** A running process caches vectors +by memory id and expects to be the only writer (`memorybank._vector_cache`), so +a vector written from outside it can sit behind a stale cached copy until it +restarts. Clearing alone is safe at any time: an unembedded memory leaves the +catalogue, which is what the cache invalidates on. + +**The story summary is not rewritten.** It is one text per adventure rather than +a bank, and `_update_story_summary` hands the model the whole of it and asks for +an updated version under the new framing rule, so the next scheduled update +should carry it over to third person by itself. If it does not, that is a +separate and much smaller fix than this one. diff --git a/plan/STATUS.md b/plan/STATUS.md index d22ee31..8976baf 100644 --- a/plan/STATUS.md +++ b/plan/STATUS.md @@ -80,7 +80,7 @@ needed; nothing requires reading a row of anyone's story. **`plan/18-persona-and-memory-quality.md` is the writeup; it is on `claude/ai-dnd-memories-summarization-3muo98`, not yet merged.** Two changes, both green -at 610 tests. +at 627 tests, plus the backfill below. **The protagonist now has a name.** An adventure carries `persona_name`, `persona_pronouns` and `persona_desc` (migrations 74-76), and the player's stat block @@ -120,6 +120,37 @@ through the shim. The app talks to an OpenAI-compatible endpoint, and `worldstate/parse.py` tolerates trailing commas because free models emit them. Point `memory_ab.py --endpoint` at the real provider to find out. +**A bank written under the old prompt can now be rewritten.** The earlier answer was to +let eviction age those memories out, on the grounds that re-summarizing would duplicate +rows. It does not have to: `backend/tools/rewrite_memories.py` rewrites a memory's text +in place from the same actions it was written from, keeping the pin, the use counts and +the node it hangs off. Run it from `backend/`; without `--write` it makes no model calls +and only reports what would change. + +``` +python -m tools.rewrite_memories # what would change +python -m tools.rewrite_memories --write --limit 3 # try three of them +python -m tools.rewrite_memories --write --embed # the whole backfill +``` + +It reads whichever database the app reads (`AIDND_DB_PATH`, or `DATABASE_URL` on the +hosted deploy), so **take a copy first** — the old text is overwritten and kept nowhere. +`--endpoint`/`--model`/`--api-key` point the summarizer somewhere else, `claude_shim.py` +included. Hand-written memories, and memories whose actions have been deleted, are left +alone; so is an adventure whose owner has no API key, because summarization spends the +user's own key and never the demo key. + +**Two things it made necessary in the app.** `memorybank.summarize_block` is now the one +place a memory prompt is assembled, so a backfilled memory cannot be written by a prompt +that never shipped. And `memorybank.source_block` reads a memory's block back out of the +story — on the lineage of the branch the memory was written on, not the branch being +played, because after a fork the same depths hold different actions on each side. + +**The vector is cleared for every memory it rewrites**, since the stored one describes +the old wording; the app's post-turn pass re-embeds them `MAX_EMBED_BATCH` per turn, or +`--embed` does it in the run. Stop the app before using `--embed`: a running process +caches vectors by memory id and expects to be the only writer. + --- ## Pick up here