Adding openai configuration and new responses api

[alvinunreal]

Related #82

[claude]

Pull Request Review: Adding OpenAI Configuration and Responses API

Overview

This PR adds support for OpenAI's Responses API alongside the existing Chat Completions API, introducing a prioritized provider selection system (OpenAI → Azure → OpenRouter). The implementation is well-structured with good test coverage.

---

✅ Strengths

Code Quality

• Clean abstraction: The determineAPIType() method provides a clear routing mechanism between APIs
• Good test coverage: Added 3 new test functions covering the Responses API endpoint, system messages, and API type determination
• Consistent error handling: Comprehensive error logging and context throughout
• Well-documented: README updates are thorough and user-friendly

Architecture

• Backwards compatible: Existing OpenRouter and Azure configurations continue to work
• Separation of concerns: Responses API logic is isolated in its own method
• Configuration priority: Clear, documented priority order for provider selection

---

🐛 Potential Issues

1. API Type Selection Logic - Potential Bug ⚠️

Location: internal/ai_client.go:119-132

The determineAPIType() method has a logic issue:

if c.config.OpenAI.APIKey != "" && c.config.OpenAI.Model == model {
    return "responses"
}

Problem: This only returns "responses" if the requested model matches config.OpenAI.Model. If a user sets openai.api_key but then tries to use a different model (e.g., through session override), it will fall through to Azure/OpenRouter instead of using OpenAI.

Impact: Users might unexpectedly use the wrong provider if they override the model at runtime.

Suggested fix:

// If OpenAI API key is configured, use Responses API
if c.config.OpenAI.APIKey != "" {
    return "responses"
}

The model validation should happen elsewhere, or at least log a warning if the model differs from the configured one.

2. Inconsistent Model Selection ⚠️

Location: internal/config_helpers.go:97-112

The GetModel() method checks both APIKey and Model for OpenAI:

if m.Config.OpenAI.APIKey != "" && m.Config.OpenAI.Model != "" {

But this creates an inconsistency:

• If openai.api_key is set but openai.model is empty, it falls through to Azure/OpenRouter
• But the user might have intended to use OpenAI with a model they'll specify later

Recommendation: Either require both or default to OpenAI if the API key is present (with a sensible default model).

3. Empty Messages Array Handling

Location: internal/ai_client.go:299-301

In the Response() method, when system message is extracted:

if len(messages) > 1 {
    input = messages[1:]
} else {
    input = []Message{}
}

If there's only a system message and no user message, the API request will have empty input. While this is caught with a clear error, it might be better to return the error earlier rather than making the API call.

4. Missing Environment Variable Documentation

The README shows examples for setting environment variables but doesn't document:

• TMUXAI_OPENAI_API_KEY
• TMUXAI_OPENAI_MODEL
• TMUXAI_OPENAI_BASE_URL

These should be explicitly listed in the environment variables section for consistency with the existing OpenRouter and Azure variables.

---

🔒 Security Considerations

✅ Good Practices

• API keys are properly masked in config display via maskAPIKey() function
• API keys transmitted via Authorization header (correct for OpenAI)
• No API keys logged in debug output

⚠️ Recommendations

1. No timeout on HTTP client: The http.Client is created without a timeout in NewAiClient() (internal/ai_client.go:111-115). This could lead to indefinite hangs.

   Suggested fix:

   return &AiClient{
       config: cfg,
       client: &http.Client{
           Timeout: 60 * time.Second,
       },
   }

2. Context cancellation: Good use of context.Context throughout, but no explicit timeout is set on the context itself. Consider adding context timeouts at the call site.

---

🚀 Performance Considerations

Good

• Uses http.Client pooling (connection reuse)
• Defers body closing properly: defer func() { _ = resp.Body.Close() }()
• Efficient JSON marshaling/unmarshaling

Potential Improvements

1. Response body size: No limit on io.ReadAll() response size (internal/ai_client.go:250, 354). A malicious/buggy API could send very large responses causing memory issues.

   Suggested fix:

   body, err := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) // 10MB limit

2. Store=true by default: Line 310 sets Store: true for all Responses API calls, which enables statefulness. This might have cost/privacy implications that should be documented or configurable.

