Address seed-packet review: 413 mapping, deadline, rollback, dedup

Gadfly findings on #94, the real ones:

- scanSeedPacket extends only the READ deadline; a slow upload + a live
  vision call runs past the server's absolute 30s WriteTimeout and the
  successful response is silently dropped (the #78 failure mode). Extend
  the write deadline too (scanWriteTimeout).
- An oversized upload tripping MaxBytesReader was mapped to 400; it's 413.
  Detect *http.MaxBytesError and report IMAGE_TOO_LARGE.
- Split imagenorm error mapping: ErrTooLarge->413, ErrUnsupported->400,
  genuine read/encode faults (and a failed file.Open)->500, not 400.
- CreateFromPacket discarded the plant it created when the lot then
  failed, contradicting its own doc. Roll the new plant back instead so
  the confirm is all-or-nothing (a fresh plant has no lots/plantings, so
  the delete is safe; log-and-continue on cleanup failure).
- Dedup: packetLotRequest and seedLotCreateRequest shared every lot
  field. Extract a seedLotFields base both use. validCategory now reuses
  plantCategories. EffectiveConfig resolves agent+vision from one
  settings-row read instead of two.
- capabilities swallowed an EffectiveVision error silently; log it.
- vision test hand-copied Extract's body (drift risk). Split generate()
  out of Extract so the hermetic test drives the real request builder.

Tests: rollback-on-lot-failure (service), oversized->413 (api).

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

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})

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

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")
+		}
 		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.

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, "[email protected]")
+
+	var buf bytes.Buffer
+	mw := multipart.NewWriter(&buf)
+	part, _ := mw.CreateFormFile("image", "big.png")
+	// A hair over scanUploadLimit (30 MiB) so MaxBytesReader trips during parsing.
+	part.Write(bytes.Repeat([]byte{0}, (30<<20)+1024))
+	mw.Close()
+
+	if w := doMultipart(t, r, "/api/v1/seed-lots/scan", mw.FormDataContentType(), &buf, cookie); w.Code != http.StatusRequestEntityTooLarge {
+		t.Errorf("oversized upload = %d, want 413", w.Code)
+	}
+}
+
 // TestCreateFromPacketAPI: confirm → plant + lot. No model involved, so the full
 // path runs through the router.
 func TestCreateFromPacketAPI(t *testing.T) {

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
 	}

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
 }

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

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) {

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(

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)
 	}