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) + } +} diff --git a/pkg/providers/anthropic/provider.go b/pkg/providers/anthropic/provider.go index 6f4aadb8b..a3704cdf3 100644 --- a/pkg/providers/anthropic/provider.go +++ b/pkg/providers/anthropic/provider.go @@ -11,6 +11,7 @@ import ( "github.com/anthropics/anthropic-sdk-go/option" "github.com/sipeed/picoclaw/pkg/providers/common" + "github.com/sipeed/picoclaw/pkg/providers/messageutil" "github.com/sipeed/picoclaw/pkg/providers/protocoltypes" ) @@ -170,8 +171,11 @@ func buildParams( anthropic.NewUserMessage(anthropic.NewToolResultBlock(msg.ToolCallID, msg.Content, false)), ) } else { + // Anthropic has no per-message author identity; render any + // sender attribution as a `[name] ` prefix without mutating + // the persisted message. anthropicMessages = append(anthropicMessages, - anthropic.NewUserMessage(anthropic.NewTextBlock(msg.Content)), + anthropic.NewUserMessage(anthropic.NewTextBlock(messageutil.ApplyUserNamePrefix(msg))), ) } case "assistant": diff --git a/pkg/providers/anthropic_messages/provider.go b/pkg/providers/anthropic_messages/provider.go index 672fb9324..6cab762d9 100644 --- a/pkg/providers/anthropic_messages/provider.go +++ b/pkg/providers/anthropic_messages/provider.go @@ -17,6 +17,7 @@ import ( "time" "github.com/sipeed/picoclaw/pkg/providers/common" + "github.com/sipeed/picoclaw/pkg/providers/messageutil" "github.com/sipeed/picoclaw/pkg/providers/protocoltypes" ) @@ -213,10 +214,13 @@ func buildRequestBody( "content": []map[string]any{toolResultBlock}, }) } else { - // Regular user message + // Regular user message. Anthropic has no per-message author + // identity, so any sender attribution from msg.Name is rendered + // as a `[name] ` prefix on the content. The persisted message + // is not mutated — only the wire payload carries the prefix. apiMessages = append(apiMessages, map[string]any{ "role": "user", - "content": msg.Content, + "content": messageutil.ApplyUserNamePrefix(msg), }) } diff --git a/pkg/providers/anthropic_messages/provider_test.go b/pkg/providers/anthropic_messages/provider_test.go index 6401d84bd..a756c0b19 100644 --- a/pkg/providers/anthropic_messages/provider_test.go +++ b/pkg/providers/anthropic_messages/provider_test.go @@ -717,3 +717,95 @@ func TestProviderChatErrors(t *testing.T) { }) } } + +// TestBuildRequestBody_UserNamePrefixed verifies that sender attribution +// (Message.Name) is rendered as a `[name] ` prefix on the user content +// sent to the Anthropic Messages API. Anthropic has no native per-message +// author identity, so prefixing is the wire-level fallback. +func TestBuildRequestBody_UserNamePrefixed(t *testing.T) { + messages := []Message{ + {Role: "user", Content: "My name is Alice", Name: "U07AB12C3DEF"}, + } + body, err := buildRequestBody(messages, nil, "test-model", map[string]any{"max_tokens": 1024}) + if err != nil { + t.Fatalf("buildRequestBody: %v", err) + } + apiMessages, ok := body["messages"].([]any) + if !ok || len(apiMessages) != 1 { + t.Fatalf("messages = %T (%d), want 1-element []any", body["messages"], len(apiMessages)) + } + first, ok := apiMessages[0].(map[string]any) + if !ok { + t.Fatalf("messages[0] = %T, want map[string]any", apiMessages[0]) + } + got := first["content"] + want := "[U07AB12C3DEF] My name is Alice" + if got != want { + t.Errorf("content = %q, want %q", got, want) + } +} + +// TestBuildRequestBody_NoNameNoPrefix verifies that messages without +// sender attribution flow through the wire path unchanged, preserving +// backward compatibility for direct (single-user) channels. +func TestBuildRequestBody_NoNameNoPrefix(t *testing.T) { + messages := []Message{ + {Role: "user", Content: "Hello, world!"}, + } + body, err := buildRequestBody(messages, nil, "test-model", map[string]any{"max_tokens": 1024}) + if err != nil { + t.Fatalf("buildRequestBody: %v", err) + } + apiMessages := body["messages"].([]any) + first := apiMessages[0].(map[string]any) + if first["content"] != "Hello, world!" { + t.Errorf("content = %q, want unchanged %q", first["content"], "Hello, world!") + } +} + +// TestBuildRequestBody_ToolResultNotPrefixed verifies that tool result +// messages (msg.ToolCallID set) are never prefixed even if Name is +// somehow set, because tool results are not user utterances. +func TestBuildRequestBody_ToolResultNotPrefixed(t *testing.T) { + messages := []Message{ + {Role: "assistant", Content: "Let me check"}, + {Role: "user", Content: `{"temp":72}`, ToolCallID: "call_1", Name: "alice"}, + } + body, err := buildRequestBody(messages, nil, "test-model", map[string]any{"max_tokens": 1024}) + if err != nil { + t.Fatalf("buildRequestBody: %v", err) + } + data, _ := json.Marshal(body) + // The tool_result block must contain the raw content, not a prefixed form. + if !strings.Contains(string(data), `"content":"{\"temp\":72}"`) { + t.Errorf("expected tool_result content to be raw JSON, got: %s", string(data)) + } + if strings.Contains(string(data), `[alice]`) { + t.Errorf("tool_result content should not carry sender prefix, got: %s", string(data)) + } +} + +// TestBuildRequestBody_MultiUserHistory verifies the issue #2702 scenario: +// two distinct users in the same session each get their own attributed +// message in the wire payload, so the model can disambiguate who said what. +func TestBuildRequestBody_MultiUserHistory(t *testing.T) { + messages := []Message{ + {Role: "user", Content: "My name is Alice", Name: "U_alice"}, + {Role: "assistant", Content: "Hi Alice!"}, + {Role: "user", Content: "What's my name?", Name: "U_bob"}, + } + body, err := buildRequestBody(messages, nil, "test-model", map[string]any{"max_tokens": 1024}) + if err != nil { + t.Fatalf("buildRequestBody: %v", err) + } + apiMessages := body["messages"].([]any) + if len(apiMessages) != 3 { + t.Fatalf("len(messages) = %d, want 3", len(apiMessages)) + } + if got := apiMessages[0].(map[string]any)["content"]; got != "[U_alice] My name is Alice" { + t.Errorf("messages[0].content = %q, want prefixed Alice content", got) + } + if got := apiMessages[2].(map[string]any)["content"]; got != "[U_bob] What's my name?" { + t.Errorf("messages[2].content = %q, want prefixed Bob content", got) + } +} diff --git a/pkg/providers/bedrock/provider_bedrock.go b/pkg/providers/bedrock/provider_bedrock.go index 3798c5fd8..b5221d4fc 100644 --- a/pkg/providers/bedrock/provider_bedrock.go +++ b/pkg/providers/bedrock/provider_bedrock.go @@ -27,6 +27,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/bedrockruntime/types" "github.com/sipeed/picoclaw/pkg/providers/common" + "github.com/sipeed/picoclaw/pkg/providers/messageutil" "github.com/sipeed/picoclaw/pkg/providers/protocoltypes" ) @@ -319,13 +320,19 @@ func convertMessages(messages []Message) ([]types.Message, []types.SystemContent } // buildUserContent builds Bedrock content blocks for a user message. +// +// Sender attribution from msg.Name is rendered as a `[name] ` prefix on +// the text block. Bedrock's Converse API has no per-message author identity +// shared across all model families, so prefixing keeps the multi-user +// attribution behavior consistent with the Anthropic adapter regardless +// of which underlying model the request lands on. func buildUserContent(msg Message) []types.ContentBlock { var content []types.ContentBlock // Add text content - if msg.Content != "" { + if textValue := messageutil.ApplyUserNamePrefix(msg); textValue != "" { content = append(content, &types.ContentBlockMemberText{ - Value: msg.Content, + Value: textValue, }) } diff --git a/pkg/providers/bedrock/provider_bedrock_test.go b/pkg/providers/bedrock/provider_bedrock_test.go index 38a5e26da..c897177d0 100644 --- a/pkg/providers/bedrock/provider_bedrock_test.go +++ b/pkg/providers/bedrock/provider_bedrock_test.go @@ -294,6 +294,42 @@ func TestBuildUserContent_SkipsNonBase64Data(t *testing.T) { assert.Len(t, content, 1) } +// TestBuildUserContent_UserNamePrefixed verifies that sender attribution +// from Message.Name is rendered as a `[name] ` prefix on the Bedrock text +// block. Bedrock's Converse API has no per-message author identity shared +// across model families, so prefixing keeps multi-user disambiguation +// consistent regardless of the underlying model (Claude, Llama, etc.). +func TestBuildUserContent_UserNamePrefixed(t *testing.T) { + msg := Message{ + Role: "user", + Content: "My name is Alice", + Name: "U_alice", + } + + content := buildUserContent(msg) + + assert.Len(t, content, 1) + textBlock, ok := content[0].(*types.ContentBlockMemberText) + require.True(t, ok) + assert.Equal(t, "[U_alice] My name is Alice", textBlock.Value) +} + +// TestBuildUserContent_NoNameNoPrefix protects backward compatibility for +// direct (single-user) channels: no sender, no prefix. +func TestBuildUserContent_NoNameNoPrefix(t *testing.T) { + msg := Message{ + Role: "user", + Content: "Hello", + } + + content := buildUserContent(msg) + + assert.Len(t, content, 1) + textBlock, ok := content[0].(*types.ContentBlockMemberText) + require.True(t, ok) + assert.Equal(t, "Hello", textBlock.Value) +} + func TestBuildAssistantContent_SkipsEmptyToolName(t *testing.T) { msg := Message{ Content: "Response", diff --git a/pkg/providers/common/common.go b/pkg/providers/common/common.go index 5e03bc0c2..0ee4f5b4e 100644 --- a/pkg/providers/common/common.go +++ b/pkg/providers/common/common.go @@ -69,9 +69,15 @@ func NewHTTPClient(proxy string) *http.Client { // openaiMessage is the wire-format message for OpenAI-compatible APIs. // It mirrors protocoltypes.Message but omits SystemParts, which is an // internal field that would be unknown to third-party endpoints. +// +// Name carries the OpenAI Chat Completions optional `name` field, used to +// disambiguate participants in multi-user conversations. It is emitted +// only when set; the upstream value has already been sanitized to match +// the API constraint `^[a-zA-Z0-9_-]{1,64}$`. type openaiMessage struct { Role string `json:"role"` Content string `json:"content"` + Name string `json:"name,omitempty"` ReasoningContent string `json:"reasoning_content,omitempty"` ToolCalls []openaiToolCall `json:"tool_calls,omitempty"` ToolCallID string `json:"tool_call_id,omitempty"` @@ -92,7 +98,8 @@ type openaiFunctionCall struct { // SerializeMessages converts internal Message structs to the OpenAI wire format. // - Strips SystemParts (unknown to third-party endpoints) // - Converts messages with Media to multipart content format (text + image_url parts) -// - Preserves ToolCallID, ToolCalls, and ReasoningContent for all messages +// - Preserves ToolCallID, ToolCalls, ReasoningContent, and Name (sender +// attribution for multi-user sessions) for all messages func SerializeMessages(messages []Message) []any { out := make([]any, 0, len(messages)) for _, m := range messages { @@ -101,6 +108,7 @@ func SerializeMessages(messages []Message) []any { out = append(out, openaiMessage{ Role: m.Role, Content: m.Content, + Name: m.Name, ReasoningContent: m.ReasoningContent, ToolCalls: toolCalls, ToolCallID: m.ToolCallID, @@ -142,6 +150,9 @@ func SerializeMessages(messages []Message) []any { "role": m.Role, "content": parts, } + if m.Name != "" { + msg["name"] = m.Name + } if m.ToolCallID != "" { msg["tool_call_id"] = m.ToolCallID } diff --git a/pkg/providers/common/common_test.go b/pkg/providers/common/common_test.go index 3cf2f4285..04b266a3e 100644 --- a/pkg/providers/common/common_test.go +++ b/pkg/providers/common/common_test.go @@ -800,3 +800,101 @@ func TestParseResponse_WithFunctionThoughtSignature(t *testing.T) { ) } } + +// --- SerializeMessages: sender attribution (Name field) tests --- + +// TestSerializeMessages_NameFieldInPlainPath verifies that the OpenAI Chat +// Completions `name` field is emitted on plain-text user messages when +// providers.Message.Name is set. This is the primary multi-user attribution +// channel for OpenAI-compatible endpoints. +func TestSerializeMessages_NameFieldInPlainPath(t *testing.T) { + messages := []Message{ + {Role: "user", Content: "My name is Alice", Name: "U07AB12C3DEF"}, + {Role: "user", Content: "What's my name?", Name: "U99XY99ZZZZZ"}, + } + result := SerializeMessages(messages) + + data, err := json.Marshal(result) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + var msgs []map[string]any + if err := json.Unmarshal(data, &msgs); err != nil { + t.Fatalf("json.Unmarshal: %v", err) + } + if len(msgs) != 2 { + t.Fatalf("len(msgs) = %d, want 2", len(msgs)) + } + if msgs[0]["name"] != "U07AB12C3DEF" { + t.Errorf("msgs[0].name = %v, want U07AB12C3DEF", msgs[0]["name"]) + } + if msgs[1]["name"] != "U99XY99ZZZZZ" { + t.Errorf("msgs[1].name = %v, want U99XY99ZZZZZ", msgs[1]["name"]) + } +} + +// TestSerializeMessages_NameFieldInMultipartPath verifies that the `name` +// field is emitted on the multipart map representation used when a user +// message also carries Media (images/audio). +func TestSerializeMessages_NameFieldInMultipartPath(t *testing.T) { + messages := []Message{ + { + Role: "user", + Content: "look at this", + Name: "alice", + Media: []string{"data:image/png;base64,abc"}, + }, + } + result := SerializeMessages(messages) + + data, err := json.Marshal(result) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + var msgs []map[string]any + if err := json.Unmarshal(data, &msgs); err != nil { + t.Fatalf("json.Unmarshal: %v", err) + } + if msgs[0]["name"] != "alice" { + t.Errorf("msgs[0].name = %v, want alice", msgs[0]["name"]) + } + // Sanity: multipart content array still present + if _, ok := msgs[0]["content"].([]any); !ok { + t.Errorf("msgs[0].content = %T, want array", msgs[0]["content"]) + } +} + +// TestSerializeMessages_NameFieldOmittedWhenEmpty verifies that the `name` +// key is absent (not just empty) for messages without sender attribution, +// preserving wire-level backward compatibility. +func TestSerializeMessages_NameFieldOmittedWhenEmpty(t *testing.T) { + messages := []Message{ + {Role: "user", Content: "hi"}, + } + result := SerializeMessages(messages) + + data, err := json.Marshal(result) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + if strings.Contains(string(data), `"name"`) { + t.Errorf("plain-path serialization should omit empty name; got %s", string(data)) + } +} + +// TestSerializeMessages_NameFieldOmittedWhenEmptyMultipart is the multipart +// counterpart of the omit-when-empty check. +func TestSerializeMessages_NameFieldOmittedWhenEmptyMultipart(t *testing.T) { + messages := []Message{ + {Role: "user", Content: "look", Media: []string{"data:image/png;base64,abc"}}, + } + result := SerializeMessages(messages) + + data, err := json.Marshal(result) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + if strings.Contains(string(data), `"name"`) { + t.Errorf("multipart-path serialization should omit empty name; got %s", string(data)) + } +} diff --git a/pkg/providers/messageutil/messageutil.go b/pkg/providers/messageutil/messageutil.go index c4382d894..cfa57cc5d 100644 --- a/pkg/providers/messageutil/messageutil.go +++ b/pkg/providers/messageutil/messageutil.go @@ -1,11 +1,73 @@ package messageutil import ( + "fmt" + "regexp" "strings" "github.com/sipeed/picoclaw/pkg/providers/protocoltypes" ) +// nameSanitizer matches characters that are not valid in the OpenAI Chat +// Completions `name` field. The OpenAI spec requires `^[a-zA-Z0-9_-]{1,64}$`. +var nameSanitizer = regexp.MustCompile(`[^a-zA-Z0-9_-]+`) + +// maxMessageNameLen mirrors the OpenAI Chat Completions hard limit on the +// `name` field. Values longer than this are truncated. +const maxMessageNameLen = 64 + +// SanitizeMessageName normalizes a raw sender identifier into a value safe +// for the OpenAI Chat Completions `name` field and stable across providers. +// Disallowed characters are coalesced into a single underscore; leading and +// trailing underscores are trimmed; the result is truncated to 64 bytes. +// +// Returns "" when raw is empty or collapses to nothing after sanitization. +// The output is pure ASCII so the truncation is byte-safe. +func SanitizeMessageName(raw string) string { + raw = strings.TrimSpace(raw) + if raw == "" { + return "" + } + cleaned := nameSanitizer.ReplaceAllString(raw, "_") + cleaned = strings.Trim(cleaned, "_") + if cleaned == "" { + return "" + } + if len(cleaned) > maxMessageNameLen { + cleaned = cleaned[:maxMessageNameLen] + } + return cleaned +} + +// IsSystemSenderID reports whether senderID identifies an internal trigger +// (cron, heartbeat, async tool callback, system channel) rather than a real +// human user. These should not propagate as message-level sender attribution +// because they don't represent distinct actors in a multi-user conversation. +func IsSystemSenderID(senderID string) bool { + id := strings.ToLower(strings.TrimSpace(senderID)) + switch id { + case "", "cron", "heartbeat", "system": + return true + } + return strings.HasPrefix(id, "async:") +} + +// ApplyUserNamePrefix returns the message content with a `[name] ` prefix +// when msg carries sender attribution that the calling adapter cannot send +// natively (Anthropic, Bedrock, etc.). The persisted msg is not mutated. +// +// Returns msg.Content unchanged when: +// - the role is not "user" +// - msg.Name is empty +// - msg.ToolCallID is set (tool result, not a user utterance) +// - msg.Content is empty (avoid producing a bare "[name] ") +func ApplyUserNamePrefix(msg protocoltypes.Message) string { + if msg.Role != "user" || msg.Name == "" || msg.ToolCallID != "" || msg.Content == "" { + return msg.Content + } + return fmt.Sprintf("[%s] %s", msg.Name, msg.Content) +} + // IsTransientAssistantThoughtMessage reports whether msg is an invalid // reasoning-only assistant history record. These "hanging" thought messages // are not a canonical persisted format and should be discarded instead of diff --git a/pkg/providers/messageutil/messageutil_test.go b/pkg/providers/messageutil/messageutil_test.go new file mode 100644 index 000000000..30512322d --- /dev/null +++ b/pkg/providers/messageutil/messageutil_test.go @@ -0,0 +1,158 @@ +// PicoClaw - Ultra-lightweight personal AI agent +// License: MIT +// +// Copyright (c) 2026 PicoClaw contributors + +package messageutil + +import ( + "reflect" + "strings" + "testing" + + "github.com/sipeed/picoclaw/pkg/providers/protocoltypes" +) + +func TestSanitizeMessageName(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"empty", "", ""}, + {"whitespace only", " ", ""}, + {"plain ascii", "alice", "alice"}, + {"discord-style id", "alice#1234", "alice_1234"}, + {"telegram numeric id", "141455495", "141455495"}, + {"slack-style id", "U07AB12C3DEF", "U07AB12C3DEF"}, + {"already valid mixed case", "Alice_Bob-99", "Alice_Bob-99"}, + {"strips leading/trailing underscores", "@alice@", "alice"}, + {"collapses runs", "a@@@b", "a_b"}, + {"non-ascii becomes underscores", "李华", ""}, + {"non-ascii mixed", "alice 李", "alice"}, + {"all special chars collapses to empty", "@@@!!!", ""}, + {"truncates to 64", strings.Repeat("a", 100), strings.Repeat("a", 64)}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SanitizeMessageName(tt.in) + if got != tt.want { + t.Errorf("SanitizeMessageName(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +func TestSanitizeMessageName_OutputAlwaysWireSafe(t *testing.T) { + inputs := []string{ + "alice#1234", "U07AB12C3DEF", "141455495", + "@@@", "李华", "alice 李", "Alice_Bob-99", + strings.Repeat("a", 200), + } + for _, in := range inputs { + got := SanitizeMessageName(in) + if got == "" { + continue + } + if len(got) > maxMessageNameLen { + t.Errorf("SanitizeMessageName(%q) length %d exceeds max %d", in, len(got), maxMessageNameLen) + } + for _, r := range got { + ok := (r >= 'a' && r <= 'z') || + (r >= 'A' && r <= 'Z') || + (r >= '0' && r <= '9') || + r == '_' || r == '-' + if !ok { + t.Errorf("SanitizeMessageName(%q) = %q contains invalid rune %q", in, got, r) + } + } + } +} + +func TestIsSystemSenderID(t *testing.T) { + tests := []struct { + in string + want bool + }{ + {"", true}, + {" ", true}, + {"cron", true}, + {"CRON", true}, + {"heartbeat", true}, + {"system", true}, + {"async:tool_call", true}, + {"ASYNC:Foo", true}, + {"alice", false}, + {"U07AB12C3DEF", false}, + {"141455495", false}, + {"alice#1234", false}, + } + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + if got := IsSystemSenderID(tt.in); got != tt.want { + t.Errorf("IsSystemSenderID(%q) = %v, want %v", tt.in, got, tt.want) + } + }) + } +} + +func TestApplyUserNamePrefix(t *testing.T) { + tests := []struct { + name string + msg protocoltypes.Message + want string + }{ + { + name: "user with name", + msg: protocoltypes.Message{Role: "user", Name: "alice", Content: "hello"}, + want: "[alice] hello", + }, + { + name: "user without name", + msg: protocoltypes.Message{Role: "user", Content: "hello"}, + want: "hello", + }, + { + name: "user with name but empty content", + msg: protocoltypes.Message{Role: "user", Name: "alice", Content: ""}, + want: "", + }, + { + name: "user tool result is not prefixed", + msg: protocoltypes.Message{Role: "user", Name: "alice", Content: `{"ok":true}`, ToolCallID: "call_1"}, + want: `{"ok":true}`, + }, + { + name: "assistant with name not prefixed", + msg: protocoltypes.Message{Role: "assistant", Name: "alice", Content: "I am the assistant"}, + want: "I am the assistant", + }, + { + name: "tool role with name not prefixed", + msg: protocoltypes.Message{Role: "tool", Name: "alice", Content: `{"x":1}`}, + want: `{"x":1}`, + }, + { + name: "system role with name not prefixed", + msg: protocoltypes.Message{Role: "system", Name: "alice", Content: "instructions"}, + want: "instructions", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ApplyUserNamePrefix(tt.msg) + if got != tt.want { + t.Errorf("ApplyUserNamePrefix(%+v) = %q, want %q", tt.msg, got, tt.want) + } + }) + } +} + +func TestApplyUserNamePrefix_DoesNotMutateInput(t *testing.T) { + msg := protocoltypes.Message{Role: "user", Name: "alice", Content: "hello"} + original := msg + _ = ApplyUserNamePrefix(msg) + if !reflect.DeepEqual(msg, original) { + t.Errorf("ApplyUserNamePrefix mutated the input message: got %+v, want %+v", msg, original) + } +} diff --git a/pkg/providers/openai_responses_common/responses_common.go b/pkg/providers/openai_responses_common/responses_common.go index 17b731ed4..403b73800 100644 --- a/pkg/providers/openai_responses_common/responses_common.go +++ b/pkg/providers/openai_responses_common/responses_common.go @@ -11,6 +11,7 @@ import ( "github.com/openai/openai-go/v3/responses" "github.com/sipeed/picoclaw/pkg/providers/common" + "github.com/sipeed/picoclaw/pkg/providers/messageutil" "github.com/sipeed/picoclaw/pkg/providers/protocoltypes" ) @@ -36,7 +37,10 @@ func TranslateMessages(messages []protocoltypes.Message) (input responses.Respon }, }) } else if len(msg.Media) > 0 { - content := BuildMultipartContent(msg.Content, msg.Media) + // Render sender attribution as a `[name] ` prefix on the + // text portion of the multipart payload. The persisted + // message is not mutated. + content := BuildMultipartContent(messageutil.ApplyUserNamePrefix(msg), msg.Media) input = append(input, responses.ResponseInputItemUnionParam{ OfInputMessage: &responses.ResponseInputItemMessageParam{ Role: "user", @@ -46,8 +50,10 @@ func TranslateMessages(messages []protocoltypes.Message) (input responses.Respon } else { input = append(input, responses.ResponseInputItemUnionParam{ OfMessage: &responses.EasyInputMessageParam{ - Role: responses.EasyInputMessageRoleUser, - Content: responses.EasyInputMessageContentUnionParam{OfString: openai.Opt(msg.Content)}, + Role: responses.EasyInputMessageRoleUser, + Content: responses.EasyInputMessageContentUnionParam{ + OfString: openai.Opt(messageutil.ApplyUserNamePrefix(msg)), + }, }, }) } diff --git a/pkg/providers/openai_responses_common/responses_common_test.go b/pkg/providers/openai_responses_common/responses_common_test.go index ace91edf0..c1d88e12d 100644 --- a/pkg/providers/openai_responses_common/responses_common_test.go +++ b/pkg/providers/openai_responses_common/responses_common_test.go @@ -577,3 +577,69 @@ func TestTranslateTools_SerializesToJSON(t *testing.T) { t.Errorf("JSON should contain web_search, got: %s", s) } } + +// --- Sender attribution (Name field) tests --- + +// TestTranslateMessages_UserNamePrefixed verifies that sender attribution +// from Message.Name is rendered as a `[name] ` prefix on plain-text user +// messages sent to the OpenAI Responses API. The API supports per-message +// name natively, but the SDK type used here (EasyInputMessageParam) does +// not surface it, so prefixing is the consistent fallback shared with +// Anthropic / Bedrock adapters. +func TestTranslateMessages_UserNamePrefixed(t *testing.T) { + msgs := []protocoltypes.Message{ + {Role: "user", Content: "My name is Alice", Name: "U_alice"}, + } + input, _ := TranslateMessages(msgs) + if len(input) != 1 || input[0].OfMessage == nil { + t.Fatalf("expected one EasyInputMessage, got %+v", input) + } + data, err := json.Marshal(input) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + if !strings.Contains(string(data), "[U_alice] My name is Alice") { + t.Errorf("expected prefixed content in payload, got: %s", string(data)) + } +} + +// TestTranslateMessages_UserNamePrefixedMultipart verifies the same +// behavior on the multipart code path used when media is attached. +func TestTranslateMessages_UserNamePrefixedMultipart(t *testing.T) { + msgs := []protocoltypes.Message{ + { + Role: "user", + Content: "look at this", + Name: "U_alice", + Media: []string{"data:image/png;base64,abc"}, + }, + } + input, _ := TranslateMessages(msgs) + if len(input) != 1 || input[0].OfInputMessage == nil { + t.Fatalf("expected InputMessage with multipart content, got %+v", input) + } + data, err := json.Marshal(input) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + if !strings.Contains(string(data), "[U_alice] look at this") { + t.Errorf("expected prefixed text part in payload, got: %s", string(data)) + } +} + +// TestTranslateMessages_ToolResultNotPrefixed protects against accidentally +// prefixing tool result outputs (msg.ToolCallID set) — those are function +// outputs, not user utterances. +func TestTranslateMessages_ToolResultNotPrefixed(t *testing.T) { + msgs := []protocoltypes.Message{ + {Role: "user", Content: `{"temp":72}`, ToolCallID: "call_1", Name: "alice"}, + } + input, _ := TranslateMessages(msgs) + if len(input) != 1 || input[0].OfFunctionCallOutput == nil { + t.Fatalf("expected FunctionCallOutput, got %+v", input) + } + data, _ := json.Marshal(input) + if strings.Contains(string(data), "[alice]") { + t.Errorf("tool result must not carry sender prefix, got: %s", string(data)) + } +} diff --git a/pkg/providers/protocoltypes/types.go b/pkg/providers/protocoltypes/types.go index bab4433e7..a7996feb3 100644 --- a/pkg/providers/protocoltypes/types.go +++ b/pkg/providers/protocoltypes/types.go @@ -79,8 +79,16 @@ type Attachment struct { } type Message struct { - Role string `json:"role"` - Content string `json:"content"` + Role string `json:"role"` + Content string `json:"content"` + // Name carries per-message sender attribution for multi-user sessions + // (Discord/Telegram/Slack groups, etc.). It is the sanitized SenderID + // from the inbound channel and is OpenAI-Chat-Completions wire-safe + // (matches `^[a-zA-Z0-9_-]{1,64}$`). Adapters that natively support a + // per-message name field (OpenAI) emit it directly; adapters that do + // not (Anthropic, Bedrock) prefix it onto user content at marshal + // time without mutating the persisted message. + Name string `json:"name,omitempty"` Media []string `json:"media,omitempty"` Attachments []Attachment `json:"attachments,omitempty"` ReasoningContent string `json:"reasoning_content,omitempty"` diff --git a/pkg/providers/protocoltypes/types_test.go b/pkg/providers/protocoltypes/types_test.go new file mode 100644 index 000000000..6e551cc0b --- /dev/null +++ b/pkg/providers/protocoltypes/types_test.go @@ -0,0 +1,66 @@ +// PicoClaw - Ultra-lightweight personal AI agent +// License: MIT +// +// Copyright (c) 2026 PicoClaw contributors + +package protocoltypes + +import ( + "encoding/json" + "strings" + "testing" +) + +// TestMessage_NameJSONRoundtrip verifies that the Name field — added for +// multi-user sender attribution (issue #2702) — round-trips through +// json.Marshal/Unmarshal. The session persistence layer (JSONL append + +// in-memory snapshot via json.MarshalIndent) relies on this transparency +// to carry sender attribution into stored history without bespoke code. +func TestMessage_NameJSONRoundtrip(t *testing.T) { + in := Message{Role: "user", Content: "hi", Name: "U_alice"} + data, err := json.Marshal(in) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + if !strings.Contains(string(data), `"name":"U_alice"`) { + t.Errorf("expected name in marshaled JSON, got: %s", string(data)) + } + var out Message + if err := json.Unmarshal(data, &out); err != nil { + t.Fatalf("json.Unmarshal: %v", err) + } + if out.Name != "U_alice" { + t.Errorf("Name after roundtrip = %q, want U_alice", out.Name) + } +} + +// TestMessage_NameOmittedWhenEmpty preserves wire/persistence backward +// compatibility for direct-channel turns and pre-2702 session files: an +// empty Name must not appear as `"name":""` in either marshaled output. +func TestMessage_NameOmittedWhenEmpty(t *testing.T) { + in := Message{Role: "user", Content: "hi"} + data, err := json.Marshal(in) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + if strings.Contains(string(data), `"name"`) { + t.Errorf("empty Name should be omitted; got: %s", string(data)) + } +} + +// TestMessage_LegacyHistoryUnmarshalsToEmptyName verifies that historical +// session files written before this field existed still load cleanly: the +// Name field is simply zero-valued. +func TestMessage_LegacyHistoryUnmarshalsToEmptyName(t *testing.T) { + legacy := []byte(`{"role":"user","content":"old message"}`) + var msg Message + if err := json.Unmarshal(legacy, &msg); err != nil { + t.Fatalf("json.Unmarshal legacy: %v", err) + } + if msg.Name != "" { + t.Errorf("legacy Name = %q, want empty", msg.Name) + } + if msg.Content != "old message" { + t.Errorf("legacy Content = %q, want preserved", msg.Content) + } +}