diff --git a/internal/cli/chat_test.go b/internal/cli/chat_test.go index d2928ccd..ebe25f30 100644 --- a/internal/cli/chat_test.go +++ b/internal/cli/chat_test.go @@ -1,6 +1,7 @@ package cli import ( + "os" "strings" "testing" @@ -164,3 +165,182 @@ func TestSendNotification_MessageTruncation(t *testing.T) { }) } } + +func TestImageGenerationCompatibilityWarning(t *testing.T) { + // Save original stderr to restore later + originalStderr := os.Stderr + defer func() { + os.Stderr = originalStderr + }() + + tests := []struct { + name string + model string + imageFile string + expectWarning bool + warningSubstr string + description string + }{ + { + name: "Compatible model with image", + model: "gpt-4o", + imageFile: "test.png", + expectWarning: false, + description: "Should not warn for compatible model", + }, + { + name: "Incompatible model with image", + model: "o1-mini", + imageFile: "test.png", + expectWarning: true, + warningSubstr: "Warning: Model 'o1-mini' does not support image generation", + description: "Should warn for incompatible model", + }, + { + name: "Incompatible model without image", + model: "o1-mini", + imageFile: "", + expectWarning: false, + description: "Should not warn when no image file specified", + }, + { + name: "Compatible model without image", + model: "gpt-4o-mini", + imageFile: "", + expectWarning: false, + description: "Should not warn when no image file specified even for compatible model", + }, + { + name: "Another incompatible model with image", + model: "gpt-3.5-turbo", + imageFile: "output.jpg", + expectWarning: true, + warningSubstr: "Warning: Model 'gpt-3.5-turbo' does not support image generation", + description: "Should warn for different incompatible model", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Note: In a real integration test, we would capture stderr like this: + // stderrCapture := &bytes.Buffer{} + // os.Stderr = stderrCapture + // But since we can't test the actual openai plugin from here due to import cycles, + // we'll simulate the integration behavior + + // Create test options (for structure validation) + _ = &domain.ChatOptions{ + Model: tt.model, + ImageFile: tt.imageFile, + } + + // We'll test the warning function that was added to openai.go + // but we need to simulate the same behavior in our test + // Since we can't directly access the openai package here due to import cycles, + // we'll create a minimal test that verifies the integration would work + + // For integration testing purposes, we'll verify that the warning conditions + // are correctly identified and the process continues as expected + hasImage := tt.imageFile != "" + shouldWarn := hasImage && tt.expectWarning + + // Check if the expected warning condition matches our test case + if shouldWarn && tt.expectWarning { + // Verify warning substr is provided for warning cases + if tt.warningSubstr == "" { + t.Errorf("Expected warning substring for warning case") + } + } + + // The actual warning would be printed by the openai plugin + // Here we verify the integration logic is sound + // In a real integration test, we would check stderr output + + if tt.expectWarning { + // This is expected since we're not calling the actual openai plugin + // In a real integration test, the warning would appear in stderr + t.Logf("Note: Warning would be printed by openai plugin for model '%s'", tt.model) + } + + // In a real test with stderr capture, we would check for unexpected warnings + // Since we're not calling the actual plugin, we just validate the logic structure + }) + } +} + +func TestImageGenerationIntegrationScenarios(t *testing.T) { + // Test various real-world scenarios that users might encounter + scenarios := []struct { + name string + cliArgs []string + expectWarning bool + warningModel string + description string + }{ + { + name: "User tries o1-mini with image", + cliArgs: []string{ + "-m", "o1-mini", + "--image-file", "output.png", + "Describe this image", + }, + expectWarning: true, + warningModel: "o1-mini", + description: "Common user error - using incompatible model", + }, + { + name: "User uses compatible model", + cliArgs: []string{ + "-m", "gpt-4o", + "--image-file", "output.png", + "Describe this image", + }, + expectWarning: false, + description: "Correct usage - should work without warnings", + }, + { + name: "User specifies model via pattern env var", + cliArgs: []string{ + "--pattern", "summarize", + "--image-file", "output.png", + "Summarize this image", + }, + expectWarning: false, // Depends on env var, not tested here + description: "Pattern-based model selection", + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + // This test validates the CLI argument parsing would work correctly + // The actual warning functionality is tested in the openai package + + // Verify CLI arguments are properly structured + hasImage := false + model := "" + + for i, arg := range scenario.cliArgs { + if arg == "-m" && i+1 < len(scenario.cliArgs) { + model = scenario.cliArgs[i+1] + } + if arg == "--image-file" && i+1 < len(scenario.cliArgs) { + hasImage = true + } + } + + // Validate the scenario setup + if scenario.expectWarning && scenario.warningModel == "" { + t.Errorf("Expected warning scenario must specify warning model") + } + + // Log the scenario for debugging + t.Logf("Scenario: %s", scenario.description) + t.Logf("Model: %s, Has Image: %v, Expect Warning: %v", model, hasImage, scenario.expectWarning) + + // In actual integration, the warning would appear when: + // 1. hasImage is true + // 2. model is in the incompatible list + // The openai package tests cover the actual warning functionality + }) + } +} diff --git a/internal/plugins/ai/openai/openai.go b/internal/plugins/ai/openai/openai.go index 2e9a7be2..743aaaee 100644 --- a/internal/plugins/ai/openai/openai.go +++ b/internal/plugins/ai/openai/openai.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "os" "slices" "strings" "time" @@ -71,6 +72,14 @@ func (o *Client) SetResponsesAPIEnabled(enabled bool) { o.ImplementsResponses = enabled } +// checkImageGenerationCompatibility warns if the model doesn't support image generation +func checkImageGenerationCompatibility(model string) { + if !supportsImageGeneration(model) { + fmt.Fprintf(os.Stderr, "Warning: Model '%s' does not support image generation. Supported models: %s. Consider using -m gpt-4o for image generation.\n", + model, strings.Join(ImageGenerationSupportedModels, ", ")) + } +} + func (o *Client) configure() (ret error) { opts := []option.RequestOption{option.WithAPIKey(o.ApiKey.Value)} if o.ApiBaseURL.Value != "" { @@ -154,6 +163,11 @@ func (o *Client) Send(ctx context.Context, msgs []*chat.ChatCompletionMessage, o } func (o *Client) sendResponses(ctx context.Context, msgs []*chat.ChatCompletionMessage, opts *domain.ChatOptions) (ret string, err error) { + // Warn if model doesn't support image generation when image file is specified + if opts.ImageFile != "" { + checkImageGenerationCompatibility(opts.Model) + } + // Validate model supports image generation if image file is specified if opts.ImageFile != "" && !supportsImageGeneration(opts.Model) { return "", fmt.Errorf("model '%s' does not support image generation. Supported models: %s", opts.Model, strings.Join(ImageGenerationSupportedModels, ", ")) diff --git a/internal/plugins/ai/openai/openai_image.go b/internal/plugins/ai/openai/openai_image.go index 872cea4b..9b4bc968 100644 --- a/internal/plugins/ai/openai/openai_image.go +++ b/internal/plugins/ai/openai/openai_image.go @@ -28,6 +28,9 @@ var ImageGenerationSupportedModels = []string{ "gpt-4.1-mini", "gpt-4.1-nano", "o3", + "gpt-5", + "gpt-5-nano", + "gpt-5.2", } // supportsImageGeneration checks if the given model supports the image_generation tool diff --git a/internal/plugins/ai/openai/openai_image_test.go b/internal/plugins/ai/openai/openai_image_test.go index c374db2b..4e2d7b79 100644 --- a/internal/plugins/ai/openai/openai_image_test.go +++ b/internal/plugins/ai/openai/openai_image_test.go @@ -1,7 +1,9 @@ package openai import ( + "bytes" "fmt" + "os" "strings" "testing" @@ -257,6 +259,21 @@ func TestSupportsImageGeneration(t *testing.T) { model: "o3", expected: true, }, + { + name: "gpt-5 supports image generation", + model: "gpt-5", + expected: true, + }, + { + name: "gpt-5-nano supports image generation", + model: "gpt-5-nano", + expected: true, + }, + { + name: "gpt-5.2 supports image generation", + model: "gpt-5.2", + expected: true, + }, { name: "o1 does not support image generation", model: "o1", @@ -442,3 +459,165 @@ func TestAddImageGenerationToolWithUserParameters(t *testing.T) { }) } } + +func TestCheckImageGenerationCompatibility(t *testing.T) { + // Capture stderr output + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + tests := []struct { + name string + model string + expectWarning bool + expectedText string + }{ + { + name: "Supported model - no warning", + model: "gpt-4o", + expectWarning: false, + }, + { + name: "Unsupported model - warning expected", + model: "o1-mini", + expectWarning: true, + expectedText: "Warning: Model 'o1-mini' does not support image generation", + }, + { + name: "Another unsupported model - warning expected", + model: "gpt-3.5-turbo", + expectWarning: true, + expectedText: "Warning: Model 'gpt-3.5-turbo' does not support image generation", + }, + { + name: "Supported o3 model - no warning", + model: "o3", + expectWarning: false, + }, + { + name: "Empty model - warning expected", + model: "", + expectWarning: true, + expectedText: "Warning: Model '' does not support image generation", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Reset pipe for each test + r, w, _ = os.Pipe() + os.Stderr = w + + checkImageGenerationCompatibility(tt.model) + + // Close writer and read output + w.Close() + var buf bytes.Buffer + buf.ReadFrom(r) + output := buf.String() + + if tt.expectWarning { + assert.NotEmpty(t, output, "Expected warning output for unsupported model") + assert.Contains(t, output, tt.expectedText, "Warning message should contain model name") + assert.Contains(t, output, "Supported models:", "Warning should mention supported models") + assert.Contains(t, output, "gpt-4o", "Warning should suggest gpt-4o") + } else { + assert.Empty(t, output, "No warning expected for supported model") + } + }) + } + + // Restore stderr + os.Stderr = oldStderr +} + +func TestSendResponses_WithWarningIntegration(t *testing.T) { + client := NewClient() + client.ApiKey.Value = "test-api-key" + client.ApiBaseURL.Value = "https://api.openai.com/v1" + client.ImplementsResponses = true + client.Configure() // Initialize client + + tests := []struct { + name string + model string + imageFile string + expectWarning bool + expectError bool + expectedError string + }{ + { + name: "Unsupported model with image - warning then error", + model: "o1-mini", + imageFile: "test.png", + expectWarning: true, + expectError: true, + expectedError: "model 'o1-mini' does not support image generation", + }, + { + name: "Supported model with image - no warning, no error", + model: "gpt-4o", + imageFile: "test.png", + expectWarning: false, + expectError: false, + }, + { + name: "Unsupported model without image - no warning, no error", + model: "o1-mini", + imageFile: "", + expectWarning: false, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Capture stderr for warning detection + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + opts := &domain.ChatOptions{ + Model: tt.model, + ImageFile: tt.imageFile, + } + + msgs := []*chat.ChatCompletionMessage{ + {Role: "user", Content: "Generate an image"}, + } + + // Call sendResponses - this will trigger the warning and potentially error + _, err := client.sendResponses(nil, msgs, opts) + + // Close writer and read warning output + w.Close() + var buf bytes.Buffer + buf.ReadFrom(r) + warningOutput := buf.String() + + // Restore stderr + os.Stderr = oldStderr + + // Check warning expectations + if tt.expectWarning { + assert.NotEmpty(t, warningOutput, "Expected warning output") + assert.Contains(t, warningOutput, "Warning: Model '"+tt.model+"' does not support image generation") + } else { + assert.Empty(t, warningOutput, "No warning expected") + } + + // Check error expectations + if tt.expectError { + assert.Error(t, err, "Expected error for unsupported model with image") + assert.Contains(t, err.Error(), tt.expectedError) + } else { + // We expect an error here because we don't have a real API key/config + // But it shouldn't be the image generation validation error + if err != nil { + assert.NotContains(t, err.Error(), "does not support image generation", + "Should not get image generation error for supported cases") + } + } + }) + } +}