fix(openai): merge custom-provider system prompts#2382
Conversation
aheritier
left a comment
There was a problem hiding this comment.
LGTM. Clean, minimal fix that reuses existing infrastructure (isCustomProvider, MergeConsecutiveMessages). Both paths are well-tested and CI passes.
One nit: the em dash → hyphen change on line 51 of the test file is unrelated noise — consider keeping commits atomic in the future.
23681e3 to
578a48d
Compare
|
needs to be rebased (at the minimum). Moving it to draft |
Signed-off-by: pandego <7780875+pandego@users.noreply.github.com>
578a48d to
ccf7977
Compare
|
Rebased this on current main and resolved the conflicts. Focused validation passes: go test ./pkg/model/provider/openai ./pkg/model/provider/oaistream -run 'TestConvertMessages_MergesConsecutiveSystemMessagesForCustomProviders|TestConvertMessages_PreservesConsecutiveSystemMessagesForOpenAIProvider|TestConvertMessages|TestMergeConsecutiveMessages' -count=1
go test ./pkg/model/provider/openai ./pkg/model/provider/oaistream -count=1 |
|
/review |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The fix correctly introduces a convertMessages wrapper that applies MergeConsecutiveMessages only for custom OpenAI-compatible providers, aligning them with the existing DMR prompt-normalization behaviour. Logic is clean and tests cover both the merging path and the native-OpenAI preservation path.
No high- or medium-severity issues found. Three minor (low) observations are attached as inline comments.
| // 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) { |
There was a problem hiding this comment.
[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.
|
|
||
| result := convertMessages(t.Context(), &latest.ModelConfig{}, modelsdev.ID{}, nil, messages) | ||
| require.Len(t, result, 3) | ||
| assert.NotNil(t, result[0].OfSystem) |
There was a problem hiding this comment.
[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)| 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) { | ||
| return oaistream.MergeConsecutiveMessages(openaiMessages) |
There was a problem hiding this comment.
[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.
Summary
openai_chatcompletionsprovidersTesting
docker run --rm -v "$PWD":/src -w /src golang:1.26 sh -lc '/usr/local/go/bin/gofmt -w pkg/model/provider/openai/client.go pkg/model/provider/openai/client_test.go && /usr/local/go/bin/go test ./pkg/model/provider/openai ./pkg/model/provider/oaistream -run "TestConvertMessages_MergesConsecutiveSystemMessagesForCustomProviders|TestConvertMessages_PreservesConsecutiveSystemMessagesForOpenAIProvider|TestConvertMessages|TestMergeConsecutiveMessages" -count=1'Closes #2327