diff --git a/.gitignore b/.gitignore index 06a8fce77..3ff195fbf 100644 --- a/.gitignore +++ b/.gitignore @@ -44,6 +44,3 @@ tasks/ # Added by goreleaser init: dist/ -akalro-dietpi.pub -akalro-dietpi -.gitignore diff --git a/config/config.example.json b/config/config.example.json index f9c12f61b..55a823009 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -79,14 +79,10 @@ "whatsapp": { "enabled": false, "bridge_url": "ws://localhost:3001", -<<<<<<< main "use_native": false, "session_store_path": "", - "allow_from": [] -======= "allow_from": [], "reasoning_channel_id": "" ->>>>>>> refactor/channel-system }, "feishu": { "enabled": false, @@ -269,4 +265,4 @@ "host": "127.0.0.1", "port": 18790 } -} \ No newline at end of file +} diff --git a/docs/design/issue-783-investigation-and-fix-plan.zh.md b/docs/design/issue-783-investigation-and-fix-plan.zh.md new file mode 100644 index 000000000..1c9fc1e70 --- /dev/null +++ b/docs/design/issue-783-investigation-and-fix-plan.zh.md @@ -0,0 +1,61 @@ +# Issue #783 调研与修复执行文档 + +## 1. 问题澄清（已确认） + +- 现象：当 `agents.*.model.primary/fallbacks` 使用 `model_name` 别名（如 `step-3.5-flash`）时，fallback 链路将别名当作真实 `provider/model` 解析，导致 `provider` 可能为空、`model` 可能错误。 +- 根因：`ResolveCandidates` 仅对字符串做 `ParseModelRef`，未先通过 `model_list` 将别名映射到真实 `model` 字段。 +- 影响： + - fallback 执行可能把别名直接发给 OpenAI-compatible provider，触发 `Unknown Model`。 + - `defaults.provider` 为空时，日志出现 `provider=` 空值。 + +## 2. 本次目标 + +- 修复 fallback 候选解析：优先通过 `model_list` 解析别名。 +- 兼容旧行为：若未命中 `model_list`，继续走原有 `ParseModelRef` 兜底。 +- 补充测试：覆盖别名、嵌套路径模型（如 `openrouter/stepfun/...`）、空默认 provider。 +- 验证代码风格：与当前仓库风格保持一致（命名、错误处理、测试结构）。 + +## 3. 联网最佳实践调研结论（已完成） + +- [x] 查阅 OpenAI-compatible 网关（如 OpenRouter）对 `model` 字段的推荐处理。 +- [x] 查阅多 provider/fallback 设计最佳实践（候选解析、日志可观测性）。 +- [x] 将外部建议映射为本仓库可执行约束。 + +外部参考要点（来自 OpenRouter/LiteLLM/Cloudflare AI Gateway 等官方文档）： + +- 优先显式配置，不依赖字符串切分推断 provider。 +- 对网关模型标识应保留完整路径语义，避免截断导致 Unknown Model。 +- fallback 与 primary 应复用同一解析策略，避免“主路径正确、降级路径错误”。 + +参考链接： + +- OpenRouter Provider Routing: https://openrouter.ai/docs/guides/routing/provider-selection +- OpenRouter Model Fallbacks: https://openrouter.ai/docs/guides/routing/model-fallbacks +- OpenRouter Chat Completion API: https://openrouter.ai/docs/api-reference/chat-completion +- LiteLLM Router Architecture: https://docs.litellm.ai/docs/router_architecture +- Cloudflare AI Gateway Chat Completion: https://developers.cloudflare.com/ai-gateway/usage/chat-completion/ + +与本仓库对应的可执行约束： + +- 在 fallback candidate 构建阶段先做 `model_name -> model_list.model` 映射。 +- 未命中映射时保留旧解析行为，保证兼容性。 +- 用新增测试锁定“别名 + 嵌套模型路径 + 空默认 provider”场景。 + +## 4. 实施步骤（顺序执行） + +- [x] Step 1: 对齐现有代码模式，定位最小改动点（`pkg/agent` + `pkg/providers`）。 +- [x] Step 2: 实现“基于 model_list 的 fallback 候选解析”。 +- [x] Step 3: 增加/更新单元测试，覆盖 issue 场景。 +- [x] Step 4: 代码风格一致性复核（与现有文件风格对照）。 +- [x] Step 5: 运行质量门禁（LSP + `make check`）。 + +## 5. 执行记录 + +- 状态：已完成 +- 已完成改动： + - `pkg/providers/fallback.go`：新增 `ResolveCandidatesWithLookup`，并保持 `ResolveCandidates` 向后兼容。 + - `pkg/agent/instance.go`：在构建 fallback candidates 前，优先通过 `model_list` 解析别名，并对无协议模型补齐默认 `openai/` 前缀后再解析。 + - `pkg/providers/fallback_test.go`：新增别名解析与去重测试。 + - `pkg/agent/instance_test.go`：新增 agent 侧别名解析到嵌套模型路径、无协议模型解析测试。 +- 风格对齐检查（完成）：与 `pkg/providers/fallback_test.go`、`pkg/providers/model_ref_test.go` 现有模式一致。 +- 质量验证（完成）：先 `make generate`，后 `make check` 全量通过。 diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index a6fd365c7..65a1fe04d 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -92,7 +92,47 @@ func NewAgentInstance( Primary: model, Fallbacks: fallbacks, } - candidates := providers.ResolveCandidates(modelCfg, defaults.Provider) + resolveFromModelList := func(raw string) (string, bool) { + ensureProtocol := func(model string) string { + model = strings.TrimSpace(model) + if model == "" { + return "" + } + if strings.Contains(model, "/") { + return model + } + return "openai/" + model + } + + raw = strings.TrimSpace(raw) + if raw == "" { + return "", false + } + + if cfg != nil { + if mc, err := cfg.GetModelConfig(raw); err == nil && mc != nil && strings.TrimSpace(mc.Model) != "" { + return ensureProtocol(mc.Model), true + } + + for i := range cfg.ModelList { + fullModel := strings.TrimSpace(cfg.ModelList[i].Model) + if fullModel == "" { + continue + } + if fullModel == raw { + return ensureProtocol(fullModel), true + } + _, modelID := providers.ExtractProtocol(fullModel) + if modelID == raw { + return ensureProtocol(fullModel), true + } + } + } + + return "", false + } + + candidates := providers.ResolveCandidatesWithLookup(modelCfg, defaults.Provider, resolveFromModelList) return &AgentInstance{ ID: agentID, diff --git a/pkg/agent/instance_test.go b/pkg/agent/instance_test.go index fcc8e9bea..af1bf2ead 100644 --- a/pkg/agent/instance_test.go +++ b/pkg/agent/instance_test.go @@ -93,3 +93,77 @@ func TestNewAgentInstance_DefaultsTemperatureWhenUnset(t *testing.T) { t.Fatalf("Temperature = %f, want %f", agent.Temperature, 0.7) } } + +func TestNewAgentInstance_ResolveCandidatesFromModelListAlias(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-instance-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "step-3.5-flash", + }, + }, + ModelList: []config.ModelConfig{ + { + ModelName: "step-3.5-flash", + Model: "openrouter/stepfun/step-3.5-flash:free", + APIBase: "https://openrouter.ai/api/v1", + }, + }, + } + + provider := &mockProvider{} + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider) + + if len(agent.Candidates) != 1 { + t.Fatalf("len(Candidates) = %d, want 1", len(agent.Candidates)) + } + if agent.Candidates[0].Provider != "openrouter" { + t.Fatalf("candidate provider = %q, want %q", agent.Candidates[0].Provider, "openrouter") + } + if agent.Candidates[0].Model != "stepfun/step-3.5-flash:free" { + t.Fatalf("candidate model = %q, want %q", agent.Candidates[0].Model, "stepfun/step-3.5-flash:free") + } +} + +func TestNewAgentInstance_ResolveCandidatesFromModelListAliasWithoutProtocol(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-instance-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "glm-5", + }, + }, + ModelList: []config.ModelConfig{ + { + ModelName: "glm-5", + Model: "glm-5", + APIBase: "https://api.z.ai/api/coding/paas/v4", + }, + }, + } + + provider := &mockProvider{} + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider) + + if len(agent.Candidates) != 1 { + t.Fatalf("len(Candidates) = %d, want 1", len(agent.Candidates)) + } + if agent.Candidates[0].Provider != "openai" { + t.Fatalf("candidate provider = %q, want %q", agent.Candidates[0].Provider, "openai") + } + if agent.Candidates[0].Model != "glm-5" { + t.Fatalf("candidate model = %q, want %q", agent.Candidates[0].Model, "glm-5") + } +} diff --git a/pkg/channels/telegram/telegram_commands.go b/pkg/channels/telegram/telegram_commands.go index ee3bfef51..496fc5e4f 100644 --- a/pkg/channels/telegram/telegram_commands.go +++ b/pkg/channels/telegram/telegram_commands.go @@ -119,7 +119,7 @@ func (c *cmd) List(ctx context.Context, message telego.Message) error { if provider == "" { provider = "configured default" } - response = fmt.Sprintf("Configured Model: %s\nProvider: %s\n\nTo change models, update config.yaml", + response = fmt.Sprintf("Configured Model: %s\nProvider: %s\n\nTo change models, update config.json", c.config.Agents.Defaults.GetModelName(), provider) case "channels": diff --git a/pkg/config/config.go b/pkg/config/config.go index 2e0215278..d84772d2b 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -172,7 +172,7 @@ type AgentDefaults struct { RestrictToWorkspace bool `json:"restrict_to_workspace" env:"PICOCLAW_AGENTS_DEFAULTS_RESTRICT_TO_WORKSPACE"` Provider string `json:"provider" env:"PICOCLAW_AGENTS_DEFAULTS_PROVIDER"` ModelName string `json:"model_name,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_MODEL_NAME"` - Model string `json:"model,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_MODEL"` // Deprecated: use model_name instead + Model string `json:"model" env:"PICOCLAW_AGENTS_DEFAULTS_MODEL"` // Deprecated: use model_name instead ModelFallbacks []string `json:"model_fallbacks,omitempty"` ImageModel string `json:"image_model,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_IMAGE_MODEL"` ImageModelFallbacks []string `json:"image_model_fallbacks,omitempty"` diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 45cdd8ec8..12fd10b50 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" ) @@ -324,6 +325,25 @@ func TestSaveConfig_FilePermissions(t *testing.T) { } } +func TestSaveConfig_IncludesEmptyLegacyModelField(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "config.json") + + cfg := DefaultConfig() + if err := SaveConfig(path, cfg); err != nil { + t.Fatalf("SaveConfig failed: %v", err) + } + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile failed: %v", err) + } + + if !strings.Contains(string(data), `"model": ""`) { + t.Fatalf("saved config should include empty legacy model field, got: %s", string(data)) + } +} + // TestConfig_Complete verifies all config fields are set func TestConfig_Complete(t *testing.T) { cfg := DefaultConfig() diff --git a/pkg/providers/fallback.go b/pkg/providers/fallback.go index ecd451ec9..7ba563b66 100644 --- a/pkg/providers/fallback.go +++ b/pkg/providers/fallback.go @@ -43,11 +43,26 @@ func NewFallbackChain(cooldown *CooldownTracker) *FallbackChain { // ResolveCandidates parses model config into a deduplicated candidate list. func ResolveCandidates(cfg ModelConfig, defaultProvider string) []FallbackCandidate { + return ResolveCandidatesWithLookup(cfg, defaultProvider, nil) +} + +func ResolveCandidatesWithLookup( + cfg ModelConfig, + defaultProvider string, + lookup func(raw string) (resolved string, ok bool), +) []FallbackCandidate { seen := make(map[string]bool) var candidates []FallbackCandidate addCandidate := func(raw string) { - ref := ParseModelRef(raw, defaultProvider) + candidateRaw := strings.TrimSpace(raw) + if lookup != nil { + if resolved, ok := lookup(candidateRaw); ok { + candidateRaw = resolved + } + } + + ref := ParseModelRef(candidateRaw, defaultProvider) if ref == nil { return } diff --git a/pkg/providers/fallback_test.go b/pkg/providers/fallback_test.go index ebba054ef..1783ebcb5 100644 --- a/pkg/providers/fallback_test.go +++ b/pkg/providers/fallback_test.go @@ -453,6 +453,75 @@ func TestResolveCandidates_EmptyPrimary(t *testing.T) { } } +func TestResolveCandidatesWithLookup_AliasResolvesToNestedModel(t *testing.T) { + cfg := ModelConfig{ + Primary: "step-3.5-flash", + Fallbacks: nil, + } + + lookup := func(raw string) (string, bool) { + if raw == "step-3.5-flash" { + return "openrouter/stepfun/step-3.5-flash:free", true + } + return "", false + } + + candidates := ResolveCandidatesWithLookup(cfg, "", lookup) + if len(candidates) != 1 { + t.Fatalf("candidates = %d, want 1", len(candidates)) + } + if candidates[0].Provider != "openrouter" { + t.Fatalf("provider = %q, want openrouter", candidates[0].Provider) + } + if candidates[0].Model != "stepfun/step-3.5-flash:free" { + t.Fatalf("model = %q, want stepfun/step-3.5-flash:free", candidates[0].Model) + } +} + +func TestResolveCandidatesWithLookup_DeduplicateAfterLookup(t *testing.T) { + cfg := ModelConfig{ + Primary: "step-3.5-flash", + Fallbacks: []string{"openrouter/stepfun/step-3.5-flash:free"}, + } + + lookup := func(raw string) (string, bool) { + if raw == "step-3.5-flash" { + return "openrouter/stepfun/step-3.5-flash:free", true + } + return "", false + } + + candidates := ResolveCandidatesWithLookup(cfg, "", lookup) + if len(candidates) != 1 { + t.Fatalf("candidates = %d, want 1", len(candidates)) + } +} + +func TestResolveCandidatesWithLookup_AliasWithoutProtocolUsesDefaultProvider(t *testing.T) { + cfg := ModelConfig{ + Primary: "glm-5", + Fallbacks: nil, + } + + lookup := func(raw string) (string, bool) { + if raw == "glm-5" { + return "glm-5", true + } + return "", false + } + + candidates := ResolveCandidatesWithLookup(cfg, "openai", lookup) + if len(candidates) != 1 { + t.Fatalf("candidates = %d, want 1", len(candidates)) + } + if candidates[0].Provider != "openai" { + t.Fatalf("provider = %q, want openai", candidates[0].Provider) + } + if candidates[0].Model != "glm-5" { + t.Fatalf("model = %q, want glm-5", candidates[0].Model) + } +} + func TestFallbackExhaustedError_Message(t *testing.T) { e := &FallbackExhaustedError{ Attempts: []FallbackAttempt{