Chat panel in the garden editor (#57)

The conversational surface, in the editor rather than on its own page. The
reason is the feedback loop: watching the canvas change as the agent works IS
the confirmation, which is exactly what makes "act freely without asking first"
tolerable. It also means the agent never has to guess which garden you mean.

Tool calls surface as they happen, in the app's own vocabulary — "Clearing a
bed", "Looking up a plant" — not raw tool names or JSON. That is the difference
between the panel feeling like it's doing something and feeling like it's hung,
which matters because a replant makes a dozen calls over tens of seconds. An
unknown tool degrades to readable words rather than showing snake_case at the
user, so the client can lag the server by a tool without looking broken.

The canvas refreshes as each step lands, not just at the end. Refreshing only on
completion would put the whole point of siting the chat here — watching it work
— behind the same wait that streaming exists to remove.

Undo sits on the turn itself, so the common case never involves opening the
History panel. It uses #49's useUndo, not a second implementation, which meant
giving that hook an UndoTarget: the chat knows a turn's change set id but not
its tally, and fabricating counts to satisfy the type would have produced a
confidently wrong "1 of 1 changes undone". describeUndo now says "Partly undone"
when it has no denominator rather than inventing one.

The panel is only offered when the instance actually has the assistant
configured, via a new /capabilities read. The routes 404 without a key, so
without this the client would have to probe for a 404 to find out — and a tab
that opens onto an apology is worse than no tab.

Streaming is hand-rolled over fetch rather than EventSource, which can only
issue GETs and this needs a POST body. The wire format is still SSE, so a proxy
that understands it doesn't buffer and the server wouldn't change if EventSource
became viable. Partial frames are buffered across chunks and a malformed frame
is skipped rather than killing a working stream.

Errors read as sentences, and as different sentences: a permission refusal, a
timeout and a model failure want different reactions. Every failure path
refreshes, because something may have landed before it failed — and says where
to look for it.

Closes #57

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

CLAUDE.md

@@ -85,12 +85,6 @@ Frontend: React 19 + Vite + Tailwind 4 + TanStack Router/Query, built into
   deliberately. `ErrForbidden` means "you can see it but may not do that".
 - **Plops (plantings) live in their parent object's local frame**, origin at the
   object's center, `-y` is north. Moving or rotating a bed moves its plants free.
-- **A plop is a clump, not a plant.** `defaultPlopRadius` is `1.5 × spacing`, so a
-  plop is three spacings across and holds `π·r²/spacing²` plants. Reasoning about
-  fills as if one plop were one plant gets the geometry wrong every time — which
-  is how #75 happened: requiring the whole circle inside the bed inset the outer
-  row by 1.5 spacings when the horticultural rule is *half* a spacing. Spacing is
-  a constraint between neighbouring plants; a bed edge is nobody's neighbour.
 - **Soft removal**: "clear bed" sets `removed_at`; the editor reads
   `removed_at IS NULL`. Hard delete is a different operation.
 - **Migrations** are numbered `.sql` files in `internal/store/migrations/`, run
@@ -98,29 +92,6 @@ Frontend: React 19 + Vite + Tailwind 4 + TanStack Router/Query, built into
 - **Every service mutation lands in history** (#48). If you add one, record it —
   see `internal/service/revisions.go`. Multi-row operations pass all their
   changes to a single `record` call so they undo as one unit.
-- **The history write is detached from cancellation on purpose.** `commitScope`
-  calls `context.WithoutCancel` — that is not a mistake to tidy up. By the time
-  a commit runs, the rows it describes are already written, so cancelling it
-  cannot undo anything; it can only leave real changes with no way to undo them.
-  This was a live bug twice (#73): a client disconnect mid-request orphaned 18
-  plantings. Fixing it per-call-site is how it came back, which is why the rule
-  lives in `commitScope` where no caller can forget it.
-
-## Testing
-
-Match the test to the failure it would catch:
-
-- **Anything addressed by its own id needs an API-level test through the router.**
-  Service tests can't see a route that was never registered — PATCH/DELETE
-  `/journal/:id` once shipped fully implemented, fully unit-tested, and
-  completely unreachable.
-- **Watch for fixtures that assert your assumptions instead of the API.** A test
-  for the undo message passed because the fixture I wrote populated a field the
-  real response leaves empty. If a test builds the thing it's testing against,
-  it is checking your mental model, not the system.
-- Some things only real use finds. The agent's whole loop is covered by
-  majordomo's scriptable fake provider (`provider/fake`), which is worth using —
-  but the three worst v2 bugs all turned up in one live session afterwards.
 
 ## Workflow
 
@@ -129,22 +100,6 @@ fix what's real → merge when the pipeline is green. Do not grade Gadfly findin
 A push to `main` builds the image and deploys to Komodo; the live instance at
 `pansy.orgrimmar.dudenhoeffer.casa` updates a few minutes later.
 
-**Gadfly reviews the PR as opened, not as merged.** The workflow triggers on
-`opened`/`reopened`/`ready_for_review` — deliberately *not* `synchronize` — so
-every commit you push afterwards, including the ones you push in response to
-Gadfly itself, is unreviewed unless you ask. Once you've stopped pushing and
-before you merge, comment **`@gadfly review`** on the PR to re-trigger it. The
-phrase is required, and this is not hypothetical: on #76 the follow-up commit
-was the one that contained a real bug.
-
-**A skipped Gadfly run reports success.** A comment without the trigger phrase
-still starts the workflow, which logs `comment does not contain trigger phrase`
-and exits green in ~2 seconds. So "the pipeline is green" does NOT mean "this
-was reviewed". Confirm a re-review actually ran by its **duration** — a real
-pass takes ~10 minutes, a skip takes 2 seconds. Don't look for a new consensus
-comment: Gadfly EDITS its existing status-board and consensus comments in place,
-so their `created_at` stays at the first review and only `updated_at` moves.
-
 Workflow- and config-only changes (CI, this file, docs) go straight to `main`
 without the PR dance.
 
@@ -163,25 +118,6 @@ 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

DESIGN.md

@@ -7,11 +7,9 @@ Work is tracked in Gitea issues; the tracking epic links every piece in dependen
 ## Decisions
 
 - **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. 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.
+- **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.
 - **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
@@ -71,8 +69,7 @@ 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 RIGHT NOW (tracks the live agent, not just config)
-GET,PATCH /settings           ← instance-wide config (admin only): agent model + on/off
+GET  /capabilities            ← what this instance can do, so the UI offers only what works
 GET,POST /gardens/:id/shares  PATCH,DELETE /gardens/:id/shares/:userId    (invite by email)
 ```
 

README.md

@@ -67,16 +67,12 @@ 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 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**.
+| `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.                            |
 
 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 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.
 
 OIDC (Authentik-first) is live too: set `PANSY_OIDC_ISSUER`, `PANSY_OIDC_CLIENT_ID`, `PANSY_OIDC_CLIENT_SECRET`, and `PANSY_BASE_URL` (needed for the redirect URI). Register `PANSY_BASE_URL` + `/api/v1/auth/oidc/callback` as the redirect URI in your IdP. `GET /auth/oidc/login` starts an authorization-code + PKCE flow; first login provisions a user just-in-time (a matching *verified* email links to an existing local account instead of duplicating it). Provider discovery is lazy, so a briefly-unreachable IdP never blocks startup or local auth. Set `PANSY_LOCAL_AUTH=false` for pure-Authentik deployments (local register/login are then rejected and hidden from `/auth/providers`).
@@ -115,8 +111,7 @@ services:
       # PANSY_OIDC_ISSUER: https://auth.example.com/application/o/pansy/
       # PANSY_OIDC_CLIENT_ID: ...
       # PANSY_OIDC_CLIENT_SECRET: ...
-      # OLLAMA_CLOUD_API_KEY: ${OLLAMA_CLOUD_API_KEY}   # enables the garden assistant
-      # PANSY_AGENT_MODEL: ollama-cloud/glm-5.2:cloud
+      # OLLAMA_CLOUD_API_KEY: ...     # enables the garden assistant
     restart: unless-stopped
 volumes:
   pansy-data:

internal/agent/runtime.go

@@ -2,18 +2,17 @@ package agent
 
 import (
 	"context"
-	"crypto/rand"
-	"encoding/hex"
 	"errors"
 	"fmt"
-	"log/slog"
 	"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/agentmodel"
+	"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
 )
@@ -39,23 +38,29 @@ type Runner struct {
 	model llm.Model
 }
 
-// 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 == "" {
+// 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() {
 		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)
+
+	// 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)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("agent: resolve model %q: %w", cfg.Agent.Model, err)
 	}
 	return &Runner{svc: svc, model: model}, nil
 }
@@ -71,6 +76,8 @@ type Turn struct {
 	Steps int `json:"steps"`
 	// Truncated is set when the run hit its step cap rather than finishing.
 	Truncated bool `json:"truncated,omitempty"`
+	// History is the transcript to feed back into the next turn.
+	History []llm.Message `json:"-"`
 }
 
 // Run executes one turn against a garden, as actorID.
@@ -93,12 +100,6 @@ func (r *Runner) Run(ctx context.Context, actorID, gardenID int64, message strin
 		return nil, err
 	}
 
