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).
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
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>
🪰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>
🟠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>
🟡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>
🟡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…
🪰 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>
🟡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>
🟡 *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>
🟡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>
🟠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.
🪰 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>
🟡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>
⚪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…
🪰 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>
🟡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>
🟡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>
🟠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>
🟡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…
🪰 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>
🟠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>
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-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:
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.
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 GetObjectbefore 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: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.
⚡ 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 beforenormalizeDegrees, 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.
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.
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: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.
⚡ 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
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected 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: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.
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected 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: [91m[1mError: [0mUnexpected 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: [91m[1mError: [0mUnexpected 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>
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 main2026-07-18 23:48:40 +00:00
steve
deleted branch phase-3-objects-api2026-07-18 23:48:40 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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/fullneeds:ListActivePlantingsForGarden(removed_at IS NULL) andListReferencedPlants(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, …)throughrequireGardenRole(editor for mutations, viewer for/full).finalizeObjectvalidates 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./fullreturns{garden, objects, plantings, plants}(plantings/plants empty, non-null — shape fixed now so the frontend types once).propsis any JSON value stored as text; PATCH is partial with a requiredversion, and a 409 returns the current row.Verification
go build/vet/test ./...green (CGO off, GOWORK=off).ErrNotFoundon every op, delete,/fullshape./full, 409 envelope, cross-user 404, polygon/no-kind → 400,propsobject round-trips to a stored JSON string.Out of scope
/fullreads them but they can't exist yet. The field editor UI (#11), the canvas (#9). Polygon shapes (post-v1).🤖 Generated with Claude Code
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-18 23:40:57Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 16 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -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 usesh.gardenFull, breaking theverbNounnaming convention every other handler follows (listGardens,createGarden,getGarden,updateGarden,deleteGarden,createObject,updateObject,deleteObject). Rename togetGardenFullfor consistency.🪰 Gadfly · advisory
@@ -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 clearcolororprops.Color *stringuses pointer-with-nil-means-absent semantics (JSONnullunmarshals to a nil pointer), andpropsFromRaw(objects.go:53-59) deliberately maps both absent and JSONnulltonil. InapplyObjectPatch,nilmeans "leave unchanged" (objects.go:196-201). Net effect: once acolororpropsvalue is set on an object, no PATCH can ever remove it — `"color": n…🪰 Gadfly · advisory
@@ -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) andinternal/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
@@ -0,0 +65,4 @@}var req objectCreateRequestif 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-85andinternal/api/objects.go:103-115—objectCreateRequest/objectUpdateRequestduplicate the same ~11 field names three times (request struct → inlineservice.ObjectInput{}literal → inlineservice.ObjectPatch{}literal) instead of following thetoInput()conversion-method convention this codebase already established ininternal/api/gardens.go:24-26(gardenFields.toInput()). Adding(req objectCreateRequest) toInput() service.ObjectInputand `(re…🪰 Gadfly · advisory
@@ -0,0 +97,4 @@}var req objectUpdateRequestif 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 onzIndex/widthCm/etc.), but the message is hardwired to"a current version is required". The handler also re-validatesversionviabinding:"required,min=1", so the message is sometimes right, but a malformedzIndexproduces a 400 claiming the problem is the missing version. Comparegardens.go:84(`"name and a current version are requir…🪰 Gadfly · advisory
@@ -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(withinternal/service/objects.go:196-201) — PATCH cannot clearcolor(orprops). Forcolor, a*stringfield withapplyObjectPatch'sif p.Color != nilguard means a JSONnull(→ nil pointer) is treated as "leave unchanged"; sending""is rejected byisHexColor(length ≠ 4/7), so there's no way to reset color to NULL. The same nil-means-unchanged pattern applies toprops(viapropsFromRaw, which is documented as intentional);colorhas the…🪰 Gadfly · advisory
@@ -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,157reusesdecodeGarden(defined ingardens_test.go) to decode object responses. The helper just unmarshals JSON into amap[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 aliasdecodeObject = decodeGardenso the call sites read honestly.🪰 Gadfly · advisory
@@ -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
@@ -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:19—objectKindsandplantableByDefaultare parallel maps that must be kept in sync. Adding a new kind toobjectKindswithout also adding it toplantableByDefaultsilently changes behavior (zero-valuefalsefor 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
@@ -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— redundantobjectKinds[in.Kind]check inCreateObject.CreateObjectchecksobjectKinds[in.Kind]at lines 82-84, then buildsoand callsfinalizeObject, which re-checksobjectKinds[o.Kind](line 211). The kind can't change between the two.UpdateObjectcorrectly relies onfinalizeObjectalone. The early check is dead defensive code;finalizeObjectis already the shared invariant gate. Fix: drop the early check and letfinalizeObjectow…🪰 Gadfly · advisory
@@ -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-126andinternal/service/objects.go:138-145—UpdateObjectandDeleteObjectrepeat 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
@@ -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 nullablecolororpropscolumns back toNULL.objectUpdateRequest.Coloris a plain*string(internal/api/objects.go:45), so Go'sencoding/jsonsets it tonilboth when the key is absent and when it's explicitly"color": null.applyObjectPatchonly assigns whenp.Color != nil(internal/service/objects.go:196-198), so an exp…🪰 Gadfly · advisory
@@ -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:221callsvalidDimensionCM, which lives ingardens.goand checks against constants namedminGardenCM/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 genericvalidDimensionCMwith neutral constants into a shared location, or give objects an explicitvalidObjectDimensionCMso the…🪰 Gadfly · advisory
@@ -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:32andinternal/store/plants.go:31— The queries useSELECT pl.*/SELECT DISTINCT p.*, deviating from the explicit column-list pattern established ingardens.go(gardenColumns) andobjects.go(objectColumns). Relying on*column ordering is fragile against future schema changes (column reordering or additions). Suggested fix: addplantingColumns/plantColumnsconstants and use them in the queries, matching the existing store pattern.🪰 Gadfly · advisory
@@ -0,0 +33,4 @@`SELECT pl.* FROM plantings plJOIN garden_objects o ON o.id = pl.object_idWHERE o.garden_id = ? AND pl.removed_at IS NULLORDER 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: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 theplantings/plantsschema later (e.g. in #12/#14),scanPlanting/scanPlantwill silently break at runtime, not compile time. Theobjects.gopath avoids this by listingobjectColumnsexplicitly. Suggested fix: when #12/#14 extend…🪰 Gadfly · advisory
@@ -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:32andinternal/store/plants.go:31— The queries useSELECT pl.*/SELECT DISTINCT p.*, deviating from the explicit column-list pattern established ingardens.go(gardenColumns) andobjects.go(objectColumns). Relying on*column ordering is fragile against future schema changes (column reordering or additions). Suggested fix: addplantingColumns/plantColumnsconstants and use them in the queries, matching the existing store pattern.🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 16 findings (4 with multi-model agreement)
internal/service/objects.go:18internal/store/plantings.go:28internal/store/plants.go:31internal/api/objects.go:6812 single-model findings (lower confidence)
internal/api/objects.go:37internal/api/api.go:84internal/api/objects.go:45internal/api/objects.go:100internal/api/objects.go:113internal/api/objects_test.go:19internal/api/objects_test.go:40internal/service/objects.go:82internal/service/objects.go:196internal/service/objects.go:221internal/store/plantings.go:36internal/service/objects.go:118Per-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:
gardensand the newobjectsgroup ininternal/api/api.go:78,89) sit behindh.requireAuth().CreateObject/UpdateObject/DeleteObject/GardenFullall funnel throughrequireGardenRole(internal/service/objects.go:77,123,143,151), the same single choke point gardens use (internal/service/gardens.go:52). VerifiedrequireGardenRolereturnsErrNotFound(notErrForbidden) 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.UpdateObject/DeleteObjectfetch the object by raw id first, but only ever return/mutate it after therequireGardenRolecheck ono.GardenIDsucceeds (internal/service/objects.go:118-131,141-148) — no data from another user's object leaks before the authz check.ObjectPatch(internal/service/objects.go:60-71) intentionally omitsKind/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.internal/store/objects.go,plantings.go,plants.gouse parameterized placeholders; no string-built SQL.props: stored as opaque text, validated only withjson.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.objectUpdateRequest.Versionisbinding:"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
propspayload 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, andinternal/domain/domain.godirectly 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 nullablecolororpropscolumns back toNULL.objectUpdateRequest.Coloris a plain*string(internal/api/objects.go:45), so Go'sencoding/jsonsets it tonilboth when the key is absent and when it's explicitly"color": null.applyObjectPatchonly assigns whenp.Color != nil(internal/service/objects.go:196-198), so an explicitnullis silently treated as "leave unchanged." The same collapsing happens forpropsviapropsFromRaw, wherelen(raw)==0andraw=="null"both returnnil(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 forcolor. A caller sending{"color": null, "version": N}to remove a custom color override gets200 OKwith the old color still in place, silently ignoring the caller's explicit intent. Confirmed no test ininternal/api/objects_test.goexercises"color": null. Confidence: medium — plausible this is a deliberate, if undocumented, decision mirroring thepropsbehavior rather than an oversight limited tocolor.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
internal/api/objects.go:71-85andinternal/api/objects.go:103-115—objectCreateRequest/objectUpdateRequestduplicate the same ~11 field names three times (request struct → inlineservice.ObjectInput{}literal → inlineservice.ObjectPatch{}literal) instead of following thetoInput()conversion-method convention this codebase already established ininternal/api/gardens.go:24-26(gardenFields.toInput()). Adding(req objectCreateRequest) toInput() service.ObjectInputand(req objectUpdateRequest) toPatch() service.ObjectPatchmethods next to the struct definitions would cut the handler bodies down and keep the field list in one place instead of three. Confirmed directly againstinternal/api/gardens.go(which defines and usestoInput()) and the actual field-by-field literals ininternal/api/objects.go.internal/service/objects.go:118-126andinternal/service/objects.go:138-145—UpdateObjectandDeleteObjectrepeat the identical fetch-then-authorize sequence: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 ininternal/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/AskUserQuestionthat 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) andinternal/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) andinternal/api/objects.go:53(propsFromRaw), applied viainternal/service/objects.go:196-201(applyObjectPatch) — The objectPATCHendpoint cannot clear nullablecolor/propsfields back toNULL.objectUpdateRequest.Coloris a plain*string(json:"color") with no tri-state wrapper, so Go's JSON decoder producesnilfor both an omittedcolorkey and an explicit"color": null;propsFromRawcollapses the same two cases forpropsby treatinglen(raw)==0and the literal"null"identically.applyObjectPatch(internal/service/objects.go:196-201) only overwriteso.Color/o.Propswhen 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 forprops.domain.GardenObject.Colorand.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:
h.requireAuth()(internal/api/api.go:78-91), which resolves the session cookie and stashes the actor;mustActorretrieves it. Unauthenticated requests get 401 before any object logic runs.requireGardenRole(internal/service/gardens.go:52): mutations requireroleEditor,/fullrequiresroleViewer. For a user with no role,effectiveGardenRolereturnsroleNone→ErrNotFound, so cross-user access is masked as "not found" (confirmed by the cross-user tests, andwriteServiceErrormapsErrNotFound→ 404)./objects/:id):UpdateObject/DeleteObjectfetch the object viaGetObjectbefore the role check (internal/service/objects.go:91,112). This is safe: a missing object yieldsErrNotFounddirectly, and a present-but-foreign object yieldsErrNotFoundfrom the subsequentrequireGardenRole(actorID, o.GardenID, …)— both surface as 404, so no existence leak across users.finalizeObject,internal/service/objects.go:204-244): kind allow-list, shape restricted to rect/circle (polygon rejected), dimensions bounded viavalidDimensionCM(1cm–100m, NaN/Inf rejected), finite X/Y/rotation, name/notes length caps, hex-color check, andpropsvalidated withjson.Valid+ capped atmaxObjectPropsLen(20k) before being stored as text. Rotation normalized to[0,360).?placeholders (internal/store/objects.go,plantings.go,plants.go); no string interpolation of user input into SQL.objectColumnsis a constant, not user-derived.csrfGuard(applied at thev1group,api.go:50), and the session cookie isSameSite=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 clearcolororprops.Color *stringuses pointer-with-nil-means-absent semantics (JSONnullunmarshals to a nil pointer), andpropsFromRaw(objects.go:53-59) deliberately maps both absent and JSONnulltonil. InapplyObjectPatch,nilmeans "leave unchanged" (objects.go:196-201). Net effect: once acolororpropsvalue is set on an object, no PATCH can ever remove it —"color": nulland"props": nullare indistinguishable from "don't touch," and there is no sentinel for "clear." ForPlantablethis is fine (*boolcan be set tofalse), 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 dedicatedclearbool) or ajson.RawMessagefor 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) — reuseddecodeGardenhelper to decode object responses. The helperdecodeGarden(defined ingardens_test.go:23as a genericmap[string]anyJSON decoder) is reused inobjects_test.goto 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 genericmap[string]anyunmarshaler, not garden-specific. Suggested fix: rename todecodeJSON/decodeBody(pre-existing helper, low priority), or add a one-line local alias in the objects test file. Verified by readinggardens_test.go:23and 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 theplantings/plantsschema later (e.g. in #12/#14),scanPlanting/scanPlantwill silently break at runtime, not compile time. Theobjects.gopath avoids this by listingobjectColumnsexplicitly. Suggested fix: when #12/#14 extend these tables, switch to an explicit column list mirroringobjectColumnsrather 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 (
finalizeObjectis correctly shared by create/update;applyObjectPatchis single-use but clearly factored), and the new files follow the surrounding store/service/api conventions (error wrapping withfmt.Errorf("store: ...: %w"), non-nil slice returns,requireGardenRoleusage). TheobjectKindsandplantableByDefaultmaps 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 (
GetGardenviarequireGardenRole, thenListObjectsForGarden,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 extraGetObjectonly fires on the rare 409. All per-row scans are single-pass with no quadratic behavior. The only redundancy is theplantings JOIN garden_objectsbeing 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:
store.UpdateObject(objects.go:107-113) mapssql.ErrNoRows→ re-fetches viaGetObject(which returnsErrNotFoundif 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, andapi/objects.go:116-124callswriteVersionConflict(c, o)with the non-nil current row — matching the establishedgardens.go:91-94pattern. Verified by reading both handlers anderrors.go:57.finalizeObject(objects.go:219-225) checksisFinitefor X/Y/rotation beforenormalizeDegrees, andvalidDimensionCM(gardens.go:170) independently guards Width/Height against NaN/Inf that would slip past</>. SobboxOverlapsGardennever sees non-finite geometry.validDimensionCMreusesminGardenCM=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.bboxOverlapsGardenuses strictleft < 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.GardenFull(objects.go:160-178) returns on the first list error;ListObjectsForGarden/ListActivePlantingsForGarden/ListReferencedPlantsalldefer rows.Close()and checkrows.Err(). Empty results are non-nil slices, so the API never serializesnullfor plantings/plants.DeleteObjectchecksRowsAffectedand returnsErrNotFoundon 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.PATCHcannot clearcolorto null:propsFromRawonly governsprops; forcolor, a JSONnullbinds to*string = nil, andapplyObjectPatchtreats nil as "leave unchanged." This is documented as intended forprops("null = leave/none"); forcolorit'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:
validDimensionCMcorrectly enforces[1cm, 100m]with NaN/Inf rejection, matching the stated bounds.normalizeDegreescorrectly folds into[0, 360); verifiedmath.Modbehavior for edge cases (450→90, −90→270, −360→0, 360→0).bboxOverlapsGardencorrectly implements the "partial overhang allowed, fully off-field rejected" rule with the top-left origin + y-downward coordinate system.UpdateObjectcorrectly setso.Version = versionbefore the store call; the store'sWHERE id = ? AND version = ?+ fallbackGetObjectcorrectly produces(current, ErrVersionConflict)orErrNotFound.ErrNotFoundviarequireGardenRoleafter loading the object; no information leaks.applyObjectPatchonly mutates non-nil fields;Props/Color/Notesabsent/null semantics are consistent with other nullable fields in the API.isHexColorcorrectly accepts#rgband#rrggbbonly, with case-insensitive hex-digit checking.plantableByDefaultcovers all valid kinds; no zero-value fallback issue.scanObject/scanPlantingcorrectly handle nullable*string/sql.Null*fields;boolToIntpattern is consistent with existing users code.propsobject → 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,157reusesdecodeGarden(defined ingardens_test.go) to decode object responses. The helper just unmarshals JSON into amap[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 aliasdecodeObject = decodeGardenso the call sites read honestly.internal/service/objects.go:221callsvalidDimensionCM, which lives ingardens.goand checks against constants namedminGardenCM/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 genericvalidDimensionCMwith neutral constants into a shared location, or give objects an explicitvalidObjectDimensionCMso the two domains don't accidentally drift together.internal/service/objects.go:18-29keeps two parallel maps (objectKindsandplantableByDefault) 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:
ListObjectsForGarden,ListActivePlantingsForGarden, andListReferencedPlantsare all single, bounded queries (no N+1).GardenFullmakes four sequential round-trips, which is expected for a one-shot load endpoint and acceptable for SQLite.scanObject/scanPlanting/scanPlantreturn 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.isHexColor,bboxOverlapsGarden,normalizeDegrees) are O(1) or operate on tiny inputs.LIMITclauses,GardenFullis explicitly designed as a one-shot editor payload that must return the complete canvas. The existingmaxGardensListeddefensive cap ingardens.godoesn'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:
UpdateObjectstore method correctly returns the current row on version conflict via a fallbackGetObjectcall, and propagates its error if that second read fails.propsFromRawsafely maps absent/null JSON tonil.applyObjectPatchonly mutates fields when the pointer is non-nil.scanObjecthandles nullablecolor/propsvia*string. Slices returned by list helpers are always non-nil (explicitly initialized as[]Type{}).finalizeObjectrejects NaN/Inf on position and rotation viaisFinite, and rejects out-of-range dimensions viavalidDimensionCM. Bounding-box overlap correctly rejects fully-off-field placement.parseIDParamrejects non-positive IDs.objectUpdateRequest.Versionusesbinding:"required,min=1". No off-by-one issues in the scan column counts.UpdateObjectfetches the row, checks authorization viarequireGardenRole(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.isHexColorsafely handles short strings with length checks before indexing.bboxOverlapsGardennever 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
requireAuth()(verified ininternal/api/api.go:78-93). Mutations (createObject/updateObject/deleteObject) go throughrequireGardenRole(..., roleEditor), and/fullthroughroleViewer(internal/service/objects.go:77,123,143,151). For PATCH/DELETE, the service fetches the object first, then re-checks the role againsto.GardenID, so a cross-user attacker getsErrNotFoundrather than any leak — verified by readingrequireGardenRole(internal/service/gardens.go:52-65) which masks non-access asErrNotFound, and confirmed by the cross-user tests.store/objects.go,store/plantings.go,store/plants.gouses?placeholders for all user-derived values.objectColumnsis a package-level string constant concatenated into the SQL, not user input.ORDER BY z_index, id/ORDER BY pl.idare fixed literals — no sort-field injection surface.objectKinds), shape restricted to rect/circle (polygon reserved → rejected), finite checks reject NaN/Inf (isFinite), dimension range viavalidDimensionCM(re-derived:minGardenCM..maxGardenCM, matches the schema's1cm..100mintent), hex color regex-equivalentisHexColor,json.Valid+ 20 KB cap onprops, and name/notes length caps. Rotation is normalized to[0,360)before storage.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.writeServiceError's default branch logs only the error, and the sentinel map deliberately returns generic messages.propsis 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, theplantable IN (0,1)CHECK (matched byboolToInt), the FKON DELETE CASCADEfor plantings, and that thepointscolumn — whichfinalizeObjectnever validates — is only ever written as NULL by this code path (polygon is rejected), so the unvalidatedPointsfield is not reachable from untrusted input in this PR.🎯 Correctness — Minor issues
Verdict: Minor issues
internal/api/objects.go:113(withinternal/service/objects.go:196-201) — PATCH cannot clearcolor(orprops). Forcolor, a*stringfield withapplyObjectPatch'sif p.Color != nilguard means a JSONnull(→ nil pointer) is treated as "leave unchanged"; sending""is rejected byisHexColor(length ≠ 4/7), so there's no way to reset color to NULL. The same nil-means-unchanged pattern applies toprops(viapropsFromRaw, which is documented as intentional);colorhas 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
objectKindscheck inCreateObjectis at line 82. I'll correct that.VERDICT: Minor issues
internal/store/plantings.go:31— phantomplantingColumnsreference in a comment. The comment says "which matchesplantingColumns/scanPlanting," but noplantingColumnsconstant exists anywhere (grep confirms the only mention is this comment). Unlikeobjects.go:13(objectColumns) andgardens.go:13(gardenColumns), neitherplantings.gonorplants.godeclares an explicit column-list constant; they rely onSELECT pl.*/SELECT p.*. This is a real consistency gap: the surrounding code deliberately pins column order with a named constant to keepscanXand the SELECT in sync, and0001_init.sqlreordering a column would silently break these scans with no compile-time signal. Fix: defineplantingColumns/plantColumnsconstants and use them, or drop the dangling comment reference.internal/store/plants.go:31—SELECT p.*with noplantColumnsconstant. Same convention break as above:plants.gorelies on table-declared column order viaSELECT DISTINCT p.*rather than a named constant likeobjectColumns/gardenColumns. A column reorder in0001_init.sqlwould silently desyncscanPlant.internal/api/objects.go:100— misleading bind-error message.c.ShouldBindJSON(&req)can fail for many reasons (malformed JSON, type error onzIndex/widthCm/etc.), but the message is hardwired to"a current version is required". The handler also re-validatesversionviabinding:"required,min=1", so the message is sometimes right, but a malformedzIndexproduces a 400 claiming the problem is the missing version. Comparegardens.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 underlyingerrtext.internal/service/objects.go:82— redundantobjectKinds[in.Kind]check inCreateObject.CreateObjectchecksobjectKinds[in.Kind]at lines 82-84, then buildsoand callsfinalizeObject, which re-checksobjectKinds[o.Kind](line 211). The kind can't change between the two.UpdateObjectcorrectly relies onfinalizeObjectalone. The early check is dead defensive code;finalizeObjectis already the shared invariant gate. Fix: drop the early check and letfinalizeObjectown it, matchingUpdateObject's shape.internal/store/plantings.go:28— duplicated row-iteration scaffold. TheQueryContext→defer rows.Close()→for rows.Next()→scanX→append→rows.Err()block is now copy-pasted verbatim acrossgardens.go,objects.go,plantings.go, andplants.go(four copies, with #12/#14 likely to add more). Low-priority tech debt; candidate for a shared genericquerySlice[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
GardenFullissues 4 sequential queries (garden viarequireGardenRole, objects, plantings, plants). All are bulk single-shot SELECTs (no N+1), all hit indexed lookups on local SQLite, and/fullis explicitly the one-shot editor load — the round-trips are not a hot path and not a regression vs. the establishedUpdateGarden/GetGardenpattern.ListObjectsForGardenhas noLIMIT, but loading every object in a garden is the designed/fullsemantics (the editor needs them all); no pagination is expected here.ORDER BY z_index, iddoes an in-memory sort since onlyidx_garden_objects_gardenexists, but per-garden object counts are small.ListActivePlantingsForGarden/ListReferencedPlantsboth joinplantings ⨝ garden_objects; today they return empty (no plantings yet, #14), and once plantings exist the joins are driven by indexed columns (garden_id,object_idPK,plant_id).SELECT DISTINCT p.*withORDER BY p.idis fine given a small per-garden plant subset.UpdateObjectre-RETURNINGs all 19 columns and, on a version conflict, issues a secondGetObject; this matches the existingUpdateGardencontract and is not on a hot path.finalizeObjectre-runsjson.Validonpropseven whenpropswasn't patched, butpropsis capped at 20 KB (maxObjectPropsLen) so the wasted parse is sub-microsecond — not worth flagging.🧯 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. TheColor *stringPATCH field can't distinguish "absent" (leave unchanged) from "explicit null" (clear):encoding/jsonunmarshals both intoreq.Color == nil, andapplyObjectPatchonly acts whenp.Color != nil. The only other way to clear, sendingcolor: "", is rejected byisHexColor(length 0 fails thelen(s) != 4 && len(s) != 7guard 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."propshas the same null-means-unchanged behavior, but there it's documented as intentional ("absent or null becomes nil (leave/none)"); forcolorit's an unstated asymmetry. Suggested fix: either accept""as "clear" in the validator (treat empty as nil beforeisHexColor), or changeobjectUpdateRequest.Colorto 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 onzIndex,widthCmas string, etc.), but the handler always responds"kind is required"(create) /"a current version is required"(update). A user sending valid JSON with a badwidthCmtype 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: [91m[1mError: [0mUnexpected 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:32andinternal/store/plants.go:31— The queries useSELECT pl.*/SELECT DISTINCT p.*, deviating from the explicit column-list pattern established ingardens.go(gardenColumns) andobjects.go(objectColumns). Relying on*column ordering is fragile against future schema changes (column reordering or additions). Suggested fix: addplantingColumns/plantColumnsconstants and use them in the queries, matching the existing store pattern.internal/store/plantings.go:30— The comment claimspl.*matchesplantingColumns / scanPlanting, but noplantingColumnsconstant 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 usesh.gardenFull, breaking theverbNounnaming convention every other handler follows (listGardens,createGarden,getGarden,updateGarden,deleteGarden,createObject,updateObject,deleteObject). Rename togetGardenFullfor consistency.internal/service/objects.go:19—objectKindsandplantableByDefaultare parallel maps that must be kept in sync. Adding a new kind toobjectKindswithout also adding it toplantableByDefaultsilently changes behavior (zero-valuefalsefor 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: [91m[1mError: [0mUnexpected 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.