agent fix

config/assistant_test.go

@@ -0,0 +1,91 @@
+package config
+
+import (
+	"strings"
+	"testing"
+)
+
+// Why the assistant is off.
+//
+// "Off" was the same answer for four different mistakes, and the only symptom
+// was a disabled composer. Nobody could tell "we have not switched it on" from
+// "somebody misspelled a variable" — which is how it stayed off for days with
+// both of us guessing.
+
+func TestAFullyConfiguredAssistantIsOn(t *testing.T) {
+	cfg := AssistantConfig{
+		Provider: "openai", BaseURL: "https://api.groq.com/openai/v1",
+		APIKey: "k", Balanced: "openai/gpt-oss-120b",
+	}
+	if !cfg.Enabled() {
+		t.Fatalf("a complete config was refused: %s", cfg.Why())
+	}
+	if cfg.Why() != "" {
+		t.Fatalf("an enabled assistant gave a reason: %q", cfg.Why())
+	}
+}
+
+func TestEachMissingPieceNamesItself(t *testing.T) {
+	for name, tc := range map[string]struct {
+		cfg  AssistantConfig
+		says string
+	}{
+		// Nothing set at all names the MODEL, not the provider. The provider is
+		// derived from the model now, so an empty one is a consequence rather
+		// than a cause — and sending an operator to set ASSISTANT_PROVIDER, a
+		// variable they no longer need, while the one they actually missed goes
+		// unmentioned, is the same "off for four reasons" problem in new words.
+		"nothing set at all": {AssistantConfig{}, "ASSISTANT_MODEL"},
+		"no provider": {
+			AssistantConfig{Balanced: "m", APIKey: "k"}, "ASSISTANT_PROVIDER"},
+		"unknown provider": {
+			AssistantConfig{Provider: "anthropik", Balanced: "m", APIKey: "k"}, "not one this server speaks"},
+		"no model":   {AssistantConfig{Provider: "openai", APIKey: "k"}, "ASSISTANT_MODEL"},
+		"no api key": {AssistantConfig{Provider: "openai", Balanced: "m", BaseURL: "https://api.groq.com/openai/v1"}, "ASSISTANT_API_KEY"},
+	} {
+		why := tc.cfg.Why()
+		if why == "" {
+			t.Fatalf("%s: reported as working", name)
+		}
+		if !strings.Contains(why, tc.says) {
+			t.Fatalf("%s: does not name the problem: %q", name, why)
+		}
+	}
+}
+
+func TestALocalModelNeedsNoKey(t *testing.T) {
+	// Ollama and LM Studio need no credential, and demanding one would refuse
+	// the setup a developer is most likely to have on their own machine.
+	for _, base := range []string{
+		"http://localhost:11434/v1",
+		"http://127.0.0.1:1234/v1",
+		"http://host.docker.internal:11434/v1",
+	} {
+		cfg := AssistantConfig{Provider: "openai", BaseURL: base, Balanced: "llama3"}
+		if !cfg.Enabled() {
+			t.Fatalf("%s was refused without a key: %s", base, cfg.Why())
+		}
+	}
+}
+
+func TestAHostedModelWithoutAKeyIsRefusedBeforeItFailsAtRuntime(t *testing.T) {
+	// Otherwise the first question a shopkeeper asks comes back as a 401 from
+	// the provider, which reads as the assistant being broken rather than as a
+	// variable nobody set.
+	cfg := AssistantConfig{Provider: "openai", BaseURL: "https://api.groq.com/openai/v1", Balanced: "m"}
+	if cfg.Enabled() {
+		t.Fatal("a hosted provider with no key reported as ready")
+	}
+}
+
+func TestTheTierFallbackDoesNotHideAMissingModel(t *testing.T) {
+	// `fast` and `deep` fall back to balanced, so a config with only those two
+	// set has no model at all for the default tier.
+	cfg := AssistantConfig{Provider: "openai", APIKey: "k", Fast: "small", Deep: "big"}
+	if cfg.Enabled() {
+		t.Fatal("an assistant with no balanced model reported as ready")
+	}
+	if cfg.ModelFor("fast") != "" && cfg.ModelFor("balanced") != "" {
+		t.Fatal("balanced resolved to something despite being unset")
+	}
+}

config/config.go

