diff --git a/workspace-server/entrypoint-tenant.sh b/workspace-server/entrypoint-tenant.sh index a625f4327..bfd11b2b6 100644 --- a/workspace-server/entrypoint-tenant.sh +++ b/workspace-server/entrypoint-tenant.sh @@ -32,19 +32,18 @@ CANVAS_PID=$! # Defaults (when sidecar IS spawned): MEMORY_PLUGIN_DATABASE_URL # falls back to the tenant's DATABASE_URL. # -# MEMORY_V2_CUTOVER is deprecated as of #1747 — the workspace-server -# binary no longer reads it (v2 is unconditional now; the legacy SQL -# fallback in mcp_tools.go is gone). The entrypoint still accepts it -# as a synonym for "operator wants the sidecar" so old CP user-data -# templates keep working through the rollout. When CP user-data drops -# the var, this branch can go. +# Phase A3 (#1792): MEMORY_V2_CUTOVER acceptance removed. The variable +# was deprecated by #1747 (binary stopped reading it) and only kept +# alive here as a synonym to bridge old CP user-data templates. With +# A3 dropping the entire v1 surface, the synonym is gone too. CP +# user-data sets MEMORY_PLUGIN_URL directly; if a stale template +# without that var ships, the sidecar simply doesn't start and the +# tenant boots without memory — loud but recoverable, same posture as +# any other required env missing. MEMORY_PLUGIN_PID="" memory_plugin_wanted="" if [ -n "$MEMORY_PLUGIN_URL" ]; then memory_plugin_wanted=1 -elif [ "$MEMORY_V2_CUTOVER" = "true" ]; then - memory_plugin_wanted=1 - echo "memory-plugin: ⚠️ MEMORY_V2_CUTOVER is deprecated (#1747) — set MEMORY_PLUGIN_URL instead. Spawning sidecar on the implied default this boot." >&2 fi if [ -z "$MEMORY_PLUGIN_DISABLE" ] && [ -n "$memory_plugin_wanted" ] && [ -n "$DATABASE_URL" ]; then # Schema isolation (issue #1733): when defaulting from the tenant diff --git a/workspace-server/internal/handlers/activity.go b/workspace-server/internal/handlers/activity.go index cef0e45ad..9bb9ebb3e 100644 --- a/workspace-server/internal/handlers/activity.go +++ b/workspace-server/internal/handlers/activity.go @@ -656,9 +656,17 @@ func parseSessionSearchParams(c *gin.Context) (string, int) { return query, limit } -// buildSessionSearchQuery composes the UNION-ALL SQL across activity_logs and -// agent_memories with an optional ILIKE filter, returning the SQL string and -// positional args ready for QueryContext. +// buildSessionSearchQuery composes the session-search SQL over +// activity_logs, returning the SQL string and positional args ready +// for QueryContext. +// +// Phase A3 (#1792): the agent_memories UNION branch was removed when +// the v1 table was dropped. Memory items no longer appear in session +// search; the canvas MemoryInspectorPanel queries /v2/memories +// directly for memory-tab content, so the UNION here only served +// callers that wanted activity + memory blended results — that +// surface is unused in production (verified via traffic audit +// 2026-05-24). func buildSessionSearchQuery(workspaceID, query string, limit int) (string, []interface{}) { sqlQuery := ` WITH session_items AS ( @@ -675,20 +683,6 @@ func buildSessionSearchQuery(workspaceID, query string, limit int) (string, []in created_at FROM activity_logs WHERE workspace_id = $1 - UNION ALL - SELECT - 'memory' AS kind, - id, - workspace_id, - scope AS label, - content, - '' AS method, - '' AS status, - NULL::jsonb AS request_body, - NULL::jsonb AS response_body, - created_at - FROM agent_memories - WHERE workspace_id = $1 ) SELECT kind, id, workspace_id, label, content, method, status, request_body, response_body, created_at FROM session_items diff --git a/workspace-server/internal/handlers/memories.go b/workspace-server/internal/handlers/memories.go index 1ee2bdaa7..97bbbfcf1 100644 --- a/workspace-server/internal/handlers/memories.go +++ b/workspace-server/internal/handlers/memories.go @@ -1,7 +1,6 @@ package handlers import ( - "context" "crypto/sha256" "encoding/hex" "encoding/json" @@ -15,27 +14,14 @@ import ( "github.com/Molecule-AI/molecule-monorepo/platform/internal/memory/client" "github.com/Molecule-AI/molecule-monorepo/platform/internal/memory/contract" "github.com/Molecule-AI/molecule-monorepo/platform/internal/memory/namespace" - "github.com/Molecule-AI/molecule-monorepo/platform/internal/registry" "github.com/gin-gonic/gin" ) -// globalMemoryDelimiter is the non-instructable prefix prepended to every -// GLOBAL-scope memory value returned to MCP clients. Prevents stored content -// from being parsed as LLM instructions in the agent's context window (#767). -// Format: [MEMORY id= scope=GLOBAL from=]: -const globalMemoryDelimiter = "[MEMORY id=%s scope=GLOBAL from=%s]: %s" - // defaultMemoryNamespace is used when a caller omits the field on POST or // when querying for memories written before migration 017. Matches the // column default in platform/migrations/017_memories_fts_namespace.up.sql. const defaultMemoryNamespace = "general" -// memoryFTSMinQueryLen is the shortest query length that gets Postgres -// full-text search treatment. Anything shorter uses ILIKE because -// tsvector requires at least one token and single characters tokenise -// to nothing in the 'english' config. -const memoryFTSMinQueryLen = 2 - // secretPatternEntry is a compiled regex + its human-readable redaction label. type secretPatternEntry struct { re *regexp.Regexp @@ -80,43 +66,25 @@ func redactSecrets(workspaceID, content string) (out string, changed bool) { return out, changed } -// EmbeddingFunc generates a 1536-dimensional dense-vector embedding for the -// given text. Must return exactly 1536 float32 values on success. -// Implementations must honour ctx cancellation. -// nil is not a valid return on success — return a non-nil error instead. -type EmbeddingFunc func(ctx context.Context, text string) ([]float32, error) - -// MemoriesHandler manages agent memory storage and recall. +// MemoriesHandler owns the legacy POST /workspaces/:id/memories surface, +// which post-#1794 routes through the v2 memory plugin. The legacy +// Search/Update/Delete methods + their HTTP routes were removed in +// #1792 (Phase A3) along with the agent_memories table they read. +// New code that needs memory reads should use the /v2/memories endpoints +// exposed by MemoriesV2Handler (canvas) or the MCP memory tools (agents). type MemoriesHandler struct { - // embed generates vector embeddings for semantic search (issue #576). - // nil disables the semantic path — all operations degrade gracefully to - // the existing FTS/ILIKE path. Read methods (Search/Get) still consult - // this; the v2 plugin owns embeddings on its own write path. - embed EmbeddingFunc - - // memv2 routes Commit writes through the v2 memory plugin (issue #1791 - // — Phase A2 step 1). When nil, Commit returns 503; the v1 SQL INSERT - // is gone, matching #1747's "plugin is the only backend" posture for - // the MCP path. Search/Update/Delete on this handler still read v1 - // — they're tracked separately so this PR stays single-axis. + // memv2 routes Commit writes through the v2 memory plugin. When nil, + // Commit returns 503 — matches #1747's "plugin is the only backend" + // posture for the MCP path. memv2 *memoryV2Deps } -// NewMemoriesHandler constructs a handler with FTS-only mode. -// Wire up semantic search with WithEmbedding. +// NewMemoriesHandler constructs a handler with no plugin wired. Call +// WithMemoryV2 to attach the production plugin client. func NewMemoriesHandler() *MemoriesHandler { return &MemoriesHandler{} } -// WithEmbedding installs a vector-embedding function. Call during router -// wiring, before the first request. Passing nil is a no-op. Chainable. -func (h *MemoriesHandler) WithEmbedding(fn EmbeddingFunc) *MemoriesHandler { - if fn != nil { - h.embed = fn - } - return h -} - // WithMemoryV2 wires the plugin client + namespace resolver so Commit // can route writes through the v2 plugin instead of raw SQL into // `agent_memories` (issue #1791). Mirrors MCPHandler.WithMemoryV2 so @@ -139,24 +107,6 @@ func (h *MemoriesHandler) withMemoryV2APIs(plugin memoryPluginAPI, resolver name return h } -// formatVector encodes a float32 embedding slice as a pgvector literal -// suitable for a ::vector cast, e.g. "[0.1,-0.05,0.42]". -// Returns an empty string for nil/empty slices. -func formatVector(v []float32) string { - if len(v) == 0 { - return "" - } - var b strings.Builder - b.WriteByte('[') - for i, x := range v { - if i > 0 { - b.WriteByte(',') - } - fmt.Fprintf(&b, "%g", x) - } - b.WriteByte(']') - return b.String() -} // Commit handles POST /workspaces/:id/memories // Stores a memory fact with a scope (LOCAL, TEAM, GLOBAL) and an optional @@ -310,434 +260,3 @@ func (h *MemoriesHandler) Commit(c *gin.Context) { // namespace — the latter is an internal storage detail. c.JSON(http.StatusCreated, gin.H{"id": memoryID, "scope": body.Scope, "namespace": namespace}) } - -// memoryRecallMaxLimit is the hard ceiling for results returned by Search. -// Callers may request fewer via ?limit=N but never more (#377). -const memoryRecallMaxLimit = 50 - -// Search handles GET /workspaces/:id/memories -// Searches memories visible to the requesting workspace. -// -// Supports: -// - ?scope=LOCAL|TEAM|GLOBAL for access-control slicing -// - ?q=... semantic search (cosine similarity) when an EmbeddingFunc is -// configured AND the query can be embedded; falls back to FTS when the -// embed call fails or no func is configured. -// - ?q=... full-text search (ts_rank ordered) when len>=memoryFTSMinQueryLen -// and no embedding is available; falls back to ILIKE for shorter strings. -// - ?namespace=... additional filter on the Holaboss-style namespace tag -// - ?limit=N max results (1–50); values >50 are silently clamped to 50 (#377) -// -// Semantic results include a "similarity_score" field (1 - cosine_distance). -func (h *MemoriesHandler) Search(c *gin.Context) { - workspaceID := c.Param("id") - scope := c.DefaultQuery("scope", "") - query := c.DefaultQuery("q", "") - namespace := c.DefaultQuery("namespace", "") - - // Parse and cap the limit. Anything ≤0 or absent → 50 (full page). - // Anything >50 → 50 (hard ceiling — never error, just clamp). - limit := memoryRecallMaxLimit - if raw := c.Query("limit"); raw != "" { - var n int - if _, err := fmt.Sscanf(raw, "%d", &n); err == nil && n > 0 && n < memoryRecallMaxLimit { - limit = n - } - } - ctx := c.Request.Context() - - // Get workspace info for access control - var parentID *string - db.DB.QueryRowContext(ctx, `SELECT parent_id FROM workspaces WHERE id = $1`, workspaceID).Scan(&parentID) - - // Try to generate a query embedding for semantic search. - // Falls back to the existing FTS/ILIKE path on failure or when no - // embedding function is configured. - semanticVec := "" - if query != "" && h.embed != nil { - if vec, err := h.embed(ctx, query); err != nil { - log.Printf("Search: embedding failed workspace=%s: %v — falling back to FTS", workspaceID, err) - } else { - semanticVec = formatVector(vec) - } - } - - var sqlQuery string - var args []interface{} - semantic := semanticVec != "" - - if semantic { - // ── Semantic search path ────────────────────────────────────────── - // Build scope-specific WHERE fragment and initial args. - isJoin := scope == "TEAM" - var baseWhere string - switch scope { - case "LOCAL": - baseWhere = `workspace_id = $1 AND scope = 'LOCAL'` - args = []interface{}{workspaceID} - case "TEAM": - if parentID != nil { - baseWhere = `m.scope = 'TEAM' AND w.status != 'removed' AND (w.parent_id = $1 OR w.id = $1)` - args = []interface{}{*parentID} - } else { - baseWhere = `m.scope = 'TEAM' AND w.status != 'removed' AND (w.parent_id = $1 OR w.id = $1)` - args = []interface{}{workspaceID} - } - case "GLOBAL": - baseWhere = `scope = 'GLOBAL'` - args = []interface{}{} - default: - baseWhere = `workspace_id = $1` - args = []interface{}{workspaceID} - } - if namespace != "" { - nsArg := nextArg(len(args)) - if isJoin { - baseWhere += ` AND m.namespace = ` + nsArg - } else { - baseWhere += ` AND namespace = ` + nsArg - } - args = append(args, namespace) - } - - // $vecPos appears twice (SELECT + ORDER BY) — PostgreSQL resolves - // both to the same bound value, so we append it only once. - vecPos := nextArg(len(args)) - limitPos := nextArg(len(args) + 1) - - if isJoin { - sqlQuery = `SELECT m.id, m.workspace_id, m.content, m.scope, m.namespace, m.created_at,` + - ` 1 - (m.embedding <=> ` + vecPos + `::vector) AS similarity_score` + - ` FROM agent_memories m JOIN workspaces w ON w.id = m.workspace_id` + - ` WHERE ` + baseWhere + ` AND m.embedding IS NOT NULL` + - ` ORDER BY m.embedding <=> ` + vecPos + `::vector` + - ` LIMIT ` + limitPos - } else { - sqlQuery = `SELECT id, workspace_id, content, scope, namespace, created_at,` + - ` 1 - (embedding <=> ` + vecPos + `::vector) AS similarity_score` + - ` FROM agent_memories` + - ` WHERE ` + baseWhere + ` AND embedding IS NOT NULL` + - ` ORDER BY embedding <=> ` + vecPos + `::vector` + - ` LIMIT ` + limitPos - } - args = append(args, semanticVec, limit) - - } else { - // ── FTS / ILIKE / plain path ────────────────────────────────────── - switch scope { - case "LOCAL": - // Only this workspace's memories - sqlQuery = `SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE workspace_id = $1 AND scope = 'LOCAL'` - args = []interface{}{workspaceID} - - case "TEAM": - // Team = self + parent + siblings (same parent_id) - if parentID != nil { - // Child workspace: team is parent + siblings sharing same parent_id - sqlQuery = `SELECT m.id, m.workspace_id, m.content, m.scope, m.namespace, m.created_at - FROM agent_memories m - JOIN workspaces w ON w.id = m.workspace_id - WHERE m.scope = 'TEAM' AND w.status != 'removed' - AND (w.parent_id = $1 OR w.id = $1)` - args = []interface{}{*parentID} - } else { - // Root workspace: team is self + direct children only - sqlQuery = `SELECT m.id, m.workspace_id, m.content, m.scope, m.namespace, m.created_at - FROM agent_memories m - JOIN workspaces w ON w.id = m.workspace_id - WHERE m.scope = 'TEAM' AND w.status != 'removed' - AND (w.parent_id = $1 OR w.id = $1)` - args = []interface{}{workspaceID} - } - - case "GLOBAL": - // All GLOBAL memories (readable by everyone) - sqlQuery = `SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE scope = 'GLOBAL'` - args = []interface{}{} - - default: - // All accessible memories - sqlQuery = `SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE workspace_id = $1` - args = []interface{}{workspaceID} - } - - // Namespace filter (optional) — applies regardless of scope. - if namespace != "" { - sqlQuery += ` AND namespace = ` + nextArg(len(args)) - args = append(args, namespace) - } - - // Text search: FTS with ts_rank ordering for multi-char queries, - // ILIKE fallback for 1-char and empty-after-tokenization edge cases. - ftsActive := false - if len(query) >= memoryFTSMinQueryLen { - sqlQuery += ` AND content_tsv @@ plainto_tsquery('english', ` + nextArg(len(args)) + `)` - args = append(args, query) - ftsActive = true - } else if query != "" { - sqlQuery += ` AND content ILIKE ` + nextArg(len(args)) - args = append(args, "%"+query+"%") - } - - if ftsActive { - // Rank FTS hits first, tie-break by recency. - sqlQuery += ` ORDER BY ts_rank(content_tsv, plainto_tsquery('english', ` + nextArg(len(args)) + `)) DESC, created_at DESC` - args = append(args, query) - } else { - sqlQuery += ` ORDER BY created_at DESC` - } - sqlQuery += ` LIMIT ` + nextArg(len(args)) - args = append(args, limit) - } - - rows, err := db.DB.QueryContext(ctx, sqlQuery, args...) - if err != nil { - log.Printf("Search memories error: %v", err) - c.JSON(http.StatusInternalServerError, gin.H{"error": "search failed"}) - return - } - defer rows.Close() - - memories := make([]map[string]interface{}, 0) - for rows.Next() { - var id, wsID, content, memScope, memNS, createdAt string - entry := map[string]interface{}{} - - if semantic { - var simScore float64 - if rows.Scan(&id, &wsID, &content, &memScope, &memNS, &createdAt, &simScore) != nil { - continue - } - entry["similarity_score"] = simScore - } else { - if rows.Scan(&id, &wsID, &content, &memScope, &memNS, &createdAt) != nil { - continue - } - } - - // Access control check for TEAM scope - if memScope == "TEAM" && wsID != workspaceID { - if !registry.CanCommunicate(workspaceID, wsID) { - continue // Skip memories from workspaces we can't reach - } - } - - // #767: wrap GLOBAL-scope content with a non-instructable delimiter so - // MCP tool outputs cannot be hijacked by stored prompt-injection payloads. - // The raw content in the DB is unchanged — only the value returned to - // callers is wrapped. Applied on both the semantic and FTS paths. - if memScope == "GLOBAL" { - content = fmt.Sprintf(globalMemoryDelimiter, id, wsID, content) - } - - entry["id"] = id - entry["workspace_id"] = wsID - entry["content"] = content - entry["scope"] = memScope - entry["namespace"] = memNS - entry["created_at"] = createdAt - memories = append(memories, entry) - } - if err := rows.Err(); err != nil { - log.Printf("Search memories rows.Err: %v", err) - } - - c.JSON(http.StatusOK, memories) -} - -// Update handles PATCH /workspaces/:id/memories/:memoryId -// -// Edits an existing semantic-memory row's content and/or namespace. -// Both body fields are optional; at least one must be present (a body -// with neither returns 400 — there's nothing to do, and silently -// no-op'ing would let a buggy client think it had succeeded). -// -// Content edits re-run the same security pipeline as Commit: secret -// redaction (#1201) on every scope, plus delimiter-spoofing escape on -// GLOBAL. Skipping either when content changes would mean an Edit -// becomes a back-door past the policies a Commit enforces. The same -// re-embedding rule applies — a stale embedding for the new content -// would silently break semantic search. GLOBAL audit log fires on -// content change so the forensic trail captures edits, not just -// initial writes. -// -// Namespace edits are validated against the same 50-char ceiling -// Commit uses; cross-scope changes (e.g. LOCAL→GLOBAL) are NOT -// supported here — that's a delete + recreate so the GLOBAL -// access-control gate (only root workspaces can write GLOBAL) gets -// re-evaluated from scratch. -// -// Returns 200 with the updated row's id+scope+namespace on success, -// 400 on bad body, 404 when the memory doesn't exist or isn't owned -// by this workspace, 500 on DB failure. -func (h *MemoriesHandler) Update(c *gin.Context) { - workspaceID := c.Param("id") - memoryID := c.Param("memoryId") - ctx := c.Request.Context() - - // json.Decode (not gin's ShouldBindJSON) so we can distinguish - // "field omitted" from "field set to empty string" — content="" is - // invalid; content omitted means "don't change content". - var body struct { - Content *string `json:"content,omitempty"` - Namespace *string `json:"namespace,omitempty"` - } - if err := json.NewDecoder(c.Request.Body).Decode(&body); err != nil { - c.JSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) - return - } - if body.Content == nil && body.Namespace == nil { - c.JSON(http.StatusBadRequest, gin.H{ - "error": "at least one of content or namespace must be set", - }) - return - } - if body.Content != nil && *body.Content == "" { - c.JSON(http.StatusBadRequest, gin.H{"error": "content cannot be empty"}) - return - } - if body.Namespace != nil { - if len(*body.Namespace) == 0 { - c.JSON(http.StatusBadRequest, gin.H{"error": "namespace cannot be empty"}) - return - } - if len(*body.Namespace) > 50 { - c.JSON(http.StatusBadRequest, gin.H{"error": "namespace must be <= 50 characters"}) - return - } - } - - // Fetch current row to discover the scope (we need it for the - // GLOBAL delimiter-escape + audit log) and to confirm ownership. - // One round-trip rather than two: SELECT ... WHERE id AND - // workspace_id covers the 404 path without an extra existence check. - var existingScope, existingContent, existingNamespace string - if err := db.DB.QueryRowContext(ctx, ` - SELECT scope, content, namespace - FROM agent_memories - WHERE id = $1 AND workspace_id = $2 - `, memoryID, workspaceID).Scan(&existingScope, &existingContent, &existingNamespace); err != nil { - // sql.ErrNoRows or any other read failure — both surface as 404 - // to avoid leaking row existence across workspaces. - c.JSON(http.StatusNotFound, gin.H{"error": "memory not found or not owned by this workspace"}) - return - } - - // Compute the new content (post-redaction, post-delimiter-escape) - // only when content is actually changing. This keeps namespace-only - // edits cheap (no embed call, no audit row). - newContent := existingContent - contentChanged := false - if body.Content != nil && *body.Content != existingContent { - c2 := *body.Content - c2, _ = redactSecrets(workspaceID, c2) - if existingScope == "GLOBAL" { - c2 = strings.ReplaceAll(c2, "[MEMORY ", "[_MEMORY ") - } - if c2 != existingContent { - newContent = c2 - contentChanged = true - } - } - - newNamespace := existingNamespace - if body.Namespace != nil && *body.Namespace != existingNamespace { - newNamespace = *body.Namespace - } - - if !contentChanged && newNamespace == existingNamespace { - // Nothing to do post-normalisation (e.g. caller passed the - // SAME content + namespace). Return the existing shape so the - // caller's response-handling can stay uniform with the change - // path — silently no-op would force every client to special- - // case 204. - c.JSON(http.StatusOK, gin.H{ - "id": memoryID, "scope": existingScope, "namespace": existingNamespace, - "changed": false, - }) - return - } - - if _, err := db.DB.ExecContext(ctx, ` - UPDATE agent_memories - SET content = $1, namespace = $2, updated_at = now() - WHERE id = $3 AND workspace_id = $4 - `, newContent, newNamespace, memoryID, workspaceID); err != nil { - log.Printf("Update memory error: %v", err) - c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to update memory"}) - return - } - - // GLOBAL content edits write an audit row mirroring Commit's #767 - // pattern. Namespace-only edits don't get an audit entry — the - // content (and its sha256) is unchanged, so there's nothing new - // for forensic replay to capture. - if existingScope == "GLOBAL" && contentChanged { - sum := sha256.Sum256([]byte(newContent)) - auditBody, _ := json.Marshal(map[string]string{ - "memory_id": memoryID, - "namespace": newNamespace, - "content_sha256": hex.EncodeToString(sum[:]), - "reason": "edited", - }) - summary := "GLOBAL memory edited: id=" + memoryID + " namespace=" + newNamespace - if _, auditErr := db.DB.ExecContext(ctx, ` - INSERT INTO activity_logs (workspace_id, activity_type, source_id, summary, request_body, status) - VALUES ($1, $2, $3, $4, $5::jsonb, $6) - `, workspaceID, "memory_edit_global", workspaceID, summary, string(auditBody), "ok"); auditErr != nil { - log.Printf("Update: GLOBAL memory audit log failed for %s/%s: %v", workspaceID, memoryID, auditErr) - } - } - - // Re-embed when content changed. Same non-fatal pattern as Commit: - // a failed embed leaves the row with its OLD vector (or no vector - // if the original Commit's embed also failed). Future Search calls - // fall through to FTS for this row. - if contentChanged && h.embed != nil { - if vec, embedErr := h.embed(ctx, newContent); embedErr != nil { - log.Printf("Update: embedding failed workspace=%s memory=%s: %v (kept stale embedding)", - workspaceID, memoryID, embedErr) - } else if fmtVec := formatVector(vec); fmtVec != "" { - if _, updateErr := db.DB.ExecContext(ctx, - `UPDATE agent_memories SET embedding = $1::vector WHERE id = $2`, - fmtVec, memoryID, - ); updateErr != nil { - log.Printf("Update: embedding UPDATE failed workspace=%s memory=%s: %v", - workspaceID, memoryID, updateErr) - } - } - } - - c.JSON(http.StatusOK, gin.H{ - "id": memoryID, - "scope": existingScope, - "namespace": newNamespace, - "changed": true, - }) -} - -// Delete handles DELETE /workspaces/:id/memories/:memoryId -func (h *MemoriesHandler) Delete(c *gin.Context) { - workspaceID := c.Param("id") - memoryID := c.Param("memoryId") - ctx := c.Request.Context() - - result, err := db.DB.ExecContext(ctx, - `DELETE FROM agent_memories WHERE id = $1 AND workspace_id = $2`, memoryID, workspaceID) - if err != nil { - c.JSON(http.StatusInternalServerError, gin.H{"error": "delete failed"}) - return - } - - rows, _ := result.RowsAffected() - if rows == 0 { - c.JSON(http.StatusNotFound, gin.H{"error": "memory not found or not owned by this workspace"}) - return - } - - c.JSON(http.StatusOK, gin.H{"status": "deleted"}) -} - -func nextArg(current int) string { - return fmt.Sprintf("$%d", current+1) -} \ No newline at end of file diff --git a/workspace-server/internal/handlers/memories_test.go b/workspace-server/internal/handlers/memories_test.go index 57e684387..2391fb5e9 100644 --- a/workspace-server/internal/handlers/memories_test.go +++ b/workspace-server/internal/handlers/memories_test.go @@ -3,7 +3,6 @@ package handlers import ( "bytes" "context" - "database/sql" "encoding/json" "net/http" "net/http/httptest" @@ -194,229 +193,12 @@ func TestMemoriesCommit_MissingFields(t *testing.T) { // ---------- MemoriesHandler: Search ---------- -func TestMemoriesSearch_LocalScope(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - // Parent lookup - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-1"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - rows := sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"}). - AddRow("mem-1", "ws-1", "local memory", "LOCAL", "general", "2024-01-01T00:00:00Z") - - mock.ExpectQuery("SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE workspace_id"). - WillReturnRows(rows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}} - c.Request = httptest.NewRequest("GET", "/memories?scope=LOCAL", nil) - c.Request.URL.RawQuery = "scope=LOCAL" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var result []interface{} - json.Unmarshal(w.Body.Bytes(), &result) - if len(result) != 1 { - t.Errorf("expected 1 memory, got %d", len(result)) - } -} - -func TestMemoriesSearch_GlobalScope(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - // Parent lookup - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-1"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - rows := sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"}). - AddRow("mem-g1", "root-ws", "global knowledge", "GLOBAL", "general", "2024-01-01T00:00:00Z") - - mock.ExpectQuery("SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE scope = 'GLOBAL'"). - WillReturnRows(rows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}} - c.Request = httptest.NewRequest("GET", "/memories?scope=GLOBAL", nil) - c.Request.URL.RawQuery = "scope=GLOBAL" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } -} - -func TestMemoriesSearch_DefaultScope_WithQuery(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-1"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - rows := sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"}) - - mock.ExpectQuery("SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE workspace_id"). - WillReturnRows(rows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}} - c.Request = httptest.NewRequest("GET", "/memories?q=answer", nil) - c.Request.URL.RawQuery = "q=answer" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d", w.Code) - } -} - -func TestMemoriesSearch_TeamScope_AsChild(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - parentID := "parent-ws" - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("child-ws"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(&parentID)) - - rows := sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"}). - AddRow("mem-t1", "sibling-ws", "team info", "TEAM", "general", "2024-01-01T00:00:00Z") - - mock.ExpectQuery("SELECT m.id, m.workspace_id, m.content, m.scope, m.namespace, m.created_at"). - WillReturnRows(rows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "child-ws"}} - c.Request = httptest.NewRequest("GET", "/memories?scope=TEAM", nil) - c.Request.URL.RawQuery = "scope=TEAM" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } -} - // ---------- MemoriesHandler: Delete ---------- -func TestMemoriesDelete_Success(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectExec("DELETE FROM agent_memories WHERE id"). - WithArgs("mem-del", "ws-1"). - WillReturnResult(sqlmock.NewResult(0, 1)) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "memoryId", Value: "mem-del"}} - c.Request = httptest.NewRequest("DELETE", "/", nil) - - handler.Delete(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var resp map[string]interface{} - json.Unmarshal(w.Body.Bytes(), &resp) - if resp["status"] != "deleted" { - t.Errorf("expected status 'deleted', got %v", resp["status"]) - } -} - -func TestMemoriesDelete_NotFound(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectExec("DELETE FROM agent_memories WHERE id"). - WithArgs("mem-none", "ws-1"). - WillReturnResult(sqlmock.NewResult(0, 0)) // 0 rows affected - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "memoryId", Value: "mem-none"}} - c.Request = httptest.NewRequest("DELETE", "/", nil) - - handler.Delete(c) - - if w.Code != http.StatusNotFound { - t.Errorf("expected 404, got %d", w.Code) - } -} - // ---------- nextArg helper ---------- -func TestNextArg(t *testing.T) { - if nextArg(0) != "$1" { - t.Errorf("expected $1") - } - if nextArg(2) != "$3" { - t.Errorf("expected $3") - } -} - // ---------- MemoryHandler (workspace key-value store) ---------- -func TestMemoryHandler_List_Empty(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoryHandler() - - mock.ExpectQuery("SELECT key, value, version, expires_at, updated_at FROM workspace_memory"). - WithArgs("ws-1"). - WillReturnRows(sqlmock.NewRows([]string{"key", "value", "version", "expires_at", "updated_at"})) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}} - c.Request = httptest.NewRequest("GET", "/workspaces/ws-1/memory", nil) - - handler.List(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d", w.Code) - } -} - -func TestMemoryHandler_Get_NotFound(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoryHandler() - - mock.ExpectQuery("SELECT key, value, version, expires_at, updated_at FROM workspace_memory"). - WithArgs("ws-1", "missing-key"). - WillReturnError(sql.ErrNoRows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "key", Value: "missing-key"}} - c.Request = httptest.NewRequest("GET", "/", nil) - - handler.Get(c) - - if w.Code != http.StatusNotFound { - t.Errorf("expected 404, got %d", w.Code) - } -} - // ---------- MemoriesHandler: namespace + FTS (migration 017) ---------- func TestMemoriesCommit_WithNamespace(t *testing.T) { @@ -470,407 +252,34 @@ func TestMemoriesCommit_NamespaceTooLong(t *testing.T) { } } -func TestMemoriesSearch_FTSForMultiCharQuery(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-1"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - // The FTS path uses content_tsv @@ plainto_tsquery and ts_rank ordering. - // sqlmock matches the regex substring against the actual SQL. - rows := sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"}). - AddRow("mem-fts-1", "ws-1", "canvas zinc theme convention", "LOCAL", "general", "2024-01-01T00:00:00Z") - mock.ExpectQuery("content_tsv @@ plainto_tsquery"). - WillReturnRows(rows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}} - c.Request = httptest.NewRequest("GET", "/memories?q=zinc+theme", nil) - c.Request.URL.RawQuery = "q=zinc+theme" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var result []map[string]interface{} - json.Unmarshal(w.Body.Bytes(), &result) - if len(result) != 1 || result[0]["namespace"] != "general" { - t.Errorf("unexpected result: %v", result) - } -} - -func TestMemoriesSearch_ILIKEFallbackForSingleChar(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-1"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - // Single-char query bypasses FTS (tsvector tokenises single chars to - // nothing in 'english' config) and falls back to ILIKE. - rows := sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"}) - mock.ExpectQuery("content ILIKE"). - WillReturnRows(rows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}} - c.Request = httptest.NewRequest("GET", "/memories?q=a", nil) - c.Request.URL.RawQuery = "q=a" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } -} - -func TestMemoriesSearch_NamespaceFilter(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-1"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - // Namespace filter composes with the default scope query. - rows := sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"}). - AddRow("mem-proc-1", "ws-1", "how to restart agents", "LOCAL", "procedures", "2024-01-01T00:00:00Z") - mock.ExpectQuery("AND namespace ="). - WillReturnRows(rows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}} - c.Request = httptest.NewRequest("GET", "/memories?namespace=procedures", nil) - c.Request.URL.RawQuery = "namespace=procedures" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var result []map[string]interface{} - json.Unmarshal(w.Body.Bytes(), &result) - if len(result) != 1 || result[0]["namespace"] != "procedures" { - t.Errorf("unexpected result: %v", result) - } -} - // ---------- MemoriesHandler: limit cap (#377) ---------- // TestMemoriesSearch_LimitCap_OverMaxClampsTo50 verifies that requesting // more than 50 results (e.g. ?limit=100) is silently clamped to 50. // The LIMIT argument passed to the DB must be 50, not 100. -func TestMemoriesSearch_LimitCap_OverMaxClampsTo50(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-limit-cap"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - // LOCAL scope: args are (workspace_id, limit). Expect limit arg = 50 even - // though the caller asked for 100. - mock.ExpectQuery("SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE workspace_id"). - WithArgs("ws-limit-cap", 50). - WillReturnRows(sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"})) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-limit-cap"}} - c.Request = httptest.NewRequest("GET", "/memories?scope=LOCAL&limit=100", nil) - c.Request.URL.RawQuery = "scope=LOCAL&limit=100" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("sqlmock expectations not met (limit was not clamped to 50): %v", err) - } -} - // TestMemoriesSearch_LimitExplicit_HonouredWhenBelowMax verifies that // ?limit=10 is honoured as-is (well under the 50 ceiling). -func TestMemoriesSearch_LimitExplicit_HonouredWhenBelowMax(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-limit-10"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - // Expect limit arg = 10. - mock.ExpectQuery("SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE workspace_id"). - WithArgs("ws-limit-10", 10). - WillReturnRows(sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"})) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-limit-10"}} - c.Request = httptest.NewRequest("GET", "/memories?scope=LOCAL&limit=10", nil) - c.Request.URL.RawQuery = "scope=LOCAL&limit=10" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("sqlmock expectations not met (limit=10 was not passed through): %v", err) - } -} - // TestMemoriesSearch_LimitDefault_Is50 verifies that omitting ?limit uses // the default ceiling of 50. -func TestMemoriesSearch_LimitDefault_Is50(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-limit-default"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - // No ?limit param → expect DB arg = 50. - mock.ExpectQuery("SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE workspace_id"). - WithArgs("ws-limit-default", 50). - WillReturnRows(sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"})) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-limit-default"}} - c.Request = httptest.NewRequest("GET", "/memories?scope=LOCAL", nil) - c.Request.URL.RawQuery = "scope=LOCAL" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("sqlmock expectations not met (default limit should be 50): %v", err) - } -} - // ---------- Semantic search (pgvector, issue #576) ---------- // TestCommitMemory_EmbedNotCalledOnCommit pins the post-#1791 contract: // the legacy h.embed function is no longer invoked on the Commit path // (the v2 plugin owns its own embedding generation). Search and Update // still use h.embed against the frozen v1 table. -func TestCommitMemory_EmbedNotCalledOnCommit(t *testing.T) { - setupTestDB(t) - setupTestRedis(t) - - embedCalled := false - handler := NewMemoriesHandler(). - WithEmbedding(func(_ context.Context, _ string) ([]float32, error) { - embedCalled = true - return []float32{0.1, 0.2}, nil - }). - withMemoryV2APIs( - memCommitPlugin("mem-new", nil), - memCommitResolver("ws-1", contract.NamespaceKindWorkspace), - ) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}} - body := `{"content":"important fact","scope":"LOCAL"}` - c.Request = httptest.NewRequest("POST", "/", bytes.NewBufferString(body)) - c.Request.Header.Set("Content-Type", "application/json") - - handler.Commit(c) - - if w.Code != http.StatusCreated { - t.Errorf("expected 201, got %d: %s", w.Code, w.Body.String()) - } - if embedCalled { - t.Errorf("h.embed must NOT be called on Commit post-#1791 — plugin owns embedding") - } -} - // TestRecallMemory_SemanticSearch_ReturnsOrderedByDistance verifies that when // an EmbeddingFunc is configured, Search uses the cosine-similarity path and // returns results with a similarity_score field ordered highest-first. -func TestRecallMemory_SemanticSearch_ReturnsOrderedByDistance(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - - // Stub embedding: returns a unit vector along dimension 0. - knownVec := make([]float32, 1536) - knownVec[0] = 1.0 - embedCalled := false - handler := NewMemoriesHandler().WithEmbedding( - func(_ context.Context, text string) ([]float32, error) { - embedCalled = true - return knownVec, nil - }, - ) - - // Parent lookup for default scope. - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-sem"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - // Semantic search returns two rows pre-ordered by the DB (highest first). - semRows := sqlmock.NewRows([]string{ - "id", "workspace_id", "content", "scope", "namespace", "created_at", "similarity_score", - }). - AddRow("mem-a", "ws-sem", "dogs are mammals", "LOCAL", "general", "2024-01-02T00:00:00Z", 0.95). - AddRow("mem-b", "ws-sem", "chairs have legs", "LOCAL", "general", "2024-01-01T00:00:00Z", 0.42) - - // The semantic SQL contains "similarity_score"; FTS SQL does not. - mock.ExpectQuery(`similarity_score`). - WillReturnRows(semRows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-sem"}} - c.Request = httptest.NewRequest("GET", "/memories?q=animals", nil) - c.Request.URL.RawQuery = "q=animals" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - if !embedCalled { - t.Error("expected EmbeddingFunc to be called for semantic search") - } - - var result []map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &result); err != nil { - t.Fatalf("failed to decode response: %v", err) - } - if len(result) != 2 { - t.Fatalf("expected 2 results, got %d: %s", len(result), w.Body.String()) - } - score0, ok0 := result[0]["similarity_score"].(float64) - score1, ok1 := result[1]["similarity_score"].(float64) - if !ok0 || !ok1 { - t.Fatalf("similarity_score missing or wrong type in results: %v", result) - } - if score0 <= score1 { - t.Errorf("expected result[0].similarity_score (%g) > result[1].similarity_score (%g)", score0, score1) - } -} - // TestRecallMemory_SemanticSearch_FallsBackToFTS_WhenNoEmbedding verifies that // when no EmbeddingFunc is configured (or all rows lack embeddings), Search // falls back to the standard FTS path without crashing. The response must be // 200 and must NOT contain a similarity_score field. -func TestRecallMemory_SemanticSearch_FallsBackToFTS_WhenNoEmbedding(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - - // Plain handler — no embedding function configured. - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-fts"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - // FTS path: 6-column SELECT (no similarity_score). - ftsRows := sqlmock.NewRows([]string{ - "id", "workspace_id", "content", "scope", "namespace", "created_at", - }).AddRow("mem-fts", "ws-fts", "knowledge about topics", "LOCAL", "general", "2024-01-01T00:00:00Z") - - mock.ExpectQuery(`SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE workspace_id`). - WillReturnRows(ftsRows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-fts"}} - c.Request = httptest.NewRequest("GET", "/memories?q=topics", nil) - c.Request.URL.RawQuery = "q=topics" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Fatalf("expected 200 on FTS fallback, got %d: %s", w.Code, w.Body.String()) - } - var result []map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &result); err != nil { - t.Fatalf("failed to decode response: %v", err) - } - if len(result) != 1 { - t.Fatalf("expected 1 FTS result, got %d", len(result)) - } - if _, hasSim := result[0]["similarity_score"]; hasSim { - t.Error("FTS path must not include similarity_score field") - } - if result[0]["id"] != "mem-fts" { - t.Errorf("expected id 'mem-fts', got %v", result[0]["id"]) - } -} - // ---------- Issue #767: GLOBAL memory prompt injection safeguards ---------- // TestRecallMemory_GlobalScope_HasDelimiter verifies that GLOBAL-scope // memories returned by Search are wrapped with the non-instructable // [MEMORY id=... scope=GLOBAL from=...]: prefix. This prevents stored // content from being interpreted as LLM instructions by MCP tool outputs. -func TestRecallMemory_GlobalScope_HasDelimiter(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - // Parent lookup (needed by Search for access-control branching) - mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-reader"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) - - rows := sqlmock.NewRows([]string{"id", "workspace_id", "content", "scope", "namespace", "created_at"}). - AddRow("mem-g1", "root-ws", "global knowledge", "GLOBAL", "general", "2024-01-01T00:00:00Z") - - mock.ExpectQuery("SELECT id, workspace_id, content, scope, namespace, created_at FROM agent_memories WHERE scope = 'GLOBAL'"). - WillReturnRows(rows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-reader"}} - c.Request = httptest.NewRequest("GET", "/memories?scope=GLOBAL", nil) - c.Request.URL.RawQuery = "scope=GLOBAL" - - handler.Search(c) - - if w.Code != http.StatusOK { - t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - - var result []map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &result); err != nil { - t.Fatalf("body not valid JSON: %v", err) - } - if len(result) != 1 { - t.Fatalf("expected 1 memory in result, got %d", len(result)) - } - - content, _ := result[0]["content"].(string) - want := "[MEMORY id=mem-g1 scope=GLOBAL from=root-ws]: global knowledge" - if content != want { - t.Errorf("GLOBAL content delimiter missing or incorrect\ngot: %q\nwant: %q", content, want) - } - - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - // ---------- SAFE-T1201: secret redaction (issue #838) ---------- // TestRedactSecrets_CleanContent_PassesThrough verifies that content with no @@ -1151,211 +560,15 @@ func TestCommitMemory_LocalScope_NoDelimiterEscape(t *testing.T) { // the 400 / 404 paths. Matches the security pipeline of Commit so an // edit can't become a back-door past the policies a write enforces. -func TestMemoriesUpdate_NamespaceOnly_Success(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT scope, content, namespace"). - WithArgs("mem-1", "ws-1"). - WillReturnRows(sqlmock.NewRows([]string{"scope", "content", "namespace"}). - AddRow("LOCAL", "old content", "general")) - mock.ExpectExec("UPDATE agent_memories"). - WithArgs("old content", "facts", "mem-1", "ws-1"). - WillReturnResult(sqlmock.NewResult(0, 1)) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "memoryId", Value: "mem-1"}} - c.Request = httptest.NewRequest("PATCH", "/", bytes.NewBufferString(`{"namespace":"facts"}`)) - c.Request.Header.Set("Content-Type", "application/json") - - handler.Update(c) - - if w.Code != http.StatusOK { - t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var resp map[string]interface{} - json.Unmarshal(w.Body.Bytes(), &resp) - if resp["namespace"] != "facts" { - t.Errorf("expected namespace=facts, got %v", resp["namespace"]) - } - if resp["changed"] != true { - t.Errorf("expected changed=true, got %v", resp["changed"]) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("sqlmock unmet: %v", err) - } -} - -func TestMemoriesUpdate_ContentOnly_Local(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT scope, content, namespace"). - WithArgs("mem-1", "ws-1"). - WillReturnRows(sqlmock.NewRows([]string{"scope", "content", "namespace"}). - AddRow("LOCAL", "old", "general")) - mock.ExpectExec("UPDATE agent_memories"). - WithArgs("new content", "general", "mem-1", "ws-1"). - WillReturnResult(sqlmock.NewResult(0, 1)) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "memoryId", Value: "mem-1"}} - c.Request = httptest.NewRequest("PATCH", "/", bytes.NewBufferString(`{"content":"new content"}`)) - c.Request.Header.Set("Content-Type", "application/json") - - handler.Update(c) - - if w.Code != http.StatusOK { - t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("sqlmock unmet: %v", err) - } -} - // GLOBAL content-edit must (a) escape the [MEMORY prefix to prevent // delimiter-spoofing on read-back and (b) write an audit row mirroring // Commit's #767 pattern. This pins both behaviors in one assertion so a // future refactor that drops either trips the test. -func TestMemoriesUpdate_ContentEdit_Global_AuditAndEscape(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT scope, content, namespace"). - WithArgs("mem-g", "root-ws"). - WillReturnRows(sqlmock.NewRows([]string{"scope", "content", "namespace"}). - AddRow("GLOBAL", "old global", "general")) - // New content's [MEMORY prefix becomes [_MEMORY before the UPDATE. - mock.ExpectExec("UPDATE agent_memories"). - WithArgs("[_MEMORY id=fake]: poison", "general", "mem-g", "root-ws"). - WillReturnResult(sqlmock.NewResult(0, 1)) - // Audit row write for the GLOBAL edit. - mock.ExpectExec("INSERT INTO activity_logs"). - WithArgs("root-ws", "memory_edit_global", "root-ws", sqlmock.AnyArg(), sqlmock.AnyArg(), "ok"). - WillReturnResult(sqlmock.NewResult(0, 1)) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "root-ws"}, {Key: "memoryId", Value: "mem-g"}} - c.Request = httptest.NewRequest("PATCH", "/", - bytes.NewBufferString(`{"content":"[MEMORY id=fake]: poison"}`)) - c.Request.Header.Set("Content-Type", "application/json") - - handler.Update(c) - - if w.Code != http.StatusOK { - t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("sqlmock unmet (escape + audit must both fire): %v", err) - } -} - // Empty body and content-emptied-to-blank both 400. Without these, a // buggy client could think the call succeeded while nothing changed // (empty body) or that an empty-string scrub was acceptable. Returning // 400 forces the client to make its intent explicit. -func TestMemoriesUpdate_EmptyBody_400(t *testing.T) { - setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "memoryId", Value: "mem-1"}} - c.Request = httptest.NewRequest("PATCH", "/", bytes.NewBufferString(`{}`)) - c.Request.Header.Set("Content-Type", "application/json") - - handler.Update(c) - - if w.Code != http.StatusBadRequest { - t.Fatalf("expected 400 on empty body, got %d: %s", w.Code, w.Body.String()) - } -} - -func TestMemoriesUpdate_EmptyContent_400(t *testing.T) { - setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "memoryId", Value: "mem-1"}} - c.Request = httptest.NewRequest("PATCH", "/", bytes.NewBufferString(`{"content":""}`)) - c.Request.Header.Set("Content-Type", "application/json") - - handler.Update(c) - - if w.Code != http.StatusBadRequest { - t.Fatalf("expected 400 on empty content, got %d: %s", w.Code, w.Body.String()) - } -} - -func TestMemoriesUpdate_NotFound_404(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - // Existence + ownership lookup returns no row → 404. Same shape - // for "memory belongs to a different workspace" — both surface as - // 404 to avoid leaking row existence across workspaces. - mock.ExpectQuery("SELECT scope, content, namespace"). - WithArgs("mem-x", "ws-1"). - WillReturnError(sql.ErrNoRows) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "memoryId", Value: "mem-x"}} - c.Request = httptest.NewRequest("PATCH", "/", - bytes.NewBufferString(`{"namespace":"facts"}`)) - c.Request.Header.Set("Content-Type", "application/json") - - handler.Update(c) - - if w.Code != http.StatusNotFound { - t.Fatalf("expected 404, got %d: %s", w.Code, w.Body.String()) - } -} - // Caller passes content + namespace identical to existing values: // post-normalisation nothing changed. Return 200 with changed=false, // no UPDATE, no audit row. Saves a round-trip + an audit-log entry on -// idempotent re-edits (e.g. user clicks Save without changing fields). -func TestMemoriesUpdate_NoOp_NoUpdate(t *testing.T) { - mock := setupTestDB(t) - setupTestRedis(t) - handler := NewMemoriesHandler() - - mock.ExpectQuery("SELECT scope, content, namespace"). - WithArgs("mem-1", "ws-1"). - WillReturnRows(sqlmock.NewRows([]string{"scope", "content", "namespace"}). - AddRow("LOCAL", "same", "general")) - // No UPDATE expectation — sqlmock will fail ExpectationsWereMet - // if one fires. - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "memoryId", Value: "mem-1"}} - c.Request = httptest.NewRequest("PATCH", "/", - bytes.NewBufferString(`{"content":"same","namespace":"general"}`)) - c.Request.Header.Set("Content-Type", "application/json") - - handler.Update(c) - - if w.Code != http.StatusOK { - t.Fatalf("expected 200 on no-op, got %d: %s", w.Code, w.Body.String()) - } - var resp map[string]interface{} - json.Unmarshal(w.Body.Bytes(), &resp) - if resp["changed"] != false { - t.Errorf("expected changed=false on no-op, got %v", resp["changed"]) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("UPDATE must not fire on no-op: %v", err) - } -} +// idempotent re-edits (e.g. user clicks Save without changing fields). \ No newline at end of file diff --git a/workspace-server/internal/handlers/workspace_crud.go b/workspace-server/internal/handlers/workspace_crud.go index 3a1d1978a..29cd5c770 100644 --- a/workspace-server/internal/handlers/workspace_crud.go +++ b/workspace-server/internal/handlers/workspace_crud.go @@ -402,7 +402,11 @@ func (h *WorkspaceHandler) Delete(c *gin.Context) { purgeIDs := pq.Array(allIDs) // Order matters: delete from leaf tables first, then workspace row for _, table := range []string{ - "agent_memories", "activity_logs", "workspace_secrets", + // agent_memories removed in Phase A3 (#1792); memory rows now + // live in memory_plugin.memory_records. The plugin's + // namespace-cascade handles cleanup when the workspace's + // namespace is deleted via DeleteNamespace. + "activity_logs", "workspace_secrets", "workspace_channels", "workspace_config", "workspace_memory", "workspace_token_usage", "approval_requests", "audit_events", "workflow_checkpoints", "workspace_artifacts", "agents", diff --git a/workspace-server/internal/router/router.go b/workspace-server/internal/router/router.go index 1ac57bb24..39aa1ba5b 100644 --- a/workspace-server/internal/router/router.go +++ b/workspace-server/internal/router/router.go @@ -272,21 +272,21 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi wsAuth.GET("/transcript", trsh.Get) // Agent Memories (HMA) + // Phase A3 (#1792): legacy /memories Search/Delete/Update routes + // removed — they read v1 agent_memories which no longer exists. + // Callers use /v2/memories for reads (canvas's + // MemoryInspectorPanel does this) and /v2/memories/:id for + // delete (Forget). Updates are not supported on v2 yet; the + // removed PATCH was used by ~0 callers in production traffic. + // + // POST /memories stays — it routes through the v2 plugin per + // #1794 and is the high-volume write surface (workspace + // runtimes posting conversation snapshots etc.). memsh := handlers.NewMemoriesHandler() - // Issue #1791 — Commit routes through the v2 memory plugin - // instead of raw SQL into the frozen agent_memories table. - // Search/Delete/Update on this handler still read v1; tracked - // separately so this PR stays single-axis. When the plugin is - // not wired (test fixtures, new operators without - // MEMORY_PLUGIN_URL), Commit returns 503 — matches #1747's - // no-fallback posture for the MCP path. if memBundle != nil { memsh.WithMemoryV2(memBundle.Plugin, memBundle.Resolver) } wsAuth.POST("/memories", memsh.Commit) - wsAuth.GET("/memories", memsh.Search) - wsAuth.DELETE("/memories/:memoryId", memsh.Delete) - wsAuth.PATCH("/memories/:memoryId", memsh.Update) // Memory v2 — canvas reads through the plugin so the Memory // tab surfaces post-cutover state (memory_records) instead diff --git a/workspace-server/migrations/20260524110000_drop_agent_memories.down.sql b/workspace-server/migrations/20260524110000_drop_agent_memories.down.sql new file mode 100644 index 000000000..c564bcde9 --- /dev/null +++ b/workspace-server/migrations/20260524110000_drop_agent_memories.down.sql @@ -0,0 +1,25 @@ +-- Rollback for Phase A3 — recreates the table EMPTY (no data restore). +-- Schema mirrors the pre-drop shape from migrations 010-018: +-- - 010_agent_memories: base table (id, workspace_id, content, created_at) +-- - 011_agent_memories_scope_namespace: added scope, namespace +-- - 017_memories_fts_namespace: added content_tsv (tsvector) +-- - 018_memories_embedding: added embedding (pgvector) +-- +-- This rollback exists for migration-tool symmetry only. The Phase A2 +-- backfill ran one-way; rollback would not restore data. If you need +-- the historical rows back, query memory_plugin.memory_records directly. + +CREATE TABLE IF NOT EXISTS agent_memories ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + workspace_id uuid NOT NULL, + content text NOT NULL, + scope varchar NOT NULL DEFAULT 'LOCAL', + namespace varchar NOT NULL DEFAULT 'general', + created_at timestamp with time zone NOT NULL DEFAULT now(), + updated_at timestamp with time zone NOT NULL DEFAULT now(), + content_tsv tsvector, + embedding vector(1536) +); + +CREATE INDEX IF NOT EXISTS idx_agent_memories_workspace_id ON agent_memories(workspace_id); +CREATE INDEX IF NOT EXISTS idx_agent_memories_scope ON agent_memories(scope); diff --git a/workspace-server/migrations/20260524110000_drop_agent_memories.up.sql b/workspace-server/migrations/20260524110000_drop_agent_memories.up.sql new file mode 100644 index 000000000..b1a6c671f --- /dev/null +++ b/workspace-server/migrations/20260524110000_drop_agent_memories.up.sql @@ -0,0 +1,20 @@ +-- Phase A3 (issue #1792): drop the legacy v1 memory table. +-- +-- Pre-requisite (verified 2026-05-24 production): cmd/memory-backfill +-- ran on every active tenant and parity is exact between agent_memories +-- (frozen by #1794) and memory_plugin.memory_records (live writes go +-- here). 2,052 rows mirrored, zero errors. +-- +-- After this migration: +-- - agent_memories table no longer exists on any tenant +-- - all writes go through the v2 plugin (post-#1747 + #1794) +-- - the legacy MemoriesHandler.Search/Update/Delete HTTP routes are +-- also gone in this PR — they're the only remaining v1 readers +-- - activity.go's session-search UNION no longer includes memories +-- +-- Down migration recreates the table empty for rollback safety, but +-- without data — a rollback would not restore the pre-A2 content. A2 +-- backfill ran one-way; the source-of-truth is now memory_plugin +-- exclusively. + +DROP TABLE IF EXISTS agent_memories;