-	// An id for this run, stamped on the change set so a row in the history list
-	// can be matched to the log lines that produced it. Without it, "the agent
-	// did something odd on Tuesday" has no thread back to what it was thinking.
-	runID := newRunID()
-	slog.Info("agent: run start", "run", runID, "garden", gardenID, "actor", actorID)
-
 	var (
 		result   *agent.Result
 		runErr   error
@@ -107,7 +108,6 @@ func (r *Runner) Run(ctx context.Context, actorID, gardenID int64, message strin
 	changeSet, err := r.svc.WithChangeSet(ctx, actorID, gardenID, service.ChangeSetOptions{
 		Source:  domain.SourceAgent,
 		Summary: turnSummary(message),
-		AgentRunID: &runID,
 	}, func(ctx context.Context) error {
 		box := NewToolbox(r.svc, actorID)
 		a := agent.New(r.model, systemPrompt(garden),
@@ -143,6 +143,7 @@ func (r *Runner) Run(ctx context.Context, actorID, gardenID int64, message strin
 	if result != nil {
 		turn.Reply = result.Output
 		turn.Steps = len(result.Steps)
+		turn.History = result.Messages
 	}
 	if turn.Reply == "" {
 		turn.Reply = fallbackReply(turn)
@@ -171,29 +172,13 @@ func fallbackReply(t *Turn) string {
 	}
 }
 
-// newRunID returns a short random identifier for one run.
-func newRunID() string {
-	var b [8]byte
-	if _, err := rand.Read(b[:]); err != nil {
-		// The id is for correlating logs, not for security. A clock-based
-		// fallback is worse than random and better than an empty string.
-		return fmt.Sprintf("t%d", time.Now().UnixNano())
-	}
-	return hex.EncodeToString(b[:])
-}
-
 // turnSummary is what the history list shows for this turn. The user's own words
 // are the most useful label available, trimmed to fit a list row.
-//
-// Trimmed by RUNES, not bytes: slicing a byte offset would cut a multibyte
-// character in half and store invalid UTF-8 in the summary — which is not a
-// hypothetical for text people type.
 func turnSummary(message string) string {
 	const max = 120
 	s := strings.Join(strings.Fields(message), " ")
-	runes := []rune(s)
-	if len(runes) > max {
-		s = strings.TrimSpace(string(runes[:max])) + "…"
+	if len(s) > max {
+		s = strings.TrimSpace(s[:max]) + "…"
 	}
 	return s
 }

internal/agent/runtime_test.go

@@ -7,12 +7,11 @@ import (
 	"fmt"
 	"strings"
 	"testing"
-	"time"
-	"unicode/utf8"
 
 	"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"
 )
@@ -231,24 +230,24 @@ func TestReadOnlyTurnWritesNoChangeSet(t *testing.T) {
 	}
 }
 
-// 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.)
+// TestNewRunnerNeedsConfiguration — an instance with no key must not get a
+// half-built runner; the caller treats the error as "no assistant" and carries on.
 func TestNewRunnerNeedsConfiguration(t *testing.T) {
 	svc, _ := newAgentTestService(t)
-	for _, tc := range []struct{ key, model string }{
-		{"", "ollama-cloud/x"},
-		{"k", ""},
-		{"k", "   "},
+	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: ""}},
 	} {
-		if _, err := NewRunner(svc, tc.key, tc.model); err == nil {
-			t.Errorf("NewRunner accepted key=%q model=%q", tc.key, tc.model)
+		if _, err := NewRunner(svc, cfg); err == nil {
+			t.Errorf("NewRunner accepted %+v", cfg.Agent)
 		}
 	}
 	// A model spec naming a provider that doesn't exist is a configuration
 	// error, not a panic at first use.
-	if _, err := NewRunner(svc, "k", "nonesuch/model"); err == nil {
+	if _, err := NewRunner(svc, &config.Config{Agent: config.AgentConfig{
+		Enabled: true, OllamaCloudAPIKey: "k", Model: "nonesuch/model",
+	}}); err == nil {
 		t.Error("NewRunner accepted an unresolvable model spec")
 	}
 }
@@ -278,81 +277,3 @@ func TestSystemPromptStatesTheCompassConvention(t *testing.T) {
 		t.Error("system prompt doesn't state the garden's size")
 	}
 }
-
-// TestPartialWorkSurvivesATimeout is the finding that mattered most on this PR.
-//
-// The run context carries a timeout. When it fires, WithChangeSet's recovery
-// path has to record what already committed — and doing that with the SAME
-// dead context would fail, losing the history for changes that really happened.
-// The user-facing message says "anything I'd already changed is in History", so
-// this isn't just a gap, it's a promise the code has to keep.
-func TestPartialWorkSurvivesATimeout(t *testing.T) {
-	ctx := context.Background()
-	svc, owner := newAgentTestService(t)
-	g, err := svc.CreateGarden(ctx, owner, service.GardenInput{Name: "Plot", WidthCM: 2000, HeightCM: 2000})
-	if err != nil {
-		t.Fatalf("garden: %v", err)
-	}
-	bed, err := svc.CreateObject(ctx, owner, g.ID, service.ObjectInput{
-		Kind: domain.KindBed, Name: "Bed", XCM: 1000, YCM: 1000, WidthCM: 400, HeightCM: 400,
-	})
-	if err != nil {
-		t.Fatalf("bed: %v", err)
-	}
-	before, _, _ := svc.GardenHistory(ctx, owner, g.ID, 0, 0)
-
-	// A turn that renames the bed, then dies with the context already cancelled.
-	cancelled, cancel := context.WithCancel(ctx)
-	r := scriptedRunner(t, svc,
-		toolCall("move_object", map[string]any{
-			"objectId": bed.ID, "xCm": 600.0, "yCm": 600.0, "version": bed.Version,
-		}),
-		fake.Step{Err: context.DeadlineExceeded},
-	)
-	// Cancel once the first tool call has landed, so the failure path runs with a
-	// dead context — exactly the timeout case.
-	go func() {
-		time.Sleep(50 * time.Millisecond)
-		cancel()
-	}()
-	_, err = r.Run(cancelled, owner, g.ID, "move the bed", nil, nil)
-	if err == nil {
-		t.Fatal("expected the turn to fail")
-	}
-
-	// The move committed, so it must be in history and undoable.
-	after, _, herr := svc.GardenHistory(ctx, owner, g.ID, 0, 0)
-	if herr != nil {
-		t.Fatalf("history: %v", herr)
-	}
-	if len(after) != len(before)+1 {
-		t.Fatalf("the failed turn recorded %d change sets, want 1 — its work is otherwise un-undoable",
-			len(after)-len(before))
-	}
-	if !strings.Contains(after[0].Summary, "failed partway") {
-		t.Errorf("summary = %q, want it marked as partial", after[0].Summary)
-	}
-	if _, conflicts, rerr := svc.RevertChangeSet(ctx, owner, after[0].ID, domain.SourceUI); rerr != nil || len(conflicts) != 0 {
-		t.Fatalf("the partial turn should be undoable: err=%v conflicts=%+v", rerr, conflicts)
-	}
-	o, _ := svc.DescribeGarden(ctx, owner, g.ID)
-	if len(o.Objects) > 0 && o.Objects[0].XCM != bed.XCM {
-		t.Errorf("undo left the bed at %v, want %v", o.Objects[0].XCM, bed.XCM)
-	}
-}
-
-// TestTurnSummaryTrimsByRunes — slicing a byte offset would cut a multibyte
-// character in half and store invalid UTF-8 in the history summary.
-func TestTurnSummaryTrimsByRunes(t *testing.T) {
-	// 200 multibyte runes: a byte slice at 120 would land mid-character.
-	got := turnSummary(strings.Repeat("🌱", 200))
-	if !utf8.ValidString(got) {
-		t.Errorf("turnSummary produced invalid UTF-8: %q", got)
-	}
-	if !strings.HasSuffix(got, "…") {
-		t.Errorf("long summary should be elided, got %q", got)
-	}
-	if n := utf8.RuneCountInString(got); n > 121 {
-		t.Errorf("summary is %d runes, want it trimmed", n)
-	}
-}

internal/agentmodel/agentmodel.go

@@ -1,57 +0,0 @@
-// 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
-}

internal/agentmodel/agentmodel_test.go

@@ -1,35 +0,0 @@
-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")
-	}
-}

internal/api/agent.go

@@ -8,8 +8,6 @@ import (
 	"io"
 	"log/slog"
 	"net/http"
-	"sync"
-	"time"
 
 	mdagent "gitea.stevedudenhoeffer.com/steve/majordomo/agent"
 	"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
@@ -26,11 +24,6 @@ import (
 // by everything at once, which reads as a hang — and the whole design rests on
 // watching the canvas change as it happens.
 
-// keepAliveInterval is how often a quiet stream emits a comment frame. Well
-// under the 30–60s idle timeout typical of reverse proxies, which is the thing
-// it exists to stay ahead of.
-const keepAliveInterval = 20 * time.Second
-
 // chatRequest is the body of POST /agent/chat.
 type chatRequest struct {
 	GardenID int64  `json:"gardenId" binding:"required"`
@@ -45,9 +38,6 @@ type chatEvent struct {
 	Done *agent.Turn `json:"done,omitempty"`
 	// Error is a turn that failed, in words meant for a person.
 	Error string `json:"error,omitempty"`
-	// Warning rides alongside Done: the turn worked, but something adjacent to it
-	// didn't, and saying nothing would be the quieter lie.
-	Warning string `json:"warning,omitempty"`
 }
 
 type stepEvent struct {
@@ -56,16 +46,6 @@ 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")
@@ -79,21 +59,29 @@ func (h *handlers) agentChat(c *gin.Context) {
 		return
 	}
 
-	stream := openEventStream(c)
-	send := stream.send
+	// Headers before the first write, and a flush straight away: a proxy that
+	// buffers the response would reintroduce exactly the silence streaming is
+	// here to remove.
+	c.Header("Content-Type", "text/event-stream")
+	c.Header("Cache-Control", "no-cache")
+	c.Header("X-Accel-Buffering", "no")
+	c.Writer.Flush()
 
-	// A model thinking hard between tool calls sends nothing for a while, and an
-	// idle proxy will cut a quiet connection. Deferred so a panic in the run
-	// can't leak the ticker goroutine; stopping it twice is harmless.
-	stopBeat := stream.keepAlive(keepAliveInterval)
-	defer stopBeat()
+	send := func(ev chatEvent) {
+		b, err := json.Marshal(ev)
+		if err != nil {
+			slog.Error("api: encode chat event", "error", err)
+			return
+		}
+		_, _ = fmt.Fprintf(c.Writer, "data: %s\n\n", b)
+		c.Writer.Flush()
+	}
 
-	turn, err := runner.Run(c.Request.Context(), actor.ID, req.GardenID, req.Message,
+	turn, err := h.agent.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)}})
 		})
-	stopBeat()
 	if err != nil {
 		// The stream is already open, so an error is an event rather than a
 		// status code — the client has committed to reading a stream by now.
@@ -101,94 +89,16 @@ func (h *handlers) agentChat(c *gin.Context) {
 		return
 	}
 
-	// The turn itself succeeded — the garden really did change — so Done goes out
-	// regardless. But if the transcript couldn't be saved, say so: a clean "done"
-	// followed by a conversation that has forgotten the exchange after a reload is
-	// exactly the kind of quiet inconsistency that makes a tool feel unreliable.
-	//
-	// Detached from the request context, because the commonest reason this fails
-	// is the client having gone away — and the exchange is worth keeping either
-	// way, since the change set it produced certainly is.
-	if _, err := h.svc.RecordAgentExchange(context.WithoutCancel(c.Request.Context()), actor.ID, req.GardenID,
+	// Record before announcing: if persistence fails, the user should see that
+	// their turn wasn't saved rather than a clean "done" followed by a thread
+	// that has forgotten it.
+	if _, err := h.svc.RecordAgentExchange(c.Request.Context(), actor.ID, req.GardenID,
 		req.Message, turn.Reply, turn.ChangeSetID); err != nil {
 		slog.Error("api: record agent exchange", "error", err, "garden", req.GardenID)
-		send(chatEvent{Done: turn, Warning: "I couldn't save this exchange, so it won't be here after a reload. Anything I changed is still on the canvas, and in History."})
-		return
 	}
 	send(chatEvent{Done: turn})
 }
 
-// eventStream serializes writes to one SSE response.
-//
-// The mutex is load-bearing, not decoration: step events are sent from the
-// agent's run goroutine while the keep-alive ticker writes from its own, and two
-// goroutines writing a ResponseWriter concurrently is a data race that corrupts
-// frames long before it crashes anything.
-type eventStream struct {
-	c  *gin.Context
-	mu sync.Mutex
-}
-
-// openEventStream puts the response into SSE mode.
-//
-// Headers go out before the first write and the stream is flushed immediately,
-// so a proxy holding the response until it looks complete can't reintroduce
-// exactly the silence streaming exists to remove.
-func openEventStream(c *gin.Context) *eventStream {
-	c.Header("Content-Type", "text/event-stream")
-	c.Header("Cache-Control", "no-cache")
-	c.Header("X-Accel-Buffering", "no")
-	c.Writer.Flush()
-	return &eventStream{c: c}
-}
-
-func (s *eventStream) send(ev chatEvent) {
-	b, err := json.Marshal(ev)
-	if err != nil {
-		slog.Error("api: encode chat event", "error", err)
-		return
-	}
-	s.write(fmt.Sprintf("data: %s\n\n", b))
-}
-
-func (s *eventStream) write(frame string) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	_, _ = io.WriteString(s.c.Writer, frame)
-	s.c.Writer.Flush()
-}
-
-// keepAlive writes an SSE comment frame on an interval until the returned
-// function is called, so a long silence while the model thinks doesn't look like
-// a dead connection to whatever sits in between. SSE ignores comment frames, so
-// this costs the client nothing.
-func (s *eventStream) keepAlive(every time.Duration) func() {
-	done := make(chan struct{})
-	stopped := make(chan struct{})
-	go func() {
-		defer close(stopped)
-		t := time.NewTicker(every)
-		defer t.Stop()
-		for {
-			select {
-			case <-done:
-				return
-			case <-s.c.Request.Context().Done():
-				return
-			case <-t.C:
-				s.write(": keep-alive\n\n")
-			}
-		}
-	}()
-	// Idempotent: the handler stops it explicitly when the run returns and again
-	// via defer, so a panic can't leak the goroutine.
-	var once sync.Once
-	return func() {
-		once.Do(func() { close(done) })
-		<-stopped
-	}
-}
-
 // getAgentHistory returns the actor's thread for a garden.
 func (h *handlers) getAgentHistory(c *gin.Context) {
 	gardenID, ok := parseIDParam(c, "id")

internal/api/agent_holder.go

@@ -1,89 +0,0 @@
-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)
-}

