fix: support custom model protocols (e.g., zai-org/GLM-4.7)

[jedi108]

Problem

When using custom models with unknown protocols (e.g., `zai-org/GLM-4.7`), the system returns an error:

```
Error creating provider: failed to create provider for model "zai-org/GLM-4.7": unknown protocol "zai-org" in model "zai-org/GLM-4.7"
```

Root Cause

In `pkg/providers/factory_provider.go`, the `ExtractProtocol()` function splits the model string by the first `/` character. For `"zai-org/GLM-4.7"`, this results in:

• `protocol="zai-org"`
• `modelID="GLM-4.7"`

Since `"zai-org"` is not in the list of supported protocols, the system returns an error.

Solution

Added a `default` case in `CreateProviderFromConfig()` that treats unknown protocols as OpenAI-compatible HTTP providers instead of returning an error:

```go
default:
// Treat unknown protocols as OpenAI-compatible
logger.WarnF("unknown protocol, falling back to OpenAI-compatible HTTP provider",
map[string]any{"protocol": protocol, "model": cfg.Model})
if cfg.APIKey == "" && cfg.APIBase == "" {
return nil, "", fmt.Errorf("api_key or api_base is required for HTTP-based protocol %q", protocol)
}
apiBase := cfg.APIBase
if apiBase == "" {
apiBase = getDefaultAPIBase("openai")
}
return NewHTTPProviderWithMaxTokensFieldAndRequestTimeout(
cfg.APIKey, apiBase, cfg.Proxy, cfg.MaxTokensField, cfg.RequestTimeout,
), cfg.Model, nil
```

Key points:

• Returns `cfg.Model` (full model name) instead of just `modelID`, so `"zai-org/GLM-4.7"` is sent to the API as-is
• Uses `NewHTTPProviderWithMaxTokensFieldAndRequestTimeout` — consistent with all other HTTP provider cases
• Logs a structured warning via `WarnF` so operators can see the fallback

Configuration

```json
{
"model_name": "zai-org/GLM-4.7",
"model": "zai-org/GLM-4.7",
"api_base": "https://your-custom-api-endpoint.com/v1",
"api_key": "..."
}
```

Testing

Added unit tests covering all three scenarios:

Test	Scenario
`TestCreateProviderFromConfig_UnknownProtocol`	Unknown protocol + `api_key` → fallback succeeds, returns full `cfg.Model` as model ID
`TestCreateProviderFromConfig_UnknownProtocolWithAPIBase`	Unknown protocol + `api_base` only (no key) → fallback succeeds (unauthenticated local endpoint)
`TestCreateProviderFromConfig_UnknownProtocolNoAuth`	Unknown protocol + no credentials → returns error

Backward Compatibility

• All existing protocol configurations continue to work unchanged
• Unknown protocols now fall back gracefully instead of hard-erroring

Fixes #661

[nikolasdehor]

Reasonable approach — falling back to OpenAI-compatible for unknown protocols is better than a hard error.

Two observations:

1. Validation gap: The check cfg.APIKey == "" && cfg.APIBase == "" uses AND, meaning if a user provides api_base but no api_key, it will pass through. That is probably fine (some local endpoints are unauthenticated), but worth noting it is intentional.

2. Missing test: There is no test for the new default branch. At minimum, a test covering (a) unknown protocol with api_base set, (b) unknown protocol with neither key nor base (expecting the error), and (c) verifying cfg.Model is returned as the model ID, would be good additions. Since this changes error behavior for all unknown prefixes, test coverage is important.

Otherwise, the change is backward-compatible and solves the reported issue.

[vs4vijay]

Lets merge this PR, we need this support.

[yinwm]

🔍 Deep Code Review

Thanks for the PR! Here's a detailed review of the changes.

---

🚨 Critical Issues (MUST FIX)

1. Test Will Fail - Test Case Not Updated

TestCreateProviderFromConfig_UnknownProtocol expects unknown protocols to return an error:

// factory_provider_test.go:223-234
func TestCreateProviderFromConfig_UnknownProtocol(t *testing.T) {
    cfg := &config.ModelConfig{
        ModelName: "test-unknown",
        Model:     "unknown-protocol/model",
        APIKey:    "test-key",  // <-- APIKey is provided
    }
    _, _, err := CreateProviderFromConfig(cfg)
    if err == nil {
        t.Fatal("CreateProviderFromConfig() expected error for unknown protocol")  // <-- This will fail!
    }
}

With this PR's change, when APIKey is provided, no error is returned. This test will fail.

