Garden objects backend: polymorphic CRUD + /full endpoint (#10) #28

Merged
steve merged 2 commits from phase-3-objects-api into main 2026-07-18 23:48:40 +00:00
Owner

Closes #10. Phase 3, backend half. Follows the #7 service conventions (actor, requireGardenRole, version-guard/409).

What's here

  • store/objects.goscanObject + Create (RETURNING), Get, ListObjectsForGarden (z_index order), version-guarded Update (RETURNING, returns the current row on conflict), Delete (plantings cascade via the 0001 FK).
  • store/plantings.go + plants.go — the read side /full needs: ListActivePlantingsForGarden (removed_at IS NULL) and ListReferencedPlants (distinct plants used by active plantings). Both return empty until #14 adds plantings; #12/#14 extend these files with their CRUD/seeds.
  • service/objects.goCreateObject / UpdateObject (partial patch) / DeleteObject / GardenFull, all (ctx, actorID, …) through requireGardenRole (editor for mutations, viewer for /full). finalizeObject validates kind + shape (rect/circle; polygon reserved → rejected), finite dims in [1cm,100m], NaN/Inf rejection, a loose bbox-overlaps-garden placement check (partial overhang allowed, fully off-field rejected), hex color, valid-JSON props, name/notes caps; normalizes rotation to [0,360). Plantable defaults by kind (bed/bag/container/in-ground → true; tree/path/structure → false), overridable.
  • api/objects.goPOST /gardens/:id/objects, PATCH /objects/:id, DELETE /objects/:id, GET /gardens/:id/full. /full returns {garden, objects, plantings, plants} (plantings/plants empty, non-null — shape fixed now so the frontend types once). props is any JSON value stored as text; PATCH is partial with a required version, and a 409 returns the current row.

Verification

  • go build/vet/test ./... green (CGO off, GOWORK=off).
  • Service tests: defaults, plantable-by-kind (+ override), rotation normalization (450→90, −90→270), validation rejects (bad kind, polygon, zero/negative dims, fully-off-field, bad color, invalid props JSON, over-long name), edge overhang allowed, partial patch leaves other fields intact + version conflict returns current, cross-user ErrNotFound on every op, delete, /full shape.
  • API tests: full CRUD + /full, 409 envelope, cross-user 404, polygon/no-kind → 400, props object round-trips to a stored JSON string.

Out of scope

  • Plantings CRUD (#14) — /full reads them but they can't exist yet. The field editor UI (#11), the canvas (#9). Polygon shapes (post-v1).

🤖 Generated with Claude Code

Closes #10. Phase 3, backend half. Follows the #7 service conventions (actor, `requireGardenRole`, version-guard/409). ## What's here - **`store/objects.go`** — `scanObject` + Create (`RETURNING`), Get, `ListObjectsForGarden` (z_index order), version-guarded Update (`RETURNING`, returns the current row on conflict), Delete (plantings cascade via the 0001 FK). - **`store/plantings.go` + `plants.go`** — the read side `/full` needs: `ListActivePlantingsForGarden` (`removed_at IS NULL`) and `ListReferencedPlants` (distinct plants used by active plantings). Both return **empty** until #14 adds plantings; #12/#14 extend these files with their CRUD/seeds. - **`service/objects.go`** — `CreateObject` / `UpdateObject` (partial patch) / `DeleteObject` / `GardenFull`, all `(ctx, actorID, …)` through `requireGardenRole` (editor for mutations, viewer for `/full`). `finalizeObject` validates kind + shape (**rect/circle**; polygon reserved → rejected), finite dims in `[1cm,100m]`, NaN/Inf rejection, a **loose bbox-overlaps-garden** placement check (partial overhang allowed, fully off-field rejected), hex color, valid-JSON props, name/notes caps; normalizes rotation to `[0,360)`. **Plantable defaults by kind** (bed/bag/container/in-ground → true; tree/path/structure → false), overridable. - **`api/objects.go`** — `POST /gardens/:id/objects`, `PATCH /objects/:id`, `DELETE /objects/:id`, `GET /gardens/:id/full`. `/full` returns `{garden, objects, plantings, plants}` (plantings/plants empty, non-null — shape fixed now so the frontend types once). `props` is any JSON value stored as text; PATCH is partial with a required `version`, and a 409 returns the current row. ## Verification - `go build`/`vet`/`test ./...` green (CGO off, GOWORK=off). - Service tests: defaults, plantable-by-kind (+ override), rotation normalization (450→90, −90→270), validation rejects (bad kind, polygon, zero/negative dims, fully-off-field, bad color, invalid props JSON, over-long name), edge overhang allowed, partial patch leaves other fields intact + version conflict returns current, cross-user `ErrNotFound` on every op, delete, `/full` shape. - API tests: full CRUD + `/full`, 409 envelope, cross-user 404, polygon/no-kind → 400, `props` object round-trips to a stored JSON string. ## Out of scope - Plantings CRUD (#14) — `/full` reads them but they can't exist yet. The field editor UI (#11), the canvas (#9). Polygon shapes (post-v1). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-18 23:30:48 +00:00
Add garden objects backend: polymorphic CRUD + /full (#10)
Build image / build-and-push (push) Successful in 6s
Gadfly review (reusable) / review (pull_request) Successful in 10m9s
Adversarial Review (Gadfly) / review (pull_request) Successful in 10m9s
3dd935fb19
Garden objects (beds, bags, containers, in-ground, trees, paths,
structures) in one polymorphic table, following the #7 service
conventions.

- store/objects.go: scanObject + Create (RETURNING), Get, ListForGarden
  (z_index order), version-guarded Update (RETURNING, returns current row
  on conflict), Delete (plantings cascade via FK).
- store/plantings.go + plants.go: the read side /full needs now —
  ListActivePlantingsForGarden (removed_at IS NULL) and
  ListReferencedPlants (distinct plants used by active plantings). Both
  return empty until #14 adds plantings; #12/#14 extend these files.
- service/objects.go: Create/Update(partial patch)/Delete/GardenFull, all
  (ctx, actorID, ...) through requireGardenRole(editor for mutations,
  viewer for /full). finalizeObject validates kind + shape (rect/circle;
  polygon reserved), finite dims in [1cm,100m], NaN/Inf rejection, loose
  bbox-overlaps-garden placement (partial overhang OK), hex color, valid
  JSON props, name/notes caps; normalizes rotation to [0,360). Plantable
  defaults by kind, overridable.
- api/objects.go: POST /gardens/:id/objects, PATCH/DELETE /objects/:id,
  GET /gardens/:id/full ({garden,objects,plantings,plants}). props is any
  JSON value stored as text; PATCH is partial + required version, 409
  returns the current row.

Tests: service (defaults, plantable-by-kind, rotation normalize,
validation rejects incl. off-field/polygon/bad-color/bad-props, partial
patch + version conflict, cross-user ErrNotFound, delete, /full shape) and
api (CRUD + /full, 409 envelope, cross-user 404, polygon/no-kind 400,
props round-trip).

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-18 23:40:57Z

claude-code/sonnet · claude-code — done

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

glm-5.2:cloud · ollama-cloud — done

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

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

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

opencode/glm-5.2:cloud · opencode — done

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

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

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

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

🪰 Gadfly consensus review — 16 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** — 16 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -81,6 +81,14 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
gardens.GET("/:id", h.getGarden)
gardens.PATCH("/:id", h.updateGarden)
gardens.DELETE("/:id", h.deleteGarden)
gardens.GET("/:id/full", h.gardenFull) // one-shot editor load

🟡 Handler name gardenFull breaks verbNoun convention

maintainability · flagged by 1 model

  • internal/api/api.go:84 — Handler registration uses h.gardenFull, breaking the verbNoun naming convention every other handler follows (listGardens, createGarden, getGarden, updateGarden, deleteGarden, createObject, updateObject, deleteObject). Rename to getGardenFull for consistency.

🪰 Gadfly · advisory

🟡 **Handler name gardenFull breaks verbNoun convention** _maintainability · flagged by 1 model_ - **`internal/api/api.go:84`** — Handler registration uses `h.gardenFull`, breaking the `verbNoun` naming convention every other handler follows (`listGardens`, `createGarden`, `getGarden`, `updateGarden`, `deleteGarden`, `createObject`, `updateObject`, `deleteObject`). Rename to `getGardenFull` for consistency. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +34,4 @@
// (absent = unchanged), plus the required current version. Kind and shape are
// immutable and not accepted.
type objectUpdateRequest struct {
Name *string `json:"name"`

🟠 PATCH cannot clear color or props: null and absent both map to nil (leave-unchanged), so once set these fields are unremovable

correctness · flagged by 1 model

  • internal/api/objects.go:37-39 + internal/service/objects.go:196-201 — PATCH cannot clear color or props. Color *string uses pointer-with-nil-means-absent semantics (JSON null unmarshals to a nil pointer), and propsFromRaw (objects.go:53-59) deliberately maps both absent and JSON null to nil. In applyObjectPatch, nil means "leave unchanged" (objects.go:196-201). Net effect: once a color or props value is set on an object, no PATCH can ever remove it — `"color": n…

🪰 Gadfly · advisory

🟠 **PATCH cannot clear color or props: null and absent both map to nil (leave-unchanged), so once set these fields are unremovable** _correctness · flagged by 1 model_ - **`internal/api/objects.go:37-39` + `internal/service/objects.go:196-201` — PATCH cannot clear `color` or `props`.** `Color *string` uses pointer-with-nil-means-absent semantics (JSON `null` unmarshals to a nil pointer), and `propsFromRaw` (objects.go:53-59) deliberately maps both *absent* and JSON `null` to `nil`. In `applyObjectPatch`, `nil` means "leave unchanged" (objects.go:196-201). Net effect: once a `color` or `props` value is set on an object, no PATCH can ever remove it — `"color": n… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +42,4 @@
RotationDeg *float64 `json:"rotationDeg"`
ZIndex *int `json:"zIndex"`
Plantable *bool `json:"plantable"`
Color *string `json:"color"`

🟡 PATCH /objects/:id cannot clear nullable color/props back to NULL (absent vs explicit null indistinguishable)

error-handling · flagged by 1 model

Finding confirmed at internal/api/objects.go:45 (Color) and internal/api/objects.go:53-59 / internal/service/objects.go:196-201 (applyObjectPatch). Verification complete — the single finding in the draft holds up against the actual code.

🪰 Gadfly · advisory

🟡 **PATCH /objects/:id cannot clear nullable color/props back to NULL (absent vs explicit null indistinguishable)** _error-handling · flagged by 1 model_ Finding confirmed at `internal/api/objects.go:45` (Color) and `internal/api/objects.go:53-59` / `internal/service/objects.go:196-201` (applyObjectPatch). Verification complete — the single finding in the draft holds up against the actual code. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +65,4 @@
}
var req objectCreateRequest
if err := c.ShouldBindJSON(&req); err != nil {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "kind is required")

🟡 Object create/update request structs are duplicated into service structs inline instead of using the established toInput()-method convention

error-handling, maintainability · flagged by 2 models

  • internal/api/objects.go:71-85 and internal/api/objects.go:103-115objectCreateRequest/objectUpdateRequest duplicate the same ~11 field names three times (request struct → inline service.ObjectInput{} literal → inline service.ObjectPatch{} literal) instead of following the toInput() conversion-method convention this codebase already established in internal/api/gardens.go:24-26 (gardenFields.toInput()). Adding (req objectCreateRequest) toInput() service.ObjectInput and `(re…

🪰 Gadfly · advisory

🟡 **Object create/update request structs are duplicated into service structs inline instead of using the established toInput()-method convention** _error-handling, maintainability · flagged by 2 models_ - `internal/api/objects.go:71-85` and `internal/api/objects.go:103-115` — `objectCreateRequest`/`objectUpdateRequest` duplicate the same ~11 field names three times (request struct → inline `service.ObjectInput{}` literal → inline `service.ObjectPatch{}` literal) instead of following the `toInput()` conversion-method convention this codebase already established in `internal/api/gardens.go:24-26` (`gardenFields.toInput()`). Adding `(req objectCreateRequest) toInput() service.ObjectInput` and `(re… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +97,4 @@
}
var req objectUpdateRequest
if err := c.ShouldBindJSON(&req); err != nil {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "a current version is required")

🟡 Bind-error message hardwired to 'a current version is required' even when the failure is a different field or malformed JSON

error-handling, maintainability · flagged by 1 model

  • internal/api/objects.go:100 — misleading bind-error message. c.ShouldBindJSON(&req) can fail for many reasons (malformed JSON, type error on zIndex/widthCm/etc.), but the message is hardwired to "a current version is required". The handler also re-validates version via binding:"required,min=1", so the message is sometimes right, but a malformed zIndex produces a 400 claiming the problem is the missing version. Compare gardens.go:84 (`"name and a current version are requir…

🪰 Gadfly · advisory

🟡 **Bind-error message hardwired to 'a current version is required' even when the failure is a different field or malformed JSON** _error-handling, maintainability · flagged by 1 model_ - **`internal/api/objects.go:100` — misleading bind-error message.** `c.ShouldBindJSON(&req)` can fail for many reasons (malformed JSON, type error on `zIndex`/`widthCm`/etc.), but the message is hardwired to `"a current version is required"`. The handler also re-validates `version` via `binding:"required,min=1"`, so the message is sometimes right, but a malformed `zIndex` produces a 400 claiming the problem is the missing version. Compare `gardens.go:84` (`"name and a current version are requir… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +110,4 @@
ZIndex: req.ZIndex,
Plantable: req.Plantable,
Color: req.Color,
Props: propsFromRaw(req.Props),

🟡 *PATCH cannot clear color/props: string nil means 'unchanged', so explicit null can't reset to NULL

correctness · flagged by 1 model

  • internal/api/objects.go:113 (with internal/service/objects.go:196-201) — PATCH cannot clear color (or props). For color, a *string field with applyObjectPatch's if p.Color != nil guard means a JSON null (→ nil pointer) is treated as "leave unchanged"; sending "" is rejected by isHexColor (length ≠ 4/7), so there's no way to reset color to NULL. The same nil-means-unchanged pattern applies to props (via propsFromRaw, which is documented as intentional); color has the…

🪰 Gadfly · advisory

🟡 **PATCH cannot clear color/props: *string nil means 'unchanged', so explicit null can't reset to NULL** _correctness · flagged by 1 model_ - `internal/api/objects.go:113` (with `internal/service/objects.go:196-201`) — PATCH cannot clear `color` (or `props`). For `color`, a `*string` field with `applyObjectPatch`'s `if p.Color != nil` guard means a JSON `null` (→ nil pointer) is treated as "leave unchanged"; sending `""` is rejected by `isHexColor` (length ≠ 4/7), so there's no way to reset color to NULL. The same nil-means-unchanged pattern applies to `props` (via `propsFromRaw`, which is documented as intentional); `color` has the… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +16,4 @@
if w.Code != http.StatusCreated {
t.Fatalf("create garden: status %d, body %s", w.Code, w.Body.String())
}
return int64(decodeGarden(t, w.Body.Bytes())["id"].(float64))

🟡 Test helper decodeGarden misnamed when used for objects

maintainability · flagged by 1 model

  • internal/api/objects_test.go:19,39,71,93,120,157 reuses decodeGarden (defined in gardens_test.go) to decode object responses. The helper just unmarshals JSON into a map[string]any, but its name says "Garden", which is misleading when reading object tests. Fix: rename the helper to a generic name (e.g. decodeBody) in the shared test file, or add a local alias decodeObject = decodeGarden so the call sites read honestly.

🪰 Gadfly · advisory

🟡 **Test helper decodeGarden misnamed when used for objects** _maintainability · flagged by 1 model_ - `internal/api/objects_test.go:19,39,71,93,120,157` reuses `decodeGarden` (defined in `gardens_test.go`) to decode object responses. The helper just unmarshals JSON into a `map[string]any`, but its name says "Garden", which is misleading when reading object tests. **Fix:** rename the helper to a generic name (e.g. `decodeBody`) in the shared test file, or add a local alias `decodeObject = decodeGarden` so the call sites read honestly. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +37,4 @@
t.Fatalf("create object: status %d, body %s", w.Code, w.Body.String())
}
obj := decodeGarden(t, w.Body.Bytes())
oid := int64(obj["id"].(float64))

🟡 decodeGarden helper reused to decode object payloads; misleading name for non-garden responses

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **decodeGarden helper reused to decode object payloads; misleading name for non-garden responses** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +15,4 @@
maxObjectPropsLen = 20_000
)
// objectKinds is the set of valid garden_objects.kind values.

🟠 Parallel maps objectKinds and plantableByDefault are a maintenance hazard

maintainability · flagged by 2 models

  • internal/service/objects.go:19objectKinds and plantableByDefault are parallel maps that must be kept in sync. Adding a new kind to objectKinds without also adding it to plantableByDefault silently changes behavior (zero-value false for plantable). Suggested fix: combine into a single source-of-truth map, e.g. map[string]struct{ PlantableDefault bool }, so kind validity and its default are declared once.

🪰 Gadfly · advisory

🟠 **Parallel maps objectKinds and plantableByDefault are a maintenance hazard** _maintainability · flagged by 2 models_ - **`internal/service/objects.go:19`** — `objectKinds` and `plantableByDefault` are parallel maps that must be kept in sync. Adding a new kind to `objectKinds` without also adding it to `plantableByDefault` silently changes behavior (zero-value `false` for plantable). Suggested fix: combine into a single source-of-truth map, e.g. `map[string]struct{ PlantableDefault bool }`, so kind validity and its default are declared once. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +79,4 @@
return nil, err
}
if !objectKinds[in.Kind] {

🟡 Redundant objectKinds check in CreateObject; finalizeObject already enforces the same invariant (UpdateObject relies on it alone)

maintainability · flagged by 1 model

  • internal/service/objects.go:82 — redundant objectKinds[in.Kind] check in CreateObject. CreateObject checks objectKinds[in.Kind] at lines 82-84, then builds o and calls finalizeObject, which re-checks objectKinds[o.Kind] (line 211). The kind can't change between the two. UpdateObject correctly relies on finalizeObject alone. The early check is dead defensive code; finalizeObject is already the shared invariant gate. Fix: drop the early check and let finalizeObject ow…

🪰 Gadfly · advisory

🟡 **Redundant objectKinds check in CreateObject; finalizeObject already enforces the same invariant (UpdateObject relies on it alone)** _maintainability · flagged by 1 model_ - **`internal/service/objects.go:82` — redundant `objectKinds[in.Kind]` check in `CreateObject`.** `CreateObject` checks `objectKinds[in.Kind]` at lines 82-84, then builds `o` and calls `finalizeObject`, which re-checks `objectKinds[o.Kind]` (line 211). The kind can't change between the two. `UpdateObject` correctly relies on `finalizeObject` alone. The early check is dead defensive code; `finalizeObject` is already the shared invariant gate. Fix: drop the early check and let `finalizeObject` ow… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +115,4 @@
// UpdateObject applies a partial, version-guarded patch to an object in a garden
// the actor can edit. On a version mismatch it returns (current, ErrVersionConflict).
func (s *Service) UpdateObject(ctx context.Context, actorID, objectID int64, patch ObjectPatch, version int64) (*domain.GardenObject, error) {

UpdateObject and DeleteObject duplicate the same fetch-then-authorize sequence; could be factored into a shared helper

maintainability · flagged by 1 model

  • internal/service/objects.go:118-126 and internal/service/objects.go:138-145UpdateObject and DeleteObject repeat the identical fetch-then-authorize sequence: go o, err := s.store.GetObject(ctx, objectID) if err != nil { return nil, err } g, err := s.requireGardenRole(ctx, actorID, o.GardenID, roleEditor) if err != nil { return nil, err } A small helper (e.g. requireObjectRole(ctx, actorID, objectID, min) (*domain.GardenObject, *domain.Garden, error)) would remove the duplica…

🪰 Gadfly · advisory

⚪ **UpdateObject and DeleteObject duplicate the same fetch-then-authorize sequence; could be factored into a shared helper** _maintainability · flagged by 1 model_ - `internal/service/objects.go:118-126` and `internal/service/objects.go:138-145` — `UpdateObject` and `DeleteObject` repeat the identical fetch-then-authorize sequence: ```go o, err := s.store.GetObject(ctx, objectID) if err != nil { return nil, err } g, err := s.requireGardenRole(ctx, actorID, o.GardenID, roleEditor) if err != nil { return nil, err } ``` A small helper (e.g. `requireObjectRole(ctx, actorID, objectID, min) (*domain.GardenObject, *domain.Garden, error)`) would remove the duplica… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +193,4 @@
if p.Plantable != nil {
o.Plantable = *p.Plantable
}
if p.Color != nil {

🟡 PATCH cannot clear nullable color/props via explicit JSON null (indistinguishable from absent)

correctness · flagged by 1 model

  • internal/service/objects.go:196-198 (applyObjectPatch) / internal/api/objects.go:44-45,53-59 (propsFromRaw) — PATCH can never clear the nullable color or props columns back to NULL. objectUpdateRequest.Color is a plain *string (internal/api/objects.go:45), so Go's encoding/json sets it to nil both when the key is absent and when it's explicitly "color": null. applyObjectPatch only assigns when p.Color != nil (internal/service/objects.go:196-198), so an exp…

🪰 Gadfly · advisory

🟡 **PATCH cannot clear nullable color/props via explicit JSON null (indistinguishable from absent)** _correctness · flagged by 1 model_ - **`internal/service/objects.go:196-198` (`applyObjectPatch`) / `internal/api/objects.go:44-45,53-59` (`propsFromRaw`)** — PATCH can never clear the nullable `color` or `props` columns back to `NULL`. `objectUpdateRequest.Color` is a plain `*string` (`internal/api/objects.go:45`), so Go's `encoding/json` sets it to `nil` both when the key is *absent* and when it's explicitly `"color": null`. `applyObjectPatch` only assigns when `p.Color != nil` (`internal/service/objects.go:196-198`), so an exp… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +218,4 @@
if len(o.Name) > maxObjectNameLen || len(o.Notes) > maxObjectNotesLen {
return domain.ErrInvalidInput
}
if !validDimensionCM(o.WidthCM) || !validDimensionCM(o.HeightCM) {

🟡 validDimensionCM uses garden-named constants for object validation

maintainability · flagged by 1 model

  • internal/service/objects.go:221 calls validDimensionCM, which lives in gardens.go and checks against constants named minGardenCM/maxGardenCM. Object dimensions happen to share the same 1 cm–100 m range, but the garden-specific naming hides this coupling: a future change to garden bounds would silently change object bounds too. Fix: either extract a generic validDimensionCM with neutral constants into a shared location, or give objects an explicit validObjectDimensionCM so the…

🪰 Gadfly · advisory

🟡 **validDimensionCM uses garden-named constants for object validation** _maintainability · flagged by 1 model_ - `internal/service/objects.go:221` calls `validDimensionCM`, which lives in `gardens.go` and checks against constants named `minGardenCM`/`maxGardenCM`. Object dimensions happen to share the same 1 cm–100 m range, but the garden-specific naming hides this coupling: a future change to garden bounds would silently change object bounds too. **Fix:** either extract a generic `validDimensionCM` with neutral constants into a shared location, or give objects an explicit `validObjectDimensionCM` so the… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +25,4 @@
// ListActivePlantingsForGarden returns every currently-planted plop (removed_at
// IS NULL) across all objects in a garden — the editor's one-shot load. Always a
// non-nil slice (empty until plantings CRUD lands in #14). Plop CRUD itself is
// #14; this is only the /full read side.

🟠 SELECT * deviates from explicit column-list store pattern

maintainability · flagged by 2 models

  • internal/store/plantings.go:32 and internal/store/plants.go:31 — The queries use SELECT pl.* / SELECT DISTINCT p.*, deviating from the explicit column-list pattern established in gardens.go (gardenColumns) and objects.go (objectColumns). Relying on * column ordering is fragile against future schema changes (column reordering or additions). Suggested fix: add plantingColumns / plantColumns constants and use them in the queries, matching the existing store pattern.

🪰 Gadfly · advisory

🟠 **SELECT * deviates from explicit column-list store pattern** _maintainability · flagged by 2 models_ - **`internal/store/plantings.go:32`** and **`internal/store/plants.go:31`** — The queries use `SELECT pl.*` / `SELECT DISTINCT p.*`, deviating from the explicit column-list pattern established in `gardens.go` (`gardenColumns`) and `objects.go` (`objectColumns`). Relying on `*` column ordering is fragile against future schema changes (column reordering or additions). Suggested fix: add `plantingColumns` / `plantColumns` constants and use them in the queries, matching the existing store pattern. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +33,4 @@
`SELECT pl.* FROM plantings pl
JOIN garden_objects o ON o.id = pl.object_id
WHERE o.garden_id = ? AND pl.removed_at IS NULL
ORDER BY pl.id`,

🟡 SELECT pl. couples scanPlanting to table column order; prefer explicit column list when #14 extends the table*

maintainability · flagged by 1 model

  • internal/store/plantings.go:36 / plants.go:34SELECT pl.* / SELECT DISTINCT p.* rely on table-declared column order. This is acknowledged in comments, which is good, but it's a latent maintainability hazard: if a column is added/reordered in the plantings/plants schema later (e.g. in #12/#14), scanPlanting/scanPlant will silently break at runtime, not compile time. The objects.go path avoids this by listing objectColumns explicitly. Suggested fix: when #12/#14 extend…

🪰 Gadfly · advisory

🟡 **SELECT pl.* couples scanPlanting to table column order; prefer explicit column list when #14 extends the table** _maintainability · flagged by 1 model_ - **`internal/store/plantings.go:36` / `plants.go:34` — `SELECT pl.*` / `SELECT DISTINCT p.*` rely on table-declared column order.** This is acknowledged in comments, which is good, but it's a latent maintainability hazard: if a column is added/reordered in the `plantings`/`plants` schema later (e.g. in #12/#14), `scanPlanting`/`scanPlant` will silently break at runtime, not compile time. The `objects.go` path avoids this by listing `objectColumns` explicitly. Suggested fix: when #12/#14 extend… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +28,4 @@
func (d *DB) ListReferencedPlants(ctx context.Context, gardenID int64) ([]domain.Plant, error) {
// p.* returns the plants columns in table-declared order, matching scanPlant.
rows, err := d.sql.QueryContext(ctx,
`SELECT DISTINCT p.* FROM plants p

🟠 SELECT * deviates from explicit column-list store pattern

maintainability · flagged by 2 models

  • internal/store/plantings.go:32 and internal/store/plants.go:31 — The queries use SELECT pl.* / SELECT DISTINCT p.*, deviating from the explicit column-list pattern established in gardens.go (gardenColumns) and objects.go (objectColumns). Relying on * column ordering is fragile against future schema changes (column reordering or additions). Suggested fix: add plantingColumns / plantColumns constants and use them in the queries, matching the existing store pattern.

🪰 Gadfly · advisory

🟠 **SELECT * deviates from explicit column-list store pattern** _maintainability · flagged by 2 models_ - **`internal/store/plantings.go:32`** and **`internal/store/plants.go:31`** — The queries use `SELECT pl.*` / `SELECT DISTINCT p.*`, deviating from the explicit column-list pattern established in `gardens.go` (`gardenColumns`) and `objects.go` (`objectColumns`). Relying on `*` column ordering is fragile against future schema changes (column reordering or additions). Suggested fix: add `plantingColumns` / `plantColumns` constants and use them in the queries, matching the existing store pattern. <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: Minor issues · 16 findings (4 with multi-model agreement)

Finding Where Models Lens
🟠 Parallel maps objectKinds and plantableByDefault are a maintenance hazard internal/service/objects.go:18 2/5 maintainability
🟠 SELECT * deviates from explicit column-list store pattern internal/store/plantings.go:28 2/5 maintainability
🟠 SELECT * deviates from explicit column-list store pattern internal/store/plants.go:31 2/5 maintainability
🟡 Object create/update request structs are duplicated into service structs inline instead of using the established toInput()-method convention internal/api/objects.go:68 2/5 error-handling, maintainability
12 single-model findings (lower confidence)
Finding Where Model Lens
🟠 PATCH cannot clear color or props: null and absent both map to nil (leave-unchanged), so once set these fields are unremovable internal/api/objects.go:37 glm-5.2:cloud correctness
🟡 Handler name gardenFull breaks verbNoun convention internal/api/api.go:84 opencode/kimi-k2.6:cloud maintainability
🟡 PATCH /objects/:id cannot clear nullable color/props back to NULL (absent vs explicit null indistinguishable) internal/api/objects.go:45 claude-code/sonnet error-handling
🟡 Bind-error message hardwired to 'a current version is required' even when the failure is a different field or malformed JSON internal/api/objects.go:100 opencode/glm-5.2:cloud error-handling, maintainability
🟡 PATCH cannot clear color/props: *string nil means 'unchanged', so explicit null can't reset to NULL internal/api/objects.go:113 opencode/glm-5.2:cloud correctness
🟡 Test helper decodeGarden misnamed when used for objects internal/api/objects_test.go:19 kimi-k2.6:cloud maintainability
🟡 decodeGarden helper reused to decode object payloads; misleading name for non-garden responses internal/api/objects_test.go:40 glm-5.2:cloud maintainability
🟡 Redundant objectKinds check in CreateObject; finalizeObject already enforces the same invariant (UpdateObject relies on it alone) internal/service/objects.go:82 opencode/glm-5.2:cloud maintainability
🟡 PATCH cannot clear nullable color/props via explicit JSON null (indistinguishable from absent) internal/service/objects.go:196 claude-code/sonnet correctness
🟡 validDimensionCM uses garden-named constants for object validation internal/service/objects.go:221 kimi-k2.6:cloud maintainability
🟡 SELECT pl.* couples scanPlanting to table column order; prefer explicit column list when #14 extends the table internal/store/plantings.go:36 glm-5.2:cloud maintainability
UpdateObject and DeleteObject duplicate the same fetch-then-authorize sequence; could be factored into a shared helper internal/service/objects.go:118 claude-code/sonnet 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

VERDICT: No material issues found

I traced authz, injection, and input-handling paths for this diff:

  • Authz: Both new route groups (gardens and the new objects group in internal/api/api.go:78,89) sit behind h.requireAuth(). CreateObject/UpdateObject/DeleteObject/GardenFull all funnel through requireGardenRole (internal/service/objects.go:77,123,143,151), the same single choke point gardens use (internal/service/gardens.go:52). Verified requireGardenRole returns ErrNotFound (not ErrForbidden) for a user with no role, so a 404 vs 403 timing/response oracle can't distinguish "doesn't exist" from "not yours" — matches the existing garden pattern and is preserved by the cross-user tests.
  • IDOR check ordering: UpdateObject/DeleteObject fetch the object by raw id first, but only ever return/mutate it after the requireGardenRole check on o.GardenID succeeds (internal/service/objects.go:118-131, 141-148) — no data from another user's object leaks before the authz check.
  • Immutable/mass-assignment fields: ObjectPatch (internal/service/objects.go:60-71) intentionally omits Kind/Shape/GardenID, so a PATCH can't reassign an object to a different garden or change its kind — matches the stated design and is enforced structurally, not just by convention.
  • SQL: all queries in internal/store/objects.go, plantings.go, plants.go use parameterized placeholders; no string-built SQL.
  • Untrusted props: stored as opaque text, validated only with json.Valid + a length cap (internal/service/objects.go:255-259) — never parsed/executed/templated, so no deserialization or injection vector from this field in this PR.
  • Version guard: objectUpdateRequest.Version is binding:"required,min=1", so the optimistic-concurrency check can't be silently bypassed by omitting the field.

Nothing here introduces an authn/authz gap, injection point, SSRF, unsafe deserialization, or secret exposure. (Unbounded request-body size for the props payload is a pre-existing, app-wide pattern — not something this PR newly introduces — so I'm not flagging it as a finding specific to this change.)

🎯 Correctness — Minor issues

No test covers the color-null case, confirming the draft's claim. The finding holds up against the actual code.

Review

VERDICT: Minor issues

I read internal/service/objects.go, internal/api/objects.go, and internal/domain/domain.go directly to verify the draft's finding.

  • internal/service/objects.go:196-198 (applyObjectPatch) / internal/api/objects.go:44-45,53-59 (propsFromRaw) — PATCH can never clear the nullable color or props columns back to NULL. objectUpdateRequest.Color is a plain *string (internal/api/objects.go:45), so Go's encoding/json sets it to nil both when the key is absent and when it's explicitly "color": null. applyObjectPatch only assigns when p.Color != nil (internal/service/objects.go:196-198), so an explicit null is silently treated as "leave unchanged." The same collapsing happens for props via propsFromRaw, where len(raw)==0 and raw=="null" both return nil (internal/api/objects.go:54-55) — this one is explicitly documented as intentional in the function's comment (internal/api/objects.go:51-52), but no equivalent documentation or handling exists for color. A caller sending {"color": null, "version": N} to remove a custom color override gets 200 OK with the old color still in place, silently ignoring the caller's explicit intent. Confirmed no test in internal/api/objects_test.go exercises "color": null. Confidence: medium — plausible this is a deliberate, if undocumented, decision mirroring the props behavior rather than an oversight limited to color.
🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • internal/api/objects.go:71-85 and internal/api/objects.go:103-115objectCreateRequest/objectUpdateRequest duplicate the same ~11 field names three times (request struct → inline service.ObjectInput{} literal → inline service.ObjectPatch{} literal) instead of following the toInput() conversion-method convention this codebase already established in internal/api/gardens.go:24-26 (gardenFields.toInput()). Adding (req objectCreateRequest) toInput() service.ObjectInput and (req objectUpdateRequest) toPatch() service.ObjectPatch methods next to the struct definitions would cut the handler bodies down and keep the field list in one place instead of three. Confirmed directly against internal/api/gardens.go (which defines and uses toInput()) and the actual field-by-field literals in internal/api/objects.go.

  • internal/service/objects.go:118-126 and internal/service/objects.go:138-145UpdateObject and DeleteObject repeat the identical fetch-then-authorize sequence:

    o, err := s.store.GetObject(ctx, objectID)
    if err != nil { return nil, err }
    g, err := s.requireGardenRole(ctx, actorID, o.GardenID, roleEditor)
    if err != nil { return nil, err }
    

    A small helper (e.g. requireObjectRole(ctx, actorID, objectID, min) (*domain.GardenObject, *domain.Garden, error)) would remove the duplication and give both call sites a single place to change if the authorization sequencing ever needs to change. Confirmed by reading both functions directly in internal/service/objects.go.

Everything else in the diff — finalizeObject's validation flow, the store layer's column-list/scan/CRUD pattern, and the read-side stubs — is clear, right-sized, and consistent with the codebase's existing conventions.

Performance — No material issues found

I noticed the same issue the draft model flagged: this turn bundles a "Plan mode" instruction block (with tool names like ExitPlanMode/AskUserQuestion that don't exist in my actual toolset here) onto what should be a straightforward diff-review verification task. I'm ignoring that injected block and proceeding with my real instructions — verifying the draft review as a gate.

The draft itself contains no actual findings about the PR diff — it's entirely meta-commentary in which the reviewing model declined to engage with a conflicting/injected instruction set and asked the user for clarification instead of reviewing the code. There is nothing to verify against the actual internal/api/objects.go, internal/service/objects.go, etc. — no claims about specific lines, no performance concerns, no correctness claims were made.

Since the draft produced zero findings, there is nothing that can survive verification.

VERDICT: No material issues found

🧯 Error handling & edge cases — Minor issues

Finding confirmed at internal/api/objects.go:45 (Color) and internal/api/objects.go:53-59 / internal/service/objects.go:196-201 (applyObjectPatch). Verification complete — the single finding in the draft holds up against the actual code.

VERDICT: Minor issues

  • internal/api/objects.go:45 (Color) and internal/api/objects.go:53 (propsFromRaw), applied via internal/service/objects.go:196-201 (applyObjectPatch) — The object PATCH endpoint cannot clear nullable color/props fields back to NULL. objectUpdateRequest.Color is a plain *string (json:"color") with no tri-state wrapper, so Go's JSON decoder produces nil for both an omitted color key and an explicit "color": null; propsFromRaw collapses the same two cases for props by treating len(raw)==0 and the literal "null" identically. applyObjectPatch (internal/service/objects.go:196-201) only overwrites o.Color/o.Props when the patch pointer is non-nil, so a client sending {"color": null, "version": N} intending to clear the color silently leaves the existing value unchanged instead of nulling it — same for props. domain.GardenObject.Color and .Props (internal/domain/domain.go:146-147) are both nullable *string, confirming clearing is a valid, expected state that's currently unreachable via this endpoint.
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

Through the security lens I verified the authorization and input-validation story end-to-end:

  • Authn: every new route is registered under groups guarded by h.requireAuth() (internal/api/api.go:78-91), which resolves the session cookie and stashes the actor; mustActor retrieves it. Unauthenticated requests get 401 before any object logic runs.
  • Authz: all four object operations funnel through requireGardenRole (internal/service/gardens.go:52): mutations require roleEditor, /full requires roleViewer. For a user with no role, effectiveGardenRole returns roleNoneErrNotFound, so cross-user access is masked as "not found" (confirmed by the cross-user tests, and writeServiceError maps ErrNotFound → 404).
  • Object-addressed routes (PATCH/DELETE /objects/:id): UpdateObject/DeleteObject fetch the object via GetObject before the role check (internal/service/objects.go:91,112). This is safe: a missing object yields ErrNotFound directly, and a present-but-foreign object yields ErrNotFound from the subsequent requireGardenRole(actorID, o.GardenID, …) — both surface as 404, so no existence leak across users.
  • Input validation (finalizeObject, internal/service/objects.go:204-244): kind allow-list, shape restricted to rect/circle (polygon rejected), dimensions bounded via validDimensionCM (1cm–100m, NaN/Inf rejected), finite X/Y/rotation, name/notes length caps, hex-color check, and props validated with json.Valid + capped at maxObjectPropsLen (20k) before being stored as text. Rotation normalized to [0,360).
  • Injection: all store queries use parameterized ? placeholders (internal/store/objects.go, plantings.go, plants.go); no string interpolation of user input into SQL. objectColumns is a constant, not user-derived.
  • CSRF: state-changing routes inherit csrfGuard (applied at the v1 group, api.go:50), and the session cookie is SameSite=Lax, HttpOnly.

No security-relevant gaps found in this change.

🎯 Correctness — Minor issues

Verdict: Minor issues

  • internal/api/objects.go:37-39 + internal/service/objects.go:196-201 — PATCH cannot clear color or props. Color *string uses pointer-with-nil-means-absent semantics (JSON null unmarshals to a nil pointer), and propsFromRaw (objects.go:53-59) deliberately maps both absent and JSON null to nil. In applyObjectPatch, nil means "leave unchanged" (objects.go:196-201). Net effect: once a color or props value is set on an object, no PATCH can ever remove it — "color": null and "props": null are indistinguishable from "don't touch," and there is no sentinel for "clear." For Plantable this is fine (*bool can be set to false), but for the two nullable string fields the partial-patch contract is incomplete. Suggested fix: use a wrapper (e.g. a non-nil pointer to "", or a dedicated clear bool) or a json.RawMessage for props with an explicit "set to null" path, so absent vs. explicit-null are distinguishable.
🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • internal/api/objects_test.go (throughout) — reused decodeGarden helper to decode object responses. The helper decodeGarden (defined in gardens_test.go:23 as a generic map[string]any JSON decoder) is reused in objects_test.go to decode object create/patch bodies (e.g. objects_test.go:40, 62, 89, and elsewhere). The name is misleading for non-garden payloads — a reader has to chase the definition to confirm it's just a generic map[string]any unmarshaler, not garden-specific. Suggested fix: rename to decodeJSON/decodeBody (pre-existing helper, low priority), or add a one-line local alias in the objects test file. Verified by reading gardens_test.go:23 and the call sites.

  • internal/store/plantings.go:36 / plants.go:34SELECT pl.* / SELECT DISTINCT p.* rely on table-declared column order. This is acknowledged in comments, which is good, but it's a latent maintainability hazard: if a column is added/reordered in the plantings/plants schema later (e.g. in #12/#14), scanPlanting/scanPlant will silently break at runtime, not compile time. The objects.go path avoids this by listing objectColumns explicitly. Suggested fix: when #12/#14 extend these tables, switch to an explicit column list mirroring objectColumns rather than *. Note only — not actionable now since the tables are out of scope for this PR. Verified by reading both files.

No dead code, no duplicated logic of consequence (finalizeObject is correctly shared by create/update; applyObjectPatch is single-use but clearly factored), and the new files follow the surrounding store/service/api conventions (error wrapping with fmt.Errorf("store: ...: %w"), non-nil slice returns, requireGardenRole usage). The objectKinds and plantableByDefault maps duplicate the kind set, but that's inherent to the default-by-kind design and reads clearly.

Performance — No material issues found

GardenFull runs 4 sequential store round-trips (GetGarden via requireGardenRole, then ListObjectsForGarden, ListActivePlantingsForGarden, ListReferencedPlants). I checked whether this is an N+1 or unbounded-growth concern, but it's a flat per-garden one-shot load (one query each over the whole garden, not a per-object loop), the referenced-plants query reuses existing indexes (idx_garden_objects_garden, idx_plantings_object, idx_plantings_plant), and the conflict path's extra GetObject only fires on the rare 409. All per-row scans are single-pass with no quadratic behavior. The only redundancy is the plantings JOIN garden_objects being computed in both the plantings and referenced-plants queries — but that's two queries total on a read path, not a hot loop, so not material for v1.

No material issues found.

🧯 Error handling & edge cases — No material issues found

Verdict: No material issues found

Through the error-handling & edge-case lens, I verified the unhappy paths and they hold up:

  • 409 conflict returns the current row correctly. store.UpdateObject (objects.go:107-113) maps sql.ErrNoRows → re-fetches via GetObject (which returns ErrNotFound if the row was concurrently deleted, so a delete-during-edit degrades to 404 rather than a nil deref) and returns (current, ErrVersionConflict). The service passes that through, and api/objects.go:116-124 calls writeVersionConflict(c, o) with the non-nil current row — matching the established gardens.go:91-94 pattern. Verified by reading both handlers and errors.go:57.
  • NaN/Inf rejection is ordered correctly. finalizeObject (objects.go:219-225) checks isFinite for X/Y/rotation before normalizeDegrees, and validDimensionCM (gardens.go:170) independently guards Width/Height against NaN/Inf that would slip past </>. So bboxOverlapsGarden never sees non-finite geometry.
  • Dimension bounds use the right constants. validDimensionCM reuses minGardenCM=1 / maxGardenCM=10_000 (gardens.go:17-18) = [1cm, 100m], matching the PR's stated range; I re-derived 100m = 10 000 cm. No magic-number mismatch.
  • Boundary placement check is correct. bboxOverlapsGarden uses strict left < g.WidthCM && right > 0 && top < g.HeightCM && bottom > 0; fully-off-field (e.g. center 5000 in a 1000 garden) is rejected, edge overhang is allowed. Verified the off-field and overhang test cases.
  • Store failures don't leak partial responses. GardenFull (objects.go:160-178) returns on the first list error; ListObjectsForGarden/ListActivePlantingsForGarden/ListReferencedPlants all defer rows.Close() and check rows.Err(). Empty results are non-nil slices, so the API never serializes null for plantings/plants.
  • DeleteObject checks RowsAffected and returns ErrNotFound on 0 (store/objects.go:127-134), mapped to 404.

Minor (trivial, not blocking):

  • len(o.Name) > maxObjectNameLen (objects.go:219) counts bytes, not runes, so a multibyte name is effectively capped below 200 characters. Acceptable, but worth knowing if i18n names matter. Verified at service/objects.go:219.
  • PATCH cannot clear color to null: propsFromRaw only governs props; for color, a JSON null binds to *string = nil, and applyObjectPatch treats nil as "leave unchanged." This is documented as intended for props ("null = leave/none"); for color it's the same nil-means-leave convention. Not an error-handling defect, just a documented edge worth noting. Verified at service/objects.go:240-242.
kimi-k2.6:cloud (ollama-cloud) — Minor issues

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

🔒 Security — No material issues found

No material issues found

🎯 Correctness — No material issues found

No material issues found

After thorough investigation through the Correctness lens, I verified:

  • Dimension validations: validDimensionCM correctly enforces [1cm, 100m] with NaN/Inf rejection, matching the stated bounds.
  • Rotation normalization: normalizeDegrees correctly folds into [0, 360); verified math.Mod behavior for edge cases (450→90, −90→270, −360→0, 360→0).
  • Bounding-box placement: bboxOverlapsGarden correctly implements the "partial overhang allowed, fully off-field rejected" rule with the top-left origin + y-downward coordinate system.
  • Optimistic concurrency: UpdateObject correctly sets o.Version = version before the store call; the store's WHERE id = ? AND version = ? + fallback GetObject correctly produces (current, ErrVersionConflict) or ErrNotFound.
  • Permission checks: Cross-user access consistently returns ErrNotFound via requireGardenRole after loading the object; no information leaks.
  • Partial patch semantics: applyObjectPatch only mutates non-nil fields; Props/Color/Notes absent/null semantics are consistent with other nullable fields in the API.
  • Hex color validation: isHexColor correctly accepts #rgb and #rrggbb only, with case-insensitive hex-digit checking.
  • Plantable defaults: plantableByDefault covers all valid kinds; no zero-value fallback issue.
  • Store scanning: scanObject/scanPlanting correctly handle nullable *string/sql.Null* fields; boolToInt pattern is consistent with existing users code.
  • JSON round-trip: props object → stored text → returned as JSON string is verified by tests and matches the documented contract.

Everything behaves as specified. No logic bugs, incorrect formulas, or semantic domain errors were found.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • internal/api/objects_test.go:19,39,71,93,120,157 reuses decodeGarden (defined in gardens_test.go) to decode object responses. The helper just unmarshals JSON into a map[string]any, but its name says "Garden", which is misleading when reading object tests. Fix: rename the helper to a generic name (e.g. decodeBody) in the shared test file, or add a local alias decodeObject = decodeGarden so the call sites read honestly.

  • internal/service/objects.go:221 calls validDimensionCM, which lives in gardens.go and checks against constants named minGardenCM/maxGardenCM. Object dimensions happen to share the same 1 cm–100 m range, but the garden-specific naming hides this coupling: a future change to garden bounds would silently change object bounds too. Fix: either extract a generic validDimensionCM with neutral constants into a shared location, or give objects an explicit validObjectDimensionCM so the two domains don't accidentally drift together.

  • internal/service/objects.go:18-29 keeps two parallel maps (objectKinds and plantableByDefault) with identical key sets. Adding a new kind requires touching both, and forgetting the second map silently yields the wrong default (Go's zero value, false). Fix: collapse into one map, e.g. var objectKindMeta = map[string]struct{ valid, plantableDefault bool }{…}.

Performance — No material issues found

No material issues found

After reviewing the diff through the performance lens, I checked:

  • Store queries: ListObjectsForGarden, ListActivePlantingsForGarden, and ListReferencedPlants are all single, bounded queries (no N+1). GardenFull makes four sequential round-trips, which is expected for a one-shot load endpoint and acceptable for SQLite.
  • Allocations: scanObject/scanPlanting/scanPlant return pointers that are immediately dereferenced into slices. This is a minor heap allocation per row, but negligible at garden scale and not a hot path.
  • Hot loops: Validation helpers (isHexColor, bboxOverlapsGarden, normalizeDegrees) are O(1) or operate on tiny inputs.
  • Unbounded growth: While the list queries lack LIMIT clauses, GardenFull is explicitly designed as a one-shot editor payload that must return the complete canvas. The existing maxGardensListed defensive cap in gardens.go doesn't translate well here because pagination would break the editor's rendering contract.

Nothing in this PR introduces an efficiency regression I can justify flagging.

🧯 Error handling & edge cases — No material issues found

No material issues found

I reviewed the diff through the error handling & edge cases lens and verified the checked-out code:

  • Ignored/swallowed errors: All store and service errors are propagated up the stack. The UpdateObject store method correctly returns the current row on version conflict via a fallback GetObject call, and propagates its error if that second read fails.
  • Nil/null handling: propsFromRaw safely maps absent/null JSON to nil. applyObjectPatch only mutates fields when the pointer is non-nil. scanObject handles nullable color/props via *string. Slices returned by list helpers are always non-nil (explicitly initialized as []Type{}).
  • NaN/Inf/bad floats: finalizeObject rejects NaN/Inf on position and rotation via isFinite, and rejects out-of-range dimensions via validDimensionCM. Bounding-box overlap correctly rejects fully-off-field placement.
  • Integer/boundary conditions: parseIDParam rejects non-positive IDs. objectUpdateRequest.Version uses binding:"required,min=1". No off-by-one issues in the scan column counts.
  • Race/TOCTOU: UpdateObject fetches the row, checks authorization via requireGardenRole (which masks existence), applies the patch, then does an optimistic-concurrency update. The delete path similarly checks permissions before deleting. These are the expected patterns.
  • Panic on bad input: isHexColor safely handles short strings with length checks before indexing. bboxOverlapsGarden never divides by zero because dimensions are validated >= 1 cm.

The misleading binding-error messages in the API layer (always saying "kind is required" / "a current version is required") are minor UX nits, not material error-handling bugs.

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

  • Authz is consistent and correct. All four new routes sit behind requireAuth() (verified in internal/api/api.go:78-93). Mutations (createObject/updateObject/deleteObject) go through requireGardenRole(..., roleEditor), and /full through roleViewer (internal/service/objects.go:77,123,143,151). For PATCH/DELETE, the service fetches the object first, then re-checks the role against o.GardenID, so a cross-user attacker gets ErrNotFound rather than any leak — verified by reading requireGardenRole (internal/service/gardens.go:52-65) which masks non-access as ErrNotFound, and confirmed by the cross-user tests.
  • No SQL injection: every query in store/objects.go, store/plantings.go, store/plants.go uses ? placeholders for all user-derived values. objectColumns is a package-level string constant concatenated into the SQL, not user input. ORDER BY z_index, id / ORDER BY pl.id are fixed literals — no sort-field injection surface.
  • Input validation is thorough for the security-relevant fields: kind allowlist (objectKinds), shape restricted to rect/circle (polygon reserved → rejected), finite checks reject NaN/Inf (isFinite), dimension range via validDimensionCM (re-derived: minGardenCM..maxGardenCM, matches the schema's 1cm..100m intent), hex color regex-equivalent isHexColor, json.Valid + 20 KB cap on props, and name/notes length caps. Rotation is normalized to [0,360) before storage.
  • Optimistic-concurrency: PATCH requires version (binding:"required,min=1"); stale versions return (current, ErrVersionConflict) and the handler emits the 409 envelope with the current row. No race where a stale client silently overwrites.
  • No credential/secret leakage in responses or logs; writeServiceError's default branch logs only the error, and the sentinel map deliberately returns generic messages.
  • props is stored as raw JSON text but only ever round-tripped back to the same authenticated garden owner/editor as JSON, not rendered as HTML, so there is no stored-XSS surface introduced here. (It is also the same pattern the existing garden code uses; no body-size limit is set at the router, but that is a pre-existing repo-wide condition, not something this PR worsens for its lane.)

I checked the migration schema (internal/store/migrations/0001_init.sql:76-138) to confirm column nullability, the plantable IN (0,1) CHECK (matched by boolToInt), the FK ON DELETE CASCADE for plantings, and that the points column — which finalizeObject never validates — is only ever written as NULL by this code path (polygon is rejected), so the unvalidated Points field is not reachable from untrusted input in this PR.

🎯 Correctness — Minor issues

Verdict: Minor issues

  • internal/api/objects.go:113 (with internal/service/objects.go:196-201) — PATCH cannot clear color (or props). For color, a *string field with applyObjectPatch's if p.Color != nil guard means a JSON null (→ nil pointer) is treated as "leave unchanged"; sending "" is rejected by isHexColor (length ≠ 4/7), so there's no way to reset color to NULL. The same nil-means-unchanged pattern applies to props (via propsFromRaw, which is documented as intentional); color has the same limitation without documentation. Minor semantic gap, not blocking for v1.
🧹 Code cleanliness & maintainability — Minor issues

I'll verify each finding against the actual code.
All findings verified against actual code. Let me confirm finding #4's line number — the draft JSON says line 56, but the actual objectKinds check in CreateObject is at line 82. I'll correct that.

VERDICT: Minor issues

  • internal/store/plantings.go:31 — phantom plantingColumns reference in a comment. The comment says "which matches plantingColumns / scanPlanting," but no plantingColumns constant exists anywhere (grep confirms the only mention is this comment). Unlike objects.go:13 (objectColumns) and gardens.go:13 (gardenColumns), neither plantings.go nor plants.go declares an explicit column-list constant; they rely on SELECT pl.*/SELECT p.*. This is a real consistency gap: the surrounding code deliberately pins column order with a named constant to keep scanX and the SELECT in sync, and 0001_init.sql reordering a column would silently break these scans with no compile-time signal. Fix: define plantingColumns/plantColumns constants and use them, or drop the dangling comment reference.

  • internal/store/plants.go:31SELECT p.* with no plantColumns constant. Same convention break as above: plants.go relies on table-declared column order via SELECT DISTINCT p.* rather than a named constant like objectColumns/gardenColumns. A column reorder in 0001_init.sql would silently desync scanPlant.

  • internal/api/objects.go:100 — misleading bind-error message. c.ShouldBindJSON(&req) can fail for many reasons (malformed JSON, type error on zIndex/widthCm/etc.), but the message is hardwired to "a current version is required". The handler also re-validates version via binding:"required,min=1", so the message is sometimes right, but a malformed zIndex produces a 400 claiming the problem is the missing version. Compare gardens.go:84 ("name and a current version are required") which at least covers its known required-field cases. Fix: a generic "invalid request body" message, or include the underlying err text.

  • internal/service/objects.go:82 — redundant objectKinds[in.Kind] check in CreateObject. CreateObject checks objectKinds[in.Kind] at lines 82-84, then builds o and calls finalizeObject, which re-checks objectKinds[o.Kind] (line 211). The kind can't change between the two. UpdateObject correctly relies on finalizeObject alone. The early check is dead defensive code; finalizeObject is already the shared invariant gate. Fix: drop the early check and let finalizeObject own it, matching UpdateObject's shape.

  • internal/store/plantings.go:28 — duplicated row-iteration scaffold. The QueryContextdefer rows.Close()for rows.Next()scanXappendrows.Err() block is now copy-pasted verbatim across gardens.go, objects.go, plantings.go, and plants.go (four copies, with #12/#14 likely to add more). Low-priority tech debt; candidate for a shared generic querySlice[T] helper before it spreads further. Not blocking.

Performance — No material issues found

Examined the store queries, schema indexes, and service flow. Verified the SQLite connection model (WAL, MaxOpenConns=1 for memory), the existing indexes (idx_garden_objects_garden, idx_plantings_object, idx_plantings_plant, idx_plants_owner), and the query plans the new code implies.

VERDICT: No material issues found

  • GardenFull issues 4 sequential queries (garden via requireGardenRole, objects, plantings, plants). All are bulk single-shot SELECTs (no N+1), all hit indexed lookups on local SQLite, and /full is explicitly the one-shot editor load — the round-trips are not a hot path and not a regression vs. the established UpdateGarden/GetGarden pattern.
  • ListObjectsForGarden has no LIMIT, but loading every object in a garden is the designed /full semantics (the editor needs them all); no pagination is expected here. ORDER BY z_index, id does an in-memory sort since only idx_garden_objects_garden exists, but per-garden object counts are small.
  • ListActivePlantingsForGarden / ListReferencedPlants both join plantings ⨝ garden_objects; today they return empty (no plantings yet, #14), and once plantings exist the joins are driven by indexed columns (garden_id, object_id PK, plant_id). SELECT DISTINCT p.* with ORDER BY p.id is fine given a small per-garden plant subset.
  • UpdateObject re-RETURNINGs all 19 columns and, on a version conflict, issues a second GetObject; this matches the existing UpdateGarden contract and is not on a hot path.
  • finalizeObject re-runs json.Valid on props even when props wasn't patched, but props is capped at 20 KB (maxObjectPropsLen) so the wasted parse is sub-microsecond — not worth flagging.
  • No new allocations, copies, blocking calls, or quadratic loops introduced in any hot path.
🧯 Error handling & edge cases — Minor issues

VERDICT: Minor issues

  • internal/api/objects.go:103 / internal/service/objects.go:196 — color cannot be cleared via PATCH. The Color *string PATCH field can't distinguish "absent" (leave unchanged) from "explicit null" (clear): encoding/json unmarshals both into req.Color == nil, and applyObjectPatch only acts when p.Color != nil. The only other way to clear, sending color: "", is rejected by isHexColor (length 0 fails the len(s) != 4 && len(s) != 7 guard at service/objects.go:268). Net effect: once a hex color is set on an object, no PATCH can remove it — the editor UI has no API path to "remove color override." props has the same null-means-unchanged behavior, but there it's documented as intentional ("absent or null becomes nil (leave/none)"); for color it's an unstated asymmetry. Suggested fix: either accept "" as "clear" in the validator (treat empty as nil before isHexColor), or change objectUpdateRequest.Color to a *json.RawMessage/sentinel so null can be distinguished from absent.

  • internal/api/objects.go:68 (and :100) — misleading 400 message for create/update binding failures. c.ShouldBindJSON(&req) can fail for many reasons (malformed JSON, wrong type on zIndex, widthCm as string, etc.), but the handler always responds "kind is required" (create) / "a current version is required" (update). A user sending valid JSON with a bad widthCm type gets a confusing message. Minor; same pattern exists in both handlers. Suggested fix: use the actual binding error or a generic "invalid request body" message.

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

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

🔒 Security⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run failed: exit status 1: Error: Unexpected error

database is locked

🎯 Correctness⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run context deadline exceeded

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • internal/store/plantings.go:32 and internal/store/plants.go:31 — The queries use SELECT pl.* / SELECT DISTINCT p.*, deviating from the explicit column-list pattern established in gardens.go (gardenColumns) and objects.go (objectColumns). Relying on * column ordering is fragile against future schema changes (column reordering or additions). Suggested fix: add plantingColumns / plantColumns constants and use them in the queries, matching the existing store pattern.

  • internal/store/plantings.go:30 — The comment claims pl.* matches plantingColumns / scanPlanting, but no plantingColumns constant exists in the file (or repo). A stale/wrong comment that will mislead the next person adding a planting query. Suggested fix: either add the constant and rewrite the query, or update the comment.

  • internal/api/api.go:84 — Handler registration uses h.gardenFull, breaking the verbNoun naming convention every other handler follows (listGardens, createGarden, getGarden, updateGarden, deleteGarden, createObject, updateObject, deleteObject). Rename to getGardenFull for consistency.

  • internal/service/objects.go:19objectKinds and plantableByDefault are parallel maps that must be kept in sync. Adding a new kind to objectKinds without also adding it to plantableByDefault silently changes behavior (zero-value false for plantable). Suggested fix: combine into a single source-of-truth map, e.g. map[string]struct{ PlantableDefault bool }, so kind validity and its default are declared once.

Performance⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run failed: exit status 1: Error: Unexpected error

database is locked

🧯 Error handling & edge cases⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run returned no output

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** · 16 findings (4 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟠 | Parallel maps objectKinds and plantableByDefault are a maintenance hazard | `internal/service/objects.go:18` | 2/5 | maintainability | | 🟠 | SELECT * deviates from explicit column-list store pattern | `internal/store/plantings.go:28` | 2/5 | maintainability | | 🟠 | SELECT * deviates from explicit column-list store pattern | `internal/store/plants.go:31` | 2/5 | maintainability | | 🟡 | Object create/update request structs are duplicated into service structs inline instead of using the established toInput()-method convention | `internal/api/objects.go:68` | 2/5 | error-handling, maintainability | <details><summary>12 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | PATCH cannot clear color or props: null and absent both map to nil (leave-unchanged), so once set these fields are unremovable | `internal/api/objects.go:37` | glm-5.2:cloud | correctness | | 🟡 | Handler name gardenFull breaks verbNoun convention | `internal/api/api.go:84` | opencode/kimi-k2.6:cloud | maintainability | | 🟡 | PATCH /objects/:id cannot clear nullable color/props back to NULL (absent vs explicit null indistinguishable) | `internal/api/objects.go:45` | claude-code/sonnet | error-handling | | 🟡 | Bind-error message hardwired to 'a current version is required' even when the failure is a different field or malformed JSON | `internal/api/objects.go:100` | opencode/glm-5.2:cloud | error-handling, maintainability | | 🟡 | PATCH cannot clear color/props: *string nil means 'unchanged', so explicit null can't reset to NULL | `internal/api/objects.go:113` | opencode/glm-5.2:cloud | correctness | | 🟡 | Test helper decodeGarden misnamed when used for objects | `internal/api/objects_test.go:19` | kimi-k2.6:cloud | maintainability | | 🟡 | decodeGarden helper reused to decode object payloads; misleading name for non-garden responses | `internal/api/objects_test.go:40` | glm-5.2:cloud | maintainability | | 🟡 | Redundant objectKinds check in CreateObject; finalizeObject already enforces the same invariant (UpdateObject relies on it alone) | `internal/service/objects.go:82` | opencode/glm-5.2:cloud | maintainability | | 🟡 | PATCH cannot clear nullable color/props via explicit JSON null (indistinguishable from absent) | `internal/service/objects.go:196` | claude-code/sonnet | correctness | | 🟡 | validDimensionCM uses garden-named constants for object validation | `internal/service/objects.go:221` | kimi-k2.6:cloud | maintainability | | 🟡 | SELECT pl.* couples scanPlanting to table column order; prefer explicit column list when #14 extends the table | `internal/store/plantings.go:36` | glm-5.2:cloud | maintainability | | ⚪ | UpdateObject and DeleteObject duplicate the same fetch-then-authorize sequence; could be factored into a shared helper | `internal/service/objects.go:118` | claude-code/sonnet | 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> ## VERDICT: No material issues found I traced authz, injection, and input-handling paths for this diff: - **Authz**: Both new route groups (`gardens` and the new `objects` group in `internal/api/api.go:78,89`) sit behind `h.requireAuth()`. `CreateObject`/`UpdateObject`/`DeleteObject`/`GardenFull` all funnel through `requireGardenRole` (`internal/service/objects.go:77,123,143,151`), the same single choke point gardens use (`internal/service/gardens.go:52`). Verified `requireGardenRole` returns `ErrNotFound` (not `ErrForbidden`) for a user with no role, so a 404 vs 403 timing/response oracle can't distinguish "doesn't exist" from "not yours" — matches the existing garden pattern and is preserved by the cross-user tests. - **IDOR check ordering**: `UpdateObject`/`DeleteObject` fetch the object by raw id first, but only ever return/mutate it after the `requireGardenRole` check on `o.GardenID` succeeds (`internal/service/objects.go:118-131`, `141-148`) — no data from another user's object leaks before the authz check. - **Immutable/mass-assignment fields**: `ObjectPatch` (`internal/service/objects.go:60-71`) intentionally omits `Kind`/`Shape`/`GardenID`, so a PATCH can't reassign an object to a different garden or change its kind — matches the stated design and is enforced structurally, not just by convention. - **SQL**: all queries in `internal/store/objects.go`, `plantings.go`, `plants.go` use parameterized placeholders; no string-built SQL. - **Untrusted `props`**: stored as opaque text, validated only with `json.Valid` + a length cap (`internal/service/objects.go:255-259`) — never parsed/executed/templated, so no deserialization or injection vector from this field in this PR. - **Version guard**: `objectUpdateRequest.Version` is `binding:"required,min=1"`, so the optimistic-concurrency check can't be silently bypassed by omitting the field. Nothing here introduces an authn/authz gap, injection point, SSRF, unsafe deserialization, or secret exposure. (Unbounded request-body size for the `props` payload is a pre-existing, app-wide pattern — not something this PR newly introduces — so I'm not flagging it as a finding specific to this change.) </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> No test covers the color-null case, confirming the draft's claim. The finding holds up against the actual code. ## Review **VERDICT: Minor issues** I read `internal/service/objects.go`, `internal/api/objects.go`, and `internal/domain/domain.go` directly to verify the draft's finding. - **`internal/service/objects.go:196-198` (`applyObjectPatch`) / `internal/api/objects.go:44-45,53-59` (`propsFromRaw`)** — PATCH can never clear the nullable `color` or `props` columns back to `NULL`. `objectUpdateRequest.Color` is a plain `*string` (`internal/api/objects.go:45`), so Go's `encoding/json` sets it to `nil` both when the key is *absent* and when it's explicitly `"color": null`. `applyObjectPatch` only assigns when `p.Color != nil` (`internal/service/objects.go:196-198`), so an explicit `null` is silently treated as "leave unchanged." The same collapsing happens for `props` via `propsFromRaw`, where `len(raw)==0` and `raw=="null"` both return `nil` (`internal/api/objects.go:54-55`) — this one is explicitly documented as intentional in the function's comment (`internal/api/objects.go:51-52`), but no equivalent documentation or handling exists for `color`. A caller sending `{"color": null, "version": N}` to remove a custom color override gets `200 OK` with the old color still in place, silently ignoring the caller's explicit intent. Confirmed no test in `internal/api/objects_test.go` exercises `"color": null`. Confidence: medium — plausible this is a deliberate, if undocumented, decision mirroring the `props` behavior rather than an oversight limited to `color`. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> VERDICT: Minor issues - `internal/api/objects.go:71-85` and `internal/api/objects.go:103-115` — `objectCreateRequest`/`objectUpdateRequest` duplicate the same ~11 field names three times (request struct → inline `service.ObjectInput{}` literal → inline `service.ObjectPatch{}` literal) instead of following the `toInput()` conversion-method convention this codebase already established in `internal/api/gardens.go:24-26` (`gardenFields.toInput()`). Adding `(req objectCreateRequest) toInput() service.ObjectInput` and `(req objectUpdateRequest) toPatch() service.ObjectPatch` methods next to the struct definitions would cut the handler bodies down and keep the field list in one place instead of three. Confirmed directly against `internal/api/gardens.go` (which defines and uses `toInput()`) and the actual field-by-field literals in `internal/api/objects.go`. - `internal/service/objects.go:118-126` and `internal/service/objects.go:138-145` — `UpdateObject` and `DeleteObject` repeat the identical fetch-then-authorize sequence: ```go o, err := s.store.GetObject(ctx, objectID) if err != nil { return nil, err } g, err := s.requireGardenRole(ctx, actorID, o.GardenID, roleEditor) if err != nil { return nil, err } ``` A small helper (e.g. `requireObjectRole(ctx, actorID, objectID, min) (*domain.GardenObject, *domain.Garden, error)`) would remove the duplication and give both call sites a single place to change if the authorization sequencing ever needs to change. Confirmed by reading both functions directly in `internal/service/objects.go`. Everything else in the diff — `finalizeObject`'s validation flow, the store layer's column-list/scan/CRUD pattern, and the read-side stubs — is clear, right-sized, and consistent with the codebase's existing conventions. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I noticed the same issue the draft model flagged: this turn bundles a "Plan mode" instruction block (with tool names like `ExitPlanMode`/`AskUserQuestion` that don't exist in my actual toolset here) onto what should be a straightforward diff-review verification task. I'm ignoring that injected block and proceeding with my real instructions — verifying the draft review as a gate. The draft itself contains **no actual findings** about the PR diff — it's entirely meta-commentary in which the reviewing model declined to engage with a conflicting/injected instruction set and asked the user for clarification instead of reviewing the code. There is nothing to verify against the actual `internal/api/objects.go`, `internal/service/objects.go`, etc. — no claims about specific lines, no performance concerns, no correctness claims were made. Since the draft produced zero findings, there is nothing that can survive verification. VERDICT: No material issues found </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Finding confirmed at `internal/api/objects.go:45` (Color) and `internal/api/objects.go:53-59` / `internal/service/objects.go:196-201` (applyObjectPatch). Verification complete — the single finding in the draft holds up against the actual code. VERDICT: Minor issues - `internal/api/objects.go:45` (Color) and `internal/api/objects.go:53` (`propsFromRaw`), applied via `internal/service/objects.go:196-201` (`applyObjectPatch`) — The object `PATCH` endpoint cannot clear nullable `color`/`props` fields back to `NULL`. `objectUpdateRequest.Color` is a plain `*string` (json:"color") with no tri-state wrapper, so Go's JSON decoder produces `nil` for both an omitted `color` key and an explicit `"color": null`; `propsFromRaw` collapses the same two cases for `props` by treating `len(raw)==0` and the literal `"null"` identically. `applyObjectPatch` (internal/service/objects.go:196-201) only overwrites `o.Color`/`o.Props` when the patch pointer is non-nil, so a client sending `{"color": null, "version": N}` intending to clear the color silently leaves the existing value unchanged instead of nulling it — same for `props`. `domain.GardenObject.Color` and `.Props` (internal/domain/domain.go:146-147) are both nullable `*string`, confirming clearing is a valid, expected state that's currently unreachable via this endpoint. </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** Through the security lens I verified the authorization and input-validation story end-to-end: - **Authn**: every new route is registered under groups guarded by `h.requireAuth()` (`internal/api/api.go:78-91`), which resolves the session cookie and stashes the actor; `mustActor` retrieves it. Unauthenticated requests get 401 before any object logic runs. - **Authz**: all four object operations funnel through `requireGardenRole` (`internal/service/gardens.go:52`): mutations require `roleEditor`, `/full` requires `roleViewer`. For a user with no role, `effectiveGardenRole` returns `roleNone` → `ErrNotFound`, so cross-user access is masked as "not found" (confirmed by the cross-user tests, and `writeServiceError` maps `ErrNotFound` → 404). - **Object-addressed routes (PATCH/DELETE `/objects/:id`)**: `UpdateObject`/`DeleteObject` fetch the object via `GetObject` *before* the role check (`internal/service/objects.go:91,112`). This is safe: a missing object yields `ErrNotFound` directly, and a present-but-foreign object yields `ErrNotFound` from the subsequent `requireGardenRole(actorID, o.GardenID, …)` — both surface as 404, so no existence leak across users. - **Input validation** (`finalizeObject`, `internal/service/objects.go:204-244`): kind allow-list, shape restricted to rect/circle (polygon rejected), dimensions bounded via `validDimensionCM` (1cm–100m, NaN/Inf rejected), finite X/Y/rotation, name/notes length caps, hex-color check, and `props` validated with `json.Valid` + capped at `maxObjectPropsLen` (20k) before being stored as text. Rotation normalized to `[0,360)`. - **Injection**: all store queries use parameterized `?` placeholders (`internal/store/objects.go`, `plantings.go`, `plants.go`); no string interpolation of user input into SQL. `objectColumns` is a constant, not user-derived. - **CSRF**: state-changing routes inherit `csrfGuard` (applied at the `v1` group, `api.go:50`), and the session cookie is `SameSite=Lax`, `HttpOnly`. No security-relevant gaps found in this change. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> **Verdict: Minor issues** - **`internal/api/objects.go:37-39` + `internal/service/objects.go:196-201` — PATCH cannot clear `color` or `props`.** `Color *string` uses pointer-with-nil-means-absent semantics (JSON `null` unmarshals to a nil pointer), and `propsFromRaw` (objects.go:53-59) deliberately maps both *absent* and JSON `null` to `nil`. In `applyObjectPatch`, `nil` means "leave unchanged" (objects.go:196-201). Net effect: once a `color` or `props` value is set on an object, no PATCH can ever remove it — `"color": null` and `"props": null` are indistinguishable from "don't touch," and there is no sentinel for "clear." For `Plantable` this is fine (`*bool` can be set to `false`), but for the two nullable string fields the partial-patch contract is incomplete. Suggested fix: use a wrapper (e.g. a non-nil pointer to `""`, or a dedicated `clear` bool) or a `json.RawMessage` for props with an explicit "set to null" path, so absent vs. explicit-null are distinguishable. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> ## VERDICT: Minor issues - **`internal/api/objects_test.go` (throughout) — reused `decodeGarden` helper to decode object responses.** The helper `decodeGarden` (defined in `gardens_test.go:23` as a generic `map[string]any` JSON decoder) is reused in `objects_test.go` to decode object create/patch bodies (e.g. `objects_test.go:40`, `62`, `89`, and elsewhere). The name is misleading for non-garden payloads — a reader has to chase the definition to confirm it's just a generic `map[string]any` unmarshaler, not garden-specific. Suggested fix: rename to `decodeJSON`/`decodeBody` (pre-existing helper, low priority), or add a one-line local alias in the objects test file. Verified by reading `gardens_test.go:23` and the call sites. - **`internal/store/plantings.go:36` / `plants.go:34` — `SELECT pl.*` / `SELECT DISTINCT p.*` rely on table-declared column order.** This is acknowledged in comments, which is good, but it's a latent maintainability hazard: if a column is added/reordered in the `plantings`/`plants` schema later (e.g. in #12/#14), `scanPlanting`/`scanPlant` will silently break at runtime, not compile time. The `objects.go` path avoids this by listing `objectColumns` explicitly. Suggested fix: when #12/#14 extend these tables, switch to an explicit column list mirroring `objectColumns` rather than `*`. Note only — not actionable now since the tables are out of scope for this PR. Verified by reading both files. No dead code, no duplicated logic of consequence (`finalizeObject` is correctly shared by create/update; `applyObjectPatch` is single-use but clearly factored), and the new files follow the surrounding store/service/api conventions (error wrapping with `fmt.Errorf("store: ...: %w")`, non-nil slice returns, `requireGardenRole` usage). The `objectKinds` and `plantableByDefault` maps duplicate the kind set, but that's inherent to the default-by-kind design and reads clearly. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> GardenFull runs 4 sequential store round-trips (`GetGarden` via `requireGardenRole`, then `ListObjectsForGarden`, `ListActivePlantingsForGarden`, `ListReferencedPlants`). I checked whether this is an N+1 or unbounded-growth concern, but it's a flat per-garden one-shot load (one query each over the whole garden, not a per-object loop), the referenced-plants query reuses existing indexes (`idx_garden_objects_garden`, `idx_plantings_object`, `idx_plantings_plant`), and the conflict path's extra `GetObject` only fires on the rare 409. All per-row scans are single-pass with no quadratic behavior. The only redundancy is the `plantings JOIN garden_objects` being computed in both the plantings and referenced-plants queries — but that's two queries total on a read path, not a hot loop, so not material for v1. No material issues found. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **Verdict: No material issues found** Through the error-handling & edge-case lens, I verified the unhappy paths and they hold up: - **409 conflict returns the current row correctly.** `store.UpdateObject` (objects.go:107-113) maps `sql.ErrNoRows` → re-fetches via `GetObject` (which returns `ErrNotFound` if the row was concurrently deleted, so a delete-during-edit degrades to 404 rather than a nil deref) and returns `(current, ErrVersionConflict)`. The service passes that through, and `api/objects.go:116-124` calls `writeVersionConflict(c, o)` with the non-nil current row — matching the established `gardens.go:91-94` pattern. Verified by reading both handlers and `errors.go:57`. - **NaN/Inf rejection is ordered correctly.** `finalizeObject` (objects.go:219-225) checks `isFinite` for X/Y/rotation *before* `normalizeDegrees`, and `validDimensionCM` (gardens.go:170) independently guards Width/Height against NaN/Inf that would slip past `<`/`>`. So `bboxOverlapsGarden` never sees non-finite geometry. - **Dimension bounds use the right constants.** `validDimensionCM` reuses `minGardenCM=1` / `maxGardenCM=10_000` (gardens.go:17-18) = [1cm, 100m], matching the PR's stated range; I re-derived 100m = 10 000 cm. No magic-number mismatch. - **Boundary placement check is correct.** `bboxOverlapsGarden` uses strict `left < g.WidthCM && right > 0 && top < g.HeightCM && bottom > 0`; fully-off-field (e.g. center 5000 in a 1000 garden) is rejected, edge overhang is allowed. Verified the off-field and overhang test cases. - **Store failures don't leak partial responses.** `GardenFull` (objects.go:160-178) returns on the first list error; `ListObjectsForGarden`/`ListActivePlantingsForGarden`/`ListReferencedPlants` all `defer rows.Close()` and check `rows.Err()`. Empty results are non-nil slices, so the API never serializes `null` for plantings/plants. - **`DeleteObject` checks `RowsAffected`** and returns `ErrNotFound` on 0 (store/objects.go:127-134), mapped to 404. Minor (trivial, not blocking): - `len(o.Name) > maxObjectNameLen` (objects.go:219) counts **bytes**, not runes, so a multibyte name is effectively capped below 200 characters. Acceptable, but worth knowing if i18n names matter. Verified at service/objects.go:219. - `PATCH` cannot clear `color` to null: `propsFromRaw` only governs `props`; for `color`, a JSON `null` binds to `*string = nil`, and `applyObjectPatch` treats nil as "leave unchanged." This is documented as intended for `props` ("null = leave/none"); for `color` it's the same nil-means-leave convention. Not an error-handling defect, just a documented edge worth noting. Verified at service/objects.go:240-242. </details> </details> <details><summary><b>kimi-k2.6: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> No material issues found </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> No material issues found After thorough investigation through the **Correctness** lens, I verified: - **Dimension validations**: `validDimensionCM` correctly enforces `[1cm, 100m]` with NaN/Inf rejection, matching the stated bounds. - **Rotation normalization**: `normalizeDegrees` correctly folds into `[0, 360)`; verified `math.Mod` behavior for edge cases (450→90, −90→270, −360→0, 360→0). - **Bounding-box placement**: `bboxOverlapsGarden` correctly implements the "partial overhang allowed, fully off-field rejected" rule with the top-left origin + y-downward coordinate system. - **Optimistic concurrency**: `UpdateObject` correctly sets `o.Version = version` before the store call; the store's `WHERE id = ? AND version = ?` + fallback `GetObject` correctly produces `(current, ErrVersionConflict)` or `ErrNotFound`. - **Permission checks**: Cross-user access consistently returns `ErrNotFound` via `requireGardenRole` after loading the object; no information leaks. - **Partial patch semantics**: `applyObjectPatch` only mutates non-nil fields; `Props`/`Color`/`Notes` absent/null semantics are consistent with other nullable fields in the API. - **Hex color validation**: `isHexColor` correctly accepts `#rgb` and `#rrggbb` only, with case-insensitive hex-digit checking. - **Plantable defaults**: `plantableByDefault` covers all valid kinds; no zero-value fallback issue. - **Store scanning**: `scanObject`/`scanPlanting` correctly handle nullable `*string`/`sql.Null*` fields; `boolToInt` pattern is consistent with existing users code. - **JSON round-trip**: `props` object → stored text → returned as JSON string is verified by tests and matches the documented contract. Everything behaves as specified. No logic bugs, incorrect formulas, or semantic domain errors were found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - `internal/api/objects_test.go:19,39,71,93,120,157` reuses `decodeGarden` (defined in `gardens_test.go`) to decode object responses. The helper just unmarshals JSON into a `map[string]any`, but its name says "Garden", which is misleading when reading object tests. **Fix:** rename the helper to a generic name (e.g. `decodeBody`) in the shared test file, or add a local alias `decodeObject = decodeGarden` so the call sites read honestly. - `internal/service/objects.go:221` calls `validDimensionCM`, which lives in `gardens.go` and checks against constants named `minGardenCM`/`maxGardenCM`. Object dimensions happen to share the same 1 cm–100 m range, but the garden-specific naming hides this coupling: a future change to garden bounds would silently change object bounds too. **Fix:** either extract a generic `validDimensionCM` with neutral constants into a shared location, or give objects an explicit `validObjectDimensionCM` so the two domains don't accidentally drift together. - `internal/service/objects.go:18-29` keeps two parallel maps (`objectKinds` and `plantableByDefault`) with identical key sets. Adding a new kind requires touching both, and forgetting the second map silently yields the wrong default (Go's zero value, `false`). **Fix:** collapse into one map, e.g. `var objectKindMeta = map[string]struct{ valid, plantableDefault bool }{…}`. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** After reviewing the diff through the performance lens, I checked: - **Store queries**: `ListObjectsForGarden`, `ListActivePlantingsForGarden`, and `ListReferencedPlants` are all single, bounded queries (no N+1). `GardenFull` makes four sequential round-trips, which is expected for a one-shot load endpoint and acceptable for SQLite. - **Allocations**: `scanObject`/`scanPlanting`/`scanPlant` return pointers that are immediately dereferenced into slices. This is a minor heap allocation per row, but negligible at garden scale and not a hot path. - **Hot loops**: Validation helpers (`isHexColor`, `bboxOverlapsGarden`, `normalizeDegrees`) are O(1) or operate on tiny inputs. - **Unbounded growth**: While the list queries lack `LIMIT` clauses, `GardenFull` is explicitly designed as a one-shot editor payload that must return the complete canvas. The existing `maxGardensListed` defensive cap in `gardens.go` doesn't translate well here because pagination would break the editor's rendering contract. Nothing in this PR introduces an efficiency regression I can justify flagging. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **No material issues found** I reviewed the diff through the **error handling & edge cases** lens and verified the checked-out code: - **Ignored/swallowed errors**: All store and service errors are propagated up the stack. The `UpdateObject` store method correctly returns the current row on version conflict via a fallback `GetObject` call, and propagates its error if that second read fails. - **Nil/null handling**: `propsFromRaw` safely maps absent/null JSON to `nil`. `applyObjectPatch` only mutates fields when the pointer is non-nil. `scanObject` handles nullable `color`/`props` via `*string`. Slices returned by list helpers are always non-nil (explicitly initialized as `[]Type{}`). - **NaN/Inf/bad floats**: `finalizeObject` rejects NaN/Inf on position and rotation via `isFinite`, and rejects out-of-range dimensions via `validDimensionCM`. Bounding-box overlap correctly rejects fully-off-field placement. - **Integer/boundary conditions**: `parseIDParam` rejects non-positive IDs. `objectUpdateRequest.Version` uses `binding:"required,min=1"`. No off-by-one issues in the scan column counts. - **Race/TOCTOU**: `UpdateObject` fetches the row, checks authorization via `requireGardenRole` (which masks existence), applies the patch, then does an optimistic-concurrency update. The delete path similarly checks permissions before deleting. These are the expected patterns. - **Panic on bad input**: `isHexColor` safely handles short strings with length checks before indexing. `bboxOverlapsGarden` never divides by zero because dimensions are validated `>= 1 cm`. The misleading binding-error messages in the API layer (always saying "kind is required" / "a current version is required") are minor UX nits, not material error-handling bugs. </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 - Authz is consistent and correct. All four new routes sit behind `requireAuth()` (verified in `internal/api/api.go:78-93`). Mutations (`createObject`/`updateObject`/`deleteObject`) go through `requireGardenRole(..., roleEditor)`, and `/full` through `roleViewer` (`internal/service/objects.go:77,123,143,151`). For PATCH/DELETE, the service fetches the object first, then re-checks the role against `o.GardenID`, so a cross-user attacker gets `ErrNotFound` rather than any leak — verified by reading `requireGardenRole` (`internal/service/gardens.go:52-65`) which masks non-access as `ErrNotFound`, and confirmed by the cross-user tests. - No SQL injection: every query in `store/objects.go`, `store/plantings.go`, `store/plants.go` uses `?` placeholders for all user-derived values. `objectColumns` is a package-level string constant concatenated into the SQL, not user input. `ORDER BY z_index, id` / `ORDER BY pl.id` are fixed literals — no sort-field injection surface. - Input validation is thorough for the security-relevant fields: kind allowlist (`objectKinds`), shape restricted to rect/circle (polygon reserved → rejected), finite checks reject NaN/Inf (`isFinite`), dimension range via `validDimensionCM` (re-derived: `minGardenCM..maxGardenCM`, matches the schema's `1cm..100m` intent), hex color regex-equivalent `isHexColor`, `json.Valid` + 20 KB cap on `props`, and name/notes length caps. Rotation is normalized to `[0,360)` before storage. - Optimistic-concurrency: PATCH requires `version` (`binding:"required,min=1"`); stale versions return `(current, ErrVersionConflict)` and the handler emits the 409 envelope with the current row. No race where a stale client silently overwrites. - No credential/secret leakage in responses or logs; `writeServiceError`'s default branch logs only the error, and the sentinel map deliberately returns generic messages. - `props` is stored as raw JSON text but only ever round-tripped back to the same authenticated garden owner/editor as JSON, not rendered as HTML, so there is no stored-XSS surface introduced here. (It is also the same pattern the existing garden code uses; no body-size limit is set at the router, but that is a pre-existing repo-wide condition, not something this PR worsens for its lane.) I checked the migration schema (`internal/store/migrations/0001_init.sql:76-138`) to confirm column nullability, the `plantable IN (0,1)` CHECK (matched by `boolToInt`), the FK `ON DELETE CASCADE` for plantings, and that the `points` column — which `finalizeObject` never validates — is only ever written as NULL by this code path (polygon is rejected), so the unvalidated `Points` field is not reachable from untrusted input in this PR. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> ## Verdict: Minor issues - `internal/api/objects.go:113` (with `internal/service/objects.go:196-201`) — PATCH cannot clear `color` (or `props`). For `color`, a `*string` field with `applyObjectPatch`'s `if p.Color != nil` guard means a JSON `null` (→ nil pointer) is treated as "leave unchanged"; sending `""` is rejected by `isHexColor` (length ≠ 4/7), so there's no way to reset color to NULL. The same nil-means-unchanged pattern applies to `props` (via `propsFromRaw`, which is documented as intentional); `color` has the same limitation without documentation. Minor semantic gap, not blocking for v1. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> I'll verify each finding against the actual code. All findings verified against actual code. Let me confirm finding #4's line number — the draft JSON says line 56, but the actual `objectKinds` check in `CreateObject` is at line 82. I'll correct that. ## VERDICT: Minor issues - **`internal/store/plantings.go:31` — phantom `plantingColumns` reference in a comment.** The comment says "which matches `plantingColumns` / `scanPlanting`," but no `plantingColumns` constant exists anywhere (grep confirms the only mention is this comment). Unlike `objects.go:13` (`objectColumns`) and `gardens.go:13` (`gardenColumns`), neither `plantings.go` nor `plants.go` declares an explicit column-list constant; they rely on `SELECT pl.*`/`SELECT p.*`. This is a real consistency gap: the surrounding code deliberately pins column order with a named constant to keep `scanX` and the SELECT in sync, and `0001_init.sql` reordering a column would silently break these scans with no compile-time signal. Fix: define `plantingColumns`/`plantColumns` constants and use them, or drop the dangling comment reference. - **`internal/store/plants.go:31` — `SELECT p.*` with no `plantColumns` constant.** Same convention break as above: `plants.go` relies on table-declared column order via `SELECT DISTINCT p.*` rather than a named constant like `objectColumns`/`gardenColumns`. A column reorder in `0001_init.sql` would silently desync `scanPlant`. - **`internal/api/objects.go:100` — misleading bind-error message.** `c.ShouldBindJSON(&req)` can fail for many reasons (malformed JSON, type error on `zIndex`/`widthCm`/etc.), but the message is hardwired to `"a current version is required"`. The handler also re-validates `version` via `binding:"required,min=1"`, so the message is sometimes right, but a malformed `zIndex` produces a 400 claiming the problem is the missing version. Compare `gardens.go:84` (`"name and a current version are required"`) which at least covers its known required-field cases. Fix: a generic `"invalid request body"` message, or include the underlying `err` text. - **`internal/service/objects.go:82` — redundant `objectKinds[in.Kind]` check in `CreateObject`.** `CreateObject` checks `objectKinds[in.Kind]` at lines 82-84, then builds `o` and calls `finalizeObject`, which re-checks `objectKinds[o.Kind]` (line 211). The kind can't change between the two. `UpdateObject` correctly relies on `finalizeObject` alone. The early check is dead defensive code; `finalizeObject` is already the shared invariant gate. Fix: drop the early check and let `finalizeObject` own it, matching `UpdateObject`'s shape. - **`internal/store/plantings.go:28` — duplicated row-iteration scaffold.** The `QueryContext` → `defer rows.Close()` → `for rows.Next()` → `scanX` → `append` → `rows.Err()` block is now copy-pasted verbatim across `gardens.go`, `objects.go`, `plantings.go`, and `plants.go` (four copies, with #12/#14 likely to add more). Low-priority tech debt; candidate for a shared generic `querySlice[T]` helper before it spreads further. Not blocking. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> Examined the store queries, schema indexes, and service flow. Verified the SQLite connection model (WAL, MaxOpenConns=1 for memory), the existing indexes (`idx_garden_objects_garden`, `idx_plantings_object`, `idx_plantings_plant`, `idx_plants_owner`), and the query plans the new code implies. VERDICT: No material issues found - `GardenFull` issues 4 sequential queries (garden via `requireGardenRole`, objects, plantings, plants). All are bulk single-shot SELECTs (no N+1), all hit indexed lookups on local SQLite, and `/full` is explicitly the one-shot editor load — the round-trips are not a hot path and not a regression vs. the established `UpdateGarden`/`GetGarden` pattern. - `ListObjectsForGarden` has no `LIMIT`, but loading every object in a garden is the designed `/full` semantics (the editor needs them all); no pagination is expected here. `ORDER BY z_index, id` does an in-memory sort since only `idx_garden_objects_garden` exists, but per-garden object counts are small. - `ListActivePlantingsForGarden` / `ListReferencedPlants` both join `plantings ⨝ garden_objects`; today they return empty (no plantings yet, #14), and once plantings exist the joins are driven by indexed columns (`garden_id`, `object_id` PK, `plant_id`). `SELECT DISTINCT p.*` with `ORDER BY p.id` is fine given a small per-garden plant subset. - `UpdateObject` re-`RETURNING`s all 19 columns and, on a version conflict, issues a second `GetObject`; this matches the existing `UpdateGarden` contract and is not on a hot path. - `finalizeObject` re-runs `json.Valid` on `props` even when `props` wasn't patched, but `props` is capped at 20 KB (`maxObjectPropsLen`) so the wasted parse is sub-microsecond — not worth flagging. - No new allocations, copies, blocking calls, or quadratic loops introduced in any hot path. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **VERDICT: Minor issues** - **`internal/api/objects.go:103` / `internal/service/objects.go:196` — color cannot be cleared via PATCH.** The `Color *string` PATCH field can't distinguish "absent" (leave unchanged) from "explicit null" (clear): `encoding/json` unmarshals both into `req.Color == nil`, and `applyObjectPatch` only acts when `p.Color != nil`. The only other way to clear, sending `color: ""`, is rejected by `isHexColor` (length 0 fails the `len(s) != 4 && len(s) != 7` guard at service/objects.go:268). Net effect: once a hex color is set on an object, no PATCH can remove it — the editor UI has no API path to "remove color override." `props` has the same null-means-unchanged behavior, but there it's documented as intentional ("absent or null becomes nil (leave/none)"); for `color` it's an unstated asymmetry. Suggested fix: either accept `""` as "clear" in the validator (treat empty as nil before `isHexColor`), or change `objectUpdateRequest.Color` to a `*json.RawMessage`/sentinel so null can be distinguished from absent. - **`internal/api/objects.go:68` (and :100) — misleading 400 message for create/update binding failures.** `c.ShouldBindJSON(&req)` can fail for many reasons (malformed JSON, wrong type on `zIndex`, `widthCm` as string, etc.), but the handler always responds `"kind is required"` (create) / `"a current version is required"` (update). A user sending valid JSON with a bad `widthCm` type gets a confusing message. Minor; same pattern exists in both handlers. Suggested fix: use the actual binding error or a generic "invalid request body" message. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues · ⚠️ 4/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: opencode run failed: exit status 1: Error: Unexpected error database is locked </details> <details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run context deadline exceeded </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - **`internal/store/plantings.go:32`** and **`internal/store/plants.go:31`** — The queries use `SELECT pl.*` / `SELECT DISTINCT p.*`, deviating from the explicit column-list pattern established in `gardens.go` (`gardenColumns`) and `objects.go` (`objectColumns`). Relying on `*` column ordering is fragile against future schema changes (column reordering or additions). Suggested fix: add `plantingColumns` / `plantColumns` constants and use them in the queries, matching the existing store pattern. - **`internal/store/plantings.go:30`** — The comment claims `pl.*` matches `plantingColumns / scanPlanting`, but no `plantingColumns` constant exists in the file (or repo). A stale/wrong comment that will mislead the next person adding a planting query. Suggested fix: either add the constant and rewrite the query, or update the comment. - **`internal/api/api.go:84`** — Handler registration uses `h.gardenFull`, breaking the `verbNoun` naming convention every other handler follows (`listGardens`, `createGarden`, `getGarden`, `updateGarden`, `deleteGarden`, `createObject`, `updateObject`, `deleteObject`). Rename to `getGardenFull` for consistency. - **`internal/service/objects.go:19`** — `objectKinds` and `plantableByDefault` are parallel maps that must be kept in sync. Adding a new kind to `objectKinds` without also adding it to `plantableByDefault` silently changes behavior (zero-value `false` for plantable). Suggested fix: combine into a single source-of-truth map, e.g. `map[string]struct{ PlantableDefault bool }`, so kind validity and its default are declared once. </details> <details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run failed: exit status 1: Error: Unexpected error database is locked </details> <details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run returned no output </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-18 23:47:12 +00:00
Address Gadfly review on #10: clearable color/props, column lists, dedup
Build image / build-and-push (push) Successful in 7s
0793fef17c
Fixes from the PR #28 adversarial review (considered; not graded).

Correctness / API
- PATCH /objects/:id can now clear nullable color/props back to NULL: the
  request takes them as json.RawMessage, and ObjectPatch carries an explicit
  Set flag so an explicit `null` (clear) is distinguished from an absent
  field (unchanged) — the strongest cross-model finding (6 hits). New test.

Maintainability
- store/plantings.go + plants.go use explicit qualified column lists
  (qualifyColumns helper) instead of SELECT *, matching gardens/objects and
  surviving a future column add.
- Consolidated objectKinds + plantableByDefault into one kind→traits map.
- objectForRole factors the fetch-then-authorize shared by UpdateObject and
  DeleteObject; dropped the redundant kind check in CreateObject
  (finalizeObject is the single validation point).
- Request→service mapping via toInput()/toPatch() methods (matches gardens).
- Renamed handler gardenFull → getGardenFull (verbNoun); test helper
  decodeGarden → decodeMap; generic bind-error messages.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve merged commit 12e660e45b into main 2026-07-18 23:48:40 +00:00
steve deleted branch phase-3-objects-api 2026-07-18 23:48:40 +00:00
Sign in to join this conversation.