Plantings backend: plop CRUD, derived counts, removed_at (#14) #33

Merged
steve merged 2 commits from phase-5-plantings-api into main 2026-07-19 02:41:22 +00:00
Owner

Closes #14. Part of epic #20 (Phase 5 — Plops). Builds on #10 (objects) and #12 (plants).

What

A planting ("plop") is a circular patch of one plant inside a plantable object, positioned in the object's local frame (so moving/rotating the bed carries its plants). removed_at soft-removes a plop — the "clear bed" / season-history seam.

domain

  • Planting gains a computed, non-persisted DerivedCount int (json:"derivedCount"). The store scan leaves it 0; the service fills it. Effective count a client shows is count ?? derivedCount.

store (internal/store/plantings.go)

  • GetPlanting, CreatePlanting, UpdatePlanting (version-guarded, same 409/current contract as the other resources), DeletePlanting — alongside the existing ListActivePlantingsForGarden /full read.

service (internal/service/plantings.go)

  • CreatePlanting / UpdatePlanting / DeletePlanting require editor role on the object's garden (via objectForRole).
  • Object must be plantable (tree/path/structure → ErrInvalidInput).
  • plant_id must be visible to the actor — a built-in or their own; an unknown or another user's plant → ErrInvalidInput (no existence leak).
  • Loose bounds: the plop's center must sit within the object's unrotated local bounds (|x| ≤ w/2, |y| ≤ h/2); the radius may overhang.
  • planted_at defaults to today; radius_cm > 0; optional count override (≥ 1) wins over derived.
  • derivedCount = max(1, round(π·r² / spacing²)) — one unit-tested helper, reused by /full (via a spacing map built from the referenced plants — no N+1) and single responses.

api

  • POST /api/v1/objects/:id/plantings, PATCH,DELETE /api/v1/plantings/:id.
  • Nullable count / label / plantedAt / removedAt use json.RawMessage so an explicit null (clear) is distinct from an absent field (unchanged). removedAt: "…" soft-removes; removedAt: null un-removes.
  • /full now enriches each active plop with its derivedCount.

Tests

  • Service: formula edge cases (tiny radius → 1, zero radius/spacing → 1), create defaults + derived, count override wins, move/resize + clear override, non-plantable rejection, foreign/unknown plant rejection, bounds (edge allowed, outside rejected, zero radius rejected), soft-remove leaves /full, version conflict (+ current row carries derivedCount), cross-user masking, delete.
  • API: create → 201 with derivedCount → /full includes it → soft-remove leaves /full → hard delete; plant-into-tree → 400; auth required.

GOWORK=off go build/vet/test ./internal/... green.

Acceptance criteria

  • Place a plop in a bed (local coords), grow its radius → derivedCount rises with r²; set count → override wins; set removedAt → it leaves /full.
  • Planting into a tree/path → 400; planting user B's custom plant as user A → rejected.
  • go test ./internal/service/... green including the count formula's edge cases.

🤖 Generated with Claude Code

Closes #14. Part of epic #20 (Phase 5 — Plops). Builds on #10 (objects) and #12 (plants). ## What A **planting** ("plop") is a circular patch of one plant inside a plantable object, positioned in the object's **local frame** (so moving/rotating the bed carries its plants). `removed_at` soft-removes a plop — the "clear bed" / season-history seam. ### domain - `Planting` gains a computed, **non-persisted** `DerivedCount int` (`json:"derivedCount"`). The store scan leaves it 0; the service fills it. Effective count a client shows is `count ?? derivedCount`. ### store (`internal/store/plantings.go`) - `GetPlanting`, `CreatePlanting`, `UpdatePlanting` (version-guarded, same 409/`current` contract as the other resources), `DeletePlanting` — alongside the existing `ListActivePlantingsForGarden` `/full` read. ### service (`internal/service/plantings.go`) - `CreatePlanting` / `UpdatePlanting` / `DeletePlanting` require **editor** role on the object's garden (via `objectForRole`). - Object must be `plantable` (tree/path/structure → `ErrInvalidInput`). - `plant_id` must be **visible** to the actor — a built-in or their own; an unknown or another user's plant → `ErrInvalidInput` (no existence leak). - Loose bounds: the plop's **center** must sit within the object's unrotated local bounds (`|x| ≤ w/2`, `|y| ≤ h/2`); the radius may overhang. - `planted_at` defaults to today; `radius_cm > 0`; optional `count` override (≥ 1) wins over derived. - **`derivedCount = max(1, round(π·r² / spacing²))`** — one unit-tested helper, reused by `/full` (via a spacing map built from the referenced plants — no N+1) and single responses. ### api - `POST /api/v1/objects/:id/plantings`, `PATCH,DELETE /api/v1/plantings/:id`. - Nullable `count` / `label` / `plantedAt` / `removedAt` use `json.RawMessage` so an explicit `null` (clear) is distinct from an absent field (unchanged). `removedAt: "…"` soft-removes; `removedAt: null` un-removes. - `/full` now enriches each active plop with its `derivedCount`. ## Tests - Service: formula edge cases (tiny radius → 1, zero radius/spacing → 1), create defaults + derived, count override wins, move/resize + clear override, non-plantable rejection, foreign/unknown plant rejection, bounds (edge allowed, outside rejected, zero radius rejected), soft-remove leaves `/full`, version conflict (+ current row carries derivedCount), cross-user masking, delete. - API: create → 201 with derivedCount → `/full` includes it → soft-remove leaves `/full` → hard delete; plant-into-tree → 400; auth required. `GOWORK=off go build/vet/test ./internal/...` green. ## Acceptance criteria - ✅ Place a plop in a bed (local coords), grow its radius → `derivedCount` rises with r²; set `count` → override wins; set `removedAt` → it leaves `/full`. - ✅ Planting into a tree/path → 400; planting user B's custom plant as user A → rejected. - ✅ `go test ./internal/service/...` green including the count formula's edge cases. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-19 02:26:41 +00:00
Add plantings backend: plop CRUD, derived counts, removed_at (#14)
Build image / build-and-push (push) Successful in 7s
Gadfly review (reusable) / review (pull_request) Successful in 6m56s
Adversarial Review (Gadfly) / review (pull_request) Successful in 6m56s
2f3699f7fa
- domain: Planting gains a computed (non-persisted) DerivedCount field.
- store/plantings.go: Get/Create/Update (version-guarded)/Delete alongside the
  existing /full read helper.
- service/plantings.go: place/move/resize/soft-remove a plop; editor role on the
  object's garden; object must be plantable; plant_id must be visible to the
  actor (built-in or own) else ErrInvalidInput; center must sit within the
  object's unrotated local bounds (radius may overhang); planted_at defaults to
  today. derivedCount = max(1, round(π·r²/spacing²)) — one unit-tested helper,
  reused by /full (via a spacing map, no N+1) and single responses.
- api: POST /objects/:id/plantings, PATCH/DELETE /plantings/:id; nullable
  count/label/plantedAt/removedAt use RawMessage so null (clear) is distinct
  from absent (unchanged). removedAt is the soft-remove / "clear bed" seam.
- /full now enriches each active plop with its derivedCount.

Service tests: formula edge cases (tiny radius → 1), defaults + derived, count
override, move/resize + clear override, non-plantable rejection, foreign/unknown
plant rejection, bounds, soft-remove leaves /full, version conflict, cross-user
masking, delete. Plus an API-level create/patch/full/delete flow.

GOWORK=off go build/vet/test ./internal/... green.

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

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-19 02:33:38Z

claude-code/sonnet · claude-code — done

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

glm-5.2:cloud · ollama-cloud — done

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

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

  • ⚠️ security — could not complete
  • correctness — Minor issues
  • maintainability — Minor issues
  • performance — No material issues found
  • 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 — No material issues found
  • 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 — Minor issues

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-19 02:33:38Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ⚠️ **security** — could not complete - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **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** — No material issues found - ✅ **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** — Minor issues <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-19 02:33:38 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 12 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** — 12 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +80,4 @@
return
}
var req plantingCreateRequest
if err := c.ShouldBindJSON(&req); err != nil {

🟡 ShouldBindJSON error swallowed by fixed misleading message in createPlanting

error-handling · flagged by 2 models

  • internal/api/plantings.go:83createPlanting swallows the actual ShouldBindJSON error and always emits a fixed message. If the request body contains malformed JSON, wrong types (e.g., "plantId": "abc"), or syntax errors, the client still receives "invalid planting: a plantId is required", which is misleading and makes debugging harder. Fix: inspect err (e.g. with errors.As for validator.ValidationErrors / json.UnmarshalTypeError) and return the underlying detail, or…

🪰 Gadfly · advisory

🟡 **ShouldBindJSON error swallowed by fixed misleading message in createPlanting** _error-handling · flagged by 2 models_ - **`internal/api/plantings.go:83`** — `createPlanting` swallows the actual `ShouldBindJSON` error and always emits a fixed message. If the request body contains malformed JSON, wrong types (e.g., `"plantId": "abc"`), or syntax errors, the client still receives *"invalid planting: a plantId is required"*, which is misleading and makes debugging harder. **Fix:** inspect `err` (e.g. with `errors.As` for `validator.ValidationErrors` / `json.UnmarshalTypeError`) and return the underlying detail, or… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +98,4 @@
return
}
var req plantingUpdateRequest
if err := c.ShouldBindJSON(&req); err != nil {

🟡 ShouldBindJSON error swallowed by fixed misleading message in updatePlanting

error-handling · flagged by 1 model

  • internal/api/plantings.go:101updatePlanting swallows the actual ShouldBindJSON error and always emits a fixed message. If the request body contains malformed JSON, wrong types, or syntax errors, the client still receives "invalid update: a current version is required", which is misleading and makes debugging harder. Fix: inspect err and return the underlying detail, or at least a generic "invalid request body" when the required field is actually present.

🪰 Gadfly · advisory

🟡 **ShouldBindJSON error swallowed by fixed misleading message in updatePlanting** _error-handling · flagged by 1 model_ - **`internal/api/plantings.go:101`** — `updatePlanting` swallows the actual `ShouldBindJSON` error and always emits a fixed message. If the request body contains malformed JSON, wrong types, or syntax errors, the client still receives *"invalid update: a current version is required"*, which is misleading and makes debugging harder. **Fix:** inspect `err` and return the underlying detail, or at least a generic "invalid request body" when the required field is actually present. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +23,4 @@
// mature spacing: max(1, round(π·r² / spacing²)). It is the single source of the
// formula (also feeds #15's display and #19's FillRegion). A non-positive radius
// or spacing collapses to the floor of 1.
func derivedCount(radiusCM, spacingCM float64) int {

🟠 derivedCount missing math.IsInf guard — corrupted radius can yield implementation-defined int conversion

error-handling · flagged by 1 model

  • internal/service/plantings.go:26-36derivedCount guards against NaN and non-positive inputs but not +Inf. If corrupted data (or a radius whose square overflows float64 to +Inf) reaches GardenFull, int(math.Round(+Inf)) produces implementation-defined results (possible wrap or panic per the Go spec). finalizePlanting already validates with isFinite (which includes !math.IsInf), so the helper should be symmetrically defensive.

🪰 Gadfly · advisory

🟠 **derivedCount missing math.IsInf guard — corrupted radius can yield implementation-defined int conversion** _error-handling · flagged by 1 model_ * **`internal/service/plantings.go:26-36`** — `derivedCount` guards against `NaN` and non-positive inputs but **not `+Inf`**. If corrupted data (or a radius whose square overflows float64 to `+Inf`) reaches `GardenFull`, `int(math.Round(+Inf))` produces implementation-defined results (possible wrap or panic per the Go spec). `finalizePlanting` already validates with `isFinite` (which includes `!math.IsInf`), so the helper should be symmetrically defensive. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +28,4 @@
return 1
}
area := math.Pi * radiusCM * radiusCM
n := int(math.Round(area / (spacingCM * spacingCM)))

🟠 derivedCount has no upper bound; huge radius / tiny spacing yields unbounded int (manual count is capped at 1e6 but derived is not)

error-handling · flagged by 2 models

  • internal/service/plantings.go:31derivedCount has no upper cap (derived path unbounded). area / spacing² (line 31) can be astronomically large for maxRadiusCM = 10_000 against a tiny spacing, and int(math.Round(...)) converts to an int with no ceiling. The manual count override is capped at maxExplicitCount = 1_000_000 (line 223), but the derived path has no equivalent clamp, so a huge-radius / tiny-spacing plop yields an unbounded derivedCount flowing into the JSON res…

🪰 Gadfly · advisory

🟠 **derivedCount has no upper bound; huge radius / tiny spacing yields unbounded int (manual count is capped at 1e6 but derived is not)** _error-handling · flagged by 2 models_ - **`internal/service/plantings.go:31` — `derivedCount` has no upper cap (derived path unbounded).** `area / spacing²` (line 31) can be astronomically large for `maxRadiusCM = 10_000` against a tiny spacing, and `int(math.Round(...))` converts to an `int` with no ceiling. The manual `count` override is capped at `maxExplicitCount = 1_000_000` (line 223), but the derived path has no equivalent clamp, so a huge-radius / tiny-spacing plop yields an unbounded `derivedCount` flowing into the JSON res… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +104,4 @@
return created, nil
}
// UpdatePlanting applies a partial, version-guarded patch to a plop in an object

🟠 UpdatePlanting re-validates unchanged position against current object bounds, permanently blocking patches (including soft-remove) after the parent object is resized smaller

error-handling · flagged by 1 model

  • internal/service/plantings.go:107-140 (UpdatePlanting) unconditionally re-validates the plop's position against the object's current bounds via finalizePlanting (line 125), even when the patch touches neither XCM nor YCM. finalizeObject (internal/service/objects.go:232-266) lets an editor shrink WidthCM/HeightCM at any time with no check against existing plantings inside the object. Once an object is resized smaller than an existing plop's stored position, `finalizePlanting…

🪰 Gadfly · advisory

🟠 **UpdatePlanting re-validates unchanged position against current object bounds, permanently blocking patches (including soft-remove) after the parent object is resized smaller** _error-handling · flagged by 1 model_ - `internal/service/plantings.go:107-140` (`UpdatePlanting`) unconditionally re-validates the plop's position against the object's *current* bounds via `finalizePlanting` (line 125), even when the patch touches neither `XCM` nor `YCM`. `finalizeObject` (`internal/service/objects.go:232-266`) lets an editor shrink `WidthCM`/`HeightCM` at any time with no check against existing plantings inside the object. Once an object is resized smaller than an existing plop's stored position, `finalizePlanting… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +110,4 @@
pl, err := s.store.GetPlanting(ctx, plantingID)
if err != nil {
return nil, err // ErrNotFound
}

🟠 UpdatePlanting missing plantable check

correctness · flagged by 1 model

  • internal/service/plantings.go:113UpdatePlanting omits the plantable check. CreatePlanting correctly rejects non-plantable objects (!o.Plantable → domain.ErrInvalidInput), but UpdatePlanting fetches the object via objectForRole and proceeds straight to finalizePlanting without ever verifying o.Plantable. The PR description explicitly states that UpdatePlanting requires a plantable object ("CreatePlanting / UpdatePlanting / DeletePlanting … Object must be plantable"), so…

🪰 Gadfly · advisory

🟠 **UpdatePlanting missing plantable check** _correctness · flagged by 1 model_ - **`internal/service/plantings.go:113` — `UpdatePlanting` omits the plantable check.** `CreatePlanting` correctly rejects non-plantable objects (`!o.Plantable → domain.ErrInvalidInput`), but `UpdatePlanting` fetches the object via `objectForRole` and proceeds straight to `finalizePlanting` without ever verifying `o.Plantable`. The PR description explicitly states that UpdatePlanting requires a plantable object ("CreatePlanting / UpdatePlanting / DeletePlanting … Object must be `plantable`"), so… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +116,4 @@
return nil, err
}
applyPlantingPatch(pl, patch)