---

📋 Test Coverage

✅ What's Tested

• OpenAI Responses API endpoint and authentication
• System message handling in Responses API
• API type determination logic
• Azure OpenAI endpoint (existing)

❌ What's Missing

1. Error scenarios: No tests for API errors, network failures, or malformed responses
2. Empty message handling: No test for edge case where system message is the only message
3. Model mismatch: No test for when requested model differs from configured model
4. Provider fallback: No test verifying the priority order (OpenAI → Azure → OpenRouter)
5. Integration test: No test verifying GetResponseFromChatMessages() correctly routes to the right API

Suggested additions:

func TestResponseAPIErrorHandling(t *testing.T) { /* test error responses */ }
func TestProviderPrioritySelection(t *testing.T) { /* test OpenAI > Azure > OpenRouter */ }
func TestResponseBodySizeLimit(t *testing.T) { /* test large response handling */ }

---

📝 Documentation

✅ Good

• Excellent README updates with clear examples
• Provider priority clearly documented
• Configuration examples for all three providers
• Benefits of Responses API explained

Suggestions

1. Add migration guide for existing users
2. Document the Store: true statefulness feature and its implications
3. Add troubleshooting section for common issues (e.g., "Why is my OpenAI config not being used?")

---

🎯 Recommendations Summary

High Priority

1. ✅ Fix determineAPIType() logic to not require exact model match
2. ✅ Add HTTP client timeout to prevent hangs
3. ✅ Add response body size limit for safety

Medium Priority

4. Improve test coverage for error scenarios and edge cases
5. Document environment variables explicitly
6. Make Store configurable or document privacy implications
7. Add early validation for empty message arrays

Low Priority

8. Consider adding metrics/logging for which provider is being used
9. Add migration guide for existing users

---

🎉 Conclusion

This is a solid PR that adds valuable functionality with good code quality and test coverage. The main concerns are around the API type selection logic and some defensive programming practices (timeouts, size limits). Once the high-priority issues are addressed, this will be ready to merge.

Recommendation: Request changes for the API selection logic and timeout issues, then approve.

Great work on maintaining backwards compatibility and providing comprehensive documentation! 👍

---

Files reviewed: README.md, config.example.yaml, config/config.go, internal/ai_client.go, internal/ai_client_test.go, internal/config_helpers.go, internal/manager.go, internal/process_message.go

[claude]

Test comment

[claude]

Code Review for PR #84: Adding OpenAI Configuration and New Responses API

Summary

This PR adds support for OpenAI's Responses API alongside the existing Chat Completions API, providing users with multiple AI provider options. The implementation is well-structured with good test coverage.

---

✅ Strengths

1. Good Test Coverage: Comprehensive unit tests for the new Responses API functionality (tests for basic responses, system messages, and API type determination)

2. Clear API Abstraction: The routing logic in determineAPIType() and GetResponseFromChatMessages() cleanly separates different API implementations

3. Backward Compatibility: Existing OpenRouter and Azure OpenAI configurations continue to work

4. Good Documentation: README updates are thorough and provide clear examples

[claude]

🐛 Potential Issues

1. Incorrect Content Type in Response Parsing (ai_client.go:392-396)

The code checks for content.Type == "output_text", but based on the test mock at line 93, the actual content type should be "text" not "output_text". This mismatch could cause the fallback parsing to fail.

Suggested fix: Change the condition to content.Type == "text"

2. Store Field Hardcoded to True (ai_client.go:311)

The Store: true field is always enabled for statefulness, but users may not want their conversations stored for privacy/cost reasons. No configuration option exists to control this behavior.

Recommendation: Add a configuration option or set to false by default.

3. Default Model gpt-5 May Not Exist (config_helpers.go:106)

The code defaults to gpt-5 when OpenAI API key is configured but no model is specified. If this model doesn't exist yet, API calls will fail.

Suggested fix: Default to gpt-4o which is known to exist

[claude]

🔒 Security Concerns

Positive:

• The maskAPIKey() function at config_helpers.go:193-198 properly protects sensitive data in config outputs

Areas for improvement:

