From aa870f10407f30fe715e0090e274cdb43b8d9fa9 Mon Sep 17 00:00:00 2001 From: xcaliber Date: Thu, 12 Mar 2026 22:49:35 +0000 Subject: [PATCH] Changeset 0.28.0.8 (#180) --- docs/ICD/memory.md | 73 ++- .../database/migrations/sqlite/008_memory.sql | 1 + server/handlers/integration_test.go | 15 + server/handlers/memory.go | 67 ++- server/handlers/memory_test.go | 435 ++++++++++++++++++ server/memory/extractor.go | 10 +- server/memory/scanner.go | 54 ++- server/models/models_memory_persona.go | 18 - server/store/postgres/memory.go | 57 +-- server/store/postgres/memory_hybrid.go | 4 - server/store/sqlite/memory.go | 32 ++ server/store/store_memory.go | 8 + server/store/store_memory_hybrid.go | 15 - src/js/memory-ui.js | 6 +- 14 files changed, 679 insertions(+), 116 deletions(-) create mode 100644 server/handlers/memory_test.go delete mode 100644 server/models/models_memory_persona.go delete mode 100644 server/store/store_memory_hybrid.go diff --git a/docs/ICD/memory.md b/docs/ICD/memory.md index db69961..21e209b 100644 --- a/docs/ICD/memory.md +++ b/docs/ICD/memory.md @@ -4,45 +4,92 @@ identifies facts, preferences, and context about the user and stores them for injection into future conversations. +**Auth:** All user endpoints require authenticated session. +Admin endpoints require platform admin role. + +--- + ### User Memory ``` GET /memories → { "data": [memory objects] } -GET /memories/count → { "count": 42 } -PUT /memories/:id ← { "content": "updated text" } +GET /memories/count → { "active": 5, "pending": 2 } +PUT /memories/:id ← { "key": "...", "value": "...", "confidence": 0.9 } DELETE /memories/:id -POST /memories/:id/approve → approve a pending memory -POST /memories/:id/reject → reject a pending memory +POST /memories/:id/approve → approve a pending_review memory +POST /memories/:id/reject → reject (archive) a pending_review memory ``` +Query params for `GET /memories`: + +| Param | Default | Description | +|-------|---------|-------------| +| `status` | `active` | Filter by status: `active`, `pending_review`, `archived` | +| `query` | — | Keyword filter on key+value (PG: full-text, SQLite: LIKE) | + Memory object: ```json { "id": "uuid", - "user_id": "uuid", - "persona_id": "uuid|null", - "scope": "user|persona", - "content": "User prefers Go for backend work", - "source_channel_id": "uuid", - "status": "pending|approved|rejected", + "scope": "user|persona|persona_user", + "owner_id": "uuid", + "user_id": "uuid|null", + "key": "preferred language", + "value": "Go with Gin framework for backend services", + "source_channel_id": "uuid|null", "confidence": 0.85, + "status": "active|pending_review|archived", "created_at": "...", "updated_at": "..." } ``` -`scope`: `user` memories apply across all conversations. `persona` -memories are specific to interactions with that Persona. +**Scopes:** + +| Scope | `owner_id` | `user_id` | Description | +|-------|-----------|-----------|-------------| +| `user` | user's ID | null | Personal facts, persist across all conversations | +| `persona` | persona's ID | null | Shared across all users of a Persona | +| `persona_user` | persona's ID | user's ID | Per-user facts within a Persona context | + +**Ownership:** `PUT` and `DELETE` enforce ownership — a user can only +modify their own user-scope memories (`owner_id == user_id`). ### Admin Memory Review ``` -GET /admin/memories/pending → { "data": [memory objects with user info] } +GET /admin/memories/pending → { "data": [memory objects] } POST /admin/memories/bulk-approve ← { "ids": ["uuid1", "uuid2"] } + → { "approved": 2 } ``` Platform admin can review and bulk-approve pending memories across all users. +### AI Tools (non-REST) + +Two tools are registered for AI use during completions: + +| Tool | Description | +|------|-------------| +| `memory_save` | Save a fact with `key`, `value`, `confidence`. Saves as `active` (bypasses review). | +| `memory_recall` | Search memories by query. Uses hybrid semantic+keyword recall when embedder is configured. | + +### Background Extraction + +A background scanner periodically finds conversations with enough +new activity and calls the utility model to extract memorable facts. +Extracted memories are saved as `pending_review`. + +Controlled by `memory_extraction_enabled` global config key and +per-persona `memory_enabled` flag. + +### Memory Injection + +Active memories are injected into the system prompt at completion +time via `BuildMemoryHint`. Uses hybrid semantic recall when an +embedder is available, falling back to keyword/confidence ranking. +Budget: ~6000 characters. + --- diff --git a/server/database/migrations/sqlite/008_memory.sql b/server/database/migrations/sqlite/008_memory.sql index e62c8cb..9fd36c6 100644 --- a/server/database/migrations/sqlite/008_memory.sql +++ b/server/database/migrations/sqlite/008_memory.sql @@ -12,6 +12,7 @@ CREATE TABLE IF NOT EXISTS memories ( confidence REAL NOT NULL DEFAULT 1.0, status TEXT NOT NULL DEFAULT 'active' CHECK (status IN ('active', 'pending_review', 'archived')), + embedding TEXT, last_compacted_at TEXT, decay_rate REAL NOT NULL DEFAULT 0.0, created_at TEXT DEFAULT (datetime('now')), diff --git a/server/handlers/integration_test.go b/server/handlers/integration_test.go index 3263847..64ee889 100644 --- a/server/handlers/integration_test.go +++ b/server/handlers/integration_test.go @@ -330,6 +330,16 @@ func setupHarness(t *testing.T) *testHarness { // Tasks (v0.28.0) taskH := NewTaskHandler(stores) protected.GET("/tasks", taskH.ListMine) + + // Memory (v0.18.0, audit v0.28.0) + memH := NewMemoryHandler(stores) + protected.GET("/memories", memH.ListMyMemories) + protected.PUT("/memories/:id", memH.UpdateMemory) + protected.DELETE("/memories/:id", memH.DeleteMemory) + protected.POST("/memories/:id/approve", memH.ApproveMemory) + protected.POST("/memories/:id/reject", memH.RejectMemory) + protected.GET("/memories/count", memH.MemoryCount) + protected.POST("/tasks", middleware.RequirePermission(authpkg.PermTaskCreate, stores), taskH.Create) protected.GET("/tasks/:id", taskH.Get) protected.PUT("/tasks/:id", middleware.RequirePermission(authpkg.PermTaskCreate, stores), taskH.Update) @@ -423,6 +433,11 @@ func setupHarness(t *testing.T) *testHarness { admin.POST("/tasks/:id/kill", taskAdm.KillRun) admin.DELETE("/tasks/:id", taskAdm.Delete) + // Admin memory review + adminMemH := NewMemoryHandler(stores) + admin.GET("/memories/pending", adminMemH.ListPendingReview) + admin.POST("/memories/bulk-approve", adminMemH.BulkApprove) + return &testHarness{router: r, t: t} } diff --git a/server/handlers/memory.go b/server/handlers/memory.go index 49ec4f4..2286e30 100644 --- a/server/handlers/memory.go +++ b/server/handlers/memory.go @@ -43,7 +43,10 @@ func (h *MemoryHandler) ListMyMemories(c *gin.Context) { return } - c.JSON(http.StatusOK, gin.H{"memories": memories}) + if memories == nil { + memories = []models.Memory{} + } + c.JSON(http.StatusOK, gin.H{"data": memories}) } // UpdateMemory updates a memory's key/value/confidence. @@ -84,7 +87,7 @@ func (h *MemoryHandler) UpdateMemory(c *gin.Context) { existing.Confidence = *body.Confidence } - if err := h.stores.Memories.Upsert(c.Request.Context(), existing); err != nil { + if err := h.stores.Memories.Update(c.Request.Context(), existing); err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -128,6 +131,11 @@ func (h *MemoryHandler) ApproveMemory(c *gin.Context) { return } + if existing.Status != models.MemoryStatusPendingReview { + c.JSON(http.StatusConflict, gin.H{"error": "only pending_review memories can be approved"}) + return + } + existing.Status = models.MemoryStatusActive if err := h.stores.Memories.Upsert(c.Request.Context(), existing); err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) @@ -140,8 +148,26 @@ func (h *MemoryHandler) ApproveMemory(c *gin.Context) { // RejectMemory archives a pending_review memory. // POST /api/v1/memories/:id/reject func (h *MemoryHandler) RejectMemory(c *gin.Context) { + userID := c.GetString("user_id") memoryID := c.Param("id") + existing, err := h.stores.Memories.GetByID(c.Request.Context(), memoryID) + if err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "memory not found"}) + return + } + + // Ownership check: user can only reject their own user-scope memories + if existing.Scope == models.MemoryScopeUser && existing.OwnerID != userID { + c.JSON(http.StatusForbidden, gin.H{"error": "not your memory"}) + return + } + + if existing.Status != models.MemoryStatusPendingReview { + c.JSON(http.StatusConflict, gin.H{"error": "only pending_review memories can be rejected"}) + return + } + if err := h.stores.Memories.Archive(c.Request.Context(), memoryID); err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return @@ -156,15 +182,23 @@ func (h *MemoryHandler) MemoryCount(c *gin.Context) { userID := c.GetString("user_id") ctx := c.Request.Context() - active, _ := h.stores.Memories.CountByOwner(ctx, models.MemoryScopeUser, userID) + active, err := h.stores.Memories.CountByOwner(ctx, models.MemoryScopeUser, userID) + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } // Count pending by listing - pending, _ := h.stores.Memories.List(ctx, models.MemoryFilter{ + pending, err := h.stores.Memories.List(ctx, models.MemoryFilter{ Scope: models.MemoryScopeUser, OwnerID: userID, Status: models.MemoryStatusPendingReview, Limit: 1000, }) + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } c.JSON(http.StatusOK, gin.H{ "active": active, @@ -177,22 +211,18 @@ func (h *MemoryHandler) MemoryCount(c *gin.Context) { // ListPendingReview returns all pending_review memories (admin only). // GET /api/v1/admin/memories/pending func (h *MemoryHandler) ListPendingReview(c *gin.Context) { - // This should be called from the admin route group which already checks admin role. - // We query across all users by listing each scope. ctx := c.Request.Context() - // Query pending memories directly via raw SQL for efficiency - var memories []models.Memory + memories, err := h.stores.Memories.ListByStatus(ctx, models.MemoryStatusPendingReview, 200) + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } - userMemories, _ := h.stores.Memories.List(ctx, models.MemoryFilter{ - Scope: models.MemoryScopeUser, - OwnerID: "%", // This won't work with List — we need a custom query - Status: models.MemoryStatusPendingReview, - Limit: 100, - }) - memories = append(memories, userMemories...) - - c.JSON(http.StatusOK, gin.H{"memories": memories}) + if memories == nil { + memories = []models.Memory{} + } + c.JSON(http.StatusOK, gin.H{"data": memories}) } // BulkApprove approves multiple memories at once. @@ -213,6 +243,9 @@ func (h *MemoryHandler) BulkApprove(c *gin.Context) { if err != nil { continue } + if existing.Status != models.MemoryStatusPendingReview { + continue + } existing.Status = models.MemoryStatusActive if err := h.stores.Memories.Upsert(ctx, existing); err == nil { approved++ diff --git a/server/handlers/memory_test.go b/server/handlers/memory_test.go new file mode 100644 index 0000000..f70470b --- /dev/null +++ b/server/handlers/memory_test.go @@ -0,0 +1,435 @@ +package handlers + +import ( + "net/http" + "testing" + + "git.gobha.me/xcaliber/chat-switchboard/database" + "git.gobha.me/xcaliber/chat-switchboard/models" +) + +// seedMemory inserts a memory directly into the DB for testing. +func seedMemory(t *testing.T, ownerID, key, value, status string) string { + t.Helper() + id := seedID() + q := dialectSQL(` + INSERT INTO memories (id, scope, owner_id, key, value, confidence, status) + VALUES ($1, $2, $3, $4, $5, $6, $7) + `) + _, err := database.TestDB.Exec(q, id, models.MemoryScopeUser, ownerID, key, value, 0.9, status) + if err != nil { + t.Fatalf("seedMemory: %v", err) + } + return id +} + +// ═══════════════════════════════════════════════ +// List +// ═══════════════════════════════════════════════ + +func TestMemory_ListEmpty(t *testing.T) { + h := setupHarness(t) + _, token := h.createAdminUser("memuser1", "memuser1@test.com") + + w := h.request("GET", "/api/v1/memories", token, nil) + if w.Code != http.StatusOK { + t.Fatalf("list: want 200, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]interface{} + decode(w, &resp) + + // Must use "data" key (envelope convention) + data, ok := resp["data"].([]interface{}) + if !ok { + t.Fatalf("response must have 'data' array key, got: %s", w.Body.String()) + } + if len(data) != 0 { + t.Fatalf("expected 0 memories, got %d", len(data)) + } +} + +func TestMemory_ListWithData(t *testing.T) { + h := setupHarness(t) + userID, token := h.createAdminUser("memuser2", "memuser2@test.com") + + seedMemory(t, userID, "lang", "Go", models.MemoryStatusActive) + seedMemory(t, userID, "editor", "Vim", models.MemoryStatusActive) + seedMemory(t, userID, "pending-fact", "something", models.MemoryStatusPendingReview) + + // Default status=active + w := h.request("GET", "/api/v1/memories", token, nil) + if w.Code != http.StatusOK { + t.Fatalf("list: want 200, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]interface{} + decode(w, &resp) + data := resp["data"].([]interface{}) + if len(data) != 2 { + t.Fatalf("expected 2 active memories, got %d", len(data)) + } + + // Explicit status=pending_review + w = h.request("GET", "/api/v1/memories?status=pending_review", token, nil) + if w.Code != http.StatusOK { + t.Fatalf("list pending: want 200, got %d", w.Code) + } + decode(w, &resp) + data = resp["data"].([]interface{}) + if len(data) != 1 { + t.Fatalf("expected 1 pending memory, got %d", len(data)) + } +} + +func TestMemory_ListIsolation(t *testing.T) { + h := setupHarness(t) + userA, tokenA := h.createAdminUser("memA", "memA@test.com") + userB, _ := h.createAdminUser("memB", "memB@test.com") + + seedMemory(t, userA, "a-fact", "user A", models.MemoryStatusActive) + seedMemory(t, userB, "b-fact", "user B", models.MemoryStatusActive) + + w := h.request("GET", "/api/v1/memories", tokenA, nil) + var resp map[string]interface{} + decode(w, &resp) + data := resp["data"].([]interface{}) + if len(data) != 1 { + t.Fatalf("user A should only see 1 memory, got %d", len(data)) + } + mem := data[0].(map[string]interface{}) + if mem["key"].(string) != "a-fact" { + t.Fatalf("wrong memory: got key %q", mem["key"]) + } +} + +// ═══════════════════════════════════════════════ +// Update +// ═══════════════════════════════════════════════ + +func TestMemory_Update(t *testing.T) { + h := setupHarness(t) + userID, token := h.createAdminUser("memupdater", "memupdater@test.com") + + memID := seedMemory(t, userID, "old-key", "old-value", models.MemoryStatusActive) + + w := h.request("PUT", "/api/v1/memories/"+memID, token, map[string]interface{}{ + "key": "new-key", + "value": "new-value", + }) + if w.Code != http.StatusOK { + t.Fatalf("update: want 200, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]interface{} + decode(w, &resp) + mem := resp["memory"].(map[string]interface{}) + if mem["key"].(string) != "new-key" { + t.Fatalf("key not updated: got %q", mem["key"]) + } + if mem["value"].(string) != "new-value" { + t.Fatalf("value not updated: got %q", mem["value"]) + } +} + +func TestMemory_UpdateOwnershipDenied(t *testing.T) { + h := setupHarness(t) + ownerID, _ := h.createAdminUser("memowner", "memowner@test.com") + _, otherToken := h.createAdminUser("memother", "memother@test.com") + + memID := seedMemory(t, ownerID, "secret", "mine", models.MemoryStatusActive) + + w := h.request("PUT", "/api/v1/memories/"+memID, otherToken, map[string]interface{}{ + "value": "hacked", + }) + if w.Code != http.StatusForbidden { + t.Fatalf("update other's memory: want 403, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestMemory_UpdateNotFound(t *testing.T) { + h := setupHarness(t) + _, token := h.createAdminUser("memghost", "memghost@test.com") + + w := h.request("PUT", "/api/v1/memories/"+seedID(), token, map[string]interface{}{ + "value": "x", + }) + if w.Code != http.StatusNotFound { + t.Fatalf("update missing: want 404, got %d", w.Code) + } +} + +// ═══════════════════════════════════════════════ +// Delete +// ═══════════════════════════════════════════════ + +func TestMemory_Delete(t *testing.T) { + h := setupHarness(t) + userID, token := h.createAdminUser("memdeleter", "memdeleter@test.com") + + memID := seedMemory(t, userID, "to-delete", "gone", models.MemoryStatusActive) + + w := h.request("DELETE", "/api/v1/memories/"+memID, token, nil) + if w.Code != http.StatusOK { + t.Fatalf("delete: want 200, got %d: %s", w.Code, w.Body.String()) + } + + // Verify gone — list should be empty + w = h.request("GET", "/api/v1/memories", token, nil) + var resp map[string]interface{} + decode(w, &resp) + data := resp["data"].([]interface{}) + if len(data) != 0 { + t.Fatalf("memory should be deleted, got %d", len(data)) + } +} + +func TestMemory_DeleteOwnershipDenied(t *testing.T) { + h := setupHarness(t) + ownerID, _ := h.createAdminUser("delowner", "delowner@test.com") + _, otherToken := h.createAdminUser("delother", "delother@test.com") + + memID := seedMemory(t, ownerID, "no-touch", "x", models.MemoryStatusActive) + + w := h.request("DELETE", "/api/v1/memories/"+memID, otherToken, nil) + if w.Code != http.StatusForbidden { + t.Fatalf("delete other's memory: want 403, got %d", w.Code) + } +} + +// ═══════════════════════════════════════════════ +// Approve +// ═══════════════════════════════════════════════ + +func TestMemory_Approve(t *testing.T) { + h := setupHarness(t) + userID, token := h.createAdminUser("memapprover", "memapprover@test.com") + + memID := seedMemory(t, userID, "pending", "val", models.MemoryStatusPendingReview) + + w := h.request("POST", "/api/v1/memories/"+memID+"/approve", token, nil) + if w.Code != http.StatusOK { + t.Fatalf("approve: want 200, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]interface{} + decode(w, &resp) + mem := resp["memory"].(map[string]interface{}) + if mem["status"].(string) != models.MemoryStatusActive { + t.Fatalf("status should be active, got %q", mem["status"]) + } +} + +func TestMemory_ApproveStatusGuard(t *testing.T) { + h := setupHarness(t) + userID, token := h.createAdminUser("memguard1", "memguard1@test.com") + + // Try to approve an already-active memory + memID := seedMemory(t, userID, "already-active", "val", models.MemoryStatusActive) + + w := h.request("POST", "/api/v1/memories/"+memID+"/approve", token, nil) + if w.Code != http.StatusConflict { + t.Fatalf("approve active memory: want 409, got %d: %s", w.Code, w.Body.String()) + } + + // Try to approve an archived memory + archivedID := seedMemory(t, userID, "archived", "val", models.MemoryStatusArchived) + w = h.request("POST", "/api/v1/memories/"+archivedID+"/approve", token, nil) + if w.Code != http.StatusConflict { + t.Fatalf("approve archived memory: want 409, got %d", w.Code) + } +} + +// ═══════════════════════════════════════════════ +// Reject +// ═══════════════════════════════════════════════ + +func TestMemory_Reject(t *testing.T) { + h := setupHarness(t) + userID, token := h.createAdminUser("memrejecter", "memrejecter@test.com") + + memID := seedMemory(t, userID, "to-reject", "val", models.MemoryStatusPendingReview) + + w := h.request("POST", "/api/v1/memories/"+memID+"/reject", token, nil) + if w.Code != http.StatusOK { + t.Fatalf("reject: want 200, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]interface{} + decode(w, &resp) + if resp["rejected"] != true { + t.Fatal("response should have rejected: true") + } +} + +func TestMemory_RejectStatusGuard(t *testing.T) { + h := setupHarness(t) + userID, token := h.createAdminUser("memguard2", "memguard2@test.com") + + // Can't reject an already-active memory + memID := seedMemory(t, userID, "active-reject", "val", models.MemoryStatusActive) + + w := h.request("POST", "/api/v1/memories/"+memID+"/reject", token, nil) + if w.Code != http.StatusConflict { + t.Fatalf("reject active memory: want 409, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestMemory_RejectOwnershipDenied(t *testing.T) { + h := setupHarness(t) + ownerID, _ := h.createAdminUser("rejowner", "rejowner@test.com") + _, otherToken := h.createAdminUser("rejother", "rejother@test.com") + + memID := seedMemory(t, ownerID, "not-yours", "val", models.MemoryStatusPendingReview) + + w := h.request("POST", "/api/v1/memories/"+memID+"/reject", otherToken, nil) + if w.Code != http.StatusForbidden { + t.Fatalf("reject other's memory: want 403, got %d: %s", w.Code, w.Body.String()) + } +} + +// ═══════════════════════════════════════════════ +// Count +// ═══════════════════════════════════════════════ + +func TestMemory_Count(t *testing.T) { + h := setupHarness(t) + userID, token := h.createAdminUser("memcounter", "memcounter@test.com") + + seedMemory(t, userID, "fact1", "a", models.MemoryStatusActive) + seedMemory(t, userID, "fact2", "b", models.MemoryStatusActive) + seedMemory(t, userID, "pend1", "c", models.MemoryStatusPendingReview) + + w := h.request("GET", "/api/v1/memories/count", token, nil) + if w.Code != http.StatusOK { + t.Fatalf("count: want 200, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]interface{} + decode(w, &resp) + + // Response shape: {"active": N, "pending": N} + active := int(resp["active"].(float64)) + pending := int(resp["pending"].(float64)) + if active != 2 { + t.Fatalf("expected 2 active, got %d", active) + } + if pending != 1 { + t.Fatalf("expected 1 pending, got %d", pending) + } +} + +// ═══════════════════════════════════════════════ +// Admin — List Pending +// ═══════════════════════════════════════════════ + +func TestMemory_AdminListPending(t *testing.T) { + h := setupHarness(t) + userA, _ := h.createAdminUser("adminmemA", "adminmemA@test.com") + userB, _ := h.createAdminUser("adminmemB", "adminmemB@test.com") + _, adminToken := h.createAdminUser("memadmin", "memadmin@test.com") + + // Seed pending memories for both users + seedMemory(t, userA, "factA", "from A", models.MemoryStatusPendingReview) + seedMemory(t, userB, "factB", "from B", models.MemoryStatusPendingReview) + seedMemory(t, userA, "active-A", "active", models.MemoryStatusActive) // should NOT appear + + w := h.request("GET", "/api/v1/admin/memories/pending", adminToken, nil) + if w.Code != http.StatusOK { + t.Fatalf("admin pending: want 200, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]interface{} + decode(w, &resp) + data := resp["data"].([]interface{}) + if len(data) != 2 { + t.Fatalf("expected 2 pending memories across users, got %d", len(data)) + } +} + +func TestMemory_AdminListPendingRequiresAdmin(t *testing.T) { + h := setupHarness(t) + database.TestDB.Exec("INSERT INTO platform_policies (key, value) VALUES ('allow_registration', 'true') ON CONFLICT (key) DO UPDATE SET value = 'true'") + database.TestDB.Exec("INSERT INTO platform_policies (key, value) VALUES ('default_user_active', 'true') ON CONFLICT (key) DO UPDATE SET value = 'true'") + _, userToken := h.registerUser("normalmem", "normalmem@test.com", "password123") + + w := h.request("GET", "/api/v1/admin/memories/pending", userToken, nil) + if w.Code != http.StatusForbidden { + t.Fatalf("non-admin should be forbidden: want 403, got %d", w.Code) + } +} + +// ═══════════════════════════════════════════════ +// Admin — Bulk Approve +// ═══════════════════════════════════════════════ + +func TestMemory_AdminBulkApprove(t *testing.T) { + h := setupHarness(t) + userID, _ := h.createAdminUser("bulkowner", "bulkowner@test.com") + _, adminToken := h.createAdminUser("bulkadmin", "bulkadmin@test.com") + + id1 := seedMemory(t, userID, "bulk1", "a", models.MemoryStatusPendingReview) + id2 := seedMemory(t, userID, "bulk2", "b", models.MemoryStatusPendingReview) + id3 := seedMemory(t, userID, "already-active", "c", models.MemoryStatusActive) + + w := h.request("POST", "/api/v1/admin/memories/bulk-approve", adminToken, map[string]interface{}{ + "ids": []string{id1, id2, id3}, + }) + if w.Code != http.StatusOK { + t.Fatalf("bulk approve: want 200, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]interface{} + decode(w, &resp) + approved := int(resp["approved"].(float64)) + if approved != 2 { + t.Fatalf("expected 2 approved (skipping already-active), got %d", approved) + } +} + +func TestMemory_AdminBulkApproveInvalidBody(t *testing.T) { + h := setupHarness(t) + _, adminToken := h.createAdminUser("bulkbad", "bulkbad@test.com") + + w := h.request("POST", "/api/v1/admin/memories/bulk-approve", adminToken, "not-json") + if w.Code != http.StatusBadRequest { + t.Fatalf("bad body: want 400, got %d", w.Code) + } +} + +// ═══════════════════════════════════════════════ +// Field Shape Verification +// ═══════════════════════════════════════════════ + +func TestMemory_FieldShape(t *testing.T) { + h := setupHarness(t) + userID, token := h.createAdminUser("memshape", "memshape@test.com") + + seedMemory(t, userID, "test-key", "test-value", models.MemoryStatusActive) + + w := h.request("GET", "/api/v1/memories", token, nil) + var resp map[string]interface{} + decode(w, &resp) + data := resp["data"].([]interface{}) + mem := data[0].(map[string]interface{}) + + // Verify all expected fields exist + requiredFields := []string{"id", "scope", "owner_id", "key", "value", "confidence", "status", "created_at", "updated_at"} + for _, f := range requiredFields { + if _, ok := mem[f]; !ok { + t.Errorf("missing field %q in memory object", f) + } + } + + // Verify no legacy "content" field + if _, ok := mem["content"]; ok { + t.Error("memory should not have 'content' field (legacy ICD)") + } + // Verify no legacy "persona_id" field + if _, ok := mem["persona_id"]; ok { + t.Error("memory should not have 'persona_id' field (legacy ICD)") + } + + // Verify scope is correct + if mem["scope"].(string) != models.MemoryScopeUser { + t.Errorf("scope should be %q, got %q", models.MemoryScopeUser, mem["scope"]) + } + + // Confidence should be a number + confidence, ok := mem["confidence"].(float64) + if !ok || confidence < 0 || confidence > 1 { + t.Errorf("confidence should be 0-1 float, got %v", mem["confidence"]) + } +} diff --git a/server/memory/extractor.go b/server/memory/extractor.go index f0de68f..fb61501 100644 --- a/server/memory/extractor.go +++ b/server/memory/extractor.go @@ -117,8 +117,14 @@ func (e *Extractor) Extract(ctx context.Context, channelID, userID, teamID, pers prompt := defaultExtractionPrompt if personaID != "" && e.stores.Personas != nil { persona, err := e.stores.Personas.GetByID(ctx, personaID) - if err == nil && persona.MemoryExtractionPrompt != nil && *persona.MemoryExtractionPrompt != "" { - prompt = *persona.MemoryExtractionPrompt + if err == nil { + // Respect persona memory_enabled flag (H6 — audit v0.28.0) + if !persona.MemoryEnabled { + return nil + } + if persona.MemoryExtractionPrompt != nil && *persona.MemoryExtractionPrompt != "" { + prompt = *persona.MemoryExtractionPrompt + } } } diff --git a/server/memory/scanner.go b/server/memory/scanner.go index 70ca0f8..a5f7b71 100644 --- a/server/memory/scanner.go +++ b/server/memory/scanner.go @@ -76,21 +76,23 @@ type candidate struct { ChannelID string UserID string TeamID string + PersonaID string // non-empty if a persona is active on the channel } func (s *Scanner) scan() { ctx := context.Background() - // Check global kill switch + // Check global kill switch. + // Default: ENABLED. Only disable if the key exists and is explicitly false. + // This inverts the previous behavior where a missing key blocked all extraction. if s.stores.GlobalConfig != nil { val, err := s.stores.GlobalConfig.Get(ctx, "memory_extraction_enabled") - if err != nil || val == nil { - return // disabled or not configured - } - // val is a map; check the "value" key - if enabled, ok := val["value"].(bool); !ok || !enabled { - return + if err == nil && val != nil { + if enabled, ok := val["value"].(bool); ok && !enabled { + return // explicitly disabled + } } + // If key is missing (err != nil or val == nil), extraction is enabled. } candidates, err := s.findCandidates(ctx) @@ -124,7 +126,7 @@ func (s *Scanner) scan() { extractCtx, cancel := context.WithTimeout(ctx, 2*time.Minute) defer cancel() - if err := s.extractor.Extract(extractCtx, cand.ChannelID, cand.UserID, cand.TeamID, ""); err != nil { + if err := s.extractor.Extract(extractCtx, cand.ChannelID, cand.UserID, cand.TeamID, cand.PersonaID); err != nil { log.Printf("⚠ memory extraction failed: channel=%s user=%s: %v", cand.ChannelID, cand.UserID, err) } @@ -135,6 +137,12 @@ func (s *Scanner) scan() { } // findCandidates queries for channels with enough new activity since last extraction. +// +// Fixes from v0.28.0 audit: +// - Uses channels.user_id (was: non-existent created_by) +// - Uses channels.team_id directly (was: JOIN to non-existent channel_users table) +// - JOINs channel_participants + personas to check memory_enabled (H6) +// - Filters out channels whose active persona has memory_enabled=false func (s *Scanner) findCandidates(ctx context.Context) ([]candidate, error) { db := database.DB if db == nil { @@ -144,11 +152,17 @@ func (s *Scanner) findCandidates(ctx context.Context) ([]candidate, error) { var query string if database.IsSQLite() { query = ` - SELECT c.id, c.created_by, COALESCE(cu.team_id, '') + SELECT c.id, c.user_id, COALESCE(c.team_id, ''), + COALESCE(cp.participant_id, '') FROM channels c - LEFT JOIN channel_users cu ON cu.channel_id = c.id AND cu.user_id = c.created_by - LEFT JOIN memory_extraction_log mel ON mel.channel_id = c.id AND mel.user_id = c.created_by - WHERE c.created_by IS NOT NULL + LEFT JOIN channel_participants cp + ON cp.channel_id = c.id AND cp.participant_type = 'persona' + LEFT JOIN personas p + ON p.id = cp.participant_id + LEFT JOIN memory_extraction_log mel + ON mel.channel_id = c.id AND mel.user_id = c.user_id + WHERE c.user_id IS NOT NULL + AND (cp.id IS NULL OR p.memory_enabled = 1) AND ( SELECT COUNT(*) FROM messages m WHERE m.channel_id = c.id @@ -161,11 +175,17 @@ func (s *Scanner) findCandidates(ctx context.Context) ([]candidate, error) { ` } else { query = ` - SELECT c.id, c.created_by, COALESCE(cu.team_id, '') + SELECT c.id, c.user_id, COALESCE(c.team_id::text, ''), + COALESCE(cp.participant_id::text, '') FROM channels c - LEFT JOIN channel_users cu ON cu.channel_id = c.id AND cu.user_id = c.created_by - LEFT JOIN memory_extraction_log mel ON mel.channel_id = c.id AND mel.user_id = c.created_by - WHERE c.created_by IS NOT NULL + LEFT JOIN channel_participants cp + ON cp.channel_id = c.id AND cp.participant_type = 'persona' + LEFT JOIN personas p + ON p.id = cp.participant_id + LEFT JOIN memory_extraction_log mel + ON mel.channel_id = c.id AND mel.user_id = c.user_id + WHERE c.user_id IS NOT NULL + AND (cp.id IS NULL OR p.memory_enabled = true) AND ( SELECT COUNT(*) FROM messages m WHERE m.channel_id = c.id @@ -187,7 +207,7 @@ func (s *Scanner) findCandidates(ctx context.Context) ([]candidate, error) { var candidates []candidate for rows.Next() { var c candidate - if err := rows.Scan(&c.ChannelID, &c.UserID, &c.TeamID); err != nil { + if err := rows.Scan(&c.ChannelID, &c.UserID, &c.TeamID, &c.PersonaID); err != nil { continue } candidates = append(candidates, c) diff --git a/server/models/models_memory_persona.go b/server/models/models_memory_persona.go deleted file mode 100644 index dec45e6..0000000 --- a/server/models/models_memory_persona.go +++ /dev/null @@ -1,18 +0,0 @@ -// ── models_memory_persona.go ──────────────── -// Phase 2 additions to the Persona struct. -// -// Add these fields to the Persona struct in models/models.go -// (after the KBIDs field): -// -// MemoryEnabled bool `json:"memory_enabled" db:"memory_enabled"` -// MemoryExtractionPrompt *string `json:"memory_extraction_prompt,omitempty" db:"memory_extraction_prompt"` -// -// Add to PersonaPatch: -// -// MemoryEnabled *bool `json:"memory_enabled,omitempty"` -// MemoryExtractionPrompt *string `json:"memory_extraction_prompt,omitempty"` -// -// This file exists purely as documentation — the actual fields -// must be added to the existing structs in models.go. - -package models diff --git a/server/store/postgres/memory.go b/server/store/postgres/memory.go index 6bddd28..bcb39b5 100644 --- a/server/store/postgres/memory.go +++ b/server/store/postgres/memory.go @@ -5,7 +5,6 @@ import ( "database/sql" "fmt" "strings" - "time" "git.gobha.me/xcaliber/chat-switchboard/models" "git.gobha.me/xcaliber/chat-switchboard/store" @@ -42,6 +41,16 @@ func (s *MemoryStore) Upsert(ctx context.Context, m *models.Memory) error { return err } +func (s *MemoryStore) Update(ctx context.Context, m *models.Memory) error { + _, err := DB.ExecContext(ctx, ` + UPDATE memories + SET key = $1, value = $2, confidence = $3, status = $4, + source_channel_id = $5, updated_at = now() + WHERE id = $6 + `, m.Key, m.Value, m.Confidence, m.Status, m.SourceChannelID, m.ID) + return err +} + func (s *MemoryStore) GetByID(ctx context.Context, id string) (*models.Memory, error) { m := &models.Memory{} err := DB.QueryRowContext(ctx, ` @@ -220,6 +229,25 @@ func (s *MemoryStore) CountByOwner(ctx context.Context, scope, ownerID string) ( return count, err } +func (s *MemoryStore) ListByStatus(ctx context.Context, status string, limit int) ([]models.Memory, error) { + if limit <= 0 || limit > 500 { + limit = 100 + } + rows, err := DB.QueryContext(ctx, ` + SELECT id, scope, owner_id, user_id, key, value, + source_channel_id, confidence, status, created_at, updated_at + FROM memories + WHERE status = $1 + ORDER BY created_at DESC + LIMIT $2 + `, status, limit) + if err != nil { + return nil, err + } + defer rows.Close() + return scanMemories(rows) +} + // ── Helpers ───────────────────────────────── func scanMemories(rows *sql.Rows) ([]models.Memory, error) { @@ -240,30 +268,5 @@ func scanMemories(rows *sql.Rows) ([]models.Memory, error) { return memories, rows.Err() } -// scanMemory scans a single memory row — unused currently but available -// for future single-row queries. -func scanMemory(row *sql.Row) (*models.Memory, error) { - m := &models.Memory{} - err := row.Scan( - &m.ID, &m.Scope, &m.OwnerID, &m.UserID, &m.Key, &m.Value, - &m.SourceChannelID, &m.Confidence, &m.Status, &m.CreatedAt, &m.UpdatedAt, - ) - if err == sql.ErrNoRows { - return nil, nil - } - return m, err -} - // ensure compile-time interface satisfaction -var _ interface { - Upsert(ctx context.Context, m *models.Memory) error - GetByID(ctx context.Context, id string) (*models.Memory, error) - List(ctx context.Context, f models.MemoryFilter) ([]models.Memory, error) - Delete(ctx context.Context, id string) error - Archive(ctx context.Context, id string) error - Recall(ctx context.Context, userID string, personaID *string, query string, limit int) ([]models.Memory, error) - CountByOwner(ctx context.Context, scope, ownerID string) (int, error) -} = (*MemoryStore)(nil) - -// ensure unused imports don't cause errors -var _ = time.Now +var _ store.MemoryStore = (*MemoryStore)(nil) diff --git a/server/store/postgres/memory_hybrid.go b/server/store/postgres/memory_hybrid.go index d58dec6..e533d7e 100644 --- a/server/store/postgres/memory_hybrid.go +++ b/server/store/postgres/memory_hybrid.go @@ -2,7 +2,6 @@ package postgres import ( "context" - "database/sql" "fmt" "strings" @@ -130,6 +129,3 @@ func mergeResults(semantic, keyword []models.Memory, limit int) []models.Memory return result } - -// ensure unused -var _ = sql.ErrNoRows diff --git a/server/store/sqlite/memory.go b/server/store/sqlite/memory.go index 70d0afb..9c3737e 100644 --- a/server/store/sqlite/memory.go +++ b/server/store/sqlite/memory.go @@ -41,6 +41,16 @@ func (s *MemoryStore) Upsert(ctx context.Context, m *models.Memory) error { return err } +func (s *MemoryStore) Update(ctx context.Context, m *models.Memory) error { + _, err := DB.ExecContext(ctx, ` + UPDATE memories + SET key = ?, value = ?, confidence = ?, status = ?, + source_channel_id = ?, updated_at = datetime('now') + WHERE id = ? + `, m.Key, m.Value, m.Confidence, m.Status, m.SourceChannelID, m.ID) + return err +} + func (s *MemoryStore) GetByID(ctx context.Context, id string) (*models.Memory, error) { m := &models.Memory{} err := DB.QueryRowContext(ctx, ` @@ -202,6 +212,25 @@ func (s *MemoryStore) CountByOwner(ctx context.Context, scope, ownerID string) ( return count, err } +func (s *MemoryStore) ListByStatus(ctx context.Context, status string, limit int) ([]models.Memory, error) { + if limit <= 0 || limit > 500 { + limit = 100 + } + rows, err := DB.QueryContext(ctx, ` + SELECT id, scope, owner_id, user_id, key, value, + source_channel_id, confidence, status, created_at, updated_at + FROM memories + WHERE status = ? + ORDER BY created_at DESC + LIMIT ? + `, status, limit) + if err != nil { + return nil, err + } + defer rows.Close() + return s.scanMemories(rows) +} + // ── Helpers ───────────────────────────────── func (s *MemoryStore) scanMemories(rows *sql.Rows) ([]models.Memory, error) { @@ -222,3 +251,6 @@ func (s *MemoryStore) scanMemories(rows *sql.Rows) ([]models.Memory, error) { } return memories, rows.Err() } + +// ensure compile-time interface satisfaction +var _ store.MemoryStore = (*MemoryStore)(nil) diff --git a/server/store/store_memory.go b/server/store/store_memory.go index d943ae7..f9f3b26 100644 --- a/server/store/store_memory.go +++ b/server/store/store_memory.go @@ -21,6 +21,10 @@ type MemoryStore interface { // If a memory with the same key exists, value/confidence/status are updated. Upsert(ctx context.Context, m *models.Memory) error + // Update modifies an existing memory by ID. Use for handler edits where + // the key may change (Upsert conflicts on composite key, not on ID). + Update(ctx context.Context, m *models.Memory) error + // GetByID returns a single memory by ID. GetByID(ctx context.Context, id string) (*models.Memory, error) @@ -41,6 +45,10 @@ type MemoryStore interface { // CountByOwner returns the number of active memories for a scope+owner. CountByOwner(ctx context.Context, scope, ownerID string) (int, error) + // ListByStatus returns all memories with the given status across all owners. + // Used by admin review endpoints. Results ordered by created_at DESC. + ListByStatus(ctx context.Context, status string, limit int) ([]models.Memory, error) + // RecallHybrid returns active memories using vector similarity + keyword search. // If queryVec is nil/empty, falls back to keyword-only (same as Recall). RecallHybrid(ctx context.Context, userID string, personaID *string, query string, queryVec []float64, limit int) ([]models.Memory, error) diff --git a/server/store/store_memory_hybrid.go b/server/store/store_memory_hybrid.go deleted file mode 100644 index 0b6516b..0000000 --- a/server/store/store_memory_hybrid.go +++ /dev/null @@ -1,15 +0,0 @@ -// ── store_memory_hybrid.go ────────────────── -// Phase 2 addition to the MemoryStore interface. -// -// Add this method to the MemoryStore interface in store/store_memory.go: -// -// // RecallHybrid returns active memories using vector similarity + keyword search. -// // If queryVec is nil/empty, falls back to keyword-only (same as Recall). -// // Merges results from applicable scopes with deduplication. -// RecallHybrid(ctx context.Context, userID string, personaID *string, query string, queryVec []float64, limit int) ([]models.Memory, error) -// -// The implementations are in: -// - store/postgres/memory_hybrid.go (pgvector cosine distance) -// - store/sqlite/memory_hybrid.go (app-level cosine similarity) - -package store diff --git a/src/js/memory-ui.js b/src/js/memory-ui.js index 980a698..f6d4b0a 100644 --- a/src/js/memory-ui.js +++ b/src/js/memory-ui.js @@ -84,7 +84,7 @@ const MemoryUI = { try { const data = await API.listMemories(this._filter.status, this._filter.query); - this._memories = data.memories || []; + this._memories = data.data || []; if (!this._memories.length) { el.innerHTML = `
No ${this._filter.status.replace('_', ' ')} memories${this._filter.query ? ' matching "' + esc(this._filter.query) + '"' : ''}
`; @@ -211,7 +211,7 @@ const MemoryUI = { async approveAllPending() { try { const data = await API.listMemories('pending_review', ''); - const ids = (data.memories || []).map(m => m.id); + const ids = (data.data || []).map(m => m.id); if (!ids.length) return UI.toast('No pending memories', 'info'); if (!await showConfirm(`Approve all ${ids.length} pending memories?`)) return; await API.bulkApproveMemories(ids); @@ -231,7 +231,7 @@ const MemoryUI = { try { const data = await API.adminListPendingMemories(); - const pending = data.memories || []; + const pending = data.data || []; el.innerHTML = `