diff --git a/cmd/codeaf/chatv3_memory_upgrade_test.go b/cmd/codeaf/chatv3_memory_upgrade_test.go new file mode 100644 index 0000000000..0da929e703 --- /dev/null +++ b/cmd/codeaf/chatv3_memory_upgrade_test.go @@ -0,0 +1,147 @@ +package main + +import ( + "database/sql" + "os" + "path/filepath" + "testing" + + "github.com/Agent-Field/codeaf/internal/home" + "github.com/Agent-Field/codeaf/internal/store" +) + +// The memories table the released build left on disk: no owner column, and an +// index set the owner-aware build's schema must not assume. It is spelled here +// rather than read from the store because this test has to keep describing the +// OLD shape after the current schema moves on. +const releasedMemoriesDDL = ` +CREATE TABLE memories ( + id TEXT PRIMARY KEY, + type TEXT NOT NULL, + scope TEXT NOT NULL, + title TEXT NOT NULL, + text TEXT NOT NULL, + tags TEXT NOT NULL DEFAULT '[]', + status TEXT NOT NULL DEFAULT 'active', + use_count INTEGER NOT NULL DEFAULT 0, + miss_count INTEGER NOT NULL DEFAULT 0, + created_seq INTEGER NOT NULL, + updated_seq INTEGER NOT NULL, + source_session TEXT NOT NULL DEFAULT '', + source_seq INTEGER NOT NULL DEFAULT 0 +); +CREATE INDEX memories_status_updated ON memories (status, updated_seq DESC); +CREATE INDEX memories_status_scope ON memories (status, scope, updated_seq DESC); +CREATE VIRTUAL TABLE memories_fts USING fts5( + memory_id UNINDEXED, + title, + text, + tags +); +` + +// THE LAUNCH A PERSON ACTUALLY MAKES. Someone who has been running codeaf for +// weeks opens the new build, and the start path asks [v3Memory] for their +// brain. It must not answer nil — which is what it did, printing "memory is off +// for this session: initialize memories schema: no such column: owner" — and +// the note they had kept must still be in it. +func TestLaunchingOnADatabaseFromBeforeOwnersKeepsTheNote(t *testing.T) { + fresh := t.TempDir() + t.Setenv("HOME", fresh) + root := filepath.Join(fresh, ".codeaf") + t.Setenv(home.EnvVar, root) + if err := os.MkdirAll(root, 0o700); err != nil { + t.Fatal(err) + } + + // The database the released build wrote, with one note the person kept. + raw, err := sql.Open("sqlite", defaultChatDB()) + if err != nil { + t.Fatalf("create the old database: %v", err) + } + if _, err := raw.Exec(releasedMemoriesDDL); err != nil { + t.Fatalf("create the old memories table: %v", err) + } + if _, err := raw.Exec(` + INSERT INTO memories (id, type, scope, title, text, tags, status, use_count, + miss_count, created_seq, updated_seq, source_session, source_seq) + VALUES ('mem_kept', 'preference', 'user', 'Keeps the receipts', + 'Keeps the receipts for the tax year.', '["finance"]', 'active', + 2, 0, 11, 12, 'session-old', 7)`); err != nil { + t.Fatalf("insert the old note: %v", err) + } + // The released build kept its own search view, so the old database carries + // one; the launch must leave it answering under the new owner filter. + if _, err := raw.Exec(` + INSERT INTO memories_fts (memory_id, title, text, tags) + SELECT id, title, text, replace(replace(replace(tags, '[', ''), ']', ''), '"', '') + FROM memories WHERE id = 'mem_kept' AND status = 'active'`); err != nil { + t.Fatalf("index the old note: %v", err) + } + if err := raw.Close(); err != nil { + t.Fatal(err) + } + + profile := t.TempDir() + brain := v3Memory(profile) + if brain == nil { + t.Fatal("launching on a database from before owners opened no brain") + } + rows, err := brain.ListMemories([]string{store.OwnerUser}, 20) + if err != nil { + t.Fatalf("read the person's memories: %v", err) + } + if len(rows) != 1 { + t.Fatalf("the person's shelf = %d rows, want the one they kept", len(rows)) + } + kept := rows[0] + if kept.ID != "mem_kept" || kept.Title != "Keeps the receipts" || + kept.Text != "Keeps the receipts for the tax year." { + t.Fatalf("the note came back as %+v, want it preserved", kept) + } + if kept.Owner != store.OwnerUser { + t.Fatalf("the note's owner = %q, want %q", kept.Owner, store.OwnerUser) + } + if kept.SourceSession != "session-old" || kept.SourceSeq != 7 { + t.Fatalf("the note's provenance = (%q, %d), want it preserved", kept.SourceSession, kept.SourceSeq) + } + // Search on the launched brain answers the note the person kept, under the + // owner the upgrade gave it. + found, err := brain.SearchMemories([]string{store.OwnerUser}, "receipts", 5) + if err != nil { + t.Fatalf("search the launched brain: %v", err) + } + if len(found) != 1 || found[0].ID != "mem_kept" { + t.Fatalf("search on the launched brain = %v, want the kept note", found) + } + if err := brain.Close(); err != nil { + t.Fatal(err) + } + + // The owner index must survive the launch, because the retrieval reads are + // written against it and the schema can no longer declare it. + check, err := sql.Open("sqlite", defaultChatDB()) + if err != nil { + t.Fatal(err) + } + defer check.Close() + var name string + if err := check.QueryRow(`SELECT name FROM sqlite_master WHERE type = 'index' AND name = 'memories_owner_active'`).Scan(&name); err != nil { + t.Fatalf("the launch did not leave the owner index: %v", err) + } + + // And the second launch reads the same note rather than doing the upgrade + // twice. + brain = v3Memory(profile) + if brain == nil { + t.Fatal("the second launch opened no brain") + } + defer brain.Close() + again, err := brain.ListMemories([]string{store.OwnerUser}, 20) + if err != nil { + t.Fatalf("read on the second launch: %v", err) + } + if len(again) != 1 || again[0].Text != kept.Text { + t.Fatalf("the second launch = %+v, want the same note", again) + } +} diff --git a/docs/changes/unreleased/1787-memory-upgrade.md b/docs/changes/unreleased/1787-memory-upgrade.md new file mode 100644 index 0000000000..34c8aed243 --- /dev/null +++ b/docs/changes/unreleased/1787-memory-upgrade.md @@ -0,0 +1,33 @@ +--- +kind: fixed +title: a store written before memory owners existed opens again on the next launch +pr: 1787 +surface: [chat, engine] +invalidates: + - "The memories schema ordered the owner index before the owner column existed, so opening any database written by an earlier release died with `initialize memories schema: no such column: owner` and the launch carried on with no brain. The index is now created beside the column by the owner migration, on a legacy store and a brand new one alike." + - "The owner migration returned early the moment the column was present, so an upgrade interrupted between the column and its index would have been left without the index forever. It now ensures the index on every open, whichever shape the store is in." + - "A memory written by an older build that is still running against an upgraded store landed with an empty owner, so no owner-filtered read — the router's shortlist, search, a forget match — could ever reach it. Every open now owns empty rows from their scope again (or quarantines them), and a row that already has an owner is never moved." +--- + +A database that failed to open under the broken build keeps its notes: the +failure happens before any memory row is deleted or rewritten, so a line's words, +its provenance and whether it was still active are all still in the file. There +is no repair command and no backup to restore — the next open of this build adds +the missing column, gives every row the owner its old scope proves, creates the +owner index, and reads the same lines back. A row the old store kept as a project +cannot prove which project it was, so it also gains the `legacy-project` +quarantine marker and needs a proven owner before a conversation sees it again; +nothing else about it is rewritten. + +Recovery is a launch of the fixed build, and installing it does not by itself +change an engine already running. On the machine holding the workspace, updating +codeaf and reconnecting or opening a conversation there is the whole repair: a +newer build replaces an older engine as the window connects, busy or not (the +turn it catches stops where it is and keeps its partial reply), and the new +engine repairs the file as it opens. A window held by an engine on another +machine is not reached by that reconnect — update codeaf there, and when its work +is safe run `codeaf engine --stop --workspace ` there first +(`codeaf engine --status` names the engine if the folder is unclear). A plain +launch with no engine holding it repairs the file at its next open. An idle +engine of the same build may restart to pick up a changed terminal environment; a +busy one keeps running and is only replaced when a newer build connects. diff --git a/internal/manual/chat/what-i-remember.md b/internal/manual/chat/what-i-remember.md index 4a823484d1..49f7d03e24 100644 --- a/internal/manual/chat/what-i-remember.md +++ b/internal/manual/chat/what-i-remember.md @@ -152,6 +152,41 @@ that teaches rather than an error. It did not always: a first run once reported every reason a file will not open, and a build old enough to print it is a build worth replacing. +## It said no such column: owner + +That sentence is the second half of `memory is off for this session:`, and it +means the saved store was written by an older codeaf that the version you just +opened did not know how to read. It is a mismatch between the file and the +program, not damage to the file. + +The repair is the newer codeaf itself. Its first open of the file adds what the +old file was missing and reads the same lines back, keeping each note's words, +where it came from and whether it was still active. A note the old store kept as +**this project** had no way to name its project, so it also gains the quarantine +marker `legacy-project` and needs a proven owner before any conversation is +shown it again. There is nothing to move aside and nothing to restore by hand. + +**A window whose conversation is held by an engine keeps talking to that engine +until a newer build takes its place.** That engine is a process of its own on the +machine holding the workspace; installing a newer codeaf beside it does not +change it. So: + +- Update codeaf on the machine holding the workspace. +- Reconnect or open a conversation in that workspace. On that same machine a + newer build replaces the older engine as the window connects, busy or not — a + turn it catches stops where it is and keeps its partial reply — and the new + engine repairs the file as it opens. +- A window held by an engine on **another** machine is not reached by this + machine's reconnect: update codeaf there too, then when its work is safe run + `codeaf engine --stop --workspace ` there and reconnect. + `codeaf engine --status` names the engine if you are unsure which folder it is. + +A conversation on this machine that no engine is holding is simpler: quit codeaf +and open it again, and the file is repaired as it opens. An idle engine of the +same build may restart to pick up a changed terminal environment; a busy one +keeps running and is only replaced when a newer build connects. Nothing already +saved is at risk while it waits. + ## Where is everything you remember kept In `~/.codeaf/graph.db`, one file, made the first time codeaf runs. Memories, diff --git a/internal/manual/chat_test.go b/internal/manual/chat_test.go index b706b99b57..ada3e24161 100644 --- a/internal/manual/chat_test.go +++ b/internal/manual/chat_test.go @@ -1522,6 +1522,11 @@ func TestTheChatManualAnswersTheQuestionsPeopleAsk(t *testing.T) { {"it said memory is off but I never turned it off", "what-i-remember"}, {"why does it say could not open graph.db", "what-i-remember"}, {"codeaf printed out of memory 14 on startup", "what-i-remember"}, + // The upgraded-store failure a person saw on the terminal: the exact + // sentence has to reach the page that tells them the file is fine and + // what to restart. + {"it said no such column: owner when codeaf opened", "what-i-remember"}, + {"memory is off for this session: initialize memories schema: no such column: owner", "what-i-remember"}, {"where is my memory file kept on disk", "what-i-remember"}, {"can I copy my memories to another machine", "what-i-remember"}, {"how do I see what codeaf remembers", "what-i-remember"}, diff --git a/internal/store/memory.go b/internal/store/memory.go index 7ac6a81f97..6efed4b29f 100644 --- a/internal/store/memory.go +++ b/internal/store/memory.go @@ -214,7 +214,13 @@ CREATE TABLE IF NOT EXISTS memories ( ); CREATE INDEX IF NOT EXISTS memories_status_updated ON memories (status, updated_seq DESC); CREATE INDEX IF NOT EXISTS memories_status_scope ON memories (status, scope, updated_seq DESC); -CREATE INDEX IF NOT EXISTS memories_owner_active ON memories (owner, updated_seq DESC) WHERE status = 'active'; +-- THE OWNER INDEX IS NOT HERE. owner is a migration column: a database written +-- before owners existed already has this table, so CREATE TABLE IF NOT EXISTS +-- leaves it alone, and an index declared here would be created against a column +-- that does not exist yet — which is exactly the open that failed with +-- "no such column: owner" on every store already on disk. It is created by +-- [migrateMemoriesOwner], beside the column it indexes, on both a legacy store +-- and a brand new one. ` const memoriesFTSSchema = ` diff --git a/internal/store/memory_owner.go b/internal/store/memory_owner.go index 85d55596b2..e794768b9c 100644 --- a/internal/store/memory_owner.go +++ b/internal/store/memory_owner.go @@ -142,26 +142,45 @@ func ownerFilterSQL(owners []string) (string, []any) { // that can do the proving ([Store.RehomeMemories]); everything else stays in // quarantine, visible to the person and to nobody's model. // -// It is idempotent — a row that already has an owner is left alone — and it is -// written in one transaction with the column it backfills. +// It is idempotent — a row that already has an owner is left alone — and it +// runs in one transaction on every open, whatever shape the table is in. +// +// THE BACKFILL IS NOT ONLY FOR THE OPEN THAT ADDS THE COLUMN, because a +// pre-owner build can still be RUNNING against a store this build already +// upgraded: it sees the column, never sets it, and writes its rows with the +// empty default. Those rows carry a real scope and would be invisible to every +// owner-filtered read forever, so every open re-runs the same scope-derived +// mapping, each statement guarded by owner is empty. A row with an owner — +// quarantined, machine, person, project — is never touched, by this pass or by +// any later one. +// +// The owner index is created here rather than in [memoriesSchema] for the same +// ordering reason: the schema runs FIRST, against a table that a database +// written before owners existed already has, so an index declared there asks +// for a column the migration has not added yet and the open dies with +// `no such column: owner` (the failure every store already on disk hit). +// Ensuring it here covers both shapes — the legacy store the backfill just +// widened, and a store that already carries the column from an earlier or +// interrupted upgrade. func migrateMemoriesOwner(db *sql.DB) error { found, err := tableHasColumn(db, "memories", "owner") if err != nil { return err } - if found { - return nil - } tx, err := db.BeginTx(context.Background(), nil) if err != nil { return fmt.Errorf("migrate memory owners: %w", err) } defer tx.Rollback() - if _, err := tx.Exec(`ALTER TABLE memories ADD COLUMN owner TEXT NOT NULL DEFAULT ''`); err != nil { - return fmt.Errorf("migrate memory owners: %w", err) + if !found { + if _, err := tx.Exec(`ALTER TABLE memories ADD COLUMN owner TEXT NOT NULL DEFAULT ''`); err != nil { + return fmt.Errorf("migrate memory owners: %w", err) + } } - // The column lands empty and is filled from scope in one pass each, so no - // row is ever left with an owner the validator would refuse. + // Every statement below is guarded by owner = '', so this is the same pass + // whether it is owning a table that has just been widened or repairing the + // rows a still-running pre-owner build wrote into one that was widened + // months ago. steps := []struct { owner, scope string }{ @@ -182,7 +201,7 @@ func migrateMemoriesOwner(db *sql.DB) error { if _, err := tx.Exec(`UPDATE memories SET owner = ? WHERE owner = ''`, OwnerLegacyProject); err != nil { return fmt.Errorf("migrate memory owners: %w", err) } - if _, err := tx.Exec(`CREATE INDEX IF NOT EXISTS memories_owner_active ON memories (owner, updated_seq DESC) WHERE status = 'active'`); err != nil { + if _, err := tx.Exec(memoriesOwnerIndexDDL); err != nil { return fmt.Errorf("migrate memory owners: %w", err) } if err := tx.Commit(); err != nil { @@ -191,6 +210,12 @@ func migrateMemoriesOwner(db *sql.DB) error { return nil } +// memoriesOwnerIndexDDL is the one spelling of the owner-keyed retrieval index. +// It is a partial index because only active rows are ever retrieved, and it +// lives beside the column it indexes — see [migrateMemoriesOwner] for why it +// cannot be declared in [memoriesSchema]. +const memoriesOwnerIndexDDL = `CREATE INDEX IF NOT EXISTS memories_owner_active ON memories (owner, updated_seq DESC) WHERE status = 'active'` + // quarantinedMemory is one legacy project row that cannot prove its owner. type quarantinedMemory struct { id string diff --git a/internal/store/memory_upgrade_test.go b/internal/store/memory_upgrade_test.go new file mode 100644 index 0000000000..076febc2ad --- /dev/null +++ b/internal/store/memory_upgrade_test.go @@ -0,0 +1,624 @@ +package store + +import ( + "database/sql" + "encoding/json" + "path/filepath" + "reflect" + "testing" +) + +// ── opening a database written before owners existed ───────────────────────── +// +// THE RELEASED BUILD'S memories TABLE HAS NO owner COLUMN. Every database on +// disk today was written by it, so the first open of the owner-aware build is +// exactly this shape — and it is not the shape the migration helper's own tests +// cover, because those build their fixture through the CURRENT schema and so +// start with the column already in place. That gap is what let a release ship +// whose open died with `no such column: owner`. +// +// The regression below builds the legacy table with its own hand-written DDL, +// on purpose: it is the one description of the old table that cannot drift when +// today's schema changes. + +// legacyMemoriesDDL is the memories table and its search view exactly as the +// released v0.7.1 build created them — no `owner` column, no owner index, and a +// materialized (not external-content) memories_fts over memory_id/title/text/tags. +// Every statement is one the old build really ran. +const legacyMemoriesTableDDL = ` +CREATE TABLE memories ( + id TEXT PRIMARY KEY, + type TEXT NOT NULL, + scope TEXT NOT NULL, + title TEXT NOT NULL, + text TEXT NOT NULL, + tags TEXT NOT NULL DEFAULT '[]', + status TEXT NOT NULL DEFAULT 'active', + use_count INTEGER NOT NULL DEFAULT 0, + miss_count INTEGER NOT NULL DEFAULT 0, + created_seq INTEGER NOT NULL, + updated_seq INTEGER NOT NULL, + source_session TEXT NOT NULL DEFAULT '', + source_seq INTEGER NOT NULL DEFAULT 0 +); +CREATE INDEX memories_status_updated ON memories (status, updated_seq DESC); +CREATE INDEX memories_status_scope ON memories (status, scope, updated_seq DESC); +` + +// legacyMemoriesFTSDDL is the search view the same schema made: a materialized +// FTS5 table, not external content, over the id and the three searchable fields. +const legacyMemoriesFTSDDL = ` +CREATE VIRTUAL TABLE memories_fts USING fts5( + memory_id UNINDEXED, + title, + text, + tags +); +` + +// legacyMemoriesDDL is the two together, which is the ordinary released store. +const legacyMemoriesDDL = legacyMemoriesTableDDL + legacyMemoriesFTSDDL + +// legacyMemoryFixture is one row of the pre-owner table, named so the proof can +// say which fact it is looking at. +type legacyMemoryFixture struct { + id, memoryType, scope, title, text, tags, status string + useCount, missCount int + createdSeq, updatedSeq int64 + sourceSession string + sourceSeq int64 +} + +// legacyMemoryRows is the fixture every open-regression test starts from: one +// row of each scope, a superseded row (history that must survive), and a row +// whose scope this build does not know (written by a later or hand-edited +// store, which the migration also has to place). +func legacyMemoryRows() []legacyMemoryFixture { + return []legacyMemoryFixture{ + { + id: "mem_user", memoryType: MemoryPreference, scope: MemoryScopeUser, + title: "Keeps the receipts", text: "Keeps the receipts for the tax year.", + tags: `["finance"]`, status: MemoryActive, useCount: 3, missCount: 1, + createdSeq: 11, updatedSeq: 12, sourceSession: "session-old", sourceSeq: 7, + }, + { + id: "mem_env", memoryType: MemoryFact, scope: MemoryScopeEnv, + title: "Runs on the amber box", text: "The build box has no IPv6.", + tags: `["infra"]`, status: MemoryActive, useCount: 0, missCount: 0, + createdSeq: 21, updatedSeq: 22, sourceSession: "session-other", sourceSeq: 9, + }, + { + id: "mem_project", memoryType: MemoryDecision, scope: MemoryScopeProject, + title: "Deploys on Tuesdays", text: "Deploys to the amber cluster every Tuesday.", + tags: `["deploy"]`, status: MemoryActive, useCount: 5, missCount: 2, + createdSeq: 31, updatedSeq: 32, sourceSession: "session-project", sourceSeq: 4, + }, + { + id: "mem_superseded", memoryType: MemoryCorrection, scope: MemoryScopeUser, + title: "Used the old registry", text: "Used the old registry for releases.", + tags: `["release"]`, status: MemorySuperseded, useCount: 1, missCount: 0, + createdSeq: 41, updatedSeq: 42, sourceSession: "session-old", sourceSeq: 8, + }, + { + id: "mem_future_scope", memoryType: MemoryFact, scope: "team", + title: "A scope from a later build", text: "A row whose scope this build does not know.", + tags: `[]`, status: MemoryActive, useCount: 0, missCount: 0, + createdSeq: 51, updatedSeq: 52, sourceSession: "", sourceSeq: 0, + }, + } +} + +// writeLegacyDatabase lays down a database in the released shape. It is raw +// SQL on purpose: no store code runs, so nothing today can quietly repair the +// fixture into the shape the test is supposed to prove is upgradeable. +func writeLegacyDatabase(t *testing.T, path string) { + writeLegacyDatabaseWithFTS(t, path, true) +} + +// writeLegacyDatabaseWithFTS is the same fixture with the lexical index left out +// entirely, which is the shape of a store written on a build whose SQLite had no +// FTS5 (or whose view was hand-removed). The memory rows are identical; only the +// search tier differs. +func writeLegacyDatabaseWithFTS(t *testing.T, path string, withFTS bool) { + t.Helper() + raw, err := sql.Open("sqlite", path) + if err != nil { + t.Fatalf("create legacy database: %v", err) + } + defer raw.Close() + ddl := legacyMemoriesTableDDL + if withFTS { + ddl = legacyMemoriesDDL + } + if _, err := raw.Exec(ddl); err != nil { + t.Fatalf("create legacy memories table: %v", err) + } + + for _, row := range legacyMemoryRows() { + if _, err := raw.Exec(` + INSERT INTO memories + (id, type, scope, title, text, tags, status, use_count, miss_count, + created_seq, updated_seq, source_session, source_seq) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + row.id, row.memoryType, row.scope, row.title, row.text, row.tags, row.status, + row.useCount, row.missCount, row.createdSeq, row.updatedSeq, row.sourceSession, row.sourceSeq); err != nil { + t.Fatalf("insert legacy row %s: %v", row.id, err) + } + if !withFTS { + continue + } + // The old build kept the search view itself, and this is its own + // statement (v0.7.1's refreshMemoryFTS), tags punctuation stripped and + // only active rows present: the fixture must hold the index the way the + // released build left it, not the way this build would rebuild it. + if _, err := raw.Exec(` + INSERT INTO memories_fts (memory_id, title, text, tags) + SELECT id, title, text, replace(replace(replace(tags, '[', ''), ']', ''), '"', '') + FROM memories WHERE id = ? AND status = ?`, row.id, MemoryActive); err != nil { + t.Fatalf("index legacy row %s: %v", row.id, err) + } + } +} + +// addLegacyOwnerColumn simulates the step a store reached before the crash this +// fix is about: the column exists (with the empty default an ALTER gives it), +// the rows are owned, and the caller decides whether the interrupted upgrade +// got as far as the index. +func addLegacyOwnerColumn(t *testing.T, path string, withIndex bool) { + t.Helper() + raw, err := sql.Open("sqlite", path) + if err != nil { + t.Fatal(err) + } + defer raw.Close() + if _, err := raw.Exec(`ALTER TABLE memories ADD COLUMN owner TEXT NOT NULL DEFAULT ''`); err != nil { + t.Fatalf("add the owner column: %v", err) + } + if _, err := raw.Exec(`UPDATE memories SET owner = 'user' WHERE scope = 'user'`); err != nil { + t.Fatal(err) + } + if withIndex { + if _, err := raw.Exec(`CREATE INDEX memories_owner_active ON memories (owner, updated_seq DESC) WHERE status = 'active'`); err != nil { + t.Fatal(err) + } + } +} + +// memoriesTableColumns is what the open left the table looking like. +func memoriesTableColumns(t *testing.T, graph *Store) map[string]bool { + t.Helper() + columns := map[string]bool{} + rows, err := graph.db.Query(`PRAGMA table_info(memories)`) + if err != nil { + t.Fatalf("read memories columns: %v", err) + } + defer rows.Close() + for rows.Next() { + var cid, notNull, primaryKey int + var name, columnType string + var defaultValue any + if err := rows.Scan(&cid, &name, &columnType, ¬Null, &defaultValue, &primaryKey); err != nil { + t.Fatalf("scan memories column: %v", err) + } + columns[name] = true + } + if err := rows.Err(); err != nil { + t.Fatalf("read memories columns: %v", err) + } + return columns +} + +// indexOnMemories answers whether one named index exists over the memories +// table — the assertion that the ordering bug moved out of the schema and into +// a migration cannot be made any other way. +func indexOnMemories(t *testing.T, graph *Store, name string) bool { + t.Helper() + var got string + err := graph.db.QueryRow(`SELECT name FROM sqlite_master WHERE type = 'index' AND tbl_name = 'memories' AND name = ?`, name).Scan(&got) + if err == sql.ErrNoRows { + return false + } + if err != nil { + t.Fatalf("read sqlite_master for %s: %v", name, err) + } + return got == name +} + +// THE UPGRADE THE RELEASE BROKE. Opening a database from before owners existed +// must add the column, own every row the way its scope proves, and leave the +// notes, their provenance, status and scope exactly as they were. +func TestOpeningADatabaseFromBeforeOwnersKeepsEveryNote(t *testing.T) { + path := filepath.Join(t.TempDir(), "legacy.db") + writeLegacyDatabase(t, path) + + graph, err := Open(path) + if err != nil { + t.Fatalf("opening a database from before owners existed: %v", err) + } + defer graph.Close() + + if !indexOnMemories(t, graph, "memories_owner_active") { + t.Fatal("the open did not leave the owner index on memories") + } + if columns := memoriesTableColumns(t, graph); !columns["owner"] { + t.Fatalf("the open did not add the owner column: %v", columns) + } + + // Every row is still there and still says what it said, whatever its status. + for _, want := range legacyMemoryRows() { + got, ok, err := graph.MemoryRecord(want.id) + if err != nil || !ok { + t.Fatalf("read %s after the upgrade: (%v, %v, %v)", want.id, got, ok, err) + } + if got.Title != want.title || got.Text != want.text { + t.Errorf("%s lost its note: (%q, %q), want (%q, %q)", want.id, got.Title, got.Text, want.title, want.text) + } + if got.Type != want.memoryType { + t.Errorf("%s type = %q, want %q", want.id, got.Type, want.memoryType) + } + if got.Scope != want.scope { + t.Errorf("%s scope = %q, want %q", want.id, got.Scope, want.scope) + } + if got.Status != want.status { + t.Errorf("%s status = %q, want %q", want.id, got.Status, want.status) + } + // A quarantined row carries the marker the migration APPENDS; every + // other row's tags are byte-for-byte what the old build wrote. + wantTags := legacyTags(t, want.tags) + if want.scope == MemoryScopeProject { + wantTags = append(wantTags, OwnerLegacyTag) + } + if !reflect.DeepEqual(got.Tags, wantTags) { + t.Errorf("%s tags = %v, want %v", want.id, got.Tags, wantTags) + } + if got.UseCount != want.useCount || got.MissCount != want.missCount { + t.Errorf("%s counters = (%d, %d), want (%d, %d)", want.id, got.UseCount, got.MissCount, want.useCount, want.missCount) + } + if got.CreatedSeq != want.createdSeq || got.UpdatedSeq != want.updatedSeq { + t.Errorf("%s seqs = (%d, %d), want (%d, %d)", want.id, got.CreatedSeq, got.UpdatedSeq, want.createdSeq, want.updatedSeq) + } + if got.SourceSession != want.sourceSession || got.SourceSeq != want.sourceSeq { + t.Errorf("%s provenance = (%q, %d), want (%q, %d)", want.id, got.SourceSession, got.SourceSeq, want.sourceSession, want.sourceSeq) + } + } + + // And the owners are the ones the scopes prove — never a guess. + for id, want := range map[string]string{ + "mem_user": OwnerUser, + "mem_env": OwnerMachine, + "mem_project": OwnerLegacyProject, + "mem_superseded": OwnerUser, + "mem_future_scope": OwnerLegacyProject, + } { + got, ok, err := graph.MemoryRecord(id) + if err != nil || !ok { + t.Fatalf("read %s for its owner: (%v, %v, %v)", id, got, ok, err) + } + if got.Owner != want { + t.Errorf("%s owner = %q, want %q", id, got.Owner, want) + } + } + // The unprovable project row carries the quarantine marker on purpose: a + // memory's source is never cleaned off it, and a person can search for the + // rows that came in blind. + project, _, err := graph.MemoryRecord("mem_project") + if err != nil { + t.Fatal(err) + } + if !containsStubTag(project.Tags, OwnerLegacyTag) { + t.Errorf("mem_project tags = %v, want the legacy marker", project.Tags) + } + if !containsStubTag(project.Tags, "deploy") { + t.Errorf("mem_project tags = %v, want the original tag kept", project.Tags) + } + + // The active rows are readable through the ordinary public read, and the + // quarantined note is not: the owner filter is the permission model, and an + // upgrade must not hand a model a row whose owner nobody could prove. + visible, err := graph.ListMemories(nil, 0) + if err != nil { + t.Fatalf("list memories after the upgrade: %v", err) + } + if len(visible) != 4 { + t.Fatalf("active memories = %v, want the four active rows", memoryIDs(visible)) + } + own, err := graph.ListMemories([]string{OwnerUser}, 0) + if err != nil { + t.Fatalf("list the person's memories: %v", err) + } + if len(own) != 1 || own[0].ID != "mem_user" { + t.Fatalf("the person's shelf = %v, want mem_user alone", memoryIDs(own)) + } + quarantined, err := graph.ListMemories([]string{OwnerLegacyProject}, 0) + if err != nil { + t.Fatalf("list the quarantine: %v", err) + } + if len(quarantined) != 2 { + t.Fatalf("the quarantine = %v, want the unprovable project row and the unknown scope", memoryIDs(quarantined)) + } + // The lexical tier answered from the rows the old build indexed, under the + // new owner filter: the note a person would search for is the one they get. + hits, err := graph.SearchMemories([]string{OwnerUser}, "receipts", 5) + if err != nil { + t.Fatalf("search after the upgrade: %v", err) + } + if len(hits) != 1 || hits[0].ID != "mem_user" { + t.Fatalf("search for 'receipts' = %v, want the preserved note", memoryIDs(hits)) + } + if hits[0].Text != "Keeps the receipts for the tax year." { + t.Fatalf("the search hit lost its text: %q", hits[0].Text) + } + // The words the old build indexed are still the words that match, and the + // owner filter decides which of them a search may return: "amber" is in this + // machine's row and in the quarantined project row, and neither search sees + // the other's. + for _, want := range []struct { + owners []string + query string + id string + }{ + {[]string{OwnerUser}, "finance", "mem_user"}, // the tag the old build wrote, punctuation and all + {[]string{OwnerMachine}, "amber", "mem_env"}, // this machine's own truth + {[]string{OwnerLegacyProject}, "amber", "mem_project"}, // the quarantine, to its own owner only + {[]string{OwnerUser}, "amber", ""}, // the person has no such line + } { + got, err := graph.SearchMemories(want.owners, want.query, 5) + if err != nil { + t.Fatalf("search %q under %v: %v", want.query, want.owners, err) + } + if want.id == "" { + if len(got) != 0 { + t.Fatalf("search %q under %v = %v, want nothing", want.query, want.owners, memoryIDs(got)) + } + continue + } + if len(got) != 1 || got[0].ID != want.id { + t.Fatalf("search %q under %v = %v, want %s", want.query, want.owners, memoryIDs(got), want.id) + } + } + // A quarantined row is not searchable under any owner a model can name. + blind, err := graph.SearchMemories([]string{OwnerUser, OwnerMachine, OwnerProject("alpha")}, "Tuesday", 5) + if err != nil { + t.Fatalf("search across real owners: %v", err) + } + if len(blind) != 0 { + t.Fatalf("the quarantine answered a model's search: %v", memoryIDs(blind)) + } +} + +func TestOpeningADatabaseFromBeforeOwnersIsIdempotent(t *testing.T) { + path := filepath.Join(t.TempDir(), "legacy.db") + writeLegacyDatabase(t, path) + + graph, err := Open(path) + if err != nil { + t.Fatalf("first open: %v", err) + } + first := map[string]Memory{} + for _, row := range legacyMemoryRows() { + got, ok, err := graph.MemoryRecord(row.id) + if err != nil || !ok { + t.Fatalf("first read %s: (%v, %v, %v)", row.id, got, ok, err) + } + first[row.id] = got + } + if err := graph.Close(); err != nil { + t.Fatal(err) + } + + for attempt := 0; attempt < 2; attempt++ { + graph, err := Open(path) + if err != nil { + t.Fatalf("open %d after the upgrade: %v", attempt+2, err) + } + for _, row := range legacyMemoryRows() { + got, ok, err := graph.MemoryRecord(row.id) + if err != nil || !ok { + t.Fatalf("read %s on open %d: (%v, %v, %v)", row.id, attempt+2, got, ok, err) + } + if !reflect.DeepEqual(got, first[row.id]) { + t.Fatalf("%s changed on open %d:\nfirst %+v\nlater %+v", row.id, attempt+2, first[row.id], got) + } + } + if err := graph.Close(); err != nil { + t.Fatal(err) + } + } +} + +// A STORE THAT ALREADY HAS THE COLUMN STILL NEEDS THE INDEX. Removing the index +// from [memoriesSchema] is only half the repair: a database that carries the +// owner column but not the index — the interrupted upgrade, the hand-edited +// store — would otherwise never get it, because [migrateMemoriesOwner] returns +// early once the column is there. +func TestOpeningAnAlreadyOwnedDatabaseWithoutTheIndexAddsIt(t *testing.T) { + path := filepath.Join(t.TempDir(), "already-owned.db") + writeLegacyDatabase(t, path) + + // Simulate the interrupted upgrade: add the column and own the rows the way + // the migration would, and stop short of the index — exactly the state a + // store reaches when a build dies between the ALTER and the CREATE INDEX. + addLegacyOwnerColumn(t, path, false) + + graph, err := Open(path) + if err != nil { + t.Fatalf("opening an already-owned database without the index: %v", err) + } + defer graph.Close() + + if !indexOnMemories(t, graph, "memories_owner_active") { + t.Fatal("the open did not add the owner index to a database that already had the column") + } + got, ok, err := graph.MemoryRecord("mem_superseded") + if err != nil || !ok || got.Owner != OwnerUser { + t.Fatalf("the already-owned row = (%+v, %v, %v), want its owner untouched", got, ok, err) + } +} + +// AN OLDER BUILD IS STILL RUNNING WHILE THE NEW ONE IS INSTALLED, so the mixed +// shape is not hypothetical: the old build opens the upgraded store, never sets +// the owner column it can see, and writes its rows with the empty default. +// Those rows carry a real scope and must be owned by it at the next open, +// without touching a single owner that is already set. +func TestOpeningADatabaseAPreOwnerBuildWroteIntoOwnsItsEmptyRows(t *testing.T) { + path := filepath.Join(t.TempDir(), "mixed.db") + writeLegacyDatabase(t, path) + addLegacyOwnerColumn(t, path, true) + + raw, err := sql.Open("sqlite", path) + if err != nil { + t.Fatal(err) + } + // One row already carries an owner a person re-homed; the repair must leave + // it exactly where it is. + if _, err := raw.Exec(`UPDATE memories SET owner = 'project:alpha' WHERE id = 'mem_user'`); err != nil { + t.Fatal(err) + } + // And the rows the older build wrote: no owner in the INSERT at all, which + // is what landing the column's empty default means. + for id, scope := range map[string]string{ + "mem_oldbuild_user": MemoryScopeUser, + "mem_oldbuild_env": MemoryScopeEnv, + "mem_oldbuild_project": MemoryScopeProject, + } { + if _, err := raw.Exec(` + INSERT INTO memories (id, type, scope, title, text, tags, status, created_seq, updated_seq) + VALUES (?, 'fact', ?, 'Written by the old build', 'A row the older build wrote.', '[]', 'active', 61, 62)`, + id, scope); err != nil { + t.Fatalf("insert row %s the old build would have written: %v", id, err) + } + } + if err := raw.Close(); err != nil { + t.Fatal(err) + } + + graph, err := Open(path) + if err != nil { + t.Fatalf("opening a store an older build wrote into: %v", err) + } + defer graph.Close() + + for id, want := range map[string]string{ + "mem_oldbuild_user": OwnerUser, + "mem_oldbuild_env": OwnerMachine, + "mem_oldbuild_project": OwnerLegacyProject, + } { + got, ok, err := graph.MemoryRecord(id) + if err != nil || !ok { + t.Fatalf("read %s: (%v, %v, %v)", id, got, ok, err) + } + if got.Owner != want { + t.Errorf("%s owner = %q, want %q", id, got.Owner, want) + } + } + // The empty project row is quarantined exactly as the first migration would + // have quarantined it, marker and all. + quarantined, _, err := graph.MemoryRecord("mem_oldbuild_project") + if err != nil { + t.Fatal(err) + } + if !containsStubTag(quarantined.Tags, OwnerLegacyTag) { + t.Errorf("the old build's project row tags = %v, want the legacy marker", quarantined.Tags) + } + // THE NONEMPTY OWNER IS UNTOUCHED. Re-homing a row is the person's act, and + // a pass that "repaired" it back to `user` would silently widen it. + kept, _, err := graph.MemoryRecord("mem_user") + if err != nil { + t.Fatal(err) + } + if kept.Owner != "project:alpha" { + t.Fatalf("an already-owned row moved to %q, want project:alpha", kept.Owner) + } + // The older build's rows are readable under the owner the scope proves, and + // the quarantine is still invisible to a retrieval. + visible, err := graph.ListMemories([]string{OwnerUser}, 0) + if err != nil { + t.Fatal(err) + } + if len(visible) != 1 || visible[0].ID != "mem_oldbuild_user" { + t.Fatalf("the person's shelf = %v, want the old build's user row", memoryIDs(visible)) + } + before := map[string]Memory{} + for _, id := range []string{"mem_user", "mem_oldbuild_user", "mem_oldbuild_env", "mem_oldbuild_project"} { + got, _, err := graph.MemoryRecord(id) + if err != nil { + t.Fatal(err) + } + before[id] = got + } + if err := graph.Close(); err != nil { + t.Fatal(err) + } + graph, err = Open(path) + if err != nil { + t.Fatalf("reopen after the mixed-version repair: %v", err) + } + defer graph.Close() + for id, want := range before { + got, _, err := graph.MemoryRecord(id) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("%s changed on the second open:\nfirst %+v\nsecond %+v", id, want, got) + } + } +} + +// A STORE WITH NO LEXICAL INDEX IS STILL THE PERSON'S MEMORY, and this is +// PRE-EXISTING behavior, deliberately unchanged by the owner repair: the open +// builds the empty search view when FTS5 is available but does not re-index rows +// the file already held, so search answers nothing while every note stays +// readable and owner-scoped. A Rebuild replays the journal and fills the view. +// An ordinary upgraded store carried its released index with it, so the repair +// has no reason to rebuild one, and nothing here broadens it to try. +func TestOpeningADatabaseWithoutItsLexicalIndexKeepsEveryNote(t *testing.T) { + path := filepath.Join(t.TempDir(), "no-fts.db") + writeLegacyDatabaseWithFTS(t, path, false) + + graph, err := Open(path) + if err != nil { + t.Fatalf("opening a database with no lexical index: %v", err) + } + defer graph.Close() + + if !indexOnMemories(t, graph, "memories_owner_active") { + t.Fatal("the open did not leave the owner index on memories") + } + kept, ok, err := graph.MemoryRecord("mem_user") + if err != nil || !ok { + t.Fatalf("read the preserved note: (%v, %v, %v)", kept, ok, err) + } + if kept.Owner != OwnerUser || kept.Text != "Keeps the receipts for the tax year." { + t.Fatalf("the note = %+v, want its words and the owner its scope proves", kept) + } + if hits, err := graph.SearchMemories([]string{OwnerUser}, "receipts", 5); err != nil || len(hits) != 0 { + t.Fatalf("search without a lexical index = (%v, %v), want the quiet tier", memoryIDs(hits), err) + } +} + +// A BRAND NEW STORE STILL GETS THE INDEX. The index moved out of the schema +// because the schema runs before the column is guaranteed; it must not move out +// of reach of the first launch, which is what a migration that only ran on +// legacy shapes would do. +func TestANewStoreCarriesTheOwnerIndex(t *testing.T) { + path := filepath.Join(t.TempDir(), "fresh.db") + graph, err := Open(path) + if err != nil { + t.Fatalf("open a new store: %v", err) + } + defer graph.Close() + if !indexOnMemories(t, graph, "memories_owner_active") { + t.Fatal("a brand new store has no owner index") + } +} + +// legacyTags reads the fixture's JSON tag list the way the store reads a row's. +func legacyTags(t *testing.T, raw string) []string { + t.Helper() + var tags []string + if err := json.Unmarshal([]byte(raw), &tags); err != nil { + t.Fatalf("fixture tags %q are not JSON: %v", raw, err) + } + return tags +}