diff --git a/CLAUDE.md b/CLAUDE.md index c263429..2e8bf9a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -163,6 +163,25 @@ Agent config: `OLLAMA_CLOUD_API_KEY`, `PANSY_AGENT_MODEL` (default strings pass verbatim to `majordomo.Parse`, so a comma-separated spec gives failover for free — don't parse that grammar in pansy. +The model and enabled flag are also **admin-editable at runtime** in Settings +(#79); the env vars are just defaults (precedence: DB setting → env → default). +Two things this makes load-bearing: + +- **The live Runner is hot-swapped, not built once.** It sits behind an + `atomic.Pointer` in `internal/api` (`agentHolder`), and its routes are + registered *unconditionally* with a nil-check on `agent.get()`. Do NOT go back + to registering the chat routes only when a key is present — a settings change + has to be able to turn the assistant on without a restart, which a missing + route can't. `/capabilities` reads the pointer, so it reflects the live state. +- **`OLLAMA_CLOUD_API_KEY` stays in the environment, never the DB.** Model + selection is a setting; the key is not. A secret in `instance_settings` lands + in every backup and in the undo history's blast radius. The Settings API + reports whether a key is *present*, never its value. +- The "how to turn a spec into a model" knowledge lives once in + `internal/agentmodel`, imported by both `agent` (to run) and `service` (to + validate a spec before storing it). It can't live in `agent` — `agent` imports + `service`, so a `service`→`agent` import would cycle. + `majordomo` is a **real dependency** now, resolved from the Gitea instance as a pseudo-version. There is no `replace` directive and there must not be one: a `replace` pointing at `../majordomo` builds on your laptop and breaks the Docker diff --git a/DESIGN.md b/DESIGN.md index ad41caa..fec4cd1 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -9,8 +9,9 @@ Work is tracked in Gitea issues; the tracking epic links every piece in dependen - **Placement model:** freeform plops (not a square-foot grid), scaled by real plant spacing. Grid snapping may come later as a toggle. - **Spacing is a plant-to-plant rule, so bed edges get half of it.** A bed edge is not a competitor for soil, light or water, so the outer row owes it half the spacing rather than a full one. `FillRegion` centres its lattice accordingly, and lets a plop — a *clump* three spacings across — cross the edge by up to half a spacing so its outermost plants land at that half-spacing. The rule, the square-foot-chart arithmetic behind it, and the failure mode it prevents are written out once in `hexCenters`; #75 is what getting it wrong looked like. - **Stack:** Go 1.26.x backend, module `gitea.stevedudenhoeffer.com/steve/pansy`; React + TypeScript + Vite + Tailwind frontend, production build embedded via `embed.FS` → one static binary (`CGO_ENABLED=0`). -- **Users:** multi-user with ownership. Users own gardens; a garden can be shared with other users as viewer (read) or editor (edit content). Owner additionally shares/deletes. +- **Users:** multi-user with ownership. Users own gardens; a garden can be shared with other users as viewer (read) or editor (edit content). Owner additionally shares/deletes. The first registered user is `is_admin` (set race-free inside the INSERT); admin gates instance-wide Settings (`requireAdmin`), the only thing that reads that flag. - **Auth:** OIDC-first (Authentik is the primary IdP), local argon2id passwords as an optional fallback. +- **Instance settings (#79):** admin-editable, instance-wide config in a single-row `instance_settings` table — pansy's first DB-stored *instance* state (everything else hangs off a garden/object). Today it holds the agent model + on/off; **secrets never move here** — `OLLAMA_CLOUD_API_KEY` stays in the env so it doesn't land in backups or the undo history. Precedence: Settings value → env → default. The live agent Runner sits behind an `atomic.Pointer` in the API layer (`agentHolder`) with its routes always registered, so a settings change swaps it with no restart and no race against in-flight requests; `/capabilities` reads that pointer, so it reports what's live rather than what was configured at boot. The model/registry knowledge lives in one leaf package (`internal/agentmodel`) that both the runner and the settings validator import — agent imports service, so it can live in neither. - **Agentic future:** integration with majordomo/executus via typed Go tools (`llm.DefineTool[Args]`) wrapping the same service layer the REST API uses — not MCP/OpenAPI. ## Domain model @@ -72,7 +73,8 @@ GET,POST /gardens/:id/journal PATCH,DELETE /journal/:id (editor writes GET /gardens/:id/journal/counts ← entries per object, for the "has notes" indicator POST /agent/chat ← SSE: step events, then the finished turn (editor only) GET,DELETE /gardens/:id/agent/history (the actor's own thread) -GET /capabilities ← what this instance can do, so the UI offers only what works +GET /capabilities ← what this instance can do RIGHT NOW (tracks the live agent, not just config) +GET,PATCH /settings ← instance-wide config (admin only): agent model + on/off GET,POST /gardens/:id/shares PATCH,DELETE /gardens/:id/shares/:userId (invite by email) GET,POST,DELETE /gardens/:id/share-link ← the public read-only token for this garden GET /public/gardens/:token ← UNAUTHENTICATED read-only /full; the token is the capability diff --git a/README.md b/README.md index af9385e..1193df8 100644 --- a/README.md +++ b/README.md @@ -67,13 +67,15 @@ The garden assistant reads three more. Setting none of them leaves the assistant | Variable | Default | Description | | ----------------------- | ------------------------------ | --------------------------------------------------------------------------- | -| `OLLAMA_CLOUD_API_KEY` | *(empty)* | Ollama Cloud API key. Without it the assistant is disabled, not broken — the chat routes simply aren't registered. | -| `PANSY_AGENT_MODEL` | `ollama-cloud/glm-5.2:cloud` | Model spec, passed verbatim to `majordomo.Parse` — a comma-separated list is a failover chain, e.g. `ollama-cloud/glm-5.2:cloud,ollama-cloud/kimi-k2.6:cloud`. | -| `PANSY_AGENT_ENABLED` | on when a key is present | Turns the assistant off without removing the key. | +| `OLLAMA_CLOUD_API_KEY` | *(empty)* | Ollama Cloud API key. Without it the assistant is off, not broken. This is the one agent value that stays in the environment — it is **never** stored in the database or editable in Settings. | +| `PANSY_AGENT_MODEL` | `ollama-cloud/glm-5.2:cloud` | Default model spec, passed verbatim to `majordomo.Parse` — a comma-separated list is a failover chain, e.g. `ollama-cloud/glm-5.2:cloud,ollama-cloud/kimi-k2.6:cloud`. An admin can override this per-instance in **Settings** without a redeploy; a blank Settings value inherits this. | +| `PANSY_AGENT_ENABLED` | on when a key is present | Default on/off for the assistant. Also overridable in Settings (which can inherit this default). | + +The model and enabled flag can be changed at runtime by an admin under **Settings** (the gear appears in the nav for admins) — the change swaps the live assistant with no restart. The env vars above are the defaults an untouched instance uses, and the API key is intentionally not among the runtime-editable settings: a secret in the database would land in every backup. Precedence for the model and enabled flag is **Settings value, if set → env var → built-in default**. The assistant acts without asking first, which is only reasonable because every turn is one undoable change set — see the History panel in the editor. -**If you set the key and the assistant still doesn't appear**, check that the variable reaches the *container*, not just your orchestrator's stack config — Compose needs it listed under the service's `environment:`. pansy logs `garden assistant disabled` at startup with which of the three conditions failed, so the answer is in the first few lines of the log. +**If you set the key and the assistant still doesn't appear**, check that the variable reaches the *container*, not just your orchestrator's stack config — Compose needs it listed under the service's `environment:`. pansy logs why the assistant is off at startup, and Settings shows the same status (a key present, the resolved model, and whether it's actually running). Local email/password auth is live (`POST /api/v1/auth/register`, `/auth/login`, `/auth/logout`, `GET /auth/me`, `GET /auth/providers`); the session is an HttpOnly cookie (`Secure` when `PANSY_BASE_URL` is https). The first account registered becomes admin, and it may register even when `PANSY_REGISTRATION=closed` to bootstrap the instance. diff --git a/internal/agent/runtime.go b/internal/agent/runtime.go index 6ac91be..ad07606 100644 --- a/internal/agent/runtime.go +++ b/internal/agent/runtime.go @@ -10,12 +10,10 @@ import ( "strings" "time" - "gitea.stevedudenhoeffer.com/steve/majordomo" "gitea.stevedudenhoeffer.com/steve/majordomo/agent" "gitea.stevedudenhoeffer.com/steve/majordomo/llm" - "gitea.stevedudenhoeffer.com/steve/majordomo/provider/ollama" - "gitea.stevedudenhoeffer.com/steve/pansy/internal/config" + "gitea.stevedudenhoeffer.com/steve/pansy/internal/agentmodel" "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" "gitea.stevedudenhoeffer.com/steve/pansy/internal/service" ) @@ -41,29 +39,23 @@ type Runner struct { model llm.Model } -// NewRunner resolves the configured model and returns a Runner, or an error if -// the assistant can't be offered. Callers should treat an error as "no -// assistant" rather than a startup failure — an instance with no key must still -// serve the app. -func NewRunner(svc *service.Service, cfg *config.Config) (*Runner, error) { - if !cfg.Agent.Ready() { +// NewRunner resolves modelSpec against pansy's registry and returns a Runner, or +// an error if the assistant can't be offered. Callers should treat an error as +// "no assistant" rather than a startup failure — an instance with no key must +// still serve the app. +// +// It takes the key and spec explicitly rather than a *config.Config so the same +// 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 == "" { return nil, errors.New("agent: not configured") } - - // A private registry, not the package-level default: pansy passes the key it - // was configured with rather than depending on ambient environment, and - // majordomo's own ollama-cloud preset reads OLLAMA_API_KEY while pansy (like - // gadfly) is configured with OLLAMA_CLOUD_API_KEY. Registering the provider - // explicitly makes that bridge visible instead of a mysterious empty token. - reg := majordomo.New() - reg.RegisterProvider(ollama.Cloud(ollama.WithToken(cfg.Agent.OllamaCloudAPIKey))) - - // The spec goes to Parse VERBATIM. The grammar — including comma-separated - // failover chains — is majordomo's, and re-implementing any of it here would - // only mean two places to update when it grows. - model, err := reg.Parse(cfg.Agent.Model) + // 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, fmt.Errorf("agent: resolve model %q: %w", cfg.Agent.Model, err) + return nil, err } return &Runner{svc: svc, model: model}, nil } diff --git a/internal/agent/runtime_test.go b/internal/agent/runtime_test.go index a331279..16791bd 100644 --- a/internal/agent/runtime_test.go +++ b/internal/agent/runtime_test.go @@ -13,7 +13,6 @@ import ( "gitea.stevedudenhoeffer.com/steve/majordomo/llm" "gitea.stevedudenhoeffer.com/steve/majordomo/provider/fake" - "gitea.stevedudenhoeffer.com/steve/pansy/internal/config" "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" "gitea.stevedudenhoeffer.com/steve/pansy/internal/service" ) @@ -232,24 +231,24 @@ func TestReadOnlyTurnWritesNoChangeSet(t *testing.T) { } } -// TestNewRunnerNeedsConfiguration — an instance with no key must not get a -// half-built runner; the caller treats the error as "no assistant" and carries on. +// TestNewRunnerNeedsConfiguration — an instance with no key or no model must not +// get a half-built runner; the caller treats the error as "no assistant" and +// carries on. (Whether the assistant is ENABLED is resolved before NewRunner is +// reached, so it isn't NewRunner's concern any more.) func TestNewRunnerNeedsConfiguration(t *testing.T) { svc, _ := newAgentTestService(t) - for _, cfg := range []*config.Config{ - {Agent: config.AgentConfig{Enabled: false, OllamaCloudAPIKey: "k", Model: "ollama-cloud/x"}}, - {Agent: config.AgentConfig{Enabled: true, OllamaCloudAPIKey: "", Model: "ollama-cloud/x"}}, - {Agent: config.AgentConfig{Enabled: true, OllamaCloudAPIKey: "k", Model: ""}}, + for _, tc := range []struct{ key, model string }{ + {"", "ollama-cloud/x"}, + {"k", ""}, + {"k", " "}, } { - if _, err := NewRunner(svc, cfg); err == nil { - t.Errorf("NewRunner accepted %+v", cfg.Agent) + if _, err := NewRunner(svc, tc.key, tc.model); err == nil { + t.Errorf("NewRunner accepted key=%q model=%q", tc.key, tc.model) } } // A model spec naming a provider that doesn't exist is a configuration // error, not a panic at first use. - if _, err := NewRunner(svc, &config.Config{Agent: config.AgentConfig{ - Enabled: true, OllamaCloudAPIKey: "k", Model: "nonesuch/model", - }}); err == nil { + if _, err := NewRunner(svc, "k", "nonesuch/model"); err == nil { t.Error("NewRunner accepted an unresolvable model spec") } } diff --git a/internal/agentmodel/agentmodel.go b/internal/agentmodel/agentmodel.go new file mode 100644 index 0000000..9b065e7 --- /dev/null +++ b/internal/agentmodel/agentmodel.go @@ -0,0 +1,57 @@ +// Package agentmodel holds the ONE place that knows how pansy turns a model spec +// into a majordomo model: which provider to register and under which token. +// +// It exists as a leaf so both internal/agent (which builds the run loop) and +// internal/service (which validates a spec before storing it as a setting) can +// share that knowledge without an import cycle — agent imports service, so the +// shared bit can live in neither of them. +package agentmodel + +import ( + "errors" + "fmt" + "strings" + + "gitea.stevedudenhoeffer.com/steve/majordomo" + "gitea.stevedudenhoeffer.com/steve/majordomo/llm" + "gitea.stevedudenhoeffer.com/steve/majordomo/provider/ollama" +) + +// registry builds the private majordomo registry pansy uses. +// +// Private, not the package-level default: pansy passes the key it was configured +// with rather than depending on ambient environment, and majordomo's own +// ollama-cloud preset reads OLLAMA_API_KEY while pansy (like gadfly) is +// configured with OLLAMA_CLOUD_API_KEY. Registering the provider explicitly +// makes that bridge visible instead of a mysterious empty token. +func registry(apiKey string) *majordomo.Registry { + reg := majordomo.New() + reg.RegisterProvider(ollama.Cloud(ollama.WithToken(apiKey))) + return reg +} + +// Resolve parses a model spec against pansy's registry into a live model. The +// spec goes to Parse VERBATIM — the grammar, including comma-separated failover +// chains, is majordomo's, and re-implementing any of it here would only mean two +// places to update when it grows. +func Resolve(apiKey, spec string) (llm.Model, error) { + if strings.TrimSpace(spec) == "" { + return nil, errors.New("agentmodel: empty model spec") + } + m, err := registry(apiKey).Parse(spec) + if err != nil { + return nil, fmt.Errorf("agentmodel: resolve %q: %w", spec, err) + } + return m, nil +} + +// Validate reports whether a spec resolves, without building anything the caller +// keeps — the cheap, deterministic check a settings save runs to reject a typo. +// +// It does NOT make a live call, so it needs no working key and won't catch a +// model that is merely absent upstream; that surfaces on first use. Parse +// resolving (known provider, well-formed spec) is the half worth doing eagerly. +func Validate(apiKey, spec string) error { + _, err := Resolve(apiKey, spec) + return err +} diff --git a/internal/agentmodel/agentmodel_test.go b/internal/agentmodel/agentmodel_test.go new file mode 100644 index 0000000..d8cda96 --- /dev/null +++ b/internal/agentmodel/agentmodel_test.go @@ -0,0 +1,35 @@ +package agentmodel + +import "testing" + +// TestValidate is what the settings PATCH relies on to reject a typo at save +// time rather than on the next chat turn. +func TestValidate(t *testing.T) { + if err := Validate("k", "ollama-cloud/glm-5.2:cloud"); err != nil { + t.Errorf("valid spec rejected: %v", err) + } + // No key needed to validate — Parse resolves the provider, it doesn't call it. + if err := Validate("", "ollama-cloud/glm-5.2:cloud"); err != nil { + t.Errorf("valid spec rejected without a key: %v", err) + } + // A comma-separated failover chain is majordomo grammar and must resolve. + if err := Validate("k", "ollama-cloud/glm-5.2:cloud,ollama-cloud/kimi-k2.6:cloud"); err != nil { + t.Errorf("failover chain rejected: %v", err) + } + for _, bad := range []string{"", " ", "nonesuch/model"} { + if err := Validate("k", bad); err == nil { + t.Errorf("Validate accepted %q", bad) + } + } +} + +// TestResolveReturnsAModel confirms a good spec yields a usable model handle. +func TestResolveReturnsAModel(t *testing.T) { + m, err := Resolve("k", "ollama-cloud/glm-5.2:cloud") + if err != nil { + t.Fatalf("resolve: %v", err) + } + if m == nil { + t.Fatal("resolve returned a nil model with no error") + } +} diff --git a/internal/api/agent.go b/internal/api/agent.go index 3eaee6d..cc6c5d8 100644 --- a/internal/api/agent.go +++ b/internal/api/agent.go @@ -56,6 +56,16 @@ type stepEvent struct { } func (h *handlers) agentChat(c *gin.Context) { + // The route is always registered, so the assistant being off is a runtime + // state, not a missing route: answer it plainly rather than 404ing a path + // that exists. Loaded once here so a settings-driven swap mid-request can't + // make it flip between the guard and the Run call. + runner := h.agent.get() + if runner == nil { + writeAPIError(c, http.StatusServiceUnavailable, "AGENT_DISABLED", "the garden assistant isn't enabled on this instance") + return + } + var req chatRequest if err := c.ShouldBindJSON(&req); err != nil { writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "a gardenId and a message are required") @@ -78,7 +88,7 @@ func (h *handlers) agentChat(c *gin.Context) { stopBeat := stream.keepAlive(keepAliveInterval) defer stopBeat() - turn, err := h.agent.Run(c.Request.Context(), actor.ID, req.GardenID, req.Message, + turn, err := runner.Run(c.Request.Context(), actor.ID, req.GardenID, req.Message, replayHistory(history), func(s mdagent.Step) { send(chatEvent{Step: &stepEvent{Index: s.Index, Tools: toolNames(s)}}) diff --git a/internal/api/agent_holder.go b/internal/api/agent_holder.go new file mode 100644 index 0000000..0c53c72 --- /dev/null +++ b/internal/api/agent_holder.go @@ -0,0 +1,89 @@ +package api + +import ( + "context" + "log/slog" + "sync" + "sync/atomic" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/agent" + "gitea.stevedudenhoeffer.com/steve/pansy/internal/service" +) + +// agentHolder owns the live assistant Runner and lets an admin swap it at +// runtime when the model settings change (#79). +// +// The Runner used to be a plain handlers field set once at boot, with the chat +// routes registered only when it existed. That made the assistant permanently +// whatever the environment said at startup. Now the routes are always +// registered and the Runner lives behind an atomic pointer, so a settings change +// can turn the assistant on, off, or onto a different model without a restart — +// and without a data race against in-flight requests reading the pointer. +// +// A nil pointer means "no assistant right now"; handlers nil-check get() rather +// than assuming a Runner is present. +type agentHolder struct { + svc *service.Service + + ptr atomic.Pointer[agent.Runner] + // rebuildMu serializes rebuilds so two concurrent settings saves can't + // interleave into a torn "resolve A, resolve B, store A, store B" swap. The + // read path (get) stays lock-free on the atomic pointer. + rebuildMu sync.Mutex +} + +// 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) *agentHolder { + h := &agentHolder{svc: svc} + h.rebuild(ctx) + return h +} + +// get returns the current Runner, or nil if the assistant is off. +func (h *agentHolder) get() *agent.Runner { return h.ptr.Load() } + +// rebuild resolves the effective agent configuration and swaps the Runner to +// match: a new one when the assistant should be on, nil when it shouldn't. It is +// safe to call at boot and from a settings save; concurrent calls serialize. +// +// It logs what it did rather than returning an error, because every caller wants +// the same thing — best-effort apply, keep serving either way — and a settings +// save must not fail just because the new model won't resolve. The save already +// validated the spec; a rebuild failure here means the environment changed under +// it, and "assistant off, with a reason in the log" is the right outcome. +func (h *agentHolder) rebuild(ctx context.Context) { + h.rebuildMu.Lock() + defer h.rebuildMu.Unlock() + + eff, err := h.svc.EffectiveAgent(ctx) + if err != nil { + // 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 + } + if !eff.Ready() { + if h.ptr.Swap(nil) != nil { + slog.Info("api: garden assistant turned off", + "enabled", eff.Enabled, "hasKey", eff.APIKey != "", "hasModel", eff.Model != "") + } + return + } + 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 + // spec should see it stop, not silently keep answering on the previous one. + slog.Error("api: garden assistant disabled (model won't resolve)", "error", err, "model", eff.Model) + h.ptr.Store(nil) + return + } + h.ptr.Store(runner) + slog.Info("api: garden assistant ready", "model", eff.Model) +} diff --git a/internal/api/agent_test.go b/internal/api/agent_test.go index 216f8ca..1d77ae3 100644 --- a/internal/api/agent_test.go +++ b/internal/api/agent_test.go @@ -6,24 +6,40 @@ import ( "testing" ) -// TestAgentRoutesAbsentWithoutAKey — an instance with no API key must start, -// serve the app, and simply not offer the assistant. The routes aren't -// registered at all, so this is a 404 rather than a handler that apologizes: -// the same shape as OIDC when unconfigured. -func TestAgentRoutesAbsentWithoutAKey(t *testing.T) { +// TestAgentDisabledWithoutAKey — an instance with no API key must start, serve +// the app, and not offer the assistant. +// +// The contract CHANGED with #79: the chat route is now always registered (so a +// settings change can turn the assistant on without a restart), so "off" is a +// runtime 503 rather than a missing route. capabilities reports agent:false, and +// the frontend keys the chat tab off that — so a user never reaches the 503. +func TestAgentDisabledWithoutAKey(t *testing.T) { r := authEngine(t, localCfg()) // localCfg has no agent configuration cookie := registerAndCookie(t, r, "noagent@example.com") gid := createGardenAPI(t, r, cookie, "G") + // Chat is refused, plainly, because there is no Runner to run. w := doJSON(t, r, http.MethodPost, "/api/v1/agent/chat", map[string]any{"gardenId": gid, "message": "plant garlic"}, cookie) - if w.Code != http.StatusNotFound { - t.Errorf("chat without a key: status %d, want 404", w.Code) + if w.Code != http.StatusServiceUnavailable { + t.Errorf("chat without a key: status %d, want 503", w.Code) } + // Capabilities advertises the assistant as unavailable, which is what the UI + // actually consults. + w = doJSON(t, r, http.MethodGet, "/api/v1/capabilities", nil, cookie) + if w.Code != http.StatusOK { + t.Fatalf("capabilities: status %d", w.Code) + } + if agent, _ := decodeMap(t, w.Body.Bytes())["agent"].(bool); agent { + t.Error("capabilities reported agent:true with no key") + } + + // History is just stored data behind the ordinary garden-role check, so it + // reads fine (empty) whether or not a Runner exists — it isn't gated on one. path := "/api/v1/gardens/" + strconv.FormatInt(gid, 10) + "/agent/history" - if w := doJSON(t, r, http.MethodGet, path, nil, cookie); w.Code != http.StatusNotFound { - t.Errorf("history without a key: status %d, want 404", w.Code) + if w := doJSON(t, r, http.MethodGet, path, nil, cookie); w.Code != http.StatusOK { + t.Errorf("history without a key: status %d, want 200 (it's data, not the model)", w.Code) } // And the rest of the app is entirely unaffected. diff --git a/internal/api/api.go b/internal/api/api.go index 5d88367..a6de108 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -6,13 +6,13 @@ package api import ( + "context" "log/slog" "net/http" "github.com/gin-gonic/gin" sloggin "github.com/samber/slog-gin" - "gitea.stevedudenhoeffer.com/steve/pansy/internal/agent" "gitea.stevedudenhoeffer.com/steve/pansy/internal/config" "gitea.stevedudenhoeffer.com/steve/pansy/internal/service" ) @@ -23,9 +23,12 @@ type handlers struct { cfg *config.Config svc *service.Service oidc *oidcClient // nil unless OIDC is configured (see config.OIDCReady) - // agent is nil unless the assistant is configured; the chat routes are only - // registered when it isn't, so a handler never has to check. - agent *agent.Runner + // agent holds the live Runner behind an atomic pointer. Unlike oidc it is + // never nil — the holder is always present and its Runner may be nil when the + // assistant is off. The chat routes are registered unconditionally and + // nil-check agent.get(), so a settings change can turn the assistant on or off + // at runtime (#79) rather than only at boot. + agent *agentHolder } // New builds the gin engine with the standard middleware stack and registers the @@ -131,37 +134,30 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine { plantings.PATCH("/:id", h.updatePlanting) plantings.DELETE("/:id", h.deletePlanting) - // The garden assistant, registered only when it can actually be offered — - // the same shape as OIDC. An instance with no API key serves the app - // normally and simply doesn't have these routes. - if !cfg.Agent.Ready() { - // Say WHY, at startup, in the logs an operator is already looking at. - // Someone who set the key and sees no assistant otherwise has nothing to - // check — and "is the variable reaching the container?" is exactly the - // question they need answered. - slog.Info("api: garden assistant disabled", - "enabled", cfg.Agent.Enabled, - "hasApiKey", cfg.Agent.OllamaCloudAPIKey != "", - "model", cfg.Agent.Model, - "hint", "needs OLLAMA_CLOUD_API_KEY set in the container's environment (not just the stack's)") - } - if cfg.Agent.Ready() { - runner, err := agent.NewRunner(svc, cfg) - if err != nil { - // Configured but unusable (an unresolvable model spec, say). Log it and - // carry on without the assistant rather than refusing to start: a - // garden planner that won't boot because of a chat feature is worse - // than one without chat. - slog.Error("api: garden assistant disabled", "error", err) - } else { - h.agent = runner - agentGroup := v1.Group("/agent", h.requireAuth()) - agentGroup.POST("/chat", h.agentChat) - gardens.GET("/:id/agent/history", h.getAgentHistory) - gardens.DELETE("/:id/agent/history", h.deleteAgentHistory) - slog.Info("api: garden assistant enabled", "model", cfg.Agent.Model) - } + // 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 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) + 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") } + agentGroup := v1.Group("/agent", h.requireAuth()) + agentGroup.POST("/chat", h.agentChat) + gardens.GET("/:id/agent/history", h.getAgentHistory) + gardens.DELETE("/:id/agent/history", h.deleteAgentHistory) + + // Instance settings: admin-only, and the first thing to enforce is_admin. + // requireAdmin runs after requireAuth (it reads the actor requireAuth stored). + settings := v1.Group("/settings", h.requireAuth(), h.requireAdmin()) + settings.GET("", h.getSettings) + settings.PATCH("", h.updateSettings) // Undo. A change set is addressed by its own id; the service resolves the // owning garden for the permission check, same as objects and plantings. @@ -203,11 +199,12 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine { // capabilities reports what this instance can actually do, so the UI offers only // what works. // -// It reports whether the runner BUILT, not whether it was configured: a -// configured-but-unresolvable model leaves the routes unregistered, and saying -// "yes" there would offer a chat tab whose first message 404s. +// It reports whether the assistant is live RIGHT NOW, not merely configured: +// the chat routes always exist, but a request while the Runner is nil is refused, +// so offering the tab must track the live Runner. Reading agent.get() (an atomic +// load) means this reflects a settings-driven swap on the very next poll. func (h *handlers) capabilities(c *gin.Context) { - c.JSON(http.StatusOK, gin.H{"agent": h.agent != nil}) + c.JSON(http.StatusOK, gin.H{"agent": h.agent.get() != nil}) } // healthz is a liveness probe: always returns {"ok": true} when the server is up. diff --git a/internal/api/settings.go b/internal/api/settings.go new file mode 100644 index 0000000..2cd8662 --- /dev/null +++ b/internal/api/settings.go @@ -0,0 +1,143 @@ +package api + +import ( + "context" + "encoding/json" + "errors" + "net/http" + + "github.com/gin-gonic/gin" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" + "gitea.stevedudenhoeffer.com/steve/pansy/internal/service" +) + +// Instance settings (#79): admin-only, instance-wide. The authoritative admin +// check is in the service; requireAdmin here is a cheap early 403 that also +// keeps the route group readable. + +// requireAdmin rejects a non-admin actor. It runs after requireAuth, so the +// actor is already resolved and carries IsAdmin — no extra query. Returns 403 +// (not 404): a logged-in user knows settings exist, they just may not touch them. +func (h *handlers) requireAdmin() gin.HandlerFunc { + return func(c *gin.Context) { + if !mustActor(c).IsAdmin { + writeAPIError(c, http.StatusForbidden, "FORBIDDEN", "admin access required") + c.Abort() + return + } + c.Next() + } +} + +// settingsResponse is what GET/PATCH /settings return. It carries the stored +// settings plus a read-only view of what's resolved and live, so the UI can show +// "inheriting ollama-cloud/glm-5.2:cloud from the environment" and whether a key +// is present — without ever exposing the key itself. +type settingsResponse struct { + Settings *domain.InstanceSettings `json:"settings"` + // Effective is the configuration actually in force after layering settings + // over the environment. + Effective effectiveView `json:"effective"` +} + +type effectiveView struct { + Model string `json:"model"` + Enabled bool `json:"enabled"` + // HasApiKey reports whether OLLAMA_CLOUD_API_KEY is set. The key itself is + // never serialized — an admin may know one exists, not what it is. + HasApiKey bool `json:"hasApiKey"` + // AgentLive is whether the assistant Runner is actually built right now. It + // can be false even when Enabled+HasApiKey are true (an unresolvable model), + // which is exactly the case the UI needs to surface. + AgentLive bool `json:"agentLive"` +} + +// 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 { + return settingsResponse{}, err + } + return settingsResponse{ + Settings: st, + Effective: effectiveView{ + Model: eff.Model, + Enabled: eff.Enabled, + HasApiKey: eff.APIKey != "", + AgentLive: h.agent.get() != nil, + }, + }, nil +} + +func (h *handlers) getSettings(c *gin.Context) { + st, err := h.svc.GetInstanceSettings(c.Request.Context(), mustActor(c).ID) + if err != nil { + writeServiceError(c, err) + return + } + 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 +// var. agentEnabled is json.RawMessage so an explicit null (inherit) is +// distinguishable from an absent field and from true/false. +type settingsUpdateRequest struct { + AgentModel string `json:"agentModel"` + AgentEnabled json.RawMessage `json:"agentEnabled"` + Version int64 `json:"version" binding:"required"` +} + +func (h *handlers) updateSettings(c *gin.Context) { + var req settingsUpdateRequest + if err := c.ShouldBindJSON(&req); err != nil { + writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "a current version is required") + return + } + + // agentEnabled: absent or null → inherit (nil); true/false → explicit override. + // 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 + } + + st, err := h.svc.UpdateInstanceSettings(c.Request.Context(), mustActor(c).ID, service.InstanceSettingsPatch{ + AgentModel: req.AgentModel, + AgentEnabled: enabled, + Version: req.Version, + }) + if err != nil { + if errors.Is(err, domain.ErrVersionConflict) { + writeVersionConflict(c, st) + return + } + writeServiceError(c, err) + return + } + + // Apply the change to the LIVE assistant. Detached from the request context: + // the write is committed and the rebuild describes it, so a client that hangs + // up now must not leave the running Runner out of step with the stored + // settings. Mirrors the same reasoning as the history-write detachment. + h.agent.rebuild(context.WithoutCancel(c.Request.Context())) + + payload, err := h.settingsPayload(c, st) + if err != nil { + writeServiceError(c, err) + return + } + c.JSON(http.StatusOK, payload) +} diff --git a/internal/api/settings_test.go b/internal/api/settings_test.go new file mode 100644 index 0000000..a7fd501 --- /dev/null +++ b/internal/api/settings_test.go @@ -0,0 +1,208 @@ +package api + +import ( + "net/http" + "testing" + + "github.com/gin-gonic/gin" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/config" +) + +// agentCfg is a config with the assistant configured — a (fake) key, on, and a +// resolvable model. The key isn't real, but ValidateAgentModel/NewRunner only +// PARSE the spec (no live call), so a Runner still builds and capabilities +// reports it live. That's enough to exercise the runtime on/off swap. +func agentCfg() *config.Config { + c := localCfg() + c.Agent = config.AgentConfig{ + Model: "ollama-cloud/glm-5.2:cloud", + OllamaCloudAPIKey: "test-key", + Enabled: true, + } + return c +} + +func settingsVersion(t *testing.T, r *gin.Engine, cookie *http.Cookie) int64 { + t.Helper() + w := doJSON(t, r, http.MethodGet, "/api/v1/settings", nil, cookie) + if w.Code != http.StatusOK { + t.Fatalf("get settings: status %d, body %s", w.Code, w.Body.String()) + } + st, _ := decodeMap(t, w.Body.Bytes())["settings"].(map[string]any) + return int64(st["version"].(float64)) +} + +// TestSettingsAdminOnly: the first registered user is admin and can read/write +// settings; a second user is not and gets 403 (not 404 — settings aren't a +// masked resource). +func TestSettingsAdminOnly(t *testing.T) { + r := authEngine(t, localCfg()) + admin := registerAndCookie(t, r, "admin@example.com") // first user → admin + member := registerAndCookie(t, r, "member@example.com") + + if w := doJSON(t, r, http.MethodGet, "/api/v1/settings", nil, admin); w.Code != http.StatusOK { + t.Fatalf("admin GET settings: status %d, body %s", w.Code, w.Body.String()) + } + + if w := doJSON(t, r, http.MethodGet, "/api/v1/settings", nil, member); w.Code != http.StatusForbidden { + t.Errorf("member GET settings: status %d, want 403", w.Code) + } + if w := doJSON(t, r, http.MethodPatch, "/api/v1/settings", + map[string]any{"agentModel": "x", "version": 1}, member); w.Code != http.StatusForbidden { + t.Errorf("member PATCH settings: status %d, want 403", w.Code) + } + // Unauthenticated is 401, before the admin check. + if w := doJSON(t, r, http.MethodGet, "/api/v1/settings", nil, nil); w.Code != http.StatusUnauthorized { + t.Errorf("anonymous GET settings: status %d, want 401", w.Code) + } +} + +// TestSettingsInheritFromEnv: an untouched instance reports the env model as +// effective, and an empty stored model keeps inheriting it. +func TestSettingsInheritFromEnv(t *testing.T) { + r := authEngine(t, agentCfg()) + admin := registerAndCookie(t, r, "admin@example.com") + + w := doJSON(t, r, http.MethodGet, "/api/v1/settings", nil, admin) + if w.Code != http.StatusOK { + t.Fatalf("get: status %d, body %s", w.Code, w.Body.String()) + } + body := decodeMap(t, w.Body.Bytes()) + st := body["settings"].(map[string]any) + eff := body["effective"].(map[string]any) + + if st["agentModel"] != "" { + t.Errorf("stored model = %v, want empty (inherit)", st["agentModel"]) + } + if eff["model"] != "ollama-cloud/glm-5.2:cloud" { + t.Errorf("effective model = %v, want the env value", eff["model"]) + } + if eff["hasApiKey"] != true || eff["agentLive"] != true { + t.Errorf("effective = %+v, want a key present and the agent live", eff) + } +} + +// TestSettingsUpdateSwapsTheRunner is the core of #79: changing settings takes +// effect on the LIVE assistant, with no restart. Driven entirely through HTTP. +func TestSettingsUpdateSwapsTheRunner(t *testing.T) { + r := authEngine(t, agentCfg()) + admin := registerAndCookie(t, r, "admin@example.com") + + capsAgent := func() bool { + w := doJSON(t, r, http.MethodGet, "/api/v1/capabilities", nil, admin) + return decodeMap(t, w.Body.Bytes())["agent"] == true + } + + // Configured and on out of the box. + if !capsAgent() { + t.Fatal("assistant should be live at boot with a key + enabled") + } + + // Turn it OFF via settings → capabilities flips immediately. + v := settingsVersion(t, r, admin) + w := doJSON(t, r, http.MethodPatch, "/api/v1/settings", + map[string]any{"agentModel": "", "agentEnabled": false, "version": v}, admin) + if w.Code != http.StatusOK { + t.Fatalf("disable: status %d, body %s", w.Code, w.Body.String()) + } + if capsAgent() { + t.Error("assistant still live after being disabled — the runner wasn't swapped") + } + // Chat now refuses, at runtime, on a route that still exists. + gid := createGardenAPI(t, r, admin, "G") + if w := doJSON(t, r, http.MethodPost, "/api/v1/agent/chat", + map[string]any{"gardenId": gid, "message": "hi"}, admin); w.Code != http.StatusServiceUnavailable { + t.Errorf("chat while disabled: status %d, want 503", w.Code) + } + + // Turn it back ON, with an explicit model, and confirm it's live again and the + // effective model reflects the change. + v = settingsVersion(t, r, admin) + w = doJSON(t, r, http.MethodPatch, "/api/v1/settings", map[string]any{ + "agentModel": "ollama-cloud/kimi-k2.6:cloud", "agentEnabled": true, "version": v, + }, admin) + if w.Code != http.StatusOK { + t.Fatalf("re-enable: status %d, body %s", w.Code, w.Body.String()) + } + if eff := decodeMap(t, w.Body.Bytes())["effective"].(map[string]any); eff["model"] != "ollama-cloud/kimi-k2.6:cloud" { + t.Errorf("effective model = %v after change, want the new one", eff["model"]) + } + if !capsAgent() { + t.Error("assistant not live after being re-enabled") + } +} + +// TestSettingsRejectsBadModel: a spec that won't resolve is a 400 at save time, +// not a broken assistant on the next turn. +func TestSettingsRejectsBadModel(t *testing.T) { + r := authEngine(t, agentCfg()) + admin := registerAndCookie(t, r, "admin@example.com") + v := settingsVersion(t, r, admin) + + if w := doJSON(t, r, http.MethodPatch, "/api/v1/settings", + map[string]any{"agentModel": "nonesuch/model", "version": v}, admin); w.Code != http.StatusBadRequest { + t.Errorf("bad model: status %d, want 400", w.Code) + } + // agentEnabled must be a bool or null, not a string. + if w := doJSON(t, r, http.MethodPatch, "/api/v1/settings", + map[string]any{"agentModel": "", "agentEnabled": "yes", "version": v}, admin); w.Code != http.StatusBadRequest { + t.Errorf("string agentEnabled: status %d, want 400", w.Code) + } +} + +// TestSettingsVersionConflict: a stale version 409s and carries the current row. +func TestSettingsVersionConflict(t *testing.T) { + r := authEngine(t, agentCfg()) + admin := registerAndCookie(t, r, "admin@example.com") + v := settingsVersion(t, r, admin) + + // First write succeeds and bumps the version. + if w := doJSON(t, r, http.MethodPatch, "/api/v1/settings", + map[string]any{"agentModel": "", "agentEnabled": false, "version": v}, admin); w.Code != http.StatusOK { + t.Fatalf("first update: status %d, body %s", w.Code, w.Body.String()) + } + // Reusing the old version conflicts. + w := doJSON(t, r, http.MethodPatch, "/api/v1/settings", + map[string]any{"agentModel": "", "agentEnabled": true, "version": v}, admin) + if w.Code != http.StatusConflict { + t.Fatalf("stale update: status %d, want 409", w.Code) + } + if cur, ok := decodeMap(t, w.Body.Bytes())["current"].(map[string]any); !ok || cur["version"].(float64) != float64(v+1) { + t.Errorf("409 body missing the current row at the bumped version: %s", w.Body.String()) + } +} + +// TestSettingsSwapUnderRace runs settings saves concurrently with chat requests, +// so `go test -race` proves the atomic swap of the live Runner is safe against +// in-flight readers. The whole point of the atomic.Pointer is this: without it, +// toggling the assistant while a request reads it is a data race. +func TestSettingsSwapUnderRace(t *testing.T) { + r := authEngine(t, agentCfg()) + admin := registerAndCookie(t, r, "admin@example.com") + + done := make(chan struct{}) + // Readers hammer capabilities, whose h.agent.get() is the SAME atomic Load the + // chat handler does — so this races the pointer read against the writer's swap + // without ever invoking the model (a real Run would hit the network on a fake + // key). If get() is race-clean here it is race-clean in chat. + for i := 0; i < 4; i++ { + go func() { + for { + select { + case <-done: + return + default: + doJSON(t, r, http.MethodGet, "/api/v1/capabilities", nil, admin) + } + } + }() + } + // Writer: flip the assistant on and off, swapping the pointer each time. + for i := 0; i < 12; i++ { + v := settingsVersion(t, r, admin) + doJSON(t, r, http.MethodPatch, "/api/v1/settings", + map[string]any{"agentModel": "", "agentEnabled": i%2 == 0, "version": v}, admin) + } + close(done) +} 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/internal/domain/domain.go b/internal/domain/domain.go index 37beac4..2ac4884 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -242,6 +242,22 @@ const ( ConflictUnsupported = "unsupported" ) +// InstanceSettings is the single row of instance-wide, admin-editable +// configuration (#79). Secrets are deliberately absent — see migration 0010. +// +// Both agent fields express "inherit from the environment unless set": +// AgentModel == "" falls back to PANSY_AGENT_MODEL; AgentEnabled == nil inherits +// the env default. EffectiveAgent resolves them against the environment. +type InstanceSettings struct { + // AgentModel overrides PANSY_AGENT_MODEL when non-empty. Passed verbatim to + // majordomo.Parse, exactly like the env var it shadows. + AgentModel string `json:"agentModel"` + // AgentEnabled overrides PANSY_AGENT_ENABLED when non-nil. nil = inherit. + AgentEnabled *bool `json:"agentEnabled"` + Version int64 `json:"version"` + UpdatedAt string `json:"updatedAt"` +} + // User is a pansy account. It may have a local password, OIDC identity, or both. type User struct { ID int64 `json:"id"` diff --git a/internal/service/instance_settings.go b/internal/service/instance_settings.go new file mode 100644 index 0000000..9c92e41 --- /dev/null +++ b/internal/service/instance_settings.go @@ -0,0 +1,110 @@ +package service + +import ( + "context" + "strings" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/agentmodel" + "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" +) + +// Instance settings (#79): admin-editable, instance-wide configuration. The +// admin gate lives HERE, in the seam, not in the handler — the same rule every +// other permission follows. The API's requireAdmin middleware is a cheap early +// 403, not the authority. + +// requireAdmin returns nil iff the actor is an admin, else ErrForbidden. +// +// ErrForbidden, not ErrNotFound: settings are not a resource whose existence is +// masked. A logged-in non-admin knows the instance has settings; they simply may +// not touch them. (Contrast objects/lots, where no-access masks existence.) +func (s *Service) requireAdmin(ctx context.Context, actorID int64) error { + u, err := s.store.GetUserByID(ctx, actorID) + if err != nil { + return err + } + if !u.IsAdmin { + return domain.ErrForbidden + } + return nil +} + +// GetInstanceSettings returns the instance settings for an admin. +func (s *Service) GetInstanceSettings(ctx context.Context, actorID int64) (*domain.InstanceSettings, error) { + if err := s.requireAdmin(ctx, actorID); err != nil { + return nil, err + } + return s.store.GetInstanceSettings(ctx) +} + +// InstanceSettingsPatch is a full replacement of the editable fields plus the +// current version. Both agent fields carry their "inherit" sentinel: an empty +// AgentModel means fall back to env, a nil AgentEnabled means inherit. +type InstanceSettingsPatch struct { + AgentModel string + AgentEnabled *bool + Version int64 +} + +// UpdateInstanceSettings applies an admin's change, version-guarded. It returns +// (current row, ErrVersionConflict) on a stale version, like every mutable +// resource. The model spec is validated before it is stored, so a typo is a 400 +// now rather than a broken assistant on the next turn. +// +// It does NOT rebuild the running agent — that is the API layer's job, because +// the live Runner lives there. The caller rebuilds on success. +func (s *Service) UpdateInstanceSettings(ctx context.Context, actorID int64, patch InstanceSettingsPatch) (*domain.InstanceSettings, error) { + if err := s.requireAdmin(ctx, actorID); err != nil { + return nil, err + } + model := strings.TrimSpace(patch.AgentModel) + // Validate a non-empty spec up front. An empty one is the "inherit env" + // sentinel and needs no check — the env value was validated at boot. + if model != "" { + if err := agentmodel.Validate(s.cfg.Agent.OllamaCloudAPIKey, model); err != nil { + return nil, domain.ErrInvalidInput + } + } + return s.store.UpdateInstanceSettings(ctx, &domain.InstanceSettings{ + AgentModel: model, + AgentEnabled: patch.AgentEnabled, + Version: patch.Version, + }) +} + +// EffectiveAgent resolves the agent configuration actually in force: DB settings +// override the environment, and the API key always comes from the environment. +// +// This is internal plumbing (the API layer calls it to build the Runner), NOT an +// admin-gated operation — resolving what's configured is not the same as editing +// it, and the agent bootstrap must work before any user is even authenticated. +type EffectiveAgent struct { + Model string + Enabled bool + APIKey string +} + +// Ready mirrors config.AgentConfig.Ready: enabled, with a key and a model. +func (e EffectiveAgent) Ready() bool { + return e.Enabled && e.APIKey != "" && e.Model != "" +} + +// EffectiveAgent reads the settings row and layers it over the environment. +func (s *Service) EffectiveAgent(ctx context.Context) (EffectiveAgent, error) { + st, err := s.store.GetInstanceSettings(ctx) + if err != nil { + return EffectiveAgent{}, err + } + eff := EffectiveAgent{ + Model: s.cfg.Agent.Model, + Enabled: s.cfg.Agent.Enabled, + APIKey: s.cfg.Agent.OllamaCloudAPIKey, + } + if st.AgentModel != "" { + eff.Model = st.AgentModel + } + if st.AgentEnabled != nil { + eff.Enabled = *st.AgentEnabled + } + return eff, nil +} diff --git a/internal/service/instance_settings_test.go b/internal/service/instance_settings_test.go new file mode 100644 index 0000000..9e0deb6 --- /dev/null +++ b/internal/service/instance_settings_test.go @@ -0,0 +1,117 @@ +package service + +import ( + "context" + "errors" + "testing" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/config" + "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" +) + +// settingsTestService builds a service whose env config carries the given agent +// model/enabled/key, so EffectiveAgent's env fallback can be exercised. +func settingsTestService(t *testing.T, envModel string, envEnabled bool, key string) (*Service, int64) { + t.Helper() + cfg := openConfig() + cfg.Agent = config.AgentConfig{Model: envModel, Enabled: envEnabled, OllamaCloudAPIKey: key} + s := newTestService(t, cfg) + admin := seedUser(t, s, "admin@example.com") // first user is admin + return s, admin +} + +// TestRequireAdmin: the first user is admin; a second is not and gets +// ErrForbidden (not ErrNotFound — settings existence isn't masked). +func TestRequireAdmin(t *testing.T) { + s, admin := settingsTestService(t, "ollama-cloud/x", true, "k") + member := seedUser(t, s, "member@example.com") + + if err := s.requireAdmin(context.Background(), admin); err != nil { + t.Errorf("admin rejected: %v", err) + } + if err := s.requireAdmin(context.Background(), member); !errors.Is(err, domain.ErrForbidden) { + t.Errorf("member requireAdmin = %v, want ErrForbidden", err) + } +} + +// TestEffectiveAgentLayering: DB settings override env; the API key always comes +// from env; the "inherit" sentinels fall back. +func TestEffectiveAgentLayering(t *testing.T) { + ctx := context.Background() + s, admin := settingsTestService(t, "ollama-cloud/env-model", true, "envkey") + + // Untouched: everything inherits env. + eff, err := s.EffectiveAgent(ctx) + if err != nil { + t.Fatalf("effective: %v", err) + } + if eff.Model != "ollama-cloud/env-model" || !eff.Enabled || eff.APIKey != "envkey" { + t.Errorf("inherited effective = %+v, want the env values", eff) + } + + // Override the model only; enabled still inherits env (true). + cur, _ := s.GetInstanceSettings(ctx, admin) + if _, err := s.UpdateInstanceSettings(ctx, admin, InstanceSettingsPatch{ + AgentModel: "ollama-cloud/glm-5.2:cloud", Version: cur.Version, + }); err != nil { + t.Fatalf("update model: %v", err) + } + eff, _ = s.EffectiveAgent(ctx) + if eff.Model != "ollama-cloud/glm-5.2:cloud" { + t.Errorf("model = %q, want the DB override", eff.Model) + } + if !eff.Enabled { + t.Error("enabled should still inherit env (true) when unset") + } + + // Now override enabled to false explicitly. + cur, _ = s.GetInstanceSettings(ctx, admin) + no := false + if _, err := s.UpdateInstanceSettings(ctx, admin, InstanceSettingsPatch{ + AgentModel: "ollama-cloud/glm-5.2:cloud", AgentEnabled: &no, Version: cur.Version, + }); err != nil { + t.Fatalf("update enabled: %v", err) + } + eff, _ = s.EffectiveAgent(ctx) + if eff.Enabled { + t.Error("enabled should be the explicit false override now") + } + if eff.Ready() { + t.Error("Ready() should be false when disabled") + } +} + +// TestUpdateInstanceSettingsRejectsBadModel: a spec that won't resolve is +// ErrInvalidInput, before it is stored. +func TestUpdateInstanceSettingsRejectsBadModel(t *testing.T) { + ctx := context.Background() + s, admin := settingsTestService(t, "ollama-cloud/x", true, "k") + cur, _ := s.GetInstanceSettings(ctx, admin) + + if _, err := s.UpdateInstanceSettings(ctx, admin, InstanceSettingsPatch{ + AgentModel: "nonesuch/model", Version: cur.Version, + }); !errors.Is(err, domain.ErrInvalidInput) { + t.Errorf("bad model = %v, want ErrInvalidInput", err) + } + + // The rejected write didn't touch the row. + after, _ := s.GetInstanceSettings(ctx, admin) + if after.Version != cur.Version || after.AgentModel != "" { + t.Errorf("a rejected update changed the row: %+v", after) + } +} + +// TestInstanceSettingsAdminGate: the read/write operations are admin-gated at the +// service seam, not just in the handler. +func TestInstanceSettingsAdminGate(t *testing.T) { + ctx := context.Background() + s, _ := settingsTestService(t, "ollama-cloud/x", true, "k") + member := seedUser(t, s, "member@example.com") + + if _, err := s.GetInstanceSettings(ctx, member); !errors.Is(err, domain.ErrForbidden) { + t.Errorf("member GetInstanceSettings = %v, want ErrForbidden", err) + } + if _, err := s.UpdateInstanceSettings(ctx, member, InstanceSettingsPatch{Version: 1}); !errors.Is(err, domain.ErrForbidden) { + t.Errorf("member UpdateInstanceSettings = %v, want ErrForbidden", err) + } +} diff --git a/internal/store/instance_settings.go b/internal/store/instance_settings.go new file mode 100644 index 0000000..fa89ce0 --- /dev/null +++ b/internal/store/instance_settings.go @@ -0,0 +1,81 @@ +package store + +import ( + "context" + "database/sql" + "errors" + "fmt" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" +) + +// The instance_settings row is seeded by migration 0010 and there is exactly one +// (CHECK id = 1), so reads never branch on existence and writes never insert. + +const instanceSettingsColumns = `agent_model, agent_enabled, version, updated_at` + +// scanInstanceSettings reads the single settings row. agent_enabled is a nullable +// INTEGER (NULL = inherit env), so it is scanned through sql.NullInt64. +func scanInstanceSettings(s scanner) (*domain.InstanceSettings, error) { + var ( + out domain.InstanceSettings + enabled sql.NullInt64 + ) + if err := s.Scan(&out.AgentModel, &enabled, &out.Version, &out.UpdatedAt); err != nil { + return nil, err + } + if enabled.Valid { + b := enabled.Int64 != 0 + out.AgentEnabled = &b + } + return &out, nil +} + +// GetInstanceSettings returns the single settings row. +func (d *DB) GetInstanceSettings(ctx context.Context) (*domain.InstanceSettings, error) { + s, err := scanInstanceSettings(d.sql.QueryRowContext(ctx, + `SELECT `+instanceSettingsColumns+` FROM instance_settings WHERE id = 1`)) + if errors.Is(err, sql.ErrNoRows) { + // The migration seeds this row, so its absence is a broken database, not a + // normal "not found" the caller should paper over. + return nil, fmt.Errorf("store: instance_settings row missing (migration 0010 not applied?)") + } + if err != nil { + return nil, fmt.Errorf("store: get instance settings: %w", err) + } + return s, nil +} + +// UpdateInstanceSettings applies a version-guarded update to the single row, +// following the same optimistic-concurrency contract as every mutable resource: +// the updated row on success, (current row, ErrVersionConflict) on a version +// mismatch. There is no ErrNotFound path — the row always exists. +// +// agentEnabled is nil to store SQL NULL (inherit env), or a pointer to store an +// explicit 0/1. +func (d *DB) UpdateInstanceSettings(ctx context.Context, s *domain.InstanceSettings) (*domain.InstanceSettings, error) { + var enabled any + if s.AgentEnabled != nil { + enabled = boolToInt(*s.AgentEnabled) + } + updated, err := scanInstanceSettings(d.sql.QueryRowContext(ctx, + `UPDATE instance_settings + SET agent_model = ?, agent_enabled = ?, + version = version + 1, + updated_at = strftime('%Y-%m-%dT%H:%M:%SZ', 'now') + WHERE id = 1 AND version = ? + RETURNING `+instanceSettingsColumns, + s.AgentModel, enabled, s.Version, + )) + if errors.Is(err, sql.ErrNoRows) { + current, gerr := d.GetInstanceSettings(ctx) + if gerr != nil { + return nil, gerr + } + return current, domain.ErrVersionConflict + } + if err != nil { + return nil, fmt.Errorf("store: update instance settings: %w", err) + } + return updated, nil +} diff --git a/internal/store/migrations/0010_instance_settings.sql b/internal/store/migrations/0010_instance_settings.sql new file mode 100644 index 0000000..32b396c --- /dev/null +++ b/internal/store/migrations/0010_instance_settings.sql @@ -0,0 +1,35 @@ +-- Instance settings (#79): the first configuration that lives in the database +-- rather than the environment. +-- +-- Until now every preference hung off a garden or object row; this is pansy's +-- first INSTANCE-level state. A single-row table (CHECK id = 1) is the least +-- surprising shape for "there is exactly one of these" — a key/value table would +-- invite typo'd keys and lose the column types. +-- +-- Only NON-SECRET agent settings live here. OLLAMA_CLOUD_API_KEY stays in the +-- environment on purpose: a copy in SQLite would land in every backup and in the +-- blast radius of the undo history. An admin can change WHICH model runs, not +-- WHOSE account pays for it. +-- +-- Both agent columns are "inherit from env unless set": +-- * agent_model = '' means fall back to PANSY_AGENT_MODEL, then the built-in +-- default. So an instance that never opens Settings behaves exactly as it +-- did before this migration, and the documented env var keeps working. +-- * agent_enabled is NULLABLE: NULL means inherit PANSY_AGENT_ENABLED's +-- behaviour (on when a key is present), 0/1 is an explicit override. A plain +-- boolean couldn't tell "admin hasn't touched this" from "admin turned it +-- off", and those must deploy differently. +-- +-- version drives the same optimistic-concurrency 409 every other mutable row +-- uses, so two admins editing at once conflict rather than clobber. +CREATE TABLE instance_settings ( + id INTEGER PRIMARY KEY CHECK (id = 1), + agent_model TEXT NOT NULL DEFAULT '', + agent_enabled INTEGER CHECK (agent_enabled IN (0, 1)), + version INTEGER NOT NULL DEFAULT 1, + updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')) +); + +-- Seed the single row so every read is a plain SELECT with no "does it exist +-- yet" branch. Inherits everything from the environment out of the box. +INSERT INTO instance_settings (id, agent_model, agent_enabled) VALUES (1, '', NULL); diff --git a/web/src/components/layout/AppShell.tsx b/web/src/components/layout/AppShell.tsx index f0554d8..88442b2 100644 --- a/web/src/components/layout/AppShell.tsx +++ b/web/src/components/layout/AppShell.tsx @@ -53,6 +53,19 @@ export function AppShell() { {l.label} ))} + {/* Settings is admin-only, matching the server's requireAdmin gate. + A non-admin who typed /settings still gets a 403 from the API — the + hidden link is convenience, not the security boundary. */} + {user?.isAdmin && ( + + Settings + + )} {user ? ( diff --git a/web/src/lib/agent.ts b/web/src/lib/agent.ts index 1fdfdbf..dafc9de 100644 --- a/web/src/lib/agent.ts +++ b/web/src/lib/agent.ts @@ -13,13 +13,21 @@ import { historyKey } from './history' const capabilitiesSchema = z.object({ agent: z.boolean() }) -/** Whether this instance has the assistant configured. Without it the panel - * isn't rendered at all — a dead button is worse than no button. */ +export const capabilitiesKey = ['capabilities'] as const + +/** Whether the assistant is live RIGHT NOW. Without it the panel isn't rendered + * at all — a dead button is worse than no button. + * + * Not `staleTime: Infinity` any more: an admin can turn the assistant on or off + * in Settings (#79), so this must be able to change under a running page. The + * settings save invalidates this key directly; the finite staleTime just means + * another admin's change is picked up on the next focus/remount rather than + * never. */ export function useCapabilities() { return useQuery({ - queryKey: ['capabilities'] as const, + queryKey: capabilitiesKey, queryFn: async () => capabilitiesSchema.parse(await api.get('/capabilities')), - staleTime: Infinity, // server config; it doesn't change under a running page + staleTime: 60_000, }) } @@ -153,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 } diff --git a/web/src/lib/settings.ts b/web/src/lib/settings.ts new file mode 100644 index 0000000..5c7fc07 --- /dev/null +++ b/web/src/lib/settings.ts @@ -0,0 +1,79 @@ +// Instance settings data layer (#79): admin-only, instance-wide configuration. +// +// The GET/PATCH return both the stored settings and a read-only "effective" view +// — what's actually in force after layering the DB over the environment — so the +// form can say "inheriting ollama-cloud/glm-5.2:cloud from the environment" and +// whether the API key is present, without the key ever crossing the wire. + +import { queryOptions, useMutation, useQuery, useQueryClient } from '@tanstack/react-query' +import { z } from 'zod' +import { ApiError, api } from './api' +import { capabilitiesKey } from './agent' + +export const instanceSettingsSchema = z.object({ + // '' means "inherit the PANSY_AGENT_MODEL env var". + agentModel: z.string(), + // null means "inherit PANSY_AGENT_ENABLED"; true/false is an explicit override. + agentEnabled: z.boolean().nullable(), + version: z.number(), + updatedAt: z.string(), +}) +export type InstanceSettings = z.infer + +export const effectiveAgentSchema = z.object({ + model: z.string(), + enabled: z.boolean(), + hasApiKey: z.boolean(), + agentLive: z.boolean(), +}) +export type EffectiveAgent = z.infer + +export const settingsResponseSchema = z.object({ + settings: instanceSettingsSchema, + effective: effectiveAgentSchema, +}) +export type SettingsResponse = z.infer + +export const settingsKey = ['settings'] as const + +export const settingsQueryOptions = queryOptions({ + queryKey: settingsKey, + queryFn: async (): Promise => + settingsResponseSchema.parse(await api.get('/settings')), +}) + +export function useSettings() { + return useQuery(settingsQueryOptions) +} + +export interface SettingsUpdate { + agentModel: string + agentEnabled: boolean | null + version: number +} + +export function useUpdateSettings() { + const qc = useQueryClient() + return useMutation({ + mutationFn: async (input: SettingsUpdate): Promise => + settingsResponseSchema.parse(await api.patch('/settings', input)), + onSuccess: (res) => { + qc.setQueryData(settingsKey, res) + // The save may have turned the assistant on or off; the editor keys its + // chat tab off /capabilities, so make it re-read rather than trust its + // cached answer. + qc.invalidateQueries({ queryKey: capabilitiesKey }) + }, + }) +} + +/** If err is a 409 version conflict, return the fresh settings it carries so a + * form can rebase; otherwise null. */ +export function conflictSettings(err: unknown): InstanceSettings | null { + if (err instanceof ApiError && err.isConflict && err.body && typeof err.body === 'object') { + const current = (err.body as { current?: unknown }).current + const parsed = instanceSettingsSchema.safeParse(current) + if (parsed.success) return parsed.data + } + return null +} diff --git a/web/src/pages/SettingsPage.tsx b/web/src/pages/SettingsPage.tsx new file mode 100644 index 0000000..c843cd2 --- /dev/null +++ b/web/src/pages/SettingsPage.tsx @@ -0,0 +1,164 @@ +import { useEffect, useState } from 'react' +import { Alert } from '@/components/ui/Alert' +import { Button } from '@/components/ui/Button' +import { Select } from '@/components/ui/Select' +import { TextField } from '@/components/ui/TextField' +import { toast } from '@/components/ui/toast' +import { errorMessage } from '@/lib/api' +import { + conflictSettings, + useSettings, + useUpdateSettings, + type EffectiveAgent, + type InstanceSettings, +} from '@/lib/settings' +import { usePageTitle } from '@/lib/usePageTitle' + +// agentEnabled is a tri-state on the wire (null = inherit env, true, false); the +// form models it as three named choices so "inherit" is a deliberate pick, not +// an empty control. +type EnabledChoice = 'inherit' | 'on' | 'off' +const toChoice = (v: boolean | null): EnabledChoice => (v === null ? 'inherit' : v ? 'on' : 'off') +const fromChoice = (c: EnabledChoice): boolean | null => (c === 'inherit' ? null : c === 'on') + +/** Admin-only instance settings (#79). The one that matters today is the agent + * model; the API key stays in the environment and is only ever reported as + * present/absent, never shown or edited. */ +export function SettingsPage() { + usePageTitle('Settings') + const settings = useSettings() + const update = useUpdateSettings() + + if (settings.isPending) { + return

Loading settings…

+ } + if (settings.isError) { + return {errorMessage(settings.error, "Couldn't load settings.")} + } + return +} + +function SettingsForm({ + data, + update, +}: { + data: { settings: InstanceSettings; effective: EffectiveAgent } + update: ReturnType +}) { + const [model, setModel] = useState(data.settings.agentModel) + const [enabled, setEnabled] = useState(toChoice(data.settings.agentEnabled)) + const [version, setVersion] = useState(data.settings.version) + const [error, setError] = useState(null) + + // Rebase the form when the query cache updates (e.g. a successful save writes + // the new row back), so the version we submit is never stale. + useEffect(() => { + setVersion(data.settings.version) + }, [data.settings.version]) + + const eff = data.effective + + const save = () => { + setError(null) + update.mutate( + { agentModel: model.trim(), agentEnabled: fromChoice(enabled), version }, + { + onSuccess: () => toast.info('Settings saved.'), + onError: (err) => { + const current = conflictSettings(err) + if (current) { + // Someone else saved first. Adopt their row so the next attempt is + // clean, and say so rather than silently discarding this edit. + setModel(current.agentModel) + setEnabled(toChoice(current.agentEnabled)) + setVersion(current.version) + setError('Someone else changed these settings just now — reloaded their version. Re-apply your change if you still want it.') + return + } + setError(errorMessage(err, "Couldn't save settings.")) + }, + }, + ) + } + + return ( +
+
+

Settings

+

+ Instance-wide, admin-only. Changes take effect immediately — no restart. +

+
+ +
+
+

Garden assistant

+

+ The model runs against your Ollama Cloud key, which is set in the environment and never + shown here. +

+
+ + + + setModel(e.target.value)} + hint={ + model.trim() === '' + ? `Empty — inheriting ${eff.model || 'the built-in default'} from the environment.` + : 'A majordomo model spec. A comma-separated list is a failover chain.' + } + /> + +