@@ -172,7 +172,60 @@ type AssistantConfig struct {
 	Deep     string
 }
 
-func (a AssistantConfig) Enabled() bool { return a.Provider != "" && a.Balanced != "" }
+func (a AssistantConfig) Enabled() bool { return a.Why() == "" }
+
+// Why says what is missing, or "" when the assistant can run.
+//
+// A sentence rather than a bool, because "off" is the same answer for four
+// different mistakes: no provider, no model, no key, a provider nobody
+// recognises. Without this the only symptom is a disabled composer, and the
+// difference between "we have not switched it on" and "somebody misspelled a
+// variable" is invisible from the outside — which is exactly where this was
+// stuck.
+// The model is reported before the provider, and that order matters. Since
+// `assistantProvider` derives the provider from the model, an empty provider
+// means the model is empty too — and naming ASSISTANT_PROVIDER first would send
+// an operator to set a variable they no longer need, while the one they
+// actually missed went unmentioned.
+func (a AssistantConfig) Why() string {
+	if a.Balanced == "" {
+		return "ASSISTANT_MODEL is not set; give it the provider's model name, " +
+			"for example openai/gpt-oss-120b"
+	}
+	switch a.Provider {
+	case "openai", "groq", "ollama", "together", "compatible":
+	case "":
+		// Not reachable through Load, which derives it. Reachable when
+		// something builds this struct by hand, and silence would be worse.
+		return "ASSISTANT_PROVIDER is not set and could not be derived"
+	default:
+		return "ASSISTANT_PROVIDER is " + a.Provider + ", which is not one this server speaks"
+	}
+	// A local provider needs no credential; a hosted one always does, and a
+	// missing key otherwise surfaces as a 401 from the provider on the first
+	// question rather than as a configuration problem.
+	if a.APIKey == "" && !isLocalEndpoint(a.BaseURL) {
+		where := a.BaseURL
+		if where == "" {
+			// Empty means the OpenAI default, which is emphatically not local.
+			// "and  is not a local endpoint" is how that read before.
+			where = "the default https://api.openai.com/v1"
+		}
+		return "ASSISTANT_API_KEY is not set, and " + where + " is not a local endpoint"
+	}
+	return ""
+}
+
+// isLocalEndpoint reports whether a base URL is something running beside us.
+//
+// Ollama and LM Studio need no key, and demanding one would refuse the setup a
+// developer is most likely to have on their own machine.
+func isLocalEndpoint(baseURL string) bool {
+	url := strings.ToLower(baseURL)
+	return strings.Contains(url, "localhost") ||
+		strings.Contains(url, "127.0.0.1") ||
+		strings.Contains(url, "host.docker.internal")
+}
 
 // ModelFor resolves a tier to a model name, falling back rather than failing.
 //
@@ -192,6 +245,22 @@ func (a AssistantConfig) ModelFor(tier string) string {
 	return a.Balanced
 }
 
+// assistantProvider reads the provider, defaulting to the one shape this
+// server speaks.
+//
+// A deployment that names a model and a key has said what it wants; making it
+// also name a protocol it has no choice about is a variable that exists only to
+// be forgotten.
+func assistantProvider() string {
+	if named := strings.ToLower(strings.TrimSpace(env("ASSISTANT_PROVIDER", ""))); named != "" {
+		return named
+	}
+	if strings.TrimSpace(env("ASSISTANT_MODEL_BALANCED", env("ASSISTANT_MODEL", ""))) != "" {
+		return "openai"
+	}
+	return ""
+}
+
 // IsProduction is true under APP_ENV=production.
 func (c *Config) IsProduction() bool { return c.AppEnv == EnvProduction }
 
@@ -249,7 +318,11 @@ func Load() (*Config, error) {
 		},
 
 		Assistant: AssistantConfig{
-			Provider: strings.ToLower(env("ASSISTANT_PROVIDER", "")),
+			// Defaults to "openai" when a model is named, because every endpoint
+			// this speaks is OpenAI-compatible and the base URL is what actually
+			// distinguishes them. One less variable to set, and one less way to
+			// have the assistant silently off.
+			Provider: assistantProvider(),
 			BaseURL:  env("ASSISTANT_BASE_URL", ""),
 			APIKey:   env("ASSISTANT_API_KEY", ""),
 			Fast:     env("ASSISTANT_MODEL_FAST", ""),