Moves the agent model out of env-only config into an admin-editable Settings section, and enforces is_admin — which has sat in the schema since migration 0001, plumbed all the way to the client, and checked nowhere.
The hard part wasn't the form
Agent config used to resolve into an opaque llm.Model at boot, and the chat routes were registered only when a key was present. So a settings toggle couldn't turn the assistant on — the route wouldn't exist. Three changes fix that:
The live Runner is hot-swapped.agentHolder holds it behind an atomic.Pointer; the chat routes are now registered unconditionally and nil-check agent.get(). A settings change swaps the Runner (on / off / different model) with no restart.
/capabilities reads the pointer, so it reports what's live, not what booted. useCapabilities drops staleTime: Infinity (the assumption this PR falsifies) and the save invalidates it.
is_admin is now enforced via requireAdmin — authoritative in the service seam, plus a cheap middleware early-403.
Secrets stay in the environment
OLLAMA_CLOUD_API_KEY is never stored in the DB — a secret in instance_settings would land in every backup and in the undo history's blast radius. An admin changes which model runs, not whose account pays. The Settings API reports whether a key is present, never its value.
Storage & resolution
Migration 0010: instance_settings, a single-row (CHECK id = 1) table — pansy's first instance-level state (everything else hangs off a garden/object). agent_model ('' = inherit env), agent_enabled (NULL = inherit env, so "admin hasn't touched this" is distinct from "admin turned it off"), version-guarded.
Precedence: Settings value → env var → built-in default. An instance that never opens Settings behaves exactly as before, so PANSY_AGENT_MODEL keeps working.
internal/agentmodel is a new leaf package: the one place that knows how to turn a spec into a model. Both agent (to run) and service (to validate before storing) import it — it can't live in agent, which imports service. The PATCH validates the spec via Parse, so a typo is a 400 at save time, not a broken assistant next turn.
Contract change worth calling out
Chat routes now always exist, so "assistant off" is a runtime 503 + capabilities:false, not a missing route (404). TestAgentDisabledWithoutAKey (renamed from …RoutesAbsent…) asserts the new shape; the frontend keys the chat tab off capabilities, so a user never reaches the 503. History routes stay ungated — they're stored data behind the ordinary garden-role check, not the model.
Frontend
/settings route with an admin guard, a Settings page (model field with an "inheriting X from the environment" hint, tri-state enabled select, and a live status dot that surfaces "configured but not running"), and a nav link shown only to admins. The admin routing guard is convenience — the server's requireAdmin is the boundary.
Verified live against the built binary
register first user → isAdmin: true
GET /settings → seeded row + effective{model:env, hasApiKey:true, agentLive:true}
PATCH enabled:false → capabilities flips to {agent:false}, logs "assistant turned off"
PATCH model:kimi, enabled:true → capabilities {agent:true}, logs "assistant ready model=…kimi…"
PATCH model:nonesuch/model → 400
The setting persisted across a process restart (version was already 1 from the seeded row on the second boot). The pointer swap is race-clean under go test -race (TestSettingsSwapUnderRace hammers capabilities — the same atomic load the chat handler does — while a writer flips the assistant; it deliberately avoids chat so no real model call is made on the fake key).
Tests
Service: admin gate on get/update, EffectiveAgent layering (model override, enabled inherit vs explicit), bad-model rejection leaves the row untouched.
API: admin-only (403 for member, 401 anonymous), env inheritance, the on→off→on swap through HTTP, bad model + non-bool agentEnabled → 400, version conflict carries the current row, and the race test.
agentmodel: Validate/Resolve, including a failover chain and no-key validation.
Notes for review
The admin check is in both the middleware and the service. The service one is authoritative (CLAUDE.md: rules in the seam); the middleware is a cheap early 403 reading the already-resolved actor. Redundant by design, not oversight.
agentModel/agentEnabled use the "inherit" sentinels ('' / null) rather than a separate "inherit" boolean. agent_enabled is genuinely nullable in SQLite; the CHECK (… IN (0,1)) still permits NULL (SQL CHECK passes unless it evaluates to false).
Docs updated in the same commit: README (precedence table + key-stays-in-env), DESIGN (decision entry + routes), CLAUDE (don't re-add conditional route registration; the key never enters the DB; the agentmodel cycle reason).
GOWORK=off go test ./... green; gofmt -l internal/ clean; tsc --noEmit, 87 vitest, and npm run build all pass.
Closes #79. Part of #86.
Moves the agent model out of env-only config into an admin-editable **Settings** section, and enforces `is_admin` — which has sat in the schema since migration 0001, plumbed all the way to the client, and checked *nowhere*.
## The hard part wasn't the form
Agent config used to resolve into an opaque `llm.Model` at boot, and the chat routes were registered *only when a key was present*. So a settings toggle couldn't turn the assistant on — the route wouldn't exist. Three changes fix that:
1. **The live Runner is hot-swapped.** `agentHolder` holds it behind an `atomic.Pointer`; the chat routes are now registered **unconditionally** and nil-check `agent.get()`. A settings change swaps the Runner (on / off / different model) with no restart.
2. **`/capabilities` reads the pointer**, so it reports what's *live*, not what booted. `useCapabilities` drops `staleTime: Infinity` (the assumption this PR falsifies) and the save invalidates it.
3. **`is_admin` is now enforced** via `requireAdmin` — authoritative in the service seam, plus a cheap middleware early-403.
## Secrets stay in the environment
`OLLAMA_CLOUD_API_KEY` is **never** stored in the DB — a secret in `instance_settings` would land in every backup and in the undo history's blast radius. An admin changes *which model* runs, not *whose account pays*. The Settings API reports whether a key is present, never its value.
## Storage & resolution
- **Migration 0010**: `instance_settings`, a single-row (`CHECK id = 1`) table — pansy's first *instance*-level state (everything else hangs off a garden/object). `agent_model` (`''` = inherit env), `agent_enabled` (`NULL` = inherit env, so "admin hasn't touched this" is distinct from "admin turned it off"), version-guarded.
- **Precedence: Settings value → env var → built-in default.** An instance that never opens Settings behaves exactly as before, so `PANSY_AGENT_MODEL` keeps working.
- **`internal/agentmodel`** is a new leaf package: the one place that knows how to turn a spec into a model. Both `agent` (to run) and `service` (to validate before storing) import it — it can't live in `agent`, which imports `service`. The PATCH validates the spec via `Parse`, so a typo is a **400 at save time**, not a broken assistant next turn.
## Contract change worth calling out
Chat routes now always exist, so "assistant off" is a runtime **503 + `capabilities:false`**, not a missing route (404). `TestAgentDisabledWithoutAKey` (renamed from `…RoutesAbsent…`) asserts the new shape; the frontend keys the chat tab off capabilities, so a user never reaches the 503. History routes stay ungated — they're stored data behind the ordinary garden-role check, not the model.
## Frontend
`/settings` route with an admin guard, a Settings page (model field with an "inheriting X from the environment" hint, tri-state enabled select, and a live status dot that surfaces "configured but not running"), and a nav link shown only to admins. The admin routing guard is convenience — the server's `requireAdmin` is the boundary.
## Verified live against the built binary
```
register first user → isAdmin: true
GET /settings → seeded row + effective{model:env, hasApiKey:true, agentLive:true}
PATCH enabled:false → capabilities flips to {agent:false}, logs "assistant turned off"
PATCH model:kimi, enabled:true → capabilities {agent:true}, logs "assistant ready model=…kimi…"
PATCH model:nonesuch/model → 400
```
The setting persisted across a process restart (version was already 1 from the seeded row on the second boot). The pointer swap is race-clean under `go test -race` (`TestSettingsSwapUnderRace` hammers `capabilities` — the same atomic load the chat handler does — while a writer flips the assistant; it deliberately avoids `chat` so no real model call is made on the fake key).
## Tests
- **Service**: admin gate on get/update, `EffectiveAgent` layering (model override, enabled inherit vs explicit), bad-model rejection leaves the row untouched.
- **API**: admin-only (403 for member, 401 anonymous), env inheritance, the **on→off→on swap through HTTP**, bad model + non-bool `agentEnabled` → 400, version conflict carries the current row, and the race test.
- **agentmodel**: `Validate`/`Resolve`, including a failover chain and no-key validation.
## Notes for review
- The admin check is in **both** the middleware and the service. The service one is authoritative (CLAUDE.md: rules in the seam); the middleware is a cheap early 403 reading the already-resolved actor. Redundant by design, not oversight.
- `agentModel`/`agentEnabled` use the "inherit" sentinels (`''` / `null`) rather than a separate "inherit" boolean. `agent_enabled` is genuinely nullable in SQLite; the `CHECK (… IN (0,1))` still permits NULL (SQL CHECK passes unless it evaluates to false).
Docs updated in the same commit: README (precedence table + key-stays-in-env), DESIGN (decision entry + routes), CLAUDE (don't re-add conditional route registration; the key never enters the DB; the `agentmodel` cycle reason).
`GOWORK=off go test ./...` green; `gofmt -l internal/` clean; `tsc --noEmit`, 87 vitest, and `npm run build` all pass.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Moves the agent model out of env-only config into an admin-editable Settings
section, and enforces is_admin for the first time — it has been in the schema
since migration 0001, plumbed to the client, and checked nowhere.
Backend:
- Migration 0010: instance_settings, a single-row (CHECK id=1) table — pansy's
first instance-level state. Holds agent_model ('' = inherit env) and
agent_enabled (NULL = inherit env), version-guarded like every mutable row.
SECRETS STAY IN ENV: OLLAMA_CLOUD_API_KEY is never stored here.
- requireAdmin at the service seam (authoritative) plus a cheap middleware
early-403. Non-admin gets 403, not 404 — settings existence isn't masked.
- EffectiveAgent resolves DB-over-env (model, enabled); key always from env.
- The live Runner is hot-swapped, not built once. agentHolder holds it behind
an atomic.Pointer; the chat routes are now registered UNCONDITIONALLY and
nil-check agent.get(), so a settings change turns the assistant on/off/onto a
new model with no restart and no race against in-flight readers. /capabilities
reads the pointer, so it reports what's live, not what booted.
- internal/agentmodel is a new leaf package holding the one place that knows how
to turn a spec into a model. Both agent (to run) and service (to validate a
spec before storing it) import it; it can't live in agent, which imports
service. Settings PATCH validates the spec via Parse, so a typo is a 400 now
rather than a broken assistant on the next turn.
Frontend:
- /settings route (admin guard), a Settings page (model field, tri-state
enabled, live status), nav link shown only to admins.
- useCapabilities drops staleTime:Infinity — the assistant can now change under
a running page — and the settings save invalidates it.
Contract change: chat routes always exist, so "assistant off" is a runtime 503
+ capabilities:false, not a missing route. Updated the test that asserted the
old shape.
Verified live against the built binary: disable flips capabilities to false and
logs it; re-enable with a new model swaps it back; a bad spec is rejected 400;
the setting persists across a restart. Swap is race-clean under `go test -race`.
Docs: README (precedence + key-stays-in-env), DESIGN (decision + routes),
CLAUDE (don't re-add conditional route registration; key never in the DB).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
(First review — the original one died on the docker.gitea.com CI outage, now resolved. This is the admin Settings + runtime model-swap PR; see the description for the notes-for-review, especially the double admin check and the atomic-pointer swap.)
@gadfly review
(First review — the original one died on the `docker.gitea.com` CI outage, now resolved. This is the admin Settings + runtime model-swap PR; see the description for the notes-for-review, especially the double admin check and the atomic-pointer swap.)
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 02:28:38Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ⚠️ **security** — could not complete
- ✅ **correctness** — No material issues found
- ⚠️ **maintainability** — could not complete
- ⚠️ **performance** — could not complete
- ⚠️ **error-handling** — could not complete
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ⚠️ **security** — could not complete
- ⚠️ **correctness** — could not complete
- ⚠️ **maintainability** — could not complete
- ✅ **performance** — Minor issues
- ⚠️ **error-handling** — could not complete
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
Verdict: Minor issues · 10 findings (2 with multi-model agreement)
Finding
Where
Models
Lens
🟡
EffectiveAgent re-reads instance_settings row that GetInstanceSettings just fetched
internal/api/settings.go:63
2/5
maintainability, performance
🟡
Stale doc comment references config.AgentConfig.Ready(), which this PR deletes
internal/service/instance_settings.go:87
2/5
maintainability
8 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
Test goroutines may outlive test function causing race/panic under -race
internal/api/settings_test.go:180
opencode/kimi-k2.6:cloud
error-handling
🟠
Swallowed validation error leaves admin with no diagnostic on bad model spec
internal/service/instance_settings.go:64
opencode/kimi-k2.6:cloud
error-handling
🟠
EffectiveAgent type and method share the same name in one package
internal/service/instance_settings.go:81
opencode/kimi-k2.6:cloud
maintainability
🟠
fromChoice silently falls through 'off' in cryptic ternary
web/src/pages/SettingsPage.tsx:22
opencode/kimi-k2.6:cloud
maintainability
🟡
PATCH /settings binds agentModel as a plain string (full-replacement semantics), so a request that omits it resets the stored model to "inherit" — intentional and documented, but inconsistent with the rest of the codebase's absent=unchanged PATCH convention
internal/api/settings.go:96
claude-code/sonnet
correctness
🟡
Enabled field doc says agent is 'the same shape as OIDC 404ing', but this PR changes agent-off to a 503 on an always-registered route
internal/config/config.go:75
opencode/glm-5.2:cloud
maintainability
🟡
requireAdmin re-queries user table when IsAdmin already verified by middleware
internal/service/instance_settings.go:27
kimi-k2.6:cloud
performance
⚪
Stale/incorrect comment claims capabilities route registration order matters and is 'after the agent below', but it's registered before and order no longer affects behavior
internal/api/api.go:59
claude-code/sonnet
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Minor issues
Frontend only consumes the boolean, never handles a raw key. That confirms the secret-handling design holds end-to-end.
Review
VERDICT: No material issues found
I checked this PR specifically for authn/authz gaps, injection, secret leakage, and insecure defaults around the new admin-gated Settings feature:
Admin enforcement is real and layered correctly.requireAdmin (internal/api/settings.go:24) is a cheap early-403 reading the already-resolved actor; the authoritative check is Service.requireAdmin (internal/service/instance_settings.go:15), which re-reads IsAdmin from the DB per call rather than trusting a cached claim. Verified ResolveSession (internal/service/auth.go:220) re-fetches the session/user from the store on every request — no stale-admin-in-token risk.
No privilege-escalation path found. Grepped for any endpoint that lets a non-admin set is_admin; none exists. settingsUpdateRequest (internal/api/settings.go) only exposes agentModel/agentEnabled/version — no mass-assignment surface.
Secret handling is correctly enforced end-to-end.OLLAMA_CLOUD_API_KEY never enters the DB (migration 0010 has no key column) and the API response (effectiveView.HasApiKey) only ever serializes a boolean. Confirmed the frontend (web/src/pages/SettingsPage.tsx) only consumes that boolean and never touches a raw key.
CSRF coverage is unchanged/inherited./settings sits under v1, which has csrfGuard() applied globally (internal/api/api.go:57), so the new PATCH route is covered like every other mutating route.
Error responses don't leak internals.writeServiceError's ErrInvalidInput branch (internal/api/errors.go:54) returns a generic "invalid input" 400 — a rejected model spec doesn't echo the underlying parse error or any registry detail back to the client.
SQL is parameterized in the new instance_settings store code — no injection surface.
Considered whether an admin-supplied model spec could be an SSRF vector now that it's reachable via API instead of only an env var (agentmodel.Resolve → majordomo.Parse). Couldn't verify majordomo's Parse/ollama.Cloud internals (module cache is outside the sandboxed directory), so I'm not reporting this — flagging it here only as unverified, not as a finding.
Outside my lens: none worth flagging — the concurrency/rebuild-lock design and doc/test thoroughness are for other reviewers.
🎯 Correctness — Minor issues
Confirmed. The settingsUpdateRequest.AgentModel field is indeed bound as a plain string (settings.go:96, not lines 39-45 as the draft cited — that range is actually settingsResponse/effectiveView), and internal/service/instance_settings_test.go:70-71 shows the test suite must re-pass AgentModel on every update specifically to avoid resetting it, which corroborates the behavior. However, the draft's suggested fix is inaccurate: this is already documented prominently, in two doc comments — internal/service/instance_settings.go:40-42 ("InstanceSettingsPatch is a full replacement of the editable fields... an empty AgentModel means fall back to env") and internal/api/settings.go:92-94 ("agentModel "" means inherit the env var") — at the same level of prominence as the objectUpdateRequest comment the draft cites for contrast. So this is a real, confirmed deviation from the rest of the codebase's convention, but a deliberate and clearly-documented one, not an oversight.
Review — 🎯 Correctness lens
VERDICT: Minor issues
internal/api/settings.go:96 (settingsUpdateRequest.AgentModel) — bound as a plain string, not the *string/json.RawMessage "absent vs. explicit" pattern used elsewhere (contrast objectUpdateRequest in internal/api/objects.go:50-65). Since "" is the domain's own sentinel for "inherit from environment" (internal/service/instance_settings.go:60-67, EffectiveAgent at line 103), a PATCH that omits agentModel entirely is indistinguishable from one that explicitly clears it, so {"agentEnabled": false, "version": N} alone silently resets the stored model back to "inherit env." This is confirmed by internal/service/instance_settings_test.go:70-71, where the test must re-supply AgentModel on every subsequent update specifically to avoid this reset.
This behavior is intentional and explicitly documented as "a full replacement of the editable fields" (internal/service/instance_settings.go:40-42) and "agentModel "" means inherit the env var" (internal/api/settings.go:92-94) — at the same prominence as the "absent = unchanged" doc comment on objectUpdateRequest that provides the contrast. It is also currently unreachable in the shipped product: web/src/pages/SettingsPage.tsx:61-64 is a single form that always submits agentModel and agentEnabled together.
Still worth flagging: it's a real inconsistency with the "omit = unchanged" convention every other mutable-resource PATCH in this codebase trains callers to expect, and it's live against any future script, curl call, or split-out form that PATCHes agentEnabled alone.
🧹 Code cleanliness & maintainability — Minor issues
Both findings confirmed against the actual code:
config.AgentConfig has no Ready method anywhere in the tree (grep confirms), yet internal/service/instance_settings.go:87's comment still says "Ready mirrors config.AgentConfig.Ready".
internal/api/api.go:62 registers /capabilities at line 62, while h.agent isn't assigned until line 141 — after, not before. The comment's claim is backwards, and it's superseded by the accurate doc on capabilities at lines 194-200.
Review
VERDICT: Minor issues
internal/service/instance_settings.go:87 — EffectiveAgent.Ready()'s doc comment says "Ready mirrors config.AgentConfig.Ready", but this same PR deletesconfig.AgentConfig.Ready (confirmed via grep — no remaining references anywhere in the tree, including internal/config/config.go). The comment now points at dead code that no longer exists, which CLAUDE.md calls out explicitly ("Stale comments are worse than none — the next reader believes them"). Fix: drop the "mirrors config.AgentConfig.Ready" clause or just describe the check plainly (enabled, with a key and a model), which the second half of the sentence already does.
internal/api/api.go:59-61 — the comment above v1.GET("/capabilities", h.capabilities) still reads "Registered after the agent below, because it reports whether the runner actually built." This was true of the pre-#79 shape, but post-refactor capabilities reads h.agent.get() off an atomic pointer at request time — registration order is irrelevant now, and the route is in fact registered beforeh.agent = newAgentHolder(...) at line 141, contradicting the comment's own claim. The accurate rationale is already given in the capabilities func doc a few lines down (194-200), so this stale comment is both wrong and redundant. Suggest deleting it or replacing with a one-liner noting registration order no longer matters here.
Everything else touched by this PR (agentHolder, settings.go, instance_settings.go in service/store, agentmodel package, migration, frontend SettingsPage.tsx/settings.ts) is well-factored: single-responsibility functions, no copy-pasted logic, naming consistent with the rest of the codebase, and the "why" comments earn their keep. No structural or duplication concerns beyond the two stale comments above.
⚡ Performance — No material issues found
No polling loop for settings — it fetches once via React Query defaults. Nothing performance-relevant there. This confirms the read path (chat, capabilities) stays cheap (single atomic load), and the settings admin path, while doing a couple of redundant EffectiveAgent DB reads per request, operates on a single-row table with no scan cost and is admin-only/low-frequency — not a justifiable regression.
VERDICT: No material issues found
Reviewed through the performance lens:
Hot path (chat/capabilities) is unaffected or improved.agentChat (internal/api/agent.go:61) now does one atomic.Pointer load (h.agent.get()) instead of a plain field read — same O(1) cost, and it's loaded once up front rather than re-read, avoiding any redundant load. capabilities (internal/api/api.go) is likewise a single atomic load. Verified agentHolder.get() (internal/api/agent_holder.go:44) is lock-free.
agentHolder.rebuild (internal/api/agent_holder.go:52) takes a mutex and does a DB read (EffectiveAgent) plus builds a fresh majordomo registry — but this only runs at boot and on an admin settings PATCH, never per-request. No hot-loop concern.
Redundant EffectiveAgent DB reads on settings PATCH: updateSettings (internal/api/settings.go) calls h.agent.rebuild (which internally calls EffectiveAgent, one SELECT on instance_settings) and then settingsPayload (which calls EffectiveAgent again) — three DB round trips total for one PATCH (UPDATE...RETURNING, then two single-row SELECTs). This is technically avoidable, but it's an admin-only, low-frequency endpoint against a single-row table with no scan cost, so the impact isn't justifiable as a real regression.
Frontend polling: useCapabilities (web/src/lib/agent.ts:30) drops staleTime: Infinity for staleTime: 60_000 — a bounded, sane refetch interval, not aggressive polling. useSettings has no refetchInterval. No new client-side hot-loop.
Nothing here rises to a genuine efficiency regression under this lens.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed this PR specifically for ignored/swallowed errors, missing cleanup, panics on bad input, and unhandled edge cases (nil, empty, zero/negative, off-by-one), verifying against the checked-out code rather than the diff alone.
What I checked and found sound:
internal/api/agent_holder.go — rebuild() correctly serializes concurrent settings saves via rebuildMu while keeping get() lock-free. An EffectiveAgent read failure is logged and the old Runner is deliberately kept (not torn down on a transient blip) — this tradeoff is explicitly reasoned about in the comments, not an oversight. A resolvable-config-but-unresolvable-model correctly swaps to nil rather than leaving a stale Runner. Verified newAgentHolder never leaves the pointer in a panicking state on boot failure.
internal/api/agent.go — runner := h.agent.get() is loaded once and reused for both the nil-check and the Run call, avoiding a TOCTOU gap where a concurrent settings swap could nil out the runner mid-request. Verified getAgentHistory/deleteAgentHistory don't touch h.agent at all, so they can't nil-panic when the assistant is off.
internal/store/instance_settings.go — scanInstanceSettings correctly handles the nullable agent_enabled via sql.NullInt64; UpdateInstanceSettings correctly passes untyped nil to bind SQL NULL (restoring "inherit" from an explicit override), and the version-mismatch path re-fetches and returns the current row with ErrVersionConflict, consistent with every other mutable resource in the codebase. GetInstanceSettings's sql.ErrNoRows case is correctly distinguished as "migration not applied" rather than papered over.
internal/service/instance_settings.go — UpdateInstanceSettings validates a non-empty model before writing (verified TestUpdateInstanceSettingsRejectsBadModel confirms the row is untouched on rejection); requireAdmin propagates a GetUserByID error unchanged rather than swallowing it into a wrong error class.
internal/agentmodel/agentmodel.go — Resolve/Validate correctly reject empty/whitespace-only specs via TrimSpace, tested for both the key-present and key-absent cases.
internal/api/settings.go — parseNullable[bool] correctly treats absent vs. explicit null vs. true/false; a non-bool value ("yes") is verified (test + code read) to 400 rather than silently coercing. Version int64 with binding:"required" mirrors the existing pattern in journal.go/seed_lots.go, and the migration seeds version = 1, so the zero-value/required gotcha doesn't bite here.
Frontend (settings.ts, SettingsPage.tsx) — mutation error path distinguishes a 409 (rebases the form to the server's current row and warns the user rather than silently discarding their edit) from other errors; loading/error/pending states are all handled before rendering the form.
I did not find any unhandled nil/empty/boundary case, swallowed error, or missing cleanup that isn't already covered by the extensive test suite (version conflicts, bad-model rejection, disabled-key 503, admin-gate 403, and the race test).
glm-5.2:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 4/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2☁️ skipped (backed off until 02:18:17.784)
🎯 Correctness — No material issues found
VERDICT: No material issues found
I verified the correctness-critical paths of this change by reading the actual code, not just the diff:
Layering / precedence (internal/service/instance_settings.go): EffectiveAgent seeds from s.cfg.Agent.Model (env-resolved, with the DefaultAgentModel fallback already applied by config.Load) and s.cfg.Agent.Enabled, then overrides Model only when st.AgentModel != "" and Enabled only when st.AgentEnabled != nil. The "inherit" sentinels ('' / nil) correctly fall through. Re-derived Ready() = Enabled && APIKey != "" && Model != "" against the seeded row (agent_model='', agent_enabled=NULL) — yields the env/default values, so an untouched instance behaves as before. ✓
Migration / nullability (0010_instance_settings.sql): agent_enabled INTEGER CHECK (agent_enabled IN (0,1)) permits NULL (a CHECK only fails on false, and NULL IN (0,1) is NULL, not false). Seed inserts (1, '', NULL). Store scans via sql.NullInt64 and maps enabled.Valid → *bool, else leaves nil. Correct tri-state. ✓
Optimistic concurrency (internal/store/instance_settings.go): UPDATE ... WHERE id = 1 AND version = ? RETURNING ...; sql.ErrNoRows → fetch current row + ErrVersionConflict; success bumps version+1. Seeded version=1, so the first PATCH with version=1 matches. The 409 path carries the current row (writeVersionConflict). ✓
Hot-swap race safety (internal/api/agent.go): agentChat loads runner := h.agent.get() once and nil-checks before binding the request, then uses that local runner.Run — a mid-request swap cannot flip the pointer between guard and use. rebuild serializes writers via rebuildMu while reads stay lock-free on the atomic. ✓
History routes ungated on the Runner: getAgentHistory/deleteAgentHistory use h.svc, never h.agent, so they correctly return 200/204 regardless of whether a Runner exists (verified at agent.go:193-217). ✓
Validate-before-store (internal/service/instance_settings.go): agentmodel.Validate(s.cfg.Agent.OllamaCloudAPIKey, model) runs on non-empty specs before the UPDATE; Validate calls Resolve, which only parses (no live call), so it works even with an empty env key. A typo → ErrInvalidInput → 400, row untouched. ✓
agentModel binding: a plain string field means a JSON null unmarshals to "" (the inherit sentinel), consistent with an explicit empty string. strings.TrimSpace in the service also collapses whitespace-only to "". ✓
agentEnabled three-way decode: parseNullable[bool] returns (nil,false) for absent and (nil,true) for explicit null — both yield enabled=nil (inherit). Verified at errors.go:79. ✓
No semantic, formula, threshold, or boundary-value defect surfaced under the correctness lens. (One doc-only imprecision — the domain.InstanceSettings comment says AgentModel "" "falls back to PANSY_AGENT_MODEL", when it actually falls back to cfg.Agent.Model, which is env-or-DefaultAgentModel. This is a comment inaccuracy, not a behavior bug, so I am not reporting it as a finding.)
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2☁️ skipped (backed off until 02:18:17.784)
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2☁️ skipped (backed off until 02:18:17.784)
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 2: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
⚡ Performance — Minor issues
Minor issues
No material issues found in hot paths — the atomic pointer swap is clean, capabilities reads are lock-free, and the admin endpoints are low-traffic. Two layering inefficiencies in the settings request paths are worth fixing:
internal/api/settings.go:63 and internal/api/settings.go:141 — settingsPayload calls h.svc.EffectiveAgent, which always does a fresh GetInstanceSettings DB read. In getSettings, the same row was just fetched by GetInstanceSettings. In updateSettings, the row was just written and returned, then rebuild calls EffectiveAgent (second read), then settingsPayload calls it again (third read). A single PATCH can hit the same row 3 times. Pass the already-loaded row into EffectiveAgent (or add an EffectiveAgentFrom variant) to cut this to one read. Verified by reading internal/service/instance_settings.go:93-110 and tracing the call chain in settings.go and agent_holder.go:60.
internal/service/instance_settings.go:27-36 — requireAdmin queries GetUserByID to check IsAdmin on every settings request, but mustActor(c).IsAdmin was already resolved during session authentication and checked by the requireAdmin() middleware. This adds a DB roundtrip to an already-gated endpoint. The author calls this "redundant by design" (service seam authority), which is fair for correctness, but from a performance lens it's still avoidable overhead on an endpoint that does little else. Consider passing the resolved user (or a small admin flag) into the service call, or accepting the middleware as sufficient and skipping the service-layer re-query for this read-only gate. Verified by reading internal/api/auth.go:109 (session resolution loads the full user), internal/api/settings.go:28-36 (middleware checks IsAdmin), and internal/service/instance_settings.go:27-36 (re-queries the DB).
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 1: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
The security model looks solid: requireAuth resolves the user (and the middleware + service both re-check IsAdmin), the key never enters the DB, and the settings row is admin-gated. Let me check a few more things — particularly how effective.model is exposed and whether model spec is a path-traversal or injection risk through Parse.
I've reviewed the change through the security lens, verifying the auth chain, secret handling, SQL parameterization, and input validation against the actual checked-out code.
VERDICT: No material issues found
Security-relevant verification I did:
internal/api/settings.go:22requireAdmin middleware reads mustActor(c).IsAdmin from the already-resolved session user (requireAuth at auth.go:109 calls svc.ResolveSession and stashes the user). Layered with internal/service/instance_settings.go:21requireAdmin, which re-fetches the user from the DB — so a demotion between middleware and service is caught. Redundant by design, as the PR notes.
internal/api/settings.go:44-54effectiveView exposes only hasApiKey: bool, never the key. The EffectiveAgent.APIKey field (instance_settings.go:84) is used solely to build the Runner; no JSON tag serializes it. Confirmed the key never enters instance_settings (migration 0010 has no key column).
internal/store/instance_settings.go:62-68 UPDATE uses ? placeholders for agent_model, agent_enabled, version; no string interpolation of user input into SQL. Same for the SELECT.
internal/service/instance_settings.go:60-67agentModel is trimmed and validated via agentmodel.Validate → Resolve → majordomo.Parse before storage; unresolvable specs return ErrInvalidInput (400). agentEnabled is decoded with parseNullable[bool] (errors.go:79) which rejects non-bool JSON. version is binding:"required".
No path/command injection surface: the spec is opaque to pansy and passed verbatim to the vendor parser; pansy never interprets it as a path or shell argument.
EffectiveAgent (the unauthenticated code path used by agentHolder.rebuild) only reads the settings row and env — it does not expose anything to a caller, and rebuild logs the model but never the key (agent_holder.go:78 logs eff.Model, not eff.APIKey).
No security findings to report.
🎯 Correctness — No material issues found
I've reviewed the key parts. Now let me check the migration of cfg.Agent.Enabled defaulting - there's a subtle issue worth checking. The EffectiveAgent layers Enabled: s.cfg.Agent.Enabled where the env default is "on when a key is present". Let me verify the config-level Enabled semantics are actually correct.
Now let me check one specific potential correctness issue: what happens at boot if PANSY_AGENT_ENABLED=false but OLLAMA_CLOUD_API_KEY set? The env default for cfg.Agent.Enabled resolves to false. Then a settings change that turns on the agent... let me check the validate flow. Validate uses the env key regardless. That's fine.
Let me also check one subtle issue: the Enabled env default of "on when key present". When admin explicitly sets agentEnabled=null (inherit), EffectiveAgent.Enabled = s.cfg.Agent.Enabled. That's true when a key is present (correct inherit semantics). Good.
Now check the test settings response agentModel round-trip for an issue with the agentEnabled field marshaling. The domain.InstanceSettings.AgentEnabled *bool with json:"agentEnabled" — when nil, marshals to null. The store scan returns nil if invalid. Good.
Let me verify one more thing — the updateSettings race test hammering capabilities while writer does PATCH. There's a potential issue: when i%2==0 flips enabled=true but agentModel="" inherit. With agentCfg() env model is ollama-cloud/glm-5.2:cloud, so resolving works. Good.
Let me verify the rebuild ordering with respect to the response. The handler calls h.agent.rebuild(...) then settingsPayload. But the rebuild is serialized via mutex; settingsPayload reads h.agent.get() which is atomic. Could a second concurrent PATCH interleave between rebuild and payload read? It would just reflect whichever swap happened. Not a correctness bug, just a stale-view possibility. Acceptable.
Now let me check one final potential issue - the chat route reads runner := h.agent.get() early, but the history fetch happens after. If the runner is swapped to nil after the get but before Run, we use a stale (now-disabled) runner. That's a benign TOCTOU since the runner is still valid. Fine.
Let me verify the more subtle issue: updateSettings calls h.svc.UpdateInstanceSettings which does requireAdmin again, then h.agent.rebuild — but the rebuild calls h.svc.EffectiveAgent which doesn't check admin. That's fine by design (it's internal).
Now check Validate semantics. The service validates the model with the env key. If the key is missing but admin sets a model spec, Validate("", "ollama-cloud/x") — does Parse work with empty token? The test asserts Validate("", "ollama-cloud/glm-5.2:cloud") succeeds. So yes.
Let me check one thing - the store UpdateInstanceSettings returning (nil, gerr) when the current row read fails after a version conflict. The handler then does writeVersionConflict(c, st) with nil st. Let me check writeVersionConflict:
Looking at errors.go:66 - c.JSON(http.StatusConflict, gin.H{"error": ..., "current": current}) with current=nil → marshals to "current":null. Not a crash, just a degraded 409. Edge case but fine.
Now let me look for the real correctness issue: when UpdateInstanceSettings succeeds, the returned st is the updated row, which is what gets sent in the response. But the rebuild happens BEFORE settingsPayload. The settingsPayload calls EffectiveAgent which re-reads the row. Good, consistent.
Let me check whether there's a subtle ordering issue with rebuild happening inside the mutex BEFORE the response is built. If rebuild fails (e.g. NewRunner error), the response still says success with AgentLive: false. That's correct.
Let me now check the critical edge case: agentHolder.rebuild when eff.Ready() is false uses h.ptr.Swap(nil). The Swap returns old value. If old was nil and new is nil, no log. Good. But it uses Store(nil) in the NewRunner error path instead of Swap, which doesn't log "turned off" in that case but does log the error. Fine.
Now let me check one potentially real issue: the agentModel PATCH validation. When admin sends agentEnabled: false AND agentModel: "nonesuch/model" — the service validates the model first and returns ErrInvalidInput WITHOUT touching the row. But the test TestSettingsRejectsBadModel sends them separately. What if both are sent together? Validate runs first, returns early, no update. Correct.
Let me check the frontend side briefly to ensure capability polling is consistent.
Now let me verify the potential correctness issue I want to confirm: when Enabled env default is true (key present) and admin has not touched settings, EffectiveAgent.Enabled = s.cfg.Agent.Enabled = true. Good. When env says PANSY_AGENT_ENABLED=false explicitly, s.cfg.Agent.Enabled = false, and admin with agentEnabled=null inherits false. Correct.
Let me check one more subtle thing — the rebuild on the very first ever newAgentHolder call at boot. It uses context.Background(). If the DB hasn't been migrated yet... no, migrations run before New. Fine.
Let me check one final concern: UpdateInstanceSettings returns the updated row but agentmodel.Validate swallows the actual error and returns ErrInvalidInput. The test confirms this. But could a valid spec be rejected because the env key is invalid? Validate uses s.cfg.Agent.OllamaCloudAPIKey — even an empty key works (test confirms). So a non-empty but invalid key still allows Parse to succeed (Parse doesn't call). Good.
I've verified enough. Let me check one last potential issue: the agentModel field trimming. Service does model := strings.TrimSpace(patch.AgentModel) and stores the trimmed version. So if admin sends " ollama-cloud/x ", it's stored trimmed. Good. But EffectiveAgent returns the stored (trimmed) value, and agentmodel.Resolve re-trims. Fine.
One subtle thing: the test TestSettingsInheritFromEnv checks eff["hasApiKey"] != true. With agentCfg() key is "test-key". Good.
Let me verify there isn't an issue with the agentEnabled JSON marshaling when it's an explicit false. The store scans to *bool pointing to false. The domain struct marshals *bool → false. Good. When nil → null. Good.
I'm satisfied. Let me also quickly check whether record is called for instance settings updates — CLAUDE.md says "every service mutation lands in history".
Confirmed: UpdateInstanceSettings does NOT call record. CLAUDE.md: "Every service mutation lands in history (#48). If you add one, record it."
This is an instance-level setting, not a garden-scoped change. Let me verify whether the change-set system supports instance-level history. Looking at WriteChangeSet:
The change-set system is garden-scoped (GardenID is required). Instance settings are not garden-scoped, so the record invariant doesn't naturally apply. The PR description even notes "secrets never move here — so it doesn't land in ... the undo history's blast radius" — suggesting instance settings are intentionally outside the history system. So not calling record is by design here, not a bug. Not a finding.
Let me do one final check on the boolToInt usage to make sure the storage round-trips correctly. boolToInt(false) = 0, boolToInt(true) = 1. The scan does enabled.Int64 != 0 → false for 0, true for 1. Correct round-trip.
I've thoroughly verified the change. My lens (correctness) finds no material issues. The semantics of the inherit sentinels, the version-guarded update, the atomic pointer swap, the admin gate, and the precedence layering are all correct. The one TOCTOU window (runner swap between get and Run) is benign and documented. The 409-with-nil-current edge case is a minor degradation, not a correctness bug for the happy path.
Verdict: No material issues found
Verified clean (correctness lens):
internal/service/instance_settings.go:93-110EffectiveAgent layering: re-derived precedence (Settings → env → default) from cfg.Agent defaults (Enabled: agentKey != "", Model: DefaultAgentModel). Inherit sentinels ("" / nil) fall through to env correctly; explicit overrides replace. Round-trips with the test matrix.
internal/store/instance_settings.go:56-80UpdateInstanceSettings: version guard WHERE id = 1 AND version = ? + version = version + 1; on sql.ErrNoRows returns (current, ErrVersionConflict). boolToInt round-trips *bool ↔ INTEGER 0/1; NULL (nil) preserved through var enabled any defaulting to nil. Re-derived from the SQL and the migration's CHECK (agent_enabled IN (0,1)) permitting NULL — correct.
internal/api/agent_holder.go:56-89rebuild: serializes via rebuildMu; Ready() false → Swap(nil); resolution failure → Store(nil) (intentionally tears down stale runner); success → Store(runner). Atomic semantics match the race test. The TOCTOU window in agentChat (runner loaded, then Run called later) is benign — a swapped-out runner remains a valid handle; documented in code.
internal/api/settings.go:101-143updateSettings: validates agentEnabled via parseNullable[bool] (absent/null → nil = inherit; true/false → override), validates model spec via service before persisting, rebuilds the live runner with context.WithoutCancel (matches the documented detached-write pattern), then returns the fresh row. 409 path passes the returned st to writeVersionConflict; the only degradation (nil current if the conflict-row read itself fails) is an edge case in store.UpdateInstanceSettings, not a happy-path bug.
internal/agentmodel/agentmodel.goResolve/Validate: empty/blank spec rejected before Parse; Parse gets the spec verbatim. Validate with empty key succeeds (Parse doesn't call upstream) — confirmed by agentmodel_test.go. Service validates with the env key, which is correct since the key is never stored.
internal/api/agent.go:63-67 503 contract: route always registered, nil-check on the loaded runner, runner cached in a local so a mid-request swap can't flip the guard/Run pair. Matches the test's TestAgentDisabledWithoutAKey.
Migration 0010: CHECK id = 1 + seed row means reads never branch; agent_enabled nullable with CHECK (… IN (0,1)) permits NULL (SQL CHECK passes unless false) — re-derived from SQLite semantics, correct.
Investigated and dismissed:
UpdateInstanceSettings not calling record — by design: the change-set system is garden-scoped (ChangeSet.GardenID required), and the PR explicitly excludes instance settings from the undo history's blast radius. Not a violation of the #48 invariant, which targets garden mutations.
writeVersionConflict(c, st) with a possibly-nil st (if store.GetInstanceSettings fails after the conflict) — degrades to "current": null, not a crash; pathological DB-fault path, not a happy-path correctness bug.
🧹 Code cleanliness & maintainability — Minor issues
I'll verify the two draft findings against the actual code.
Both findings confirmed.
internal/service/instance_settings.go:87 — comment references config.AgentConfig.Ready, which no longer exists (grep confirms zero definitions). The EffectiveAgent.Ready method exists but the comment points at the deleted one.
internal/config/config.go:75 — the Enabled field doc says "the same shape as OIDC 404ing when unconfigured," but this PR's contract change is that agent routes are now always registered (503 when off). Let me verify the api.go registration change to confirm.
Both findings are confirmed. The agent chat route is always registered (api.go:147) and returns 503/AGENT_DISABLED when off (agent.go:65), confirming the OIDC-404ing analogy at config.go:75 is now stale.
VERDICT: Minor issues
internal/service/instance_settings.go:87 — The doc comment // Ready mirrors config.AgentConfig.Ready: enabled, with a key and a model. references config.AgentConfig.Ready(), a method this PR deletes. Grep across the repo confirms zero remaining definitions of AgentConfig.Ready. The EffectiveAgent.Ready method still exists but the comment points at a now-removed symbol — exactly the "stale comments are worse than none" case CLAUDE.md flags. Fix: reword to describe EffectiveAgent.Ready on its own terms (e.g. "Ready reports whether the assistant can be offered: enabled, with a key and a model.") rather than mirroring the removed method.
internal/config/config.go:75 — The Enabled field doc ends with "the same shape as OIDC 404ing when unconfigured." This PR's central contract change is that the agent routes are now registered unconditionally (off = 503 AGENT_DISABLED on a live route, confirmed at internal/api/agent.go:65 and internal/api/api.go:147, not a missing-route 404), so the OIDC-404ing analogy is now wrong for the agent path. The PR edits this file (it deleted Ready() immediately below at what is now the OIDCConfig.Enabled boundary), so the stale comment is in scope. Fix: drop the comparison or reword to reflect the new 503 shape (e.g. "...starts cleanly with the assistant off and reported as unavailable via /capabilities").
Both are comment-only fixes; no logic is affected.
⚡ Performance — No material issues found
The performance-critical paths (chat, capabilities polled by the editor) are unchanged in cost or improved (atomic load, no DB). Let me verify the redundant DB reads in the settings handlers.
I've verified the hot paths (chat, capabilities) and the new settings paths against the checked-out code. Here's my assessment through the performance lens.
VERDICT: No material issues found
Through the performance lens:
Hot paths are unchanged or improved.agentChat (internal/api/agent.go:63) reads the runner via h.agent.get() — a single atomic.Pointer.Load, cheaper than the old struct-field read and race-clean. capabilities (internal/api/api.go:202) is the same atomic load. Neither adds a DB query or allocation per request. The per-turn chat work (runner.Run) is untouched.
rebuild does not contend with reads.rebuildMu serializes only rebuild calls (rare: settings saves); get() is lock-free on the atomic pointer (internal/api/agent_holder.go:45,57). A settings save cannot block chat or capabilities reads.
Redundant DB reads on the settings path are on a cold path.getSettings reads the row via GetInstanceSettings then re-reads it via EffectiveAgent in settingsPayload (internal/api/settings.go:79,63); updateSettings reads it a third time via rebuild→EffectiveAgent (internal/api/settings.go:135 + internal/api/agent_holder.go:60). All three hit the same single-row SELECT by id = 1. This is wasteful but only an admin saving settings triggers it — not a hot path, and the query is point-looked by PK. Not material.
agentmodel.Resolve allocates a fresh majordomo.Registry per call (internal/agentmodel/agentmodel.go:28-31), invoked on boot and on each settings save (once via Validate, once via rebuild→NewRunner). Again, cold paths only; the chat hot path reuses the already-built Runner.
useCapabilities changed from staleTime: Infinity to 60_000 (web/src/lib/agent.ts:30), so every mounted editor now refetches /capabilities after 60s of staleness (and on focus). This is a new periodic poll, but the endpoint is a single atomic load + tiny JSON, and the interval is deliberate (to pick up another admin's change). Not a regression worth flagging.
🧯 Error handling & edge cases — No material issues found
I'll review this PR through the error-handling/edge-cases lens. Let me verify the key files.
Now let me check the migration runner — does pansy handle the case where migration 0010 isn't applied yet but code tries to read settings at boot?
Let me verify the error-message flow for the bad-model case more closely.
Now I have a thorough understanding. Let me verify a couple final edge cases around the version conflict path and the st nil-safety.
Now let me verify the version-conflict path returns a non-nil st so writeVersionConflict(c, st) doesn't serialize nil.
I have everything I need. Let me also check one thing: whether the version conflict path returns nil st on a requireAdmin failure (the errors.Is(err, ErrVersionConflict) check).
Now I've completed my investigation through the error-handling & edge-cases lens. Let me consolidate findings.
The version-conflict path: store returns (current, ErrVersionConflict), service returns it unchanged, handler writes st under current. Confirmed: current is non-nil on the conflict path (store returns current row). Safe.
The admin-gate path: service's requireAdmin returns (nil, ErrForbidden) when not admin — but the handler's errors.Is(err, ErrVersionConflict) is false there, so it falls to writeServiceError. Safe. (And middleware catches it first anyway.)
The parseNullable[bool] edge case: req.AgentEnabled is json.RawMessage. With an absent field, len(raw)==0 → returns (nil, false, nil) → inherit. With null → (nil, true, nil) → inherit. With true/false → pointer. With "yes" → unmarshal error → 400. Solid.
One concern — the rebuild race between EffectiveAgent read and NewRunner: if settings change concurrently the rebuildMu serializes. Fine.
A genuine edge-case issue: in rebuild, when NewRunner fails after a settings save that turned the assistant from working→broken, h.ptr.Store(nil) tears down the previously-working runner. That's documented as intentional. Not a bug.
Let me check one more: what happens at boot if the DB read in newAgentHolder fails (e.g., transient). rebuild logs and returns, leaving ptr nil. Boot proceeds. Good — non-fatal as documented.
Now the RequireAdmin middleware calls mustActor(c) which panics if actor not set. But requireAdmin runs after requireAuth which sets the actor. Safe ordering.
Let me double check the UpdateInstanceSettings validate-with-env-key concern: it validates against s.cfg.Agent.OllamaCloudAPIKey (env key), but the live runner uses the same env key. Consistent. But — agentmodel.Validate only parses, doesn't call. The test confirms Validate("", spec) succeeds. So validating with env key when key is absent still works. Edge case handled.
I've found no material error-handling issues. Let me write the verdict.
VERDICT: No material issues found
Reviewing the diff through the error-handling & edge-cases lens, I verified the unhappy paths and found them handled deliberately and correctly:
agentHolder.rebuild (agent_holder.go:60-86): EffectiveAgent failure is logged and the existing Runner is preserved (not torn down on a blip); NewRunner failure stores nil (tearing down a stale runner rather than silently keeping the old one) — both paths documented and intentional. Boot (newAgentHolder) is non-fatal on resolution failure, confirmed by reading api.go:141 + main.go:50 migration ordering.
store.UpdateInstanceSettings (instance_settings.go:70-76): sql.ErrNoRows (version mismatch) fetches the current row and returns (current, ErrVersionConflict); the secondary GetInstanceSettings error is propagated, not swallowed. The handler's errors.Is(err, ErrVersionConflict) branch is only entered on that sentinel, so the nil st from a non-conflict error (e.g. ErrForbidden from requireAdmin) flows to writeServiceError instead. Verified by reading settings.go:122-129 and the service's requireAdmin (instance_settings.go:21-30, returns ErrForbidden, not ErrVersionConflict).
parseNullable[bool] (settings.go:111): absent (len==0), explicit null, and a non-bool value are all distinct — the first two inherit, the third is a 400. Verified against parseNullable in errors.go:79-91.
scanInstanceSettings (instance_settings.go:19-32): agent_enabled NULL → sql.NullInt64.Valid=false → AgentEnabled stays nil (inherit); 0/1 → pointer to false/true. No nil-pointer deref.
agentChat (agent.go:58-67): the Runner is loaded once into a local before the guard and the Run call, so a mid-request swap can't flip it between the nil-check and use — exactly the race the comment calls out.
requireAdmin middleware (settings.go:22-31): runs after requireAuth (verified route group order in api.go), so mustActor (which panics on a missing actor) is safe.
Migration/seed ordering: Migrate runs before api.New (main.go:50 vs 57), so the seeded single row exists before any GetInstanceSettings read at boot. The store's sql.ErrNoRows branch is treated as a broken-DB fault, not papered over — correct given the seed.
Validate-without-a-key edge (agentmodel.go:54-57 + test): Validate only parses (no live call), so it works with an empty key; confirmed by TestValidate's no-key case.
No swallowed errors, no missing defers, no unchecked nils, and the boundary conditions (empty spec, NULL vs absent vs non-bool, version conflict, race during swap, transient DB fault at boot) are each handled with an explicit, documented decision.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
I reviewed the PR through the security lens across all changed files. Here's what I verified:
Admin enforcement — isAdmin is now checked in both the requireAdmin middleware (internal/api/settings.go:22-31) and authoritatively in the service seam (internal/service/instance_settings.go:21-30). Non-admins receive 403 on both GET and PATCH. Tests confirm this.
No API key leakage — OLLAMA_CLOUD_API_KEY never enters the database and is never serialized in the API. The settings response only exposes hasApiKey: boolean (internal/api/settings.go:49). The effectiveView struct deliberately omits the actual key.
Model spec validation — agentmodel.Validate resolves the spec through majordomo.Parse before storage (internal/service/instance_settings.go:63-66), so a typo is rejected at save time with 400. The spec is not used in SQL, not executed as shell code, and not rendered unsafely.
SQL safety — Migration and store code use parameterized queries throughout. The instanceSettingsColumns constant is hardcoded, not user-derived.
Race safety — The agentHolder uses atomic.Pointer for lock-free reads and sync.Mutex to serialize rebuilds. The chat handler snapshots the pointer once at entry (internal/api/agent.go:63), so a mid-request swap cannot tear.
Atomicity of version-conflict response — The version-conflict path returns the current row, but that row has no secret fields and is only reachable by admins (the service gate rejects non-admins before the store layer).
No SSRF/command injection via model spec — The spec goes only to majordomo.Parse against a registry that explicitly registers only the ollama.Cloud provider. There's no path for the spec to influence URLs, commands, or file paths outside majordomo's typed provider model.
Nothing in this PR introduces authz gaps, injection vectors, secret leakage, or unsafe handling of untrusted data.
🎯 Correctness — No material issues found
No material issues found
Through the Correctness lens, the change is solid. Key observations:
Atomic pointer swap is correct: agentHolder serializes rebuilds with sync.Mutex while reads (get()) stay lock-free via atomic.Pointer. The old Runner is replaced; new requests see the new one, and in-flight requests hold their reference until done. No use-after-free or torn-read risk.
Precedence layering is correct: EffectiveAgent applies DB overrides (agent_model non-empty, agent_enabled non-nil) atop env defaults. The env model can never be empty because config.Load() falls back to DefaultAgentModel when PANSY_AGENT_MODEL is unset or blank. The inherit sentinel ("" / nil) correctly falls through.
Validation gate is at the right layer: UpdateInstanceSettings validates the model spec via agentmodel.Validate before storing, so a bad spec is ErrInvalidInput (400) at save time. Empty model skips validation because it means "inherit" — the env value was already checked at boot when the initial Runner built (or failed to).
Migration schema is correct: agent_enabled INTEGER CHECK (agent_enabled IN (0, 1)) permits NULL because SQLite CHECK passes when the expression is not false (NULL is allowed). The comment in the PR notes explicitly calls this out, and it matches SQLite semantics.
503 contract change is handled end-to-end: The chat handler nil-checks agent.get() and returns 503 AGENT_DISABLED. The frontend's streamChat maps 503 to a human message, and useCapabilities drops staleTime: Infinity so the tab state reflects reality. The history routes remain on the gardens group (not agentGroup), so they stay accessible when the assistant is off, which is the stated design.
Context detachment in rebuild mirrors existing pattern: context.WithoutCancel in updateSettings before rebuild matches the commitScope/RecordAgentExchange pattern — once the DB write is committed, a client disconnect must not leave the live Runner out of step with stored settings.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/pages/SettingsPage.tsx:22 — fromChoice relies on a cryptic ternary (c === 'inherit' ? null : c === 'on') where 'off' silently falls through to false because 'off' !== 'on'. It happens to be correct today, but if EnabledChoice ever gains a value the compiler won't catch the missing branch and the function will silently misbehave. Use an explicit switch or if/else chain so every choice is handled deliberately.
internal/service/instance_settings.go:81 and :93 — EffectiveAgent is both a struct type and a *Service method in the same package. This hurts godoc readability and IDE navigation; searching for the type jumps to the method and vice-versa. Rename the method to something distinct like ResolveEffectiveAgent or CurrentAgentConfig.
internal/api/settings.go:63 / internal/service/instance_settings.go:94 — settingsPayload triggers a second GetInstanceSettings DB read even though its caller (getSettings) already holds the row. updateSettings compounds this: it calls EffectiveAgent in rebuild and again in settingsPayload, so a single PATCH can hit the same row three or four times. This is a leaky abstraction: EffectiveAgent should accept an already-loaded *domain.InstanceSettings (or expose a helper that layers settings over env without fetching), so callers that already have the row don't pay for redundant queries.
⚡ Performance — No material issues found
No material issues found
I examined the changes through the Performance lens and found nothing material.
The hot path (/capabilities, chat handler) uses atomic pointer loads (h.agent.get()) — lock-free and correct. No contention under read load.
agentHolder.rebuild serializes writes with a mutex, but rebuilds only happen on admin settings changes (rare). The lock is not held across any I/O; model resolution is an in-memory parse.
EffectiveAgent does re-read the settings row on every call, which means GET /settings and PATCH /settings issue an extra identical SELECT. This is real but cold-path (admin-only, low volume) and not material to overall system performance.
Runner replacement via atomic.Pointer.Store is safe; old runners become unreachable and are GC'd when in-flight requests finish. No unbounded growth.
No missing pagination/limits, N+1 queries in loops, unnecessary allocations in hot paths, or blocking calls on request-serving paths.
🧯 Error handling & edge cases — Minor issues
Minor issues
internal/service/instance_settings.go:64 — agentmodel.Validate returns a specific error (e.g. empty spec, unknown provider, malformed failover chain), but UpdateInstanceSettings swallows it and returns a generic domain.ErrInvalidInput. An admin POSTing a bad model gets a 400 with code INVALID_INPUT and message "invalid input", with no hint that it's the model spec that failed. The underlying err should at least be logged so an operator can diagnose it, or the API should surface it. Verified by reading internal/service/instance_settings.go and internal/api/errors.go:54.
internal/api/settings_test.go:180-208 — TestSettingsSwapUnderRace spawns 4 reader goroutines that loop until <-done, then closes done and returns immediately. The test does not wait for the goroutines to finish, so they may still be running (and calling doJSON, which uses testing.T) after the test function exits. This is a real race under go test -race that can panic or produce spurious race reports. Add a sync.WaitGroup and wait for the workers before returning. Verified by reading the test body and the goroutine spawn/exit pattern.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Minor issues** · 10 findings (2 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | EffectiveAgent re-reads instance_settings row that GetInstanceSettings just fetched | `internal/api/settings.go:63` | 2/5 | maintainability, performance |
| 🟡 | Stale doc comment references config.AgentConfig.Ready(), which this PR deletes | `internal/service/instance_settings.go:87` | 2/5 | maintainability |
<details><summary>8 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Test goroutines may outlive test function causing race/panic under -race | `internal/api/settings_test.go:180` | opencode/kimi-k2.6:cloud | error-handling |
| 🟠 | Swallowed validation error leaves admin with no diagnostic on bad model spec | `internal/service/instance_settings.go:64` | opencode/kimi-k2.6:cloud | error-handling |
| 🟠 | EffectiveAgent type and method share the same name in one package | `internal/service/instance_settings.go:81` | opencode/kimi-k2.6:cloud | maintainability |
| 🟠 | fromChoice silently falls through 'off' in cryptic ternary | `web/src/pages/SettingsPage.tsx:22` | opencode/kimi-k2.6:cloud | maintainability |
| 🟡 | PATCH /settings binds agentModel as a plain string (full-replacement semantics), so a request that omits it resets the stored model to "inherit" — intentional and documented, but inconsistent with the rest of the codebase's absent=unchanged PATCH convention | `internal/api/settings.go:96` | claude-code/sonnet | correctness |
| 🟡 | Enabled field doc says agent is 'the same shape as OIDC 404ing', but this PR changes agent-off to a 503 on an always-registered route | `internal/config/config.go:75` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | requireAdmin re-queries user table when IsAdmin already verified by middleware | `internal/service/instance_settings.go:27` | kimi-k2.6:cloud | performance |
| ⚪ | Stale/incorrect comment claims capabilities route registration order matters and is 'after the agent below', but it's registered before and order no longer affects behavior | `internal/api/api.go:59` | claude-code/sonnet | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Frontend only consumes the boolean, never handles a raw key. That confirms the secret-handling design holds end-to-end.
## Review
**VERDICT: No material issues found**
I checked this PR specifically for authn/authz gaps, injection, secret leakage, and insecure defaults around the new admin-gated Settings feature:
- **Admin enforcement is real and layered correctly.** `requireAdmin` (`internal/api/settings.go:24`) is a cheap early-403 reading the already-resolved actor; the authoritative check is `Service.requireAdmin` (`internal/service/instance_settings.go:15`), which re-reads `IsAdmin` from the DB per call rather than trusting a cached claim. Verified `ResolveSession` (`internal/service/auth.go:220`) re-fetches the session/user from the store on every request — no stale-admin-in-token risk.
- **No privilege-escalation path found.** Grepped for any endpoint that lets a non-admin set `is_admin`; none exists. `settingsUpdateRequest` (`internal/api/settings.go`) only exposes `agentModel`/`agentEnabled`/`version` — no mass-assignment surface.
- **Secret handling is correctly enforced end-to-end.** `OLLAMA_CLOUD_API_KEY` never enters the DB (migration 0010 has no key column) and the API response (`effectiveView.HasApiKey`) only ever serializes a boolean. Confirmed the frontend (`web/src/pages/SettingsPage.tsx`) only consumes that boolean and never touches a raw key.
- **CSRF coverage is unchanged/inherited.** `/settings` sits under `v1`, which has `csrfGuard()` applied globally (`internal/api/api.go:57`), so the new PATCH route is covered like every other mutating route.
- **Error responses don't leak internals.** `writeServiceError`'s `ErrInvalidInput` branch (`internal/api/errors.go:54`) returns a generic "invalid input" 400 — a rejected model spec doesn't echo the underlying parse error or any registry detail back to the client.
- **SQL is parameterized** in the new `instance_settings` store code — no injection surface.
- Considered whether an admin-supplied model spec could be an SSRF vector now that it's reachable via API instead of only an env var (`agentmodel.Resolve` → `majordomo.Parse`). Couldn't verify majordomo's `Parse`/`ollama.Cloud` internals (module cache is outside the sandboxed directory), so I'm not reporting this — flagging it here only as unverified, not as a finding.
Outside my lens: none worth flagging — the concurrency/rebuild-lock design and doc/test thoroughness are for other reviewers.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Confirmed. The `settingsUpdateRequest.AgentModel` field is indeed bound as a plain `string` (settings.go:96, not lines 39-45 as the draft cited — that range is actually `settingsResponse`/`effectiveView`), and `internal/service/instance_settings_test.go:70-71` shows the test suite must re-pass `AgentModel` on every update specifically to avoid resetting it, which corroborates the behavior. However, the draft's suggested fix is inaccurate: this **is** already documented prominently, in two doc comments — `internal/service/instance_settings.go:40-42` ("`InstanceSettingsPatch` is a full replacement of the editable fields... an empty AgentModel means fall back to env") and `internal/api/settings.go:92-94` ("agentModel `""` means inherit the env var") — at the same level of prominence as the `objectUpdateRequest` comment the draft cites for contrast. So this is a real, confirmed deviation from the rest of the codebase's convention, but a deliberate and clearly-documented one, not an oversight.
## Review — 🎯 Correctness lens
**VERDICT: Minor issues**
- `internal/api/settings.go:96` (`settingsUpdateRequest.AgentModel`) — bound as a plain `string`, not the `*string`/`json.RawMessage` "absent vs. explicit" pattern used elsewhere (contrast `objectUpdateRequest` in `internal/api/objects.go:50-65`). Since `""` is the domain's own sentinel for "inherit from environment" (`internal/service/instance_settings.go:60-67`, `EffectiveAgent` at line 103), a PATCH that omits `agentModel` entirely is indistinguishable from one that explicitly clears it, so `{"agentEnabled": false, "version": N}` alone silently resets the stored model back to "inherit env." This is confirmed by `internal/service/instance_settings_test.go:70-71`, where the test must re-supply `AgentModel` on every subsequent update specifically to avoid this reset.
- This behavior is intentional and explicitly documented as "a full replacement of the editable fields" (`internal/service/instance_settings.go:40-42`) and "agentModel `""` means inherit the env var" (`internal/api/settings.go:92-94`) — at the same prominence as the "absent = unchanged" doc comment on `objectUpdateRequest` that provides the contrast. It is also currently unreachable in the shipped product: `web/src/pages/SettingsPage.tsx:61-64` is a single form that always submits `agentModel` and `agentEnabled` together.
- Still worth flagging: it's a real inconsistency with the "omit = unchanged" convention every other mutable-resource PATCH in this codebase trains callers to expect, and it's live against any future script, curl call, or split-out form that PATCHes `agentEnabled` alone.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings confirmed against the actual code:
1. `config.AgentConfig` has no `Ready` method anywhere in the tree (grep confirms), yet `internal/service/instance_settings.go:87`'s comment still says "Ready mirrors config.AgentConfig.Ready".
2. `internal/api/api.go:62` registers `/capabilities` at line 62, while `h.agent` isn't assigned until line 141 — after, not before. The comment's claim is backwards, and it's superseded by the accurate doc on `capabilities` at lines 194-200.
## Review
**VERDICT: Minor issues**
- `internal/service/instance_settings.go:87` — `EffectiveAgent.Ready()`'s doc comment says "Ready mirrors config.AgentConfig.Ready", but this same PR *deletes* `config.AgentConfig.Ready` (confirmed via grep — no remaining references anywhere in the tree, including `internal/config/config.go`). The comment now points at dead code that no longer exists, which CLAUDE.md calls out explicitly ("Stale comments are worse than none — the next reader believes them"). Fix: drop the "mirrors config.AgentConfig.Ready" clause or just describe the check plainly (`enabled, with a key and a model`), which the second half of the sentence already does.
- `internal/api/api.go:59-61` — the comment above `v1.GET("/capabilities", h.capabilities)` still reads "Registered after the agent below, because it reports whether the runner actually built." This was true of the pre-#79 shape, but post-refactor `capabilities` reads `h.agent.get()` off an atomic pointer at request time — registration order is irrelevant now, and the route is in fact registered *before* `h.agent = newAgentHolder(...)` at line 141, contradicting the comment's own claim. The accurate rationale is already given in the `capabilities` func doc a few lines down (194-200), so this stale comment is both wrong and redundant. Suggest deleting it or replacing with a one-liner noting registration order no longer matters here.
Everything else touched by this PR (`agentHolder`, `settings.go`, `instance_settings.go` in service/store, `agentmodel` package, migration, frontend `SettingsPage.tsx`/`settings.ts`) is well-factored: single-responsibility functions, no copy-pasted logic, naming consistent with the rest of the codebase, and the "why" comments earn their keep. No structural or duplication concerns beyond the two stale comments above.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No polling loop for settings — it fetches once via React Query defaults. Nothing performance-relevant there. This confirms the read path (chat, capabilities) stays cheap (single atomic load), and the settings admin path, while doing a couple of redundant `EffectiveAgent` DB reads per request, operates on a single-row table with no scan cost and is admin-only/low-frequency — not a justifiable regression.
## VERDICT: No material issues found
Reviewed through the performance lens:
- **Hot path (chat/capabilities) is unaffected or improved.** `agentChat` (`internal/api/agent.go:61`) now does one `atomic.Pointer` load (`h.agent.get()`) instead of a plain field read — same O(1) cost, and it's loaded once up front rather than re-read, avoiding any redundant load. `capabilities` (`internal/api/api.go`) is likewise a single atomic load. Verified `agentHolder.get()` (`internal/api/agent_holder.go:44`) is lock-free.
- **`agentHolder.rebuild`** (`internal/api/agent_holder.go:52`) takes a mutex and does a DB read (`EffectiveAgent`) plus builds a fresh majordomo registry — but this only runs at boot and on an admin settings PATCH, never per-request. No hot-loop concern.
- **Redundant `EffectiveAgent` DB reads on settings PATCH**: `updateSettings` (`internal/api/settings.go`) calls `h.agent.rebuild` (which internally calls `EffectiveAgent`, one `SELECT` on `instance_settings`) and then `settingsPayload` (which calls `EffectiveAgent` again) — three DB round trips total for one PATCH (`UPDATE...RETURNING`, then two single-row `SELECT`s). This is technically avoidable, but it's an admin-only, low-frequency endpoint against a single-row table with no scan cost, so the impact isn't justifiable as a real regression.
- **Frontend polling**: `useCapabilities` (`web/src/lib/agent.ts:30`) drops `staleTime: Infinity` for `staleTime: 60_000` — a bounded, sane refetch interval, not aggressive polling. `useSettings` has no `refetchInterval`. No new client-side hot-loop.
Nothing here rises to a genuine efficiency regression under this lens.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
I reviewed this PR specifically for ignored/swallowed errors, missing cleanup, panics on bad input, and unhandled edge cases (nil, empty, zero/negative, off-by-one), verifying against the checked-out code rather than the diff alone.
What I checked and found sound:
- **`internal/api/agent_holder.go`** — `rebuild()` correctly serializes concurrent settings saves via `rebuildMu` while keeping `get()` lock-free. An `EffectiveAgent` read failure is logged and the old Runner is deliberately kept (not torn down on a transient blip) — this tradeoff is explicitly reasoned about in the comments, not an oversight. A resolvable-config-but-unresolvable-model correctly swaps to `nil` rather than leaving a stale Runner. Verified `newAgentHolder` never leaves the pointer in a panicking state on boot failure.
- **`internal/api/agent.go`** — `runner := h.agent.get()` is loaded once and reused for both the nil-check and the `Run` call, avoiding a TOCTOU gap where a concurrent settings swap could nil out the runner mid-request. Verified `getAgentHistory`/`deleteAgentHistory` don't touch `h.agent` at all, so they can't nil-panic when the assistant is off.
- **`internal/store/instance_settings.go`** — `scanInstanceSettings` correctly handles the nullable `agent_enabled` via `sql.NullInt64`; `UpdateInstanceSettings` correctly passes untyped `nil` to bind SQL NULL (restoring "inherit" from an explicit override), and the version-mismatch path re-fetches and returns the current row with `ErrVersionConflict`, consistent with every other mutable resource in the codebase. `GetInstanceSettings`'s `sql.ErrNoRows` case is correctly distinguished as "migration not applied" rather than papered over.
- **`internal/service/instance_settings.go`** — `UpdateInstanceSettings` validates a non-empty model *before* writing (verified `TestUpdateInstanceSettingsRejectsBadModel` confirms the row is untouched on rejection); `requireAdmin` propagates a `GetUserByID` error unchanged rather than swallowing it into a wrong error class.
- **`internal/agentmodel/agentmodel.go`** — `Resolve`/`Validate` correctly reject empty/whitespace-only specs via `TrimSpace`, tested for both the key-present and key-absent cases.
- **`internal/api/settings.go`** — `parseNullable[bool]` correctly treats absent vs. explicit `null` vs. `true`/`false`; a non-bool value (`"yes"`) is verified (test + code read) to 400 rather than silently coercing. `Version int64` with `binding:"required"` mirrors the existing pattern in `journal.go`/`seed_lots.go`, and the migration seeds `version = 1`, so the zero-value/`required` gotcha doesn't bite here.
- **Frontend (`settings.ts`, `SettingsPage.tsx`)** — mutation error path distinguishes a 409 (rebases the form to the server's current row and warns the user rather than silently discarding their edit) from other errors; loading/error/pending states are all handled before rendering the form.
I did not find any unhandled nil/empty/boundary case, swallowed error, or missing cleanup that isn't already covered by the extensive test suite (version conflicts, bad-model rejection, disabled-key 503, admin-gate 403, and the race test).
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 4/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2:cloud: skipped (backed off until 02:18:17.784)
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT: No material issues found**
I verified the correctness-critical paths of this change by reading the actual code, not just the diff:
- **Layering / precedence** (`internal/service/instance_settings.go`): `EffectiveAgent` seeds from `s.cfg.Agent.Model` (env-resolved, with the `DefaultAgentModel` fallback already applied by `config.Load`) and `s.cfg.Agent.Enabled`, then overrides `Model` only when `st.AgentModel != ""` and `Enabled` only when `st.AgentEnabled != nil`. The "inherit" sentinels (`''` / `nil`) correctly fall through. Re-derived `Ready() = Enabled && APIKey != "" && Model != ""` against the seeded row (`agent_model=''`, `agent_enabled=NULL`) — yields the env/default values, so an untouched instance behaves as before. ✓
- **Migration / nullability** (`0010_instance_settings.sql`): `agent_enabled INTEGER CHECK (agent_enabled IN (0,1))` permits NULL (a CHECK only fails on *false*, and `NULL IN (0,1)` is NULL, not false). Seed inserts `(1, '', NULL)`. Store scans via `sql.NullInt64` and maps `enabled.Valid` → `*bool`, else leaves `nil`. Correct tri-state. ✓
- **Optimistic concurrency** (`internal/store/instance_settings.go`): `UPDATE ... WHERE id = 1 AND version = ? RETURNING ...`; `sql.ErrNoRows` → fetch current row + `ErrVersionConflict`; success bumps `version+1`. Seeded version=1, so the first PATCH with version=1 matches. The 409 path carries the current row (`writeVersionConflict`). ✓
- **Hot-swap race safety** (`internal/api/agent.go`): `agentChat` loads `runner := h.agent.get()` once and nil-checks before binding the request, then uses that local `runner.Run` — a mid-request swap cannot flip the pointer between guard and use. `rebuild` serializes writers via `rebuildMu` while reads stay lock-free on the atomic. ✓
- **History routes ungated on the Runner**: `getAgentHistory`/`deleteAgentHistory` use `h.svc`, never `h.agent`, so they correctly return 200/204 regardless of whether a Runner exists (verified at `agent.go:193-217`). ✓
- **Validate-before-store** (`internal/service/instance_settings.go`): `agentmodel.Validate(s.cfg.Agent.OllamaCloudAPIKey, model)` runs on non-empty specs before the UPDATE; `Validate` calls `Resolve`, which only parses (no live call), so it works even with an empty env key. A typo → `ErrInvalidInput` → 400, row untouched. ✓
- **`agentModel` binding**: a plain `string` field means a JSON `null` unmarshals to `""` (the inherit sentinel), consistent with an explicit empty string. `strings.TrimSpace` in the service also collapses whitespace-only to `""`. ✓
- **`agentEnabled` three-way decode**: `parseNullable[bool]` returns `(nil,false)` for absent and `(nil,true)` for explicit null — both yield `enabled=nil` (inherit). Verified at `errors.go:79`. ✓
No semantic, formula, threshold, or boundary-value defect surfaced under the correctness lens. (One doc-only imprecision — the `domain.InstanceSettings` comment says AgentModel `""` "falls back to PANSY_AGENT_MODEL", when it actually falls back to `cfg.Agent.Model`, which is env-or-`DefaultAgentModel`. This is a comment inaccuracy, not a behavior bug, so I am not reporting it as a finding.)
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2:cloud: skipped (backed off until 02:18:17.784)
</details>
<details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2:cloud: skipped (backed off until 02:18:17.784)
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2:cloud: ollama-cloud/glm-5.2:cloud: HTTP 429: too many concurrent requests
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 4/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: ollama-cloud/kimi-k2.6:cloud: HTTP 429: too many concurrent requests
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 2: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: ollama-cloud/kimi-k2.6:cloud: HTTP 429: too many concurrent requests
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: ollama-cloud/kimi-k2.6:cloud: HTTP 429: too many concurrent requests
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Minor issues**
**No material issues found** in hot paths — the atomic pointer swap is clean, capabilities reads are lock-free, and the admin endpoints are low-traffic. Two layering inefficiencies in the settings request paths are worth fixing:
- `internal/api/settings.go:63` and `internal/api/settings.go:141` — `settingsPayload` calls `h.svc.EffectiveAgent`, which always does a fresh `GetInstanceSettings` DB read. In `getSettings`, the same row was just fetched by `GetInstanceSettings`. In `updateSettings`, the row was just written and returned, then `rebuild` calls `EffectiveAgent` (second read), then `settingsPayload` calls it again (third read). A single PATCH can hit the same row **3 times**. Pass the already-loaded row into `EffectiveAgent` (or add an `EffectiveAgentFrom` variant) to cut this to one read.
*Verified by reading `internal/service/instance_settings.go:93-110` and tracing the call chain in `settings.go` and `agent_holder.go:60`.*
- `internal/service/instance_settings.go:27-36` — `requireAdmin` queries `GetUserByID` to check `IsAdmin` on every settings request, but `mustActor(c).IsAdmin` was already resolved during session authentication and checked by the `requireAdmin()` middleware. This adds a DB roundtrip to an already-gated endpoint. The author calls this "redundant by design" (service seam authority), which is fair for correctness, but from a performance lens it's still avoidable overhead on an endpoint that does little else. Consider passing the resolved user (or a small admin flag) into the service call, or accepting the middleware as sufficient and skipping the service-layer re-query for this read-only gate.
*Verified by reading `internal/api/auth.go:109` (session resolution loads the full user), `internal/api/settings.go:28-36` (middleware checks `IsAdmin`), and `internal/service/instance_settings.go:27-36` (re-queries the DB).*
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 1: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: ollama-cloud/kimi-k2.6:cloud: HTTP 429: too many concurrent requests
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
The security model looks solid: requireAuth resolves the user (and the middleware + service both re-check IsAdmin), the key never enters the DB, and the settings row is admin-gated. Let me check a few more things — particularly how `effective.model` is exposed and whether model spec is a path-traversal or injection risk through `Parse`.
I've reviewed the change through the security lens, verifying the auth chain, secret handling, SQL parameterization, and input validation against the actual checked-out code.
**VERDICT: No material issues found**
Security-relevant verification I did:
- `internal/api/settings.go:22` `requireAdmin` middleware reads `mustActor(c).IsAdmin` from the already-resolved session user (`requireAuth` at `auth.go:109` calls `svc.ResolveSession` and stashes the user). Layered with `internal/service/instance_settings.go:21` `requireAdmin`, which re-fetches the user from the DB — so a demotion between middleware and service is caught. Redundant by design, as the PR notes.
- `internal/api/settings.go:44-54` `effectiveView` exposes only `hasApiKey: bool`, never the key. The `EffectiveAgent.APIKey` field (`instance_settings.go:84`) is used solely to build the Runner; no JSON tag serializes it. Confirmed the key never enters `instance_settings` (migration 0010 has no key column).
- `internal/store/instance_settings.go:62-68` UPDATE uses `?` placeholders for `agent_model`, `agent_enabled`, `version`; no string interpolation of user input into SQL. Same for the SELECT.
- `internal/service/instance_settings.go:60-67` `agentModel` is trimmed and validated via `agentmodel.Validate` → `Resolve` → `majordomo.Parse` before storage; unresolvable specs return `ErrInvalidInput` (400). `agentEnabled` is decoded with `parseNullable[bool]` (`errors.go:79`) which rejects non-bool JSON. `version` is `binding:"required"`.
- No path/command injection surface: the spec is opaque to pansy and passed verbatim to the vendor parser; pansy never interprets it as a path or shell argument.
- `EffectiveAgent` (the unauthenticated code path used by `agentHolder.rebuild`) only reads the settings row and env — it does not expose anything to a caller, and `rebuild` logs the model but never the key (`agent_holder.go:78` logs `eff.Model`, not `eff.APIKey`).
No security findings to report.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've reviewed the key parts. Now let me check the migration of `cfg.Agent.Enabled` defaulting - there's a subtle issue worth checking. The `EffectiveAgent` layers `Enabled: s.cfg.Agent.Enabled` where the env default is "on when a key is present". Let me verify the config-level `Enabled` semantics are actually correct.
Now let me check one specific potential correctness issue: what happens at boot if `PANSY_AGENT_ENABLED=false` but `OLLAMA_CLOUD_API_KEY` set? The env default for `cfg.Agent.Enabled` resolves to `false`. Then a settings change that turns on the agent... let me check the validate flow. Validate uses the env key regardless. That's fine.
Let me also check one subtle issue: the `Enabled` env default of "on when key present". When admin explicitly sets `agentEnabled=null` (inherit), `EffectiveAgent.Enabled = s.cfg.Agent.Enabled`. That's `true` when a key is present (correct inherit semantics). Good.
Now check the test settings response `agentModel` round-trip for an issue with the `agentEnabled` field marshaling. The `domain.InstanceSettings.AgentEnabled *bool` with `json:"agentEnabled"` — when nil, marshals to `null`. The store scan returns `nil` if invalid. Good.
Let me verify one more thing — the `updateSettings` race test hammering capabilities while writer does PATCH. There's a potential issue: when `i%2==0` flips enabled=true but agentModel="" inherit. With `agentCfg()` env model is `ollama-cloud/glm-5.2:cloud`, so resolving works. Good.
Let me verify the rebuild ordering with respect to the response. The handler calls `h.agent.rebuild(...)` then `settingsPayload`. But the rebuild is serialized via mutex; `settingsPayload` reads `h.agent.get()` which is atomic. Could a second concurrent PATCH interleave between rebuild and payload read? It would just reflect whichever swap happened. Not a correctness bug, just a stale-view possibility. Acceptable.
Now let me check one final potential issue - the chat route reads `runner := h.agent.get()` early, but the history fetch happens after. If the runner is swapped to nil after the get but before Run, we use a stale (now-disabled) runner. That's a benign TOCTOU since the runner is still valid. Fine.
Let me verify the more subtle issue: `updateSettings` calls `h.svc.UpdateInstanceSettings` which does `requireAdmin` again, then `h.agent.rebuild` — but the rebuild calls `h.svc.EffectiveAgent` which **doesn't** check admin. That's fine by design (it's internal).
Now check `Validate` semantics. The service validates the model with the env key. If the key is missing but admin sets a model spec, `Validate("", "ollama-cloud/x")` — does Parse work with empty token? The test asserts `Validate("", "ollama-cloud/glm-5.2:cloud")` succeeds. So yes.
Let me check one thing - the store `UpdateInstanceSettings` returning `(nil, gerr)` when the current row read fails after a version conflict. The handler then does `writeVersionConflict(c, st)` with nil `st`. Let me check `writeVersionConflict`:
Looking at errors.go:66 - `c.JSON(http.StatusConflict, gin.H{"error": ..., "current": current})` with `current=nil` → marshals to `"current":null`. Not a crash, just a degraded 409. Edge case but fine.
Now let me look for the real correctness issue: when `UpdateInstanceSettings` succeeds, the returned `st` is the updated row, which is what gets sent in the response. But the `rebuild` happens BEFORE `settingsPayload`. The settingsPayload calls `EffectiveAgent` which re-reads the row. Good, consistent.
Let me check whether there's a subtle ordering issue with `rebuild` happening inside the mutex BEFORE the response is built. If rebuild fails (e.g. NewRunner error), the response still says success with `AgentLive: false`. That's correct.
Let me now check the critical edge case: `agentHolder.rebuild` when `eff.Ready()` is false uses `h.ptr.Swap(nil)`. The `Swap` returns old value. If old was nil and new is nil, no log. Good. But it uses `Store(nil)` in the NewRunner error path instead of Swap, which doesn't log "turned off" in that case but does log the error. Fine.
Now let me check one potentially real issue: the `agentModel` PATCH validation. When admin sends `agentEnabled: false` AND `agentModel: "nonesuch/model"` — the service validates the model first and returns `ErrInvalidInput` WITHOUT touching the row. But the test `TestSettingsRejectsBadModel` sends them separately. What if both are sent together? Validate runs first, returns early, no update. Correct.
Let me check the frontend side briefly to ensure capability polling is consistent.
Now let me verify the potential correctness issue I want to confirm: when `Enabled` env default is `true` (key present) and admin has not touched settings, `EffectiveAgent.Enabled = s.cfg.Agent.Enabled = true`. Good. When env says `PANSY_AGENT_ENABLED=false` explicitly, `s.cfg.Agent.Enabled = false`, and admin with `agentEnabled=null` inherits false. Correct.
Let me check one more subtle thing — the rebuild on the very first ever `newAgentHolder` call at boot. It uses `context.Background()`. If the DB hasn't been migrated yet... no, migrations run before `New`. Fine.
Let me check one final concern: `UpdateInstanceSettings` returns the updated row but `agentmodel.Validate` swallows the actual error and returns `ErrInvalidInput`. The test confirms this. But could a *valid* spec be rejected because the env key is invalid? Validate uses `s.cfg.Agent.OllamaCloudAPIKey` — even an empty key works (test confirms). So a non-empty but invalid key still allows Parse to succeed (Parse doesn't call). Good.
I've verified enough. Let me check one last potential issue: the `agentModel` field trimming. Service does `model := strings.TrimSpace(patch.AgentModel)` and stores the trimmed version. So if admin sends `" ollama-cloud/x "`, it's stored trimmed. Good. But `EffectiveAgent` returns the stored (trimmed) value, and `agentmodel.Resolve` re-trims. Fine.
One subtle thing: the test `TestSettingsInheritFromEnv` checks `eff["hasApiKey"] != true`. With `agentCfg()` key is "test-key". Good.
Let me verify there isn't an issue with the `agentEnabled` JSON marshaling when it's an explicit `false`. The store scans to `*bool` pointing to false. The domain struct marshals `*bool` → `false`. Good. When nil → `null`. Good.
I'm satisfied. Let me also quickly check whether `record` is called for instance settings updates — CLAUDE.md says "every service mutation lands in history".
Confirmed: `UpdateInstanceSettings` does NOT call `record`. CLAUDE.md: "Every service mutation lands in history (#48). If you add one, record it."
This is an instance-level setting, not a garden-scoped change. Let me verify whether the change-set system supports instance-level history. Looking at `WriteChangeSet`:
The change-set system is garden-scoped (`GardenID` is required). Instance settings are not garden-scoped, so the `record` invariant doesn't naturally apply. The PR description even notes "secrets never move here — so it doesn't land in ... the undo history's blast radius" — suggesting instance settings are intentionally outside the history system. So not calling `record` is by design here, not a bug. Not a finding.
Let me do one final check on the `boolToInt` usage to make sure the storage round-trips correctly. `boolToInt(false) = 0`, `boolToInt(true) = 1`. The scan does `enabled.Int64 != 0` → false for 0, true for 1. Correct round-trip.
I've thoroughly verified the change. My lens (correctness) finds no material issues. The semantics of the inherit sentinels, the version-guarded update, the atomic pointer swap, the admin gate, and the precedence layering are all correct. The one TOCTOU window (runner swap between get and Run) is benign and documented. The 409-with-nil-current edge case is a minor degradation, not a correctness bug for the happy path.
**Verdict: No material issues found**
- **Verified clean (correctness lens):**
- `internal/service/instance_settings.go:93-110` `EffectiveAgent` layering: re-derived precedence (Settings → env → default) from `cfg.Agent` defaults (`Enabled: agentKey != ""`, `Model: DefaultAgentModel`). Inherit sentinels (`""` / `nil`) fall through to env correctly; explicit overrides replace. Round-trips with the test matrix.
- `internal/store/instance_settings.go:56-80` `UpdateInstanceSettings`: version guard `WHERE id = 1 AND version = ?` + `version = version + 1`; on `sql.ErrNoRows` returns `(current, ErrVersionConflict)`. `boolToInt` round-trips `*bool` ↔ INTEGER 0/1; `NULL` (nil) preserved through `var enabled any` defaulting to nil. Re-derived from the SQL and the migration's `CHECK (agent_enabled IN (0,1))` permitting NULL — correct.
- `internal/api/agent_holder.go:56-89` `rebuild`: serializes via `rebuildMu`; `Ready()` false → `Swap(nil)`; resolution failure → `Store(nil)` (intentionally tears down stale runner); success → `Store(runner)`. Atomic semantics match the race test. The TOCTOU window in `agentChat` (runner loaded, then `Run` called later) is benign — a swapped-out runner remains a valid handle; documented in code.
- `internal/api/settings.go:101-143` `updateSettings`: validates `agentEnabled` via `parseNullable[bool]` (absent/null → nil = inherit; true/false → override), validates model spec via service before persisting, rebuilds the live runner with `context.WithoutCancel` (matches the documented detached-write pattern), then returns the fresh row. 409 path passes the returned `st` to `writeVersionConflict`; the only degradation (nil `current` if the conflict-row read itself fails) is an edge case in `store.UpdateInstanceSettings`, not a happy-path bug.
- `internal/agentmodel/agentmodel.go` `Resolve`/`Validate`: empty/blank spec rejected before `Parse`; Parse gets the spec verbatim. `Validate` with empty key succeeds (Parse doesn't call upstream) — confirmed by `agentmodel_test.go`. Service validates with the env key, which is correct since the key is never stored.
- `internal/api/agent.go:63-67` 503 contract: route always registered, nil-check on the loaded runner, runner cached in a local so a mid-request swap can't flip the guard/Run pair. Matches the test's `TestAgentDisabledWithoutAKey`.
- Migration 0010: `CHECK id = 1` + seed row means reads never branch; `agent_enabled` nullable with `CHECK (… IN (0,1))` permits NULL (SQL CHECK passes unless false) — re-derived from SQLite semantics, correct.
- **Investigated and dismissed:**
- `UpdateInstanceSettings` not calling `record` — by design: the change-set system is garden-scoped (`ChangeSet.GardenID` required), and the PR explicitly excludes instance settings from the undo history's blast radius. Not a violation of the #48 invariant, which targets garden mutations.
- `writeVersionConflict(c, st)` with a possibly-nil `st` (if `store.GetInstanceSettings` fails after the conflict) — degrades to `"current": null`, not a crash; pathological DB-fault path, not a happy-path correctness bug.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
I'll verify the two draft findings against the actual code.
Both findings confirmed.
1. `internal/service/instance_settings.go:87` — comment references `config.AgentConfig.Ready`, which no longer exists (grep confirms zero definitions). The `EffectiveAgent.Ready` method exists but the comment points at the deleted one.
2. `internal/config/config.go:75` — the `Enabled` field doc says "the same shape as OIDC 404ing when unconfigured," but this PR's contract change is that agent routes are now always registered (503 when off). Let me verify the api.go registration change to confirm.
Both findings are confirmed. The agent chat route is always registered (api.go:147) and returns 503/AGENT_DISABLED when off (agent.go:65), confirming the OIDC-404ing analogy at config.go:75 is now stale.
**VERDICT: Minor issues**
- **`internal/service/instance_settings.go:87`** — The doc comment `// Ready mirrors config.AgentConfig.Ready: enabled, with a key and a model.` references `config.AgentConfig.Ready()`, a method this PR deletes. Grep across the repo confirms zero remaining definitions of `AgentConfig.Ready`. The `EffectiveAgent.Ready` method still exists but the comment points at a now-removed symbol — exactly the "stale comments are worse than none" case CLAUDE.md flags. Fix: reword to describe `EffectiveAgent.Ready` on its own terms (e.g. "Ready reports whether the assistant can be offered: enabled, with a key and a model.") rather than mirroring the removed method.
- **`internal/config/config.go:75`** — The `Enabled` field doc ends with "the same shape as OIDC 404ing when unconfigured." This PR's central contract change is that the agent routes are now registered *unconditionally* (off = 503 `AGENT_DISABLED` on a live route, confirmed at `internal/api/agent.go:65` and `internal/api/api.go:147`, not a missing-route 404), so the OIDC-404ing analogy is now wrong for the agent path. The PR edits this file (it deleted `Ready()` immediately below at what is now the `OIDCConfig.Enabled` boundary), so the stale comment is in scope. Fix: drop the comparison or reword to reflect the new 503 shape (e.g. "...starts cleanly with the assistant off and reported as unavailable via `/capabilities`").
Both are comment-only fixes; no logic is affected.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
The performance-critical paths (chat, capabilities polled by the editor) are unchanged in cost or improved (atomic load, no DB). Let me verify the redundant DB reads in the settings handlers.
I've verified the hot paths (chat, capabilities) and the new settings paths against the checked-out code. Here's my assessment through the performance lens.
**VERDICT: No material issues found**
Through the performance lens:
- **Hot paths are unchanged or improved.** `agentChat` (`internal/api/agent.go:63`) reads the runner via `h.agent.get()` — a single `atomic.Pointer.Load`, cheaper than the old struct-field read and race-clean. `capabilities` (`internal/api/api.go:202`) is the same atomic load. Neither adds a DB query or allocation per request. The per-turn chat work (`runner.Run`) is untouched.
- **`rebuild` does not contend with reads.** `rebuildMu` serializes only rebuild calls (rare: settings saves); `get()` is lock-free on the atomic pointer (`internal/api/agent_holder.go:45,57`). A settings save cannot block chat or capabilities reads.
- **Redundant DB reads on the settings path are on a cold path.** `getSettings` reads the row via `GetInstanceSettings` then re-reads it via `EffectiveAgent` in `settingsPayload` (`internal/api/settings.go:79,63`); `updateSettings` reads it a third time via `rebuild`→`EffectiveAgent` (`internal/api/settings.go:135` + `internal/api/agent_holder.go:60`). All three hit the same single-row `SELECT` by `id = 1`. This is wasteful but only an admin saving settings triggers it — not a hot path, and the query is point-looked by PK. Not material.
- **`agentmodel.Resolve` allocates a fresh `majordomo.Registry` per call** (`internal/agentmodel/agentmodel.go:28-31`), invoked on boot and on each settings save (once via `Validate`, once via `rebuild`→`NewRunner`). Again, cold paths only; the chat hot path reuses the already-built `Runner`.
- **`useCapabilities` changed from `staleTime: Infinity` to `60_000`** (`web/src/lib/agent.ts:30`), so every mounted editor now refetches `/capabilities` after 60s of staleness (and on focus). This is a new periodic poll, but the endpoint is a single atomic load + tiny JSON, and the interval is deliberate (to pick up another admin's change). Not a regression worth flagging.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I'll review this PR through the error-handling/edge-cases lens. Let me verify the key files.
Now let me check the migration runner — does pansy handle the case where migration 0010 isn't applied yet but code tries to read settings at boot?
Let me verify the error-message flow for the bad-model case more closely.
Now I have a thorough understanding. Let me verify a couple final edge cases around the version conflict path and the `st` nil-safety.
Now let me verify the version-conflict path returns a non-nil `st` so `writeVersionConflict(c, st)` doesn't serialize nil.
I have everything I need. Let me also check one thing: whether the version conflict path returns nil `st` on a requireAdmin failure (the `errors.Is(err, ErrVersionConflict)` check).
Now I've completed my investigation through the error-handling & edge-cases lens. Let me consolidate findings.
The version-conflict path: store returns `(current, ErrVersionConflict)`, service returns it unchanged, handler writes `st` under `current`. Confirmed: `current` is non-nil on the conflict path (store returns current row). Safe.
The admin-gate path: service's `requireAdmin` returns `(nil, ErrForbidden)` when not admin — but the handler's `errors.Is(err, ErrVersionConflict)` is false there, so it falls to `writeServiceError`. Safe. (And middleware catches it first anyway.)
The `parseNullable[bool]` edge case: `req.AgentEnabled` is `json.RawMessage`. With an absent field, `len(raw)==0` → returns `(nil, false, nil)` → inherit. With `null` → `(nil, true, nil)` → inherit. With `true`/`false` → pointer. With `"yes"` → unmarshal error → 400. Solid.
`scanInstanceSettings` empty handling: `agent_enabled` is `sql.NullInt64`; NULL → `enabled.Valid=false` → `AgentEnabled` stays nil. Good.
One concern — the `rebuild` race between `EffectiveAgent` read and `NewRunner`: if settings change concurrently the rebuildMu serializes. Fine.
A genuine edge-case issue: in `rebuild`, when `NewRunner` fails after a settings save that turned the assistant from working→broken, `h.ptr.Store(nil)` tears down the previously-working runner. That's documented as intentional. Not a bug.
Let me check one more: what happens at boot if the DB read in `newAgentHolder` fails (e.g., transient). `rebuild` logs and returns, leaving `ptr` nil. Boot proceeds. Good — non-fatal as documented.
Now the `RequireAdmin` middleware calls `mustActor(c)` which panics if actor not set. But `requireAdmin` runs after `requireAuth` which sets the actor. Safe ordering.
Let me double check the `UpdateInstanceSettings` validate-with-env-key concern: it validates against `s.cfg.Agent.OllamaCloudAPIKey` (env key), but the live runner uses the same env key. Consistent. But — `agentmodel.Validate` only parses, doesn't call. The test confirms `Validate("", spec)` succeeds. So validating with env key when key is absent still works. Edge case handled.
I've found no material error-handling issues. Let me write the verdict.
**VERDICT: No material issues found**
Reviewing the diff through the error-handling & edge-cases lens, I verified the unhappy paths and found them handled deliberately and correctly:
- **`agentHolder.rebuild`** (`agent_holder.go:60-86`): `EffectiveAgent` failure is logged and the existing Runner is preserved (not torn down on a blip); `NewRunner` failure stores nil (tearing down a stale runner rather than silently keeping the old one) — both paths documented and intentional. Boot (`newAgentHolder`) is non-fatal on resolution failure, confirmed by reading `api.go:141` + `main.go:50` migration ordering.
- **`store.UpdateInstanceSettings`** (`instance_settings.go:70-76`): `sql.ErrNoRows` (version mismatch) fetches the current row and returns `(current, ErrVersionConflict)`; the secondary `GetInstanceSettings` error is propagated, not swallowed. The handler's `errors.Is(err, ErrVersionConflict)` branch is only entered on that sentinel, so the `nil st` from a non-conflict error (e.g. `ErrForbidden` from `requireAdmin`) flows to `writeServiceError` instead. Verified by reading `settings.go:122-129` and the service's `requireAdmin` (`instance_settings.go:21-30`, returns `ErrForbidden`, not `ErrVersionConflict`).
- **`parseNullable[bool]`** (`settings.go:111`): absent (`len==0`), explicit `null`, and a non-bool value are all distinct — the first two inherit, the third is a 400. Verified against `parseNullable` in `errors.go:79-91`.
- **`scanInstanceSettings`** (`instance_settings.go:19-32`): `agent_enabled` NULL → `sql.NullInt64.Valid=false` → `AgentEnabled` stays nil (inherit); 0/1 → pointer to false/true. No nil-pointer deref.
- **`agentChat`** (`agent.go:58-67`): the Runner is loaded once into a local before the guard and the `Run` call, so a mid-request swap can't flip it between the nil-check and use — exactly the race the comment calls out.
- **`requireAdmin` middleware** (`settings.go:22-31`): runs after `requireAuth` (verified route group order in `api.go`), so `mustActor` (which panics on a missing actor) is safe.
- **Migration/seed ordering**: `Migrate` runs before `api.New` (`main.go:50` vs `57`), so the seeded single row exists before any `GetInstanceSettings` read at boot. The store's `sql.ErrNoRows` branch is treated as a broken-DB fault, not papered over — correct given the seed.
- **Validate-without-a-key edge** (`agentmodel.go:54-57` + test): `Validate` only parses (no live call), so it works with an empty key; confirmed by `TestValidate`'s no-key case.
No swallowed errors, no missing defers, no unchecked nils, and the boundary conditions (empty spec, NULL vs absent vs non-bool, version conflict, race during swap, transient DB fault at boot) are each handled with an explicit, documented decision.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
I reviewed the PR through the security lens across all changed files. Here's what I verified:
**Admin enforcement** — `isAdmin` is now checked in both the `requireAdmin` middleware (`internal/api/settings.go:22-31`) and authoritatively in the service seam (`internal/service/instance_settings.go:21-30`). Non-admins receive 403 on both GET and PATCH. Tests confirm this.
**No API key leakage** — `OLLAMA_CLOUD_API_KEY` never enters the database and is never serialized in the API. The settings response only exposes `hasApiKey: boolean` (`internal/api/settings.go:49`). The `effectiveView` struct deliberately omits the actual key.
**Model spec validation** — `agentmodel.Validate` resolves the spec through majordomo.Parse before storage (`internal/service/instance_settings.go:63-66`), so a typo is rejected at save time with 400. The spec is not used in SQL, not executed as shell code, and not rendered unsafely.
**SQL safety** — Migration and store code use parameterized queries throughout. The `instanceSettingsColumns` constant is hardcoded, not user-derived.
**Race safety** — The `agentHolder` uses `atomic.Pointer` for lock-free reads and `sync.Mutex` to serialize rebuilds. The chat handler snapshots the pointer once at entry (`internal/api/agent.go:63`), so a mid-request swap cannot tear.
**Atomicity of version-conflict response** — The version-conflict path returns the current row, but that row has no secret fields and is only reachable by admins (the service gate rejects non-admins before the store layer).
**No SSRF/command injection via model spec** — The spec goes only to `majordomo.Parse` against a registry that explicitly registers only the `ollama.Cloud` provider. There's no path for the spec to influence URLs, commands, or file paths outside majordomo's typed provider model.
Nothing in this PR introduces authz gaps, injection vectors, secret leakage, or unsafe handling of untrusted data.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
Through the Correctness lens, the change is solid. Key observations:
- **Atomic pointer swap is correct**: `agentHolder` serializes rebuilds with `sync.Mutex` while reads (`get()`) stay lock-free via `atomic.Pointer`. The old Runner is replaced; new requests see the new one, and in-flight requests hold their reference until done. No use-after-free or torn-read risk.
- **Precedence layering is correct**: `EffectiveAgent` applies DB overrides (`agent_model` non-empty, `agent_enabled` non-nil) atop env defaults. The env model can never be empty because `config.Load()` falls back to `DefaultAgentModel` when `PANSY_AGENT_MODEL` is unset or blank. The inherit sentinel (`""` / `nil`) correctly falls through.
- **Validation gate is at the right layer**: `UpdateInstanceSettings` validates the model spec via `agentmodel.Validate` before storing, so a bad spec is `ErrInvalidInput` (400) at save time. Empty model skips validation because it means "inherit" — the env value was already checked at boot when the initial Runner built (or failed to).
- **Migration schema is correct**: `agent_enabled INTEGER CHECK (agent_enabled IN (0, 1))` permits `NULL` because SQLite CHECK passes when the expression is not *false* (NULL is allowed). The comment in the PR notes explicitly calls this out, and it matches SQLite semantics.
- **503 contract change is handled end-to-end**: The chat handler nil-checks `agent.get()` and returns 503 `AGENT_DISABLED`. The frontend's `streamChat` maps 503 to a human message, and `useCapabilities` drops `staleTime: Infinity` so the tab state reflects reality. The history routes remain on the `gardens` group (not `agentGroup`), so they stay accessible when the assistant is off, which is the stated design.
- **Context detachment in rebuild mirrors existing pattern**: `context.WithoutCancel` in `updateSettings` before `rebuild` matches the `commitScope`/`RecordAgentExchange` pattern — once the DB write is committed, a client disconnect must not leave the live Runner out of step with stored settings.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`web/src/pages/SettingsPage.tsx:22`** — `fromChoice` relies on a cryptic ternary (`c === 'inherit' ? null : c === 'on'`) where `'off'` silently falls through to `false` because `'off' !== 'on'`. It happens to be correct today, but if `EnabledChoice` ever gains a value the compiler won't catch the missing branch and the function will silently misbehave. Use an explicit `switch` or `if/else` chain so every choice is handled deliberately.
- **`internal/service/instance_settings.go:81`** and `:93` — `EffectiveAgent` is both a struct type and a `*Service` method in the same package. This hurts godoc readability and IDE navigation; searching for the type jumps to the method and vice-versa. Rename the method to something distinct like `ResolveEffectiveAgent` or `CurrentAgentConfig`.
- **`internal/api/settings.go:63`** / **`internal/service/instance_settings.go:94`** — `settingsPayload` triggers a second `GetInstanceSettings` DB read even though its caller (`getSettings`) already holds the row. `updateSettings` compounds this: it calls `EffectiveAgent` in `rebuild` and again in `settingsPayload`, so a single PATCH can hit the same row three or four times. This is a leaky abstraction: `EffectiveAgent` should accept an already-loaded `*domain.InstanceSettings` (or expose a helper that layers settings over env without fetching), so callers that already have the row don't pay for redundant queries.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
I examined the changes through the Performance lens and found nothing material.
- The hot path (`/capabilities`, chat handler) uses atomic pointer loads (`h.agent.get()`) — lock-free and correct. No contention under read load.
- `agentHolder.rebuild` serializes writes with a mutex, but rebuilds only happen on admin settings changes (rare). The lock is not held across any I/O; model resolution is an in-memory parse.
- `EffectiveAgent` does re-read the settings row on every call, which means `GET /settings` and `PATCH /settings` issue an extra identical SELECT. This is real but cold-path (admin-only, low volume) and not material to overall system performance.
- Runner replacement via `atomic.Pointer.Store` is safe; old runners become unreachable and are GC'd when in-flight requests finish. No unbounded growth.
- No missing pagination/limits, N+1 queries in loops, unnecessary allocations in hot paths, or blocking calls on request-serving paths.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- **`internal/service/instance_settings.go:64`** — `agentmodel.Validate` returns a specific error (e.g. empty spec, unknown provider, malformed failover chain), but `UpdateInstanceSettings` swallows it and returns a generic `domain.ErrInvalidInput`. An admin POSTing a bad model gets a 400 with code `INVALID_INPUT` and message `"invalid input"`, with no hint that it's the model spec that failed. The underlying `err` should at least be logged so an operator can diagnose it, or the API should surface it.
*Verified by reading `internal/service/instance_settings.go` and `internal/api/errors.go:54`.*
- **`internal/api/settings_test.go:180-208`** — `TestSettingsSwapUnderRace` spawns 4 reader goroutines that loop until `<-done`, then closes `done` and returns immediately. The test does **not** wait for the goroutines to finish, so they may still be running (and calling `doJSON`, which uses `testing.T`) after the test function exits. This is a real race under `go test -race` that can panic or produce spurious race reports. Add a `sync.WaitGroup` and wait for the workers before returning.
*Verified by reading the test body and the goroutine spawn/exit pattern.*
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- Remove config.AgentConfig.Ready() — dead after the refactor (no non-test
callers), and its doc comment ("route registration and capabilities gate on
this") was now false. EffectiveAgent.Ready() carries the same logic and IS
used, see below.
- agentHolder now uses eff.Ready() and eff.APIKey instead of an inline check and
a redundant apiKey field it held separately. This makes EffectiveAgent.APIKey/
Ready() production-used rather than test-only, drops the duplicated readiness
check, and simplifies newAgentHolder's signature.
- settingsPayload no longer swallows an EffectiveAgent error into a misleading
empty "effective" view (which would read as "nothing configured"). It returns
the error; the handlers surface it as a 500. EffectiveAgent re-reads the row
GetInstanceSettings just returned, so a failure there is a real DB fault.
- Frontend streamChat handles 503 (assistant disabled at runtime) distinctly
from 404 (endpoint absent) — the backend returns 503 AGENT_DISABLED now, which
the old code mislabelled.
- Fixed the api.go comment that still said a disabled request gets a 404 — it's
a 503.
- updateSettings uses the shared parseNullable[bool] instead of a bespoke
parseNullableBool (now removed).
- NewRunner drops its empty-spec guard; agentmodel.Resolve already owns that
check, so one place decides what a valid spec is.
- rebuild-on-resolve-error now documents WHY it keeps the current Runner rather
than tearing down a working assistant on a transient DB blip: the state is
persisted, the next rebuild reconciles, and killing a live assistant on a read
hiccup is worse than a brief stale window.
Not changed: the store's version-conflict fallback returning GetInstanceSettings'
error instead of ErrVersionConflict when that read also fails. That's the exact
pattern every other version-guarded update uses (gardens/objects/plants); making
only this one differ would be the inconsistency. A DB read failing immediately
after the guarded UPDATE on the same local SQLite file is a disk-fault edge case,
and surfacing that error is defensible.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Worked the blocking round — the three multi-model findings and most singles:
Dead AgentConfig.Ready() removed (no non-test callers; its doc was now false).
settingsPayload swallowing EffectiveAgent errors → it now returns the error and the handlers surface a 500, instead of a misleading empty "effective" view that read as "nothing configured".
Frontend still treating disabled-assistant as 404 → streamChat now handles the 503 AGENT_DISABLED distinctly, and the stale api.go "404" comment is fixed.
EffectiveAgent.APIKey/Ready() production-dead → the holder now uses eff.Ready() and eff.APIKey instead of an inline check and a separate apiKey field it duplicated; that also simplified newAgentHolder.
parseNullableBool duplicating the generic → replaced with parseNullable[bool].
Empty-spec guard duplicated in NewRunner/agentmodel.Resolve → dropped from NewRunner; Resolve owns it.
rebuild leaving a stale runner on EffectiveAgent error → kept the behaviour but documented why: the state is already persisted, the next rebuild reconciles, and tearing down a working assistant on a transient local-SQLite read blip is worse than a brief stale window. Open to changing it if you'd rather it fail closed (set nil on resolve error) — that has its own downside (a DB hiccup turns off a working assistant), so I left it fail-safe.
Not changed, with reason: the store's version-conflict fallback returning GetInstanceSettings' error instead of ErrVersionConflict when that read also fails. That's the identical pattern in UpdateGarden/UpdateObject/UpdatePlanting — making only this one differ would be the inconsistency, and it's a disk-fault edge case (a read failing right after the guarded UPDATE on the same local file).
Re-verified: full suite green, settings swap still race-clean under -race, tsc/vitest pass.
@gadfly review
Worked the blocking round — the three multi-model findings and most singles:
- **Dead `AgentConfig.Ready()`** removed (no non-test callers; its doc was now false).
- **`settingsPayload` swallowing `EffectiveAgent` errors** → it now returns the error and the handlers surface a 500, instead of a misleading empty "effective" view that read as "nothing configured".
- **Frontend still treating disabled-assistant as 404** → `streamChat` now handles the 503 `AGENT_DISABLED` distinctly, and the stale api.go "404" comment is fixed.
- **`EffectiveAgent.APIKey`/`Ready()` production-dead** → the holder now uses `eff.Ready()` and `eff.APIKey` instead of an inline check and a separate `apiKey` field it duplicated; that also simplified `newAgentHolder`.
- **`parseNullableBool` duplicating the generic** → replaced with `parseNullable[bool]`.
- **Empty-spec guard duplicated** in `NewRunner`/`agentmodel.Resolve` → dropped from `NewRunner`; Resolve owns it.
- **`rebuild` leaving a stale runner on `EffectiveAgent` error** → kept the behaviour but documented *why*: the state is already persisted, the next rebuild reconciles, and tearing down a working assistant on a transient local-SQLite read blip is worse than a brief stale window. Open to changing it if you'd rather it fail closed (set nil on resolve error) — that has its own downside (a DB hiccup turns off a working assistant), so I left it fail-safe.
**Not changed, with reason:** the store's version-conflict fallback returning `GetInstanceSettings`' error instead of `ErrVersionConflict` when *that* read also fails. That's the identical pattern in `UpdateGarden`/`UpdateObject`/`UpdatePlanting` — making only this one differ would be the inconsistency, and it's a disk-fault edge case (a read failing right after the guarded UPDATE on the same local file).
Re-verified: full suite green, settings swap still race-clean under `-race`, `tsc`/vitest pass.
🪰Gadfly consensus review — 8 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 8 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡EffectiveAgent re-reads instance_settings row that GetInstanceSettings just fetched
maintainability, performance · flagged by 2 models
internal/api/settings.go:63 and internal/api/settings.go:141 — settingsPayload calls h.svc.EffectiveAgent, which always does a fresh GetInstanceSettings DB read. In getSettings, the same row was just fetched by GetInstanceSettings. In updateSettings, the row was just written and returned, then rebuild calls EffectiveAgent (second read), then settingsPayload calls it again (third read). A single PATCH can hit the same row 3 times. Pass the already-loaded row into `Effe…
🪰 Gadfly · advisory
🟡 **EffectiveAgent re-reads instance_settings row that GetInstanceSettings just fetched**
_maintainability, performance · flagged by 2 models_
- `internal/api/settings.go:63` and `internal/api/settings.go:141` — `settingsPayload` calls `h.svc.EffectiveAgent`, which always does a fresh `GetInstanceSettings` DB read. In `getSettings`, the same row was just fetched by `GetInstanceSettings`. In `updateSettings`, the row was just written and returned, then `rebuild` calls `EffectiveAgent` (second read), then `settingsPayload` calls it again (third read). A single PATCH can hit the same row **3 times**. Pass the already-loaded row into `Effe…
<sub>🪰 Gadfly · advisory</sub>
🟡PATCH /settings binds agentModel as a plain string (full-replacement semantics), so a request that omits it resets the stored model to "inherit" — intentional and documented, but inconsistent with the rest of the codebase's absent=unchanged PATCH convention
correctness · flagged by 1 model
internal/api/settings.go:96 (settingsUpdateRequest.AgentModel) — bound as a plain string, not the *string/json.RawMessage "absent vs. explicit" pattern used elsewhere (contrast objectUpdateRequest in internal/api/objects.go:50-65). Since "" is the domain's own sentinel for "inherit from environment" (internal/service/instance_settings.go:60-67, EffectiveAgent at line 103), a PATCH that omits agentModel entirely is indistinguishable from one that explicitly clears it, so `…
🪰 Gadfly · advisory
🟡 **PATCH /settings binds agentModel as a plain string (full-replacement semantics), so a request that omits it resets the stored model to "inherit" — intentional and documented, but inconsistent with the rest of the codebase's absent=unchanged PATCH convention**
_correctness · flagged by 1 model_
- `internal/api/settings.go:96` (`settingsUpdateRequest.AgentModel`) — bound as a plain `string`, not the `*string`/`json.RawMessage` "absent vs. explicit" pattern used elsewhere (contrast `objectUpdateRequest` in `internal/api/objects.go:50-65`). Since `""` is the domain's own sentinel for "inherit from environment" (`internal/service/instance_settings.go:60-67`, `EffectiveAgent` at line 103), a PATCH that omits `agentModel` entirely is indistinguishable from one that explicitly clears it, so `…
<sub>🪰 Gadfly · advisory</sub>
🟠Test goroutines may outlive test function causing race/panic under -race
error-handling · flagged by 1 model
internal/api/settings_test.go:180-208 — TestSettingsSwapUnderRace spawns 4 reader goroutines that loop until <-done, then closes done and returns immediately. The test does not wait for the goroutines to finish, so they may still be running (and calling doJSON, which uses testing.T) after the test function exits. This is a real race under go test -race that can panic or produce spurious race reports. Add a sync.WaitGroup and wait for the workers before returning. *Verif…
🪰 Gadfly · advisory
🟠 **Test goroutines may outlive test function causing race/panic under -race**
_error-handling · flagged by 1 model_
- **`internal/api/settings_test.go:180-208`** — `TestSettingsSwapUnderRace` spawns 4 reader goroutines that loop until `<-done`, then closes `done` and returns immediately. The test does **not** wait for the goroutines to finish, so they may still be running (and calling `doJSON`, which uses `testing.T`) after the test function exits. This is a real race under `go test -race` that can panic or produce spurious race reports. Add a `sync.WaitGroup` and wait for the workers before returning. *Verif…
<sub>🪰 Gadfly · advisory</sub>
🟡requireAdmin re-queries user table when IsAdmin already verified by middleware
performance · flagged by 1 model
internal/service/instance_settings.go:27-36 — requireAdmin queries GetUserByID to check IsAdmin on every settings request, but mustActor(c).IsAdmin was already resolved during session authentication and checked by the requireAdmin() middleware. This adds a DB roundtrip to an already-gated endpoint. The author calls this "redundant by design" (service seam authority), which is fair for correctness, but from a performance lens it's still avoidable overhead on an endpoint that does li…
🪰 Gadfly · advisory
🟡 **requireAdmin re-queries user table when IsAdmin already verified by middleware**
_performance · flagged by 1 model_
- `internal/service/instance_settings.go:27-36` — `requireAdmin` queries `GetUserByID` to check `IsAdmin` on every settings request, but `mustActor(c).IsAdmin` was already resolved during session authentication and checked by the `requireAdmin()` middleware. This adds a DB roundtrip to an already-gated endpoint. The author calls this "redundant by design" (service seam authority), which is fair for correctness, but from a performance lens it's still avoidable overhead on an endpoint that does li…
<sub>🪰 Gadfly · advisory</sub>
🟠Swallowed validation error leaves admin with no diagnostic on bad model spec
error-handling · flagged by 1 model
internal/service/instance_settings.go:64 — agentmodel.Validate returns a specific error (e.g. empty spec, unknown provider, malformed failover chain), but UpdateInstanceSettings swallows it and returns a generic domain.ErrInvalidInput. An admin POSTing a bad model gets a 400 with code INVALID_INPUT and message "invalid input", with no hint that it's the model spec that failed. The underlying err should at least be logged so an operator can diagnose it, or the API should surfa…
🪰 Gadfly · advisory
🟠 **Swallowed validation error leaves admin with no diagnostic on bad model spec**
_error-handling · flagged by 1 model_
- **`internal/service/instance_settings.go:64`** — `agentmodel.Validate` returns a specific error (e.g. empty spec, unknown provider, malformed failover chain), but `UpdateInstanceSettings` swallows it and returns a generic `domain.ErrInvalidInput`. An admin POSTing a bad model gets a 400 with code `INVALID_INPUT` and message `"invalid input"`, with no hint that it's the model spec that failed. The underlying `err` should at least be logged so an operator can diagnose it, or the API should surfa…
<sub>🪰 Gadfly · advisory</sub>
🟠EffectiveAgent type and method share the same name in one package
maintainability · flagged by 1 model
internal/service/instance_settings.go:81 and :93 — EffectiveAgent is both a struct type and a *Service method in the same package. This hurts godoc readability and IDE navigation; searching for the type jumps to the method and vice-versa. Rename the method to something distinct like ResolveEffectiveAgent or CurrentAgentConfig.
🪰 Gadfly · advisory
🟠 **EffectiveAgent type and method share the same name in one package**
_maintainability · flagged by 1 model_
- **`internal/service/instance_settings.go:81`** and `:93` — `EffectiveAgent` is both a struct type and a `*Service` method in the same package. This hurts godoc readability and IDE navigation; searching for the type jumps to the method and vice-versa. Rename the method to something distinct like `ResolveEffectiveAgent` or `CurrentAgentConfig`.
<sub>🪰 Gadfly · advisory</sub>
🟡Stale doc comment references config.AgentConfig.Ready(), which this PR deletes
maintainability · flagged by 2 models
internal/service/instance_settings.go:87 — comment references config.AgentConfig.Ready, which no longer exists (grep confirms zero definitions). The EffectiveAgent.Ready method exists but the comment points at the deleted one.
🪰 Gadfly · advisory
🟡 **Stale doc comment references config.AgentConfig.Ready(), which this PR deletes**
_maintainability · flagged by 2 models_
1. `internal/service/instance_settings.go:87` — comment references `config.AgentConfig.Ready`, which no longer exists (grep confirms zero definitions). The `EffectiveAgent.Ready` method exists but the comment points at the deleted one.
<sub>🪰 Gadfly · advisory</sub>
🟠fromChoice silently falls through 'off' in cryptic ternary
maintainability · flagged by 1 model
web/src/pages/SettingsPage.tsx:22 — fromChoice relies on a cryptic ternary (c === 'inherit' ? null : c === 'on') where 'off' silently falls through to false because 'off' !== 'on'. It happens to be correct today, but if EnabledChoice ever gains a value the compiler won't catch the missing branch and the function will silently misbehave. Use an explicit switch or if/else chain so every choice is handled deliberately.
🪰 Gadfly · advisory
🟠 **fromChoice silently falls through 'off' in cryptic ternary**
_maintainability · flagged by 1 model_
- **`web/src/pages/SettingsPage.tsx:22`** — `fromChoice` relies on a cryptic ternary (`c === 'inherit' ? null : c === 'on'`) where `'off'` silently falls through to `false` because `'off' !== 'on'`. It happens to be correct today, but if `EnabledChoice` ever gains a value the compiler won't catch the missing branch and the function will silently misbehave. Use an explicit `switch` or `if/else` chain so every choice is handled deliberately.
<sub>🪰 Gadfly · advisory</sub>
steve
merged commit a13acedd90 into main2026-07-22 02:53:55 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #79. Part of #86.
Moves the agent model out of env-only config into an admin-editable Settings section, and enforces
is_admin— which has sat in the schema since migration 0001, plumbed all the way to the client, and checked nowhere.The hard part wasn't the form
Agent config used to resolve into an opaque
llm.Modelat boot, and the chat routes were registered only when a key was present. So a settings toggle couldn't turn the assistant on — the route wouldn't exist. Three changes fix that:agentHolderholds it behind anatomic.Pointer; the chat routes are now registered unconditionally and nil-checkagent.get(). A settings change swaps the Runner (on / off / different model) with no restart./capabilitiesreads the pointer, so it reports what's live, not what booted.useCapabilitiesdropsstaleTime: Infinity(the assumption this PR falsifies) and the save invalidates it.is_adminis now enforced viarequireAdmin— authoritative in the service seam, plus a cheap middleware early-403.Secrets stay in the environment
OLLAMA_CLOUD_API_KEYis never stored in the DB — a secret ininstance_settingswould land in every backup and in the undo history's blast radius. An admin changes which model runs, not whose account pays. The Settings API reports whether a key is present, never its value.Storage & resolution
instance_settings, a single-row (CHECK id = 1) table — pansy's first instance-level state (everything else hangs off a garden/object).agent_model(''= inherit env),agent_enabled(NULL= inherit env, so "admin hasn't touched this" is distinct from "admin turned it off"), version-guarded.PANSY_AGENT_MODELkeeps working.internal/agentmodelis a new leaf package: the one place that knows how to turn a spec into a model. Bothagent(to run) andservice(to validate before storing) import it — it can't live inagent, which importsservice. The PATCH validates the spec viaParse, so a typo is a 400 at save time, not a broken assistant next turn.Contract change worth calling out
Chat routes now always exist, so "assistant off" is a runtime 503 +
capabilities:false, not a missing route (404).TestAgentDisabledWithoutAKey(renamed from…RoutesAbsent…) asserts the new shape; the frontend keys the chat tab off capabilities, so a user never reaches the 503. History routes stay ungated — they're stored data behind the ordinary garden-role check, not the model.Frontend
/settingsroute with an admin guard, a Settings page (model field with an "inheriting X from the environment" hint, tri-state enabled select, and a live status dot that surfaces "configured but not running"), and a nav link shown only to admins. The admin routing guard is convenience — the server'srequireAdminis the boundary.Verified live against the built binary
The setting persisted across a process restart (version was already 1 from the seeded row on the second boot). The pointer swap is race-clean under
go test -race(TestSettingsSwapUnderRacehammerscapabilities— the same atomic load the chat handler does — while a writer flips the assistant; it deliberately avoidschatso no real model call is made on the fake key).Tests
EffectiveAgentlayering (model override, enabled inherit vs explicit), bad-model rejection leaves the row untouched.agentEnabled→ 400, version conflict carries the current row, and the race test.Validate/Resolve, including a failover chain and no-key validation.Notes for review
agentModel/agentEnableduse the "inherit" sentinels (''/null) rather than a separate "inherit" boolean.agent_enabledis genuinely nullable in SQLite; theCHECK (… IN (0,1))still permits NULL (SQL CHECK passes unless it evaluates to false).Docs updated in the same commit: README (precedence table + key-stays-in-env), DESIGN (decision entry + routes), CLAUDE (don't re-add conditional route registration; the key never enters the DB; the
agentmodelcycle reason).GOWORK=off go test ./...green;gofmt -l internal/clean;tsc --noEmit, 87 vitest, andnpm run buildall pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Moves the agent model out of env-only config into an admin-editable Settings section, and enforces is_admin for the first time — it has been in the schema since migration 0001, plumbed to the client, and checked nowhere. Backend: - Migration 0010: instance_settings, a single-row (CHECK id=1) table — pansy's first instance-level state. Holds agent_model ('' = inherit env) and agent_enabled (NULL = inherit env), version-guarded like every mutable row. SECRETS STAY IN ENV: OLLAMA_CLOUD_API_KEY is never stored here. - requireAdmin at the service seam (authoritative) plus a cheap middleware early-403. Non-admin gets 403, not 404 — settings existence isn't masked. - EffectiveAgent resolves DB-over-env (model, enabled); key always from env. - The live Runner is hot-swapped, not built once. agentHolder holds it behind an atomic.Pointer; the chat routes are now registered UNCONDITIONALLY and nil-check agent.get(), so a settings change turns the assistant on/off/onto a new model with no restart and no race against in-flight readers. /capabilities reads the pointer, so it reports what's live, not what booted. - internal/agentmodel is a new leaf package holding the one place that knows how to turn a spec into a model. Both agent (to run) and service (to validate a spec before storing it) import it; it can't live in agent, which imports service. Settings PATCH validates the spec via Parse, so a typo is a 400 now rather than a broken assistant on the next turn. Frontend: - /settings route (admin guard), a Settings page (model field, tri-state enabled, live status), nav link shown only to admins. - useCapabilities drops staleTime:Infinity — the assistant can now change under a running page — and the settings save invalidates it. Contract change: chat routes always exist, so "assistant off" is a runtime 503 + capabilities:false, not a missing route. Updated the test that asserted the old shape. Verified live against the built binary: disable flips capabilities to false and logs it; re-enable with a new model swaps it back; a bad spec is rejected 400; the setting persists across a restart. Swap is race-clean under `go test -race`. Docs: README (precedence + key-stays-in-env), DESIGN (decision + routes), CLAUDE (don't re-add conditional route registration; key never in the DB). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ@gadfly review
(First review — the original one died on the
docker.gitea.comCI outage, now resolved. This is the admin Settings + runtime model-swap PR; see the description for the notes-for-review, especially the double admin check and the atomic-pointer swap.)🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 02:28:38Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 10 findings (2 with multi-model agreement)
internal/api/settings.go:63internal/service/instance_settings.go:878 single-model findings (lower confidence)
internal/api/settings_test.go:180internal/service/instance_settings.go:64internal/service/instance_settings.go:81web/src/pages/SettingsPage.tsx:22internal/api/settings.go:96internal/config/config.go:75internal/service/instance_settings.go:27internal/api/api.go:59Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Frontend only consumes the boolean, never handles a raw key. That confirms the secret-handling design holds end-to-end.
Review
VERDICT: No material issues found
I checked this PR specifically for authn/authz gaps, injection, secret leakage, and insecure defaults around the new admin-gated Settings feature:
requireAdmin(internal/api/settings.go:24) is a cheap early-403 reading the already-resolved actor; the authoritative check isService.requireAdmin(internal/service/instance_settings.go:15), which re-readsIsAdminfrom the DB per call rather than trusting a cached claim. VerifiedResolveSession(internal/service/auth.go:220) re-fetches the session/user from the store on every request — no stale-admin-in-token risk.is_admin; none exists.settingsUpdateRequest(internal/api/settings.go) only exposesagentModel/agentEnabled/version— no mass-assignment surface.OLLAMA_CLOUD_API_KEYnever enters the DB (migration 0010 has no key column) and the API response (effectiveView.HasApiKey) only ever serializes a boolean. Confirmed the frontend (web/src/pages/SettingsPage.tsx) only consumes that boolean and never touches a raw key./settingssits underv1, which hascsrfGuard()applied globally (internal/api/api.go:57), so the new PATCH route is covered like every other mutating route.writeServiceError'sErrInvalidInputbranch (internal/api/errors.go:54) returns a generic "invalid input" 400 — a rejected model spec doesn't echo the underlying parse error or any registry detail back to the client.instance_settingsstore code — no injection surface.agentmodel.Resolve→majordomo.Parse). Couldn't verify majordomo'sParse/ollama.Cloudinternals (module cache is outside the sandboxed directory), so I'm not reporting this — flagging it here only as unverified, not as a finding.Outside my lens: none worth flagging — the concurrency/rebuild-lock design and doc/test thoroughness are for other reviewers.
🎯 Correctness — Minor issues
Confirmed. The
settingsUpdateRequest.AgentModelfield is indeed bound as a plainstring(settings.go:96, not lines 39-45 as the draft cited — that range is actuallysettingsResponse/effectiveView), andinternal/service/instance_settings_test.go:70-71shows the test suite must re-passAgentModelon every update specifically to avoid resetting it, which corroborates the behavior. However, the draft's suggested fix is inaccurate: this is already documented prominently, in two doc comments —internal/service/instance_settings.go:40-42("InstanceSettingsPatchis a full replacement of the editable fields... an empty AgentModel means fall back to env") andinternal/api/settings.go:92-94("agentModel""means inherit the env var") — at the same level of prominence as theobjectUpdateRequestcomment the draft cites for contrast. So this is a real, confirmed deviation from the rest of the codebase's convention, but a deliberate and clearly-documented one, not an oversight.Review — 🎯 Correctness lens
VERDICT: Minor issues
internal/api/settings.go:96(settingsUpdateRequest.AgentModel) — bound as a plainstring, not the*string/json.RawMessage"absent vs. explicit" pattern used elsewhere (contrastobjectUpdateRequestininternal/api/objects.go:50-65). Since""is the domain's own sentinel for "inherit from environment" (internal/service/instance_settings.go:60-67,EffectiveAgentat line 103), a PATCH that omitsagentModelentirely is indistinguishable from one that explicitly clears it, so{"agentEnabled": false, "version": N}alone silently resets the stored model back to "inherit env." This is confirmed byinternal/service/instance_settings_test.go:70-71, where the test must re-supplyAgentModelon every subsequent update specifically to avoid this reset.internal/service/instance_settings.go:40-42) and "agentModel""means inherit the env var" (internal/api/settings.go:92-94) — at the same prominence as the "absent = unchanged" doc comment onobjectUpdateRequestthat provides the contrast. It is also currently unreachable in the shipped product:web/src/pages/SettingsPage.tsx:61-64is a single form that always submitsagentModelandagentEnabledtogether.agentEnabledalone.🧹 Code cleanliness & maintainability — Minor issues
Both findings confirmed against the actual code:
config.AgentConfighas noReadymethod anywhere in the tree (grep confirms), yetinternal/service/instance_settings.go:87's comment still says "Ready mirrors config.AgentConfig.Ready".internal/api/api.go:62registers/capabilitiesat line 62, whileh.agentisn't assigned until line 141 — after, not before. The comment's claim is backwards, and it's superseded by the accurate doc oncapabilitiesat lines 194-200.Review
VERDICT: Minor issues
internal/service/instance_settings.go:87—EffectiveAgent.Ready()'s doc comment says "Ready mirrors config.AgentConfig.Ready", but this same PR deletesconfig.AgentConfig.Ready(confirmed via grep — no remaining references anywhere in the tree, includinginternal/config/config.go). The comment now points at dead code that no longer exists, which CLAUDE.md calls out explicitly ("Stale comments are worse than none — the next reader believes them"). Fix: drop the "mirrors config.AgentConfig.Ready" clause or just describe the check plainly (enabled, with a key and a model), which the second half of the sentence already does.internal/api/api.go:59-61— the comment abovev1.GET("/capabilities", h.capabilities)still reads "Registered after the agent below, because it reports whether the runner actually built." This was true of the pre-#79 shape, but post-refactorcapabilitiesreadsh.agent.get()off an atomic pointer at request time — registration order is irrelevant now, and the route is in fact registered beforeh.agent = newAgentHolder(...)at line 141, contradicting the comment's own claim. The accurate rationale is already given in thecapabilitiesfunc doc a few lines down (194-200), so this stale comment is both wrong and redundant. Suggest deleting it or replacing with a one-liner noting registration order no longer matters here.Everything else touched by this PR (
agentHolder,settings.go,instance_settings.goin service/store,agentmodelpackage, migration, frontendSettingsPage.tsx/settings.ts) is well-factored: single-responsibility functions, no copy-pasted logic, naming consistent with the rest of the codebase, and the "why" comments earn their keep. No structural or duplication concerns beyond the two stale comments above.⚡ Performance — No material issues found
No polling loop for settings — it fetches once via React Query defaults. Nothing performance-relevant there. This confirms the read path (chat, capabilities) stays cheap (single atomic load), and the settings admin path, while doing a couple of redundant
EffectiveAgentDB reads per request, operates on a single-row table with no scan cost and is admin-only/low-frequency — not a justifiable regression.VERDICT: No material issues found
Reviewed through the performance lens:
agentChat(internal/api/agent.go:61) now does oneatomic.Pointerload (h.agent.get()) instead of a plain field read — same O(1) cost, and it's loaded once up front rather than re-read, avoiding any redundant load.capabilities(internal/api/api.go) is likewise a single atomic load. VerifiedagentHolder.get()(internal/api/agent_holder.go:44) is lock-free.agentHolder.rebuild(internal/api/agent_holder.go:52) takes a mutex and does a DB read (EffectiveAgent) plus builds a fresh majordomo registry — but this only runs at boot and on an admin settings PATCH, never per-request. No hot-loop concern.EffectiveAgentDB reads on settings PATCH:updateSettings(internal/api/settings.go) callsh.agent.rebuild(which internally callsEffectiveAgent, oneSELECToninstance_settings) and thensettingsPayload(which callsEffectiveAgentagain) — three DB round trips total for one PATCH (UPDATE...RETURNING, then two single-rowSELECTs). This is technically avoidable, but it's an admin-only, low-frequency endpoint against a single-row table with no scan cost, so the impact isn't justifiable as a real regression.useCapabilities(web/src/lib/agent.ts:30) dropsstaleTime: InfinityforstaleTime: 60_000— a bounded, sane refetch interval, not aggressive polling.useSettingshas norefetchInterval. No new client-side hot-loop.Nothing here rises to a genuine efficiency regression under this lens.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed this PR specifically for ignored/swallowed errors, missing cleanup, panics on bad input, and unhandled edge cases (nil, empty, zero/negative, off-by-one), verifying against the checked-out code rather than the diff alone.
What I checked and found sound:
internal/api/agent_holder.go—rebuild()correctly serializes concurrent settings saves viarebuildMuwhile keepingget()lock-free. AnEffectiveAgentread failure is logged and the old Runner is deliberately kept (not torn down on a transient blip) — this tradeoff is explicitly reasoned about in the comments, not an oversight. A resolvable-config-but-unresolvable-model correctly swaps tonilrather than leaving a stale Runner. VerifiednewAgentHoldernever leaves the pointer in a panicking state on boot failure.internal/api/agent.go—runner := h.agent.get()is loaded once and reused for both the nil-check and theRuncall, avoiding a TOCTOU gap where a concurrent settings swap could nil out the runner mid-request. VerifiedgetAgentHistory/deleteAgentHistorydon't touchh.agentat all, so they can't nil-panic when the assistant is off.internal/store/instance_settings.go—scanInstanceSettingscorrectly handles the nullableagent_enabledviasql.NullInt64;UpdateInstanceSettingscorrectly passes untypednilto bind SQL NULL (restoring "inherit" from an explicit override), and the version-mismatch path re-fetches and returns the current row withErrVersionConflict, consistent with every other mutable resource in the codebase.GetInstanceSettings'ssql.ErrNoRowscase is correctly distinguished as "migration not applied" rather than papered over.internal/service/instance_settings.go—UpdateInstanceSettingsvalidates a non-empty model before writing (verifiedTestUpdateInstanceSettingsRejectsBadModelconfirms the row is untouched on rejection);requireAdminpropagates aGetUserByIDerror unchanged rather than swallowing it into a wrong error class.internal/agentmodel/agentmodel.go—Resolve/Validatecorrectly reject empty/whitespace-only specs viaTrimSpace, tested for both the key-present and key-absent cases.internal/api/settings.go—parseNullable[bool]correctly treats absent vs. explicitnullvs.true/false; a non-bool value ("yes") is verified (test + code read) to 400 rather than silently coercing.Version int64withbinding:"required"mirrors the existing pattern injournal.go/seed_lots.go, and the migration seedsversion = 1, so the zero-value/requiredgotcha doesn't bite here.settings.ts,SettingsPage.tsx) — mutation error path distinguishes a 409 (rebases the form to the server's current row and warns the user rather than silently discarding their edit) from other errors; loading/error/pending states are all handled before rendering the form.I did not find any unhandled nil/empty/boundary case, swallowed error, or missing cleanup that isn't already covered by the extensive test suite (version conflicts, bad-model rejection, disabled-key 503, admin-gate 403, and the race test).
glm-5.2:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 4/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2☁️ skipped (backed off until 02:18:17.784)
🎯 Correctness — No material issues found
VERDICT: No material issues found
I verified the correctness-critical paths of this change by reading the actual code, not just the diff:
internal/service/instance_settings.go):EffectiveAgentseeds froms.cfg.Agent.Model(env-resolved, with theDefaultAgentModelfallback already applied byconfig.Load) ands.cfg.Agent.Enabled, then overridesModelonly whenst.AgentModel != ""andEnabledonly whenst.AgentEnabled != nil. The "inherit" sentinels (''/nil) correctly fall through. Re-derivedReady() = Enabled && APIKey != "" && Model != ""against the seeded row (agent_model='',agent_enabled=NULL) — yields the env/default values, so an untouched instance behaves as before. ✓0010_instance_settings.sql):agent_enabled INTEGER CHECK (agent_enabled IN (0,1))permits NULL (a CHECK only fails on false, andNULL IN (0,1)is NULL, not false). Seed inserts(1, '', NULL). Store scans viasql.NullInt64and mapsenabled.Valid→*bool, else leavesnil. Correct tri-state. ✓internal/store/instance_settings.go):UPDATE ... WHERE id = 1 AND version = ? RETURNING ...;sql.ErrNoRows→ fetch current row +ErrVersionConflict; success bumpsversion+1. Seeded version=1, so the first PATCH with version=1 matches. The 409 path carries the current row (writeVersionConflict). ✓internal/api/agent.go):agentChatloadsrunner := h.agent.get()once and nil-checks before binding the request, then uses that localrunner.Run— a mid-request swap cannot flip the pointer between guard and use.rebuildserializes writers viarebuildMuwhile reads stay lock-free on the atomic. ✓getAgentHistory/deleteAgentHistoryuseh.svc, neverh.agent, so they correctly return 200/204 regardless of whether a Runner exists (verified atagent.go:193-217). ✓internal/service/instance_settings.go):agentmodel.Validate(s.cfg.Agent.OllamaCloudAPIKey, model)runs on non-empty specs before the UPDATE;ValidatecallsResolve, which only parses (no live call), so it works even with an empty env key. A typo →ErrInvalidInput→ 400, row untouched. ✓agentModelbinding: a plainstringfield means a JSONnullunmarshals to""(the inherit sentinel), consistent with an explicit empty string.strings.TrimSpacein the service also collapses whitespace-only to"". ✓agentEnabledthree-way decode:parseNullable[bool]returns(nil,false)for absent and(nil,true)for explicit null — both yieldenabled=nil(inherit). Verified aterrors.go:79. ✓No semantic, formula, threshold, or boundary-value defect surfaced under the correctness lens. (One doc-only imprecision — the
domain.InstanceSettingscomment says AgentModel"""falls back to PANSY_AGENT_MODEL", when it actually falls back tocfg.Agent.Model, which is env-or-DefaultAgentModel. This is a comment inaccuracy, not a behavior bug, so I am not reporting it as a finding.)🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2☁️ skipped (backed off until 02:18:17.784)
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2☁️ skipped (backed off until 02:18:17.784)
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests
kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues · ⚠️ 4/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 2: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
⚡ Performance — Minor issues
Minor issues
No material issues found in hot paths — the atomic pointer swap is clean, capabilities reads are lock-free, and the admin endpoints are low-traffic. Two layering inefficiencies in the settings request paths are worth fixing:
internal/api/settings.go:63andinternal/api/settings.go:141—settingsPayloadcallsh.svc.EffectiveAgent, which always does a freshGetInstanceSettingsDB read. IngetSettings, the same row was just fetched byGetInstanceSettings. InupdateSettings, the row was just written and returned, thenrebuildcallsEffectiveAgent(second read), thensettingsPayloadcalls it again (third read). A single PATCH can hit the same row 3 times. Pass the already-loaded row intoEffectiveAgent(or add anEffectiveAgentFromvariant) to cut this to one read.Verified by reading
internal/service/instance_settings.go:93-110and tracing the call chain insettings.goandagent_holder.go:60.internal/service/instance_settings.go:27-36—requireAdminqueriesGetUserByIDto checkIsAdminon every settings request, butmustActor(c).IsAdminwas already resolved during session authentication and checked by therequireAdmin()middleware. This adds a DB roundtrip to an already-gated endpoint. The author calls this "redundant by design" (service seam authority), which is fair for correctness, but from a performance lens it's still avoidable overhead on an endpoint that does little else. Consider passing the resolved user (or a small admin flag) into the service call, or accepting the middleware as sufficient and skipping the service-layer re-query for this read-only gate.Verified by reading
internal/api/auth.go:109(session resolution loads the full user),internal/api/settings.go:28-36(middleware checksIsAdmin), andinternal/service/instance_settings.go:27-36(re-queries the DB).🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 1: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
The security model looks solid: requireAuth resolves the user (and the middleware + service both re-check IsAdmin), the key never enters the DB, and the settings row is admin-gated. Let me check a few more things — particularly how
effective.modelis exposed and whether model spec is a path-traversal or injection risk throughParse.I've reviewed the change through the security lens, verifying the auth chain, secret handling, SQL parameterization, and input validation against the actual checked-out code.
VERDICT: No material issues found
Security-relevant verification I did:
internal/api/settings.go:22requireAdminmiddleware readsmustActor(c).IsAdminfrom the already-resolved session user (requireAuthatauth.go:109callssvc.ResolveSessionand stashes the user). Layered withinternal/service/instance_settings.go:21requireAdmin, which re-fetches the user from the DB — so a demotion between middleware and service is caught. Redundant by design, as the PR notes.internal/api/settings.go:44-54effectiveViewexposes onlyhasApiKey: bool, never the key. TheEffectiveAgent.APIKeyfield (instance_settings.go:84) is used solely to build the Runner; no JSON tag serializes it. Confirmed the key never entersinstance_settings(migration 0010 has no key column).internal/store/instance_settings.go:62-68UPDATE uses?placeholders foragent_model,agent_enabled,version; no string interpolation of user input into SQL. Same for the SELECT.internal/service/instance_settings.go:60-67agentModelis trimmed and validated viaagentmodel.Validate→Resolve→majordomo.Parsebefore storage; unresolvable specs returnErrInvalidInput(400).agentEnabledis decoded withparseNullable[bool](errors.go:79) which rejects non-bool JSON.versionisbinding:"required".EffectiveAgent(the unauthenticated code path used byagentHolder.rebuild) only reads the settings row and env — it does not expose anything to a caller, andrebuildlogs the model but never the key (agent_holder.go:78logseff.Model, noteff.APIKey).No security findings to report.
🎯 Correctness — No material issues found
I've reviewed the key parts. Now let me check the migration of
cfg.Agent.Enableddefaulting - there's a subtle issue worth checking. TheEffectiveAgentlayersEnabled: s.cfg.Agent.Enabledwhere the env default is "on when a key is present". Let me verify the config-levelEnabledsemantics are actually correct.Now let me check one specific potential correctness issue: what happens at boot if
PANSY_AGENT_ENABLED=falsebutOLLAMA_CLOUD_API_KEYset? The env default forcfg.Agent.Enabledresolves tofalse. Then a settings change that turns on the agent... let me check the validate flow. Validate uses the env key regardless. That's fine.Let me also check one subtle issue: the
Enabledenv default of "on when key present". When admin explicitly setsagentEnabled=null(inherit),EffectiveAgent.Enabled = s.cfg.Agent.Enabled. That'struewhen a key is present (correct inherit semantics). Good.Now check the test settings response
agentModelround-trip for an issue with theagentEnabledfield marshaling. Thedomain.InstanceSettings.AgentEnabled *boolwithjson:"agentEnabled"— when nil, marshals tonull. The store scan returnsnilif invalid. Good.Let me verify one more thing — the
updateSettingsrace test hammering capabilities while writer does PATCH. There's a potential issue: wheni%2==0flips enabled=true but agentModel="" inherit. WithagentCfg()env model isollama-cloud/glm-5.2:cloud, so resolving works. Good.Let me verify the rebuild ordering with respect to the response. The handler calls
h.agent.rebuild(...)thensettingsPayload. But the rebuild is serialized via mutex;settingsPayloadreadsh.agent.get()which is atomic. Could a second concurrent PATCH interleave between rebuild and payload read? It would just reflect whichever swap happened. Not a correctness bug, just a stale-view possibility. Acceptable.Now let me check one final potential issue - the chat route reads
runner := h.agent.get()early, but the history fetch happens after. If the runner is swapped to nil after the get but before Run, we use a stale (now-disabled) runner. That's a benign TOCTOU since the runner is still valid. Fine.Let me verify the more subtle issue:
updateSettingscallsh.svc.UpdateInstanceSettingswhich doesrequireAdminagain, thenh.agent.rebuild— but the rebuild callsh.svc.EffectiveAgentwhich doesn't check admin. That's fine by design (it's internal).Now check
Validatesemantics. The service validates the model with the env key. If the key is missing but admin sets a model spec,Validate("", "ollama-cloud/x")— does Parse work with empty token? The test assertsValidate("", "ollama-cloud/glm-5.2:cloud")succeeds. So yes.Let me check one thing - the store
UpdateInstanceSettingsreturning(nil, gerr)when the current row read fails after a version conflict. The handler then doeswriteVersionConflict(c, st)with nilst. Let me checkwriteVersionConflict:Looking at errors.go:66 -
c.JSON(http.StatusConflict, gin.H{"error": ..., "current": current})withcurrent=nil→ marshals to"current":null. Not a crash, just a degraded 409. Edge case but fine.Now let me look for the real correctness issue: when
UpdateInstanceSettingssucceeds, the returnedstis the updated row, which is what gets sent in the response. But therebuildhappens BEFOREsettingsPayload. The settingsPayload callsEffectiveAgentwhich re-reads the row. Good, consistent.Let me check whether there's a subtle ordering issue with
rebuildhappening inside the mutex BEFORE the response is built. If rebuild fails (e.g. NewRunner error), the response still says success withAgentLive: false. That's correct.Let me now check the critical edge case:
agentHolder.rebuildwheneff.Ready()is false usesh.ptr.Swap(nil). TheSwapreturns old value. If old was nil and new is nil, no log. Good. But it usesStore(nil)in the NewRunner error path instead of Swap, which doesn't log "turned off" in that case but does log the error. Fine.Now let me check one potentially real issue: the
agentModelPATCH validation. When admin sendsagentEnabled: falseANDagentModel: "nonesuch/model"— the service validates the model first and returnsErrInvalidInputWITHOUT touching the row. But the testTestSettingsRejectsBadModelsends them separately. What if both are sent together? Validate runs first, returns early, no update. Correct.Let me check the frontend side briefly to ensure capability polling is consistent.
Now let me verify the potential correctness issue I want to confirm: when
Enabledenv default istrue(key present) and admin has not touched settings,EffectiveAgent.Enabled = s.cfg.Agent.Enabled = true. Good. When env saysPANSY_AGENT_ENABLED=falseexplicitly,s.cfg.Agent.Enabled = false, and admin withagentEnabled=nullinherits false. Correct.Let me check one more subtle thing — the rebuild on the very first ever
newAgentHoldercall at boot. It usescontext.Background(). If the DB hasn't been migrated yet... no, migrations run beforeNew. Fine.Let me check one final concern:
UpdateInstanceSettingsreturns the updated row butagentmodel.Validateswallows the actual error and returnsErrInvalidInput. The test confirms this. But could a valid spec be rejected because the env key is invalid? Validate usess.cfg.Agent.OllamaCloudAPIKey— even an empty key works (test confirms). So a non-empty but invalid key still allows Parse to succeed (Parse doesn't call). Good.I've verified enough. Let me check one last potential issue: the
agentModelfield trimming. Service doesmodel := strings.TrimSpace(patch.AgentModel)and stores the trimmed version. So if admin sends" ollama-cloud/x ", it's stored trimmed. Good. ButEffectiveAgentreturns the stored (trimmed) value, andagentmodel.Resolvere-trims. Fine.One subtle thing: the test
TestSettingsInheritFromEnvcheckseff["hasApiKey"] != true. WithagentCfg()key is "test-key". Good.Let me verify there isn't an issue with the
agentEnabledJSON marshaling when it's an explicitfalse. The store scans to*boolpointing to false. The domain struct marshals*bool→false. Good. When nil →null. Good.I'm satisfied. Let me also quickly check whether
recordis called for instance settings updates — CLAUDE.md says "every service mutation lands in history".Confirmed:
UpdateInstanceSettingsdoes NOT callrecord. CLAUDE.md: "Every service mutation lands in history (#48). If you add one, record it."This is an instance-level setting, not a garden-scoped change. Let me verify whether the change-set system supports instance-level history. Looking at
WriteChangeSet:The change-set system is garden-scoped (
GardenIDis required). Instance settings are not garden-scoped, so therecordinvariant doesn't naturally apply. The PR description even notes "secrets never move here — so it doesn't land in ... the undo history's blast radius" — suggesting instance settings are intentionally outside the history system. So not callingrecordis by design here, not a bug. Not a finding.Let me do one final check on the
boolToIntusage to make sure the storage round-trips correctly.boolToInt(false) = 0,boolToInt(true) = 1. The scan doesenabled.Int64 != 0→ false for 0, true for 1. Correct round-trip.I've thoroughly verified the change. My lens (correctness) finds no material issues. The semantics of the inherit sentinels, the version-guarded update, the atomic pointer swap, the admin gate, and the precedence layering are all correct. The one TOCTOU window (runner swap between get and Run) is benign and documented. The 409-with-nil-current edge case is a minor degradation, not a correctness bug for the happy path.
Verdict: No material issues found
Verified clean (correctness lens):
internal/service/instance_settings.go:93-110EffectiveAgentlayering: re-derived precedence (Settings → env → default) fromcfg.Agentdefaults (Enabled: agentKey != "",Model: DefaultAgentModel). Inherit sentinels (""/nil) fall through to env correctly; explicit overrides replace. Round-trips with the test matrix.internal/store/instance_settings.go:56-80UpdateInstanceSettings: version guardWHERE id = 1 AND version = ?+version = version + 1; onsql.ErrNoRowsreturns(current, ErrVersionConflict).boolToIntround-trips*bool↔ INTEGER 0/1;NULL(nil) preserved throughvar enabled anydefaulting to nil. Re-derived from the SQL and the migration'sCHECK (agent_enabled IN (0,1))permitting NULL — correct.internal/api/agent_holder.go:56-89rebuild: serializes viarebuildMu;Ready()false →Swap(nil); resolution failure →Store(nil)(intentionally tears down stale runner); success →Store(runner). Atomic semantics match the race test. The TOCTOU window inagentChat(runner loaded, thenRuncalled later) is benign — a swapped-out runner remains a valid handle; documented in code.internal/api/settings.go:101-143updateSettings: validatesagentEnabledviaparseNullable[bool](absent/null → nil = inherit; true/false → override), validates model spec via service before persisting, rebuilds the live runner withcontext.WithoutCancel(matches the documented detached-write pattern), then returns the fresh row. 409 path passes the returnedsttowriteVersionConflict; the only degradation (nilcurrentif the conflict-row read itself fails) is an edge case instore.UpdateInstanceSettings, not a happy-path bug.internal/agentmodel/agentmodel.goResolve/Validate: empty/blank spec rejected beforeParse; Parse gets the spec verbatim.Validatewith empty key succeeds (Parse doesn't call upstream) — confirmed byagentmodel_test.go. Service validates with the env key, which is correct since the key is never stored.internal/api/agent.go:63-67503 contract: route always registered, nil-check on the loaded runner, runner cached in a local so a mid-request swap can't flip the guard/Run pair. Matches the test'sTestAgentDisabledWithoutAKey.CHECK id = 1+ seed row means reads never branch;agent_enablednullable withCHECK (… IN (0,1))permits NULL (SQL CHECK passes unless false) — re-derived from SQLite semantics, correct.Investigated and dismissed:
UpdateInstanceSettingsnot callingrecord— by design: the change-set system is garden-scoped (ChangeSet.GardenIDrequired), and the PR explicitly excludes instance settings from the undo history's blast radius. Not a violation of the #48 invariant, which targets garden mutations.writeVersionConflict(c, st)with a possibly-nilst(ifstore.GetInstanceSettingsfails after the conflict) — degrades to"current": null, not a crash; pathological DB-fault path, not a happy-path correctness bug.🧹 Code cleanliness & maintainability — Minor issues
I'll verify the two draft findings against the actual code.
Both findings confirmed.
internal/service/instance_settings.go:87— comment referencesconfig.AgentConfig.Ready, which no longer exists (grep confirms zero definitions). TheEffectiveAgent.Readymethod exists but the comment points at the deleted one.internal/config/config.go:75— theEnabledfield doc says "the same shape as OIDC 404ing when unconfigured," but this PR's contract change is that agent routes are now always registered (503 when off). Let me verify the api.go registration change to confirm.Both findings are confirmed. The agent chat route is always registered (api.go:147) and returns 503/AGENT_DISABLED when off (agent.go:65), confirming the OIDC-404ing analogy at config.go:75 is now stale.
VERDICT: Minor issues
internal/service/instance_settings.go:87— The doc comment// Ready mirrors config.AgentConfig.Ready: enabled, with a key and a model.referencesconfig.AgentConfig.Ready(), a method this PR deletes. Grep across the repo confirms zero remaining definitions ofAgentConfig.Ready. TheEffectiveAgent.Readymethod still exists but the comment points at a now-removed symbol — exactly the "stale comments are worse than none" case CLAUDE.md flags. Fix: reword to describeEffectiveAgent.Readyon its own terms (e.g. "Ready reports whether the assistant can be offered: enabled, with a key and a model.") rather than mirroring the removed method.internal/config/config.go:75— TheEnabledfield doc ends with "the same shape as OIDC 404ing when unconfigured." This PR's central contract change is that the agent routes are now registered unconditionally (off = 503AGENT_DISABLEDon a live route, confirmed atinternal/api/agent.go:65andinternal/api/api.go:147, not a missing-route 404), so the OIDC-404ing analogy is now wrong for the agent path. The PR edits this file (it deletedReady()immediately below at what is now theOIDCConfig.Enabledboundary), so the stale comment is in scope. Fix: drop the comparison or reword to reflect the new 503 shape (e.g. "...starts cleanly with the assistant off and reported as unavailable via/capabilities").Both are comment-only fixes; no logic is affected.
⚡ Performance — No material issues found
The performance-critical paths (chat, capabilities polled by the editor) are unchanged in cost or improved (atomic load, no DB). Let me verify the redundant DB reads in the settings handlers.
I've verified the hot paths (chat, capabilities) and the new settings paths against the checked-out code. Here's my assessment through the performance lens.
VERDICT: No material issues found
Through the performance lens:
agentChat(internal/api/agent.go:63) reads the runner viah.agent.get()— a singleatomic.Pointer.Load, cheaper than the old struct-field read and race-clean.capabilities(internal/api/api.go:202) is the same atomic load. Neither adds a DB query or allocation per request. The per-turn chat work (runner.Run) is untouched.rebuilddoes not contend with reads.rebuildMuserializes only rebuild calls (rare: settings saves);get()is lock-free on the atomic pointer (internal/api/agent_holder.go:45,57). A settings save cannot block chat or capabilities reads.getSettingsreads the row viaGetInstanceSettingsthen re-reads it viaEffectiveAgentinsettingsPayload(internal/api/settings.go:79,63);updateSettingsreads it a third time viarebuild→EffectiveAgent(internal/api/settings.go:135+internal/api/agent_holder.go:60). All three hit the same single-rowSELECTbyid = 1. This is wasteful but only an admin saving settings triggers it — not a hot path, and the query is point-looked by PK. Not material.agentmodel.Resolveallocates a freshmajordomo.Registryper call (internal/agentmodel/agentmodel.go:28-31), invoked on boot and on each settings save (once viaValidate, once viarebuild→NewRunner). Again, cold paths only; the chat hot path reuses the already-builtRunner.useCapabilitieschanged fromstaleTime: Infinityto60_000(web/src/lib/agent.ts:30), so every mounted editor now refetches/capabilitiesafter 60s of staleness (and on focus). This is a new periodic poll, but the endpoint is a single atomic load + tiny JSON, and the interval is deliberate (to pick up another admin's change). Not a regression worth flagging.🧯 Error handling & edge cases — No material issues found
I'll review this PR through the error-handling/edge-cases lens. Let me verify the key files.
Now let me check the migration runner — does pansy handle the case where migration 0010 isn't applied yet but code tries to read settings at boot?
Let me verify the error-message flow for the bad-model case more closely.
Now I have a thorough understanding. Let me verify a couple final edge cases around the version conflict path and the
stnil-safety.Now let me verify the version-conflict path returns a non-nil
stsowriteVersionConflict(c, st)doesn't serialize nil.I have everything I need. Let me also check one thing: whether the version conflict path returns nil
ston a requireAdmin failure (theerrors.Is(err, ErrVersionConflict)check).Now I've completed my investigation through the error-handling & edge-cases lens. Let me consolidate findings.
The version-conflict path: store returns
(current, ErrVersionConflict), service returns it unchanged, handler writesstundercurrent. Confirmed:currentis non-nil on the conflict path (store returns current row). Safe.The admin-gate path: service's
requireAdminreturns(nil, ErrForbidden)when not admin — but the handler'serrors.Is(err, ErrVersionConflict)is false there, so it falls towriteServiceError. Safe. (And middleware catches it first anyway.)The
parseNullable[bool]edge case:req.AgentEnabledisjson.RawMessage. With an absent field,len(raw)==0→ returns(nil, false, nil)→ inherit. Withnull→(nil, true, nil)→ inherit. Withtrue/false→ pointer. With"yes"→ unmarshal error → 400. Solid.scanInstanceSettingsempty handling:agent_enabledissql.NullInt64; NULL →enabled.Valid=false→AgentEnabledstays nil. Good.One concern — the
rebuildrace betweenEffectiveAgentread andNewRunner: if settings change concurrently the rebuildMu serializes. Fine.A genuine edge-case issue: in
rebuild, whenNewRunnerfails after a settings save that turned the assistant from working→broken,h.ptr.Store(nil)tears down the previously-working runner. That's documented as intentional. Not a bug.Let me check one more: what happens at boot if the DB read in
newAgentHolderfails (e.g., transient).rebuildlogs and returns, leavingptrnil. Boot proceeds. Good — non-fatal as documented.Now the
RequireAdminmiddleware callsmustActor(c)which panics if actor not set. ButrequireAdminruns afterrequireAuthwhich sets the actor. Safe ordering.Let me double check the
UpdateInstanceSettingsvalidate-with-env-key concern: it validates againsts.cfg.Agent.OllamaCloudAPIKey(env key), but the live runner uses the same env key. Consistent. But —agentmodel.Validateonly parses, doesn't call. The test confirmsValidate("", spec)succeeds. So validating with env key when key is absent still works. Edge case handled.I've found no material error-handling issues. Let me write the verdict.
VERDICT: No material issues found
Reviewing the diff through the error-handling & edge-cases lens, I verified the unhappy paths and found them handled deliberately and correctly:
agentHolder.rebuild(agent_holder.go:60-86):EffectiveAgentfailure is logged and the existing Runner is preserved (not torn down on a blip);NewRunnerfailure stores nil (tearing down a stale runner rather than silently keeping the old one) — both paths documented and intentional. Boot (newAgentHolder) is non-fatal on resolution failure, confirmed by readingapi.go:141+main.go:50migration ordering.store.UpdateInstanceSettings(instance_settings.go:70-76):sql.ErrNoRows(version mismatch) fetches the current row and returns(current, ErrVersionConflict); the secondaryGetInstanceSettingserror is propagated, not swallowed. The handler'serrors.Is(err, ErrVersionConflict)branch is only entered on that sentinel, so thenil stfrom a non-conflict error (e.g.ErrForbiddenfromrequireAdmin) flows towriteServiceErrorinstead. Verified by readingsettings.go:122-129and the service'srequireAdmin(instance_settings.go:21-30, returnsErrForbidden, notErrVersionConflict).parseNullable[bool](settings.go:111): absent (len==0), explicitnull, and a non-bool value are all distinct — the first two inherit, the third is a 400. Verified againstparseNullableinerrors.go:79-91.scanInstanceSettings(instance_settings.go:19-32):agent_enabledNULL →sql.NullInt64.Valid=false→AgentEnabledstays nil (inherit); 0/1 → pointer to false/true. No nil-pointer deref.agentChat(agent.go:58-67): the Runner is loaded once into a local before the guard and theRuncall, so a mid-request swap can't flip it between the nil-check and use — exactly the race the comment calls out.requireAdminmiddleware (settings.go:22-31): runs afterrequireAuth(verified route group order inapi.go), somustActor(which panics on a missing actor) is safe.Migrateruns beforeapi.New(main.go:50vs57), so the seeded single row exists before anyGetInstanceSettingsread at boot. The store'ssql.ErrNoRowsbranch is treated as a broken-DB fault, not papered over — correct given the seed.agentmodel.go:54-57+ test):Validateonly parses (no live call), so it works with an empty key; confirmed byTestValidate's no-key case.No swallowed errors, no missing defers, no unchecked nils, and the boundary conditions (empty spec, NULL vs absent vs non-bool, version conflict, race during swap, transient DB fault at boot) are each handled with an explicit, documented decision.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
I reviewed the PR through the security lens across all changed files. Here's what I verified:
Admin enforcement —
isAdminis now checked in both therequireAdminmiddleware (internal/api/settings.go:22-31) and authoritatively in the service seam (internal/service/instance_settings.go:21-30). Non-admins receive 403 on both GET and PATCH. Tests confirm this.No API key leakage —
OLLAMA_CLOUD_API_KEYnever enters the database and is never serialized in the API. The settings response only exposeshasApiKey: boolean(internal/api/settings.go:49). TheeffectiveViewstruct deliberately omits the actual key.Model spec validation —
agentmodel.Validateresolves the spec through majordomo.Parse before storage (internal/service/instance_settings.go:63-66), so a typo is rejected at save time with 400. The spec is not used in SQL, not executed as shell code, and not rendered unsafely.SQL safety — Migration and store code use parameterized queries throughout. The
instanceSettingsColumnsconstant is hardcoded, not user-derived.Race safety — The
agentHolderusesatomic.Pointerfor lock-free reads andsync.Mutexto serialize rebuilds. The chat handler snapshots the pointer once at entry (internal/api/agent.go:63), so a mid-request swap cannot tear.Atomicity of version-conflict response — The version-conflict path returns the current row, but that row has no secret fields and is only reachable by admins (the service gate rejects non-admins before the store layer).
No SSRF/command injection via model spec — The spec goes only to
majordomo.Parseagainst a registry that explicitly registers only theollama.Cloudprovider. There's no path for the spec to influence URLs, commands, or file paths outside majordomo's typed provider model.Nothing in this PR introduces authz gaps, injection vectors, secret leakage, or unsafe handling of untrusted data.
🎯 Correctness — No material issues found
No material issues found
Through the Correctness lens, the change is solid. Key observations:
Atomic pointer swap is correct:
agentHolderserializes rebuilds withsync.Mutexwhile reads (get()) stay lock-free viaatomic.Pointer. The old Runner is replaced; new requests see the new one, and in-flight requests hold their reference until done. No use-after-free or torn-read risk.Precedence layering is correct:
EffectiveAgentapplies DB overrides (agent_modelnon-empty,agent_enablednon-nil) atop env defaults. The env model can never be empty becauseconfig.Load()falls back toDefaultAgentModelwhenPANSY_AGENT_MODELis unset or blank. The inherit sentinel (""/nil) correctly falls through.Validation gate is at the right layer:
UpdateInstanceSettingsvalidates the model spec viaagentmodel.Validatebefore storing, so a bad spec isErrInvalidInput(400) at save time. Empty model skips validation because it means "inherit" — the env value was already checked at boot when the initial Runner built (or failed to).Migration schema is correct:
agent_enabled INTEGER CHECK (agent_enabled IN (0, 1))permitsNULLbecause SQLite CHECK passes when the expression is not false (NULL is allowed). The comment in the PR notes explicitly calls this out, and it matches SQLite semantics.503 contract change is handled end-to-end: The chat handler nil-checks
agent.get()and returns 503AGENT_DISABLED. The frontend'sstreamChatmaps 503 to a human message, anduseCapabilitiesdropsstaleTime: Infinityso the tab state reflects reality. The history routes remain on thegardensgroup (notagentGroup), so they stay accessible when the assistant is off, which is the stated design.Context detachment in rebuild mirrors existing pattern:
context.WithoutCancelinupdateSettingsbeforerebuildmatches thecommitScope/RecordAgentExchangepattern — once the DB write is committed, a client disconnect must not leave the live Runner out of step with stored settings.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/pages/SettingsPage.tsx:22—fromChoicerelies on a cryptic ternary (c === 'inherit' ? null : c === 'on') where'off'silently falls through tofalsebecause'off' !== 'on'. It happens to be correct today, but ifEnabledChoiceever gains a value the compiler won't catch the missing branch and the function will silently misbehave. Use an explicitswitchorif/elsechain so every choice is handled deliberately.internal/service/instance_settings.go:81and:93—EffectiveAgentis both a struct type and a*Servicemethod in the same package. This hurts godoc readability and IDE navigation; searching for the type jumps to the method and vice-versa. Rename the method to something distinct likeResolveEffectiveAgentorCurrentAgentConfig.internal/api/settings.go:63/internal/service/instance_settings.go:94—settingsPayloadtriggers a secondGetInstanceSettingsDB read even though its caller (getSettings) already holds the row.updateSettingscompounds this: it callsEffectiveAgentinrebuildand again insettingsPayload, so a single PATCH can hit the same row three or four times. This is a leaky abstraction:EffectiveAgentshould accept an already-loaded*domain.InstanceSettings(or expose a helper that layers settings over env without fetching), so callers that already have the row don't pay for redundant queries.⚡ Performance — No material issues found
No material issues found
I examined the changes through the Performance lens and found nothing material.
/capabilities, chat handler) uses atomic pointer loads (h.agent.get()) — lock-free and correct. No contention under read load.agentHolder.rebuildserializes writes with a mutex, but rebuilds only happen on admin settings changes (rare). The lock is not held across any I/O; model resolution is an in-memory parse.EffectiveAgentdoes re-read the settings row on every call, which meansGET /settingsandPATCH /settingsissue an extra identical SELECT. This is real but cold-path (admin-only, low volume) and not material to overall system performance.atomic.Pointer.Storeis safe; old runners become unreachable and are GC'd when in-flight requests finish. No unbounded growth.🧯 Error handling & edge cases — Minor issues
Minor issues
internal/service/instance_settings.go:64—agentmodel.Validatereturns a specific error (e.g. empty spec, unknown provider, malformed failover chain), butUpdateInstanceSettingsswallows it and returns a genericdomain.ErrInvalidInput. An admin POSTing a bad model gets a 400 with codeINVALID_INPUTand message"invalid input", with no hint that it's the model spec that failed. The underlyingerrshould at least be logged so an operator can diagnose it, or the API should surface it.Verified by reading
internal/service/instance_settings.goandinternal/api/errors.go:54.internal/api/settings_test.go:180-208—TestSettingsSwapUnderRacespawns 4 reader goroutines that loop until<-done, then closesdoneand returns immediately. The test does not wait for the goroutines to finish, so they may still be running (and callingdoJSON, which usestesting.T) after the test function exits. This is a real race undergo test -racethat can panic or produce spurious race reports. Add async.WaitGroupand wait for the workers before returning.Verified by reading the test body and the goroutine spawn/exit pattern.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
- Remove config.AgentConfig.Ready() — dead after the refactor (no non-test callers), and its doc comment ("route registration and capabilities gate on this") was now false. EffectiveAgent.Ready() carries the same logic and IS used, see below. - agentHolder now uses eff.Ready() and eff.APIKey instead of an inline check and a redundant apiKey field it held separately. This makes EffectiveAgent.APIKey/ Ready() production-used rather than test-only, drops the duplicated readiness check, and simplifies newAgentHolder's signature. - settingsPayload no longer swallows an EffectiveAgent error into a misleading empty "effective" view (which would read as "nothing configured"). It returns the error; the handlers surface it as a 500. EffectiveAgent re-reads the row GetInstanceSettings just returned, so a failure there is a real DB fault. - Frontend streamChat handles 503 (assistant disabled at runtime) distinctly from 404 (endpoint absent) — the backend returns 503 AGENT_DISABLED now, which the old code mislabelled. - Fixed the api.go comment that still said a disabled request gets a 404 — it's a 503. - updateSettings uses the shared parseNullable[bool] instead of a bespoke parseNullableBool (now removed). - NewRunner drops its empty-spec guard; agentmodel.Resolve already owns that check, so one place decides what a valid spec is. - rebuild-on-resolve-error now documents WHY it keeps the current Runner rather than tearing down a working assistant on a transient DB blip: the state is persisted, the next rebuild reconciles, and killing a live assistant on a read hiccup is worse than a brief stale window. Not changed: the store's version-conflict fallback returning GetInstanceSettings' error instead of ErrVersionConflict when that read also fails. That's the exact pattern every other version-guarded update uses (gardens/objects/plants); making only this one differ would be the inconsistency. A DB read failing immediately after the guarded UPDATE on the same local SQLite file is a disk-fault edge case, and surfacing that error is defensible. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ@gadfly review
Worked the blocking round — the three multi-model findings and most singles:
AgentConfig.Ready()removed (no non-test callers; its doc was now false).settingsPayloadswallowingEffectiveAgenterrors → it now returns the error and the handlers surface a 500, instead of a misleading empty "effective" view that read as "nothing configured".streamChatnow handles the 503AGENT_DISABLEDdistinctly, and the stale api.go "404" comment is fixed.EffectiveAgent.APIKey/Ready()production-dead → the holder now useseff.Ready()andeff.APIKeyinstead of an inline check and a separateapiKeyfield it duplicated; that also simplifiednewAgentHolder.parseNullableBoolduplicating the generic → replaced withparseNullable[bool].NewRunner/agentmodel.Resolve→ dropped fromNewRunner; Resolve owns it.rebuildleaving a stale runner onEffectiveAgenterror → kept the behaviour but documented why: the state is already persisted, the next rebuild reconciles, and tearing down a working assistant on a transient local-SQLite read blip is worse than a brief stale window. Open to changing it if you'd rather it fail closed (set nil on resolve error) — that has its own downside (a DB hiccup turns off a working assistant), so I left it fail-safe.Not changed, with reason: the store's version-conflict fallback returning
GetInstanceSettings' error instead ofErrVersionConflictwhen that read also fails. That's the identical pattern inUpdateGarden/UpdateObject/UpdatePlanting— making only this one differ would be the inconsistency, and it's a disk-fault edge case (a read failing right after the guarded UPDATE on the same local file).Re-verified: full suite green, settings swap still race-clean under
-race,tsc/vitest pass.🪰 Gadfly consensus review — 8 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +60,4 @@// 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())🟡 EffectiveAgent re-reads instance_settings row that GetInstanceSettings just fetched
maintainability, performance · flagged by 2 models
internal/api/settings.go:63andinternal/api/settings.go:141—settingsPayloadcallsh.svc.EffectiveAgent, which always does a freshGetInstanceSettingsDB read. IngetSettings, the same row was just fetched byGetInstanceSettings. InupdateSettings, the row was just written and returned, thenrebuildcallsEffectiveAgent(second read), thensettingsPayloadcalls it again (third read). A single PATCH can hit the same row 3 times. Pass the already-loaded row into `Effe…🪰 Gadfly · advisory
@@ -0,0 +93,4 @@// 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"`🟡 PATCH /settings binds agentModel as a plain string (full-replacement semantics), so a request that omits it resets the stored model to "inherit" — intentional and documented, but inconsistent with the rest of the codebase's absent=unchanged PATCH convention
correctness · flagged by 1 model
internal/api/settings.go:96(settingsUpdateRequest.AgentModel) — bound as a plainstring, not the*string/json.RawMessage"absent vs. explicit" pattern used elsewhere (contrastobjectUpdateRequestininternal/api/objects.go:50-65). Since""is the domain's own sentinel for "inherit from environment" (internal/service/instance_settings.go:60-67,EffectiveAgentat line 103), a PATCH that omitsagentModelentirely is indistinguishable from one that explicitly clears it, so `…🪰 Gadfly · advisory
@@ -0,0 +177,4 @@// 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) {🟠 Test goroutines may outlive test function causing race/panic under -race
error-handling · flagged by 1 model
internal/api/settings_test.go:180-208—TestSettingsSwapUnderRacespawns 4 reader goroutines that loop until<-done, then closesdoneand returns immediately. The test does not wait for the goroutines to finish, so they may still be running (and callingdoJSON, which usestesting.T) after the test function exits. This is a real race undergo test -racethat can panic or produce spurious race reports. Add async.WaitGroupand wait for the workers before returning. *Verif…🪰 Gadfly · advisory
@@ -0,0 +24,4 @@return err}if !u.IsAdmin {return domain.ErrForbidden🟡 requireAdmin re-queries user table when IsAdmin already verified by middleware
performance · flagged by 1 model
internal/service/instance_settings.go:27-36—requireAdminqueriesGetUserByIDto checkIsAdminon every settings request, butmustActor(c).IsAdminwas already resolved during session authentication and checked by therequireAdmin()middleware. This adds a DB roundtrip to an already-gated endpoint. The author calls this "redundant by design" (service seam authority), which is fair for correctness, but from a performance lens it's still avoidable overhead on an endpoint that does li…🪰 Gadfly · advisory
@@ -0,0 +61,4 @@// 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 {🟠 Swallowed validation error leaves admin with no diagnostic on bad model spec
error-handling · flagged by 1 model
internal/service/instance_settings.go:64—agentmodel.Validatereturns a specific error (e.g. empty spec, unknown provider, malformed failover chain), butUpdateInstanceSettingsswallows it and returns a genericdomain.ErrInvalidInput. An admin POSTing a bad model gets a 400 with codeINVALID_INPUTand message"invalid input", with no hint that it's the model spec that failed. The underlyingerrshould at least be logged so an operator can diagnose it, or the API should surfa…🪰 Gadfly · advisory
@@ -0,0 +78,4 @@// 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 {🟠 EffectiveAgent type and method share the same name in one package
maintainability · flagged by 1 model
internal/service/instance_settings.go:81and:93—EffectiveAgentis both a struct type and a*Servicemethod in the same package. This hurts godoc readability and IDE navigation; searching for the type jumps to the method and vice-versa. Rename the method to something distinct likeResolveEffectiveAgentorCurrentAgentConfig.🪰 Gadfly · advisory
@@ -0,0 +84,4 @@APIKey string}// Ready mirrors config.AgentConfig.Ready: enabled, with a key and a model.🟡 Stale doc comment references config.AgentConfig.Ready(), which this PR deletes
maintainability · flagged by 2 models
internal/service/instance_settings.go:87— comment referencesconfig.AgentConfig.Ready, which no longer exists (grep confirms zero definitions). TheEffectiveAgent.Readymethod exists but the comment points at the deleted one.🪰 Gadfly · advisory
@@ -0,0 +19,4 @@// 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')🟠 fromChoice silently falls through 'off' in cryptic ternary
maintainability · flagged by 1 model
web/src/pages/SettingsPage.tsx:22—fromChoicerelies on a cryptic ternary (c === 'inherit' ? null : c === 'on') where'off'silently falls through tofalsebecause'off' !== 'on'. It happens to be correct today, but ifEnabledChoiceever gains a value the compiler won't catch the missing branch and the function will silently misbehave. Use an explicitswitchorif/elsechain so every choice is handled deliberately.🪰 Gadfly · advisory