Seed-packet capture: vision model, extraction, catalog match, create (backend) #94
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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,14 @@ 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.
|
||||
|
|
||||
vision := false
|
||||
if vis, err := h.svc.EffectiveVision(c.Request.Context()); err == nil {
|
||||
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.
|
||||
|
||||
@@ -0,0 +1,136 @@
|
||||
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
|
||||
|
||||
// scanSeedPacket reads an uploaded packet photo and returns a proposal.
|
||||
func (h *handlers) scanSeedPacket(c *gin.Context) {
|
||||
// Extend the read deadline for the (potentially large, potentially slow)
|
||||
// upload. Best-effort: if the writer doesn't support it, the default applies.
|
||||
_ = http.NewResponseController(c.Writer).SetReadDeadline(time.Now().Add(scanReadTimeout))
|
||||
|
gitea-actions
commented
🔴 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>
|
||||
|
||||
c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, scanUploadLimit)
|
||||
file, err := c.FormFile("image")
|
||||
if err != nil {
|
||||
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "attach an image as the \"image\" field")
|
||||
return
|
||||
}
|
||||
f, err := file.Open()
|
||||
|
gitea-actions
commented
🟠 file.Open() server I/O error returned as 400 INVALID_INPUT error-handling · flagged by 1 model
🪰 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>
|
||||
if err != nil {
|
||||
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "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, format, err := imagenorm.Normalize(f, imagenorm.Options{})
|
||||
|
gitea-actions
commented
🟠 imagenorm I/O errors misclassified as 400 INVALID_INPUT error-handling · flagged by 1 model
🪰 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 err != nil {
|
||||
if errors.Is(err, imagenorm.ErrTooLarge) {
|
||||
writeAPIError(c, http.StatusRequestEntityTooLarge, "IMAGE_TOO_LARGE", "that image is too large — try a smaller photo")
|
||||
return
|
||||
}
|
||||
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "that doesn't look like an image we can read (JPEG, PNG, HEIC or WebP)")
|
||||
return
|
||||
}
|
||||
_ = format // available for logging if wanted
|
||||
|
gitea-actions
commented
⚪ format return value discarded speculatively ("available for logging if wanted") maintainability · flagged by 2 models
🪰 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>
|
||||
|
||||
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
|
||||
// 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)
|
||||
}
|
||||
|
||||
// packetLotRequest is the lot half of a confirm. It's seedLotCreateRequest
|
||||
// WITHOUT plantId — the plant comes from the confirm's plantId/newPlant choice,
|
||||
// not the lot body, and reusing seedLotCreateRequest would wrongly require one.
|
||||
type packetLotRequest struct {
|
||||
|
gitea-actions
commented
🟡 packetLotRequest and its toInput() duplicate seedLotCreateRequest field-for-field maintainability · flagged by 3 models
🪰 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>
|
||||
Vendor string `json:"vendor"`
|
||||
SourceURL string `json:"sourceUrl"`
|
||||
SKU string `json:"sku"`
|
||||
LotCode string `json:"lotCode"`
|
||||
PurchasedAt *string `json:"purchasedAt"`
|
||||
PackedForYear *int `json:"packedForYear"`
|
||||
|
gitea-actions
commented
🟡 packetLotRequest duplicates seedLotCreateRequest fields and toInput logic maintainability · flagged by 1 model
🪰 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>
|
||||
Quantity float64 `json:"quantity"`
|
||||
Unit string `json:"unit" binding:"required"`
|
||||
CostCents *int `json:"costCents"`
|
||||
GerminationPct *float64 `json:"germinationPct"`
|
||||
Notes string `json:"notes"`
|
||||
}
|
||||
|
||||
func (r packetLotRequest) toInput() service.SeedLotInput {
|
||||
return service.SeedLotInput{
|
||||
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,
|
||||
}
|
||||
}
|
||||
|
||||
// fromPacketRequest confirms a proposal: exactly one of plantId (attach to an
|
||||
// existing plant) or newPlant (create a variety), plus the lot to record.
|
||||
type fromPacketRequest struct {
|
||||
PlantID *int64 `json:"plantId"`
|
||||
NewPlant *plantCreateRequest `json:"newPlant"`
|
||||
Lot packetLotRequest `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)
|
||||
}
|
||||
@@ -0,0 +1,219 @@
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
// 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())
|
||||
}
|
||||
}
|
||||
@@ -51,6 +51,10 @@ 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
|
||||
@@ -64,13 +68,19 @@ func (h *handlers) settingsPayload(c *gin.Context, st *domain.InstanceSettings)
|
||||
if err != nil {
|
||||
return settingsResponse{}, err
|
||||
}
|
||||
vis, err := h.svc.EffectiveVision(c.Request.Context())
|
||||
|
gitea-actions
commented
⚪ 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>
|
||||
if err != nil {
|
||||
return settingsResponse{}, err
|
||||
}
|
||||
return settingsResponse{
|
||||
Settings: st,
|
||||
Effective: effectiveView{
|
||||
Model: eff.Model,
|
||||
Enabled: eff.Enabled,
|
||||
HasApiKey: eff.APIKey != "",
|
||||
AgentLive: h.agent.get() != nil,
|
||||
Model: eff.Model,
|
||||
Enabled: eff.Enabled,
|
||||
HasApiKey: eff.APIKey != "",
|
||||
AgentLive: h.agent.get() != nil,
|
||||
VisionModel: vis.Model,
|
||||
VisionReady: vis.Ready(),
|
||||
},
|
||||
}, nil
|
||||
}
|
||||
@@ -95,6 +105,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 +128,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 {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
})
|
||||
}
|
||||
@@ -108,3 +113,32 @@ func (s *Service) EffectiveAgent(ctx context.Context) (EffectiveAgent, error) {
|
||||
}
|
||||
return eff, nil
|
||||
}
|
||||
|
||||
// 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.
|
||||
// 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) {
|
||||
|
gitea-actions
commented
🟡 EffectiveVision duplicates EffectiveAgent's structure and redundant settings-row fetch maintainability · flagged by 1 model
🪰 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>
|
||||
st, err := s.store.GetInstanceSettings(ctx)
|
||||
if err != nil {
|
||||
return EffectiveVision{}, err
|
||||
}
|
||||
eff := EffectiveVision{
|
||||
Model: s.cfg.Agent.VisionModel,
|
||||
APIKey: s.cfg.Agent.OllamaCloudAPIKey,
|
||||
}
|
||||
if st.VisionModel != "" {
|
||||
eff.Model = st.VisionModel
|
||||
}
|
||||
return eff, nil
|
||||
}
|
||||
|
||||
@@ -0,0 +1,218 @@
|
||||
package service
|
||||
|
||||
import (
|
||||
"context"
|
||||
"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 "".
|
||||
func validCategory(c string) string {
|
||||
|
gitea-actions
commented
🟠 validCategory duplicates the plantCategories enum map; two sources of truth for the same category set maintainability · flagged by 2 models
🪰 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>
|
||||
switch c {
|
||||
case domain.CategoryVegetable, domain.CategoryHerb, domain.CategoryFlower,
|
||||
domain.CategoryFruit, domain.CategoryTreeShrub, domain.CategoryCover:
|
||||
return c
|
||||
default:
|
||||
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 left behind
|
||||
// rather than rolled back: a stray catalog entry is harmless and editable,
|
||||
// whereas silently discarding a variety the user just confirmed is worse. The
|
||||
// caller sees the error and can retry the lot against the now-existing plant.
|
||||
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
|
||||
// ACL (built-ins + their own); a plant they can't see is ErrNotFound.
|
||||
plant, err := s.visiblePlant(ctx, actorID, *in.PlantID)
|
||||
|
gitea-actions
commented
🟡 Existing-plant confirm path issues a redundant second GetPlant query (visiblePlant called again inside CreateSeedLot) performance · flagged by 1 model
🪰 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>
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
res.Plant = plant
|
||||
}
|
||||
|
||||
lotIn := in.Lot
|
||||
lotIn.PlantID = res.Plant.ID
|
||||
lot, err := s.CreateSeedLot(ctx, actorID, lotIn)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
|
gitea-actions
commented
🟡 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>
|
||||
}
|
||||
res.Lot = lot
|
||||
return res, nil
|
||||
}
|
||||
@@ -0,0 +1,197 @@
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
// 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)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
@@ -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 '';
|
||||
@@ -0,0 +1,68 @@
|
||||
// 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/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 majordomo.Generate[SeedPacket](ctx, model, majordomo.Request{
|
||||
Messages: []majordomo.Message{
|
||||
majordomo.UserParts(
|
||||
majordomo.Text(extractPrompt),
|
||||
majordomo.Image("image/jpeg", jpeg),
|
||||
),
|
||||
},
|
||||
})
|
||||
}
|
||||
@@ -0,0 +1,119 @@
|
||||
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()
|
||||
}
|
||||
|
||||
// extractVia is Extract with a caller-supplied registry, so the test can point it
|
||||
// at a fake provider instead of resolving a live model. It mirrors Extract's body
|
||||
// exactly apart from where the model comes from.
|
||||
func extractVia(t *testing.T, m llm.Model, jpeg []byte) (SeedPacket, error) {
|
||||
|
gitea-actions
commented
🟠 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
🪰 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>
|
||||
t.Helper()
|
||||
return majordomo.Generate[SeedPacket](context.Background(), m, majordomo.Request{
|
||||
Messages: []majordomo.Message{
|
||||
majordomo.UserParts(majordomo.Text(extractPrompt), majordomo.Image("image/jpeg", jpeg)),
|
||||
},
|
||||
})
|
||||
|
gitea-actions
commented
🟡 extractVia manually mirrors Extract's majordomo call, creating a maintenance trap maintainability · flagged by 1 model
🪰 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>
|
||||
}
|
||||
|
||||
// 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.
|
||||
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",
|
||||
"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 := extractVia(t, 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 := extractVia(t, 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")
|
||||
}
|
||||
}
|
||||
🔴 EffectiveVision DB error silently swallowed in capabilities endpoint
error-handling, performance · flagged by 5 models
internal/api/api.go:215-217—EffectiveVisionerror silently swallowed. Incapabilities, if the DB query fails (e.g., locked row, connection timeout), the error is discarded andvisionis reported asfalsewith 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