Address Gadfly findings on #90 (blocking round)

- Remove config.AgentConfig.Ready() — dead after the refactor (no non-test
  callers), and its doc comment ("route registration and capabilities gate on
  this") was now false. EffectiveAgent.Ready() carries the same logic and IS
  used, see below.
- agentHolder now uses eff.Ready() and eff.APIKey instead of an inline check and
  a redundant apiKey field it held separately. This makes EffectiveAgent.APIKey/
  Ready() production-used rather than test-only, drops the duplicated readiness
  check, and simplifies newAgentHolder's signature.
- settingsPayload no longer swallows an EffectiveAgent error into a misleading
  empty "effective" view (which would read as "nothing configured"). It returns
  the error; the handlers surface it as a 500. EffectiveAgent re-reads the row
  GetInstanceSettings just returned, so a failure there is a real DB fault.
- Frontend streamChat handles 503 (assistant disabled at runtime) distinctly
  from 404 (endpoint absent) — the backend returns 503 AGENT_DISABLED now, which
  the old code mislabelled.
- Fixed the api.go comment that still said a disabled request gets a 404 — it's
  a 503.
- updateSettings uses the shared parseNullable[bool] instead of a bespoke
  parseNullableBool (now removed).
- NewRunner drops its empty-spec guard; agentmodel.Resolve already owns that
  check, so one place decides what a valid spec is.
- rebuild-on-resolve-error now documents WHY it keeps the current Runner rather
  than tearing down a working assistant on a transient DB blip: the state is
  persisted, the next rebuild reconciles, and killing a live assistant on a read
  hiccup is worse than a brief stale window.

Not changed: the store's version-conflict fallback returning GetInstanceSettings'
error instead of ErrVersionConflict when that read also fails. That's the exact
pattern every other version-guarded update uses (gardens/objects/plants); making
only this one differ would be the inconsistency. A DB read failing immediately
after the guarded UPDATE on the same local SQLite file is a disk-fault edge case,
and surfacing that error is defensible.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

internal/agent/runtime.go

@@ -48,9 +48,11 @@ type Runner struct {
 // constructor serves both boot (from env) and a runtime settings change (from
 // the DB) — the Runner has no idea which one configured it.
 func NewRunner(svc *service.Service, apiKey, modelSpec string) (*Runner, error) {
-	if apiKey == "" || strings.TrimSpace(modelSpec) == "" {
+	if apiKey == "" {
 		return nil, errors.New("agent: not configured")
 	}
+	// agentmodel.Resolve already rejects an empty/blank spec, so don't duplicate
+	// that guard here — one place decides what a valid spec is.
 	model, err := agentmodel.Resolve(apiKey, modelSpec)
 	if err != nil {
 		return nil, err

internal/api/agent_holder.go

@@ -24,10 +24,6 @@ import (
 // than assuming a Runner is present.
 type agentHolder struct {
 	svc *service.Service
-	// apiKey is the OLLAMA_CLOUD_API_KEY from the environment. It never changes at
-	// runtime — only the model spec and the enabled flag are settings — and it is
-	// deliberately not something the settings UI can read or write.
-	apiKey string
 
 	ptr atomic.Pointer[agent.Runner]
 	// rebuildMu serializes rebuilds so two concurrent settings saves can't
@@ -39,8 +35,8 @@ type agentHolder struct {
 // newAgentHolder builds the holder and resolves the initial Runner from whatever
 // the settings + environment currently say. A resolution failure is logged and
 // left as "no assistant", never fatal: a garden planner must still boot.
-func newAgentHolder(ctx context.Context, svc *service.Service, apiKey string) *agentHolder {
+func newAgentHolder(ctx context.Context, svc *service.Service) *agentHolder {
-	h := &agentHolder{svc: svc, apiKey: apiKey}
+	h := &agentHolder{svc: svc}
 	h.rebuild(ctx)
 	return h
 }
@@ -63,18 +59,23 @@ func (h *agentHolder) rebuild(ctx context.Context) {
 
 	eff, err := h.svc.EffectiveAgent(ctx)
 	if err != nil {
-		slog.Error("api: could not resolve agent settings; leaving assistant unchanged", "error", err)
+		// EffectiveAgent errors only if the settings row can't be read — a DB fault,
+		// and a very transient one when it happens right after a settings write. We
+		// keep the current Runner rather than tear down a working assistant on a
+		// blip: the state is already persisted, so the next rebuild (any later save,
+		// or a restart) reconciles it. Loud, because a persistent failure here means
+		// the live assistant no longer matches stored settings.
+		slog.Error("api: could not resolve agent settings; leaving the assistant as-is", "error", err)
 		return
 	}
-	// The env key is authoritative; EffectiveAgent reports it, but the holder was
+	if !eff.Ready() {
-	// handed it directly at construction, so use that.
-	if !eff.Enabled || h.apiKey == "" || eff.Model == "" {
 		if h.ptr.Swap(nil) != nil {
-			slog.Info("api: garden assistant turned off", "enabled", eff.Enabled, "hasModel", eff.Model != "")
+			slog.Info("api: garden assistant turned off",
+				"enabled", eff.Enabled, "hasKey", eff.APIKey != "", "hasModel", eff.Model != "")
 		}
 		return
 	}
-	runner, err := agent.NewRunner(h.svc, h.apiKey, eff.Model)
+	runner, err := agent.NewRunner(h.svc, eff.APIKey, eff.Model)
 	if err != nil {
 		// Configured but unusable. Turn the assistant off rather than leaving a
 		// stale Runner on the old model — an admin who just pointed it at a broken

internal/api/api.go

@@ -132,13 +132,13 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
 	// The garden assistant. Its routes are registered UNCONDITIONALLY and the live
 	// Runner sits behind an atomic pointer in the holder, so a settings change can
 	// turn the assistant on or off at runtime (#79). Each handler nil-checks
-	// agent.get(); a request while the assistant is off gets a clean 404/"not
+	// agent.get(); a chat request while the assistant is off gets a clean 503
-	// available", not a panic and not a missing route.
+	// (AGENT_DISABLED), not a panic and not a missing route.
 	//
 	// The holder resolves its initial Runner from settings + environment at
 	// construction. If the key never reaches the container, the assistant is off
 	// and the reason is logged below — the same operability need #72 added.
-	h.agent = newAgentHolder(context.Background(), svc, cfg.Agent.OllamaCloudAPIKey)
+	h.agent = newAgentHolder(context.Background(), svc)
 	if cfg.Agent.OllamaCloudAPIKey == "" {
 		slog.Info("api: garden assistant has no API key",
 			"hint", "set OLLAMA_CLOUD_API_KEY in the container's environment (not just the stack's); the model can be chosen in Settings")

internal/api/settings.go

@@ -53,22 +53,26 @@ type effectiveView struct {
 	AgentLive bool `json:"agentLive"`
 }
 
-func (h *handlers) settingsPayload(c *gin.Context, st *domain.InstanceSettings) settingsResponse {
+// settingsPayload builds the response, or an error. It does NOT swallow an
+// EffectiveAgent failure into a misleading empty "effective" view — an empty
+// view would report no model and no key, which reads as "nothing configured"
+// rather than "we couldn't read it". Since EffectiveAgent re-reads the same row
+// GetInstanceSettings just returned, a failure here is a genuine DB fault worth
+// surfacing as a 500, not papering over.
+func (h *handlers) settingsPayload(c *gin.Context, st *domain.InstanceSettings) (settingsResponse, error) {
 	eff, err := h.svc.EffectiveAgent(c.Request.Context())
 	if err != nil {
-		// The settings row read succeeded to get here, so this is unexpected; fall
+		return settingsResponse{}, err
-		// back to an empty effective view rather than failing the whole response.
-		eff = service.EffectiveAgent{}
 	}
 	return settingsResponse{
 		Settings: st,
 		Effective: effectiveView{
 			Model:     eff.Model,
 			Enabled:   eff.Enabled,
-			HasApiKey: h.cfg.Agent.OllamaCloudAPIKey != "",
+			HasApiKey: eff.APIKey != "",
 			AgentLive: h.agent.get() != nil,
 		},
-	}
+	}, nil
 }
 
 func (h *handlers) getSettings(c *gin.Context) {
@@ -77,7 +81,12 @@ func (h *handlers) getSettings(c *gin.Context) {
 		writeServiceError(c, err)
 		return
 	}
-	c.JSON(http.StatusOK, h.settingsPayload(c, st))
+	payload, err := h.settingsPayload(c, st)
+	if err != nil {
+		writeServiceError(c, err)
+		return
+	}
+	c.JSON(http.StatusOK, payload)
 }
 
 // settingsUpdateRequest is the PATCH body. agentModel "" means inherit the env
@@ -97,7 +106,9 @@ func (h *handlers) updateSettings(c *gin.Context) {
 	}
 
 	// agentEnabled: absent or null → inherit (nil); true/false → explicit override.
-	enabled, err := parseNullableBool(req.AgentEnabled)
+	// The shared parseNullable does exactly this three-way decode; present is
+	// irrelevant here because absent and null both mean "inherit".
+	enabled, _, err := parseNullable[bool](req.AgentEnabled)
 	if err != nil {
 		writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "agentEnabled must be true, false, or null")
 		return
@@ -123,18 +134,10 @@ func (h *handlers) updateSettings(c *gin.Context) {
 	// settings. Mirrors the same reasoning as the history-write detachment.
 	h.agent.rebuild(context.WithoutCancel(c.Request.Context()))
 
-	c.JSON(http.StatusOK, h.settingsPayload(c, st))
+	payload, err := h.settingsPayload(c, st)
-}
+	if err != nil {
-
+		writeServiceError(c, err)
-// parseNullableBool maps a JSON field that may be absent, null, or a bool to a
+		return
-// *bool: nil for absent/null (inherit), else a pointer to the value.
-func parseNullableBool(raw json.RawMessage) (*bool, error) {
-	if len(raw) == 0 || string(raw) == "null" {
-		return nil, nil
 	}
-	var b bool
+	c.JSON(http.StatusOK, payload)
-	if err := json.Unmarshal(raw, &b); err != nil {
-		return nil, err
-	}
-	return &b, nil
 }

internal/config/config.go

@@ -76,13 +76,6 @@ type AgentConfig struct {
 	Enabled bool
 }
 
-// Ready reports whether the assistant can actually be offered. Both the route
-// registration and whatever advertises capabilities gate on this, so what's
-// advertised always matches what's live.
-func (a AgentConfig) Ready() bool {
-	return a.Enabled && a.OllamaCloudAPIKey != "" && a.Model != ""
-}
-
 // Enabled reports whether enough OIDC config is present to attempt discovery.
 func (o OIDCConfig) Enabled() bool {
 	return o.Issuer != "" && o.ClientID != ""

web/src/lib/agent.ts

@@ -161,8 +161,13 @@ export async function streamChat(
     return
   }
   if (!res.ok || !res.body) {
+    // 503 is the assistant being turned off at runtime (#79) — the route exists,
+    // there's just no model behind it. Distinct from a 404, which would mean the
+    // whole endpoint is absent.
     handlers.onError(
-      res.status === 404
+      res.status === 503
+        ? "The assistant isn't enabled on this instance."
+        : res.status === 404
           ? "This instance doesn't have the assistant configured."
           : 'The assistant is not available right now.',
     )