internal/api/agent_test.go

@@ -6,40 +6,24 @@ import (
 	"testing"
 )
 
-// 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) {
+// 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) {
 	r := authEngine(t, localCfg()) // localCfg has no agent configuration
 	cookie := registerAndCookie(t, r, "[email protected]")
 	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.StatusServiceUnavailable {
-		t.Errorf("chat without a key: status %d, want 503", w.Code)
+	if w.Code != http.StatusNotFound {
+		t.Errorf("chat without a key: status %d, want 404", 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.StatusOK {
-		t.Errorf("history without a key: status %d, want 200 (it's data, not the model)", w.Code)
+	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)
 	}
 
 	// And the rest of the app is entirely unaffected.

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,12 +23,9 @@ type handlers struct {
 	cfg  *config.Config
 	svc  *service.Service
 	oidc *oidcClient // nil unless OIDC is configured (see config.OIDCReady)
-	// 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
+	// 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
 }
 
 // New builds the gin engine with the standard middleware stack and registers the
@@ -56,10 +53,12 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
 	// is set; see csrfGuard).
 	v1.Use(h.csrfGuard())
 	v1.GET("/healthz", healthz)
-	// What this instance can actually do, so the UI offers only what works.
-	// Registered after the agent below, because it reports whether the runner
-	// actually built — not merely whether it was configured to.
-	v1.GET("/capabilities", h.capabilities)
+	// What this instance can actually do, so the UI offers only what works. The
+	// agent routes 404 when unconfigured; without this the client would have to
+	// probe for a 404 to find that out, and a dead button is worse than no button.
+	v1.GET("/capabilities", func(c *gin.Context) {
+		c.JSON(http.StatusOK, gin.H{"agent": cfg.Agent.Ready()})
+	})
 
 	// Auth endpoints are exempt from requireAuth (you can't be logged in yet);
 	// /me is the one that needs a session. Feature routers in later issues attach
@@ -129,30 +128,26 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
 	plantings.PATCH("/:id", h.updatePlanting)
 	plantings.DELETE("/:id", h.deletePlanting)
 
-	// 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")
-	}
+	// 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() {
+		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)
-
-	// 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)
+			slog.Info("api: garden assistant enabled", "model", cfg.Agent.Model)
+		}
+	}
 
 	// 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.
@@ -191,17 +186,6 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
 	return r
 }
 
-// capabilities reports what this instance can actually do, so the UI offers only
-// what works.
-//
-// 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.get() != nil})
-}
-
 // healthz is a liveness probe: always returns {"ok": true} when the server is up.
 func healthz(c *gin.Context) {
 	c.JSON(http.StatusOK, gin.H{"ok": true})

internal/api/settings.go

@@ -1,143 +0,0 @@
-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)
-}

internal/api/settings_test.go

@@ -1,208 +0,0 @@
-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, "[email protected]") // first user → admin
-	member := registerAndCookie(t, r, "[email protected]")
-
-	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, "[email protected]")
-
-	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, "[email protected]")
-
-	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, "[email protected]")
-	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, "[email protected]")
-	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, "[email protected]")
-
-	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)
-}

internal/config/config.go

@@ -76,6 +76,13 @@ 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 != ""

internal/domain/domain.go

@@ -242,22 +242,6 @@ 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"`

internal/service/instance_settings.go

@@ -1,110 +0,0 @@
-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
-}

internal/service/instance_settings_test.go

@@ -1,117 +0,0 @@
-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, "[email protected]") // 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, "[email protected]")
-
-	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, "[email protected]")
-
-	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)
-	}
-}

internal/service/ops.go

@@ -28,9 +28,13 @@ type Region struct {
 	MinX, MinY, MaxX, MaxY float64
 }
 
+// contains reports whether a local point lies in the region.
+func (r Region) contains(x, y float64) bool {
+	return x >= r.MinX && x <= r.MaxX && y >= r.MinY && y <= r.MaxY
+}
+
 // clampTo intersects the region with an object's local bounds (±halfW, ±halfH),
