From 6e5d88d4c696dc65902ce95d6bf201ece504aa26 Mon Sep 17 00:00:00 2001 From: francisco-orkes Date: Thu, 13 Aug 2026 12:34:13 -0700 Subject: [PATCH 1/2] fix: report the server's AI providers and drop doctor's model catalogue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit doctor's AI Providers section read only the local shell's environment. Agents execute on the server, which resolves provider credentials from its own configuration, so a shell without keys reported zero providers configured against a server that had several — while printing the server URL directly above the list. doctor now asks the server for provider status and reports that alongside the local environment check, which still governs the deploy and worker paths. A host-managed deployment and a server without the endpoint are both stated plainly and neither counts as a warning; the lookup is bounded so an unreachable server is a finding rather than a hang. The per-provider model lists are gone with it. The endpoint reports which providers are configured but carries no model names, and a hardcoded list cannot stay true: two of the Anthropic ids doctor advertised had already been withdrawn, and copying one produced a failed execution on the server rather than a validation error. Refs #117 Refs #103 Co-Authored-By: Claude Opus 5 (1M context) --- cmd/doctor.go | 189 +++++++++++++++++++++------ cmd/doctor_test.go | 113 +++++++++++++++- internal/providers/providers.go | 84 ++++++++++++ internal/providers/providers_test.go | 120 +++++++++++++++++ test/e2e/doctor.bats | 9 +- 5 files changed, 469 insertions(+), 46 deletions(-) create mode 100644 internal/providers/providers.go create mode 100644 internal/providers/providers_test.go diff --git a/cmd/doctor.go b/cmd/doctor.go index c7e3677..ebbc73c 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -14,22 +14,32 @@ package cmd import ( + "context" + "errors" "fmt" + "io" "os" "os/exec" "strings" + "time" "github.com/spf13/cobra" "github.com/conductor-oss/conductor-cli/internal" + "github.com/conductor-oss/conductor-cli/internal/providers" ) // aiProvider describes an LLM provider and the env vars that configure it. +// +// It deliberately names no models. doctor can know which providers are configured; +// which model ids a provider currently serves is not something a CLI release can keep +// true, and stale ids fail at execution time on the server rather than at validation +// (see #103). type aiProvider struct { - name string - envVars []string // all must be set for the provider to be "configured" - warns []providerWarning // conditional warnings, checked when the provider is opted into - models []string // example model ids + name string + serverName string // identifier the server reports for this provider + envVars []string // all must be set for the provider to be "configured" + warns []providerWarning // conditional warnings, checked when the provider is opted into } type providerWarning struct { @@ -41,11 +51,12 @@ type providerWarning struct { // aiProviders is the data-driven registry doctor reports on. Adding a provider is a // data change, not new control flow. var aiProviders = []aiProvider{ - {name: "OpenAI", envVars: []string{"OPENAI_API_KEY"}, models: []string{"openai/gpt-4o", "openai/gpt-4o-mini"}}, - {name: "Anthropic", envVars: []string{"ANTHROPIC_API_KEY"}, models: []string{"anthropic/claude-sonnet-4-20250514", "anthropic/claude-3-5-sonnet-20241022"}}, + {name: "OpenAI", serverName: "openai", envVars: []string{"OPENAI_API_KEY"}}, + {name: "Anthropic", serverName: "anthropic", envVars: []string{"ANTHROPIC_API_KEY"}}, { - name: "Google Gemini", - envVars: []string{"GEMINI_API_KEY", "GOOGLE_CLOUD_PROJECT"}, + name: "Google Gemini", + serverName: "gemini", + envVars: []string{"GEMINI_API_KEY", "GOOGLE_CLOUD_PROJECT"}, warns: []providerWarning{{ condition: func() bool { return os.Getenv("GEMINI_API_KEY") != "" && os.Getenv("GOOGLE_CLOUD_PROJECT") == "" @@ -53,11 +64,11 @@ var aiProviders = []aiProvider{ message: "GEMINI_API_KEY is set but GOOGLE_CLOUD_PROJECT is missing", fix: "export GOOGLE_CLOUD_PROJECT=your-gcp-project-id", }}, - models: []string{"google_gemini/gemini-2.0-flash", "google_gemini/gemini-1.5-pro"}, }, { - name: "Azure OpenAI", - envVars: []string{"AZURE_OPENAI_API_KEY", "AZURE_OPENAI_ENDPOINT"}, + name: "Azure OpenAI", + serverName: "azureopenai", + envVars: []string{"AZURE_OPENAI_API_KEY", "AZURE_OPENAI_ENDPOINT"}, warns: []providerWarning{{ condition: func() bool { return os.Getenv("AZURE_OPENAI_API_KEY") != "" && os.Getenv("AZURE_OPENAI_DEPLOYMENT") == "" @@ -65,11 +76,11 @@ var aiProviders = []aiProvider{ message: "AZURE_OPENAI_DEPLOYMENT is not set (required to route requests)", fix: "export AZURE_OPENAI_DEPLOYMENT=your-deployment-name", }}, - models: []string{"azure_openai/gpt-4o"}, }, { - name: "AWS Bedrock", - envVars: []string{"AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY"}, + name: "AWS Bedrock", + serverName: "aws_bedrock", + envVars: []string{"AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY"}, warns: []providerWarning{{ condition: func() bool { return os.Getenv("AWS_ACCESS_KEY_ID") != "" && os.Getenv("AWS_DEFAULT_REGION") == "" && os.Getenv("AWS_REGION") == "" @@ -77,14 +88,33 @@ var aiProviders = []aiProvider{ message: "No AWS region set — defaults to us-east-1", fix: "export AWS_DEFAULT_REGION=us-east-1", }}, - models: []string{"aws_bedrock/anthropic.claude-3-5-sonnet-20241022-v2:0"}, }, - {name: "Mistral", envVars: []string{"MISTRAL_API_KEY"}, models: []string{"mistral/mistral-large-latest"}}, - {name: "Cohere", envVars: []string{"COHERE_API_KEY"}, models: []string{"cohere/command-r-plus"}}, - {name: "Grok", envVars: []string{"XAI_API_KEY"}, models: []string{"grok/grok-3"}}, - {name: "Perplexity", envVars: []string{"PERPLEXITY_API_KEY"}, models: []string{"perplexity/sonar-pro"}}, - {name: "Hugging Face", envVars: []string{"HUGGINGFACE_API_KEY"}, models: []string{"hugging_face/meta-llama/Llama-3-70b-chat-hf"}}, - {name: "Stability AI", envVars: []string{"STABILITY_API_KEY"}, models: []string{"stabilityai/sd3.5-large"}}, + {name: "Mistral", serverName: "mistral", envVars: []string{"MISTRAL_API_KEY"}}, + {name: "Cohere", serverName: "cohere", envVars: []string{"COHERE_API_KEY"}}, + {name: "Grok", serverName: "grok", envVars: []string{"XAI_API_KEY"}}, + {name: "Perplexity", serverName: "perplexity", envVars: []string{"PERPLEXITY_API_KEY"}}, + {name: "Hugging Face", serverName: "huggingface", envVars: []string{"HUGGINGFACE_API_KEY"}}, + {name: "Stability AI", serverName: "stabilityai", envVars: []string{"STABILITY_API_KEY"}}, +} + +// serverOnlyProviders are reported by the server but configured without env vars, so +// they have no entry in the local registry. Only their display name is needed. +var serverOnlyProviders = map[string]string{"ollama": "Ollama"} + +// displayProviderName maps a server provider identifier to the CLI's display name, so +// one provider reads as one provider rather than two spellings of it. Unknown names +// pass through unchanged — a provider the server gained since this release should be +// reported, not hidden. +func displayProviderName(serverName string) string { + for _, p := range aiProviders { + if p.serverName == serverName { + return p.name + } + } + if name, ok := serverOnlyProviders[serverName]; ok { + return name + } + return serverName } var doctorCmd = &cobra.Command{ @@ -119,36 +149,111 @@ func runDoctor(cmd *cobra.Command, args []string) error { fmt.Println(" -- No authentication configured (anonymous; OSS only)") } - fmt.Println("\nAI Providers") - configured := 0 + // Agents run on the server, so ask it what it can dial before reporting what this + // shell happens to hold. The transport imposes no timeout of its own; a + // diagnostic must not hang on an unreachable server. + ctx, cancel := context.WithTimeout(cmd.Context(), providerStatusTimeout) + defer cancel() + st, err := providers.Fetch(ctx, t) + printServerProviders(os.Stdout, st, err) + + configured, warnings := printLocalProviders(os.Stdout) + issues += warnings + + fmt.Printf("\n%d AI provider(s) configured in this environment", configured) + if issues > 0 { + fmt.Printf(", %d warning(s)", issues) + } + fmt.Println(".") + return nil +} + +// providerStatusTimeout bounds the provider-status lookup. doctor reports on a server +// that may be unreachable, which is itself a finding rather than a reason to block. +// +// The bound is generous because the endpoint is measurably slow: a local OSS server +// took 7-8s to answer, and not because of the ollama reachability probe it advertises +// (a refused connect returns immediately). A timeout tight enough to feel snappy would +// report "no providers" on a server that has them, which is the very confusion this +// section exists to remove. +const providerStatusTimeout = 20 * time.Second + +// printServerProviders reports the providers the server has configured. Neither a +// host-managed deployment nor a server without the endpoint is a fault in the user's +// setup, so both are stated plainly and neither counts as a warning. +func printServerProviders(w io.Writer, st providers.Status, err error) { + fmt.Fprintln(w, "\nAI Providers (server)") + + switch { + case errors.Is(err, providers.ErrUnsupported): + fmt.Fprintln(w, " -- This server does not report provider status") + fmt.Fprintln(w, " (AI integrations are disabled, or the server predates the endpoint)") + return + case err != nil: + fmt.Fprintf(w, " -- Could not read provider status: %v\n", err) + return + case st.ManagedByHost: + fmt.Fprintln(w, " -- Provider configuration is owned by the host deployment") + fmt.Fprintln(w, " (per-provider detail is not reported by this server)") + return + case len(st.Providers) == 0: + fmt.Fprintln(w, " -- The server reported no providers") + return + } + + for _, p := range st.Providers { + mark := "--" + if p.Configured { + mark = "ok" + } + fmt.Fprintf(w, " %s %s%s\n", mark, displayProviderName(p.Name), serverProviderDetail(p)) + } +} + +// serverProviderDetail renders the extras the server reports for URL-based providers. +// Reachability is probed from the server's own network, which is precisely the fact a +// client cannot determine for itself. +func serverProviderDetail(p providers.Provider) string { + if p.BaseURL == "" && p.Reachable == nil { + return "" + } + parts := []string{} + if p.BaseURL != "" { + parts = append(parts, p.BaseURL) + } + if p.Reachable != nil { + if *p.Reachable { + parts = append(parts, "reachable") + } else { + parts = append(parts, "unreachable from the server") + } + } + return " (" + strings.Join(parts, ", ") + ")" +} + +// printLocalProviders reports what this shell has configured, which still governs the +// deploy and worker paths. It returns the configured count and the number of warnings +// raised. +func printLocalProviders(w io.Writer) (configured, warnings int) { + fmt.Fprintln(w, "\nAI Providers (local environment)") for _, p := range aiProviders { - opted := providerOptedIn(p) if isProviderConfigured(p) { configured++ - fmt.Printf(" ok %s (%s)\n", p.name, strings.Join(p.envVars, ", ")) - for _, m := range p.models { - fmt.Printf(" %s\n", m) - } + fmt.Fprintf(w, " ok %s (%s)\n", p.name, strings.Join(p.envVars, ", ")) } else { - fmt.Printf(" -- %s (%s)\n", p.name, strings.Join(p.envVars, ", ")) + fmt.Fprintf(w, " -- %s (%s)\n", p.name, strings.Join(p.envVars, ", ")) } - if opted { - for _, w := range p.warns { - if w.condition() { - fmt.Printf(" ! %s\n", w.message) - fmt.Printf(" %s\n", w.fix) - issues++ + if providerOptedIn(p) { + for _, warn := range p.warns { + if warn.condition() { + fmt.Fprintf(w, " ! %s\n", warn.message) + fmt.Fprintf(w, " %s\n", warn.fix) + warnings++ } } } } - - fmt.Printf("\n%d AI provider(s) configured", configured) - if issues > 0 { - fmt.Printf(", %d warning(s)", issues) - } - fmt.Println(".") - return nil + return configured, warnings } // isProviderConfigured reports whether every required env var for a provider is set. diff --git a/cmd/doctor_test.go b/cmd/doctor_test.go index a4575b2..ce9f1f8 100644 --- a/cmd/doctor_test.go +++ b/cmd/doctor_test.go @@ -13,7 +13,118 @@ package cmd -import "testing" +import ( + "errors" + "strings" + "testing" + + "github.com/conductor-oss/conductor-cli/internal/providers" +) + +func boolPtr(b bool) *bool { return &b } + +func TestServerProvidersReportConfiguredState(t *testing.T) { + var sb strings.Builder + printServerProviders(&sb, providers.Status{Providers: []providers.Provider{ + {Name: "openai", Configured: true}, + {Name: "anthropic", Configured: false}, + }}, nil) + out := sb.String() + + if !strings.Contains(out, "OpenAI") || !strings.Contains(out, "Anthropic") { + t.Errorf("both providers should be listed:\n%s", out) + } + // The whole point of the section: the reader must not mistake it for the local + // environment check that follows. + if !strings.Contains(strings.ToLower(out), "server") { + t.Errorf("section should attribute the report to the server:\n%s", out) + } +} + +func TestServerProvidersShowReachabilityWhenProbed(t *testing.T) { + var sb strings.Builder + printServerProviders(&sb, providers.Status{Providers: []providers.Provider{ + {Name: "ollama", Configured: true, BaseURL: "http://localhost:11434", Reachable: boolPtr(false)}, + }}, nil) + out := sb.String() + + if !strings.Contains(out, "http://localhost:11434") { + t.Errorf("base url should be shown:\n%s", out) + } + if !strings.Contains(strings.ToLower(out), "unreachable") { + t.Errorf("a failed probe should be called out:\n%s", out) + } +} + +func TestServerProvidersReportHostManagedConfiguration(t *testing.T) { + var sb strings.Builder + printServerProviders(&sb, providers.Status{ManagedByHost: true}, nil) + out := strings.ToLower(sb.String()) + + if !strings.Contains(out, "host") { + t.Errorf("host-managed configuration should be named as such:\n%s", out) + } + if strings.Contains(out, "error") || strings.Contains(out, "failed") { + t.Errorf("host-managed configuration is not a failure:\n%s", out) + } +} + +// A server with AI integrations disabled is a normal deployment, not a broken one. +func TestServerProvidersReportMissingEndpointPlainly(t *testing.T) { + var sb strings.Builder + printServerProviders(&sb, providers.Status{}, providers.ErrUnsupported) + out := sb.String() + + if strings.TrimSpace(out) == "" { + t.Fatal("an unsupported endpoint should still say something") + } + if strings.Contains(strings.ToLower(out), "error") { + t.Errorf("should read as information, not an error:\n%s", out) + } +} + +func TestServerProvidersReportUnreachableServer(t *testing.T) { + var sb strings.Builder + printServerProviders(&sb, providers.Status{}, errors.New("dial tcp: connection refused")) + + if strings.TrimSpace(sb.String()) == "" { + t.Fatal("a failed lookup should still say something") + } +} + +// Server and CLI spell the same provider differently. One provider, one name. +func TestServerProviderNamesMapToDisplayNames(t *testing.T) { + for serverName, want := range map[string]string{ + "gemini": "Google Gemini", + "azureopenai": "Azure OpenAI", + "huggingface": "Hugging Face", + "aws_bedrock": "AWS Bedrock", + "openai": "OpenAI", + "ollama": "Ollama", // server-only: not in the local registry at all + } { + if got := displayProviderName(serverName); got != want { + t.Errorf("displayProviderName(%q) = %q, want %q", serverName, got, want) + } + } +} + +// Regression guard for #103: doctor reported model ids it could not keep current, and +// two of them had already been withdrawn by the provider. Model identifiers carry a +// "provider/model" shape; nothing else in this section contains a slash. +func TestLocalProvidersAdvertiseNoModelIdentifiers(t *testing.T) { + var sb strings.Builder + printLocalProviders(&sb) + out := sb.String() + + for _, line := range strings.Split(out, "\n") { + if strings.Contains(line, "/") { + t.Errorf("doctor should name no model identifiers, got: %q", line) + } + } + if !strings.Contains(out, "Anthropic") { + t.Errorf("providers should still be listed:\n%s", out) + } +} func TestProviderConfiguredRequiresAllEnvVars(t *testing.T) { p := aiProvider{name: "Test", envVars: []string{"DOC_TEST_KEY", "DOC_TEST_ENDPOINT"}} diff --git a/internal/providers/providers.go b/internal/providers/providers.go new file mode 100644 index 0000000..55cae40 --- /dev/null +++ b/internal/providers/providers.go @@ -0,0 +1,84 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +// Package providers reads the server's view of which AI providers it can dial. +// +// Agents execute on the server, which resolves provider credentials from its own +// environment, credential store, or properties. A client shell's OPENAI_API_KEY says +// nothing about that, so the CLI asks rather than guesses. +// +// The transport applies no timeout of its own (request lifetime belongs to the +// caller's context), so callers bound this themselves — a diagnostic command must not +// hang on an unreachable server. +package providers + +import ( + "context" + "errors" + "fmt" + "net/http" + + "github.com/conductor-oss/conductor-cli/internal/transport" +) + +const statusPath = "/providers/status" + +// ErrUnsupported means the server has no provider-status route: AI integrations are +// disabled, or the deployment predates the endpoint. It describes the deployment, not +// a failure, and callers are expected to report it and carry on. +var ErrUnsupported = errors.New("server does not expose provider status") + +// Status is the server's provider configuration. +type Status struct { + // ManagedByHost is true when the server is embedded in a host that owns provider + // configuration. Providers is then empty by design rather than by omission; the + // wire contract is forward-compatible, so per-provider detail may appear later. + ManagedByHost bool `json:"managedByHost"` + Providers []Provider `json:"providers"` +} + +// Provider is one provider's status as the server sees it. +type Provider struct { + Name string `json:"name"` + Configured bool `json:"configured"` + // BaseURL is reported only for URL-based providers (ollama). + BaseURL string `json:"baseUrl,omitempty"` + // Reachable is probed from the server's own network — a fact no client can + // observe for itself. It is a pointer because "not probed" and "probed, and it + // failed" are different answers. + Reachable *bool `json:"reachable,omitempty"` +} + +// Fetch reads provider status from the server. A missing route yields ErrUnsupported. +func Fetch(ctx context.Context, t transport.Config) (Status, error) { + var st Status + if err := t.DoJSON(ctx, http.MethodGet, statusPath, nil, &st); err != nil { + var apiErr *transport.APIError + if errors.As(err, &apiErr) && apiErr.Status == http.StatusNotFound { + return Status{}, ErrUnsupported + } + return Status{}, fmt.Errorf("read provider status: %w", err) + } + return st, nil +} + +// Configured returns the names of the providers the server reports as configured. +func (s Status) Configured() []string { + var names []string + for _, p := range s.Providers { + if p.Configured { + names = append(names, p.Name) + } + } + return names +} diff --git a/internal/providers/providers_test.go b/internal/providers/providers_test.go new file mode 100644 index 0000000..f3ab172 --- /dev/null +++ b/internal/providers/providers_test.go @@ -0,0 +1,120 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package providers + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/conductor-oss/conductor-cli/internal/transport" +) + +// stub serves one canned response and records the paths it was asked for. +func stub(t *testing.T, status int, body string) (transport.Config, *[]string) { + t.Helper() + var paths []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + paths = append(paths, r.URL.Path) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + return transport.Config{BaseURL: srv.URL + "/api"}, &paths +} + +func TestFetchReadsPerProviderStatus(t *testing.T) { + cfg, paths := stub(t, http.StatusOK, `{"managedByHost":false,"providers":[ + {"name":"openai","configured":true}, + {"name":"anthropic","configured":false}, + {"name":"ollama","configured":true,"baseUrl":"http://localhost:11434","reachable":false}]}`) + + st, err := Fetch(context.Background(), cfg) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if st.ManagedByHost { + t.Error("ManagedByHost should be false") + } + if len(st.Providers) != 3 { + t.Fatalf("got %d providers, want 3", len(st.Providers)) + } + if !st.Providers[0].Configured || st.Providers[0].Name != "openai" { + t.Errorf("first provider = %+v", st.Providers[0]) + } + if st.Providers[1].Configured { + t.Error("anthropic should be unconfigured") + } + + ollama := st.Providers[2] + if ollama.BaseURL != "http://localhost:11434" { + t.Errorf("ollama BaseURL = %q", ollama.BaseURL) + } + if ollama.Reachable == nil || *ollama.Reachable { + t.Errorf("ollama Reachable = %v, want a non-nil false", ollama.Reachable) + } + + // Reachability is only reported for providers the server probes; the absence of + // the field must stay distinguishable from a probe that failed. + if st.Providers[0].Reachable != nil { + t.Error("openai should carry no reachability") + } + + if len(*paths) != 1 { + t.Fatalf("made %d requests, want exactly 1: %v", len(*paths), *paths) + } + if (*paths)[0] != "/api/providers/status" { + t.Errorf("requested %q", (*paths)[0]) + } +} + +func TestFetchReportsHostManagedConfiguration(t *testing.T) { + cfg, _ := stub(t, http.StatusOK, `{"managedByHost":true,"providers":[]}`) + + st, err := Fetch(context.Background(), cfg) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if !st.ManagedByHost { + t.Error("ManagedByHost should be true") + } + if len(st.Providers) != 0 { + t.Errorf("host-managed response should carry no per-provider detail, got %d", len(st.Providers)) + } +} + +// A server with AI integrations disabled has no such route. That is a fact about the +// deployment, not a failure, so callers need to tell it apart from a real error. +func TestFetchTreatsMissingEndpointAsUnsupported(t *testing.T) { + cfg, _ := stub(t, http.StatusNotFound, `{"status":404,"message":"Not Found"}`) + + if _, err := Fetch(context.Background(), cfg); !errors.Is(err, ErrUnsupported) { + t.Fatalf("err = %v, want ErrUnsupported", err) + } +} + +func TestFetchSurfacesOtherErrors(t *testing.T) { + cfg, _ := stub(t, http.StatusInternalServerError, `{"status":500,"message":"boom"}`) + + _, err := Fetch(context.Background(), cfg) + if err == nil { + t.Fatal("expected an error") + } + if errors.Is(err, ErrUnsupported) { + t.Error("a 500 is a real failure, not an unsupported endpoint") + } +} diff --git a/test/e2e/doctor.bats b/test/e2e/doctor.bats index 96853e3..306e23f 100644 --- a/test/e2e/doctor.bats +++ b/test/e2e/doctor.bats @@ -64,14 +64,17 @@ setup_file() { [[ "$output" == *"OPENAI_API_KEY"* ]] } -# Regression guard for #103: doctor advertises specific model strings that have +# Regression guard for #103: doctor advertised specific model strings that had # drifted out of date (two Anthropic models return 404 from the provider API). -# Asserts the desired end state — that doctor does not print known-dead models. +# doctor now names no models at all, so the two dead ids are covered by the general +# assertion as well as by name. @test "7. Doctor does not advertise retired model identifiers" { - skip "known broken: #103 — doctor hardcodes stale model strings" run bash -c "./conductor doctor 2>&1" echo "Output: $output" [ "$status" -eq 0 ] [[ "$output" != *"claude-sonnet-4-20250514"* ]] [[ "$output" != *"claude-3-5-sonnet-20241022"* ]] + # No model identifier of any provider/model shape, dead or alive. + run bash -c "./conductor doctor 2>&1 | grep -E '^ +[a-z_]+/[A-Za-z0-9._-]+'" + [ "$status" -ne 0 ] } From 0fe6a115b87236e300d851805d6027c17e8a0029 Mon Sep 17 00:00:00 2001 From: francisco-orkes Date: Thu, 13 Aug 2026 12:39:32 -0700 Subject: [PATCH 2/2] feat: require an explicit --model on agent init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit agent init wrote a fixed OpenAI model into every generated config regardless of what was configured, then printed a run command that could not succeed without an OpenAI key. The failure landed server-side at execution time as a FAILED workflow rather than a validation error, which is an expensive way to learn the model was never usable. There is no better default to pick. The server reports which providers it can dial but carries no model names, so any built-in choice is a guess a CLI release cannot keep true — the same drift that left doctor advertising two withdrawn Anthropic ids. init now requires --model, and when it is missing it names the providers the server reports as configured. That lookup is a courtesy, not a precondition: it is bounded, and init still scaffolds a config with no server reachable at all. Provisional pending team discussion of the first-run experience, and kept as a single commit so it can be reverted on its own. Refs #103 Co-Authored-By: Claude Opus 5 (1M context) --- cmd/agent.go | 52 +++++++++++++++-- cmd/agent_init_model_test.go | 107 +++++++++++++++++++++++++++++++++++ test/e2e/agent.bats | 22 +++++-- 3 files changed, 170 insertions(+), 11 deletions(-) create mode 100644 cmd/agent_init_model_test.go diff --git a/cmd/agent.go b/cmd/agent.go index 6993c40..1136f38 100644 --- a/cmd/agent.go +++ b/cmd/agent.go @@ -15,6 +15,7 @@ package cmd import ( "bytes" + "context" "encoding/json" "fmt" "os" @@ -29,6 +30,7 @@ import ( "github.com/conductor-oss/conductor-cli/internal" "github.com/conductor-oss/conductor-cli/internal/agent" + "github.com/conductor-oss/conductor-cli/internal/providers" ) // Defaults that are policy, not server contract — named so they are not magic @@ -36,10 +38,14 @@ import ( const ( defaultExecutionSearchSize = 50 defaultPruneOlderThanDays = 30 - defaultInitModel = "openai/gpt-4o" defaultInitMaxTurns = 25 ) +// initProviderLookupTimeout bounds the provider lookup on init's error path. Naming +// the providers that actually work is the point of the error, so it is worth a short +// wait — but never an unbounded one, and init still works with no server at all. +const initProviderLookupTimeout = 10 * time.Second + var agentCmd = &cobra.Command{ Use: "agent", Aliases: []string{"a"}, @@ -318,9 +324,9 @@ var agentInitCmd = &cobra.Command{ SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { name := args[0] - model := initModel - if model == "" { - model = defaultInitModel + model, err := resolveInitModel(cmd.Context(), initModel) + if err != nil { + return err } cfg := map[string]any{ "name": name, @@ -335,7 +341,6 @@ var agentInitCmd = &cobra.Command{ } var data []byte - var err error ext := "yaml" if initFormat == "json" { ext = "json" @@ -357,6 +362,41 @@ var agentInitCmd = &cobra.Command{ }, } +// resolveInitModel returns the model to write into a generated config, or an error +// naming what the user can pick from. +// +// init used to default to a fixed OpenAI model whatever was configured, so the run +// command it printed on the next line could not succeed for anyone without an OpenAI +// key — and the failure landed server-side at execution time, as a FAILED workflow +// rather than a validation error. There is no better default available: the server +// reports which providers it can dial but no model names, so any built-in choice is a +// guess this release cannot keep true (see #103). +func resolveInitModel(ctx context.Context, flagValue string) (string, error) { + if flagValue != "" { + return flagValue, nil + } + + const base = "no model specified: pass --model provider/model" + + lookup, cancel := context.WithTimeout(ctx, initProviderLookupTimeout) + defer cancel() + + // The provider list is a courtesy on an error path. init scaffolds a local file + // and must keep working with no server reachable at all. + st, err := providers.Fetch(lookup, internal.Transport()) + if err != nil { + return "", fmt.Errorf("%s (run 'conductor doctor' to see which providers are configured)", base) + } + configured := st.Configured() + if len(configured) == 0 { + if st.ManagedByHost { + return "", fmt.Errorf("%s (provider configuration is owned by the host deployment)", base) + } + return "", fmt.Errorf("%s (the server reports no configured AI providers)", base) + } + return "", fmt.Errorf("%s — providers configured on the server: %s", base, strings.Join(configured, ", ")) +} + // ---- helpers (file I/O and formatting live here, in the cmd layer) ---- // loadAgentConfig reads a YAML or JSON agent config from disk and returns it as @@ -481,7 +521,7 @@ func init() { agentPruneCmd.Flags().BoolVar(&pruneArchive, "archive", false, "Archive tasks instead of hard-deleting") agentPruneCmd.Flags().BoolVar(&pruneDryRun, "dry-run", false, "Show what would be pruned without deleting") - agentInitCmd.Flags().StringVarP(&initModel, "model", "", "", "LLM model (default: "+defaultInitModel+")") + agentInitCmd.Flags().StringVarP(&initModel, "model", "", "", "LLM model as provider/model (required)") agentInitCmd.Flags().StringVarP(&initStrategy, "strategy", "s", "", "Multi-agent strategy (handoff, sequential, parallel, ...)") agentInitCmd.Flags().StringVarP(&initFormat, "format", "f", "yaml", "Output format: yaml or json") diff --git a/cmd/agent_init_model_test.go b/cmd/agent_init_model_test.go new file mode 100644 index 0000000..c66b928 --- /dev/null +++ b/cmd/agent_init_model_test.go @@ -0,0 +1,107 @@ +/* + * Copyright 2026 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package cmd + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/conductor-oss/conductor-cli/internal" + "github.com/conductor-oss/conductor-cli/internal/transport" +) + +// pointTransportAt swaps the shared transport for the duration of a test. +func pointTransportAt(t *testing.T, status int, body string) { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + })) + previous := internal.Transport() + internal.SetTransport(transport.Config{BaseURL: srv.URL + "/api"}) + t.Cleanup(func() { + srv.Close() + internal.SetTransport(previous) + }) +} + +func TestResolveInitModelReturnsAnExplicitModelVerbatim(t *testing.T) { + // Pointed at a server that would fail the lookup: an explicit model must not + // depend on the server being reachable at all. + pointTransportAt(t, http.StatusInternalServerError, `{"message":"boom"}`) + + got, err := resolveInitModel(context.Background(), "anthropic/some-model") + if err != nil { + t.Fatalf("resolveInitModel: %v", err) + } + if got != "anthropic/some-model" { + t.Errorf("model = %q, want it passed through unchanged", got) + } +} + +func TestResolveInitModelNamesConfiguredProviders(t *testing.T) { + pointTransportAt(t, http.StatusOK, `{"managedByHost":false,"providers":[ + {"name":"openai","configured":false}, + {"name":"anthropic","configured":true}, + {"name":"perplexity","configured":true}]}`) + + _, err := resolveInitModel(context.Background(), "") + if err == nil { + t.Fatal("expected an error when no model is given") + } + msg := err.Error() + if !strings.Contains(msg, "--model") { + t.Errorf("error should name the flag to pass: %q", msg) + } + if !strings.Contains(msg, "anthropic") || !strings.Contains(msg, "perplexity") { + t.Errorf("error should name the configured providers: %q", msg) + } + if strings.Contains(msg, "openai") { + t.Errorf("error should not offer an unconfigured provider: %q", msg) + } +} + +// init scaffolds a local file. It must stay usable with no server reachable, so the +// provider list is a courtesy rather than a precondition. +func TestResolveInitModelStillFailsUsefullyWithoutAServer(t *testing.T) { + pointTransportAt(t, http.StatusNotFound, `{"status":404,"message":"Not Found"}`) + + _, err := resolveInitModel(context.Background(), "") + if err == nil { + t.Fatal("expected an error when no model is given") + } + msg := err.Error() + if !strings.Contains(msg, "--model") { + t.Errorf("error should name the flag to pass: %q", msg) + } + if !strings.Contains(msg, "doctor") { + t.Errorf("error should point at a way to find the answer: %q", msg) + } +} + +func TestResolveInitModelReportsHostManagedConfiguration(t *testing.T) { + pointTransportAt(t, http.StatusOK, `{"managedByHost":true,"providers":[]}`) + + _, err := resolveInitModel(context.Background(), "") + if err == nil { + t.Fatal("expected an error when no model is given") + } + if !strings.Contains(err.Error(), "host") { + t.Errorf("error should explain why no providers are listed: %q", err.Error()) + } +} diff --git a/test/e2e/agent.bats b/test/e2e/agent.bats index bd33457..f83b63d 100644 --- a/test/e2e/agent.bats +++ b/test/e2e/agent.bats @@ -14,8 +14,9 @@ AGENT_NAME="e2e_agent_probe" -# A model verified to exist; the `agent init` default (openai/gpt-4o) is not -# usable unless an OpenAI key happens to be configured — see #103. +# A model verified to exist. `agent init` no longer supplies a default at all — the +# server reports which providers it can dial but no model names, so every config has +# to name one explicitly (see #103). LLM_MODEL="anthropic/claude-haiku-4-5-20251001" setup_file() { @@ -84,7 +85,7 @@ run_bounded() { # bats test_tags=tier:pr @test "1. Agent init creates a YAML config" { - run bash -c "cd '$BATS_TEST_TMPDIR' && '$PWD/conductor' agent init inittest 2>&1" + run bash -c "cd '$BATS_TEST_TMPDIR' && '$PWD/conductor' agent init inittest --model $LLM_MODEL 2>&1" echo "Output: $output" [ "$status" -eq 0 ] [ -f "$BATS_TEST_TMPDIR/inittest.yaml" ] @@ -92,7 +93,7 @@ run_bounded() { # bats test_tags=tier:pr @test "2. Agent init --format json creates a JSON config" { - run bash -c "cd '$BATS_TEST_TMPDIR' && '$PWD/conductor' agent init jsontest --format json 2>&1" + run bash -c "cd '$BATS_TEST_TMPDIR' && '$PWD/conductor' agent init jsontest --format json --model $LLM_MODEL 2>&1" echo "Output: $output" [ "$status" -eq 0 ] [ -f "$BATS_TEST_TMPDIR/jsontest.json" ] @@ -100,12 +101,23 @@ run_bounded() { # bats test_tags=tier:pr @test "3. Agent init --strategy records the strategy" { - run bash -c "cd '$BATS_TEST_TMPDIR' && '$PWD/conductor' agent init strattest --strategy handoff 2>&1" + run bash -c "cd '$BATS_TEST_TMPDIR' && '$PWD/conductor' agent init strattest --strategy handoff --model $LLM_MODEL 2>&1" echo "Output: $output" [ "$status" -eq 0 ] grep -q 'handoff' "$BATS_TEST_TMPDIR/strattest.yaml" } +# Regression guard for #103: init used to write a fixed OpenAI model whatever was +# configured, then print a run command that could not succeed. +# bats test_tags=tier:pr +@test "3b. Agent init without --model fails and writes nothing" { + run bash -c "cd '$BATS_TEST_TMPDIR' && '$PWD/conductor' agent init nomodel 2>&1" + echo "Output: $output" + [ "$status" -ne 0 ] + [[ "$output" == *"--model"* ]] + [ ! -f "$BATS_TEST_TMPDIR/nomodel.yaml" ] +} + # bats test_tags=tier:pr @test "4. Agent list succeeds" { require_agents_api