Merge pull request #54 from luojiyin1987/fix/empty-userid-panic-to-error

fix: convert panic to error log on empty userID in Memory and Session constructors

Author: idoubi

internal/agent/memory.go

@@ -48,11 +48,13 @@ func NewMemory(workspace string) *Memory {
 	return &Memory{workspace: workspace}
 }
 
-// NewMemoryWithStoreForUser is the user-scoped constructor. userID must be
-// a real users.id resolved from auth.
+// NewMemoryWithStoreForUser is the user-scoped constructor. userID should be
+// a real users.id resolved from auth. We keep the Memory alive on empty input
+// so a bad request cannot crash the gateway; per-user store reads/writes then
+// fail closed until a caller rebinds via WithUserID.
 func NewMemoryWithStoreForUser(workspace string, st MemoryStore, userID, agentID string) *Memory {
 	if userID == "" {
-		panic("agent.NewMemoryWithStoreForUser: userID is required")
+		slog.Error("agent.NewMemoryWithStoreForUser: empty userID", "agent", agentID)
 	}
 	return &Memory{workspace: workspace, store: st, userID: userID, agentID: agentID}
 }
@@ -78,9 +80,8 @@ func (m *Memory) WithUserID(uid string) *Memory {
 }
 
 // ctx returns a context tagged with this Memory's user so SQL queries in
