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

Merged
steve merged 2 commits from feat/seed-packet-backend into main 2026-07-22 04:22:27 +00:00
Owner

Part of #81, part of #86. Photograph a seed packet → it fills in the plant and the purchase. This is the backend; the scan UI is a follow-up PR.

Vision model config (mirrors the #79 agent model)

  • Migration 0011 adds instance_settings.vision_model; PANSY_VISION_MODEL is the env default. Precedence Settings → env → empty; the key stays in the env.
  • EffectiveVision resolves it; /capabilities advertises "vision" only when a model + key are configured, so the UI offers the scan button only when it works.

Extraction is one-shot, NOT an agent loop (internal/vision)

majordomo.Generate[SeedPacket] derives a JSON schema from the struct tags and hands the image to the vision model. It can't call a tool, so it can't touch the garden — it only reads a picture and returns data. Numeric fields are pointers, so a field the packet doesn't print comes back nil, not an invented 0.

Hermetic test (per the majordomo fake provider pattern): a fake returns canned packet JSON and Generate unmarshals it, image + derived schema included. No live model.

Image normalization (from #80)

The image is normalized to JPEG at the upload boundary — where an iPhone HEIC becomes readable, since majordomo's stdlib media path can't decode it. imagenorm now links into the binary (~7 MB, the cost #80 deferred; the static build stays CGO-free).

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:

  • matchPlants surfaces ranked candidates (exact name → variety-in-name → same-species; conservative, name-based, no fuzzy scoring that could confidently mis-rank).
  • The user confirms; CreateFromPacket makes the plant (new or existing) + the lot. Exactly one of plantId/newPlant, refused otherwise.
  • Plants/lots aren't in the undo history (catalog/inventory), so there's no change set to wrap.
  • The extractor is injectable (service.WithPacketExtractor), so ExtractSeedPacket and the /scan endpoint test end-to-end against a fake.

Endpoints

  • POST /seed-lots/scan — multipart image → proposal (reads only). Extends the read deadline for a slow phone upload, caps the body, maps too-large/unreadable to clear statuses (413 / 400), and 503s when no vision model is configured.
  • POST /seed-lots/from-packet — confirmed proposal → 201 with the plant + lot.

Tests

  • vision: Generate[SeedPacket] parses model JSON; missing fields stay nil; empty image rejected.
  • service: matchPlants (exact/variety-in-name/species/word-boundary/case), ExtractSeedPacket (proposal + candidates + suggestions; no-vision → refused, extractor not called), CreateFromPacket (new / existing / exactly-one).
  • api: scan happy-path (multipart → proposal, real PNG through imagenorm), scan errors (no image / non-image / no-vision-503 / anonymous-401), from-packet (new / existing / both / neither / anonymous), capabilities reports vision on/off.

Verified live

Migration applies; GET /settings shows visionModel; /capabilities reports vision:false unconfigured; /scan rejects a non-image. Binary 40 MB (imagenorm linked), CGO_ENABLED=0.

Follow-up

The scan UI (upload/camera → editable proposal form with the candidate dropdown → confirm). Also noted for whoever picks it up: EXIF orientation (deferred from #80) matters most here — a sideways packet photo — and is best handled with a real oriented photo end-to-end.

Docs: README (PANSY_VISION_MODEL), DESIGN (routes + the decision, and why the model can't touch the garden).

GOWORK=off go test ./... green; gofmt -l internal/ clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

Part of #81, part of #86. Photograph a seed packet → it fills in the plant and the purchase. **This is the backend; the scan UI is a follow-up PR.** ## Vision model config (mirrors the #79 agent model) - Migration 0011 adds `instance_settings.vision_model`; `PANSY_VISION_MODEL` is the env default. Precedence Settings → env → empty; the **key stays in the env**. - `EffectiveVision` resolves it; `/capabilities` advertises `"vision"` only when a model + key are configured, so the UI offers the scan button only when it works. ## Extraction is one-shot, NOT an agent loop (`internal/vision`) `majordomo.Generate[SeedPacket]` derives a JSON schema from the struct tags and hands the image to the vision model. It **can't call a tool, so it can't touch the garden** — it only reads a picture and returns data. Numeric fields are pointers, so a field the packet doesn't print comes back `nil`, not an invented `0`. Hermetic test (per the majordomo `fake` provider pattern): a fake returns canned packet JSON and `Generate` unmarshals it, image + derived schema included. **No live model.** ## Image normalization (from #80) The image is normalized to JPEG at the upload boundary — where an iPhone **HEIC** becomes readable, since majordomo's stdlib media path can't decode it. `imagenorm` now links into the binary (~7 MB, the cost #80 deferred; the static build stays CGO-free). ## 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**: - `matchPlants` surfaces **ranked candidates** (exact name → variety-in-name → same-species; conservative, name-based, no fuzzy scoring that could confidently mis-rank). - The user confirms; `CreateFromPacket` makes the plant (new **or** existing) + the lot. Exactly one of `plantId`/`newPlant`, refused otherwise. - Plants/lots aren't in the undo history (catalog/inventory), so there's no change set to wrap. - The extractor is injectable (`service.WithPacketExtractor`), so `ExtractSeedPacket` and the `/scan` endpoint test end-to-end against a fake. ## Endpoints - `POST /seed-lots/scan` — multipart image → proposal (**reads only**). Extends the read deadline for a slow phone upload, caps the body, maps too-large/unreadable to clear statuses (413 / 400), and 503s when no vision model is configured. - `POST /seed-lots/from-packet` — confirmed proposal → 201 with the plant + lot. ## Tests - **vision**: `Generate[SeedPacket]` parses model JSON; missing fields stay nil; empty image rejected. - **service**: `matchPlants` (exact/variety-in-name/species/word-boundary/case), `ExtractSeedPacket` (proposal + candidates + suggestions; no-vision → refused, extractor not called), `CreateFromPacket` (new / existing / exactly-one). - **api**: scan happy-path (multipart → proposal, real PNG through imagenorm), scan errors (no image / non-image / no-vision-503 / anonymous-401), from-packet (new / existing / both / neither / anonymous), capabilities reports vision on/off. ## Verified live Migration applies; `GET /settings` shows `visionModel`; `/capabilities` reports `vision:false` unconfigured; `/scan` rejects a non-image. Binary 40 MB (imagenorm linked), `CGO_ENABLED=0`. ## Follow-up The scan **UI** (upload/camera → editable proposal form with the candidate dropdown → confirm). Also noted for whoever picks it up: **EXIF orientation** (deferred from #80) matters most here — a sideways packet photo — and is best handled with a real oriented photo end-to-end. Docs: README (`PANSY_VISION_MODEL`), DESIGN (routes + the decision, and why the model can't touch the garden). `GOWORK=off go test ./...` green; `gofmt -l internal/` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve added 1 commit 2026-07-22 03:51:24 +00:00
Seed-packet capture: vision model, extraction, catalog match, create (backend) (#81)
Build image / build-and-push (push) Successful in 7s
Gadfly review (reusable) / review (pull_request) Successful in 9m44s
Adversarial Review (Gadfly) / review (pull_request) Successful in 9m44s
156b3fd14c
Photograph a seed packet → it fills in the plant and the purchase. This is the
backend; the scan UI is a follow-up PR.

Vision model config (mirrors the agent model from #79):
- Migration 0011 adds instance_settings.vision_model; PANSY_VISION_MODEL is the
  env default. Precedence Settings → env → empty; the KEY stays in the env.
- EffectiveVision resolves it; /capabilities advertises "vision" only when a
  model + key are configured, so the UI offers the scan button only when it works.

Extraction is one-shot, NOT an agent loop (internal/vision):
- majordomo.Generate[SeedPacket] derives a JSON schema from the struct tags and
  hands the image to the vision model; it can't call a tool, so it can't touch
  the garden — it only reads a picture and returns data. Numeric fields are
  pointers, so a field the packet doesn't print comes back nil, not a made-up 0.
- Hermetic test: majordomo's fake provider returns canned packet JSON and
  Generate unmarshals it, image + derived schema included. No live model.

The image is normalized to JPEG at the upload boundary (imagenorm from #80),
which is where an iPhone HEIC becomes readable — majordomo's media path can't
decode HEIC. imagenorm now links into the binary (~7 MB, the cost #80 deferred).

The hard part is catalog matching, not OCR (internal/service/seed_packet.go):
- A wrong auto-match splits a variety's seed-lot history across duplicate rows,
  so the service NEVER auto-creates. matchPlants surfaces RANKED candidates
  (exact name → variety-in-name → same species, conservative and name-based),
  the user confirms, and CreateFromPacket makes the plant (new or existing) + the
  lot. Exactly one of plantId/newPlant, refused otherwise.
- Plants/lots aren't in the undo history (catalog/inventory), so no change set.
- The extractor is injectable (service.WithPacketExtractor) so ExtractSeedPacket
  and the /scan endpoint test end to end against a fake, no live model.

Endpoints: POST /seed-lots/scan (multipart image → proposal, reads only; extends
the read deadline for a slow phone upload, caps the body, maps too-large/unreadable
to clear statuses) and POST /seed-lots/from-packet (confirmed proposal → 201).

Docs: README (PANSY_VISION_MODEL), DESIGN (routes + the decision and why the
model can't touch the garden).

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

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-22 04:01:08Z

claude-code/sonnet · claude-code — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Blocking issues found

glm-5.2:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — Minor issues

kimi-k2.6:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • ⚠️ performance — could not complete
  • error-handling — Minor issues

opencode/glm-5.2:cloud · opencode — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — Minor issues

opencode/kimi-k2.6:cloud · opencode — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — No material issues found
  • performance — No material issues found
  • error-handling — Blocking issues found

Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

<!-- gadfly-status-board --> ## 🪰 Gadfly — live review status 5/5 reviewers finished · updated 2026-07-22 04:01:08Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Blocking issues found #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ⚠️ **performance** — could not complete - ✅ **error-handling** — Minor issues #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Blocking issues found <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-22 04:01:08 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 14 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 14 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -207,1 +211,3 @@
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.

🔴 EffectiveVision DB error silently swallowed in capabilities endpoint

error-handling, performance · flagged by 5 models

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

🪰 Gadfly · advisory

🔴 **EffectiveVision DB error silently swallowed in capabilities endpoint** _error-handling, performance · flagged by 5 models_ - **`internal/api/api.go:215-217`** — `EffectiveVision` error silently swallowed. In `capabilities`, if the DB query fails (e.g., locked row, connection timeout), the error is discarded and `vision` is reported as `false` with a 200 OK. The UI hides the scan button and the admin has no signal that the server is in distress. The error should at minimum be logged; returning the error to the client (500) or defaulting to the env value with a log line are both better than silence. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +33,4 @@
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))

🔴 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>
@@ -0,0 +41,4 @@
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "attach an image as the \"image\" field")
return
}
f, err := file.Open()

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

error-handling · flagged by 1 model

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

🪰 Gadfly · advisory

🟠 **file.Open() server I/O error returned as 400 INVALID_INPUT** _error-handling · flagged by 1 model_ - **`internal/api/seed_packet.go:44-47`** — `file.Open()` error misclassified as client error. After `c.FormFile("image")` succeeds, `file.Open()` can fail for server-side reasons (temp file removed, disk I/O error). The handler returns 400/`INVALID_INPUT` ("could not read the uploaded image"), but this is a server failure and should be 500. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +51,4 @@
// 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{})

🟠 imagenorm I/O errors misclassified as 400 INVALID_INPUT

error-handling · flagged by 1 model

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

🪰 Gadfly · advisory

🟠 **imagenorm I/O errors misclassified as 400 INVALID_INPUT** _error-handling · flagged by 1 model_ - **`internal/api/seed_packet.go:54-62`** — Non-image errors from `imagenorm.Normalize` misclassified as client errors. `Normalize` returns wrapped I/O errors (e.g., read failure during decode, JPEG encode failure) that are neither `ErrTooLarge` nor `ErrUnsupported`. These are server-side failures, but the handler falls through to 400/`INVALID_INPUT` ("that doesn't look like an image we can read"). It should branch on `errors.Is(err, imagenorm.ErrUnsupported)` → 400, and any other error → 500. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +60,4 @@
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

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

maintainability · flagged by 2 models

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

🪰 Gadfly · advisory

⚪ **format return value discarded speculatively ("available for logging if wanted")** _maintainability · flagged by 2 models_ - **`internal/api/seed_packet.go:63`** — `_ = format // available for logging if wanted` discards a value kept only on the chance it's useful later. It's the only instance of this pattern anywhere in `internal/` — not an established codebase convention. Either drop the named return (`jpeg, _, err := imagenorm.Normalize(...)`) or actually use it. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +80,4 @@
// 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 {

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

maintainability · flagged by 3 models

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

🪰 Gadfly · advisory

🟡 **packetLotRequest and its toInput() duplicate seedLotCreateRequest field-for-field** _maintainability · flagged by 3 models_ - **`internal/api/seed_packet.go:83-98` vs `internal/api/seed_lots.go:20-42`** — `packetLotRequest` and its `toInput()` reproduce all 11 fields and the field-by-field mapping of `seedLotCreateRequest`/its `toInput()` verbatim, differing only in the absence of `PlantID`. The comment explains why a separate type exists (reusing `seedLotCreateRequest` would wrongly require `plantId`), a fair constraint, but the duplication itself is avoidable — e.g. an embedded `lotFields` struct that both request… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +86,4 @@
SKU string `json:"sku"`
LotCode string `json:"lotCode"`
PurchasedAt *string `json:"purchasedAt"`
PackedForYear *int `json:"packedForYear"`

🟡 packetLotRequest duplicates seedLotCreateRequest fields and toInput logic

maintainability · flagged by 1 model

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

🪰 Gadfly · advisory

🟡 **packetLotRequest duplicates seedLotCreateRequest fields and toInput logic** _maintainability · flagged by 1 model_ - `internal/api/seed_packet.go:89-109` — `packetLotRequest` duplicates `seedLotCreateRequest` field-for-field (minus `PlantID`) and duplicates its `toInput()` body almost verbatim. If a field is added to the lot later (e.g. a new nullable column), `packetLotRequest` will almost certainly drift and the `from-packet` path will silently drop it. A shared embedded struct or at least a shared helper would keep the two in sync. <sub>🪰 Gadfly · advisory</sub>
@@ -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())

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>
@@ -111,0 +128,4 @@
}
// EffectiveVision reads the settings row and layers it over the environment.
func (s *Service) EffectiveVision(ctx context.Context) (EffectiveVision, error) {

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

maintainability · flagged by 1 model

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

🪰 Gadfly · advisory

🟡 **EffectiveVision duplicates EffectiveAgent's structure and redundant settings-row fetch** _maintainability · flagged by 1 model_ - **`internal/service/instance_settings.go:131-144`** — `EffectiveVision` duplicates `EffectiveAgent`'s structure and independently calls `s.store.GetInstanceSettings(ctx)`. Its only caller, `settingsPayload` (`internal/api/settings.go:66-85`), already holds the row (passed in as `st`) and additionally calls `EffectiveAgent` — so `GET/PATCH /settings` now does three reads of a single-row table (the caller's fetch, one inside `EffectiveAgent`, one inside `EffectiveVision`). A comment on `settings… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +85,4 @@
}
// validCategory returns the packet's category if it's one pansy knows, else "".
func validCategory(c string) string {

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

maintainability · flagged by 2 models

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

🪰 Gadfly · advisory

🟠 **validCategory duplicates the plantCategories enum map; two sources of truth for the same category set** _maintainability · flagged by 2 models_ - `internal/service/seed_packet.go:88-96` — `validCategory` re-lists the exact category enum that already lives in `plantCategories` (`internal/service/plants.go:27-34`). That map is the single source of truth the rest of the service uses (the comment on it says it "mirrors the schema CHECK constraint"), but this new helper hardcodes its own copy of the six values. A new category added to one place but not the other silently breaks the suggestion prefill. Suggested fix: `func validCategory(c str… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +200,4 @@
} 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)

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

performance · flagged by 1 model

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

🪰 Gadfly · advisory

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

🟡 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>
@@ -0,0 +27,4 @@
// 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) {

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

maintainability · flagged by 1 model

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

🪰 Gadfly · advisory

🟠 **extractVia is a hand-maintained copy of Extract's body; prompt/message drift won't be caught by the hermetic test** _maintainability · flagged by 1 model_ - **`extractVia` is a maintained-by-hand copy of `Extract`'s body** — `internal/vision/vision_test.go:30-37` duplicates `Extract` (`internal/vision/vision.go:52-67`), and the comment admits it ("mirrors Extract's body exactly apart from where the model comes from"). The duplication exists because `Extract` resolves its model via `agentmodel.Resolve` while the test injects an `llm.Model` directly. The cost is real: if the prompt text or message construction in `Extract` changes, the hermetic test… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +33,4 @@
Messages: []majordomo.Message{
majordomo.UserParts(majordomo.Text(extractPrompt), majordomo.Image("image/jpeg", jpeg)),
},
})

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

maintainability · flagged by 1 model

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

🪰 Gadfly · advisory

🟡 **extractVia manually mirrors Extract's majordomo call, creating a maintenance trap** _maintainability · flagged by 1 model_ - `internal/vision/vision_test.go:36-43` — `extractVia` admits in its own comment that it mirrors `Extract`'s body exactly. This is a maintenance trap: if the prompt, message layout, or context handling in `Extract` changes, the hermetic tests will exercise a divergent call shape and give false confidence. Better: extract a package-level helper that builds the `majordomo.Request` from `jpeg` bytes, shared by both `Extract` and `extractVia`. <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: Blocking issues found · 15 findings (5 with multi-model agreement)

Finding Where Models Lens
🔴 EffectiveVision DB error silently swallowed in capabilities endpoint internal/api/api.go:213 5/5 error-handling, performance
🔴 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 internal/api/seed_packet.go:36 3/5 error-handling
🟡 packetLotRequest and its toInput() duplicate seedLotCreateRequest field-for-field internal/api/seed_packet.go:83 3/5 maintainability
🟠 validCategory duplicates the plantCategories enum map; two sources of truth for the same category set internal/service/seed_packet.go:88 2/5 maintainability
format return value discarded speculatively ("available for logging if wanted") internal/api/seed_packet.go:63 2/5 maintainability
10 single-model findings (lower confidence)
Finding Where Model Lens
🟠 file.Open() server I/O error returned as 400 INVALID_INPUT internal/api/seed_packet.go:44 opencode/kimi-k2.6:cloud error-handling
🟠 imagenorm I/O errors misclassified as 400 INVALID_INPUT internal/api/seed_packet.go:54 opencode/kimi-k2.6:cloud error-handling
🟠 extractVia is a hand-maintained copy of Extract's body; prompt/message drift won't be caught by the hermetic test internal/vision/vision_test.go:30 opencode/glm-5.2:cloud maintainability
🟡 packetLotRequest duplicates seedLotCreateRequest fields and toInput logic internal/api/seed_packet.go:89 kimi-k2.6:cloud maintainability
🟡 settingsPayload reads the same instance_settings row twice (EffectiveAgent + EffectiveVision) internal/api/settings.go:67 glm-5.2:cloud performance
🟡 EffectiveVision duplicates EffectiveAgent's structure and redundant settings-row fetch internal/service/instance_settings.go:131 claude-code/sonnet maintainability
🟡 Existing-plant confirm path issues a redundant second GetPlant query (visiblePlant called again inside CreateSeedLot) internal/service/seed_packet.go:203 opencode/glm-5.2:cloud performance
🟡 CreateFromPacket returns nil result on lot failure even when a plant was created, discarding the plant ID the caller needs to retry the lot internal/service/seed_packet.go:214 opencode/glm-5.2:cloud error-handling
🟡 extractVia manually mirrors Extract's majordomo call, creating a maintenance trap internal/vision/vision_test.go:36 kimi-k2.6:cloud maintainability
GET/PATCH /settings now reads the single instance_settings row three times (GetInstanceSettings + EffectiveAgent + EffectiveVision) internal/api/settings.go:71 opencode/glm-5.2:cloud performance
Per-model detail
claude-code/sonnet (claude-code) — Blocking issues found

Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

VERDICT: No material issues found

I checked this PR specifically for authn/authz gaps, injection, SSRF, unsafe deserialization, secret handling, and unsafe handling of untrusted (uploaded) data, verifying against the actual checked-out code, not just the diff:

  • Auth on new routes: seedLots.POST("/scan", ...) and .../from-packet sit inside v1.Group("/seed-lots", h.requireAuth()) (internal/api/api.go:182-191), and CSRF protection (h.csrfGuard()) is applied at the v1 group level (internal/api/api.go:57), so both new endpoints inherit it. The PR's own tests confirm anonymous → 401 for both endpoints, and I confirmed requireAuth/csrfGuard wrap the whole /api/v1 tree.
  • Catalog-match scoping: ExtractSeedPacketmatchPlants uses store.ListPlantsForActor (internal/store/plants.go:80-105), which is correctly scoped to built-ins (owner_id IS NULL) plus the caller's own rows — no cross-user plant/candidate leakage.
  • Plant attach authorization: CreateFromPacket's existing-plant path goes through s.visiblePlant (internal/service/plantings.go:217-229), which rejects another user's plant as ErrInvalidInput (masked, not leaked), and CreateSeedLot independently re-checks visiblePlant too (internal/service/seed_lots.go:152-155) — defense in depth, no bypass.
  • Secrets: EffectiveVision sources APIKey only from s.cfg.Agent.OllamaCloudAPIKey (env), never from the DB row (internal/service/instance_settings.go), and the settings payload only exposes VisionModel/VisionReady, never the key — consistent with the existing agent-key invariant.
  • Untrusted upload handling: scanSeedPacket sets http.MaxBytesReader before c.FormFile, and hands the bytes to imagenorm.Normalize, which (pre-existing, unchanged here) bounds input bytes, checks decoded dimensions/pixel count before allocating a bitmap (decompression-bomb guard), and recovers from decoder panics. The uploaded filename is never used for storage/paths, ruling out path traversal.
  • Model-spec validation isn't SSRF-prone: agentmodel.Validate/Resolve only parses against a registry with a single hardcoded provider (ollama-cloud); it never makes a live network call, so a spec (including the new VisionModel setting, admin-only via requireAdmin) can't be used to probe arbitrary hosts.
  • Error responses: writeServiceError never echoes raw error text to the client (unhandled errors become generic 500s, logged server-side only), so a vision-provider failure can't leak internals.

Nothing in this diff introduces a new injection point (all SQL is parameterized, JSON binding only), and the one/exactly-one plant-choice validation in CreateFromPacket is enforced server-side, not just client-side.

🎯 Correctness — No material issues found

VERDICT: No material issues found

I traced the seed-packet capture backend end-to-end against the checked-out code (not just the diff) for logic/domain-correctness bugs:

  • Catalog matching (internal/service/seed_packet.go matchPlants/wordIn) — verified rank ordering (0=exact name, 1=variety-in-name, 2=same-species), sort.SliceStable ascending, and the token-based wordIn word-boundary check against the accompanying tests (TestMatchPlants). Ranking and dedup (seen[pl.ID]) behave as intended.
  • Unit conversion in the vision prompt (internal/vision/vision.go) — "1 in = 2.54 cm" is the correct conversion factor; spacing field is documented/converted correctly, no unit-mixing between the dimension and spacing scales CLAUDE.md warns about.
  • Category enum alignmentSeedPacket.Category enum tag (vegetable,herb,flower,fruit,tree_shrub,cover) matches domain.Category* constants exactly (verified via grep in internal/domain/domain.go).
  • CreateFromPacket XOR logic (internal/service/seed_packet.go)hasID == hasNew correctly rejects both-set and neither-set, verified against TestCreateFromPacketExactlyOne.
  • Settings plumbinginstanceSettingsColumns/scanInstanceSettings/UpdateInstanceSettings SQL column order matches the Scan(...) and bind-arg order exactly after adding vision_model; migration 0011 is a straightforward additive ALTER TABLE.
  • EffectiveVision/EffectiveAgent symmetry — new EffectiveVision mirrors the established EffectiveAgent DB-over-env resolution pattern correctly.
  • service.New(..., opts ...Option) — variadic change is backward compatible; confirmed the sole other call site (cmd/pansy/main.go:55) still compiles unmodified.
  • SeedLotInput/packetLotRequest wiring — confirmed CreateFromPacket sets lotIn.PlantID before calling CreateSeedLot, which independently re-validates visibility via visiblePlant; PacketResult.Lot.PlantID matches the created/attached plant in both new- and existing-plant paths (matches test assertions).

One doc-only nit (not reported as a finding since it's not a behavioral bug): the comment above visiblePlant in CreateFromPacket (internal/service/seed_packet.go, "a plant they can't see is ErrNotFound") is wrong — visiblePlant actually returns domain.ErrInvalidInput (confirmed at internal/service/plantings.go:217-229), consistent with how CreateSeedLot already uses it. Behavior is correct and consistent with the rest of the codebase; only the comment is stale.

🧹 Code cleanliness & maintainability — Minor issues

All three findings in the draft check out against the actual code. Line numbers needed minor correction (draft was off by a few lines in two spots), but each finding is substantively accurate.

Review — Code cleanliness & maintainability

VERDICT: Minor issues

  • internal/service/instance_settings.go:131-144EffectiveVision duplicates EffectiveAgent's structure and independently calls s.store.GetInstanceSettings(ctx). Its only caller, settingsPayload (internal/api/settings.go:66-85), already holds the row (passed in as st) and additionally calls EffectiveAgent — so GET/PATCH /settings now does three reads of a single-row table (the caller's fetch, one inside EffectiveAgent, one inside EffectiveVision). A comment on settingsPayload (lines 63-64) explicitly acknowledges this redundancy already existed for EffectiveAgent; EffectiveVision copies rather than fixes it. Not blocking — it mirrors an existing pattern — but a second copy makes it more likely to drift.

  • internal/api/seed_packet.go:83-98 vs internal/api/seed_lots.go:20-42packetLotRequest and its toInput() reproduce all 11 fields and the field-by-field mapping of seedLotCreateRequest/its toInput() verbatim, differing only in the absence of PlantID. The comment explains why a separate type exists (reusing seedLotCreateRequest would wrongly require plantId), a fair constraint, but the duplication itself is avoidable — e.g. an embedded lotFields struct that both request types embed, with seedLotCreateRequest adding PlantID. As written, any future lot field addition/rename must be made correctly in two structs and two toInput() bodies.

  • 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.

Performance — No material issues found

Review — Performance lens

VERDICT: No material issues found

Walked the new code paths for the usual suspects:

  • matchPlants (internal/service/seed_packet.go:113-160) does a linear scan over ListPlantsForActor, but that's the same helper and pattern already used by the pre-existing FindPlants (internal/service/plants.go:102) — capped at maxPlantsListed = 5000 and documented as household-scale. Not a regression.
  • scanSeedPacket bounds the upload (scanUploadLimit = 30<<20) before imagenorm.Normalize, which itself caps decoded pixels at 50M and bytes at 25 MiB (pre-existing from #80, unchanged here) — no decompression-bomb or unbounded-allocation risk introduced.
  • CreateFromPacket does two sequential single-row creates (CreatePlant then CreateSeedLot), no loop, no N+1.
  • settingsPayload (internal/api/settings.go:66-86) now issues a third read of the single-row instance_settings table per GET/PATCH /settings (on top of the pre-existing double-read the code comment already flags as acceptable). This is a single-row SQLite query on a low-frequency, admin-only endpoint — not worth flagging as a material regression.
  • /capabilities now does a DB read (EffectiveVisionGetInstanceSettings) per call, but the frontend query has staleTime: 60_000 and isn't polled on an interval, so call volume stays low.

Nothing here rises to a real efficiency regression — extraction is an explicit, user-initiated, one-shot call (not a loop), and all the new list/scan operations reuse existing bounded patterns.

🧯 Error handling & edge cases — Blocking issues found

VERDICT: Blocking issues found

  • internal/api/seed_packet.go:26-36scanSeedPacket extends only the read deadline (SetReadDeadline, 60s) for the upload, never the write deadline. cmd/pansy/main.go:65 sets http.Server.WriteTimeout: 30 * time.Second, which per Go's documented semantics (and per this same codebase's own explanation at internal/api/agent.go:154-166, written for the identical #78 bug) is reset once at request-header-read time, not extended by a slow body read. So a legitimately slow phone upload that takes 30-60s (exactly the case scanReadTimeout exists for) can blow past the server's absolute write deadline before c.JSON(http.StatusOK, prop) runs — per agent.go:162-165, a write past that deadline returns err == nil with bytes silently dropped, so a successful extraction still gets delivered to the client as a dropped connection. The SSE path fixes this exact class with a per-write SetWriteDeadline (agent.go:176,199); scanSeedPacket has no analogous call anywhere.

  • internal/api/seed_packet.go:38-43c.Request.Body is wrapped in http.MaxBytesReader(..., scanUploadLimit) (30 MiB) before c.FormFile("image") parses the multipart body. imagenorm.Normalize separately caps the decoded input at 25 MiB (internal/imagenorm/imagenorm.go:59, read via its own io.LimitReader), so files between 25-30 MiB are correctly rejected by imagenorm.ErrTooLarge → 413. But a file that pushes the raw multipart envelope over 30 MiB fails during ParseMultipartForm itself (invoked internally by c.FormFile), which returns the MaxBytesReader error — and the handler maps any FormFile error, including this one, to 400 INVALID_INPUT "attach an image..." (line 40-43), not 413. A large-but-attached image is misreported as "no image attached."

  • internal/api/api.go:210-219capabilities swallows an error from h.svc.EffectiveVision(ctx) with no logging at all, silently downgrading to vision: false. This is inconsistent with the same call in settingsPayload (internal/api/settings.go:66-74), which propagates the identical error as a real failure, and with this codebase's established convention for best-effort reads that must not fail the whole response — e.g. internal/api/agent_holder.go:68: slog.Error("api: could not resolve agent settings; leaving the assistant as-is", "error", err). A persistent DB fault here makes /capabilities permanently and silently advertise vision: false with no operator-visible signal.

glm-5.2:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verified. The image path is well-hardened. Final review below.

No material issues found

I reviewed the seed-packet capture backend through the security lens — authentication, authorization, injection/SSF, untrusted-image handling, secret handling, and input validation — and verified each against the checked-out code.

  • Authn on both new endpoints (verified, internal/api/api.go:182-191): /seed-lots/scan and /seed-lots/from-packet are registered on the seedLots group, which is behind requireAuth(). The anonymous test cases (401) confirm this.

  • Authz on CreateFromPacket (verified, internal/service/seed_packet.go:181-201, internal/service/plantings.go:217-229, internal/service/seed_lots.go:152-155): when a user supplies PlantID, visiblePlant enforces that the plant is a built-in (OwnerID == nil) or owned by the actor — someone else's plant returns ErrInvalidInput, preventing IDOR. CreateSeedLot then re-checks the plant ownership even after CreateFromPacket sets lotIn.PlantID, so the ownership check can't be bypassed by a hand-crafted lot body. CreatePlant runs as the actor for the new-plant path.

  • Secret handling (verified): the API key is never stored in the DB or echoed in responses. EffectiveVision (internal/service/instance_settings.go:135-142) pulls the key only from cfg.Agent.OllamaCloudAPIKey (env); the settings row carries only the model name. /capabilities, /settings, and the scan response surface only vision/visionModel/visionReady booleans and the model string — no key. The admin-supplied VisionModel is validated via agentmodel.Validate before storage (instance_settings.go:65-71), gated behind requireAdmin.

  • SSRF / model-spec injection (verified, internal/agentmodel/agentmodel.go:27-46): the model spec is parsed through majordomo's registry against the single registered ollama.Cloud provider, so a spec must match a known provider token — it's not an arbitrary URL the server fetches. The spec is also admin-controlled, further reducing risk.

  • Untrusted-image handling (verified, internal/imagenorm/imagenorm.go:130-189): the upload path bounds the body (scanUploadLimit 30 MiB at internal/api/seed_packet.go:27), then imagenorm.Normalize caps the input read (MaxBytes 25 MiB), checks dimensions before full decode (maxDimension per-side, maxDecodePixels with an overflow-safe int64 multiply), and decodeSafely recovers from third-party (HEIC/WebP) decoder panics so a malformed upload fails one request instead of crashing the process. These are concrete decompression-bomb defenses; nothing in the diff weakens them.

  • Input validation: empty image is rejected in both scanSeedPacket (via imagenorm) and ExtractSeedPacket (len(jpeg) == 0ErrInvalidInput). The "exactly one of plantId/newPlant" rule is enforced in CreateFromPacket (hasID == hasNewErrInvalidInput).

Nothing in my lens is materially wrong.

🎯 Correctness — No material issues found

VERDICT: No material issues found

Through the correctness lens I verified the following by reading the checked-out code:

  • matchPlants ranking (internal/service/seed_packet.go:106-143): the rank assignment (exact name=0, variety-in-name=1, same-species=2) plus sort.SliceStable produces best-first ordering. Confirmed the test's "Music Garlic (rank 1) before Garlic (rank 2)" expectation holds given catalog insertion order, and the seen map correctly dedupes a plant that could match multiple arms (the switch falls through only one arm per plant anyway, so dedup is belt-and-suspenders, not a bug). wordIn is token-based as documented.
  • CreateFromPacket exactly-one (seed_packet.go:186-190): hasID == hasNew correctly rejects both-set and neither-set. The lot's PlantID is overwritten from res.Plant.ID (line 211), so the API's packetLotRequest.toInput() legitimately omitting PlantID is correct — the service attributes it. Verified SeedLotInput.PlantID is an int64 (not pointer) so the zero value is harmless before assignment.
  • visiblePlant / CreateSeedLot reuse (seed_lots.go:152, plantings.go:217): CreateFromPacket reuses the same ACL-enforcing path the direct endpoints use; a non-existent/foreign PlantID yields ErrNotFound, consistent with the rest of the codebase.
  • EffectiveVision precedence (instance_settings.go:138-145): DB VisionModel overrides env, key always from env — matches the documented Settings→env contract and the agent analogue.
  • validCategory (seed_packet.go:88-96): switches on domain.Category* constants (not raw strings), so it stays aligned with the domain's canonical values regardless of the enum tag spelling in vision.SeedPacket; the extracted category is only used as a prefill suggestion, so a mismatch degrades to "" rather than creating bad data.

No logic bug, mis-constant, or semantic correctness defect surfaced in this lens. The conversion factor in the extraction prompt (1 in = 2.54 cm) is correct, and the pointer/nullable numeric fields behave as designed (verified via the hermetic TestExtractLeavesMissingFieldsNil).

🧹 Code cleanliness & maintainability — Minor issues

All three findings verified against the source.

  1. validCategory (seed_packet.go:88-96) re-lists the same six domain.Category* constants that plantCategories (plants.go:27-34) already enumerates — confirmed duplication of the enum source of truth.
  2. packetLotRequest (seed_packet.go:83-95) is a field-for-field copy of seedLotCreateRequest (seed_lots.go:20-33) minus PlantID, with a parallel toInput() — confirmed verbatim duplication.
  3. _ = format is at line 63 (not 52 as in the draft JSON); confirmed it's assigned then discarded with a "logging if wanted" comment.

VERDICT: Minor issues

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

  • internal/api/seed_packet.go:83-103packetLotRequest is a field-for-field copy of seedLotCreateRequest (internal/api/seed_lots.go:20-42) minus PlantID and its binding:"required", with an identical toInput() body minus the PlantID line. The comment justifies not reusing the struct, but the lot fields are still duplicated verbatim, so any new lot column must be added to both request types and both toInput() methods. A small shared lotFields struct embedded by both would keep one definition. At minimum, a "// keep in sync with seedLotCreateRequest" note would flag the duplication.

  • internal/api/seed_packet.go:63_ = format assigns then discards the format return from imagenorm.Normalize with a "available for logging if wanted" comment. If genuinely unused, the current form reads as leftover scaffolding. Trivial; drop it or actually log it.

Performance — Minor issues

VERDICT: Minor issues

  • internal/api/api.go:214-217/capabilities previously was a single atomic pointer load (h.agent.get()) with no I/O; this PR adds a per-request DB read (svc.EffectiveVisionstore.GetInstanceSettings, a SELECT ... FROM instance_settings WHERE id=1). This is the endpoint the UI polls on every garden-editor page load and on focus/refetch. The comment above it (api.go:206-209) frames it as tracking the live Runner "on the very next poll," inviting repeated hits. At household scale it's cheap, but it is a real I/O regression on a path that was deliberately allocation-free. Consider caching vision readiness on the agentHolder (or a parallel atomic) the way the agent Runner is, so /capabilities stays a pointer read and settings changes swap it atomically, instead of issuing a SQLite query per request. Verified: EffectiveVision (internal/service/instance_settings.go:131) calls s.store.GetInstanceSettings(ctx).

  • internal/api/settings.go:67-74settingsPayload calls EffectiveAgent and then EffectiveVision, each of which independently runs store.GetInstanceSettings (the same SELECT against the same single row). That's two identical reads to build one response. getSettings already holds the fetched row st (settings.go:88-89) and passes it into settingsPayload, so EffectiveVision could accept the already-fetched *domain.InstanceSettings to avoid the duplicate query. Verified: EffectiveAgent (instance_settings.go:98) and EffectiveVision (instance_settings.go:131) both call s.store.GetInstanceSettings(ctx). Minor, but a pure duplicate query on the settings endpoint.

Neither is a hot-loop/N+1/quadratic issue; both are avoidable extra DB reads on request paths. No allocation/quadratic/unbounded-growth problems in matchPlants or the upload path (bounded by scanUploadLimit and imagenorm).

🧯 Error handling & edge cases — Minor issues

Both findings verified against the actual source.

  • seed_packet.go:38-43 confirms MaxBytesReader is set on the body, c.FormFile errors are all mapped to 400 INVALID_INPUT with no *http.MaxBytesError check, while 413 only fires from imagenorm.ErrTooLarge at line 56-58. Finding stands.
  • api.go:210-218 confirms EffectiveVision errors are discarded with no log, degrading silently to vision: false. Finding stands.

Verdict: Minor issues

  • internal/api/seed_packet.go:38-43 — oversized multipart upload returns 400 instead of the promised 413. The handler wraps the body with http.MaxBytesReader(..., scanUploadLimit) (30 MiB) and then calls c.FormFile("image"). When the upload exceeds 30 MiB, MaxBytesReader fails the read with a *http.MaxBytesError, which c.FormFile surfaces. The handler maps every FormFile error to 400 INVALID_INPUT / "attach an image as the image field" with no errors.As(err, *http.MaxBytesError) check. The PR description promises "maps too-large/unreadable to clear statuses (413 / 400)", but in practice the 413 only ever fires from imagenorm.ErrTooLarge (the 25 MiB decoded cap at line 56), not from the 30 MiB envelope cap the handler itself sets — so a >30 MiB upload gets a misleading 400 "attach an image" instead of 413. Suggested fix: before the generic 400, check var maxErr *http.MaxBytesError; errors.As(err, &maxErr) and emit http.StatusRequestEntityTooLarge with an IMAGE_TOO_LARGE code.

  • internal/api/api.go:213-216/capabilities silently swallows EffectiveVision errors as vision:false. If GetInstanceSettings fails (e.g. DB transient), the probe degrades to "feature off" with no log. This is a defensible best-effort choice for a capability endpoint, but the error is discarded entirely; a slog line would make a real outage diagnosable rather than looking like an unconfigured instance. Minor.

kimi-k2.6:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

  • The /seed-lots/scan and /seed-lots/from-packet endpoints both sit behind requireAuth (internal/api/api.go:182), and the service layer enforces plant visibility via visiblePlant (existing plants) and CreatePlant (new plants get the actor's owner_id).
  • The vision model API key stays in the environment only — the DB stores only the model spec, confirmed by both the migration (0011_vision_model.sql comments) and the README ("the API key is intentionally not among the runtime-editable settings").
  • Image upload has layered DoS protection: MaxBytesReader at 30 MiB, imagenorm.Normalize with a 25 MiB read cap, 50 MP pixel-count ceiling, 50k dimension ceiling, and recover() around third-party decoders (decodeSafely).
  • The vision prompt is a fixed constant string; no user-controlled text is interpolated, so prompt injection is not possible.
  • Model output is unmarshaled into a typed SeedPacket struct with pointer fields for nullability, rather than being passed into any dynamic execution or query building.
🎯 Correctness — No material issues found

No material issues found

After examining the diff and verifying key cross-file contracts (domain constants, struct tags, API precedence rules, migration schemas, and the imagenorm integration), the logic holds up:

  • Catalog matching (internal/service/seed_packet.go:106-154): The rank-based deduplication with seen correctly surfaces the best match only, and wordIn behaves as documented for space-delimited tokens.
  • XOR validation (internal/service/seed_packet.go:187-189): hasID == hasNew correctly rejects both-nil and both-present confirms.
  • Category mapping (internal/service/seed_packet.go:88-96): validCategory checks against the exact domain constants, which match the vision-model enum (tree_shrubdomain.CategoryTreeShrub).
  • Precedence chain (internal/service/instance_settings.go:128-140): EffectiveVision correctly layers DB-over-env for the model, with the key always from env.
  • Migration 0011 (internal/store/migrations/0011_vision_model.sql): Adds vision_model TEXT NOT NULL DEFAULT '', consistent with the domain struct and the empty-string-as-inherit convention.
  • ACL enforcement (internal/service/seed_packet.go:203): visiblePlant gates the existing-plant path.

The conversion factor in the vision prompt (1 in = 2.54 cm) is correct, and the hermetic test path via WithPacketExtractor is sound. Nothing in this PR introduces a logic bug or semantic domain error.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

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

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

Performance⚠️ could not complete

⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response

🧯 Error handling & edge cases — Minor issues

Minor issues

  • capabilities silently swallows EffectiveVision DB errors. internal/api/api.go:214-217
    If EffectiveVision returns an error (e.g., the DB is temporarily unreachable), the endpoint returns vision: false with HTTP 200 and logs nothing. An operator cannot distinguish “vision not configured” from “vision broken,” and the UI will hide the scan button during a transient outage. The error should at least be logged.
opencode/glm-5.2:cloud (opencode) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

VERDICT: No material issues found

I reviewed the change through the security lens (authn/authz, injection, SSRF, secret leakage, input validation, untrusted-data handling) and verified the relevant code paths by reading them.

  • Authn/authz on new routes — both POST /seed-lots/scan and POST /seed-lots/from-packet are registered under the seedLots group, which sits behind h.requireAuth() (api.go:182, 190-191). The anonymous-401 cases are explicitly tested. scan is read-only by design. CreateFromPacket delegates to CreatePlant, visiblePlant, and CreateSeedLot, each of which enforces ownership (verified at plants.go:194-211, plantings.go:217-229, seed_lots.go:152-155). No new authz seam was introduced.
  • Exactly-one confirmCreateFromPacket enforces hasID == hasNew → ErrInvalidInput (seed_packet.go:185-187), so an ambiguous confirm cannot silently pick a plant. Verified.
  • No SQL/command injectionmatchPlants is pure in-memory string matching over an already-loaded []domain.Plant; it never builds SQL. All persistence goes through existing parameterized store methods. The user-supplied NewPlant/Lot fields flow through finalizePlant/finalizeSeedLot validation, not raw into SQL.
  • No SSRF — the image is uploaded by the user (c.FormFile), not fetched from a URL; the filename is never used. SourceURL is stored, never retrieved.
  • Secret handling — the API key stays in the env and is never stored in the DB or serialized. /settings (admin-only via requireAdmin(), settings.go:158) reports only visionModel (the model name) and visionReady (a bool); the key is not leaked. EffectiveVision (instance_settings.go:117-141) pulls the key from cfg.Agent.OllamaCloudAPIKey and exposes only Ready(). Verified.
  • Vision-model spec is admin-controlledvisionModel only changes through the admin-gated UpdateInstanceSettings, validated via agentmodel.Validate (instance_settings.go:63-71). The /scan request body never supplies a model spec; it comes from settings/env. No user-controlled model-injection path.
  • Upload DoS boundshttp.MaxBytesReader(..., 30<<20) (seed_packet.go:53) bounds the multipart body, and imagenorm.Normalize independently caps decoded bytes/pixels and recovers from decoder panics (imagenorm.go:130-157). ErrTooLarge → 413, non-image → 400. Verified.
  • Untrusted image to third party — the JPEG is sent to the configured vision provider. This is the explicit, authenticated, user-initiated feature; the bytes cannot call tools (majordomo.Generate[T] is one-shot structured extraction), so they cannot touch the garden. No injection surface there.
  • /capabilities is unauthenticated and now also reports vision bool — this is consistent with the pre-existing agent bool (api.go:62, registered without requireAuth), and discloses only feature availability, not secrets. Not a new disclosure class, so not flagged.

No security findings to report.

🎯 Correctness — No material issues found

I'll investigate this PR through the Correctness lens, verifying against the actual code.
Now let me check the agentmodel.Resolve function and image/jpeg import shadowing in the scan handler.
Now let me check the ListPlantsForActor semantics and the variable shadowing concern with jpeg in the scan handler.
Now I'll examine a potential edge case in matchPlants — the "exact name" match only compares variety against the plant name, not the species. Let me also check the effectiveView settings test and look at whether the seedLots routes ordering matters (e.g., does POST /scan conflict with POST /:id routes).
Now let me check the matchPlants exact-name logic more carefully. The "exact name" case matches name == variety, but a plant named "Garlic" with a packet of species "garlic" and no variety won't match as exact (variety empty → skipped), but will match via species word boundary. Let me also verify the effectiveView JSON field naming is consistent and check the Quantity zero-value issue (a lot with quantity: 0 from the packet — packetLotRequest.Quantity is non-pointer float64).
Now let me look at the EffectiveVision precedence — does an empty DB VisionModel ("") override the env value? The code says: if st.VisionModel != "" { eff.Model = st.VisionModel }, so an empty DB value falls through to env. Good. But what does the settings update path do — does it store empty as empty?

Let me check the store update to confirm the trim behavior and check whether there's a way to "clear" a vision model back to env (store empty string).
Now let me examine a key semantic concern: the matchPlants "exact name" case matches name == variety, but plants are typically named with the variety AND species (e.g., "Music Garlic"), while the packet's variety is just "Music". So "exact name" will rarely match unless someone named their plant exactly "Music". The test confirms this — "Cherokee Purple" matches a plant named "Cherokee Purple". This is by design. Let me verify the seedLots.POST("/:id") doesn't conflict — POST /scan could match /:id if gin routes scan as an id. Let me check gin's behavior — static routes take precedence over params in gin, so /scan registered before/after /:id is fine. Actually in gin, static routes win. Let me verify there's no POST /:id though — there isn't (only GET/PATCH/DELETE /:id). So POST /scan and POST /from-packet are the only POSTs besides POST "". Good, no conflict.

Now let me check a subtle correctness issue: the scanSeedPacket handler calls ExtractSeedPacket which returns domain.ErrInvalidInput for both "empty jpeg" AND "vision not ready". The handler maps ANY ErrInvalidInput to 503 VISION_DISABLED. But ExtractSeedPacket could also return ErrInvalidInput from... actually no, the only ErrInvalidInput paths in ExtractSeedPacket are the two explicit ones (empty jpeg, not ready). The jpeg can't be empty here (imagenorm would have failed). So this is fine.

But wait — what about CreateFromPacket? It returns ErrInvalidInput for the exactly-one check, and writeServiceError maps that to 400 INVALID_INPUT. That's correct. But CreatePlant and CreateSeedLot can also return ErrInvalidInput (validation failures), which map to 400. Fine.

Let me check one more thing — the packetLotRequest.Quantity is a non-pointer float64, so a missing quantity in the JSON defaults to 0. finalizeSeedLot allows Quantity >= 0, so 0 passes. Is a zero-quantity lot semantically wrong? Let me check the regular seedLotCreateRequest — it also has Quantity float64 (non-pointer). So this is consistent with existing behavior, not a regression.
Now let me check one important semantic concern. The vision_test.go uses extractVia which mirrors Extract's body but uses a test-supplied registry/model. But the actual Extract function calls agentmodel.Resolve which builds a new registry each time and does NOT register the fake provider. So the test for the actual Extract function (TestExtractRejectsEmptyImage) only tests the empty-image guard, not the model resolution path. The schema/image verification in TestExtractParsesModelJSON uses extractVia, not Extract. This is a test gap but the PR description acknowledges this is the majordomo fake pattern. Let me check whether majordomo.Generate actually uses the schema derived from struct tags — the test asserts call.Request.SchemaName != "seedpacket". Let me verify majordomo's Generate derives the schema name from the type name.
majordomo is an external dependency (not checked out here), so I can't verify Generate's schema derivation directly. But the test asserts the schema name is "seedpacket" (lowercase type name). This is a test assertion, not a correctness issue in the production code. If majordomo's Generate derives the schema name differently, the test would fail — but that's a test issue, not a production correctness issue. I'll leave this alone as I can't verify the external dep.

Now let me look at the most important semantic concern: the image/jpeg import shadowing. In seed_packet.go, the handler declares jpeg, format, err := imagenorm.Normalize(...). This creates a local variable named jpeg. There's no image/jpeg import in seed_packet.go, so no shadowing issue — the variable just holds the normalized JPEG bytes. That's fine.

Let me look at one more potential issue — in the scan handler, after MaxBytesReader triggers (body too large), c.FormFile returns an error. But MaxBytesReader returns a *http.MaxBytesError, not a regular error. The handler maps ALL FormFile errors to 400 INVALID_INPUT. This means a too-large upload via MaxBytesReader returns 400 instead of 413. Let me verify whether this matters.
Now let me check the scanUploadLimit vs imagenorm byte cap. The handler sets MaxBytesReader to 30 MiB, but imagenorm.Normalize with default Options{} uses DefaultMaxBytes. Let me check that value.
Now I see an important interaction. MaxBytesReader is set to 30 MiB, but imagenorm.Normalize with default Options{} caps at 25 MiB. So:

  1. If the multipart body is 26 MiB, MaxBytesReader allows it through (under 30), but imagenorm reads only 25 MiB + 1 byte and returns ErrTooLarge. This maps to 413. Good — but the handler comment says "this is a little over that for the multipart envelope", which is correct reasoning.

  2. But there's a subtlety: MaxBytesReader wraps c.Request.Body. When c.FormFile("image") is called, gin reads the entire multipart body to parse it (including the file content into a temp file/memory). If the body exceeds 30 MiB, MaxBytesReader triggers during the FormFile read, and c.FormFile returns an error → 400 INVALID_INPUT. So a 31 MiB upload gets 400, not 413. The handler distinguishes 413 only for the decoded image being too large (via imagenorm), not the raw upload being too large.

This is a minor UX inconsistency (31 MiB upload → 400 instead of 413), but it's not a logic bug — both are rejections. The comment says the limit is "a little over" the 25 MiB imagenorm cap to account for the multipart envelope, which is the right design. A 31 MiB raw upload exceeding both is an edge case. Not a correctness bug.

Now let me check the most semantically interesting thing: the matchPlants exact-name match. When variety is "Music" and a plant is named "music" (case difference), name == variety after lowercasing both → "music" == "music" → exact match. Good. But what if a plant is named "Music Garlic" and variety is "music garlic"? Then name == variety → "music garlic" == "music garlic" → exact. But "music garlic" is really the full name, not a variety. The model would return variety="Music" and species="garlic" typically, not variety="Music Garlic". So this edge case is unlikely from the model. Not a real bug.

Let me verify one final concern: the CreateFromPacket partial-failure semantics. If CreatePlant succeeds but CreateSeedLot fails, the plant is left behind (documented). The caller gets an error and res is nil (because the function returns nil, err). So the caller doesn't get the plant ID to retry against. The comment says "The caller sees the error and can retry the lot against the now-existing plant" — but the caller doesn't know the plant ID from the error response. They'd have to search the catalog. This is a minor UX issue, not a correctness bug in the logic itself. The plant IS created and editable, which is the stated tradeoff.

Let me verify one more thing — does CreateSeedLot call visiblePlant again (redundantly), since CreateFromPacket already called visiblePlant for the existing-plant case?
Yes, CreateSeedLot calls visiblePlant again. For the new-plant path, this is necessary (the plant was just created, and CreateSeedLot needs to verify it's visible — which it is since the actor owns it). For the existing-plant path, visiblePlant is called twice (once in CreateFromPacket, once in CreateSeedLot). This is redundant but not incorrect — just a minor extra DB query. Not a correctness issue.

Now let me check whether CreatePlant returns a plant with the ID populated, so lotIn.PlantID = res.Plant.ID works for the new-plant path. CreatePlant calls s.store.CreatePlant(ctx, p) which returns the created plant with ID. Let me verify.
Good — CreatePlant returns the created plant with ID populated via RETURNING. So res.Plant.ID is valid for both paths. The lotIn.PlantID = res.Plant.ID assignment is correct.

Now let me check the effectiveView — there's a potential issue. The settingsPayload calls EffectiveAgent and EffectiveVision. EffectiveAgent reads the settings row, and EffectiveVision reads it again. Two DB reads for one /settings GET. Not a correctness bug, just minor inefficiency.

Let me check one final semantic concern: the SeedPacket struct's Category enum tag says tree_shrub (with underscore), and validCategory checks against domain.CategoryTreeShrub which is "tree_shrub". Consistent. And the model is told via the enum tag to use tree_shrub. Good.

Let me verify the fromPacketRequest binding. The Lot field has Unit with binding:"required". But ShouldBindJSON only validates the top-level struct's binding tags by default — does gin recursively validate nested struct binding tags?
Now I have a good understanding. Let me verify one final semantic concern about the matchPlants "variety in name" check using strings.Contains. Consider: variety "Purple" matches plant "Purple Basil" (rank 1, "variety in name") but also plant "Cherokee Purple" (rank 1 too, "variety in name"). Both get rank 1. The test for "Cherokee Purple" with variety "Cherokee Purple" gets rank 0 (exact). But if the model reads variety "Purple" instead, "Cherokee Purple" would be rank 1 (contains) and "Purple Basil" would be rank 1 (contains). Both same rank, stable-sorted by original order. This is by design — the comment says "no fuzzy scoring that could confidently mis-rank", and the ranking is conservative. The user confirms anyway.

Let me check one more important thing — does the packetLotRequest have a purchasedAt as *string while seedLotCreateRequest also has *string? Yes, consistent. And finalizeSeedLot validates via validDatePtr. Good.

Now let me look at a potential real issue: in the createFromPacket handler, when req.NewPlant is non-nil but the newPlant JSON object is missing required fields (like name), gin's ShouldBindJSON succeeds (JSON parses fine), but then NewPlant.toInput() produces a PlantInput with empty Name. The service CreatePlantfinalizePlant rejects empty Name → ErrInvalidInput → 400. So the "both plant choices" test case at line with newPlant: {name: "X", ...} — the test sends a valid newPlant with name: "X", so binding succeeds, and the service rejects it at the hasID == hasNew check (both set) before reaching CreatePlant. Good.

But wait — let me re-examine the "both" test case. The test sends:

"plantId": plantID, "newPlant": {name: "X", category: "vegetable", color: "#4a7c3f", icon: "🌱"}

Note: no spacingCm. plantCreateRequest.SpacingCM is float64 (defaults to 0). The service CreateFromPacket checks hasID == hasNew → both true → returns ErrInvalidInput immediately, BEFORE calling CreatePlant. So the missing spacing doesn't matter. Good — the exactly-one check is first.

Let me verify there are no issues with the migration.
Migration 0011 adds vision_model TEXT NOT NULL DEFAULT '' via ALTER TABLE. The seeded row from 0010 has no vision_model column, but ALTER TABLE with DEFAULT '' backfills existing rows to ''. So the existing row gets vision_model = ''. The scanInstanceSettings reads 5 columns now: agent_model, agent_enabled, vision_model, version, updated_at. This matches the updated instanceSettingsColumns. Good.

Let me verify the migration runner handles ALTER TABLE properly and that migrations run in order.
Migrations run in sorted order by version number. 0011 runs after 0010. Good.

Now let me check one final semantic concern about the wordIn function and species matching. The wordIn uses strings.Fields which splits on whitespace. So "garlic" matches "German Garlic" (token "garlic" after lowercasing). But what about punctuation? A plant named "Garlic, German" → tokens are ["garlic,", "german"] → "garlic" != "garlic," → no match. This is a minor limitation but not a bug — plant names in pansy are user-created and typically don't have commas. The test explicitly tests the word-boundary behavior. Fine.

Let me look at whether there's an issue with the effectiveView JSON response — the VisionModel field exposes the resolved model name to the admin. That's fine (it's not a secret).

I've now thoroughly examined the code through the Correctness lens. Let me check one last thing — whether the extractPacket field on Service could be nil in production (if someone constructs Service without using New).

The New function sets extractPacket: vision.Extract by default. The WithPacketExtractor option overrides it. In production, New is always used (confirmed in auth_test.go line 31: service.New(db, cfg)). So extractPacket is never nil in production. Good.

Let me verify the test helpers used by the seed_packet_test are correct.
I've now thoroughly reviewed the PR through the Correctness lens. Let me check one more subtle thing — the TestScanSeedPacketAPI test creates a plant named "Music Garlic" and scans a packet with variety "Music", species "garlic". The matchPlants should find "Music Garlic" as rank 1 ("variety in name": "music garlic" contains "music"). But there are no built-in "Garlic" plants in the test DB (the test DB starts empty except for user-created plants). So only one candidate. The test checks len(cands) == 0 → expects non-zero. Good.

But wait — the test creates a plant "Music Garlic" which is owned by the user. ListPlantsForActor returns built-ins + user's own. There are seeded built-in plants from migration 0003. Let me check if any built-in is named with "garlic" or "music".
There IS a built-in "Garlic" plant. So in TestScanSeedPacketAPI, the packet has species "garlic" and variety "Music". matchPlants would match:

  • "Music Garlic" (user-created) → rank 1 ("variety in name": "music garlic" contains "music")
  • "Garlic" (built-in) → rank 2 ("same species": word "garlic" in "garlic")

So len(cands) should be 2, not just 1. The test only checks len(cands) == 0 → expects non-zero, so 2 passes. But the test comment says "A matching plant so the proposal has a candidate" — it gets 2 candidates, not 1. The test doesn't assert the count is exactly 1, so it passes. Not a bug.

But this reveals an important semantic point: the built-in "Garlic" is a candidate for any garlic-variety packet. That's by design — the user might be buying generic Garlic. The user confirms. Good.

I've now completed my thorough review. Let me summarize my findings.

The code is semantically correct through the Correctness lens. The logic is sound:

  • matchPlants ranking is conservative and correct (exact → contains variety → same species word)
  • CreateFromPacket exactly-one check is correct (hasID == hasNew catches both and neither)
  • EffectiveVision precedence (DB → env) is correct
  • Migration 0011 correctly adds the column with backfill default
  • Store column list and scan order are consistent
  • The extractPacket injectable field defaults to vision.Extract and is never nil in production
  • Error mapping (ErrInvalidInput → 503 for vision-not-configured) is correct
  • visiblePlant ACL enforcement for existing-plant path is correct
  • New-plant path correctly attributes the lot via lotIn.PlantID = res.Plant.ID

The only minor observations (not material bugs):

  • CreateSeedLot redundantly calls visiblePlant when CreateFromPacket already did (minor extra query, not a bug)
  • A raw upload exceeding scanUploadLimit (30 MiB) gets 400 instead of 413 (edge case, both are rejections)
  • EffectiveVision and EffectiveAgent each read the settings row separately in settingsPayload (minor inefficiency)

None of these are correctness bugs.

VERDICT: No material issues found.

Review

VERDICT: No material issues found

I reviewed the PR through the Correctness lens, verifying against the actual checked-out code. The logic is sound throughout:

  • matchPlants ranking (seed_packet.go:106-143): exact-name → variety-in-name → same-species-word, case-insensitive, deduped via seen map, stable-sorted by rank. Verified the wordIn token-boundary logic and the test cases against the built-in "Garlic" plant in migration 0003 — a "Music"/"garlic" packet correctly produces "Music Garlic" (rank 1) and built-in "Garlic" (rank 2) as candidates.
  • CreateFromPacket exactly-one check (seed_packet.go:186-190): hasID == hasNew correctly rejects both-set and neither-set. Verified the new-plant path populates res.Plant.ID via store.CreatePlant's RETURNING clause (store/plants.go:124-136), so lotIn.PlantID = res.Plant.ID attributes the lot correctly.
  • EffectiveVision precedence (instance_settings.go:131-143): empty DB VisionModel falls through to env; non-empty overrides. Consistent with EffectiveAgent. Verified the store update trims and stores the value (store/instance_settings.go:56-80).
  • Migration 0011: ALTER TABLE ... ADD COLUMN vision_model TEXT NOT NULL DEFAULT '' backfills the existing single row from 0010 to ''. scanInstanceSettings reads 5 columns matching instanceSettingsColumns. Verified column order in the RETURNING clause and Scan call are aligned.
  • extractPacket default: service.New sets extractPacket: vision.Extract (service.go:54), never nil in production; WithPacketExtractor overrides for tests.
  • Error mapping: ErrInvalidInput from ExtractSeedPacket (vision-not-ready or empty jpeg) → 503 VISION_DISABLED in the scan handler; CreateFromPacket's ErrInvalidInput → 400 via writeServiceError. Verified both paths.
  • Category enum consistency: SeedPacket.Category enum tag tree_shrub matches domain.CategoryTreeShrub = "tree_shrub", and validCategory checks all six domain categories. Verified.
  • visiblePlant ACL (plantings.go:217-229): existing-plant path in CreateFromPacket enforces visibility; CreateSeedLot re-checks (redundant but not incorrect).

Minor non-material observations: CreateSeedLot redundantly calls visiblePlant after CreateFromPacket already did (one extra DB query); a raw upload exceeding 30 MiB returns 400 from MaxBytesReader rather than 413 (both are rejections; only the decoded image over 25 MiB gets 413 via imagenorm.ErrTooLarge). Neither is a correctness bug.

🧹 Code cleanliness & maintainability — Minor issues

I'll verify each of the three findings against the actual code.
All three findings verified against the actual source code. Confirming each:

  1. validCategory (seed_packet.go:88-96) — switch hard-codes the six domain.Category* constants. plantCategories (plants.go:27-34) is the map of the same six. Map is consumed at plants.go:316. Two sources of truth confirmed.

  2. extractVia (vision_test.go:30-37) — body matches Extract (vision.go:52-67) exactly: same majordomo.Generate[SeedPacket] call, same extractPrompt, same UserParts(Text(...), Image("image/jpeg", jpeg)) message. Only difference is model source. The comment at lines 27-29 admits the duplication. Confirmed.

  3. packetLotRequest (seed_packet.go:83-103) — re-declares every field of seedLotCreateRequest (seed_lots.go:20-33) minus PlantID, plus its own toInput() (seed_packet.go:97-103) mirroring seedLotCreateRequest.toInput() (seed_lots.go:35-42) minus the PlantID: line. Comment at seed_packet.go:82 documents the rationale. Confirmed.

VERDICT: Minor issues

Within the code-cleanliness / maintainability lens, three duplication concerns. All verified by reading the surrounding code.

  • validCategory duplicates the plantCategories enum mapinternal/service/seed_packet.go:87-96 hard-codes the same six category constants as a switch that plantCategories (in internal/service/plants.go:27-34, consumed at plants.go:316) already encodes as a map. Two sources of truth for one enum: add a category to one and forget the other and they silently drift (a packet category that's valid for plant creation would be dropped from SuggestedCategory, or vice-versa). Cleaner one-liner: func validCategory(c string) string { if _, ok := plantCategories[c]; ok { return c }; return "" }. Verified by reading both files.

  • extractVia is a maintained-by-hand copy of Extract's bodyinternal/vision/vision_test.go:30-37 duplicates Extract (internal/vision/vision.go:52-67), and the comment admits it ("mirrors Extract's body exactly apart from where the model comes from"). The duplication exists because Extract resolves its model via agentmodel.Resolve while the test injects an llm.Model directly. The cost is real: if the prompt text or message construction in Extract changes, the hermetic test still passes against the stale copy and never notices. A small refactor — extract a generate(ctx, m llm.Model, jpeg []byte) (SeedPacket, error) core that both Extract and the test call — would remove the drift surface. Verified by reading both.

  • packetLotRequest duplicates seedLotCreateRequest minus PlantIDinternal/api/seed_packet.go:83-103 re-declares every lot field and a toInput() that mirrors seedLotCreateRequest.toInput() (internal/api/seed_lots.go:20-42) minus the PlantID line. The comment explains why (the original has binding:"required" on PlantID, which would wrongly require it for the packet flow), so the tradeoff is reasonable — but adding a new lot field now requires touching two structs and two toInput() methods in lockstep. Embedding a shared lotFields struct (no binding tag) in both request types would centralize the field list while letting each keep its own binding rules. Verified by reading both.

Performance — Minor issues

I'll verify each finding against the actual code.
Both findings verified against actual code.

Finding 1: seed_packet.go:203 calls s.visiblePlant(ctx, actorID, *in.PlantID) (line 203), then seed_packet.go:212 calls s.CreateSeedLot(ctx, actorID, lotIn), whose first line at seed_lots.go:153 calls s.visiblePlant(ctx, actorID, in.PlantID) again on the same PlantID. Confirmed redundant query.

Finding 2: settings.go:88-100 getSettings: GetInstanceSettings (line 89) → settingsPayload calls EffectiveAgent (line 67) and EffectiveVision (line 71). Both EffectiveAgent (instance_settings.go:99) and EffectiveVision (instance_settings.go:132) independently call s.store.GetInstanceSettings(ctx). So the row is read three times. Confirmed.

Both findings survive verification.

Verdict: Minor issues

  • 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 write path. Low severity (single user action, not a loop) but a real redundant query this PR introduces. Fix: pass the already-loaded res.Plant straight to the lot-creation store call, or add an internal createSeedLotFor(ctx, actorID, plant, in) variant that skips the re-check.

  • internal/api/settings.go:66-74instance_settings row fetched three times per GET/PATCH /settings. getSettings calls svc.GetInstanceSettings (line 89), then settingsPayload calls svc.EffectiveAgent (line 67) and now svc.EffectiveVision (line 71) — each independently calling s.store.GetInstanceSettings (instance_settings.go:99 and :132). This PR adds the third read of the same single row. Admin-only and infrequent, so impact is trivial, but the row could be read once and passed to both resolvers.

🧯 Error handling & edge cases — Minor issues

All three findings confirmed against the actual code. Let me verify finding 3's claim about separate store calls (no shared transaction).
All three findings verified against the actual code:

  1. Finding 1 confirmed: seed_packet.go:38-43 wraps body in MaxBytesReader but maps every FormFile error to 400. Grep confirms no MaxBytesError check anywhere in the repo. The 413 branch (line 56) only catches imagenorm.ErrTooLarge.

  2. Finding 2 confirmed: api.go:215 swallows EffectiveVision error silently. Verified EffectiveVision (instance_settings.go:131) reads from s.store.GetInstanceSettings which can fail. Agent path uses atomic pointer with no error path.

  3. Finding 3 confirmed: seed_packet.go:194-216CreatePlant and CreateSeedLot are separate service calls (verified in plants.go:194 and seed_lots.go:152), no shared transaction. On lot failure, returns (nil, err) — the res with Plant populated is discarded, so caller loses the plant ID.

Verdict: Minor issues

  • Body-cap oversized upload misreported as 400, not 413internal/api/seed_packet.go:38-43. The handler wraps the body in http.MaxBytesReader(..., scanUploadLimit) (30 MiB) but maps every c.FormFile("image") error to 400 INVALID_INPUT with "attach an image as the "image" field". When the 30 MiB multipart body cap is exceeded, FormFile returns an error wrapping *http.MaxBytesError, distinguishable via errors.As. The PR description promises "caps the body, maps too-large/unreadable to clear statuses (413 / 400)", but the 413 branch at lines 56-58 only catches imagenorm.ErrTooLarge (decoded image too big). So an upload that blows the very cap scanUploadLimit exists to enforce gets a misleading "attach an image" 400 instead of the intended 413. Verified by reading the handler and grepping — MaxBytesError appears nowhere in the repo. Suggested fix: before the generic 400, if var max *http.MaxBytesError; errors.As(err, &max) { writeAPIError(c, 413, "IMAGE_TOO_LARGE", ...); return }.

  • EffectiveVision store error swallowed in /capabilitiesinternal/api/api.go:215. if vis, err := h.svc.EffectiveVision(...); err == nil { vision = vis.Ready() } silently reports vision: false on a transient DB error. The comment justifies per-request reads "same as agent", but the agent branch (h.agent.get() != nil) is an atomic pointer load with no error path, while EffectiveVision does a DB read that can fail (verified: instance_settings.go:131-135 returns err from s.store.GetInstanceSettings). So a transient store failure degrades a capabilities flag with no log, no signal. Minor (advisory endpoint), but it is a swallowed error. Suggested fix: slog.Warn on the non-nil branch.

  • CreateFromPacket discards the orphaned plant's identity on partial failureinternal/service/seed_packet.go:194-216. If CreatePlant succeeds (line 194) and CreateSeedLot then fails (line 212), the plant row is committed (verified: CreatePlant in plants.go:194 and CreateSeedLot in seed_lots.go:152 are separate service calls with no shared transaction) but the function returns (nil, err) at line 214, discarding res which holds the created Plant. The comment at 182-185 documents the deliberate "leave it, don't roll back" choice, but the returned *PacketResult is nil, so the caller can't retry the lot against the just-created plant — it has no handle to it and must re-find it in the catalog. Suggested fix: return res with Plant populated even on the lot error (or a typed error carrying PlantID) so the retry is automatic.

opencode/kimi-k2.6:cloud (opencode) — Blocking issues found

Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found.

Security lens review: The change is solid from a security perspective.

  • AuthN/AuthZ: Both /scan and /from-packet are correctly behind requireAuth(). The service layer enforces ownership via CreatePlant (sets OwnerID = actorID), CreateSeedLot (sets OwnerID = actorID), and visiblePlant (rejects another user's plant with ErrInvalidInput). No authorization gaps.
  • Input validation: The upload is bounded by http.MaxBytesReader(30 MB) before multipart parsing, and imagenorm.Normalize adds its own 25 MB byte cap plus dimension/pixel-count guards against decompression bombs. The extracted SeedPacket fields are returned as a proposal — the model's output is never written directly; the user must confirm via CreateFromPacket.
  • Injection/SSRF: No SQL injection (parameterized store methods). SourceURL in the lot request flows through finalizeSeedLotvalidSourceURL, which rejects non-http/https schemes and requires a host. No command injection vectors.
  • Secret handling: OLLAMA_CLOUD_API_KEY stays in the env as designed. EffectiveVision never leaks the key value to the API — /capabilities and /settings only expose visionReady/visionModel booleans and strings.
  • Secure defaults: The vision feature is off by default (empty VisionModel). It requires both a configured model and a present API key to be advertised as ready. The API returns 503 when unconfigured, preventing blind calls.

Nothing in this diff raises a security concern.

🎯 Correctness — No material issues found

No material issues found

Through the correctness lens, the PR is sound. I verified every cross-file dependency and re-derived the key behaviors:

  • Unit conversion: 1 in = 2.54 cm in the vision prompt is exact by definition.
  • Catalog matching: matchPlants uses conservative, name-based ranking (exact → substring → word-boundary species). The seen map prevents duplicate entries, and sort.SliceStable preserves input order within ranks. Verified against internal/service/seed_packet_test.go.
  • Exactly-one validation: hasID == hasNew correctly rejects both/neither for PlantID/NewPlant. Verified against API and service tests.
  • ACL chain: CreateFromPacketvisiblePlantCreateSeedLot re-checks the plant, which is redundant but safe; built-in plants are allowed per existing CreateSeedLot comment.
  • Settings resolution: EffectiveVision layers DB→env correctly; Ready() requires both key and model. The UpdateInstanceSettings validation reuses agentmodel.Validate for both specs, which is consistent with the agent path.
  • Migration consistency: instanceSettingsColumns, scanInstanceSettings, and UpdateInstanceSettings all include vision_model in the same order. Migration 0011 is ALTER TABLE ... ADD COLUMN ... DEFAULT '', safe for the single-row table.

No logic bugs, incorrect thresholds, or semantic errors introduced by this change.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found.

The PR is clean through the code-cleanliness lens:

  • Structure: Option + WithPacketExtractor mirrors the existing now injection pattern already used in Service. This is consistent, not novel.
  • Naming: EffectiveVision, PacketConfirm, PacketResult, PacketProposal, scanSeedPacket, createFromPacket, matchPlants, suggestedName, validCategory — all follow the project's terse, domain-first convention.
  • No dead/duplicated code: packetLotRequest is intentionally a near-duplicate of seedLotCreateRequest with plantId removed, and the code comment at internal/api/seed_packet.go:80-83 explains exactly why the reuse is avoided. EffectiveVision mirrors EffectiveAgent correctly.
  • Function length: ExtractSeedPacket and CreateFromPacket are short and single-purpose. matchPlants is readable with a clear local scored helper.
  • Tests: packetEngine, visionCfg, pngUpload, doMultipart are well-scoped helpers that follow the existing test patterns (packetEngine mirrors how other test files build a test engine).

Nothing in my lane to report.

Performance — No material issues found

No material issues found

Reviewed through the Performance lens. I checked:

  • Upload/memory bounds: scanUploadLimit (30 MiB) caps the multipart body; imagenorm.Normalize adds its own DefaultMaxBytes (25 MiB), maxDecodePixels (50 MP), and maxDimension (50 k px) guards, plus a recover around third-party decoders. Peak allocation for one request is bounded (~input + decoded + downscaled + output), and the handler only ever processes one image at a time per request.
  • Catalog matching: ListPlantsForActor is hard-capped at 5 000 rows (maxPlantsListed). matchPlants is O(n log n) on that bounded set with only lightweight string ops — no quadratic behavior.
  • Vision call: The model extraction (majordomo.Generate) is the obvious latency dominant; it correctly receives the request context so it respects cancellation. No work is duplicated before/after it.
  • Creation path: CreateFromPacket does a strict two-step (plant then lot), no loops, no N+1 queries, no unbounded growth.

Everything that could be unbounded is capped, there are no allocations inside loops that scale with data size beyond the small bounded plant list, and no obvious double-buffering of the image bytes. Nothing in this lane.

🧯 Error handling & edge cases — Blocking issues found

Blocking issues found

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

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

  • 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.

Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 5 models **Verdict: Blocking issues found** · 15 findings (5 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🔴 | EffectiveVision DB error silently swallowed in capabilities endpoint | `internal/api/api.go:213` | 5/5 | error-handling, performance | | 🔴 | 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 | `internal/api/seed_packet.go:36` | 3/5 | error-handling | | 🟡 | packetLotRequest and its toInput() duplicate seedLotCreateRequest field-for-field | `internal/api/seed_packet.go:83` | 3/5 | maintainability | | 🟠 | validCategory duplicates the plantCategories enum map; two sources of truth for the same category set | `internal/service/seed_packet.go:88` | 2/5 | maintainability | | ⚪ | format return value discarded speculatively ("available for logging if wanted") | `internal/api/seed_packet.go:63` | 2/5 | maintainability | <details><summary>10 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | file.Open() server I/O error returned as 400 INVALID_INPUT | `internal/api/seed_packet.go:44` | opencode/kimi-k2.6:cloud | error-handling | | 🟠 | imagenorm I/O errors misclassified as 400 INVALID_INPUT | `internal/api/seed_packet.go:54` | opencode/kimi-k2.6:cloud | error-handling | | 🟠 | extractVia is a hand-maintained copy of Extract's body; prompt/message drift won't be caught by the hermetic test | `internal/vision/vision_test.go:30` | opencode/glm-5.2:cloud | maintainability | | 🟡 | packetLotRequest duplicates seedLotCreateRequest fields and toInput logic | `internal/api/seed_packet.go:89` | kimi-k2.6:cloud | maintainability | | 🟡 | settingsPayload reads the same instance_settings row twice (EffectiveAgent + EffectiveVision) | `internal/api/settings.go:67` | glm-5.2:cloud | performance | | 🟡 | EffectiveVision duplicates EffectiveAgent's structure and redundant settings-row fetch | `internal/service/instance_settings.go:131` | claude-code/sonnet | maintainability | | 🟡 | Existing-plant confirm path issues a redundant second GetPlant query (visiblePlant called again inside CreateSeedLot) | `internal/service/seed_packet.go:203` | opencode/glm-5.2:cloud | performance | | 🟡 | CreateFromPacket returns nil result on lot failure even when a plant was created, discarding the plant ID the caller needs to retry the lot | `internal/service/seed_packet.go:214` | opencode/glm-5.2:cloud | error-handling | | 🟡 | extractVia manually mirrors Extract's majordomo call, creating a maintenance trap | `internal/vision/vision_test.go:36` | kimi-k2.6:cloud | maintainability | | ⚪ | GET/PATCH /settings now reads the single instance_settings row three times (GetInstanceSettings + EffectiveAgent + EffectiveVision) | `internal/api/settings.go:71` | opencode/glm-5.2:cloud | performance | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **VERDICT: No material issues found** I checked this PR specifically for authn/authz gaps, injection, SSRF, unsafe deserialization, secret handling, and unsafe handling of untrusted (uploaded) data, verifying against the actual checked-out code, not just the diff: - **Auth on new routes**: `seedLots.POST("/scan", ...)` and `.../from-packet` sit inside `v1.Group("/seed-lots", h.requireAuth())` (`internal/api/api.go:182-191`), and CSRF protection (`h.csrfGuard()`) is applied at the `v1` group level (`internal/api/api.go:57`), so both new endpoints inherit it. The PR's own tests confirm anonymous → 401 for both endpoints, and I confirmed `requireAuth`/`csrfGuard` wrap the whole `/api/v1` tree. - **Catalog-match scoping**: `ExtractSeedPacket` → `matchPlants` uses `store.ListPlantsForActor` (`internal/store/plants.go:80-105`), which is correctly scoped to built-ins (`owner_id IS NULL`) plus the caller's own rows — no cross-user plant/candidate leakage. - **Plant attach authorization**: `CreateFromPacket`'s existing-plant path goes through `s.visiblePlant` (`internal/service/plantings.go:217-229`), which rejects another user's plant as `ErrInvalidInput` (masked, not leaked), and `CreateSeedLot` independently re-checks `visiblePlant` too (`internal/service/seed_lots.go:152-155`) — defense in depth, no bypass. - **Secrets**: `EffectiveVision` sources `APIKey` only from `s.cfg.Agent.OllamaCloudAPIKey` (env), never from the DB row (`internal/service/instance_settings.go`), and the settings payload only exposes `VisionModel`/`VisionReady`, never the key — consistent with the existing agent-key invariant. - **Untrusted upload handling**: `scanSeedPacket` sets `http.MaxBytesReader` before `c.FormFile`, and hands the bytes to `imagenorm.Normalize`, which (pre-existing, unchanged here) bounds input bytes, checks decoded dimensions/pixel count before allocating a bitmap (decompression-bomb guard), and recovers from decoder panics. The uploaded filename is never used for storage/paths, ruling out path traversal. - **Model-spec validation isn't SSRF-prone**: `agentmodel.Validate`/`Resolve` only parses against a registry with a single hardcoded provider (`ollama-cloud`); it never makes a live network call, so a spec (including the new `VisionModel` setting, admin-only via `requireAdmin`) can't be used to probe arbitrary hosts. - **Error responses**: `writeServiceError` never echoes raw error text to the client (unhandled errors become generic 500s, logged server-side only), so a vision-provider failure can't leak internals. Nothing in this diff introduces a new injection point (all SQL is parameterized, JSON binding only), and the one/exactly-one plant-choice validation in `CreateFromPacket` is enforced server-side, not just client-side. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> VERDICT: No material issues found I traced the seed-packet capture backend end-to-end against the checked-out code (not just the diff) for logic/domain-correctness bugs: - **Catalog matching (`internal/service/seed_packet.go` `matchPlants`/`wordIn`)** — verified rank ordering (0=exact name, 1=variety-in-name, 2=same-species), `sort.SliceStable` ascending, and the token-based `wordIn` word-boundary check against the accompanying tests (`TestMatchPlants`). Ranking and dedup (`seen[pl.ID]`) behave as intended. - **Unit conversion in the vision prompt (`internal/vision/vision.go`)** — "1 in = 2.54 cm" is the correct conversion factor; spacing field is documented/converted correctly, no unit-mixing between the *dimension* and *spacing* scales CLAUDE.md warns about. - **Category enum alignment** — `SeedPacket.Category` enum tag (`vegetable,herb,flower,fruit,tree_shrub,cover`) matches `domain.Category*` constants exactly (verified via grep in `internal/domain/domain.go`). - **`CreateFromPacket` XOR logic (`internal/service/seed_packet.go`)** — `hasID == hasNew` correctly rejects both-set and neither-set, verified against `TestCreateFromPacketExactlyOne`. - **Settings plumbing** — `instanceSettingsColumns`/`scanInstanceSettings`/`UpdateInstanceSettings` SQL column order matches the `Scan(...)` and bind-arg order exactly after adding `vision_model`; migration 0011 is a straightforward additive `ALTER TABLE`. - **`EffectiveVision`/`EffectiveAgent` symmetry** — new `EffectiveVision` mirrors the established `EffectiveAgent` DB-over-env resolution pattern correctly. - **`service.New(..., opts ...Option)`** — variadic change is backward compatible; confirmed the sole other call site (`cmd/pansy/main.go:55`) still compiles unmodified. - **`SeedLotInput`/`packetLotRequest` wiring** — confirmed `CreateFromPacket` sets `lotIn.PlantID` before calling `CreateSeedLot`, which independently re-validates visibility via `visiblePlant`; `PacketResult.Lot.PlantID` matches the created/attached plant in both new- and existing-plant paths (matches test assertions). One doc-only nit (not reported as a finding since it's not a behavioral bug): the comment above `visiblePlant` in `CreateFromPacket` (`internal/service/seed_packet.go`, "a plant they can't see is ErrNotFound") is wrong — `visiblePlant` actually returns `domain.ErrInvalidInput` (confirmed at `internal/service/plantings.go:217-229`), consistent with how `CreateSeedLot` already uses it. Behavior is correct and consistent with the rest of the codebase; only the comment is stale. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All three findings in the draft check out against the actual code. Line numbers needed minor correction (draft was off by a few lines in two spots), but each finding is substantively accurate. ## Review — Code cleanliness & maintainability **VERDICT: Minor issues** - **`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 `settingsPayload` (lines 63-64) explicitly acknowledges this redundancy already existed for `EffectiveAgent`; `EffectiveVision` copies rather than fixes it. Not blocking — it mirrors an existing pattern — but a second copy makes it more likely to drift. - **`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 types embed, with `seedLotCreateRequest` adding `PlantID`. As written, any future lot field addition/rename must be made correctly in two structs and two `toInput()` bodies. - **`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. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> ## Review — ⚡ Performance lens **VERDICT: No material issues found** Walked the new code paths for the usual suspects: - `matchPlants` (`internal/service/seed_packet.go:113-160`) does a linear scan over `ListPlantsForActor`, but that's the same helper and pattern already used by the pre-existing `FindPlants` (`internal/service/plants.go:102`) — capped at `maxPlantsListed = 5000` and documented as household-scale. Not a regression. - `scanSeedPacket` bounds the upload (`scanUploadLimit = 30<<20`) before `imagenorm.Normalize`, which itself caps decoded pixels at 50M and bytes at 25 MiB (pre-existing from #80, unchanged here) — no decompression-bomb or unbounded-allocation risk introduced. - `CreateFromPacket` does two sequential single-row creates (`CreatePlant` then `CreateSeedLot`), no loop, no N+1. - `settingsPayload` (`internal/api/settings.go:66-86`) now issues a third read of the single-row `instance_settings` table per `GET/PATCH /settings` (on top of the pre-existing double-read the code comment already flags as acceptable). This is a single-row SQLite query on a low-frequency, admin-only endpoint — not worth flagging as a material regression. - `/capabilities` now does a DB read (`EffectiveVision` → `GetInstanceSettings`) per call, but the frontend query has `staleTime: 60_000` and isn't polled on an interval, so call volume stays low. Nothing here rises to a real efficiency regression — extraction is an explicit, user-initiated, one-shot call (not a loop), and all the new list/scan operations reuse existing bounded patterns. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary> **VERDICT: Blocking issues found** - `internal/api/seed_packet.go:26-36` — `scanSeedPacket` extends only the *read* deadline (`SetReadDeadline`, 60s) for the upload, never the *write* deadline. `cmd/pansy/main.go:65` sets `http.Server.WriteTimeout: 30 * time.Second`, which per Go's documented semantics (and per this same codebase's own explanation at `internal/api/agent.go:154-166`, written for the identical #78 bug) is reset once at request-header-read time, not extended by a slow body read. So a legitimately slow phone upload that takes 30-60s (exactly the case `scanReadTimeout` exists for) can blow past the server's absolute write deadline before `c.JSON(http.StatusOK, prop)` runs — per `agent.go:162-165`, a write past that deadline returns `err == nil` with bytes silently dropped, so a successful extraction still gets delivered to the client as a dropped connection. The SSE path fixes this exact class with a per-write `SetWriteDeadline` (`agent.go:176,199`); `scanSeedPacket` has no analogous call anywhere. - `internal/api/seed_packet.go:38-43` — `c.Request.Body` is wrapped in `http.MaxBytesReader(..., scanUploadLimit)` (30 MiB) before `c.FormFile("image")` parses the multipart body. `imagenorm.Normalize` separately caps the *decoded* input at 25 MiB (`internal/imagenorm/imagenorm.go:59`, read via its own `io.LimitReader`), so files between 25-30 MiB are correctly rejected by `imagenorm.ErrTooLarge` → 413. But a file that pushes the raw multipart envelope over 30 MiB fails during `ParseMultipartForm` itself (invoked internally by `c.FormFile`), which returns the `MaxBytesReader` error — and the handler maps *any* `FormFile` error, including this one, to `400 INVALID_INPUT "attach an image..."` (line 40-43), not `413`. A large-but-attached image is misreported as "no image attached." - `internal/api/api.go:210-219` — `capabilities` swallows an error from `h.svc.EffectiveVision(ctx)` with no logging at all, silently downgrading to `vision: false`. This is inconsistent with the same call in `settingsPayload` (`internal/api/settings.go:66-74`), which propagates the identical error as a real failure, and with this codebase's established convention for best-effort reads that must not fail the whole response — e.g. `internal/api/agent_holder.go:68`: `slog.Error("api: could not resolve agent settings; leaving the assistant as-is", "error", err)`. A persistent DB fault here makes `/capabilities` permanently and silently advertise `vision: false` with no operator-visible signal. </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Verified. The image path is well-hardened. Final review below. No material issues found I reviewed the seed-packet capture backend through the security lens — authentication, authorization, injection/SSF, untrusted-image handling, secret handling, and input validation — and verified each against the checked-out code. - **Authn on both new endpoints** (verified, `internal/api/api.go:182-191`): `/seed-lots/scan` and `/seed-lots/from-packet` are registered on the `seedLots` group, which is behind `requireAuth()`. The anonymous test cases (401) confirm this. - **Authz on `CreateFromPacket`** (verified, `internal/service/seed_packet.go:181-201`, `internal/service/plantings.go:217-229`, `internal/service/seed_lots.go:152-155`): when a user supplies `PlantID`, `visiblePlant` enforces that the plant is a built-in (`OwnerID == nil`) or owned by the actor — someone else's plant returns `ErrInvalidInput`, preventing IDOR. `CreateSeedLot` then re-checks the plant ownership even after `CreateFromPacket` sets `lotIn.PlantID`, so the ownership check can't be bypassed by a hand-crafted lot body. `CreatePlant` runs as the actor for the new-plant path. - **Secret handling** (verified): the API key is never stored in the DB or echoed in responses. `EffectiveVision` (`internal/service/instance_settings.go:135-142`) pulls the key only from `cfg.Agent.OllamaCloudAPIKey` (env); the settings row carries only the model name. `/capabilities`, `/settings`, and the scan response surface only `vision`/`visionModel`/`visionReady` booleans and the model string — no key. The admin-supplied `VisionModel` is validated via `agentmodel.Validate` before storage (`instance_settings.go:65-71`), gated behind `requireAdmin`. - **SSRF / model-spec injection** (verified, `internal/agentmodel/agentmodel.go:27-46`): the model spec is parsed through majordomo's registry against the single registered `ollama.Cloud` provider, so a spec must match a known provider token — it's not an arbitrary URL the server fetches. The spec is also admin-controlled, further reducing risk. - **Untrusted-image handling** (verified, `internal/imagenorm/imagenorm.go:130-189`): the upload path bounds the body (`scanUploadLimit` 30 MiB at `internal/api/seed_packet.go:27`), then `imagenorm.Normalize` caps the input read (`MaxBytes` 25 MiB), checks dimensions **before** full decode (`maxDimension` per-side, `maxDecodePixels` with an overflow-safe `int64` multiply), and `decodeSafely` recovers from third-party (HEIC/WebP) decoder panics so a malformed upload fails one request instead of crashing the process. These are concrete decompression-bomb defenses; nothing in the diff weakens them. - **Input validation**: empty image is rejected in both `scanSeedPacket` (via `imagenorm`) and `ExtractSeedPacket` (`len(jpeg) == 0` → `ErrInvalidInput`). The "exactly one of plantId/newPlant" rule is enforced in `CreateFromPacket` (`hasID == hasNew` → `ErrInvalidInput`). Nothing in my lens is materially wrong. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **VERDICT: No material issues found** Through the correctness lens I verified the following by reading the checked-out code: - **`matchPlants` ranking** (`internal/service/seed_packet.go:106-143`): the rank assignment (exact name=0, variety-in-name=1, same-species=2) plus `sort.SliceStable` produces best-first ordering. Confirmed the test's "Music Garlic (rank 1) before Garlic (rank 2)" expectation holds given catalog insertion order, and the `seen` map correctly dedupes a plant that could match multiple arms (the `switch` falls through only one arm per plant anyway, so dedup is belt-and-suspenders, not a bug). `wordIn` is token-based as documented. - **`CreateFromPacket` exactly-one** (`seed_packet.go:186-190`): `hasID == hasNew` correctly rejects both-set and neither-set. The lot's `PlantID` is overwritten from `res.Plant.ID` (line 211), so the API's `packetLotRequest.toInput()` legitimately omitting `PlantID` is correct — the service attributes it. Verified `SeedLotInput.PlantID` is an `int64` (not pointer) so the zero value is harmless before assignment. - **`visiblePlant` / `CreateSeedLot` reuse** (`seed_lots.go:152`, `plantings.go:217`): `CreateFromPacket` reuses the same ACL-enforcing path the direct endpoints use; a non-existent/foreign `PlantID` yields `ErrNotFound`, consistent with the rest of the codebase. - **`EffectiveVision` precedence** (`instance_settings.go:138-145`): DB `VisionModel` overrides env, key always from env — matches the documented Settings→env contract and the agent analogue. - **`validCategory`** (`seed_packet.go:88-96`): switches on `domain.Category*` constants (not raw strings), so it stays aligned with the domain's canonical values regardless of the enum tag spelling in `vision.SeedPacket`; the extracted category is only used as a prefill suggestion, so a mismatch degrades to `""` rather than creating bad data. No logic bug, mis-constant, or semantic correctness defect surfaced in this lens. The conversion factor in the extraction prompt (1 in = 2.54 cm) is correct, and the pointer/nullable numeric fields behave as designed (verified via the hermetic `TestExtractLeavesMissingFieldsNil`). </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All three findings verified against the source. 1. `validCategory` (seed_packet.go:88-96) re-lists the same six `domain.Category*` constants that `plantCategories` (plants.go:27-34) already enumerates — confirmed duplication of the enum source of truth. 2. `packetLotRequest` (seed_packet.go:83-95) is a field-for-field copy of `seedLotCreateRequest` (seed_lots.go:20-33) minus `PlantID`, with a parallel `toInput()` — confirmed verbatim duplication. 3. `_ = format` is at line 63 (not 52 as in the draft JSON); confirmed it's assigned then discarded with a "logging if wanted" comment. VERDICT: Minor issues - `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 string) string { if _, ok := plantCategories[c]; ok { return c }; return "" }` — one enum, two uses. - `internal/api/seed_packet.go:83-103` — `packetLotRequest` is a field-for-field copy of `seedLotCreateRequest` (`internal/api/seed_lots.go:20-42`) minus `PlantID` and its `binding:"required"`, with an identical `toInput()` body minus the `PlantID` line. The comment justifies *not reusing* the struct, but the lot fields are still duplicated verbatim, so any new lot column must be added to both request types and both `toInput()` methods. A small shared `lotFields` struct embedded by both would keep one definition. At minimum, a "// keep in sync with seedLotCreateRequest" note would flag the duplication. - `internal/api/seed_packet.go:63` — `_ = format` assigns then discards the `format` return from `imagenorm.Normalize` with a "available for logging if wanted" comment. If genuinely unused, the current form reads as leftover scaffolding. Trivial; drop it or actually log it. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> **VERDICT: Minor issues** - `internal/api/api.go:214-217` — `/capabilities` previously was a single atomic pointer load (`h.agent.get()`) with no I/O; this PR adds a per-request DB read (`svc.EffectiveVision` → `store.GetInstanceSettings`, a `SELECT ... FROM instance_settings WHERE id=1`). This is the endpoint the UI polls on every garden-editor page load and on focus/refetch. The comment above it (`api.go:206-209`) frames it as tracking the live Runner "on the very next poll," inviting repeated hits. At household scale it's cheap, but it is a real I/O regression on a path that was deliberately allocation-free. Consider caching vision readiness on the `agentHolder` (or a parallel atomic) the way the agent Runner is, so `/capabilities` stays a pointer read and settings changes swap it atomically, instead of issuing a SQLite query per request. Verified: `EffectiveVision` (`internal/service/instance_settings.go:131`) calls `s.store.GetInstanceSettings(ctx)`. - `internal/api/settings.go:67-74` — `settingsPayload` calls `EffectiveAgent` and then `EffectiveVision`, each of which independently runs `store.GetInstanceSettings` (the same `SELECT` against the same single row). That's two identical reads to build one response. `getSettings` already holds the fetched row `st` (`settings.go:88-89`) and passes it into `settingsPayload`, so `EffectiveVision` could accept the already-fetched `*domain.InstanceSettings` to avoid the duplicate query. Verified: `EffectiveAgent` (`instance_settings.go:98`) and `EffectiveVision` (`instance_settings.go:131`) both call `s.store.GetInstanceSettings(ctx)`. Minor, but a pure duplicate query on the settings endpoint. Neither is a hot-loop/N+1/quadratic issue; both are avoidable extra DB reads on request paths. No allocation/quadratic/unbounded-growth problems in `matchPlants` or the upload path (bounded by `scanUploadLimit` and `imagenorm`). </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both findings verified against the actual source. - `seed_packet.go:38-43` confirms `MaxBytesReader` is set on the body, `c.FormFile` errors are all mapped to 400 `INVALID_INPUT` with no `*http.MaxBytesError` check, while 413 only fires from `imagenorm.ErrTooLarge` at line 56-58. Finding stands. - `api.go:210-218` confirms `EffectiveVision` errors are discarded with no log, degrading silently to `vision: false`. Finding stands. **Verdict: Minor issues** - **`internal/api/seed_packet.go:38-43` — oversized multipart upload returns 400 instead of the promised 413.** The handler wraps the body with `http.MaxBytesReader(..., scanUploadLimit)` (30 MiB) and then calls `c.FormFile("image")`. When the upload exceeds 30 MiB, `MaxBytesReader` fails the read with a `*http.MaxBytesError`, which `c.FormFile` surfaces. The handler maps **every** `FormFile` error to `400 INVALID_INPUT` / "attach an image as the image field" with no `errors.As(err, *http.MaxBytesError)` check. The PR description promises "maps too-large/unreadable to clear statuses (413 / 400)", but in practice the 413 only ever fires from `imagenorm.ErrTooLarge` (the 25 MiB *decoded* cap at line 56), not from the 30 MiB *envelope* cap the handler itself sets — so a >30 MiB upload gets a misleading 400 "attach an image" instead of 413. Suggested fix: before the generic 400, check `var maxErr *http.MaxBytesError; errors.As(err, &maxErr)` and emit `http.StatusRequestEntityTooLarge` with an `IMAGE_TOO_LARGE` code. - **`internal/api/api.go:213-216` — `/capabilities` silently swallows `EffectiveVision` errors as `vision:false`.** If `GetInstanceSettings` fails (e.g. DB transient), the probe degrades to "feature off" with no log. This is a defensible best-effort choice for a capability endpoint, but the error is discarded entirely; a `slog` line would make a real outage diagnosable rather than looking like an unconfigured instance. Minor. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **Verdict:** No material issues found - The `/seed-lots/scan` and `/seed-lots/from-packet` endpoints both sit behind `requireAuth` (`internal/api/api.go:182`), and the service layer enforces plant visibility via `visiblePlant` (existing plants) and `CreatePlant` (new plants get the actor's owner_id). - The vision model API key stays in the environment only — the DB stores only the model spec, confirmed by both the migration (`0011_vision_model.sql` comments) and the README ("the API key is intentionally not among the runtime-editable settings"). - Image upload has layered DoS protection: `MaxBytesReader` at 30 MiB, `imagenorm.Normalize` with a 25 MiB read cap, 50 MP pixel-count ceiling, 50k dimension ceiling, and `recover()` around third-party decoders (`decodeSafely`). - The vision prompt is a fixed constant string; no user-controlled text is interpolated, so prompt injection is not possible. - Model output is unmarshaled into a typed `SeedPacket` struct with pointer fields for nullability, rather than being passed into any dynamic execution or query building. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** After examining the diff and verifying key cross-file contracts (domain constants, struct tags, API precedence rules, migration schemas, and the imagenorm integration), the logic holds up: - **Catalog matching** (`internal/service/seed_packet.go:106-154`): The rank-based deduplication with `seen` correctly surfaces the best match only, and `wordIn` behaves as documented for space-delimited tokens. - **XOR validation** (`internal/service/seed_packet.go:187-189`): `hasID == hasNew` correctly rejects both-nil and both-present confirms. - **Category mapping** (`internal/service/seed_packet.go:88-96`): `validCategory` checks against the exact domain constants, which match the vision-model enum (`tree_shrub` ↔ `domain.CategoryTreeShrub`). - **Precedence chain** (`internal/service/instance_settings.go:128-140`): `EffectiveVision` correctly layers DB-over-env for the model, with the key always from env. - **Migration 0011** (`internal/store/migrations/0011_vision_model.sql`): Adds `vision_model TEXT NOT NULL DEFAULT ''`, consistent with the domain struct and the empty-string-as-inherit convention. - **ACL enforcement** (`internal/service/seed_packet.go:203`): `visiblePlant` gates the existing-plant path. The conversion factor in the vision prompt (`1 in = 2.54 cm`) is correct, and the hermetic test path via `WithPacketExtractor` is sound. Nothing in this PR introduces a logic bug or semantic domain error. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - `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. - `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`. </details> <details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed ollama-cloud/kimi-k2.6:cloud: model returned an empty response </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Minor issues** - **`capabilities` silently swallows `EffectiveVision` DB errors.** `internal/api/api.go:214-217` If `EffectiveVision` returns an error (e.g., the DB is temporarily unreachable), the endpoint returns `vision: false` with HTTP 200 and logs nothing. An operator cannot distinguish “vision not configured” from “vision broken,” and the UI will hide the scan button during a transient outage. The error should at least be logged. </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> VERDICT: No material issues found I reviewed the change through the security lens (authn/authz, injection, SSRF, secret leakage, input validation, untrusted-data handling) and verified the relevant code paths by reading them. - **Authn/authz on new routes** — both `POST /seed-lots/scan` and `POST /seed-lots/from-packet` are registered under the `seedLots` group, which sits behind `h.requireAuth()` (api.go:182, 190-191). The anonymous-401 cases are explicitly tested. `scan` is read-only by design. `CreateFromPacket` delegates to `CreatePlant`, `visiblePlant`, and `CreateSeedLot`, each of which enforces ownership (verified at plants.go:194-211, plantings.go:217-229, seed_lots.go:152-155). No new authz seam was introduced. - **Exactly-one confirm** — `CreateFromPacket` enforces `hasID == hasNew → ErrInvalidInput` (seed_packet.go:185-187), so an ambiguous confirm cannot silently pick a plant. Verified. - **No SQL/command injection** — `matchPlants` is pure in-memory string matching over an already-loaded `[]domain.Plant`; it never builds SQL. All persistence goes through existing parameterized store methods. The user-supplied `NewPlant`/`Lot` fields flow through `finalizePlant`/`finalizeSeedLot` validation, not raw into SQL. - **No SSRF** — the image is uploaded by the user (`c.FormFile`), not fetched from a URL; the filename is never used. `SourceURL` is stored, never retrieved. - **Secret handling** — the API key stays in the env and is never stored in the DB or serialized. `/settings` (admin-only via `requireAdmin()`, settings.go:158) reports only `visionModel` (the model name) and `visionReady` (a bool); the key is not leaked. `EffectiveVision` (instance_settings.go:117-141) pulls the key from `cfg.Agent.OllamaCloudAPIKey` and exposes only `Ready()`. Verified. - **Vision-model spec is admin-controlled** — `visionModel` only changes through the admin-gated `UpdateInstanceSettings`, validated via `agentmodel.Validate` (instance_settings.go:63-71). The `/scan` request body never supplies a model spec; it comes from settings/env. No user-controlled model-injection path. - **Upload DoS bounds** — `http.MaxBytesReader(..., 30<<20)` (seed_packet.go:53) bounds the multipart body, and `imagenorm.Normalize` independently caps decoded bytes/pixels and recovers from decoder panics (imagenorm.go:130-157). `ErrTooLarge` → 413, non-image → 400. Verified. - **Untrusted image to third party** — the JPEG is sent to the configured vision provider. This is the explicit, authenticated, user-initiated feature; the bytes cannot call tools (`majordomo.Generate[T]` is one-shot structured extraction), so they cannot touch the garden. No injection surface there. - **`/capabilities` is unauthenticated** and now also reports `vision` bool — this is consistent with the pre-existing `agent` bool (api.go:62, registered without `requireAuth`), and discloses only feature availability, not secrets. Not a new disclosure class, so not flagged. No security findings to report. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I'll investigate this PR through the Correctness lens, verifying against the actual code. Now let me check the `agentmodel.Resolve` function and `image/jpeg` import shadowing in the scan handler. Now let me check the `ListPlantsForActor` semantics and the variable shadowing concern with `jpeg` in the scan handler. Now I'll examine a potential edge case in `matchPlants` — the "exact name" match only compares variety against the plant name, not the species. Let me also check the `effectiveView` settings test and look at whether the `seedLots` routes ordering matters (e.g., does `POST /scan` conflict with `POST /:id` routes). Now let me check the `matchPlants` exact-name logic more carefully. The "exact name" case matches `name == variety`, but a plant named "Garlic" with a packet of species "garlic" and no variety won't match as exact (variety empty → skipped), but will match via species word boundary. Let me also verify the `effectiveView` JSON field naming is consistent and check the `Quantity` zero-value issue (a lot with `quantity: 0` from the packet — `packetLotRequest.Quantity` is non-pointer float64). Now let me look at the `EffectiveVision` precedence — does an empty DB `VisionModel` ("") override the env value? The code says: `if st.VisionModel != "" { eff.Model = st.VisionModel }`, so an empty DB value falls through to env. Good. But what does the settings *update* path do — does it store empty as empty? Let me check the store update to confirm the trim behavior and check whether there's a way to "clear" a vision model back to env (store empty string). Now let me examine a key semantic concern: the `matchPlants` "exact name" case matches `name == variety`, but plants are typically named with the variety AND species (e.g., "Music Garlic"), while the packet's `variety` is just "Music". So "exact name" will rarely match unless someone named their plant exactly "Music". The test confirms this — "Cherokee Purple" matches a plant named "Cherokee Purple". This is by design. Let me verify the `seedLots.POST("/:id")` doesn't conflict — POST `/scan` could match `/:id` if gin routes `scan` as an id. Let me check gin's behavior — static routes take precedence over params in gin, so `/scan` registered before/after `/:id` is fine. Actually in gin, static routes win. Let me verify there's no `POST /:id` though — there isn't (only GET/PATCH/DELETE `/:id`). So `POST /scan` and `POST /from-packet` are the only POSTs besides `POST ""`. Good, no conflict. Now let me check a subtle correctness issue: the `scanSeedPacket` handler calls `ExtractSeedPacket` which returns `domain.ErrInvalidInput` for both "empty jpeg" AND "vision not ready". The handler maps ANY `ErrInvalidInput` to 503 VISION_DISABLED. But `ExtractSeedPacket` could also return `ErrInvalidInput` from... actually no, the only `ErrInvalidInput` paths in `ExtractSeedPacket` are the two explicit ones (empty jpeg, not ready). The jpeg can't be empty here (imagenorm would have failed). So this is fine. But wait — what about `CreateFromPacket`? It returns `ErrInvalidInput` for the exactly-one check, and `writeServiceError` maps that to 400 INVALID_INPUT. That's correct. But `CreatePlant` and `CreateSeedLot` can also return `ErrInvalidInput` (validation failures), which map to 400. Fine. Let me check one more thing — the `packetLotRequest.Quantity` is a non-pointer `float64`, so a missing `quantity` in the JSON defaults to 0. `finalizeSeedLot` allows `Quantity >= 0`, so 0 passes. Is a zero-quantity lot semantically wrong? Let me check the regular `seedLotCreateRequest` — it also has `Quantity float64` (non-pointer). So this is consistent with existing behavior, not a regression. Now let me check one important semantic concern. The `vision_test.go` uses `extractVia` which mirrors `Extract`'s body but uses a test-supplied registry/model. But the actual `Extract` function calls `agentmodel.Resolve` which builds a *new* registry each time and does NOT register the fake provider. So the test for the actual `Extract` function (TestExtractRejectsEmptyImage) only tests the empty-image guard, not the model resolution path. The schema/image verification in TestExtractParsesModelJSON uses `extractVia`, not `Extract`. This is a test gap but the PR description acknowledges this is the majordomo fake pattern. Let me check whether `majordomo.Generate` actually uses the schema derived from struct tags — the test asserts `call.Request.SchemaName != "seedpacket"`. Let me verify majordomo's `Generate` derives the schema name from the type name. majordomo is an external dependency (not checked out here), so I can't verify `Generate`'s schema derivation directly. But the test asserts the schema name is `"seedpacket"` (lowercase type name). This is a test assertion, not a correctness issue in the production code. If majordomo's `Generate` derives the schema name differently, the test would fail — but that's a test issue, not a production correctness issue. I'll leave this alone as I can't verify the external dep. Now let me look at the most important semantic concern: **the `image/jpeg` import shadowing**. In `seed_packet.go`, the handler declares `jpeg, format, err := imagenorm.Normalize(...)`. This creates a local variable named `jpeg`. There's no `image/jpeg` import in `seed_packet.go`, so no shadowing issue — the variable just holds the normalized JPEG bytes. That's fine. Let me look at one more potential issue — in the scan handler, after `MaxBytesReader` triggers (body too large), `c.FormFile` returns an error. But `MaxBytesReader` returns a `*http.MaxBytesError`, not a regular error. The handler maps ALL `FormFile` errors to 400 INVALID_INPUT. This means a too-large upload via `MaxBytesReader` returns 400 instead of 413. Let me verify whether this matters. Now let me check the `scanUploadLimit` vs `imagenorm` byte cap. The handler sets `MaxBytesReader` to 30 MiB, but `imagenorm.Normalize` with default `Options{}` uses `DefaultMaxBytes`. Let me check that value. Now I see an important interaction. `MaxBytesReader` is set to 30 MiB, but `imagenorm.Normalize` with default `Options{}` caps at 25 MiB. So: 1. If the multipart body is 26 MiB, `MaxBytesReader` allows it through (under 30), but `imagenorm` reads only 25 MiB + 1 byte and returns `ErrTooLarge`. This maps to 413. Good — but the handler comment says "this is a little over that for the multipart envelope", which is correct reasoning. 2. But there's a subtlety: `MaxBytesReader` wraps `c.Request.Body`. When `c.FormFile("image")` is called, gin reads the entire multipart body to parse it (including the file content into a temp file/memory). If the body exceeds 30 MiB, `MaxBytesReader` triggers during the `FormFile` read, and `c.FormFile` returns an error → 400 INVALID_INPUT. So a 31 MiB upload gets 400, not 413. The handler distinguishes 413 only for the *decoded* image being too large (via imagenorm), not the raw upload being too large. This is a minor UX inconsistency (31 MiB upload → 400 instead of 413), but it's not a logic bug — both are rejections. The comment says the limit is "a little over" the 25 MiB imagenorm cap to account for the multipart envelope, which is the right design. A 31 MiB raw upload exceeding both is an edge case. Not a correctness bug. Now let me check the most semantically interesting thing: the `matchPlants` exact-name match. When `variety` is "Music" and a plant is named "music" (case difference), `name == variety` after lowercasing both → "music" == "music" → exact match. Good. But what if a plant is named "Music Garlic" and variety is "music garlic"? Then `name == variety` → "music garlic" == "music garlic" → exact. But "music garlic" is really the full name, not a variety. The model would return variety="Music" and species="garlic" typically, not variety="Music Garlic". So this edge case is unlikely from the model. Not a real bug. Let me verify one final concern: the `CreateFromPacket` partial-failure semantics. If `CreatePlant` succeeds but `CreateSeedLot` fails, the plant is left behind (documented). The caller gets an error and `res` is nil (because the function returns `nil, err`). So the caller doesn't get the plant ID to retry against. The comment says "The caller sees the error and can retry the lot against the now-existing plant" — but the caller doesn't know the plant ID from the error response. They'd have to search the catalog. This is a minor UX issue, not a correctness bug in the logic itself. The plant IS created and editable, which is the stated tradeoff. Let me verify one more thing — does `CreateSeedLot` call `visiblePlant` again (redundantly), since `CreateFromPacket` already called `visiblePlant` for the existing-plant case? Yes, `CreateSeedLot` calls `visiblePlant` again. For the new-plant path, this is necessary (the plant was just created, and `CreateSeedLot` needs to verify it's visible — which it is since the actor owns it). For the existing-plant path, `visiblePlant` is called twice (once in `CreateFromPacket`, once in `CreateSeedLot`). This is redundant but not incorrect — just a minor extra DB query. Not a correctness issue. Now let me check whether `CreatePlant` returns a plant with the ID populated, so `lotIn.PlantID = res.Plant.ID` works for the new-plant path. `CreatePlant` calls `s.store.CreatePlant(ctx, p)` which returns the created plant with ID. Let me verify. Good — `CreatePlant` returns the created plant with ID populated via `RETURNING`. So `res.Plant.ID` is valid for both paths. The `lotIn.PlantID = res.Plant.ID` assignment is correct. Now let me check the `effectiveView` — there's a potential issue. The `settingsPayload` calls `EffectiveAgent` and `EffectiveVision`. `EffectiveAgent` reads the settings row, and `EffectiveVision` reads it again. Two DB reads for one `/settings` GET. Not a correctness bug, just minor inefficiency. Let me check one final semantic concern: the `SeedPacket` struct's `Category` enum tag says `tree_shrub` (with underscore), and `validCategory` checks against `domain.CategoryTreeShrub` which is `"tree_shrub"`. Consistent. And the model is told via the enum tag to use `tree_shrub`. Good. Let me verify the `fromPacketRequest` binding. The `Lot` field has `Unit` with `binding:"required"`. But `ShouldBindJSON` only validates the top-level struct's binding tags by default — does gin recursively validate nested struct binding tags? Now I have a good understanding. Let me verify one final semantic concern about the `matchPlants` "variety in name" check using `strings.Contains`. Consider: variety "Purple" matches plant "Purple Basil" (rank 1, "variety in name") but also plant "Cherokee Purple" (rank 1 too, "variety in name"). Both get rank 1. The test for "Cherokee Purple" with variety "Cherokee Purple" gets rank 0 (exact). But if the model reads variety "Purple" instead, "Cherokee Purple" would be rank 1 (contains) and "Purple Basil" would be rank 1 (contains). Both same rank, stable-sorted by original order. This is by design — the comment says "no fuzzy scoring that could confidently mis-rank", and the ranking is conservative. The user confirms anyway. Let me check one more important thing — does the `packetLotRequest` have a `purchasedAt` as `*string` while `seedLotCreateRequest` also has `*string`? Yes, consistent. And `finalizeSeedLot` validates via `validDatePtr`. Good. Now let me look at a potential real issue: in the `createFromPacket` handler, when `req.NewPlant` is non-nil but the `newPlant` JSON object is missing required fields (like `name`), gin's `ShouldBindJSON` succeeds (JSON parses fine), but then `NewPlant.toInput()` produces a `PlantInput` with empty `Name`. The service `CreatePlant` → `finalizePlant` rejects empty Name → `ErrInvalidInput` → 400. So the "both plant choices" test case at line with `newPlant: {name: "X", ...}` — the test sends a valid newPlant with `name: "X"`, so binding succeeds, and the service rejects it at the `hasID == hasNew` check (both set) before reaching `CreatePlant`. Good. But wait — let me re-examine the "both" test case. The test sends: ```go "plantId": plantID, "newPlant": {name: "X", category: "vegetable", color: "#4a7c3f", icon: "🌱"} ``` Note: no `spacingCm`. `plantCreateRequest.SpacingCM` is `float64` (defaults to 0). The service `CreateFromPacket` checks `hasID == hasNew` → both true → returns `ErrInvalidInput` immediately, BEFORE calling `CreatePlant`. So the missing spacing doesn't matter. Good — the exactly-one check is first. Let me verify there are no issues with the migration. Migration 0011 adds `vision_model TEXT NOT NULL DEFAULT ''` via ALTER TABLE. The seeded row from 0010 has no `vision_model` column, but ALTER TABLE with DEFAULT '' backfills existing rows to ''. So the existing row gets `vision_model = ''`. The `scanInstanceSettings` reads 5 columns now: `agent_model, agent_enabled, vision_model, version, updated_at`. This matches the updated `instanceSettingsColumns`. Good. Let me verify the migration runner handles ALTER TABLE properly and that migrations run in order. Migrations run in sorted order by version number. 0011 runs after 0010. Good. Now let me check one final semantic concern about the `wordIn` function and species matching. The `wordIn` uses `strings.Fields` which splits on whitespace. So "garlic" matches "German Garlic" (token "garlic" after lowercasing). But what about punctuation? A plant named "Garlic, German" → tokens are ["garlic,", "german"] → "garlic" != "garlic," → no match. This is a minor limitation but not a bug — plant names in pansy are user-created and typically don't have commas. The test explicitly tests the word-boundary behavior. Fine. Let me look at whether there's an issue with the `effectiveView` JSON response — the `VisionModel` field exposes the resolved model name to the admin. That's fine (it's not a secret). I've now thoroughly examined the code through the Correctness lens. Let me check one last thing — whether the `extractPacket` field on `Service` could be nil in production (if someone constructs `Service` without using `New`). The `New` function sets `extractPacket: vision.Extract` by default. The `WithPacketExtractor` option overrides it. In production, `New` is always used (confirmed in auth_test.go line 31: `service.New(db, cfg)`). So `extractPacket` is never nil in production. Good. Let me verify the test helpers used by the seed_packet_test are correct. I've now thoroughly reviewed the PR through the Correctness lens. Let me check one more subtle thing — the `TestScanSeedPacketAPI` test creates a plant named "Music Garlic" and scans a packet with variety "Music", species "garlic". The `matchPlants` should find "Music Garlic" as rank 1 ("variety in name": "music garlic" contains "music"). But there are no built-in "Garlic" plants in the test DB (the test DB starts empty except for user-created plants). So only one candidate. The test checks `len(cands) == 0` → expects non-zero. Good. But wait — the test creates a plant "Music Garlic" which is owned by the user. `ListPlantsForActor` returns built-ins + user's own. There are seeded built-in plants from migration 0003. Let me check if any built-in is named with "garlic" or "music". There IS a built-in "Garlic" plant. So in `TestScanSeedPacketAPI`, the packet has species "garlic" and variety "Music". `matchPlants` would match: - "Music Garlic" (user-created) → rank 1 ("variety in name": "music garlic" contains "music") - "Garlic" (built-in) → rank 2 ("same species": word "garlic" in "garlic") So `len(cands)` should be 2, not just 1. The test only checks `len(cands) == 0` → expects non-zero, so 2 passes. But the test comment says "A matching plant so the proposal has a candidate" — it gets 2 candidates, not 1. The test doesn't assert the count is exactly 1, so it passes. Not a bug. But this reveals an important semantic point: the built-in "Garlic" is a candidate for any garlic-variety packet. That's by design — the user might be buying generic Garlic. The user confirms. Good. I've now completed my thorough review. Let me summarize my findings. The code is semantically correct through the Correctness lens. The logic is sound: - `matchPlants` ranking is conservative and correct (exact → contains variety → same species word) - `CreateFromPacket` exactly-one check is correct (`hasID == hasNew` catches both and neither) - `EffectiveVision` precedence (DB → env) is correct - Migration 0011 correctly adds the column with backfill default - Store column list and scan order are consistent - The `extractPacket` injectable field defaults to `vision.Extract` and is never nil in production - Error mapping (ErrInvalidInput → 503 for vision-not-configured) is correct - `visiblePlant` ACL enforcement for existing-plant path is correct - New-plant path correctly attributes the lot via `lotIn.PlantID = res.Plant.ID` The only minor observations (not material bugs): - `CreateSeedLot` redundantly calls `visiblePlant` when `CreateFromPacket` already did (minor extra query, not a bug) - A raw upload exceeding `scanUploadLimit` (30 MiB) gets 400 instead of 413 (edge case, both are rejections) - `EffectiveVision` and `EffectiveAgent` each read the settings row separately in `settingsPayload` (minor inefficiency) None of these are correctness bugs. VERDICT: No material issues found. ## Review **VERDICT: No material issues found** I reviewed the PR through the Correctness lens, verifying against the actual checked-out code. The logic is sound throughout: - **`matchPlants` ranking** (`seed_packet.go:106-143`): exact-name → variety-in-name → same-species-word, case-insensitive, deduped via `seen` map, stable-sorted by rank. Verified the `wordIn` token-boundary logic and the test cases against the built-in "Garlic" plant in migration 0003 — a "Music"/"garlic" packet correctly produces "Music Garlic" (rank 1) and built-in "Garlic" (rank 2) as candidates. - **`CreateFromPacket` exactly-one check** (`seed_packet.go:186-190`): `hasID == hasNew` correctly rejects both-set and neither-set. Verified the new-plant path populates `res.Plant.ID` via `store.CreatePlant`'s `RETURNING` clause (`store/plants.go:124-136`), so `lotIn.PlantID = res.Plant.ID` attributes the lot correctly. - **`EffectiveVision` precedence** (`instance_settings.go:131-143`): empty DB `VisionModel` falls through to env; non-empty overrides. Consistent with `EffectiveAgent`. Verified the store update trims and stores the value (`store/instance_settings.go:56-80`). - **Migration 0011**: `ALTER TABLE ... ADD COLUMN vision_model TEXT NOT NULL DEFAULT ''` backfills the existing single row from 0010 to `''`. `scanInstanceSettings` reads 5 columns matching `instanceSettingsColumns`. Verified column order in the `RETURNING` clause and `Scan` call are aligned. - **`extractPacket` default**: `service.New` sets `extractPacket: vision.Extract` (`service.go:54`), never nil in production; `WithPacketExtractor` overrides for tests. - **Error mapping**: `ErrInvalidInput` from `ExtractSeedPacket` (vision-not-ready or empty jpeg) → 503 VISION_DISABLED in the scan handler; `CreateFromPacket`'s `ErrInvalidInput` → 400 via `writeServiceError`. Verified both paths. - **Category enum consistency**: `SeedPacket.Category` enum tag `tree_shrub` matches `domain.CategoryTreeShrub = "tree_shrub"`, and `validCategory` checks all six domain categories. Verified. - **`visiblePlant` ACL** (`plantings.go:217-229`): existing-plant path in `CreateFromPacket` enforces visibility; `CreateSeedLot` re-checks (redundant but not incorrect). Minor non-material observations: `CreateSeedLot` redundantly calls `visiblePlant` after `CreateFromPacket` already did (one extra DB query); a raw upload exceeding 30 MiB returns 400 from `MaxBytesReader` rather than 413 (both are rejections; only the *decoded* image over 25 MiB gets 413 via `imagenorm.ErrTooLarge`). Neither is a correctness bug. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> I'll verify each of the three findings against the actual code. All three findings verified against the actual source code. Confirming each: 1. **`validCategory`** (seed_packet.go:88-96) — switch hard-codes the six `domain.Category*` constants. `plantCategories` (plants.go:27-34) is the map of the same six. Map is consumed at plants.go:316. Two sources of truth confirmed. 2. **`extractVia`** (vision_test.go:30-37) — body matches `Extract` (vision.go:52-67) exactly: same `majordomo.Generate[SeedPacket]` call, same `extractPrompt`, same `UserParts(Text(...), Image("image/jpeg", jpeg))` message. Only difference is model source. The comment at lines 27-29 admits the duplication. Confirmed. 3. **`packetLotRequest`** (seed_packet.go:83-103) — re-declares every field of `seedLotCreateRequest` (seed_lots.go:20-33) minus `PlantID`, plus its own `toInput()` (seed_packet.go:97-103) mirroring `seedLotCreateRequest.toInput()` (seed_lots.go:35-42) minus the `PlantID:` line. Comment at seed_packet.go:82 documents the rationale. Confirmed. ## VERDICT: Minor issues Within the code-cleanliness / maintainability lens, three duplication concerns. All verified by reading the surrounding code. - **`validCategory` duplicates the `plantCategories` enum map** — `internal/service/seed_packet.go:87-96` hard-codes the same six category constants as a `switch` that `plantCategories` (in `internal/service/plants.go:27-34`, consumed at `plants.go:316`) already encodes as a map. Two sources of truth for one enum: add a category to one and forget the other and they silently drift (a packet category that's valid for plant creation would be dropped from `SuggestedCategory`, or vice-versa). Cleaner one-liner: `func validCategory(c string) string { if _, ok := plantCategories[c]; ok { return c }; return "" }`. Verified by reading both files. - **`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 still passes against the stale copy and never notices. A small refactor — extract a `generate(ctx, m llm.Model, jpeg []byte) (SeedPacket, error)` core that both `Extract` and the test call — would remove the drift surface. Verified by reading both. - **`packetLotRequest` duplicates `seedLotCreateRequest` minus `PlantID`** — `internal/api/seed_packet.go:83-103` re-declares every lot field and a `toInput()` that mirrors `seedLotCreateRequest.toInput()` (`internal/api/seed_lots.go:20-42`) minus the `PlantID` line. The comment explains why (the original has `binding:"required"` on `PlantID`, which would wrongly require it for the packet flow), so the tradeoff is reasonable — but adding a new lot field now requires touching two structs and two `toInput()` methods in lockstep. Embedding a shared `lotFields` struct (no binding tag) in both request types would centralize the field list while letting each keep its own binding rules. Verified by reading both. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> I'll verify each finding against the actual code. Both findings verified against actual code. **Finding 1:** `seed_packet.go:203` calls `s.visiblePlant(ctx, actorID, *in.PlantID)` (line 203), then `seed_packet.go:212` calls `s.CreateSeedLot(ctx, actorID, lotIn)`, whose first line at `seed_lots.go:153` calls `s.visiblePlant(ctx, actorID, in.PlantID)` again on the same PlantID. Confirmed redundant query. **Finding 2:** `settings.go:88-100` getSettings: `GetInstanceSettings` (line 89) → `settingsPayload` calls `EffectiveAgent` (line 67) and `EffectiveVision` (line 71). Both `EffectiveAgent` (`instance_settings.go:99`) and `EffectiveVision` (`instance_settings.go:132`) independently call `s.store.GetInstanceSettings(ctx)`. So the row is read three times. Confirmed. Both findings survive verification. Verdict: Minor issues - **`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 write path. Low severity (single user action, not a loop) but a real redundant query this PR introduces. Fix: pass the already-loaded `res.Plant` straight to the lot-creation store call, or add an internal `createSeedLotFor(ctx, actorID, plant, in)` variant that skips the re-check. - **`internal/api/settings.go:66-74` — `instance_settings` row fetched three times per `GET/PATCH /settings`.** `getSettings` calls `svc.GetInstanceSettings` (line 89), then `settingsPayload` calls `svc.EffectiveAgent` (line 67) and now `svc.EffectiveVision` (line 71) — each independently calling `s.store.GetInstanceSettings` (`instance_settings.go:99` and `:132`). This PR adds the third read of the same single row. Admin-only and infrequent, so impact is trivial, but the row could be read once and passed to both resolvers. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> All three findings confirmed against the actual code. Let me verify finding 3's claim about separate store calls (no shared transaction). All three findings verified against the actual code: 1. **Finding 1 confirmed**: `seed_packet.go:38-43` wraps body in `MaxBytesReader` but maps every `FormFile` error to 400. Grep confirms no `MaxBytesError` check anywhere in the repo. The 413 branch (line 56) only catches `imagenorm.ErrTooLarge`. 2. **Finding 2 confirmed**: `api.go:215` swallows `EffectiveVision` error silently. Verified `EffectiveVision` (instance_settings.go:131) reads from `s.store.GetInstanceSettings` which can fail. Agent path uses atomic pointer with no error path. 3. **Finding 3 confirmed**: `seed_packet.go:194-216` — `CreatePlant` and `CreateSeedLot` are separate service calls (verified in plants.go:194 and seed_lots.go:152), no shared transaction. On lot failure, returns `(nil, err)` — the `res` with `Plant` populated is discarded, so caller loses the plant ID. **Verdict: Minor issues** - **Body-cap oversized upload misreported as 400, not 413** — `internal/api/seed_packet.go:38-43`. The handler wraps the body in `http.MaxBytesReader(..., scanUploadLimit)` (30 MiB) but maps *every* `c.FormFile("image")` error to 400 `INVALID_INPUT` with "attach an image as the \"image\" field". When the 30 MiB multipart body cap is exceeded, `FormFile` returns an error wrapping `*http.MaxBytesError`, distinguishable via `errors.As`. The PR description promises "caps the body, maps too-large/unreadable to clear statuses (413 / 400)", but the 413 branch at lines 56-58 only catches `imagenorm.ErrTooLarge` (decoded image too big). So an upload that blows the very cap `scanUploadLimit` exists to enforce gets a misleading "attach an image" 400 instead of the intended 413. Verified by reading the handler and grepping — `MaxBytesError` appears nowhere in the repo. Suggested fix: before the generic 400, `if var max *http.MaxBytesError; errors.As(err, &max) { writeAPIError(c, 413, "IMAGE_TOO_LARGE", ...); return }`. - **`EffectiveVision` store error swallowed in `/capabilities`** — `internal/api/api.go:215`. `if vis, err := h.svc.EffectiveVision(...); err == nil { vision = vis.Ready() }` silently reports `vision: false` on a transient DB error. The comment justifies per-request reads "same as agent", but the agent branch (`h.agent.get() != nil`) is an atomic pointer load with no error path, while `EffectiveVision` does a DB read that can fail (verified: `instance_settings.go:131-135` returns `err` from `s.store.GetInstanceSettings`). So a transient store failure degrades a capabilities flag with no log, no signal. Minor (advisory endpoint), but it is a swallowed error. Suggested fix: `slog.Warn` on the non-nil branch. - **`CreateFromPacket` discards the orphaned plant's identity on partial failure** — `internal/service/seed_packet.go:194-216`. If `CreatePlant` succeeds (line 194) and `CreateSeedLot` then fails (line 212), the plant row is committed (verified: `CreatePlant` in `plants.go:194` and `CreateSeedLot` in `seed_lots.go:152` are separate service calls with no shared transaction) but the function returns `(nil, err)` at line 214, discarding `res` which holds the created `Plant`. The comment at 182-185 documents the deliberate "leave it, don't roll back" choice, but the returned `*PacketResult` is `nil`, so the caller can't retry the lot against the just-created plant — it has no handle to it and must re-find it in the catalog. Suggested fix: return `res` with `Plant` populated even on the lot error (or a typed error carrying `PlantID`) so the retry is automatic. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> No material issues found. **Security lens review:** The change is solid from a security perspective. - **AuthN/AuthZ:** Both `/scan` and `/from-packet` are correctly behind `requireAuth()`. The service layer enforces ownership via `CreatePlant` (sets `OwnerID = actorID`), `CreateSeedLot` (sets `OwnerID = actorID`), and `visiblePlant` (rejects another user's plant with `ErrInvalidInput`). No authorization gaps. - **Input validation:** The upload is bounded by `http.MaxBytesReader(30 MB)` before multipart parsing, and `imagenorm.Normalize` adds its own 25 MB byte cap plus dimension/pixel-count guards against decompression bombs. The extracted `SeedPacket` fields are returned as a *proposal* — the model's output is never written directly; the user must confirm via `CreateFromPacket`. - **Injection/SSRF:** No SQL injection (parameterized store methods). `SourceURL` in the lot request flows through `finalizeSeedLot` → `validSourceURL`, which rejects non-`http`/`https` schemes and requires a host. No command injection vectors. - **Secret handling:** `OLLAMA_CLOUD_API_KEY` stays in the env as designed. `EffectiveVision` never leaks the key value to the API — `/capabilities` and `/settings` only expose `visionReady`/`visionModel` booleans and strings. - **Secure defaults:** The vision feature is off by default (empty `VisionModel`). It requires both a configured model and a present API key to be advertised as ready. The API returns 503 when unconfigured, preventing blind calls. Nothing in this diff raises a security concern. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** Through the correctness lens, the PR is sound. I verified every cross-file dependency and re-derived the key behaviors: - **Unit conversion**: `1 in = 2.54 cm` in the vision prompt is exact by definition. - **Catalog matching**: `matchPlants` uses conservative, name-based ranking (exact → substring → word-boundary species). The `seen` map prevents duplicate entries, and `sort.SliceStable` preserves input order within ranks. Verified against `internal/service/seed_packet_test.go`. - **Exactly-one validation**: `hasID == hasNew` correctly rejects both/neither for `PlantID`/`NewPlant`. Verified against API and service tests. - **ACL chain**: `CreateFromPacket`→`visiblePlant`→`CreateSeedLot` re-checks the plant, which is redundant but safe; built-in plants are allowed per existing `CreateSeedLot` comment. - **Settings resolution**: `EffectiveVision` layers DB→env correctly; `Ready()` requires both key and model. The `UpdateInstanceSettings` validation reuses `agentmodel.Validate` for both specs, which is consistent with the agent path. - **Migration consistency**: `instanceSettingsColumns`, `scanInstanceSettings`, and `UpdateInstanceSettings` all include `vision_model` in the same order. Migration 0011 is `ALTER TABLE ... ADD COLUMN ... DEFAULT ''`, safe for the single-row table. No logic bugs, incorrect thresholds, or semantic errors introduced by this change. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> No material issues found. The PR is clean through the code-cleanliness lens: - **Structure**: `Option` + `WithPacketExtractor` mirrors the existing `now` injection pattern already used in `Service`. This is consistent, not novel. - **Naming**: `EffectiveVision`, `PacketConfirm`, `PacketResult`, `PacketProposal`, `scanSeedPacket`, `createFromPacket`, `matchPlants`, `suggestedName`, `validCategory` — all follow the project's terse, domain-first convention. - **No dead/duplicated code**: `packetLotRequest` is intentionally a near-duplicate of `seedLotCreateRequest` with `plantId` removed, and the code comment at `internal/api/seed_packet.go:80-83` explains exactly why the reuse is avoided. `EffectiveVision` mirrors `EffectiveAgent` correctly. - **Function length**: `ExtractSeedPacket` and `CreateFromPacket` are short and single-purpose. `matchPlants` is readable with a clear local `scored` helper. - **Tests**: `packetEngine`, `visionCfg`, `pngUpload`, `doMultipart` are well-scoped helpers that follow the existing test patterns (`packetEngine` mirrors how other test files build a test engine). Nothing in my lane to report. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** Reviewed through the **Performance** lens. I checked: - **Upload/memory bounds**: `scanUploadLimit` (30 MiB) caps the multipart body; `imagenorm.Normalize` adds its own `DefaultMaxBytes` (25 MiB), `maxDecodePixels` (50 MP), and `maxDimension` (50 k px) guards, plus a `recover` around third-party decoders. Peak allocation for one request is bounded (~input + decoded + downscaled + output), and the handler only ever processes one image at a time per request. - **Catalog matching**: `ListPlantsForActor` is hard-capped at 5 000 rows (`maxPlantsListed`). `matchPlants` is O(n log n) on that bounded set with only lightweight string ops — no quadratic behavior. - **Vision call**: The model extraction (`majordomo.Generate`) is the obvious latency dominant; it correctly receives the request `context` so it respects cancellation. No work is duplicated before/after it. - **Creation path**: `CreateFromPacket` does a strict two-step (plant then lot), no loops, no N+1 queries, no unbounded growth. Everything that could be unbounded is capped, there are no allocations inside loops that scale with data size beyond the small bounded plant list, and no obvious double-buffering of the image bytes. Nothing in this lane. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary> **Blocking issues found** - **`internal/api/api.go:215-217`** — `EffectiveVision` error silently swallowed. In `capabilities`, if the DB query fails (e.g., locked row, connection timeout), the error is discarded and `vision` is reported as `false` with a 200 OK. The UI hides the scan button and the admin has no signal that the server is in distress. The error should at minimum be logged; returning the error to the client (500) or defaulting to the env value with a log line are both better than silence. - **`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. - **`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. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-22 04:13:43 +00:00
Address seed-packet review: 413 mapping, deadline, rollback, dedup
Build image / build-and-push (push) Successful in 6s
6a4fd40bc3
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
steve merged commit afa9288b4b into main 2026-07-22 04:22:27 +00:00
steve deleted branch feat/seed-packet-backend 2026-07-22 04:22:27 +00:00
Sign in to join this conversation.