diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go index 4047ab74d..3636e6c78 100644 --- a/pkg/agent/agent_test.go +++ b/pkg/agent/agent_test.go @@ -266,6 +266,85 @@ func TestProcessMessage_IncludesCurrentSenderInDynamicContext(t *testing.T) { } } +// TestProcessMessage_AttachesSenderAttributionToUserMessage exercises the +// end-to-end flow for issue #2702: a real-user inbound message reaches +// the provider with a sanitized Name field so multi-user conversations +// can be disambiguated by the model. The sanitization step is what makes +// the value safe for OpenAI's `name` constraint (`^[a-zA-Z0-9_-]{1,64}$`). +func TestProcessMessage_AttachesSenderAttributionToUserMessage(t *testing.T) { + tmpDir := t.TempDir() + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + ModelName: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + msgBus := bus.NewMessageBus() + provider := &recordingProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + if _, err := al.processMessage(context.Background(), testInboundMessage(bus.InboundMessage{ + Channel: "discord", + SenderID: "discord:123", + Sender: bus.SenderInfo{DisplayName: "Alice"}, + ChatID: "group-1", + Content: "My name is Alice", + })); err != nil { + t.Fatalf("processMessage(): %v", err) + } + + if len(provider.lastMessages) == 0 { + t.Fatal("provider received no messages") + } + last := provider.lastMessages[len(provider.lastMessages)-1] + if last.Role != "user" || last.Content != "My name is Alice" { + t.Fatalf("last message = %+v, want user/My name is Alice", last) + } + // "discord:123" → "discord_123" (colon disallowed in OpenAI name). + if last.Name != "discord_123" { + t.Errorf("last.Name = %q, want discord_123 (sanitized)", last.Name) + } +} + +// TestProcessMessage_NoSenderAttributionForCronTrigger verifies that +// synthetic trigger sources (cron/heartbeat/async callbacks) do not +// produce per-message sender attribution. Otherwise an Anthropic adapter +// would render `[cron] [System: cron] ...` double prefixes. +func TestProcessMessage_NoSenderAttributionForCronTrigger(t *testing.T) { + tmpDir := t.TempDir() + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + ModelName: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + msgBus := bus.NewMessageBus() + provider := &recordingProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + if _, err := al.ProcessDirectWithChannel( + context.Background(), "scheduled task", "agent:main:cli:direct:cron", "cli", "direct", + ); err != nil { + t.Fatalf("ProcessDirectWithChannel(): %v", err) + } + + if len(provider.lastMessages) == 0 { + t.Fatal("provider received no messages") + } + last := provider.lastMessages[len(provider.lastMessages)-1] + if last.Name != "" { + t.Errorf("last.Name = %q, want empty for cron-triggered message", last.Name) + } +} + func TestProcessMessage_UseCommandLoadsRequestedSkill(t *testing.T) { tmpDir := t.TempDir() skillDir := filepath.Join(tmpDir, "skills", "shell") diff --git a/pkg/agent/context.go b/pkg/agent/context.go index ecde7c33e..c6b4a1e2a 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -806,7 +806,7 @@ func (cb *ContextBuilder) BuildMessagesFromPrompt(req PromptBuildRequest) []prov // multimodal providers receive the uploaded image even when the user sends // no accompanying text. if strings.TrimSpace(req.CurrentMessage) != "" || len(req.Media) > 0 { - messages = append(messages, userPromptMessage(req.CurrentMessage, req.Media)) + messages = append(messages, userPromptMessage(req.CurrentMessage, req.Media, req.SenderID)) } return messages diff --git a/pkg/agent/pipeline_setup.go b/pkg/agent/pipeline_setup.go index 219e4e5de..fa6dff530 100644 --- a/pkg/agent/pipeline_setup.go +++ b/pkg/agent/pipeline_setup.go @@ -69,12 +69,11 @@ func (p *Pipeline) SetupTurn(ctx context.Context, ts *turnState) (*turnExecution } if !ts.opts.NoHistory && (strings.TrimSpace(ts.userMessage) != "" || len(ts.media) > 0) { - rootMsg := userPromptMessage(ts.userMessage, ts.media) - if len(rootMsg.Media) > 0 { - ts.agent.Sessions.AddFullMessage(ts.sessionKey, rootMsg) - } else { - ts.agent.Sessions.AddMessage(ts.sessionKey, rootMsg.Role, rootMsg.Content) - } + rootMsg := userPromptMessage(ts.userMessage, ts.media, ts.opts.Dispatch.SenderID()) + // AddFullMessage preserves all message fields (Name, Media, etc.). + // AddMessage(role, content) would silently drop Name attribution, + // breaking multi-user history disambiguation. + ts.agent.Sessions.AddFullMessage(ts.sessionKey, rootMsg) ts.recordPersistedMessage(rootMsg) ts.ingestMessage(ctx, p.al, rootMsg) } diff --git a/pkg/agent/prompt_turn.go b/pkg/agent/prompt_turn.go index 588a8f00f..578d97ea5 100644 --- a/pkg/agent/prompt_turn.go +++ b/pkg/agent/prompt_turn.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/sipeed/picoclaw/pkg/providers" + "github.com/sipeed/picoclaw/pkg/providers/messageutil" ) func promptBuildRequestForTurn( @@ -95,11 +96,20 @@ func promptMessageWithDefaultMetadata( return promptMessageWithMetadata(msg, layer, slot, source) } -func userPromptMessage(content string, media []string) providers.Message { +// userPromptMessage builds the per-turn user message that gets persisted +// to session history and sent to the LLM. senderID is the raw inbound +// sender ID; it is sanitized into the OpenAI-compatible Name field unless +// it identifies a system trigger (cron, heartbeat, async callback), in +// which case Name is left empty so synthetic events do not appear as +// distinct human users in the conversation. +func userPromptMessage(content string, media []string, senderID string) providers.Message { msg := providers.Message{ Role: "user", Content: content, } + if !messageutil.IsSystemSenderID(senderID) { + msg.Name = messageutil.SanitizeMessageName(senderID) + } if len(media) > 0 { msg.Media = append([]string(nil), media...) } diff --git a/pkg/agent/prompt_turn_test.go b/pkg/agent/prompt_turn_test.go new file mode 100644 index 000000000..f043d519a --- /dev/null +++ b/pkg/agent/prompt_turn_test.go @@ -0,0 +1,83 @@ +// PicoClaw - Ultra-lightweight personal AI agent +// License: MIT +// +// Copyright (c) 2026 PicoClaw contributors + +package agent + +import ( + "testing" +) + +// TestUserPromptMessage_AttachesSanitizedSenderName verifies that +// userPromptMessage stores a sanitized sender attribution in +// providers.Message.Name when given a real human sender. This is the +// core wiring change for issue #2702. +func TestUserPromptMessage_AttachesSanitizedSenderName(t *testing.T) { + tests := []struct { + name string + senderID string + wantName string + }{ + {"slack-style id", "U07AB12C3DEF", "U07AB12C3DEF"}, + {"discord-style id sanitized", "alice#1234", "alice_1234"}, + {"telegram numeric", "141455495", "141455495"}, + {"unicode display becomes empty", "李华", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + msg := userPromptMessage("hello", nil, tt.senderID) + if msg.Role != "user" { + t.Errorf("Role = %q, want user", msg.Role) + } + if msg.Content != "hello" { + t.Errorf("Content = %q, want hello", msg.Content) + } + if msg.Name != tt.wantName { + t.Errorf("Name = %q, want %q", msg.Name, tt.wantName) + } + }) + } +} + +// TestUserPromptMessage_SkipsAttributionForSystemSenders verifies that +// synthetic trigger sources (cron, heartbeat, async callbacks) do not +// produce per-message attribution. These are not distinct human users, +// so leaking them into Name would create spurious "user identities" in +// the OpenAI wire format and a confusing `[cron] [System: ...]` double +// prefix on Anthropic-style adapters. +func TestUserPromptMessage_SkipsAttributionForSystemSenders(t *testing.T) { + systemIDs := []string{"", "cron", "heartbeat", "system", "async:read_file"} + for _, id := range systemIDs { + t.Run(id, func(t *testing.T) { + msg := userPromptMessage("triggered work", nil, id) + if msg.Name != "" { + t.Errorf("Name = %q, want empty for synthetic sender %q", msg.Name, id) + } + }) + } +} + +// TestUserPromptMessage_NoNameMeansNoAttribution covers the empty-sender +// path explicitly so future refactors don't accidentally start tagging +// anonymous direct-channel turns. +func TestUserPromptMessage_NoNameMeansNoAttribution(t *testing.T) { + msg := userPromptMessage("anonymous message", nil, "") + if msg.Name != "" { + t.Errorf("Name = %q, want empty for anonymous sender", msg.Name) + } +} + +// TestUserPromptMessage_PreservesMedia is a regression guard: the +// existing media-passthrough behavior must continue to work after the +// signature change. +func TestUserPromptMessage_PreservesMedia(t *testing.T) { + media := []string{"data:image/png;base64,abc"} + msg := userPromptMessage("look", media, "alice") + if len(msg.Media) != 1 || msg.Media[0] != media[0] { + t.Errorf("Media = %v, want %v", msg.Media, media) + } + if msg.Name != "alice" { + t.Errorf("Name = %q, want alice", msg.Name) + } +}