Seed-packet capture: vision model, extraction, catalog match, create (backend) #94

Merged
steve merged 2 commits from feat/seed-packet-backend into main 2026-07-22 04:22:27 +00:00
17 changed files with 1198 additions and 37 deletions
+4 -1
View File
@@ -12,6 +12,7 @@ Work is tracked in Gitea issues; the tracking epic links every piece in dependen
- **Users:** multi-user with ownership. Users own gardens; a garden can be shared with other users as viewer (read) or editor (edit content). Owner additionally shares/deletes. The first registered user is `is_admin` (set race-free inside the INSERT); admin gates instance-wide Settings (`requireAdmin`), the only thing that reads that flag.
- **Auth:** OIDC-first (Authentik is the primary IdP), local argon2id passwords as an optional fallback.
- **Instance settings (#79):** admin-editable, instance-wide config in a single-row `instance_settings` table — pansy's first DB-stored *instance* state (everything else hangs off a garden/object). Today it holds the agent model + on/off; **secrets never move here**`OLLAMA_CLOUD_API_KEY` stays in the env so it doesn't land in backups or the undo history. Precedence: Settings value → env → default. The live agent Runner sits behind an `atomic.Pointer` in the API layer (`agentHolder`) with its routes always registered, so a settings change swaps it with no restart and no race against in-flight requests; `/capabilities` reads that pointer, so it reports what's live rather than what was configured at boot. The model/registry knowledge lives in one leaf package (`internal/agentmodel`) that both the runner and the settings validator import — agent imports service, so it can live in neither.
- **Seed-packet capture (#81):** photograph a packet → a *vision* model (separate `vision_model` setting) reads it into structured fields via one-shot `majordomo.Generate[SeedPacket]` — NOT an agent loop, so the extraction can't touch the garden; it only reads a picture and returns data. The image is normalized to JPEG at the upload boundary (`internal/imagenorm`: decodes HEIC/webp/png/jpeg, since majordomo's stdlib media path can't do HEIC — the iPhone default). The hard part is **catalog matching, not OCR**: a wrong auto-match splits a variety's seed-lot history across duplicate rows, so the service NEVER auto-creates — it surfaces ranked candidates (`matchPlants`) and the user confirms, then `CreateFromPacket` makes the plant (new or existing) + the lot. Plants/lots aren't in the undo history (they're catalog/inventory), so there's no change set to wrap. The extractor is injectable on the service (`WithPacketExtractor`) so the whole path tests hermetically against majordomo's `fake` provider.
- **Agentic future:** integration with majordomo/executus via typed Go tools (`llm.DefineTool[Args]`) wrapping the same service layer the REST API uses — not MCP/OpenAPI.
## Domain model
@@ -69,12 +70,14 @@ POST /objects/:id/fill ← hex-pack a region with one plant; region by co
POST /objects/:id/clear ← soft-remove every active plop, as ONE change set
GET,POST /plants PATCH,DELETE /plants/:id (own plants only)
GET,POST /seed-lots GET,PATCH,DELETE /seed-lots/:id (own lots only; private)
POST /seed-lots/scan ← multipart image → a seed-packet proposal (reads only, no writes)
POST /seed-lots/from-packet ← confirmed proposal → a plant (new or existing) + a lot
GET,POST /gardens/:id/journal PATCH,DELETE /journal/:id (editor writes; author edits own)
GET /gardens/:id/journal/counts ← entries per object, for the "has notes" indicator
POST /agent/chat ← SSE: step events, then the finished turn (editor only)
GET,DELETE /gardens/:id/agent/history (the actor's own thread)
GET /capabilities ← what this instance can do RIGHT NOW (tracks the live agent, not just config)
GET,PATCH /settings ← instance-wide config (admin only): agent model + on/off
GET,PATCH /settings ← instance-wide config (admin only): agent model + on/off, vision model
GET,POST /gardens/:id/shares PATCH,DELETE /gardens/:id/shares/:userId (invite by email)
GET,POST,DELETE /gardens/:id/share-link ← the public read-only token for this garden
GET /public/gardens/:token ← UNAUTHENTICATED read-only /full; the token is the capability
+2 -1
View File
@@ -70,8 +70,9 @@ The garden assistant reads three more. Setting none of them leaves the assistant
| `OLLAMA_CLOUD_API_KEY` | *(empty)* | Ollama Cloud API key. Without it the assistant is off, not broken. This is the one agent value that stays in the environment — it is **never** stored in the database or editable in Settings. |
| `PANSY_AGENT_MODEL` | `ollama-cloud/glm-5.2:cloud` | Default model spec, passed verbatim to `majordomo.Parse` — a comma-separated list is a failover chain, e.g. `ollama-cloud/glm-5.2:cloud,ollama-cloud/kimi-k2.6:cloud`. An admin can override this per-instance in **Settings** without a redeploy; a blank Settings value inherits this. |
| `PANSY_AGENT_ENABLED` | on when a key is present | Default on/off for the assistant. Also overridable in Settings (which can inherit this default). |
| `PANSY_VISION_MODEL` | *(empty)* | Default model for **seed-packet capture** (photograph a packet → it fills in the plant + purchase). A *vision-capable* model (the chat model may not be). Empty = the feature isn't offered. Runs against the same `OLLAMA_CLOUD_API_KEY`, and is overridable in Settings. |
The model and enabled flag can be changed at runtime by an admin under **Settings** (the gear appears in the nav for admins) — the change swaps the live assistant with no restart. The env vars above are the defaults an untouched instance uses, and the API key is intentionally not among the runtime-editable settings: a secret in the database would land in every backup. Precedence for the model and enabled flag is **Settings value, if set → env var → built-in default**.
The agent model + enabled flag, and the vision model, can be changed at runtime by an admin under **Settings** (the gear appears in the nav for admins) — an agent change swaps the live assistant with no restart. The env vars above are the defaults an untouched instance uses, and the API key is intentionally not among the runtime-editable settings: a secret in the database would land in every backup. Precedence is **Settings value, if set → env var → built-in default**.
The assistant acts without asking first, which is only reasonable because every turn is one undoable change set — see the History panel in the editor.
+17 -1
View File
@@ -185,6 +185,10 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
seedLots.GET("/:id", h.getSeedLot)
seedLots.PATCH("/:id", h.updateSeedLot)
seedLots.DELETE("/:id", h.deleteSeedLot)
// Seed-packet capture (#81): scan a photo into a proposal, then create the
// plant + lot from the confirmed proposal. scan reads only.
seedLots.POST("/scan", h.scanSeedPacket)
seedLots.POST("/from-packet", h.createFromPacket)
// Public, unauthenticated read of a garden by its share token. Deliberately
// NOT behind requireAuth: the token is the capability, so a logged-out visitor
@@ -204,7 +208,19 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
// so offering the tab must track the live Runner. Reading agent.get() (an atomic
// load) means this reflects a settings-driven swap on the very next poll.
func (h *handlers) capabilities(c *gin.Context) {
c.JSON(http.StatusOK, gin.H{"agent": h.agent.get() != nil})
// vision advertises whether seed-packet scanning (#81) can be offered — a
// configured, resolvable vision model + a key. Read per-request so a settings
// change is reflected on the next poll, same as agent.
Review

🔴 EffectiveVision DB error silently swallowed in capabilities endpoint

error-handling, performance · flagged by 5 models

  • internal/api/api.go:215-217EffectiveVision error silently swallowed. In capabilities, if the DB query fails (e.g., locked row, connection timeout), the error is discarded and vision is reported as false with a 200 OK. The UI hides the scan button and the admin has no signal that the server is in distress. The error should at minimum be logged; returning the error to the client (500) or defaulting to the env value with a log line are both better than silence.

🪰 Gadfly · advisory

🔴 **EffectiveVision DB error silently swallowed in capabilities endpoint** _error-handling, performance · flagged by 5 models_ - **`internal/api/api.go:215-217`** — `EffectiveVision` error silently swallowed. In `capabilities`, if the DB query fails (e.g., locked row, connection timeout), the error is discarded and `vision` is reported as `false` with a 200 OK. The UI hides the scan button and the admin has no signal that the server is in distress. The error should at minimum be logged; returning the error to the client (500) or defaulting to the env value with a log line are both better than silence. <sub>🪰 Gadfly · advisory</sub>
vision := false
if vis, err := h.svc.EffectiveVision(c.Request.Context()); err != nil {
// A read fault here means the DB is unhappy; report vision off (safe: the
// UI just hides a button) but don't do it silently — the same best-effort
// settings reads elsewhere log rather than swallow.
slog.Error("api: could not resolve vision settings for capabilities", "error", err)
} else {
vision = vis.Ready()
}
c.JSON(http.StatusOK, gin.H{"agent": h.agent.get() != nil, "vision": vision})
}
// healthz is a liveness probe: always returns {"ok": true} when the server is up.
+24 -8
View File
@@ -16,9 +16,12 @@ import (
// the buyer — a lot is never shared along with a garden — so every handler here
// scopes to the session actor with no garden in the picture.
// seedLotCreateRequest is the body for POST /seed-lots.
type seedLotCreateRequest struct {
PlantID int64 `json:"plantId" binding:"required"`
// seedLotFields is the lot half of a create body — every field EXCEPT which plant
// it attaches to. seedLotCreateRequest adds a required plantId; the seed-packet
// confirm supplies none (the plant comes from its plantId/newPlant choice), so it
// embeds these fields directly. Sharing one struct keeps the two request shapes —
// and their validation — from drifting apart.
type seedLotFields struct {
Vendor string `json:"vendor"`
SourceURL string `json:"sourceUrl"`
SKU string `json:"sku"`
@@ -32,15 +35,28 @@ type seedLotCreateRequest struct {
Notes string `json:"notes"`
}
func (r seedLotCreateRequest) toInput() service.SeedLotInput {
// toInput builds the service input with no plant attribution; callers that know
// the plant (the create handler; the packet confirm) set PlantID afterwards.
func (f seedLotFields) toInput() service.SeedLotInput {
return service.SeedLotInput{
PlantID: r.PlantID, Vendor: r.Vendor, SourceURL: r.SourceURL, SKU: r.SKU,
LotCode: r.LotCode, PurchasedAt: r.PurchasedAt, PackedForYear: r.PackedForYear,
Quantity: r.Quantity, Unit: r.Unit, CostCents: r.CostCents,
GerminationPct: r.GerminationPct, Notes: r.Notes,
Vendor: f.Vendor, SourceURL: f.SourceURL, SKU: f.SKU, LotCode: f.LotCode,
PurchasedAt: f.PurchasedAt, PackedForYear: f.PackedForYear, Quantity: f.Quantity,
Unit: f.Unit, CostCents: f.CostCents, GerminationPct: f.GerminationPct, Notes: f.Notes,
}
}
// seedLotCreateRequest is the body for POST /seed-lots.
type seedLotCreateRequest struct {
PlantID int64 `json:"plantId" binding:"required"`
seedLotFields
}
func (r seedLotCreateRequest) toInput() service.SeedLotInput {
in := r.seedLotFields.toInput()
in.PlantID = r.PlantID
return in
}
// seedLotUpdateRequest is the body for PATCH /seed-lots/:id: every field
// optional, plus the required current version. The nullable columns are
// json.RawMessage so an explicit null (clear it) is distinguishable from an
+135
View File
@@ -0,0 +1,135 @@
package api
import (
"errors"
"net/http"
"time"
"github.com/gin-gonic/gin"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/imagenorm"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
)
// Seed-packet capture (#81). Two steps, deliberately separate:
// POST /seed-lots/scan multipart image → a proposal (reads only)
// POST /seed-lots/from-packet confirmed proposal → a plant + lot
// The scan never writes; creation happens only from an explicit confirm, so a
// misread can't add anything to the catalog on its own.
// scanUploadLimit bounds the multipart body. imagenorm caps the decoded image at
// 25 MiB; this is a little over that for the multipart envelope. A phone photo is
// a few MB, so this is generous.
const scanUploadLimit = 30 << 20
// scanReadTimeout is how long we allow the image upload to take. The server's
// default ReadTimeout (15s) is fine for JSON but tight for a multi-megabyte photo
// on a slow phone connection, so this endpoint extends it — the same
// ResponseController mechanism the SSE path uses for writes (#78).
const scanReadTimeout = 60 * time.Second
// scanWriteTimeout extends the write deadline for the same reason. The server's
// absolute WriteTimeout (30s) is measured from the start of the request, but this
// handler's response can't be written until AFTER a slow upload AND a live vision
// call — together easily past 30s. Without this, a successful extraction's
// response is silently dropped: the exact failure mode #78 fixed for SSE.
Outdated
Review

🔴 scanSeedPacket extends the read deadline but not the write deadline, so a slow upload trips the server's absolute 30s WriteTimeout and silently drops a successful extraction's response

error-handling · flagged by 3 models

🪰 Gadfly · advisory

🔴 **scanSeedPacket extends the read deadline but not the write deadline, so a slow upload trips the server's absolute 30s WriteTimeout and silently drops a successful extraction's response** _error-handling · flagged by 3 models_ <sub>🪰 Gadfly · advisory</sub>
const scanWriteTimeout = 120 * time.Second
// scanSeedPacket reads an uploaded packet photo and returns a proposal.
func (h *handlers) scanSeedPacket(c *gin.Context) {
// Extend both deadlines for the (potentially large, potentially slow) upload
// and the live vision call that follows. Best-effort: if the writer doesn't
// support it, the server defaults apply.
rc := http.NewResponseController(c.Writer)
Outdated
Review

🟠 file.Open() server I/O error returned as 400 INVALID_INPUT

error-handling · flagged by 1 model

  • internal/api/seed_packet.go:44-47file.Open() error misclassified as client error. After c.FormFile("image") succeeds, file.Open() can fail for server-side reasons (temp file removed, disk I/O error). The handler returns 400/INVALID_INPUT ("could not read the uploaded image"), but this is a server failure and should be 500.

🪰 Gadfly · advisory

🟠 **file.Open() server I/O error returned as 400 INVALID_INPUT** _error-handling · flagged by 1 model_ - **`internal/api/seed_packet.go:44-47`** — `file.Open()` error misclassified as client error. After `c.FormFile("image")` succeeds, `file.Open()` can fail for server-side reasons (temp file removed, disk I/O error). The handler returns 400/`INVALID_INPUT` ("could not read the uploaded image"), but this is a server failure and should be 500. <sub>🪰 Gadfly · advisory</sub>
_ = rc.SetReadDeadline(time.Now().Add(scanReadTimeout))
_ = rc.SetWriteDeadline(time.Now().Add(scanWriteTimeout))
c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, scanUploadLimit)
file, err := c.FormFile("image")
if err != nil {
// A body over scanUploadLimit trips MaxBytesReader — that's 413, not a
// malformed request. Everything else here is a genuinely missing/garbled
// multipart field.
var tooBig *http.MaxBytesError
Outdated
Review

🟠 imagenorm I/O errors misclassified as 400 INVALID_INPUT

error-handling · flagged by 1 model

  • internal/api/seed_packet.go:54-62 — Non-image errors from imagenorm.Normalize misclassified as client errors. Normalize returns wrapped I/O errors (e.g., read failure during decode, JPEG encode failure) that are neither ErrTooLarge nor ErrUnsupported. These are server-side failures, but the handler falls through to 400/INVALID_INPUT ("that doesn't look like an image we can read"). It should branch on errors.Is(err, imagenorm.ErrUnsupported) → 400, and any other error → 500.

🪰 Gadfly · advisory

🟠 **imagenorm I/O errors misclassified as 400 INVALID_INPUT** _error-handling · flagged by 1 model_ - **`internal/api/seed_packet.go:54-62`** — Non-image errors from `imagenorm.Normalize` misclassified as client errors. `Normalize` returns wrapped I/O errors (e.g., read failure during decode, JPEG encode failure) that are neither `ErrTooLarge` nor `ErrUnsupported`. These are server-side failures, but the handler falls through to 400/`INVALID_INPUT` ("that doesn't look like an image we can read"). It should branch on `errors.Is(err, imagenorm.ErrUnsupported)` → 400, and any other error → 500. <sub>🪰 Gadfly · advisory</sub>
if errors.As(err, &tooBig) {
writeAPIError(c, http.StatusRequestEntityTooLarge, "IMAGE_TOO_LARGE", "that image is too large — try a smaller photo")
return
}
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "attach an image as the \"image\" field")
return
}
f, err := file.Open()
if err != nil {
Review

format return value discarded speculatively ("available for logging if wanted")

maintainability · flagged by 2 models

  • internal/api/seed_packet.go:63_ = format // available for logging if wanted discards a value kept only on the chance it's useful later. It's the only instance of this pattern anywhere in internal/ — not an established codebase convention. Either drop the named return (jpeg, _, err := imagenorm.Normalize(...)) or actually use it.

🪰 Gadfly · advisory

⚪ **format return value discarded speculatively ("available for logging if wanted")** _maintainability · flagged by 2 models_ - **`internal/api/seed_packet.go:63`** — `_ = format // available for logging if wanted` discards a value kept only on the chance it's useful later. It's the only instance of this pattern anywhere in `internal/` — not an established codebase convention. Either drop the named return (`jpeg, _, err := imagenorm.Normalize(...)`) or actually use it. <sub>🪰 Gadfly · advisory</sub>
// Opening the parsed upload failed on our side, not the client's.
writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "could not read the uploaded image")
return
}
defer f.Close()
// Normalize to JPEG (decodes HEIC/webp/png/jpeg, downscales, re-encodes) so
// everything downstream — including the vision model — only sees a format it
// can read. This is where an iPhone HEIC becomes usable.
jpeg, _, err := imagenorm.Normalize(f, imagenorm.Options{})
if err != nil {
switch {
case errors.Is(err, imagenorm.ErrTooLarge):
writeAPIError(c, http.StatusRequestEntityTooLarge, "IMAGE_TOO_LARGE", "that image is too large — try a smaller photo")
case errors.Is(err, imagenorm.ErrUnsupported):
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "that doesn't look like an image we can read (JPEG, PNG, HEIC or WebP)")
default:
// A read or re-encode fault is ours, not bad input.
writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "could not process the uploaded image")
}
Review

🟡 packetLotRequest and its toInput() duplicate seedLotCreateRequest field-for-field

maintainability · flagged by 3 models

  • internal/api/seed_packet.go:83-98 vs internal/api/seed_lots.go:20-42packetLotRequest and its toInput() reproduce all 11 fields and the field-by-field mapping of seedLotCreateRequest/its toInput() verbatim, differing only in the absence of PlantID. The comment explains why a separate type exists (reusing seedLotCreateRequest would wrongly require plantId), a fair constraint, but the duplication itself is avoidable — e.g. an embedded lotFields struct that both request…

🪰 Gadfly · advisory

🟡 **packetLotRequest and its toInput() duplicate seedLotCreateRequest field-for-field** _maintainability · flagged by 3 models_ - **`internal/api/seed_packet.go:83-98` vs `internal/api/seed_lots.go:20-42`** — `packetLotRequest` and its `toInput()` reproduce all 11 fields and the field-by-field mapping of `seedLotCreateRequest`/its `toInput()` verbatim, differing only in the absence of `PlantID`. The comment explains why a separate type exists (reusing `seedLotCreateRequest` would wrongly require `plantId`), a fair constraint, but the duplication itself is avoidable — e.g. an embedded `lotFields` struct that both request… <sub>🪰 Gadfly · advisory</sub>
return
}
prop, err := h.svc.ExtractSeedPacket(c.Request.Context(), mustActor(c).ID, jpeg)
if err != nil {
// A missing vision model surfaces as ErrInvalidInput from the service; give
Review

🟡 packetLotRequest duplicates seedLotCreateRequest fields and toInput logic

maintainability · flagged by 1 model

  • internal/api/seed_packet.go:89-109packetLotRequest duplicates seedLotCreateRequest field-for-field (minus PlantID) and duplicates its toInput() body almost verbatim. If a field is added to the lot later (e.g. a new nullable column), packetLotRequest will almost certainly drift and the from-packet path will silently drop it. A shared embedded struct or at least a shared helper would keep the two in sync.

🪰 Gadfly · advisory

🟡 **packetLotRequest duplicates seedLotCreateRequest fields and toInput logic** _maintainability · flagged by 1 model_ - `internal/api/seed_packet.go:89-109` — `packetLotRequest` duplicates `seedLotCreateRequest` field-for-field (minus `PlantID`) and duplicates its `toInput()` body almost verbatim. If a field is added to the lot later (e.g. a new nullable column), `packetLotRequest` will almost certainly drift and the `from-packet` path will silently drop it. A shared embedded struct or at least a shared helper would keep the two in sync. <sub>🪰 Gadfly · advisory</sub>
// it a clearer message than the generic 400, since the UI shouldn't have
// offered the button at all in that case.
if errors.Is(err, domain.ErrInvalidInput) {
writeAPIError(c, http.StatusServiceUnavailable, "VISION_DISABLED", "packet scanning isn't set up on this instance")
return
}
writeServiceError(c, err)
return
}
c.JSON(http.StatusOK, prop)
}
// fromPacketRequest confirms a proposal: exactly one of plantId (attach to an
// existing plant) or newPlant (create a variety), plus the lot to record. The lot
// is seedLotFields — the create body's lot half WITHOUT plantId, since the plant
// comes from the plantId/newPlant choice, not the lot body.
type fromPacketRequest struct {
PlantID *int64 `json:"plantId"`
NewPlant *plantCreateRequest `json:"newPlant"`
Lot seedLotFields `json:"lot"`
}
// createFromPacket turns a confirmed proposal into a plant + lot.
func (h *handlers) createFromPacket(c *gin.Context) {
var req fromPacketRequest
if err := c.ShouldBindJSON(&req); err != nil {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "a lot and exactly one of plantId or newPlant are required")
return
}
confirm := service.PacketConfirm{
PlantID: req.PlantID,
Lot: req.Lot.toInput(), // no plantId in the lot body; the service attributes it
}
if req.NewPlant != nil {
in := req.NewPlant.toInput()
confirm.NewPlant = &in
}
res, err := h.svc.CreateFromPacket(c.Request.Context(), mustActor(c).ID, confirm)
if err != nil {
writeServiceError(c, err)
return
}
c.JSON(http.StatusCreated, res)
}
+237
View File
@@ -0,0 +1,237 @@
package api
import (
"bytes"
"context"
"image"
"image/png"
"mime/multipart"
"net/http"
"net/http/httptest"
"testing"
"github.com/gin-gonic/gin"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/store"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/vision"
)
// packetEngine builds an engine whose service reads seed packets via the given
// canned extractor, so the scan endpoint can be tested without a live model.
func packetEngine(t *testing.T, cfg *config.Config, extract func() (vision.SeedPacket, error)) *gin.Engine {
t.Helper()
gin.SetMode(gin.TestMode)
db, err := store.Open(":memory:")
if err != nil {
t.Fatalf("store.Open: %v", err)
}
t.Cleanup(func() { db.Close() })
if err := db.Migrate(context.Background()); err != nil {
t.Fatalf("Migrate: %v", err)
}
svc := service.New(db, cfg, service.WithPacketExtractor(
func(context.Context, string, string, []byte) (vision.SeedPacket, error) { return extract() },
))
return New(cfg, svc)
}
// visionCfg is a config with a vision model + key configured, so packet scanning
// is available.
func visionCfg() *config.Config {
c := localCfg()
c.Agent = config.AgentConfig{OllamaCloudAPIKey: "k", VisionModel: "ollama-cloud/vision:cloud"}
return c
}
// pngUpload builds a multipart body with a real PNG under the "image" field.
func pngUpload(t *testing.T) (body *bytes.Buffer, contentType string) {
t.Helper()
var img bytes.Buffer
if err := png.Encode(&img, image.NewRGBA(image.Rect(0, 0, 32, 24))); err != nil {
t.Fatalf("encode png: %v", err)
}
var buf bytes.Buffer
w := multipart.NewWriter(&buf)
part, err := w.CreateFormFile("image", "packet.png")
if err != nil {
t.Fatalf("form file: %v", err)
}
part.Write(img.Bytes())
w.Close()
return &buf, w.FormDataContentType()
}
func doMultipart(t *testing.T, r *gin.Engine, path, contentType string, body *bytes.Buffer, cookie *http.Cookie) *httptest.ResponseRecorder {
t.Helper()
req := httptest.NewRequest(http.MethodPost, path, body)
req.Header.Set("Content-Type", contentType)
if cookie != nil {
req.AddCookie(cookie)
}
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
return w
}
// TestScanSeedPacketAPI: a multipart image → a proposal, end to end through the
// router. The extractor is canned; imagenorm runs for real on the uploaded PNG.
func TestScanSeedPacketAPI(t *testing.T) {
r := packetEngine(t, visionCfg(), func() (vision.SeedPacket, error) {
return vision.SeedPacket{Species: "garlic", Variety: "Music", Category: "vegetable"}, nil
})
cookie := registerAndCookie(t, r, "[email protected]")
// A matching plant so the proposal has a candidate.
createPlantAPI(t, r, cookie, "Music Garlic", 15)
body, ct := pngUpload(t)
w := doMultipart(t, r, "/api/v1/seed-lots/scan", ct, body, cookie)
if w.Code != http.StatusOK {
t.Fatalf("scan: status %d, body %s", w.Code, w.Body.String())
}
res := decodeMap(t, w.Body.Bytes())
pkt, _ := res["packet"].(map[string]any)
if pkt["variety"] != "Music" {
t.Errorf("packet variety = %v", pkt["variety"])
}
if cands, _ := res["candidates"].([]any); len(cands) == 0 {
t.Error("expected a candidate match for Music Garlic")
}
if res["suggestedName"] != "Music" {
t.Errorf("suggestedName = %v", res["suggestedName"])
}
}
// TestScanSeedPacketErrorsAPI: no image, unreadable bytes, and vision-not-
// configured each get their own clear status.
func TestScanSeedPacketErrorsAPI(t *testing.T) {
// Vision configured, so we reach imagenorm / the extractor.
r := packetEngine(t, visionCfg(), func() (vision.SeedPacket, error) {
return vision.SeedPacket{Variety: "X"}, nil
})
cookie := registerAndCookie(t, r, "[email protected]")
// No file field → 400.
if w := doMultipart(t, r, "/api/v1/seed-lots/scan", "multipart/form-data; boundary=x", bytes.NewBufferString(""), cookie); w.Code != http.StatusBadRequest {
t.Errorf("no image = %d, want 400", w.Code)
}
// A file that isn't an image → 400 (imagenorm rejects it).
var buf bytes.Buffer
mw := multipart.NewWriter(&buf)
part, _ := mw.CreateFormFile("image", "notes.txt")
part.Write([]byte("this is not an image"))
mw.Close()
if w := doMultipart(t, r, "/api/v1/seed-lots/scan", mw.FormDataContentType(), &buf, cookie); w.Code != http.StatusBadRequest {
t.Errorf("non-image = %d, want 400", w.Code)
}
// Vision NOT configured → 503, even with a valid image.
r2 := packetEngine(t, localCfg(), func() (vision.SeedPacket, error) { return vision.SeedPacket{}, nil })
cookie2 := registerAndCookie(t, r2, "[email protected]")
body, ct := pngUpload(t)
if w := doMultipart(t, r2, "/api/v1/seed-lots/scan", ct, body, cookie2); w.Code != http.StatusServiceUnavailable {
t.Errorf("no vision model = %d, want 503", w.Code)
}
// Unauthenticated → 401.
b3, ct3 := pngUpload(t)
if w := doMultipart(t, r, "/api/v1/seed-lots/scan", ct3, b3, nil); w.Code != http.StatusUnauthorized {
t.Errorf("anonymous scan = %d, want 401", w.Code)
}
}
// TestScanSeedPacketTooLargeAPI: a body over the multipart cap trips
// MaxBytesReader, which must surface as 413 (too large), not 400 (malformed).
func TestScanSeedPacketTooLargeAPI(t *testing.T) {
r := packetEngine(t, visionCfg(), func() (vision.SeedPacket, error) { return vision.SeedPacket{}, nil })
cookie := registerAndCookie(t, r, "[email protected]")
var buf bytes.Buffer
mw := multipart.NewWriter(&buf)
part, _ := mw.CreateFormFile("image", "big.png")
// A hair over scanUploadLimit (30 MiB) so MaxBytesReader trips during parsing.
part.Write(bytes.Repeat([]byte{0}, (30<<20)+1024))
mw.Close()
if w := doMultipart(t, r, "/api/v1/seed-lots/scan", mw.FormDataContentType(), &buf, cookie); w.Code != http.StatusRequestEntityTooLarge {
t.Errorf("oversized upload = %d, want 413", w.Code)
}
}
// TestCreateFromPacketAPI: confirm → plant + lot. No model involved, so the full
// path runs through the router.
func TestCreateFromPacketAPI(t *testing.T) {
r := packetEngine(t, visionCfg(), func() (vision.SeedPacket, error) { return vision.SeedPacket{}, nil })
cookie := registerAndCookie(t, r, "[email protected]")
// New plant + lot.
w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots/from-packet", map[string]any{
"newPlant": map[string]any{"name": "Music Garlic", "category": "vegetable", "color": "#4a7c3f", "icon": "🧄", "spacingCm": 15},
"lot": map[string]any{"vendor": "Johnny's", "quantity": 8, "unit": "bulbs"},
}, cookie)
if w.Code != http.StatusCreated {
t.Fatalf("new plant confirm: status %d, body %s", w.Code, w.Body.String())
}
res := decodeMap(t, w.Body.Bytes())
if res["plantIsNew"] != true {
t.Errorf("plantIsNew = %v, want true", res["plantIsNew"])
}
plantObj, _ := res["plant"].(map[string]any)
plantID := int64(plantObj["id"].(float64))
lotObj, _ := res["lot"].(map[string]any)
if int64(lotObj["plantId"].(float64)) != plantID {
t.Errorf("lot not attributed to the new plant")
}
// Existing plant + lot.
w = doJSON(t, r, http.MethodPost, "/api/v1/seed-lots/from-packet", map[string]any{
"plantId": plantID,
"lot": map[string]any{"vendor": "Fedco", "quantity": 10, "unit": "bulbs"},
}, cookie)
if w.Code != http.StatusCreated {
t.Fatalf("existing plant confirm: status %d, body %s", w.Code, w.Body.String())
}
if decodeMap(t, w.Body.Bytes())["plantIsNew"] != false {
t.Error("plantIsNew should be false for an existing plant")
}
// Both plantId and newPlant → 400.
if w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots/from-packet", map[string]any{
"plantId": plantID,
"newPlant": map[string]any{"name": "X", "category": "vegetable", "color": "#4a7c3f", "icon": "🌱"},
"lot": map[string]any{"vendor": "V", "quantity": 1, "unit": "seeds"},
}, cookie); w.Code != http.StatusBadRequest {
t.Errorf("both plant choices = %d, want 400", w.Code)
}
// Neither → 400.
if w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots/from-packet", map[string]any{
"lot": map[string]any{"vendor": "V", "quantity": 1, "unit": "seeds"},
}, cookie); w.Code != http.StatusBadRequest {
t.Errorf("neither plant choice = %d, want 400", w.Code)
}
// Unauthenticated → 401.
if w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots/from-packet", nil, nil); w.Code != http.StatusUnauthorized {
t.Errorf("anonymous confirm = %d, want 401", w.Code)
}
}
// TestCapabilitiesReportsVision: /capabilities advertises vision only when a
// vision model is configured.
func TestCapabilitiesReportsVision(t *testing.T) {
on := packetEngine(t, visionCfg(), func() (vision.SeedPacket, error) { return vision.SeedPacket{}, nil })
cookie := registerAndCookie(t, on, "[email protected]")
w := doJSON(t, on, http.MethodGet, "/api/v1/capabilities", nil, cookie)
if decodeMap(t, w.Body.Bytes())["vision"] != true {
t.Errorf("vision should be true with a model configured: %s", w.Body.String())
}
off := packetEngine(t, localCfg(), func() (vision.SeedPacket, error) { return vision.SeedPacket{}, nil })
cookie2 := registerAndCookie(t, off, "[email protected]")
w = doJSON(t, off, http.MethodGet, "/api/v1/capabilities", nil, cookie2)
if decodeMap(t, w.Body.Bytes())["vision"] != false {
t.Errorf("vision should be false with no model: %s", w.Body.String())
}
}
+17 -8
View File
@@ -51,26 +51,33 @@ type effectiveView struct {
// can be false even when Enabled+HasApiKey are true (an unresolvable model),
// which is exactly the case the UI needs to surface.
AgentLive bool `json:"agentLive"`
// VisionModel is the resolved seed-packet model (DB-over-env). VisionReady is
// whether capture can actually be offered (a key and a model).
VisionModel string `json:"visionModel"`
VisionReady bool `json:"visionReady"`
}
// settingsPayload builds the response, or an error. It does NOT swallow an
// EffectiveAgent failure into a misleading empty "effective" view — an empty
// EffectiveConfig failure into a misleading empty "effective" view — an empty
// view would report no model and no key, which reads as "nothing configured"
// rather than "we couldn't read it". Since EffectiveAgent re-reads the same row
// rather than "we couldn't read it". Since EffectiveConfig re-reads the same row
// GetInstanceSettings just returned, a failure here is a genuine DB fault worth
// surfacing as a 500, not papering over.
// surfacing as a 500, not papering over. It also resolves the agent and vision
// views from ONE row read rather than fetching the single-row table twice.
func (h *handlers) settingsPayload(c *gin.Context, st *domain.InstanceSettings) (settingsResponse, error) {
eff, err := h.svc.EffectiveAgent(c.Request.Context())
eff, vis, err := h.svc.EffectiveConfig(c.Request.Context())
if err != nil {
return settingsResponse{}, err
}
Review

GET/PATCH /settings now reads the single instance_settings row three times (GetInstanceSettings + EffectiveAgent + EffectiveVision)

performance · flagged by 1 model

🪰 Gadfly · advisory

⚪ **GET/PATCH /settings now reads the single instance_settings row three times (GetInstanceSettings + EffectiveAgent + EffectiveVision)** _performance · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
return settingsResponse{
Settings: st,
Effective: effectiveView{
Model: eff.Model,
Enabled: eff.Enabled,
HasApiKey: eff.APIKey != "",
AgentLive: h.agent.get() != nil,
Model: eff.Model,
Enabled: eff.Enabled,
HasApiKey: eff.APIKey != "",
AgentLive: h.agent.get() != nil,
VisionModel: vis.Model,
VisionReady: vis.Ready(),
},
}, nil
}
@@ -95,6 +102,7 @@ func (h *handlers) getSettings(c *gin.Context) {
type settingsUpdateRequest struct {
AgentModel string `json:"agentModel"`
AgentEnabled json.RawMessage `json:"agentEnabled"`
VisionModel string `json:"visionModel"`
Version int64 `json:"version" binding:"required"`
}
@@ -117,6 +125,7 @@ func (h *handlers) updateSettings(c *gin.Context) {
st, err := h.svc.UpdateInstanceSettings(c.Request.Context(), mustActor(c).ID, service.InstanceSettingsPatch{
AgentModel: req.AgentModel,
AgentEnabled: enabled,
VisionModel: req.VisionModel,
Version: req.Version,
})
if err != nil {
+8
View File
@@ -74,6 +74,11 @@ type AgentConfig struct {
// a key is present, so an instance with no key starts cleanly and simply
// doesn't offer the agent — the same shape as OIDC 404ing when unconfigured.
Enabled bool
// VisionModel is the model that reads a photographed seed packet
// (PANSY_VISION_MODEL). Empty by default: the seed-packet capture feature is
// only offered when a vision-capable model is configured (in env or Settings)
// and a key is present. Passed verbatim to majordomo.Parse, like Model.
VisionModel string
}
// Enabled reports whether enough OIDC config is present to attempt discovery.
@@ -120,6 +125,9 @@ func Load() *Config {
// opt-in, and making people set a second flag to use what they just
// configured is a papercut with no upside.
Enabled: envBool("PANSY_AGENT_ENABLED", agentKey != ""),
// No default vision model: unlike chat there's no obvious safe default,
// and the feature stays off until an admin names one that can see.
VisionModel: envStr("PANSY_VISION_MODEL", ""),
}
if cfg.Registration != RegistrationOpen && cfg.Registration != RegistrationClosed {
+7 -3
View File
@@ -253,9 +253,13 @@ type InstanceSettings struct {
// majordomo.Parse, exactly like the env var it shadows.
AgentModel string `json:"agentModel"`
// AgentEnabled overrides PANSY_AGENT_ENABLED when non-nil. nil = inherit.
AgentEnabled *bool `json:"agentEnabled"`
Version int64 `json:"version"`
UpdatedAt string `json:"updatedAt"`
AgentEnabled *bool `json:"agentEnabled"`
// VisionModel overrides PANSY_VISION_MODEL when non-empty; the model that
// reads a photographed seed packet (#81). Empty = feature off unless the env
// var names one.
VisionModel string `json:"visionModel"`
Version int64 `json:"version"`
UpdatedAt string `json:"updatedAt"`
}
// User is a pansy account. It may have a local password, OIDC identity, or both.
+63 -5
View File
@@ -43,6 +43,7 @@ func (s *Service) GetInstanceSettings(ctx context.Context, actorID int64) (*doma
type InstanceSettingsPatch struct {
AgentModel string
AgentEnabled *bool
VisionModel string
Version int64
}
@@ -58,16 +59,20 @@ func (s *Service) UpdateInstanceSettings(ctx context.Context, actorID int64, pat
return nil, err
}
model := strings.TrimSpace(patch.AgentModel)
// Validate a non-empty spec up front. An empty one is the "inherit env"
vision := strings.TrimSpace(patch.VisionModel)
// Validate non-empty specs up front. An empty one is the "inherit env"
// sentinel and needs no check — the env value was validated at boot.
if model != "" {
if err := agentmodel.Validate(s.cfg.Agent.OllamaCloudAPIKey, model); err != nil {
return nil, domain.ErrInvalidInput
for _, spec := range []string{model, vision} {
if spec != "" {
if err := agentmodel.Validate(s.cfg.Agent.OllamaCloudAPIKey, spec); err != nil {
return nil, domain.ErrInvalidInput
}
}
}
return s.store.UpdateInstanceSettings(ctx, &domain.InstanceSettings{
AgentModel: model,
AgentEnabled: patch.AgentEnabled,
VisionModel: vision,
Version: patch.Version,
})
}
@@ -95,6 +100,13 @@ func (s *Service) EffectiveAgent(ctx context.Context) (EffectiveAgent, error) {
if err != nil {
return EffectiveAgent{}, err
}
return s.agentOver(st), nil
}
// agentOver layers a settings row over the env-derived agent defaults. Split out
// so EffectiveConfig can resolve agent AND vision from a single row read instead
// of fetching the same one-row table twice.
func (s *Service) agentOver(st *domain.InstanceSettings) EffectiveAgent {
eff := EffectiveAgent{
Model: s.cfg.Agent.Model,
Enabled: s.cfg.Agent.Enabled,
@@ -106,5 +118,51 @@ func (s *Service) EffectiveAgent(ctx context.Context) (EffectiveAgent, error) {
if st.AgentEnabled != nil {
eff.Enabled = *st.AgentEnabled
}
return eff, nil
return eff
}
// EffectiveVision resolves the vision configuration in force for seed-packet
// capture (#81): the model from DB-over-env, the key always from env.
type EffectiveVision struct {
Model string
APIKey string
}
// Ready reports whether packet capture can be offered: a key and a vision model.
Review

🟡 EffectiveVision duplicates EffectiveAgent's structure and redundant settings-row fetch

maintainability · flagged by 1 model

  • internal/service/instance_settings.go:131-144EffectiveVision duplicates EffectiveAgent's structure and independently calls s.store.GetInstanceSettings(ctx). Its only caller, settingsPayload (internal/api/settings.go:66-85), already holds the row (passed in as st) and additionally calls EffectiveAgent — so GET/PATCH /settings now does three reads of a single-row table (the caller's fetch, one inside EffectiveAgent, one inside EffectiveVision). A comment on `settings…

🪰 Gadfly · advisory

🟡 **EffectiveVision duplicates EffectiveAgent's structure and redundant settings-row fetch** _maintainability · flagged by 1 model_ - **`internal/service/instance_settings.go:131-144`** — `EffectiveVision` duplicates `EffectiveAgent`'s structure and independently calls `s.store.GetInstanceSettings(ctx)`. Its only caller, `settingsPayload` (`internal/api/settings.go:66-85`), already holds the row (passed in as `st`) and additionally calls `EffectiveAgent` — so `GET/PATCH /settings` now does three reads of a single-row table (the caller's fetch, one inside `EffectiveAgent`, one inside `EffectiveVision`). A comment on `settings… <sub>🪰 Gadfly · advisory</sub>
// There's no separate enabled flag — configuring a vision model IS enabling it.
func (e EffectiveVision) Ready() bool {
return e.APIKey != "" && e.Model != ""
}
// EffectiveVision reads the settings row and layers it over the environment.
func (s *Service) EffectiveVision(ctx context.Context) (EffectiveVision, error) {
st, err := s.store.GetInstanceSettings(ctx)
if err != nil {
return EffectiveVision{}, err
}
return s.visionOver(st), nil
}
// visionOver layers a settings row over the env-derived vision defaults. See
// agentOver for why this is split from EffectiveVision.
func (s *Service) visionOver(st *domain.InstanceSettings) EffectiveVision {
eff := EffectiveVision{
Model: s.cfg.Agent.VisionModel,
APIKey: s.cfg.Agent.OllamaCloudAPIKey,
}
if st.VisionModel != "" {
eff.Model = st.VisionModel
}
return eff
}
// EffectiveConfig resolves the agent AND vision configuration from ONE settings
// read, for callers (the settings view) that need both — the single-row table
// would otherwise be fetched twice for one response.
func (s *Service) EffectiveConfig(ctx context.Context) (EffectiveAgent, EffectiveVision, error) {
st, err := s.store.GetInstanceSettings(ctx)
if err != nil {
return EffectiveAgent{}, EffectiveVision{}, err
}
return s.agentOver(st), s.visionOver(st), nil
}
+227
View File
@@ -0,0 +1,227 @@
package service
import (
"context"
"log/slog"
"sort"
"strings"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/vision"
)
// Seed-packet capture (#81): read a photographed packet, propose a plant + lot,
// let the user confirm. The two halves are deliberately separate operations:
// extraction only READS (a picture in, a proposal out — it can't touch the
// garden), and creation happens later, from the confirmed proposal, so a
// misread never writes anything on its own.
// PacketPlantMatch is a candidate existing plant the packet might be, with why it
// matched, so the UI can pre-select the likely one and let the user override.
type PacketPlantMatch struct {
Plant domain.Plant `json:"plant"`
// Reason is a short human tag: "exact name", "variety in name", "same species".
Reason string `json:"reason"`
}
// PacketProposal is what a scan returns: the fields read off the packet, plus
// candidate existing plants it might already be. Nothing is created yet.
type PacketProposal struct {
Packet vision.SeedPacket `json:"packet"`
// Candidates are existing plants the packet may match, best first. Empty means
// "probably a new variety" — the UI then offers to create one.
Candidates []PacketPlantMatch `json:"candidates"`
// SuggestedName is the variety (or species) to prefill a new-plant name with.
SuggestedName string `json:"suggestedName"`
// SuggestedCategory is the packet's category if it's a valid one, for prefill.
SuggestedCategory string `json:"suggestedCategory"`
}
// ExtractSeedPacket reads a (JPEG) packet photo and proposes a plant + lot for
// the actor to confirm. It needs a configured vision model; with none it returns
// ErrInvalidInput (the API layer turns "not configured" into a clear message and
// never offers the feature in the first place).
//
// The extraction runs as the actor only in the sense that the catalog match is
// scoped to what they can see; the model call itself has no ACL — it just reads
// a picture the actor uploaded.
func (s *Service) ExtractSeedPacket(ctx context.Context, actorID int64, jpeg []byte) (*PacketProposal, error) {
if len(jpeg) == 0 {
return nil, domain.ErrInvalidInput
}
vis, err := s.EffectiveVision(ctx)
if err != nil {
return nil, err
}
if !vis.Ready() {
// No vision model configured — the feature isn't available.
return nil, domain.ErrInvalidInput
}
packet, err := s.extractPacket(ctx, vis.APIKey, vis.Model, jpeg)
if err != nil {
return nil, err
}
plants, err := s.store.ListPlantsForActor(ctx, actorID)
if err != nil {
return nil, err
}
return &PacketProposal{
Packet: packet,
Candidates: matchPlants(packet, plants),
SuggestedName: suggestedName(packet),
SuggestedCategory: validCategory(packet.Category),
}, nil
}
// suggestedName is the variety if the packet named one, else the species — what
// to prefill a new plant's name with. "Music" beats "garlic" when both are read.
func suggestedName(p vision.SeedPacket) string {
if v := strings.TrimSpace(p.Variety); v != "" {
return v
}
return strings.TrimSpace(p.Species)
}
// validCategory returns the packet's category if it's one pansy knows, else "".
Review

🟠 validCategory duplicates the plantCategories enum map; two sources of truth for the same category set

maintainability · flagged by 2 models

  • internal/service/seed_packet.go:88-96validCategory re-lists the exact category enum that already lives in plantCategories (internal/service/plants.go:27-34). That map is the single source of truth the rest of the service uses (the comment on it says it "mirrors the schema CHECK constraint"), but this new helper hardcodes its own copy of the six values. A new category added to one place but not the other silently breaks the suggestion prefill. Suggested fix: `func validCategory(c str…

🪰 Gadfly · advisory

🟠 **validCategory duplicates the plantCategories enum map; two sources of truth for the same category set** _maintainability · flagged by 2 models_ - `internal/service/seed_packet.go:88-96` — `validCategory` re-lists the exact category enum that already lives in `plantCategories` (`internal/service/plants.go:27-34`). That map is the single source of truth the rest of the service uses (the comment on it says it "mirrors the schema CHECK constraint"), but this new helper hardcodes its own copy of the six values. A new category added to one place but not the other silently breaks the suggestion prefill. Suggested fix: `func validCategory(c str… <sub>🪰 Gadfly · advisory</sub>
// It reuses plantCategories — the same set CreatePlant validates against — so a
// new category can't be accepted by one path and rejected by the other.
func validCategory(c string) string {
if _, ok := plantCategories[c]; ok {
return c
}
return ""
}
// matchPlants ranks existing plants the packet might already be, best first.
//
// This is the crux of "create both, linked" (#81): getting it wrong makes a
// duplicate catalog entry that then splits a variety's seed-lot history across
// two rows. So it NEVER decides — it only surfaces candidates for the user to
// confirm. Matching is deliberately conservative and name-based (no fuzzy
// scoring that could confidently mis-rank): exact variety name, variety appearing
// within a plant's name, then same species word. Case-insensitive.
func matchPlants(p vision.SeedPacket, plants []domain.Plant) []PacketPlantMatch {
variety := strings.ToLower(strings.TrimSpace(p.Variety))
species := strings.ToLower(strings.TrimSpace(p.Species))
// rank: lower is better; keep only matched plants.
type scored struct {
match PacketPlantMatch
rank int
}
var out []scored
seen := map[int64]bool{}
add := func(pl domain.Plant, rank int, reason string) {
if seen[pl.ID] {
return
}
seen[pl.ID] = true
out = append(out, scored{PacketPlantMatch{Plant: pl, Reason: reason}, rank})
}
for _, pl := range plants {
name := strings.ToLower(pl.Name)
switch {
case variety != "" && name == variety:
add(pl, 0, "exact name")
case variety != "" && strings.Contains(name, variety):
add(pl, 1, "variety in name")
case species != "" && wordIn(name, species):
add(pl, 2, "same species")
}
}
sort.SliceStable(out, func(i, j int) bool { return out[i].rank < out[j].rank })
matches := make([]PacketPlantMatch, len(out))
for i, s := range out {
matches[i] = s.match
}
return matches
}
// wordIn reports whether word appears as a whole space-delimited token in name,
// so "garlic" matches "German Garlic" but not "garlicky-thing".
func wordIn(name, word string) bool {
for _, tok := range strings.Fields(name) {
if tok == word {
return true
}
}
return false
}
// PacketConfirm is a user-confirmed proposal to turn into rows.
//
// Plant selection is explicit: either PlantID names an existing plant to attach
// the lot to, or NewPlant carries the fields to create one. Exactly one — the
// service refuses both or neither, so an ambiguous confirm can't silently pick.
type PacketConfirm struct {
// PlantID attaches the lot to an existing plant. Set this XOR NewPlant.
PlantID *int64
// NewPlant creates a variety. Set this XOR PlantID.
NewPlant *PlantInput
// Lot is the purchase to record against whichever plant results.
Lot SeedLotInput
}
// PacketResult is what a confirm produced.
type PacketResult struct {
Plant *domain.Plant `json:"plant"`
Lot *domain.SeedLot `json:"lot"`
PlantIsNew bool `json:"plantIsNew"`
}
// CreateFromPacket turns a confirmed proposal into a plant (new or existing) plus
// a seed lot attributed to it. Unlike garden edits these rows aren't in the undo
// history — plants and lots are catalog/inventory, created directly — so there is
// no change set to wrap; the two creations just happen in sequence.
//
// If a new plant is created but the lot then fails, the plant is rolled back so
// the confirm is all-or-nothing. Otherwise a bad lot (say, an invalid unit) would
// strand a half-made catalog entry the user never asked for on its own, and the
// error return gives the HTTP caller no handle to it. A just-created plant has no
// plantings or lots yet, so the delete is safe; if it somehow can't be undone we
// log and still surface the original lot error, not the cleanup one.
func (s *Service) CreateFromPacket(ctx context.Context, actorID int64, in PacketConfirm) (*PacketResult, error) {
hasID, hasNew := in.PlantID != nil, in.NewPlant != nil
if hasID == hasNew {
return nil, domain.ErrInvalidInput // exactly one of existing / new
}
res := &PacketResult{}
if hasNew {
plant, err := s.CreatePlant(ctx, actorID, *in.NewPlant)
if err != nil {
return nil, err
}
res.Plant = plant
res.PlantIsNew = true
} else {
// Attach to an existing plant the actor can see. visiblePlant enforces the
Review

🟡 Existing-plant confirm path issues a redundant second GetPlant query (visiblePlant called again inside CreateSeedLot)

performance · flagged by 1 model

  • internal/service/seed_packet.go:203 + internal/service/seed_lots.go:153 — redundant visiblePlant lookup on the existing-plant confirm path. CreateFromPacket (existing-plant branch) calls s.visiblePlant(ctx, actorID, *in.PlantID) (one store.GetPlant), then calls s.CreateSeedLot, whose first line (seed_lots.go:153) calls s.visiblePlant again on the same PlantID — a second GetPlant for the row already loaded into res.Plant. A per-confirm redundant DB roundtrip on a wri…

🪰 Gadfly · advisory

🟡 **Existing-plant confirm path issues a redundant second GetPlant query (visiblePlant called again inside CreateSeedLot)** _performance · flagged by 1 model_ - **`internal/service/seed_packet.go:203` + `internal/service/seed_lots.go:153` — redundant `visiblePlant` lookup on the existing-plant confirm path.** `CreateFromPacket` (existing-plant branch) calls `s.visiblePlant(ctx, actorID, *in.PlantID)` (one `store.GetPlant`), then calls `s.CreateSeedLot`, whose first line (`seed_lots.go:153`) calls `s.visiblePlant` again on the same `PlantID` — a second `GetPlant` for the row already loaded into `res.Plant`. A per-confirm redundant DB roundtrip on a wri… <sub>🪰 Gadfly · advisory</sub>
// ACL (built-ins + their own); a plant they can't see is ErrNotFound.
plant, err := s.visiblePlant(ctx, actorID, *in.PlantID)
if err != nil {
return nil, err
}
res.Plant = plant
}
lotIn := in.Lot
lotIn.PlantID = res.Plant.ID
lot, err := s.CreateSeedLot(ctx, actorID, lotIn)
Review

🟡 CreateFromPacket returns nil result on lot failure even when a plant was created, discarding the plant ID the caller needs to retry the lot

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟡 **CreateFromPacket returns nil result on lot failure even when a plant was created, discarding the plant ID the caller needs to retry the lot** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
if err != nil {
if res.PlantIsNew {
// Roll back the plant we just made for this confirm; keep the lot error.
if delErr := s.DeletePlant(ctx, actorID, res.Plant.ID); delErr != nil {
slog.Error("service: could not roll back plant after packet lot failed",
"error", delErr, "plant", res.Plant.ID)
}
}
return nil, err
}
res.Lot = lot
return res, nil
}
+232
View File
@@ -0,0 +1,232 @@
package service
import (
"context"
"errors"
"testing"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/vision"
)
func packet(species, variety, category string) vision.SeedPacket {
return vision.SeedPacket{Species: species, Variety: variety, Category: category}
}
func plant(id int64, name string) domain.Plant {
return domain.Plant{ID: id, Name: name, Category: domain.CategoryVegetable}
}
// TestMatchPlants pins the catalog-matching heuristic: it surfaces candidates,
// best first, and never invents a match — the whole point, since a wrong auto-
// match would fragment a variety's seed-lot history across duplicate rows.
func TestMatchPlants(t *testing.T) {
catalog := []domain.Plant{
plant(1, "Garlic"),
plant(2, "Music Garlic"),
plant(3, "Cherokee Purple"),
plant(4, "Basil"),
}
t.Run("exact variety wins, ranked above looser matches", func(t *testing.T) {
got := matchPlants(packet("tomato", "Cherokee Purple", "vegetable"), catalog)
if len(got) == 0 || got[0].Plant.ID != 3 || got[0].Reason != "exact name" {
t.Fatalf("want Cherokee Purple exact first, got %+v", got)
}
})
t.Run("variety within a name, plus same-species, ordered", func(t *testing.T) {
// "Music" garlic: "Music Garlic" contains the variety (rank 1); "Garlic"
// shares the species word (rank 2).
got := matchPlants(packet("garlic", "Music", "vegetable"), catalog)
if len(got) != 2 {
t.Fatalf("got %d candidates, want 2: %+v", len(got), got)
}
if got[0].Plant.ID != 2 || got[0].Reason != "variety in name" {
t.Errorf("first = %+v, want Music Garlic / variety in name", got[0])
}
if got[1].Plant.ID != 1 || got[1].Reason != "same species" {
t.Errorf("second = %+v, want Garlic / same species", got[1])
}
})
t.Run("case-insensitive", func(t *testing.T) {
got := matchPlants(packet("", "cherokee purple", ""), catalog)
if len(got) == 0 || got[0].Plant.ID != 3 {
t.Errorf("case-insensitive exact match failed: %+v", got)
}
})
t.Run("no match → empty (a new variety)", func(t *testing.T) {
if got := matchPlants(packet("okra", "Clemson Spineless", "vegetable"), catalog); len(got) != 0 {
t.Errorf("want no candidates, got %+v", got)
}
})
t.Run("species word boundary, not substring", func(t *testing.T) {
// "garlic" should not match a hypothetical "garlicky" — wordIn is token-based.
got := matchPlants(packet("garlic", "", ""), []domain.Plant{plant(9, "Garlicky Mustard")})
if len(got) != 0 {
t.Errorf("substring shouldn't match on species: %+v", got)
}
})
}
// visionTestService builds a service with a configured vision model and a canned
// extractor, so ExtractSeedPacket can run with no live model.
func visionTestService(t *testing.T, out vision.SeedPacket, extractErr error) (*Service, int64) {
t.Helper()
cfg := openConfig()
cfg.Agent.OllamaCloudAPIKey = "k"
cfg.Agent.VisionModel = "ollama-cloud/vision:cloud"
s := newTestService(t, cfg)
s.extractPacket = func(ctx context.Context, apiKey, model string, jpeg []byte) (vision.SeedPacket, error) {
return out, extractErr
}
owner := seedUser(t, s, "[email protected]")
return s, owner
}
// TestExtractSeedPacket exercises the orchestration: canned packet → proposal
// with catalog candidates + prefill suggestions.
func TestExtractSeedPacket(t *testing.T) {
ctx := context.Background()
s, owner := visionTestService(t, packet("garlic", "Music", "vegetable"), nil)
// Seed a matching plant.
if _, err := s.CreatePlant(ctx, owner, PlantInput{
Name: "Music Garlic", Category: domain.CategoryVegetable, SpacingCM: 15, Color: "#4a7c3f", Icon: "🧄",
}); err != nil {
t.Fatalf("seed plant: %v", err)
}
prop, err := s.ExtractSeedPacket(ctx, owner, []byte("jpeg-bytes"))
if err != nil {
t.Fatalf("extract: %v", err)
}
if prop.Packet.Variety != "Music" {
t.Errorf("packet variety = %q", prop.Packet.Variety)
}
if len(prop.Candidates) == 0 || prop.Candidates[0].Plant.Name != "Music Garlic" {
t.Errorf("expected Music Garlic candidate, got %+v", prop.Candidates)
}
if prop.SuggestedName != "Music" || prop.SuggestedCategory != domain.CategoryVegetable {
t.Errorf("suggestions = %q/%q", prop.SuggestedName, prop.SuggestedCategory)
}
}
// TestExtractSeedPacketNeedsVisionModel: with no vision model configured, the
// feature is unavailable (ErrInvalidInput), and the extractor is never called.
func TestExtractSeedPacketNeedsVisionModel(t *testing.T) {
s := newTestService(t, openConfig()) // no vision model, no key
called := false
s.extractPacket = func(ctx context.Context, _, _ string, _ []byte) (vision.SeedPacket, error) {
called = true
return vision.SeedPacket{}, nil
}
owner := seedUser(t, s, "[email protected]")
if _, err := s.ExtractSeedPacket(context.Background(), owner, []byte("x")); !errors.Is(err, domain.ErrInvalidInput) {
t.Errorf("err = %v, want ErrInvalidInput", err)
}
if called {
t.Error("extractor was called despite no configured vision model")
}
}
// TestCreateFromPacketNewPlant: a confirm with NewPlant creates the plant and a
// lot attributed to it, in one call.
func TestCreateFromPacketNewPlant(t *testing.T) {
ctx := context.Background()
s, owner := visionTestService(t, vision.SeedPacket{}, nil)
res, err := s.CreateFromPacket(ctx, owner, PacketConfirm{
NewPlant: &PlantInput{Name: "Music Garlic", Category: domain.CategoryVegetable, SpacingCM: 15, Color: "#4a7c3f", Icon: "🧄"},
Lot: SeedLotInput{Vendor: "Johnny's", Quantity: 8, Unit: domain.UnitBulbs},
})
if err != nil {
t.Fatalf("create: %v", err)
}
if !res.PlantIsNew || res.Plant.Name != "Music Garlic" {
t.Errorf("plant = %+v, isNew=%v", res.Plant, res.PlantIsNew)
}
if res.Lot == nil || res.Lot.PlantID != res.Plant.ID {
t.Errorf("lot not attributed to the new plant: %+v", res.Lot)
}
}
// TestCreateFromPacketExistingPlant: a confirm with PlantID attaches the lot to
// the existing plant and creates nothing new.
func TestCreateFromPacketExistingPlant(t *testing.T) {
ctx := context.Background()
s, owner := visionTestService(t, vision.SeedPacket{}, nil)
existing, err := s.CreatePlant(ctx, owner, PlantInput{
Name: "Garlic", Category: domain.CategoryVegetable, SpacingCM: 15, Color: "#4a7c3f", Icon: "🧄",
})
if err != nil {
t.Fatalf("seed plant: %v", err)
}
res, err := s.CreateFromPacket(ctx, owner, PacketConfirm{
PlantID: &existing.ID,
Lot: SeedLotInput{Vendor: "Fedco", Quantity: 10, Unit: domain.UnitBulbs},
})
if err != nil {
t.Fatalf("create: %v", err)
}
if res.PlantIsNew || res.Plant.ID != existing.ID {
t.Errorf("should attach to existing plant, got %+v isNew=%v", res.Plant, res.PlantIsNew)
}
if res.Lot.PlantID != existing.ID {
t.Errorf("lot plantId = %d, want %d", res.Lot.PlantID, existing.ID)
}
}
// TestCreateFromPacketRollsBackNewPlantOnLotFailure: if the lot fails after a new
// plant was created for the confirm, the plant is rolled back so a bad lot can't
// strand a half-made catalog entry the user never asked for on its own.
func TestCreateFromPacketRollsBackNewPlantOnLotFailure(t *testing.T) {
ctx := context.Background()
s, owner := visionTestService(t, vision.SeedPacket{}, nil)
before, err := s.ListPlants(ctx, owner)
if err != nil {
t.Fatalf("list before: %v", err)
}
// A bogus unit makes CreateSeedLot fail AFTER the plant is created.
_, err = s.CreateFromPacket(ctx, owner, PacketConfirm{
NewPlant: &PlantInput{Name: "Rollback Garlic", Category: domain.CategoryVegetable, SpacingCM: 15, Color: "#4a7c3f", Icon: "🧄"},
Lot: SeedLotInput{Vendor: "Johnny's", Quantity: 8, Unit: "furlongs"},
})
if !errors.Is(err, domain.ErrInvalidInput) {
t.Fatalf("err = %v, want ErrInvalidInput from the bad unit", err)
}
after, err := s.ListPlants(ctx, owner)
if err != nil {
t.Fatalf("list after: %v", err)
}
if len(after) != len(before) {
t.Errorf("plant count %d → %d: the rolled-back plant was left behind", len(before), len(after))
}
for _, p := range after {
if p.Name == "Rollback Garlic" {
t.Errorf("plant %q survived a failed lot; rollback didn't fire", p.Name)
}
}
}
// TestCreateFromPacketExactlyOne: both or neither of PlantID/NewPlant is refused,
// so an ambiguous confirm can't silently pick.
func TestCreateFromPacketExactlyOne(t *testing.T) {
ctx := context.Background()
s, owner := visionTestService(t, vision.SeedPacket{}, nil)
id := int64(1)
for _, in := range []PacketConfirm{
{}, // neither
{PlantID: &id, NewPlant: &PlantInput{Name: "X"}}, // both
} {
if _, err := s.CreateFromPacket(ctx, owner, in); !errors.Is(err, domain.ErrInvalidInput) {
t.Errorf("CreateFromPacket(%+v) err = %v, want ErrInvalidInput", in, err)
}
}
}
+27 -6
View File
@@ -9,6 +9,7 @@
package service
import (
"context"
"crypto/rand"
"crypto/sha256"
"encoding/base64"
@@ -18,6 +19,7 @@ import (
"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/store"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/vision"
)
// timeLayout is the ISO-8601 UTC format used for every full timestamp pansy
@@ -41,16 +43,35 @@ type Service struct {
// produced by timingHash (fixed salt, no RNG) so it is always present — an
// empty one would silently re-open account enumeration.
dummyHash string
// extractPacket reads a photographed seed packet (#81). Injectable so tests
// can supply a canned packet instead of calling a live vision model — the
// same reason `now` is injectable. Defaults to vision.Extract.
extractPacket func(ctx context.Context, apiKey, modelSpec string, jpeg []byte) (vision.SeedPacket, error)
}
// Option customizes a Service at construction. The only current use is injecting
// a seed-packet extractor in tests so they don't call a live vision model.
type Option func(*Service)
// WithPacketExtractor overrides how a photographed seed packet is read (#81).
// Production uses vision.Extract; a test supplies a canned reader.
func WithPacketExtractor(fn func(ctx context.Context, apiKey, modelSpec string, jpeg []byte) (vision.SeedPacket, error)) Option {
return func(s *Service) { s.extractPacket = fn }
}
// New constructs a Service.
func New(st *store.DB, cfg *config.Config) *Service {
return &Service{
store: st,
cfg: cfg,
now: time.Now,
dummyHash: timingHash(),
func New(st *store.DB, cfg *config.Config, opts ...Option) *Service {
s := &Service{
store: st,
cfg: cfg,
now: time.Now,
dummyHash: timingHash(),
extractPacket: vision.Extract,
}
for _, o := range opts {
o(s)
}
return s
}
// formatTime renders a time as pansy's canonical UTC string.
+4 -4
View File
@@ -12,7 +12,7 @@ import (
// The instance_settings row is seeded by migration 0010 and there is exactly one
// (CHECK id = 1), so reads never branch on existence and writes never insert.
const instanceSettingsColumns = `agent_model, agent_enabled, version, updated_at`
const instanceSettingsColumns = `agent_model, agent_enabled, vision_model, version, updated_at`
// scanInstanceSettings reads the single settings row. agent_enabled is a nullable
// INTEGER (NULL = inherit env), so it is scanned through sql.NullInt64.
@@ -21,7 +21,7 @@ func scanInstanceSettings(s scanner) (*domain.InstanceSettings, error) {
out domain.InstanceSettings
enabled sql.NullInt64
)
if err := s.Scan(&out.AgentModel, &enabled, &out.Version, &out.UpdatedAt); err != nil {
if err := s.Scan(&out.AgentModel, &enabled, &out.VisionModel, &out.Version, &out.UpdatedAt); err != nil {
return nil, err
}
if enabled.Valid {
@@ -60,12 +60,12 @@ func (d *DB) UpdateInstanceSettings(ctx context.Context, s *domain.InstanceSetti
}
updated, err := scanInstanceSettings(d.sql.QueryRowContext(ctx,
`UPDATE instance_settings
SET agent_model = ?, agent_enabled = ?,
SET agent_model = ?, agent_enabled = ?, vision_model = ?,
version = version + 1,
updated_at = strftime('%Y-%m-%dT%H:%M:%SZ', 'now')
WHERE id = 1 AND version = ?
RETURNING `+instanceSettingsColumns,
s.AgentModel, enabled, s.Version,
s.AgentModel, enabled, s.VisionModel, s.Version,
))
if errors.Is(err, sql.ErrNoRows) {
current, gerr := d.GetInstanceSettings(ctx)
@@ -0,0 +1,10 @@
-- Vision model setting (#81): the model that reads a photographed seed packet.
--
-- Separate from agent_model because it's a different capability — extracting
-- structured fields from an image needs a vision-capable model, which the chat
-- model may not be. Same "inherit from env unless set" contract as agent_model:
-- '' falls back to PANSY_VISION_MODEL, then the feature is simply not offered.
--
-- Like agent_model, the API KEY is NOT stored here — the vision model runs
-- against the same OLLAMA_CLOUD_API_KEY from the environment.
ALTER TABLE instance_settings ADD COLUMN vision_model TEXT NOT NULL DEFAULT '';
+77
View File
@@ -0,0 +1,77 @@
// Package vision reads a photographed seed packet into structured fields (#81).
//
// It is one-shot structured extraction, NOT an agent loop: majordomo.Generate[T]
// derives a JSON schema from the SeedPacket struct, hands the image to a vision
// model, and unmarshals the reply into SeedPacket. Because it can't call a tool,
// it can't touch the garden — it only reads a picture and returns data, which the
// service then turns into a plant + lot after the user confirms.
package vision
import (
"context"
"fmt"
"gitea.stevedudenhoeffer.com/steve/majordomo"
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/agentmodel"
)
// SeedPacket is what a vision model reads off a packet. The json/description/enum
// tags drive the schema majordomo.Generate derives; pointer fields are nullable,
// so a field the packet doesn't print comes back nil rather than a made-up zero.
//
// These are the packet's PRINTED facts. Mapping them onto a pansy Plant + SeedLot
// (and deciding whether the variety is one already in the catalog) is the
// service's job, not the model's.
type SeedPacket struct {
Species string `json:"species" description:"the plant species in plain words, e.g. tomato, garlic, basil"`
Variety string `json:"variety" description:"the cultivar or variety name, e.g. Cherokee Purple; empty if the packet only names a species"`
Category string `json:"category" enum:"vegetable,herb,flower,fruit,tree_shrub,cover" description:"the single best-fit category"`
Vendor string `json:"vendor" description:"the seed company, e.g. Johnny's Selected Seeds"`
SKU string `json:"sku" description:"the vendor's item/product number, if printed"`
LotCode string `json:"lotCode" description:"the lot or batch code, if printed"`
PackedForYear *int `json:"packedForYear" description:"the 'packed for' or 'sell by' year, if printed"`
DaysToMaturity *int `json:"daysToMaturity" description:"days to maturity/harvest, if printed"`
SpacingCM *float64 `json:"spacingCm" description:"recommended in-row spacing in CENTIMETERS; convert if the packet uses inches"`
SeedCount *int `json:"seedCount" description:"approximate seed count in the packet, if printed"`
}
// extractPrompt tells the model the conventions it can't guess: centimeters, and
// that a missing field must be left empty rather than invented.
const extractPrompt = `You are reading a photograph of a seed packet. Extract only what is actually printed on it.
Rules:
- Spacing must be in CENTIMETERS. If the packet gives inches, convert (1 in = 2.54 cm).
- If a field is not printed on the packet, leave it empty or null. Do not guess or fill from general knowledge.
- "variety" is the cultivar name (e.g. "Cherokee Purple"); "species" is the plain plant name (e.g. "tomato").`
// Extract runs one vision extraction: it resolves the model spec against pansy's
// registry, sends the JPEG with the prompt, and returns the parsed SeedPacket.
// The image bytes should already be normalized to JPEG (see internal/imagenorm).
//
// It makes a live model call, so callers give it a bounded context.
func Extract(ctx context.Context, apiKey, modelSpec string, jpeg []byte) (SeedPacket, error) {
if len(jpeg) == 0 {
return SeedPacket{}, fmt.Errorf("vision: empty image")
}
model, err := agentmodel.Resolve(apiKey, modelSpec)
if err != nil {
return SeedPacket{}, err
}
return generate(ctx, model, jpeg)
}
// generate makes the actual one-shot call against an already-resolved model. It
// is split from Extract on purpose: the hermetic test drives THIS function with a
// fake model, so the prompt, the derived schema and the image part it builds are
// the real ones the live path uses — not a hand-copied double that could drift.
func generate(ctx context.Context, model llm.Model, jpeg []byte) (SeedPacket, error) {
return majordomo.Generate[SeedPacket](ctx, model, majordomo.Request{
Messages: []majordomo.Message{
majordomo.UserParts(
majordomo.Text(extractPrompt),
majordomo.Image("image/jpeg", jpeg),
),
},
})
}
+107
View File
@@ -0,0 +1,107 @@
package vision
import (
"bytes"
"context"
"image"
"image/jpeg"
"testing"
"gitea.stevedudenhoeffer.com/steve/majordomo"
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
"gitea.stevedudenhoeffer.com/steve/majordomo/provider/fake"
)
// tinyJPEG returns a real, sniffable JPEG. The chain runs media.Normalize before
// the provider, which checks the image's magic bytes, so a string literal won't
// do — the bytes must actually be a JPEG.
func tinyJPEG(t *testing.T) []byte {
t.Helper()
var b bytes.Buffer
if err := jpeg.Encode(&b, image.NewRGBA(image.Rect(0, 0, 8, 8)), nil); err != nil {
t.Fatalf("encode jpeg: %v", err)
}
return b.Bytes()
}
// TestExtractParsesModelJSON is the hermetic proof that the extraction path works
// end to end without a live model: a fake vision model returns canned packet JSON
// and Generate[SeedPacket] unmarshals it into the struct, image and schema
// included.
Review

🟠 extractVia is a hand-maintained copy of Extract's body; prompt/message drift won't be caught by the hermetic test

maintainability · flagged by 1 model

  • extractVia is a maintained-by-hand copy of Extract's bodyinternal/vision/vision_test.go:30-37 duplicates Extract (internal/vision/vision.go:52-67), and the comment admits it ("mirrors Extract's body exactly apart from where the model comes from"). The duplication exists because Extract resolves its model via agentmodel.Resolve while the test injects an llm.Model directly. The cost is real: if the prompt text or message construction in Extract changes, the hermetic test…

🪰 Gadfly · advisory

🟠 **extractVia is a hand-maintained copy of Extract's body; prompt/message drift won't be caught by the hermetic test** _maintainability · flagged by 1 model_ - **`extractVia` is a maintained-by-hand copy of `Extract`'s body** — `internal/vision/vision_test.go:30-37` duplicates `Extract` (`internal/vision/vision.go:52-67`), and the comment admits it ("mirrors Extract's body exactly apart from where the model comes from"). The duplication exists because `Extract` resolves its model via `agentmodel.Resolve` while the test injects an `llm.Model` directly. The cost is real: if the prompt text or message construction in `Extract` changes, the hermetic test… <sub>🪰 Gadfly · advisory</sub>
func TestExtractParsesModelJSON(t *testing.T) {
reg := majordomo.New(majordomo.WithoutEnvProviders())
fp := fake.New("fp") // default caps advertise structured output + images
reg.RegisterProvider(fp)
fp.Enqueue("vision", fake.Reply(`{
"species": "garlic",
Review

🟡 extractVia manually mirrors Extract's majordomo call, creating a maintenance trap

maintainability · flagged by 1 model

  • internal/vision/vision_test.go:36-43extractVia admits in its own comment that it mirrors Extract's body exactly. This is a maintenance trap: if the prompt, message layout, or context handling in Extract changes, the hermetic tests will exercise a divergent call shape and give false confidence. Better: extract a package-level helper that builds the majordomo.Request from jpeg bytes, shared by both Extract and extractVia.

🪰 Gadfly · advisory

🟡 **extractVia manually mirrors Extract's majordomo call, creating a maintenance trap** _maintainability · flagged by 1 model_ - `internal/vision/vision_test.go:36-43` — `extractVia` admits in its own comment that it mirrors `Extract`'s body exactly. This is a maintenance trap: if the prompt, message layout, or context handling in `Extract` changes, the hermetic tests will exercise a divergent call shape and give false confidence. Better: extract a package-level helper that builds the `majordomo.Request` from `jpeg` bytes, shared by both `Extract` and `extractVia`. <sub>🪰 Gadfly · advisory</sub>
"variety": "Music",
"category": "vegetable",
"vendor": "Johnny's",
"sku": "2761",
"lotCode": "L-42",
"packedForYear": 2026,
"daysToMaturity": 240,
"spacingCm": 15,
"seedCount": 8
}`))
m, err := reg.Parse("fp/vision")
if err != nil {
t.Fatalf("parse: %v", err)
}
got, err := generate(context.Background(), m, tinyJPEG(t))
if err != nil {
t.Fatalf("extract: %v", err)
}
if got.Variety != "Music" || got.Species != "garlic" || got.Category != "vegetable" {
t.Errorf("unexpected packet: %+v", got)
}
if got.SpacingCM == nil || *got.SpacingCM != 15 {
t.Errorf("spacingCm = %v, want 15", got.SpacingCM)
}
if got.PackedForYear == nil || *got.PackedForYear != 2026 {
t.Errorf("packedForYear = %v, want 2026", got.PackedForYear)
}
// The image and the derived schema really reached the model.
call := fp.Calls()[0]
if call.Request.SchemaName != "seedpacket" {
t.Errorf("schema name = %q, want seedpacket", call.Request.SchemaName)
}
var sawImage bool
for _, p := range call.Request.Messages[0].Parts {
if _, ok := p.(llm.ImagePart); ok {
sawImage = true
}
}
if !sawImage {
t.Error("the image part didn't reach the model")
}
}
// TestExtractLeavesMissingFieldsNil: a packet that only prints a species comes
// back with nil pointers for the numbers, not invented zeros — the whole reason
// the numeric fields are pointers.
func TestExtractLeavesMissingFieldsNil(t *testing.T) {
reg := majordomo.New(majordomo.WithoutEnvProviders())
fp := fake.New("fp")
reg.RegisterProvider(fp)
fp.Enqueue("vision", fake.Reply(`{"species":"basil","category":"herb"}`))
m, _ := reg.Parse("fp/vision")
got, err := generate(context.Background(), m, tinyJPEG(t))
if err != nil {
t.Fatalf("extract: %v", err)
}
if got.SpacingCM != nil || got.DaysToMaturity != nil || got.PackedForYear != nil || got.SeedCount != nil {
t.Errorf("missing numeric fields should be nil, got %+v", got)
}
}
// TestExtractRejectsEmptyImage: no bytes, no call.
func TestExtractRejectsEmptyImage(t *testing.T) {
if _, err := Extract(context.Background(), "k", "fp/vision", nil); err == nil {
t.Error("Extract accepted an empty image")
}
}