🟠 UpdatePlanting always re-validates position bounds against the current object, blocking soft-remove/other edits on plops orphaned outside bounds by a later object resize

correctness · flagged by 1 model

  • internal/service/plantings.go:119-127 (UpdatePlanting) unconditionally calls finalizePlanting(pl, o) after applyPlantingPatch, re-validating the plop's x_cm/y_cm against the object's current WidthCM/HeightCM even when the patch doesn't touch position (e.g. a pure removedAt soft-remove). internal/service/objects.go:202-207,138-150 lets an editor shrink an object's dimensions via PATCH /objects/:id (applyObjectPatch) with no check against existing plantings in `finalize…

🪰 Gadfly · advisory

🟠 **UpdatePlanting always re-validates position bounds against the current object, blocking soft-remove/other edits on plops orphaned outside bounds by a later object resize** _correctness · flagged by 1 model_ - `internal/service/plantings.go:119-127` (`UpdatePlanting`) unconditionally calls `finalizePlanting(pl, o)` after `applyPlantingPatch`, re-validating the plop's `x_cm`/`y_cm` against the object's *current* `WidthCM`/`HeightCM` even when the patch doesn't touch position (e.g. a pure `removedAt` soft-remove). `internal/service/objects.go:202-207,138-150` lets an editor shrink an object's dimensions via `PATCH /objects/:id` (`applyObjectPatch`) with no check against existing plantings in `finalize… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +145,4 @@
pl, err := s.store.GetPlanting(ctx, plantingID)
if err != nil {
return err
}

🟡 DeletePlanting missing plantable check