• No validation that API keys follow expected formats
• No sanitization of model names before sending to API
• Error messages like logger.Error("API returned error: %s", body) could potentially log sensitive information

Recommendation: Add basic validation to catch configuration errors early and be more selective about what gets logged from error responses.

---

⚡ Performance Considerations

1. The Responses API enables statefulness, but the implementation doesn't utilize PreviousResponseID for conversation continuity. Each request is stateless despite Store: true.

2. Response parsing with two attempts (first OutputText, then iterating Output items) is acceptable for reliability.

[claude]

📋 Code Quality & Test Coverage

Code Quality - Strong:

• Good naming: Functions and variables are well-named and self-documenting
• Error handling: Comprehensive with good context in error messages
• Type safety: Proper use of Go idioms and type safety
• Logging: Good use of debug/error logging for troubleshooting

Test Coverage - Good but could be better:

Strong points:

• Tests cover happy path and error cases
• System message handling is tested
• API type determination logic is tested

Missing tests:

• No test for when response.OutputText is empty but Output items contain text
• No test for error responses from the API
• No test for canceled context handling in Response()

[claude]

📝 Minor Suggestions

1. config.example.yaml:11-13 - Comment says "With openRouter" but should be "With OpenRouter" (capital R)

2. ai_client.go:53 - ResponseInput interface{} - Consider using "any" instead of "interface{}" (Go 1.18+)

3. README.md - The provider priority order is mentioned twice in slightly different ways - consolidate for clarity

4. config_helpers.go:18 - Consider adding openai.api_key and openai.base_url to AllowedConfigKeys for runtime configuration

[claude]

🎯 Recommendations

Before merging (critical):

1. Fix the content type mismatch in response parsing (content.Type should be "text" not "output_text")
2. Reconsider the default model name or add validation (gpt-5 may not exist yet)
3. Document or make configurable the Store: true behavior
4. Add test for the Output items fallback parsing

Future improvements:

1. Implement proper state management with PreviousResponseID
2. Add configuration validation on startup
3. Consider rate limiting/retry logic for API calls
4. Add metrics/telemetry for API usage patterns

---

Verdict

This is a solid addition that follows the codebase patterns well. The main concern is the content type mismatch which could cause runtime failures. Once that's addressed, this PR will be ready to merge.

Recommendation: Request changes for the content type fix, then approve.

---

🤖 This review was generated with assistance from Claude Code

[alvinunreal]

---

[claude]

Code Review: Adding OpenAI Configuration and New Responses API

Summary

This PR adds support for OpenAI's Responses API alongside the existing Chat Completions API, introducing a provider priority system (OpenAI → Azure OpenAI → OpenRouter). Overall, the implementation is solid with good test coverage. Below are my findings across different categories.

---

✅ Strengths

1. Comprehensive test coverage - New tests for the Responses API, system message handling, and API type determination
2. Good error handling - Multiple fallback mechanisms for parsing response content
3. Backward compatibility - Existing functionality preserved while adding new capabilities
4. Clear documentation - README updated with detailed provider configuration examples
5. Session override support - Config helpers properly handle OpenAI settings

---

🐛 Potential Bugs & Issues

Critical

1. Session Override Inconsistency in GetModel() (config_helpers.go:119-142)

The GetModel() method doesn't respect session overrides for OpenAI configuration, while it does for OpenRouter. This creates inconsistent behavior:

func (m *Manager) GetModel() string {
    // BUG: Uses m.Config.OpenAI.APIKey directly instead of m.GetOpenAIAPIKey()
    if m.Config.OpenAI.APIKey != "" {
        if m.Config.OpenAI.Model != "" {
            return m.GetOpenAIModel()  // Good - uses getter
        }
        return "gpt-5-codex"
    }
    
    // Same issue with Azure
    if m.Config.AzureOpenAI.APIKey != "" { ... }
    
    // OpenRouter correctly uses getter
    return m.GetOpenRouterModel()
}

Fix: Use m.GetOpenAIAPIKey() and equivalent getters for Azure to respect runtime overrides:

func (m *Manager) GetModel() string {
    if m.GetOpenAIAPIKey() != "" {
        model := m.GetOpenAIModel()
        if model != "" {
            return model
        }
        return "gpt-5-codex"
    }
    // ... similar for Azure
}

