review(memory): #1747 — OFFSEC test depth + legacy redaction + cutover env deprecation

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) <noreply@anthropic.com>

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}"

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{}{