diff --git a/server/pkg/agent/codebuddy_discovery_fallback_test.go b/server/pkg/agent/codebuddy_discovery_fallback_test.go index 0eeadedbcb..d726927db7 100644 --- a/server/pkg/agent/codebuddy_discovery_fallback_test.go +++ b/server/pkg/agent/codebuddy_discovery_fallback_test.go @@ -4,220 +4,290 @@ package agent import ( "context" + "encoding/json" "os" "path/filepath" - "strconv" "strings" "testing" ) -// writeCodebuddyStub writes an executable stub at /codebuddy whose -// `--help` behaviour is supplied by body. Tests use a stub rather than the -// real CLI so the default suite never executes a user-installed agent binary. -func writeCodebuddyStub(t *testing.T, body string) string { +// codebuddyACPSessionResult is the shape CodeBuddy 2.130.0 actually returns from +// `session/new` over `codebuddy --acp`, trimmed to four models. Captured from the +// real CLI: the catalog lives under models.availableModels with an advertised +// currentModelId, and the effort catalog rides along in configOptions as +// thought_level — including the `enabled` choice that is a session toggle rather +// than a valid `--effort` argument. +const codebuddyACPSessionResult = `{"sessionId":"ses-codebuddy","models":{"currentModelId":"hy3",` + + `"availableModels":[` + + `{"modelId":"hy3","name":"Hy3","description":"x0.00 credits"},` + + `{"modelId":"glm-5.2","name":"GLM-5.2","description":"x0.79 credits"},` + + `{"modelId":"kimi-k3-1","name":"Kimi-K3","description":"x1.62 credits"},` + + `{"modelId":"deepseek-v3-2-volc","name":"DeepSeek-V3.2","description":"x0.29 credits"}]},` + + `"configOptions":[` + + `{"type":"select","id":"mode","name":"Permission Mode","currentValue":"default","options":[{"value":"default","name":"Default"}]},` + + `{"type":"select","id":"thought_level","name":"Deep Thinking","category":"thought_level","currentValue":"enabled","options":[` + + `{"value":"minimal","name":"Minimal"},{"value":"low","name":"Low"},{"value":"medium","name":"Medium"},` + + `{"value":"high","name":"High"},{"value":"xhigh","name":"X-High"},{"value":"max","name":"Max"},` + + `{"value":"enabled","name":"On (default)"}]}]}` + +// writeCodebuddyACPStub writes an executable stub that speaks just enough ACP for +// discovery. sessionResult is returned from session/new; when it is empty the +// stub fails that call, which is how a not-logged-in CLI is simulated. +func writeCodebuddyACPStub(t *testing.T, sessionResult string) string { t.Helper() - path := filepath.Join(t.TempDir(), "codebuddy") - if err := os.WriteFile(path, []byte(body), 0o755); err != nil { - t.Fatalf("write codebuddy stub: %v", err) + dir := t.TempDir() + path := filepath.Join(dir, "codebuddy") + sessionReply := `printf '{"jsonrpc":"2.0","id":%s,"result":` + sessionResult + `}\n' "$id"` + if sessionResult == "" { + sessionReply = `printf '{"jsonrpc":"2.0","id":%s,"error":{"code":-32000,"message":"not authenticated"}}\n' "$id"` + } + script := `#!/bin/sh +# Minimal CodeBuddy ACP stub: initialize + session/new only, which is all model +# discovery drives. --version answers so the runtime-registration probe is happy. +case "$1" in + --version) echo '2.130.0'; exit 0 ;; +esac +while IFS= read -r line; do + id=$(printf '%s' "$line" | sed -n 's/.*"id":\([0-9]*\).*/\1/p') + case "$line" in + *'"method":"initialize"'*) + printf '{"jsonrpc":"2.0","id":%s,"result":{"protocolVersion":1,"agentCapabilities":{"loadSession":true},"authMethods":[{"id":"external","name":"Login with Google/Github"}]}}\n' "$id" + ;; + *'"method":"session/new"'*) + ` + sessionReply + ` + exit 0 + ;; + esac +done +` + if err := os.WriteFile(path, []byte(script), 0o755); err != nil { + t.Fatalf("write codebuddy ACP stub: %v", err) } return path } -// resetCodebuddyHelpCache drops any memoised --help output for path so each -// case actually re-runs the stub. codebuddyHelpStore is a package global. -func resetCodebuddyHelpCache(t *testing.T, path string) { +func resetCodebuddyDiscoveryCaches(t *testing.T) { t.Helper() - drop := func() { - codebuddyHelpMu.Lock() - delete(codebuddyHelpStore, path) - codebuddyHelpMu.Unlock() + clear := func() { + modelCacheMu.Lock() + delete(modelCache, "codebuddy") + modelCacheMu.Unlock() + resetThinkingCacheForTests() } - drop() - t.Cleanup(drop) + clear() + t.Cleanup(clear) } -// TestCodebuddyHelpOutputRejectsFailedExec is the MUL-5549 regression: a -// `#!/usr/bin/env node` codebuddy whose interpreter is not on a GUI-launched -// daemon's PATH exits 127 and prints `env: node: No such file or directory` on -// stderr. codebuddyHelpOutput used to discard the exit status and hand that -// stderr back as if it were help text, so every parser downstream silently -// fell back — and the failure was then memoised for codebuddyHelpTTL. -func TestCodebuddyHelpOutputRejectsFailedExec(t *testing.T) { - path := writeCodebuddyStub(t, "#!/bin/sh\necho 'env: node: No such file or directory' >&2\nexit 127\n") - resetCodebuddyHelpCache(t, path) - - if got := codebuddyHelpOutput(context.Background(), path); got != "" { - t.Fatalf("a non-zero exit must yield no help text, got %q", got) - } - - codebuddyHelpMu.Lock() - _, cached := codebuddyHelpStore[path] - codebuddyHelpMu.Unlock() - if cached { - t.Error("a failed --help must not be cached; it would pin the failure for codebuddyHelpTTL") - } -} - -// TestDiscoverCodebuddyModelsMarksFallback pins that every degraded path is -// reported as Fallback. Before MUL-5549 all three returned (staticModels, nil), -// which the daemon reported as a successful discovery and the server then -// cached as this runtime's real catalog for 24h. -func TestDiscoverCodebuddyModelsMarksFallback(t *testing.T) { - for _, tc := range []struct { - name string - path string - }{ - {"binary missing", missingAgentExecutable(t, "codebuddy")}, - {"help exec fails", writeCodebuddyStub(t, "#!/bin/sh\necho 'env: node: No such file or directory' >&2\nexit 127\n")}, - {"help has no model line", writeCodebuddyStub(t, "#!/bin/sh\necho 'Usage: codebuddy [options]'\n")}, - } { - t.Run(tc.name, func(t *testing.T) { - resetCodebuddyHelpCache(t, tc.path) - - catalog, err := discoverCodebuddyModels(context.Background(), tc.path) - if err != nil { - t.Fatalf("discoverCodebuddyModels: %v", err) - } - if !catalog.Fallback { - t.Error("a degraded discovery must be marked Fallback") - } - // The models are still returned — the picker stays populated. - if len(catalog.Models) == 0 { - t.Error("expected the static stand-in to still be offered to the UI") - } - }) - } -} - -// TestDiscoverCodebuddyModelsRealHelpIsNotFallback is the other half: a stub -// emitting the real v2.130.0 `--model` line must parse and must NOT be marked -// Fallback, so a genuine catalog still reaches the cache. -func TestDiscoverCodebuddyModelsRealHelpIsNotFallback(t *testing.T) { - const helpLine = ` --model Model for the current session. Please provide the model ID. ` + - `Currently supported: (hy3, glm-5.2, minimax-m3, kimi-k3-1, deepseek-v4-pro)` - path := writeCodebuddyStub(t, "#!/bin/sh\ncat <<'EOF'\nUsage: codebuddy [options]\n"+helpLine+"\nEOF\n") - resetCodebuddyHelpCache(t, path) +// TestDiscoverCodebuddyModelsFromACP is the core of the migration (MUL-5549): +// the catalog now comes from the ACP handshake, so IDs, display names AND the +// default model all come from CodeBuddy instead of being guessed from the ID. +func TestDiscoverCodebuddyModelsFromACP(t *testing.T) { + resetCodebuddyDiscoveryCaches(t) + path := writeCodebuddyACPStub(t, codebuddyACPSessionResult) catalog, err := discoverCodebuddyModels(context.Background(), path) if err != nil { t.Fatalf("discoverCodebuddyModels: %v", err) } if catalog.Fallback { - t.Error("a parsed catalog must not be marked Fallback") + t.Fatal("a successful ACP handshake must not be marked Fallback") } - if len(catalog.Models) != 5 || catalog.Models[0].ID != "hy3" { - t.Fatalf("unexpected catalog: %+v", catalog.Models) + if len(catalog.Models) != 4 { + t.Fatalf("expected 4 models, got %d: %+v", len(catalog.Models), catalog.Models) + } + + // Labels are the CLI's own names. The old --help path had to derive these + // from the ID and got them wrong: kimi-k3-1 became "Kimi K3 1" and + // deepseek-v3-2-volc became "Deepseek V3 2 Volc". + wantLabel := map[string]string{ + "hy3": "Hy3", + "glm-5.2": "GLM-5.2", + "kimi-k3-1": "Kimi-K3", + "deepseek-v3-2-volc": "DeepSeek-V3.2", } - // The static stand-in shares no IDs with the real catalog, which is what - // made the silent fallback user-visible in the first place. for _, m := range catalog.Models { - for _, static := range codebuddyStaticModels() { - if m.ID == static.ID { - t.Fatalf("real and fallback catalogs must stay distinguishable, both have %q", m.ID) - } + if want, ok := wantLabel[m.ID]; !ok { + t.Errorf("unexpected model %q", m.ID) + } else if m.Label != want { + t.Errorf("label(%q) = %q, want the CLI's own %q", m.ID, m.Label, want) + } + } + + // The default is the advertised currentModelId, not "whichever came first". + var defaults []string + for _, m := range catalog.Models { + if m.Default { + defaults = append(defaults, m.ID) + } + } + if len(defaults) != 1 || defaults[0] != "hy3" { + t.Errorf("default models = %v, want exactly [hy3] from currentModelId", defaults) + } + + // The ACP payload has no vendor field and CodeBuddy's ids are bare, so + // without the prefix post-pass every model would land in one unlabelled + // group instead of the picker's per-vendor sections. + wantProvider := map[string]string{ + "hy3": "hunyuan", + "glm-5.2": "zhipu", + "kimi-k3-1": "kimi", + "deepseek-v3-2-volc": "deepseek", + } + for _, m := range catalog.Models { + if want := wantProvider[m.ID]; m.Provider != want { + t.Errorf("provider(%q) = %q, want %q — the picker groups on this", m.ID, m.Provider, want) } } } -// countingCodebuddyStub writes a codebuddy stub that appends a line to a -// counter file on every `--help` invocation, so a test can assert how many -// times the (slow, 35s-capped) command actually ran. helpBody is emitted on -// stdout for --help; --version always succeeds so runtime registration and the -// thinking-cache key lookup behave normally. -func countingCodebuddyStub(t *testing.T, helpBody string, helpExit int) (path, counter string) { - t.Helper() - dir := t.TempDir() - counter = filepath.Join(dir, "help-calls") - path = filepath.Join(dir, "codebuddy") - script := "#!/bin/sh\n" + - "case \"$1\" in\n" + - " --version) echo '2.130.0'; exit 0 ;;\n" + - " --help) echo call >> " + counter + "\n" + - " cat <<'HELPEOF'\n" + helpBody + "\nHELPEOF\n" + - " exit " + itoa(helpExit) + " ;;\n" + - "esac\n" + - "exit 0\n" - if err := os.WriteFile(path, []byte(script), 0o755); err != nil { - t.Fatalf("write counting stub: %v", err) +// TestCodebuddyModelProviderCoversRealCatalog pins vendor inference against every +// ID shape CodeBuddy 2.130.0 actually advertises. A miss here is invisible in the +// backend but collapses the picker into one unlabelled list. +func TestCodebuddyModelProviderCoversRealCatalog(t *testing.T) { + t.Parallel() + for id, want := range map[string]string{ + // Live ACP catalog, all 16 ids. + "hy3": "hunyuan", "glm-5.2": "zhipu", "glm-5.1": "zhipu", "glm-5.0": "zhipu", + "glm-5.0-turbo": "zhipu", "glm-5v-turbo": "zhipu", "glm-4.7": "zhipu", + "minimax-m3": "minimax", "minimax-m2.7": "minimax", + "kimi-k3-1": "kimi", "kimi-k2.7": "kimi", "kimi-k2.6": "kimi", "kimi-k2.5": "kimi", + "deepseek-v4-pro": "deepseek", "deepseek-v4-flash": "deepseek", "deepseek-v3-2-volc": "deepseek", + // Static fallback ids, which must group too. + "claude-sonnet-4.6": "anthropic", "claude-opus-4.7": "anthropic", + "gemini-3.1-pro": "google", "gpt-5.5": "openai", + "deepseek-v3-2-volc-ioa": "deepseek", + } { + if got := codebuddyModelProvider(id); got != want { + t.Errorf("codebuddyModelProvider(%q) = %q, want %q", id, got, want) + } } - return path, counter } -func itoa(n int) string { return strconv.Itoa(n) } +// TestCodebuddyStaticModelsCarryProviders guards the fallback path's grouping: +// those entries hardcode Provider rather than going through the post-pass, so a +// new entry added without one would silently break grouping there instead. +func TestCodebuddyStaticModelsCarryProviders(t *testing.T) { + t.Parallel() + for _, m := range codebuddyStaticModels() { + if m.Provider == "" { + t.Errorf("static fallback model %q has no Provider; the picker would not group it", m.ID) + } + if want := codebuddyModelProvider(m.ID); m.Provider != want { + t.Errorf("static fallback %q has Provider %q but prefix inference says %q", m.ID, m.Provider, want) + } + } +} -func helpCallCount(t *testing.T, counter string) int { - t.Helper() - b, err := os.ReadFile(counter) +// TestDiscoverCodebuddyModelsACPEffort pins the effort catalog coming out of the +// same handshake — no second process — and the `enabled` filtering. +func TestDiscoverCodebuddyModelsACPEffort(t *testing.T) { + resetCodebuddyDiscoveryCaches(t) + path := writeCodebuddyACPStub(t, codebuddyACPSessionResult) + + catalog, err := discoverCodebuddyModels(context.Background(), path) if err != nil { - if os.IsNotExist(err) { - return 0 - } - t.Fatalf("read counter: %v", err) + t.Fatalf("discoverCodebuddyModels: %v", err) + } + thinking := catalog.Models[0].Thinking + if thinking == nil { + t.Fatal("expected the effort catalog to be annotated from the same session/new response") + } + var got []string + for _, lvl := range thinking.SupportedLevels { + got = append(got, lvl.Value) + } + want := []string{"minimal", "low", "medium", "high", "xhigh", "max"} + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("levels = %v, want %v", got, want) + } + // `enabled` is advertised by ACP but `--effort enabled` is not a valid + // command line, so it must not reach the picker... + for _, lvl := range thinking.SupportedLevels { + if lvl.Value == "enabled" { + t.Error("`enabled` is a session toggle, not an --effort value; it must be filtered out") + } + } + // ...and since it is the advertised currentValue, DefaultLevel must stay + // empty ("runtime decides") rather than echo a value we cannot pass. + if thinking.DefaultLevel != "" { + t.Errorf("DefaultLevel = %q, want empty because currentValue was the unusable `enabled`", thinking.DefaultLevel) } - return len(strings.Fields(string(b))) } -// TestListModelsCodebuddyRunsHelpAtMostOnce is the MUL-5549 follow-up: model -// discovery and effort discovery both read `codebuddy --help`, and the effort -// pass used to call it independently. That was masked while a failed --help was -// wrongly memoised; once failures correctly stopped being cached, the failure -// path ran the 35s command TWICE in one request — past the server's 60s running -// timeout, so the request timed out and the late report was discarded as stale. -// The user then got no list at all, not even the fallback. -// -// Both paths must therefore run --help exactly once. This goes through the full -// ListModels entry point, since that is where the second call was introduced. -func TestListModelsCodebuddyRunsHelpAtMostOnce(t *testing.T) { - const realHelp = "Usage: codebuddy [options]\n" + - " --model Currently supported: (hy3, glm-5.2, kimi-k3-1)\n" + - " --effort Reasoning effort level (low, medium, high, xhigh)" +// TestParseACPCodebuddyEffortDefault covers the other branch: when CodeBuddy +// advertises a real level as current, we echo it. +func TestParseACPCodebuddyEffortDefault(t *testing.T) { + raw := json.RawMessage(`{"configOptions":[{"id":"thought_level","currentValue":"high","options":[` + + `{"value":"low"},{"value":"high"},{"value":"enabled"}]}]}`) + levels, def := parseACPCodebuddyEffort(raw) + if strings.Join(levels, ",") != "low,high" { + t.Errorf("levels = %v, want [low high]", levels) + } + if def != "high" { + t.Errorf("DefaultLevel = %q, want high", def) + } +} +// TestDiscoverCodebuddyModelsFallsBackOnACPFailure covers the not-logged-in / +// unreachable-CLI cases. The stand-in is still offered so the picker stays +// usable, but it must be marked Fallback so the server can never cache it as +// this runtime's real catalog (MUL-5549), and the effort picker must still work. +func TestDiscoverCodebuddyModelsFallsBackOnACPFailure(t *testing.T) { for _, tc := range []struct { - name string - helpBody string - helpExit int - wantFallback bool + name string + path func(t *testing.T) string }{ - {name: "help succeeds", helpBody: realHelp, helpExit: 0}, - // The original #6180 failure: interpreter missing, exit 127. - {name: "help fails", helpBody: "env: node: No such file or directory", helpExit: 127, wantFallback: true}, - // Help runs but carries no model line (older//unauthenticated CLI). - {name: "help unparseable", helpBody: "Usage: codebuddy [options]", helpExit: 0, wantFallback: true}, + { + name: "binary missing", + path: func(t *testing.T) string { return missingAgentExecutable(t, "codebuddy") }, + }, + { + name: "session/new refused (not logged in)", + path: func(t *testing.T) string { return writeCodebuddyACPStub(t, "") }, + }, + { + name: "session/new carries no catalog", + path: func(t *testing.T) string { return writeCodebuddyACPStub(t, `{"sessionId":"ses-empty"}`) }, + }, } { t.Run(tc.name, func(t *testing.T) { - path, counter := countingCodebuddyStub(t, tc.helpBody, tc.helpExit) - resetCodebuddyHelpCache(t, path) - // cachedDiscovery and the thinking cache are package globals; clear - // both so this case actually executes the stub. - modelCacheMu.Lock() - delete(modelCache, "codebuddy") - modelCacheMu.Unlock() - resetThinkingCacheForTests() - t.Cleanup(resetThinkingCacheForTests) - - catalog, err := ListModels(context.Background(), "codebuddy", path) + resetCodebuddyDiscoveryCaches(t) + catalog, err := discoverCodebuddyModels(context.Background(), tc.path(t)) if err != nil { - t.Fatalf("ListModels(codebuddy): %v", err) + t.Fatalf("discoverCodebuddyModels: %v", err) } - if catalog.Fallback != tc.wantFallback { - t.Errorf("Fallback = %v, want %v", catalog.Fallback, tc.wantFallback) + if !catalog.Fallback { + t.Error("a degraded discovery must be marked Fallback") } - if got := helpCallCount(t, counter); got != 1 { - t.Errorf("`codebuddy --help` ran %d times, want exactly 1 — "+ - "two 35s attempts in one request exceed the server's 60s running timeout", got) - } - // Whichever path was taken, the picker still gets models and the - // thinking picker still gets levels. if len(catalog.Models) == 0 { - t.Fatal("expected a non-empty catalog") + t.Error("expected the static stand-in to still be offered to the UI") } if catalog.Models[0].Thinking == nil || len(catalog.Models[0].Thinking.SupportedLevels) == 0 { - t.Error("expected effort levels to be annotated on both the real and fallback paths") + t.Error("the fallback path must still annotate effort levels") } }) } } +// TestCodebuddyStaticEffortFallbackCoversFlagValues keeps the offline fallback +// honest against the flag it feeds: every level offered must be one `--effort` +// accepts, and the set should not silently shrink below what the CLI supports. +func TestCodebuddyStaticEffortFallbackCoversFlagValues(t *testing.T) { + t.Parallel() + for _, level := range codebuddyStaticEffortFallback { + if !codebuddyFlagEffortValues[level] { + t.Errorf("static fallback offers %q, which `--effort` does not accept", level) + } + if !IsKnownThinkingValue("codebuddy", level) { + t.Errorf("static fallback offers %q, which the server-side gate rejects", level) + } + } + if len(codebuddyStaticEffortFallback) != len(codebuddyFlagEffortValues) { + t.Errorf("static fallback has %d levels but --effort accepts %d; they drifted apart", + len(codebuddyStaticEffortFallback), len(codebuddyFlagEffortValues)) + } +} + // TestCachedDiscoveryDoesNotCacheFallback pins that a fallback never occupies // the daemon's 60s discovery cache: the next request must be free to retry. func TestCachedDiscoveryDoesNotCacheFallback(t *testing.T) { diff --git a/server/pkg/agent/codebuddy_test.go b/server/pkg/agent/codebuddy_test.go index 2f0e72ba74..d6497ca4d8 100644 --- a/server/pkg/agent/codebuddy_test.go +++ b/server/pkg/agent/codebuddy_test.go @@ -380,79 +380,6 @@ func TestCodebuddyHandleAssistantText(t *testing.T) { } } -func TestParseCodebuddyModels_FullHelp(t *testing.T) { - t.Parallel() - helpOutput := `Usage: codebuddy [options] [command] [prompt] - -Options: - --model Model for the current session. Please provide the model ID. Currently supported: (claude-sonnet-4.6, claude-opus-4.7, gemini-3.1-pro, gpt-5.5, glm-5.1-ioa, minimax-m2.7-ioa, kimi-k2.6-ioa, hy3-preview-ioa, deepseek-v3-2-volc-ioa) - --effort Reasoning effort level (low, medium, high, xhigh) -` - models := parseCodebuddyModels(helpOutput) - if len(models) != 9 { - t.Fatalf("expected 9 models, got %d: %+v", len(models), models) - } - if !models[0].Default { - t.Error("first model should be marked as default") - } - if models[0].ID != "claude-sonnet-4.6" { - t.Errorf("first model ID = %q, want claude-sonnet-4.6", models[0].ID) - } - if models[0].Provider != "anthropic" { - t.Errorf("claude model provider = %q, want anthropic", models[0].Provider) - } - // Spot check providers - providers := map[string]string{} - for _, m := range models { - providers[m.ID] = m.Provider - } - checks := map[string]string{ - "gpt-5.5": "openai", - "gemini-3.1-pro": "google", - "glm-5.1-ioa": "zhipu", - "minimax-m2.7-ioa": "minimax", - "kimi-k2.6-ioa": "kimi", - "hy3-preview-ioa": "hunyuan", - "deepseek-v3-2-volc-ioa": "deepseek", - } - for id, want := range checks { - if got := providers[id]; got != want { - t.Errorf("provider(%q) = %q, want %q", id, got, want) - } - } -} - -func TestParseCodebuddyModels_Malformed(t *testing.T) { - t.Parallel() - models := parseCodebuddyModels("totally unrelated output\nno model line here") - if len(models) != 0 { - t.Fatalf("expected 0 models from malformed output, got %d", len(models)) - } -} - -func TestParseCodebuddyEffortHelp(t *testing.T) { - t.Parallel() - helpOutput := ` --effort Reasoning effort level (low, medium, high, xhigh)` - levels := parseCodebuddyEffortHelp(helpOutput) - expected := []string{"low", "medium", "high", "xhigh"} - if len(levels) != len(expected) { - t.Fatalf("expected %d levels, got %d: %v", len(expected), len(levels), levels) - } - for i, l := range levels { - if l != expected[i] { - t.Errorf("level[%d]: expected %q, got %q", i, expected[i], l) - } - } -} - -func TestParseCodebuddyEffortHelp_Missing(t *testing.T) { - t.Parallel() - levels := parseCodebuddyEffortHelp("no effort line here") - if len(levels) != 0 { - t.Fatalf("expected nil for missing effort line, got %v", levels) - } -} - func TestIsKnownThinkingValue_Codebuddy(t *testing.T) { t.Parallel() cases := []struct { @@ -460,12 +387,17 @@ func TestIsKnownThinkingValue_Codebuddy(t *testing.T) { want bool }{ {"", true}, + {"minimal", true}, {"low", true}, {"medium", true}, {"high", true}, {"xhigh", true}, - {"max", false}, + // CodeBuddy 2.130.0 advertises `max`; the gate used to reject it. + {"max", true}, {"none", false}, + // ACP advertises `enabled` as a session toggle, but `--effort enabled` + // is not a valid command line, so the gate must not accept it. + {"enabled", false}, } for _, tc := range cases { got := IsKnownThinkingValue("codebuddy", tc.value) diff --git a/server/pkg/agent/models.go b/server/pkg/agent/models.go index fb0028a5d9..01b2dec7b1 100644 --- a/server/pkg/agent/models.go +++ b/server/pkg/agent/models.go @@ -10,7 +10,6 @@ import ( "log/slog" "os" "os/exec" - "regexp" "sort" "strings" "sync" @@ -963,6 +962,11 @@ type acpDiscoveryProvider struct { // Legacy discovery providers keep their empty-list behavior; Grok enables // this so it can log the actual fallback reason. strictErrors bool + // annotate receives the parsed catalog plus the raw session/new result so a + // provider can enrich models from parts of the response the shared parser + // ignores. CodeBuddy uses it to read its effort catalog out of the same + // handshake, which is why it needs no separate discovery call at all. + annotate func([]Model, json.RawMessage) } // discoverACPModels runs the ACP handshake for any agent CLI that @@ -1139,6 +1143,9 @@ func discoverACPModels(ctx context.Context, executablePath string, p acpDiscover if err := runCtx.Err(); err != nil { return fail("completion", err) } + if p.annotate != nil { + p.annotate(models, sessionResult) + } return models, nil } @@ -1721,81 +1728,57 @@ func isOpenclawIdentifier(s string) bool { // ── CodeBuddy model discovery ── -// codebuddyModelRe matches the `--model ... Currently supported: (m1, m2, ...)` -// line in `codebuddy --help` output. -var codebuddyModelRe = regexp.MustCompile(`--model\s*<[^>]+>\s*.*?Currently supported:\s*\(([^)]+)\)`) - -// discoverCodebuddyModels runs `codebuddy --help` and extracts the supported -// model list from its output, falling back to a static list when the binary is -// missing or the output cannot be parsed. +// discoverCodebuddyModels asks CodeBuddy for its catalog over ACP +// (`codebuddy --acp`), the same handshake Copilot / Kimi / Kiro / Qoder / Grok / +// TRAE already use. `session/new` answers with a structured catalog under +// `models.availableModels` plus a `currentModelId`, which is what the shared +// parseACPSessionNewModels reads. // -// It runs `--help` AT MOST ONCE per call: the single capture feeds both the -// model catalog and the per-model effort catalog, and the fallback paths skip -// the effort pass entirely. A slow or failing --help therefore cannot be paid -// for twice inside one model-list request, which would exceed the server's 60s -// running timeout and cost the user even the fallback list (MUL-5549). +// This replaces scraping the `--model` line out of `codebuddy --help` (MUL-5549). +// The help text carried IDs and nothing else, so labels had to be guessed from +// the ID and produced names CodeBuddy does not use ("Kimi K3 1" for what the CLI +// calls Kimi-K3, "Deepseek V3 2 Volc" for DeepSeek-V3.2), the default model was +// a "first entry wins" guess rather than the advertised currentModelId, and the +// effort catalog needed a second regex over the same output. Measured against +// CodeBuddy 2.130.0 the handshake is also markedly faster than --help — which +// matters because that command is slow enough to have been the prime suspect for +// the timeouts behind #6180. +// +// Falls back to the static catalog (marked Fallback, so it can never be cached +// as authoritative) when the handshake fails — including the not-logged-in case, +// where session/new may legitimately refuse. func discoverCodebuddyModels(ctx context.Context, executablePath string) (Catalog, error) { - if executablePath == "" { - executablePath = "codebuddy" - } - if _, err := exec.LookPath(executablePath); err != nil { + models, err := discoverACPModels(ctx, executablePath, acpDiscoveryProvider{ + defaultBin: "codebuddy", + clientName: "multica-model-discovery", + tmpdirPrefix: "multica-codebuddy-discovery-", + acpArgs: []string{"--acp"}, + strictErrors: true, + annotate: annotateCodebuddyThinkingFromACP, + }) + if err != nil || len(models) == 0 { + if err != nil { + slog.Debug("codebuddy model discovery fell back to static catalog", "error", err) + } return codebuddyFallbackCatalog(), nil } - helpOut := codebuddyHelpOutput(ctx, executablePath) - if helpOut == "" { - return codebuddyFallbackCatalog(), nil + // Same post-pass Copilot runs: the ACP payload carries no vendor, and + // acpModelEntry can only recover one from a `vendor:model` id. CodeBuddy's + // ids are bare (`glm-5.2`, `kimi-k3-1`), so without this every model lands + // in one unlabelled group instead of the Zhipu / Kimi / DeepSeek sections + // the picker renders from Provider. + for i := range models { + if models[i].Provider == "" { + models[i].Provider = codebuddyModelProvider(models[i].ID) + } } - models := parseCodebuddyModels(helpOut) - if len(models) == 0 { - return codebuddyFallbackCatalog(), nil - } - annotateCodebuddyThinking(ctx, models, executablePath, helpOut) return Catalog{Models: models}, nil } -// codebuddyFallbackCatalog is the static stand-in for a failed discovery. -// -// It applies the static effort levels directly instead of shelling out again: -// whatever broke `--help` for the model catalog (missing binary, missing `node` -// interpreter, timeout) breaks it for the effort catalog too, and a second 35s -// attempt inside one request would push past the server's 60s running timeout — -// costing the user even the fallback list this function exists to provide -// (MUL-5549). -func codebuddyFallbackCatalog() Catalog { - models := codebuddyStaticModels() - applyCodebuddyStaticThinking(models) - return Catalog{Models: models, Fallback: true} -} - -// parseCodebuddyModels extracts model IDs from codebuddy --help output. -// The help text contains a line like: -// -// --model ... Currently supported: (model1, model2, ...) -// -// The first model in the list is marked as default. -func parseCodebuddyModels(helpOutput string) []Model { - match := codebuddyModelRe.FindStringSubmatch(helpOutput) - if len(match) < 2 { - return nil - } - raw := strings.Split(match[1], ",") - var models []Model - for _, s := range raw { - id := strings.TrimSpace(s) - if id == "" { - continue - } - models = append(models, Model{ - ID: id, - Label: codebuddyModelLabel(id), - Provider: codebuddyModelProvider(id), - Default: len(models) == 0, - }) - } - return models -} - -// codebuddyModelProvider infers a provider name from a model ID prefix. +// codebuddyModelProvider infers a vendor from a CodeBuddy model ID prefix. +// CodeBuddy aggregates several vendors under its own account, and neither the +// ACP catalog nor the static fallback carries a vendor field, so the ID prefix +// is the only signal available for grouping the picker. func codebuddyModelProvider(id string) string { switch { case strings.HasPrefix(id, "claude-"): @@ -1819,25 +1802,23 @@ func codebuddyModelProvider(id string) string { } } -// codebuddyModelLabel generates a human-readable label from a model ID. -// Capitalizes each dash-separated part; special-cases GPT/GLM to uppercase -// and rewrites the "-ioa" suffix as "IOA". -func codebuddyModelLabel(id string) string { - parts := strings.Split(id, "-") - for i, p := range parts { - if strings.EqualFold(p, "gpt") || strings.EqualFold(p, "glm") { - parts[i] = strings.ToUpper(p) - } else if strings.EqualFold(p, "ioa") { - parts[i] = "IOA" - } else if len(p) > 0 { - parts[i] = strings.ToUpper(p[:1]) + p[1:] - } - } - return strings.Join(parts, " ") +// codebuddyFallbackCatalog is the static stand-in for a failed discovery. It +// applies the static effort levels locally rather than probing again: whatever +// broke the ACP handshake (binary missing, CLI not logged in, timeout) would +// break a second attempt too. +func codebuddyFallbackCatalog() Catalog { + models := codebuddyStaticModels() + applyCodebuddyStaticThinking(models) + return Catalog{Models: models, Fallback: true} } -// codebuddyStaticModels is the fallback catalog when dynamic discovery -// fails (binary missing, parse error, timeout). +// codebuddyStaticModels is the fallback catalog when ACP discovery fails +// (binary missing, CLI not logged in, handshake timeout). +// +// These IDs do not overlap CodeBuddy's real catalog at all, so this list is a +// last-resort affordance to keep the picker usable, never an answer. It is +// always returned marked Fallback so it cannot be cached as authoritative +// (MUL-5549). func codebuddyStaticModels() []Model { return []Model{ {ID: "claude-sonnet-4.6", Label: "Claude Sonnet 4.6", Provider: "anthropic", Default: true}, diff --git a/server/pkg/agent/thinking.go b/server/pkg/agent/thinking.go index e9f3c52a66..bd1150df4f 100644 --- a/server/pkg/agent/thinking.go +++ b/server/pkg/agent/thinking.go @@ -3,7 +3,6 @@ package agent import ( "context" "encoding/json" - "log/slog" "os/exec" "regexp" "strings" @@ -438,125 +437,27 @@ func codexThinkingFromDebugModel(m codexDebugModel) *ModelThinking { // ── CodeBuddy ──────────────────────────────────────────────────────── // -// CodeBuddy uses the same `--effort ` flag as Claude but with a -// different level set (no `max`). Discovery parses `--help` identically -// to the claude approach. All models get the same effort levels since -// CodeBuddy doesn't document per-model restrictions. - -var codebuddyEffortRe = regexp.MustCompile(`--effort\s*(?:<[^>]+>)?\s*[^(]*\(([^)]+)\)`) +// CodeBuddy uses the same `--effort ` flag as Claude. The level set is +// discovered from the `thought_level` config option in the ACP session/new +// response — the same handshake that yields the model catalog — so no extra +// process is spawned for it. All models share one effort catalog because +// CodeBuddy advertises it per session, not per model. var codebuddyEffortLabel = map[string]string{ - "low": "Low", - "medium": "Medium", - "high": "High", - "xhigh": "Extra high", + "minimal": "Minimal", + "low": "Low", + "medium": "Medium", + "high": "High", + "xhigh": "Extra high", + "max": "Max", } -var codebuddyStaticEffortFallback = []string{"low", "medium", "high", "xhigh"} - -// codebuddyHelpCache memoises the raw --help output across discovery rounds: -// CodeBuddy's --help can take ~30s, so re-running it for every model-list -// request would be painful. -// -// It is NOT what keeps a single request down to one invocation. That is -// structural: discoverCodebuddyModels captures the help text once and hands the -// string to the model and effort parsers, and its failure paths skip the effort -// pass entirely (MUL-5549). Relying on the cache for that would be wrong — a -// failed --help is deliberately not cached, so the second caller would re-run -// the full 35s timeout. -var ( - codebuddyHelpMu sync.Mutex - codebuddyHelpStore = map[string]codebuddyHelpEntry{} -) - -const codebuddyHelpTTL = 60 * time.Second - -type codebuddyHelpEntry struct { - output string - expiresAt time.Time -} - -// codebuddyHelpOutput runs `codebuddy --help` (cached for codebuddyHelpTTL) and -// returns "" when the command could not be run. -// -// discoverCodebuddyModels is its only caller. The effort parser deliberately -// takes an already-captured string instead of calling this itself, so one -// discovery round can never pay the 35s timeout twice (MUL-5549). Keep it that -// way: a new caller here reintroduces that bug on the failure path, where -// nothing is cached to absorb the second run. -func codebuddyHelpOutput(ctx context.Context, executablePath string) string { - if executablePath == "" { - executablePath = "codebuddy" - } - key := executablePath - codebuddyHelpMu.Lock() - if entry, ok := codebuddyHelpStore[key]; ok && time.Now().Before(entry.expiresAt) { - codebuddyHelpMu.Unlock() - return entry.output - } - codebuddyHelpMu.Unlock() - - runCtx, cancel := context.WithTimeout(ctx, 35*time.Second) - defer cancel() - cmd := exec.CommandContext(runCtx, executablePath, "--help") - hideAgentWindow(cmd) - out, err := cmd.CombinedOutput() - if err != nil { - // A non-zero exit or a timeout means this is not usable help text, and - // CombinedOutput folds stderr in, so the failure message itself would - // be returned as if it were help. The canonical case: `codebuddy` is a - // `#!/usr/bin/env node` script installed under nvm, and a GUI-launched - // daemon does not inherit the interactive PATH, so the shebang fails - // and the "output" is `env: node: No such file or directory`. Returning - // that made every parser here silently fall back, and caching it pinned - // the failure for codebuddyHelpTTL (MUL-5549). - slog.Debug("codebuddy --help failed", "path", executablePath, "error", err) - return "" - } - result := string(out) - - if result != "" { - codebuddyHelpMu.Lock() - codebuddyHelpStore[key] = codebuddyHelpEntry{output: result, expiresAt: time.Now().Add(codebuddyHelpTTL)} - codebuddyHelpMu.Unlock() - } - return result -} - -// annotateCodebuddyThinking derives the effort catalog from helpOut — the SAME -// `codebuddy --help` capture the model catalog was parsed from — rather than -// running the command again. -// -// It used to call codebuddyHelpOutput itself. That was free while a failed -// --help was (wrongly) memoised, but once failures correctly stopped being -// cached it meant one ListModels could pay the 35s timeout twice, blowing past -// the server's 60s running timeout and denying the user even the fallback list -// (MUL-5549). Callers on the failure path skip this entirely and use -// codebuddyFallbackCatalog, which applies the static levels without exec'ing. -func annotateCodebuddyThinking(ctx context.Context, models []Model, executablePath, helpOut string) { - if executablePath == "" { - executablePath = "codebuddy" - } - version, _ := DetectVersion(ctx, executablePath) - key := thinkingCacheKey{provider: "codebuddy", executablePath: executablePath, cliVersion: version} - if cached, ok := thinkingCacheGet(key); ok { - for i := range models { - if t, ok := cached[models[i].ID]; ok && t != nil { - models[i].Thinking = t - } - } - return - } - - result := codebuddyThinkingByModel(models, codebuddyEffortSuperset(helpOut)) - thinkingCachePut(key, result) - - for i := range models { - if t, ok := result[models[i].ID]; ok && t != nil { - models[i].Thinking = t - } - } -} +// codebuddyStaticEffortFallback is used when discovery cannot reach the CLI. +// It lists every level `--effort` accepts (confirmed against CodeBuddy 2.130.0, +// which advertises minimal/low/medium/high/xhigh/max) — the previous value +// omitted `minimal` and `max`, so a working install still lost two real levels +// whenever discovery degraded. +var codebuddyStaticEffortFallback = []string{"minimal", "low", "medium", "high", "xhigh", "max"} // codebuddyThinkingByModel maps every model onto the shared effort catalog // built from levels. CodeBuddy advertises one `--effort` set for the whole CLI, @@ -584,9 +485,9 @@ func codebuddyThinkingByModel(models []Model, levels []string) map[string]*Model return result } -// applyCodebuddyStaticThinking annotates models with the static effort -// fallback. Used on the discovery-failure path, where re-running --help to -// discover the real levels would just fail again — slowly. +// applyCodebuddyStaticThinking annotates models with the static effort fallback. +// Used when discovery could not reach the CLI, or reached it but got no +// recognisable thought_level option back. func applyCodebuddyStaticThinking(models []Model) { result := codebuddyThinkingByModel(models, codebuddyStaticEffortFallback) for i := range models { @@ -596,34 +497,101 @@ func applyCodebuddyStaticThinking(models []Model) { } } -// codebuddyEffortSuperset extracts the `--effort` levels from an already -// captured `codebuddy --help`, falling back to the static set when the help -// text carries no parseable effort line. -func codebuddyEffortSuperset(helpOut string) []string { - if helpOut == "" { - return append([]string(nil), codebuddyStaticEffortFallback...) - } - parsed := parseCodebuddyEffortHelp(helpOut) - if len(parsed) == 0 { - return append([]string(nil), codebuddyStaticEffortFallback...) - } - return parsed +// codebuddyFlagEffortValues are the tokens `codebuddy --effort ` accepts. +// +// The ACP `thought_level` option advertises one extra choice, `enabled` +// ("On (default)"), which is a session-level toggle rather than a flag argument. +// The daemon passes the selected level straight through to `--effort` +// (codebuddy.go), so surfacing `enabled` in the picker would let a user build a +// command line CodeBuddy rejects. Filter against this set instead of trusting +// the advertised list wholesale. +var codebuddyFlagEffortValues = map[string]bool{ + "minimal": true, + "low": true, + "medium": true, + "high": true, + "xhigh": true, + "max": true, } -func parseCodebuddyEffortHelp(helpText string) []string { - match := codebuddyEffortRe.FindStringSubmatch(helpText) - if len(match) < 2 { - return nil +// annotateCodebuddyThinkingFromACP fills in each model's effort catalog from the +// `thought_level` config option carried by the SAME `session/new` response the +// models came from — so the effort catalog costs no extra process at all. It +// replaces a second regex pass over `codebuddy --help` (MUL-5549). +// +// CodeBuddy advertises one effort set for the whole CLI rather than per model, +// so every entry shares it. Levels the `--effort` flag would reject are dropped, +// and a currentValue outside the flag set (the default `enabled`) becomes an +// empty DefaultLevel, which the UI renders as a generic "Default" instead of +// inventing a level we cannot pass through. +func annotateCodebuddyThinkingFromACP(models []Model, sessionResult json.RawMessage) { + levels, defaultLevel := parseACPCodebuddyEffort(sessionResult) + if len(levels) == 0 { + applyCodebuddyStaticThinking(models) + return } - var out []string - for _, raw := range strings.Split(match[1], ",") { - token := strings.TrimSpace(raw) - if token == "" { + result := codebuddyThinkingByModel(models, levels) + for _, thinking := range result { + thinking.DefaultLevel = defaultLevel + } + for i := range models { + if t, ok := result[models[i].ID]; ok && t != nil { + models[i].Thinking = t + } + } +} + +// parseACPCodebuddyEffort extracts the effort levels and the advertised default +// from an ACP session/new result. Returns no levels when the response carries no +// recognisable thought_level option, which makes the caller fall back to the +// static set rather than hiding the thinking picker entirely. +func parseACPCodebuddyEffort(raw json.RawMessage) (levels []string, defaultLevel string) { + type acpChoice struct { + Value string `json:"value"` + } + type acpOption struct { + ID string `json:"id"` + Category string `json:"category"` + CurrentValue string `json:"currentValue"` + CurrentValueSnake string `json:"current_value"` + Options []acpChoice `json:"options"` + } + var resp struct { + ConfigOptions []acpOption `json:"configOptions"` + ConfigOptionsSnake []acpOption `json:"config_options"` + } + if err := json.Unmarshal(raw, &resp); err != nil { + return nil, "" + } + options := resp.ConfigOptions + if len(options) == 0 { + options = resp.ConfigOptionsSnake + } + for _, opt := range options { + if !strings.EqualFold(strings.TrimSpace(opt.ID), "thought_level") && + !strings.EqualFold(strings.TrimSpace(opt.Category), "thought_level") { continue } - out = append(out, token) + seen := map[string]bool{} + for _, choice := range opt.Options { + value := strings.TrimSpace(choice.Value) + if value == "" || seen[value] || !codebuddyFlagEffortValues[value] { + continue + } + seen[value] = true + levels = append(levels, value) + } + current := strings.TrimSpace(opt.CurrentValue) + if current == "" { + current = strings.TrimSpace(opt.CurrentValueSnake) + } + // Only echo a default we could actually pass to --effort. + if codebuddyFlagEffortValues[current] { + defaultLevel = current + } + return levels, defaultLevel } - return out + return nil, "" } // ── Shared validation ──────────────────────────────────────────────── @@ -774,11 +742,16 @@ var providerThinkingEnums = map[string]map[string]bool{ "xhigh": true, "max": true, }, + // Confirmed against CodeBuddy 2.130.0's advertised thought_level catalog. + // `minimal` and `max` were missing here, so the server rejected two levels + // the CLI genuinely accepts. "codebuddy": { - "low": true, - "medium": true, - "high": true, - "xhigh": true, + "minimal": true, + "low": true, + "medium": true, + "high": true, + "xhigh": true, + "max": true, }, // Grok 4.5's documented --effort levels. It cannot disable reasoning and // does not accept none, minimal, or xhigh.