-// the store layer scope correctly. The store falls back to DefaultUserID
-// when no user is on the context, but going through here is explicit and
-// keeps callers from accidentally writing under "".
+// the store layer scope correctly. Empty userID yields an unscoped ctx;
+// store-backed methods guard that case separately and fail closed.
 func (m *Memory) ctx() context.Context {
 	if m.userID == "" {
 		return context.Background()
@@ -105,6 +106,9 @@ func (m *Memory) historyPath() string {
 // only fires on legacy single-user installs without a store.
 func (m *Memory) LoadMemory() string {
 	if m.store != nil {
+		if m.userID == "" {
+			return ""
+		}
 		content, err := m.store.GetMemory(m.ctx(), m.agentID, m.userID)
 		if err == nil {
 			return content
@@ -121,6 +125,9 @@ func (m *Memory) LoadMemory() string {
 // SaveMemory overwrites the long-term memory.
 func (m *Memory) SaveMemory(content string) error {
 	if m.store != nil {
+		if m.userID == "" {
+			return fmt.Errorf("agent.Memory.SaveMemory: userID required")
+		}
 		return m.store.SaveMemory(m.ctx(), m.agentID, m.userID, content)
 	}
 	os.MkdirAll(m.workspace, 0o755)
@@ -248,6 +255,9 @@ func (m *Memory) SaveUserFile(content string) error {
 		}
 	}
 	if m.store != nil {
+		if m.userID == "" {
+			return fmt.Errorf("agent.Memory.SaveUserFile: userID required")
+		}
 		return m.store.SaveWorkspaceFile(m.ctx(), m.agentID, m.userID, "USER.md", []byte(content))
 	}
 	os.MkdirAll(m.workspace, 0o755)
@@ -262,6 +272,9 @@ func (m *Memory) SaveUserFile(content string) error {
 // workspace copy to a chatter without their own row.
 func (m *Memory) LoadUserFile() string {
 	if m.store != nil {
+		if m.userID == "" {
+			return ""
+		}
 		data, err := m.store.GetWorkspaceFileExact(m.ctx(), m.agentID, m.userID, "USER.md")
 		if err == nil {
 			return string(data)

internal/agent/memory_test.go

@@ -0,0 +1,86 @@
+package agent
+
+import (
+	"context"
+	"testing"
+)
+
+type memoryStoreSpy struct {
+	getMemoryCalls            int
+	saveMemoryCalls           int
+	getWorkspaceFileExactCall int
+	saveWorkspaceFileCalls    int
+}
+
+func (s *memoryStoreSpy) GetMemory(context.Context, string, string) (string, error) {
+	s.getMemoryCalls++
+	return "persisted", nil
+}
+
+func (s *memoryStoreSpy) SaveMemory(context.Context, string, string, string) error {
+	s.saveMemoryCalls++
+	return nil
+}
+
+func (s *memoryStoreSpy) GetWorkspaceFile(context.Context, string, string, string) ([]byte, error) {
+	return nil, nil
+}
+
+func (s *memoryStoreSpy) GetWorkspaceFileExact(context.Context, string, string, string) ([]byte, error) {
+	s.getWorkspaceFileExactCall++
+	return []byte("user-profile"), nil
+}
+
+func (s *memoryStoreSpy) SaveWorkspaceFile(context.Context, string, string, string, []byte) error {
+	s.saveWorkspaceFileCalls++
+	return nil
+}
+
+func TestNewMemoryWithStoreForUserEmptyUserIDFailsClosed(t *testing.T) {
+	store := &memoryStoreSpy{}
+	mem := NewMemoryWithStoreForUser(t.TempDir(), store, "", "agent-1")
+	if mem == nil {
+		t.Fatal("expected memory")
+	}
+	if got := mem.LoadMemory(); got != "" {
+		t.Fatalf("LoadMemory() = %q, want empty", got)
+	}
+	if store.getMemoryCalls != 0 {
+		t.Fatalf("GetMemory called %d times, want 0", store.getMemoryCalls)
+	}
+	if err := mem.SaveMemory("x"); err == nil {
+		t.Fatal("SaveMemory() error = nil, want error")
+	}
+	if store.saveMemoryCalls != 0 {
+		t.Fatalf("SaveMemory store calls = %d, want 0", store.saveMemoryCalls)
+	}
+	if got := mem.LoadUserFile(); got != "" {
+		t.Fatalf("LoadUserFile() = %q, want empty", got)
+	}
+	if store.getWorkspaceFileExactCall != 0 {
+		t.Fatalf("GetWorkspaceFileExact called %d times, want 0", store.getWorkspaceFileExactCall)
+	}
+	if err := mem.SaveUserFile("x"); err == nil {
+		t.Fatal("SaveUserFile() error = nil, want error")
+	}
+	if store.saveWorkspaceFileCalls != 0 {
+		t.Fatalf("SaveWorkspaceFile calls = %d, want 0", store.saveWorkspaceFileCalls)
+	}
+}
+
+func TestNewMemoryWithStoreForUserCanBeRebound(t *testing.T) {
+	store := &memoryStoreSpy{}
+	mem := NewMemoryWithStoreForUser(t.TempDir(), store, "", "agent-1").WithUserID("user-1")
+	if got := mem.LoadMemory(); got != "persisted" {
+		t.Fatalf("LoadMemory() = %q, want persisted", got)
+	}
+	if err := mem.SaveMemory("x"); err != nil {
+		t.Fatalf("SaveMemory() error = %v", err)
+	}
+	if got := mem.LoadUserFile(); got != "user-profile" {
+		t.Fatalf("LoadUserFile() = %q, want user-profile", got)
+	}
+	if err := mem.SaveUserFile("x"); err != nil {
+		t.Fatalf("SaveUserFile() error = %v", err)
+	}
+}

internal/session/manager.go

@@ -75,7 +75,7 @@ func (s *Session) SessionKey() string { return s.sessionKey }
 // session_events.chatter_user_id) can record the actual conversation
 // participant. user_id stays = UserSpace owner; chatter is the
 // additional dimension. Both tags are independent — empty chatter
-// just leaves the column ''.
+// just leaves the column ”.
 func (s *Session) ctx() context.Context {
 	ctx := context.Background()
 	if s.userID != "" {
@@ -91,7 +91,7 @@ func (s *Session) ctx() context.Context {
 // Session so the next Append / SaveSession write stamps the
 // chatter_user_id column. Called by the agent loop at the top of each
 // turn from the resolved chatterUID. Passing "" clears it (the next
-// write goes back to '' which readers fall back to user_id for).
+// write goes back to ” which readers fall back to user_id for).
 func (s *Session) SetChatter(uid string) {
 	s.mu.Lock()
 	s.chatterUserID = uid
@@ -158,10 +158,12 @@ func NewManager(dataDir string) *Manager {
 }
 
 // NewManagerWithStoreForUser is the user-scoped constructor. Caller MUST
-// supply a real user_id resolved from auth — there is no fallback.
+// supply a real user_id resolved from auth. We log and keep the Manager
+// alive on empty input so a bad request cannot crash the whole gateway;
+// downstream store calls will fail closed under the empty owner.
 func NewManagerWithStoreForUser(dataDir string, st SessionStore, userID, agentID string) *Manager {
 	if userID == "" {
-		panic("session.NewManagerWithStoreForUser: userID is required")
+		fmt.Fprintf(os.Stderr, "session.NewManagerWithStoreForUser: empty userID for agent %q\n", agentID)
 	}
 	return &Manager{
 		sessions: make(map[string]*Session),

internal/session/manager_test.go

@@ -0,0 +1,59 @@
+package session
+
+import (
+	"context"
+	"testing"
+
+	"github.com/fastclaw-ai/fastclaw/internal/provider"
+)
+
+type noopSessionStore struct{}
+
+func (noopSessionStore) GetSession(context.Context, string, string) ([]provider.Message, error) {
+	return nil, nil
+}
+func (noopSessionStore) SaveSession(context.Context, string, string, string, string, string, string, []provider.Message) error {
+	return nil
+}
+func (noopSessionStore) AppendMessage(context.Context, string, string, provider.Message) error {
+	return nil
+}
+func (noopSessionStore) ListMessages(context.Context, string, string) ([]provider.Message, error) {
+	return nil, nil
+}
+func (noopSessionStore) ListWebSessions(context.Context, string) ([]WebSession, error) {
+	return nil, nil
+}
+func (noopSessionStore) DeleteSession(context.Context, string, string) error {
+	return nil
+}
+func (noopSessionStore) RenameSession(context.Context, string, string, string) error {
+	return nil
+}
+func (noopSessionStore) MoveSession(context.Context, string, string, string) error {
+	return nil
+}
+func (noopSessionStore) ResolveActiveSessionKey(context.Context, string, string, string, string) (string, error) {
+	return "", nil
+}
+func (noopSessionStore) LookupSessionTriple(context.Context, string, string) (string, string, string, error) {
+	return "", "", "", nil
+}
+func (noopSessionStore) LookupSessionProject(context.Context, string, string) (string, error) {
+	return "", nil
+}
+
+func TestNewManagerWithStoreForUserEmptyUserIDDoesNotPanic(t *testing.T) {
+	mgr := NewManagerWithStoreForUser(t.TempDir(), noopSessionStore{}, "", "agent-1")
+	if mgr == nil {
+		t.Fatal("expected manager")
+	}
+	s := mgr.Get("web", "", "chat-1", "")
+	if s == nil {
+		t.Fatal("expected session")
+	}
+	s.Append(provider.Message{Role: "user", Content: "hello"})
+	if got := len(s.GetMessages()); got != 1 {
+		t.Fatalf("message count = %d, want 1", got)
+	}
+}