Medium

2. API Key Check Inconsistency in determineAPIType() (ai_client.go:118-132)

The method uses c.config.OpenAI.APIKey directly instead of potentially checking session overrides. This could cause API routing issues if users override the API key at runtime.

3. Missing Validation for Model Name (config_helpers.go:128)

The default model "gpt-5-codex" might not be the correct identifier. OpenAI models typically use formats like "gpt-4" or "gpt-3.5-turbo". Verify this is a valid model name or update documentation accordingly.

4. Store Parameter Hardcoded (ai_client.go:311)

Store: true, // Enable statefulness by default

This is hardcoded without a configuration option. Consider making this configurable since statefulness has implications for API usage and costs.

---

🎯 Code Quality & Best Practices

Good Practices

✅ Proper use of omitempty JSON tags
✅ Context propagation for cancellation
✅ Detailed logging throughout
✅ Defensive nil checks for pointers

Suggestions

1. Type Safety for ResponseInput (ai_client.go:53)

type ResponseInput interface{}

Using interface{} reduces type safety. Consider:

type ResponseInput []Message

This makes the API clearer and catches type errors at compile time.

2. Magic Strings in Response Parsing (ai_client.go:391-393)

if (content.Type == "output_text" || content.Type == "text") && content.Text != "" {

Define constants for these type strings:

const (
    ResponseTypeMessage    = "message"
    ResponseTypeReasoning  = "reasoning"
    ContentTypeOutputText  = "output_text"
    ContentTypeText        = "text"
    StatusCompleted        = "completed"
)

3. Duplicate Error Logging (ai_client.go:280, 402)

Both methods log "No response content" errors. Consider extracting to a shared helper or standardizing the format.

---

⚡ Performance Considerations

Minor Optimizations

1. String Concatenation in URL Building (ai_client.go:319)

url := baseURL + "/responses"

For consistency with other parts of the codebase, consider using fmt.Sprintf or strings.Join, though the performance impact here is negligible.

2. Repeated Config Access

The GetModel() method accesses m.Config multiple times. While Go compilers optimize this well, consider caching the config pointer if called frequently in hot paths.

No Major Concerns

• Single HTTP client instance properly reused ✅
• Response body properly closed with defer ✅
• No unnecessary allocations in hot paths ✅

---

🔒 Security Concerns

Good Security Practices

✅ API keys properly masked in config display (config_helpers.go:214-220)
✅ Authorization header properly set
✅ No API keys logged in debug statements

Recommendations

1. API Key Validation

Consider adding validation for API key format at configuration load time to catch configuration errors early:

func validateAPIKey(key string, provider string) error {
    if key == "" {
        return nil // Empty is ok, just won't be used
    }
    // Add provider-specific validation
    // e.g., OpenAI keys start with "sk-"
    return nil
}

2. Response Size Limits

The code reads entire response bodies into memory:

body, err := io.ReadAll(resp.Body)

Consider adding a size limit to prevent memory exhaustion:

body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize))

---

🧪 Test Coverage

Excellent Coverage ✅

• ✅ Basic Responses API endpoint test
• ✅ System message handling test
• ✅ API type determination test with all scenarios
• ✅ Existing Azure test maintained

Suggested Additional Tests

1. Error Response Handling

   func TestOpenAIResponsesAPIError(t *testing.T) {
       // Test handling of error responses from API
   }

2. Empty Response Handling

   func TestOpenAIResponsesEmptyOutput(t *testing.T) {
       // Test the fallback logic when no output_text
   }

3. Context Cancellation

   func TestOpenAIResponsesContextCancel(t *testing.T) {
       // Test context cancellation during request
   }

4. Session Override Behavior

   func TestGetModelWithSessionOverrides(t *testing.T) {
       // Test that GetModel respects session overrides
   }

---

📝 Documentation

Excellent Documentation ✅

The README updates are thorough and well-structured:

• Clear provider priority explanation
• Multiple configuration examples
• Environment variable documentation
• Session configuration examples

Minor Suggestion

