From 2aa6e8adf3ff7b896d1bf3958c1f15cb4fb32402 Mon Sep 17 00:00:00 2001 From: hongming Date: Sat, 23 May 2026 15:06:52 -0700 Subject: [PATCH 1/2] =?UTF-8?q?refactor(memory):=20#1733=20A1=20=E2=80=94?= =?UTF-8?q?=20kill=20v1=20SQL=20fallback,=20v2=20plugin=20is=20the=20only?= =?UTF-8?q?=20backend?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CTO decision 2026-05-23: same Postgres, separate schema, v2 plugin is SSOT for tenant memory. A0 (PR #1742) isolates plugin tables under the `memory_plugin` schema. A1 (this PR) removes the silent fallback paths that route to `agent_memories` when MEMORY_PLUGIN_URL is unset, so a half-configured deployment fails loudly on the first memory call rather than quietly writing to a frozen table no one reads. ## Surfaces removed 1. **`toolCommitMemory` legacy SQL branch** (`internal/handlers/mcp_tools.go`). Was: route through v2 shim when `memoryV2Available() == nil`, else INSERT directly into `agent_memories`. Now: error `memory plugin is not configured (set MEMORY_PLUGIN_URL)` when v2 is not wired; route through the shim otherwise. ~30 LOC of inline SQL + scope validation gone — the shim already enforces the same scope rules (it just talks to the plugin). 2. **`toolRecallMemory` legacy SQL branch** (same file). Was: three scope-specific SELECTs on `agent_memories` when v2 unwired. Now: same error semantics as commit. ~60 LOC gone. 3. **`AdminMemoriesHandler.Export` legacy SQL branch** (`internal/handlers/admin_memories.go`). Was: when `cutoverActive() == false` (which checked both `MEMORY_V2_CUTOVER` *and* v2 wiring), run a direct `SELECT FROM agent_memories JOIN workspaces`. Now: respond `503 Service Unavailable` with hint `memory plugin is not configured (set MEMORY_PLUGIN_URL)`. Plugin path unchanged. 4. **`AdminMemoriesHandler.Import` legacy SQL branch** (same file). Was: per-entry workspace lookup + dedup check + INSERT into `agent_memories`. Now: same 503 when plugin not wired. 5. **`cutoverActive()` + `envMemoryV2Cutover` constant** removed — replaced with `memoryV2Wired()` which only checks `h.plugin != nil && h.resolver != nil`. The `MEMORY_V2_CUTOVER` env var is no longer read inside the platform binary (CP user-data still inspects it before spawning the sidecar, but workspace-server does not branch on it). Tenant operators upgrading to this build cannot accidentally stay in v1 by unsetting `MEMORY_V2_CUTOVER`. 6. **Boot-time guard simplification** in `internal/memory/wiring/wiring.go`. The `cutover` boolean and its two loud-WARN log lines are gone (the half-configured-cutover failure mode they guarded against is structurally impossible now). The one remaining boot log when `MEMORY_PLUGIN_URL` is unset explicitly tells the operator that v2 MCP tools will return errors until the env is set. ## Surfaces deliberately NOT removed in this PR - **`MemoriesHandler` REST endpoints** (`POST/GET/DELETE/PATCH /workspaces/:id/memories` in `memories.go`). These are the canvas-side surface and still write to `agent_memories`. Their cutover is the #1734 work (canvas Memory tab fix); coupling that here would make this PR much bigger and would conflict with the tab rewrite already drafted in #1734. - **`seedInitialMemories`** (`workspace_provision.go`) still writes to `agent_memories` during workspace creation. Its v2 migration is deferred to Phase A3 (when we drop the table entirely) — early migration would force every test that exercises workspace creation to wire a v2 stub. - **REST `/admin/memories/export` and `/admin/memories/import` request-decode path** — the `400 Bad Request` reject on malformed JSON runs before any plugin check and is preserved by the surviving `TestAdminMemories_Import_InvalidJSON`. ## Tests rewritten - `TestCutoverActive` → `TestMemoryV2Wired` (the env-flag dimension is gone; only wiring matters now). - `TestExport_LegacyPathWhenCutoverInactive` → `TestExport_503When PluginNotWired` + new sibling `TestImport_503WhenPluginNotWired`. - `TestToolCommitMemory_FallsThroughToLegacyWhenV2Unwired` → `TestToolCommitMemory_ErrorsWhenV2Unwired` (asserts on the error-message hint at `MEMORY_PLUGIN_URL`). - `TestToolRecallMemory_FallsThroughToLegacyWhenV2Unwired` → `TestToolRecallMemory_ErrorsWhenV2Unwired` (same). - `TestBuild_WarnsWhenCutoverWithoutPluginURL`, `TestBuild_LoudWarnWhenCutoverAndProbeFails`, `TestBuild_NoWarnWhenNeitherSet`, `TestBuild_QuietProbeFailWhenCutoverOff` → consolidated to `TestBuild_LogsWhenURLUnset` + `TestBuild_LogsWhenProbeFails`. ## Tests deleted (legacy SQL coverage now redundant) - `TestAdminMemories_Export_{Success,Empty,QueryError,RedactsSecrets}`, `TestAdminMemories_Import_{Success,WorkspaceNotFound_SkipsEntry, DuplicateSkipped,RedactsSecretsBeforeDedup,PreservesCreatedAt}` — every assertion was about the deleted SQL path. The plugin path is fully covered by 16 tests in `admin_memories_cutover_test.go` (listed in the file header for traceability). - `TestMCPHandler_CommitMemory_LocalScope_Success`, `TestMCPHandler_CommitMemory_SecretInContent_IsRedactedBeforeInsert`, `TestMCPHandler_CommitMemory_CleanContent_PassesThrough`, `TestMCPHandler_RecallMemory_LocalScope_Empty` — same reason; the v2 shim path is covered by `TestToolCommitMemory_RoutesThroughV2WhenWired`, `TestToolRecallMemory_RoutesThroughV2WhenWired`, and every test in `mcp_tools_memory_v2_test.go`. - `TestMCPHandler_CommitMemory_GlobalScope_Blocked_ScrubsInternalError` and its `RecallMemory_` sibling are KEPT (renamed to drop the `GlobalScope_Blocked_` qualifier) because they validate the OFFSEC-001 JSON-RPC scrub layer in `mcp.go dispatchRPC`, which is orthogonal to the memory backend. Net diff: −752 LOC across 9 files. ## Stage gates **Stage A — local build + test sweep**: - `go vet ./...` clean. - `go test -count=1 ./internal/handlers/... ./internal/memory/... ./cmd/memory-plugin-postgres/...` all green (~25 s). - Manual: confirmed `memoryV2Available()` still returns its hint message ("set MEMORY_PLUGIN_URL"), and that the JSON-RPC scrub replaces it with the OFFSEC-001 constant. **Stage B / C — deferred to staging deploy after gating PR lands**. ## Gating **DO NOT merge until PR #1742 (A0 schema isolation) has been merged and rolled out to every tenant's image cache.** A tenant booting this build without a reachable memory plugin sidecar will return errors on every `commit_memory` / `recall_memory` / admin export+import call. That's the intended behavior — but it should only happen after the sidecar is reliably running on every tenant EC2. The existing tenant entrypoint (`entrypoint-tenant.sh:35-66`, since 2026-05-14) already aborts boot when `MEMORY_V2_CUTOVER=true` and the plugin doesn't come up in 30s, so SaaS tenants are protected by the CP boot gate — but only IF they're running the post-2026-05-14 image. Audit `org_instances.fly_machine_id` boot logs before flipping. ## Risks - Self-hosted operators who haven't set `MEMORY_PLUGIN_URL` will get errors instead of working `commit_memory`. The error message tells them exactly what to set. Acceptable trade-off for SSOT. - Tenants currently running on a pre-2026-05-14 tenant image (entrypoint doesn't spawn sidecar) will start failing memory calls on first restart after the new core image rolls out. Confirm CP has recycled every tenant onto the current image before merging. - `seedInitialMemories` still writes to `agent_memories`; rows there remain readable to any operator query bypassing the API. They'll go away with the table in Phase A3. Refs: #1733 (memory SSOT consolidation), #1742 (Phase A0 — schema isolation, gates this PR), #1734 (Memory tab UI fix, unblocked once this lands). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/admin_memories.go | 158 ++----- .../handlers/admin_memories_cutover_test.go | 74 ++-- .../internal/handlers/admin_memories_test.go | 406 ++---------------- .../internal/handlers/mcp_test.go | 232 ++-------- .../internal/handlers/mcp_tools.go | 129 +----- .../handlers/mcp_tools_memory_legacy_shim.go | 13 +- .../mcp_tools_memory_legacy_shim_test.go | 35 +- .../internal/memory/wiring/wiring.go | 39 +- .../internal/memory/wiring/wiring_test.go | 76 ++-- 9 files changed, 205 insertions(+), 957 deletions(-) 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..f1dcff2f9 100644 --- a/workspace-server/internal/handlers/mcp_test.go +++ b/workspace-server/internal/handlers/mcp_test.go @@ -485,63 +485,30 @@ func TestMCPHandler_ListPeers_ReturnsSiblings(t *testing.T) { // tools/call — commit_memory // ───────────────────────────────────────────────────────────────────────────── -func TestMCPHandler_CommitMemory_LocalScope_Success(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: +// 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). // -// 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) { +// 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_RequestErrors_ScrubsInternalError verifies the +// OFFSEC-001 / #259 scrub contract on the commit_memory tool: any internal +// error gets replaced with the constant "tool call failed" + code -32000. +// Before A1 (issue #1733) this asserted on the GLOBAL-scope block; after +// A1 the underlying error is "memory plugin is not configured", which is +// equally not-leaked — proving the scrub layer is what's enforcing the +// contract, not the specific error string upstream. +func TestMCPHandler_CommitMemory_RequestErrors_ScrubsInternalError(t *testing.T) { h, mock := newMCPHandler(t) // No DB expectations — handler must abort before touching the DB (C3). @@ -610,112 +577,22 @@ 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) - - // 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) - if !changed { - t.Fatalf("precondition failed — redactSecrets must change the test content; got unchanged %q", expected) - } - if bytes.Contains([]byte(expected), []byte("sk-ant-xxxxxxxxxxxxxxxx")) { - t.Fatalf("precondition failed — redacted content still contains raw secret: %s", expected) - } - - 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{}{ - "jsonrpc": "2.0", - "id": 99, - "method": "tools/call", - "params": map[string]interface{}{ - "name": "commit_memory", - "arguments": map[string]interface{}{ - "content": rawContent, - "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 JSON-RPC error: %+v", resp.Error) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("sqlmock mismatch — content was NOT redacted before insert: %v", err) - } -} - -// 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 err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("clean content should pass through unchanged: %v", err) - } -} +// 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. // ───────────────────────────────────────────────────────────────────────────── // 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_RequestErrors_ScrubsInternalError mirrors the +// commit_memory scrub test on the recall_memory path. After A1 (issue +// #1733) the upstream error is "memory plugin is not configured" rather +// than the GLOBAL-scope block, but the scrub layer (mcp.go dispatchRPC) +// still must replace it with the constant "tool call failed". +func TestMCPHandler_RecallMemory_RequestErrors_ScrubsInternalError(t *testing.T) { h, mock := newMCPHandler(t) // No DB expectations — handler must abort before touching the DB. @@ -770,42 +647,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) } } -- 2.52.0 From c2b6e8bdb5488b0d97aa5d54850ca53de053b636 Mon Sep 17 00:00:00 2001 From: hongming Date: Sat, 23 May 2026 17:09:18 -0700 Subject: [PATCH 2/2] =?UTF-8?q?review(memory):=20#1747=20=E2=80=94=20OFFSE?= =?UTF-8?q?C=20test=20depth=20+=20legacy=20redaction=20+=20cutover=20env?= =?UTF-8?q?=20deprecation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three reviewer-flagged fixes on PR #1747: 1. OFFSEC-001 scrub tests no longer vacuous (review finding C2). `TestMCPHandler_CommitMemory_RequestErrors_ScrubsInternalError` and its recall sibling triggered via `scope=GLOBAL`, but post-A1 the handler short-circuits in `memoryV2Available()` BEFORE the GLOBAL block runs — so the leaked-tokens loop (`GLOBAL`, `scope`, `permitted`, `bridge`, `LOCAL`, `TEAM`) was scanning an error string that doesn't contain any of them ("memory plugin is not configured"). The test passed even if mcp.go:dispatchRPC's scrub was deleted entirely. Fix: wire `withMemoryV2APIs(&stubMemoryPlugin{}, rootNamespace Resolver())` so the request reaches `scopeToWritableNamespace` / `scopeToReadableNamespaces` where the actual GLOBAL block lives. Renamed `CommitMemory_RequestErrors_ScrubsInternalError` → `CommitMemory_GlobalScope_ScrubsInternalError` (and the recall sibling) to match what the test actually pins now. Verified by `go test -v` showing the internal log line `mcp: tool call failed workspace=ws-1 tool=commit_memory: GLOBAL scope is not permitted via the MCP bridge — use LOCAL or TEAM` — the actual leaky string is now reaching the scrub layer, so the negative-token assertions are real proof. 2. New `TestMCPHandler_CommitMemory_LegacyName_RedactionAtPlugin` (review finding N6). The deleted SQL-path version of this test pinned the SAFE-T1201 redaction contract against `agent_memories` INSERT args. Post-A1 the legacy `commit_memory` MCP tool routes through the shim → `toolCommitMemoryV2` → plugin; the plugin path redacts at mcp_tools_memory_v2.go:122. This test calls the legacy tool via JSON-RPC envelope with three secret-shaped patterns (ANTHROPIC_API_KEY=, Bearer token, sk-* prefix), uses a stub plugin that captures the `MemoryWrite.Content` it receives, and asserts the captured content is the redacted string — not the raw secret. Catches a regression where someone inlines the shim and skips redaction. 3. `MEMORY_V2_CUTOVER` deprecated in `entrypoint-tenant.sh` (review finding N1). The workspace-server binary stopped reading the env in PR #1747's main commit (`wiring.go` cleanup), but the entrypoint still keyed off it OR'd with `MEMORY_PLUGIN_URL`. Surfaced as a transition warning: when only `MEMORY_V2_CUTOVER` is set (no `MEMORY_PLUGIN_URL`), the entrypoint logs a loud deprecation notice and spawns the sidecar anyway so old CP user-data templates keep working through the rollout. Once CP user-data drops the var (separate CP PR), the `MEMORY_V2_CUTOVER` branch can be deleted from the entrypoint. Also filed: - **#1755** — tracking issue for migrating `seedInitialMemories` through the v2 plugin before Phase A3 drops `agent_memories` (review finding C3). Required before A3 can land or workspace creation breaks. Refs: PR #1747 review (CTO 2026-05-23 dispatched subagent), findings C2 (OFFSEC scrub depth), N1 (cutover env drift), N6 (lost legacy- name redaction coverage). C3 spun out to #1755. Co-Authored-By: Claude Opus 4.7 (1M context) --- workspace-server/entrypoint-tenant.sh | 19 ++- .../internal/handlers/mcp_test.go | 121 +++++++++++++++--- 2 files changed, 119 insertions(+), 21 deletions(-) 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/mcp_test.go b/workspace-server/internal/handlers/mcp_test.go index f1dcff2f9..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" ) @@ -501,16 +502,29 @@ func TestMCPHandler_ListPeers_ReturnsSiblings(t *testing.T) { // not configured" — but the client-visible message stays "tool call // failed", which is what the scrub assertion actually proves. -// TestMCPHandler_CommitMemory_RequestErrors_ScrubsInternalError verifies the -// OFFSEC-001 / #259 scrub contract on the commit_memory tool: any internal -// error gets replaced with the constant "tool call failed" + code -32000. -// Before A1 (issue #1733) this asserted on the GLOBAL-scope block; after -// A1 the underlying error is "memory plugin is not configured", which is -// equally not-leaked — proving the scrub layer is what's enforcing the -// contract, not the specific error string upstream. -func TestMCPHandler_CommitMemory_RequestErrors_ScrubsInternalError(t *testing.T) { +// 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) - // 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", @@ -552,7 +566,7 @@ func TestMCPHandler_CommitMemory_RequestErrors_ScrubsInternalError(t *testing.T) } // (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 @@ -583,17 +597,92 @@ func TestMCPHandler_CommitMemory_RequestErrors_ScrubsInternalError(t *testing.T) // (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()) + + rawContent := "key=ANTHROPIC_API_KEY=sk-ant-xxxxxxxxxxxxxxxx auth=Bearer ghp_yyyyyyyyyyyyy note=sk-proj-zzzzzzzzzzzzzzzzzzzz" + wantRedacted, changed := redactSecrets("root-1", rawContent) + if !changed { + t.Fatalf("precondition failed — redactSecrets must change the test content; got %q", wantRedacted) + } + if bytes.Contains([]byte(wantRedacted), []byte("sk-ant-xxxxxxxxxxxxxxxx")) { + t.Fatalf("precondition failed — redacted content still contains raw secret: %s", wantRedacted) + } + + w := mcpPost(t, h, "root-1", map[string]interface{}{ + "jsonrpc": "2.0", + "id": 99, + "method": "tools/call", + "params": map[string]interface{}{ + "name": "commit_memory", + "arguments": map[string]interface{}{ + "content": rawContent, + "scope": "LOCAL", + }, + }, + }) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + var resp mcpResponse + 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) + } + + // 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") + } + if captured.Content == rawContent { + t.Errorf("legacy commit_memory leaked raw secret to plugin: %q", captured.Content) + } + 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) + } +} + // ───────────────────────────────────────────────────────────────────────────── // tools/call — recall_memory // ───────────────────────────────────────────────────────────────────────────── -// TestMCPHandler_RecallMemory_RequestErrors_ScrubsInternalError mirrors the -// commit_memory scrub test on the recall_memory path. After A1 (issue -// #1733) the upstream error is "memory plugin is not configured" rather -// than the GLOBAL-scope block, but the scrub layer (mcp.go dispatchRPC) -// still must replace it with the constant "tool call failed". -func TestMCPHandler_RecallMemory_RequestErrors_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{}{ -- 2.52.0