Closes#16. Part of epic #20 (Phase 6 — Sharing). Extends the requireGardenRole choke point (#7) to consult garden_shares.
What
domain
Garden gains a computedMyRole ("owner"/"editor"/"viewer", omitempty, never persisted). New ShareWithUser (share + recipient email/name) for the UI. Sentinels: ErrShareUserNotFound, ErrCannotShareWithSelf, ErrShareExists.
store
shares.go — GetShareRole, ListSharesForGarden (joined with users for email/name), CreateShare (UNIQUE → ErrShareExists), UpdateShareRole, DeleteShare.
requireGardenRole now consults shares: owner (implicit via owner_id), else the share's role, else masked ErrNotFound. It stamps MyRole on every read-through, so GET /gardens/:id, /full, list, etc. all carry the actor's role.
UpdateGarden tightened to OWNER-only — the matrix says editors edit contents, not the garden's metadata.
UpdatePlanting only re-checks plant visibility when the plant is changed — otherwise a shared editor couldn't edit a plop that uses the owner's private plant (caught by the ACL matrix test).
shares.go — ListShares/AddShare/UpdateShareRole (owner only), RemoveShare (owner or a recipient leaving). AddShare targets an existing account by exact email; unknown → 404, self → 400, duplicate → 409.
api
GET,POST /api/v1/gardens/:id/shares and PATCH,DELETE /api/v1/gardens/:id/shares/:userId. New error codes SHARE_USER_NOT_FOUND / CANNOT_SHARE_WITH_SELF / SHARE_EXISTS.
GOWORK=off go build/vet/test ./internal/... green.
Acceptance criteria
✅ A shares with B as viewer → B lists the garden (myRole: "viewer"), reads /full, gets 403 on mutations; A upgrades B to editor → B moves an object but still can't rename the garden or see shares; A removes the share → B gets 404.
✅ The ACL matrix test is green and covers every row.
✅ Sharing an unknown email → 404 with a clear message.
Note: the /gardens list now returns shared gardens too. They'll render as plain cards (with edit/delete that 403 for non-owners) until #17 adds role badges + read-only gating — that's the intended progression. The frontend's gardenSchema ignores the extra myRole key (zod strips unknowns), so nothing breaks in the interim.
Closes #16. Part of epic #20 (Phase 6 — Sharing). Extends the `requireGardenRole` choke point (#7) to consult `garden_shares`.
## What
### domain
- `Garden` gains a **computed** `MyRole` (`"owner"`/`"editor"`/`"viewer"`, `omitempty`, never persisted). New `ShareWithUser` (share + recipient email/name) for the UI. Sentinels: `ErrShareUserNotFound`, `ErrCannotShareWithSelf`, `ErrShareExists`.
### store
- `shares.go` — `GetShareRole`, `ListSharesForGarden` (joined with `users` for email/name), `CreateShare` (UNIQUE → `ErrShareExists`), `UpdateShareRole`, `DeleteShare`.
- `gardens.go` — `ListGardensForActor` returns **owned + shared-with-me** gardens (UNION), each row carrying `my_role`.
### service
- **`requireGardenRole` now consults shares**: owner (implicit via `owner_id`), else the share's role, else masked `ErrNotFound`. It stamps `MyRole` on every read-through, so `GET /gardens/:id`, `/full`, list, etc. all carry the actor's role.
- **`UpdateGarden` tightened to OWNER-only** — the matrix says editors edit *contents*, not the garden's metadata.
- **`UpdatePlanting` only re-checks plant visibility when the plant is *changed*** — otherwise a shared editor couldn't edit a plop that uses the owner's private plant (caught by the ACL matrix test).
- `shares.go` — `ListShares`/`AddShare`/`UpdateShareRole` (owner only), `RemoveShare` (owner **or** a recipient leaving). `AddShare` targets an existing account by **exact email**; unknown → 404, self → 400, duplicate → 409.
### api
- `GET,POST /api/v1/gardens/:id/shares` and `PATCH,DELETE /api/v1/gardens/:id/shares/:userId`. New error codes `SHARE_USER_NOT_FOUND` / `CANNOT_SHARE_WITH_SELF` / `SHARE_EXISTS`.
## Tests
- **The authoritative ACL matrix**: `{owner, editor, viewer, stranger} × {read /full, mutate object, mutate planting, edit garden meta, delete garden, manage shares}` — nil = allowed, `ErrForbidden` = visible-but-insufficient, `ErrNotFound` = masked.
- Shared gardens carry `my_role`; `AddShare` error cases (unknown/self/bad-role/duplicate); role upgrade + editor-can-move-object-but-not-manage-shares; recipient self-leave; a non-participant can't remove others.
- Two-user HTTP flow (viewer → editor → unshare) + auth-required.
`GOWORK=off go build/vet/test ./internal/...` green.
## Acceptance criteria
- ✅ A shares with B as viewer → B lists the garden (`myRole: "viewer"`), reads `/full`, gets 403 on mutations; A upgrades B to editor → B moves an object but still can't rename the garden or see shares; A removes the share → B gets 404.
- ✅ The ACL matrix test is green and covers every row.
- ✅ Sharing an unknown email → 404 with a clear message.
> Note: the `/gardens` list now returns shared gardens too. They'll render as plain cards (with edit/delete that 403 for non-owners) until #17 adds role badges + read-only gating — that's the intended progression. The frontend's `gardenSchema` ignores the extra `myRole` key (zod strips unknowns), so nothing breaks in the interim.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
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 03:43:12Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — No material issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — Minor issues
- ✅ **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** — Blocking issues found
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — No material issues found
- ✅ **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>
🪰Gadfly consensus review — 12 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 12 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡"owner" role string duplicated between service String() and store SQL literal; no domain.RoleOwner constant
maintainability · flagged by 2 models
internal/service/gardens.go:39 — "owner" role string is hardcoded in gardenRole.String() while the store SQL at internal/store/gardens.go:78 emits 'owner' AS my_role as a literal. There is no domain.RoleOwner constant (only RoleViewer/RoleEditor). If either side drifts, owned-garden MyRole silently mismatches. Fix: add domain.RoleOwner = "owner" and reference it in both places. - internal/store/shares.go:61 — ListSharesForGarden inlines the share-column scan ins…
🪰 Gadfly · advisory
🟡 **"owner" role string duplicated between service String() and store SQL literal; no domain.RoleOwner constant**
_maintainability · flagged by 2 models_
- **`internal/service/gardens.go:39`** — `"owner"` role string is hardcoded in `gardenRole.String()` while the store SQL at `internal/store/gardens.go:78` emits `'owner' AS my_role` as a literal. There is no `domain.RoleOwner` constant (only `RoleViewer`/`RoleEditor`). If either side drifts, owned-garden `MyRole` silently mismatches. Fix: add `domain.RoleOwner = "owner"` and reference it in both places. - **`internal/store/shares.go:61`** — `ListSharesForGarden` inlines the share-column scan ins…
<sub>🪰 Gadfly · advisory</sub>
🟡UpdateGarden hardcodes roleOwner.String() instead of reusing requireGardenRole result
maintainability · flagged by 1 model
internal/service/gardens.go:151 — UpdateGarden hardcodes updated.MyRole = roleOwner.String() instead of reusing the role already computed and stamped by requireGardenRole. The returned garden from requireGardenRole is discarded (if _, err := ...), so the permission result is recalculated by hand. This is brittle: if the model ever allows non-owners through (co-owners, admin override), the response will falsely claim "owner". Capture the garden from requireGardenRole and use its…
🪰 Gadfly · advisory
🟡 **UpdateGarden hardcodes roleOwner.String() instead of reusing requireGardenRole result**
_maintainability · flagged by 1 model_
- `internal/service/gardens.go:151` — `UpdateGarden` hardcodes `updated.MyRole = roleOwner.String()` instead of reusing the role already computed and stamped by `requireGardenRole`. The returned garden from `requireGardenRole` is discarded (`if _, err := ...`), so the permission result is recalculated by hand. This is brittle: if the model ever allows non-owners through (co-owners, admin override), the response will falsely claim `"owner"`. Capture the garden from `requireGardenRole` and use its…
<sub>🪰 Gadfly · advisory</sub>
🟠Plant-visibility re-check triggers on patch field presence, not actual value change, defeating the shared-editor fix for a no-op plantId resend
error-handling, maintainability · flagged by 1 model
internal/service/plantings.go:129-133 — The "only re-check plant visibility when the plant is changed" guard checks whether patch.PlantID is present, not whether the value actually differs from the planting's current PlantID: go if patch.PlantID != nil { if _, err := s.visiblePlant(ctx, actorID, pl.PlantID); err != nil { return nil, err } } If a caller sends plantId in the PATCH body equal to the plop's existing plant (e.g. a user re-opens a plant picker and re-selects…
🪰 Gadfly · advisory
🟠 **Plant-visibility re-check triggers on patch field presence, not actual value change, defeating the shared-editor fix for a no-op plantId resend**
_error-handling, maintainability · flagged by 1 model_
- **`internal/service/plantings.go:129-133`** — The "only re-check plant visibility when the plant is *changed*" guard checks whether `patch.PlantID` is *present*, not whether the value actually *differs* from the planting's current `PlantID`: ```go if patch.PlantID != nil { if _, err := s.visiblePlant(ctx, actorID, pl.PlantID); err != nil { return nil, err } } ``` If a caller sends `plantId` in the PATCH body equal to the plop's existing plant (e.g. a user re-opens a plant picker and re-selects…
<sub>🪰 Gadfly · advisory</sub>
🟡Redundant duplicate GetPlant query in UpdatePlanting when the plant is changed
error-handling, performance · flagged by 2 models
internal/service/plantings.go:134 — GetPlant after a plant-id change can return ErrNotFound and surface as a raw 404 where ErrInvalidInput (400) was intended. When patch.PlantID != nil, the code first calls s.visiblePlant(...) which maps a missing/invisible plant to ErrInvalidInput (line 130, confirmed at plantings.go:176-187). But visiblePlant already fetches the plant and returns it — that value is discarded (if _, err := ...), and then line 134 calls `s.store.GetPlan…
🪰 Gadfly · advisory
🟡 **Redundant duplicate GetPlant query in UpdatePlanting when the plant is changed**
_error-handling, performance · flagged by 2 models_
- `internal/service/plantings.go:134` — **`GetPlant` after a plant-id change can return `ErrNotFound` and surface as a raw 404 where `ErrInvalidInput` (400) was intended.** When `patch.PlantID != nil`, the code first calls `s.visiblePlant(...)` which maps a missing/invisible plant to `ErrInvalidInput` (line 130, confirmed at `plantings.go:176-187`). But `visiblePlant` already fetches the plant and returns it — that value is discarded (`if _, err := ...`), and then line 134 calls `s.store.GetPlan…
<sub>🪰 Gadfly · advisory</sub>
🟡AddShare exposes an authenticated user-enumeration oracle (unknown email → 404, known → 201/409)
security · flagged by 1 model
internal/service/shares.go:30 (AddShare) — authenticated user-enumeration oracle. Any authenticated owner of a garden can probe whether an arbitrary email is a registered pansy account: calling POST /gardens/:id/shares with {"email": "<guess>", "role": "viewer"} returns 404 SHARE_USER_NOT_FOUND for an unknown email (mapping at internal/api/errors.go:33-34) vs. 201 (or 409 SHARE_EXISTS per errors.go:37-38) for a known one. The owner can then DELETE the share to clean up.…
🪰 Gadfly · advisory
🟡 **AddShare exposes an authenticated user-enumeration oracle (unknown email → 404, known → 201/409)**
_security · flagged by 1 model_
- `internal/service/shares.go:30` (`AddShare`) — **authenticated user-enumeration oracle.** Any authenticated owner of a garden can probe whether an arbitrary email is a registered pansy account: calling `POST /gardens/:id/shares` with `{"email": "<guess>", "role": "viewer"}` returns `404 SHARE_USER_NOT_FOUND` for an unknown email (mapping at `internal/api/errors.go:33-34`) vs. `201` (or `409 SHARE_EXISTS` per `errors.go:37-38`) for a known one. The owner can then `DELETE` the share to clean up.…
<sub>🪰 Gadfly · advisory</sub>
🟡AddShare 404 for unknown email enables authenticated account enumeration
security · flagged by 1 model
internal/service/shares.go:39 (AddShare) — authenticated account enumeration.GetUserByEmail returning domain.ErrNotFound is converted to ErrShareUserNotFound, which writeServiceError maps to a distinct 404 SHARE_USER_NOT_FOUND with message "no account with that email"; a valid target yields 409/201. Since any authenticated user can create a garden (becoming its owner) and then call AddShare on it, any logged-in user can probe arbitrary email addresses to learn whether th…
🪰 Gadfly · advisory
🟡 **AddShare 404 for unknown email enables authenticated account enumeration**
_security · flagged by 1 model_
- `internal/service/shares.go:39` (`AddShare`) — **authenticated account enumeration.** `GetUserByEmail` returning `domain.ErrNotFound` is converted to `ErrShareUserNotFound`, which `writeServiceError` maps to a distinct 404 `SHARE_USER_NOT_FOUND` with message "no account with that email"; a valid target yields 409/201. Since any authenticated user can create a garden (becoming its owner) and then call `AddShare` on it, *any* logged-in user can probe arbitrary email addresses to learn whether th…
<sub>🪰 Gadfly · advisory</sub>
🔴RemoveShare bypasses requireGardenRole authorization choke point
maintainability, security · flagged by 5 models
RemoveShare bypasses the requireGardenRole authorization choke point, allowing an actor with no role on a garden to reach store.DeleteShare. internal/service/shares.go:66 opens the garden with a raw store.GetGarden instead of funneling through requireGardenRole — the comment on requireGardenRole calls it “THE place authorization for a garden is decided.” A stranger can call RemoveShare with targetUserID == actorID; the ad-hoc check `g.OwnerID != actorID && actorID != targ…
🪰 Gadfly · advisory
🔴 **RemoveShare bypasses requireGardenRole authorization choke point**
_maintainability, security · flagged by 5 models_
- **`RemoveShare` bypasses the `requireGardenRole` authorization choke point**, allowing an actor with no role on a garden to reach `store.DeleteShare`. `internal/service/shares.go:66` opens the garden with a raw `store.GetGarden` instead of funneling through `requireGardenRole` — the comment on `requireGardenRole` calls it “THE place authorization for a garden is decided.” A stranger can call `RemoveShare` with `targetUserID == actorID`; the ad-hoc check `g.OwnerID != actorID && actorID != targ…
<sub>🪰 Gadfly · advisory</sub>
🟡Owner removing a non-existent share returns ErrNotFound (404) instead of 204; behavior is defensible but undocumented beyond a comment
error-handling, maintainability, security · flagged by 2 models
internal/service/shares.go:71 — Owner removing a non-existent share / self-leave of a non-share returns ErrNotFound instead of 204. In RemoveShare, the guard is g.OwnerID != actorID && actorID != targetUserID (confirmed). The GetGarden lookup at line 67 masks the garden from strangers (good). But the owner path and the self-leave path both fall through to s.store.DeleteShare (line 76), which returns domain.ErrNotFound when 0 rows are affected (confirmed at `internal/store/sha…
🪰 Gadfly · advisory
🟡 **Owner removing a non-existent share returns ErrNotFound (404) instead of 204; behavior is defensible but undocumented beyond a comment**
_error-handling, maintainability, security · flagged by 2 models_
- `internal/service/shares.go:71` — **Owner removing a non-existent share / self-leave of a non-share returns `ErrNotFound` instead of 204.** In `RemoveShare`, the guard is `g.OwnerID != actorID && actorID != targetUserID` (confirmed). The `GetGarden` lookup at line 67 masks the garden from strangers (good). But the owner path and the self-leave path both fall through to `s.store.DeleteShare` (line 76), which returns `domain.ErrNotFound` when 0 rows are affected (confirmed at `internal/store/sha…
<sub>🪰 Gadfly · advisory</sub>
⚪wantErr helper is local to shares_test.go but generally useful; other tests re-implement the same assertion inline
maintainability · flagged by 1 model
internal/service/gardens.go:39 — "owner" role string is hardcoded in gardenRole.String() while the store SQL at internal/store/gardens.go:78 emits 'owner' AS my_role as a literal. There is no domain.RoleOwner constant (only RoleViewer/RoleEditor). If either side drifts, owned-garden MyRole silently mismatches. Fix: add domain.RoleOwner = "owner" and reference it in both places. - internal/store/shares.go:61 — ListSharesForGarden inlines the share-column scan ins…
🪰 Gadfly · advisory
⚪ **wantErr helper is local to shares_test.go but generally useful; other tests re-implement the same assertion inline**
_maintainability · flagged by 1 model_
- **`internal/service/gardens.go:39`** — `"owner"` role string is hardcoded in `gardenRole.String()` while the store SQL at `internal/store/gardens.go:78` emits `'owner' AS my_role` as a literal. There is no `domain.RoleOwner` constant (only `RoleViewer`/`RoleEditor`). If either side drifts, owned-garden `MyRole` silently mismatches. Fix: add `domain.RoleOwner = "owner"` and reference it in both places. - **`internal/store/shares.go:61`** — `ListSharesForGarden` inlines the share-column scan ins…
<sub>🪰 Gadfly · advisory</sub>
🟡SQL hardcodes 'owner' role string duplicating service-layer gardenRole.String()
maintainability · flagged by 1 model
internal/store/gardens.go:78 — The ListGardensForActor query hardcodes the string literal 'owner' for the owner role ('owner' AS my_role), while the service layer derives the same value via roleOwner.String(). This duplicates the role-string definition across store and service layers; if the API contract changes, both places must be updated or they drift apart.
🪰 Gadfly · advisory
🟡 **SQL hardcodes 'owner' role string duplicating service-layer gardenRole.String()**
_maintainability · flagged by 1 model_
- `internal/store/gardens.go:78` — The `ListGardensForActor` query hardcodes the string literal `'owner'` for the owner role (`'owner' AS my_role`), while the service layer derives the same value via `roleOwner.String()`. This duplicates the role-string definition across store and service layers; if the API contract changes, both places must be updated or they drift apart.
<sub>🪰 Gadfly · advisory</sub>
🟠ListSharesForGarden is unbounded — missing LIMIT clause
performance · flagged by 1 model
internal/store/shares.go:46 — ListSharesForGarden queries garden_shares joined with users but has no LIMIT clause, making the result set unbounded. ListGardensForActor (in the same PR area) explicitly caps at maxGardensListed = 1000 as a "defensive backstop against a pathologically large result"; the shares list should have the same defense. Even at normal sharing scales this is unlikely to be hit, but the endpoint should not be the only unbounded list query in the surface. *…
🪰 Gadfly · advisory
🟠 **ListSharesForGarden is unbounded — missing LIMIT clause**
_performance · flagged by 1 model_
- **`internal/store/shares.go:46`** — `ListSharesForGarden` queries `garden_shares` joined with `users` but has no `LIMIT` clause, making the result set unbounded. `ListGardensForActor` (in the same PR area) explicitly caps at `maxGardensListed = 1000` as a "defensive backstop against a pathologically large result"; the shares list should have the same defense. Even at normal sharing scales this is unlikely to be hit, but the endpoint should not be the only unbounded list query in the surface. *…
<sub>🪰 Gadfly · advisory</sub>
🟡ListSharesForGarden inlines the share-column scan, duplicating shareColumns order instead of extending scanShare
maintainability · flagged by 1 model
internal/service/gardens.go:39 — "owner" role string is hardcoded in gardenRole.String() while the store SQL at internal/store/gardens.go:78 emits 'owner' AS my_role as a literal. There is no domain.RoleOwner constant (only RoleViewer/RoleEditor). If either side drifts, owned-garden MyRole silently mismatches. Fix: add domain.RoleOwner = "owner" and reference it in both places. - internal/store/shares.go:61 — ListSharesForGarden inlines the share-column scan ins…
🪰 Gadfly · advisory
🟡 **ListSharesForGarden inlines the share-column scan, duplicating shareColumns order instead of extending scanShare**
_maintainability · flagged by 1 model_
- **`internal/service/gardens.go:39`** — `"owner"` role string is hardcoded in `gardenRole.String()` while the store SQL at `internal/store/gardens.go:78` emits `'owner' AS my_role` as a literal. There is no `domain.RoleOwner` constant (only `RoleViewer`/`RoleEditor`). If either side drifts, owned-garden `MyRole` silently mismatches. Fix: add `domain.RoleOwner = "owner"` and reference it in both places. - **`internal/store/shares.go:61`** — `ListSharesForGarden` inlines the share-column scan ins…
<sub>🪰 Gadfly · advisory</sub>
Good — sharing routes sit under gardens group with h.requireAuth() and CSRF guard applies globally. qualifyColumns only operates on the hardcoded gardenColumns/shareColumns constants, not user input, so no injection risk. Everything else checks out: parameterized queries throughout, DB-level CHECK (role IN ('viewer','editor')) backs the app-layer isShareRole validation, email lookup is case-insensitive by design, and the visiblePlant bypass in UpdatePlanting doesn't newly disclose anything — GardenFull already returns all referenced plants to any garden viewer/editor via ListReferencedPlants, unrelated to this diff.
I'm satisfied this diff has no material security issues.
VERDICT: No material issues found
I reviewed this diff specifically for authn/authz gaps, injection, IDOR, and information disclosure:
Authorization funnel intact: every new share endpoint routes through requireGardenRole/explicit owner checks in internal/service/shares.go. Verified ListShares/AddShare/UpdateShareRole require roleOwner, and RemoveShare's manual owner-or-self check (internal/service/shares.go:68-72) can't be bypassed — a stranger passing their own ID as target just hits a real "no such share" 404 rather than an authorization bypass (traced through DeleteShare's RowsAffected check in internal/store/shares.go).
SQL injection: all new queries (internal/store/shares.go, internal/store/gardens.go) use parameterized ? placeholders. qualifyColumns (internal/store/sqlite.go:105) is only ever applied to the hardcoded gardenColumns/shareColumns constants, never to request input.
Role escalation: isShareRole restricts grantable roles to viewer/editor (owner can't be granted via a share), and this is backed by a DB CHECK (role IN ('viewer','editor')) constraint (internal/store/migrations/0001_init.sql:61) — defense in depth.
Email targeting for AddShare: verified GetUserByEmail is case-insensitive by column collation (comment + implementation in internal/store/users.go:94-98), so no case-mismatch bypass; self-share is checked by user ID after lookup, not by string comparison, so case games don't matter.
UpdatePlanting visibility bypass (skipping visiblePlant when patch.PlantID is nil): checked whether this newly leaks a private plant's data to a shared editor. It does not — GardenFull (internal/service/objects.go:161-183, unchanged by this PR) already returns every plant referenced anywhere in the garden via ListReferencedPlants to any viewer/editor, so the plant's data was already exposed to garden participants regardless of this patch.
Routes are behind auth + CSRF: the new /shares routes are registered inside the gardens group which already carries h.requireAuth(), and the whole v1 group carries h.csrfGuard() (internal/api/api.go:47,72-88).
Nothing else in this diff (error mapping in internal/api/errors.go, the MyRole computed field, the ShareWithUser join) introduces new untrusted-input handling or a new trust boundary.
🎯 Correctness — No material issues found
Verdict: No material issues found
I reviewed the sharing ACL logic (requireGardenRole/effectiveGardenRole), the new garden_shares CRUD, the ListGardensForActor UNION query, and the UpdatePlanting plant-visibility change, cross-checking each against the actual checked-out files (internal/service/gardens.go, internal/service/plantings.go, internal/service/shares.go, internal/store/gardens.go, internal/store/shares.go, internal/api/errors.go, internal/api/api.go, and the garden_shares schema in migrations/0001_init.sql).
Specific things I verified rather than assumed:
gardenRole ordering (roleNone < roleViewer < roleEditor < roleOwner) makes the role < min comparisons in requireGardenRole correct, and effectiveGardenRole correctly special-cases the owner (never a garden_shares row, matching the schema comment) before falling back to GetShareRole.
Every mutation path (objectForRole, UpdatePlanting, DeleteGarden, GardenFull) funnels through requireGardenRole, so shares are honored everywhere, not just on the garden read path — no bypass found.
UpdatePlanting's new "only re-check plant visibility when patch.PlantID != nil" gate correctly uses the pointer-nil convention already used by every other field in PlantingPatch/plantingUpdateRequest (internal/api/plantings.go:40), so a client that omits plantId from the JSON body won't trip the visibility check on an unrelated edit — this isn't a loophole, it's consistent with the existing partial-PATCH contract, and it's covered by TestGardenACLMatrix's mutPlop/editor case.
ListGardensForActor's UNION ALL query: both branches project the same 11 columns in the same order, the outer ORDER BY created_at, id binds to the first branch's column names (valid SQLite UNION column-naming rule), and a user can never appear in both branches simultaneously since AddShare explicitly rejects self-sharing (ErrCannotShareWithSelf) and owners never get a share row.
RemoveShare's participant check (g.OwnerID != actorID && actorID != targetUserID → masked ErrNotFound) correctly allows owner-removes-anyone and self-leave while blocking a non-participant from removing someone else's share, matching TestUpdateAndRemoveShare's "stranger remove" case.
CreateShare's duplicate-share handling maps to the actual UNIQUE (garden_id, user_id) constraint in the schema — no other unique constraint on that table, so isUniqueViolation can't misattribute a different failure.
isShareRole correctly restricts AddShare/UpdateShareRole to viewer/editor, blocking privilege escalation to an owner share row.
Error-mapping order in errors.go is irrelevant here since the new sentinels are independent (not wrapped) errors, so placement in the switch doesn't cause a case to be shadowed.
One minor observation that I'm not raising as a finding since it doesn't produce incorrect behavior against any stated requirement: UpdateShareRole (store) increments version but never checks a caller-supplied version, unlike every other mutable resource's stated "sync contract" (see the comment on store.UpdateGarden). That's a design inconsistency (no lost-update protection for concurrent share edits), not a logic bug — shares are owner-only mutations, low contention, and nothing in the PR's acceptance criteria requires OCC on shares.
🧹 Code cleanliness & maintainability — Minor issues
Confirmed: visiblePlant internally calls s.store.GetPlant and returns that same plant — so when patch.PlantID != nil, UpdatePlanting truly fetches the row twice. All three draft findings verified against the actual code.
VERDICT: Minor issues
internal/service/plantings.go:129-137 — UpdatePlanting discards the *domain.Plant returned by s.visiblePlant (via if _, err := s.visiblePlant(...)) when patch.PlantID != nil, then unconditionally re-fetches the same row via s.store.GetPlant(ctx, pl.PlantID) at line 134 just to get the spacing for the derived count. CreatePlanting (same file, lines 82/108) shows the intended pattern of capturing and reusing the plant visiblePlant returns. This is a wasted DB round-trip whenever a planting's plant is being changed.
internal/service/shares.go:66-77 — RemoveShare bypasses the requireGardenRole choke point and instead calls s.store.GetGarden directly (confirmed via grep: this is the only other call site of s.store.GetGarden in the service package besides requireGardenRole itself, at gardens.go:67), hand-rolling its own "load or NotFound" + owner-or-self check. This works against the file's own stated design — the comment on requireGardenRole in gardens.go:58-65 calls it "THE place authorization for a garden is decided… so neither can skip a check." Could be written as g, err := s.requireGardenRole(ctx, actorID, gardenID, roleViewer) followed by the owner-or-self check, keeping a single source of truth for the "load garden, mask as NotFound" step.
internal/service/gardens.go:36-47 — gardenRole.String() sources the editor/viewer strings from domain.RoleEditor/domain.RoleViewer but hardcodes the literal "owner" at line 39 instead of a matching constant (confirmed via grep of internal/domain/domain.go: only RoleViewer/RoleEditor are defined, no RoleOwner). Minor inconsistency; low impact since it's centralized in this one method.
⚡ Performance — Minor issues
All claims verified against actual source.
VERDICT: Minor issues
internal/service/plantings.go:129-137 — In UpdatePlanting, when patch.PlantID != nil, the code calls s.visiblePlant(ctx, actorID, pl.PlantID) (which internally calls s.store.GetPlant, plantings.go:177) purely to validate visibility and discards the returned *domain.Plant, then unconditionally calls s.store.GetPlant(ctx, pl.PlantID) again at line 134 to get the plant for the derived-count calculation. That's two identical SELECT queries against plants for the same plantID on the plant-changed path, where CreatePlanting (line 82) fetches once and reuses the result. Low absolute cost (one extra indexed single-row lookup per planting update), but avoidable.
Suggested fix: reuse the *domain.Plant returned by visiblePlant when patch.PlantID != nil instead of re-fetching via s.store.GetPlant.
Everything else in this diff checked out fine from a performance standpoint:
ListGardensForActor's UNION query (internal/store/gardens.go:75) is backed by idx_gardens_owner and idx_garden_shares_user (confirmed in internal/store/migrations/0001_init.sql:53,69).
GetShareRole (internal/store/shares.go:30) used in effectiveGardenRole is a single indexed point lookup per non-owner permission check, not called in a loop.
email has a UNIQUE COLLATE NOCASE index (migrations/0001_init.sql:16) and garden_shares has UNIQUE (garden_id, user_id) (migrations/0001_init.sql:66), so AddShare's lookup/uniqueness checks are indexed.
ListSharesForGarden (internal/store/shares.go:45) is a single joined query, no N+1.
🧯 Error handling & edge cases — Minor issues
This matches the diff; nothing new to flag. The verification is complete — the single finding in the draft holds up fully against the code.
Review — 🧯 Error handling & edge cases
VERDICT: Minor issues
internal/service/plantings.go:129-133 — The "only re-check plant visibility when the plant is changed" guard checks whether patch.PlantID is present, not whether the value actually differs from the planting's current PlantID:
If a caller sends plantId in the PATCH body equal to the plop's existing plant (e.g. a user re-opens a plant picker and re-selects the currently-assigned plant, or a future/alternate client resends the full object), this still runs visiblePlant, which fails with ErrInvalidInput for a shared editor working with the owner's private plant — exactly the scenario this PR says it fixed ("otherwise a shared editor couldn't edit a plop that uses the owner's private plant"). The correct guard should compare against the plop's pre-patch PlantID (e.g. capture the original before applyPlantingPatch mutates pl, then compare *patch.PlantID != original), not merely check for a non-nil pointer.
Verified: applyPlantingPatch (line 124) mutates pl.PlantID before the check at line 129, which only tests presence of patch.PlantID, not a diff against the pre-patch value.
Verified no test coverage: grepped all PlantingPatch{ call sites in internal/service/plantings_test.go and internal/service/shares_test.go — none exercise PlantID at all, so this gap is untested.
Verified reachability: web/src/pages/GardenEditorPage.tsx:129-138's onPickPlant is the only frontend call site sending plantId (web/src/lib/objects.ts:314), and it unconditionally sends plantId on any picker selection with no guard against re-picking the already-assigned plant — so the gap is live today, not merely theoretical for future clients.
Both findings are confirmed against the actual code.
Verdict: Minor issues
Through the security lens, the ACL choke point (requireGardenRole → effectiveGardenRole → share lookup) is sound: owner is implicit via owner_id, non-owners resolve through garden_shares, and roleNone is masked to ErrNotFound so existence isn't leaked to strangers. All SQL uses parameterized placeholders; the shareColumns/qualifyColumns interpolation is over static, trusted constants — no injection. Role values are validated by isShareRole before reaching the store, so "owner"/arbitrary roles can't be persisted as a share. UpdatePlanting's relaxed plant check is safe: a plant can only be reassigned when patch.PlantID != nil, which still routes through visiblePlant and rejects another user's private plant — no privilege escalation. RemoveShare's self-leave path keys off actorID == targetUserID (session vs. path), so a user can only delete their own share; any cross-user attempt falls through to ErrNotFound, consistently masked whether or not the garden exists. Verified by reading internal/service/shares.go, internal/store/shares.go, internal/service/gardens.go, internal/service/plantings.go, and internal/api/errors.go.
Findings:
internal/service/shares.go:39 (AddShare) — authenticated account enumeration.GetUserByEmail returning domain.ErrNotFound is converted to ErrShareUserNotFound, which writeServiceError maps to a distinct 404 SHARE_USER_NOT_FOUND with message "no account with that email"; a valid target yields 409/201. Since any authenticated user can create a garden (becoming its owner) and then call AddShare on it, any logged-in user can probe arbitrary email addresses to learn whether they correspond to an existing account. This is a known tradeoff of share-by-email flows, but it is a real user-enumeration vector. If account privacy matters here, consider returning a generic success/queued response (or rate-limiting) instead of a distinct 404 for unknown emails. Verified against AddShare (line 37-40) and internal/api/errors.go:33.
internal/service/shares.go:76 (RemoveShare) — minor UX: owner-self-removal / no-share cases return ErrNotFound to the owner. Not a vulnerability (the owner is authorized), but the owner path doesn't pre-check that a share row exists for targetUserID; DeleteShare returns domain.ErrNotFound on 0 rows affected (confirmed in internal/store/shares.go:115-127), which writeServiceError maps to a generic 404. Purely a consistency/UX nit, not a security defect.
🎯 Correctness — No material issues found
I've verified all findings. Let me confirm the key claims:
Finding 6 check: The code at gardens.go:149-152 is:
The store (gardens.go:116-138) returns (current, ErrVersionConflict) on a version mismatch, where current is non-nil. So updated != nil is true on the conflict path, and MyRoleis stamped. The draft's claim that the conflict row is returned "without a role stamp" is false — drop.
Finding 2 check: GetPlant returns ErrNotFound for a missing plant (plants.go:100-101). But the plant_id FK is ON DELETE RESTRICT (migrations/0001_init.sql:126), so a referenced plant cannot be deleted while plantings exist — unreachable. Not a live bug.
Findings 1, 3, 4, 5 are all self-described as "correct / no issue / nuance / trivial" — non-issues.
Every finding is either a non-issue (explicitly confirmed correct by the draft itself) or, in the case of finding 6, contradicted by the actual code.
Verdict: No material issues found
The ACL choke point (requireGardenRole → effectiveGardenRole → GetShareRole) is sound. The role ranking (roleNone < roleViewer < roleEditor < roleOwner) is consistent, and UpdateGarden's tightening to roleOwner is correct. The ListGardensForActor UNION query emits consistent created_at/id/my_role columns across both branches (verified against gardenColumns and qualifyColumns). The UpdatePlanting visibility check correctly validates the changed-to plant (since applyPlantingPatch runs first at line 124). The UpdateGarden conflict path does carry MyRole (the if updated != nil guard covers the non-nil current row returned on ErrVersionConflict). The shares service error mapping (ErrShareUserNotFound→404, ErrCannotShareWithSelf→400, ErrShareExists→409) is consistent with the handler/store behavior.
🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the source. Confirming:
"owner" magic string: domain.RoleViewer/RoleEditor exist (domain.go:72-73), but RoleOwner does not. The literal "owner" appears in gardenRole.String() (service/gardens.go:39), the SQL 'owner' AS my_role (store/gardens.go:78), and the test own[0].MyRole != "owner" (service/shares_test.go:121). Note: the draft's claim about api/shares_test.go:51 is inaccurate (that line is a read full check, not an owner comparison — "owner" only appears in a comment at line 65 there), but the core finding holds across the three confirmed sites.
RemoveShare bypasses requireGardenRole: Confirmed at service/shares.go:66-77 — it calls s.store.GetGarden directly and does a manual g.OwnerID != actorID && actorID != targetUserID check, while ListShares/AddShare/UpdateShareRole all route through requireGardenRole(..., roleOwner). The divergence is intentional (self-leave case) and already commented, but the maintainability observation is accurate.
VERDICT: Minor issues
"owner" is a magic string with no constant, unlike viewer/editor.domain defines RoleViewer and RoleEditor (domain.go:72-73), but the owner role is a bare literal in three places that must stay in sync: gardenRole.String() returns "owner" (service/gardens.go:39), the ListGardensForActor SQL hardcodes 'owner' AS my_role (store/gardens.go:78), and the service test compares own[0].MyRole != "owner" (service/shares_test.go:121). If the canonical owner label ever changes, these diverge silently. Add a domain.RoleOwner = "owner" constant and use it in String() and the SQL (via a bind parameter or concatenation), mirroring the viewer/editor treatment.
RemoveShare bypasses the requireGardenRole choke point and re-implements the garden-fetch + owner check. service/shares.go:66-77 calls s.store.GetGarden directly and does g.OwnerID != actorID && actorID != targetUserID, whereas ListShares/AddShare/UpdateShareRole all gate through requireGardenRole(..., roleOwner). The divergence is justified (a recipient self-leaving isn't the owner, so an owner-role gate would reject them, and a viewer-role gate would still need OwnerID afterward), but it duplicates the "fetch garden, propagate ErrNotFound" logic that requireGardenRole already encapsulates and leaves RemoveShare as the one share op that doesn't route through the central ACL path. The existing comment explains the self-leave case; consider also noting that it intentionally skips requireGardenRole so the divergence isn't read as an oversight.
scanGardenWithRole near-duplicates scanGarden (store/gardens.go:15-37), differing only by the trailing &g.MyRole. This is the idiomatic Go cost of rows.Scan arg lists and is acceptable, but if more "garden + computed column" variants appear, a single scanner taking a variadic tail would curb the copy-paste. Trivial for now.
⚡ Performance — Minor issues
Verdict: Minor issues found
Through the performance lens, the sharing change is generally sound:
ListGardensForActor uses a single UNION ALL over two indexed branches (idx_gardens_owner, idx_garden_shares_user) — no per-row N+1, and the LIMIT ? backstop is preserved. Good.
ListSharesForGarden is one JOIN query backed by idx_garden_shares_garden. Good.
requireGardenRole's extra GetShareRole lookup is a single indexed point lookup (UNIQUE(garden_id, user_id)), and owners short-circuit before it. Acceptable per-request overhead; no N+1 because ListGardens resolves roles in the UNION query rather than calling requireGardenRole per row.
One avoidable duplicate query:
internal/service/plantings.go:129-134 — on the plant-change path, visiblePlant(ctx, actorID, pl.PlantID) already loads the plant (it calls s.store.GetPlant internally and returns *domain.Plant, confirmed at plantings.go:176-188), but its return value is discarded (if _, err := ...). Then line 134 unconditionally calls s.store.GetPlant(ctx, pl.PlantID) again purely to get SpacingCM for the derived count. So when a plop's plant is changed, the plant row is fetched twice by primary key instead of once. It's a non-hot path (changing a plop's plant is uncommon) and an indexed PK lookup, so impact is small, but it's a straightforwardly avoidable extra query. Suggested fix: capture the plant from visiblePlant (plant, err := s.visiblePlant(...)) and reuse it for derivedCount / SpacingCM on the change branch, falling back to the single GetPlant only on the no-change branch.
No other material performance regressions found in my lane.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
internal/service/plantings.go:134 — GetPlant after a plant-id change can return ErrNotFound and surface as a raw 404 where ErrInvalidInput (400) was intended. When patch.PlantID != nil, the code first calls s.visiblePlant(...) which maps a missing/invisible plant to ErrInvalidInput (line 130, confirmed at plantings.go:176-187). But visiblePlant already fetches the plant and returns it — that value is discarded (if _, err := ...), and then line 134 calls s.store.GetPlant(ctx, pl.PlantID) a second time. If the plant was deleted between the two calls (race), the second GetPlant returns domain.ErrNotFound, which the API maps to a generic NOT_FOUND (404) rather than a 400. Narrow TOCTOU window; low severity. Suggested fix: reuse the plant returned by visiblePlant instead of re-fetching, or wrap the second GetPlantErrNotFound as ErrInvalidInput.
internal/service/shares.go:71 — Owner removing a non-existent share / self-leave of a non-share returns ErrNotFound instead of 204. In RemoveShare, the guard is g.OwnerID != actorID && actorID != targetUserID (confirmed). The GetGarden lookup at line 67 masks the garden from strangers (good). But the owner path and the self-leave path both fall through to s.store.DeleteShare (line 76), which returns domain.ErrNotFound when 0 rows are affected (confirmed at internal/store/shares.go:125-126). So an owner removing a user who was never shared with — or a recipient "leaving" a garden they were never shared with — gets a 404 instead of the 204 the API intends (internal/api/shares.go:84). Minor; behavior is defensible but the only signal is the inline comment. No fix strictly required.
The sharing implementation is sound from a security perspective. The requireGardenRole choke point correctly masks existence (ErrNotFound for no access) and enforces the role hierarchy (owner > editor > viewer). Garden metadata updates are tightened to roleOwner, and share CRUD is restricted to the owner (with self-removal as the only recipient exception). All SQL queries in the new store code use parameterized placeholders (?) with no user-input concatenation. The ListGardensForActor union query is constructed from compile-time constants only.
🎯 Correctness — No material issues found
No material issues found. The sharing ACL logic is sound: requireGardenRole correctly falls back to garden_shares and masks existence (ErrNotFound) when the actor has no role, while UpdateGarden is correctly tightened to roleOwner per the matrix. RemoveShare bypasses requireGardenRole but still enforces the same policy (owner-or-self-only, everything else masked). The UpdatePlanting change correctly gates visiblePlant on PlantID mutation, allowing shared editors to edit non-plant fields of a plop that references an owner-private plant. Tests cover the full ACL matrix and all edge cases described.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/service/shares.go:67 — RemoveShare bypasses the requireGardenRole authorization chokepoint and does a raw s.store.GetGarden followed by a manual owner/self check. The service documents requireGardenRole as "THE place authorization for a garden is decided", and every other garden-scoped operation funnels through it. Going around it creates a maintenance hazard: future changes to that function (audit logging, rate limiting, extra checks) won't apply to share removal. The fix is to call requireGardenRole(ctx, actorID, gardenID, roleViewer) first, then apply the owner-or-self rule.
internal/store/gardens.go:78 — The ListGardensForActor query hardcodes the string literal 'owner' for the owner role ('owner' AS my_role), while the service layer derives the same value via roleOwner.String(). This duplicates the role-string definition across store and service layers; if the API contract changes, both places must be updated or they drift apart.
internal/service/gardens.go:151 — UpdateGarden hardcodes updated.MyRole = roleOwner.String() instead of reusing the role already computed and stamped by requireGardenRole. The returned garden from requireGardenRole is discarded (if _, err := ...), so the permission result is recalculated by hand. This is brittle: if the model ever allows non-owners through (co-owners, admin override), the response will falsely claim "owner". Capture the garden from requireGardenRole and use its MyRole.
⚡ Performance — Minor issues
Verdict: Minor issues
internal/store/shares.go:46 — ListSharesForGarden queries garden_shares joined with users but has no LIMIT clause, making the result set unbounded. ListGardensForActor (in the same PR area) explicitly caps at maxGardensListed = 1000 as a "defensive backstop against a pathologically large result"; the shares list should have the same defense. Even at normal sharing scales this is unlikely to be hit, but the endpoint should not be the only unbounded list query in the surface. Suggested fix: Add LIMIT ? parameterized with a constant like maxSharesListed = 1000 (or reuse maxGardensListed).
🧯 Error handling & edge cases — No material issues found
Both findings are confirmed against the actual code:
AddShare (line 30): unknown email → ErrShareUserNotFound → 404, known → CreateShare returns 201 (or 409 if ErrShareExists). Confirmed enumeration oracle, gated to authenticated garden owners.
RemoveShare (line 71): the branch g.OwnerID != actorID && actorID != targetUserID returns ErrNotFound for a participant removing another's share. Action correctly denied; status mislabel only.
VERDICT: Minor issues
internal/service/shares.go:30 (AddShare) — authenticated user-enumeration oracle. Any authenticated owner of a garden can probe whether an arbitrary email is a registered pansy account: calling POST /gardens/:id/shares with {"email": "<guess>", "role": "viewer"} returns 404 SHARE_USER_NOT_FOUND for an unknown email (mapping at internal/api/errors.go:33-34) vs. 201 (or 409 SHARE_EXISTS per errors.go:37-38) for a known one. The owner can then DELETE the share to clean up. Gated to authenticated garden owners (not anonymous), and the PR description notes v1 deliberately shares-by-existing-account rather than sending invitations, so it's a known tradeoff — but the enumeration implication isn't called out, and at an app with registration-closed/OIDC it could leak which identities have accounts. Suggested mitigations: a generic "invite sent"-style 202 regardless of whether the account exists (requires an out-of-band channel v1 lacks), per-actor rate-limiting of share-create attempts, or at minimum documenting the enumeration as accepted for v1.
internal/service/shares.go:71 (RemoveShare) — minor authz-response mislabel (not a vuln): a participant non-owner (viewer/editor) attempting to remove another user's share gets ErrNotFound (404) via the g.OwnerID != actorID && actorID != targetUserID branch, even though they already know the garden exists (they have access to it). The action is correctly denied, so there's no privilege issue; it's just a 404-where-403-would-fit, and the docstring's "existence isn't revealed to non-participants" rationale doesn't apply to participants. Suggested fix: treat the "actor has some access but isn't owner/self" case as ErrForbidden. Confidence low — this is arguably intended, and harmless.
🎯 Correctness — No material issues found
Verdict: No material issues found
I reviewed the change through the correctness lens, verifying against the checked-out code:
requireGardenRole / effectiveGardenRole (service/gardens.go:66-106): owner branch returns immediately; share lookup falls through to GetShareRole; unknown/corrupt share role collapses to roleNone → ErrNotFound (masked). The MyRole stamp happens only after the role/min gates pass, so roleNone.String() (the "" default) is never produced for a response. Verified.
gardenRole.String() (service/gardens.go:36-47): "owner" literal vs domain.RoleEditor/RoleViewer constants — both are "editor"/"viewer" (domain.go:72-73), matching the DB CHECK (role IN ('viewer','editor')) and the 'owner' AS my_role literal in the UNION. Consistent.
ListGardensForActor SQL (store/gardens.go:75-87): the outer SELECT gardenColumns, my_role FROM (...) ORDER BY created_at DESC, id DESC references the UNION's projected columns positionally; qualifyColumns("g", gardenColumns) re-aliases the second branch to match; both branches project 11 columns and scanGardenWithRole reads 11. Owner-can't-have-a-share-row invariant (enforced by ErrCannotShareWithSelf + UNIQUE) prevents duplicate rows. Verified.
UpdateGarden tightened to owner-only (service/gardens.go:139-154): requireGardenRole(roleOwner); the version-conflict path also gets MyRole="owner" via the if updated != nil guard. Correct.
UpdatePlanting plant-visibility change (service/plantings.go:124-137): applyPlantingPatch runs first, so pl.PlantID is the new id when patch.PlantID != nil; visiblePlant is then called on that new id — the gate is correct. The unconditional store.GetPlant afterward is only for derivedCount, and plants with plantings can't be deleted (ON DELETE RESTRICT → ErrPlantInUse), so a stale PlantID isn't a real path. Verified.
AddShare/UpdateShareRole/RemoveShare (service/shares.go): role validated by isShareRole (rejects "owner" and garbage); self-share rejected via target.ID == actorID after the email lookup; duplicate caught by the UNIQUE index → ErrShareExists. RemoveShare's g.OwnerID != actorID && actorID != targetUserID correctly admits only owner-or-self and masks everyone else with ErrNotFound. Verified.
writeServiceError mapping (api/errors.go:33-38): the three new sentinels map to 404/400/409 respectively, matching the PR description and the domain.Err* definitions. Verified.
ACL matrix test (service/shares_test.go): I traced each row — owner/editor/viewer/stranger × the six operations — against the implemented role ranking (roleNone < roleViewer < roleEditor < roleOwner) and the masking rule. Each wantErr expectation matches what the code produces (e.g. editor→ErrForbidden on UpdateGarden because roleEditor < roleOwner but roleEditor != roleNone; stranger→ErrNotFound because roleNone).
No correctness defects found in this lane.
🧹 Code cleanliness & maintainability — Minor issues
All five findings verified against the actual code. Each is confirmed:
gardens.go:39 returns "owner" literal; store/gardens.go:78 has 'owner' AS my_role literal; domain.go has RoleViewer/RoleEditor but no RoleOwner. Confirmed.
store/shares.go:61-64 inlines the scan; scanShare exists at lines 15-24; scanGardenWithRole shows the extending pattern. Confirmed.
service/shares.go:66-77 does its own GetGarden+OwnerID check, no requireGardenRole call, no comment. Confirmed.
service/shares.go:74-75 comment attributes the ErrNotFound property to the owner/self-leave split. Confirmed.
wantErr only in shares_test.go; 54 inline = %v, want assertions across other service test files. Confirmed.
VERDICT: Minor issues
internal/service/gardens.go:39 — "owner" role string is hardcoded in gardenRole.String() while the store SQL at internal/store/gardens.go:78 emits 'owner' AS my_role as a literal. There is no domain.RoleOwner constant (only RoleViewer/RoleEditor). If either side drifts, owned-garden MyRole silently mismatches. Fix: add domain.RoleOwner = "owner" and reference it in both places.
internal/store/shares.go:61 — ListSharesForGarden inlines the share-column scan instead of extending scanShare, breaking the file's scanX(scanner) helper pattern (scanGarden/scanShare/scanGardenWithRole all exist to keep column order in one place). The inline scan duplicates shareColumns order; if shareColumns is reordered, scanShare updates but this scan silently desyncs. Fix: a scanShareWithUser(s, email, displayName) that calls scanShare then scans the two extra columns, mirroring scanGardenWithRole.
internal/service/shares.go:66 — RemoveShare bypasses the requireGardenRole choke point with its own s.store.GetGarden + g.OwnerID != actorID check. Partly forced by the self-leave semantic (a recipient has no owner role), but there's no comment explaining the divergence, inviting a future reader to "fix" it. A one-line comment ("deliberately not requireGardenRole: a recipient self-leaving has no role to check") would prevent that.
internal/service/shares.go:74 — The comment's "missing row is ErrNotFound either way" is a property of DeleteShare (returns ErrNotFound on RowsAffected == 0), independent of the owner/self-leave split; the comment reads as if the property depends on the branch. Minor misleading wording.
internal/service/shares_test.go:12 — wantErr is local to shares_test.go but generally useful; the rest of the service suite re-implements the same t.Errorf("... = %v, want ...", err, ...) assertion inline (54 occurrences across plants_test.go, objects_test.go, gardens_test.go, plantings_test.go, auth_test.go). Promoting it to a shared testing.go would reduce churn.
⚡ Performance — No material issues found
I'll investigate this PR through the performance lens. Let me verify the key performance-sensitive paths.
Let me check GardenFull, UpdateObject, and UpdatePlanting to see if the new share-lookup call in requireGardenRole introduces N+1 issues on hot paths.
Let me check the schema for indexes on garden_shares (critical for the share-lookup query performance), since requireGardenRole now does that lookup on every garden operation.
The schema has appropriate indexes (UNIQUE on (garden_id, user_id) and separate single-column indexes). The GetShareRole lookup uses both columns which the UNIQUE index efficiently serves.
Let me verify one concern: in requireGardenRole, the share lookup happens on every garden operation, including the owner case after GetGarden. Wait — looking again, effectiveGardenRole short-circuits with if g.OwnerID == actorIDbefore the share query. Good. So owners don't pay the extra query.
But for non-owners on any garden operation (every plantings/objects update by a shared editor), each call to requireGardenRole now does 2 queries instead of 1: GetGarden + GetShareRole. And objectForRole → requireGardenRole. That's one extra indexed-point query per mutation. That's a reasonable, necessary cost for ACL enforcement — not a regression in the sense of avoidable work.
Let me check GardenFull for any concern — it calls requireGardenRole once then bulk-loads. Fine.
Let me check the ListGardensForActor SQL — the UNION ALL with the LIMIT outside. Since both subqueries are unbounded, the database must materialize both fully before applying LIMIT. For an owner with many gardens + many shares, that's two full scans combined. But both halves use indexed predicates (owner_id = ? and s.user_id = ?), and maxGardensListed=1000 caps it. This is fine at household scale, and the comment explicitly says so.
Let me check ListSharesForGarden — ORDER BY u.display_name COLLATE NOCASE, s.id — this requires a sort on the joined result. No index on users.display_name. But this is bounded by the number of shares per garden (small), so it's not a concern.
Let me verify there's no N+1 in the new UpdatePlanting flow: when patch.PlantID != nil, it now calls visiblePlant (which does GetPlant) AND then s.store.GetPlant again unconditionally at line 134. That's a duplicate plant fetch — let me look closer.
Confirmed: when patch.PlantID != nil, the same plant is fetched twice (once by visiblePlant, once at line 134). This was already the case before this PR (the old code called visiblePlant which calls GetPlant, then the code at line 134 also called GetPlant). Wait — let me check the original behavior. The diff shows the old code did plant, err := s.visiblePlant(ctx, actorID, pl.PlantID) unconditionally, and now line 134 calls s.store.GetPlant unconditionally. So before: 1 fetch always. After: 1 fetch always (line 134), plus an extra fetch only when PlantID is being changed. So this is a small new duplication only on plant-change operations.
This is a minor, avoidable extra query on a non-hot path (changing a plop's plant). Not material at household scale, but worth a one-line fix (reuse the plant returned by visiblePlant). Let me note it but as trivial.
Now let me check whether there's a more material concern: every mutation by a shared editor goes through requireGardenRole → GetGarden + GetShareRole. For UpdatePlanting/UpdateObject/DeletePlanting/DeleteObject, that's via objectForRole → requireGardenRole. One extra indexed single-row query per mutation. Necessary for correctness; not a regression worth flagging.
Let me check ListGardensForActor for one more concern — the UNION ALL with the outer LIMIT, and the inner SELECT ... 'owner' AS my_role FROM gardens WHERE owner_id = ? — there's no ORDER BY inside, so SQLite will produce both result sets and then sort/limit. With maxGardensListed=1000, fine. No material issue.
Let me verify the getGardenFull handler doesn't trigger per-object N+1.
No N+1 there. The GardenFull is well-batched.
Let me do a final check on whether the ListSharesForGarden query is called from a hot path. It's only called by ListShares (owner only, UI-driven). Not hot. Fine.
I've completed my performance review. Let me write up findings.
VERDICT: No material issues found
I reviewed the PR through the performance lens, examining:
requireGardenRole share lookup (service/gardens.go:66-83, service/gardens.go:87-106): Each garden-scoped operation now potentially issues a GetShareRole query. Verified that effectiveGardenRole short-circuits via if g.OwnerID == actorIDbefore the share query, so owners pay no extra cost. Non-owners pay one extra indexed single-row lookup (garden_shares has a UNIQUE on (garden_id, user_id), confirmed in migrations/0001_init.sql:66). That is the necessary cost of ACL enforcement on shared gardens, not a regression.
ListGardensForActor UNION ALL (store/gardens.go:75-87): The outer LIMIT requires materializing both halves, but each half uses an indexed predicate (owner_id / s.user_id) and the result is capped at maxGardensListed=1000. Appropriate at the stated household scale; the code comment explicitly defers real pagination to post-v1. Not material.
GardenFull (service/objects.go:161-189): Still single authorization check + three batched list queries + a map for DerivedCount. No N+1 introduced.
ListSharesForGarden (store/shares.go:45-73): UI-only path, bounded by shares-per-garden. The ORDER BY u.display_name COLLATE NOCASE sort is over a small set; no index needed. Not a hot path.
UpdatePlanting duplicate plant fetch (service/plantings.go:129-134): When patch.PlantID != nil, visiblePlant calls GetPlant and then line 134 calls GetPlant again on the same pl.PlantID. Pre-PR this fetched the plant once unconditionally via visiblePlant; post-PR it fetches once unconditionally (line 134) plus a second fetch only when the plant is being changed. This is one extra indexed single-row query on a non-hot mutation path (changing a plop's plant), trivially fixable by reusing the plant returned from visiblePlant. I consider this trivial — not material at household scale, and not on a hot loop. Mentioning only for completeness.
No blocking or minor performance issues in this lens; the duplicate-plant-fetch is the only thing I'd even note, and it's trivial.
🧯 Error handling & edge cases — No material issues found
I'll review this PR through the error handling & edge cases lens. Let me examine the actual code to verify the diff's behavior.
Looking through the error-handling & edge-cases lens, I verified the new share code paths, the UpdatePlanting visibility change, and the UpdateGarden/requireGardenRole role-stamping by reading the actual files and the schema.
VERDICT: No material issues found
I checked the unhappy paths the diff introduces and they hold up:
RemoveShare (service/shares.go:66-77): the g.OwnerID != actorID && actorID != targetUserID guard correctly masks existence for non-participants with ErrNotFound, while letting both the owner-remove-anyone and recipient-self-leave paths through. I confirmed a stranger gets ErrNotFound from GetGarden (garden missing) or from the guard (garden exists, no access) — same error, no existence leak. A recipient trying to remove another recipient's share also falls into the guard → ErrNotFound (the test covers the stranger variant; the logic is identical for a co-recipient).
AddShare (service/shares.go:30-50): ordering is correct — owner check before role validation (so a non-owner probing with a bad role gets ErrForbidden, not ErrInvalidInput); GetUserByEmail's ErrNotFound is mapped to ErrShareUserNotFound; the UNIQUE race is caught by isUniqueViolation → ErrShareExists in store.CreateShare (store/shares.go:86), so two concurrent AddShare calls for the same pair can't both succeed.
UpdatePlanting (service/plantings.go:124-137): the visibility re-check is correctly gated on patch.PlantID != nil (after applyPlantingPatch has already updated pl.PlantID), so a shared editor editing radius/label on the owner's private-plant plop no longer fails; the unconditional s.store.GetPlant for the derived count is safe because the plantings → plants FK is ON DELETE RESTRICT (verified in migrations/0001_init.sql:126), so a referenced plant always exists. Errors from GetPlant/visiblePlant are propagated, not swallowed.
UpdateGarden (service/gardens.go:149-153): the if updated != nil guard correctly stamps MyRole on both the success row and the version-conflict current row returned by store.UpdateGarden (store/gardens.go:128-133), and correctly skips stamping on the (nil, ErrNotFound) "garden deleted mid-flight" path. No error is swallowed.
ListSharesForGarden / ListGardensForActor (store/shares.go:56, store/gardens.go:91): both defer rows.Close() and check rows.Err() after iteration, matching the existing pattern. Empty results return non-nil []domain.{ShareWithUser,Garden}{} (not JSON null), which the API test asserts.
GetShareRole (store/shares.go:30-41): sql.ErrNoRows → ( "", false, nil ) is cleanly distinguished from a real DB error; effectiveGardenRole's default branch collapses a corrupt role string to roleNone → ErrNotFound, so a bad DB row can't elevate privilege.
The ListGardensForActor UNION ALL can't double-count a garden for its owner, because AddShare's target.ID == actorID guard (ErrCannotShareWithSelf) prevents an owner from ever having a share row on their own garden — verified the self-share guard is the only insert path.
No swallowed errors, no missing defer, no panics on nil/empty/zero input, and the masked-existence contract (ErrNotFound for both missing and forbidden) is preserved across every new branch.
opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
RemoveShare bypasses the requireGardenRole authorization choke point, allowing an actor with no role on a garden to reach store.DeleteShare. internal/service/shares.go:66 opens the garden with a raw store.GetGarden instead of funneling through requireGardenRole — the comment on requireGardenRole calls it “THE place authorization for a garden is decided.” A stranger can call RemoveShare with targetUserID == actorID; the ad-hoc check g.OwnerID != actorID && actorID != targetUserID passes, so the stranger reaches DeleteShare. While the parameterized query prevents SQL injection and only the actor’s own (non-existent) row would be targeted, the stranger has still bypassed the choke point and can distinguish garden existence via timing (for an existing garden GetGarden succeeds before DeleteShare is reached; for a missing garden GetGarden fails immediately). More importantly, it breaks the architectural invariant that every garden-touching operation routes through requireGardenRole. Fix: Add g, err := s.requireGardenRole(ctx, actorID, gardenID, roleViewer) at the top of RemoveShare; then keep the existing owner-or-self check. roleViewer is the right minimum because both owners (who may remove anyone) and shared viewers/editors (who may self-leave) must satisfy it.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The sharing backend code is clean, consistent with existing patterns, and well-documented. A few notes:
The wantErr helper in internal/service/shares_test.go:12 duplicates the same helper pattern used in other test files, but is isolated to this new file and fits the local style.
scanGardenWithRole in internal/store/gardens.go:28 is a clear, minimal extension of scanGarden that follows the established scanner pattern.
The gardenRole.String() method at internal/service/gardens.go:36 cleanly centralizes role string rendering rather than sprinkling string literals.
Error mapping in internal/api/errors.go and route registration in internal/api/api.go follow the existing conventions exactly.
⚡ Performance — No material issues found
No material issues found
🧯 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: Blocking issues found** · 13 findings (4 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | RemoveShare bypasses requireGardenRole authorization choke point | `internal/service/shares.go:66` | 5/5 | maintainability, security |
| 🟡 | "owner" role string duplicated between service String() and store SQL literal; no domain.RoleOwner constant | `internal/service/gardens.go:39` | 2/5 | maintainability |
| 🟡 | Redundant duplicate GetPlant query in UpdatePlanting when the plant is changed | `internal/service/plantings.go:134` | 2/5 | error-handling, performance |
| 🟡 | Owner removing a non-existent share returns ErrNotFound (404) instead of 204; behavior is defensible but undocumented beyond a comment | `internal/service/shares.go:71` | 2/5 | error-handling, maintainability, security |
<details><summary>9 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Plant-visibility re-check triggers on patch field presence, not actual value change, defeating the shared-editor fix for a no-op plantId resend | `internal/service/plantings.go:129` | claude-code/sonnet | error-handling, maintainability |
| 🟠 | ListSharesForGarden is unbounded — missing LIMIT clause | `internal/store/shares.go:46` | kimi-k2.6:cloud | performance |
| 🟡 | "owner" role is a magic string duplicated across SQL/service/tests with no RoleOwner constant (viewer/editor have one) | `internal/domain/domain.go:72` | glm-5.2:cloud | maintainability |
| 🟡 | UpdateGarden hardcodes roleOwner.String() instead of reusing requireGardenRole result | `internal/service/gardens.go:151` | kimi-k2.6:cloud | maintainability |
| 🟡 | AddShare exposes an authenticated user-enumeration oracle (unknown email → 404, known → 201/409) | `internal/service/shares.go:30` | opencode/glm-5.2:cloud | security |
| 🟡 | AddShare 404 for unknown email enables authenticated account enumeration | `internal/service/shares.go:39` | glm-5.2:cloud | security |
| 🟡 | SQL hardcodes 'owner' role string duplicating service-layer gardenRole.String() | `internal/store/gardens.go:78` | kimi-k2.6:cloud | maintainability |
| 🟡 | ListSharesForGarden inlines the share-column scan, duplicating shareColumns order instead of extending scanShare | `internal/store/shares.go:61` | opencode/glm-5.2:cloud | maintainability |
| ⚪ | wantErr helper is local to shares_test.go but generally useful; other tests re-implement the same assertion inline | `internal/service/shares_test.go:12` | opencode/glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Good — sharing routes sit under `gardens` group with `h.requireAuth()` and CSRF guard applies globally. `qualifyColumns` only operates on the hardcoded `gardenColumns`/`shareColumns` constants, not user input, so no injection risk. Everything else checks out: parameterized queries throughout, DB-level `CHECK (role IN ('viewer','editor'))` backs the app-layer `isShareRole` validation, email lookup is case-insensitive by design, and the `visiblePlant` bypass in `UpdatePlanting` doesn't newly disclose anything — `GardenFull` already returns all referenced plants to any garden viewer/editor via `ListReferencedPlants`, unrelated to this diff.
I'm satisfied this diff has no material security issues.
## VERDICT: No material issues found
I reviewed this diff specifically for authn/authz gaps, injection, IDOR, and information disclosure:
- **Authorization funnel intact**: every new share endpoint routes through `requireGardenRole`/explicit owner checks in `internal/service/shares.go`. Verified `ListShares`/`AddShare`/`UpdateShareRole` require `roleOwner`, and `RemoveShare`'s manual owner-or-self check (`internal/service/shares.go:68-72`) can't be bypassed — a stranger passing their own ID as target just hits a real "no such share" 404 rather than an authorization bypass (traced through `DeleteShare`'s `RowsAffected` check in `internal/store/shares.go`).
- **SQL injection**: all new queries (`internal/store/shares.go`, `internal/store/gardens.go`) use parameterized `?` placeholders. `qualifyColumns` (`internal/store/sqlite.go:105`) is only ever applied to the hardcoded `gardenColumns`/`shareColumns` constants, never to request input.
- **Role escalation**: `isShareRole` restricts grantable roles to viewer/editor (owner can't be granted via a share), and this is backed by a DB `CHECK (role IN ('viewer','editor'))` constraint (`internal/store/migrations/0001_init.sql:61`) — defense in depth.
- **Email targeting for `AddShare`**: verified `GetUserByEmail` is case-insensitive by column collation (comment + implementation in `internal/store/users.go:94-98`), so no case-mismatch bypass; self-share is checked by user ID after lookup, not by string comparison, so case games don't matter.
- **`UpdatePlanting` visibility bypass** (skipping `visiblePlant` when `patch.PlantID` is nil): checked whether this newly leaks a private plant's data to a shared editor. It does not — `GardenFull` (`internal/service/objects.go:161-183`, unchanged by this PR) already returns every plant referenced anywhere in the garden via `ListReferencedPlants` to any viewer/editor, so the plant's data was already exposed to garden participants regardless of this patch.
- **Routes are behind auth + CSRF**: the new `/shares` routes are registered inside the `gardens` group which already carries `h.requireAuth()`, and the whole `v1` group carries `h.csrfGuard()` (`internal/api/api.go:47,72-88`).
Nothing else in this diff (error mapping in `internal/api/errors.go`, the `MyRole` computed field, the `ShareWithUser` join) introduces new untrusted-input handling or a new trust boundary.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
## Verdict: No material issues found
I reviewed the sharing ACL logic (`requireGardenRole`/`effectiveGardenRole`), the new `garden_shares` CRUD, the `ListGardensForActor` UNION query, and the `UpdatePlanting` plant-visibility change, cross-checking each against the actual checked-out files (`internal/service/gardens.go`, `internal/service/plantings.go`, `internal/service/shares.go`, `internal/store/gardens.go`, `internal/store/shares.go`, `internal/api/errors.go`, `internal/api/api.go`, and the `garden_shares` schema in `migrations/0001_init.sql`).
Specific things I verified rather than assumed:
- `gardenRole` ordering (`roleNone < roleViewer < roleEditor < roleOwner`) makes the `role < min` comparisons in `requireGardenRole` correct, and `effectiveGardenRole` correctly special-cases the owner (never a `garden_shares` row, matching the schema comment) before falling back to `GetShareRole`.
- Every mutation path (`objectForRole`, `UpdatePlanting`, `DeleteGarden`, `GardenFull`) funnels through `requireGardenRole`, so shares are honored everywhere, not just on the garden read path — no bypass found.
- `UpdatePlanting`'s new "only re-check plant visibility when `patch.PlantID != nil`" gate correctly uses the pointer-nil convention already used by every other field in `PlantingPatch`/`plantingUpdateRequest` (`internal/api/plantings.go:40`), so a client that omits `plantId` from the JSON body won't trip the visibility check on an unrelated edit — this isn't a loophole, it's consistent with the existing partial-PATCH contract, and it's covered by `TestGardenACLMatrix`'s `mutPlop/editor` case.
- `ListGardensForActor`'s UNION ALL query: both branches project the same 11 columns in the same order, the outer `ORDER BY created_at, id` binds to the first branch's column names (valid SQLite UNION column-naming rule), and a user can never appear in both branches simultaneously since `AddShare` explicitly rejects self-sharing (`ErrCannotShareWithSelf`) and owners never get a share row.
- `RemoveShare`'s participant check (`g.OwnerID != actorID && actorID != targetUserID` → masked `ErrNotFound`) correctly allows owner-removes-anyone and self-leave while blocking a non-participant from removing someone else's share, matching `TestUpdateAndRemoveShare`'s "stranger remove" case.
- `CreateShare`'s duplicate-share handling maps to the actual `UNIQUE (garden_id, user_id)` constraint in the schema — no other unique constraint on that table, so `isUniqueViolation` can't misattribute a different failure.
- `isShareRole` correctly restricts `AddShare`/`UpdateShareRole` to `viewer`/`editor`, blocking privilege escalation to an `owner` share row.
- Error-mapping order in `errors.go` is irrelevant here since the new sentinels are independent (not wrapped) errors, so placement in the `switch` doesn't cause a case to be shadowed.
One minor observation that I'm not raising as a finding since it doesn't produce incorrect behavior against any stated requirement: `UpdateShareRole` (store) increments `version` but never checks a caller-supplied version, unlike every other mutable resource's stated "sync contract" (see the comment on `store.UpdateGarden`). That's a design inconsistency (no lost-update protection for concurrent share edits), not a logic bug — shares are owner-only mutations, low contention, and nothing in the PR's acceptance criteria requires OCC on shares.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Confirmed: `visiblePlant` internally calls `s.store.GetPlant` and returns that same plant — so when `patch.PlantID != nil`, `UpdatePlanting` truly fetches the row twice. All three draft findings verified against the actual code.
VERDICT: Minor issues
- `internal/service/plantings.go:129-137` — `UpdatePlanting` discards the `*domain.Plant` returned by `s.visiblePlant` (via `if _, err := s.visiblePlant(...)`) when `patch.PlantID != nil`, then unconditionally re-fetches the same row via `s.store.GetPlant(ctx, pl.PlantID)` at line 134 just to get the spacing for the derived count. `CreatePlanting` (same file, lines 82/108) shows the intended pattern of capturing and reusing the plant `visiblePlant` returns. This is a wasted DB round-trip whenever a planting's plant is being changed.
- `internal/service/shares.go:66-77` — `RemoveShare` bypasses the `requireGardenRole` choke point and instead calls `s.store.GetGarden` directly (confirmed via grep: this is the only other call site of `s.store.GetGarden` in the service package besides `requireGardenRole` itself, at `gardens.go:67`), hand-rolling its own "load or NotFound" + owner-or-self check. This works against the file's own stated design — the comment on `requireGardenRole` in `gardens.go:58-65` calls it "THE place authorization for a garden is decided… so neither can skip a check." Could be written as `g, err := s.requireGardenRole(ctx, actorID, gardenID, roleViewer)` followed by the owner-or-self check, keeping a single source of truth for the "load garden, mask as NotFound" step.
- `internal/service/gardens.go:36-47` — `gardenRole.String()` sources the editor/viewer strings from `domain.RoleEditor`/`domain.RoleViewer` but hardcodes the literal `"owner"` at line 39 instead of a matching constant (confirmed via grep of `internal/domain/domain.go`: only `RoleViewer`/`RoleEditor` are defined, no `RoleOwner`). Minor inconsistency; low impact since it's centralized in this one method.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
All claims verified against actual source.
VERDICT: Minor issues
- `internal/service/plantings.go:129-137` — In `UpdatePlanting`, when `patch.PlantID != nil`, the code calls `s.visiblePlant(ctx, actorID, pl.PlantID)` (which internally calls `s.store.GetPlant`, `plantings.go:177`) purely to validate visibility and discards the returned `*domain.Plant`, then unconditionally calls `s.store.GetPlant(ctx, pl.PlantID)` again at line 134 to get the plant for the derived-count calculation. That's two identical `SELECT` queries against `plants` for the same `plantID` on the plant-changed path, where `CreatePlanting` (line 82) fetches once and reuses the result. Low absolute cost (one extra indexed single-row lookup per planting update), but avoidable.
- Suggested fix: reuse the `*domain.Plant` returned by `visiblePlant` when `patch.PlantID != nil` instead of re-fetching via `s.store.GetPlant`.
Everything else in this diff checked out fine from a performance standpoint:
- `ListGardensForActor`'s UNION query (`internal/store/gardens.go:75`) is backed by `idx_gardens_owner` and `idx_garden_shares_user` (confirmed in `internal/store/migrations/0001_init.sql:53,69`).
- `GetShareRole` (`internal/store/shares.go:30`) used in `effectiveGardenRole` is a single indexed point lookup per non-owner permission check, not called in a loop.
- `email` has a `UNIQUE COLLATE NOCASE` index (`migrations/0001_init.sql:16`) and `garden_shares` has `UNIQUE (garden_id, user_id)` (`migrations/0001_init.sql:66`), so `AddShare`'s lookup/uniqueness checks are indexed.
- `ListSharesForGarden` (`internal/store/shares.go:45`) is a single joined query, no N+1.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
This matches the diff; nothing new to flag. The verification is complete — the single finding in the draft holds up fully against the code.
## Review — 🧯 Error handling & edge cases
**VERDICT: Minor issues**
- **`internal/service/plantings.go:129-133`** — The "only re-check plant visibility when the plant is *changed*" guard checks whether `patch.PlantID` is *present*, not whether the value actually *differs* from the planting's current `PlantID`:
```go
if patch.PlantID != nil {
if _, err := s.visiblePlant(ctx, actorID, pl.PlantID); err != nil {
return nil, err
}
}
```
If a caller sends `plantId` in the PATCH body equal to the plop's existing plant (e.g. a user re-opens a plant picker and re-selects the currently-assigned plant, or a future/alternate client resends the full object), this still runs `visiblePlant`, which fails with `ErrInvalidInput` for a shared editor working with the owner's private plant — exactly the scenario this PR says it fixed ("otherwise a shared editor couldn't edit a plop that uses the owner's private plant"). The correct guard should compare against the plop's pre-patch `PlantID` (e.g. capture the original before `applyPlantingPatch` mutates `pl`, then compare `*patch.PlantID != original`), not merely check for a non-nil pointer.
- Verified: `applyPlantingPatch` (line 124) mutates `pl.PlantID` before the check at line 129, which only tests presence of `patch.PlantID`, not a diff against the pre-patch value.
- Verified no test coverage: grepped all `PlantingPatch{` call sites in `internal/service/plantings_test.go` and `internal/service/shares_test.go` — none exercise `PlantID` at all, so this gap is untested.
- Verified reachability: `web/src/pages/GardenEditorPage.tsx:129-138`'s `onPickPlant` is the only frontend call site sending `plantId` (`web/src/lib/objects.ts:314`), and it unconditionally sends `plantId` on any picker selection with no guard against re-picking the already-assigned plant — so the gap is live today, not merely theoretical for future clients.
</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> — Minor issues</summary>
Both findings are confirmed against the actual code.
**Verdict: Minor issues**
Through the security lens, the ACL choke point (`requireGardenRole` → `effectiveGardenRole` → share lookup) is sound: owner is implicit via `owner_id`, non-owners resolve through `garden_shares`, and `roleNone` is masked to `ErrNotFound` so existence isn't leaked to strangers. All SQL uses parameterized placeholders; the `shareColumns`/`qualifyColumns` interpolation is over static, trusted constants — no injection. Role values are validated by `isShareRole` before reaching the store, so `"owner"`/arbitrary roles can't be persisted as a share. `UpdatePlanting`'s relaxed plant check is safe: a plant can only be *reassigned* when `patch.PlantID != nil`, which still routes through `visiblePlant` and rejects another user's private plant — no privilege escalation. `RemoveShare`'s self-leave path keys off `actorID == targetUserID` (session vs. path), so a user can only delete their own share; any cross-user attempt falls through to `ErrNotFound`, consistently masked whether or not the garden exists. Verified by reading `internal/service/shares.go`, `internal/store/shares.go`, `internal/service/gardens.go`, `internal/service/plantings.go`, and `internal/api/errors.go`.
Findings:
- `internal/service/shares.go:39` (`AddShare`) — **authenticated account enumeration.** `GetUserByEmail` returning `domain.ErrNotFound` is converted to `ErrShareUserNotFound`, which `writeServiceError` maps to a distinct 404 `SHARE_USER_NOT_FOUND` with message "no account with that email"; a valid target yields 409/201. Since any authenticated user can create a garden (becoming its owner) and then call `AddShare` on it, *any* logged-in user can probe arbitrary email addresses to learn whether they correspond to an existing account. This is a known tradeoff of share-by-email flows, but it is a real user-enumeration vector. If account privacy matters here, consider returning a generic success/queued response (or rate-limiting) instead of a distinct 404 for unknown emails. Verified against `AddShare` (line 37-40) and `internal/api/errors.go:33`.
- `internal/service/shares.go:76` (`RemoveShare`) — **minor UX: owner-self-removal / no-share cases return `ErrNotFound` to the owner.** Not a vulnerability (the owner is authorized), but the owner path doesn't pre-check that a share row exists for `targetUserID`; `DeleteShare` returns `domain.ErrNotFound` on 0 rows affected (confirmed in `internal/store/shares.go:115-127`), which `writeServiceError` maps to a generic 404. Purely a consistency/UX nit, not a security defect.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified all findings. Let me confirm the key claims:
**Finding 6 check**: The code at gardens.go:149-152 is:
```go
updated, err := s.store.UpdateGarden(ctx, g)
if updated != nil {
updated.MyRole = roleOwner.String()
}
```
The store (gardens.go:116-138) returns `(current, ErrVersionConflict)` on a version mismatch, where `current` is **non-nil**. So `updated != nil` is true on the conflict path, and `MyRole` **is** stamped. The draft's claim that the conflict row is returned "without a role stamp" is **false** — drop.
**Finding 2 check**: GetPlant returns `ErrNotFound` for a missing plant (plants.go:100-101). But the `plant_id` FK is `ON DELETE RESTRICT` (migrations/0001_init.sql:126), so a referenced plant cannot be deleted while plantings exist — unreachable. Not a live bug.
**Findings 1, 3, 4, 5** are all self-described as "correct / no issue / nuance / trivial" — non-issues.
Every finding is either a non-issue (explicitly confirmed correct by the draft itself) or, in the case of finding 6, contradicted by the actual code.
**Verdict: No material issues found**
The ACL choke point (`requireGardenRole` → `effectiveGardenRole` → `GetShareRole`) is sound. The role ranking (`roleNone < roleViewer < roleEditor < roleOwner`) is consistent, and `UpdateGarden`'s tightening to `roleOwner` is correct. The `ListGardensForActor` UNION query emits consistent `created_at`/`id`/`my_role` columns across both branches (verified against `gardenColumns` and `qualifyColumns`). The `UpdatePlanting` visibility check correctly validates the changed-to plant (since `applyPlantingPatch` runs first at line 124). The `UpdateGarden` conflict path does carry `MyRole` (the `if updated != nil` guard covers the non-nil `current` row returned on `ErrVersionConflict`). The shares service error mapping (`ErrShareUserNotFound`→404, `ErrCannotShareWithSelf`→400, `ErrShareExists`→409) is consistent with the handler/store behavior.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings verified against the source. Confirming:
1. **`"owner"` magic string**: `domain.RoleViewer`/`RoleEditor` exist (domain.go:72-73), but `RoleOwner` does not. The literal `"owner"` appears in `gardenRole.String()` (service/gardens.go:39), the SQL `'owner' AS my_role` (store/gardens.go:78), and the test `own[0].MyRole != "owner"` (service/shares_test.go:121). Note: the draft's claim about api/shares_test.go:51 is inaccurate (that line is a `read full` check, not an owner comparison — "owner" only appears in a comment at line 65 there), but the core finding holds across the three confirmed sites.
2. **`RemoveShare` bypasses `requireGardenRole`**: Confirmed at service/shares.go:66-77 — it calls `s.store.GetGarden` directly and does a manual `g.OwnerID != actorID && actorID != targetUserID` check, while `ListShares`/`AddShare`/`UpdateShareRole` all route through `requireGardenRole(..., roleOwner)`. The divergence is intentional (self-leave case) and already commented, but the maintainability observation is accurate.
**VERDICT: Minor issues**
- **`"owner"` is a magic string with no constant, unlike `viewer`/`editor`.** `domain` defines `RoleViewer` and `RoleEditor` (domain.go:72-73), but the owner role is a bare literal in three places that must stay in sync: `gardenRole.String()` returns `"owner"` (service/gardens.go:39), the `ListGardensForActor` SQL hardcodes `'owner' AS my_role` (store/gardens.go:78), and the service test compares `own[0].MyRole != "owner"` (service/shares_test.go:121). If the canonical owner label ever changes, these diverge silently. Add a `domain.RoleOwner = "owner"` constant and use it in `String()` and the SQL (via a bind parameter or concatenation), mirroring the viewer/editor treatment.
- **`RemoveShare` bypasses the `requireGardenRole` choke point and re-implements the garden-fetch + owner check.** service/shares.go:66-77 calls `s.store.GetGarden` directly and does `g.OwnerID != actorID && actorID != targetUserID`, whereas `ListShares`/`AddShare`/`UpdateShareRole` all gate through `requireGardenRole(..., roleOwner)`. The divergence is justified (a recipient self-leaving isn't the owner, so an owner-role gate would reject them, and a viewer-role gate would still need `OwnerID` afterward), but it duplicates the "fetch garden, propagate `ErrNotFound`" logic that `requireGardenRole` already encapsulates and leaves `RemoveShare` as the one share op that doesn't route through the central ACL path. The existing comment explains the self-leave case; consider also noting that it intentionally skips `requireGardenRole` so the divergence isn't read as an oversight.
- **`scanGardenWithRole` near-duplicates `scanGarden`** (store/gardens.go:15-37), differing only by the trailing `&g.MyRole`. This is the idiomatic Go cost of `rows.Scan` arg lists and is acceptable, but if more "garden + computed column" variants appear, a single scanner taking a variadic tail would curb the copy-paste. Trivial for now.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Verdict: Minor issues found**
Through the performance lens, the sharing change is generally sound:
- `ListGardensForActor` uses a single `UNION ALL` over two indexed branches (`idx_gardens_owner`, `idx_garden_shares_user`) — no per-row N+1, and the `LIMIT ?` backstop is preserved. Good.
- `ListSharesForGarden` is one JOIN query backed by `idx_garden_shares_garden`. Good.
- `requireGardenRole`'s extra `GetShareRole` lookup is a single indexed point lookup (`UNIQUE(garden_id, user_id)`), and owners short-circuit before it. Acceptable per-request overhead; no N+1 because `ListGardens` resolves roles in the UNION query rather than calling `requireGardenRole` per row.
One avoidable duplicate query:
- `internal/service/plantings.go:129-134` — on the plant-change path, `visiblePlant(ctx, actorID, pl.PlantID)` already loads the plant (it calls `s.store.GetPlant` internally and returns `*domain.Plant`, confirmed at `plantings.go:176-188`), but its return value is discarded (`if _, err := ...`). Then line 134 unconditionally calls `s.store.GetPlant(ctx, pl.PlantID)` again purely to get `SpacingCM` for the derived count. So when a plop's plant is changed, the plant row is fetched twice by primary key instead of once. It's a non-hot path (changing a plop's plant is uncommon) and an indexed PK lookup, so impact is small, but it's a straightforwardly avoidable extra query. Suggested fix: capture the plant from `visiblePlant` (`plant, err := s.visiblePlant(...)`) and reuse it for `derivedCount` / `SpacingCM` on the change branch, falling back to the single `GetPlant` only on the no-change branch.
No other material performance regressions found in my lane.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**VERDICT: Minor issues**
- `internal/service/plantings.go:134` — **`GetPlant` after a plant-id change can return `ErrNotFound` and surface as a raw 404 where `ErrInvalidInput` (400) was intended.** When `patch.PlantID != nil`, the code first calls `s.visiblePlant(...)` which maps a missing/invisible plant to `ErrInvalidInput` (line 130, confirmed at `plantings.go:176-187`). But `visiblePlant` already fetches the plant and returns it — that value is discarded (`if _, err := ...`), and then line 134 calls `s.store.GetPlant(ctx, pl.PlantID)` a second time. If the plant was deleted between the two calls (race), the second `GetPlant` returns `domain.ErrNotFound`, which the API maps to a generic `NOT_FOUND` (404) rather than a 400. Narrow TOCTOU window; low severity. Suggested fix: reuse the plant returned by `visiblePlant` instead of re-fetching, or wrap the second `GetPlant` `ErrNotFound` as `ErrInvalidInput`.
- `internal/service/shares.go:71` — **Owner removing a non-existent share / self-leave of a non-share returns `ErrNotFound` instead of 204.** In `RemoveShare`, the guard is `g.OwnerID != actorID && actorID != targetUserID` (confirmed). The `GetGarden` lookup at line 67 masks the garden from strangers (good). But the owner path and the self-leave path both fall through to `s.store.DeleteShare` (line 76), which returns `domain.ErrNotFound` when 0 rows are affected (confirmed at `internal/store/shares.go:125-126`). So an owner removing a user who was never shared with — or a recipient "leaving" a garden they were never shared with — gets a 404 instead of the 204 the API intends (`internal/api/shares.go:84`). Minor; behavior is defensible but the only signal is the inline comment. No fix strictly required.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found.
The sharing implementation is sound from a security perspective. The `requireGardenRole` choke point correctly masks existence (`ErrNotFound` for no access) and enforces the role hierarchy (`owner` > `editor` > `viewer`). Garden metadata updates are tightened to `roleOwner`, and share CRUD is restricted to the owner (with self-removal as the only recipient exception). All SQL queries in the new store code use parameterized placeholders (`?`) with no user-input concatenation. The `ListGardensForActor` union query is constructed from compile-time constants only.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found. The sharing ACL logic is sound: `requireGardenRole` correctly falls back to `garden_shares` and masks existence (`ErrNotFound`) when the actor has no role, while `UpdateGarden` is correctly tightened to `roleOwner` per the matrix. `RemoveShare` bypasses `requireGardenRole` but still enforces the same policy (owner-or-self-only, everything else masked). The `UpdatePlanting` change correctly gates `visiblePlant` on `PlantID` mutation, allowing shared editors to edit non-plant fields of a plop that references an owner-private plant. Tests cover the full ACL matrix and all edge cases described.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `internal/service/shares.go:67` — `RemoveShare` bypasses the `requireGardenRole` authorization chokepoint and does a raw `s.store.GetGarden` followed by a manual owner/self check. The service documents `requireGardenRole` as "THE place authorization for a garden is decided", and every other garden-scoped operation funnels through it. Going around it creates a maintenance hazard: future changes to that function (audit logging, rate limiting, extra checks) won't apply to share removal. The fix is to call `requireGardenRole(ctx, actorID, gardenID, roleViewer)` first, then apply the owner-or-self rule.
- `internal/store/gardens.go:78` — The `ListGardensForActor` query hardcodes the string literal `'owner'` for the owner role (`'owner' AS my_role`), while the service layer derives the same value via `roleOwner.String()`. This duplicates the role-string definition across store and service layers; if the API contract changes, both places must be updated or they drift apart.
- `internal/service/gardens.go:151` — `UpdateGarden` hardcodes `updated.MyRole = roleOwner.String()` instead of reusing the role already computed and stamped by `requireGardenRole`. The returned garden from `requireGardenRole` is discarded (`if _, err := ...`), so the permission result is recalculated by hand. This is brittle: if the model ever allows non-owners through (co-owners, admin override), the response will falsely claim `"owner"`. Capture the garden from `requireGardenRole` and use its `MyRole`.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Verdict:** Minor issues
- **`internal/store/shares.go:46`** — `ListSharesForGarden` queries `garden_shares` joined with `users` but has no `LIMIT` clause, making the result set unbounded. `ListGardensForActor` (in the same PR area) explicitly caps at `maxGardensListed = 1000` as a "defensive backstop against a pathologically large result"; the shares list should have the same defense. Even at normal sharing scales this is unlikely to be hit, but the endpoint should not be the only unbounded list query in the surface.
**Suggested fix:** Add `LIMIT ?` parameterized with a constant like `maxSharesListed = 1000` (or reuse `maxGardensListed`).
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
Both findings are confirmed against the actual code:
1. `AddShare` (line 30): unknown email → `ErrShareUserNotFound` → 404, known → `CreateShare` returns 201 (or 409 if `ErrShareExists`). Confirmed enumeration oracle, gated to authenticated garden owners.
2. `RemoveShare` (line 71): the branch `g.OwnerID != actorID && actorID != targetUserID` returns `ErrNotFound` for a participant removing another's share. Action correctly denied; status mislabel only.
**VERDICT: Minor issues**
- `internal/service/shares.go:30` (`AddShare`) — **authenticated user-enumeration oracle.** Any authenticated owner of a garden can probe whether an arbitrary email is a registered pansy account: calling `POST /gardens/:id/shares` with `{"email": "<guess>", "role": "viewer"}` returns `404 SHARE_USER_NOT_FOUND` for an unknown email (mapping at `internal/api/errors.go:33-34`) vs. `201` (or `409 SHARE_EXISTS` per `errors.go:37-38`) for a known one. The owner can then `DELETE` the share to clean up. Gated to authenticated garden owners (not anonymous), and the PR description notes v1 deliberately shares-by-existing-account rather than sending invitations, so it's a known tradeoff — but the enumeration implication isn't called out, and at an app with registration-closed/OIDC it could leak which identities have accounts. Suggested mitigations: a generic "invite sent"-style 202 regardless of whether the account exists (requires an out-of-band channel v1 lacks), per-actor rate-limiting of share-create attempts, or at minimum documenting the enumeration as accepted for v1.
- `internal/service/shares.go:71` (`RemoveShare`) — minor authz-response mislabel (not a vuln): a *participant* non-owner (viewer/editor) attempting to remove another user's share gets `ErrNotFound` (404) via the `g.OwnerID != actorID && actorID != targetUserID` branch, even though they already know the garden exists (they have access to it). The action is correctly denied, so there's no privilege issue; it's just a 404-where-403-would-fit, and the docstring's "existence isn't revealed to non-participants" rationale doesn't apply to participants. Suggested fix: treat the "actor has some access but isn't owner/self" case as `ErrForbidden`. Confidence low — this is arguably intended, and harmless.
</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:
- **`requireGardenRole` / `effectiveGardenRole`** (service/gardens.go:66-106): owner branch returns immediately; share lookup falls through to `GetShareRole`; unknown/corrupt share role collapses to `roleNone` → `ErrNotFound` (masked). The `MyRole` stamp happens only after the role/min gates pass, so `roleNone.String()` (the `""` default) is never produced for a response. Verified.
- **`gardenRole.String()`** (service/gardens.go:36-47): "owner" literal vs `domain.RoleEditor`/`RoleViewer` constants — both are "editor"/"viewer" (domain.go:72-73), matching the DB `CHECK (role IN ('viewer','editor'))` and the `'owner' AS my_role` literal in the UNION. Consistent.
- **`ListGardensForActor` SQL** (store/gardens.go:75-87): the outer `SELECT gardenColumns, my_role FROM (...) ORDER BY created_at DESC, id DESC` references the UNION's projected columns positionally; `qualifyColumns("g", gardenColumns)` re-aliases the second branch to match; both branches project 11 columns and `scanGardenWithRole` reads 11. Owner-can't-have-a-share-row invariant (enforced by `ErrCannotShareWithSelf` + UNIQUE) prevents duplicate rows. Verified.
- **`UpdateGarden` tightened to owner-only** (service/gardens.go:139-154): `requireGardenRole(roleOwner)`; the version-conflict path also gets `MyRole="owner"` via the `if updated != nil` guard. Correct.
- **`UpdatePlanting` plant-visibility change** (service/plantings.go:124-137): `applyPlantingPatch` runs first, so `pl.PlantID` is the *new* id when `patch.PlantID != nil`; `visiblePlant` is then called on that new id — the gate is correct. The unconditional `store.GetPlant` afterward is only for `derivedCount`, and plants with plantings can't be deleted (ON DELETE RESTRICT → `ErrPlantInUse`), so a stale PlantID isn't a real path. Verified.
- **`AddShare`/`UpdateShareRole`/`RemoveShare`** (service/shares.go): role validated by `isShareRole` (rejects "owner" and garbage); self-share rejected via `target.ID == actorID` after the email lookup; duplicate caught by the UNIQUE index → `ErrShareExists`. `RemoveShare`'s `g.OwnerID != actorID && actorID != targetUserID` correctly admits only owner-or-self and masks everyone else with `ErrNotFound`. Verified.
- **`writeServiceError` mapping** (api/errors.go:33-38): the three new sentinels map to 404/400/409 respectively, matching the PR description and the `domain.Err*` definitions. Verified.
- **ACL matrix test** (service/shares_test.go): I traced each row — owner/editor/viewer/stranger × the six operations — against the implemented role ranking (`roleNone < roleViewer < roleEditor < roleOwner`) and the masking rule. Each `wantErr` expectation matches what the code produces (e.g. editor→`ErrForbidden` on `UpdateGarden` because `roleEditor < roleOwner` but `roleEditor != roleNone`; stranger→`ErrNotFound` because `roleNone`).
No correctness defects found in this lane.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All five findings verified against the actual code. Each is confirmed:
1. `gardens.go:39` returns `"owner"` literal; `store/gardens.go:78` has `'owner' AS my_role` literal; `domain.go` has `RoleViewer`/`RoleEditor` but no `RoleOwner`. Confirmed.
2. `store/shares.go:61-64` inlines the scan; `scanShare` exists at lines 15-24; `scanGardenWithRole` shows the extending pattern. Confirmed.
3. `service/shares.go:66-77` does its own GetGarden+OwnerID check, no `requireGardenRole` call, no comment. Confirmed.
4. `service/shares.go:74-75` comment attributes the ErrNotFound property to the owner/self-leave split. Confirmed.
5. `wantErr` only in shares_test.go; 54 inline `= %v, want` assertions across other service test files. Confirmed.
VERDICT: Minor issues
- **`internal/service/gardens.go:39`** — `"owner"` role string is hardcoded in `gardenRole.String()` while the store SQL at `internal/store/gardens.go:78` emits `'owner' AS my_role` as a literal. There is no `domain.RoleOwner` constant (only `RoleViewer`/`RoleEditor`). If either side drifts, owned-garden `MyRole` silently mismatches. Fix: add `domain.RoleOwner = "owner"` and reference it in both places.
- **`internal/store/shares.go:61`** — `ListSharesForGarden` inlines the share-column scan instead of extending `scanShare`, breaking the file's `scanX(scanner)` helper pattern (`scanGarden`/`scanShare`/`scanGardenWithRole` all exist to keep column order in one place). The inline scan duplicates `shareColumns` order; if `shareColumns` is reordered, `scanShare` updates but this scan silently desyncs. Fix: a `scanShareWithUser(s, email, displayName)` that calls `scanShare` then scans the two extra columns, mirroring `scanGardenWithRole`.
- **`internal/service/shares.go:66`** — `RemoveShare` bypasses the `requireGardenRole` choke point with its own `s.store.GetGarden` + `g.OwnerID != actorID` check. Partly forced by the self-leave semantic (a recipient has no owner role), but there's no comment explaining the divergence, inviting a future reader to "fix" it. A one-line comment ("deliberately not requireGardenRole: a recipient self-leaving has no role to check") would prevent that.
- **`internal/service/shares.go:74`** — The comment's "missing row is ErrNotFound either way" is a property of `DeleteShare` (returns `ErrNotFound` on `RowsAffected == 0`), independent of the owner/self-leave split; the comment reads as if the property depends on the branch. Minor misleading wording.
- **`internal/service/shares_test.go:12`** — `wantErr` is local to `shares_test.go` but generally useful; the rest of the service suite re-implements the same `t.Errorf("... = %v, want ...", err, ...)` assertion inline (54 occurrences across `plants_test.go`, `objects_test.go`, `gardens_test.go`, `plantings_test.go`, `auth_test.go`). Promoting it to a shared `testing.go` would reduce churn.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I'll investigate this PR through the performance lens. Let me verify the key performance-sensitive paths.
Let me check `GardenFull`, `UpdateObject`, and `UpdatePlanting` to see if the new share-lookup call in `requireGardenRole` introduces N+1 issues on hot paths.
Let me check the schema for indexes on `garden_shares` (critical for the share-lookup query performance), since `requireGardenRole` now does that lookup on every garden operation.
The schema has appropriate indexes (UNIQUE on `(garden_id, user_id)` and separate single-column indexes). The `GetShareRole` lookup uses both columns which the UNIQUE index efficiently serves.
Let me verify one concern: in `requireGardenRole`, the share lookup happens on every garden operation, including the owner case after `GetGarden`. Wait — looking again, `effectiveGardenRole` short-circuits with `if g.OwnerID == actorID` *before* the share query. Good. So owners don't pay the extra query.
But for non-owners on any garden operation (every plantings/objects update by a shared editor), each call to `requireGardenRole` now does 2 queries instead of 1: `GetGarden` + `GetShareRole`. And `objectForRole` → `requireGardenRole`. That's one extra indexed-point query per mutation. That's a reasonable, necessary cost for ACL enforcement — not a regression in the sense of avoidable work.
Let me check `GardenFull` for any concern — it calls `requireGardenRole` once then bulk-loads. Fine.
Let me check the `ListGardensForActor` SQL — the UNION ALL with the `LIMIT` outside. Since both subqueries are unbounded, the database must materialize both fully before applying `LIMIT`. For an owner with many gardens + many shares, that's two full scans combined. But both halves use indexed predicates (`owner_id = ?` and `s.user_id = ?`), and `maxGardensListed=1000` caps it. This is fine at household scale, and the comment explicitly says so.
Let me check `ListSharesForGarden` — `ORDER BY u.display_name COLLATE NOCASE, s.id` — this requires a sort on the joined result. No index on `users.display_name`. But this is bounded by the number of shares per garden (small), so it's not a concern.
Let me verify there's no N+1 in the new `UpdatePlanting` flow: when `patch.PlantID != nil`, it now calls `visiblePlant` (which does `GetPlant`) AND then `s.store.GetPlant` again unconditionally at line 134. That's a duplicate plant fetch — let me look closer.
Confirmed: when `patch.PlantID != nil`, the same plant is fetched twice (once by `visiblePlant`, once at line 134). This was already the case before this PR (the old code called `visiblePlant` which calls `GetPlant`, then the code at line 134 also called `GetPlant`). Wait — let me check the original behavior. The diff shows the old code did `plant, err := s.visiblePlant(ctx, actorID, pl.PlantID)` unconditionally, and now line 134 calls `s.store.GetPlant` unconditionally. So before: 1 fetch always. After: 1 fetch always (line 134), plus an extra fetch only when `PlantID` is being changed. So this is a small new duplication only on plant-change operations.
This is a minor, avoidable extra query on a non-hot path (changing a plop's plant). Not material at household scale, but worth a one-line fix (reuse the plant returned by `visiblePlant`). Let me note it but as trivial.
Now let me check whether there's a more material concern: every mutation by a shared editor goes through `requireGardenRole` → `GetGarden` + `GetShareRole`. For `UpdatePlanting`/`UpdateObject`/`DeletePlanting`/`DeleteObject`, that's via `objectForRole` → `requireGardenRole`. One extra indexed single-row query per mutation. Necessary for correctness; not a regression worth flagging.
Let me check `ListGardensForActor` for one more concern — the UNION ALL with the outer LIMIT, and the inner `SELECT ... 'owner' AS my_role FROM gardens WHERE owner_id = ?` — there's no `ORDER BY` inside, so SQLite will produce both result sets and then sort/limit. With `maxGardensListed=1000`, fine. No material issue.
Let me verify the `getGardenFull` handler doesn't trigger per-object N+1.
No N+1 there. The `GardenFull` is well-batched.
Let me do a final check on whether the `ListSharesForGarden` query is called from a hot path. It's only called by `ListShares` (owner only, UI-driven). Not hot. Fine.
I've completed my performance review. Let me write up findings.
VERDICT: No material issues found
I reviewed the PR through the performance lens, examining:
- **`requireGardenRole` share lookup** (`service/gardens.go:66-83`, `service/gardens.go:87-106`): Each garden-scoped operation now potentially issues a `GetShareRole` query. Verified that `effectiveGardenRole` short-circuits via `if g.OwnerID == actorID` *before* the share query, so owners pay no extra cost. Non-owners pay one extra indexed single-row lookup (`garden_shares` has a UNIQUE on `(garden_id, user_id)`, confirmed in `migrations/0001_init.sql:66`). That is the necessary cost of ACL enforcement on shared gardens, not a regression.
- **`ListGardensForActor` UNION ALL** (`store/gardens.go:75-87`): The outer `LIMIT` requires materializing both halves, but each half uses an indexed predicate (`owner_id` / `s.user_id`) and the result is capped at `maxGardensListed=1000`. Appropriate at the stated household scale; the code comment explicitly defers real pagination to post-v1. Not material.
- **`GardenFull`** (`service/objects.go:161-189`): Still single authorization check + three batched list queries + a map for `DerivedCount`. No N+1 introduced.
- **`ListSharesForGarden`** (`store/shares.go:45-73`): UI-only path, bounded by shares-per-garden. The `ORDER BY u.display_name COLLATE NOCASE` sort is over a small set; no index needed. Not a hot path.
- **`UpdatePlanting` duplicate plant fetch** (`service/plantings.go:129-134`): When `patch.PlantID != nil`, `visiblePlant` calls `GetPlant` and then line 134 calls `GetPlant` again on the same `pl.PlantID`. Pre-PR this fetched the plant once unconditionally via `visiblePlant`; post-PR it fetches once unconditionally (line 134) plus a second fetch only when the plant is being changed. This is one extra indexed single-row query on a non-hot mutation path (changing a plop's plant), trivially fixable by reusing the plant returned from `visiblePlant`. I consider this trivial — not material at household scale, and not on a hot loop. Mentioning only for completeness.
No blocking or minor performance issues in this lens; the duplicate-plant-fetch is the only thing I'd even note, and it's trivial.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I'll review this PR through the error handling & edge cases lens. Let me examine the actual code to verify the diff's behavior.
Looking through the error-handling & edge-cases lens, I verified the new share code paths, the `UpdatePlanting` visibility change, and the `UpdateGarden`/`requireGardenRole` role-stamping by reading the actual files and the schema.
**VERDICT: No material issues found**
I checked the unhappy paths the diff introduces and they hold up:
- `RemoveShare` (`service/shares.go:66-77`): the `g.OwnerID != actorID && actorID != targetUserID` guard correctly masks existence for non-participants with `ErrNotFound`, while letting both the owner-remove-anyone and recipient-self-leave paths through. I confirmed a stranger gets `ErrNotFound` from `GetGarden` (garden missing) or from the guard (garden exists, no access) — same error, no existence leak. A recipient trying to remove *another* recipient's share also falls into the guard → `ErrNotFound` (the test covers the stranger variant; the logic is identical for a co-recipient).
- `AddShare` (`service/shares.go:30-50`): ordering is correct — owner check before role validation (so a non-owner probing with a bad role gets `ErrForbidden`, not `ErrInvalidInput`); `GetUserByEmail`'s `ErrNotFound` is mapped to `ErrShareUserNotFound`; the UNIQUE race is caught by `isUniqueViolation` → `ErrShareExists` in `store.CreateShare` (`store/shares.go:86`), so two concurrent `AddShare` calls for the same pair can't both succeed.
- `UpdatePlanting` (`service/plantings.go:124-137`): the visibility re-check is correctly gated on `patch.PlantID != nil` (after `applyPlantingPatch` has already updated `pl.PlantID`), so a shared editor editing radius/label on the owner's private-plant plop no longer fails; the unconditional `s.store.GetPlant` for the derived count is safe because the `plantings → plants` FK is `ON DELETE RESTRICT` (verified in `migrations/0001_init.sql:126`), so a referenced plant always exists. Errors from `GetPlant`/`visiblePlant` are propagated, not swallowed.
- `UpdateGarden` (`service/gardens.go:149-153`): the `if updated != nil` guard correctly stamps `MyRole` on both the success row and the version-conflict `current` row returned by `store.UpdateGarden` (`store/gardens.go:128-133`), and correctly skips stamping on the `(nil, ErrNotFound)` "garden deleted mid-flight" path. No error is swallowed.
- `ListSharesForGarden` / `ListGardensForActor` (`store/shares.go:56`, `store/gardens.go:91`): both `defer rows.Close()` and check `rows.Err()` after iteration, matching the existing pattern. Empty results return non-nil `[]domain.{ShareWithUser,Garden}{}` (not JSON `null`), which the API test asserts.
- `GetShareRole` (`store/shares.go:30-41`): `sql.ErrNoRows` → `( "", false, nil )` is cleanly distinguished from a real DB error; `effectiveGardenRole`'s `default` branch collapses a corrupt role string to `roleNone` → `ErrNotFound`, so a bad DB row can't elevate privilege.
- The `ListGardensForActor` UNION ALL can't double-count a garden for its owner, because `AddShare`'s `target.ID == actorID` guard (`ErrCannotShareWithSelf`) prevents an owner from ever having a share row on their own garden — verified the self-share guard is the only insert path.
No swallowed errors, no missing `defer`, no panics on nil/empty/zero input, and the masked-existence contract (`ErrNotFound` for both missing and forbidden) is preserved across every new branch.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking issues found · ⚠️ 2/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Blocking issues found</summary>
**Blocking issues found**
- **`RemoveShare` bypasses the `requireGardenRole` authorization choke point**, allowing an actor with no role on a garden to reach `store.DeleteShare`.
`internal/service/shares.go:66` opens the garden with a raw `store.GetGarden` instead of funneling through `requireGardenRole` — the comment on `requireGardenRole` calls it “THE place authorization for a garden is decided.” A stranger can call `RemoveShare` with `targetUserID == actorID`; the ad-hoc check `g.OwnerID != actorID && actorID != targetUserID` passes, so the stranger reaches `DeleteShare`. While the parameterized query prevents SQL injection and only the actor’s own (non-existent) row would be targeted, the stranger has still bypassed the choke point and can distinguish garden existence via timing (for an existing garden `GetGarden` succeeds before `DeleteShare` is reached; for a missing garden `GetGarden` fails immediately). More importantly, it breaks the architectural invariant that every garden-touching operation routes through `requireGardenRole`.
**Fix:** Add `g, err := s.requireGardenRole(ctx, actorID, gardenID, roleViewer)` at the top of `RemoveShare`; then keep the existing owner-or-self check. `roleViewer` is the right minimum because both owners (who may remove anyone) and shared viewers/editors (who may self-leave) must satisfy it.
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found.
The sharing backend code is clean, consistent with existing patterns, and well-documented. A few notes:
* The `wantErr` helper in `internal/service/shares_test.go:12` duplicates the same helper pattern used in other test files, but is isolated to this new file and fits the local style.
* `scanGardenWithRole` in `internal/store/gardens.go:28` is a clear, minimal extension of `scanGarden` that follows the established scanner pattern.
* The `gardenRole.String()` method at `internal/service/gardens.go:36` cleanly centralizes role string rendering rather than sprinkling string literals.
* Error mapping in `internal/api/errors.go` and route registration in `internal/api/api.go` follow the existing conventions exactly.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
</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>
- RemoveShare now routes through requireGardenRole(roleViewer) — the standard
authorization choke point (masks existence for non-participants) — then applies
the owner-or-self rule on top (a participant removing someone else's share is
now ErrForbidden, not ErrNotFound). No more bespoke GetGarden+manual check.
- Add domain.RoleOwner constant; gardenRole.String() and the tests use it instead
of the bare "owner" literal.
- UpdatePlanting fetches the plant once and only re-checks visibility when the
plant id actually CHANGES (new ≠ old), so a no-op plantId resend can't break a
shared editor editing a plop that uses the owner's private plant.
- Bound ListSharesForGarden with a LIMIT backstop.
Skipped: AddShare email-existence disclosure (the issue explicitly accepts it as
inherent to email-based sharing), 404-on-missing-share (correct DELETE
semantics), and the join-scan/test-helper dedup nits.
GOWORK=off go build/vet/test ./internal/... green.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve
merged commit 2a86f87b50 into main2026-07-19 03:49:55 +00:00
steve
deleted branch phase-6-sharing-api2026-07-19 03:49:55 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #16. Part of epic #20 (Phase 6 — Sharing). Extends the
requireGardenRolechoke point (#7) to consultgarden_shares.What
domain
Gardengains a computedMyRole("owner"/"editor"/"viewer",omitempty, never persisted). NewShareWithUser(share + recipient email/name) for the UI. Sentinels:ErrShareUserNotFound,ErrCannotShareWithSelf,ErrShareExists.store
shares.go—GetShareRole,ListSharesForGarden(joined withusersfor email/name),CreateShare(UNIQUE →ErrShareExists),UpdateShareRole,DeleteShare.gardens.go—ListGardensForActorreturns owned + shared-with-me gardens (UNION), each row carryingmy_role.service
requireGardenRolenow consults shares: owner (implicit viaowner_id), else the share's role, else maskedErrNotFound. It stampsMyRoleon every read-through, soGET /gardens/:id,/full, list, etc. all carry the actor's role.UpdateGardentightened to OWNER-only — the matrix says editors edit contents, not the garden's metadata.UpdatePlantingonly re-checks plant visibility when the plant is changed — otherwise a shared editor couldn't edit a plop that uses the owner's private plant (caught by the ACL matrix test).shares.go—ListShares/AddShare/UpdateShareRole(owner only),RemoveShare(owner or a recipient leaving).AddSharetargets an existing account by exact email; unknown → 404, self → 400, duplicate → 409.api
GET,POST /api/v1/gardens/:id/sharesandPATCH,DELETE /api/v1/gardens/:id/shares/:userId. New error codesSHARE_USER_NOT_FOUND/CANNOT_SHARE_WITH_SELF/SHARE_EXISTS.Tests
{owner, editor, viewer, stranger} × {read /full, mutate object, mutate planting, edit garden meta, delete garden, manage shares}— nil = allowed,ErrForbidden= visible-but-insufficient,ErrNotFound= masked.my_role;AddShareerror cases (unknown/self/bad-role/duplicate); role upgrade + editor-can-move-object-but-not-manage-shares; recipient self-leave; a non-participant can't remove others.GOWORK=off go build/vet/test ./internal/...green.Acceptance criteria
myRole: "viewer"), reads/full, gets 403 on mutations; A upgrades B to editor → B moves an object but still can't rename the garden or see shares; A removes the share → B gets 404.🤖 Generated with Claude Code
- domain: Garden gains a computed MyRole ("owner"/"editor"/"viewer"); new ShareWithUser (share + recipient identity); sentinels ErrShareUserNotFound, ErrCannotShareWithSelf, ErrShareExists. - store/shares.go: GetShareRole, ListSharesForGarden (joined with users), CreateShare (UNIQUE → ErrShareExists), UpdateShareRole, DeleteShare. - store/gardens.go: ListGardensForActor returns owned + shared-with-me gardens, each carrying my_role (replaces the owner-only list). - service: requireGardenRole now consults garden_shares (owner implicit, else the share's role, else masked ErrNotFound) and stamps MyRole on every read-through. UpdateGarden tightened to OWNER-only (editors edit contents, not the garden). UpdatePlanting only re-checks plant visibility when the plant is CHANGED, so a shared editor can edit a plop that uses the owner's private plant. - service/shares.go: ListShares/AddShare/UpdateShareRole (owner only), RemoveShare (owner or a recipient leaving). AddShare targets an existing account by exact email; unknown → 404, self → 400, duplicate → 409. - api: GET,POST /gardens/:id/shares and PATCH,DELETE /gardens/:id/shares/:userId. Tests: the full {owner, editor, viewer, stranger} × {read, mutate object, mutate planting, edit garden, delete garden, manage shares} ACL matrix; shared gardens carry my_role; AddShare error cases; role upgrade + self-leave; plus a two-user HTTP flow. The matrix caught the editor-cannot-edit-owner's-plant bug above. GOWORK=off go build/vet/test ./internal/... green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-19 03:43:12Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 12 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -35,0 +36,4 @@func (r gardenRole) String() string {switch r {case roleOwner:return "owner"🟡 "owner" role string duplicated between service String() and store SQL literal; no domain.RoleOwner constant
maintainability · flagged by 2 models
internal/service/gardens.go:39—"owner"role string is hardcoded ingardenRole.String()while the store SQL atinternal/store/gardens.go:78emits'owner' AS my_roleas a literal. There is nodomain.RoleOwnerconstant (onlyRoleViewer/RoleEditor). If either side drifts, owned-gardenMyRolesilently mismatches. Fix: adddomain.RoleOwner = "owner"and reference it in both places. -internal/store/shares.go:61—ListSharesForGardeninlines the share-column scan ins…🪰 Gadfly · advisory
@@ -110,1 +149,3 @@return s.store.UpdateGarden(ctx, g)updated, err := s.store.UpdateGarden(ctx, g)if updated != nil {updated.MyRole = roleOwner.String() // only the owner reaches here🟡 UpdateGarden hardcodes roleOwner.String() instead of reusing requireGardenRole result
maintainability · flagged by 1 model
internal/service/gardens.go:151—UpdateGardenhardcodesupdated.MyRole = roleOwner.String()instead of reusing the role already computed and stamped byrequireGardenRole. The returned garden fromrequireGardenRoleis discarded (if _, err := ...), so the permission result is recalculated by hand. This is brittle: if the model ever allows non-owners through (co-owners, admin override), the response will falsely claim"owner". Capture the garden fromrequireGardenRoleand use its…🪰 Gadfly · advisory
@@ -127,0 +126,4 @@// existing plop may reference a plant the actor can't see (e.g. a shared// editor working in the owner's garden with the owner's private plant), and// editing its radius/label/date must still work.if patch.PlantID != nil {🟠 Plant-visibility re-check triggers on patch field presence, not actual value change, defeating the shared-editor fix for a no-op plantId resend
error-handling, maintainability · flagged by 1 model
internal/service/plantings.go:129-133— The "only re-check plant visibility when the plant is changed" guard checks whetherpatch.PlantIDis present, not whether the value actually differs from the planting's currentPlantID:go if patch.PlantID != nil { if _, err := s.visiblePlant(ctx, actorID, pl.PlantID); err != nil { return nil, err } }If a caller sendsplantIdin the PATCH body equal to the plop's existing plant (e.g. a user re-opens a plant picker and re-selects…🪰 Gadfly · advisory
@@ -127,0 +131,4 @@return nil, err}}plant, err := s.store.GetPlant(ctx, pl.PlantID) // for the derived count🟡 Redundant duplicate GetPlant query in UpdatePlanting when the plant is changed
error-handling, performance · flagged by 2 models
internal/service/plantings.go:134—GetPlantafter a plant-id change can returnErrNotFoundand surface as a raw 404 whereErrInvalidInput(400) was intended. Whenpatch.PlantID != nil, the code first callss.visiblePlant(...)which maps a missing/invisible plant toErrInvalidInput(line 130, confirmed atplantings.go:176-187). ButvisiblePlantalready fetches the plant and returns it — that value is discarded (if _, err := ...), and then line 134 calls `s.store.GetPlan…🪰 Gadfly · advisory
@@ -0,0 +27,4 @@// exact email of an existing account (v1 has no invitation emails). Owner only.// Unknown email → ErrShareUserNotFound; the owner's own email →// ErrCannotShareWithSelf; an already-shared user → ErrShareExists.func (s *Service) AddShare(ctx context.Context, actorID, gardenID int64, email, role string) (*domain.GardenShare, error) {🟡 AddShare exposes an authenticated user-enumeration oracle (unknown email → 404, known → 201/409)
security · flagged by 1 model
internal/service/shares.go:30(AddShare) — authenticated user-enumeration oracle. Any authenticated owner of a garden can probe whether an arbitrary email is a registered pansy account: callingPOST /gardens/:id/shareswith{"email": "<guess>", "role": "viewer"}returns404 SHARE_USER_NOT_FOUNDfor an unknown email (mapping atinternal/api/errors.go:33-34) vs.201(or409 SHARE_EXISTSpererrors.go:37-38) for a known one. The owner can thenDELETEthe share to clean up.…🪰 Gadfly · advisory
@@ -0,0 +36,4 @@}target, err := s.store.GetUserByEmail(ctx, strings.TrimSpace(email))if errors.Is(err, domain.ErrNotFound) {return nil, domain.ErrShareUserNotFound🟡 AddShare 404 for unknown email enables authenticated account enumeration
security · flagged by 1 model
internal/service/shares.go:39(AddShare) — authenticated account enumeration.GetUserByEmailreturningdomain.ErrNotFoundis converted toErrShareUserNotFound, whichwriteServiceErrormaps to a distinct 404SHARE_USER_NOT_FOUNDwith message "no account with that email"; a valid target yields 409/201. Since any authenticated user can create a garden (becoming its owner) and then callAddShareon it, any logged-in user can probe arbitrary email addresses to learn whether th…🪰 Gadfly · advisory
@@ -0,0 +63,4 @@// RemoveShare revokes a share. The garden owner may remove anyone; a recipient// may remove themselves ("leave garden"). Any other actor gets the garden's// masked ErrNotFound (existence isn't revealed to non-participants).func (s *Service) RemoveShare(ctx context.Context, actorID, gardenID, targetUserID int64) error {🔴 RemoveShare bypasses requireGardenRole authorization choke point
maintainability, security · flagged by 5 models
RemoveSharebypasses therequireGardenRoleauthorization choke point, allowing an actor with no role on a garden to reachstore.DeleteShare.internal/service/shares.go:66opens the garden with a rawstore.GetGardeninstead of funneling throughrequireGardenRole— the comment onrequireGardenRolecalls it “THE place authorization for a garden is decided.” A stranger can callRemoveSharewithtargetUserID == actorID; the ad-hoc check `g.OwnerID != actorID && actorID != targ…🪰 Gadfly · advisory
@@ -0,0 +68,4 @@if err != nil {return err // ErrNotFound}if g.OwnerID != actorID && actorID != targetUserID {🟡 Owner removing a non-existent share returns ErrNotFound (404) instead of 204; behavior is defensible but undocumented beyond a comment
error-handling, maintainability, security · flagged by 2 models
internal/service/shares.go:71— Owner removing a non-existent share / self-leave of a non-share returnsErrNotFoundinstead of 204. InRemoveShare, the guard isg.OwnerID != actorID && actorID != targetUserID(confirmed). TheGetGardenlookup at line 67 masks the garden from strangers (good). But the owner path and the self-leave path both fall through tos.store.DeleteShare(line 76), which returnsdomain.ErrNotFoundwhen 0 rows are affected (confirmed at `internal/store/sha…🪰 Gadfly · advisory
@@ -0,0 +9,4 @@)// wantErr asserts err matches want (nil = expected success, else errors.Is).func wantErr(t *testing.T, name string, err, want error) {⚪ wantErr helper is local to shares_test.go but generally useful; other tests re-implement the same assertion inline
maintainability · flagged by 1 model
internal/service/gardens.go:39—"owner"role string is hardcoded ingardenRole.String()while the store SQL atinternal/store/gardens.go:78emits'owner' AS my_roleas a literal. There is nodomain.RoleOwnerconstant (onlyRoleViewer/RoleEditor). If either side drifts, owned-gardenMyRolesilently mismatches. Fix: adddomain.RoleOwner = "owner"and reference it in both places. -internal/store/shares.go:61—ListSharesForGardeninlines the share-column scan ins…🪰 Gadfly · advisory
@@ -64,2 +77,2 @@`SELECT `+gardenColumns+` FROM gardens WHERE owner_id = ? ORDER BY created_at DESC, id DESC LIMIT ?`,ownerID, maxGardensListed,`SELECT `+gardenColumns+`, my_role FROM (SELECT `+gardenColumns+`, 'owner' AS my_role FROM gardens WHERE owner_id = ?🟡 SQL hardcodes 'owner' role string duplicating service-layer gardenRole.String()
maintainability · flagged by 1 model
internal/store/gardens.go:78— TheListGardensForActorquery hardcodes the string literal'owner'for the owner role ('owner' AS my_role), while the service layer derives the same value viaroleOwner.String(). This duplicates the role-string definition across store and service layers; if the API contract changes, both places must be updated or they drift apart.🪰 Gadfly · advisory
@@ -0,0 +43,4 @@// ListSharesForGarden returns a garden's shares joined with each recipient's// email and display name, for the sharing UI. Always a non-nil slice.func (d *DB) ListSharesForGarden(ctx context.Context, gardenID int64) ([]domain.ShareWithUser, error) {rows, err := d.sql.QueryContext(ctx,🟠 ListSharesForGarden is unbounded — missing LIMIT clause
performance · flagged by 1 model
internal/store/shares.go:46—ListSharesForGardenqueriesgarden_sharesjoined withusersbut has noLIMITclause, making the result set unbounded.ListGardensForActor(in the same PR area) explicitly caps atmaxGardensListed = 1000as a "defensive backstop against a pathologically large result"; the shares list should have the same defense. Even at normal sharing scales this is unlikely to be hit, but the endpoint should not be the only unbounded list query in the surface. *…🪰 Gadfly · advisory
@@ -0,0 +58,4 @@shares := []domain.ShareWithUser{}for rows.Next() {var sh domain.ShareWithUserif err := rows.Scan(🟡 ListSharesForGarden inlines the share-column scan, duplicating shareColumns order instead of extending scanShare
maintainability · flagged by 1 model
internal/service/gardens.go:39—"owner"role string is hardcoded ingardenRole.String()while the store SQL atinternal/store/gardens.go:78emits'owner' AS my_roleas a literal. There is nodomain.RoleOwnerconstant (onlyRoleViewer/RoleEditor). If either side drifts, owned-gardenMyRolesilently mismatches. Fix: adddomain.RoleOwner = "owner"and reference it in both places. -internal/store/shares.go:61—ListSharesForGardeninlines the share-column scan ins…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 13 findings (4 with multi-model agreement)
internal/service/shares.go:66internal/service/gardens.go:39internal/service/plantings.go:134internal/service/shares.go:719 single-model findings (lower confidence)
internal/service/plantings.go:129internal/store/shares.go:46internal/domain/domain.go:72internal/service/gardens.go:151internal/service/shares.go:30internal/service/shares.go:39internal/store/gardens.go:78internal/store/shares.go:61internal/service/shares_test.go:12Per-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
Good — sharing routes sit under
gardensgroup withh.requireAuth()and CSRF guard applies globally.qualifyColumnsonly operates on the hardcodedgardenColumns/shareColumnsconstants, not user input, so no injection risk. Everything else checks out: parameterized queries throughout, DB-levelCHECK (role IN ('viewer','editor'))backs the app-layerisShareRolevalidation, email lookup is case-insensitive by design, and thevisiblePlantbypass inUpdatePlantingdoesn't newly disclose anything —GardenFullalready returns all referenced plants to any garden viewer/editor viaListReferencedPlants, unrelated to this diff.I'm satisfied this diff has no material security issues.
VERDICT: No material issues found
I reviewed this diff specifically for authn/authz gaps, injection, IDOR, and information disclosure:
requireGardenRole/explicit owner checks ininternal/service/shares.go. VerifiedListShares/AddShare/UpdateShareRolerequireroleOwner, andRemoveShare's manual owner-or-self check (internal/service/shares.go:68-72) can't be bypassed — a stranger passing their own ID as target just hits a real "no such share" 404 rather than an authorization bypass (traced throughDeleteShare'sRowsAffectedcheck ininternal/store/shares.go).internal/store/shares.go,internal/store/gardens.go) use parameterized?placeholders.qualifyColumns(internal/store/sqlite.go:105) is only ever applied to the hardcodedgardenColumns/shareColumnsconstants, never to request input.isShareRolerestricts grantable roles to viewer/editor (owner can't be granted via a share), and this is backed by a DBCHECK (role IN ('viewer','editor'))constraint (internal/store/migrations/0001_init.sql:61) — defense in depth.AddShare: verifiedGetUserByEmailis case-insensitive by column collation (comment + implementation ininternal/store/users.go:94-98), so no case-mismatch bypass; self-share is checked by user ID after lookup, not by string comparison, so case games don't matter.UpdatePlantingvisibility bypass (skippingvisiblePlantwhenpatch.PlantIDis nil): checked whether this newly leaks a private plant's data to a shared editor. It does not —GardenFull(internal/service/objects.go:161-183, unchanged by this PR) already returns every plant referenced anywhere in the garden viaListReferencedPlantsto any viewer/editor, so the plant's data was already exposed to garden participants regardless of this patch./sharesroutes are registered inside thegardensgroup which already carriesh.requireAuth(), and the wholev1group carriesh.csrfGuard()(internal/api/api.go:47,72-88).Nothing else in this diff (error mapping in
internal/api/errors.go, theMyRolecomputed field, theShareWithUserjoin) introduces new untrusted-input handling or a new trust boundary.🎯 Correctness — No material issues found
Verdict: No material issues found
I reviewed the sharing ACL logic (
requireGardenRole/effectiveGardenRole), the newgarden_sharesCRUD, theListGardensForActorUNION query, and theUpdatePlantingplant-visibility change, cross-checking each against the actual checked-out files (internal/service/gardens.go,internal/service/plantings.go,internal/service/shares.go,internal/store/gardens.go,internal/store/shares.go,internal/api/errors.go,internal/api/api.go, and thegarden_sharesschema inmigrations/0001_init.sql).Specific things I verified rather than assumed:
gardenRoleordering (roleNone < roleViewer < roleEditor < roleOwner) makes therole < mincomparisons inrequireGardenRolecorrect, andeffectiveGardenRolecorrectly special-cases the owner (never agarden_sharesrow, matching the schema comment) before falling back toGetShareRole.objectForRole,UpdatePlanting,DeleteGarden,GardenFull) funnels throughrequireGardenRole, so shares are honored everywhere, not just on the garden read path — no bypass found.UpdatePlanting's new "only re-check plant visibility whenpatch.PlantID != nil" gate correctly uses the pointer-nil convention already used by every other field inPlantingPatch/plantingUpdateRequest(internal/api/plantings.go:40), so a client that omitsplantIdfrom the JSON body won't trip the visibility check on an unrelated edit — this isn't a loophole, it's consistent with the existing partial-PATCH contract, and it's covered byTestGardenACLMatrix'smutPlop/editorcase.ListGardensForActor's UNION ALL query: both branches project the same 11 columns in the same order, the outerORDER BY created_at, idbinds to the first branch's column names (valid SQLite UNION column-naming rule), and a user can never appear in both branches simultaneously sinceAddShareexplicitly rejects self-sharing (ErrCannotShareWithSelf) and owners never get a share row.RemoveShare's participant check (g.OwnerID != actorID && actorID != targetUserID→ maskedErrNotFound) correctly allows owner-removes-anyone and self-leave while blocking a non-participant from removing someone else's share, matchingTestUpdateAndRemoveShare's "stranger remove" case.CreateShare's duplicate-share handling maps to the actualUNIQUE (garden_id, user_id)constraint in the schema — no other unique constraint on that table, soisUniqueViolationcan't misattribute a different failure.isShareRolecorrectly restrictsAddShare/UpdateShareRoletoviewer/editor, blocking privilege escalation to anownershare row.errors.gois irrelevant here since the new sentinels are independent (not wrapped) errors, so placement in theswitchdoesn't cause a case to be shadowed.One minor observation that I'm not raising as a finding since it doesn't produce incorrect behavior against any stated requirement:
UpdateShareRole(store) incrementsversionbut never checks a caller-supplied version, unlike every other mutable resource's stated "sync contract" (see the comment onstore.UpdateGarden). That's a design inconsistency (no lost-update protection for concurrent share edits), not a logic bug — shares are owner-only mutations, low contention, and nothing in the PR's acceptance criteria requires OCC on shares.🧹 Code cleanliness & maintainability — Minor issues
Confirmed:
visiblePlantinternally callss.store.GetPlantand returns that same plant — so whenpatch.PlantID != nil,UpdatePlantingtruly fetches the row twice. All three draft findings verified against the actual code.VERDICT: Minor issues
internal/service/plantings.go:129-137—UpdatePlantingdiscards the*domain.Plantreturned bys.visiblePlant(viaif _, err := s.visiblePlant(...)) whenpatch.PlantID != nil, then unconditionally re-fetches the same row vias.store.GetPlant(ctx, pl.PlantID)at line 134 just to get the spacing for the derived count.CreatePlanting(same file, lines 82/108) shows the intended pattern of capturing and reusing the plantvisiblePlantreturns. This is a wasted DB round-trip whenever a planting's plant is being changed.internal/service/shares.go:66-77—RemoveSharebypasses therequireGardenRolechoke point and instead callss.store.GetGardendirectly (confirmed via grep: this is the only other call site ofs.store.GetGardenin the service package besidesrequireGardenRoleitself, atgardens.go:67), hand-rolling its own "load or NotFound" + owner-or-self check. This works against the file's own stated design — the comment onrequireGardenRoleingardens.go:58-65calls it "THE place authorization for a garden is decided… so neither can skip a check." Could be written asg, err := s.requireGardenRole(ctx, actorID, gardenID, roleViewer)followed by the owner-or-self check, keeping a single source of truth for the "load garden, mask as NotFound" step.internal/service/gardens.go:36-47—gardenRole.String()sources the editor/viewer strings fromdomain.RoleEditor/domain.RoleViewerbut hardcodes the literal"owner"at line 39 instead of a matching constant (confirmed via grep ofinternal/domain/domain.go: onlyRoleViewer/RoleEditorare defined, noRoleOwner). Minor inconsistency; low impact since it's centralized in this one method.⚡ Performance — Minor issues
All claims verified against actual source.
VERDICT: Minor issues
internal/service/plantings.go:129-137— InUpdatePlanting, whenpatch.PlantID != nil, the code callss.visiblePlant(ctx, actorID, pl.PlantID)(which internally callss.store.GetPlant,plantings.go:177) purely to validate visibility and discards the returned*domain.Plant, then unconditionally callss.store.GetPlant(ctx, pl.PlantID)again at line 134 to get the plant for the derived-count calculation. That's two identicalSELECTqueries againstplantsfor the sameplantIDon the plant-changed path, whereCreatePlanting(line 82) fetches once and reuses the result. Low absolute cost (one extra indexed single-row lookup per planting update), but avoidable.*domain.Plantreturned byvisiblePlantwhenpatch.PlantID != nilinstead of re-fetching vias.store.GetPlant.Everything else in this diff checked out fine from a performance standpoint:
ListGardensForActor's UNION query (internal/store/gardens.go:75) is backed byidx_gardens_ownerandidx_garden_shares_user(confirmed ininternal/store/migrations/0001_init.sql:53,69).GetShareRole(internal/store/shares.go:30) used ineffectiveGardenRoleis a single indexed point lookup per non-owner permission check, not called in a loop.emailhas aUNIQUE COLLATE NOCASEindex (migrations/0001_init.sql:16) andgarden_shareshasUNIQUE (garden_id, user_id)(migrations/0001_init.sql:66), soAddShare's lookup/uniqueness checks are indexed.ListSharesForGarden(internal/store/shares.go:45) is a single joined query, no N+1.🧯 Error handling & edge cases — Minor issues
This matches the diff; nothing new to flag. The verification is complete — the single finding in the draft holds up fully against the code.
Review — 🧯 Error handling & edge cases
VERDICT: Minor issues
internal/service/plantings.go:129-133— The "only re-check plant visibility when the plant is changed" guard checks whetherpatch.PlantIDis present, not whether the value actually differs from the planting's currentPlantID:plantIdin the PATCH body equal to the plop's existing plant (e.g. a user re-opens a plant picker and re-selects the currently-assigned plant, or a future/alternate client resends the full object), this still runsvisiblePlant, which fails withErrInvalidInputfor a shared editor working with the owner's private plant — exactly the scenario this PR says it fixed ("otherwise a shared editor couldn't edit a plop that uses the owner's private plant"). The correct guard should compare against the plop's pre-patchPlantID(e.g. capture the original beforeapplyPlantingPatchmutatespl, then compare*patch.PlantID != original), not merely check for a non-nil pointer.applyPlantingPatch(line 124) mutatespl.PlantIDbefore the check at line 129, which only tests presence ofpatch.PlantID, not a diff against the pre-patch value.PlantingPatch{call sites ininternal/service/plantings_test.goandinternal/service/shares_test.go— none exercisePlantIDat all, so this gap is untested.web/src/pages/GardenEditorPage.tsx:129-138'sonPickPlantis the only frontend call site sendingplantId(web/src/lib/objects.ts:314), and it unconditionally sendsplantIdon any picker selection with no guard against re-picking the already-assigned plant — so the gap is live today, not merely theoretical for future clients.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Both findings are confirmed against the actual code.
Verdict: Minor issues
Through the security lens, the ACL choke point (
requireGardenRole→effectiveGardenRole→ share lookup) is sound: owner is implicit viaowner_id, non-owners resolve throughgarden_shares, androleNoneis masked toErrNotFoundso existence isn't leaked to strangers. All SQL uses parameterized placeholders; theshareColumns/qualifyColumnsinterpolation is over static, trusted constants — no injection. Role values are validated byisShareRolebefore reaching the store, so"owner"/arbitrary roles can't be persisted as a share.UpdatePlanting's relaxed plant check is safe: a plant can only be reassigned whenpatch.PlantID != nil, which still routes throughvisiblePlantand rejects another user's private plant — no privilege escalation.RemoveShare's self-leave path keys offactorID == targetUserID(session vs. path), so a user can only delete their own share; any cross-user attempt falls through toErrNotFound, consistently masked whether or not the garden exists. Verified by readinginternal/service/shares.go,internal/store/shares.go,internal/service/gardens.go,internal/service/plantings.go, andinternal/api/errors.go.Findings:
internal/service/shares.go:39(AddShare) — authenticated account enumeration.GetUserByEmailreturningdomain.ErrNotFoundis converted toErrShareUserNotFound, whichwriteServiceErrormaps to a distinct 404SHARE_USER_NOT_FOUNDwith message "no account with that email"; a valid target yields 409/201. Since any authenticated user can create a garden (becoming its owner) and then callAddShareon it, any logged-in user can probe arbitrary email addresses to learn whether they correspond to an existing account. This is a known tradeoff of share-by-email flows, but it is a real user-enumeration vector. If account privacy matters here, consider returning a generic success/queued response (or rate-limiting) instead of a distinct 404 for unknown emails. Verified againstAddShare(line 37-40) andinternal/api/errors.go:33.internal/service/shares.go:76(RemoveShare) — minor UX: owner-self-removal / no-share cases returnErrNotFoundto the owner. Not a vulnerability (the owner is authorized), but the owner path doesn't pre-check that a share row exists fortargetUserID;DeleteSharereturnsdomain.ErrNotFoundon 0 rows affected (confirmed ininternal/store/shares.go:115-127), whichwriteServiceErrormaps to a generic 404. Purely a consistency/UX nit, not a security defect.🎯 Correctness — No material issues found
I've verified all findings. Let me confirm the key claims:
Finding 6 check: The code at gardens.go:149-152 is:
The store (gardens.go:116-138) returns
(current, ErrVersionConflict)on a version mismatch, wherecurrentis non-nil. Soupdated != nilis true on the conflict path, andMyRoleis stamped. The draft's claim that the conflict row is returned "without a role stamp" is false — drop.Finding 2 check: GetPlant returns
ErrNotFoundfor a missing plant (plants.go:100-101). But theplant_idFK isON DELETE RESTRICT(migrations/0001_init.sql:126), so a referenced plant cannot be deleted while plantings exist — unreachable. Not a live bug.Findings 1, 3, 4, 5 are all self-described as "correct / no issue / nuance / trivial" — non-issues.
Every finding is either a non-issue (explicitly confirmed correct by the draft itself) or, in the case of finding 6, contradicted by the actual code.
Verdict: No material issues found
The ACL choke point (
requireGardenRole→effectiveGardenRole→GetShareRole) is sound. The role ranking (roleNone < roleViewer < roleEditor < roleOwner) is consistent, andUpdateGarden's tightening toroleOwneris correct. TheListGardensForActorUNION query emits consistentcreated_at/id/my_rolecolumns across both branches (verified againstgardenColumnsandqualifyColumns). TheUpdatePlantingvisibility check correctly validates the changed-to plant (sinceapplyPlantingPatchruns first at line 124). TheUpdateGardenconflict path does carryMyRole(theif updated != nilguard covers the non-nilcurrentrow returned onErrVersionConflict). The shares service error mapping (ErrShareUserNotFound→404,ErrCannotShareWithSelf→400,ErrShareExists→409) is consistent with the handler/store behavior.🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the source. Confirming:
"owner"magic string:domain.RoleViewer/RoleEditorexist (domain.go:72-73), butRoleOwnerdoes not. The literal"owner"appears ingardenRole.String()(service/gardens.go:39), the SQL'owner' AS my_role(store/gardens.go:78), and the testown[0].MyRole != "owner"(service/shares_test.go:121). Note: the draft's claim about api/shares_test.go:51 is inaccurate (that line is aread fullcheck, not an owner comparison — "owner" only appears in a comment at line 65 there), but the core finding holds across the three confirmed sites.RemoveSharebypassesrequireGardenRole: Confirmed at service/shares.go:66-77 — it callss.store.GetGardendirectly and does a manualg.OwnerID != actorID && actorID != targetUserIDcheck, whileListShares/AddShare/UpdateShareRoleall route throughrequireGardenRole(..., roleOwner). The divergence is intentional (self-leave case) and already commented, but the maintainability observation is accurate.VERDICT: Minor issues
"owner"is a magic string with no constant, unlikeviewer/editor.domaindefinesRoleViewerandRoleEditor(domain.go:72-73), but the owner role is a bare literal in three places that must stay in sync:gardenRole.String()returns"owner"(service/gardens.go:39), theListGardensForActorSQL hardcodes'owner' AS my_role(store/gardens.go:78), and the service test comparesown[0].MyRole != "owner"(service/shares_test.go:121). If the canonical owner label ever changes, these diverge silently. Add adomain.RoleOwner = "owner"constant and use it inString()and the SQL (via a bind parameter or concatenation), mirroring the viewer/editor treatment.RemoveSharebypasses therequireGardenRolechoke point and re-implements the garden-fetch + owner check. service/shares.go:66-77 callss.store.GetGardendirectly and doesg.OwnerID != actorID && actorID != targetUserID, whereasListShares/AddShare/UpdateShareRoleall gate throughrequireGardenRole(..., roleOwner). The divergence is justified (a recipient self-leaving isn't the owner, so an owner-role gate would reject them, and a viewer-role gate would still needOwnerIDafterward), but it duplicates the "fetch garden, propagateErrNotFound" logic thatrequireGardenRolealready encapsulates and leavesRemoveShareas the one share op that doesn't route through the central ACL path. The existing comment explains the self-leave case; consider also noting that it intentionally skipsrequireGardenRoleso the divergence isn't read as an oversight.scanGardenWithRolenear-duplicatesscanGarden(store/gardens.go:15-37), differing only by the trailing&g.MyRole. This is the idiomatic Go cost ofrows.Scanarg lists and is acceptable, but if more "garden + computed column" variants appear, a single scanner taking a variadic tail would curb the copy-paste. Trivial for now.⚡ Performance — Minor issues
Verdict: Minor issues found
Through the performance lens, the sharing change is generally sound:
ListGardensForActoruses a singleUNION ALLover two indexed branches (idx_gardens_owner,idx_garden_shares_user) — no per-row N+1, and theLIMIT ?backstop is preserved. Good.ListSharesForGardenis one JOIN query backed byidx_garden_shares_garden. Good.requireGardenRole's extraGetShareRolelookup is a single indexed point lookup (UNIQUE(garden_id, user_id)), and owners short-circuit before it. Acceptable per-request overhead; no N+1 becauseListGardensresolves roles in the UNION query rather than callingrequireGardenRoleper row.One avoidable duplicate query:
internal/service/plantings.go:129-134— on the plant-change path,visiblePlant(ctx, actorID, pl.PlantID)already loads the plant (it callss.store.GetPlantinternally and returns*domain.Plant, confirmed atplantings.go:176-188), but its return value is discarded (if _, err := ...). Then line 134 unconditionally callss.store.GetPlant(ctx, pl.PlantID)again purely to getSpacingCMfor the derived count. So when a plop's plant is changed, the plant row is fetched twice by primary key instead of once. It's a non-hot path (changing a plop's plant is uncommon) and an indexed PK lookup, so impact is small, but it's a straightforwardly avoidable extra query. Suggested fix: capture the plant fromvisiblePlant(plant, err := s.visiblePlant(...)) and reuse it forderivedCount/SpacingCMon the change branch, falling back to the singleGetPlantonly on the no-change branch.No other material performance regressions found in my lane.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
internal/service/plantings.go:134—GetPlantafter a plant-id change can returnErrNotFoundand surface as a raw 404 whereErrInvalidInput(400) was intended. Whenpatch.PlantID != nil, the code first callss.visiblePlant(...)which maps a missing/invisible plant toErrInvalidInput(line 130, confirmed atplantings.go:176-187). ButvisiblePlantalready fetches the plant and returns it — that value is discarded (if _, err := ...), and then line 134 callss.store.GetPlant(ctx, pl.PlantID)a second time. If the plant was deleted between the two calls (race), the secondGetPlantreturnsdomain.ErrNotFound, which the API maps to a genericNOT_FOUND(404) rather than a 400. Narrow TOCTOU window; low severity. Suggested fix: reuse the plant returned byvisiblePlantinstead of re-fetching, or wrap the secondGetPlantErrNotFoundasErrInvalidInput.internal/service/shares.go:71— Owner removing a non-existent share / self-leave of a non-share returnsErrNotFoundinstead of 204. InRemoveShare, the guard isg.OwnerID != actorID && actorID != targetUserID(confirmed). TheGetGardenlookup at line 67 masks the garden from strangers (good). But the owner path and the self-leave path both fall through tos.store.DeleteShare(line 76), which returnsdomain.ErrNotFoundwhen 0 rows are affected (confirmed atinternal/store/shares.go:125-126). So an owner removing a user who was never shared with — or a recipient "leaving" a garden they were never shared with — gets a 404 instead of the 204 the API intends (internal/api/shares.go:84). Minor; behavior is defensible but the only signal is the inline comment. No fix strictly required.kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
The sharing implementation is sound from a security perspective. The
requireGardenRolechoke point correctly masks existence (ErrNotFoundfor no access) and enforces the role hierarchy (owner>editor>viewer). Garden metadata updates are tightened toroleOwner, and share CRUD is restricted to the owner (with self-removal as the only recipient exception). All SQL queries in the new store code use parameterized placeholders (?) with no user-input concatenation. TheListGardensForActorunion query is constructed from compile-time constants only.🎯 Correctness — No material issues found
No material issues found. The sharing ACL logic is sound:
requireGardenRolecorrectly falls back togarden_sharesand masks existence (ErrNotFound) when the actor has no role, whileUpdateGardenis correctly tightened toroleOwnerper the matrix.RemoveSharebypassesrequireGardenRolebut still enforces the same policy (owner-or-self-only, everything else masked). TheUpdatePlantingchange correctly gatesvisiblePlantonPlantIDmutation, allowing shared editors to edit non-plant fields of a plop that references an owner-private plant. Tests cover the full ACL matrix and all edge cases described.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/service/shares.go:67—RemoveSharebypasses therequireGardenRoleauthorization chokepoint and does a raws.store.GetGardenfollowed by a manual owner/self check. The service documentsrequireGardenRoleas "THE place authorization for a garden is decided", and every other garden-scoped operation funnels through it. Going around it creates a maintenance hazard: future changes to that function (audit logging, rate limiting, extra checks) won't apply to share removal. The fix is to callrequireGardenRole(ctx, actorID, gardenID, roleViewer)first, then apply the owner-or-self rule.internal/store/gardens.go:78— TheListGardensForActorquery hardcodes the string literal'owner'for the owner role ('owner' AS my_role), while the service layer derives the same value viaroleOwner.String(). This duplicates the role-string definition across store and service layers; if the API contract changes, both places must be updated or they drift apart.internal/service/gardens.go:151—UpdateGardenhardcodesupdated.MyRole = roleOwner.String()instead of reusing the role already computed and stamped byrequireGardenRole. The returned garden fromrequireGardenRoleis discarded (if _, err := ...), so the permission result is recalculated by hand. This is brittle: if the model ever allows non-owners through (co-owners, admin override), the response will falsely claim"owner". Capture the garden fromrequireGardenRoleand use itsMyRole.⚡ Performance — Minor issues
Verdict: Minor issues
internal/store/shares.go:46—ListSharesForGardenqueriesgarden_sharesjoined withusersbut has noLIMITclause, making the result set unbounded.ListGardensForActor(in the same PR area) explicitly caps atmaxGardensListed = 1000as a "defensive backstop against a pathologically large result"; the shares list should have the same defense. Even at normal sharing scales this is unlikely to be hit, but the endpoint should not be the only unbounded list query in the surface.Suggested fix: Add
LIMIT ?parameterized with a constant likemaxSharesListed = 1000(or reusemaxGardensListed).🧯 Error handling & edge cases — No material issues found
No material issues found
opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Both findings are confirmed against the actual code:
AddShare(line 30): unknown email →ErrShareUserNotFound→ 404, known →CreateSharereturns 201 (or 409 ifErrShareExists). Confirmed enumeration oracle, gated to authenticated garden owners.RemoveShare(line 71): the branchg.OwnerID != actorID && actorID != targetUserIDreturnsErrNotFoundfor a participant removing another's share. Action correctly denied; status mislabel only.VERDICT: Minor issues
internal/service/shares.go:30(AddShare) — authenticated user-enumeration oracle. Any authenticated owner of a garden can probe whether an arbitrary email is a registered pansy account: callingPOST /gardens/:id/shareswith{"email": "<guess>", "role": "viewer"}returns404 SHARE_USER_NOT_FOUNDfor an unknown email (mapping atinternal/api/errors.go:33-34) vs.201(or409 SHARE_EXISTSpererrors.go:37-38) for a known one. The owner can thenDELETEthe share to clean up. Gated to authenticated garden owners (not anonymous), and the PR description notes v1 deliberately shares-by-existing-account rather than sending invitations, so it's a known tradeoff — but the enumeration implication isn't called out, and at an app with registration-closed/OIDC it could leak which identities have accounts. Suggested mitigations: a generic "invite sent"-style 202 regardless of whether the account exists (requires an out-of-band channel v1 lacks), per-actor rate-limiting of share-create attempts, or at minimum documenting the enumeration as accepted for v1.internal/service/shares.go:71(RemoveShare) — minor authz-response mislabel (not a vuln): a participant non-owner (viewer/editor) attempting to remove another user's share getsErrNotFound(404) via theg.OwnerID != actorID && actorID != targetUserIDbranch, even though they already know the garden exists (they have access to it). The action is correctly denied, so there's no privilege issue; it's just a 404-where-403-would-fit, and the docstring's "existence isn't revealed to non-participants" rationale doesn't apply to participants. Suggested fix: treat the "actor has some access but isn't owner/self" case asErrForbidden. Confidence low — this is arguably intended, and harmless.🎯 Correctness — No material issues found
Verdict: No material issues found
I reviewed the change through the correctness lens, verifying against the checked-out code:
requireGardenRole/effectiveGardenRole(service/gardens.go:66-106): owner branch returns immediately; share lookup falls through toGetShareRole; unknown/corrupt share role collapses toroleNone→ErrNotFound(masked). TheMyRolestamp happens only after the role/min gates pass, soroleNone.String()(the""default) is never produced for a response. Verified.gardenRole.String()(service/gardens.go:36-47): "owner" literal vsdomain.RoleEditor/RoleViewerconstants — both are "editor"/"viewer" (domain.go:72-73), matching the DBCHECK (role IN ('viewer','editor'))and the'owner' AS my_roleliteral in the UNION. Consistent.ListGardensForActorSQL (store/gardens.go:75-87): the outerSELECT gardenColumns, my_role FROM (...) ORDER BY created_at DESC, id DESCreferences the UNION's projected columns positionally;qualifyColumns("g", gardenColumns)re-aliases the second branch to match; both branches project 11 columns andscanGardenWithRolereads 11. Owner-can't-have-a-share-row invariant (enforced byErrCannotShareWithSelf+ UNIQUE) prevents duplicate rows. Verified.UpdateGardentightened to owner-only (service/gardens.go:139-154):requireGardenRole(roleOwner); the version-conflict path also getsMyRole="owner"via theif updated != nilguard. Correct.UpdatePlantingplant-visibility change (service/plantings.go:124-137):applyPlantingPatchruns first, sopl.PlantIDis the new id whenpatch.PlantID != nil;visiblePlantis then called on that new id — the gate is correct. The unconditionalstore.GetPlantafterward is only forderivedCount, and plants with plantings can't be deleted (ON DELETE RESTRICT →ErrPlantInUse), so a stale PlantID isn't a real path. Verified.AddShare/UpdateShareRole/RemoveShare(service/shares.go): role validated byisShareRole(rejects "owner" and garbage); self-share rejected viatarget.ID == actorIDafter the email lookup; duplicate caught by the UNIQUE index →ErrShareExists.RemoveShare'sg.OwnerID != actorID && actorID != targetUserIDcorrectly admits only owner-or-self and masks everyone else withErrNotFound. Verified.writeServiceErrormapping (api/errors.go:33-38): the three new sentinels map to 404/400/409 respectively, matching the PR description and thedomain.Err*definitions. Verified.roleNone < roleViewer < roleEditor < roleOwner) and the masking rule. EachwantErrexpectation matches what the code produces (e.g. editor→ErrForbiddenonUpdateGardenbecauseroleEditor < roleOwnerbutroleEditor != roleNone; stranger→ErrNotFoundbecauseroleNone).No correctness defects found in this lane.
🧹 Code cleanliness & maintainability — Minor issues
All five findings verified against the actual code. Each is confirmed:
gardens.go:39returns"owner"literal;store/gardens.go:78has'owner' AS my_roleliteral;domain.gohasRoleViewer/RoleEditorbut noRoleOwner. Confirmed.store/shares.go:61-64inlines the scan;scanShareexists at lines 15-24;scanGardenWithRoleshows the extending pattern. Confirmed.service/shares.go:66-77does its own GetGarden+OwnerID check, norequireGardenRolecall, no comment. Confirmed.service/shares.go:74-75comment attributes the ErrNotFound property to the owner/self-leave split. Confirmed.wantErronly in shares_test.go; 54 inline= %v, wantassertions across other service test files. Confirmed.VERDICT: Minor issues
internal/service/gardens.go:39—"owner"role string is hardcoded ingardenRole.String()while the store SQL atinternal/store/gardens.go:78emits'owner' AS my_roleas a literal. There is nodomain.RoleOwnerconstant (onlyRoleViewer/RoleEditor). If either side drifts, owned-gardenMyRolesilently mismatches. Fix: adddomain.RoleOwner = "owner"and reference it in both places.internal/store/shares.go:61—ListSharesForGardeninlines the share-column scan instead of extendingscanShare, breaking the file'sscanX(scanner)helper pattern (scanGarden/scanShare/scanGardenWithRoleall exist to keep column order in one place). The inline scan duplicatesshareColumnsorder; ifshareColumnsis reordered,scanShareupdates but this scan silently desyncs. Fix: ascanShareWithUser(s, email, displayName)that callsscanSharethen scans the two extra columns, mirroringscanGardenWithRole.internal/service/shares.go:66—RemoveSharebypasses therequireGardenRolechoke point with its owns.store.GetGarden+g.OwnerID != actorIDcheck. Partly forced by the self-leave semantic (a recipient has no owner role), but there's no comment explaining the divergence, inviting a future reader to "fix" it. A one-line comment ("deliberately not requireGardenRole: a recipient self-leaving has no role to check") would prevent that.internal/service/shares.go:74— The comment's "missing row is ErrNotFound either way" is a property ofDeleteShare(returnsErrNotFoundonRowsAffected == 0), independent of the owner/self-leave split; the comment reads as if the property depends on the branch. Minor misleading wording.internal/service/shares_test.go:12—wantErris local toshares_test.gobut generally useful; the rest of the service suite re-implements the samet.Errorf("... = %v, want ...", err, ...)assertion inline (54 occurrences acrossplants_test.go,objects_test.go,gardens_test.go,plantings_test.go,auth_test.go). Promoting it to a sharedtesting.gowould reduce churn.⚡ Performance — No material issues found
I'll investigate this PR through the performance lens. Let me verify the key performance-sensitive paths.
Let me check
GardenFull,UpdateObject, andUpdatePlantingto see if the new share-lookup call inrequireGardenRoleintroduces N+1 issues on hot paths.Let me check the schema for indexes on
garden_shares(critical for the share-lookup query performance), sincerequireGardenRolenow does that lookup on every garden operation.The schema has appropriate indexes (UNIQUE on
(garden_id, user_id)and separate single-column indexes). TheGetShareRolelookup uses both columns which the UNIQUE index efficiently serves.Let me verify one concern: in
requireGardenRole, the share lookup happens on every garden operation, including the owner case afterGetGarden. Wait — looking again,effectiveGardenRoleshort-circuits withif g.OwnerID == actorIDbefore the share query. Good. So owners don't pay the extra query.But for non-owners on any garden operation (every plantings/objects update by a shared editor), each call to
requireGardenRolenow does 2 queries instead of 1:GetGarden+GetShareRole. AndobjectForRole→requireGardenRole. That's one extra indexed-point query per mutation. That's a reasonable, necessary cost for ACL enforcement — not a regression in the sense of avoidable work.Let me check
GardenFullfor any concern — it callsrequireGardenRoleonce then bulk-loads. Fine.Let me check the
ListGardensForActorSQL — the UNION ALL with theLIMIToutside. Since both subqueries are unbounded, the database must materialize both fully before applyingLIMIT. For an owner with many gardens + many shares, that's two full scans combined. But both halves use indexed predicates (owner_id = ?ands.user_id = ?), andmaxGardensListed=1000caps it. This is fine at household scale, and the comment explicitly says so.Let me check
ListSharesForGarden—ORDER BY u.display_name COLLATE NOCASE, s.id— this requires a sort on the joined result. No index onusers.display_name. But this is bounded by the number of shares per garden (small), so it's not a concern.Let me verify there's no N+1 in the new
UpdatePlantingflow: whenpatch.PlantID != nil, it now callsvisiblePlant(which doesGetPlant) AND thens.store.GetPlantagain unconditionally at line 134. That's a duplicate plant fetch — let me look closer.Confirmed: when
patch.PlantID != nil, the same plant is fetched twice (once byvisiblePlant, once at line 134). This was already the case before this PR (the old code calledvisiblePlantwhich callsGetPlant, then the code at line 134 also calledGetPlant). Wait — let me check the original behavior. The diff shows the old code didplant, err := s.visiblePlant(ctx, actorID, pl.PlantID)unconditionally, and now line 134 callss.store.GetPlantunconditionally. So before: 1 fetch always. After: 1 fetch always (line 134), plus an extra fetch only whenPlantIDis being changed. So this is a small new duplication only on plant-change operations.This is a minor, avoidable extra query on a non-hot path (changing a plop's plant). Not material at household scale, but worth a one-line fix (reuse the plant returned by
visiblePlant). Let me note it but as trivial.Now let me check whether there's a more material concern: every mutation by a shared editor goes through
requireGardenRole→GetGarden+GetShareRole. ForUpdatePlanting/UpdateObject/DeletePlanting/DeleteObject, that's viaobjectForRole→requireGardenRole. One extra indexed single-row query per mutation. Necessary for correctness; not a regression worth flagging.Let me check
ListGardensForActorfor one more concern — the UNION ALL with the outer LIMIT, and the innerSELECT ... 'owner' AS my_role FROM gardens WHERE owner_id = ?— there's noORDER BYinside, so SQLite will produce both result sets and then sort/limit. WithmaxGardensListed=1000, fine. No material issue.Let me verify the
getGardenFullhandler doesn't trigger per-object N+1.No N+1 there. The
GardenFullis well-batched.Let me do a final check on whether the
ListSharesForGardenquery is called from a hot path. It's only called byListShares(owner only, UI-driven). Not hot. Fine.I've completed my performance review. Let me write up findings.
VERDICT: No material issues found
I reviewed the PR through the performance lens, examining:
requireGardenRoleshare lookup (service/gardens.go:66-83,service/gardens.go:87-106): Each garden-scoped operation now potentially issues aGetShareRolequery. Verified thateffectiveGardenRoleshort-circuits viaif g.OwnerID == actorIDbefore the share query, so owners pay no extra cost. Non-owners pay one extra indexed single-row lookup (garden_shareshas a UNIQUE on(garden_id, user_id), confirmed inmigrations/0001_init.sql:66). That is the necessary cost of ACL enforcement on shared gardens, not a regression.ListGardensForActorUNION ALL (store/gardens.go:75-87): The outerLIMITrequires materializing both halves, but each half uses an indexed predicate (owner_id/s.user_id) and the result is capped atmaxGardensListed=1000. Appropriate at the stated household scale; the code comment explicitly defers real pagination to post-v1. Not material.GardenFull(service/objects.go:161-189): Still single authorization check + three batched list queries + a map forDerivedCount. No N+1 introduced.ListSharesForGarden(store/shares.go:45-73): UI-only path, bounded by shares-per-garden. TheORDER BY u.display_name COLLATE NOCASEsort is over a small set; no index needed. Not a hot path.UpdatePlantingduplicate plant fetch (service/plantings.go:129-134): Whenpatch.PlantID != nil,visiblePlantcallsGetPlantand then line 134 callsGetPlantagain on the samepl.PlantID. Pre-PR this fetched the plant once unconditionally viavisiblePlant; post-PR it fetches once unconditionally (line 134) plus a second fetch only when the plant is being changed. This is one extra indexed single-row query on a non-hot mutation path (changing a plop's plant), trivially fixable by reusing the plant returned fromvisiblePlant. I consider this trivial — not material at household scale, and not on a hot loop. Mentioning only for completeness.No blocking or minor performance issues in this lens; the duplicate-plant-fetch is the only thing I'd even note, and it's trivial.
🧯 Error handling & edge cases — No material issues found
I'll review this PR through the error handling & edge cases lens. Let me examine the actual code to verify the diff's behavior.
Looking through the error-handling & edge-cases lens, I verified the new share code paths, the
UpdatePlantingvisibility change, and theUpdateGarden/requireGardenRolerole-stamping by reading the actual files and the schema.VERDICT: No material issues found
I checked the unhappy paths the diff introduces and they hold up:
RemoveShare(service/shares.go:66-77): theg.OwnerID != actorID && actorID != targetUserIDguard correctly masks existence for non-participants withErrNotFound, while letting both the owner-remove-anyone and recipient-self-leave paths through. I confirmed a stranger getsErrNotFoundfromGetGarden(garden missing) or from the guard (garden exists, no access) — same error, no existence leak. A recipient trying to remove another recipient's share also falls into the guard →ErrNotFound(the test covers the stranger variant; the logic is identical for a co-recipient).AddShare(service/shares.go:30-50): ordering is correct — owner check before role validation (so a non-owner probing with a bad role getsErrForbidden, notErrInvalidInput);GetUserByEmail'sErrNotFoundis mapped toErrShareUserNotFound; the UNIQUE race is caught byisUniqueViolation→ErrShareExistsinstore.CreateShare(store/shares.go:86), so two concurrentAddSharecalls for the same pair can't both succeed.UpdatePlanting(service/plantings.go:124-137): the visibility re-check is correctly gated onpatch.PlantID != nil(afterapplyPlantingPatchhas already updatedpl.PlantID), so a shared editor editing radius/label on the owner's private-plant plop no longer fails; the unconditionals.store.GetPlantfor the derived count is safe because theplantings → plantsFK isON DELETE RESTRICT(verified inmigrations/0001_init.sql:126), so a referenced plant always exists. Errors fromGetPlant/visiblePlantare propagated, not swallowed.UpdateGarden(service/gardens.go:149-153): theif updated != nilguard correctly stampsMyRoleon both the success row and the version-conflictcurrentrow returned bystore.UpdateGarden(store/gardens.go:128-133), and correctly skips stamping on the(nil, ErrNotFound)"garden deleted mid-flight" path. No error is swallowed.ListSharesForGarden/ListGardensForActor(store/shares.go:56,store/gardens.go:91): bothdefer rows.Close()and checkrows.Err()after iteration, matching the existing pattern. Empty results return non-nil[]domain.{ShareWithUser,Garden}{}(not JSONnull), which the API test asserts.GetShareRole(store/shares.go:30-41):sql.ErrNoRows→( "", false, nil )is cleanly distinguished from a real DB error;effectiveGardenRole'sdefaultbranch collapses a corrupt role string toroleNone→ErrNotFound, so a bad DB row can't elevate privilege.ListGardensForActorUNION ALL can't double-count a garden for its owner, becauseAddShare'starget.ID == actorIDguard (ErrCannotShareWithSelf) prevents an owner from ever having a share row on their own garden — verified the self-share guard is the only insert path.No swallowed errors, no missing
defer, no panics on nil/empty/zero input, and the masked-existence contract (ErrNotFoundfor both missing and forbidden) is preserved across every new branch.opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Blocking issues found
Blocking issues found
RemoveSharebypasses therequireGardenRoleauthorization choke point, allowing an actor with no role on a garden to reachstore.DeleteShare.internal/service/shares.go:66opens the garden with a rawstore.GetGardeninstead of funneling throughrequireGardenRole— the comment onrequireGardenRolecalls it “THE place authorization for a garden is decided.” A stranger can callRemoveSharewithtargetUserID == actorID; the ad-hoc checkg.OwnerID != actorID && actorID != targetUserIDpasses, so the stranger reachesDeleteShare. While the parameterized query prevents SQL injection and only the actor’s own (non-existent) row would be targeted, the stranger has still bypassed the choke point and can distinguish garden existence via timing (for an existing gardenGetGardensucceeds beforeDeleteShareis reached; for a missing gardenGetGardenfails immediately). More importantly, it breaks the architectural invariant that every garden-touching operation routes throughrequireGardenRole.Fix: Add
g, err := s.requireGardenRole(ctx, actorID, gardenID, roleViewer)at the top ofRemoveShare; then keep the existing owner-or-self check.roleVieweris the right minimum because both owners (who may remove anyone) and shared viewers/editors (who may self-leave) must satisfy it.🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The sharing backend code is clean, consistent with existing patterns, and well-documented. A few notes:
wantErrhelper ininternal/service/shares_test.go:12duplicates the same helper pattern used in other test files, but is isolated to this new file and fits the local style.scanGardenWithRoleininternal/store/gardens.go:28is a clear, minimal extension ofscanGardenthat follows the established scanner pattern.gardenRole.String()method atinternal/service/gardens.go:36cleanly centralizes role string rendering rather than sprinkling string literals.internal/api/errors.goand route registration ininternal/api/api.gofollow the existing conventions exactly.⚡ Performance — No material issues found
No material issues found
🧯 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.