In README.md line 117, the model name "gpt-5-codex" should be verified against OpenAI's actual model naming. Consider adding a note about checking OpenAI's documentation for available models.

---

🎯 Summary

This is a well-implemented PR with good structure, testing, and documentation. The main concerns are:

1. Critical: Fix session override handling in GetModel() for consistency
2. Medium: Validate the default model name "gpt-5-codex"
3. Nice-to-have: Add response size limits and additional error handling tests

The code is production-ready after addressing the session override issue. Great work on maintaining backward compatibility and adding comprehensive tests! 🚀

---

Recommendation: Approve with minor changes requested for the session override handling.

[alvinunreal]

---

[claude]

Pull Request Review: OpenAI Responses API Integration

Overview

This PR adds support for OpenAI's Responses API alongside the existing Chat Completions API, with a priority-based provider selection system (OpenAI → Azure → OpenRouter).

✅ Strengths

Code Quality

• Well-structured API abstraction: Clean separation between Responses API and Chat Completions API types
• Comprehensive test coverage: Added 4 new test functions covering the Responses API, system messages, API type determination, and session overrides
• Backward compatibility: Existing functionality is preserved while adding new capabilities
• Good documentation: README clearly explains provider options and priority

Best Practices

• Proper error handling throughout the new code paths
• Consistent logging with debug statements for troubleshooting
• Clean HTTP client usage with context support for cancellation
• Session override pattern maintained across all providers

⚠️ Issues & Recommendations

1. Critical: Model Name Typo in Documentation

Location: README.md, config.example.yaml, and multiple locations

The model name gpt-5-codex appears throughout the documentation and tests. As of January 2025, this model does not exist in OpenAI's public API.

Recommendation: Use valid model names like gpt-4o, gpt-4-turbo, or gpt-3.5-turbo. Or clarify if this is for a future/preview model.

Files to update: README.md:117, 356 | config.example.yaml:12 | internal/ai_client_test.go:56 | internal/config_helpers.go:150

2. Security: API Key Exposure Risk

Consider adding a security note about never committing API keys to version control.

3. Potential Bug: Empty Input Handling

Location: internal/ai_client.go:303-321

When only a system message is provided without user message, the code returns an error. Verify if the Responses API supports this use case.

4. Performance: HTTP Client Timeout

Consider adding configurable timeouts for API requests to prevent hanging connections.

5. Code Duplication: Header Setting

Location: internal/ai_client.go:335-339

The headers HTTP-Referer and X-Title are set only for Responses API. Either apply consistently or document why they're API-specific.

6. Test Coverage: Integration Tests

Add test for error path where all API keys are missing.

7. Type Safety: ResponseInput Interface

Location: internal/ai_client.go:53

ResponseInput is defined as interface{}, which loses type safety. Consider using a more specific type.

8. Documentation: API Selection Logic

Location: internal/ai_client.go:118-131

Add comments explaining priority logic and consider allowing explicit provider selection.

9. Minor: Magic Strings

Define constants for API type strings (responses, azure, openrouter).

10. Logging Consistency

Location: internal/ai_client.go:401

Truncate large responses in error logs to avoid log pollution.

🔒 Security Considerations

✅ API Key Handling: Keys are never logged
✅ Input Validation: Proper validation of API responses
✅ Context Cancellation: Properly handled
✅ HTTPS: Default URLs use HTTPS

Minor concern: Validate that custom base_url configurations use HTTPS.

📊 Test Coverage Assessment

Coverage: Good ✅

• Responses API endpoint tested
• System message handling tested
• API type determination tested
• Session overrides tested

Missing coverage: Error responses, network timeouts, malformed JSON, rate limiting

🎯 Summary

This is a solid PR with good backward compatibility and test coverage.

Main issues:

1. Critical: Update model name from gpt-5-codex to a valid model
2. Important: Document API selection behavior more clearly
3. Nice-to-have: Address code quality suggestions above

Recommendation: ✅ Approve with minor changes (fix model name)

Great work on maintaining backward compatibility and comprehensive testing! 🎉

[aptdnfapt]

hi man what are we doing now ? porting to native tool call ? i gave up one this when i saw the tmux mcp which can do everything this does