-// so a fill can't plant outside the object it was aimed at. A region that misses
-// the object entirely comes back empty — see empty().
+// so an oversized caller-supplied region can't make hexCenters loop forever.
 func (r Region) clampTo(halfW, halfH float64) Region {
 	return Region{
 		MinX: math.Max(r.MinX, -halfW), MinY: math.Max(r.MinY, -halfH),
@@ -38,16 +42,6 @@ func (r Region) clampTo(halfW, halfH float64) Region {
 	}
 }
 
-// empty reports whether the region encloses nothing.
-//
-// This exists because clampTo expresses "no overlap" by INVERTING the region —
-// Max clamps below Min — rather than by zeroing it, which is not something a
-// reader guesses. Naming it once here beats a bare `MaxX < MinX` at each place
-// that has to care.
-func (r Region) empty() bool {
-	return r.MaxX < r.MinX || r.MaxY < r.MinY
-}
-
 // rect builds a rectangular region.
 func rect(minX, minY, maxX, maxY float64) Region {
 	return Region{MinX: minX, MinY: minY, MaxX: maxX, MaxY: maxY}
@@ -100,11 +94,9 @@ func defaultPlopRadius(spacingCM float64) float64 {
 // FillRegion lays a hex-packed field of plops of one plant across a region of a
 // plantable object the actor can edit. Plop radius comes from the plant's spacing
 // (or spacingOverride) via defaultPlopRadius; centers sit on a hex lattice at 2×
-// radius pitch, centered in the region, and set in from each edge by the plop's
-// radius less half a spacing — see hexCenters for why that half-spacing is what
-// the edge is owed. A candidate is skipped when its plop would sit entirely
-// inside an existing active plop (so re-filling doesn't stack duplicates).
-// Returns the plops it created.
+// radius pitch, kept where the center is inside the region. A candidate is
+// skipped when its plop would sit entirely inside an existing active plop (so
+// re-filling doesn't stack duplicates). Returns the plops it created.
 func (s *Service) FillRegion(ctx context.Context, actorID, objectID int64, region Region, plantID int64, spacingOverride *float64) ([]domain.Planting, error) {
 	o, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor)
 	if err != nil {
@@ -114,9 +106,9 @@ func (s *Service) FillRegion(ctx context.Context, actorID, objectID int64, regio
 }
 
 // fillLoaded is the shared body of FillRegion/FillNamedRegion given an object
-// already loaded and authorized (roleEditor). It rejects a non-finite region,
-// clamps the region to the object's bounds, refuses fills over maxFillPlops, and
-// inserts the whole batch in one transaction rather than one round-trip per plop.
+// already loaded and authorized (roleEditor). It clamps the region to the
+// object's bounds, refuses fills over maxFillPlops, and inserts the whole batch
+// in one transaction rather than one round-trip per plop.
 func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.GardenObject, region Region, plantID int64, spacingOverride *float64) ([]domain.Planting, error) {
 	if !o.Plantable {
 		return nil, domain.ErrInvalidInput
@@ -137,21 +129,9 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde
 		return nil, domain.ErrInvalidInput
 	}
 
-	// A caller-supplied region is arbitrary floats, and non-finite ones survive
-	// everything downstream: clamping keeps them, the inverted-region guard can't
-	// see NaN (it compares false both ways), and fitAxis centres on them happily.
-	// Nothing corrupt reaches the table — SQLite stores NaN as NULL and the NOT
-	// NULL constraint refuses it — but the caller gets an opaque store error for
-	// NaN, and for +Inf a silent zero-plop success. Both are lies about what went
-	// wrong; say "bad input" here instead.
-	if !isFinite(region.MinX) || !isFinite(region.MinY) ||
-		!isFinite(region.MaxX) || !isFinite(region.MaxY) {
-		return nil, domain.ErrInvalidInput
-	}
-
 	region = region.clampTo(o.WidthCM/2, o.HeightCM/2)
-	centers, total := hexCenters(region, radius, spacing, maxFillPlops)
-	if total > maxFillPlops {
+	centers := hexCenters(region, radius)
+	if len(centers) > maxFillPlops {
 		return nil, domain.ErrInvalidInput // region too large for this spacing; ask for less
 	}
 
@@ -189,112 +169,32 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde
 
 type localPoint struct{ x, y float64 }
 
-// hexCenters returns hex-packed lattice centers filling a region: rows radius·√3
-// apart, alternate rows offset by half a pitch, at a 2×radius pitch. The lattice
-// is CENTERED, so the leftover is shared between opposite edges instead of piling
-// up against the far one.
-//
-// # How close to the edge the outer row goes
-//
-// Spacing is a constraint BETWEEN NEIGHBOURING PLANTS competing for the same
-// soil, light and water. A bed edge is not a competitor, so the outer row only
-// owes it HALF the spacing — the half it would otherwise share with a neighbour.
-// That is the arithmetic inside every square-foot-gardening chart: 4 per square
-// is 6" apart and 3" from the square's edge; 9 per square is 4" apart and 2"
-// from the edge. Garlic at 9 per square goes in 2" from the frame, not 6".
-//
-// A plop is a CLUMP, not a plant — defaultPlopRadius makes it 1.5×spacing, so
-// three spacings across — and its plants sit out to its rim. So keeping the whole
-// circle inside the bed would inset the outer row by a full 1.5 spacings, three
-// times what the rule allows. Instead the clump may hang over the edge by up to
-// half a spacing, which puts its outermost plants exactly the half-spacing from
-// the edge that the rule asks for. Overhang is capped there and nowhere near the
-// full radius: a clump mostly outside the bed is a drawing of plants in the path.
-//
-// Do not "simplify" this back to anchoring at the region's min corner. That is
-// what #75 was: staggered rows start a full pitch in, and the leftover all lands
-// on the far edge, where clumps hang outside a bed that nothing clips them to.
-//
-// # Counting before building
-//
-// hexCenters returns the total alongside the points, and works that total out
-// BEFORE building anything: a fill large enough to be refused shouldn't allocate
-// its whole lattice first just to be counted and thrown away. Over `limit` it
-// returns (nil, total), so the caller can still refuse with the real number.
-func hexCenters(r Region, radius, spacing float64, limit int) ([]localPoint, int) {
+// hexCenters returns hex-packed lattice centers whose center lies in the region.
+// Rows are spaced radius·√3 apart and every other row is offset by radius, the
+// standard hexagonal packing at a 2×radius pitch. The lattice is anchored one
+// radius inside the region's min corner so the first plop sits inside it.
+func hexCenters(r Region, radius float64) []localPoint {
 	if radius <= 0 {
-		return nil, 0
-	}
-	// An empty region has no inside to plant. The old loop-until-past-MaxX form
-	// got this for free by never entering the loop; counting positions up front
-	// does not, and would site a plop off the bed.
-	if r.empty() {
-		return nil, 0
+		return nil
 	}
 	pitch := 2 * radius
 	rowH := pitch * math.Sqrt(3) / 2
-
-	// How far a clump's centre must stay inside the edge: its own radius, less the
-	// half-spacing of overhang the rule allows. Never negative, and never past the
-	// centre of the clump.
-	inset := math.Max(0, radius-math.Max(0, spacing)/2)
-
-	rows, y0 := fitAxis(r.MaxY-r.MinY, rowH, inset)
-	cols, x0 := fitAxis(r.MaxX-r.MinX, pitch, inset)
-
-	// Exact, not an upper bound: staggered rows hold one fewer, so rows*cols would
-	// over-reserve by ~12% — and, more to the point, allocating it is the thing we
-	// are trying to avoid when the answer is "too many".
-	staggered := cols
-	if cols > 1 {
-		staggered = cols - 1
+	const eps = 1e-6
+	var pts []localPoint
+	row := 0
+	for y := r.MinY + radius; y <= r.MaxY+eps; y += rowH {
+		xStart := r.MinX + radius
+		if row%2 == 1 {
+			xStart += radius
 		}
-	total := (rows+1)/2*cols + rows/2*staggered
-	if total > limit {
-		return nil, total
-	}
-
-	pts := make([]localPoint, 0, total)
-	for row := 0; row < rows; row++ {
-		y := r.MinY + y0 + float64(row)*rowH
-		n, x := cols, r.MinX+x0
-		// The stagger falls out of centering: an offset row holds one fewer plop,
-		// and centering THAT run puts it exactly half a pitch off its neighbours.
-		// A single-column region has nothing to stagger against.
-		if row%2 == 1 && cols > 1 {
-			n, x = staggered, r.MinX+x0+pitch/2
-		}
-		for i := 0; i < n; i++ {
-			pts = append(pts, localPoint{x + float64(i)*pitch, y})
+		for x := xStart; x <= r.MaxX+eps; x += pitch {
+			if r.contains(x, y) {
+				pts = append(pts, localPoint{x, y})
 			}
 		}
-	return pts, total
-}
-
-// fitAxis returns how many lattice positions fit along a span at `step`, keeping
-// at least `inset` from each end, and the offset from the span's start that
-// centers them — so the leftover is split between the two edges rather than all
-// landing on the far one.
-//
-// A span too small to hold even one position at that inset still gets one, in the
-// middle: filling a bed narrower than a single plop with one plop is a better
-// answer than refusing to plant it.
-//
-// The step<=0 half of that guard is currently unreachable — hexCenters, the only
-// caller, returns early unless radius > 0, which makes both steps it passes
-// positive. It stays because dividing by a non-positive step yields ±Inf and then
-// a garbage int conversion, and a helper this small should not require reading
-// its caller to know it is safe. Deliberate, not an oversight.
-func fitAxis(length, step, inset float64) (n int, start float64) {
-	if step <= 0 || length < 2*inset {
-		return 1, length / 2
+		row++
 	}
-	// The epsilon keeps an exact fit from being lost to floating point — a 60cm
-	// span at a 30cm step should give 2 positions, not 1 because the division
-	// landed on 0.9999999.
-	const eps = 1e-9
-	n = int(math.Floor((length-2*inset)/step+eps)) + 1
-	return n, (length - float64(n-1)*step) / 2
+	return pts
 }
 
 // coveredByExisting reports whether a new plop (center, radius) would sit

internal/service/ops_test.go

@@ -3,8 +3,6 @@ package service
 import (
 	"context"
 	"errors"
-	"math"
-	"sort"
 	"testing"
 
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
@@ -73,165 +71,6 @@ func TestDefaultPlopRadius(t *testing.T) {
 	}
 }
 
-// TestHexCentersEdgeInset pins the spacing rule the packing exists to honour:
-// spacing is a constraint between neighbouring plants, so a bed edge — which is
-// nobody's neighbour — is owed half a pitch, not a whole one.
-//
-// The bug this guards against was visible to anyone who filled a bed: staggered
-// rows began a full pitch in, leaving a bare strip a whole plop wide down one
-// side of every other row, while the far edge had plops hanging off it.
-func TestHexCentersEdgeInset(t *testing.T) {
-	for _, tc := range []struct {
-		name                  string
-		w, h, radius, spacing float64
-		wantRowStarts         []float64 // x of the first plop in rows 0 and 1
-	}{
-		// 4ft × 8ft bed, garlic at 15cm spacing → radius 22.5, pitch 45. Three
-		// columns, the outer ones overhanging by 6.5cm — under the 7.5cm the rule
-		// allows. Anchored at the corner this row started at -38.5 and its
-		// staggered neighbour a full 45 further in still.
-		{"4ft bed of garlic", 122, 244, 22.5, 15, []float64{-45, -22.5}},
-		// An exact fit: 90 wide at pitch 30 → 3 columns, no overhang needed.
-		{"exact fit", 90, 90, 15, 10, []float64{-30, -15}},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			r := rect(-tc.w/2, -tc.h/2, tc.w/2, tc.h/2)
-			pts, total := hexCenters(r, tc.radius, tc.spacing, maxFillPlops)
-			if len(pts) == 0 {
-				t.Fatal("no centers")
-			}
-			// The count is derived up front so an oversized fill is refused without
-			// building its lattice — which only works if it matches what gets built.
-			if total != len(pts) {
-				t.Errorf("reported total %d, built %d", total, len(pts))
-			}
-
-			// A clump may cross the edge, but only by the half-spacing the rule
-			// allows — never enough to be mostly out in the path.
-			budget := tc.spacing / 2
-			for _, p := range pts {
-				over := math.Max(
-					math.Max(r.MinX-(p.x-tc.radius), (p.x+tc.radius)-r.MaxX),
-					math.Max(r.MinY-(p.y-tc.radius), (p.y+tc.radius)-r.MaxY),
-				)
-				if over > budget+1e-6 {
-					t.Errorf("plop at (%.1f,%.1f) overhangs by %.2f, budget %.2f", p.x, p.y, over, budget)
-				}
-			}
-
-			// The margins match on opposite edges: the leftover is shared, not piled
-			// against the far side.
-			minX, maxX, minY, maxY := pts[0].x, pts[0].x, pts[0].y, pts[0].y
-			for _, p := range pts {
-				minX, maxX = math.Min(minX, p.x), math.Max(maxX, p.x)
-				minY, maxY = math.Min(minY, p.y), math.Max(maxY, p.y)
-			}
-			if w, e := minX-r.MinX, r.MaxX-maxX; math.Abs(w-e) > 1e-6 {
-				t.Errorf("lopsided horizontally: west margin %.2f, east %.2f", w, e)
-			}
-			if n, s := minY-r.MinY, r.MaxY-maxY; math.Abs(n-s) > 1e-6 {
-				t.Errorf("lopsided vertically: north margin %.2f, south %.2f", n, s)
-			}
-
-			// The staggered row is offset by HALF a pitch, not a whole one.
-			starts := map[float64]float64{}
-			for _, p := range pts {
-				if x, ok := starts[p.y]; !ok || p.x < x {
-					starts[p.y] = p.x
-				}
-			}
-			ys := make([]float64, 0, len(starts))
-			for y := range starts {
-				ys = append(ys, y)
-			}
-			sort.Float64s(ys)
-			for i, want := range tc.wantRowStarts {
-				if i >= len(ys) {
-					t.Fatalf("only %d rows, want at least %d", len(ys), len(tc.wantRowStarts))
-				}
-				if got := starts[ys[i]]; math.Abs(got-want) > 1e-6 {
-					t.Errorf("row %d starts at x=%.2f, want %.2f", i, got, want)
-				}
-			}
-		})
-	}
-}
-
-// TestHexCentersTinyRegion covers a region too small to hold a plop at the
-// half-pitch inset: planting one in the middle beats refusing to plant at all.
-//
-// The off-centre case earns its place — a region symmetric about the origin
-// can't tell "the middle of the region" from "the origin", so on its own it
-// would pass for an implementation that just returned (0,0).
-func TestHexCentersTinyRegion(t *testing.T) {
-	for _, tc := range []struct {
-		name         string
-		r            Region
-		wantX, wantY float64
-	}{
-		{"centred on the origin", rect(-5, -5, 5, 5), 0, 0},
-		{"off in a corner", rect(20, -40, 30, -30), 25, -35},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			pts, _ := hexCenters(tc.r, 15, 10, maxFillPlops)
-			if len(pts) != 1 || pts[0].x != tc.wantX || pts[0].y != tc.wantY {
-				t.Errorf("got %+v, want one plop at (%v,%v)", pts, tc.wantX, tc.wantY)
-			}
-		})
-	}
-}
-
-// TestFillRegionRejectsNonFiniteRegion: non-finite bounds survive clamping and
-// the inverted-region guard (NaN compares false both ways). Without the explicit
-// check, NaN surfaced as a raw store error ("NOT NULL constraint failed") and
-// +Inf as a silent success that planted nothing — neither of which tells the
-// caller what it actually did wrong.
-func TestFillRegionRejectsNonFiniteRegion(t *testing.T) {
-	ctx := context.Background()
-	s := newTestService(t, openConfig())
-	owner := seedUser(t, s, "[email protected]")
-	g, _ := s.CreateGarden(ctx, owner, GardenInput{Name: "Big", WidthCM: 2000, HeightCM: 2000})
-	bed := seedFillBed(t, s, owner, g.ID, 100, 100)
-	plant := seedOwnPlant(t, s, owner, 10)
-
-	nan := math.NaN()
-	for _, r := range []Region{
-		{MinX: nan, MinY: -50, MaxX: 50, MaxY: 50},
-		{MinX: -50, MinY: -50, MaxX: 50, MaxY: math.Inf(1)},
-	} {
-		created, err := s.FillRegion(ctx, owner, bed.ID, r, plant.ID, nil)
-		if !errors.Is(err, domain.ErrInvalidInput) {
-			t.Errorf("FillRegion(%+v) err = %v, want ErrInvalidInput", r, err)
-		}
-		for _, p := range created {
-			if !isFinite(p.XCM) || !isFinite(p.YCM) {
-				t.Errorf("persisted a plop with non-finite coordinates: %+v", p)
-			}
-		}
-	}
-}
-
-// TestFillRegionOutsideObjectPlantsNothing covers a region that misses the object
-// entirely. clampTo inverts such a region rather than emptying it, and an
-// inverted region must plant nothing — not one plop at some point off the bed.
-func TestFillRegionOutsideObjectPlantsNothing(t *testing.T) {
-	ctx := context.Background()
-	s := newTestService(t, openConfig())
-	owner := seedUser(t, s, "[email protected]")
-	g, _ := s.CreateGarden(ctx, owner, GardenInput{Name: "Big", WidthCM: 2000, HeightCM: 2000})
-	bed := seedFillBed(t, s, owner, g.ID, 100, 100) // local bounds ±50
-	plant := seedOwnPlant(t, s, owner, 10)
-
-	// Wholly east of the bed: clampTo gives MinX=500, MaxX=50.
-	created, err := s.FillRegion(ctx, owner, bed.ID, rect(500, -50, 600, 50), plant.ID, nil)
-	if err != nil {
-		t.Fatalf("FillRegion: %v", err)
-	}
-	if len(created) != 0 {
-		t.Errorf("filled %d plops for a region outside the bed, want 0: %+v", len(created), created)
-	}
-}
-
 // seedFillBed makes a plantable bed of the given size centered in a big garden.
 func seedFillBed(t *testing.T, s *Service, owner, gardenID int64, w, h float64) *domain.GardenObject {
 	t.Helper()
@@ -260,24 +99,16 @@ func TestFillRegionDeterministicPacking(t *testing.T) {
 	if err != nil {
 		t.Fatalf("FillRegion: %v", err)
 	}
-	// Hex lattice on [-30,30]² at pitch 30, rows ~26 apart, centered: a row of 2
-	// (x=±15), then a staggered row of 1 (x=0) → 3 plops.
-	//
-	// This was 4 while the lattice was anchored at the min corner, and the fourth
-	// sat at x=30 — centred ON the east edge, so half of it lay outside the bed,
-	// well past the half-spacing (5cm here) the rule allows. Packing one fewer
-	// plop is the point of the fix, not a regression in it.
-	if len(created) != 3 {
-		t.Fatalf("filled %d plops, want 3 (60×60 bed, radius 15)", len(created))
+	// Hex lattice on [-30,30]² at pitch 30, rows ~26 apart → 4 plops (2 rows × 2).
+	if len(created) != 4 {
+		t.Fatalf("filled %d plops, want 4 (60×60 bed, radius 15)", len(created))
 	}
 	for _, p := range created {
 		if p.RadiusCM != 15 || p.PlantedAt == nil || p.DerivedCount < 1 {
 			t.Errorf("unexpected created plop: %+v", p)
 		}
-		// This bed fits its lattice exactly, so nothing should need to overhang.
-		if p.XCM-p.RadiusCM < -30 || p.XCM+p.RadiusCM > 30 ||
-			p.YCM-p.RadiusCM < -30 || p.YCM+p.RadiusCM > 30 {
-			t.Errorf("plop overhangs a bed it fits inside: %+v", p)
+		if p.XCM < -30 || p.XCM > 30 || p.YCM < -30 || p.YCM > 30 {
+			t.Errorf("plop center out of bed bounds: %+v", p)
 		}
 	}
 

internal/service/revisions.go

@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
-	"sort"
 	"sync"
 
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
@@ -100,8 +99,7 @@ type ChangeSetOptions struct {
 	Source string
 	// Summary is the one-line description shown in the history list.
 	Summary string
-	// AgentRunID joins this change set back to the agent run that produced it,
-	// so a change in the history list can be correlated with the run's log lines.
+	// AgentRunID joins this change set back to the executus run that produced it.
 	AgentRunID *string
 }
 
@@ -150,19 +148,11 @@ func partialSummary(summary string) string {
 // commitScope writes a scope's buffered revisions as one change set. revertsID is
 // set only by RevertChangeSet. A scope with no revisions writes nothing — an
 // operation that changed nothing doesn't belong in history.
-//
-// The write is DETACHED FROM CANCELLATION, always, and that belongs here rather
-// than at each call site so no caller can be the one that forgets. By the time a
-// commit runs, the data changes it describes have already been written — so
-// cancelling it cannot undo anything. It can only lose the record of what
-// happened and leave real changes with no way to undo them, which is the one
-// thing the whole change-set design exists to prevent.
 func (s *Service) commitScope(ctx context.Context, sc *changeScope, revertsID *int64) (*domain.ChangeSet, error) {
 	revs := sc.taken()
 	if len(revs) == 0 {
 		return nil, nil
 	}
-	ctx = context.WithoutCancel(ctx)
 	return s.store.WriteChangeSet(ctx, &domain.ChangeSet{
 		GardenID:   sc.gardenID,
 		ActorID:    sc.actorID,
@@ -203,13 +193,9 @@ func (s *Service) record(ctx context.Context, gardenID, actorID int64, summary s
 		sc.append(revs)
 		return
 	}
-	// Auto-scope: one operation, its own change set. Written through the same
-	// detached path as everything else — a REST client that hangs up right after
-	// its PATCH landed must not leave that change without history, and this is
-	// the path virtually every mutation takes.
-	auto := &changeScope{gardenID: gardenID, actorID: actorID, source: domain.SourceUI, summary: summary}
-	auto.append(revs)
-	if _, err := s.commitScope(ctx, auto, nil); err != nil {
+	if _, err := s.store.WriteChangeSet(ctx, &domain.ChangeSet{
+		GardenID: gardenID, ActorID: actorID, Source: domain.SourceUI, Summary: summary,
+	}, revs); err != nil {
 		slog.Error("service: record change set", "error", err, "garden", gardenID, "summary", summary)
 	}
 }
@@ -321,7 +307,6 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6
 		if err != nil {
 			// Record what actually landed before surfacing the failure, so the
 			// partial revert is visible and undoable rather than orphaned.
-			// (commitScope detaches from cancellation itself.)
 			if _, cerr := s.commitScope(ctx, sc, &target.ID); cerr != nil {
 				slog.Error("service: partial revert could not be recorded", "error", cerr, "changeSet", changeSetID)
 			}
@@ -342,48 +327,9 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6
 	if err != nil {
 		return nil, nil, err
 	}
-	// Fill in the per-op counts. commitScope returns the freshly inserted row,
-	// which carries no tally — and a caller receiving a change set with an empty
-	// Counts cannot tell "nothing was reverted" from "the tally wasn't loaded".
-	// That ambiguity is not theoretical: it made the chat panel report a
-	// successful undo as "nothing left to undo".
-	if cs != nil {
-		cs.Counts = countRevisions(sc.taken())
-	}
 	return cs, conflicts, nil
 }
 
-// countRevisions tallies revisions by (entity type, op).
-//
-// This deliberately reproduces in Go what ListChangeSets does in SQL, because a
-// change set that has just been written has no rows to GROUP BY yet — the
-// alternative is a second round trip to count what we are already holding.
-// The parity is a real cross-layer contract, and TestRevertResultCarriesItsCounts
-// compares the two breakdowns row for row so it can't drift silently.
-func countRevisions(revs []domain.Revision) []domain.ChangeCount {
-	type key struct{ entityType, op string }
-	seen := map[key]int{}
-	order := []key{}
-	for _, r := range revs {
-		k := key{r.EntityType, r.Op}
-		if _, ok := seen[k]; !ok {
-			order = append(order, k)
-		}
-		seen[k]++
-	}
-	sort.Slice(order, func(i, j int) bool {
-		if order[i].entityType != order[j].entityType {
-			return order[i].entityType < order[j].entityType
-		}
-		return order[i].op < order[j].op
-	})
-	counts := make([]domain.ChangeCount, 0, len(order))
-	for _, k := range order {
-		counts = append(counts, domain.ChangeCount{EntityType: k.entityType, Op: k.op, N: seen[k]})
-	}
-	return counts
-}
-
 // entityKey identifies a row across the entity types a revision can name.
 type entityKey struct {
 	entityType string

internal/service/revisions_test.go

@@ -704,192 +704,3 @@ func TestClearObjectOnlyClearsWhatItSnapshotted(t *testing.T) {
 		t.Errorf("cleared %d of %d with %d revisions — all three should match", n, len(before), len(revs))
 	}
 }
-
-// TestRevertResultCarriesItsCounts — found by using the thing.
-//
-// commitScope returns the freshly inserted row, which carries no tally. A caller
-// receiving a change set with empty Counts cannot tell "nothing was reverted"
-// from "the tally wasn't loaded", and the chat panel read that ambiguity the
-// wrong way: a successful undo reported "nothing left to undo", which is the
-// worst possible thing to say about an action that just worked.
-//
-// It also pins the cross-layer contract that fix created. countRevisions tallies
-// in Go what ListChangeSets tallies in SQL, and nothing but this test stops the
-// two drifting — so it compares the FULL per-(entity, op) breakdown, not just
-// the totals, which would agree even if the groupings had diverged.
-func TestRevertResultCarriesItsCounts(t *testing.T) {
-	s := newTestService(t, openConfig())
-	owner := seedUser(t, s, "[email protected]")
-	g := seedGarden(t, s, owner)
-	bed := seedBed(t, s, owner, g.ID)
-	plant := seedOwnPlant(t, s, owner, 15)
-	ctx := context.Background()
-
-	if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil {
-		t.Fatalf("fill: %v", err)
-	}
-	// A second, different kind of change, so the breakdown has more than one row
-	// to get wrong.
-	if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{Name: strPtr("North Bed")}, bed.Version); err != nil {
-		t.Fatalf("rename: %v", err)
-	}
-	sets := history(t, s, owner, g.ID)
-	rename, fill := sets[0], sets[1]
-
-	undoRename, conflicts, err := s.RevertChangeSet(ctx, owner, rename.ID, domain.SourceUI)
-	if err != nil || len(conflicts) != 0 {
-		t.Fatalf("revert rename: err=%v conflicts=%+v", err, conflicts)
-	}
-	undoFill, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI)
-	if err != nil || len(conflicts) != 0 {
-		t.Fatalf("revert fill: err=%v conflicts=%+v", err, conflicts)
-	}
-
-	for _, undo := range []*domain.ChangeSet{undoRename, undoFill} {
-		if undo == nil {
-			t.Fatal("a revert that did work returned no change set")
-		}
-		if len(undo.Counts) == 0 {
-			t.Fatalf("change set %d came back with no tally — an empty tally reads as 'nothing happened'", undo.ID)
-		}
-	}
-	// Undoing a fill deletes every plop it created, so the tallies must match.
-	if got, want := countsTotal(undoFill.Counts), countsTotal(fill.Counts); got != want {
-		t.Errorf("undo of the fill reports %d changes, want %d", got, want)
-	}
-
-	// The SQL tally and the Go tally must agree, row for row.
-	listed := history(t, s, owner, g.ID)
-	byID := map[int64][]domain.ChangeCount{}
-	for _, cs := range listed {
-		byID[cs.ID] = cs.Counts
-	}
-	for _, undo := range []*domain.ChangeSet{undoRename, undoFill} {
-		fromSQL, ok := byID[undo.ID]
-		if !ok {
-			t.Fatalf("revert %d is missing from the history list", undo.ID)
-		}
-		if !sameCounts(undo.Counts, fromSQL) {
-			t.Errorf("change set %d: revert returned %+v, the list read says %+v — countRevisions and the SQL grouping have drifted",
-				undo.ID, undo.Counts, fromSQL)
-		}
-	}
-}
-
-// sameCounts compares two tallies regardless of order.
-func sameCounts(a, b []domain.ChangeCount) bool {
-	if len(a) != len(b) {
-		return false
-	}
-	index := func(cs []domain.ChangeCount) map[string]int {
-		m := map[string]int{}
-		for _, c := range cs {
-			m[c.EntityType+"/"+c.Op] = c.N
-		}
-		return m
-	}
-	ia, ib := index(a), index(b)
-	for k, v := range ia {
-		if ib[k] != v {
-			return false
-		}
-	}
-	return true
-}
-
-// countsTotal sums a tally — the server-side twin of the client's totalChanges.
-func countsTotal(counts []domain.ChangeCount) int {
-	n := 0
-	for _, c := range counts {
-		n += c.N
-	}
-	return n
-}
-
-// TestSucceededTurnRecordsEvenIfTheCallerWentAway is the production bug.
-//
-// The failure path was detached from cancellation; the SUCCESS path was not. An
-// agent turn whose client disconnects can still COMPLETE — and then the success
-// path committed with a dead context, the write failed, and real changes were
-// left with no change set and no way to undo them. Found live with 18 plantings
-// behind no history at all.
-//
-// Nothing about a commit needs the caller to still be there: by the time it
-// runs, the data it describes has already been written.
-func TestSucceededTurnRecordsEvenIfTheCallerWentAway(t *testing.T) {
-	s := newTestService(t, openConfig())
-	owner := seedUser(t, s, "[email protected]")
-	g := seedGarden(t, s, owner)
-	bed := seedBed(t, s, owner, g.ID)
-	plant := seedOwnPlant(t, s, owner, 15)
-
-	ctx, cancel := context.WithCancel(context.Background())
-	before := len(history(t, s, owner, g.ID))
-
-	// fn does its work and SUCCEEDS, but the caller goes away before it returns.
-	cs, err := s.WithChangeSet(ctx, owner, g.ID, ChangeSetOptions{
-		Source: domain.SourceAgent, Summary: "plant beans in the second bed",
-	}, func(ctx context.Context) error {
-		if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil {
-			return err
-		}
-		cancel() // the client disconnects, mid-turn, after the work landed
-		return nil
-	})
-	if err != nil {
-		t.Fatalf("WithChangeSet: %v", err)
-	}
-	if cs == nil {
-		t.Fatal("the turn changed things but produced no change set")
-	}
-
-	after := history(t, s, owner, g.ID)
-	if len(after) != before+1 {
-		t.Fatalf("recorded %d change sets, want 1", len(after)-before)
-	}
-	if after[0].Summary != "plant beans in the second bed" {
-		t.Errorf("summary = %q; a completed turn shouldn't be marked partial", after[0].Summary)
-	}
-	// And it's undoable, which is the entire point.
-	if _, conflicts, err := s.RevertChangeSet(context.Background(), owner, cs.ID, domain.SourceUI); err != nil || len(conflicts) != 0 {
-		t.Fatalf("the recorded turn should be undoable: err=%v conflicts=%+v", err, conflicts)
-	}
-	active, _ := s.store.ListActivePlantingsForObject(context.Background(), bed.ID)
-	if len(active) != 0 {
-		t.Errorf("%d plantings survived the undo", len(active))
-	}
-}
-
-// TestAutoScopedMutationRecordsEvenIfTheCallerWentAway — the same rule for the
-// path virtually every mutation takes.
-//
-// A plain REST PATCH auto-scopes into its own change set. If the client hangs up
-// between the row landing and the change set being written, that change is
-// orphaned exactly as an agent turn's was — and this path is used far more.
-func TestAutoScopedMutationRecordsEvenIfTheCallerWentAway(t *testing.T) {
-	s := newTestService(t, openConfig())
-	owner := seedUser(t, s, "[email protected]")
-	g := seedGarden(t, s, owner)
-	bed := seedBed(t, s, owner, g.ID)
-	before := len(history(t, s, owner, g.ID))
-
-	// The row write and the history write share this context; cancelling after
-	// the mutation returns is the client hanging up mid-request.
-	ctx, cancel := context.WithCancel(context.Background())
-	if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{XCM: f64Ptr(300)}, bed.Version); err != nil {
-		t.Fatalf("UpdateObject: %v", err)
-	}
-	cancel()
-
-	after := history(t, s, owner, g.ID)
-	if len(after) != before+1 {
-		t.Fatalf("recorded %d change sets, want 1 — the move is otherwise un-undoable", len(after)-before)
-	}
-	if _, conflicts, err := s.RevertChangeSet(context.Background(), owner, after[0].ID, domain.SourceUI); err != nil || len(conflicts) != 0 {
-		t.Fatalf("the recorded move should be undoable: err=%v conflicts=%+v", err, conflicts)
-	}
-	back, _ := s.store.GetObject(context.Background(), bed.ID)
-	if back.XCM != bed.XCM {
-		t.Errorf("undo left x at %v, want %v", back.XCM, bed.XCM)
-	}
-}

internal/store/instance_settings.go

@@ -1,81 +0,0 @@
-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
-}

internal/store/migrations/0010_instance_settings.sql

@@ -1,35 +0,0 @@
--- 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);

internal/store/revisions.go

@@ -119,9 +119,7 @@ func (d *DB) ListChangeSets(ctx context.Context, gardenID int64, limit, offset i
 		return sets, nil
 	}
 
-	// One grouped query for the whole page rather than N per-row counts. The
-	// service's countRevisions mirrors this grouping for change sets it has just
-	// written; TestRevertResultCarriesItsCounts holds the two in step.
+	// One grouped query for the whole page rather than N per-row counts.
 	countRows, err := d.sql.QueryContext(ctx,
 		`SELECT change_set_id, entity_type, op, COUNT(*)
 		   FROM revisions

web/src/components/layout/AppShell.tsx

@@ -53,19 +53,6 @@ export function AppShell() {
                   {l.label}
                 </Link>
               ))}
-            {/* 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 && (
-              <Link
-                to="/settings"
-                className={navLinkBase}
-                activeProps={{ className: navLinkActive }}
-                inactiveProps={{ className: navLinkInactive }}
-              >
-                Settings
-              </Link>
-            )}
           </div>
 
           {user ? (

web/src/editor/ChatPanel.tsx

@@ -1,6 +1,5 @@
-import { useEffect, useRef, useState, type ReactNode } from 'react'
+import { useEffect, useRef, useState } from 'react'
 import { Alert } from '@/components/ui/Alert'
-import { errorMessage } from '@/lib/api'
 import { Button } from '@/components/ui/Button'
 import { TextArea } from '@/components/ui/TextArea'
 import { cn } from '@/lib/cn'
@@ -34,17 +33,12 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
   // The turn in flight: what we sent, the steps so far, and how it ended.
   const [pending, setPending] = useState<{ message: string; steps: AgentStep[] } | null>(null)
   const [error, setError] = useState<string | null>(null)
-  const [warning, setWarning] = useState<string | null>(null)
   const abort = useRef<AbortController | null>(null)
   const bottom = useRef<HTMLDivElement>(null)
 
-  // Deliberately NOT aborted on unmount. Selecting an object auto-switches the
-  // rail to the inspector, so aborting here would mean clicking the canvas
-  // mid-turn silently killed the turn — and the canvas is exactly what you're
-  // meant to be watching. The request continues, the exchange is persisted
-  // server-side, and coming back to this tab shows it. Only Stop aborts, and
-  // even that only stops us READING: the turn keeps running server-side, which
-  // is why its work still lands in History either way.
+  // Abort an in-flight turn when the panel goes away, so a closed tab doesn't
+  // leave a read hanging.
+  useEffect(() => () => abort.current?.abort(), [])
 
   // Follow the conversation as it grows, including mid-turn as steps arrive.
   useEffect(() => {
@@ -56,7 +50,6 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
     if (!message || pending) return
     setInput('')
     setError(null)
-    setWarning(null)
     setPending({ message, steps: [] })
 
     const controller = new AbortController()
@@ -68,24 +61,22 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
       {
         onStep: (step) => {
           setPending((p) => (p ? { ...p, steps: [...p.steps, step] } : p))
-          // The canvas updating under the conversation is the whole point of
-          // putting the chat here, so refresh it as each step lands — but only
-          // it: nothing else can have changed until the turn commits.
-          refresh.canvas()
+          // Refresh as each step lands, not just at the end: the canvas updating
+          // under the conversation is the whole point of putting the chat here.
+          refresh()
         },
         onDone: (turn: AgentTurn) => {
           setPending(null)
-          refresh.everything()
+          refresh()
           if (turn.truncated) {
             setError('That turned into more steps than I should take at once — check what changed before continuing.')
           }
         },
-        onWarning: setWarning,
         onError: (message) => {
           setPending(null)
           setError(message)
           // Something may still have landed before it failed.
-          refresh.everything()
+          refresh()
         },
       },
       controller.signal,
@@ -101,11 +92,7 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
         {messages.length > 0 && (
           <button
             type="button"
-            onClick={() =>
-              clear.mutate(undefined, {
-                onError: (err) => setError(errorMessage(err, "Couldn't clear the conversation.")),
-              })
-            }
+            onClick={() => clear.mutate()}
             disabled={clear.isPending}
             className="rounded px-1.5 py-0.5 text-xs text-muted outline-none hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40"
           >
@@ -121,13 +108,6 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
       )}
 
       <div className="flex min-h-0 flex-1 flex-col gap-2 overflow-y-auto">
-        {history.isPending && <p className="text-sm text-muted">Loading the conversation…</p>}
-        {/* A failed load rendering as an empty thread would look like the
-            conversation had been lost, which is a much worse thing to believe. */}
-        {history.isError && (
-          <Alert>{errorMessage(history.error, "Couldn't load the conversation.")}</Alert>
-        )}
-
         {history.isSuccess && messages.length === 0 && !pending && (
           <p className="text-sm text-muted">
             Ask for what you want and it'll do it — “change the garlic bed to cucumbers this year”, “fill the
@@ -166,7 +146,6 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
           </>
         )}
 
-        {warning && <Alert tone="info">{warning}</Alert>}
         {error && <Alert>{error}</Alert>}
         <div ref={bottom} />
       </div>
@@ -198,7 +177,7 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
               onClick={() => {
                 abort.current?.abort()
                 setPending(null)
-                refresh.everything()
+                refresh()
               }}
             >
               Stop
@@ -220,7 +199,7 @@ function Bubble({
 }: {
   role: 'user' | 'assistant'
   body: string
-  children?: ReactNode
+  children?: React.ReactNode
 }) {
   const mine = role === 'user'
   return (

web/src/editor/HistoryPanel.tsx

@@ -116,7 +116,7 @@ function HistoryEntry({
           <UndoButton
             changeSet={changeSet}
             undo={undo}
-            className="w-32 shrink-0 text-right"
+            className="w-32 shrink-0"
             label={reverted ? 'Undo again' : 'Undo'}
           />
         )}

web/src/editor/UndoButton.tsx

@@ -43,12 +43,8 @@ const TONE_CLASS: Record<Exclude<UndoOutcome['tone'], 'pending'>, string> = {
 
 function OutcomeNote({ outcome }: { outcome: UndoOutcome }) {
   if (outcome.tone === 'pending') return null
-  // No text alignment of its own: the container decides. The history list stacks
-  // this to the right of an entry, the chat panel puts it under a left-aligned
-  // message, and a hard-coded text-right made the chat's copy read against its
-  // own column.
   return (
-    <p role="status" className={cn('text-xs', TONE_CLASS[outcome.tone])}>
+    <p role="status" className={cn('text-right text-xs', TONE_CLASS[outcome.tone])}>
       {outcome.message}
     </p>
   )

web/src/lib/agent.ts

@@ -13,21 +13,13 @@ import { historyKey } from './history'
 
 const capabilitiesSchema = z.object({ agent: z.boolean() })
 
-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. */
+/** 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 function useCapabilities() {
   return useQuery({
-    queryKey: capabilitiesKey,
+    queryKey: ['capabilities'] as const,
     queryFn: async () => capabilitiesSchema.parse(await api.get('/capabilities')),
-    staleTime: 60_000,
+    staleTime: Infinity, // server config; it doesn't change under a running page
   })
 }
 
@@ -105,29 +97,10 @@ export function describeStep(step: AgentStep): string {
   return [...new Set(labels)].join(', ')
 }
 
-const chatEventSchema = z.object({
-  step: z.object({ index: z.number(), tools: z.array(z.string()) }).optional(),
-  done: z
-    .object({
-      reply: z.string(),
-      changeSetId: z.number().optional(),
-      steps: z.number(),
-      truncated: z.boolean().optional(),
-    })
-    .optional(),
-  error: z.string().optional(),
-  // The turn worked but something adjacent to it didn't — currently, the
-  // exchange couldn't be saved. Dropping this on the floor would recreate
-  // exactly the silent swallow the server added it to avoid.
-  warning: z.string().optional(),
-})
-
 export interface StreamHandlers {
   onStep: (step: AgentStep) => void
   onDone: (turn: AgentTurn) => void
   onError: (message: string) => void
-  /** The turn succeeded, but something alongside it didn't. */
-  onWarning?: (message: string) => void
 }
 
 /**
@@ -153,21 +126,12 @@ export async function streamChat(
       signal,
     })
   } catch {
-    // An abort here is the caller's own doing — Stop, or navigating away — not a
-    // failure to report back to them. The read loop below already knew this; the
-    // request path did not.
-    if (signal?.aborted) return
     handlers.onError('Could not reach the server.')
     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 === 503
-        ? "The assistant isn't enabled on this instance."
-        : res.status === 404
+      res.status === 404
         ? "This instance doesn't have the assistant configured."
         : 'The assistant is not available right now.',
     )
@@ -198,12 +162,13 @@ export async function streamChat(
     for (const frame of frames) {
       const line = frame.split('\n').find((l) => l.startsWith('data:'))
       if (!line) continue
-      // Parsed AND validated: a malformed or unexpected frame shouldn't kill a
-      // working stream, and shouldn't be trusted into the UI either.
-      const parsed = chatEventSchema.safeParse(safeJson(line.slice(5).trim()))
-      if (!parsed.success) continue
-      const e = parsed.data
-      if (e.warning) handlers.onWarning?.(e.warning)
+      let event: unknown
+      try {
+        event = JSON.parse(line.slice(5).trim())
+      } catch {
+        continue // a malformed frame shouldn't kill a working stream
+      }
+      const e = event as { step?: AgentStep; done?: AgentTurn; error?: string }
       if (e.error) handlers.onError(e.error)
       else if (e.step) handlers.onStep(e.step)
       else if (e.done) handlers.onDone(e.done)
@@ -211,35 +176,13 @@ export async function streamChat(
   }
 }
 
-function safeJson(raw: string): unknown {
-  try {
-    return JSON.parse(raw)
-  } catch {
-    return null
-  }
-}
-
-/**
- * Refreshes for the two moments that need different amounts of work.
- *
- * Mid-turn, only the canvas can have changed: the change set isn't written until
- * the turn commits, and the exchange isn't stored until it finishes. Refetching
- * those on every step would be up to 2×(N−1) requests per turn for data that
- * cannot have moved.
- */
+/** Refresh everything a turn may have changed: the canvas, the history list,
+ *  and the conversation itself. */
 export function useAgentRefresh(gardenId: number) {
   const qc = useQueryClient()
-  const canvas = () => {
+  return () => {
     void qc.invalidateQueries({ queryKey: gardenFullKey(gardenId) })
-  }
-  return {
-    /** After a step: the garden may have changed under the conversation. */
-    canvas,
-    /** After a turn: the change set and the stored exchange exist now too. */
-    everything: () => {
-      canvas()
     void qc.invalidateQueries({ queryKey: historyKey(gardenId) })
     void qc.invalidateQueries({ queryKey: agentHistoryKey(gardenId) })
-    },
   }
 }

web/src/lib/history.test.ts

@@ -49,14 +49,6 @@ describe('describeConflict', () => {
 describe('describeUndo', () => {
   const target = changeSet({ counts: [{ entityType: 'object', op: 'update', n: 3 }] })
 
-  // The bug this pair exists for: a real revert response carries a change set
-  // whose counts the server didn't populate. Reading that as "nothing happened"
-  // told the user a successful undo had done nothing.
-  it('trusts the change set, not the tally, for whether anything happened', () => {
-    const out = describeUndo({ id: 5 }, { changeSet: changeSet({ id: 6, counts: [] }), conflicts: [] })
-    expect(out).toEqual({ tone: 'ok', message: 'Undone.' })
-  })
-
   it('is a plain success when nothing conflicted', () => {
     const out = describeUndo(target, {
       changeSet: changeSet({ id: 2, counts: [{ entityType: 'object', op: 'update', n: 3 }] }),
@@ -69,7 +61,6 @@ describe('describeUndo', () => {
   // already a no-op — reachable by undoing a creation whose object is gone.
   // Claiming "Undone." there reports work that didn't happen.
   it("doesn't claim to have undone a no-op", () => {
-    // A NULL change set — not an empty tally — is the server's no-op signal.
     const out = describeUndo(target, { changeSet: null, conflicts: [] })
     expect(out.tone).toBe('ok')
     expect(out.message).toBe('Nothing left to undo — this was already reversed.')

web/src/lib/history.ts

@@ -113,10 +113,9 @@ export function useRevertChangeSet(gardenId: number) {
   })
 }
 
-/** How many rows a change set touched, for "3 changes" in the list. Undefined
- *  counts total zero, which callers read as "no denominator to quote". */
-export function totalChanges(cs: { counts?: ChangeCount[] }): number {
-  return (cs.counts ?? []).reduce((sum, c) => sum + c.n, 0)
+/** How many rows a change set touched, for "3 changes" in the list. */
+export function totalChanges(cs: ChangeSet): number {
+  return cs.counts.reduce((sum, c) => sum + c.n, 0)
 }
 
 const ENTITY_NOUNS: Record<ChangeCount['entityType'], [string, string]> = {
@@ -224,23 +223,20 @@ export interface UndoOutcome {
  */
 export function describeUndo(target: UndoTarget, result: RevertResult): UndoOutcome {
   const skipped = result.conflicts.map(describeConflict).join('; ')
-  // A NULL change set is the server's signal that nothing needed doing. An empty
-  // `counts` is not the same thing and must not be read as one — that conflation
-  // made a successful undo report "nothing left to undo", which is the worst
-  // possible thing to tell someone about an action that just worked.
-  const didSomething = result.changeSet != null
   const applied = result.changeSet ? totalChanges(result.changeSet) : 0
   if (result.conflicts.length === 0) {
-    // Reachable by undoing a creation whose object is already gone.
-    if (!didSomething) return { tone: 'ok', message: 'Nothing left to undo — this was already reversed.' }
+    // The server answers 200 with a null change set when every revision was
+    // already a no-op — reachable by undoing a creation whose object is gone.
+    // Claiming "Undone." there would be reporting work that didn't happen.
+    if (applied === 0) return { tone: 'ok', message: 'Nothing left to undo — this was already reversed.' }
     return { tone: 'ok', message: 'Undone.' }
   }
-  if (!didSomething) {
+  if (applied === 0) {
     return { tone: 'error', message: `Nothing was undone — ${skipped}.` }
   }
   // Only claim a denominator when we have one. "2 of 3" from a caller that
   // never knew the total would be a number invented to fill a sentence.
-  const total = totalChanges(target)
-  const scale = total > 0 && applied > 0 ? `${applied} of ${total} changes undone` : 'Partly undone'
+  const total = target.counts ? target.counts.reduce((sum, c) => sum + c.n, 0) : 0
+  const scale = total > 0 ? `${applied} of ${total} changes undone` : 'Partly undone'
   return { tone: 'partial', message: `${scale} — ${skipped}.` }
 }

web/src/lib/settings.ts

@@ -1,79 +0,0 @@
-// 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<typeof instanceSettingsSchema>
-
-export const effectiveAgentSchema = z.object({
-  model: z.string(),
-  enabled: z.boolean(),
-  hasApiKey: z.boolean(),
-  agentLive: z.boolean(),
-})
-export type EffectiveAgent = z.infer<typeof effectiveAgentSchema>
-
-export const settingsResponseSchema = z.object({
-  settings: instanceSettingsSchema,
-  effective: effectiveAgentSchema,
-})
-export type SettingsResponse = z.infer<typeof settingsResponseSchema>
-
-export const settingsKey = ['settings'] as const
-
-export const settingsQueryOptions = queryOptions({
-  queryKey: settingsKey,
-  queryFn: async (): Promise<SettingsResponse> =>
-    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<SettingsResponse> =>
-      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
-}

web/src/pages/SettingsPage.tsx

@@ -1,164 +0,0 @@
-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 <p className="text-sm text-muted">Loading settings…</p>
-  }
-  if (settings.isError) {
-    return <Alert>{errorMessage(settings.error, "Couldn't load settings.")}</Alert>
-  }
-  return <SettingsForm data={settings.data} update={update} />
-}
-
-function SettingsForm({
-  data,
-  update,
-}: {
-  data: { settings: InstanceSettings; effective: EffectiveAgent }
-  update: ReturnType<typeof useUpdateSettings>
-}) {
-  const [model, setModel] = useState(data.settings.agentModel)
-  const [enabled, setEnabled] = useState<EnabledChoice>(toChoice(data.settings.agentEnabled))
-  const [version, setVersion] = useState(data.settings.version)
-  const [error, setError] = useState<string | null>(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 (
-    <div className="flex max-w-2xl flex-col gap-6">
-      <div>
-        <h1 className="text-xl font-semibold text-fg">Settings</h1>
-        <p className="mt-1 text-sm text-muted">
-          Instance-wide, admin-only. Changes take effect immediately — no restart.
-        </p>
-      </div>
-
-      <section className="flex flex-col gap-4 rounded-lg border border-border p-4">
-        <div>
-          <h2 className="text-sm font-semibold text-fg">Garden assistant</h2>
-          <p className="mt-1 text-xs text-muted">
-            The model runs against your Ollama Cloud key, which is set in the environment and never
-            shown here.
-          </p>
-        </div>
-
-        <AgentStatus eff={eff} />
-
-        <TextField
-          label="Model"
-          name="agentModel"
-          placeholder={eff.model || 'ollama-cloud/glm-5.2:cloud'}
-          value={model}
-          onChange={(e) => 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.'
-          }
-        />
-
-        <Select
-          label="Enabled"
-          name="agentEnabled"
-          value={enabled}
-          onChange={(e) => setEnabled(e.target.value as EnabledChoice)}
-          options={[
-            { value: 'inherit', label: 'Inherit from environment' },
-            { value: 'on', label: 'On' },
-            { value: 'off', label: 'Off' },
-          ]}
-        />
-      </section>
-
-      {error && <Alert>{error}</Alert>}
-
-      <div className="flex justify-end">
-        <Button onClick={save} disabled={update.isPending}>
-          {update.isPending ? 'Saving…' : 'Save changes'}
-        </Button>
-      </div>
-    </div>
-  )
-}
-
-// AgentStatus surfaces the gap the capabilities endpoint exists for: enabled +
-// a key does not guarantee the assistant is actually running (an unresolvable
-// model leaves it down), and that's precisely what an admin needs to see.
-function AgentStatus({ eff }: { eff: EffectiveAgent }) {
-  const [tone, text] = eff.agentLive
-    ? (['ok', `Live on ${eff.model}`] as const)
-    : !eff.hasApiKey
-      ? (['warn', 'No API key set in the environment — the assistant is off.'] as const)
-      : !eff.enabled
-        ? (['warn', 'Turned off.'] as const)
-        : (['warn', `Configured but not running — check the model (${eff.model}).`] as const)
-
-  return (
-    <div className="flex items-center gap-2 text-sm">
-      <span
-        className={
-          'inline-block h-2 w-2 rounded-full ' + (tone === 'ok' ? 'bg-emerald-500' : 'bg-amber-500')
-        }
-        aria-hidden
-      />
-      <span className={tone === 'ok' ? 'text-fg' : 'text-muted'}>{text}</span>
-    </div>
-  )
-}

web/src/router.tsx

@@ -14,7 +14,6 @@ import { GardensPage } from '@/pages/GardensPage'
 import { GardenEditorPage } from '@/pages/GardenEditorPage'
 import { PublicGardenPage } from '@/pages/PublicGardenPage'
 import { PlantsPage } from '@/pages/PlantsPage'
-import { SettingsPage } from '@/pages/SettingsPage'
 import { meQueryOptions } from '@/lib/auth'
 import { queryClient } from '@/lib/queryClient'
 import { safeRedirectPath } from '@/lib/redirect'
@@ -49,20 +48,6 @@ async function requireGuest(context: RouterContext, redirectTo: string) {
   }
 }
 
-// requireAdmin: authenticated AND admin. A non-admin who navigates to /settings
-// is sent to /gardens rather than shown a page whose API calls would 403. This
-// is convenience routing, not the security boundary — the server's requireAdmin
-// is authoritative.
-async function requireAdmin(context: RouterContext, path: string) {
-  const me = await context.queryClient.ensureQueryData(meQueryOptions)
-  if (!me) {
-    throw redirect({ to: '/login', search: { redirect: path } })
-  }
-  if (!me.isAdmin) {
-    throw redirect({ to: '/gardens' })
-  }
-}
-
 const indexRoute = createRoute({
   getParentRoute: () => rootRoute,
   path: '/',
@@ -124,13 +109,6 @@ const plantsRoute = createRoute({
   component: PlantsPage,
 })
 
-const settingsRoute = createRoute({
-  getParentRoute: () => rootRoute,
-  path: 'settings',
-  beforeLoad: ({ context, location }) => requireAdmin(context, location.href),
-  component: SettingsPage,
-})
-
 // Public read-only garden by share token. Deliberately has NO beforeLoad auth
 // guard, so a logged-out visitor viewing a shared link is never redirected to
 // /login or OIDC.
@@ -147,7 +125,6 @@ const routeTree = rootRoute.addChildren([
   gardensRoute,
   gardenEditorRoute,
   plantsRoute,
-  settingsRoute,
   publicGardenRoute,
 ])