refactor(memory): #1733 A1 — kill v1 SQL fallback, v2 plugin is the only backend (#1747)
Block internal-flavored paths / Block forbidden paths (push) Waiting to run
ci-arm64-advisory / fast-checks (push) Waiting to run
CI / Detect changes (push) Waiting to run
CI / Platform (Go) (push) Blocked by required conditions
CI / Canvas (Next.js) (push) Blocked by required conditions
CI / Shellcheck (E2E scripts) (push) Blocked by required conditions
CI / Canvas Deploy Reminder (push) Blocked by required conditions
CI / Python Lint & Test (push) Waiting to run
CI / all-required (push) Waiting to run
E2E API Smoke Test / detect-changes (push) Waiting to run
E2E API Smoke Test / E2E API Smoke Test (push) Blocked by required conditions
E2E Chat / detect-changes (push) Waiting to run
E2E Chat / E2E Chat (push) Blocked by required conditions
E2E Staging Canvas (Playwright) / detect-changes (push) Waiting to run
E2E Staging Canvas (Playwright) / Canvas tabs E2E (push) Blocked by required conditions
Handlers Postgres Integration / detect-changes (push) Waiting to run
Handlers Postgres Integration / Handlers Postgres Integration (push) Blocked by required conditions
Harness Replays / detect-changes (push) Waiting to run
Harness Replays / Harness Replays (push) Blocked by required conditions
Lint forbidden tenant-env keys / Scan workspace_secrets writers for forbidden env keys (push) Waiting to run
Lint no tenant GITEA or GITHUB token write / Scan for repo-host token write into tenant workspace surface (push) Waiting to run
Secret scan / Scan diff for credential-shaped strings (push) Waiting to run
Lint shellcheck (arm64 pilot) / shellcheck-arm64 (pilot) (push) Successful in 8s
publish-workspace-server-image / build-and-push (push) Successful in 3m8s
publish-workspace-server-image / Production auto-deploy (push) Has been cancelled
E2E Peer Visibility (literal MCP list_peers) / E2E Peer Visibility (local) (push) Successful in 2m18s
E2E Peer Visibility (literal MCP list_peers) / E2E Peer Visibility (push) Failing after 2m35s

CTO-bypass merge per 2026-05-24 directive; SOP-6 checklist filled + persona-acked, REQUEST_CHANGES dismissed, dispatched-review evidence in PR comments.
This commit was merged in pull request #1747.
This commit is contained in:
2026-05-24 02:45:56 +00:00
parent 36c63798eb
commit 69bcc55ad3
10 changed files with 283 additions and 937 deletions
+14 -5
View File
@@ -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}"
@@ -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
@@ -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:<id> 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())
}
}
@@ -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)
}
}
+103 -168
View File
@@ -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
+13 -116
View File
@@ -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)
}
// ─────────────────────────────────────────────────────────────────────────────
@@ -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:<self>
@@ -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)
}
}
@@ -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)
}
@@ -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)
}
}