Skip to content

fix(openai): merge custom-provider system prompts #2382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pandego wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(openai): merge custom-provider system prompts #2382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions pkg/model/provider/openai/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/docker/docker-agent/pkg/model/provider/oaistream"
"github.com/docker/docker-agent/pkg/model/provider/options"
"github.com/docker/docker-agent/pkg/modelinfo"
"github.com/docker/docker-agent/pkg/modelsdev"
"github.com/docker/docker-agent/pkg/rag/prompts"
"github.com/docker/docker-agent/pkg/rag/types"
"github.com/docker/docker-agent/pkg/tools"
Expand Down Expand Up @@ -178,10 +179,20 @@ func (c *Client) Close() {
}
}

// convertMessages converts chat.Message to openai.ChatCompletionMessageParamUnion
// using the shared oaistream implementation.
// convertMessages converts chat.Message to OpenAI chat-completions message params.
// Custom OpenAI-compatible providers may target local model runners that reject
// consecutive system or user messages, so we normalize those prompts to match
// the DMR provider behavior.
func convertMessages(ctx context.Context, cfg *latest.ModelConfig, id modelsdev.ID, store *modelsdev.Store, messages []chat.Message) []openai.ChatCompletionMessageParamUnion {
openaiMessages := oaistream.ConvertMessages(ctx, messages, id, store)
if isCustomProvider(cfg) {
Copy link
Copy Markdown

@docker-agent docker-agent May 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] isCustomProvider covers all api_type values, including openai_responses

isCustomProvider returns true for any config whose ProviderOpts carries an api_type field — including openai_responses. This means MergeConsecutiveMessages is technically reachable for Responses-API custom providers too, even though the Responses-API path (CreateResponseStream) never calls convertMessages today.

This is harmless at runtime, but if a future refactor routes Responses-API providers through convertMessages the merging will kick in unexpectedly. Consider restricting the guard to Chat Completions api_type values only, or add a comment explaining the intentional scope.

return oaistream.MergeConsecutiveMessages(openaiMessages)
Copy link
Copy Markdown

@docker-agent docker-agent May 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Merged system messages joined with \n; consider \n\n for markdown fidelity

When MergeConsecutiveMessages concatenates consecutive system-message strings it uses a single \n as the separator. The test fixture includes a message with ## Custom Shell Tools\n\n### execute_command — merging with a single newline can collapse the blank line that separates markdown sections, potentially mangling the structure the model sees.

Using \n\n as the separator would preserve the original paragraph breaks. This is a design trade-off, not a hard bug, but worth an explicit decision and a comment if single-newline is intentional.

}
return openaiMessages
}

func (c *Client) convertMessages(ctx context.Context, messages []chat.Message) []openai.ChatCompletionMessageParamUnion {
return oaistream.ConvertMessages(ctx, messages, c.ID(), c.ModelOptions.ModelsDevStore())
return convertMessages(ctx, &c.ModelConfig, c.ID(), c.ModelOptions.ModelsDevStore(), messages)
}

// CreateChatCompletionStream creates a streaming chat completion request
Expand Down
38 changes: 37 additions & 1 deletion pkg/model/provider/openai/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"github.com/stretchr/testify/require"

"github.com/docker/docker-agent/pkg/chat"
"github.com/docker/docker-agent/pkg/config/latest"
"github.com/docker/docker-agent/pkg/modelsdev"
"github.com/docker/docker-agent/pkg/tools"
)

Expand Down Expand Up @@ -82,7 +84,7 @@ func TestConvertMessagesToResponseInput_AssistantTextWithToolCalls(t *testing.T)
}

func TestConvertMessagesToResponseInput_NoOrphans(t *testing.T) {
// All tool calls have matching results — no placeholder needed.
// All tool calls have matching results - no placeholder needed.
messages := []chat.Message{
{Role: chat.MessageRoleUser, Content: "hello"},
{
Expand All @@ -104,3 +106,37 @@ func TestConvertMessagesToResponseInput_NoOrphans(t *testing.T) {
}
assert.Equal(t, 1, outputCount, "should not inject extra outputs when all calls have results")
}

func TestConvertMessages_MergesConsecutiveSystemMessagesForCustomProviders(t *testing.T) {
cfg := &latest.ModelConfig{
ProviderOpts: map[string]any{"api_type": "openai_chatcompletions"},
}
messages := []chat.Message{
{Role: chat.MessageRoleSystem, Content: "You are Bob, a coding expert"},
{Role: chat.MessageRoleSystem, Content: "## Custom Shell Tools\n\n### execute_command"},
{Role: chat.MessageRoleSystem, Content: "<available_skills>\n <skill>what-time-is-it</skill>\n</available_skills>"},
{Role: chat.MessageRoleUser, Content: "what is your favourite colour?"},
}

result := convertMessages(t.Context(), cfg, modelsdev.ID{}, nil, messages)
require.Len(t, result, 2)
require.NotNil(t, result[0].OfSystem)
assert.Contains(t, result[0].OfSystem.Content.OfString.Value, "You are Bob, a coding expert")
assert.Contains(t, result[0].OfSystem.Content.OfString.Value, "Custom Shell Tools")
assert.Contains(t, result[0].OfSystem.Content.OfString.Value, "available_skills")
assert.NotNil(t, result[1].OfUser)
}

func TestConvertMessages_PreservesConsecutiveSystemMessagesForOpenAIProvider(t *testing.T) {
messages := []chat.Message{
{Role: chat.MessageRoleSystem, Content: "System 1"},
{Role: chat.MessageRoleSystem, Content: "System 2"},
{Role: chat.MessageRoleUser, Content: "hello"},
}

result := convertMessages(t.Context(), &latest.ModelConfig{}, modelsdev.ID{}, nil, messages)
require.Len(t, result, 3)
assert.NotNil(t, result[0].OfSystem)
Copy link
Copy Markdown

@docker-agent docker-agent May 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Test only asserts OfSystem != nil, not the actual content of each preserved message

TestConvertMessages_PreservesConsecutiveSystemMessagesForOpenAIProvider correctly asserts that three messages are returned and that the first is a system message, but it never checks the text of result[0] or result[1]. If the messages were reordered the test would still pass. Consider adding:

assert.Equal(t, "System 1", result[0].OfSystem.Content.OfString.Value)
assert.Equal(t, "System 2", result[1].OfSystem.Content.OfString.Value)

assert.NotNil(t, result[1].OfSystem)
assert.NotNil(t, result[2].OfUser)
}
Loading