diff --git a/internal/api/api.go b/internal/api/api.go index ba75781..62e472d 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -212,7 +212,12 @@ func (h *handlers) capabilities(c *gin.Context) { // 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 { + 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}) diff --git a/internal/api/seed_lots.go b/internal/api/seed_lots.go index d70419d..d8b995b 100644 --- a/internal/api/seed_lots.go +++ b/internal/api/seed_lots.go @@ -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 diff --git a/internal/api/seed_packet.go b/internal/api/seed_packet.go index e31965e..cb519e3 100644 --- a/internal/api/seed_packet.go +++ b/internal/api/seed_packet.go @@ -29,21 +29,40 @@ const scanUploadLimit = 30 << 20 // 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. +const scanWriteTimeout = 120 * 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)) + // 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) + _ = 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 + 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 { - writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "could not read the uploaded image") + // 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() @@ -51,16 +70,19 @@ func (h *handlers) scanSeedPacket(c *gin.Context) { // 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{}) + jpeg, _, err := imagenorm.Normalize(f, imagenorm.Options{}) if err != nil { - if errors.Is(err, imagenorm.ErrTooLarge) { + switch { + case errors.Is(err, imagenorm.ErrTooLarge): writeAPIError(c, http.StatusRequestEntityTooLarge, "IMAGE_TOO_LARGE", "that image is too large — try a smaller photo") - return + 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") } - 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 prop, err := h.svc.ExtractSeedPacket(c.Request.Context(), mustActor(c).ID, jpeg) if err != nil { @@ -77,37 +99,14 @@ func (h *handlers) scanSeedPacket(c *gin.Context) { 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 { - Vendor string `json:"vendor"` - SourceURL string `json:"sourceUrl"` - SKU string `json:"sku"` - LotCode string `json:"lotCode"` - PurchasedAt *string `json:"purchasedAt"` - PackedForYear *int `json:"packedForYear"` - 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. +// 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 packetLotRequest `json:"lot"` + Lot seedLotFields `json:"lot"` } // createFromPacket turns a confirmed proposal into a plant + lot. diff --git a/internal/api/seed_packet_test.go b/internal/api/seed_packet_test.go index 18fdcbe..87d4255 100644 --- a/internal/api/seed_packet_test.go +++ b/internal/api/seed_packet_test.go @@ -141,6 +141,24 @@ func TestScanSeedPacketErrorsAPI(t *testing.T) { } } +// 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, "toobig@example.com") + + 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) { diff --git a/internal/api/settings.go b/internal/api/settings.go index 5e2bb65..a2cc6eb 100644 --- a/internal/api/settings.go +++ b/internal/api/settings.go @@ -58,17 +58,14 @@ type effectiveView struct { } // 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()) - if err != nil { - return settingsResponse{}, err - } - vis, err := h.svc.EffectiveVision(c.Request.Context()) + eff, vis, err := h.svc.EffectiveConfig(c.Request.Context()) if err != nil { return settingsResponse{}, err } diff --git a/internal/service/instance_settings.go b/internal/service/instance_settings.go index 814d3d4..6a121c5 100644 --- a/internal/service/instance_settings.go +++ b/internal/service/instance_settings.go @@ -100,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, @@ -111,7 +118,7 @@ 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 @@ -133,6 +140,12 @@ func (s *Service) EffectiveVision(ctx context.Context) (EffectiveVision, error) 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, @@ -140,5 +153,16 @@ func (s *Service) EffectiveVision(ctx context.Context) (EffectiveVision, error) if st.VisionModel != "" { eff.Model = st.VisionModel } - return eff, nil + 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 } diff --git a/internal/service/seed_packet.go b/internal/service/seed_packet.go index 8b3f1c4..796a1f8 100644 --- a/internal/service/seed_packet.go +++ b/internal/service/seed_packet.go @@ -2,6 +2,7 @@ package service import ( "context" + "log/slog" "sort" "strings" @@ -85,14 +86,13 @@ func suggestedName(p vision.SeedPacket) string { } // validCategory returns the packet's category if it's one pansy knows, else "". +// 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 { - switch c { - case domain.CategoryVegetable, domain.CategoryHerb, domain.CategoryFlower, - domain.CategoryFruit, domain.CategoryTreeShrub, domain.CategoryCover: + if _, ok := plantCategories[c]; ok { return c - default: - return "" } + return "" } // matchPlants ranks existing plants the packet might already be, best first. @@ -179,10 +179,12 @@ type PacketResult struct { // 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. +// 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 { @@ -211,6 +213,13 @@ func (s *Service) CreateFromPacket(ctx context.Context, actorID int64, in Packet lotIn.PlantID = res.Plant.ID lot, err := s.CreateSeedLot(ctx, actorID, lotIn) 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 diff --git a/internal/service/seed_packet_test.go b/internal/service/seed_packet_test.go index ee213c1..e33c3bd 100644 --- a/internal/service/seed_packet_test.go +++ b/internal/service/seed_packet_test.go @@ -180,6 +180,41 @@ func TestCreateFromPacketExistingPlant(t *testing.T) { } } +// 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) { diff --git a/internal/vision/vision.go b/internal/vision/vision.go index 6c11952..bb78bf9 100644 --- a/internal/vision/vision.go +++ b/internal/vision/vision.go @@ -12,6 +12,7 @@ import ( "fmt" "gitea.stevedudenhoeffer.com/steve/majordomo" + "gitea.stevedudenhoeffer.com/steve/majordomo/llm" "gitea.stevedudenhoeffer.com/steve/pansy/internal/agentmodel" ) @@ -57,6 +58,14 @@ func Extract(ctx context.Context, apiKey, modelSpec string, jpeg []byte) (SeedPa 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( diff --git a/internal/vision/vision_test.go b/internal/vision/vision_test.go index d414a57..d137d37 100644 --- a/internal/vision/vision_test.go +++ b/internal/vision/vision_test.go @@ -24,18 +24,6 @@ func tinyJPEG(t *testing.T) []byte { 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) { - t.Helper() - return majordomo.Generate[SeedPacket](context.Background(), m, majordomo.Request{ - Messages: []majordomo.Message{ - majordomo.UserParts(majordomo.Text(extractPrompt), majordomo.Image("image/jpeg", jpeg)), - }, - }) -} - // 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 @@ -62,7 +50,7 @@ func TestExtractParsesModelJSON(t *testing.T) { t.Fatalf("parse: %v", err) } - got, err := extractVia(t, m, tinyJPEG(t)) + got, err := generate(context.Background(), m, tinyJPEG(t)) if err != nil { t.Fatalf("extract: %v", err) } @@ -102,7 +90,7 @@ func TestExtractLeavesMissingFieldsNil(t *testing.T) { fp.Enqueue("vision", fake.Reply(`{"species":"basil","category":"herb"}`)) m, _ := reg.Parse("fp/vision") - got, err := extractVia(t, m, tinyJPEG(t)) + got, err := generate(context.Background(), m, tinyJPEG(t)) if err != nil { t.Fatalf("extract: %v", err) }