feat: Add Qwen CLI provider support and merge upstream#1750
feat: Add Qwen CLI provider support and merge upstream#1750dome wants to merge 8 commits intosipeed:mainfrom
Conversation
Add support for Qwen Code CLI (qwen) as a new LLM provider. This enables users to use the qwen CLI tool as a provider, similar to the existing claude-cli and codex-cli providers. Changes: - Add QwenCliProvider implementation with JSON event parsing - Add comprehensive unit tests (32 test cases) - Register qwen-cli protocol in factory_provider.go - Add providerTypeQwenCLI in factory.go - Add qwen-code to default model list in defaults.go
There was a problem hiding this comment.
Pull request overview
Adds a new local qwen CLI–backed provider to the PicoClaw providers layer, enabling qwen-cli/... models to be selected via config and used like other CLI providers.
Changes:
- Introduces
QwenCliProviderthat runs theqwenCLI as a subprocess and parses JSON event output intoLLMResponse. - Extends provider factories to recognize
qwen-cliprotocol /qwen-codeprovider selection. - Adds a default
qwen-codemodel entry and comprehensive unit tests for the new provider.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/providers/qwen_cli_provider.go | Implements the Qwen CLI provider, prompt building, and JSON event parsing. |
| pkg/providers/qwen_cli_provider_test.go | Adds unit tests for CLI execution behavior, prompt building, JSON parsing, and config factory integration. |
| pkg/providers/factory.go | Allows selecting Qwen CLI via agents.defaults.provider values. |
| pkg/providers/factory_provider.go | Adds qwen-cli protocol handling in CreateProviderFromConfig. |
| pkg/config/defaults.go | Adds a default qwen-code model entry using qwen-cli/qwen-code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| case "error": | ||
| lastError = event.Error.Message |
| if ctx.Err() == context.Canceled { | ||
| return nil, ctx.Err() |
| ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) | ||
| defer cancel() | ||
|
|
||
| start := time.Now() | ||
| _, err := p.Chat(ctx, []Message{ | ||
| {Role: "user", Content: "Hello"}, | ||
| }, nil, "", nil) | ||
| elapsed := time.Since(start) | ||
|
|
||
| if err == nil { | ||
| t.Fatal("Chat() expected error on context cancellation") | ||
| } | ||
| // Should fail well before the full 2s sleep completes | ||
| if elapsed > 3*time.Second { | ||
| t.Errorf("Chat() took %v, expected to fail faster via context cancellation", elapsed) | ||
| } |
| case "qwen-cli", "qwen-code", "qwencode": | ||
| workspace := cfg.WorkspacePath() | ||
| if workspace == "" { | ||
| workspace = "." | ||
| } | ||
| sel.providerType = providerTypeQwenCLI | ||
| sel.workspace = workspace | ||
| return sel, nil |
There was a problem hiding this comment.
Pull request overview
Adds a new local Qwen CLI-backed LLM provider to PicoClaw, integrates it into provider factories, and ships a default model_list entry so it can be selected via config.
Changes:
- Introduce
QwenCliProviderthat shells out toqwenand parses its JSON event output. - Extend provider factory logic to recognize
qwen-cliprotocol / provider selection paths. - Add
qwen-codedefault model entry (qwen-cli/qwen-code) in config defaults, plus comprehensive unit tests for the new provider.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/providers/qwen_cli_provider.go | New CLI-based provider implementation and JSON event parsing. |
| pkg/providers/qwen_cli_provider_test.go | Unit/integration-style tests for prompt building, JSON parsing, and factory integration. |
| pkg/providers/factory.go | Adds provider selection routing for qwen-cli / aliases in legacy selection logic. |
| pkg/providers/factory_provider.go | Adds qwen-cli protocol support to CreateProviderFromConfig. |
| pkg/config/defaults.go | Adds a default qwen-code model entry using the new qwen-cli provider. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| usage = &UsageInfo{ | ||
| PromptTokens: event.Usage.InputTokens, | ||
| CompletionTokens: event.Usage.OutputTokens, | ||
| TotalTokens: event.Usage.TotalTokens, |
There was a problem hiding this comment.
In usage parsing, TotalTokens is set directly from event.Usage.TotalTokens, but qwenUsage marks total_tokens as optional (omitempty). If total_tokens is omitted (or zero), this reports TotalTokens as 0 even when input/output tokens are present. Consider computing TotalTokens as InputTokens+OutputTokens when TotalTokens is 0 (and optionally only populating UsageInfo when tokens are non-zero) to match how other providers report usage.
| usage = &UsageInfo{ | |
| PromptTokens: event.Usage.InputTokens, | |
| CompletionTokens: event.Usage.OutputTokens, | |
| TotalTokens: event.Usage.TotalTokens, | |
| promptTokens := event.Usage.InputTokens | |
| completionTokens := event.Usage.OutputTokens | |
| totalTokens := event.Usage.TotalTokens | |
| if totalTokens == 0 && (promptTokens > 0 || completionTokens > 0) { | |
| totalTokens = promptTokens + completionTokens | |
| } | |
| usage = &UsageInfo{ | |
| PromptTokens: promptTokens, | |
| CompletionTokens: completionTokens, | |
| TotalTokens: totalTokens, |
| } | ||
| } | ||
| case "error": | ||
| lastError = event.Error.Message |
There was a problem hiding this comment.
case "error": lastError = event.Error.Message can panic when the CLI emits an "error" event without an error object (i.e., event.Error is nil). Add a nil check (and optionally fall back to event.Result/Type) before dereferencing event.Error.
| lastError = event.Error.Message | |
| if event.Error != nil && event.Error.Message != "" { | |
| lastError = event.Error.Message | |
| } else if event.Result != "" { | |
| lastError = event.Result | |
| } else { | |
| lastError = "error event with no message" | |
| } |
| case "qwen-cli", "qwen-code", "qwencode": | ||
| workspace := cfg.WorkspacePath() | ||
| if workspace == "" { | ||
| workspace = "." | ||
| } | ||
| sel.providerType = providerTypeQwenCLI | ||
| sel.workspace = workspace | ||
| return sel, nil |
There was a problem hiding this comment.
A new explicit provider branch was added for qwen-cli/qwen-code, but resolveProviderSelection has table-driven tests for other providers and currently has no coverage for this new branch. Add a test case asserting providerTypeQwenCLI is selected and that the workspace defaulting behavior is correct.
| } | ||
| // Should fail well before the full 2s sleep completes | ||
| if elapsed > 3*time.Second { | ||
| t.Errorf("Chat() took %v, expected to fail faster via context cancellation", elapsed) | ||
| } |
There was a problem hiding this comment.
The context-cancellation test checks elapsed > 3*time.Second, but the mock sleeps for 2s. This assertion will still pass even if cancellation is broken (the command could run the full 2s and still be <=3s). Tighten the bound (e.g., assert elapsed is well below the sleep duration) and/or assert the error is context.DeadlineExceeded.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import ( | ||
| "github.com/sipeed/picoclaw/pkg/auth" | ||
| ) |
There was a problem hiding this comment.
This file references fmt/strings/config and other identifiers (e.g., fmt.Errorf, strings.ToLower, config.Config) but the import block only imports pkg/auth. As-is, pkg/providers won’t compile; add the required imports (and remove unused ones) or split logic into separate files with the correct imports.
| sel.apiBase = cfg.Providers.Anthropic.APIBase | ||
| if sel.apiBase == "" { | ||
| sel.apiBase = defaultAnthropicAPIBase | ||
| } | ||
| sel.providerType = providerTypeClaudeAuth |
There was a problem hiding this comment.
defaultAnthropicAPIBase is referenced here but isn’t defined anywhere in pkg/providers (search only finds these call sites). This causes a compile error; either define the constant (likely the default Anthropic base URL) or inline the value / reuse an existing helper.
| if err != nil { | ||
| if ctx.Err() == context.Canceled { | ||
| return nil, ctx.Err() | ||
| } | ||
| if stderrStr := stderr.String(); stderrStr != "" { | ||
| return nil, fmt.Errorf("qwen cli error: %s", stderrStr) | ||
| } | ||
| return nil, fmt.Errorf("qwen cli error: %w", err) |
There was a problem hiding this comment.
When exec.CommandContext exits due to a context deadline, ctx.Err() will be context.DeadlineExceeded, but this code only returns ctx.Err() for context.Canceled. Consider returning ctx.Err() whenever it’s non-nil so callers can reliably detect timeouts vs. CLI failures.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { | ||
| model := cfg.Agents.Defaults.GetModelName() | ||
| providerName := strings.ToLower(cfg.Agents.Defaults.Provider) | ||
| lowerModel := strings.ToLower(model) | ||
|
|
||
| if providerName == "" && model == "" { | ||
| return providerSelection{}, fmt.Errorf("no model configured: agents.defaults.model is empty") | ||
| } |
There was a problem hiding this comment.
resolveProviderSelection uses config.Config, fmt, and strings, but factory.go only imports pkg/auth. As-is, this file won’t compile due to missing imports. Add the required imports (and remove any unused ones) so the new provider selection logic builds.
| // Supported protocols: openai, litellm, novita, anthropic, anthropic-messages, | ||
| // antigravity, claude-cli, codex-cli, qwen-cli, github-copilot |
There was a problem hiding this comment.
The updated comment listing “Supported protocols” is now misleading: CreateProviderFromConfig supports many more protocols (e.g. azure/azure-openai, bedrock, openrouter, groq, etc.) than the short list shown here. Please either expand the comment to match the switch cases, or revert to wording that points readers to the switch as the authoritative list.
| // Supported protocols: openai, litellm, novita, anthropic, anthropic-messages, | |
| // antigravity, claude-cli, codex-cli, qwen-cli, github-copilot | |
| // Supported protocols are determined by the switch on `protocol` below; | |
| // see that switch for the authoritative list. |
| case "anthropic", "claude": | ||
| if cfg.Providers.Anthropic.APIKey != "" || cfg.Providers.Anthropic.AuthMethod != "" { | ||
| if cfg.Providers.Anthropic.AuthMethod == "oauth" || cfg.Providers.Anthropic.AuthMethod == "token" { | ||
| sel.apiBase = cfg.Providers.Anthropic.APIBase | ||
| if sel.apiBase == "" { | ||
| sel.apiBase = defaultAnthropicAPIBase | ||
| } | ||
| sel.providerType = providerTypeClaudeAuth | ||
| return sel, nil | ||
| } | ||
| sel.apiKey = cfg.Providers.Anthropic.APIKey | ||
| sel.apiBase = cfg.Providers.Anthropic.APIBase | ||
| sel.proxy = cfg.Providers.Anthropic.Proxy | ||
| if sel.apiBase == "" { | ||
| sel.apiBase = defaultAnthropicAPIBase | ||
| } |
There was a problem hiding this comment.
defaultAnthropicAPIBase is referenced here but does not appear to be defined anywhere else in the repo (search shows only these references). This will fail to compile. Either define the constant in providers (consistent with other defaults), or inline the default URL string as done in CreateProviderFromConfig ("https://api.anthropic.com/v1").
📝 Description
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist