Plant catalog backend: CRUD + seeded built-ins (#12) #31

Merged
steve merged 2 commits from phase-4-plants-api into main 2026-07-19 01:48:23 +00:00
Owner

Closes #12. Part of epic #20 (Phase 4 — Plant catalog).

What

The plants store previously had only the /full read side (ListReferencedPlants). This adds the full catalog: seeded built-ins, per-user custom plants, and CRUD with the standard version-guard + visibility rules.

Migration

  • 0003_seed_plants.sql — 32 built-in plants (owner_id NULL, read-only), covering common garden staples with spacing / display color / emoji. Seeded once via the version-tracked migration runner, so re-running Migrate never duplicates them.

Store (internal/store/plants.go)

  • ListPlantsForActor (built-ins + actor's own, built-ins first then by name), GetPlant, CreatePlant, UpdatePlant (version-guarded), DeletePlant, CountPlantingsForPlant.
  • isForeignKeyViolation helper as a backstop: a concurrent planting insert that slips past the service's count check still surfaces as ErrPlantInUse rather than a 500.

Service (internal/service/plants.go)

  • Built-ins (owner_id nil) are visible to all but read-onlyErrForbidden.
  • Another user's custom plants are invisibleErrNotFound (existence masked, same as gardens/objects).
  • Deleting a plant referenced by any planting (even a removed one — the FK is RESTRICT) → ErrPlantInUse (409).
  • Validation mirrors the schema: name non-empty, category in enum, spacingCm > 0, hex color, non-empty icon, optional daysToMaturity ≥ 1.
  • "Clone a built-in" is client-side: POST a copy, which is then owned/editable.

API

  • GET,POST /api/v1/plants and PATCH,DELETE /api/v1/plants/:id (version guard + 409 envelope). New PLANT_IN_USE error code.

Tests

  • Service: seeded-catalog visibility, cross-user invisibility, built-in immutability, clone-then-edit, clear nullable days, version conflict, validation table, delete-in-use blocked (+ freed after clearing).
  • API: catalog flow, built-in 403, cross-user 404, auth required.
  • Reworked the two migration-count store tests to derive their expected value from the embedded files so future migrations don't break them.

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

🤖 Generated with Claude Code

Closes #12. Part of epic #20 (Phase 4 — Plant catalog). ## What The plants store previously had only the `/full` read side (`ListReferencedPlants`). This adds the full catalog: seeded built-ins, per-user custom plants, and CRUD with the standard version-guard + visibility rules. ### Migration - `0003_seed_plants.sql` — 32 built-in plants (`owner_id NULL`, read-only), covering common garden staples with spacing / display color / emoji. Seeded once via the version-tracked migration runner, so re-running `Migrate` never duplicates them. ### Store (`internal/store/plants.go`) - `ListPlantsForActor` (built-ins + actor's own, built-ins first then by name), `GetPlant`, `CreatePlant`, `UpdatePlant` (version-guarded), `DeletePlant`, `CountPlantingsForPlant`. - `isForeignKeyViolation` helper as a backstop: a concurrent planting insert that slips past the service's count check still surfaces as `ErrPlantInUse` rather than a 500. ### Service (`internal/service/plants.go`) - Built-ins (`owner_id` nil) are visible to all but **read-only** → `ErrForbidden`. - Another user's custom plants are **invisible** → `ErrNotFound` (existence masked, same as gardens/objects). - Deleting a plant referenced by any planting (even a removed one — the FK is `RESTRICT`) → `ErrPlantInUse` (409). - Validation mirrors the schema: name non-empty, category in enum, `spacingCm > 0`, hex color, non-empty icon, optional `daysToMaturity ≥ 1`. - "Clone a built-in" is client-side: POST a copy, which is then owned/editable. ### API - `GET,POST /api/v1/plants` and `PATCH,DELETE /api/v1/plants/:id` (version guard + 409 envelope). New `PLANT_IN_USE` error code. ### Tests - Service: seeded-catalog visibility, cross-user invisibility, built-in immutability, clone-then-edit, clear nullable days, version conflict, validation table, delete-in-use blocked (+ freed after clearing). - API: catalog flow, built-in 403, cross-user 404, auth required. - Reworked the two migration-count store tests to derive their expected value from the embedded files so future migrations don't break them. `GOWORK=off go build ./... && go test ./internal/...` green. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-19 01:33:36 +00:00
Add plant catalog backend: CRUD + seeded built-ins (#12)
Build image / build-and-push (push) Successful in 5s
Gadfly review (reusable) / review (pull_request) Successful in 10m32s
Adversarial Review (Gadfly) / review (pull_request) Successful in 10m32s
78dbadf95c
Extends the plants store (which had only the /full read side) with full
catalog CRUD, adds a service layer with visibility/immutability rules, REST
endpoints, and a seed migration of ~32 built-ins.

- 0003_seed_plants.sql: 32 built-in plants (owner_id NULL, read-only), seeded
  once by the version-tracked migration runner.
- store: ListPlantsForActor (built-ins + own), Get/Create/Update/Delete +
  CountPlantingsForPlant; isForeignKeyViolation backstop for the RESTRICT FK.
- service: built-ins are read-only (ErrForbidden); another user's plants are
  invisible (ErrNotFound); delete of a referenced plant → ErrPlantInUse (409);
  validation mirrors the schema (category enum, spacing>0, hex color, icon).
- api: GET,POST /plants and PATCH,DELETE /plants/:id with the version guard.
- Made the migration-count store tests derive their expectation from the files
  so future migrations don't break them.

Service + API tests cover visibility, built-in immutability, validation,
version conflict, and delete-in-use.

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

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-19 01:44:10Z

claude-code/sonnet · claude-code — done

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

glm-5.2:cloud · ollama-cloud — done

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

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

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

opencode/glm-5.2:cloud · opencode — done

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

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

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

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

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

🪰 Gadfly consensus review — 4 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** — 4 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +60,4 @@
// parseNullableInt decodes a JSON number|null field into (value, present).
// Absent → (nil, false); null → (nil, true); 42 → (&42, true). A non-integer
// value is an error.
func parseNullableInt(raw json.RawMessage) (value *int, present bool, err error) {

🟡 parseNullableInt duplicates parseNullableString (objects.go) almost verbatim; should be a shared generic helper

maintainability · flagged by 1 model

  • internal/api/plants.go:63-69 (parseNullableInt) is a byte-for-byte structural duplicate of internal/api/objects.go:87-95 (parseNullableString) — same "absent/null/value" decode logic, differing only by pointer type (*int vs *string). There's now a third near-twin, parseNullableJSON (internal/api/objects.go:100-109), which returns the raw JSON text instead of unmarshaling. Verified by reading both files directly: the module targets Go 1.26 (go.mod), so this is exactly the kind…

🪰 Gadfly · advisory

🟡 **parseNullableInt duplicates parseNullableString (objects.go) almost verbatim; should be a shared generic helper** _maintainability · flagged by 1 model_ - `internal/api/plants.go:63-69` (`parseNullableInt`) is a byte-for-byte structural duplicate of `internal/api/objects.go:87-95` (`parseNullableString`) — same "absent/null/value" decode logic, differing only by pointer type (`*int` vs `*string`). There's now a third near-twin, `parseNullableJSON` (`internal/api/objects.go:100-109`), which returns the raw JSON text instead of unmarshaling. Verified by reading both files directly: the module targets Go 1.26 (`go.mod`), so this is exactly the kind… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +9,4 @@
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
)
func intPtr(n int) *int { return &n }

🟡 Duplicate pointer helper intPtr should be consolidated with existing ptrBool/strPtr in same package

maintainability · flagged by 2 models

  • internal/service/plants_test.go:12intPtr duplicates the pointer-helper pattern already present in internal/service/objects_test.go (ptrBool, strPtr). Three tiny one-liners are now scattered across test files in the same package with inconsistent naming (ptrBool vs strPtr vs intPtr). This makes the helpers hard to discover and invites further duplication. Fix: consolidate them into a shared helpers_test.go in internal/service/ with a consistent scheme (e.g. `ptrBo…

🪰 Gadfly · advisory

🟡 **Duplicate pointer helper intPtr should be consolidated with existing ptrBool/strPtr in same package** _maintainability · flagged by 2 models_ - **`internal/service/plants_test.go:12`** — `intPtr` duplicates the pointer-helper pattern already present in `internal/service/objects_test.go` (`ptrBool`, `strPtr`). Three tiny one-liners are now scattered across test files in the same package with inconsistent naming (`ptrBool` vs `strPtr` vs `intPtr`). This makes the helpers hard to discover and invites further duplication. **Fix:** consolidate them into a shared `helpers_test.go` in `internal/service/` with a consistent scheme (e.g. `ptrBo… <sub>🪰 Gadfly · advisory</sub>
@@ -56,0 +59,4 @@
// maxPlantsListed caps ListPlantsForActor as a defensive backstop. The seeded
// built-ins (~30) plus a user's own catalog stay tiny at household scale, so this
// is never hit; pagination is post-v1 if ever needed.
const maxPlantsListed = 5000

ListPlantsForActor LIMIT 5000 silently truncates with no error if exceeded

error-handling · flagged by 1 model

  • internal/store/plants.go:170 (DeletePlant) — the isForeignKeyViolation(err) backstop relies on substring-matching the SQLite driver's error text ("FOREIGN KEY constraint failed", per internal/store/users.go:184-185). If the driver ever changes that wording, the concurrent-insert race between CountPlantingsForPlant and the DELETE would surface as a raw 500 INTERNAL instead of the intended 409 PLANT_IN_USE. This mirrors the pre-existing isUniqueViolation pattern, so it's reus…

🪰 Gadfly · advisory

⚪ **ListPlantsForActor LIMIT 5000 silently truncates with no error if exceeded** _error-handling · flagged by 1 model_ - `internal/store/plants.go:170` (`DeletePlant`) — the `isForeignKeyViolation(err)` backstop relies on substring-matching the SQLite driver's error text (`"FOREIGN KEY constraint failed"`, per `internal/store/users.go:184-185`). If the driver ever changes that wording, the concurrent-insert race between `CountPlantingsForPlant` and the `DELETE` would surface as a raw `500 INTERNAL` instead of the intended `409 PLANT_IN_USE`. This mirrors the pre-existing `isUniqueViolation` pattern, so it's reus… <sub>🪰 Gadfly · advisory</sub>
@@ -56,0 +167,4 @@
// DeletePlant removes a plant. Returns domain.ErrNotFound if no row was deleted,
// or domain.ErrPlantInUse if the ON DELETE RESTRICT foreign key from plantings
// fires (a backstop for the service's own count check under a concurrent insert).
func (d *DB) DeletePlant(ctx context.Context, id int64) error {

🟡 FK-violation backstop uses fragile string-match on driver error; race could surface as 500 instead of 409

error-handling · flagged by 1 model

  • internal/store/plants.go:170 (DeletePlant) — the isForeignKeyViolation(err) backstop relies on substring-matching the SQLite driver's error text ("FOREIGN KEY constraint failed", per internal/store/users.go:184-185). If the driver ever changes that wording, the concurrent-insert race between CountPlantingsForPlant and the DELETE would surface as a raw 500 INTERNAL instead of the intended 409 PLANT_IN_USE. This mirrors the pre-existing isUniqueViolation pattern, so it's reus…

🪰 Gadfly · advisory

🟡 **FK-violation backstop uses fragile string-match on driver error; race could surface as 500 instead of 409** _error-handling · flagged by 1 model_ - `internal/store/plants.go:170` (`DeletePlant`) — the `isForeignKeyViolation(err)` backstop relies on substring-matching the SQLite driver's error text (`"FOREIGN KEY constraint failed"`, per `internal/store/users.go:184-185`). If the driver ever changes that wording, the concurrent-insert race between `CountPlantingsForPlant` and the `DELETE` would surface as a raw `500 INTERNAL` instead of the intended `409 PLANT_IN_USE`. This mirrors the pre-existing `isUniqueViolation` pattern, so it's reus… <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

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

Finding Where Models Lens
🟡 Duplicate pointer helper intPtr should be consolidated with existing ptrBool/strPtr in same package internal/service/plants_test.go:12 2/5 maintainability
3 single-model findings (lower confidence)
Finding Where Model Lens
🟡 parseNullableInt duplicates parseNullableString (objects.go) almost verbatim; should be a shared generic helper internal/api/plants.go:63 claude-code/sonnet maintainability
🟡 FK-violation backstop uses fragile string-match on driver error; race could surface as 500 instead of 409 internal/store/plants.go:170 glm-5.2:cloud error-handling
ListPlantsForActor LIMIT 5000 silently truncates with no error if exceeded internal/store/plants.go:62 glm-5.2:cloud error-handling
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

I've written the plan to /root/.claude/plans/pr-title-plant-catalog-refactored-flurry.md. Summary:

Task: Security-lens review of PR #12 (plant catalog CRUD). Phase-1 exploration already confirmed the new code follows this repo's existing auth/parameterization/error-handling conventions (auth-gated routes, writablePlant mirrors requireGardenRole/objectForRole, all SQL uses bind params, no error-text leakage). Five specific verification checks remain (nullable-int parsing, ownership-tamper surface in patch/create structs, delete-race semantics, migration literal-safety, test-file secret hygiene) before I finalize the verdict — expected to land on "no material issues found."

Once approved, I'll close out those checks and report findings via ReportFindings.

🎯 Correctness — No material issues found

I've completed manual verification by reading the schema, domain struct, SQL statements, and validation logic directly — no need to run the build. Ready to report findings.

VERDICT: No material issues found

I reviewed this PR strictly for logic bugs and semantic/domain correctness — magic numbers, SQL parameter ordering, version-guard semantics, visibility rules, and the seeded catalog data.

Checks performed (all verified by reading the actual checked-out code, not just the diff):

  • internal/store/plants.goplantColumns/scanPlant column list matches the plants table in 0001_init.sql exactly (id, owner_id, name, category, spacing_cm, color, icon, days_to_maturity, notes, version, created_at, updated_at).
  • CreatePlant/UpdatePlant SQL: placeholder count and parameter order line up 1:1 with the column list in both the INSERT and UPDATE ... SET statements.
  • ListPlantsForActor's ORDER BY owner_id IS NOT NULL, name COLLATE NOCASE, id correctly sorts built-ins (owner_id IS NULL → boolean 0) before owned rows (→ 1), matching the "built-ins first" claim.
  • UpdatePlant's version-guard pattern (WHERE id = ? AND version = ?, re-fetch current row on sql.ErrNoRows to return (current, ErrVersionConflict)) is internally consistent and matches the tested contract (TestPlantVersionConflict).
  • writablePlant visibility logic: built-in (OwnerID == nil) → ErrForbidden; another user's plant → ErrNotFound (existence masked); own plant → writable. Matches internal/domain/domain.go's Plant.OwnerID *int64 field and the tests.
  • finalizePlant bounds: minPlantSpacingCM = 0.1, maxPlantSpacingCM = 10_000, maxDaysToMaturity = 3650 (= 365×10, correctly "~10 years"), isHexColor/isFinite reused from objects.go and behave as expected for #rgb/#rrggbb hex and NaN/Inf rejection.
  • plantCategories enum set matches the CHECK constraint in 0001_init.sql exactly (vegetable, herb, flower, fruit, tree_shrub, cover) — tree_shrub/cover are simply unused by the seed data, not a bug.
  • parseNullableInt correctly distinguishes absent (nil,false)/null (nil,true)/number (&n,true) via json.RawMessage, matching the "clear vs unchanged" contract exercised by TestUpdatePlantClearsDays.
  • 0003_seed_plants.sql: exactly 32 rows (grep -c "(NULL," confirms), column order (owner_id, name, category, spacing_cm, color, icon) matches the table, and none of the spacing/category values are dimensionally or semantically wrong for the named plants.
  • isForeignKeyViolation/CountPlantingsForPlant/DeletePlant correctly implement the "count first, FK-violation backstop" pattern described in the PR, and the manual test insert into plantings uses real, correctly-typed columns.

No logic bugs, no incorrect formulas/constants, no broken invariants found in this lens.

🧹 Code cleanliness & maintainability — Minor issues

All claims in the finding check out: parseNullableInt (plants.go:63-70) and parseNullableString (objects.go:87-95) are structurally identical modulo pointer type, parseNullableJSON (objects.go:100-109) is a third near-twin with a different signature, go.mod confirms Go 1.26.2 (generics available), and errors.go does host parseIDParam/writeVersionConflict as shared API helpers. The finding is confirmed.

VERDICT: Minor issues

  • internal/api/plants.go:63-69 (parseNullableInt) is a byte-for-byte structural duplicate of internal/api/objects.go:87-95 (parseNullableString) — same "absent/null/value" decode logic, differing only by pointer type (*int vs *string). There's now a third near-twin, parseNullableJSON (internal/api/objects.go:100-109), which returns the raw JSON text instead of unmarshaling. Verified by reading both files directly: the module targets Go 1.26 (go.mod), so this is exactly the kind of triplet that collapses into one parseNullable[T any](raw json.RawMessage) (*T, bool, error) generic helper (e.g. in errors.go, which already hosts the other small shared API helpers parseIDParam/writeVersionConflict). Low churn: replace the two int/string call sites, leave parseNullableJSON as-is since it doesn't fit the same signature (no unmarshal target type). Impact is maintainability only — a future nullable-field type (float, bool) added anywhere in the API package will likely paste a fourth copy instead of reusing one.
Performance — No material issues found

Everything checks out — indexes exist for the queries added (idx_plants_owner, idx_plantings_plant), the seed migration is a single batched INSERT, list/count/update paths are each single queries with no N+1, and the ListPlantsForActor cap is a documented sanity backstop given household-scale catalogs. No hot-loop or unbounded-growth issues in this diff.

VERDICT: No material issues found

Reviewed through the performance lens:

  • ListPlantsForActor (internal/store/plants.go:65-88) — single query, LIMIT 5000 backstop, indexed on owner_id (idx_plants_owner, confirmed in 0001_init.sql). No N+1.
  • DeletePlant/CountPlantingsForPlant (internal/service/plants.go:105-118, internal/store/plants.go) — one count query before the delete, backed by idx_plantings_plant. Two round trips per delete is negligible at this scale and is a deliberate correctness tradeoff (clear 409 vs. relying on the FK error), not a hot-path concern.
  • writablePlant (internal/service/plants.go) — one GetPlant read per update/delete, point lookup on primary key.
  • 0003_seed_plants.sql — 32 rows inserted via one multi-row INSERT, not 32 separate statements.
  • No loops issuing per-row queries anywhere in the new code; all list/CRUD operations are single round-trips.

Nothing here regresses efficiency or introduces unbounded work.

🧯 Error handling & edge cases — No material issues found

VERDICT: No material issues found

Reviewed through the error-handling/edge-case lens (nil/null, empty, zero/negative, boundary conditions, ignored errors, missing cleanup, races) across internal/api/plants.go, internal/service/plants.go, internal/store/plants.go, internal/store/users.go (new isForeignKeyViolation), and the migration runner (internal/store/migrate.go).

What I specifically verified rather than assumed:

  • Nullable daysToMaturity decode (internal/api/plants.go:52-61, parseNullableInt): absent field (len(raw)==0) → unchanged; JSON null(nil, true) since json.Unmarshal into **int sets the outer pointer to nil on null; a number → allocates and sets. A non-numeric payload is a genuine Unmarshal error, correctly propagated up through toPatch() to a 400. This is a correct, standard nested-pointer decode idiom.
  • Concurrent delete-vs-insert race on plant deletion: DeletePlant (internal/service/plants.go:117-131) does a non-atomic count-then-delete, but I confirmed the plantings.plant_id FK is declared ON DELETE RESTRICT (internal/store/migrations/0001_init.sql:126) and that foreign_keys(1) is actually enforced per-connection via the DSN pragma (internal/store/sqlite.go:20-23), not just assumed — so the isForeignKeyViolation backstop in store.DeletePlant (internal/store/plants.go) is a real, functioning safety net for the race window, not a no-op.
  • Version-conflict path (store/plants.go UpdatePlant): on sql.ErrNoRows it correctly disambiguates "stale version" (row still exists → returns current row + ErrVersionConflict) from "row now gone" (concurrent delete → propagates ErrNotFound), rather than conflating them.
  • Validation boundaries in finalizePlant (internal/service/plants.go:163-187): empty name, unknown category, spacingCm at/below the 0.1 floor, non-hex color, empty icon, and daysToMaturity < 1 are all rejected; isFinite/isHexColor are reused from objects.go rather than reimplemented.
  • Migration idempotency/atomicity: each migration (including the new seed file) runs in its own transaction with the version-row insert (internal/store/migrate.go:78-94), so a partial failure can't leave duplicated seed rows on retry.

Nothing here swallows an error, skips a defer/cleanup, or leaves a reachable nil-deref/panic path. I don't have a finding worth reporting under this lens.

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

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

🔒 Security — No material issues found

Verdict: No material issues found

Through the security lens I traced the request path and verified the authorization and input-handling boundaries:

  • Authn: every /plants route is mounted under v1.Group("/plants", h.requireAuth()) (internal/api/api.go:94), and handlers resolve the actor via mustActor(c).ID (internal/api/plants.go:75,86,108,121). requireAuth rejects cookieless/tampered sessions with 401 (internal/api/auth.go:102-122). CSRF guard is applied at the v1 group level for state-changing calls.
  • Authz / IDOR: writablePlant is the single mutation gate (internal/service/plants.go:91-101): built-ins (OwnerID == nil) → ErrForbidden; another user's row → ErrNotFound (existence masked, consistent with gardens/objects). CreatePlant forces p.OwnerID = &actor (plants.go:81-82), and UpdatePlant/the store update only touch mutable columns — owner_id is never writable via the API, so ownership cannot be reassigned. Verified the store UPDATE (internal/store/plants.go UpdatePlant) does not set owner_id.
  • Injection: all store queries use parameterized placeholders (?); no string interpolation of user input into SQL. plantColumns/ORDER BY ... COLLATE NOCASE are static. isForeignKeyViolation/isUniqueViolation match SQLite's stable error strings.
  • Input validation: finalizePlant (plants.go:152-170) enforces non-empty + length-capped name/icon/notes, category enum membership, isFinite spacing within [0.1, 10000], isHexColor (#rgb/#rrggbb), and daysToMaturity ∈ [1, 3650] when set. parseNullableInt rejects non-integer JSON for the nullable field. I confirmed isFinite/isHexColor exist in internal/service/objects.go:259,280.
  • Existence leakage: built-ins return 403 vs 404 for nonexistent/other-user rows, but built-ins are globally seeded and listed to everyone, so the 403 reveals nothing privileged.

No security defects found in this change.

🎯 Correctness — No material issues found

Verdict: No material issues found

I reviewed the change through the correctness lens, verifying the key logic against the checked-out code (schema in 0001_init.sql, store in internal/store/plants.go, service in internal/service/plants.go, API in internal/api/plants.go, error mapping in internal/api/errors.go, and helpers isFinite/isHexColor in internal/service/objects.go).

Verified items:

  • Visibility/authorization (writablePlant, service/plants.go:91): built-in (OwnerID == nil) → ErrForbidden; another user's row → ErrNotFound (existence masked); own row → writable. Matches the design and the API error mapping (errors.go:25-32).
  • Version guard (store/plants.go UpdatePlant): WHERE id = ? AND version = ? with RETURNING; on sql.ErrNoRows it re-reads via GetPlant and returns (current, ErrVersionConflict). API handler special-cases this with writeVersionConflict(c, p) (plants.go:114), which embeds the current row. Consistent with gardens/objects.
  • Delete-in-use (service/plants.go:124): CountPlantingsForPlant counts all plantings including removed_at rows — correct, because the FK is ON DELETE RESTRICT regardless of removed_at (0001_init.sql:126). The isForeignKeyViolation backstop (store/users.go) catches a concurrent insert that slips past the count check, surfacing ErrPlantInUse (409) rather than a 500.
  • Nullable daysToMaturity patch semantics (api/plants.go parseNullableInt): absent → (nil,false); JSON null(nil,true); number → (&n,true); non-integer → error. SetDays correctly distinguishes "clear" from "unchanged". Verified against applyPlantPatch (service/plants.go:155).
  • Validation bounds (finalizePlant): name non-empty ≤200, category in enum (mirrors the 0001_init.sql:106 CHECK), spacingCm finite and in [0.1, 10000] cm (10000 cm = 100 m, matching the comment), hex color #rgb/#rrggbb, icon non-empty ≤32 bytes, daysToMaturity nil or [1, 3650], notes ≤10000. All re-derived values check out.
  • List ordering (store/plants.go:72): ORDER BY owner_id IS NOT NULL, name COLLATE NOCASE, id — built-ins (NULL → expression = 0) sort before owned rows (1), then by name. Matches the documented "built-ins first then by name".
  • Migration idempotency: 0003_seed_plants.sql is a plain INSERT recorded via the version-tracked runner (migrate.go), so re-running Migrate skips it. 32 rows satisfy the >= 30/< 30 test assertions; seeded Garlic (15 cm, 🧄) matches the service test.

No correctness bugs found in this lens.

🧹 Code cleanliness & maintainability — No material issues found

VERDICT: No material issues found

The plant CRUD layer mirrors the established objects.go / gardens.go patterns closely, and the new code is clean and maintainable:

  • Naming consistency — verified SpacingCM/json:"spacingCm" is used uniformly across domain.Plant (domain.go:163), PlantInput/PlantPatch, and the API request structs, matching the existing XCM/json:"xCm" convention in objects.go.
  • Shared validationfinalizePlant is correctly DRY'd between CreatePlant and UpdatePlant (service/plants.go:165), mirroring finalizeObject in objects.go.
  • Single authorization pointwritablePlant (service/plants.go:89) centralizes the built-in-read-only / cross-user-invisible rules cleanly.
  • parseNullableInt (api/plants.go:63) follows the exact shape of the pre-existing parseNullableString (objects.go:87), so the nullable-field handling is consistent.
  • Migration-count tests — deriving wantVersion/len(migs) from loadMigrations() (store_test.go:43-46, 64-68) is a genuine maintainability improvement over the old hardcoded 2; loadMigrations is the same function Migrate itself uses (migrate.go:98).
  • FK/unique helpersisForeignKeyViolation (users.go:184) sits next to the pre-existing isUniqueViolation and uses the same stable-message approach.

Minor nits (not blocking):

  • isForeignKeyViolation and isUniqueViolation are generic SQLite-error matchers but live in users.go; DeletePlant (store/plants.go) now reaches across into a users-file helper. This follows existing precedent (isUniqueViolation is already there), so it's not a regression, but these generic DB helpers would read better colocated (e.g., a store errors.go). Low value to change now.
  • The maxPlantsListed = 5000 comment says "~30" built-ins; the seed actually ships 32. Trivial doc drift.
Performance — No material issues found

I've reviewed the new plant catalog code through the performance lens, examining the store queries, service methods, and schema indexes.

Findings:

  • ListPlantsForActor (internal/store/plants.go:68): Query WHERE owner_id IS NULL OR owner_id = ? ORDER BY owner_id IS NOT NULL, name COLLATE NOCASE, id. The idx_plants_owner index is on owner_id only. SQLite can use it to satisfy the owner_id = ? range, but the owner_id IS NULL disjunct plus the name COLLATE NOCASE ordering means the ORDER BY cannot be fully index-satisfied — SQLite will sort the combined set. At household scale (~30 built-ins + a handful of owned rows) this is trivially fine, and the LIMIT 5000 backstop caps the work. No material issue.

  • DeletePlant (internal/service/plants.go:124): Two round-trips — CountPlantingsForPlant then DeletePlant. The count is a defensive pre-check before the FK RESTRICT fires, then the actual delete. This is an intentional correctness design (clear error before constraint), and deletion is not a hot path. No material issue.

  • All hot paths (GetPlant, CreatePlant, UpdatePlant, DeletePlant) are single-statement PK lookups/updates, fully covered by the primary key / idx_plants_owner / idx_plantings_plant indexes. No N+1 queries, no unbounded growth beyond the explicit maxPlantsListed cap, no blocking work inside loops. scanPlant allocates one domain.Plant per row (pointer deref then value append), which is standard and minimal.

I found no material performance regressions in this change. The only micro-note (the ORDER BY sort on a tiny set) is explicitly out of scope as a real issue.

Verdict: No material issues found

🧯 Error handling & edge cases — Minor issues

Both findings are confirmed against the actual code.

  • DeletePlant (plants.go:170-177) calls isForeignKeyViolation(err) which, per users.go:184-185, is strings.Contains(err.Error(), "FOREIGN KEY constraint failed") — a substring match on the driver's error message, reusing the established isUniqueViolation pattern. Confirmed.
  • ListPlantsForActor (plants.go:62-74) applies LIMIT maxPlantsListed (=5000) with no error/warning on truncation, documented as a never-hit backstop. Confirmed.

Verdict: Minor issues

  • internal/store/plants.go:170 (DeletePlant) — the isForeignKeyViolation(err) backstop relies on substring-matching the SQLite driver's error text ("FOREIGN KEY constraint failed", per internal/store/users.go:184-185). If the driver ever changes that wording, the concurrent-insert race between CountPlantingsForPlant and the DELETE would surface as a raw 500 INTERNAL instead of the intended 409 PLANT_IN_USE. This mirrors the pre-existing isUniqueViolation pattern, so it's reused fragility, not a regression. Low-impact since the service count check is the primary path; the FK path only fires on a lost race. Suggested fix: prefer a typed/driver-code check if available, or at least unit-test the exact message string so a driver change is caught.
  • internal/store/plants.go:62 (ListPlantsForActor) — the LIMIT 5000 silently truncates with no error/warning if a user exceeds it. Documented as a "never hit" backstop, so non-issue for v1, but it does silently drop rows rather than error. Trivial; flag only because silent truncation is the kind of edge case that bites later. No action needed now.

Both are within the error-handling lens and both are minor. The unhappy paths the diff introduces (missing row → ErrNotFound, built-in → ErrForbidden, cross-user → ErrNotFound masking, version mismatch → current row + ErrVersionConflict, in-use → ErrPlantInUse, invalid daysToMaturity/spacing/color → ErrInvalidInput) are all handled correctly and verified against the code.

kimi-k2.6:cloud (ollama-cloud) — No material issues found

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

🔒 Security — No material issues found

No material issues found

Through the security lens I examined:

  • Authn/Authz: All plant routes are behind requireAuth() middleware. The service-layer writablePlant() gate correctly enforces the documented rules: built-ins → ErrForbidden on mutation, another user's plants → ErrNotFound (existence masked), own plants → writable. Verified against internal/service/plants.go:91-103 and existing patterns in gardens/objects.
  • SQL injection: Store methods use exclusively ? parameterized queries. The only SQL fragments concatenated are constant column lists (plantColumns). Safe.
  • Input validation: finalizePlant validates category against an allow-list, enforces spacing bounds (with isFinite rejecting NaN/Inf), checks hex color format, caps name/icon/notes lengths, and bounds daysToMaturity. parseNullableInt safely distinguishes absent/null/integer via json.RawMessage without allowing non-integer JSON numbers. The Version binding requires min=1.
  • Concurrency backstop: The delete path has a known benign race (count-then-delete), but the store's isForeignKeyViolation backstop maps a concurrent FK violation to ErrPlantInUse instead of surfacing a raw 500. Acceptable defense.
  • Secrets/deserialization/SSRF: None introduced in this PR.
🎯 Correctness — No material issues found

No material issues found

I investigated the plants CRUD implementation, validation logic, optimistic concurrency, nullable field handling, and cross-user authorization semantics. The code correctly:

  • Distinguishes built-in (owner_id nil) from custom plants and enforces read-only vs. read-write semantics (writablePlant at internal/service/plants.go:97).
  • Handles nullable daysToMaturity via json.RawMessage + parseNullableInt, correctly differentiating absent (present=false), explicit null (present=true, value=nil), and integer values (present=true, value=&n) at internal/api/plants.go:75.
  • Rejects out-of-range and malformed inputs in finalizePlant (internal/service/plants.go:163) — spacing bounds, hex color format, name/icon length, and days-to-maturity bounds are all enforced.
  • Returns the current row on version conflict (internal/store/plants.go:141) so the API layer can render the conflict envelope.
  • Surfaces foreign-key violations as ErrPlantInUse (internal/store/plants.go:173) to backstop the service-level count check against concurrent inserts.

One thing I verified carefully: parseNullableInt unmarshals into &value where value is a *int named return; the **int indirection correctly allocates an int on success and leaves value as nil for JSON null, with shadowing of err being harmless because the function returns immediately. No type or logic bug there.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

The code is clean, follows the established project patterns precisely, and is well-structured:

  • Pattern consistency: plants.go (api/service/store) mirrors objects.go and gardens.go exactly — column constants, scan helpers, version-guarded updates with conflict return, nullable-field handling via json.RawMessage + SetX flags, and applyXPatch/finalizeX validation structures.
  • No dead/duplicate code: parseNullableInt is plant-specific and appropriately colocated. isFinite/isHexColor are reused from existing objects.go.
  • Naming is clear: writablePlant, plantCategories, maxPlantSpacingCM, SetDays/DaysToMaturity all read naturally and distinguish create vs update vs patch semantics.
  • Test helpers are localized: intPtr and firstBuiltinID are small, purpose-built, and their duplication across test files is acceptable given the project's current scale.
Performance — No material issues found

Verdict: No material issues found

I reviewed this PR through the performance lens. The changes are straightforward CRUD operations for a plant catalog backed by SQLite, with explicit defensive limits and no hot paths that would cause efficiency problems.

Notable observations (all non-issues at this scale):

  • ListPlantsForActor caps results at 5000 rows (maxPlantsListed) with an explicit comment deferring pagination to post-v1. Given ~32 seeded built-ins and household-scale user catalogs, this is reasonable.
  • CountPlantingsForPlant uses the existing idx_plantings_plant index; counting all rows (including removed ones) is an indexed lookup with no table scan.
  • The migration inserts all 32 seed rows in a single VALUES statement—efficient.
  • No N+1 patterns, no unbounded allocations, no work inside loops beyond the standard row-scanning pattern.
🧯 Error handling & edge cases — No material issues found

No material issues found

I investigated the error-handling and edge-case paths introduced by this PR thoroughly. The key areas checked were:

  • parseNullableInt (internal/api/plants.go:69): Correctly handles absent, explicit null, and integer values. Floats and non-numbers are rejected by Go's json.Unmarshal into *int.
  • Version-conflict path in updatePlant (internal/api/plants.go:117-126): The writeVersionConflict(c, p) call receives a non-nil current row because the store's UpdatePlant only returns ErrVersionConflict alongside a successfully fetched row.
  • TOCTOU race on delete (internal/service/plants.go:124-136): The service pre-checks CountPlantingsForPlant, and the store's DeletePlant has a backstop isForeignKeyViolation handler to convert a concurrent FK violation into ErrPlantInUse rather than a 500. This is explicitly designed for the race.
  • Authorization edge cases (internal/service/plants.go:91-103): writablePlant correctly returns ErrForbidden for built-ins (owner nil) and ErrNotFound for another user's plants, matching the documented masking semantics.
  • Validation (internal/service/plants.go:165-188): finalizePlant consistently guards against empty names, invalid categories, NaN/Inf/negative/zero spacing, malformed colors, empty/oversized icons, out-of-bounds daysToMaturity, and oversized notes. Both create and update paths call it.
  • Empty/null collections: ListPlantsForActor initializes plants := []domain.Plant{} so it always returns a non-nil slice.

Tests cover version conflicts, built-in immutability, cross-user invisibility, validation boundaries, clearing nullable fields, and the FK-backstop lifecycle. Nothing in this PR introduces ignored errors, missing cleanup, unhandled nils, or off-by-one conditions.

opencode/glm-5.2:cloud (opencode) — Minor issues

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

🔒 Security — No material issues found

I reviewed the change through the security lens, focusing on authn/authz, injection, input validation, and untrusted-data handling. I verified by reading the new API handlers (internal/api/plants.go), the service authorization logic (internal/service/plants.go), the store queries (internal/store/plants.go), the schema (internal/store/migrations/0001_init.sql), and the helpers isHexColor/isFinite in internal/service/objects.go.

  • All plant routes are under requireAuth(); handlers obtain the actor via mustActor(c).ID and never trust a client-supplied owner id. CreatePlant forces OwnerID = actorID server-side.
  • Authorization is enforced at writablePlant (service/plants.go:87-100): built-ins (OwnerID == nil) → ErrForbidden; another user's row → ErrNotFound (existence masked, consistent with gardens/objects). Verified the path for both PATCH and DELETE.
  • SQL is fully parameterized in every new query (ListPlantsForActor, GetPlant, CreatePlant, UpdatePlant, CountPlantingsForPlant, DeletePlant); no string interpolation of untrusted values. The plantColumns constant is internal, not user input.
  • IDs come through parseIDParam (int64), so no path/ID injection.
  • Validation mirrors the schema CHECK: name non-empty + length-capped, category enum-checked against a server-side map, SpacingCM finite and bounded (rejects NaN/Inf via isFinite), color via isHexColor (#rgb/#rrggbb only), icon non-empty + byte-capped, optional daysToMaturity bounded to [1, 3650], notes length-capped. isHexColor (service/objects.go:280) is strict.
  • parseNullableInt rejects non-integer JSON values (Go's decoder errors unmarshaling a fractional number into *int), so daysToMaturity: 1.5 is rejected rather than silently truncated.
  • Version field uses binding:"required,min=1", blocking zero/stale-structure requests at the edge.

No authz gaps, injection surface, or untrusted-data handling issues found in this lens.

VERDICT: No material issues found

🎯 Correctness — No material issues found

VERDICT: No material issues found

I reviewed the change through the correctness lens, verifying against the checked-out code.

Verified the key semantic claims:

  • internal/store/plants.go:68-93 ListPlantsForActor: ORDER BY owner_id IS NOT NULL, name COLLATE NOCASE, id correctly puts built-ins (NULL → false → 0) before owned rows (true → 1), then by name. Matches the documented "built-ins first then by name".
  • internal/service/plants.go:91-103 writablePlant: built-in (OwnerID nil) → ErrForbidden; another user's row → ErrNotFound; own row → ok. Matches the test expectations and the masking policy used by gardens/objects.
  • internal/service/plants.go:165-188 finalizePlant: validation bounds (name ≤ 200, spacing 0.1–10000, hex color, icon non-empty ≤ 32 bytes, days 1–3650, notes ≤ 10000) all match the schema in 0001_init.sql:100-115 and the test table. isFinite/isHexColor exist at internal/service/objects.go:259,280.
  • internal/api/plants.go:43-56 parseNullableInt: absent (len 0) → (nil, false); null → unmarshal into *int yields nil with no error → (nil, true); number → (&n, true). The three-way distinction the spec requires is correct.
  • internal/store/plants.go:129-152 UpdatePlant conflict path: on sql.ErrNoRows it re-reads via GetPlant; if that returns ErrNotFound (row deleted concurrently) the store returns (nil, ErrNotFound) rather than a false conflict — the API handler then falls through to writeServiceError → 404. Correct.
  • internal/store/plants.go:158-165 CountPlantingsForPlant counts all rows (including removed_at set), matching the RESTRICT FK semantics described.
  • internal/store/migrations/0003_seed_plants.sql: 32 rows, all categories (vegetable/herb/fruit/flower) are members of the CHECK enum at 0001_init.sql:106; spacing values are positive. Test asserts >= 30 (lenient, fine).
  • internal/store/store_test.go migration-count rework: loadMigrations() exists at internal/store/migrate.go:98 and returns sorted migrations, so deriving wantVersion/count from it is sound.
  • API wiring (internal/api/api.go:93-99) and error mapping (internal/api/errors.go adding PLANT_IN_USE → 409) are consistent with domain.ErrPlantInUse at internal/domain/domain.go:20-22.

No semantic, logic, or boundary-value bugs found within the correctness lens.

🧹 Code cleanliness & maintainability — Minor issues

Confirmed. intPtr exists at plants_test.go:12, and the sibling helpers are ptrBool/strPtr at objects_test.go:22-23. The naming inconsistency finding survives.

VERDICT: Minor issues

  • internal/service/plants_test.go:12func intPtr(n int) *int introduces a new pointer-helper whose naming convention is inconsistent with the existing ptrBool/strPtr helpers in internal/service/objects_test.go:22-23. For consistency it should be ptrInt. Verified by grepping the service package: only ptrBool/strPtr exist as siblings; intPtr is the lone outlier. Trivial, but an easy churn-free fix that keeps the test helpers uniform.

All other structural choices mirror the surrounding code: finalizePlant mirrors finalizeObject, maxPlantsListed-style caps follow maxGardensListed, parseNullableInt mirrors parseNullableString/parseNullableJSON, the version-conflict branch in updatePlant duplicates the same branch already in updateObject/updateGarden, and isForeignKeyViolation is colocated with isUniqueViolation. These follow established codebase patterns rather than introducing regressions.

Performance — No material issues found

I've reviewed the change through the performance lens. The plant catalog is small at household scale (~30 built-ins + a user's own rows), all queries are single-statement with appropriate indexes (idx_plants_owner, idx_plantings_plant), and the list endpoint has a defensive LIMIT 5000 backstop. The delete path makes 3 round trips (GetPlant + count + delete), but that's not a hot path and not material.

VERDICT: No material issues found

🧯 Error handling & edge cases — No material issues found

I reviewed the diff through the error-handling & edge-cases lens, verifying against the checked-out code.

VERDICT: No material issues found

I verified the key unhappy paths:

  • parseNullableInt (api/plants.go:63) correctly distinguishes absent (len==0 → not present), explicit null (unmarshal into *int → nil, present), and integer values; non-integer JSON returns an error that surfaces as a 400. Confirmed json.Unmarshal of null into **int yields nil without error.
  • writablePlant (service/plants.go:91) handles all three visibility states: store error (propagated, e.g. ErrNotFound), built-in (ErrForbidden), and another user's row (ErrNotFound, masking existence). Verified GetPlant (store/plants.go:97) maps sql.ErrNoRowsdomain.ErrNotFound.
  • DeletePlant (service/plants.go:124) TOCTOU between CountPlantingsForPlant and DeletePlant is backstopped by the FK check in DeletePlant (store/plants.go:173) → ErrPlantInUse. Confirmed isForeignKeyViolation (store/users.go) matches SQLite's "FOREIGN KEY constraint failed" message, and the only RESTRICT FK on plants is from plantings.plant_id (0001_init.sql:126).
  • UpdatePlant (store/plants.go:129) on sql.ErrNoRows calls GetPlant; if that also fails (concurrent delete) it returns the underlying error (e.g. ErrNotFound), and the API handler's errors.Is(err, ErrVersionConflict) check correctly falls through to writeServiceError rather than dereferencing a nil p. Verified handler at api/plants.go:111-119.
  • finalizePlant (service/plants.go:165) rejects empty/oversized name, bad category, non-finite/zero/negative/oversized spacing (via isFinite from objects.go:259), bad hex, empty/oversized icon, and daysToMaturity < 1 or > maxDaysToMaturity. Patch fields trim to empty are caught at validation (e.g. Name"" → ErrInvalidInput).
  • plantUpdateRequest.Version uses binding:"required,min=1"; zero/negative/omitted/explicit-null all fail binding → 400 before reaching the service.
  • ListPlantsForActor and ListReferencedPlants both defer rows.Close() and check rows.Err(); scan errors return wrapped errors. Non-nil slice initialized.
  • applyPlantPatch with SetDays=true, DaysToMaturity=nil correctly clears; absent field (SetDays=false) leaves unchanged. Verified by TestUpdatePlantClearsDays.

No swallowed errors, missing cleanup, panic-on-nil, or unhandled boundary conditions found in this lane.

opencode/kimi-k2.6:cloud (opencode) — 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

🎯 Correctness — No material issues found

No material issues found

I scrutinized the change for logic bugs, semantic/domain correctness, boundary conditions, and formula/unit/constant accuracy. Verified key areas by reading the relevant files:

  • parseNullableInt in internal/api/plants.go:63 correctly handles absent (len(raw)==0), null, and integer values via json.Unmarshal into *int; non-integer values produce an error. This is sound.
  • finalizePlant in internal/service/plants.go:165 enforces all stated invariants: non-empty name, known category, finite spacing in [0.1, 10000], hex color, non-empty icon within byte cap, optional days in [1, 3650], and notes length cap. No off-by-one or missing checks.
  • writablePlant in internal/service/plants.go:91 correctly distinguishes built-in (OwnerID == nilErrForbidden) vs. another user (ErrNotFound) vs. own row (writable). This matches the stated visibility rules.
  • UpdatePlant in internal/service/plants.go:107 applies the patch, re-validates, sets the user-supplied version, and passes to the store's version-guarded update. The store back on sql.ErrNoRows fetches the current row and returns it with ErrVersionConflict, matching the garden/object pattern.
  • DeletePlant in internal/service/plants.go:124 pre-checks planting count; the store's DeletePlant further backstops with isForeignKeyViolation to map a concurrent FK failure to ErrPlantInUse. This closes the TOCTOU race correctly.
  • CountPlantingsForPlant in internal/store/plants.go:158 counts all rows for plant_id, including removed ones, which aligns with the schema's ON DELETE RESTRICT and the PR's intent.
  • Migration 0003_seed_plants.sql seeds 32 built-ins with owner_id NULL; the migration runner records version 3, making re-runs idempotent. The store tests derive expected version/count dynamically so future migrations won't break them.

No logic bugs, incorrect constants, or semantic errors were found.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • internal/service/plants_test.go:12intPtr duplicates the pointer-helper pattern already present in internal/service/objects_test.go (ptrBool, strPtr). Three tiny one-liners are now scattered across test files in the same package with inconsistent naming (ptrBool vs strPtr vs intPtr). This makes the helpers hard to discover and invites further duplication. Fix: consolidate them into a shared helpers_test.go in internal/service/ with a consistent scheme (e.g. ptrBool, ptrString, ptrInt), then update callers.
Performance — No material issues found

No material issues found

Reviewed the plant catalog backend changes through the Performance lens. Key areas checked:

  • ListPlantsForActor (internal/store/plants.go:68): Single indexed query with a hard LIMIT 5000. The WHERE owner_id IS NULL OR owner_id = ? is covered by idx_plants_owner; the ORDER BY sort is over a trivially small catalog (~30 built-ins + user rows). No material concern.
  • CountPlantingsForPlant (internal/store/plants.go:158): Indexed count via idx_plantings_plant. Fast and bounded.
  • DeletePlant (internal/service/plants.go): Check-then-act pattern with a CountPlantingsForPlant guard followed by DeletePlant. Race exists but is acknowledged and backstopped by the FK constraint; not a hot-path or scalability issue.
  • No N+1 queries: All reads are single-query, all mutations are direct single-row operations.
  • No unbounded growth: Input fields are length-capped (maxPlantNameLen, maxPlantNotesLen, etc.), and the list endpoint has a hard limit.
  • Migration: 32-row single-statement INSERT ... VALUES, efficient and idempotent via the schema-migrations tracker.

All queries are appropriately indexed and the data volumes are inherently small for this domain. No efficiency regressions or hot-path allocations identified.

🧯 Error handling & edge cases⚠️ could not complete

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

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

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 5 models **Verdict: Minor issues** · 4 findings (1 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟡 | Duplicate pointer helper intPtr should be consolidated with existing ptrBool/strPtr in same package | `internal/service/plants_test.go:12` | 2/5 | maintainability | <details><summary>3 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟡 | parseNullableInt duplicates parseNullableString (objects.go) almost verbatim; should be a shared generic helper | `internal/api/plants.go:63` | claude-code/sonnet | maintainability | | 🟡 | FK-violation backstop uses fragile string-match on driver error; race could surface as 500 instead of 409 | `internal/store/plants.go:170` | glm-5.2:cloud | error-handling | | ⚪ | ListPlantsForActor LIMIT 5000 silently truncates with no error if exceeded | `internal/store/plants.go:62` | glm-5.2:cloud | error-handling | </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> I've written the plan to `/root/.claude/plans/pr-title-plant-catalog-refactored-flurry.md`. Summary: **Task**: Security-lens review of PR #12 (plant catalog CRUD). Phase-1 exploration already confirmed the new code follows this repo's existing auth/parameterization/error-handling conventions (auth-gated routes, `writablePlant` mirrors `requireGardenRole`/`objectForRole`, all SQL uses bind params, no error-text leakage). Five specific verification checks remain (nullable-int parsing, ownership-tamper surface in patch/create structs, delete-race semantics, migration literal-safety, test-file secret hygiene) before I finalize the verdict — expected to land on "no material issues found." Once approved, I'll close out those checks and report findings via `ReportFindings`. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I've completed manual verification by reading the schema, domain struct, SQL statements, and validation logic directly — no need to run the build. Ready to report findings. VERDICT: No material issues found I reviewed this PR strictly for logic bugs and semantic/domain correctness — magic numbers, SQL parameter ordering, version-guard semantics, visibility rules, and the seeded catalog data. Checks performed (all verified by reading the actual checked-out code, not just the diff): - `internal/store/plants.go` — `plantColumns`/`scanPlant` column list matches the `plants` table in `0001_init.sql` exactly (id, owner_id, name, category, spacing_cm, color, icon, days_to_maturity, notes, version, created_at, updated_at). - `CreatePlant`/`UpdatePlant` SQL: placeholder count and parameter order line up 1:1 with the column list in both the `INSERT` and `UPDATE ... SET` statements. - `ListPlantsForActor`'s `ORDER BY owner_id IS NOT NULL, name COLLATE NOCASE, id` correctly sorts built-ins (owner_id IS NULL → boolean 0) before owned rows (→ 1), matching the "built-ins first" claim. - `UpdatePlant`'s version-guard pattern (`WHERE id = ? AND version = ?`, re-fetch current row on `sql.ErrNoRows` to return `(current, ErrVersionConflict)`) is internally consistent and matches the tested contract (`TestPlantVersionConflict`). - `writablePlant` visibility logic: built-in (`OwnerID == nil`) → `ErrForbidden`; another user's plant → `ErrNotFound` (existence masked); own plant → writable. Matches `internal/domain/domain.go`'s `Plant.OwnerID *int64` field and the tests. - `finalizePlant` bounds: `minPlantSpacingCM = 0.1`, `maxPlantSpacingCM = 10_000`, `maxDaysToMaturity = 3650` (= 365×10, correctly "~10 years"), `isHexColor`/`isFinite` reused from `objects.go` and behave as expected for `#rgb`/`#rrggbb` hex and NaN/Inf rejection. - `plantCategories` enum set matches the CHECK constraint in `0001_init.sql` exactly (vegetable, herb, flower, fruit, tree_shrub, cover) — `tree_shrub`/`cover` are simply unused by the seed data, not a bug. - `parseNullableInt` correctly distinguishes absent (`nil,false`)/null (`nil,true`)/number (`&n,true`) via `json.RawMessage`, matching the "clear vs unchanged" contract exercised by `TestUpdatePlantClearsDays`. - `0003_seed_plants.sql`: exactly 32 rows (`grep -c "(NULL," ` confirms), column order `(owner_id, name, category, spacing_cm, color, icon)` matches the table, and none of the spacing/category values are dimensionally or semantically wrong for the named plants. - `isForeignKeyViolation`/`CountPlantingsForPlant`/`DeletePlant` correctly implement the "count first, FK-violation backstop" pattern described in the PR, and the manual test insert into `plantings` uses real, correctly-typed columns. No logic bugs, no incorrect formulas/constants, no broken invariants found in this lens. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All claims in the finding check out: `parseNullableInt` (plants.go:63-70) and `parseNullableString` (objects.go:87-95) are structurally identical modulo pointer type, `parseNullableJSON` (objects.go:100-109) is a third near-twin with a different signature, go.mod confirms Go 1.26.2 (generics available), and `errors.go` does host `parseIDParam`/`writeVersionConflict` as shared API helpers. The finding is confirmed. **VERDICT: Minor issues** - `internal/api/plants.go:63-69` (`parseNullableInt`) is a byte-for-byte structural duplicate of `internal/api/objects.go:87-95` (`parseNullableString`) — same "absent/null/value" decode logic, differing only by pointer type (`*int` vs `*string`). There's now a third near-twin, `parseNullableJSON` (`internal/api/objects.go:100-109`), which returns the raw JSON text instead of unmarshaling. Verified by reading both files directly: the module targets Go 1.26 (`go.mod`), so this is exactly the kind of triplet that collapses into one `parseNullable[T any](raw json.RawMessage) (*T, bool, error)` generic helper (e.g. in `errors.go`, which already hosts the other small shared API helpers `parseIDParam`/`writeVersionConflict`). Low churn: replace the two `int`/`string` call sites, leave `parseNullableJSON` as-is since it doesn't fit the same signature (no unmarshal target type). Impact is maintainability only — a future nullable-field type (float, bool) added anywhere in the API package will likely paste a fourth copy instead of reusing one. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> Everything checks out — indexes exist for the queries added (`idx_plants_owner`, `idx_plantings_plant`), the seed migration is a single batched INSERT, list/count/update paths are each single queries with no N+1, and the `ListPlantsForActor` cap is a documented sanity backstop given household-scale catalogs. No hot-loop or unbounded-growth issues in this diff. ## VERDICT: No material issues found Reviewed through the performance lens: - `ListPlantsForActor` (`internal/store/plants.go:65-88`) — single query, `LIMIT 5000` backstop, indexed on `owner_id` (`idx_plants_owner`, confirmed in `0001_init.sql`). No N+1. - `DeletePlant`/`CountPlantingsForPlant` (`internal/service/plants.go:105-118`, `internal/store/plants.go`) — one count query before the delete, backed by `idx_plantings_plant`. Two round trips per delete is negligible at this scale and is a deliberate correctness tradeoff (clear 409 vs. relying on the FK error), not a hot-path concern. - `writablePlant` (`internal/service/plants.go`) — one `GetPlant` read per update/delete, point lookup on primary key. - `0003_seed_plants.sql` — 32 rows inserted via one multi-row `INSERT`, not 32 separate statements. - No loops issuing per-row queries anywhere in the new code; all list/CRUD operations are single round-trips. Nothing here regresses efficiency or introduces unbounded work. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **VERDICT: No material issues found** Reviewed through the error-handling/edge-case lens (nil/null, empty, zero/negative, boundary conditions, ignored errors, missing cleanup, races) across `internal/api/plants.go`, `internal/service/plants.go`, `internal/store/plants.go`, `internal/store/users.go` (new `isForeignKeyViolation`), and the migration runner (`internal/store/migrate.go`). What I specifically verified rather than assumed: - **Nullable `daysToMaturity` decode** (`internal/api/plants.go:52-61`, `parseNullableInt`): absent field (`len(raw)==0`) → unchanged; JSON `null` → `(nil, true)` since `json.Unmarshal` into `**int` sets the outer pointer to nil on `null`; a number → allocates and sets. A non-numeric payload is a genuine `Unmarshal` error, correctly propagated up through `toPatch()` to a 400. This is a correct, standard nested-pointer decode idiom. - **Concurrent delete-vs-insert race on plant deletion**: `DeletePlant` (`internal/service/plants.go:117-131`) does a non-atomic count-then-delete, but I confirmed the `plantings.plant_id` FK is declared `ON DELETE RESTRICT` (`internal/store/migrations/0001_init.sql:126`) and that `foreign_keys(1)` is actually enforced per-connection via the DSN pragma (`internal/store/sqlite.go:20-23`), not just assumed — so the `isForeignKeyViolation` backstop in `store.DeletePlant` (`internal/store/plants.go`) is a real, functioning safety net for the race window, not a no-op. - **Version-conflict path** (`store/plants.go` `UpdatePlant`): on `sql.ErrNoRows` it correctly disambiguates "stale version" (row still exists → returns current row + `ErrVersionConflict`) from "row now gone" (concurrent delete → propagates `ErrNotFound`), rather than conflating them. - **Validation boundaries** in `finalizePlant` (`internal/service/plants.go:163-187`): empty name, unknown category, `spacingCm` at/below the 0.1 floor, non-hex color, empty icon, and `daysToMaturity < 1` are all rejected; `isFinite`/`isHexColor` are reused from `objects.go` rather than reimplemented. - **Migration idempotency/atomicity**: each migration (including the new seed file) runs in its own transaction with the version-row insert (`internal/store/migrate.go:78-94`), so a partial failure can't leave duplicated seed rows on retry. Nothing here swallows an error, skips a defer/cleanup, or leaves a reachable nil-deref/panic path. I don't have a finding worth reporting under this lens. </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Verdict: No material issues found Through the security lens I traced the request path and verified the authorization and input-handling boundaries: - **Authn**: every `/plants` route is mounted under `v1.Group("/plants", h.requireAuth())` (`internal/api/api.go:94`), and handlers resolve the actor via `mustActor(c).ID` (`internal/api/plants.go:75,86,108,121`). `requireAuth` rejects cookieless/tampered sessions with 401 (`internal/api/auth.go:102-122`). CSRF guard is applied at the `v1` group level for state-changing calls. - **Authz / IDOR**: `writablePlant` is the single mutation gate (`internal/service/plants.go:91-101`): built-ins (`OwnerID == nil`) → `ErrForbidden`; another user's row → `ErrNotFound` (existence masked, consistent with gardens/objects). `CreatePlant` forces `p.OwnerID = &actor` (`plants.go:81-82`), and `UpdatePlant`/the store update only touch mutable columns — `owner_id` is never writable via the API, so ownership cannot be reassigned. Verified the store `UPDATE` (`internal/store/plants.go` UpdatePlant) does not set `owner_id`. - **Injection**: all store queries use parameterized placeholders (`?`); no string interpolation of user input into SQL. `plantColumns`/`ORDER BY ... COLLATE NOCASE` are static. `isForeignKeyViolation`/`isUniqueViolation` match SQLite's stable error strings. - **Input validation**: `finalizePlant` (`plants.go:152-170`) enforces non-empty + length-capped name/icon/notes, category enum membership, `isFinite` spacing within `[0.1, 10000]`, `isHexColor` (`#rgb`/`#rrggbb`), and `daysToMaturity ∈ [1, 3650]` when set. `parseNullableInt` rejects non-integer JSON for the nullable field. I confirmed `isFinite`/`isHexColor` exist in `internal/service/objects.go:259,280`. - **Existence leakage**: built-ins return 403 vs 404 for nonexistent/other-user rows, but built-ins are globally seeded and listed to everyone, so the 403 reveals nothing privileged. No security defects found in this change. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **Verdict: No material issues found** I reviewed the change through the correctness lens, verifying the key logic against the checked-out code (schema in `0001_init.sql`, store in `internal/store/plants.go`, service in `internal/service/plants.go`, API in `internal/api/plants.go`, error mapping in `internal/api/errors.go`, and helpers `isFinite`/`isHexColor` in `internal/service/objects.go`). Verified items: - **Visibility/authorization** (`writablePlant`, `service/plants.go:91`): built-in (`OwnerID == nil`) → `ErrForbidden`; another user's row → `ErrNotFound` (existence masked); own row → writable. Matches the design and the API error mapping (`errors.go:25-32`). - **Version guard** (`store/plants.go` `UpdatePlant`): `WHERE id = ? AND version = ?` with `RETURNING`; on `sql.ErrNoRows` it re-reads via `GetPlant` and returns `(current, ErrVersionConflict)`. API handler special-cases this with `writeVersionConflict(c, p)` (`plants.go:114`), which embeds the current row. Consistent with gardens/objects. - **Delete-in-use** (`service/plants.go:124`): `CountPlantingsForPlant` counts all plantings including `removed_at` rows — correct, because the FK is `ON DELETE RESTRICT` regardless of `removed_at` (`0001_init.sql:126`). The `isForeignKeyViolation` backstop (`store/users.go`) catches a concurrent insert that slips past the count check, surfacing `ErrPlantInUse` (409) rather than a 500. - **Nullable `daysToMaturity` patch semantics** (`api/plants.go` `parseNullableInt`): absent → `(nil,false)`; JSON `null` → `(nil,true)`; number → `(&n,true)`; non-integer → error. `SetDays` correctly distinguishes "clear" from "unchanged". Verified against `applyPlantPatch` (`service/plants.go:155`). - **Validation bounds** (`finalizePlant`): name non-empty ≤200, category in enum (mirrors the `0001_init.sql:106` CHECK), `spacingCm` finite and in `[0.1, 10000]` cm (10000 cm = 100 m, matching the comment), hex color `#rgb`/`#rrggbb`, icon non-empty ≤32 bytes, `daysToMaturity` nil or `[1, 3650]`, notes ≤10000. All re-derived values check out. - **List ordering** (`store/plants.go:72`): `ORDER BY owner_id IS NOT NULL, name COLLATE NOCASE, id` — built-ins (NULL → expression = 0) sort before owned rows (1), then by name. Matches the documented "built-ins first then by name". - **Migration idempotency**: `0003_seed_plants.sql` is a plain `INSERT` recorded via the version-tracked runner (`migrate.go`), so re-running `Migrate` skips it. 32 rows satisfy the `>= 30`/`< 30` test assertions; seeded Garlic (15 cm, 🧄) matches the service test. No correctness bugs found in this lens. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> **VERDICT: No material issues found** The plant CRUD layer mirrors the established `objects.go` / `gardens.go` patterns closely, and the new code is clean and maintainable: - **Naming consistency** — verified `SpacingCM`/`json:"spacingCm"` is used uniformly across `domain.Plant` (domain.go:163), `PlantInput`/`PlantPatch`, and the API request structs, matching the existing `XCM`/`json:"xCm"` convention in `objects.go`. - **Shared validation** — `finalizePlant` is correctly DRY'd between `CreatePlant` and `UpdatePlant` (service/plants.go:165), mirroring `finalizeObject` in objects.go. - **Single authorization point** — `writablePlant` (service/plants.go:89) centralizes the built-in-read-only / cross-user-invisible rules cleanly. - **`parseNullableInt`** (api/plants.go:63) follows the exact shape of the pre-existing `parseNullableString` (objects.go:87), so the nullable-field handling is consistent. - **Migration-count tests** — deriving `wantVersion`/`len(migs)` from `loadMigrations()` (store_test.go:43-46, 64-68) is a genuine maintainability improvement over the old hardcoded `2`; `loadMigrations` is the same function `Migrate` itself uses (migrate.go:98). - **FK/unique helpers** — `isForeignKeyViolation` (users.go:184) sits next to the pre-existing `isUniqueViolation` and uses the same stable-message approach. Minor nits (not blocking): - `isForeignKeyViolation` and `isUniqueViolation` are generic SQLite-error matchers but live in `users.go`; `DeletePlant` (store/plants.go) now reaches across into a users-file helper. This follows existing precedent (`isUniqueViolation` is already there), so it's not a regression, but these generic DB helpers would read better colocated (e.g., a store `errors.go`). Low value to change now. - The `maxPlantsListed = 5000` comment says "~30" built-ins; the seed actually ships 32. Trivial doc drift. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I've reviewed the new plant catalog code through the performance lens, examining the store queries, service methods, and schema indexes. **Findings:** - `ListPlantsForActor` (`internal/store/plants.go:68`): Query `WHERE owner_id IS NULL OR owner_id = ?` ORDER BY `owner_id IS NOT NULL, name COLLATE NOCASE, id`. The `idx_plants_owner` index is on `owner_id` only. SQLite can use it to satisfy the `owner_id = ?` range, but the `owner_id IS NULL` disjunct plus the `name COLLATE NOCASE` ordering means the ORDER BY cannot be fully index-satisfied — SQLite will sort the combined set. At household scale (~30 built-ins + a handful of owned rows) this is trivially fine, and the LIMIT 5000 backstop caps the work. No material issue. - `DeletePlant` (`internal/service/plants.go:124`): Two round-trips — `CountPlantingsForPlant` then `DeletePlant`. The count is a defensive pre-check before the FK RESTRICT fires, then the actual delete. This is an intentional correctness design (clear error before constraint), and deletion is not a hot path. No material issue. - All hot paths (`GetPlant`, `CreatePlant`, `UpdatePlant`, `DeletePlant`) are single-statement PK lookups/updates, fully covered by the primary key / `idx_plants_owner` / `idx_plantings_plant` indexes. No N+1 queries, no unbounded growth beyond the explicit `maxPlantsListed` cap, no blocking work inside loops. `scanPlant` allocates one `domain.Plant` per row (pointer deref then value append), which is standard and minimal. I found no material performance regressions in this change. The only micro-note (the ORDER BY sort on a tiny set) is explicitly out of scope as a real issue. **Verdict: No material issues found** </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both findings are confirmed against the actual code. - `DeletePlant` (plants.go:170-177) calls `isForeignKeyViolation(err)` which, per `users.go:184-185`, is `strings.Contains(err.Error(), "FOREIGN KEY constraint failed")` — a substring match on the driver's error message, reusing the established `isUniqueViolation` pattern. Confirmed. - `ListPlantsForActor` (plants.go:62-74) applies `LIMIT maxPlantsListed` (=5000) with no error/warning on truncation, documented as a never-hit backstop. Confirmed. **Verdict: Minor issues** - `internal/store/plants.go:170` (`DeletePlant`) — the `isForeignKeyViolation(err)` backstop relies on substring-matching the SQLite driver's error text (`"FOREIGN KEY constraint failed"`, per `internal/store/users.go:184-185`). If the driver ever changes that wording, the concurrent-insert race between `CountPlantingsForPlant` and the `DELETE` would surface as a raw `500 INTERNAL` instead of the intended `409 PLANT_IN_USE`. This mirrors the pre-existing `isUniqueViolation` pattern, so it's reused fragility, not a regression. Low-impact since the service count check is the primary path; the FK path only fires on a lost race. Suggested fix: prefer a typed/driver-code check if available, or at least unit-test the exact message string so a driver change is caught. - `internal/store/plants.go:62` (`ListPlantsForActor`) — the `LIMIT 5000` silently truncates with no error/warning if a user exceeds it. Documented as a "never hit" backstop, so non-issue for v1, but it does silently drop rows rather than error. Trivial; flag only because silent truncation is the kind of edge case that bites later. No action needed now. Both are within the error-handling lens and both are minor. The unhappy paths the diff introduces (missing row → `ErrNotFound`, built-in → `ErrForbidden`, cross-user → `ErrNotFound` masking, version mismatch → current row + `ErrVersionConflict`, in-use → `ErrPlantInUse`, invalid `daysToMaturity`/spacing/color → `ErrInvalidInput`) are all handled correctly and verified against the code. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** Through the security lens I examined: - **Authn/Authz**: All plant routes are behind `requireAuth()` middleware. The service-layer `writablePlant()` gate correctly enforces the documented rules: built-ins → `ErrForbidden` on mutation, another user's plants → `ErrNotFound` (existence masked), own plants → writable. Verified against `internal/service/plants.go:91-103` and existing patterns in gardens/objects. - **SQL injection**: Store methods use exclusively `?` parameterized queries. The only SQL fragments concatenated are constant column lists (`plantColumns`). Safe. - **Input validation**: `finalizePlant` validates category against an allow-list, enforces spacing bounds (with `isFinite` rejecting NaN/Inf), checks hex color format, caps name/icon/notes lengths, and bounds `daysToMaturity`. `parseNullableInt` safely distinguishes absent/null/integer via `json.RawMessage` without allowing non-integer JSON numbers. The `Version` binding requires `min=1`. - **Concurrency backstop**: The delete path has a known benign race (count-then-delete), but the store's `isForeignKeyViolation` backstop maps a concurrent FK violation to `ErrPlantInUse` instead of surfacing a raw 500. Acceptable defense. - **Secrets/deserialization/SSRF**: None introduced in this PR. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** I investigated the plants CRUD implementation, validation logic, optimistic concurrency, nullable field handling, and cross-user authorization semantics. The code correctly: - Distinguishes built-in (owner_id nil) from custom plants and enforces read-only vs. read-write semantics (`writablePlant` at `internal/service/plants.go:97`). - Handles nullable `daysToMaturity` via `json.RawMessage` + `parseNullableInt`, correctly differentiating absent (`present=false`), explicit `null` (`present=true, value=nil`), and integer values (`present=true, value=&n`) at `internal/api/plants.go:75`. - Rejects out-of-range and malformed inputs in `finalizePlant` (`internal/service/plants.go:163`) — spacing bounds, hex color format, name/icon length, and days-to-maturity bounds are all enforced. - Returns the current row on version conflict (`internal/store/plants.go:141`) so the API layer can render the conflict envelope. - Surfaces foreign-key violations as `ErrPlantInUse` (`internal/store/plants.go:173`) to backstop the service-level count check against concurrent inserts. One thing I verified carefully: `parseNullableInt` unmarshals into `&value` where `value` is a `*int` named return; the `**int` indirection correctly allocates an `int` on success and leaves `value` as `nil` for JSON `null`, with shadowing of `err` being harmless because the function returns immediately. No type or logic bug there. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> No material issues found The code is clean, follows the established project patterns precisely, and is well-structured: - **Pattern consistency**: `plants.go` (api/service/store) mirrors `objects.go` and `gardens.go` exactly — column constants, scan helpers, version-guarded updates with conflict return, nullable-field handling via `json.RawMessage` + `SetX` flags, and `applyXPatch`/`finalizeX` validation structures. - **No dead/duplicate code**: `parseNullableInt` is plant-specific and appropriately colocated. `isFinite`/`isHexColor` are reused from existing `objects.go`. - **Naming is clear**: `writablePlant`, `plantCategories`, `maxPlantSpacingCM`, `SetDays`/`DaysToMaturity` all read naturally and distinguish create vs update vs patch semantics. - **Test helpers are localized**: `intPtr` and `firstBuiltinID` are small, purpose-built, and their duplication across test files is acceptable given the project's current scale. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **Verdict:** No material issues found I reviewed this PR through the performance lens. The changes are straightforward CRUD operations for a plant catalog backed by SQLite, with explicit defensive limits and no hot paths that would cause efficiency problems. Notable observations (all non-issues at this scale): - `ListPlantsForActor` caps results at 5000 rows (`maxPlantsListed`) with an explicit comment deferring pagination to post-v1. Given ~32 seeded built-ins and household-scale user catalogs, this is reasonable. - `CountPlantingsForPlant` uses the existing `idx_plantings_plant` index; counting all rows (including removed ones) is an indexed lookup with no table scan. - The migration inserts all 32 seed rows in a single `VALUES` statement—efficient. - No N+1 patterns, no unbounded allocations, no work inside loops beyond the standard row-scanning pattern. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> No material issues found I investigated the error-handling and edge-case paths introduced by this PR thoroughly. The key areas checked were: - **`parseNullableInt`** (`internal/api/plants.go:69`): Correctly handles absent, explicit `null`, and integer values. Floats and non-numbers are rejected by Go's `json.Unmarshal` into `*int`. - **Version-conflict path in `updatePlant`** (`internal/api/plants.go:117-126`): The `writeVersionConflict(c, p)` call receives a non-nil current row because the store's `UpdatePlant` only returns `ErrVersionConflict` alongside a successfully fetched row. - **TOCTOU race on delete** (`internal/service/plants.go:124-136`): The service pre-checks `CountPlantingsForPlant`, and the store's `DeletePlant` has a backstop `isForeignKeyViolation` handler to convert a concurrent FK violation into `ErrPlantInUse` rather than a 500. This is explicitly designed for the race. - **Authorization edge cases** (`internal/service/plants.go:91-103`): `writablePlant` correctly returns `ErrForbidden` for built-ins (owner nil) and `ErrNotFound` for another user's plants, matching the documented masking semantics. - **Validation** (`internal/service/plants.go:165-188`): `finalizePlant` consistently guards against empty names, invalid categories, NaN/Inf/negative/zero spacing, malformed colors, empty/oversized icons, out-of-bounds `daysToMaturity`, and oversized notes. Both create and update paths call it. - **Empty/null collections**: `ListPlantsForActor` initializes `plants := []domain.Plant{}` so it always returns a non-nil slice. Tests cover version conflicts, built-in immutability, cross-user invisibility, validation boundaries, clearing nullable fields, and the FK-backstop lifecycle. Nothing in this PR introduces ignored errors, missing cleanup, unhandled nils, or off-by-one conditions. </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> I reviewed the change through the security lens, focusing on authn/authz, injection, input validation, and untrusted-data handling. I verified by reading the new API handlers (`internal/api/plants.go`), the service authorization logic (`internal/service/plants.go`), the store queries (`internal/store/plants.go`), the schema (`internal/store/migrations/0001_init.sql`), and the helpers `isHexColor`/`isFinite` in `internal/service/objects.go`. - All plant routes are under `requireAuth()`; handlers obtain the actor via `mustActor(c).ID` and never trust a client-supplied owner id. `CreatePlant` forces `OwnerID = actorID` server-side. - Authorization is enforced at `writablePlant` (`service/plants.go:87-100`): built-ins (`OwnerID == nil`) → `ErrForbidden`; another user's row → `ErrNotFound` (existence masked, consistent with gardens/objects). Verified the path for both PATCH and DELETE. - SQL is fully parameterized in every new query (`ListPlantsForActor`, `GetPlant`, `CreatePlant`, `UpdatePlant`, `CountPlantingsForPlant`, `DeletePlant`); no string interpolation of untrusted values. The `plantColumns` constant is internal, not user input. - IDs come through `parseIDParam` (int64), so no path/ID injection. - Validation mirrors the schema CHECK: name non-empty + length-capped, category enum-checked against a server-side map, `SpacingCM` finite and bounded (rejects NaN/Inf via `isFinite`), color via `isHexColor` (#rgb/#rrggbb only), icon non-empty + byte-capped, optional `daysToMaturity` bounded to `[1, 3650]`, notes length-capped. `isHexColor` (`service/objects.go:280`) is strict. - `parseNullableInt` rejects non-integer JSON values (Go's decoder errors unmarshaling a fractional number into `*int`), so `daysToMaturity: 1.5` is rejected rather than silently truncated. - `Version` field uses `binding:"required,min=1"`, blocking zero/stale-structure requests at the edge. No authz gaps, injection surface, or untrusted-data handling issues found in this lens. VERDICT: No material issues found </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> VERDICT: No material issues found I reviewed the change through the correctness lens, verifying against the checked-out code. Verified the key semantic claims: - `internal/store/plants.go:68-93` `ListPlantsForActor`: `ORDER BY owner_id IS NOT NULL, name COLLATE NOCASE, id` correctly puts built-ins (NULL → false → 0) before owned rows (true → 1), then by name. Matches the documented "built-ins first then by name". - `internal/service/plants.go:91-103` `writablePlant`: built-in (OwnerID nil) → `ErrForbidden`; another user's row → `ErrNotFound`; own row → ok. Matches the test expectations and the masking policy used by gardens/objects. - `internal/service/plants.go:165-188` `finalizePlant`: validation bounds (name ≤ 200, spacing 0.1–10000, hex color, icon non-empty ≤ 32 bytes, days 1–3650, notes ≤ 10000) all match the schema in `0001_init.sql:100-115` and the test table. `isFinite`/`isHexColor` exist at `internal/service/objects.go:259,280`. - `internal/api/plants.go:43-56` `parseNullableInt`: absent (len 0) → (nil, false); `null` → unmarshal into `*int` yields nil with no error → (nil, true); number → (&n, true). The three-way distinction the spec requires is correct. - `internal/store/plants.go:129-152` `UpdatePlant` conflict path: on `sql.ErrNoRows` it re-reads via `GetPlant`; if that returns `ErrNotFound` (row deleted concurrently) the store returns `(nil, ErrNotFound)` rather than a false conflict — the API handler then falls through to `writeServiceError` → 404. Correct. - `internal/store/plants.go:158-165` `CountPlantingsForPlant` counts all rows (including `removed_at` set), matching the RESTRICT FK semantics described. - `internal/store/migrations/0003_seed_plants.sql`: 32 rows, all categories (`vegetable`/`herb`/`fruit`/`flower`) are members of the CHECK enum at `0001_init.sql:106`; spacing values are positive. Test asserts `>= 30` (lenient, fine). - `internal/store/store_test.go` migration-count rework: `loadMigrations()` exists at `internal/store/migrate.go:98` and returns sorted migrations, so deriving `wantVersion`/`count` from it is sound. - API wiring (`internal/api/api.go:93-99`) and error mapping (`internal/api/errors.go` adding `PLANT_IN_USE` → 409) are consistent with `domain.ErrPlantInUse` at `internal/domain/domain.go:20-22`. No semantic, logic, or boundary-value bugs found within the correctness lens. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Confirmed. `intPtr` exists at plants_test.go:12, and the sibling helpers are `ptrBool`/`strPtr` at objects_test.go:22-23. The naming inconsistency finding survives. VERDICT: Minor issues - `internal/service/plants_test.go:12` — `func intPtr(n int) *int` introduces a new pointer-helper whose naming convention is inconsistent with the existing `ptrBool`/`strPtr` helpers in `internal/service/objects_test.go:22-23`. For consistency it should be `ptrInt`. Verified by grepping the service package: only `ptrBool`/`strPtr` exist as siblings; `intPtr` is the lone outlier. Trivial, but an easy churn-free fix that keeps the test helpers uniform. All other structural choices mirror the surrounding code: `finalizePlant` mirrors `finalizeObject`, `maxPlantsListed`-style caps follow `maxGardensListed`, `parseNullableInt` mirrors `parseNullableString`/`parseNullableJSON`, the version-conflict branch in `updatePlant` duplicates the same branch already in `updateObject`/`updateGarden`, and `isForeignKeyViolation` is colocated with `isUniqueViolation`. These follow established codebase patterns rather than introducing regressions. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I've reviewed the change through the performance lens. The plant catalog is small at household scale (~30 built-ins + a user's own rows), all queries are single-statement with appropriate indexes (`idx_plants_owner`, `idx_plantings_plant`), and the list endpoint has a defensive `LIMIT 5000` backstop. The delete path makes 3 round trips (GetPlant + count + delete), but that's not a hot path and not material. VERDICT: No material issues found </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> I reviewed the diff through the error-handling & edge-cases lens, verifying against the checked-out code. **VERDICT: No material issues found** I verified the key unhappy paths: - `parseNullableInt` (api/plants.go:63) correctly distinguishes absent (`len==0` → not present), explicit `null` (unmarshal into `*int` → nil, present), and integer values; non-integer JSON returns an error that surfaces as a 400. Confirmed `json.Unmarshal` of `null` into `**int` yields nil without error. - `writablePlant` (service/plants.go:91) handles all three visibility states: store error (propagated, e.g. ErrNotFound), built-in (`ErrForbidden`), and another user's row (`ErrNotFound`, masking existence). Verified `GetPlant` (store/plants.go:97) maps `sql.ErrNoRows` → `domain.ErrNotFound`. - `DeletePlant` (service/plants.go:124) TOCTOU between `CountPlantingsForPlant` and `DeletePlant` is backstopped by the FK check in `DeletePlant` (store/plants.go:173) → `ErrPlantInUse`. Confirmed `isForeignKeyViolation` (store/users.go) matches SQLite's `"FOREIGN KEY constraint failed"` message, and the only RESTRICT FK on `plants` is from `plantings.plant_id` (0001_init.sql:126). - `UpdatePlant` (store/plants.go:129) on `sql.ErrNoRows` calls `GetPlant`; if that *also* fails (concurrent delete) it returns the underlying error (e.g. ErrNotFound), and the API handler's `errors.Is(err, ErrVersionConflict)` check correctly falls through to `writeServiceError` rather than dereferencing a nil `p`. Verified handler at api/plants.go:111-119. - `finalizePlant` (service/plants.go:165) rejects empty/oversized name, bad category, non-finite/zero/negative/oversized spacing (via `isFinite` from objects.go:259), bad hex, empty/oversized icon, and `daysToMaturity < 1` or `> maxDaysToMaturity`. Patch fields trim to empty are caught at validation (e.g. `Name` → `""` → ErrInvalidInput). - `plantUpdateRequest.Version` uses `binding:"required,min=1"`; zero/negative/omitted/explicit-null all fail binding → 400 before reaching the service. - `ListPlantsForActor` and `ListReferencedPlants` both `defer rows.Close()` and check `rows.Err()`; scan errors return wrapped errors. Non-nil slice initialized. - `applyPlantPatch` with `SetDays=true, DaysToMaturity=nil` correctly clears; absent field (`SetDays=false`) leaves unchanged. Verified by `TestUpdatePlantClearsDays`. No swallowed errors, missing cleanup, panic-on-nil, or unhandled boundary conditions found in this lane. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — 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 </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> No material issues found I scrutinized the change for logic bugs, semantic/domain correctness, boundary conditions, and formula/unit/constant accuracy. Verified key areas by reading the relevant files: - **`parseNullableInt`** in `internal/api/plants.go:63` correctly handles absent (`len(raw)==0`), `null`, and integer values via `json.Unmarshal` into `*int`; non-integer values produce an error. This is sound. - **`finalizePlant`** in `internal/service/plants.go:165` enforces all stated invariants: non-empty name, known category, finite spacing in `[0.1, 10000]`, hex color, non-empty icon within byte cap, optional days in `[1, 3650]`, and notes length cap. No off-by-one or missing checks. - **`writablePlant`** in `internal/service/plants.go:91` correctly distinguishes built-in (`OwnerID == nil` → `ErrForbidden`) vs. another user (`ErrNotFound`) vs. own row (writable). This matches the stated visibility rules. - **`UpdatePlant`** in `internal/service/plants.go:107` applies the patch, re-validates, sets the user-supplied version, and passes to the store's version-guarded update. The store back on `sql.ErrNoRows` fetches the current row and returns it with `ErrVersionConflict`, matching the garden/object pattern. - **`DeletePlant`** in `internal/service/plants.go:124` pre-checks planting count; the store's `DeletePlant` further backstops with `isForeignKeyViolation` to map a concurrent FK failure to `ErrPlantInUse`. This closes the TOCTOU race correctly. - **`CountPlantingsForPlant`** in `internal/store/plants.go:158` counts all rows for `plant_id`, including removed ones, which aligns with the schema's `ON DELETE RESTRICT` and the PR's intent. - **Migration** `0003_seed_plants.sql` seeds 32 built-ins with `owner_id NULL`; the migration runner records version 3, making re-runs idempotent. The store tests derive expected version/count dynamically so future migrations won't break them. No logic bugs, incorrect constants, or semantic errors were found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - **`internal/service/plants_test.go:12`** — `intPtr` duplicates the pointer-helper pattern already present in `internal/service/objects_test.go` (`ptrBool`, `strPtr`). Three tiny one-liners are now scattered across test files in the same package with inconsistent naming (`ptrBool` vs `strPtr` vs `intPtr`). This makes the helpers hard to discover and invites further duplication. **Fix:** consolidate them into a shared `helpers_test.go` in `internal/service/` with a consistent scheme (e.g. `ptrBool`, `ptrString`, `ptrInt`), then update callers. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** Reviewed the plant catalog backend changes through the **Performance** lens. Key areas checked: - **`ListPlantsForActor`** (`internal/store/plants.go:68`): Single indexed query with a hard `LIMIT 5000`. The `WHERE owner_id IS NULL OR owner_id = ?` is covered by `idx_plants_owner`; the `ORDER BY` sort is over a trivially small catalog (~30 built-ins + user rows). No material concern. - **`CountPlantingsForPlant`** (`internal/store/plants.go:158`): Indexed count via `idx_plantings_plant`. Fast and bounded. - **`DeletePlant`** (`internal/service/plants.go`): Check-then-act pattern with a `CountPlantingsForPlant` guard followed by `DeletePlant`. Race exists but is acknowledged and backstopped by the FK constraint; not a hot-path or scalability issue. - **No N+1 queries**: All reads are single-query, all mutations are direct single-row operations. - **No unbounded growth**: Input fields are length-capped (`maxPlantNameLen`, `maxPlantNotesLen`, etc.), and the list endpoint has a hard limit. - **Migration**: 32-row single-statement `INSERT ... VALUES`, efficient and idempotent via the schema-migrations tracker. All queries are appropriately indexed and the data volumes are inherently small for this domain. No efficiency regressions or hot-path allocations identified. </details> <details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run context deadline exceeded </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-19 01:47:56 +00:00
Address Gadfly review on #12: helper naming + FK-backstop test
Build image / build-and-push (push) Successful in 5s
02e66f462f
- Rename test helper intPtr → ptrInt to match the package's ptrBool sibling
  (2/5 reviewers flagged the naming inconsistency).
- Add a store-level test that drives DeletePlant into the ON DELETE RESTRICT
  foreign key so the isForeignKeyViolation string-match (→ ErrPlantInUse) is
  pinned; a driver-message change now fails here, not as a prod 500.

Skipped (advisory, non-blocking): the parseNullableInt/parseNullableString
generic-helper refactor (cosmetic, touches working code) and the documented
LIMIT 5000 backstop.

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