diff --git a/workspace-server/entrypoint-tenant.sh b/workspace-server/entrypoint-tenant.sh index 0f2d6ddef..3c380efd8 100644 --- a/workspace-server/entrypoint-tenant.sh +++ b/workspace-server/entrypoint-tenant.sh @@ -23,19 +23,28 @@ CANVAS_PID=$! # Memory v2 sidecar (built-in postgres plugin). See Dockerfile entrypoint # comment for rationale. # -# Spawn-gating: only start the sidecar when the operator has indicated -# they want it (MEMORY_V2_CUTOVER=true OR MEMORY_PLUGIN_URL set). -# Without that signal, the sidecar adds zero value and risks aborting -# tenant boot via the 30s health gate when the tenant Postgres lacks +# Spawn-gating: start the sidecar when MEMORY_PLUGIN_URL is set. +# Without it, the sidecar adds zero value and risks aborting tenant +# boot via the 30s health gate when the tenant Postgres lacks # pgvector. Caught on staging redeploy 2026-05-05: # pq: extension "vector" is not available # # 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. MEMORY_PLUGIN_PID="" memory_plugin_wanted="" -if [ "$MEMORY_V2_CUTOVER" = "true" ] || [ -n "$MEMORY_PLUGIN_URL" ]; then +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 : "${MEMORY_PLUGIN_DATABASE_URL:=$DATABASE_URL}" diff --git a/workspace-server/internal/handlers/admin_memories.go b/workspace-server/internal/handlers/admin_memories.go index d8a1e828a..380d877f9 100644 --- a/workspace-server/internal/handlers/admin_memories.go +++ b/workspace-server/internal/handlers/admin_memories.go @@ -5,7 +5,6 @@ import ( "database/sql" "log" "net/http" - "os" "strings" "time" @@ -16,19 +15,12 @@ import ( "github.com/gin-gonic/gin" ) -// envMemoryV2Cutover gates whether admin export/import routes through -// the v2 plugin (PR-8 / RFC #2728). When unset, the legacy direct-DB -// path runs unchanged so operators who haven't enabled the plugin -// keep working. -const envMemoryV2Cutover = "MEMORY_V2_CUTOVER" - // AdminMemoriesHandler provides bulk export/import of agent memories for // backup and restore across Docker rebuilds (issue #1051). // -// PR-8 (RFC #2728): when wired with the v2 plugin via WithMemoryV2 AND -// MEMORY_V2_CUTOVER is true, export reads from the plugin's namespaces -// and import writes through the plugin. Both paths preserve the -// SAFE-T1201 redaction shipped in F1084 + F1085. +// Issue #1733: the v2 plugin is the only supported backend. Export +// reads from the plugin's namespaces; import writes through the plugin. +// Both paths preserve the SAFE-T1201 redaction shipped in F1084 + F1085. type AdminMemoriesHandler struct { plugin adminMemoriesPlugin resolver adminMemoriesResolver @@ -69,12 +61,12 @@ func (h *AdminMemoriesHandler) withMemoryV2APIs(plugin adminMemoriesPlugin, reso return h } -// cutoverActive reports whether the export/import path should route -// through the v2 plugin. -func (h *AdminMemoriesHandler) cutoverActive() bool { - if os.Getenv(envMemoryV2Cutover) != "true" { - return false - } +// memoryV2Wired reports whether the v2 plugin + resolver are attached. +// Issue #1733: v2 is now the only path; this replaces the prior +// cutoverActive() gate (which also checked MEMORY_V2_CUTOVER=true) — +// the env-flag double-check is gone since there's no legacy fallback +// to choose against. +func (h *AdminMemoriesHandler) memoryV2Wired() bool { return h.plugin != nil && h.resolver != nil } @@ -97,48 +89,19 @@ type memoryExportEntry struct { // before returning so that any credentials stored before SAFE-T1201 (#838) // was applied do not leak out via the admin export endpoint. // -// CUTOVER (PR-8 / RFC #2728): when MEMORY_V2_CUTOVER=true and the v2 -// plugin is wired, reads from the plugin instead of agent_memories. +// Issue #1733: reads exclusively from the v2 plugin. The legacy direct +// agent_memories scan is gone — operators without a configured plugin +// get a 503 explaining the required setup. func (h *AdminMemoriesHandler) Export(c *gin.Context) { ctx := c.Request.Context() - if h.cutoverActive() { - h.exportViaPlugin(c, ctx) + if !h.memoryV2Wired() { + c.JSON(http.StatusServiceUnavailable, gin.H{ + "error": "memory plugin is not configured (set MEMORY_PLUGIN_URL)", + }) return } - - rows, err := db.DB.QueryContext(ctx, ` - SELECT am.id, am.content, am.scope, am.namespace, am.created_at, - w.name AS workspace_name - FROM agent_memories am - JOIN workspaces w ON am.workspace_id = w.id - ORDER BY am.created_at - `) - if err != nil { - log.Printf("admin/memories/export: query error: %v", err) - c.JSON(http.StatusInternalServerError, gin.H{"error": "export query failed"}) - return - } - defer rows.Close() - - memories := make([]memoryExportEntry, 0) - for rows.Next() { - var m memoryExportEntry - if err := rows.Scan(&m.ID, &m.Content, &m.Scope, &m.Namespace, &m.CreatedAt, &m.WorkspaceName); err != nil { - log.Printf("admin/memories/export: scan error: %v", err) - continue - } - // F1084 / #1131: redact secrets before returning so pre-SAFE-T1201 - // memories (stored before redactSecrets was mandatory) don't leak. - redacted, _ := redactSecrets(m.WorkspaceName, m.Content) - m.Content = redacted - memories = append(memories, m) - } - if err := rows.Err(); err != nil { - log.Printf("admin/memories/export: rows error: %v", err) - } - - c.JSON(http.StatusOK, memories) + h.exportViaPlugin(c, ctx) } // memoryImportEntry is the JSON shape accepted on import. Matches export format. @@ -160,8 +123,9 @@ type memoryImportEntry struct { // with embedded credentials cannot land unredacted in agent_memories (SAFE-T1201 // parity with the commit_memory MCP bridge path). // -// CUTOVER (PR-8 / RFC #2728): when MEMORY_V2_CUTOVER=true and the v2 -// plugin is wired, writes through the plugin instead of agent_memories. +// Issue #1733: writes exclusively through the v2 plugin. The legacy +// direct agent_memories insert path is gone — operators without a +// configured plugin get a 503 explaining the required setup. func (h *AdminMemoriesHandler) Import(c *gin.Context) { ctx := c.Request.Context() @@ -171,85 +135,13 @@ func (h *AdminMemoriesHandler) Import(c *gin.Context) { return } - if h.cutoverActive() { - h.importViaPlugin(c, ctx, entries) + if !h.memoryV2Wired() { + c.JSON(http.StatusServiceUnavailable, gin.H{ + "error": "memory plugin is not configured (set MEMORY_PLUGIN_URL)", + }) return } - - imported := 0 - skipped := 0 - errors := 0 - - for _, entry := range entries { - // 1. Resolve workspace by name - var workspaceID string - err := db.DB.QueryRowContext(ctx, - `SELECT id FROM workspaces WHERE name = $1 LIMIT 1`, - entry.WorkspaceName, - ).Scan(&workspaceID) - if err != nil { - log.Printf("admin/memories/import: workspace %q not found, skipping", entry.WorkspaceName) - skipped++ - continue - } - - // F1085 / #1132: scrub credential patterns before persistence so that - // imported memories with secrets don't bypass SAFE-T1201 (#838). - // Must run BEFORE the dedup check so the redacted content is what - // gets stored — otherwise re-importing the same backup would produce - // a duplicate with different (original, unredacted) content. - content, _ := redactSecrets(workspaceID, entry.Content) - - // 2. Check for duplicate (same workspace + content + scope) using - // the redacted content so that two backups with the same original - // secret (same placeholder output) are treated as duplicates. - var exists bool - - err = db.DB.QueryRowContext(ctx, - `SELECT EXISTS(SELECT 1 FROM agent_memories WHERE workspace_id = $1 AND content = $2 AND scope = $3)`, - workspaceID, content, entry.Scope, - ).Scan(&exists) - if err != nil { - log.Printf("admin/memories/import: duplicate check error for workspace %q: %v", entry.WorkspaceName, err) - errors++ - continue - } - if exists { - skipped++ - continue - } - - // 3. Insert the memory, preserving original created_at if provided - namespace := entry.Namespace - if namespace == "" { - namespace = "general" - } - - if entry.CreatedAt != "" { - _, err = db.DB.ExecContext(ctx, - `INSERT INTO agent_memories (workspace_id, content, scope, namespace, created_at) VALUES ($1, $2, $3, $4, $5)`, - workspaceID, content, entry.Scope, namespace, entry.CreatedAt, - ) - } else { - _, err = db.DB.ExecContext(ctx, - `INSERT INTO agent_memories (workspace_id, content, scope, namespace) VALUES ($1, $2, $3, $4)`, - workspaceID, content, entry.Scope, namespace, - ) - } - if err != nil { - log.Printf("admin/memories/import: insert error for workspace %q: %v", entry.WorkspaceName, err) - errors++ - continue - } - imported++ - } - - c.JSON(http.StatusOK, gin.H{ - "imported": imported, - "skipped": skipped, - "errors": errors, - "total": len(entries), - }) + h.importViaPlugin(c, ctx, entries) } // exportViaPlugin reads memories from the v2 plugin and emits them in diff --git a/workspace-server/internal/handlers/admin_memories_cutover_test.go b/workspace-server/internal/handlers/admin_memories_cutover_test.go index ee2b6c26e..879ade233 100644 --- a/workspace-server/internal/handlers/admin_memories_cutover_test.go +++ b/workspace-server/internal/handlers/admin_memories_cutover_test.go @@ -101,26 +101,24 @@ func installMockDB(t *testing.T) sqlmock.Sqlmock { return mock } -// --- cutoverActive --- +// --- memoryV2Wired --- -func TestCutoverActive(t *testing.T) { +func TestMemoryV2Wired(t *testing.T) { cases := []struct { name string - envVal string plugin adminMemoriesPlugin resolver adminMemoriesResolver want bool }{ - {"env unset", "", &stubAdminPlugin{}, adminRootResolver(), false}, - {"env true but unwired", "true", nil, nil, false}, - {"env false", "false", &stubAdminPlugin{}, adminRootResolver(), false}, - {"env true wired", "true", &stubAdminPlugin{}, adminRootResolver(), true}, + {"both nil", nil, nil, false}, + {"plugin only", &stubAdminPlugin{}, nil, false}, + {"resolver only", nil, adminRootResolver(), false}, + {"both wired", &stubAdminPlugin{}, adminRootResolver(), true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - t.Setenv(envMemoryV2Cutover, tc.envVal) h := &AdminMemoriesHandler{plugin: tc.plugin, resolver: tc.resolver} - if got := h.cutoverActive(); got != tc.want { + if got := h.memoryV2Wired(); got != tc.want { t.Errorf("got %v, want %v", got, tc.want) } }) @@ -147,7 +145,6 @@ func TestWithMemoryV2APIs_AttachesDeps(t *testing.T) { // --- Export via plugin --- func TestExport_RoutesThroughPluginWhenCutoverActive(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("WITH RECURSIVE chain"). @@ -191,7 +188,6 @@ func TestExport_RoutesThroughPluginWhenCutoverActive(t *testing.T) { } func TestExport_DeduplicatesByMemoryID(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) // Two workspaces, both will see the same team-shared memory. @@ -222,7 +218,6 @@ func TestExport_DeduplicatesByMemoryID(t *testing.T) { } func TestExport_SkipsWorkspaceWhenResolverFails(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("WITH RECURSIVE chain"). WillReturnRows(sqlmock.NewRows([]string{"id", "name", "root_id"}). @@ -244,7 +239,6 @@ func TestExport_SkipsWorkspaceWhenResolverFails(t *testing.T) { } func TestExport_SkipsWorkspaceWhenPluginSearchFails(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("WITH RECURSIVE chain"). WillReturnRows(sqlmock.NewRows([]string{"id", "name", "root_id"}). @@ -268,7 +262,6 @@ func TestExport_SkipsWorkspaceWhenPluginSearchFails(t *testing.T) { } func TestExport_WorkspacesQueryFails(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("WITH RECURSIVE chain"). WillReturnError(errors.New("db dead")) @@ -287,7 +280,6 @@ func TestExport_WorkspacesQueryFails(t *testing.T) { } func TestExport_EmptyReadable(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("WITH RECURSIVE chain"). WillReturnRows(sqlmock.NewRows([]string{"id", "name", "root_id"}). @@ -309,7 +301,6 @@ func TestExport_EmptyReadable(t *testing.T) { } func TestExport_RedactsSecretsInPluginPath(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("WITH RECURSIVE chain"). WillReturnRows(sqlmock.NewRows([]string{"id", "name", "root_id"}). @@ -337,7 +328,6 @@ func TestExport_RedactsSecretsInPluginPath(t *testing.T) { // --- Import via plugin --- func TestImport_RoutesThroughPluginWhenCutoverActive(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("SELECT id::text FROM workspaces"). WithArgs("alpha"). @@ -368,7 +358,6 @@ func TestImport_RoutesThroughPluginWhenCutoverActive(t *testing.T) { } func TestImport_SkipsUnknownWorkspace(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("SELECT id::text FROM workspaces"). WithArgs("ghost"). @@ -395,7 +384,6 @@ func TestImport_SkipsUnknownWorkspace(t *testing.T) { } func TestImport_PluginUpsertNamespaceError(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("SELECT id::text FROM workspaces"). WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("root-1")) @@ -425,7 +413,6 @@ func TestImport_PluginUpsertNamespaceError(t *testing.T) { } func TestImport_PluginCommitError(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("SELECT id::text FROM workspaces"). WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("root-1")) @@ -455,7 +442,6 @@ func TestImport_PluginCommitError(t *testing.T) { } func TestImport_RedactsBeforePluginSeesContent(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("SELECT id::text FROM workspaces"). WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("root-1")) @@ -482,7 +468,6 @@ func TestImport_RedactsBeforePluginSeesContent(t *testing.T) { } func TestImport_SkipsUnknownScope(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("SELECT id::text FROM workspaces"). WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("root-1")) @@ -508,7 +493,6 @@ func TestImport_SkipsUnknownScope(t *testing.T) { } func TestImport_SkipsWhenResolverErrors(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("SELECT id::text FROM workspaces"). WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("root-1")) @@ -545,7 +529,6 @@ func TestImport_SkipsWhenResolverErrors(t *testing.T) { // + org:root-1. (Children's workspace: namespaces must be // included or admin export silently drops their private memories.) func TestExport_BatchesPluginCallsByRoot(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("WITH RECURSIVE chain"). @@ -605,7 +588,6 @@ func (r perWorkspaceResolver) WritableNamespaces(_ context.Context, ws string) ( // workspace:rootID + team:rootID + org:rootID — every child workspace's // private memories were silently dropped from admin export. func TestExport_IncludesEveryMembersPrivateNamespace(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "true") mock := installMockDB(t) mock.ExpectQuery("WITH RECURSIVE chain"). @@ -775,25 +757,43 @@ func TestSkipImport_ErrorMessage(t *testing.T) { } } -// --- Confirm legacy paths still work when env is unset --- +// --- 503 when plugin is not wired (issue #1733) --- +// +// The legacy SQL-backed Export/Import path was removed; both endpoints +// now respond 503 with a clear hint when v2 isn't configured. -func TestExport_LegacyPathWhenCutoverInactive(t *testing.T) { - t.Setenv(envMemoryV2Cutover, "") - mock := installMockDB(t) - mock.ExpectQuery("SELECT am.id, am.content, am.scope, am.namespace"). - WillReturnRows(sqlmock.NewRows([]string{"id", "content", "scope", "namespace", "created_at", "workspace_name"})) - - h := NewAdminMemoriesHandler().withMemoryV2APIs(&stubAdminPlugin{}, adminRootResolver()) +func TestExport_503WhenPluginNotWired(t *testing.T) { + installMockDB(t) + h := NewAdminMemoriesHandler() // no WithMemoryV2 → plugin nil gin.SetMode(gin.TestMode) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("GET", "/admin/memories/export", nil) h.Export(c) - if w.Code != http.StatusOK { - t.Errorf("code = %d body=%s", w.Code, w.Body.String()) + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("code = %d body=%s", w.Code, w.Body.String()) } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("legacy SQL path not exercised: %v", err) + if !strings.Contains(w.Body.String(), "MEMORY_PLUGIN_URL") { + t.Errorf("body must hint at MEMORY_PLUGIN_URL: %s", w.Body.String()) + } +} + +func TestImport_503WhenPluginNotWired(t *testing.T) { + installMockDB(t) + h := NewAdminMemoriesHandler() + gin.SetMode(gin.TestMode) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest("POST", "/admin/memories/import", + bytes.NewBufferString(`[]`)) + c.Request.Header.Set("Content-Type", "application/json") + h.Import(c) + + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("code = %d body=%s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "MEMORY_PLUGIN_URL") { + t.Errorf("body must hint at MEMORY_PLUGIN_URL: %s", w.Body.String()) } } diff --git a/workspace-server/internal/handlers/admin_memories_test.go b/workspace-server/internal/handlers/admin_memories_test.go index b28811905..a57eebcfc 100644 --- a/workspace-server/internal/handlers/admin_memories_test.go +++ b/workspace-server/internal/handlers/admin_memories_test.go @@ -2,220 +2,46 @@ package handlers import ( "bytes" - "database/sql" - "encoding/json" "net/http" "net/http/httptest" "testing" - "time" - "github.com/DATA-DOG/go-sqlmock" "github.com/gin-gonic/gin" ) -// newAdminMemoriesHandler is a test helper that returns an AdminMemoriesHandler. -func newAdminMemoriesHandler() *AdminMemoriesHandler { - return NewAdminMemoriesHandler() -} - -// adminPost builds a POST /admin/memories/import request. -func adminPost(t *testing.T, h *AdminMemoriesHandler, body interface{}) *httptest.ResponseRecorder { - t.Helper() - b, _ := json.Marshal(body) - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Request = httptest.NewRequest("POST", "/admin/memories/import", bytes.NewReader(b)) - c.Request.Header.Set("Content-Type", "application/json") - h.Import(c) - return w -} - -// adminGet builds a GET /admin/memories/export request. -func adminGet(t *testing.T, h *AdminMemoriesHandler) *httptest.ResponseRecorder { - t.Helper() - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Request = httptest.NewRequest("GET", "/admin/memories/export", nil) - h.Export(c) - return w -} - -// ───────────────────────────────────────────────────────────────────────────── -// Export tests -// ───────────────────────────────────────────────────────────────────────────── - -func TestAdminMemories_Export_Success(t *testing.T) { - mock := setupTestDB(t) - h := newAdminMemoriesHandler() - - now := time.Now().UTC().Truncate(time.Second) - rows := sqlmock.NewRows([]string{"id", "content", "scope", "namespace", "created_at", "workspace_name"}). - AddRow("mem-1", "hello world", "LOCAL", "ws-1", now, "my-workspace"). - AddRow("mem-2", "another fact", "TEAM", "ws-1", now, "my-workspace") - - mock.ExpectQuery("SELECT am.id, am.content, am.scope, am.namespace, am.created_at,"). - WillReturnRows(rows) - - w := adminGet(t, h) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - - var memories []map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &memories); err != nil { - t.Fatalf("failed to parse response: %v", err) - } - if len(memories) != 2 { - t.Errorf("expected 2 memories, got %d", len(memories)) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -func TestAdminMemories_Export_Empty(t *testing.T) { - mock := setupTestDB(t) - h := newAdminMemoriesHandler() - - rows := sqlmock.NewRows([]string{"id", "content", "scope", "namespace", "created_at", "workspace_name"}) - mock.ExpectQuery("SELECT am.id, am.content, am.scope, am.namespace, am.created_at,"). - WillReturnRows(rows) - - w := adminGet(t, h) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var memories []interface{} - if err := json.Unmarshal(w.Body.Bytes(), &memories); err != nil { - t.Fatalf("failed to parse response: %v", err) - } - if len(memories) != 0 { - t.Errorf("expected 0 memories, got %d", len(memories)) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -func TestAdminMemories_Export_QueryError(t *testing.T) { - mock := setupTestDB(t) - h := newAdminMemoriesHandler() - - mock.ExpectQuery("SELECT am.id, am.content, am.scope, am.namespace, am.created_at,"). - WillReturnError(sql.ErrConnDone) - - w := adminGet(t, h) - - if w.Code != http.StatusInternalServerError { - t.Errorf("expected 500, got %d: %s", w.Code, w.Body.String()) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -func TestAdminMemories_Export_RedactsSecrets(t *testing.T) { - mock := setupTestDB(t) - h := newAdminMemoriesHandler() - - // Content with a secret pattern. Export must call redactSecrets and return - // the redacted form, not the raw credential. - secretContent := "Remember to use OPENAI_API_KEY=sk-1234567890abcdefgh for the model" - redacted, _ := redactSecrets("my-workspace", secretContent) - - now := time.Now().UTC().Truncate(time.Second) - rows := sqlmock.NewRows([]string{"id", "content", "scope", "namespace", "created_at", "workspace_name"}). - AddRow("mem-secret", secretContent, "LOCAL", "my-workspace", now, "my-workspace") - - mock.ExpectQuery("SELECT am.id, am.content, am.scope, am.namespace, am.created_at,"). - WillReturnRows(rows) - - w := adminGet(t, h) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - - var memories []map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &memories); err != nil { - t.Fatalf("failed to parse response: %v", err) - } - if len(memories) != 1 { - t.Fatalf("expected 1 memory, got %d", len(memories)) - } - // The exported content must be the REDACTED version, not the raw secret. - if content, ok := memories[0]["content"].(string); ok { - if content == secretContent { - t.Errorf("Export returned raw secret %q — F1084 regression: redactSecrets not called", secretContent) - } - if content != redacted { - t.Errorf("Export content = %q, want redacted %q", content, redacted) - } - // Confirm the redacted version doesn't contain the raw key fragment. - if len(content) > 10 && content == "OPENAI_API_KEY=[REDACTED:" { - t.Errorf("redaction appears incomplete: %q", content) - } - } - - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -// ───────────────────────────────────────────────────────────────────────────── -// Import tests -// ───────────────────────────────────────────────────────────────────────────── - -func TestAdminMemories_Import_Success(t *testing.T) { - mock := setupTestDB(t) - h := newAdminMemoriesHandler() - - // Workspace lookup returns one row. - mock.ExpectQuery("SELECT id FROM workspaces WHERE name = \\$1"). - WithArgs("my-workspace"). - WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("ws-uuid-1")) - - // Duplicate check returns false. - mock.ExpectQuery("SELECT EXISTS"). - WithArgs("ws-uuid-1", sqlmock.AnyArg(), "LOCAL"). - WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(false)) - - // Insert succeeds. Handler uses 4-arg INSERT when created_at is absent. - mock.ExpectExec("INSERT INTO agent_memories"). - WithArgs("ws-uuid-1", sqlmock.AnyArg(), "LOCAL", "general"). - WillReturnResult(sqlmock.NewResult(1, 1)) - - w := adminPost(t, h, []map[string]interface{}{ - { - "content": "important fact", - "scope": "LOCAL", - "workspace_name": "my-workspace", - }, - }) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var resp map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { - t.Fatalf("failed to parse response: %v", err) - } - if resp["imported"].(float64) != 1 { - t.Errorf("imported = %v, want 1", resp["imported"]) - } - if resp["skipped"].(float64) != 0 { - t.Errorf("skipped = %v, want 0", resp["skipped"]) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} +// Issue #1733: every legacy SQL-path test in this file was removed when +// the v1 fallback was deleted from AdminMemoriesHandler. The v2-plugin +// coverage (the only path now) lives in admin_memories_cutover_test.go: +// +// - TestExport_RoutesThroughPluginWhenCutoverActive +// - TestExport_DeduplicatesByMemoryID +// - TestExport_SkipsWorkspaceWhenResolverFails +// - TestExport_SkipsWorkspaceWhenPluginSearchFails +// - TestExport_WorkspacesQueryFails +// - TestExport_EmptyReadable +// - TestExport_RedactsSecretsInPluginPath +// - TestExport_BatchesPluginCallsByRoot +// - TestExport_IncludesEveryMembersPrivateNamespace +// - TestImport_RoutesThroughPluginWhenCutoverActive +// - TestImport_SkipsUnknownWorkspace +// - TestImport_PluginUpsertNamespaceError +// - TestImport_PluginCommitError +// - TestImport_RedactsBeforePluginSeesContent +// - TestImport_SkipsUnknownScope +// - TestImport_SkipsWhenResolverErrors +// - TestExport_503WhenPluginNotWired (new in A1) +// - TestImport_503WhenPluginNotWired (new in A1) +// +// Only the JSON-envelope rejection test stays here because it runs +// before the plugin gate. +// TestAdminMemories_Import_InvalidJSON verifies that a malformed +// payload is rejected with HTTP 400 before any plugin or DB call is +// attempted. This guards the request-decode path independent of the +// memory backend choice. func TestAdminMemories_Import_InvalidJSON(t *testing.T) { _ = setupTestDB(t) - h := newAdminMemoriesHandler() + h := NewAdminMemoriesHandler() w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -227,175 +53,3 @@ func TestAdminMemories_Import_InvalidJSON(t *testing.T) { t.Errorf("expected 400, got %d: %s", w.Code, w.Body.String()) } } - -func TestAdminMemories_Import_WorkspaceNotFound_SkipsEntry(t *testing.T) { - mock := setupTestDB(t) - h := newAdminMemoriesHandler() - - // Workspace lookup returns no rows. - mock.ExpectQuery("SELECT id FROM workspaces WHERE name = \\$1"). - WithArgs("ghost-workspace"). - WillReturnError(sql.ErrNoRows) - - w := adminPost(t, h, []map[string]interface{}{ - { - "content": "some fact", - "scope": "LOCAL", - "workspace_name": "ghost-workspace", - }, - }) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var resp map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { - t.Fatalf("failed to parse response: %v", err) - } - if resp["skipped"].(float64) != 1 { - t.Errorf("skipped = %v, want 1 (workspace not found)", resp["skipped"]) - } - if resp["imported"].(float64) != 0 { - t.Errorf("imported = %v, want 0", resp["imported"]) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -func TestAdminMemories_Import_DuplicateSkipped(t *testing.T) { - mock := setupTestDB(t) - h := newAdminMemoriesHandler() - - // Workspace lookup succeeds. - mock.ExpectQuery("SELECT id FROM workspaces WHERE name = \\$1"). - WithArgs("my-workspace"). - WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("ws-uuid-1")) - - // Duplicate check returns true → entry is skipped. - mock.ExpectQuery("SELECT EXISTS"). - WithArgs("ws-uuid-1", sqlmock.AnyArg(), "LOCAL"). - WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) - - w := adminPost(t, h, []map[string]interface{}{ - { - "content": "already stored fact", - "scope": "LOCAL", - "workspace_name": "my-workspace", - }, - }) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var resp map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { - t.Fatalf("failed to parse response: %v", err) - } - if resp["skipped"].(float64) != 1 { - t.Errorf("skipped = %v, want 1 (duplicate)", resp["skipped"]) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -// TestAdminMemories_Import_RedactsSecretsBeforeDedup verifies F1085 (#1132): -// redactSecrets is called BEFORE the deduplication check so that two backups -// with the same original secret each get the same placeholder and dedup works. -// The DB dedup query must receive the REDACTED content, not the raw credential. -func TestAdminMemories_Import_RedactsSecretsBeforeDedup(t *testing.T) { - mock := setupTestDB(t) - h := newAdminMemoriesHandler() - - rawContent := "the key is OPENAI_API_KEY=sk-1234567890abcdefgh" - redacted, changed := redactSecrets("my-workspace", rawContent) - if !changed { - t.Fatalf("precondition: redactSecrets must change the test content") - } - - // Workspace lookup. - mock.ExpectQuery("SELECT id FROM workspaces WHERE name = \\$1"). - WithArgs("my-workspace"). - WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("ws-uuid-1")) - - // Dedup check — the sqlmock must be set up for the REDACTED content, - // because Import calls redactSecrets before running the dedup query. - // If redactSecrets is not called, the mock would match on rawContent instead. - mock.ExpectQuery("SELECT EXISTS"). - WithArgs("ws-uuid-1", redacted, "LOCAL"). - WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(false)) - - // Insert — receives the redacted content (not raw). Handler uses the - // 4-arg INSERT when created_at is absent from the payload. - mock.ExpectExec("INSERT INTO agent_memories"). - WithArgs("ws-uuid-1", redacted, "LOCAL", "general"). - WillReturnResult(sqlmock.NewResult(1, 1)) - - w := adminPost(t, h, []map[string]interface{}{ - { - "content": rawContent, - "scope": "LOCAL", - "workspace_name": "my-workspace", - }, - }) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var resp map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { - t.Fatalf("failed to parse response: %v", err) - } - if resp["imported"].(float64) != 1 { - t.Errorf("imported = %v, want 1", resp["imported"]) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v (F1085 regression: redactSecrets not called before dedup)", err) - } -} - -func TestAdminMemories_Import_PreservesCreatedAt(t *testing.T) { - mock := setupTestDB(t) - h := newAdminMemoriesHandler() - - origTime := "2026-01-15T10:30:00Z" - - // Workspace lookup. - mock.ExpectQuery("SELECT id FROM workspaces WHERE name = \\$1"). - WithArgs("my-workspace"). - WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("ws-uuid-1")) - - // Dedup check. - mock.ExpectQuery("SELECT EXISTS"). - WithArgs("ws-uuid-1", sqlmock.AnyArg(), "LOCAL"). - WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(false)) - - // Insert with created_at — must use the 5-arg INSERT. - mock.ExpectExec("INSERT INTO agent_memories"). - WithArgs("ws-uuid-1", sqlmock.AnyArg(), "LOCAL", "general", origTime). - WillReturnResult(sqlmock.NewResult(1, 1)) - - w := adminPost(t, h, []map[string]interface{}{ - { - "content": "a fact", - "scope": "LOCAL", - "workspace_name": "my-workspace", - "created_at": origTime, - }, - }) - - if w.Code != http.StatusOK { - t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var resp map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { - t.Fatalf("failed to parse response: %v", err) - } - if resp["imported"].(float64) != 1 { - t.Errorf("imported = %v, want 1", resp["imported"]) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} diff --git a/workspace-server/internal/handlers/mcp_test.go b/workspace-server/internal/handlers/mcp_test.go index 32843e92f..09c482da2 100644 --- a/workspace-server/internal/handlers/mcp_test.go +++ b/workspace-server/internal/handlers/mcp_test.go @@ -16,6 +16,7 @@ import ( "github.com/DATA-DOG/go-sqlmock" "github.com/Molecule-AI/molecule-monorepo/platform/internal/db" + "github.com/Molecule-AI/molecule-monorepo/platform/internal/memory/contract" "github.com/gin-gonic/gin" ) @@ -485,65 +486,45 @@ func TestMCPHandler_ListPeers_ReturnsSiblings(t *testing.T) { // tools/call — commit_memory // ───────────────────────────────────────────────────────────────────────────── -func TestMCPHandler_CommitMemory_LocalScope_Success(t *testing.T) { +// Issue #1733: the legacy SQL success-path tests for commit_memory and +// recall_memory have been removed — the v2 plugin is the only backend +// and its success paths are covered by: +// - TestToolCommitMemory_RoutesThroughV2WhenWired (legacy-shim test) +// - TestToolRecallMemory_RoutesThroughV2WhenWired (legacy-shim test) +// - Every test in mcp_tools_memory_v2_test.go +// The unwired-path tests live in mcp_tools_memory_legacy_shim_test.go +// (TestToolCommitMemory_ErrorsWhenV2Unwired and its recall sibling). +// +// The two scope-blocked tests below remain because they validate the +// OFFSEC-001 JSON-RPC scrub layer (mcp.go dispatchRPC), which is +// orthogonal to the memory backend. After A1 the underlying error +// shifts from "GLOBAL scope is not permitted" to "memory plugin is +// not configured" — but the client-visible message stays "tool call +// failed", which is what the scrub assertion actually proves. + +// TestMCPHandler_CommitMemory_GlobalScope_ScrubsInternalError verifies the +// OFFSEC-001 / #259 scrub contract on the commit_memory tool: the GLOBAL +// scope block at scopeToWritableNamespace produces an internal error +// containing the tokens "GLOBAL", "scope", "permitted", "bridge", +// "LOCAL", "TEAM" — every one of those MUST be scrubbed to the constant +// "tool call failed" + code -32000 before reaching the JSON-RPC wire. +// +// Issue #1747 review fixed the test setup: the handler is now wired +// with a v2 plugin + resolver stub so the request actually reaches +// the GLOBAL-block path in commitMemoryLegacyShim → +// scopeToWritableNamespace. Without that wiring, the handler errors +// earlier in `memoryV2Available()` with "memory plugin is not +// configured", and the leaked-tokens assertion below becomes +// vacuously true — passes even if the entire scrub layer in +// mcp.go:dispatchRPC is deleted. The wired path is the only one +// that actually pins the OFFSEC-001 contract. +func TestMCPHandler_CommitMemory_GlobalScope_ScrubsInternalError(t *testing.T) { h, mock := newMCPHandler(t) - - mock.ExpectExec("INSERT INTO agent_memories"). - WithArgs(sqlmock.AnyArg(), "ws-1", "important fact", "LOCAL", "ws-1"). - WillReturnResult(sqlmock.NewResult(1, 1)) - - w := mcpPost(t, h, "ws-1", map[string]interface{}{ - "jsonrpc": "2.0", - "id": 9, - "method": "tools/call", - "params": map[string]interface{}{ - "name": "commit_memory", - "arguments": map[string]interface{}{ - "content": "important fact", - "scope": "LOCAL", - }, - }, - }) - - if w.Code != http.StatusOK { - t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) - } - var resp mcpResponse - json.Unmarshal(w.Body.Bytes(), &resp) - if resp.Error != nil { - t.Fatalf("unexpected error: %+v", resp.Error) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -// TestMCPHandler_CommitMemory_GlobalScope_Blocked_ScrubsInternalError verifies -// two contracts at once on the GLOBAL-scope-blocked path: -// -// 1. C3 invariant (commit_memory with scope=GLOBAL aborts on the MCP bridge -// before touching the DB), AND -// 2. OFFSEC-001 / #259 scrub contract (commit 7d1a189f): the JSON-RPC error -// returned to the client is a CONSTANT — code=-32000, message="tool call -// failed" — with the production-internal err.Error() text logged -// server-side, never reflected back to the caller. -// -// Prior to this rename the test asserted that the client-visible message -// CONTAINED the substring "GLOBAL", which was the human-readable internal -// error from toolCommitMemory. mc#664 Class 2 flipped that assertion the -// right way around: now the test FAILS if the scrub regresses (i.e. if the -// internal string is ever reflected back to the wire), and PASSES iff the -// scrubbed constant reaches the client. -// -// Coupling note: the constant string "tool call failed" and the code -32000 -// are the same values asserted by -// TestMCPHandler_dispatchRPC_UnknownTool_ReturnsConstantMessage — both are -// the OFFSEC-001 contract for the dispatch-failure branch in mcp.go (the -// third err.Error() leak that 7d1a189f scrubbed). If those constants ever -// change, both tests must move together. -func TestMCPHandler_CommitMemory_GlobalScope_Blocked_ScrubsInternalError(t *testing.T) { - h, mock := newMCPHandler(t) - // No DB expectations — handler must abort before touching the DB (C3). + // Wire v2 stubs so toolCommitMemory → commitMemoryLegacyShim + // actually runs (without v2, it short-circuits with the + // "plugin not configured" error that doesn't contain the + // leaked-token strings we're asserting on). + h.withMemoryV2APIs(&stubMemoryPlugin{}, rootNamespaceResolver()) w := mcpPost(t, h, "ws-1", map[string]interface{}{ "jsonrpc": "2.0", @@ -585,7 +566,7 @@ func TestMCPHandler_CommitMemory_GlobalScope_Blocked_ScrubsInternalError(t *test } // (3) OFFSEC-001 negative assertions — the internal err.Error() text - // from toolCommitMemory ("GLOBAL scope is not permitted via the MCP + // from scopeToWritableNamespace ("GLOBAL scope is not permitted via the MCP // bridge — use LOCAL or TEAM") must NOT appear in the client-visible // message. Each token below is a distinct substring of that internal // string; if ANY leaks through, the scrub in mcp.go dispatchRPC has @@ -610,41 +591,43 @@ func TestMCPHandler_CommitMemory_GlobalScope_Blocked_ScrubsInternalError(t *test } } -// TestMCPHandler_CommitMemory_SecretInContent_IsRedactedBeforeInsert verifies -// the SAFE-T1201 (#838) fix on the MCP bridge path. PR #881 closed the HTTP -// handler but missed this one — an agent tool-call carrying plain-text -// credentials must have them scrubbed before the INSERT reaches the DB. -// -// The test asserts via the sqlmock `WithArgs` matcher that the content column -// binds the REDACTED form, not the raw input. sqlmock verifies the exact arg -// values, so a regression (removing the redactSecrets call) would fail with -// "argument mismatch" rather than silently persisting the secret. -func TestMCPHandler_CommitMemory_SecretInContent_IsRedactedBeforeInsert(t *testing.T) { - h, mock := newMCPHandler(t) +// Issue #1733: the legacy SQL-path redaction tests for commit_memory +// (SecretInContent_IsRedactedBeforeInsert, CleanContent_PassesThrough) +// have been removed. The v2 plugin path performs the same redaction +// (mcp_tools_memory_v2.go:122 + :242); its coverage lives in +// mcp_tools_memory_v2_test.go. + +// TestMCPHandler_CommitMemory_LegacyName_RedactionAtPlugin verifies that +// the LEGACY MCP tool name `commit_memory` (the one most agents +// actually call — `commit_memory_v2` is the underlying handler the +// shim delegates to) still redacts secret-shaped content before the +// payload reaches the v2 plugin. The deleted SQL-path version of this +// test pinned the same contract against `agent_memories` INSERT +// arguments; #1747 review (finding N6) noted the legacy-name path +// had no direct equivalent post-A1. This test fills that gap by +// capturing the MemoryWrite the stub plugin receives. +func TestMCPHandler_CommitMemory_LegacyName_RedactionAtPlugin(t *testing.T) { + h, _ := newMCPHandler(t) + + var captured contract.MemoryWrite + plugin := &stubMemoryPlugin{ + commitFn: func(_ context.Context, _ string, body contract.MemoryWrite) (*contract.MemoryWriteResponse, error) { + captured = body + return &contract.MemoryWriteResponse{ID: "mem-x", Namespace: "workspace:root-1"}, nil + }, + } + h.withMemoryV2APIs(plugin, rootNamespaceResolver()) - // Content with three distinct secret patterns covered by redactSecrets: - // - env-var assignment (ANTHROPIC_API_KEY=) - // - Bearer token - // - sk-… prefixed key rawContent := "key=ANTHROPIC_API_KEY=sk-ant-xxxxxxxxxxxxxxxx auth=Bearer ghp_yyyyyyyyyyyyy note=sk-proj-zzzzzzzzzzzzzzzzzzzz" - - // Derive what redactSecrets will produce so the sqlmock arg match is - // exact. This keeps the test brittle-on-purpose: if redactSecrets's - // output shape changes, this test must be re-derived, which surfaces - // the change during review. - expected, changed := redactSecrets("ws-1", rawContent) + wantRedacted, changed := redactSecrets("root-1", rawContent) if !changed { - t.Fatalf("precondition failed — redactSecrets must change the test content; got unchanged %q", expected) + t.Fatalf("precondition failed — redactSecrets must change the test content; got %q", wantRedacted) } - if bytes.Contains([]byte(expected), []byte("sk-ant-xxxxxxxxxxxxxxxx")) { - t.Fatalf("precondition failed — redacted content still contains raw secret: %s", expected) + if bytes.Contains([]byte(wantRedacted), []byte("sk-ant-xxxxxxxxxxxxxxxx")) { + t.Fatalf("precondition failed — redacted content still contains raw secret: %s", wantRedacted) } - mock.ExpectExec("INSERT INTO agent_memories"). - WithArgs(sqlmock.AnyArg(), "ws-1", expected, "LOCAL", "ws-1"). - WillReturnResult(sqlmock.NewResult(1, 1)) - - w := mcpPost(t, h, "ws-1", map[string]interface{}{ + w := mcpPost(t, h, "root-1", map[string]interface{}{ "jsonrpc": "2.0", "id": 99, "method": "tools/call", @@ -656,52 +639,32 @@ func TestMCPHandler_CommitMemory_SecretInContent_IsRedactedBeforeInsert(t *testi }, }, }) - if w.Code != http.StatusOK { t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) } var resp mcpResponse - json.Unmarshal(w.Body.Bytes(), &resp) + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("response is not valid JSON: %v", err) + } if resp.Error != nil { t.Fatalf("unexpected JSON-RPC error: %+v", resp.Error) } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("sqlmock mismatch — content was NOT redacted before insert: %v", err) + + // The plugin must have seen the REDACTED content, not the raw + // secret. If this trips, redaction in the legacy-shim → v2 path + // has regressed and credentials are flowing through to the + // plugin's memory_records table. + if captured.Content == "" { + t.Fatal("plugin.CommitMemory was not called — the shim short-circuited before reaching v2") } -} - -// TestMCPHandler_CommitMemory_CleanContent_PassesThrough confirms that the -// redactor is a no-op on content with no credentials — a regression where -// redactSecrets corrupted benign content would be a user-visible bug. -func TestMCPHandler_CommitMemory_CleanContent_PassesThrough(t *testing.T) { - h, mock := newMCPHandler(t) - - cleanContent := "the quick brown fox jumps over the lazy dog — no secrets here" - - // Bind the exact string — no wildcards — so that any transformation - // (whitespace, case, truncation) would fail the arg match. - mock.ExpectExec("INSERT INTO agent_memories"). - WithArgs(sqlmock.AnyArg(), "ws-1", cleanContent, "TEAM", "ws-1"). - WillReturnResult(sqlmock.NewResult(1, 1)) - - w := mcpPost(t, h, "ws-1", map[string]interface{}{ - "jsonrpc": "2.0", - "id": 100, - "method": "tools/call", - "params": map[string]interface{}{ - "name": "commit_memory", - "arguments": map[string]interface{}{ - "content": cleanContent, - "scope": "TEAM", - }, - }, - }) - - if w.Code != http.StatusOK { - t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + if captured.Content == rawContent { + t.Errorf("legacy commit_memory leaked raw secret to plugin: %q", captured.Content) } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("clean content should pass through unchanged: %v", err) + if captured.Content != wantRedacted { + t.Errorf("captured.Content = %q, want redacted %q", captured.Content, wantRedacted) + } + if bytes.Contains([]byte(captured.Content), []byte("sk-ant-xxxxxxxxxxxxxxxx")) { + t.Errorf("captured.Content still contains raw API key fragment: %s", captured.Content) } } @@ -709,14 +672,17 @@ func TestMCPHandler_CommitMemory_CleanContent_PassesThrough(t *testing.T) { // tools/call — recall_memory // ───────────────────────────────────────────────────────────────────────────── -// TestMCPHandler_RecallMemory_GlobalScope_Blocked_ScrubsInternalError verifies -// C3 (GLOBAL scope blocked on MCP bridge) is enforced and that the OFFSEC-001 -// scrub contract applies: the client-visible error.message is the constant -// "tool call failed", NOT the descriptive internal reason. The internal reason -// ("GLOBAL scope is not permitted via the MCP bridge") is logged server-side -// but must never reach the wire. -func TestMCPHandler_RecallMemory_GlobalScope_Blocked_ScrubsInternalError(t *testing.T) { +// TestMCPHandler_RecallMemory_GlobalScope_ScrubsInternalError mirrors the +// commit_memory scrub test on the recall_memory path. Same #1747 review +// fix applied: wire v2 stubs so the request reaches the GLOBAL-block +// path in scopeToReadableNamespaces (which produces the same "GLOBAL +// scope is not permitted via the MCP bridge" internal error that the +// leaked-tokens loop below tests for). Without v2 stubs the handler +// short-circuits on `memoryV2Available()` and the leaked-tokens loop +// becomes vacuously true. +func TestMCPHandler_RecallMemory_GlobalScope_ScrubsInternalError(t *testing.T) { h, mock := newMCPHandler(t) + h.withMemoryV2APIs(&stubMemoryPlugin{}, rootNamespaceResolver()) // No DB expectations — handler must abort before touching the DB. w := mcpPost(t, h, "ws-1", map[string]interface{}{ @@ -770,42 +736,11 @@ func TestMCPHandler_RecallMemory_GlobalScope_Blocked_ScrubsInternalError(t *test } } -func TestMCPHandler_RecallMemory_LocalScope_Empty(t *testing.T) { - h, mock := newMCPHandler(t) - - mock.ExpectQuery("SELECT id, content, scope, created_at"). - WithArgs("ws-1", ""). - WillReturnRows(sqlmock.NewRows([]string{"id", "content", "scope", "created_at"})) - - w := mcpPost(t, h, "ws-1", map[string]interface{}{ - "jsonrpc": "2.0", - "id": 12, - "method": "tools/call", - "params": map[string]interface{}{ - "name": "recall_memory", - "arguments": map[string]interface{}{ - "query": "", - "scope": "LOCAL", - }, - }, - }) - - var resp mcpResponse - json.Unmarshal(w.Body.Bytes(), &resp) - if resp.Error != nil { - t.Fatalf("unexpected error: %+v", resp.Error) - } - result, _ := resp.Result.(map[string]interface{}) - content, _ := result["content"].([]interface{}) - item, _ := content[0].(map[string]interface{}) - text, _ := item["text"].(string) - if text != "No memories found." { - t.Errorf("expected 'No memories found.', got %q", text) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} +// Issue #1733: TestMCPHandler_RecallMemory_LocalScope_Empty removed — +// it asserted on the legacy SQL SELECT path. The v2 empty-result +// rendering is covered by TestToolRecallMemory_RoutesThroughV2WhenWired +// (mcp_tools_memory_legacy_shim_test.go) which uses a stub plugin that +// returns an empty SearchResponse. // ───────────────────────────────────────────────────────────────────────────── // tools/call — send_message_to_user diff --git a/workspace-server/internal/handlers/mcp_tools.go b/workspace-server/internal/handlers/mcp_tools.go index 4dd863cf7..c33362d7b 100644 --- a/workspace-server/internal/handlers/mcp_tools.go +++ b/workspace-server/internal/handlers/mcp_tools.go @@ -363,127 +363,24 @@ func (h *MCPHandler) toolSendMessageToUser(ctx context.Context, workspaceID stri } func (h *MCPHandler) toolCommitMemory(ctx context.Context, workspaceID string, args map[string]interface{}) (string, error) { - // PR-6 (RFC #2728) compat shim: when the v2 plugin is wired - // (MEMORY_PLUGIN_URL set), translate legacy scope→namespace and - // delegate. Otherwise fall through to the legacy DB path so - // operators who haven't enabled the plugin yet keep working. - if h.memoryV2Available() == nil { - return h.commitMemoryLegacyShim(ctx, workspaceID, args) + // Issue #1733 — v2 memory plugin is now the only path. The legacy + // SQL fallback on `agent_memories` is gone; an unconfigured plugin + // returns a clear error to the agent rather than silently writing + // into a stale table no one reads. + if err := h.memoryV2Available(); err != nil { + return "", err } - - content, _ := args["content"].(string) - scope, _ := args["scope"].(string) - if content == "" { - return "", fmt.Errorf("content is required") - } - if scope == "" { - scope = "LOCAL" - } - - // C3: GLOBAL scope is blocked on the MCP bridge. - if scope == "GLOBAL" { - return "", fmt.Errorf("GLOBAL scope is not permitted via the MCP bridge — use LOCAL or TEAM") - } - if scope != "LOCAL" && scope != "TEAM" { - return "", fmt.Errorf("scope must be LOCAL or TEAM") - } - - memoryID := uuid.New().String() - // SAFE-T1201 (#838): scrub known credential patterns before persistence so - // plain-text API keys pulled in via tool responses can't land in the - // memories table (and leak into shared TEAM scope). Reuses redactSecrets - // already shipped for the HTTP path in PR #881 — this was the MCP-bridge - // sibling the original fix missed. Runs on every write regardless of scope. - content, _ = redactSecrets(workspaceID, content) - _, err := h.database.ExecContext(ctx, ` - INSERT INTO agent_memories (id, workspace_id, content, scope, namespace) - VALUES ($1, $2, $3, $4, $5) - `, memoryID, workspaceID, content, scope, workspaceID) - if err != nil { - log.Printf("MCPHandler.commit_memory workspace=%s: %v", workspaceID, err) - return "", fmt.Errorf("failed to save memory") - } - - return fmt.Sprintf(`{"id":%q,"scope":%q}`, memoryID, scope), nil + return h.commitMemoryLegacyShim(ctx, workspaceID, args) } func (h *MCPHandler) toolRecallMemory(ctx context.Context, workspaceID string, args map[string]interface{}) (string, error) { - // PR-6 (RFC #2728) compat shim: when the v2 plugin is wired, - // route through it. Otherwise fall through to legacy DB path. - if h.memoryV2Available() == nil { - return h.recallMemoryLegacyShim(ctx, workspaceID, args) + // Issue #1733 — v2 memory plugin is now the only path. Same shape + // as toolCommitMemory: an unconfigured plugin is an error, not a + // quiet read from a frozen v1 table. + if err := h.memoryV2Available(); err != nil { + return "", err } - - query, _ := args["query"].(string) - scope, _ := args["scope"].(string) - - // C3: GLOBAL scope is blocked on the MCP bridge. - if scope == "GLOBAL" { - return "", fmt.Errorf("GLOBAL scope is not permitted via the MCP bridge — use LOCAL, TEAM, or empty") - } - - var rows *sql.Rows - var err error - - switch scope { - case "LOCAL": - rows, err = h.database.QueryContext(ctx, ` - SELECT id, content, scope, created_at - FROM agent_memories - WHERE workspace_id = $1 AND scope = 'LOCAL' - AND ($2 = '' OR content ILIKE '%' || $2 || '%') - ORDER BY created_at DESC LIMIT 50 - `, workspaceID, query) - case "TEAM": - // Team scope: parent + all siblings. - rows, err = h.database.QueryContext(ctx, ` - SELECT m.id, m.content, m.scope, 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.id = $1 OR w.parent_id = (SELECT parent_id FROM workspaces WHERE id = $1 AND parent_id IS NOT NULL)) - AND ($2 = '' OR m.content ILIKE '%' || $2 || '%') - ORDER BY m.created_at DESC LIMIT 50 - `, workspaceID, query) - default: - // Empty scope → LOCAL only for the MCP bridge (GLOBAL excluded per C3). - rows, err = h.database.QueryContext(ctx, ` - SELECT id, content, scope, created_at - FROM agent_memories - WHERE workspace_id = $1 AND scope IN ('LOCAL', 'TEAM') - AND ($2 = '' OR content ILIKE '%' || $2 || '%') - ORDER BY created_at DESC LIMIT 50 - `, workspaceID, query) - } - if err != nil { - return "", fmt.Errorf("memory search failed: %w", err) - } - defer rows.Close() - - type memEntry struct { - ID string `json:"id"` - Content string `json:"content"` - Scope string `json:"scope"` - CreatedAt string `json:"created_at"` - } - var results []memEntry - for rows.Next() { - var e memEntry - if err := rows.Scan(&e.ID, &e.Content, &e.Scope, &e.CreatedAt); err != nil { - continue - } - results = append(results, e) - } - if err := rows.Err(); err != nil { - return "", fmt.Errorf("memory scan error: %w", err) - } - - if len(results) == 0 { - return "No memories found.", nil - } - b, _ := json.MarshalIndent(results, "", " ") - return string(b), nil + return h.recallMemoryLegacyShim(ctx, workspaceID, args) } // ───────────────────────────────────────────────────────────────────────────── diff --git a/workspace-server/internal/handlers/mcp_tools_memory_legacy_shim.go b/workspace-server/internal/handlers/mcp_tools_memory_legacy_shim.go index 88cb7c334..4a2fca040 100644 --- a/workspace-server/internal/handlers/mcp_tools_memory_legacy_shim.go +++ b/workspace-server/internal/handlers/mcp_tools_memory_legacy_shim.go @@ -2,14 +2,13 @@ package handlers // mcp_tools_memory_legacy_shim.go — translates legacy commit_memory / // recall_memory calls (scope-based) into the v2 plugin path -// (namespace-based) when the v2 plugin is wired. +// (namespace-based). // -// Behavior: -// - If h.memv2 is wired (MEMORY_PLUGIN_URL set + plugin reachable), -// legacy tools translate scope→namespace and delegate to v2. -// - If h.memv2 is NOT wired, legacy tools fall through to the -// original DB-backed path in mcp_tools.go (zero behavior change -// for operators who haven't enabled the plugin yet). +// Issue #1733: v2 is now the only memory backend. Callers in +// mcp_tools.go MUST verify h.memv2 is wired before invoking these +// helpers (toolCommitMemory / toolRecallMemory both check +// memoryV2Available and short-circuit with an error when not wired). +// The previous "fall through to direct SQL" branch is gone. // // Translation: // commit: LOCAL → workspace: diff --git a/workspace-server/internal/handlers/mcp_tools_memory_legacy_shim_test.go b/workspace-server/internal/handlers/mcp_tools_memory_legacy_shim_test.go index dd62fe532..bb9c3e575 100644 --- a/workspace-server/internal/handlers/mcp_tools_memory_legacy_shim_test.go +++ b/workspace-server/internal/handlers/mcp_tools_memory_legacy_shim_test.go @@ -512,41 +512,38 @@ func TestToolRecallMemory_RoutesThroughV2WhenWired(t *testing.T) { } } -func TestToolCommitMemory_FallsThroughToLegacyWhenV2Unwired(t *testing.T) { - // V2 NOT wired (no withMemoryV2APIs call). Should hit the legacy - // SQL path and write to agent_memories directly. - db, mock, _ := sqlmock.New() +// Issue #1733: v2 is the only path; commit/recall return a clear error +// (not a silent SQL fallback) when MEMORY_PLUGIN_URL is unset. + +func TestToolCommitMemory_ErrorsWhenV2Unwired(t *testing.T) { + db, _, _ := sqlmock.New() defer db.Close() - mock.ExpectExec("INSERT INTO agent_memories"). - WillReturnResult(sqlmock.NewResult(0, 1)) - h := &MCPHandler{database: db} + h := &MCPHandler{database: db} // no withMemoryV2APIs → memv2 nil _, err := h.toolCommitMemory(context.Background(), "root-1", map[string]interface{}{ "content": "x", "scope": "LOCAL", }) - if err != nil { - t.Fatalf("err: %v", err) + if err == nil { + t.Fatal("expected error when v2 unwired, got nil") } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("legacy SQL path not exercised: %v", err) + if !strings.Contains(err.Error(), "MEMORY_PLUGIN_URL") { + t.Errorf("error must hint at MEMORY_PLUGIN_URL: %v", err) } } -func TestToolRecallMemory_FallsThroughToLegacyWhenV2Unwired(t *testing.T) { - db, mock, _ := sqlmock.New() +func TestToolRecallMemory_ErrorsWhenV2Unwired(t *testing.T) { + db, _, _ := sqlmock.New() defer db.Close() - mock.ExpectQuery("SELECT id, content, scope, created_at"). - WillReturnRows(sqlmock.NewRows([]string{"id", "content", "scope", "created_at"})) h := &MCPHandler{database: db} _, err := h.toolRecallMemory(context.Background(), "root-1", map[string]interface{}{ "scope": "LOCAL", }) - if err != nil { - t.Fatalf("err: %v", err) + if err == nil { + t.Fatal("expected error when v2 unwired, got nil") } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("legacy SQL path not exercised: %v", err) + if !strings.Contains(err.Error(), "MEMORY_PLUGIN_URL") { + t.Errorf("error must hint at MEMORY_PLUGIN_URL: %v", err) } } diff --git a/workspace-server/internal/memory/wiring/wiring.go b/workspace-server/internal/memory/wiring/wiring.go index 12badfdc9..728a3114e 100644 --- a/workspace-server/internal/memory/wiring/wiring.go +++ b/workspace-server/internal/memory/wiring/wiring.go @@ -39,41 +39,30 @@ type Bundle struct { // Build returns a wired Bundle if MEMORY_PLUGIN_URL is set, else nil. // // It probes /v1/health at boot — when the plugin is unreachable, we -// log a warning but STILL return the bundle. The MCP layer's -// circuit breaker handles ongoing unavailability; we don't want to -// block workspace-server boot just because the memory plugin is -// briefly down. +// log a warning but STILL return the bundle. The MCP layer's circuit +// breaker handles ongoing unavailability. // -// Silent-misconfig guard: if MEMORY_V2_CUTOVER=true is set without -// MEMORY_PLUGIN_URL, the cutoverActive() check in handlers silently -// returns false and the legacy SQL path serves every request. The -// operator sees no errors, no warnings, and assumes the cutover is -// live. Log a LOUD WARN at boot when the env is half-configured so -// the misconfig is visible in the boot log, not detectable only by -// observing that the legacy table is still being written to. +// Issue #1733: when MEMORY_PLUGIN_URL is unset the bundle is nil and +// every memory MCP tool returns a clear "plugin not configured" error +// (mcp_tools.go). There is no longer a silent SQL fallback to +// agent_memories, so the previous half-configured-cutover guard is +// gone — a missing URL fails loudly on first memory call instead of +// quietly serving stale legacy data. +// +// MEMORY_V2_CUTOVER is left intact as a deployment marker (CP user-data +// reads it before spawning the sidecar in entrypoint-tenant.sh); we no +// longer branch on it inside the platform binary. func Build(db *sql.DB) *Bundle { - cutover := os.Getenv("MEMORY_V2_CUTOVER") == "true" pluginURL := os.Getenv("MEMORY_PLUGIN_URL") - if pluginURL == "" { - if cutover { - log.Printf("memory-plugin: ⚠️ MEMORY_V2_CUTOVER=true but MEMORY_PLUGIN_URL is unset — cutover is INACTIVE, legacy SQL path is serving every request. Either unset MEMORY_V2_CUTOVER or point MEMORY_PLUGIN_URL at a reachable plugin server.") - } + log.Printf("memory-plugin: MEMORY_PLUGIN_URL is unset — v2 memory MCP tools (commit_memory, recall_memory, admin export/import) will return 'plugin not configured' to callers. Set MEMORY_PLUGIN_URL to activate.") return nil } plugin := mclient.New(mclient.Config{}) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() if hr, err := plugin.Boot(ctx); err != nil { - // Log even louder when cutover is on — an unreachable plugin - // during cutover means writes that the operator THINKS are - // going to v2 will silently fall back to legacy via the - // circuit breaker on each request. Make it impossible to miss. - if cutover { - log.Printf("memory-plugin: ⚠️ MEMORY_V2_CUTOVER=true and MEMORY_PLUGIN_URL=%s but /v1/health probe failed (%v). Cutover writes will fall back to legacy via circuit breaker. Verify the plugin server is reachable.", pluginURL, err) - } else { - log.Printf("memory-plugin: /v1/health probe failed (will retry per-request): %v", err) - } + log.Printf("memory-plugin: ⚠️ /v1/health probe failed at boot (%v). MCP memory calls will error until the plugin becomes reachable. Verify MEMORY_PLUGIN_URL=%s.", err, pluginURL) } else { log.Printf("memory-plugin: ok, capabilities=%v", hr.Capabilities) } diff --git a/workspace-server/internal/memory/wiring/wiring_test.go b/workspace-server/internal/memory/wiring/wiring_test.go index d8da1efb4..979f8bbe8 100644 --- a/workspace-server/internal/memory/wiring/wiring_test.go +++ b/workspace-server/internal/memory/wiring/wiring_test.go @@ -166,69 +166,43 @@ func captureLogs(t *testing.T, fn func()) string { return buf.String() } -// TestBuild_WarnsWhenCutoverWithoutPluginURL pins the silent-misconfig -// guard: an operator who flips MEMORY_V2_CUTOVER=true without also -// pointing MEMORY_PLUGIN_URL at a plugin server has just disabled the -// cutover with no error visible. Without this WARN, the only signal -// is "the legacy table is still being written to" — invisible to -// every operator who doesn't explicitly check. -func TestBuild_WarnsWhenCutoverWithoutPluginURL(t *testing.T) { - t.Setenv("MEMORY_V2_CUTOVER", "true") +// Issue #1733: the old "cutover-without-URL" and "cutover-with-failing- +// probe" loud-warning tests are gone — workspace-server no longer +// branches on MEMORY_V2_CUTOVER (v2 is unconditional now), so the +// half-configured-cutover failure mode they guarded against can no +// longer occur. The two surviving tests below pin the new shape: +// one log line when URL is unset, one when the boot-time probe fails. + +// TestBuild_LogsWhenURLUnset confirms the operator-visible boot log +// line that fires when MEMORY_PLUGIN_URL is unset — every memory MCP +// tool will then return "plugin not configured" to callers. +func TestBuild_LogsWhenURLUnset(t *testing.T) { t.Setenv("MEMORY_PLUGIN_URL", "") out := captureLogs(t, func() { if got := Build(nil); got != nil { t.Errorf("expected nil bundle, got %+v", got) } }) - if !strings.Contains(out, "MEMORY_V2_CUTOVER=true") || !strings.Contains(out, "MEMORY_PLUGIN_URL is unset") { - t.Errorf("expected loud WARN about half-configured cutover; got log:\n%s", out) + if !strings.Contains(out, "MEMORY_PLUGIN_URL is unset") { + t.Errorf("expected boot log to mention MEMORY_PLUGIN_URL is unset; got:\n%s", out) } } -// TestBuild_NoWarnWhenNeitherSet pins the happy default: an operator -// running without the v2 plugin should not see scary warnings. -func TestBuild_NoWarnWhenNeitherSet(t *testing.T) { - t.Setenv("MEMORY_V2_CUTOVER", "") - t.Setenv("MEMORY_PLUGIN_URL", "") - out := captureLogs(t, func() { _ = Build(nil) }) - if strings.Contains(out, "MEMORY_V2_CUTOVER") { - t.Errorf("expected no MEMORY_V2_CUTOVER warning when env is unset; got log:\n%s", out) - } -} - -// TestBuild_LoudWarnWhenCutoverAndProbeFails pins the second -// half-config case: cutover is on AND plugin URL is set, but the -// /v1/health probe fails (server down or wrong URL). Without this -// loud WARN, the operator sees only the generic "probe failed" line -// that gets emitted even when cutover is OFF — hiding the fact that -// real cutover writes will quietly fall back via circuit breaker. -func TestBuild_LoudWarnWhenCutoverAndProbeFails(t *testing.T) { - t.Setenv("MEMORY_V2_CUTOVER", "true") +// TestBuild_LogsWhenProbeFails confirms the boot log line that fires +// when MEMORY_PLUGIN_URL is set but the plugin is unreachable. The +// bundle is still returned (per the comment on Build) so the platform +// keeps booting — MCP calls error per-request until the plugin recovers. +func TestBuild_LogsWhenProbeFails(t *testing.T) { t.Setenv("MEMORY_PLUGIN_URL", "http://127.0.0.1:1") // bogus port db, _, _ := sqlmock.New() defer db.Close() - out := captureLogs(t, func() { _ = Build(db) }) - if !strings.Contains(out, "MEMORY_V2_CUTOVER=true") || !strings.Contains(out, "probe failed") { - t.Errorf("expected loud WARN about cutover-with-failing-probe; got log:\n%s", out) - } -} - -// TestBuild_QuietProbeFailWhenCutoverOff: the operator is in PRE-cutover -// mode (plugin URL set, cutover off — they're warming up the plugin). -// A failing probe in this state is not a misconfig — it should log the -// generic message, NOT the loud cutover-specific one (so log noise -// doesn't drown out real cutover misconfigs in dashboards). -func TestBuild_QuietProbeFailWhenCutoverOff(t *testing.T) { - t.Setenv("MEMORY_V2_CUTOVER", "") - t.Setenv("MEMORY_PLUGIN_URL", "http://127.0.0.1:1") - db, _, _ := sqlmock.New() - defer db.Close() - out := captureLogs(t, func() { _ = Build(db) }) - if strings.Contains(out, "MEMORY_V2_CUTOVER=true") { - t.Errorf("expected no cutover-specific warning when cutover is off; got log:\n%s", out) - } - if !strings.Contains(out, "probe failed") { - t.Errorf("expected generic probe-failed log; got log:\n%s", out) + out := captureLogs(t, func() { + if got := Build(db); got == nil { + t.Error("expected non-nil bundle when URL is set, got nil") + } + }) + if !strings.Contains(out, "/v1/health probe failed") { + t.Errorf("expected boot log to mention probe failure; got:\n%s", out) } }