From 9ab454373b1566258b3566fc88562775c3cc2368 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 22:17:25 -0400 Subject: [PATCH] Address Gadfly findings on #90 (blocking round) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/agent/runtime.go | 4 +++- internal/api/agent_holder.go | 25 ++++++++++---------- internal/api/api.go | 6 ++--- internal/api/settings.go | 45 +++++++++++++++++++----------------- internal/config/config.go | 7 ------ web/src/lib/agent.ts | 11 ++++++--- 6 files changed, 51 insertions(+), 47 deletions(-) diff --git a/internal/agent/runtime.go b/internal/agent/runtime.go index 7be5171..ad07606 100644 --- a/internal/agent/runtime.go +++ b/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 diff --git a/internal/api/agent_holder.go b/internal/api/agent_holder.go index 82d4d4b..0c53c72 100644 --- a/internal/api/agent_holder.go +++ b/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 { - h := &agentHolder{svc: svc, apiKey: apiKey} +func newAgentHolder(ctx context.Context, svc *service.Service) *agentHolder { + 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 - // handed it directly at construction, so use that. - if !eff.Enabled || h.apiKey == "" || eff.Model == "" { + if !eff.Ready() { 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 diff --git a/internal/api/api.go b/internal/api/api.go index c85fc3d..b866fd6 100644 --- a/internal/api/api.go +++ b/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 - // available", not a panic and not a missing route. + // agent.get(); a chat request while the assistant is off gets a clean 503 + // (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") diff --git a/internal/api/settings.go b/internal/api/settings.go index 3812a97..2cd8662 100644 --- a/internal/api/settings.go +++ b/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 - // back to an empty effective view rather than failing the whole response. - eff = service.EffectiveAgent{} + return settingsResponse{}, err } 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)) -} - -// parseNullableBool maps a JSON field that may be absent, null, or a bool to a -// *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 + payload, err := h.settingsPayload(c, st) + if err != nil { + writeServiceError(c, err) + return } - var b bool - if err := json.Unmarshal(raw, &b); err != nil { - return nil, err - } - return &b, nil + c.JSON(http.StatusOK, payload) } diff --git a/internal/config/config.go b/internal/config/config.go index d3752dc..380e039 100644 --- a/internal/config/config.go +++ b/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 != "" diff --git a/web/src/lib/agent.ts b/web/src/lib/agent.ts index 4cd5600..dafc9de 100644 --- a/web/src/lib/agent.ts +++ b/web/src/lib/agent.ts @@ -161,10 +161,15 @@ 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 - ? "This instance doesn't have the assistant configured." - : 'The assistant is not available right now.', + 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.', ) return }