correctness · flagged by 1 model

  • internal/service/plantings.go:148DeletePlanting similarly omits the plantable check. Hard-deleting a planting in a non-plantable object is arguably less problematic (it's cleanup), but the PR description groups Delete with the same plantable requirement. If the intent is to allow cleanup, the PR description should be updated; otherwise add the same guard.

🪰 Gadfly · advisory

🟡 **DeletePlanting missing plantable check** _correctness · flagged by 1 model_ - **`internal/service/plantings.go:148` — `DeletePlanting` similarly omits the plantable check.** Hard-deleting a planting in a non-plantable object is arguably less problematic (it's cleanup), but the PR description groups Delete with the same plantable requirement. If the intent is to allow cleanup, the PR description should be updated; otherwise add the same guard. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +171,4 @@
// enrichDerived best-effort sets p.DerivedCount from its plant's spacing (used
// when we don't already hold the plant, e.g. a conflict's current row).
func (s *Service) enrichDerived(ctx context.Context, p *domain.Planting) {

🟡 enrichDerived swallows GetPlant error; 409 current row may carry derivedCount 0 inconsistently with normal GET

error-handling · flagged by 3 models

  • internal/service/plantings.go:174enrichDerived swallows GetPlant error; 409 current row may carry derivedCount: 0. On the version-conflict path (lines 132-134), enrichDerived ignores the GetPlant error (best-effort by design), so the returned current row's DerivedCount stays 0 if the plant lookup fails — inconsistent with a normal GET that would carry a positive value. Low severity; the degraded response carries no signal of the swallowed failure.

🪰 Gadfly · advisory

🟡 **enrichDerived swallows GetPlant error; 409 current row may carry derivedCount 0 inconsistently with normal GET** _error-handling · flagged by 3 models_ - **`internal/service/plantings.go:174` — `enrichDerived` swallows `GetPlant` error; 409 current row may carry `derivedCount: 0`.** On the version-conflict path (lines 132-134), `enrichDerived` ignores the `GetPlant` error (best-effort by design), so the returned `current` row's `DerivedCount` stays 0 if the plant lookup fails — inconsistent with a normal GET that would carry a positive value. Low severity; the degraded response carries no signal of the swallowed failure. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +208,4 @@
// finalizePlanting validates a built/merged plop against its parent object.
// Shared by create and update so both enforce the same invariants.
func finalizePlanting(p *domain.Planting, o *domain.GardenObject) error {

finalizePlanting validation order tucks date checks after unrelated count/label checks, hurting readability

maintainability · flagged by 1 model

  • internal/service/plantings.go:211finalizePlanting validation order tucks the date checks (:229) last, after unrelated count (:223) and label (:226) checks. Confirmed: order is radius → coords → bounds → count → label → dates. Readability nit only; behavior is correct. Not blocking.

🪰 Gadfly · advisory

⚪ **finalizePlanting validation order tucks date checks after unrelated count/label checks, hurting readability** _maintainability · flagged by 1 model_ - **`internal/service/plantings.go:211` — `finalizePlanting` validation order tucks the date checks (`:229`) last, after unrelated count (`:223`) and label (`:226`) checks.** Confirmed: order is radius → coords → bounds → count → label → dates. Readability nit only; behavior is correct. Not blocking. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +226,4 @@
if p.Label != nil && len(*p.Label) > maxPlantingLabelLen {
return domain.ErrInvalidInput
}
if !validDatePtr(p.PlantedAt) || !validDatePtr(p.RemovedAt) {

🟡 finalizePlanting never checks removedAt is on/after plantedAt, allowing a nonsensical removed-before-planted interval

error-handling · flagged by 3 models

  • internal/service/plantings.go:229-231 (finalizePlanting) validates PlantedAt and RemovedAt individually via validDatePtr but never checks RemovedAt is on/after PlantedAt. Confirmed no such comparison exists anywhere in the service or store layer (internal/store/plantings.go just persists both fields as given). A client can soft-remove a plop with removedAt earlier than plantedAt, silently producing a nonsensical interval.

🪰 Gadfly · advisory

🟡 **finalizePlanting never checks removedAt is on/after plantedAt, allowing a nonsensical removed-before-planted interval** _error-handling · flagged by 3 models_ - `internal/service/plantings.go:229-231` (`finalizePlanting`) validates `PlantedAt` and `RemovedAt` individually via `validDatePtr` but never checks `RemovedAt` is on/after `PlantedAt`. Confirmed no such comparison exists anywhere in the service or store layer (`internal/store/plantings.go` just persists both fields as given). A client can soft-remove a plop with `removedAt` earlier than `plantedAt`, silently producing a nonsensical interval. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +242,4 @@
}
// trimStringPtr trims a *string, preserving nil (distinct from empty).
func trimStringPtr(s *string) *string {

🟡 trimStringPtr helper diverges from the inline strings.TrimSpace idiom used elsewhere in the service

maintainability · flagged by 2 models

  • internal/service/plantings.go:245trimStringPtr diverges from the inline strings.TrimSpace idiom used elsewhere in the service. Confirmed: objects.go:194/:225 and plants.go:141/:153/:159 all trim inline (o.Name = strings.TrimSpace(*p.Name)), whereas plantings.go introduces a trimStringPtr helper used at two call sites (:89, :199). Minor consistency nit; harmless either way.

🪰 Gadfly · advisory

🟡 **trimStringPtr helper diverges from the inline strings.TrimSpace idiom used elsewhere in the service** _maintainability · flagged by 2 models_ - **`internal/service/plantings.go:245` — `trimStringPtr` diverges from the inline `strings.TrimSpace` idiom used elsewhere in the service.** Confirmed: `objects.go:194`/`:225` and `plants.go:141`/`:153`/`:159` all trim inline (`o.Name = strings.TrimSpace(*p.Name)`), whereas `plantings.go` introduces a `trimStringPtr` helper used at two call sites (`:89`, `:199`). Minor consistency nit; harmless either way. <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

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

Finding Where Models Lens
🟡 enrichDerived swallows GetPlant error; 409 current row may carry derivedCount 0 inconsistently with normal GET internal/service/plantings.go:174 3/5 error-handling
🟡 finalizePlanting never checks removedAt is on/after plantedAt, allowing a nonsensical removed-before-planted interval internal/service/plantings.go:229 3/5 error-handling
🟠 derivedCount has no upper bound; huge radius / tiny spacing yields unbounded int (manual count is capped at 1e6 but derived is not) internal/service/plantings.go:31 2/5 error-handling
🟡 ShouldBindJSON error swallowed by fixed misleading message in createPlanting internal/api/plantings.go:83 2/5 error-handling
🟡 trimStringPtr helper diverges from the inline strings.TrimSpace idiom used elsewhere in the service internal/service/plantings.go:245 2/5 maintainability
10 single-model findings (lower confidence)
Finding Where Model Lens
🟠 derivedCount missing math.IsInf guard — corrupted radius can yield implementation-defined int conversion internal/service/plantings.go:26 opencode/kimi-k2.6:cloud error-handling
🟠 UpdatePlanting re-validates unchanged position against current object bounds, permanently blocking patches (including soft-remove) after the parent object is resized smaller internal/service/plantings.go:107 claude-code/sonnet error-handling
🟠 UpdatePlanting missing plantable check internal/service/plantings.go:113 kimi-k2.6:cloud correctness
🟠 UpdatePlanting always re-validates position bounds against the current object, blocking soft-remove/other edits on plops orphaned outside bounds by a later object resize internal/service/plantings.go:119 claude-code/sonnet correctness
🟡 ShouldBindJSON error swallowed by fixed misleading message in updatePlanting internal/api/plantings.go:101 kimi-k2.6:cloud error-handling
🟡 DeletePlanting missing plantable check internal/service/plantings.go:148 kimi-k2.6:cloud correctness
🟡 timeLayout comment claims 'every timestamp' but plantings now store date-only planted_at/removed_at via a separate dateLayout constant internal/service/service.go:23 glm-5.2:cloud maintainability
Stale FullGarden doc says Plantings/Plants stay empty until #14; this PR populates them internal/service/objects.go:72 opencode/glm-5.2:cloud maintainability
finalizePlanting validation order tucks date checks after unrelated count/label checks, hurting readability internal/service/plantings.go:211 glm-5.2:cloud maintainability
Stale doc on ListActivePlantingsForGarden references #14 as not-yet-landed, but this PR lands it internal/store/plantings.go:31 opencode/glm-5.2:cloud maintainability
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

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

🔒 Security — No material issues found

These parse safely (bounded JSON unmarshal, no injection). Authorization, plant-visibility masking, and SQL parameterization all check out. No security issues found in this diff.

VERDICT: No material issues found

Verified via the actual repo code:

  • Authorization: CreatePlanting/UpdatePlanting/DeletePlanting all funnel through objectForRolerequireGardenRole (internal/service/objects.go:124, internal/service/gardens.go:52), which enforces role and masks existence with ErrNotFound for gardens the actor can't see — consistent with the existing objects/gardens pattern.
  • IDOR / existence leak: UpdatePlanting/DeletePlanting fetch the planting by raw id first (internal/service/plantings.go:114,149), but a not-owned planting's containing garden then fails objectForRole with ErrNotFound too, so the two failure modes are indistinguishable to the caller — no existence oracle.
  • Cross-user plant reference: visiblePlant (internal/service/plantings.go:~180) rejects both unknown and another user's custom plants with the same ErrInvalidInput, not a distinguishing error — no enumeration of other users' plant IDs. Confirmed built-ins (OwnerID == nil) are allowed for anyone.
  • SQL injection: all store queries use parameterized ? placeholders (internal/store/plantings.go); plantingColumns is a compile-time constant, never user input.
  • Input validation: finalizePlanting bounds radius (0 < r ≤ 10,000cm), rejects NaN/Inf via isFinite, caps label length (200) and explicit count (1..1,000,000), and validates date strings against a fixed layout (time.Parse, no format-string injection risk).
  • Nullable-field parsing (parseNullableInt/parseNullableString/json.RawMessage): bounded json.Unmarshal calls, no panics or type confusion possible from attacker-controlled JSON.
  • Auth middleware: both new route groups (objects.POST(".../plantings") and the plantings group) sit under h.requireAuth() — confirmed in internal/api/api.go.

Outside my lens: none worth flagging.

🎯 Correctness — Minor issues

No test covers the resize-then-soft-remove sequence, confirming the draft's claim. The finding is fully verified against the actual code: applyObjectPatch/finalizeObject in objects.go permit shrinking WidthCM/HeightCM with no check against existing plantings, and UpdatePlanting unconditionally re-validates bounds via finalizePlanting using the freshly-fetched current object, regardless of which patch fields were actually set — so a status-only patch (e.g. removedAt soft-remove) on a planting orphaned by a later resize is permanently rejected, while DeletePlanting bypasses finalizePlanting entirely.

VERDICT: Minor issues

  • internal/service/plantings.go:119-127 (UpdatePlanting) unconditionally calls finalizePlanting(pl, o) after applyPlantingPatch, re-validating the plop's x_cm/y_cm against the object's current WidthCM/HeightCM even when the patch doesn't touch position (e.g. a pure removedAt soft-remove). internal/service/objects.go:202-207,138-150 lets an editor shrink an object's dimensions via PATCH /objects/:id (applyObjectPatch) with no check against existing plantings in finalizeObject. Once an object is resized smaller than an existing planting's stored position, finalizePlanting's bounds check (plantings.go:220: math.Abs(p.XCM) > o.WidthCM/2 || math.Abs(p.YCM) > o.HeightCM/2) rejects every subsequent UpdatePlanting call with ErrInvalidInput, including status-only edits like soft-remove — the only way to clear the stale plop becomes DeletePlanting (plantings.go:144-153), which skips finalizePlanting entirely and hard-deletes it, losing the season-history record soft-remove is meant to preserve. Confirmed no test (plantings_test.go) covers a resize-then-update sequence.
    • Suggested fix: only re-validate geometry (position/radius bounds) when the patch actually changes XCM/YCM/RadiusCM, so status-only patches aren't blocked by a since-changed object size.
🧹 Code cleanliness & maintainability — No material issues found

All test helpers are properly reused from existing test files rather than duplicated. The new plantings_test.go only adds the two path helpers it genuinely needs (plantingPath, objectPlantingsPath).

VERDICT: No material issues found

I reviewed internal/service/plantings.go, internal/api/plantings.go, internal/store/plantings.go, and internal/domain/domain.go against the established patterns in objects.go/plants.go (both service and api layers). The PR is a clean, disciplined follow of existing conventions:

  • The nullable-field Set* flag idiom, parseNullableInt/parseNullableString (api/objects.go, api/plants.go), parseIDParam, mustActor, writeVersionConflict, isFinite are all reused rather than reimplemented.
  • derivedCount is a single well-placed helper reused consistently across create, update, version-conflict enrichment, and GardenFull — no copy-pasted formula.
  • Struct field layout, doc-comment style, and handler structure in internal/api/plantings.go mirror objects.go almost line-for-line.
  • Test helpers (objectPath, fullPath, doJSON, authEngine, etc.) are reused from existing _test.go files; only the two genuinely new path helpers were added.
  • No dead code, no deep nesting, no oversized functions — finalizePlanting/applyPlantingPatch mirror the shape of finalizeObject/applyObjectPatch exactly.

Minor, non-blocking observation (not filed as a finding): enrichDerived re-fetches the plant via s.store.GetPlant on a version conflict rather than reusing the plant already fetched earlier in UpdatePlanting — but this is necessary, not sloppy, since the conflicting current row can reference a different plant_id than the one just validated, so a second fetch is correct, not a maintainability smell.

Performance — No material issues found

VERDICT: No material issues found

I reviewed the new store/service code (internal/service/plantings.go, internal/service/objects.go's GardenFull enrichment, internal/store/plantings.go) specifically for N+1s, hot-loop work, unbounded growth, and missing indexes.

  • GardenFull's derivedCount enrichment (internal/service/objects.go:179-187) builds a map[int64]float64 from the already-loaded plants slice and does an O(1) lookup per planting — verified this is a single pass over both slices, not a query per row, so the PR's "no N+1" claim in the description holds up.
  • internal/store/plants.go:31-57's ListReferencedPlants (pre-existing, unchanged by this diff) does the plant/planting join in one query with DISTINCT, and internal/store/migrations/0001_init.sql:138-139 confirms idx_plantings_object and idx_plantings_plant exist, so the joins used by both ListActivePlantingsForGarden and ListReferencedPlants are indexed.
  • CreatePlanting/UpdatePlanting/DeletePlanting (internal/service/plantings.go) each issue a small, constant number of single-row queries (objectForRoleGetObject+role check, visiblePlantGetPlant, then one store write) — no loops over collections, matching the existing pattern already used by objects.go's create/update/delete, so this isn't a regression introduced here.
  • enrichDerived (internal/service/plantings.go:180-186) only runs on the version-conflict path, not the hot path, and is a single GetPlant call.
  • derivedCount itself is pure O(1) arithmetic; no allocation or loop concerns even at the maxRadiusCM ceiling.

Nothing here introduces an N+1 query, a loop-bound query, unbounded result growth, or a missing index. The diff is clean from a performance standpoint.

🧯 Error handling & edge cases — Minor issues

Both findings confirmed directly against the store layer too — no comparison check exists anywhere in the write path. Both findings from the draft hold up.

VERDICT: Minor issues

  • internal/service/plantings.go:107-140 (UpdatePlanting) unconditionally re-validates the plop's position against the object's current bounds via finalizePlanting (line 125), even when the patch touches neither XCM nor YCM. finalizeObject (internal/service/objects.go:232-266) lets an editor shrink WidthCM/HeightCM at any time with no check against existing plantings inside the object. Once an object is resized smaller than an existing plop's stored position, finalizePlanting (internal/service/plantings.go:220-222, math.Abs(p.XCM) > o.WidthCM/2 || math.Abs(p.YCM) > o.HeightCM/2) will reject every subsequent PATCH /plantings/:id on that row — including patches that only touch label, count, or plantId. This also blocks the soft-remove ("clear bed") mechanism, since removedAt is set via the same UpdatePlanting/finalizePlanting path (internal/api/plantings.go PATCH handler → service.PlantingPatch.SetRemovedAt), while DeletePlanting (internal/service/plantings.go:144-153) skips finalizePlanting entirely and still works. Confirmed by reading finalizePlanting, finalizeObject, and both the update and delete call paths; no code path re-validates only on touched fields, and no test covers resize-then-patch/soft-remove.

  • internal/service/plantings.go:229-231 (finalizePlanting) validates PlantedAt and RemovedAt individually via validDatePtr but never checks RemovedAt is on/after PlantedAt. Confirmed no such comparison exists anywhere in the service or store layer (internal/store/plantings.go just persists both fields as given). A client can soft-remove a plop with removedAt earlier than plantedAt, silently producing a nonsensical interval.

Everything else in the draft's "checked out cleanly" section was not re-verified here (out of scope for the flagged findings) but nothing in the reviewed files contradicts those conclusions.

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

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

🔒 Security — No material issues found

VERDICT: No material issues found

Security review of the plantings CRUD through the authn/authz/injection/validation lens:

  • Authorization (editor role): CreatePlanting, UpdatePlanting, and DeletePlanting all gate through objectForRole(..., roleEditor) (internal/service/plantings.go:70,114,149), which calls requireGardenRole and masks a non-viewable garden as ErrNotFound (internal/service/gardens.go:52-65). Cross-user create/update/delete all surface as ErrNotFound (verified at plantings_test.go TestPlantingCrossUserIsNotFound). No IDOR.
  • Existence masking on plant refs: visiblePlant (plantings.go:158-170) collapses both unknown and foreign-owned plants to ErrInvalidInput rather than ErrNotFound, avoiding a plant-existence oracle. Built-ins (OwnerID == nil) are referenceable by anyone.
  • Object plantability: non-plantable kinds rejected with ErrInvalidInput (plantings.go:74-76), consistent with objectKinds (objects.go:21-28).
  • Input validation: finalizePlanting (plantings.go:211-233) enforces finite/positive/bounded radius, finite center, center-in-local-bounds, count range [1, maxExplicitCount], label length, and YYYY-MM-DD dates via validDatePtr. No unbounded or unvalidated user input reaches the store.
  • Injection: all store queries use parameterized ? placeholders (internal/store/plantings.go:62,79,95,131); column lists are static constants (plantingColumns). No string interpolation of user data.
  • Update ordering: applyPlantingPatch runs before visiblePlant/finalizePlanting, so a patched plantId/radiusCm/count is re-validated against the actor and the parent object before persisting (plantings.go:119-127). A patch can't sneak a foreign plant past the check.
  • Conflict path: on ErrVersionConflict the returned current row is enriched via enrichDerived only with the spacing-derived count of the planting's own plant; the actor already holds editor role on the object, so no new data exposure.

I could not identify a material security defect. The only borderline item — enrichDerived reading a plant row without re-running the visibility check (plantings.go:174-179) — only exposes a spacing-derived integer for a plant the prior editor already placed in a garden the actor can edit; not a meaningful leak, so not reported.

🎯 Correctness — No material issues found

Verdict: No material issues found

I reviewed the change through the Correctness lens, verifying against the checked-out code (store layer, domain types, service objectForRole, parseNullableString/parseNullableInt, and the /full enrichment path).

  • Derived-count formula (internal/service/plantings.go:24): re-derived max(1, round(π·r² / spacing²)) from first principles. The unit test cases check out: r=10/sp=10 → π·100/100=3.14→3; r=50/sp=10 → π·2500/100=78.54→79 (Go's math.Round rounds half away from zero); r=30/sp=15 → π·900/225=12.57→13. Non-positive radius/spacing and NaN correctly collapse to the floor of 1. No unit/threshold error.
  • Nullable patch semantics: removedAt: nullparseNullableString yields (nil, true)SetRemovedAt=true, RemovedAt=nil, which applyPlantingPatch writes back as nil (un-remove). removedAt: "…" soft-removes. Absent field → (nil, false) → unchanged. Verified against parseNullableString in internal/api/objects.go:87. Behavior matches the documented contract.
  • Bounds check (finalizePlanting, plantings.go:194): checks |x| ≤ w/2, |y| ≤ h/2 against the object's unrotated local bounds, ignoring RotationDeg. This is semantically correct because plops live in the object's local frame (which rotates with the object), matching the PR's stated model; not a bug.
  • Cross-user masking: UpdatePlanting/DeletePlanting call GetPlanting then objectForRole(..., roleEditor). For a non-editor, objectForRolerequireGardenRole returns ErrNotFound (verified objects.go:124), so a foreign user gets ErrNotFound, not ErrInvalidInput — no existence leak and matches the test expectation.
  • Conflict enrichment: on ErrVersionConflict, the store returns the DB's current row (via GetPlanting, not the locally mutated pl), and the service enriches it with enrichDerived. The API hands that row to writeVersionConflict. Path is sound; the mutated pl is correctly discarded on conflict.
  • /full spacing map: built from ListReferencedPlants (distinct plants used by active plantings), so every active plop's PlantID is in the map; a missing entry would floor to derivedCount(r, 0)=1, a safe fallback. No N+1.
  • visiblePlant on update: re-validated against the post-patch PlantID, so changing plantId to a foreign/unknown plant is rejected with ErrInvalidInput (no existence leak). Correct.

No material correctness defects in this lens.

🧹 Code cleanliness & maintainability — Minor issues

All three JSON findings verified against the actual code. The fourth and fifth draft bullets explicitly state "no issue," so they correctly never appeared in the findings block.

Verdict: Minor issues

  • internal/service/plantings.go:245trimStringPtr diverges from the inline strings.TrimSpace idiom used elsewhere in the service. Confirmed: objects.go:194/:225 and plants.go:141/:153/:159 all trim inline (o.Name = strings.TrimSpace(*p.Name)), whereas plantings.go introduces a trimStringPtr helper used at two call sites (:89, :199). Minor consistency nit; harmless either way.

  • internal/service/service.go:23timeLayout comment claims "every timestamp pansy stores," now newly inaccurate. Confirmed: the comment at service.go:23-25 reads "the ISO-8601 UTC format used for every timestamp pansy stores," but plantings.go:13 adds dateLayout = "2006-01-02" and plantings store date-only planted_at/removed_at via validDatePtr. The two constants legitimately coexist; only the stale "every" wording is misleading. Worth a one-line comment touch-up.

  • internal/service/plantings.go:211finalizePlanting validation order tucks the date checks (:229) last, after unrelated count (:223) and label (:226) checks. Confirmed: order is radius → coords → bounds → count → label → dates. Readability nit only; behavior is correct. Not blocking.

Performance — No material issues found

VERDICT: No material issues found

Performance lens review of the plantings CRUD + derived-count enrichment:

  • GardenFull enrichment (internal/service/objects.go:179-187) — verified clean. It reuses the already-loaded plants slice to build a single spacing map (O(P)), then fills each plop's DerivedCount via map lookup (O(N)). No per-planting query, no N+1. The two underlying store reads (ListActivePlantingsForGarden, ListReferencedPlants) are each a single JOINed query scoped by garden_id.
  • Single create/update responses (internal/service/plantings.go)CreatePlanting/UpdatePlanting each do at most one extra GetPlant (via visiblePlant) to fetch spacing, plus the objectForRole GetObject. That's a constant number of queries per mutation, acceptable for a write path.
  • Conflict path (UpdatePlanting service) — on version mismatch the store re-fetches the current row and the service calls enrichDerived (one more GetPlant). This only runs on the rare conflict branch, not the hot path.
  • derivedCount helper — pure arithmetic, no allocation; fine to call per-row in the /full loop.
  • Store writesCreatePlanting/UpdatePlanting use INSERT/UPDATE ... RETURNING (single round trip), DeletePlanting a single Exec. No row-by-row loops over batches.

No unbounded growth, missing limits, or quadratic behavior introduced. (The ListPlantsForActor 5000-row cap is pre-existing and untouched by this PR.)

🧯 Error handling & edge cases — Minor issues

All four findings confirmed against the actual source code. Let me verify the line references are accurate.

  • Finding 1: derivedCount at line 26-36, the unbounded int(math.Round(...)) is line 31. maxRadiusCM=10000, tiny spacing → unbounded int, no cap on derived path (only maxExplicitCount=1e6 for manual). ✓
  • Finding 2: enrichDerived (lines 174-179) silently ignores GetPlant error; conflict path (lines 132-134) leaves DerivedCount=0 on failure. ✓
  • Finding 3: createPlanting (lines 83-85) hardcodes "a plantId is required" for any ShouldBindJSON failure. ✓
  • Finding 4: validDatePtr (lines 236-242) only validates format, no cross-field/future checks; called at line 229. ✓

Verdict: Minor issues

  • internal/service/plantings.go:31derivedCount has no upper cap (derived path unbounded). area / spacing² (line 31) can be astronomically large for maxRadiusCM = 10_000 against a tiny spacing, and int(math.Round(...)) converts to an int with no ceiling. The manual count override is capped at maxExplicitCount = 1_000_000 (line 223), but the derived path has no equivalent clamp, so a huge-radius / tiny-spacing plop yields an unbounded derivedCount flowing into the JSON response. Not a crash on 64-bit, but an unbounded edge case. Suggested fix: clamp derivedCount to a sane ceiling (e.g. maxExplicitCount or a separate maxDerivedCount).

  • internal/service/plantings.go:174enrichDerived swallows GetPlant error; 409 current row may carry derivedCount: 0. On the version-conflict path (lines 132-134), enrichDerived ignores the GetPlant error (best-effort by design), so the returned current row's DerivedCount stays 0 if the plant lookup fails — inconsistent with a normal GET that would carry a positive value. Low severity; the degraded response carries no signal of the swallowed failure.

  • internal/api/plantings.go:84createPlanting binding error always reports "a plantId is required" even for unrelated JSON/type errors. ShouldBindJSON (line 83) can fail for many reasons (bad JSON syntax, wrong types on xCm/yCm/radiusCm), but the handler always returns INVALID_INPUT with "invalid planting: a plantId is required". radiusCm and xCm/yCm have no validation tags, so a non-numeric value fails binding with a misleading message. Minor diagnostic/UX issue.

  • internal/service/plantings.go:229validDatePtr does not reject future/implausible dates or check removedAt vs plantedAt ordering. validDatePtr (lines 236-242) only validates the YYYY-MM-DD format; removedAt earlier than plantedAt, or plantedAt far in the future, is accepted (line 229). No cross-field constraint exists. Likely acceptable per spec; noted as unverified whether any constraint was intended.

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⚠️ 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

🎯 Correctness — Minor issues

Verdict: Minor issues found

  • internal/service/plantings.go:113UpdatePlanting omits the plantable check. CreatePlanting correctly rejects non-plantable objects (!o.Plantable → domain.ErrInvalidInput), but UpdatePlanting fetches the object via objectForRole and proceeds straight to finalizePlanting without ever verifying o.Plantable. The PR description explicitly states that UpdatePlanting requires a plantable object ("CreatePlanting / UpdatePlanting / DeletePlanting … Object must be plantable"), so this is a gap in the stated contract. Plantings in objects that are (or become) non-plantable can still be moved, resized, or have their plant changed.

    Fix: Add if !o.Plantable { return nil, domain.ErrInvalidInput } after the objectForRole call in UpdatePlanting, mirroring CreatePlanting.

  • internal/service/plantings.go:148DeletePlanting similarly omits the plantable check. Hard-deleting a planting in a non-plantable object is arguably less problematic (it's cleanup), but the PR description groups Delete with the same plantable requirement. If the intent is to allow cleanup, the PR description should be updated; otherwise add the same guard.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • internal/service/plantings.go:245trimStringPtr is a package-private helper that duplicates a one-liner already written inline throughout the codebase (strings.TrimSpace(*p.Label) in applyObjectPatch, applyPlantPatch, etc.). Adding a one-off helper for plantings only is inconsistent and leaks a tiny abstraction. Fix: Drop trimStringPtr and use strings.TrimSpace(*in.Label) / strings.TrimSpace(*p.Label) inline, matching applyObjectPatch and applyPlantPatch. (Verified by reading service/objects.go, service/plants.go, and service/plantings.go.)
Performance — No material issues found

No material issues found.

  • Verified: GardenFull at internal/service/objects.go:181-187 pre-builds a spacing map from already-loaded referenced plants and does O(1) lookups in the plantings loop — no N+1 query pattern.
  • Verified: derivedCount is a pure float computation with no allocations, called once per planting in the loop. Not a hot-path concern.
  • Verified: enrichDerived at internal/service/plantings.go:174-179 does an extra GetPlant only on the version-conflict error path, not on the success path. Appropriate.
  • Verified: applyPlantingPatch at internal/service/plantings.go:182-207 mutates the struct in place with no heap allocations.
  • Verified: plantingUpdateRequest uses json.RawMessage for nullable fields, which holds a slice reference into the existing body rather than copying data upfront.

The PR avoids the classic pitfalls: no queries inside loops, no unnecessary struct copies, no unbounded string building, and the /full enrichment is a single linear pass over already-fetched data.

🧯 Error handling & edge cases — Minor issues

Verdict: Minor issues

Findings:

  • internal/api/plantings.go:83createPlanting swallows the actual ShouldBindJSON error and always emits a fixed message. If the request body contains malformed JSON, wrong types (e.g., "plantId": "abc"), or syntax errors, the client still receives "invalid planting: a plantId is required", which is misleading and makes debugging harder.
    Fix: inspect err (e.g. with errors.As for validator.ValidationErrors / json.UnmarshalTypeError) and return the underlying detail, or at least a generic "invalid request body" when the required field is actually present.

  • internal/api/plantings.go:101updatePlanting swallows the actual ShouldBindJSON error and always emits a fixed message. If the request body contains malformed JSON, wrong types, or syntax errors, the client still receives "invalid update: a current version is required", which is misleading and makes debugging harder.
    Fix: inspect err and return the underlying detail, or at least a generic "invalid request body" when the required field is actually present.

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

Through the security lens I traced the tainted paths and authorization checks:

  • Authn/authz: All planting routes are under requireAuth() (api.go:92-98). Create checks objectForRole(..., roleEditor) before any validation; Update/Delete load the planting then re-resolve its object's garden via objectForRole(..., roleEditor). Cross-user probes return ErrNotFound (verified at service/plantings.go:69-76,108-117,123-127 and confirmed by TestPlantingCrossUserIsNotFound), so no existence leak across the garden boundary.
  • Plant visibility / existence leak: visiblePlant (plantings.go:158-168) collapses both unknown and foreign-owned plants to ErrInvalidInput — same error for "doesn't exist" and "someone else's", so no oracle. Built-ins (OwnerID nil) are referenceable by anyone, which is the intended catalog behavior.
  • SQL injection: All new store queries (GetPlanting, CreatePlanting, UpdatePlanting, DeletePlanting) use ? placeholders exclusively; plantingColumns is a constant identifier list, not user input. No string interpolation of tainted values (store/plantings.go:60-133).
  • Input validation: finalizePlanting (plantings.go:204-232) rejects non-finite/non-positive/oversized radius, non-finite x/y, out-of-bounds center, count outside [1, 1_000_000], label > 200 chars, and malformed YYYY-MM-DD dates. The create request requires plantId via binding; the PATCH requires version ≥ 1. Nullable fields use json.RawMessage + parseNullable* so explicit null (clear) is distinct from absent (unchanged), and parseNullableInt/parseNullableString error on wrong-typed JSON (verified at api/plants.go:63-71, api/objects.go:87-95).
  • Soft-remove semantics: removedAt: "<date>" soft-removes (leaves /full), removedAt: null un-removes; both gated by editor role. No way for a viewer to mutate state.
  • No SSRF / deserialization / secret leakage: No URL handling, no unsafe deserialization (only json.Unmarshal of typed structs), no credentials touched.

I verified the cross-file behavior by reading service/objects.go (objectForRole, requireGardenRole, isFinite), service/gardens.go (role enforcement), api/errors.go (writeServiceError/parseIDParam), api/plants.go/api/objects.go (the shared parseNullable* helpers), and store/plantings.go (query parameterization and the existing /full read).

🎯 Correctness — No material issues found

I'll review this through the Correctness lens, verifying the code against the checked-out repository.
I have enough context. Let me re-verify the critical correctness concerns. Let me re-derive the derivedCount formula and check the visiblePlant ownership check, plus the update-path ordering with pl.Version = version vs DB query ordering.
I've verified the code thoroughly. Let me compile the findings.

Verification summary:

  • derivedCount formula: max(1, round(π·r²/spacing²)). Test cases re-derived correctly: r=10/sp=10 → π·100/100 = 3.14 → round 3 ✓; r=50/sp=10 → π·2500/100 = 78.54 → round 79 ✓; r=30/sp=15 → π·900/225 = 12.57 → round 13 ✓.
  • visiblePlant: p.OwnerID != nil && *p.OwnerID != actorID → ErrInvalidInput; nil OwnerID (built-in) allowed ✓.
  • objectForRole ordering, pl.Version = version then store update — matches the gardens/objects/plants siblings.
  • parseNullableInt/parseNullableString semantics (absent vs null) match the patch flag contract.
  • The ListReferencedPlants / ListActivePlantingsForGarden queries both filter removed_at IS NULL, so the spacing map covers every active planting's plant ✓.
  • enrichDerived in conflict path: updated is the current row returned from the store; uses best-effort GetPlant ✓.

One correctness concern worth flagging: in UpdatePlanting, the success-path DerivedCount is computed from plant.SpacingCM where plant was fetched from s.visiblePlant(...) before the store write. Since plant.SpacingCM is just a read value, that's fine. The enrichDerived fallback also re-fetches. Both are consistent.

VERDICT: No material issues found

  • Formula re-derived from first principles; test expectations match (π·r²/s² rounded, floored at 1; non-positive radius or spacing → 1; NaN guarded). Verified at internal/service/plantings.go:26-36.
  • Nullable patch plumbing (absent vs explicit null) is correctly threaded through parseNullableInt/parseNullableString (verified at internal/api/plants.go:63 and internal/api/objects.go:87) into SetCount/SetLabel/SetPlantedAt/SetRemovedAt flags; applyPlantingPatch honors each. removedAt: null un-removes; removedAt: "<date>" soft-removes; /full filters removed_at IS NULL in both the plantings list and the referenced-plants query — consistent.
  • Authorization flow through objectForRole(..., roleEditor) is reused identically across create/update/delete; visiblePlant masks unknown/foreign plants as ErrInvalidInput (no existence leak), built-ins (OwnerID == nil) allowed — verified against domain.Plant.OwnerID (*int64, nil = built-in).
  • Version-guard contract mirrors the gardens/objects/plants siblings: pl.Version = version set just before store update; on sql.ErrNoRows the store returns (current, ErrVersionConflict); service enriches the conflict-current row via enrichDerived. The API handler's errors.Is(err, ErrVersionConflict) branch writes writeVersionConflict(c, p) with the current row. Verified at internal/store/plantings.go:93-116, internal/service/plantings.go:130-139, internal/api/plantings.go:110-118.
  • Bounds check uses the object's unrotated local half-extents (|x| ≤ w/2, |y| ≤ h/2) as documented; edge values allowed (>, not ), outside rejected. Verified at internal/service/plantings.go:218-222; tests confirm edge (x=100,y=-100 on a ±100 box) passes and x=150 fails.
  • Create defaulting plantedAt to today via injectable s.now() consistent with the rest of the service; validDatePtr validates YYYY-MM-DD for both plantedAt and removedAt.
  • GardenFull enrich path: builds spacing map from ListReferencedPlants (which already filters removed_at IS NULL and joins the same plantings) — no N+1, and every active plop's PlantID is guaranteed present in the map (otherwise spacing defaults to 0 → derivedCount floors to 1, matching the documented "non-positive spacing collapses to 1" rule). Verified at internal/service/objects.go:175-188 and internal/store/plants.go:31-57.
🧹 Code cleanliness & maintainability — Minor issues

Both findings verified against actual code. The doc comments at internal/store/plantings.go:31-32 and internal/service/objects.go:72 are verbatim as the draft claims, and both reference #14 as future work while this PR lands that work.

Review

Verdict: Minor issues

  • internal/store/plantings.go:31-32 — the doc on ListActivePlantingsForGarden still says "empty until plantings CRUD lands in #14" and "Plop CRUD itself is #14; this is only the /full read side." That work has now landed in this very PR (CRUD is defined further down in the same file), making the comment self-contradictory. Suggested fix: drop the "until plantings CRUD lands in #14" sentence, e.g. "Always a non-nil slice; this is the /full read side — plop CRUD lives further down in this file."
  • internal/service/objects.go:71-73FullGarden's doc comment ("Plantings/Plants stay empty until #14 populates plantings; the shape is fixed now so the frontend types against it once.") is now stale: this PR populates both Plantings and Plants in GardenFull. Suggested fix: replace with "Garden plus everything drawn in it (objects, active plantings, and the referenced plants)." This file is touched by the PR's GardenFull change, so it's in scope.

Both are stale-doc nits at churn the PR already touches; no structural or readability defects found. The new plantings.go service file cleanly mirrors the objects.go shape (Create/Update/Delete + applyXxxPatch + finalizeXxx + objectForRole reuse), reuses the existing parseNullableInt/parseNullableString API helpers rather than re-implementing them, and derivedCount is correctly factored as the single source for both the /full enrichment and the single-response paths.

Performance — No material issues found

VERDICT: No material issues found

Through the performance lens, the change is clean:

  • GardenFull (internal/service/objects.go:179-187) enriches derived counts with a single spacing map built from the already-loaded plants slice, then one O(N) pass over plantings. No N+1 — the PR description's claim holds up against the code: ListActivePlantingsForGarden and ListReferencedPlants are each a single JOINed query, and the enrichment is pure map lookups.
  • derivedCount is constant-time math (verified internal/service/plantings.go:23-35); no loops or allocations per call.
  • CreatePlanting / UpdatePlanting / DeletePlanting each issue a small fixed number of point queries (GetPlanting, objectForRoleGetObject+role check, visiblePlantGetPlant, then the write). No per-row loops, no unbounded growth. The conflict path's extra enrichDerived adds one GetPlant only on the rare version-conflict branch — acceptable.
  • applyPlantingPatch / finalizePlanting are in-memory, O(1) field assignments.
  • The spacing map is pre-sized to len(plants); no unbounded growth on the hot /full read.

I considered the lack of LIMIT on ListActivePlantingsForGarden (now reachable with real data via this PR's CRUD), but that function is unchanged by this diff and is bounded by a single garden's plops at household scale; the added enrichment is proportional, not a regression.

🧯 Error handling & edge cases — Minor issues

Both findings confirmed.

VERDICT: Minor issues

  • internal/service/plantings.go:229removedAt earlier than plantedAt is accepted. finalizePlanting calls validDatePtr on each date independently (line 229) and never compares them. A PATCH like {"removedAt":"2020-01-01","version":1} on a plop planted 2026-07-19 soft-removes with a harvest date predating the planting, producing a nonsensical "season-history" row. Suggested fix: when both are non-nil, require removedAt >= plantedAt (string comparison works for YYYY-MM-DD) and return ErrInvalidInput otherwise. Verified by reading finalizePlanting (lines 211–233) — no cross-field check exists.

  • internal/service/plantings.go:174-179enrichDerived can violate the documented max(1, …) floor. On the version-conflict path, if s.store.GetPlant fails, the error is silently dropped and p.DerivedCount is left at its zero value, contradicting the max(1, …) invariant stated in internal/domain/domain.go (DerivedCount comment) and the derivedCount helper. Impact is small (the plant FK is ON DELETE RESTRICT, so this only triggers on a transient DB error) and the code is documented "best-effort," but a 409 current row carrying derivedCount: 0 is still an inconsistency a client could observe. Suggested fix: set p.DerivedCount = 1 on the error branch (or leave the field unset and document that conflict-row enrichment is best-effort/optional) rather than silently leaving 0.

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

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

🔒 Security — No material issues found

No material issues found

Security lens review:

  • Authn/Authz: All new endpoints (POST /objects/:id/plantings, PATCH /plantings/:id, DELETE /plantings/:id) are registered under groups that apply h.requireAuth(). The service layer consistently gates mutations through objectForRole(..., roleEditor), which masks existence of cross-user resources as ErrNotFound (verified by TestPlantingCrossUserIsNotFound). Plant visibility is enforced via visiblePlant, rejecting another user's custom plants with ErrInvalidInput and not leaking existence.
  • Injection: All store methods (GetPlanting, CreatePlanting, UpdatePlanting, DeletePlanting, ListActivePlantingsForGarden) use ? placeholders exclusively; no user-controlled strings are interpolated into SQL.
  • Input validation: finalizePlanting validates radius, coordinates, count, label length, and date formats. parseIDParam rejects non-positive IDs. parseNullableInt/parseNullableString on json.RawMessage correctly distinguish absent vs. explicit-null and reject malformed values.
  • SSRF / unsafe deserialization / secrets: None present in this change.
  • Insecure defaults: planted_at defaults to the current UTC date; omitted count correctly defers to derivedCount. Soft-remove semantics (removed_at) are opt-in via explicit update.

No security-relevant defects were identified.

🎯 Correctness — No material issues found

No material issues found

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

Performance — No material issues found

Verdict: No material issues found

  • The /full read (GardenFull) builds a spacing map from already-fetched plants and does O(1) map look-ups per planting to compute DerivedCount, avoiding any N+1 query pattern. Verified by reading internal/service/objects.go:179-187.
  • Individual CRUD endpoints (CreatePlanting, UpdatePlanting, DeletePlanting) each do a small, bounded number of indexed single-row lookups (GetPlanting, GetObject/requireGardenRole, GetPlant, and the mutating statement). No hot-path regressions.
  • The derivedCount formula is a pure, O(1) mathematical function with no allocations.
  • enrichDerived on the version-conflict error path does issue one extra GetPlant query, but this is an exceptional case and therefore not a hot-path concern.
🧯 Error handling & edge cases — Minor issues

Verdict: Minor issues

Findings:

  • internal/service/plantings.go:26-36derivedCount guards against NaN and non-positive inputs but not +Inf. If corrupted data (or a radius whose square overflows float64 to +Inf) reaches GardenFull, int(math.Round(+Inf)) produces implementation-defined results (possible wrap or panic per the Go spec). finalizePlanting already validates with isFinite (which includes !math.IsInf), so the helper should be symmetrically defensive.

  • internal/service/plantings.go:31 & internal/domain/domain.go:194 — Integer overflow on 32-bit builds. With maxRadiusCM = 10_000 and minPlantSpacingCM = 0.1, the formula yields ~31.4 billion (π·10⁸ / 0.01), which exceeds math.MaxInt32 (~2.1B). On 32-bit Go int(math.Round(...)) silently wraps. DerivedCount (and the existing Count *int) should be int64, or the helper should clamp to math.MaxInt.

  • internal/service/plantings.go:174-179enrichDerived swallows GetPlant errors and leaves DerivedCount at its zero value (0). Since the valid range for derived counts is ≥ 1, returning 0 in a version-conflict response is misleading to the client. Falling back to 1 (the documented floor) would be a safer best-effort value.

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: Minor issues** · 15 findings (5 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟡 | enrichDerived swallows GetPlant error; 409 current row may carry derivedCount 0 inconsistently with normal GET | `internal/service/plantings.go:174` | 3/5 | error-handling | | 🟡 | finalizePlanting never checks removedAt is on/after plantedAt, allowing a nonsensical removed-before-planted interval | `internal/service/plantings.go:229` | 3/5 | error-handling | | 🟠 | derivedCount has no upper bound; huge radius / tiny spacing yields unbounded int (manual count is capped at 1e6 but derived is not) | `internal/service/plantings.go:31` | 2/5 | error-handling | | 🟡 | ShouldBindJSON error swallowed by fixed misleading message in createPlanting | `internal/api/plantings.go:83` | 2/5 | error-handling | | 🟡 | trimStringPtr helper diverges from the inline strings.TrimSpace idiom used elsewhere in the service | `internal/service/plantings.go:245` | 2/5 | maintainability | <details><summary>10 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | derivedCount missing math.IsInf guard — corrupted radius can yield implementation-defined int conversion | `internal/service/plantings.go:26` | opencode/kimi-k2.6:cloud | error-handling | | 🟠 | UpdatePlanting re-validates unchanged position against current object bounds, permanently blocking patches (including soft-remove) after the parent object is resized smaller | `internal/service/plantings.go:107` | claude-code/sonnet | error-handling | | 🟠 | UpdatePlanting missing plantable check | `internal/service/plantings.go:113` | kimi-k2.6:cloud | correctness | | 🟠 | UpdatePlanting always re-validates position bounds against the current object, blocking soft-remove/other edits on plops orphaned outside bounds by a later object resize | `internal/service/plantings.go:119` | claude-code/sonnet | correctness | | 🟡 | ShouldBindJSON error swallowed by fixed misleading message in updatePlanting | `internal/api/plantings.go:101` | kimi-k2.6:cloud | error-handling | | 🟡 | DeletePlanting missing plantable check | `internal/service/plantings.go:148` | kimi-k2.6:cloud | correctness | | 🟡 | timeLayout comment claims 'every timestamp' but plantings now store date-only planted_at/removed_at via a separate dateLayout constant | `internal/service/service.go:23` | glm-5.2:cloud | maintainability | | ⚪ | Stale FullGarden doc says Plantings/Plants stay empty until #14; this PR populates them | `internal/service/objects.go:72` | opencode/glm-5.2:cloud | maintainability | | ⚪ | finalizePlanting validation order tucks date checks after unrelated count/label checks, hurting readability | `internal/service/plantings.go:211` | glm-5.2:cloud | maintainability | | ⚪ | Stale doc on ListActivePlantingsForGarden references #14 as not-yet-landed, but this PR lands it | `internal/store/plantings.go:31` | opencode/glm-5.2:cloud | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> These parse safely (bounded JSON unmarshal, no injection). Authorization, plant-visibility masking, and SQL parameterization all check out. No security issues found in this diff. ## VERDICT: No material issues found Verified via the actual repo code: - **Authorization**: `CreatePlanting`/`UpdatePlanting`/`DeletePlanting` all funnel through `objectForRole` → `requireGardenRole` (`internal/service/objects.go:124`, `internal/service/gardens.go:52`), which enforces role and masks existence with `ErrNotFound` for gardens the actor can't see — consistent with the existing objects/gardens pattern. - **IDOR / existence leak**: `UpdatePlanting`/`DeletePlanting` fetch the planting by raw id first (`internal/service/plantings.go:114,149`), but a not-owned planting's containing garden then fails `objectForRole` with `ErrNotFound` too, so the two failure modes are indistinguishable to the caller — no existence oracle. - **Cross-user plant reference**: `visiblePlant` (`internal/service/plantings.go:~180`) rejects both unknown and another user's custom plants with the same `ErrInvalidInput`, not a distinguishing error — no enumeration of other users' plant IDs. Confirmed built-ins (`OwnerID == nil`) are allowed for anyone. - **SQL injection**: all store queries use parameterized `?` placeholders (`internal/store/plantings.go`); `plantingColumns` is a compile-time constant, never user input. - **Input validation**: `finalizePlanting` bounds radius (`0 < r ≤ 10,000cm`), rejects NaN/Inf via `isFinite`, caps label length (200) and explicit count (1..1,000,000), and validates date strings against a fixed layout (`time.Parse`, no format-string injection risk). - **Nullable-field parsing** (`parseNullableInt`/`parseNullableString`/json.RawMessage): bounded `json.Unmarshal` calls, no panics or type confusion possible from attacker-controlled JSON. - **Auth middleware**: both new route groups (`objects.POST(".../plantings")` and the `plantings` group) sit under `h.requireAuth()` — confirmed in `internal/api/api.go`. Outside my lens: none worth flagging. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> No test covers the resize-then-soft-remove sequence, confirming the draft's claim. The finding is fully verified against the actual code: `applyObjectPatch`/`finalizeObject` in objects.go permit shrinking `WidthCM`/`HeightCM` with no check against existing plantings, and `UpdatePlanting` unconditionally re-validates bounds via `finalizePlanting` using the freshly-fetched current object, regardless of which patch fields were actually set — so a status-only patch (e.g. `removedAt` soft-remove) on a planting orphaned by a later resize is permanently rejected, while `DeletePlanting` bypasses `finalizePlanting` entirely. VERDICT: Minor issues - `internal/service/plantings.go:119-127` (`UpdatePlanting`) unconditionally calls `finalizePlanting(pl, o)` after `applyPlantingPatch`, re-validating the plop's `x_cm`/`y_cm` against the object's *current* `WidthCM`/`HeightCM` even when the patch doesn't touch position (e.g. a pure `removedAt` soft-remove). `internal/service/objects.go:202-207,138-150` lets an editor shrink an object's dimensions via `PATCH /objects/:id` (`applyObjectPatch`) with no check against existing plantings in `finalizeObject`. Once an object is resized smaller than an existing planting's stored position, `finalizePlanting`'s bounds check (`plantings.go:220`: `math.Abs(p.XCM) > o.WidthCM/2 || math.Abs(p.YCM) > o.HeightCM/2`) rejects every subsequent `UpdatePlanting` call with `ErrInvalidInput`, including status-only edits like soft-remove — the only way to clear the stale plop becomes `DeletePlanting` (`plantings.go:144-153`), which skips `finalizePlanting` entirely and hard-deletes it, losing the season-history record soft-remove is meant to preserve. Confirmed no test (`plantings_test.go`) covers a resize-then-update sequence. - Suggested fix: only re-validate geometry (position/radius bounds) when the patch actually changes `XCM`/`YCM`/`RadiusCM`, so status-only patches aren't blocked by a since-changed object size. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> All test helpers are properly reused from existing test files rather than duplicated. The new `plantings_test.go` only adds the two path helpers it genuinely needs (`plantingPath`, `objectPlantingsPath`). VERDICT: No material issues found I reviewed `internal/service/plantings.go`, `internal/api/plantings.go`, `internal/store/plantings.go`, and `internal/domain/domain.go` against the established patterns in `objects.go`/`plants.go` (both service and api layers). The PR is a clean, disciplined follow of existing conventions: - The nullable-field `Set*` flag idiom, `parseNullableInt`/`parseNullableString` (api/objects.go, api/plants.go), `parseIDParam`, `mustActor`, `writeVersionConflict`, `isFinite` are all reused rather than reimplemented. - `derivedCount` is a single well-placed helper reused consistently across create, update, version-conflict enrichment, and `GardenFull` — no copy-pasted formula. - Struct field layout, doc-comment style, and handler structure in `internal/api/plantings.go` mirror `objects.go` almost line-for-line. - Test helpers (`objectPath`, `fullPath`, `doJSON`, `authEngine`, etc.) are reused from existing `_test.go` files; only the two genuinely new path helpers were added. - No dead code, no deep nesting, no oversized functions — `finalizePlanting`/`applyPlantingPatch` mirror the shape of `finalizeObject`/`applyObjectPatch` exactly. Minor, non-blocking observation (not filed as a finding): `enrichDerived` re-fetches the plant via `s.store.GetPlant` on a version conflict rather than reusing the `plant` already fetched earlier in `UpdatePlanting` — but this is necessary, not sloppy, since the conflicting current row can reference a different `plant_id` than the one just validated, so a second fetch is correct, not a maintainability smell. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **VERDICT: No material issues found** I reviewed the new store/service code (`internal/service/plantings.go`, `internal/service/objects.go`'s `GardenFull` enrichment, `internal/store/plantings.go`) specifically for N+1s, hot-loop work, unbounded growth, and missing indexes. - `GardenFull`'s `derivedCount` enrichment (`internal/service/objects.go:179-187`) builds a `map[int64]float64` from the already-loaded `plants` slice and does an O(1) lookup per planting — verified this is a single pass over both slices, not a query per row, so the PR's "no N+1" claim in the description holds up. - `internal/store/plants.go:31-57`'s `ListReferencedPlants` (pre-existing, unchanged by this diff) does the plant/planting join in one query with `DISTINCT`, and `internal/store/migrations/0001_init.sql:138-139` confirms `idx_plantings_object` and `idx_plantings_plant` exist, so the joins used by both `ListActivePlantingsForGarden` and `ListReferencedPlants` are indexed. - `CreatePlanting`/`UpdatePlanting`/`DeletePlanting` (`internal/service/plantings.go`) each issue a small, constant number of single-row queries (`objectForRole` → `GetObject`+role check, `visiblePlant` → `GetPlant`, then one store write) — no loops over collections, matching the existing pattern already used by `objects.go`'s create/update/delete, so this isn't a regression introduced here. - `enrichDerived` (`internal/service/plantings.go:180-186`) only runs on the version-conflict path, not the hot path, and is a single `GetPlant` call. - `derivedCount` itself is pure O(1) arithmetic; no allocation or loop concerns even at the `maxRadiusCM` ceiling. Nothing here introduces an N+1 query, a loop-bound query, unbounded result growth, or a missing index. The diff is clean from a performance standpoint. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both findings confirmed directly against the store layer too — no comparison check exists anywhere in the write path. Both findings from the draft hold up. ## VERDICT: Minor issues - `internal/service/plantings.go:107-140` (`UpdatePlanting`) unconditionally re-validates the plop's position against the object's *current* bounds via `finalizePlanting` (line 125), even when the patch touches neither `XCM` nor `YCM`. `finalizeObject` (`internal/service/objects.go:232-266`) lets an editor shrink `WidthCM`/`HeightCM` at any time with no check against existing plantings inside the object. Once an object is resized smaller than an existing plop's stored position, `finalizePlanting` (`internal/service/plantings.go:220-222`, `math.Abs(p.XCM) > o.WidthCM/2 || math.Abs(p.YCM) > o.HeightCM/2`) will reject *every* subsequent `PATCH /plantings/:id` on that row — including patches that only touch `label`, `count`, or `plantId`. This also blocks the soft-remove ("clear bed") mechanism, since `removedAt` is set via the same `UpdatePlanting`/`finalizePlanting` path (`internal/api/plantings.go` PATCH handler → `service.PlantingPatch.SetRemovedAt`), while `DeletePlanting` (`internal/service/plantings.go:144-153`) skips `finalizePlanting` entirely and still works. Confirmed by reading `finalizePlanting`, `finalizeObject`, and both the update and delete call paths; no code path re-validates only on touched fields, and no test covers resize-then-patch/soft-remove. - `internal/service/plantings.go:229-231` (`finalizePlanting`) validates `PlantedAt` and `RemovedAt` individually via `validDatePtr` but never checks `RemovedAt` is on/after `PlantedAt`. Confirmed no such comparison exists anywhere in the service or store layer (`internal/store/plantings.go` just persists both fields as given). A client can soft-remove a plop with `removedAt` earlier than `plantedAt`, silently producing a nonsensical interval. Everything else in the draft's "checked out cleanly" section was not re-verified here (out of scope for the flagged findings) but nothing in the reviewed files contradicts those conclusions. </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> **VERDICT: No material issues found** Security review of the plantings CRUD through the authn/authz/injection/validation lens: - **Authorization (editor role):** `CreatePlanting`, `UpdatePlanting`, and `DeletePlanting` all gate through `objectForRole(..., roleEditor)` (`internal/service/plantings.go:70,114,149`), which calls `requireGardenRole` and masks a non-viewable garden as `ErrNotFound` (`internal/service/gardens.go:52-65`). Cross-user create/update/delete all surface as `ErrNotFound` (verified at `plantings_test.go` `TestPlantingCrossUserIsNotFound`). No IDOR. - **Existence masking on plant refs:** `visiblePlant` (`plantings.go:158-170`) collapses both unknown and foreign-owned plants to `ErrInvalidInput` rather than `ErrNotFound`, avoiding a plant-existence oracle. Built-ins (`OwnerID == nil`) are referenceable by anyone. - **Object plantability:** non-plantable kinds rejected with `ErrInvalidInput` (`plantings.go:74-76`), consistent with `objectKinds` (`objects.go:21-28`). - **Input validation:** `finalizePlanting` (`plantings.go:211-233`) enforces finite/positive/bounded radius, finite center, center-in-local-bounds, count range `[1, maxExplicitCount]`, label length, and `YYYY-MM-DD` dates via `validDatePtr`. No unbounded or unvalidated user input reaches the store. - **Injection:** all store queries use parameterized `?` placeholders (`internal/store/plantings.go:62,79,95,131`); column lists are static constants (`plantingColumns`). No string interpolation of user data. - **Update ordering:** `applyPlantingPatch` runs before `visiblePlant`/`finalizePlanting`, so a patched `plantId`/`radiusCm`/`count` is re-validated against the actor and the parent object before persisting (`plantings.go:119-127`). A patch can't sneak a foreign plant past the check. - **Conflict path:** on `ErrVersionConflict` the returned current row is enriched via `enrichDerived` only with the spacing-derived count of the planting's own plant; the actor already holds editor role on the object, so no new data exposure. I could not identify a material security defect. The only borderline item — `enrichDerived` reading a plant row without re-running the visibility check (`plantings.go:174-179`) — only exposes a spacing-derived integer for a plant the prior editor already placed in a garden the actor can edit; not a meaningful leak, so not reported. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> ## Verdict: No material issues found I reviewed the change through the Correctness lens, verifying against the checked-out code (store layer, domain types, service `objectForRole`, `parseNullableString`/`parseNullableInt`, and the `/full` enrichment path). - **Derived-count formula** (`internal/service/plantings.go:24`): re-derived `max(1, round(π·r² / spacing²))` from first principles. The unit test cases check out: r=10/sp=10 → π·100/100=3.14→3; r=50/sp=10 → π·2500/100=78.54→79 (Go's `math.Round` rounds half away from zero); r=30/sp=15 → π·900/225=12.57→13. Non-positive radius/spacing and NaN correctly collapse to the floor of 1. No unit/threshold error. - **Nullable patch semantics**: `removedAt: null` → `parseNullableString` yields `(nil, true)` → `SetRemovedAt=true, RemovedAt=nil`, which `applyPlantingPatch` writes back as nil (un-remove). `removedAt: "…"` soft-removes. Absent field → `(nil, false)` → unchanged. Verified against `parseNullableString` in `internal/api/objects.go:87`. Behavior matches the documented contract. - **Bounds check** (`finalizePlanting`, `plantings.go:194`): checks `|x| ≤ w/2`, `|y| ≤ h/2` against the object's **unrotated** local bounds, ignoring `RotationDeg`. This is semantically correct because plops live in the object's local frame (which rotates with the object), matching the PR's stated model; not a bug. - **Cross-user masking**: `UpdatePlanting`/`DeletePlanting` call `GetPlanting` then `objectForRole(..., roleEditor)`. For a non-editor, `objectForRole` → `requireGardenRole` returns `ErrNotFound` (verified `objects.go:124`), so a foreign user gets `ErrNotFound`, not `ErrInvalidInput` — no existence leak and matches the test expectation. - **Conflict enrichment**: on `ErrVersionConflict`, the store returns the DB's *current* row (via `GetPlanting`, not the locally mutated `pl`), and the service enriches it with `enrichDerived`. The API hands that row to `writeVersionConflict`. Path is sound; the mutated `pl` is correctly discarded on conflict. - **`/full` spacing map**: built from `ListReferencedPlants` (distinct plants used by active plantings), so every active plop's `PlantID` is in the map; a missing entry would floor to `derivedCount(r, 0)=1`, a safe fallback. No N+1. - **`visiblePlant` on update**: re-validated against the *post-patch* `PlantID`, so changing `plantId` to a foreign/unknown plant is rejected with `ErrInvalidInput` (no existence leak). Correct. No material correctness defects in this lens. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All three JSON findings verified against the actual code. The fourth and fifth draft bullets explicitly state "no issue," so they correctly never appeared in the findings block. ## Verdict: Minor issues - **`internal/service/plantings.go:245` — `trimStringPtr` diverges from the inline `strings.TrimSpace` idiom used elsewhere in the service.** Confirmed: `objects.go:194`/`:225` and `plants.go:141`/`:153`/`:159` all trim inline (`o.Name = strings.TrimSpace(*p.Name)`), whereas `plantings.go` introduces a `trimStringPtr` helper used at two call sites (`:89`, `:199`). Minor consistency nit; harmless either way. - **`internal/service/service.go:23` — `timeLayout` comment claims "every timestamp pansy stores," now newly inaccurate.** Confirmed: the comment at `service.go:23-25` reads "the ISO-8601 UTC format used for every timestamp pansy stores," but `plantings.go:13` adds `dateLayout = "2006-01-02"` and plantings store date-only `planted_at`/`removed_at` via `validDatePtr`. The two constants legitimately coexist; only the stale "every" wording is misleading. Worth a one-line comment touch-up. - **`internal/service/plantings.go:211` — `finalizePlanting` validation order tucks the date checks (`:229`) last, after unrelated count (`:223`) and label (`:226`) checks.** Confirmed: order is radius → coords → bounds → count → label → dates. Readability nit only; behavior is correct. Not blocking. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> VERDICT: No material issues found Performance lens review of the plantings CRUD + derived-count enrichment: - **`GardenFull` enrichment (`internal/service/objects.go:179-187`)** — verified clean. It reuses the already-loaded `plants` slice to build a single `spacing` map (O(P)), then fills each plop's `DerivedCount` via map lookup (O(N)). No per-planting query, no N+1. The two underlying store reads (`ListActivePlantingsForGarden`, `ListReferencedPlants`) are each a single JOINed query scoped by `garden_id`. - **Single create/update responses (`internal/service/plantings.go`)** — `CreatePlanting`/`UpdatePlanting` each do at most one extra `GetPlant` (via `visiblePlant`) to fetch spacing, plus the `objectForRole` `GetObject`. That's a constant number of queries per mutation, acceptable for a write path. - **Conflict path (`UpdatePlanting` service)** — on version mismatch the store re-fetches the current row and the service calls `enrichDerived` (one more `GetPlant`). This only runs on the rare conflict branch, not the hot path. - **`derivedCount` helper** — pure arithmetic, no allocation; fine to call per-row in the /full loop. - **Store writes** — `CreatePlanting`/`UpdatePlanting` use `INSERT/UPDATE ... RETURNING` (single round trip), `DeletePlanting` a single `Exec`. No row-by-row loops over batches. No unbounded growth, missing limits, or quadratic behavior introduced. (The `ListPlantsForActor` 5000-row cap is pre-existing and untouched by this PR.) </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> All four findings confirmed against the actual source code. Let me verify the line references are accurate. - Finding 1: `derivedCount` at line 26-36, the unbounded `int(math.Round(...))` is line 31. `maxRadiusCM=10000`, tiny spacing → unbounded int, no cap on derived path (only `maxExplicitCount=1e6` for manual). ✓ - Finding 2: `enrichDerived` (lines 174-179) silently ignores `GetPlant` error; conflict path (lines 132-134) leaves `DerivedCount=0` on failure. ✓ - Finding 3: `createPlanting` (lines 83-85) hardcodes "a plantId is required" for any `ShouldBindJSON` failure. ✓ - Finding 4: `validDatePtr` (lines 236-242) only validates format, no cross-field/future checks; called at line 229. ✓ # Verdict: Minor issues - **`internal/service/plantings.go:31` — `derivedCount` has no upper cap (derived path unbounded).** `area / spacing²` (line 31) can be astronomically large for `maxRadiusCM = 10_000` against a tiny spacing, and `int(math.Round(...))` converts to an `int` with no ceiling. The manual `count` override is capped at `maxExplicitCount = 1_000_000` (line 223), but the derived path has no equivalent clamp, so a huge-radius / tiny-spacing plop yields an unbounded `derivedCount` flowing into the JSON response. Not a crash on 64-bit, but an unbounded edge case. Suggested fix: clamp `derivedCount` to a sane ceiling (e.g. `maxExplicitCount` or a separate `maxDerivedCount`). - **`internal/service/plantings.go:174` — `enrichDerived` swallows `GetPlant` error; 409 current row may carry `derivedCount: 0`.** On the version-conflict path (lines 132-134), `enrichDerived` ignores the `GetPlant` error (best-effort by design), so the returned `current` row's `DerivedCount` stays 0 if the plant lookup fails — inconsistent with a normal GET that would carry a positive value. Low severity; the degraded response carries no signal of the swallowed failure. - **`internal/api/plantings.go:84` — `createPlanting` binding error always reports "a plantId is required" even for unrelated JSON/type errors.** `ShouldBindJSON` (line 83) can fail for many reasons (bad JSON syntax, wrong types on `xCm`/`yCm`/`radiusCm`), but the handler always returns `INVALID_INPUT` with "invalid planting: a plantId is required". `radiusCm` and `xCm`/`yCm` have no validation tags, so a non-numeric value fails binding with a misleading message. Minor diagnostic/UX issue. - **`internal/service/plantings.go:229` — `validDatePtr` does not reject future/implausible dates or check `removedAt` vs `plantedAt` ordering.** `validDatePtr` (lines 236-242) only validates the `YYYY-MM-DD` format; `removedAt` earlier than `plantedAt`, or `plantedAt` far in the future, is accepted (line 229). No cross-field constraint exists. Likely acceptable per spec; noted as unverified whether any constraint was intended. </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> — ⚠️ 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>🎯 Correctness</b> — Minor issues</summary> **Verdict:** Minor issues found - **`internal/service/plantings.go:113` — `UpdatePlanting` omits the plantable check.** `CreatePlanting` correctly rejects non-plantable objects (`!o.Plantable → domain.ErrInvalidInput`), but `UpdatePlanting` fetches the object via `objectForRole` and proceeds straight to `finalizePlanting` without ever verifying `o.Plantable`. The PR description explicitly states that UpdatePlanting requires a plantable object ("CreatePlanting / UpdatePlanting / DeletePlanting … Object must be `plantable`"), so this is a gap in the stated contract. Plantings in objects that are (or become) non-plantable can still be moved, resized, or have their plant changed. **Fix:** Add `if !o.Plantable { return nil, domain.ErrInvalidInput }` after the `objectForRole` call in `UpdatePlanting`, mirroring `CreatePlanting`. - **`internal/service/plantings.go:148` — `DeletePlanting` similarly omits the plantable check.** Hard-deleting a planting in a non-plantable object is arguably less problematic (it's cleanup), but the PR description groups Delete with the same plantable requirement. If the intent is to allow cleanup, the PR description should be updated; otherwise add the same guard. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - **`internal/service/plantings.go:245`** — `trimStringPtr` is a package-private helper that duplicates a one-liner already written inline throughout the codebase (`strings.TrimSpace(*p.Label)` in `applyObjectPatch`, `applyPlantPatch`, etc.). Adding a one-off helper for plantings only is inconsistent and leaks a tiny abstraction. **Fix:** Drop `trimStringPtr` and use `strings.TrimSpace(*in.Label)` / `strings.TrimSpace(*p.Label)` inline, matching `applyObjectPatch` and `applyPlantPatch`. *(Verified by reading `service/objects.go`, `service/plants.go`, and `service/plantings.go`.)* </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found. - **Verified:** `GardenFull` at `internal/service/objects.go:181-187` pre-builds a `spacing` map from already-loaded referenced plants and does O(1) lookups in the `plantings` loop — no N+1 query pattern. - **Verified:** `derivedCount` is a pure float computation with no allocations, called once per planting in the loop. Not a hot-path concern. - **Verified:** `enrichDerived` at `internal/service/plantings.go:174-179` does an extra `GetPlant` only on the version-conflict error path, not on the success path. Appropriate. - **Verified:** `applyPlantingPatch` at `internal/service/plantings.go:182-207` mutates the struct in place with no heap allocations. - **Verified:** `plantingUpdateRequest` uses `json.RawMessage` for nullable fields, which holds a slice reference into the existing body rather than copying data upfront. The PR avoids the classic pitfalls: no queries inside loops, no unnecessary struct copies, no unbounded string building, and the `/full` enrichment is a single linear pass over already-fetched data. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Verdict:** Minor issues **Findings:** - **`internal/api/plantings.go:83`** — `createPlanting` swallows the actual `ShouldBindJSON` error and always emits a fixed message. If the request body contains malformed JSON, wrong types (e.g., `"plantId": "abc"`), or syntax errors, the client still receives *"invalid planting: a plantId is required"*, which is misleading and makes debugging harder. **Fix:** inspect `err` (e.g. with `errors.As` for `validator.ValidationErrors` / `json.UnmarshalTypeError`) and return the underlying detail, or at least a generic "invalid request body" when the required field is actually present. - **`internal/api/plantings.go:101`** — `updatePlanting` swallows the actual `ShouldBindJSON` error and always emits a fixed message. If the request body contains malformed JSON, wrong types, or syntax errors, the client still receives *"invalid update: a current version is required"*, which is misleading and makes debugging harder. **Fix:** inspect `err` and return the underlying detail, or at least a generic "invalid request body" when the required field is actually present. </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 Through the security lens I traced the tainted paths and authorization checks: - **Authn/authz**: All planting routes are under `requireAuth()` (`api.go:92-98`). Create checks `objectForRole(..., roleEditor)` before any validation; Update/Delete load the planting then re-resolve its object's garden via `objectForRole(..., roleEditor)`. Cross-user probes return `ErrNotFound` (verified at `service/plantings.go:69-76,108-117,123-127` and confirmed by `TestPlantingCrossUserIsNotFound`), so no existence leak across the garden boundary. - **Plant visibility / existence leak**: `visiblePlant` (`plantings.go:158-168`) collapses both unknown and foreign-owned plants to `ErrInvalidInput` — same error for "doesn't exist" and "someone else's", so no oracle. Built-ins (OwnerID nil) are referenceable by anyone, which is the intended catalog behavior. - **SQL injection**: All new store queries (`GetPlanting`, `CreatePlanting`, `UpdatePlanting`, `DeletePlanting`) use `?` placeholders exclusively; `plantingColumns` is a constant identifier list, not user input. No string interpolation of tainted values (`store/plantings.go:60-133`). - **Input validation**: `finalizePlanting` (`plantings.go:204-232`) rejects non-finite/non-positive/oversized radius, non-finite x/y, out-of-bounds center, count outside `[1, 1_000_000]`, label > 200 chars, and malformed `YYYY-MM-DD` dates. The create request requires `plantId` via binding; the PATCH requires `version ≥ 1`. Nullable fields use `json.RawMessage` + `parseNullable*` so explicit `null` (clear) is distinct from absent (unchanged), and `parseNullableInt`/`parseNullableString` error on wrong-typed JSON (verified at `api/plants.go:63-71`, `api/objects.go:87-95`). - **Soft-remove semantics**: `removedAt: "<date>"` soft-removes (leaves `/full`), `removedAt: null` un-removes; both gated by editor role. No way for a viewer to mutate state. - **No SSRF / deserialization / secret leakage**: No URL handling, no unsafe deserialization (only `json.Unmarshal` of typed structs), no credentials touched. I verified the cross-file behavior by reading `service/objects.go` (`objectForRole`, `requireGardenRole`, `isFinite`), `service/gardens.go` (role enforcement), `api/errors.go` (`writeServiceError`/`parseIDParam`), `api/plants.go`/`api/objects.go` (the shared `parseNullable*` helpers), and `store/plantings.go` (query parameterization and the existing `/full` read). </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I'll review this through the Correctness lens, verifying the code against the checked-out repository. I have enough context. Let me re-verify the critical correctness concerns. Let me re-derive the derivedCount formula and check the visiblePlant ownership check, plus the update-path ordering with `pl.Version = version` vs DB query ordering. I've verified the code thoroughly. Let me compile the findings. **Verification summary:** - `derivedCount` formula: `max(1, round(π·r²/spacing²))`. Test cases re-derived correctly: r=10/sp=10 → π·100/100 = 3.14 → round 3 ✓; r=50/sp=10 → π·2500/100 = 78.54 → round 79 ✓; r=30/sp=15 → π·900/225 = 12.57 → round 13 ✓. - `visiblePlant`: `p.OwnerID != nil && *p.OwnerID != actorID` → ErrInvalidInput; nil OwnerID (built-in) allowed ✓. - `objectForRole` ordering, `pl.Version = version` then store update — matches the gardens/objects/plants siblings. - `parseNullableInt`/`parseNullableString` semantics (absent vs null) match the patch flag contract. - The `ListReferencedPlants` / `ListActivePlantingsForGarden` queries both filter `removed_at IS NULL`, so the spacing map covers every active planting's plant ✓. - `enrichDerived` in conflict path: `updated` is the current row returned from the store; uses best-effort GetPlant ✓. One correctness concern worth flagging: in `UpdatePlanting`, the success-path `DerivedCount` is computed from `plant.SpacingCM` where `plant` was fetched from `s.visiblePlant(...)` *before* the store write. Since `plant.SpacingCM` is just a read value, that's fine. The `enrichDerived` fallback also re-fetches. Both are consistent. **VERDICT: No material issues found** - Formula re-derived from first principles; test expectations match (`π·r²/s²` rounded, floored at 1; non-positive radius or spacing → 1; NaN guarded). Verified at `internal/service/plantings.go:26-36`. - Nullable patch plumbing (absent vs explicit null) is correctly threaded through `parseNullableInt`/`parseNullableString` (verified at `internal/api/plants.go:63` and `internal/api/objects.go:87`) into `SetCount`/`SetLabel`/`SetPlantedAt`/`SetRemovedAt` flags; `applyPlantingPatch` honors each. `removedAt: null` un-removes; `removedAt: "<date>"` soft-removes; `/full` filters `removed_at IS NULL` in both the plantings list and the referenced-plants query — consistent. - Authorization flow through `objectForRole(..., roleEditor)` is reused identically across create/update/delete; `visiblePlant` masks unknown/foreign plants as `ErrInvalidInput` (no existence leak), built-ins (`OwnerID == nil`) allowed — verified against `domain.Plant.OwnerID` (`*int64`, nil = built-in). - Version-guard contract mirrors the gardens/objects/plants siblings: `pl.Version = version` set just before store update; on `sql.ErrNoRows` the store returns `(current, ErrVersionConflict)`; service enriches the conflict-current row via `enrichDerived`. The API handler's `errors.Is(err, ErrVersionConflict)` branch writes `writeVersionConflict(c, p)` with the current row. Verified at `internal/store/plantings.go:93-116`, `internal/service/plantings.go:130-139`, `internal/api/plantings.go:110-118`. - Bounds check uses the object's unrotated local half-extents (`|x| ≤ w/2`, `|y| ≤ h/2`) as documented; edge values allowed (`>`, not `≥`), outside rejected. Verified at `internal/service/plantings.go:218-222`; tests confirm edge (x=100,y=-100 on a ±100 box) passes and x=150 fails. - Create defaulting `plantedAt` to today via injectable `s.now()` consistent with the rest of the service; `validDatePtr` validates `YYYY-MM-DD` for both `plantedAt` and `removedAt`. - `GardenFull` enrich path: builds spacing map from `ListReferencedPlants` (which already filters `removed_at IS NULL` and joins the same plantings) — no N+1, and every active plop's `PlantID` is guaranteed present in the map (otherwise spacing defaults to 0 → `derivedCount` floors to 1, matching the documented "non-positive spacing collapses to 1" rule). Verified at `internal/service/objects.go:175-188` and `internal/store/plants.go:31-57`. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Both findings verified against actual code. The doc comments at `internal/store/plantings.go:31-32` and `internal/service/objects.go:72` are verbatim as the draft claims, and both reference #14 as future work while this PR lands that work. ## Review **Verdict: Minor issues** - `internal/store/plantings.go:31-32` — the doc on `ListActivePlantingsForGarden` still says "empty until plantings CRUD lands in #14" and "Plop CRUD itself is #14; this is only the /full read side." That work has now landed in this very PR (CRUD is defined further down in the same file), making the comment self-contradictory. Suggested fix: drop the "until plantings CRUD lands in #14" sentence, e.g. "Always a non-nil slice; this is the /full read side — plop CRUD lives further down in this file." - `internal/service/objects.go:71-73` — `FullGarden`'s doc comment ("Plantings/Plants stay empty until #14 populates plantings; the shape is fixed now so the frontend types against it once.") is now stale: this PR populates both `Plantings` and `Plants` in `GardenFull`. Suggested fix: replace with "Garden plus everything drawn in it (objects, active plantings, and the referenced plants)." This file is touched by the PR's `GardenFull` change, so it's in scope. Both are stale-doc nits at churn the PR already touches; no structural or readability defects found. The new `plantings.go` service file cleanly mirrors the `objects.go` shape (`Create`/`Update`/`Delete` + `applyXxxPatch` + `finalizeXxx` + `objectForRole` reuse), reuses the existing `parseNullableInt`/`parseNullableString` API helpers rather than re-implementing them, and `derivedCount` is correctly factored as the single source for both the `/full` enrichment and the single-response paths. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> VERDICT: No material issues found Through the performance lens, the change is clean: - `GardenFull` (`internal/service/objects.go:179-187`) enriches derived counts with a single `spacing` map built from the already-loaded `plants` slice, then one O(N) pass over `plantings`. No N+1 — the PR description's claim holds up against the code: `ListActivePlantingsForGarden` and `ListReferencedPlants` are each a single JOINed query, and the enrichment is pure map lookups. - `derivedCount` is constant-time math (verified `internal/service/plantings.go:23-35`); no loops or allocations per call. - `CreatePlanting` / `UpdatePlanting` / `DeletePlanting` each issue a small fixed number of point queries (`GetPlanting`, `objectForRole` → `GetObject`+role check, `visiblePlant` → `GetPlant`, then the write). No per-row loops, no unbounded growth. The conflict path's extra `enrichDerived` adds one `GetPlant` only on the rare version-conflict branch — acceptable. - `applyPlantingPatch` / `finalizePlanting` are in-memory, O(1) field assignments. - The `spacing` map is pre-sized to `len(plants)`; no unbounded growth on the hot `/full` read. I considered the lack of `LIMIT` on `ListActivePlantingsForGarden` (now reachable with real data via this PR's CRUD), but that function is unchanged by this diff and is bounded by a single garden's plops at household scale; the added enrichment is proportional, not a regression. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both findings confirmed. ## VERDICT: Minor issues - **`internal/service/plantings.go:229` — `removedAt` earlier than `plantedAt` is accepted.** `finalizePlanting` calls `validDatePtr` on each date independently (line 229) and never compares them. A `PATCH` like `{"removedAt":"2020-01-01","version":1}` on a plop planted `2026-07-19` soft-removes with a harvest date predating the planting, producing a nonsensical "season-history" row. Suggested fix: when both are non-nil, require `removedAt >= plantedAt` (string comparison works for `YYYY-MM-DD`) and return `ErrInvalidInput` otherwise. Verified by reading `finalizePlanting` (lines 211–233) — no cross-field check exists. - **`internal/service/plantings.go:174-179` — `enrichDerived` can violate the documented `max(1, …)` floor.** On the version-conflict path, if `s.store.GetPlant` fails, the error is silently dropped and `p.DerivedCount` is left at its zero value, contradicting the `max(1, …)` invariant stated in `internal/domain/domain.go` (DerivedCount comment) and the `derivedCount` helper. Impact is small (the plant FK is `ON DELETE RESTRICT`, so this only triggers on a transient DB error) and the code is documented "best-effort," but a 409 `current` row carrying `derivedCount: 0` is still an inconsistency a client could observe. Suggested fix: set `p.DerivedCount = 1` on the error branch (or leave the field unset and document that conflict-row enrichment is best-effort/optional) rather than silently leaving 0. </details> </details> <details><summary><b>opencode/kimi-k2.6: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> No material issues found *Security lens review*: - **Authn/Authz**: All new endpoints (`POST /objects/:id/plantings`, `PATCH /plantings/:id`, `DELETE /plantings/:id`) are registered under groups that apply `h.requireAuth()`. The service layer consistently gates mutations through `objectForRole(..., roleEditor)`, which masks existence of cross-user resources as `ErrNotFound` (verified by `TestPlantingCrossUserIsNotFound`). Plant visibility is enforced via `visiblePlant`, rejecting another user's custom plants with `ErrInvalidInput` and not leaking existence. - **Injection**: All store methods (`GetPlanting`, `CreatePlanting`, `UpdatePlanting`, `DeletePlanting`, `ListActivePlantingsForGarden`) use `?` placeholders exclusively; no user-controlled strings are interpolated into SQL. - **Input validation**: `finalizePlanting` validates radius, coordinates, count, label length, and date formats. `parseIDParam` rejects non-positive IDs. `parseNullableInt`/`parseNullableString` on `json.RawMessage` correctly distinguish absent vs. explicit-null and reject malformed values. - **SSRF / unsafe deserialization / secrets**: None present in this change. - **Insecure defaults**: `planted_at` defaults to the current UTC date; omitted `count` correctly defers to `derivedCount`. Soft-remove semantics (`removed_at`) are opt-in via explicit update. No security-relevant defects were identified. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> No material issues found </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> No material issues found </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **Verdict:** No material issues found - The `/full` read (`GardenFull`) builds a `spacing` map from already-fetched plants and does O(1) map look-ups per planting to compute `DerivedCount`, avoiding any N+1 query pattern. Verified by reading `internal/service/objects.go:179-187`. - Individual CRUD endpoints (`CreatePlanting`, `UpdatePlanting`, `DeletePlanting`) each do a small, bounded number of indexed single-row lookups (`GetPlanting`, `GetObject`/`requireGardenRole`, `GetPlant`, and the mutating statement). No hot-path regressions. - The `derivedCount` formula is a pure, O(1) mathematical function with no allocations. - `enrichDerived` on the version-conflict error path does issue one extra `GetPlant` query, but this is an exceptional case and therefore not a hot-path concern. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Verdict:** Minor issues **Findings:** * **`internal/service/plantings.go:26-36`** — `derivedCount` guards against `NaN` and non-positive inputs but **not `+Inf`**. If corrupted data (or a radius whose square overflows float64 to `+Inf`) reaches `GardenFull`, `int(math.Round(+Inf))` produces implementation-defined results (possible wrap or panic per the Go spec). `finalizePlanting` already validates with `isFinite` (which includes `!math.IsInf`), so the helper should be symmetrically defensive. * **`internal/service/plantings.go:31` & `internal/domain/domain.go:194`** — Integer overflow on 32-bit builds. With `maxRadiusCM = 10_000` and `minPlantSpacingCM = 0.1`, the formula yields ~31.4 billion (`π·10⁸ / 0.01`), which exceeds `math.MaxInt32` (~2.1B). On 32-bit Go `int(math.Round(...))` silently wraps. `DerivedCount` (and the existing `Count *int`) should be `int64`, or the helper should clamp to `math.MaxInt`. * **`internal/service/plantings.go:174-179`** — `enrichDerived` swallows `GetPlant` errors and leaves `DerivedCount` at its zero value (`0`). Since the valid range for derived counts is `≥ 1`, returning `0` in a version-conflict response is misleading to the client. Falling back to `1` (the documented floor) would be a safer best-effort value. </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-19 02:39:50 +00:00
Address Gadfly review on #14: bounds trap, date order, count cap
Build image / build-and-push (push) Successful in 5s
8f736048b9
- UpdatePlanting: only re-check the object-bounds when the position is actually
  being moved. A plop orphaned outside its object by a later resize stays
  editable/removable instead of becoming a row you can't fix or delete.
- finalizePlanting: reject removed_at before planted_at.
- derivedCount: guard Inf (not just NaN) and cap the result at maxExplicitCount
  (1e6) — the same ceiling a manual override honors — so a huge radius / tiny
  spacing can't overflow a 32-bit int or return an absurd value.
- Refresh stale docs that referenced #14 as not-yet-landed (FullGarden,
  ListActivePlantingsForGarden, ListReferencedPlants) and note the date-only
  layout beside timeLayout.

Deliberately did NOT add a plantable re-check to Update/Delete: existing plops
must stay editable/removable even if their object was later marked non-plantable
(same trap as the bounds case).

Tests: derived-count cap, removed-before-planted rejection, and edit/move of a
plop orphaned by an object shrink.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve merged commit f4e5dab98c into main 2026-07-19 02:41:22 +00:00
steve deleted branch phase-5-plantings-api 2026-07-19 02:41:23 +00:00
Sign in to join this conversation.