---

2. Wrong Constructor Used - Missing RequestTimeout Parameter

The PR uses:

return NewHTTPProviderWithMaxTokensField(cfg.APIKey, apiBase, cfg.Proxy, cfg.MaxTokensField), cfg.Model, nil

But all other HTTP providers use:

return NewHTTPProviderWithMaxTokensFieldAndRequestTimeout(
    cfg.APIKey,
    apiBase,
    cfg.Proxy,
    cfg.MaxTokensField,
    cfg.RequestTimeout,  // <-- Missing this parameter!
), modelID, nil

This is a bug - users' RequestTimeout configuration will be ignored.

---

⚠️ Medium Issues (SHOULD FIX)

3. Return Value Semantic Inconsistency

The function documentation states it returns "model ID (without protocol prefix)":

// Returns the provider, the model ID (without protocol prefix), and any error.

But the PR returns cfg.Model (full model name) instead of modelID:

return ..., cfg.Model, nil  // Returns "zai-org/GLM-4.7"
// Instead of
return ..., modelID, nil    // Returns "GLM-4.7"

This breaks the function's semantic contract. While the intent may be to send the full model name to the API, this approach:

• Is inconsistent with other cases
• May cause unexpected behavior in other callers

Suggestion: If the full model name is truly needed, update the function's documentation or have the caller handle it.

---

4. Missing Warning Log

When users use an unknown protocol, the system silently treats it as OpenAI-compatible. This can lead to:

• No feedback when users have typos (e.g., anthropic-typo/claude)
• Difficulty debugging configuration issues

Suggested fix:

default:
    log.Printf("WARN: unknown protocol %q, treating as OpenAI-compatible", protocol)
    // ...

---

5. Default API Base Security Concern

apiBase := cfg.APIBase
if apiBase == "" {
    apiBase = getDefaultAPIBase("openai")  // Returns "https://api.openai.com/v1"
}

If a user configures:

{
  "model": "zai-org/GLM-4.7",
  "api_key": "my-custom-key"  // Forgot to configure api_base
}

Requests will be sent to the official OpenAI API using the user's custom API key, leading to authentication failures or worse.

Suggestion: For unknown protocols, api_base should be required, not optional:

default:
    if cfg.APIBase == "" {
        return nil, "", fmt.Errorf("api_base is required for custom protocol %q", protocol)
    }
    // ...

---

💡 Suggestions

6. Better Error Message

Current:

fmt.Errorf("api_key or api_base is required for HTTP-based protocol %q", protocol)

Suggested:

fmt.Errorf("api_key or api_base is required for custom protocol %q (treating as OpenAI-compatible)", protocol)

This helps users understand how the system interprets their configuration.

---

✅ Positive Feedback

1. Solves a real problem: Supporting custom OpenAI-compatible APIs is a reasonable requirement
2. Backward compatible: Behavior for known protocols remains unchanged
3. Clear PR description: Problem, root cause, and solution are well documented

---

📝 Summary

Type	Count
🚨 Critical Issues	2
⚠️ Medium Issues	3
💡 Suggestions	1

Recommendation: Not ready to merge. Please address:

1. Update or remove TestCreateProviderFromConfig_UnknownProtocol test
2. Use NewHTTPProviderWithMaxTokensFieldAndRequestTimeout to support RequestTimeout
3. Consider whether returning the full model name is necessary; if so, update function documentation
4. Add a warning log for unknown protocols
5. Consider requiring api_base for unknown protocols

[CLAassistant]

All committers have signed the CLA.

[jedi108]

Thanks for the detailed review @nikolasdehor! All points have been addressed:

1. AND validation — intentional, allows unauthenticated local endpoints (e.g. api_base: http://localhost:8080/v1 without a key). Documented in the PR description.

2. Tests — added three cases covering all scenarios you mentioned:

   • Unknown protocol + api_key → fallback succeeds, cfg.Model returned as model ID
   • Unknown protocol + api_base only → fallback succeeds (covers the intentional AND case)
   • Unknown protocol + no credentials → returns error

Also updated to use NewHTTPProviderWithMaxTokensFieldAndRequestTimeout (consistent with all other HTTP cases after the recent upstream changes).

@yinwm would appreciate your thoughts since you reviewed the provider area recently — happy to adjust anything.

[sipeed-bot]

@jedi108 Hi! This PR has had no activity for over 2 weeks, so I'm closing it for now to keep things organized. Feel free to reopen anytime if you'd like to continue.