Merge pull request #2757 from Molecule-AI/fix/memory-v2-wiring-real-tests

Memory v2 wiring: replace decorative tests with real integration

workspace-server/internal/memory/wiring/wiring_test.go

@@ -2,7 +2,9 @@ package wiring
 
 import (
 	"context"
-	"errors"
+	"net/http"
+	"net/http/httptest"
+	"sync"
 	"testing"
 
 	"github.com/DATA-DOG/go-sqlmock"
@@ -56,57 +58,103 @@ func TestNamespaceCleanupFn_NilPlugin(t *testing.T) {
 	}
 }
 
-// TestNamespaceCleanupFn_NamespaceFormat pins that the closure
-// computes the right namespace string for a workspace-id input
-// ("workspace:<id>") — this is the contract the plugin expects and
-// the I5 fixup tests rely on.
+// TestNamespaceCleanupFn_HitsPluginAtCorrectNamespace is the real
+// integration gate for the closure: it spins up an httptest.Server
+// that records every DELETE request, points MEMORY_PLUGIN_URL at it,
+// runs Build(), then invokes the returned closure and asserts the
+// server saw `DELETE /v1/namespaces/workspace:<id>`.
 //
-// We can't easily inject a mock plugin into Build's output (it
-// constructs a real *mclient.Client). Instead we verify behavior
-// indirectly: a closure-returning helper with a stub plugin would be
-// ideal, but the goal here is to pin the namespace-string format so
-// future refactors don't silently break it. Direct test of the
-// underlying string is the cheapest gate.
-func TestNamespaceCleanupFn_NamespaceFormat(t *testing.T) {
-	// Build a closure that records the namespace it was called with.
-	// We test the FORMAT directly because the closure inside
-	// NamespaceCleanupFn is an internal-only string concatenation.
-	want := "workspace:abc-123"
-	got := "workspace:" + "abc-123"
-	if got != want {
-		t.Errorf("namespace format drift: got %q, want %q", got, want)
+// This replaces two earlier tests that exercised parallel
+// implementations rather than the production closure (caught in
+// self-review).
+func TestNamespaceCleanupFn_HitsPluginAtCorrectNamespace(t *testing.T) {
+	var (
+		mu          sync.Mutex
+		gotPaths    []string
+		gotMethods  []string
+	)
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		mu.Lock()
+		gotPaths = append(gotPaths, r.URL.Path)
+		gotMethods = append(gotMethods, r.Method)
+		mu.Unlock()
+		switch r.URL.Path {
+		case "/v1/health":
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"status":"ok","version":"1.0.0","capabilities":[]}`))
+		default:
+			w.WriteHeader(http.StatusNoContent)
+		}
+	}))
+	t.Cleanup(srv.Close)
+
+	t.Setenv("MEMORY_PLUGIN_URL", srv.URL)
+	db, _, _ := sqlmock.New()
+	defer db.Close()
+
+	bundle := Build(db)
+	if bundle == nil {
+		t.Fatal("Build returned nil with MEMORY_PLUGIN_URL set")
+	}
+	cleanup := bundle.NamespaceCleanupFn()
+	if cleanup == nil {
+		t.Fatal("NamespaceCleanupFn returned nil with non-nil Plugin")
+	}
+
+	cleanup(context.Background(), "abc-123")
+
+	mu.Lock()
+	defer mu.Unlock()
+	// Two requests expected: /v1/health probe at Boot + DELETE for cleanup.
+	foundDelete := false
+	for i, p := range gotPaths {
+		if gotMethods[i] == "DELETE" && p == "/v1/namespaces/workspace:abc-123" {
+			foundDelete = true
+		}
+	}
+	if !foundDelete {
+		t.Errorf("expected DELETE /v1/namespaces/workspace:abc-123, got %v",
+			pathsAndMethods(gotPaths, gotMethods))
 	}
 }
 
-// stubPluginCallTracker is for the integration-shaped test below.
-type stubPluginCallTracker struct {
-	called []string
-	err    error
-}
+// TestNamespaceCleanupFn_PluginErrorDoesNotPanic exercises the failure
+// path for real: server returns 500 on DELETE; the closure must log
+// and return without propagating. Replaces the parallel-implementation
+// version that didn't actually test the production code.
+func TestNamespaceCleanupFn_PluginErrorDoesNotPanic(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/v1/health" {
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"status":"ok","version":"1.0.0","capabilities":[]}`))
+			return
+		}
+		http.Error(w, "boom", http.StatusInternalServerError)
+	}))
+	t.Cleanup(srv.Close)
 
-func (s *stubPluginCallTracker) Delete(_ context.Context, ns string) error {
-	s.called = append(s.called, ns)
-	return s.err
-}
+	t.Setenv("MEMORY_PLUGIN_URL", srv.URL)
+	db, _, _ := sqlmock.New()
+	defer db.Close()
 
-// TestNamespaceCleanupFn_FailureLogsButReturns: simulate the
-// production behavior where a plugin DeleteNamespace fails. The
-// closure logs and returns; it MUST NOT panic or propagate.
-//
-// Direct test of the bundle's closure isn't feasible without
-// dependency injection at the Bundle struct level. We test the
-// behavioral contract via a parallel implementation instead — the
-// callsite in workspace_crud.go calls the closure unconditionally,
-// so any panic would be production-visible.
-func TestNamespaceCleanupFn_FailureLogsButReturns(t *testing.T) {
-	tracker := &stubPluginCallTracker{err: errors.New("plugin dead")}
-	// Mirror the closure's logic against the stub.
-	cleanup := func(ctx context.Context, workspaceID string) {
-		ns := "workspace:" + workspaceID
-		_ = tracker.Delete(ctx, ns) // production logs but doesn't propagate
-	}
+	bundle := Build(db)
+	cleanup := bundle.NamespaceCleanupFn()
+
+	// Must not panic, must not propagate the 500. Recovering with
+	// defer is belt-and-suspenders — production calls this from a
+	// for-loop in workspace_crud.go that has no recover.
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("cleanup panicked on plugin 500: %v", r)
+		}
+	}()
 	cleanup(context.Background(), "ws-1")
-	if len(tracker.called) != 1 || tracker.called[0] != "workspace:ws-1" {
-		t.Errorf("called = %v, want [workspace:ws-1]", tracker.called)
-	}
+}
+
+func pathsAndMethods(paths, methods []string) []string {
+	out := make([]string, len(paths))
+	for i := range paths {
+		out[i] = methods[i] + " " + paths[i]
+	}
+	return out
 }