Gardens CRUD + service-layer conventions (actor, version guard, 409) (#7) #26

Merged
steve merged 2 commits from phase-2-gardens-api into main 2026-07-18 22:40:50 +00:00
Owner

Closes #7. Phase 2, step 1 of the tracking epic #20. This establishes the service-layer conventions every later backend issue copies.

Conventions established

  • Actor everywhere: every service method is func (s *Service) X(ctx, actorID int64, …). Handlers pass mustActor(c).ID; future agent tools will pass the same.
  • requireGardenRole(ctx, actor, gardenID, min) is THE authorization point (roles viewer < editor < owner). Owner is implicit via owner_id; #16 extends effectiveGardenRole to consult garden_shares and nothing else changes. A user with no role gets ErrNotFound (existence masked), not ErrForbidden — so cross-user access is a 404, documented.
  • Version-guard / 409: UPDATE … WHERE id=? AND version=? RETURNING …; a no-match re-reads to distinguish (current row, ErrVersionConflict) from ErrNotFound. The 409 envelope — {"error":{"code":"VERSION_CONFLICT","message":…}, "current":{…fresh row…}} — is written by writeVersionConflict and is the contract for every mutable resource (kept consistent with the standard error object rather than the brief's illustrative string form).

What's here

  • store/gardens.go — Create, Get, ListGardensForOwner (non-nil slice, newest first), version-guarded Update, Delete.
  • service/gardens.go — the methods above + validation (name required; 0 dims default to 10 m on create / rejected on update; negatives always rejected; unit metric|imperial; 100 m sanity cap).
  • api/gardens.goGET,POST /api/v1/gardens, GET,PATCH,DELETE /api/v1/gardens/:id behind requireAuth; writeResourceError (404/403/400/409) + parseIDParam.

Verification

  • go build/vet/test ./... green (CGO off, GOWORK=off).
  • Service tests: defaults, validation, owned-only list, version-conflict returns the current row + retry succeeds, cross-user ErrNotFound, delete/re-delete.
  • API tests: full create→list→get→patch(v)→delete flow, the 409 envelope shape, cross-user 404 + empty list, auth-required (401), create/id validation (400).
  • Against the running binary: creating an imperial 4 ft × 8 ft garden stores 122 × 244 cm, and the list returns it.

Out of scope

  • /gardens/:id/full (#10), sharing (#16), the gardens UI (#8).

🤖 Generated with Claude Code

Closes #7. Phase 2, step 1 of the tracking epic #20. This establishes the **service-layer conventions every later backend issue copies**. ## Conventions established - **Actor everywhere**: every service method is `func (s *Service) X(ctx, actorID int64, …)`. Handlers pass `mustActor(c).ID`; future agent tools will pass the same. - **`requireGardenRole(ctx, actor, gardenID, min)`** is THE authorization point (roles `viewer < editor < owner`). Owner is implicit via `owner_id`; **#16 extends `effectiveGardenRole` to consult `garden_shares`** and nothing else changes. A user with **no** role gets `ErrNotFound` (existence masked), not `ErrForbidden` — so cross-user access is a 404, documented. - **Version-guard / 409**: `UPDATE … WHERE id=? AND version=? RETURNING …`; a no-match re-reads to distinguish `(current row, ErrVersionConflict)` from `ErrNotFound`. The 409 envelope — `{"error":{"code":"VERSION_CONFLICT","message":…}, "current":{…fresh row…}}` — is written by `writeVersionConflict` and is **the contract for every mutable resource** (kept consistent with the standard error object rather than the brief's illustrative string form). ## What's here - `store/gardens.go` — Create, Get, ListGardensForOwner (non-nil slice, newest first), version-guarded Update, Delete. - `service/gardens.go` — the methods above + validation (name required; `0` dims default to 10 m on create / rejected on update; negatives always rejected; unit `metric|imperial`; 100 m sanity cap). - `api/gardens.go` — `GET,POST /api/v1/gardens`, `GET,PATCH,DELETE /api/v1/gardens/:id` behind `requireAuth`; `writeResourceError` (404/403/400/409) + `parseIDParam`. ## Verification - `go build`/`vet`/`test ./...` green (CGO off, GOWORK=off). - Service tests: defaults, validation, owned-only list, version-conflict returns the current row + retry succeeds, cross-user `ErrNotFound`, delete/re-delete. - API tests: full create→list→get→patch(v)→delete flow, the 409 envelope shape, cross-user 404 + empty list, auth-required (401), create/id validation (400). - Against the running binary: creating an imperial 4 ft × 8 ft garden stores `122 × 244 cm`, and the list returns it. ## Out of scope - `/gardens/:id/full` (#10), sharing (#16), the gardens UI (#8). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-18 22:20:59 +00:00
Add gardens CRUD + service-layer conventions (#7)
Build image / build-and-push (push) Successful in 4s
Gadfly review (reusable) / review (pull_request) Successful in 9m56s
Adversarial Review (Gadfly) / review (pull_request) Successful in 9m56s
f39ed52868
Establishes the patterns every later backend issue copies: the actor
parameter, centralized role checks, and the version-guard/409 sync
protocol. The service layer is the seam both REST handlers and future
agent tools call, so permissions live here, not in handlers.

- service/gardens.go: Service methods take (ctx, actorID, args).
  requireGardenRole(ctx, actor, gardenID, min) is THE authorization point
  — owner is implicit via owner_id now; #16 extends it to consult
  garden_shares. A user with no role gets ErrNotFound (existence masked),
  not ErrForbidden. Create/Get/List/Update/Delete with input validation
  (name required, 0 dims default to 10 m on create / rejected on update,
  negatives always rejected, unit metric|imperial, 100 m cap).
- store/gardens.go: version-guarded UPDATE ... WHERE id=? AND version=?
  RETURNING; a no-match re-reads to return (current row,
  ErrVersionConflict) vs ErrNotFound. ListGardensForOwner returns a
  non-nil slice.
- api/gardens.go: GET,POST /gardens and GET,PATCH,DELETE /gardens/:id
  behind requireAuth. writeVersionConflict documents the 409 envelope
  ({error:{code,message}, current:{...}}) — the contract for every
  mutable resource. writeResourceError maps ErrNotFound/Forbidden/
  InvalidInput/VersionConflict; parseIDParam guards path ids.

Tests: service (defaults, validation, owned-only list, version
conflict returns current + retry, cross-user ErrNotFound, delete) and
api (full CRUD flow, 409 envelope shape, cross-user 404, auth required,
create validation). Verified against the running binary: create stores
imperial 122x244 cm and list returns it.

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

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-18 22:30:56Z

claude-code/sonnet · claude-code — done

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

glm-5.2:cloud · ollama-cloud — done

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

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

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

opencode/glm-5.2:cloud · opencode — done

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

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

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

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

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

🪰 Gadfly consensus review — 11 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** — 11 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +16,4 @@
// endpoints. Dimensions are centimeters (the API is metric-only; imperial is a
// display concern). On create, omitted dimensions default server-side; on
// update, version is required and every field is replaced.
type gardenCreateRequest struct {

🟠 gardenCreateRequest and gardenUpdateRequest duplicate identical fields and toInput methods

maintainability · flagged by 1 model

  • internal/api/gardens.go:19-41gardenCreateRequest and gardenUpdateRequest duplicate five identical fields and their toInput() methods are byte-for-byte copies. This is unnecessary duplication within the same file; adding a field in the future requires touching four places. Use a shared base struct (e.g., gardenFields) and embed it, or make toInput() a method on the base so the translation lives in one place.

🪰 Gadfly · advisory

🟠 **gardenCreateRequest and gardenUpdateRequest duplicate identical fields and toInput methods** _maintainability · flagged by 1 model_ - **`internal/api/gardens.go:19-41`** — `gardenCreateRequest` and `gardenUpdateRequest` duplicate five identical fields and their `toInput()` methods are byte-for-byte copies. This is unnecessary duplication within the same file; adding a field in the future requires touching four places. Use a shared base struct (e.g., `gardenFields`) and embed it, or make `toInput()` a method on the base so the translation lives in one place. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +30,4 @@
HeightCM float64 `json:"heightCm"`
UnitPref string `json:"unitPref"`
Notes string `json:"notes"`
Version int64 `json:"version" binding:"required"`

🟠 Negative version passes binding and causes misleading 409 conflict

error-handling, maintainability · flagged by 4 models

  • internal/api/gardens.go:33 / internal/service/gardens.go:95 — negative version yields misleading 409 gardenUpdateRequest.Version only carries binding:"required", which for an int64 rejects zero but allows negatives. A negative version reaches the store, fails the WHERE version = ? guard, and triggers the re-read path, so the handler returns 409 VERSION_CONFLICT instead of 400. The client cannot retry from a negative version. Fix: validate version >= 1 in `parseIDPar…

🪰 Gadfly · advisory

🟠 **Negative version passes binding and causes misleading 409 conflict** _error-handling, maintainability · flagged by 4 models_ - **`internal/api/gardens.go:33` / `internal/service/gardens.go:95` — negative version yields misleading 409** `gardenUpdateRequest.Version` only carries `binding:"required"`, which for an `int64` rejects zero but **allows negatives**. A negative version reaches the store, fails the `WHERE version = ?` guard, and triggers the re-read path, so the handler returns **409 VERSION_CONFLICT** instead of 400. The client cannot retry from a negative version. *Fix:* validate `version >= 1` in `parseIDPar… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +116,4 @@
// parseIDParam reads a positive int64 path parameter, writing a 400 and
// returning ok=false on a malformed value.
func parseIDParam(c *gin.Context, name string) (int64, bool) {

🟠 parseIDParam is a generic utility placed in a feature-specific file

maintainability · flagged by 1 model

  • internal/api/gardens.go:119-126parseIDParam is a generic path-parameter utility (any :id route needs it), but it is defined in the gardens handler file. Future resource endpoints will either duplicate it or import it awkwardly from a feature-specific file. Move it to api.go (or a shared API util file) so every handler can reach it without cross-importing gardens code.

🪰 Gadfly · advisory

🟠 **parseIDParam is a generic utility placed in a feature-specific file** _maintainability · flagged by 1 model_ - **`internal/api/gardens.go:119-126`** — `parseIDParam` is a generic path-parameter utility (any `:id` route needs it), but it is defined in the gardens handler file. Future resource endpoints will either duplicate it or import it awkwardly from a feature-specific file. Move it to `api.go` (or a shared API util file) so every handler can reach it without cross-importing gardens code. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +136,4 @@
})
}
// writeResourceError maps the service-layer sentinel errors shared by every

🟠 writeResourceError described as shared by every resource but defined in gardens.go

maintainability · flagged by 2 models

  • internal/api/gardens.go:139-157writeResourceError is described as "shared by every resource," yet it lives in gardens.go. If it is truly meant to be shared by objects, plants, and every later resource, it should sit next to writeAPIError in a central API file (e.g., api.go or spa.go) so later issues don't leave it stranded or duplicate it.

🪰 Gadfly · advisory

🟠 **writeResourceError described as shared by every resource but defined in gardens.go** _maintainability · flagged by 2 models_ - **`internal/api/gardens.go:139-157`** — `writeResourceError` is described as "shared by every resource," yet it lives in `gardens.go`. If it is truly meant to be shared by objects, plants, and every later resource, it should sit next to `writeAPIError` in a central API file (e.g., `api.go` or `spa.go`) so later issues don't leave it stranded or duplicate it. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +145,4 @@
writeAPIError(c, http.StatusNotFound, "NOT_FOUND", "not found")
case errors.Is(err, domain.ErrForbidden):
writeAPIError(c, http.StatusForbidden, "FORBIDDEN", "you don't have access")
case errors.Is(err, domain.ErrInvalidInput):

🟡 writeResourceError duplicates ErrInvalidInput mapping + default slog line from writeServiceError

maintainability · flagged by 1 model

  • internal/api/gardens.go:148-156 vs internal/api/auth.go:232-236 — duplicated error-mapping tail. writeResourceError and writeServiceError both map ErrInvalidInput → 400 INVALID_INPUT "invalid input" and share an identical default branch, including the verbatim slog.Error("api: unhandled service error", "error", err) line. The auth-specific sentinels genuinely belong only in writeServiceError, so the split is fine — but the shared tail (InvalidInput + default slog+500) could b…

🪰 Gadfly · advisory

🟡 **writeResourceError duplicates ErrInvalidInput mapping + default slog line from writeServiceError** _maintainability · flagged by 1 model_ - **`internal/api/gardens.go:148-156` vs `internal/api/auth.go:232-236` — duplicated error-mapping tail.** `writeResourceError` and `writeServiceError` both map `ErrInvalidInput → 400 INVALID_INPUT "invalid input"` and share an identical default branch, including the verbatim `slog.Error("api: unhandled service error", "error", err)` line. The auth-specific sentinels genuinely belong only in `writeServiceError`, so the split is fine — but the shared tail (InvalidInput + default slog+500) could b… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +150,4 @@
case errors.Is(err, domain.ErrVersionConflict):
// Reached only if a caller forgot to special-case the conflict (which
// needs the current row); still return a coherent 409.
writeAPIError(c, http.StatusConflict, "VERSION_CONFLICT", "the resource was modified; refetch and retry")

🟡 409 envelope shape diverges between writeVersionConflict and writeResourceError's ErrVersionConflict branch

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **409 envelope shape diverges between writeVersionConflict and writeResourceError's ErrVersionConflict branch** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +12,4 @@
// absurd or overflow-y values.
const (
defaultGardenCM = 1000
maxGardenCM = 100_000

🔴 maxGardenCM constant is 1 km instead of documented 100 m

correctness, error-handling, maintainability · flagged by 5 models

  • internal/service/gardens.go:15maxGardenCM is 100_000 cm (1 km), but the comment and PR description both state a 100 m sanity cap. 100 m = 10,000 cm, so the constant is off by a factor of 10. This means the validation happily accepts gardens up to 1 km in size, which violates the documented contract and defeats the purpose of the sanity cap. Fix: Change maxGardenCM = 100_000 to maxGardenCM = 10_000.

🪰 Gadfly · advisory

🔴 **maxGardenCM constant is 1 km instead of documented 100 m** _correctness, error-handling, maintainability · flagged by 5 models_ - **`internal/service/gardens.go:15`** — `maxGardenCM` is `100_000` cm (1 km), but the comment and PR description both state a **100 m** sanity cap. 100 m = 10,000 cm, so the constant is off by a factor of 10. This means the validation happily accepts gardens up to 1 km in size, which violates the documented contract and defeats the purpose of the sanity cap. **Fix:** Change `maxGardenCM = 100_000` to `maxGardenCM = 10_000`. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +116,4 @@
// gardenFromInput validates and normalizes input into a domain.Garden (without
// identity/ownership fields). With applyDefaults, absent dimensions/unit are
// filled in (create); without it, every field must be supplied (update).
func gardenFromInput(in GardenInput, applyDefaults bool) (*domain.Garden, error) {

🟠 Unbounded Name/Notes length from untrusted input (memory/storage DoS)

maintainability, security · flagged by 1 model

  • internal/service/gardens.go:119-154 — no length bound on Name/Notes from untrusted input. gardenFromInput only TrimSpaces and rejects empty; it never caps the length of Name or Notes. gardenCreateRequest/gardenUpdateRequest use only binding:"required" with no max= tag. I grepped the repo for any MaxBytesReader/global body limit and found none (the only max= binding in the codebase is on auth.go passwords), so an authenticated user can POST a garden with a multi-…

🪰 Gadfly · advisory

🟠 **Unbounded Name/Notes length from untrusted input (memory/storage DoS)** _maintainability, security · flagged by 1 model_ - **`internal/service/gardens.go:119-154` — no length bound on `Name`/`Notes` from untrusted input.** `gardenFromInput` only `TrimSpace`s and rejects empty; it never caps the length of `Name` or `Notes`. `gardenCreateRequest`/`gardenUpdateRequest` use only `binding:"required"` with no `max=` tag. I grepped the repo for any `MaxBytesReader`/global body limit and found none (the only `max=` binding in the codebase is on `auth.go` passwords), so an authenticated user can POST a garden with a multi-… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +133,4 @@
height = defaultGardenCM
}
}
if width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM {

🔴 NaN dimension values bypass validation bounds

error-handling, security · flagged by 2 models

  • internal/service/gardens.go:136 — NaN bypasses dimension validation The check width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM uses ordered comparisons that are both false for NaN, so a client sending {"widthCm":0,"heightCm":NaN} (or any NaN combination) passes validation on create (after defaulting the non-NaN field) and on update. NaN is then stored in the SQLite REAL column. Fix: add an explicit NaN guard with math.IsNaN before the bounds check.

🪰 Gadfly · advisory

🔴 **NaN dimension values bypass validation bounds** _error-handling, security · flagged by 2 models_ - **`internal/service/gardens.go:136` — NaN bypasses dimension validation** The check `width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM` uses ordered comparisons that are **both false for NaN**, so a client sending `{"widthCm":0,"heightCm":NaN}` (or any NaN combination) passes validation on create (after defaulting the non-NaN field) and on update. NaN is then stored in the SQLite `REAL` column. *Fix:* add an explicit NaN guard with `math.IsNaN` before the bounds check. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +25,4 @@
// CreateGarden inserts a garden (owner_id, name, dimensions, unit, notes already
// set and validated by the service) and returns the stored row.
func (d *DB) CreateGarden(ctx context.Context, g *domain.Garden) (*domain.Garden, error) {

CreateGarden does INSERT + a second SELECT instead of INSERT ... RETURNING (double round-trip per create)

performance · flagged by 1 model

  • internal/store/gardens.go:28CreateGarden does an INSERT then a second GetGarden round-trip to return the stored row (insert id → SELECT by id). SQLite supports INSERT … RETURNING, so this could be one statement. The same pattern is already used in users.go:CreateUser, so it's consistent with house style and not on a hot path; mentioning only because it doubles the round-trips per create. Low impact, unverified whether any caller cares about the cost.

🪰 Gadfly · advisory

⚪ **CreateGarden does INSERT + a second SELECT instead of INSERT ... RETURNING (double round-trip per create)** _performance · flagged by 1 model_ - `internal/store/gardens.go:28` — `CreateGarden` does an `INSERT` then a second `GetGarden` round-trip to return the stored row (insert id → SELECT by id). SQLite supports `INSERT … RETURNING`, so this could be one statement. The same pattern is already used in `users.go:CreateUser`, so it's consistent with house style and not on a hot path; mentioning only because it doubles the round-trips per create. Low impact, unverified whether any caller cares about the cost. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +57,4 @@
// ListGardensForOwner returns the gardens owned by ownerID, newest first. The
// slice is always non-nil (an empty list, not null). Shared-with-me gardens are
// added in #16.
func (d *DB) ListGardensForOwner(ctx context.Context, ownerID int64) ([]domain.Garden, error) {

🟠 ListGardensForOwner has no LIMIT/pagination — unbounded result set and allocation on a hot list endpoint, and this is the convention later list resources copy

performance · flagged by 2 models

  • internal/store/gardens.go:60-82ListGardensForOwner issues an unbounded SELECT … WHERE owner_id = ? ORDER BY created_at DESC, id DESC with no LIMIT. The GET /api/v1/gardens list endpoint (hot, cookie-authed, one row per garden) returns the owner's entire garden set every call, and ListGardensForOwner materializes the full []domain.Garden in memory before returning. As an owner accumulates gardens (and later, in #16, shared gardens are unioned in), the result set and per-request…

🪰 Gadfly · advisory

🟠 **ListGardensForOwner has no LIMIT/pagination — unbounded result set and allocation on a hot list endpoint, and this is the convention later list resources copy** _performance · flagged by 2 models_ - `internal/store/gardens.go:60-82` — `ListGardensForOwner` issues an unbounded `SELECT … WHERE owner_id = ? ORDER BY created_at DESC, id DESC` with no `LIMIT`. The `GET /api/v1/gardens` list endpoint (hot, cookie-authed, one row per garden) returns the owner's entire garden set every call, and `ListGardensForOwner` materializes the full `[]domain.Garden` in memory before returning. As an owner accumulates gardens (and later, in #16, shared gardens are unioned in), the result set and per-request… <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

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

Finding Where Models Lens
🔴 maxGardenCM constant is 1 km instead of documented 100 m internal/service/gardens.go:15 5/5 correctness, error-handling, maintainability
🟠 Negative version passes binding and causes misleading 409 conflict internal/api/gardens.go:33 4/5 error-handling, maintainability
🔴 NaN dimension values bypass validation bounds internal/service/gardens.go:136 2/5 error-handling, security
🟠 writeResourceError described as shared by every resource but defined in gardens.go internal/api/gardens.go:139 2/5 maintainability
🟠 ListGardensForOwner has no LIMIT/pagination — unbounded result set and allocation on a hot list endpoint, and this is the convention later list resources copy internal/store/gardens.go:60 2/5 performance
6 single-model findings (lower confidence)
Finding Where Model Lens
🟠 gardenCreateRequest and gardenUpdateRequest duplicate identical fields and toInput methods internal/api/gardens.go:19 opencode/kimi-k2.6:cloud maintainability
🟠 parseIDParam is a generic utility placed in a feature-specific file internal/api/gardens.go:119 opencode/kimi-k2.6:cloud maintainability
🟠 Unbounded Name/Notes length from untrusted input (memory/storage DoS) internal/service/gardens.go:119 opencode/glm-5.2:cloud maintainability, security
🟡 writeResourceError duplicates ErrInvalidInput mapping + default slog line from writeServiceError internal/api/gardens.go:148 opencode/glm-5.2:cloud maintainability
🟡 409 envelope shape diverges between writeVersionConflict and writeResourceError's ErrVersionConflict branch internal/api/gardens.go:153 glm-5.2:cloud maintainability
CreateGarden does INSERT + a second SELECT instead of INSERT ... RETURNING (double round-trip per create) internal/store/gardens.go:28 opencode/glm-5.2:cloud performance
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

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

🔒 Security — No material issues found

VERDICT: No material issues found

I verified the authorization, injection, and input-validation surfaces of this PR against the checked-out code:

  • IDOR / cross-user accessrequireGardenRole (internal/service/gardens.go:59) is called before every mutating/reading operation and correctly derives role solely from g.OwnerID == actorID; a non-owner gets ErrNotFound (existence masked) confirmed by TestCrossUserAccessIsNotFound. ListGardensForOwner is parameterized by ownerID bound to the authenticated actor (internal/store/gardens.go:47), so no cross-tenant leakage. The version-conflict re-read path in store.UpdateGarden (internal/store/gardens.go:104) re-fetches by the same g.ID already authorized earlier in the same call — not attacker-controlled, so it doesn't reopen an IDOR window.
  • Actor spoofing / mass assignmentactorID always comes from mustActor(c).ID, which is populated by requireAuth from the server-side session (internal/api/auth.go:110-117), never from the request body. gardenCreateRequest/gardenUpdateRequest have no ownerId/id field, so a client cannot reassign ownership or target another row via the body.
  • SQL injection — all queries in internal/store/gardens.go use ? placeholders with driver-level parameter binding; no string concatenation of user input into SQL.
  • CSRFgardens := v1.Group("/gardens", h.requireAuth()) (internal/api/api.go:78) is a subgroup of v1, which already has v1.Use(h.csrfGuard()) applied (internal/api/api.go:50), so the new state-changing endpoints inherit the existing Origin check.
  • Route auth coveragerequireAuth() is applied at the group level to all five garden routes, verified by TestGardenRequiresAuth.
  • Error handling / info leakagewriteResourceError and the writeVersionConflict 409 body only ever return data the actor is already authorized to see (the row was already authorized before either code path can return it); unhandled errors are logged server-side and returned as a generic 500, no internal details leak.

One trivial, low-confidence observation, not blocking:

  • gardenFromInput (internal/service/gardens.go:112) validates presence and numeric bounds for Name/Notes but places no upper bound on string length, and there's no request-body size limit anywhere in the server (cmd/pansy/main.go only sets ReadTimeout). A large name/notes payload could be stored unbounded. This mirrors an existing gap (DisplayName in internal/api/auth.go:28 is equally unbounded), so it isn't a regression introduced by this PR, just an existing pattern it continues.
🎯 Correctness — Minor issues

The test does reference maxGardenCM (the same constant), confirming it can't independently catch the off-by-10x bug, exactly as the draft states.

Corrected review

VERDICT: Minor issues

  • internal/service/gardens.go:15maxGardenCM = 100_000. The code comment ("a generous sanity cap (100 m)") states the intended cap is 100 meters. 1 m = 100 cm, so 100 m = 10,000 cm — the constant should be 10_000, not 100_000. As written the cap permits gardens up to 1000 m (1 km) per side, 10x the documented/intended limit. Verified: the gardens table (internal/store/migrations/0001_init.sql:41-46) has width_cm/height_cm as plain REAL NOT NULL with no CHECK constraint, so this service-layer constant is the only enforcement point. The existing test (gardens_test.go:51-52, WidthCM: maxGardenCM + 1 / HeightCM: maxGardenCM * 10) references the same (wrong) constant rather than an independently-derived value, so it can't catch this.
    • Impact: a garden 1 km on a side can be created; likely not exploitable, but defeats the stated purpose of the cap and will surprise anyone relying on the documented 100 m ceiling.
    • Fix: maxGardenCM = 10_000.

Everything else in the draft's correctness lane checks out on inspection and is not disputed.

🧹 Code cleanliness & maintainability — Minor issues

Both findings in the draft check out against the actual source. Here's the verified review:

VERDICT: Minor issues

  • internal/api/gardens.go:36-41gardenCreateRequest.toInput() and gardenUpdateRequest.toInput() have byte-for-byte identical bodies (map the same 5 fields into service.GardenInput). Verified by reading both methods directly under the struct definitions. Since gardenUpdateRequest already carries every field gardenCreateRequest has plus Version, embedding would drop the duplicate method and keep the two request shapes from drifting apart as fields are added later:

    type gardenUpdateRequest struct {
        gardenCreateRequest
        Version int64 `json:"version" binding:"required"`
    }
    

    Low-churn, and removes one of the two toInput() definitions entirely.

  • internal/api/gardens.go:142-146 vs internal/api/auth.go:216-237writeResourceError's default branch (slog.Error("api: unhandled service error", …) + 500 INTERNAL) is a verbatim copy of writeServiceError's default branch in auth.go:234-237. Confirmed by reading both switch statements directly. Minor and arguably intentional (the PR explicitly frames writeResourceError as a template future resources will copy), so this is a trivial nit rather than something to block on — a small shared writeUnhandledError(c, err) helper would remove the repeat if it recurs a third time.

Everything else in this lens is clean: store/gardens.go mirrors store/users.go's scanner/columns-const/error-wrapping conventions closely, service/gardens.go functions are short and single-purpose, naming (gardenRole, requireGardenRole, effectiveGardenRole) is clear and well-commented, and the test files reuse the existing doJSON/authEngine/newTestService/mustRegister helpers rather than re-implementing them.

Performance — No material issues found

VERDICT: No material issues found

Findings review under the performance lens:

  • internal/store/gardens.go:20-33 (CreateGarden): does INSERT via ExecContext, then a second GetGarden round-trip to fetch the row, instead of an INSERT ... RETURNING ... single-statement fetch — which UpdateGarden (same file, line ~99) already uses and which the driver (modernc.org/sqlite, confirmed in go.mod) supports. This doubles DB calls on every garden create. Verified it mirrors an existing pattern (internal/store/users.go:54-77 CreateUser does the identical Exec+GetUserByID follow-up), so it's consistent with the codebase's existing style rather than a regression introduced by this PR, and the cost is a single extra in-process SQLite query (no network hop) — negligible in absolute terms. Noting only because the PR's own framing is "conventions every later backend issue copies," and this leaves the store layer with two different conventions (RETURNING vs. Exec+refetch) for structurally identical inserts. Not blocking.
  • internal/store/gardens.go:52-56 (ListGardensForOwner): unbounded SELECT ... WHERE owner_id = ? with no LIMIT/pagination. Verified idx_gardens_owner exists (internal/store/migrations/0001_init.sql:53), so the query itself is indexed and cheap; per-user garden counts are naturally small for this domain (a personal gardening app), so I don't think this rises to a material issue worth blocking on.

No N+1 patterns, hot-loop work, or quadratic behavior found in the service/API layers — requireGardenRole and the CRUD handlers each do exactly one store call per request.

🧯 Error handling & edge cases — No material issues found

The draft review provided contains no actual findings — it's just meta-commentary from the prior agent explaining that it wrote a review to a plan file and encountered a tooling limitation (ExitPlanMode unavailable). No specific claims, bugs, or line-level findings were included in the draft text for me to verify against the code. I attempted to read the referenced plan file (/root/.claude/plans/pr-title-gardens-crud-zippy-cosmos.md) to recover the actual findings, but I don't have permission to access it.

Since there is nothing to verify and my role is strictly a verification gate (not a fresh review), the corrected output is:

VERDICT: No material issues found

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

Reviewed through the security lens (authn/authz, injection, input validation, IDOR, secret leakage). Verified against the checked-out code:

  • Authn: All five garden routes are mounted under v1.Group("/gardens", h.requireAuth()) (api.go:78). requireAuth (auth.go:103-123) resolves the session cookie via ResolveSession, rejects missing/tampered/expired tokens with 401, and stashes the user in context. mustActor(c) reads that stashed user — the actor identity is never taken from the request body. ✓
  • Authz / IDOR: actorID for every service call comes from mustActor(c).ID, and gardenID comes from the validated path param. requireGardenRole (service/gardens.go) loads the row by ID and compares g.OwnerID == actorID; a non-owner gets ErrNotFound (existence masked), verified at service/gardens.go:64-69 and confirmed by the cross-user test. UpdateGarden requires roleEditor, DeleteGarden requires roleOwner — both satisfied only by the owner today. ListGardens is scoped to ListGardensForOwner(actorID) with no attacker-controllable owner filter. ✓
  • Injection: All store queries use ? placeholders for every user-supplied value (name, dims, unit, notes, id, version). gardenColumns is a package-level constant string concatenated into SELECT/RETURNING — not user input. ✓
  • Input validation: gardenFromInput trims and rejects blank names, rejects non-positive and over-cap dimensions, and enforces unit_pref ∈ {metric, imperial}. parseIDParam rejects non-numeric and < 1 ids. Update requires an explicit version (binding:"required"). ✓
  • Version-conflict leak: On conflict, the store returns the fresh current row, and the handler only reaches writeVersionConflict after requireGardenRole(roleEditor) already authorized the actor — so the fresh row is only disclosed to an authorized editor/owner, not to a cross-user (who hits ErrNotFound first). ✓
  • CSRF: The v1 group runs csrfGuard (api.go:50), which blocks state-changing cross-origin requests when PANSY_BASE_URL is set. ✓

No secret/credential handling is touched by this change.

🎯 Correctness — Minor issues

VERDICT: Minor issues

  • internal/service/gardens.go:15maxGardenCM is off by 10× from the documented cap. The field comments (gardens.go:10-12) state the sanity cap is 100 m, and the store schema uses centimeters (internal/store/migrations/0001_init.sql uses width_cm REAL / "All measurements are centimeters"; confirmed in internal/domain/domain.go as well). 100 m = 10,000 cm, but maxGardenCM = 100_000 cm = 1,000 m = 1 km. The validation at gardens.go:136 (width > maxGardenCM || height > maxGardenCM) therefore permits gardens up to 1 km on a side instead of the intended 100 m. Re-deriving from first principles: intended cap (100 m) × 100 cm/m = 10,000, not 100,000. Either the constant should be 10_000 (matching the doc) or the doc should say "1 km" — but a 1 km "garden" sanity cap is implausible, so the constant is the bug (an extra zero). Note defaultGardenCM = 1000 cm = 10 m correctly matches its "10 m square" comment, which makes the 100 m → 100,000 cm error the more likely typo. Verified by reading gardens.go:10-16 and the validation at :136; the test at gardens_test.go:50-52 only checks maxGardenCM+1 and maxGardenCM*10 are rejected, so it passes either way and doesn't catch the discrepancy. Suggested fix: maxGardenCM = 10_000 (or update the comment to "1000 m / 1 km" if 1 km was truly intended).
🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • internal/api/gardens.go:36-41 — two near-identical toInput() methods. gardenCreateRequest.toInput() (line 36) and gardenUpdateRequest.toInput() (line 39) are byte-for-byte identical in body. The only structural difference between the two request structs is Version int64 on the update variant (line 33). Per the surrounding convention this is exactly the leaky copy-paste the maintainability lens targets; a single embedded/shared struct or a free function would collapse it. Suggested fix: extract func gardenInputFromRequest(name string, w, h float64, unit, notes string) service.GardenInput, or have gardenUpdateRequest embed gardenCreateRequest. Confidence: high (verified by reading both methods).

  • internal/api/gardens.go:132-137,150-153 — the 409 envelope has two divergent shapes. writeVersionConflict emits {error, current} (lines 133-135), while the ErrVersionConflict fallback branch in writeResourceError emits the flat writeAPIError shape with no current row (line 153). The comment at lines 151-152 justifies the fallback as a "caller forgot to special-case" guard, but the contract for a version-guarded 409 (declared at line 130 as "the contract for every version-guarded resource") is now split across two functions with silently different bodies — a future handler that routes a version conflict through writeResourceError instead of special-casing will produce a 409 missing the current row the client needs. Suggested fix: have writeResourceError's ErrVersionConflict branch call into the same envelope path, or document that it must never be hit and panic/log distinctly. Confidence: high (verified both functions; the normal updateGarden path special-cases the conflict at line 95 and works correctly, so the divergence only affects the fallback).

No dead code, naming, or compile problems found; parseIDParam, writeResourceError, the role-rank helpers, and the store's scanner reuse read cleanly and follow the existing writeAPIError/writeServiceError and userColumns/scanUser patterns.

Performance — Minor issues

VERDICT: Minor issues

  • internal/store/gardens.go:60-82ListGardensForOwner issues an unbounded SELECT … WHERE owner_id = ? ORDER BY created_at DESC, id DESC with no LIMIT. The GET /api/v1/gardens list endpoint (hot, cookie-authed, one row per garden) returns the owner's entire garden set every call, and ListGardensForOwner materializes the full []domain.Garden in memory before returning. As an owner accumulates gardens (and later, in #16, shared gardens are unioned in), the result set and per-request allocation grow without bound. The idx_gardens_owner index keeps the filter cheap, but the result materialization is uncapped. I verified no query in the store package uses a LIMIT (grep for LIMIT returned no matches), so this is consistent with current conventions — but as the list convention every later resource copies (per the PR's stated goal of establishing service-layer conventions), it's worth adding a default cap now (e.g. LIMIT 200 or cursor pagination) rather than retrofitting it across many resources later. Suggested fix: add a LIMIT (and ideally an id cursor / before param) to ListGardensForOwner and thread it through ListGardens.

  • internal/store/gardens.go:15-24 + :71-76 — minor, not material: scanGarden allocates a *domain.Garden on the heap, then ListGardensForOwner copies *g into the slice, so each row pays a pointer allocation plus a value copy. A per-row scan directly into a pre-grown slice element would avoid the intermediate pointer alloc. This is a micro-optimization and the list is currently small/capped only by the unbounded-query issue above, so I'd only bother if the pagination work is done. Not blocking.

I verified both points by reading internal/store/gardens.go, internal/service/gardens.go, the migration schema (idx_gardens_owner exists, no LIMIT anywhere in the store package), and the API handler. No N+1, no blocking calls on hot paths, no quadratic behavior; the version-conflict path's extra GetGarden re-read is necessary to distinguish 404 vs 409 and is at most one additional indexed PK lookup, which is fine.

🧯 Error handling & edge cases — Minor issues

VERDICT: Minor issues

Reviewed the store/service/API error paths end-to-end against the actual source. The unhappy-path handling is mostly solid: sentinel errors propagate cleanly, ErrNotFound masks existence for unauthorized users, the version-conflict path correctly re-reads and returns the fresh row (store/gardens.go:103-109), ListGardensForOwner returns a non-nil slice, parseIDParam rejects 0/negative/malformed ids, and the default branch of writeResourceError logs and returns 500. Two edge-case issues stand out:

  • internal/service/gardens.go:15maxGardenCM = 100_000 contradicts the documented cap, making the boundary 10× too lax. The comment (lines 10-12) states a "100 m sanity cap"; 100 m = 10,000 cm, but the constant is 100,000 cm = 1,000 m. The validation width > maxGardenCM (line 136) therefore lets a 999 m garden through while rejecting anything above 1 km — the "absurd/overflow" guard the comment claims is 10× weaker than intended. Confirmed by reading the constant and the check. Fix: maxGardenCM = 10_000 (or correct the comment to "1000 m", but the documented intent is clearly 100 m).

  • internal/api/gardens.go:33 / internal/service/gardens.go:104 — a negative version is accepted and surfaces as a 409 instead of a 400. gardenUpdateRequest.Version is int64 with binding:"required" (line 33), which only rejects the zero value; {"version": -1} passes binding and flows to store.UpdateGarden, whose WHERE id=? AND version=-1 (store/gardens.go:99) never matches, so it re-reads and returns (current, ErrVersionConflict) (store/gardens.go:103-109). A malformed negative version is thus reported to the client as a real concurrency conflict rather than an input error. Confirmed by tracing through parseIDParam (which does reject negative ids at api/gardens.go:121) vs. the version path (which does not). Suggested fix: reject version < 1 in the handler/service before calling the store.

No swallowed errors or missing cleanup were found: rows.Close() is deferred in ListGardensForOwner, LastInsertId/RowsAffected errors are wrapped, and the default branch of writeResourceError logs and returns 500.

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

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

🔒 Security — No material issues found

No material issues found

  • One potential hardening opportunity, not a current vulnerability: gardenFromInput in internal/service/gardens.go:126 applies no upper bound to Name or Notes length. The DB column is unbounded TEXT, so an authenticated actor can post arbitrarily large strings, consuming memory/storage. A cap (e.g. 255 chars for name, a few KiB for notes) would close a self-DoS / resource-exhaustion vector and is worth establishing now since this PR sets conventions every later resource copies.
  • Verified: all store queries use ? placeholders (internal/store/gardens.go:31, 42, 54, 85, 116) — no SQL injection path.
  • Verified: single-resource endpoints all funnel through requireGardenRole (internal/service/gardens.go:71) with the correct min role (viewer for GET, editor for PATCH, owner for DELETE). Cross-user access returns ErrNotFound, masking existence (internal/service/gardens.go:78).
  • Verified: OwnerID is set server-side in CreateGarden (internal/service/gardens.go:97) and never taken from user input — no mass-assignment of ownership.
  • Verified: UpdateGarden in the store uses WHERE id = ? AND version = ? (internal/store/gardens.go:85), and on conflict the current row is only returned to callers that already passed requireGardenRole with roleEditor, so no information leak to unauthorized users.
🎯 Correctness — Minor issues

Minor issues

  • internal/service/gardens.go:15maxGardenCM is 1,000 m, not the documented 100 m
    The constant maxGardenCM = 100_000 corresponds to 1,000 m (1 km), while the preceding comment explicitly calls it a “generous sanity cap (100 m)”. A 100 m cap should be 10_000 cm. Because of the extra zero, gardens up to 1 km are accepted, which is well beyond the intended limit and could cause downstream rendering or numeric issues.
    Fix: change 100_00010_000.
🧹 Code cleanliness & maintainability⚠️ could not complete

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

Performance — No material issues found

No material issues found

The diff is clean from a performance perspective:

  • No N+1 queries: each handler calls the service once, and each service method issues a single store query (two on the version-conflict path, which is the exceptional case by design).
  • No unbounded hot-path growth: ListGardensForOwner filters to a single owner_id, so result sets are naturally bounded by how many gardens one user owns; the app domain makes multi-thousand-garden users implausible.
  • Efficient update path: UpdateGarden uses UPDATE … RETURNING, so the happy path is a single round-trip.
  • No missing critical index: idx_gardens_owner already exists on gardens(owner_id).
  • No allocation hot loops: strings.TrimSpace in gardenFromInput runs once per request, not in a loop.

The extra GetGarden call when a version conflict is detected (store/gardens.go:111) is intentional and only fires on the 409 path; removing it would break the API contract.

🧯 Error handling & edge cases — No material issues found

No material issues found

The diff is clean from an error-handling and edge-cases perspective.

  • Nil/empty collections: ListGardensForOwner initializes gardens := []domain.Garden{}, guaranteeing a non-nil empty slice. Verified by reading internal/store/gardens.go:70.
  • Zero/negative dimensions: gardenFromInput correctly defaults 0 on create, rejects 0 on update, and rejects negatives in both paths. Verified by reading internal/service/gardens.go:128-138.
  • Version-conflict edge case: The store's UpdateGarden handles sql.ErrNoRows by re-reading to distinguish "gone" (ErrNotFound) from "stale version" (ErrVersionConflict + current row). Verified by reading internal/store/gardens.go:93-115.
  • Cross-user access masking: requireGardenRole returns ErrNotFound (not ErrForbidden) when the actor has no role, correctly masking existence. Verified by reading internal/service/gardens.go:47-59.
  • Missing cleanup: ListGardensForOwner defers rows.Close(). Verified by reading internal/store/gardens.go:68.
  • Unhandled errors: No swallowed errors. All store and service errors are propagated up to handlers and mapped to HTTP responses.
opencode/glm-5.2:cloud (opencode) — Minor issues

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

🔒 Security — Minor issues

I'll verify each finding by reading the actual code.
Now let me verify the claims about body limits and the migration columns.
Both findings are confirmed against the actual code:

  1. Unbounded Name/Notes: gardenFromInput (line 119-154) only TrimSpaces and checks for empty — no length cap. The request structs (lines 19-34) use only binding:"required" on Name, no max= tag, and Notes has no binding tag at all. Migration uses unbounded TEXT (lines 44, 48). Grep confirms no MaxBytesReader or global body limit anywhere in the repo. Survives.

  2. Subnormal-positive dimensions: Line 136 check is width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM. A value like 1e-300 passes all four conditions. Survives.

Review

VERDICT: Minor issues

Through the security lens, the authorization design is sound:

  • All garden routes sit behind requireAuth (internal/api/api.go:78), and the actor ID is sourced from the resolved session via mustActor(c).ID (internal/api/gardens.go:46,60,73,91,110) — never from the request body, so owner spoofing on create is impossible (internal/service/gardens.go:78 sets g.OwnerID = actorID).
  • Every read/mutate funnels through requireGardenRole (internal/service/gardens.go:47), and a user with no role gets ErrNotFound rather than ErrForbidden — existence is masked (verified the cross-user tests in internal/service/gardens_test.go:158 and internal/api/gardens_test.go:133).
  • All SQL in internal/store/gardens.go uses ? placeholders (no string interpolation of user input), so no injection. parseIDParam (internal/api/gardens.go:119) parses to int64 and rejects < 1.
  • The version-guarded UPDATE … WHERE id=? AND version=? RETURNING … (internal/store/gardens.go:95-101) prevents lost updates; the fresh row returned on 409 only goes to an already-authorized editor, so no cross-user leak.
  • The default branch of writeResourceError logs err server-side but returns a generic "internal error" to the client (internal/api/gardens.go:155-156) — no internal-detail leakage.
  • csrfGuard is applied to the whole v1 group (internal/api/api.go:50), covering POST/PATCH/DELETE on /gardens when PANSY_BASE_URL is set.

Findings:

  • internal/service/gardens.go:119-154 — no length bound on Name/Notes from untrusted input. gardenFromInput only TrimSpaces and rejects empty; it never caps the length of Name or Notes. gardenCreateRequest/gardenUpdateRequest use only binding:"required" with no max= tag. I grepped the repo for any MaxBytesReader/global body limit and found none (the only max= binding in the codebase is on auth.go passwords), so an authenticated user can POST a garden with a multi-megabyte name/notes (or repeatedly PATCH to grow notes) with no server-side bound. The store column is unbounded TEXT (internal/store/migrations/0001_init.sql:44,48). This is a memory/storage-DoS vector available to any registered user. Suggested fix: add max= binding tags (e.g. max=200 for name, max=10000 for notes) and/or an explicit length check in gardenFromInput returning ErrInvalidInput, plus consider a global http.MaxBytesReader middleware. Verified by reading gardenFromInput, the request structs, and grepping for body-limit/length enforcement.

  • internal/service/gardens.go:136 — subnormal-positive dimensions slip past validation. The guard is width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM. A JSON number like 1e-300 (or any tiny positive value) satisfies > 0 and <= maxGardenCM, so it is accepted and stored as a valid garden dimension. This is a data-integrity gap from untrusted input rather than a direct exploit (and NaN/Inf are not representable as JSON numbers, so they cannot be smuggled in via ShouldBindJSON). Low impact, but worth tightening to a positive minimum (e.g. reject width < 1 cm) for defense in depth. Verified by reading the comparison and the float64 field types.

🎯 Correctness — Minor issues

VERDICT: Minor issues

  • internal/service/gardens.go:15maxGardenCM = 100_000 is 10× too large. The comment at lines 10–12 explicitly states the cap is "100 m", and 100 m = 10,000 cm (1 m = 100 cm). 100_000 cm is actually 1,000 m (1 km). The schema (internal/store/migrations/0001_init.sql:45-46) has no CHECK constraint on width_cm/height_cm, so this constant is the sole guard against absurd dimensions — and it permits a 1 km × 1 km garden, exactly the "absurd" case the comment says it prevents. Fix: maxGardenCM = 10_000 (100 m). Verified against the source and the migration; existing tests use maxGardenCM + 1 and maxGardenCM * 10 so they stay valid after the fix.
🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • internal/service/gardens.go:11-16 — comment contradicts the constant. The comment says "the max is a generous sanity cap (100 m)" but maxGardenCM = 100_000 cm = 1000 m (100 m would be 10_000 cm). The test uses WidthCM: maxGardenCM + 1, so it validates against the constant rather than the documented bound and won't catch the discrepancy. Either the comment is wrong or the constant is; either way a reader can't trust the stated cap. Verified by reading the file and re-deriving the unit conversion. Suggested fix: decide which is intended; if 100 m, set maxGardenCM = 10_000.

  • internal/api/gardens.go:19-41 — duplicated request structs and identical toInput() methods. gardenCreateRequest and gardenUpdateRequest share every field except Version, and their toInput() bodies are literally identical (lines 36-41). Since the PR description frames this as the convention-setter later backends will copy, the duplication will propagate to every resource's create/update handlers. Suggested fix: embed a shared gardenFields struct (or have one struct with Version *int64) with a single toInput(). Verified by reading the file.

  • internal/api/gardens.go:148-156 vs internal/api/auth.go:232-236 — duplicated error-mapping tail. writeResourceError and writeServiceError both map ErrInvalidInput → 400 INVALID_INPUT "invalid input" and share an identical default branch, including the verbatim slog.Error("api: unhandled service error", "error", err) line. The auth-specific sentinels genuinely belong only in writeServiceError, so the split is fine — but the shared tail (InvalidInput + default slog+500) could be factored into a common helper now that there are two mappers in the package. Verified by reading both. Minor.

  • internal/service/gardens.go:119applyDefaults bool flag argument. gardenFromInput(in, true) / gardenFromInput(in, false) is a classic flag argument that changes the function's contract (defaults vs. reject-zeros) at the call site; readers have to jump to the body to know which. Since this is a convention-setter, consider two named functions (normalizeCreateInput / normalizeUpdateInput) sharing a validate helper. Minor / stylistic.

No dead code, no naming problems, no broken patterns in the store layer (it mirrors users.go's scanner/xxxColumns/scanXxx convention correctly), and the route registration follows the existing auth group style.

Performance — Minor issues

VERDICT: Minor issues

  • internal/store/gardens.go:60ListGardensForOwner has no LIMIT/pagination. It loads every garden a user owns into memory in one query (SELECT … WHERE owner_id = ? ORDER BY created_at DESC, id DESC). Today the per-user count is small, but this is the canonical "list the actor's gardens" endpoint and the shape #16 will extend (sharing adds rows the query must also return). As written, an account with thousands of gardens builds a slice of thousands of domain.Garden values on every GET /api/v1/gardens. Worth at least a defensive LIMIT (e.g. 1000) now, or committing to pagination before the list grows. Verified by reading the migration: idx_gardens_owner covers the owner_id filter, so the query itself is index-backed — the concern is only the unbounded result materialization, not a missing index.

  • internal/store/gardens.go:28CreateGarden does an INSERT then a second GetGarden round-trip to return the stored row (insert id → SELECT by id). SQLite supports INSERT … RETURNING, so this could be one statement. The same pattern is already used in users.go:CreateUser, so it's consistent with house style and not on a hot path; mentioning only because it doubles the round-trips per create. Low impact, unverified whether any caller cares about the cost.

🧯 Error handling & edge cases — No material issues found

Looking through the error-handling lens. Let me verify the validation bounds and the conflict-path nil handling.
Looking at the error-handling/edge-case lens. Let me check the validation boundary case for maxGardenCM + 1 with float64, and the UpdateGarden conflict-path error wrapping.
Now I have everything I need to assess the error-handling lens. Let me review the findings I want to confirm:

Key edge cases to check:

  1. updateGarden handler ErrVersionConflict path with g==nil: At api/gardens.go:95-96, when UpdateGarden returns ErrVersionConflict, the service contract says it returns (current, ErrVersionConflict) — so g is non-nil. But what about the other sentinel errors? On ErrInvalidInput (e.g., negative dims detected in gardenFromInput), UpdateGarden returns (nil, ErrInvalidInput). The handler checks errors.Is(err, domain.ErrVersionConflict) first → false, so falls through to writeResourceError → maps to 400. Safe. Good.

  2. store.UpdateGarden race: After sql.ErrNoRows it calls d.GetGarden to distinguish gone vs stale. There's a TOCTOU window but that's not really an error-handling bug.

  3. CreateGarden error from LastInsertId: SQLite via modernc supports it. But if LastInsertId returns an error (rare), GetGarden is skipped — error is propagated. Fine.

  4. ListGardensForOwner scan error during iteration: Returns immediately on scan error, leaving rows to be closed by defer. Good.

  5. Negative dimensions on update with applyDefaults=false: width <= 0 catches negatives. Good. Test case covers negative width on create.

  6. width > maxGardenCM with float: maxGardenCM = 100_000. maxGardenCM + 1 in test = 100001. Float comparison fine.

  7. Empty body / non-JSON POST to createGarden: c.ShouldBindJSON returns an error → 400. Good.

  8. writeVersionConflict(c, nil): Could g be nil here? Service UpdateGarden returns (nil, err) only when requireGardenRole fails or gardenFromInput fails — neither returns ErrVersionConflict. The store returns (current, ErrVersionConflict) where current is non-nil (GetGarden succeeded). So g is non-nil in the conflict path. Safe.

Let me double-check one subtle issue: in store.UpdateGarden, when the re-read's GetGarden returns a real (non-ErrNotFound) error, UpdateGarden returns (nil, gerr) with gerr wrapped — meaning the conflict envelope isn't written, which is correct behavior.

Let me check whether the conflict-envelope's "current" could include stale data — no, it's freshly re-read.

Let me check writeAPIError for completeness:

Verdict: No material issues found

Through the error-handling & edge-case lens, I verified the unhappy paths introduced by this diff:

  • Conflict-path nil safety (api/gardens.go:91-101): I checked the contract carefully. service.UpdateGarden only returns (non-nil, ErrVersionConflict) — the store's re-read returns (current, ErrVersionConflict) with current non-nil (it only reaches that branch when GetGarden succeeded), and the service passes the store's pair through unchanged. The other error branches (requireGardenRole, gardenFromInput) return (nil, non-conflict-err), so they correctly skip the writeVersionConflict(c, g) branch and fall to writeResourceError. No nil-deref on the conflict envelope. Verified by reading service/gardens.go:95-106 and store/gardens.go:103-114.
  • Validation boundaries (service/gardens.go:119-155): width <= 0 || height <= 0 correctly catches both zero (the update path where defaults are not applied) and negatives on either path; > maxGardenCM is a clean float compare. Name trimmed-then-empty check, unit whitelist, and the asymmetric defaulting (applyDefaults only on create) are all handled. No NaN-specific guard, but encoding/json doesn't produce NaN from valid JSON and the DB stores REAL — out of scope for this diff.
  • Store error propagation (store/gardens.go): ListGardensForOwner checks both rows.Next() scan errors and rows.Err() after the loop; rows.Close() is deferred. DeleteGarden distinguishes 0-rows-affected → ErrNotFound. UpdateGarden's sql.ErrNoRows branch re-reads and maps gone-vs-stale correctly; a real error from the re-read is returned unwrapped-as-is (gerr, which is itself wrapped in GetGarden) rather than masked.
  • parseIDParam (api/gardens.go:119-126): id < 1 rejects zero/negative; ParseInt rejects non-numeric and overflow → 400. Good.
  • Auth gating: every handler resolves the actor via mustActor(c) after requireAuth; an unauthenticated request is aborted before reaching the handler, so mustActor can't panic on these routes in the request path.
  • Empty-collection edge case: ListGardensForOwner initializes gardens := []domain.Garden{}, so an owner with no gardens returns a non-nil empty slice — matching the [] test assertion and avoiding a null JSON response. Verified at store/gardens.go:70.

Nothing in the error-handling/edge-case lane is materially wrong.

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

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

🔒 Security — No material issues found

Verdict: No material issues found

I verified the security-sensitive paths introduced by this PR and found no exploitable gaps.

Checks performed

  • Authn/authz: All garden routes (GET /gardens, POST /gardens, GET /gardens/:id, PATCH /gardens/:id, DELETE /gardens/:id) are registered behind h.requireAuth() (verified in internal/api/api.go:78). mustActor pulls the authenticated user from the gin context set by requireAuth; there is no handler-level bypass.
  • Authorization logic: requireGardenRole in internal/service/gardens.go:47 is the single gate. It calls GetGarden, then checks ownership via effectiveGardenRole. A user with no role gets ErrNotFound (existence masked, 404), not ErrForbidden. Cross-user IDOR is mitigated: I traced TestGardenCrossUserIsNotFound in the API tests and TestCrossUserAccessIsNotFound in the service tests to confirm Bob cannot learn Alice’s garden exists.
  • SQL injection: Every store method (internal/store/gardens.go:28–132) uses ? placeholders. The only string concatenation is gardenColumns, a hardcoded constant. No tainted input reaches the query text.
  • Input validation: parseIDParam rejects non-positive int64s. gardenFromInput enforces name presence, trims whitespace, validates dimensions (positive, capped at 100 m), and whitelists unitPref to metric|imperial. Zero dimensions default on create and are rejected on update. No injection vectors via untrusted dimensions or names.
  • CSRF: State-changing methods (POST, PATCH, DELETE) on /gardens fall under the existing csrfGuard middleware (internal/api/api.go:50), which checks the Origin header when PANSY_BASE_URL is configured. SameSite=Lax on the session cookie provides defense in depth.
  • Version-conflict TOCTOU: UpdateGarden uses WHERE id=? AND version=? RETURNING …. On sql.ErrNoRows it re-reads to distinguish “gone” from “stale.” The fresh row is only returned to a caller who already passed requireGardenRole with at least roleEditor, so no information leakage occurs.
  • Deserialization / SSRF / secrets: None introduced.
  • Unsafe defaults: Session cookie settings (HttpOnly, SameSite=Lax, Secure conditional on HTTPS) are pre-existing and unchanged.

One might note that Notes and Name lack an upper-length bound, which is technically missing input validation. For a local SQLite garden app this amounts to a trivial DoS nit rather than a material vulnerability, so I am leaving it out.

🎯 Correctness — Blocking issues found

Blocking issues found

  • internal/service/gardens.go:15maxGardenCM is 100_000 cm (1 km), but the comment and PR description both state a 100 m sanity cap.
    100 m = 10,000 cm, so the constant is off by a factor of 10.
    This means the validation happily accepts gardens up to 1 km in size, which violates the documented contract and defeats the purpose of the sanity cap.
    Fix: Change maxGardenCM = 100_000 to maxGardenCM = 10_000.

Verified by reading the source file and independently re-deriving the conversion (1 m = 100 cm ⇒ 100 m = 10,000 cm).

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • internal/api/gardens.go:19-41gardenCreateRequest and gardenUpdateRequest duplicate five identical fields and their toInput() methods are byte-for-byte copies. This is unnecessary duplication within the same file; adding a field in the future requires touching four places. Use a shared base struct (e.g., gardenFields) and embed it, or make toInput() a method on the base so the translation lives in one place.

  • internal/api/gardens.go:119-126parseIDParam is a generic path-parameter utility (any :id route needs it), but it is defined in the gardens handler file. Future resource endpoints will either duplicate it or import it awkwardly from a feature-specific file. Move it to api.go (or a shared API util file) so every handler can reach it without cross-importing gardens code.

  • internal/api/gardens.go:139-157writeResourceError is described as "shared by every resource," yet it lives in gardens.go. If it is truly meant to be shared by objects, plants, and every later resource, it should sit next to writeAPIError in a central API file (e.g., api.go or spa.go) so later issues don't leave it stranded or duplicate it.

Performance — No material issues found

No material issues found.

🧯 Error handling & edge cases — Blocking issues found

Blocking issues found

  • internal/service/gardens.go:136 — NaN bypasses dimension validation
    The check width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM uses ordered comparisons that are both false for NaN, so a client sending {"widthCm":0,"heightCm":NaN} (or any NaN combination) passes validation on create (after defaulting the non-NaN field) and on update. NaN is then stored in the SQLite REAL column.
    Fix: add an explicit NaN guard with math.IsNaN before the bounds check.

  • internal/api/gardens.go:33 / internal/service/gardens.go:95 — negative version yields misleading 409
    gardenUpdateRequest.Version only carries binding:"required", which for an int64 rejects zero but allows negatives. A negative version reaches the store, fails the WHERE version = ? guard, and triggers the re-read path, so the handler returns 409 VERSION_CONFLICT instead of 400. The client cannot retry from a negative version.
    Fix: validate version >= 1 in parseIDParam style (or in the service layer) and return ErrInvalidInput.

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

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 5 models **Verdict: Blocking issues found** · 11 findings (5 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🔴 | maxGardenCM constant is 1 km instead of documented 100 m | `internal/service/gardens.go:15` | 5/5 | correctness, error-handling, maintainability | | 🟠 | Negative version passes binding and causes misleading 409 conflict | `internal/api/gardens.go:33` | 4/5 | error-handling, maintainability | | 🔴 | NaN dimension values bypass validation bounds | `internal/service/gardens.go:136` | 2/5 | error-handling, security | | 🟠 | writeResourceError described as shared by every resource but defined in gardens.go | `internal/api/gardens.go:139` | 2/5 | maintainability | | 🟠 | ListGardensForOwner has no LIMIT/pagination — unbounded result set and allocation on a hot list endpoint, and this is the convention later list resources copy | `internal/store/gardens.go:60` | 2/5 | performance | <details><summary>6 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | gardenCreateRequest and gardenUpdateRequest duplicate identical fields and toInput methods | `internal/api/gardens.go:19` | opencode/kimi-k2.6:cloud | maintainability | | 🟠 | parseIDParam is a generic utility placed in a feature-specific file | `internal/api/gardens.go:119` | opencode/kimi-k2.6:cloud | maintainability | | 🟠 | Unbounded Name/Notes length from untrusted input (memory/storage DoS) | `internal/service/gardens.go:119` | opencode/glm-5.2:cloud | maintainability, security | | 🟡 | writeResourceError duplicates ErrInvalidInput mapping + default slog line from writeServiceError | `internal/api/gardens.go:148` | opencode/glm-5.2:cloud | maintainability | | 🟡 | 409 envelope shape diverges between writeVersionConflict and writeResourceError's ErrVersionConflict branch | `internal/api/gardens.go:153` | glm-5.2:cloud | maintainability | | ⚪ | CreateGarden does INSERT + a second SELECT instead of INSERT ... RETURNING (double round-trip per create) | `internal/store/gardens.go:28` | opencode/glm-5.2:cloud | performance | </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 verified the authorization, injection, and input-validation surfaces of this PR against the checked-out code: - **IDOR / cross-user access** — `requireGardenRole` (`internal/service/gardens.go:59`) is called before every mutating/reading operation and correctly derives role solely from `g.OwnerID == actorID`; a non-owner gets `ErrNotFound` (existence masked) confirmed by `TestCrossUserAccessIsNotFound`. `ListGardensForOwner` is parameterized by `ownerID` bound to the authenticated actor (`internal/store/gardens.go:47`), so no cross-tenant leakage. The version-conflict re-read path in `store.UpdateGarden` (`internal/store/gardens.go:104`) re-fetches by the *same* `g.ID` already authorized earlier in the same call — not attacker-controlled, so it doesn't reopen an IDOR window. - **Actor spoofing / mass assignment** — `actorID` always comes from `mustActor(c).ID`, which is populated by `requireAuth` from the server-side session (`internal/api/auth.go:110-117`), never from the request body. `gardenCreateRequest`/`gardenUpdateRequest` have no `ownerId`/`id` field, so a client cannot reassign ownership or target another row via the body. - **SQL injection** — all queries in `internal/store/gardens.go` use `?` placeholders with driver-level parameter binding; no string concatenation of user input into SQL. - **CSRF** — `gardens := v1.Group("/gardens", h.requireAuth())` (`internal/api/api.go:78`) is a subgroup of `v1`, which already has `v1.Use(h.csrfGuard())` applied (`internal/api/api.go:50`), so the new state-changing endpoints inherit the existing Origin check. - **Route auth coverage** — `requireAuth()` is applied at the group level to all five garden routes, verified by `TestGardenRequiresAuth`. - **Error handling / info leakage** — `writeResourceError` and the `writeVersionConflict` 409 body only ever return data the actor is already authorized to see (the row was already authorized before either code path can return it); unhandled errors are logged server-side and returned as a generic 500, no internal details leak. One trivial, low-confidence observation, not blocking: - `gardenFromInput` (`internal/service/gardens.go:112`) validates presence and numeric bounds for `Name`/`Notes` but places no upper bound on string length, and there's no request-body size limit anywhere in the server (`cmd/pansy/main.go` only sets `ReadTimeout`). A large `name`/`notes` payload could be stored unbounded. This mirrors an existing gap (`DisplayName` in `internal/api/auth.go:28` is equally unbounded), so it isn't a regression introduced by this PR, just an existing pattern it continues. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> The test does reference `maxGardenCM` (the same constant), confirming it can't independently catch the off-by-10x bug, exactly as the draft states. ## Corrected review **VERDICT: Minor issues** - `internal/service/gardens.go:15` — `maxGardenCM = 100_000`. The code comment ("a generous sanity cap (100 m)") states the intended cap is 100 meters. 1 m = 100 cm, so 100 m = 10,000 cm — the constant should be `10_000`, not `100_000`. As written the cap permits gardens up to 1000 m (1 km) per side, 10x the documented/intended limit. Verified: the `gardens` table (`internal/store/migrations/0001_init.sql:41-46`) has `width_cm`/`height_cm` as plain `REAL NOT NULL` with no `CHECK` constraint, so this service-layer constant is the only enforcement point. The existing test (`gardens_test.go:51-52`, `WidthCM: maxGardenCM + 1` / `HeightCM: maxGardenCM * 10`) references the same (wrong) constant rather than an independently-derived value, so it can't catch this. - Impact: a garden 1 km on a side can be created; likely not exploitable, but defeats the stated purpose of the cap and will surprise anyone relying on the documented 100 m ceiling. - Fix: `maxGardenCM = 10_000`. Everything else in the draft's correctness lane checks out on inspection and is not disputed. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Both findings in the draft check out against the actual source. Here's the verified review: **VERDICT: Minor issues** - `internal/api/gardens.go:36-41` — `gardenCreateRequest.toInput()` and `gardenUpdateRequest.toInput()` have byte-for-byte identical bodies (map the same 5 fields into `service.GardenInput`). Verified by reading both methods directly under the struct definitions. Since `gardenUpdateRequest` already carries every field `gardenCreateRequest` has plus `Version`, embedding would drop the duplicate method and keep the two request shapes from drifting apart as fields are added later: ```go type gardenUpdateRequest struct { gardenCreateRequest Version int64 `json:"version" binding:"required"` } ``` Low-churn, and removes one of the two `toInput()` definitions entirely. - `internal/api/gardens.go:142-146` vs `internal/api/auth.go:216-237` — `writeResourceError`'s default branch (`slog.Error("api: unhandled service error", …)` + 500 `INTERNAL`) is a verbatim copy of `writeServiceError`'s default branch in `auth.go:234-237`. Confirmed by reading both switch statements directly. Minor and arguably intentional (the PR explicitly frames `writeResourceError` as a template future resources will copy), so this is a trivial nit rather than something to block on — a small shared `writeUnhandledError(c, err)` helper would remove the repeat if it recurs a third time. Everything else in this lens is clean: `store/gardens.go` mirrors `store/users.go`'s scanner/columns-const/error-wrapping conventions closely, `service/gardens.go` functions are short and single-purpose, naming (`gardenRole`, `requireGardenRole`, `effectiveGardenRole`) is clear and well-commented, and the test files reuse the existing `doJSON`/`authEngine`/`newTestService`/`mustRegister` helpers rather than re-implementing them. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> VERDICT: No material issues found Findings review under the performance lens: - `internal/store/gardens.go:20-33` (`CreateGarden`): does `INSERT` via `ExecContext`, then a second `GetGarden` round-trip to fetch the row, instead of an `INSERT ... RETURNING ...` single-statement fetch — which `UpdateGarden` (same file, line ~99) already uses and which the driver (`modernc.org/sqlite`, confirmed in `go.mod`) supports. This doubles DB calls on every garden create. Verified it mirrors an existing pattern (`internal/store/users.go:54-77` `CreateUser` does the identical Exec+`GetUserByID` follow-up), so it's consistent with the codebase's existing style rather than a regression introduced by this PR, and the cost is a single extra in-process SQLite query (no network hop) — negligible in absolute terms. Noting only because the PR's own framing is "conventions every later backend issue copies," and this leaves the store layer with two different conventions (RETURNING vs. Exec+refetch) for structurally identical inserts. Not blocking. - `internal/store/gardens.go:52-56` (`ListGardensForOwner`): unbounded `SELECT ... WHERE owner_id = ?` with no `LIMIT`/pagination. Verified `idx_gardens_owner` exists (`internal/store/migrations/0001_init.sql:53`), so the query itself is indexed and cheap; per-user garden counts are naturally small for this domain (a personal gardening app), so I don't think this rises to a material issue worth blocking on. No N+1 patterns, hot-loop work, or quadratic behavior found in the service/API layers — `requireGardenRole` and the CRUD handlers each do exactly one store call per request. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> The draft review provided contains no actual findings — it's just meta-commentary from the prior agent explaining that it wrote a review to a plan file and encountered a tooling limitation (`ExitPlanMode` unavailable). No specific claims, bugs, or line-level findings were included in the draft text for me to verify against the code. I attempted to read the referenced plan file (`/root/.claude/plans/pr-title-gardens-crud-zippy-cosmos.md`) to recover the actual findings, but I don't have permission to access it. Since there is nothing to verify and my role is strictly a verification gate (not a fresh review), the corrected output is: VERDICT: No material issues found </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** Reviewed through the security lens (authn/authz, injection, input validation, IDOR, secret leakage). Verified against the checked-out code: - **Authn**: All five garden routes are mounted under `v1.Group("/gardens", h.requireAuth())` (api.go:78). `requireAuth` (auth.go:103-123) resolves the session cookie via `ResolveSession`, rejects missing/tampered/expired tokens with 401, and stashes the user in context. `mustActor(c)` reads that stashed user — the actor identity is never taken from the request body. ✓ - **Authz / IDOR**: `actorID` for every service call comes from `mustActor(c).ID`, and `gardenID` comes from the validated path param. `requireGardenRole` (service/gardens.go) loads the row by ID and compares `g.OwnerID == actorID`; a non-owner gets `ErrNotFound` (existence masked), verified at service/gardens.go:64-69 and confirmed by the cross-user test. `UpdateGarden` requires `roleEditor`, `DeleteGarden` requires `roleOwner` — both satisfied only by the owner today. `ListGardens` is scoped to `ListGardensForOwner(actorID)` with no attacker-controllable owner filter. ✓ - **Injection**: All store queries use `?` placeholders for every user-supplied value (name, dims, unit, notes, id, version). `gardenColumns` is a package-level constant string concatenated into SELECT/RETURNING — not user input. ✓ - **Input validation**: `gardenFromInput` trims and rejects blank names, rejects non-positive and over-cap dimensions, and enforces `unit_pref ∈ {metric, imperial}`. `parseIDParam` rejects non-numeric and `< 1` ids. Update requires an explicit `version` (binding:"required"). ✓ - **Version-conflict leak**: On conflict, the store returns the fresh current row, and the handler only reaches `writeVersionConflict` after `requireGardenRole(roleEditor)` already authorized the actor — so the fresh row is only disclosed to an authorized editor/owner, not to a cross-user (who hits `ErrNotFound` first). ✓ - **CSRF**: The `v1` group runs `csrfGuard` (api.go:50), which blocks state-changing cross-origin requests when `PANSY_BASE_URL` is set. ✓ No secret/credential handling is touched by this change. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> ## VERDICT: Minor issues - **`internal/service/gardens.go:15` — `maxGardenCM` is off by 10× from the documented cap.** The field comments (`gardens.go:10-12`) state the sanity cap is **100 m**, and the store schema uses centimeters (`internal/store/migrations/0001_init.sql` uses `width_cm REAL` / "All measurements are centimeters"; confirmed in `internal/domain/domain.go` as well). 100 m = 10,000 cm, but `maxGardenCM = 100_000` cm = **1,000 m = 1 km**. The validation at `gardens.go:136` (`width > maxGardenCM || height > maxGardenCM`) therefore permits gardens up to 1 km on a side instead of the intended 100 m. Re-deriving from first principles: intended cap (100 m) × 100 cm/m = 10,000, not 100,000. Either the constant should be `10_000` (matching the doc) or the doc should say "1 km" — but a 1 km "garden" sanity cap is implausible, so the constant is the bug (an extra zero). Note `defaultGardenCM = 1000` cm = 10 m correctly matches its "10 m square" comment, which makes the 100 m → 100,000 cm error the more likely typo. Verified by reading `gardens.go:10-16` and the validation at `:136`; the test at `gardens_test.go:50-52` only checks `maxGardenCM+1` and `maxGardenCM*10` are rejected, so it passes either way and doesn't catch the discrepancy. Suggested fix: `maxGardenCM = 10_000` (or update the comment to "1000 m / 1 km" if 1 km was truly intended). </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **VERDICT: Minor issues** - **`internal/api/gardens.go:36-41` — two near-identical `toInput()` methods.** `gardenCreateRequest.toInput()` (line 36) and `gardenUpdateRequest.toInput()` (line 39) are byte-for-byte identical in body. The only structural difference between the two request structs is `Version int64` on the update variant (line 33). Per the surrounding convention this is exactly the leaky copy-paste the maintainability lens targets; a single embedded/shared struct or a free function would collapse it. Suggested fix: extract `func gardenInputFromRequest(name string, w, h float64, unit, notes string) service.GardenInput`, or have `gardenUpdateRequest` embed `gardenCreateRequest`. Confidence: high (verified by reading both methods). - **`internal/api/gardens.go:132-137,150-153` — the 409 envelope has two divergent shapes.** `writeVersionConflict` emits `{error, current}` (lines 133-135), while the `ErrVersionConflict` fallback branch in `writeResourceError` emits the flat `writeAPIError` shape with no `current` row (line 153). The comment at lines 151-152 justifies the fallback as a "caller forgot to special-case" guard, but the contract for a version-guarded 409 (declared at line 130 as "the contract for every version-guarded resource") is now split across two functions with silently different bodies — a future handler that routes a version conflict through `writeResourceError` instead of special-casing will produce a 409 missing the `current` row the client needs. Suggested fix: have `writeResourceError`'s `ErrVersionConflict` branch call into the same envelope path, or document that it must never be hit and panic/log distinctly. Confidence: high (verified both functions; the normal updateGarden path special-cases the conflict at line 95 and works correctly, so the divergence only affects the fallback). No dead code, naming, or compile problems found; `parseIDParam`, `writeResourceError`, the role-rank helpers, and the store's `scanner` reuse read cleanly and follow the existing `writeAPIError`/`writeServiceError` and `userColumns`/`scanUser` patterns. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> **VERDICT: Minor issues** - `internal/store/gardens.go:60-82` — `ListGardensForOwner` issues an unbounded `SELECT … WHERE owner_id = ? ORDER BY created_at DESC, id DESC` with no `LIMIT`. The `GET /api/v1/gardens` list endpoint (hot, cookie-authed, one row per garden) returns the owner's entire garden set every call, and `ListGardensForOwner` materializes the full `[]domain.Garden` in memory before returning. As an owner accumulates gardens (and later, in #16, shared gardens are unioned in), the result set and per-request allocation grow without bound. The `idx_gardens_owner` index keeps the filter cheap, but the result materialization is uncapped. I verified no query in the store package uses a `LIMIT` (grep for `LIMIT` returned no matches), so this is consistent with current conventions — but as the *list* convention every later resource copies (per the PR's stated goal of establishing service-layer conventions), it's worth adding a default cap now (e.g. `LIMIT 200` or cursor pagination) rather than retrofitting it across many resources later. Suggested fix: add a `LIMIT` (and ideally an `id` cursor / `before` param) to `ListGardensForOwner` and thread it through `ListGardens`. - `internal/store/gardens.go:15-24` + `:71-76` — minor, not material: `scanGarden` allocates a `*domain.Garden` on the heap, then `ListGardensForOwner` copies `*g` into the slice, so each row pays a pointer allocation plus a value copy. A per-row scan directly into a pre-grown slice element would avoid the intermediate pointer alloc. This is a micro-optimization and the list is currently small/capped only by the unbounded-query issue above, so I'd only bother if the pagination work is done. Not blocking. I verified both points by reading `internal/store/gardens.go`, `internal/service/gardens.go`, the migration schema (`idx_gardens_owner` exists, no `LIMIT` anywhere in the store package), and the API handler. No N+1, no blocking calls on hot paths, no quadratic behavior; the version-conflict path's extra `GetGarden` re-read is necessary to distinguish 404 vs 409 and is at most one additional indexed PK lookup, which is fine. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> ## VERDICT: Minor issues Reviewed the store/service/API error paths end-to-end against the actual source. The unhappy-path handling is mostly solid: sentinel errors propagate cleanly, `ErrNotFound` masks existence for unauthorized users, the version-conflict path correctly re-reads and returns the fresh row (`store/gardens.go:103-109`), `ListGardensForOwner` returns a non-nil slice, `parseIDParam` rejects `0`/negative/malformed ids, and the default branch of `writeResourceError` logs and returns 500. Two edge-case issues stand out: - **`internal/service/gardens.go:15` — `maxGardenCM = 100_000` contradicts the documented cap, making the boundary 10× too lax.** The comment (lines 10-12) states a "100 m sanity cap"; 100 m = 10,000 cm, but the constant is 100,000 cm = 1,000 m. The validation `width > maxGardenCM` (line 136) therefore lets a 999 m garden through while rejecting anything above 1 km — the "absurd/overflow" guard the comment claims is 10× weaker than intended. Confirmed by reading the constant and the check. Fix: `maxGardenCM = 10_000` (or correct the comment to "1000 m", but the documented intent is clearly 100 m). - **`internal/api/gardens.go:33` / `internal/service/gardens.go:104` — a negative `version` is accepted and surfaces as a 409 instead of a 400.** `gardenUpdateRequest.Version` is `int64` with `binding:"required"` (line 33), which only rejects the zero value; `{"version": -1}` passes binding and flows to `store.UpdateGarden`, whose `WHERE id=? AND version=-1` (store/gardens.go:99) never matches, so it re-reads and returns `(current, ErrVersionConflict)` (store/gardens.go:103-109). A malformed negative version is thus reported to the client as a real concurrency conflict rather than an input error. Confirmed by tracing through `parseIDParam` (which *does* reject negative ids at api/gardens.go:121) vs. the version path (which does not). Suggested fix: reject `version < 1` in the handler/service before calling the store. No swallowed errors or missing cleanup were found: `rows.Close()` is deferred in `ListGardensForOwner`, `LastInsertId`/`RowsAffected` errors are wrapped, and the default branch of `writeResourceError` logs and returns 500. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** * One potential hardening opportunity, not a current vulnerability: `gardenFromInput` in `internal/service/gardens.go:126` applies no upper bound to `Name` or `Notes` length. The DB column is unbounded `TEXT`, so an authenticated actor can post arbitrarily large strings, consuming memory/storage. A cap (e.g. 255 chars for name, a few KiB for notes) would close a self-DoS / resource-exhaustion vector and is worth establishing now since this PR sets conventions every later resource copies. * Verified: all store queries use `?` placeholders (`internal/store/gardens.go:31`, `42`, `54`, `85`, `116`) — no SQL injection path. * Verified: single-resource endpoints all funnel through `requireGardenRole` (`internal/service/gardens.go:71`) with the correct min role (`viewer` for GET, `editor` for PATCH, `owner` for DELETE). Cross-user access returns `ErrNotFound`, masking existence (`internal/service/gardens.go:78`). * Verified: `OwnerID` is set server-side in `CreateGarden` (`internal/service/gardens.go:97`) and never taken from user input — no mass-assignment of ownership. * Verified: `UpdateGarden` in the store uses `WHERE id = ? AND version = ?` (`internal/store/gardens.go:85`), and on conflict the current row is only returned to callers that already passed `requireGardenRole` with `roleEditor`, so no information leak to unauthorized users. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> **Minor issues** - **`internal/service/gardens.go:15` — `maxGardenCM` is 1,000 m, not the documented 100 m** The constant `maxGardenCM = 100_000` corresponds to 1,000 m (1 km), while the preceding comment explicitly calls it a “generous sanity cap (100 m)”. A 100 m cap should be `10_000` cm. Because of the extra zero, gardens up to 1 km are accepted, which is well beyond the intended limit and could cause downstream rendering or numeric issues. **Fix:** change `100_000` → `10_000`. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed ollama-cloud/kimi-k2.6:cloud: model returned an empty response </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found The diff is clean from a performance perspective: - **No N+1 queries**: each handler calls the service once, and each service method issues a single store query (two on the version-conflict path, which is the exceptional case by design). - **No unbounded hot-path growth**: `ListGardensForOwner` filters to a single `owner_id`, so result sets are naturally bounded by how many gardens one user owns; the app domain makes multi-thousand-garden users implausible. - **Efficient update path**: `UpdateGarden` uses `UPDATE … RETURNING`, so the happy path is a single round-trip. - **No missing critical index**: `idx_gardens_owner` already exists on `gardens(owner_id)`. - **No allocation hot loops**: `strings.TrimSpace` in `gardenFromInput` runs once per request, not in a loop. The extra `GetGarden` call when a version conflict is detected (`store/gardens.go:111`) is intentional and only fires on the 409 path; removing it would break the API contract. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **No material issues found** The diff is clean from an error-handling and edge-cases perspective. - **Nil/empty collections**: `ListGardensForOwner` initializes `gardens := []domain.Garden{}`, guaranteeing a non-nil empty slice. Verified by reading `internal/store/gardens.go:70`. - **Zero/negative dimensions**: `gardenFromInput` correctly defaults `0` on create, rejects `0` on update, and rejects negatives in both paths. Verified by reading `internal/service/gardens.go:128-138`. - **Version-conflict edge case**: The store's `UpdateGarden` handles `sql.ErrNoRows` by re-reading to distinguish "gone" (`ErrNotFound`) from "stale version" (`ErrVersionConflict` + current row). Verified by reading `internal/store/gardens.go:93-115`. - **Cross-user access masking**: `requireGardenRole` returns `ErrNotFound` (not `ErrForbidden`) when the actor has no role, correctly masking existence. Verified by reading `internal/service/gardens.go:47-59`. - **Missing cleanup**: `ListGardensForOwner` defers `rows.Close()`. Verified by reading `internal/store/gardens.go:68`. - **Unhandled errors**: No swallowed errors. All store and service errors are propagated up to handlers and mapped to HTTP responses. </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> — Minor issues</summary> I'll verify each finding by reading the actual code. Now let me verify the claims about body limits and the migration columns. Both findings are confirmed against the actual code: 1. **Unbounded Name/Notes**: `gardenFromInput` (line 119-154) only `TrimSpace`s and checks for empty — no length cap. The request structs (lines 19-34) use only `binding:"required"` on Name, no `max=` tag, and Notes has no binding tag at all. Migration uses unbounded `TEXT` (lines 44, 48). Grep confirms no `MaxBytesReader` or global body limit anywhere in the repo. **Survives.** 2. **Subnormal-positive dimensions**: Line 136 check is `width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM`. A value like `1e-300` passes all four conditions. **Survives.** ## Review **VERDICT: Minor issues** Through the security lens, the authorization design is sound: - All garden routes sit behind `requireAuth` (`internal/api/api.go:78`), and the actor ID is sourced from the resolved session via `mustActor(c).ID` (`internal/api/gardens.go:46,60,73,91,110`) — never from the request body, so owner spoofing on create is impossible (`internal/service/gardens.go:78` sets `g.OwnerID = actorID`). - Every read/mutate funnels through `requireGardenRole` (`internal/service/gardens.go:47`), and a user with no role gets `ErrNotFound` rather than `ErrForbidden` — existence is masked (verified the cross-user tests in `internal/service/gardens_test.go:158` and `internal/api/gardens_test.go:133`). - All SQL in `internal/store/gardens.go` uses `?` placeholders (no string interpolation of user input), so no injection. `parseIDParam` (`internal/api/gardens.go:119`) parses to int64 and rejects `< 1`. - The version-guarded `UPDATE … WHERE id=? AND version=? RETURNING …` (`internal/store/gardens.go:95-101`) prevents lost updates; the fresh row returned on 409 only goes to an already-authorized editor, so no cross-user leak. - The default branch of `writeResourceError` logs `err` server-side but returns a generic "internal error" to the client (`internal/api/gardens.go:155-156`) — no internal-detail leakage. - `csrfGuard` is applied to the whole `v1` group (`internal/api/api.go:50`), covering POST/PATCH/DELETE on `/gardens` when `PANSY_BASE_URL` is set. Findings: - **`internal/service/gardens.go:119-154` — no length bound on `Name`/`Notes` from untrusted input.** `gardenFromInput` only `TrimSpace`s and rejects empty; it never caps the length of `Name` or `Notes`. `gardenCreateRequest`/`gardenUpdateRequest` use only `binding:"required"` with no `max=` tag. I grepped the repo for any `MaxBytesReader`/global body limit and found none (the only `max=` binding in the codebase is on `auth.go` passwords), so an authenticated user can POST a garden with a multi-megabyte `name`/`notes` (or repeatedly PATCH to grow `notes`) with no server-side bound. The store column is unbounded `TEXT` (`internal/store/migrations/0001_init.sql:44,48`). This is a memory/storage-DoS vector available to any registered user. Suggested fix: add `max=` binding tags (e.g. `max=200` for name, `max=10000` for notes) and/or an explicit length check in `gardenFromInput` returning `ErrInvalidInput`, plus consider a global `http.MaxBytesReader` middleware. Verified by reading `gardenFromInput`, the request structs, and grepping for body-limit/length enforcement. - **`internal/service/gardens.go:136` — subnormal-positive dimensions slip past validation.** The guard is `width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM`. A JSON number like `1e-300` (or any tiny positive value) satisfies `> 0` and `<= maxGardenCM`, so it is accepted and stored as a valid garden dimension. This is a data-integrity gap from untrusted input rather than a direct exploit (and `NaN`/`Inf` are not representable as JSON numbers, so they cannot be smuggled in via `ShouldBindJSON`). Low impact, but worth tightening to a positive minimum (e.g. reject `width < 1` cm) for defense in depth. Verified by reading the comparison and the float64 field types. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> VERDICT: Minor issues - `internal/service/gardens.go:15` — `maxGardenCM = 100_000` is 10× too large. The comment at lines 10–12 explicitly states the cap is "100 m", and 100 m = 10,000 cm (1 m = 100 cm). `100_000` cm is actually 1,000 m (1 km). The schema (`internal/store/migrations/0001_init.sql:45-46`) has no CHECK constraint on `width_cm`/`height_cm`, so this constant is the sole guard against absurd dimensions — and it permits a 1 km × 1 km garden, exactly the "absurd" case the comment says it prevents. Fix: `maxGardenCM = 10_000` (100 m). Verified against the source and the migration; existing tests use `maxGardenCM + 1` and `maxGardenCM * 10` so they stay valid after the fix. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **VERDICT: Minor issues** - **`internal/service/gardens.go:11-16` — comment contradicts the constant.** The comment says "the max is a generous sanity cap (100 m)" but `maxGardenCM = 100_000` cm = **1000 m** (100 m would be `10_000` cm). The test uses `WidthCM: maxGardenCM + 1`, so it validates against the constant rather than the documented bound and won't catch the discrepancy. Either the comment is wrong or the constant is; either way a reader can't trust the stated cap. Verified by reading the file and re-deriving the unit conversion. Suggested fix: decide which is intended; if 100 m, set `maxGardenCM = 10_000`. - **`internal/api/gardens.go:19-41` — duplicated request structs and identical `toInput()` methods.** `gardenCreateRequest` and `gardenUpdateRequest` share every field except `Version`, and their `toInput()` bodies are literally identical (lines 36-41). Since the PR description frames this as the convention-setter later backends will copy, the duplication will propagate to every resource's create/update handlers. Suggested fix: embed a shared `gardenFields` struct (or have one struct with `Version *int64`) with a single `toInput()`. Verified by reading the file. - **`internal/api/gardens.go:148-156` vs `internal/api/auth.go:232-236` — duplicated error-mapping tail.** `writeResourceError` and `writeServiceError` both map `ErrInvalidInput → 400 INVALID_INPUT "invalid input"` and share an identical default branch, including the verbatim `slog.Error("api: unhandled service error", "error", err)` line. The auth-specific sentinels genuinely belong only in `writeServiceError`, so the split is fine — but the shared tail (InvalidInput + default slog+500) could be factored into a common helper now that there are two mappers in the package. Verified by reading both. Minor. - **`internal/service/gardens.go:119` — `applyDefaults bool` flag argument.** `gardenFromInput(in, true)` / `gardenFromInput(in, false)` is a classic flag argument that changes the function's contract (defaults vs. reject-zeros) at the call site; readers have to jump to the body to know which. Since this is a convention-setter, consider two named functions (`normalizeCreateInput` / `normalizeUpdateInput`) sharing a `validate` helper. Minor / stylistic. No dead code, no naming problems, no broken patterns in the store layer (it mirrors `users.go`'s `scanner`/`xxxColumns`/`scanXxx` convention correctly), and the route registration follows the existing `auth` group style. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> VERDICT: Minor issues - `internal/store/gardens.go:60` — `ListGardensForOwner` has no `LIMIT`/pagination. It loads every garden a user owns into memory in one query (`SELECT … WHERE owner_id = ? ORDER BY created_at DESC, id DESC`). Today the per-user count is small, but this is the canonical "list the actor's gardens" endpoint and the shape #16 will extend (sharing adds rows the query must also return). As written, an account with thousands of gardens builds a slice of thousands of `domain.Garden` values on every `GET /api/v1/gardens`. Worth at least a defensive `LIMIT` (e.g. 1000) now, or committing to pagination before the list grows. Verified by reading the migration: `idx_gardens_owner` covers the `owner_id` filter, so the query itself is index-backed — the concern is only the unbounded result materialization, not a missing index. - `internal/store/gardens.go:28` — `CreateGarden` does an `INSERT` then a second `GetGarden` round-trip to return the stored row (insert id → SELECT by id). SQLite supports `INSERT … RETURNING`, so this could be one statement. The same pattern is already used in `users.go:CreateUser`, so it's consistent with house style and not on a hot path; mentioning only because it doubles the round-trips per create. Low impact, unverified whether any caller cares about the cost. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> Looking through the error-handling lens. Let me verify the validation bounds and the conflict-path nil handling. Looking at the error-handling/edge-case lens. Let me check the validation boundary case for `maxGardenCM + 1` with float64, and the `UpdateGarden` conflict-path error wrapping. Now I have everything I need to assess the error-handling lens. Let me review the findings I want to confirm: **Key edge cases to check:** 1. **`updateGarden` handler `ErrVersionConflict` path with `g==nil`**: At `api/gardens.go:95-96`, when `UpdateGarden` returns `ErrVersionConflict`, the service contract says it returns `(current, ErrVersionConflict)` — so `g` is non-nil. But what about the other sentinel errors? On `ErrInvalidInput` (e.g., negative dims detected in `gardenFromInput`), `UpdateGarden` returns `(nil, ErrInvalidInput)`. The handler checks `errors.Is(err, domain.ErrVersionConflict)` first → false, so falls through to `writeResourceError` → maps to 400. Safe. Good. 2. **`store.UpdateGarden` race**: After `sql.ErrNoRows` it calls `d.GetGarden` to distinguish gone vs stale. There's a TOCTOU window but that's not really an error-handling bug. 3. **`CreateGarden` error from `LastInsertId`**: SQLite via modernc supports it. But if `LastInsertId` returns an error (rare), `GetGarden` is skipped — error is propagated. Fine. 4. **`ListGardensForOwner` scan error during iteration**: Returns immediately on scan error, leaving `rows` to be closed by `defer`. Good. 5. **Negative dimensions on update with `applyDefaults=false`**: `width <= 0` catches negatives. Good. Test case covers negative width on create. 6. **`width > maxGardenCM` with float**: `maxGardenCM = 100_000`. `maxGardenCM + 1` in test = 100001. Float comparison fine. 7. **Empty body / non-JSON POST to createGarden**: `c.ShouldBindJSON` returns an error → 400. Good. 8. **`writeVersionConflict(c, nil)`**: Could `g` be nil here? Service `UpdateGarden` returns `(nil, err)` only when `requireGardenRole` fails or `gardenFromInput` fails — neither returns `ErrVersionConflict`. The store returns `(current, ErrVersionConflict)` where `current` is non-nil (GetGarden succeeded). So `g` is non-nil in the conflict path. Safe. Let me double-check one subtle issue: in `store.UpdateGarden`, when the re-read's `GetGarden` returns a real (non-ErrNotFound) error, `UpdateGarden` returns `(nil, gerr)` with `gerr` wrapped — meaning the conflict envelope isn't written, which is correct behavior. Let me check whether the conflict-envelope's "current" could include stale data — no, it's freshly re-read. Let me check `writeAPIError` for completeness: ## Verdict: No material issues found Through the error-handling & edge-case lens, I verified the unhappy paths introduced by this diff: - **Conflict-path nil safety** (`api/gardens.go:91-101`): I checked the contract carefully. `service.UpdateGarden` only returns `(non-nil, ErrVersionConflict)` — the store's re-read returns `(current, ErrVersionConflict)` with `current` non-nil (it only reaches that branch when `GetGarden` succeeded), and the service passes the store's pair through unchanged. The other error branches (`requireGardenRole`, `gardenFromInput`) return `(nil, non-conflict-err)`, so they correctly skip the `writeVersionConflict(c, g)` branch and fall to `writeResourceError`. No nil-deref on the conflict envelope. Verified by reading `service/gardens.go:95-106` and `store/gardens.go:103-114`. - **Validation boundaries** (`service/gardens.go:119-155`): `width <= 0 || height <= 0` correctly catches both zero (the update path where defaults are *not* applied) and negatives on either path; `> maxGardenCM` is a clean float compare. Name trimmed-then-empty check, unit whitelist, and the asymmetric defaulting (`applyDefaults` only on create) are all handled. No NaN-specific guard, but `encoding/json` doesn't produce NaN from valid JSON and the DB stores REAL — out of scope for this diff. - **Store error propagation** (`store/gardens.go`): `ListGardensForOwner` checks both `rows.Next()` scan errors and `rows.Err()` after the loop; `rows.Close()` is deferred. `DeleteGarden` distinguishes 0-rows-affected → `ErrNotFound`. `UpdateGarden`'s `sql.ErrNoRows` branch re-reads and maps gone-vs-stale correctly; a real error from the re-read is returned unwrapped-as-is (`gerr`, which is itself wrapped in `GetGarden`) rather than masked. - **`parseIDParam`** (`api/gardens.go:119-126`): `id < 1` rejects zero/negative; `ParseInt` rejects non-numeric and overflow → 400. Good. - **Auth gating**: every handler resolves the actor via `mustActor(c)` after `requireAuth`; an unauthenticated request is aborted before reaching the handler, so `mustActor` can't panic on these routes in the request path. - **Empty-collection edge case**: `ListGardensForOwner` initializes `gardens := []domain.Garden{}`, so an owner with no gardens returns a non-nil empty slice — matching the `[]` test assertion and avoiding a null JSON response. Verified at `store/gardens.go:70`. Nothing in the error-handling/edge-case lane is materially wrong. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **Verdict: No material issues found** I verified the security-sensitive paths introduced by this PR and found no exploitable gaps. **Checks performed** - **Authn/authz**: All garden routes (`GET /gardens`, `POST /gardens`, `GET /gardens/:id`, `PATCH /gardens/:id`, `DELETE /gardens/:id`) are registered behind `h.requireAuth()` (verified in `internal/api/api.go:78`). `mustActor` pulls the authenticated user from the gin context set by `requireAuth`; there is no handler-level bypass. - **Authorization logic**: `requireGardenRole` in `internal/service/gardens.go:47` is the single gate. It calls `GetGarden`, then checks ownership via `effectiveGardenRole`. A user with no role gets `ErrNotFound` (existence masked, 404), not `ErrForbidden`. Cross-user IDOR is mitigated: I traced `TestGardenCrossUserIsNotFound` in the API tests and `TestCrossUserAccessIsNotFound` in the service tests to confirm Bob cannot learn Alice’s garden exists. - **SQL injection**: Every store method (`internal/store/gardens.go:28–132`) uses `?` placeholders. The only string concatenation is `gardenColumns`, a hardcoded constant. No tainted input reaches the query text. - **Input validation**: `parseIDParam` rejects non-positive `int64`s. `gardenFromInput` enforces name presence, trims whitespace, validates dimensions (positive, capped at 100 m), and whitelists `unitPref` to `metric|imperial`. Zero dimensions default on create and are rejected on update. No injection vectors via untrusted dimensions or names. - **CSRF**: State-changing methods (POST, PATCH, DELETE) on `/gardens` fall under the existing `csrfGuard` middleware (`internal/api/api.go:50`), which checks the `Origin` header when `PANSY_BASE_URL` is configured. `SameSite=Lax` on the session cookie provides defense in depth. - **Version-conflict TOCTOU**: `UpdateGarden` uses `WHERE id=? AND version=? RETURNING …`. On `sql.ErrNoRows` it re-reads to distinguish “gone” from “stale.” The fresh row is only returned to a caller who already passed `requireGardenRole` with at least `roleEditor`, so no information leakage occurs. - **Deserialization / SSRF / secrets**: None introduced. - **Unsafe defaults**: Session cookie settings (`HttpOnly`, `SameSite=Lax`, `Secure` conditional on HTTPS) are pre-existing and unchanged. One might note that `Notes` and `Name` lack an upper-length bound, which is technically missing input validation. For a local SQLite garden app this amounts to a trivial DoS nit rather than a material vulnerability, so I am leaving it out. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> **Blocking issues found** - **`internal/service/gardens.go:15`** — `maxGardenCM` is `100_000` cm (1 km), but the comment and PR description both state a **100 m** sanity cap. 100 m = 10,000 cm, so the constant is off by a factor of 10. This means the validation happily accepts gardens up to 1 km in size, which violates the documented contract and defeats the purpose of the sanity cap. **Fix:** Change `maxGardenCM = 100_000` to `maxGardenCM = 10_000`. Verified by reading the source file and independently re-deriving the conversion (1 m = 100 cm ⇒ 100 m = 10,000 cm). </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - **`internal/api/gardens.go:19-41`** — `gardenCreateRequest` and `gardenUpdateRequest` duplicate five identical fields and their `toInput()` methods are byte-for-byte copies. This is unnecessary duplication within the same file; adding a field in the future requires touching four places. Use a shared base struct (e.g., `gardenFields`) and embed it, or make `toInput()` a method on the base so the translation lives in one place. - **`internal/api/gardens.go:119-126`** — `parseIDParam` is a generic path-parameter utility (any `:id` route needs it), but it is defined in the gardens handler file. Future resource endpoints will either duplicate it or import it awkwardly from a feature-specific file. Move it to `api.go` (or a shared API util file) so every handler can reach it without cross-importing gardens code. - **`internal/api/gardens.go:139-157`** — `writeResourceError` is described as "shared by every resource," yet it lives in `gardens.go`. If it is truly meant to be shared by objects, plants, and every later resource, it should sit next to `writeAPIError` in a central API file (e.g., `api.go` or `spa.go`) so later issues don't leave it stranded or duplicate it. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary> **Blocking issues found** - **`internal/service/gardens.go:136` — NaN bypasses dimension validation** The check `width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM` uses ordered comparisons that are **both false for NaN**, so a client sending `{"widthCm":0,"heightCm":NaN}` (or any NaN combination) passes validation on create (after defaulting the non-NaN field) and on update. NaN is then stored in the SQLite `REAL` column. *Fix:* add an explicit NaN guard with `math.IsNaN` before the bounds check. - **`internal/api/gardens.go:33` / `internal/service/gardens.go:95` — negative version yields misleading 409** `gardenUpdateRequest.Version` only carries `binding:"required"`, which for an `int64` rejects zero but **allows negatives**. A negative version reaches the store, fails the `WHERE version = ?` guard, and triggers the re-read path, so the handler returns **409 VERSION_CONFLICT** instead of 400. The client cannot retry from a negative version. *Fix:* validate `version >= 1` in `parseIDParam` style (or in the service layer) and return `ErrInvalidInput`. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-18 22:39:29 +00:00
Address Gadfly review on #7: garden cap, dim validation, shared errors
Build image / build-and-push (push) Successful in 5s
3fbefa4e05
Fixes from the PR #26 adversarial review (graded 18 real / 0 false positive).

Correctness / security
- maxGardenCM fixed to 10_000 (100 m), matching its comment — it was
  100_000 cm (1 km), 10x too lax (5 models flagged this).
- Dimension validation now rejects NaN/Inf (which slip past naive
  comparisons) and subnormal-tiny positives, via a finite [1cm, 100m]
  check. Name (200) and notes (10_000) are length-capped so untrusted
  input can't balloon storage.
- Update version binding is `required,min=1`, so a negative/zero version
  is a 400, not a 409.

Maintainability / performance
- One unified writeServiceError (new errors.go) maps every auth + resource
  sentinel; writeResourceError removed. writeVersionConflict and
  parseIDParam moved to errors.go (shared, not in the gardens feature file).
- Request structs share an embedded gardenFields (one toInput).
- CreateGarden uses INSERT ... RETURNING (one round-trip).
- ListGardensForOwner has a defensive LIMIT (pagination is post-v1).

Tests: name/notes length, NaN/Inf/subnormal dims, dimension-at-cap valid,
negative/zero version -> 400.

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