Adds a way to duplicate a garden, so a layout can be forked and experimented on without touching the original.
API
POST /gardens/:id/copy — optional {"name": "..."} body; a blank/absent name derives "<source> (copy)". Returns 201 + the new garden.
Copies the garden's settings, its objects, and each object's currently-planted plops.
Deliberately not carried over:
public_token — the share link is a capability granted to the original; a copy must not silently inherit a live public URL.
garden_shares — a copy is private to whoever made it.
removed plantings — the copy is a fresh layout, not the original's history.
Owner-only, for now
A copy's plops keep pointing at the source'splant_ids, and a custom plant is owned by one user. Letting a viewer copy someone else's garden would hand them a garden referencing plants outside their catalog, and would pin the original owner's plants against deletion (plantings.plant_id is ON DELETE RESTRICT). Copying a shared garden needs a plant-cloning policy first; the owner's own copy is the case that's unambiguously correct today.
Implementation
The whole copy runs in one transaction, so a failure part-way leaves no half-populated garden. The object/planting list scans are factored into queryObjects/queryPlantings over a small queryer interface, so the same scan code serves both a plain read and a read inside that transaction — and each drains its cursor before returning, since SQLite can't hold a cursor open and write on the same connection.
UI
A Copy action on the owner's garden card opens a modal prefilled with the derived name, then lands you in the new garden.
Tests
Service: contents duplicated + source untouched, removed plops skipped, naming (explicit / derived / over-cap rejected / rune-boundary truncation), permissions (stranger 404, editor 403, missing 404), no inherited public link, no inherited shares. API: no-body and named copy, list count, 401/404/400 paths.
Verified locally end-to-end in a browser: create → Copy → modal → lands in the copy.
Adds a way to duplicate a garden, so a layout can be forked and experimented on without touching the original.
## API
`POST /gardens/:id/copy` — optional `{"name": "..."}` body; a blank/absent name derives `"<source> (copy)"`. Returns `201` + the new garden.
Copies the garden's settings, its objects, and each object's **currently-planted** plops.
Deliberately **not** carried over:
- `public_token` — the share link is a capability granted to the original; a copy must not silently inherit a live public URL.
- `garden_shares` — a copy is private to whoever made it.
- removed plantings — the copy is a fresh layout, not the original's history.
## Owner-only, for now
A copy's plops keep pointing at the **source's** `plant_id`s, and a custom plant is owned by one user. Letting a viewer copy someone else's garden would hand them a garden referencing plants outside their catalog, and would pin the original owner's plants against deletion (`plantings.plant_id` is `ON DELETE RESTRICT`). Copying a *shared* garden needs a plant-cloning policy first; the owner's own copy is the case that's unambiguously correct today.
## Implementation
The whole copy runs in one transaction, so a failure part-way leaves no half-populated garden. The object/planting list scans are factored into `queryObjects`/`queryPlantings` over a small `queryer` interface, so the same scan code serves both a plain read and a read inside that transaction — and each drains its cursor before returning, since SQLite can't hold a cursor open and write on the same connection.
## UI
A `Copy` action on the owner's garden card opens a modal prefilled with the derived name, then lands you in the new garden.
## Tests
Service: contents duplicated + source untouched, removed plops skipped, naming (explicit / derived / over-cap rejected / rune-boundary truncation), permissions (stranger 404, editor 403, missing 404), no inherited public link, no inherited shares. API: no-body and named copy, list count, 401/404/400 paths.
Verified locally end-to-end in a browser: create → Copy → modal → lands in the copy.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Adds POST /gardens/:id/copy: duplicates a garden the actor owns, along
with its objects and their currently-planted plops, into a new garden
owned by the actor. A blank name derives "<source> (copy)".
Deliberately not carried over:
* public_token — the share link is a capability granted to the
original; a copy must not silently inherit a live public URL
* garden_shares — a copy is private to whoever made it
* removed plantings — the copy is a fresh layout, not a history
Owner-only for now: a copy's plops keep pointing at the SOURCE's
plant_ids, so letting a viewer copy someone else's garden would hand
them plants outside their catalog and pin the original owner's plants
against deletion (plantings.plant_id is ON DELETE RESTRICT). Copying a
shared garden needs a plant-cloning policy first.
The whole copy runs in one transaction, so a failure part-way leaves no
half-populated garden. The object/planting list scans are factored into
queryObjects/queryPlantings so the same code serves a plain read and a
read inside that transaction.
UI: a Copy action on the owner's garden card opens a modal prefilled
with the derived name, then lands in the new garden.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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-21 03:56:46Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 6 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** — 6 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟠Inconsistent optional-body pattern uses ContentLength instead of io.EOF
error-handling, maintainability · flagged by 4 models
internal/api/gardens.go:120 — The copyGarden handler checks c.Request.ContentLength != 0 to decide whether to parse an optional JSON body, but the codebase already has an established pattern for this in internal/api/public.go:56: try c.ShouldBindJSON(&req) and ignore io.EOF. The ContentLength guard is both inconsistent with surrounding code and less robust (e.g., chunked encoding or HTTP/2 can leave ContentLength == -1 even with no body, which would cause the handler to incorre…
🪰 Gadfly · advisory
🟠 **Inconsistent optional-body pattern uses ContentLength instead of io.EOF**
_error-handling, maintainability · flagged by 4 models_
- `internal/api/gardens.go:120` — The `copyGarden` handler checks `c.Request.ContentLength != 0` to decide whether to parse an optional JSON body, but the codebase already has an established pattern for this in `internal/api/public.go:56`: try `c.ShouldBindJSON(&req)` and ignore `io.EOF`. The `ContentLength` guard is both inconsistent with surrounding code and less robust (e.g., chunked encoding or HTTP/2 can leave `ContentLength == -1` even with no body, which would cause the handler to incorre…
<sub>🪰 Gadfly · advisory</sub>
🟡Misleadingly-named dead variable kept in TestCopyGardenSkipsRemovedPlantings — it is actually cleared, not kept
maintainability · flagged by 1 model
internal/service/gardens_test.go:348 (TestCopyGardenSkipsRemovedPlantings) — the local var kept (declared line 348, silenced via _ = kept at line 364) is misleadingly named and is dead code. It's created, never asserted on, and then discarded just to silence "declared and not used." Worse, the name actively misleads: ClearObjectPlantings (internal/store/plantings.go:76-92), invoked via ClearObject (internal/service/ops.go:221-227), soft-removes every active plop on the object…
🪰 Gadfly · advisory
🟡 **Misleadingly-named dead variable `kept` in TestCopyGardenSkipsRemovedPlantings — it is actually cleared, not kept**
_maintainability · flagged by 1 model_
- `internal/service/gardens_test.go:348` (`TestCopyGardenSkipsRemovedPlantings`) — the local var `kept` (declared line 348, silenced via `_ = kept` at line 364) is misleadingly named and is dead code. It's created, never asserted on, and then discarded just to silence "declared and not used." Worse, the name actively misleads: `ClearObjectPlantings` (`internal/store/plantings.go:76-92`), invoked via `ClearObject` (`internal/service/ops.go:221-227`), soft-removes *every* active plop on the object…
<sub>🪰 Gadfly · advisory</sub>
🟡Unused variable suppressed with _ = kept instead of using _ in original assignment
maintainability · flagged by 3 models
internal/service/gardens_test.go:364 (in TestCopyGardenSkipsRemovedPlantings) — The test assigns a planting to kept only to suppress it with _ = kept. Using _, err := s.CreatePlanting(...) on that line avoids the unused-variable workaround.
🪰 Gadfly · advisory
🟡 **Unused variable suppressed with _ = kept instead of using _ in original assignment**
_maintainability · flagged by 3 models_
- `internal/service/gardens_test.go:364` (in `TestCopyGardenSkipsRemovedPlantings`) — The test assigns a planting to `kept` only to suppress it with `_ = kept`. Using `_, err := s.CreatePlanting(...)` on that line avoids the unused-variable workaround.
<sub>🪰 Gadfly · advisory</sub>
🟠CopyGarden inserts objects/plantings one row at a time inside a single write transaction, serializing all other writers under WAL for O(n) round-trips
maintainability, performance · flagged by 2 models
internal/store/gardens.go:216-253 — CopyGarden executes one INSERT ... RETURNING per source object and one INSERT per active planting, sequentially, inside a single write transaction. There is no cap on how many objects/plantings a garden can hold (maxGardensListed at internal/store/gardens.go:50 only bounds the list endpoint). The DB is opened in WAL mode with busy_timeout(5000) (internal/store/sqlite.go:20-21), so only one writer can proceed app-wide; every other user's wri…
🪰 Gadfly · advisory
🟠 **CopyGarden inserts objects/plantings one row at a time inside a single write transaction, serializing all other writers under WAL for O(n) round-trips**
_maintainability, performance · flagged by 2 models_
- `internal/store/gardens.go:216-253` — `CopyGarden` executes one `INSERT ... RETURNING` per source object and one `INSERT` per active planting, sequentially, inside a single write transaction. There is no cap on how many objects/plantings a garden can hold (`maxGardensListed` at `internal/store/gardens.go:50` only bounds the *list* endpoint). The DB is opened in WAL mode with `busy_timeout(5000)` (`internal/store/sqlite.go:20-21`), so only one writer can proceed app-wide; every other user's wri…
<sub>🪰 Gadfly · advisory</sub>
⚪Generic queryer interface declared in users.go rather than a shared store file
maintainability · flagged by 1 model
internal/store/gardens.go:217 and internal/store/plantings.go:112/:135 — the INSERT INTO garden_objects … and INSERT INTO plantings … column lists are duplicated verbatim between CreateObject/CreatePlanting and the in-transaction copy in CopyGarden. The copy can't call CreateObject/CreatePlanting directly because those bind to d.sql rather than tx, so the duplication is a deliberate tradeoff, but it means a future schema/column change must be edited in two (objects) / t…
🪰 Gadfly · advisory
⚪ **Generic queryer interface declared in users.go rather than a shared store file**
_maintainability · flagged by 1 model_
- `internal/store/gardens.go:217` and `internal/store/plantings.go:112`/`:135` — the `INSERT INTO garden_objects …` and `INSERT INTO plantings …` column lists are duplicated verbatim between `CreateObject`/`CreatePlanting` and the in-transaction copy in `CopyGarden`. The copy can't call `CreateObject`/`CreatePlanting` directly because those bind to `d.sql` rather than `tx`, so the duplication is a deliberate tradeoff, but it means a future schema/column change must be edited in two (objects) / t…
<sub>🪰 Gadfly · advisory</sub>
🟠Client-side copy-name derivation diverges from server truncation logic
correctness, error-handling, maintainability · flagged by 3 models
web/src/components/gardens/CopyGardenModal.tsx:20 — The modal pre-derives the copy name with `${garden.name} (copy)`, duplicating the server's copyName() logic but without the byte-cap truncation. If the source name is long, the prefilled default can exceed maxGardenNameLen and will be rejected by the service when submitted unchanged. The client and server derivations are out of sync. Fix: either share the truncation logic (e.g., a common util) or prefill with a name capped the…
🪰 Gadfly · advisory
🟠 **Client-side copy-name derivation diverges from server truncation logic**
_correctness, error-handling, maintainability · flagged by 3 models_
- `web/src/components/gardens/CopyGardenModal.tsx:20` — The modal pre-derives the copy name with `` `${garden.name} (copy)` ``, duplicating the server's `copyName()` logic but without the byte-cap truncation. If the source name is long, the prefilled default can exceed `maxGardenNameLen` and will be rejected by the service when submitted unchanged. The client and server derivations are out of sync. **Fix:** either share the truncation logic (e.g., a common util) or prefill with a name capped the…
<sub>🪰 Gadfly · advisory</sub>
All static constants — no injection surface. The security-relevant design (owner-only copy, no token/share inheritance, parameterized SQL throughout) checks out and is well-tested. Nothing material in my lens.
VERDICT: No material issues found
Authorization: CopyGarden (internal/service/gardens.go:172) correctly gates on roleOwner via requireGardenRole, which returns ErrNotFound for no-access and ErrForbidden for insufficient access (viewer/editor) — verified against requireGardenRole/effectiveGardenRole (internal/service/gardens.go:69-105) and exercised by TestCopyGardenPermissions.
IDOR/cross-tenant reference risk: the PR's stated rationale for owner-only copying (avoiding a copy whose plops reference another user's plant_ids, and avoiding pinning those plants against ON DELETE RESTRICT) is actually enforced in code, not just asserted in comments.
SQL injection: all queries in internal/store/gardens.goCopyGarden use parameterized ? placeholders; gardenColumns/objectColumns/plantingColumns/qualifyColumns are static Go constants, not built from request input — no injection surface.
Secret/capability leakage: public_token is deliberately excluded from the INSERT ... SELECT column list in CopyGarden (internal/store/gardens.go:172), and the column has no default other than NULL, so a copy starts with no public link — confirmed via migration 0004_garden_public_token.sql and TestCopyGardenDoesNotInheritPublicLink. garden_shares is likewise never touched, confirmed by TestCopyGardenDoesNotInheritShares.
CSRF: the new route sits under the existing v1 group with h.csrfGuard() applied at the group level (internal/api/api.go), so it isn't missing CSRF protection that other mutating endpoints have.
Input handling: name is length-capped (maxGardenNameLen) and rejected rather than silently truncated when explicitly supplied; the request body is optional and a non-string name is rejected by ShouldBindJSON with 400. The ContentLength != 0 check (internal/api/gardens.go) can mis-trigger on chunked requests with ContentLength == -1, but that's a correctness/robustness nit, not a security issue (worst case is a spurious 400), so it's outside this lens.
🎯 Correctness — Minor issues
Confirms GardenFormModal (create/edit) also has no maxLength cap, so a garden name up to 200 bytes is indeed creatable through the normal UI, then reachable via copy. The finding fully checks out.
Corrected review
VERDICT: Minor issues
web/src/components/gardens/CopyGardenModal.tsx:20 — the modal prefills the name field with `${garden.name} (copy)` with no length cap, but the server (internal/service/gardens.go:175-181) only applies the graceful copyName truncation when the incoming name is blank; a non-blank name over maxGardenNameLen (200 bytes, internal/service/gardens.go:20) hits the strict domain.ErrInvalidInput path instead. Since GardenFormModal.tsx also has no maxLength on the name field (confirmed no maxLength prop anywhere in web/src/components/gardens/GardenFormModal.tsx or web/src/components/ui/TextField.tsx), a garden name legitimately created near the 200-byte cap becomes uncopyable without the user first editing the prefilled field — appending " (copy)" (7 bytes) pushes it over the cap and the request is rejected with a 400, surfaced to the user as "Could not copy the garden."
Fix: cap the client-side default to the same room copyName leaves (mirroring its truncation logic), or have the service truncate rather than reject over-cap names on this endpoint.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
internal/service/gardens_test.go:348 (TestCopyGardenSkipsRemovedPlantings) — the local var kept (declared line 348, silenced via _ = kept at line 364) is misleadingly named and is dead code. It's created, never asserted on, and then discarded just to silence "declared and not used." Worse, the name actively misleads: ClearObjectPlantings (internal/store/plantings.go:76-92), invoked via ClearObject (internal/service/ops.go:221-227), soft-removes every active plop on the object with no per-row selectivity — so kept is cleared right along with the other, unnamed planting at line 357's ClearObject call. It is not "kept" at all. A future reader skimming this test could easily conclude one plop survives the clear while the other doesn't, which isn't what's being tested. Drop the variable (mirror the if _, err := s.CreatePlanting(...) form already used two lines below for the plop that's supposed to be cleared) or rename it to something that doesn't imply survival, e.g. clearedA/clearedB.
internal/store/gardens.go:216-230 and internal/store/gardens.go:246-252 — CopyGarden's per-row inserts duplicate the exact INSERT SQL and argument list already living in CreateObject (internal/store/objects.go:38-46) and CreatePlanting/CreatePlantings (internal/store/plantings.go:110-121, :135-137), just issued against tx instead of d.sql. The PR already introduces exactly the abstraction that would fix this on the read side (queryer in internal/store/users.go:24-26, shared by queryObjects/queryPlantings); doing the analogous thing for these inserts (an execer-style interface satisfied by both *sql.DB and *sql.Tx, with CreateObject/CreatePlanting parameterized over it) would have let CopyGarden call the existing helpers instead of re-typing the SQL. This does follow a pre-existing precedent in the codebase (CreatePlantings already duplicates CreatePlanting's INSERT for its tx-batch case), so it's not a new pattern this PR invented, but the PR had the ideal opportunity — and the tooling already in hand — to close that gap instead of extending it to a third copy.
⚡ Performance — Minor issues
All claims check out: the loop structure (one INSERT...RETURNING per object, one INSERT per planting, lines 216-253), the WAL + busy_timeout(5000) pragmas (sqlite.go:20-21), the absence of any per-garden object/planting cap, maxGardensListed only bounding the list endpoint (gardens.go:50), and the indexes existing in the migration. The finding is accurate.
VERDICT: Minor issues
internal/store/gardens.go:216-253 — CopyGarden executes one INSERT ... RETURNING per source object and one INSERT per active planting, sequentially, inside a single write transaction. There is no cap on how many objects/plantings a garden can hold (maxGardensListed at internal/store/gardens.go:50 only bounds the list endpoint). The DB is opened in WAL mode with busy_timeout(5000) (internal/store/sqlite.go:20-21), so only one writer can proceed app-wide; every other user's write queues behind busy_timeout while this transaction is open. For a garden with many objects/plantings, the copy becomes N+M sequential round-trips held under one write lock — the copy's own latency scales linearly with garden size, and it serializes all other users' writes for that whole window (a concurrent write from an unrelated user can block or fail with SQLITE_BUSY). A multi-row INSERT ... VALUES (...),(...),... RETURNING ... (or chunked batching) would cut the round-trip count from O(n) to O(1) for both phases. Severity is moderate since it requires an unusually large garden to bite in practice.
🧯 Error handling & edge cases — Minor issues
This confirms the store/service code matches the draft's description exactly — transaction handling, error wrapping, sql.ErrNoRows mapping, and copyName's rune-safe truncation are all as claimed. The only actionable finding is the ContentLength check in gardens.go:120, which I independently verified: Go's server sets ContentLength == -1 (not 0) for a request with Transfer-Encoding: chunked and no Content-Length header, so c.Request.ContentLength != 0 is true for an unknown-length body, sending it into ShouldBindJSON where an empty chunked body yields io.EOF, incorrectly rejected as 400. The sibling handler createShareLink (public.go:56) already uses the correct err != nil && !errors.Is(err, io.EOF) pattern, confirming this is a real regression/inconsistency, not a false positive.
VERDICT: Minor issues
internal/api/gardens.go:120 — copyGarden gates JSON body parsing on c.Request.ContentLength != 0, but Go sets ContentLength to -1 (not 0) when the body length is unknown (e.g. Transfer-Encoding: chunked with no body). In that case the handler calls ShouldBindJSON on an effectively empty body, which fails with io.EOF, and gets turned into a 400 INVALID_INPUT "name must be a string" — incorrectly rejecting a legitimate no-body copy request. The sibling handler createShareLink (internal/api/public.go:56) already implements this correctly: if err := c.ShouldBindJSON(&req); err != nil && !errors.Is(err, io.EOF) { ... }, which is agnostic to how the empty body was framed. copyGarden reinvents the optional-body check with a strictly weaker condition. The existing test (gardens_test.go) doesn't catch this because httptest.NewRequest always sets ContentLength to the exact buffer length (0 for a nil/empty body), so the chunked/-1 path is untested.
Fix: mirror the public.go pattern — always call ShouldBindJSON and ignore only io.EOF, rather than gating on ContentLength.
Through the security lens, I verified the copy feature's authorization, capability-leakage, input-validation, and injection surfaces against the actual code:
Authorization (authz): CopyGarden calls s.requireGardenRole(ctx, actorID, gardenID, roleOwner) (internal/service/gardens.go:171), so strangers get ErrNotFound (→ 404) and non-owner editors get ErrForbidden (→ 403), matching the stated owner-only policy. The route is registered under gardens := v1.Group("/gardens", h.requireAuth()) (internal/api/api.go:78), so unauthenticated requests are rejected before the handler runs.
Actor identity is not user-controllable: mustActor(c).ID is derived from the session (internal/api/auth.go:209 via actorKey), not from the request body or query; the new garden's owner_id is set to this trusted actorID in the store (internal/store/gardens.go:191, ownerID param). No IDOR path.
Capability leakage — public_token: The garden-copy INSERT … SELECT explicitly enumerates columns (owner_id, name, width_cm, height_cm, unit_pref, notes, grid_size_cm, snap_to_grid) and does not include public_token (internal/store/gardens.go:192-197). Since public_token defaults to NULL (migration 0004), the copy starts with no public link. Confirmed it cannot silently inherit a live capability.
Capability leakage — garden_shares: The store copy transaction never inserts into garden_shares, so the copy is private to the copier. Confirmed.
Removed-planting history: The planting scan filters pl.removed_at IS NULL (internal/store/gardens.go:225), so soft-removed plops aren't carried over — no history/capability bleed.
Input validation: The name is TrimSpace'd, blank-derived, and length-capped at maxGardenNameLen (200) with ErrInvalidInput on overflow (internal/service/gardens.go:175-180); copyName truncates on a rune boundary via ToValidUTF8 to keep results valid UTF-8 (internal/service/gardens.go:193-198). The handler rejects non-string name with 400 (internal/api/gardens.go:118). No unbounded/unvalidated string reaches storage.
Injection: All SQL uses parameterized placeholders (?) for every user- or source-derived value across the garden, object, and planting inserts (internal/store/gardens.go:192-237); no string interpolation of untrusted input into a query. The column-list constants (gardenColumns, objectColumns) are compile-time string literals, not user input.
No security issues found within this lens.
🎯 Correctness — No material issues found
Verdict: No material issues found
I reviewed the change through the correctness lens, verifying the schema, column lists, truncation math, and transaction flow against the checked-out code.
copyName truncation math (internal/service/gardens.go:193): room = maxGardenNameLen(200) - len(" (copy)")(7) = 193. When len(src) > 193, it slices src[:193], runs ToValidUTF8(..., "") (empty replacement → only shrinks, never grows, and repairs any rune split at the byte boundary) then TrimSpace (only shrinks). Result + suffix ≤ 193 + 7 = 200 = cap. Verified against gardenFromInput's len(name) > maxGardenNameLen byte-length check — units are consistent (bytes, not runes). The TestCopyGardenNameRuneBoundary case (100 × "é" = 200 bytes) truncates to 193 bytes = 96 complete é + a dangling lead byte that ToValidUTF8 drops → 192 + 7 = 199 ≤ 200, valid UTF-8. Correct.
Garden row copy (internal/store/gardens.go:189): INSERT ... SELECT copies width_cm, height_cm, unit_pref, notes, grid_size_cm, snap_to_grid and supplies owner_id, name; version, created_at, updated_at, and public_token fall back to defaults (so no inherited share link, fresh version=1). Verified against migrations/0004 (public_token is a nullable added column, not in gardenColumns) and 0001_init (version default 1). Correct.
Object copy (internal/store/gardens.go:217): 17 insert columns match the 17 placeholders and match CreateObject's column list at objects.go:38; version/timestamps default. z_index is copied per-row so the ORDER BY id scan (vs. the live list's z_index, id) doesn't affect correctness. Correct.
Planting copy (internal/store/gardens.go:247): query joins source objects (o.garden_id = srcID) and filters removed_at IS NULL, so only active plops are read and they reference source object IDs; the objectIDs map re-parents each onto the copied object. removed_at is intentionally not inserted (defaults NULL = active). count/label/planted_at (all nullable) are passed through. Correct.
TOCTOU on source existence: requireGardenRole reads the garden, then store.CopyGarden re-checks via INSERT ... SELECT ... WHERE id = ?; a vanished source yields sql.ErrNoRows → domain.ErrNotFound. Correct.
queryer interface + cursor draining: *sql.Tx and d.sql both satisfy queryer; queryObjects/queryPlantings fully drain and defer rows.Close() before returning, so the subsequent writes on the same tx connection are safe. Verified at objects.go:78 and plantings.go:44. Correct.
ListActivePlantingsForObject refactor (plantings.go:68): now routes through queryPlantings with unqualified plantingColumns; scanPlanting scans by position so the unqualified select still lines up. Correct.
No correctness defects found in this lens.
🧹 Code cleanliness & maintainability — Minor issues
All three findings are confirmed against the actual code:
gardens.go:217 — The INSERT INTO garden_objects column list at gardens.go:217-221 is verbatim-duplicated from CreateObject at objects.go:38-42. The plantings INSERT column list at gardens.go:247-248 is duplicated from both CreatePlanting (plantings.go:112-114) and CreatePlantings (plantings.go:135-137) — three places total. Confirmed.
users.go:22 — The queryer interface is declared at users.go:22-26 and is a generic store-wide abstraction. (Note: scanner also lives in users.go:18, so the draft's "or the file holding scanner" phrasing is slightly off, but the core point — queryer is a shared helper misplaced in a user-specific file — holds.) Confirmed as trivial.
gardens_test.go:364 — kept is assigned at line 348 and only referenced via _ = kept at line 364. Confirmed.
Verdict: Minor issues
internal/store/gardens.go:217 and internal/store/plantings.go:112/:135 — the INSERT INTO garden_objects … and INSERT INTO plantings … column lists are duplicated verbatim between CreateObject/CreatePlanting and the in-transaction copy in CopyGarden. The copy can't call CreateObject/CreatePlanting directly because those bind to d.sql rather than tx, so the duplication is a deliberate tradeoff, but it means a future schema/column change must be edited in two (objects) / three (plantings) places. A small shared insertObjectSQL/insertPlantingSQL constant for the (cols) VALUES (?,?,…) RETURNING … text would keep them in sync at zero behavioral cost. Verified by reading both files.
internal/store/users.go:22 — the new queryer interface is a generic store-wide abstraction but is declared in users.go, which is otherwise about user rows. For a shared store helper this is a mildly leaky/misplaced home; a store.go would be the natural spot and would make discovery easier. Verified by reading the file.
internal/service/gardens_test.go:364 — kept is assigned from CreatePlanting and then only referenced via _ = kept to silence the unused-variable check. Just dropping the assignment (_, err := s.CreatePlanting(...)) or naming it _ would be cleaner than the throwaway _ = kept line. Verified by reading the test.
No dead code, no confusing names, no over-long functions, and the new code follows the surrounding patterns (error wrapping prefixes, defer tx.Rollback(), boolToInt, column constants) consistently.
⚡ Performance — No material issues found
No material issues found
I verified the store implementation (internal/store/gardens.go:179-259) and the refactored query helpers (internal/store/objects.go, internal/store/plantings.go).
Through the performance lens:
The copy does N individual INSERT ... RETURNING for objects and M individual INSERTs for plantings (gardens.go:215-253). This is N+M round-trips rather than a single batched copy, but it's an owner-initiated, one-shot operation (not a hot path), and the per-row RETURNING is genuinely needed to build the source→dest objectIDs map for planting re-parenting. SQLite offers no clean batched-RETURNING-with-remapping alternative here, so this is inherent, not a regression.
queryObjects/queryPlantings fully materialize the source rows into slices before any write (gardens.go:206-237). That's required by the SQLite single-connection cursor constraint the PR calls out, and the make(map[int64]int64, len(srcObjects)) pre-sizing avoids reallocation churn. Memory is O(objects + plantings), unbounded in principle, but bounded in practice by a single garden's contents on a user-triggered action.
The whole copy runs in one transaction, holding SQLite's single write lock for its duration. For a large garden this serializes other writers briefly, but again it's a user-initiated copy, not a read-path or list-path concern.
No hot-path regressions, no N+1 on a list/read path, no missing pagination on a frequently-hit endpoint. Nothing material to flag in this lens.
🧯 Error handling & edge cases — Minor issues
The finding's reasoning about Go's net/http semantics is correct: Request.ContentLength is -1 for chunked transfers, so != 0 is true, and ShouldBindJSON on an empty body returns io.EOF → the 400 path. The code at line 120 matches the draft's description. This is a legitimate, low-severity edge case.
VERDICT: Minor issues found
internal/api/gardens.go:120 — The "body absent" detection uses c.Request.ContentLength != 0. For a chunked-transfer POST (Transfer-Encoding: chunked, no Content-Length), Go sets Request.ContentLength = -1, so != 0 is true and the handler falls into ShouldBindJSON on an empty body, which returns io.EOF → a 400 "name must be a string". A genuinely empty chunked body should behave like "no body" (derive the default name), but instead it's rejected. Low-severity edge case (chunked empty POSTs are unusual, and rejecting one isn't dangerous), but it is an inconsistent unhappy path. The cleanest fix is to attempt the decode only when a body is actually present (e.g. peek c.Request.Body), or gate on ContentLength > 0 so -1 falls through to the default-name path (noting that would silently swallow a malformed chunked body).
From the 🔒 Security lens, the change is well-constructed:
Authn/Authz: The POST /gardens/:id/copy route sits behind the existing requireAuth() middleware. The service layer enforces owner-only access via requireGardenRole(..., roleOwner) — strangers receive ErrNotFound (information hiding) and editors receive ErrForbidden, matching the PR's deliberate access policy.
SQL injection: All store queries use ? placeholders. The column-list constants (gardenColumns, objectColumns, plantingColumns) are hard-coded, never derived from user input.
Transaction consistency / TOCTOU: The entire copy runs inside a single *sql.Tx. Reads of source objects and plantings are drained before writes, respecting SQLite's same-connection cursor limitation, so the transaction snapshot stays consistent.
Sensitive-data exclusion: public_token and garden_shares are intentionally not copied, and the store's INSERT ... SELECT only copies the explicitly listed non-sensitive metadata columns. The resulting garden starts private.
Input validation: The parseIDParam helper rejects non-numeric and non-positive IDs with 400. The name is trimmed and capped at maxGardenNameLen bytes; copyName uses strings.ToValidUTF8 to avoid splitting multi-byte runes when trimming for the suffix.
No SSRF / deserialization / secret-leakage paths: The feature is an entirely internal database duplication with no external network calls or unmarshalling of untrusted complex types.
🎯 Correctness — No material issues found
No material issues found
The implementation is correct through the correctness lens. I verified:
Transaction integrity: The CopyGarden store method uses BeginTx with defer tx.Rollback() and a final tx.Commit(), ensuring no half-populated garden survives a failure.
Cursor draining: queryObjects and queryPlantings both fully consume and close their *sql.Rows via defer rows.Close() before returning, satisfying the SQLite constraint that you cannot hold a read cursor open while writing on the same connection within a transaction.
Rune-boundary truncation: copyName uses strings.ToValidUTF8 after byte slicing and before appending the suffix, guaranteeing valid UTF-8. The test TestCopyGardenNameRuneBoundary confirms this.
Length invariant: copyName always produces len(result) <= maxGardenNameLen because it only truncates when len(src) > (maxGardenNameLen - len(suffix)), and the final result is at most room + len(suffix) == maxGardenNameLen.
Version reset: Neither the garden INSERT...SELECT nor the object/planting INSERTs copy version, so the schema defaults (DEFAULT 1) give the copy fresh version 1 values, matching the test expectation.
Excluded fields: public_token and garden_shares are deliberately omitted from copying; version, created_at, updated_at are reset via defaults; only active plantings (removed_at IS NULL) are copied.
Permission check: The service calls requireGardenRole(ctx, actorID, gardenID, roleOwner), correctly restricting copying to owners and returning ErrForbidden for editors and ErrNotFound for strangers, as verified by tests.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/components/gardens/CopyGardenModal.tsx:20 — The modal pre-derives the copy name with `${garden.name} (copy)`, duplicating the server's copyName() logic but without the byte-cap truncation. If the source name is long, the prefilled default can exceed maxGardenNameLen and will be rejected by the service when submitted unchanged. The client and server derivations are out of sync. Fix: either share the truncation logic (e.g., a common util) or prefill with a name capped the same way the server does.
⚡ Performance — No material issues found
No material issues found
Reviewed through the Performance lens. The deep-copy implementation in internal/store/gardens.go does materialize all source objects and plantings in memory before writing, and performs individual INSERT statements per object and per planting inside a single transaction. While this is technically an N+1 insert pattern, it is bounded by garden size (user-managed, typically small), runs on an explicit user action (not a hot path), and correctly drains cursors before writing to respect SQLite's same-connection limitations. No hot-loop allocations, unbounded growth, missing limits, or blocking call regressions were identified.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/components/gardens/CopyGardenModal.tsx:20 — The modal prefills the copy name as `${garden.name} (copy)` without applying the server's 200-byte name-length cap (maxGardenNameLen). If the source garden name is 194–200 bytes, the derived name exceeds the limit and the server rejects it with ErrInvalidInput (400). The user opens the modal to a default value that is guaranteed to fail. Fix: Reuse the backend truncation logic in the frontend (cap source name to make room for the 7-byte " (copy)" suffix, trimming on a valid UTF-8 boundary), or derive the default name from the API rather than precomputing it.
I reviewed this PR strictly through the security lens (authn/authz, injection, secret leakage, input validation, unsafe untrusted data). I verified each concern against the checked-out code, not just the diff:
Authz (owner-only) — CopyGarden calls s.requireGardenRole(ctx, actorID, gardenID, roleOwner) (internal/service/gardens.go:171), and requireGardenRole returns ErrNotFound for users with no role and ErrForbidden for insufficient role (internal/service/gardens.go:69-86). Strangers don't even learn the garden exists; the test confirms stranger→404 and editor→403. ✓
Route is auth-gated — POST /gardens/:id/copy is registered inside the gardens group under h.requireAuth() (internal/api/api.go:78,84), so anonymous → 401, and POST is covered by csrfGuard (internal/api/api.go:50). ✓
No public_token inheritance — The INSERT INTO gardens at internal/store/gardens.go:190-193 lists columns explicitly and omits public_token; the column defaults to NULL (internal/store/migrations/0004_garden_public_token.sql:5). gardenColumns also excludes it, so it can't leak via the API response. The copy starts with no live share URL. ✓
No garden_shares inheritance — Only gardens, garden_objects, and active plantings are inserted; no shares rows. ✓
plant_id reuse is owner-scoped — The PR's stated reason for owner-only is that copied plops keep the source's plant_ids. Since authorization is enforced as owner-only, the actor copying is the owner of those plants, so this doesn't hand a stranger a reference to someone else's catalog. The ON DELETE RESTRICT consequence is a correctness/feature-scoping concern (acknowledged in the design), not a security bypass. ✓
SQL injection — Every query uses ? placeholders for user-supplied values (name, srcID, ownerID). The string-concatenated bits (gardenColumns, objectColumns, plantingColumns, qualifyColumns(...)) are compile-time constants, not request input. ✓
ID + body validation — parseIDParam rejects non-numeric ids → 400 (internal/api/errors.go:77); the optional body is only decoded when ContentLength != 0, and ShouldBindJSON rejects a non-string name (e.g. {"name":42}) → 400, as tested. Over-long names → ErrInvalidInput → 400 (internal/service/gardens.go:179-181). Trusting ContentLength here is a minor optimization, not a vulnerability — an attacker can't exploit it to bypass validation. ✓
No mass-assignment — owner_id in the copy is supplied as the mustActor(c).ID from the session, not from the request body; created.MyRole is set server-side to roleOwner (internal/service/gardens.go:186). The request body only carries name. ✓
No XSS in the UI — CopyGardenModal.tsx renders garden.name and created.name as React text content (no dangerouslySetInnerHTML), so the name is escaped. ✓
Nothing in my lens turned up a material issue.
🎯 Correctness — No material issues found
I'll verify the key claims in the draft review against the actual code.
The code matches the draft's claims. Let me verify the supporting functions referenced (requireGardenRole, boolToInt, qualifyColumns, cursor closing).
All supporting functions exist. The draft's verification is accurate. The draft found no material issues and verified each claim against actual code — which I've now independently confirmed. The gadfly-findings array is empty, matching the draft.
No material issues found.
copyName truncation math (service/gardens.go:193-199): confirmed maxGardenNameLen=200, suffix 7 bytes, room=193. Boundary cases hold; multi-byte rune split repaired by ToValidUTF8(..., "") (empty replacement, only shrinks) then TrimSpace. Cannot exceed 200 bytes.
Object copy (store/gardens.go:216-225): column list matches objectColumns; boolToInt applied to the two INTEGER boolean columns; z_index preserved. boolToInt confirmed defined at store/users.go:173.
Planting copy (store/gardens.go:246-249): inserts object_id (remapped via objectIDs), plant_id, x_cm, y_cm, radius_cm, count, label, planted_at; omits removed_at (NULL → active, consistent with the removed_at IS NULL filter), version (1), timestamps. The "unreachable" missing-object branch is genuinely unreachable given the join.
Authorization (service/gardens.go:170-171): requireGardenRole(..., roleOwner) confirmed at service/gardens.go:69; masks non-owners as ErrNotFound and insufficient-role as ErrForbidden, matching test expectations.
Handler body parsing (api/gardens.go:114-122): ContentLength != 0 skips binding for empty bodies while still binding chunked (ContentLength == -1) bodies; wrong-typed name → 400.
Transaction integrity (store/gardens.go:180-258): defer tx.Rollback() + explicit tx.Commit(); all writes via tx. queryObjects/queryPlantings confirmed defined at store/objects.go:78 and store/plantings.go:44 (drain+close before subsequent writes), respecting SQLite's no-concurrent-cursor constraint.
qualifyColumns confirmed at store/sqlite.go:105.
🧹 Code cleanliness & maintainability — Minor issues
I'll verify each finding against the actual code.
All three findings verified. Now let me check the useCopyGarden hook to confirm finding #1's claim about the UI always sending { name: name ?? '' }.
Confirmed: api.post with { name: name ?? '' } always sends a body, so the API's ContentLength != 0 branch is always hit for the UI client. The component passes name.trim() which is non-empty (button disabled if empty), but the body is sent regardless. Finding #1 verified.
All three findings are confirmed against the code. Let me also verify the test naming concern (finding #3) — kept is indeed soft-removed by ClearObject (line 357) and never asserted.
Verified. The three findings survive:
internal/api/gardens.go:120 — UI always sends { name: name ?? '' } via useCopyGarden, so the ContentLength != 0 branch is dead for the web client (only the curl/test path exercises the no-body branch).
web/src/components/gardens/CopyGardenModal.tsx:20 — " (copy)" suffix hardcoded client-side; copyName in internal/service/gardens.go defines it server-side. Duplication confirmed.
internal/service/gardens_test.go:364 — _ = kept discards the variable; kept was created at line 350, then ClearObject (line 357) soft-removes all plantings including kept, and kept is never used in any assertion.
Minor issues
internal/api/gardens.go:120 — the c.Request.ContentLength != 0 branch advertises an optional-body contract, but the only real caller (useCopyGarden in web/src/lib/gardens.ts:98) always posts { name: name ?? '' }, so the no-body path is only exercised by the curl/test caller. The API surface and the UI client disagree on which contract is authoritative; a future reader will wonder which is canonical. Cosmetic leaky-abstraction smell, not blocking.
web/src/components/gardens/CopyGardenModal.tsx:20 — useState(\${garden.name} (copy)`)reconstructs the server'scopyNamederivation client-side (the" (copy)"suffix lives ininternal/service/gardens.go`). The component comment claims the prefill is "the server's own default so what you see is what you get," but no server call fetches it — the suffix is duplicated. If the server's suffix or truncation rule changes, the UI's prefill silently drifts; the test suite asserts the suffix as a contract, so the duplication is a latent maintainability trap.
internal/service/gardens_test.go:364 — _ = kept discards an assigned-but-unused variable. kept is created at line 350, then soft-removed by ClearObject at line 357, and never asserted. The name kept is misleading (it's actually the cleared plop) and the discard is cosmetic noise. Rename to reflect its fate or drop the assignment and use _ at creation.
⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens:
The copy path is O(objects + plantings) with one round-trip per object and one per planting insert (internal/store/gardens.go:215-253). This is linear, not quadratic, and it's a user-initiated one-shot operation rather than a hot path, so the per-row INSERT overhead is acceptable. A bulk/INSERT ... SELECT rewrite could reduce round-trips but isn't material here.
queryObjects/queryPlantings fully drain their cursors before the transaction issues writes (internal/store/objects.go:78-97, internal/store/plantings.go:44-63), correctly avoiding the SQLite "open cursor + write on same connection" trap. The reads happen once each, before the insert loops — no cursor-held-during-write regression.
requireGardenRole reads the source garden row, then the store's INSERT ... SELECT reads it again (internal/service/gardens.go:171, internal/store/gardens.go:189-195). One extra single-row read on a non-hot path; not worth changing.
The two in-memory slices (srcObjects, srcPlantings) and the objectIDs map scale with garden size. Bounded by a single garden's contents on a copy action; no unbounded/paginated-growth concern.
I verified these by reading internal/store/gardens.go, internal/store/objects.go, internal/store/plantings.go, and internal/service/gardens.go at the checked-out state.
🧯 Error handling & edge cases — No material issues found
Now let me look at the copyName rune-boundary truncation carefully. The truncation uses src[:room] which slices by bytes, then ToValidUTF8 cleans up. But it trims to room bytes, not on a rune boundary. Let me check the test case.
Let me check the UI error handling for the modal and the API.
Now let me re-examine copyName more carefully. maxGardenNameLen = 200, suffix = " (copy)" = 7 bytes, room = 193. If len(src) > 193, truncate to src[:193] (byte slice, possibly splitting a rune), then ToValidUTF8 replaces invalid sequences with empty string. But TrimSpace is applied AFTER ToValidUTF8 — wait, no: strings.TrimSpace(strings.ToValidUTF8(src[:room], "")). So ToValidUTF8 cleans partial rune, then TrimSpace trims whitespace. Then src + suffix. Length check: result ≤ room + len(suffix) = 200. OK.
But there's a subtle issue: if src[:room] ends mid-rune AND the rune before is a valid character, ToValidUTF8 drops the partial bytes. Fine. If room falls such that the last byte is part of a multi-byte rune boundary that includes whitespace... edge cases are handled. Tests pass. The truncation logic seems correct.
Let me check the queryPlantings reuse — the diff modified ListActivePlantingsForObject to use queryPlantings, but the original used plantingColumns unqualified. Let me verify the new version still works.
Now let me check whether requireGardenRole returns a garden with MyRole populated, and whether src.Name is reliable. Also let me verify the CopyGarden store function doesn't check that ownerID actually exists (FK violation edge case).
Now let me check whether the INSERT ... SELECT from the store can return sql.ErrNoRows properly and whether the scanGarden handles the sql.ErrNoRows correctly when the source is gone.
Now let me check the UI mutation — what does gardenSchema.parse do, and is the returned object the right shape (does the copy endpoint return a full Garden)? Also check that created.id exists in the schema.
Now let me check the ContentLength != 0 check in the API handler — what about a request with Content-Length: 0 but a body, or chunked encoding (no Content-Length)?
Now let me check if there are any unique constraints that could fail on copy (e.g., garden name uniqueness, or public_token uniqueness — but public_token is not copied).
Now let me verify the Count field handling in copy. The plantings INSERT copies count as p.Count (a *int). If Count is NULL in DB, the scan reads it as nil, and the INSERT inserts nil. That works.
Let me look at how PlantedAt is handled — it's a *string. In the copy, p.PlantedAt is passed directly. If NULL, nil passed to INSERT — fine.
Now, a key edge case: the copy INSERT for plantings does NOT copy removed_at, version, created_at, updated_at. The defaults handle those. Good — removed_at defaults NULL (active), version defaults 1, timestamps default now. That's correct for a fresh layout.
Let me check whether there's an issue with the Label field — it's *string, nullable. Same handling. Fine.
Now let me check the points column in objects — it's *string, nullable. The copy passes o.Points directly. Fine.
Let me verify the Color and Props nullable handling in the object copy. Looking at the INSERT: o.Color, o.Props are *string passed directly. Fine.
Now, one important edge case in the store CopyGarden: what if srcID == ownerID... no, srcID is gardenID, ownerID is userID. Different.
Let me check: is there any validation that ownerID (the actor) still exists when the copy is made? The actor is authenticated, so they exist. The INSERT into gardens has owner_id REFERENCES users(id), so FK is satisfied. Fine.
Now, the critical edge case I want to verify: the ContentLength != 0 check. With chunked transfer encoding, ContentLength is -1, not 0. So a chunked POST with an actual body would skip decoding, leaving req.Name empty, and the service derives the default name. That's a silent behavior difference but not an error handling issue per se — the body would just be ignored. Not a crash, not a swallowed error. The browser always sends Content-Length for POST with body. Minor.
Let me check: what if ShouldBindJSON succeeds but the JSON has extra fields or name is null? null for a string field → Go leaves it as ""? Actually json.Unmarshal of null into a string leaves it unchanged (empty string default). So {"name": null} → req.Name = "" → derived name. Fine.
Let me verify the writeServiceError mapping for ErrInvalidInput → 400.
Now let me double-check the copyName truncation edge case where the source name has trailing whitespace. Suppose src.Name (after the source was created) has trailing whitespace that was trimmed at creation. Actually CreateGarden trims the name. But src here comes from requireGardenRole → GetGarden, which reads the stored (already-trimmed) name. So src.Name is already trimmed. The copyName function trims again — harmless.
But there's a subtle issue: in copyName, if len(src) > room, it does strings.TrimSpace(strings.ToValidUTF8(src[:room], "")). The TrimSpace could shrink the result further, making len(src) < room, then src + suffix < maxGardenNameLen. Fine — under cap. But what if after trimming, src becomes empty? Then result is just " (copy)" = 7 bytes. That's a valid name. Edge case but handled.
Now, is there a scenario where copyName returns a name that's empty after... no, suffix is always appended. Fine.
Let me check one more thing: the ToValidUTF8 is applied to src[:room] which is a byte slice. If src contains invalid UTF-8 (shouldn't, since it came from DB which stored valid UTF-8), but defensive. Fine.
Let me verify the queryObjects/queryPlantings defer rows.Close() — they have defer rows.Close(). Good, no cursor leak.
Now let me check the tx.QueryRowContext for the object INSERT inside the loop — if it returns an error, we return and the deferred tx.Rollback() runs. Good. The defer rows.Close() in queryObjects runs. Good.
Let me check: in the store CopyGarden, after queryObjects drains the cursor and returns, we then do INSERTs in a loop on tx. Since the cursor is closed, no conflict. Good.
Now, the one thing I want to verify: does tx.QueryRowContext for the INSERTs conflict with the still-open cursor from queryPlantings? No — queryPlantings drains and closes before returning. So when we do tx.ExecContext for planting inserts, the cursor is closed. Good.
Let me verify there's no issue with scanGarden being called with tx.QueryRowContext — *sql.Tx.QueryRowContext returns *sql.Row which implements scanner (has Scan). Good.
Good. *sql.Tx implements both QueryContext (satisfies queryer) and QueryRowContext returns *sql.Row which satisfies scanner. Everything compiles.
Now let me consider the error-handling lens more carefully. The defer tx.Rollback() with errcheck nolint — this is the standard pattern. After commit, rollback returns sql.ErrTxDone which is ignored. Fine. On the error paths before commit, rollback runs and cleans up. Good.
One concern: in the store CopyGarden, when queryObjects or queryPlantings returns an error, we return err directly (not wrapped). The queryObjects/queryPlantings already wrap with "store: list objects/plantings". But in the copy context, "list objects" is slightly misleading (it's a copy operation). Minor, not an error-handling bug.
Let me check one potential real issue: the INSERT ... SELECT ... FROM gardens WHERE id = ? with RETURNING. Does SQLite support INSERT ... SELECT ... RETURNING? Modern SQLite (3.35+) does. The project uses it elsewhere (CreateGarden uses RETURNING). So fine.
But a subtle edge: if srcID doesn't exist, the SELECT yields 0 rows, INSERT inserts nothing, and RETURNING yields no rows. tx.QueryRowContext then returns *sql.Row whose .Scan() returns sql.ErrNoRows. The code checks errors.Is(err, sql.ErrNoRows) → domain.ErrNotFound. Good, verified at line 196.
Now, let me check a potential issue: the transaction isolation. We read the source garden, then read its objects, then read its plantings — all within the tx. If another transaction modifies the source concurrently, we get a consistent snapshot (SQLite serializable). Good.
But wait — the requireGardenRole call in the SERVICE happens OUTSIDE the store transaction (it's a separate GetGarden call). So there's a TOCTOU window: the garden could be deleted between the role check and the store copy. But if deleted, the INSERT ... SELECT yields no rows → ErrNotFound. So the store handles it. Good — no panic, clean error.
Let me verify there's no nil-pointer issue: src from requireGardenRole is used for src.Name. If requireGardenRole returns nil garden with nil error? No — it returns g, nil only with a valid garden. Good.
Now let me check the created return from store — if scanGarden succeeds, created is non-nil. Then service sets created.MyRole. No nil deref. Good.
Let me check the test file for the seedBed, seedOwnPlant, seedGarden, seedUser helpers to make sure they exist.
All helpers exist. Now let me do a final check on the copyName function for a tricky edge: what if maxGardenNameLen - len(suffix) could be negative? maxGardenNameLen = 200, len(suffix) = 7, room = 193. Always positive. No overflow.
What if len(src) is huge? src[:room] is a valid slice (room ≤ len(src) since we're in the len(src) > room branch). Fine.
I've thoroughly examined the error-handling and edge cases. Let me look at one last thing — the Count field in the planting copy. Let me verify the INSERT column list matches the values. The INSERT: (object_id, plant_id, x_cm, y_cm, radius_cm, count, label, planted_at) — 8 columns, 8 placeholders ?, ?, ?, ?, ?, ?, ?, ?. Values: objectID, p.PlantID, p.XCM, p.YCM, p.RadiusCM, p.Count, p.Label, p.PlantedAt — 8 values. Matches. Good.
The copy does NOT carry removed_at, so the new planting is active (NULL). Good. It does NOT carry version (defaults to 1), created_at/updated_at (default to now). Good — fresh row.
I'm confident the error handling and edge cases are well-covered. Let me verify one potential concern: the UI modal's onClose is called BEFORE navigate. If onClose unmounts the modal/component, does navigate still work? onClose sets dialog to null in the parent, which unmounts CopyGardenModal. But navigate is called from within the onSubmit async function which already holds the navigate reference. The useNavigate hook returns a stable function. Calling it after unmount is fine (TanStack Router's navigate is not tied to component lifecycle). And toast.info also works after unmount. This is a common pattern. No error-handling issue.
Actually, wait — onClose() is called, then navigate(). If onClose triggers a state update that causes re-render, the current execution context (the async function) continues fine. No issue.
Let me check: is there a race where copy.isPending stays true and the user can't interact? No — mutateAsync resolves, isPending becomes false. The error path sets error and the user can retry. Good error recovery.
One more: the modal's submit button is disabled={copy.isPending || name.trim() === ''}. If the user submits a whitespace-only name, the button is disabled — but they could still press Enter in the TextField (form submit). The onSubmit does name.trim() and sends it. If empty after trim, the server derives the default name. Wait — name.trim() === '' disables the button, so the form can't be submitted via button click. But Enter key in a text field submits the form. The button is disabled, so Enter won't trigger it (disabled submit button blocks form submission). Actually, in HTML, if the submit button is disabled, Enter in a text input may still submit the form in some browsers. But onSubmit handles it: name.trim() → empty string sent → server derives default. No error. Fine.
I've completed my review through the error-handling/edge-cases lens. Everything is robust:
Transaction rollback via defer handles partial failures
Cursors are drained before writes (SQLite constraint)
Missing source → ErrNotFound via sql.ErrNoRows
TOCTOU between role check and copy is handled by the INSERT...SELECT returning no rows
Name truncation handles rune boundaries and produces valid UTF-8 under the cap
Empty/whitespace names fall through to derivation
Over-long explicit names rejected
Optional body parsed safely with fallback
FK constraints satisfied (actor exists)
No nil derefs on the return paths
No material error-handling or edge-case issues found.
Verdict: No material issues found
Reviewed the deep-copy change end-to-end through the error-handling & edge-cases lens. The unhappy paths are well-covered:
Store CopyGarden transaction (internal/store/gardens.go:179-259): defer tx.Rollback() runs on every error path and is a no-op after commit; partial failures leave no half-populated garden. Verified the INSERT ... SELECT ... RETURNING returns sql.ErrNoRows when the source is gone, mapped to domain.ErrNotFound (line 196-197) — so the TOCTOU window between the service-level requireGardenRole read and the store write degrades cleanly rather than panicking.
Cursor discipline (internal/store/objects.go:78-97, plantings.go:44-63): both queryObjects/queryPlantingsdefer rows.Close() and fully drain before returning, so the subsequent tx.ExecContext/tx.QueryRowContext writes don't hit SQLite's "cursor open while writing on the same connection" limit. Verified the in-memory single-connection rationale in the comments.
Name derivation (internal/service/gardens.go:193-199): copyName truncates src[:room] (byte slice) then strings.ToValidUTF8(..., "") repairs any split rune, then TrimSpace; result is always ≤ room + len(suffix) = maxGardenNameLen. Confirmed room = 200 - 7 = 193 > 0, no underflow. The dedicated TestCopyGardenNameRuneBoundary exercises the multi-byte case and asserts utf8.ValidString + length cap.
Input bounds: blank/absent name → derivation; over-long explicit name → ErrInvalidInput (line 179); API handler treats ContentLength != 0 as "a body was sent" and rejects malformed JSON as 400. {"name": null} decodes to "" → derivation, not a crash.
Planting copy (internal/store/gardens.go:246-252): 8 columns / 8 placeholders / 8 values match; count/label/planted_at nullable * fields pass through as nil for NULL source values; removed_at/version/timestamps deliberately omitted so defaults yield a fresh active row.
Unreachable-planting guard (line 241-244): the objectIDs map miss returns a wrapped error rather than proceeding with a zero objectID — defensive against a logic break.
UI modal (CopyGardenModal.tsx): mutateAsync errors are caught and surfaced via Alert, copy.isPending disables actions during flight, and name.trim() === '' blocks submission. The created return is zod-parsed before use.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
No material issues found through the security lens.
AuthZ/owner-only guard verified: CopyGarden calls requireGardenRole(..., roleOwner) at internal/service/gardens.go:171, so only owners can copy. Confirmed by service tests: strangers get ErrNotFound, editors get ErrForbidden. The UI correctly only shows the Copy button for owners.
No public_token leakage: The new garden is inserted with INSERT ... SELECT that omits public_token; GetPublicShareLink on the copy returns empty token, and PublicGarden with the source token still resolves to the source (tested in service tests).
No share inheritance: garden_shares rows are not copied; confirmed by service tests.
No SQL injection: Parameterized queries used throughout; no string concatenation of user-controlled data.
No path/template/command injection: Only integer IDs parsed from params; names are plain strings without template evaluation.
Name length cap enforced: Explicit names over maxGardenNameLen are rejected with ErrInvalidInput; derived names are truncated safely with ToValidUTF8.
No SSRF/unsafe deserialization: Nothing in the diff opens outbound connections or deserializes untrusted data.
Transaction safety: Copy runs in one transaction with rollback on failure; no partial-state exposure.
🎯 Correctness — No material issues found
No material issues found. All logic paths, formulas, and edge cases were verified and are correct.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/api/gardens.go:120 — The copyGarden handler checks c.Request.ContentLength != 0 to decide whether to parse an optional JSON body, but the codebase already has an established pattern for this in internal/api/public.go:56: try c.ShouldBindJSON(&req) and ignore io.EOF. The ContentLength guard is both inconsistent with surrounding code and less robust (e.g., chunked encoding or HTTP/2 can leave ContentLength == -1 even with no body, which would cause the handler to incorrectly reject a valid no-body request as INVALID_INPUT). Fix: Remove the ContentLength guard and use if err := c.ShouldBindJSON(&req); err != nil && !errors.Is(err, io.EOF) { … }, adding "io" to the imports.
internal/service/gardens_test.go:364 (in TestCopyGardenSkipsRemovedPlantings) — The test assigns a planting to kept only to suppress it with _ = kept. Using _, err := s.CreatePlanting(...) on that line avoids the unused-variable workaround.
⚡ Performance — No material issues found
No material issues found.
The copy implementation is structurally sound from a performance standpoint:
Single transaction: CopyGarden wraps everything in one BEGIN...COMMIT block, avoiding partial-state overhead and extra round-trips.
No N+1 queries: Objects and plantings are each fetched with one query (the planting query uses a JOIN garden_objects to scope by garden). Reparenting is done in-memory via the objectIDs map.
Bounded allocations: The objectIDs map is pre-sized (make(..., len(srcObjects))). Result slices are drained and closed before writes, which is a deliberate SQLite-safe pattern, not an allocation leak.
No hot-path quadratic behavior: The loops over srcObjects and srcPlantings are linear and triggered by an explicit user action (POST /copy), not a background or high-frequency path.
Name length bounded: maxGardenNameLen prevents unbounded input.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
I investigated the copy-garden flow through the error-handling & edge-cases lens and found the unhappy paths are properly covered:
Transaction safety — CopyGarden in internal/store/gardens.go:179 opens a transaction, has defer tx.Rollback(), and only commits after all inserts succeed. Any failure mid-way rolls back, leaving no half-copied garden.
Empty/whitespace-only name — CopyGarden in internal/service/gardens.go:175 trims the name and falls back to copyName(src.Name) when blank, matching the documented behavior.
Over-long name — Rejected with ErrInvalidInput rather than silently truncated (internal/service/gardens.go:179).
Rune-boundary truncation — copyName (internal/service/gardens.go:193) uses strings.ToValidUTF8 on the sliced prefix so multi-byte runes are never split.
Orphaned planting guard — The objectIDs lookup in internal/store/gardens.go:241 is defensive: if an active planting somehow references an object that wasn’t copied, it returns an explicit error and the deferred rollback cleans up.
Optional request body — The API handler (internal/api/gardens.go:120) only binds JSON when ContentLength != 0, so a no-body POST correctly triggers the default-name path.
Zero objects / zero plantings — Both queryObjects and queryPlantings return non-nil empty slices; the loops simply don’t execute and the empty garden commits successfully.
No ignored errors, no missing cleanup, no unhandled nil/empty/zero cases, and no boundary issues were found in this change.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Minor issues** · 6 findings (4 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | Inconsistent optional-body pattern uses ContentLength instead of io.EOF | `internal/api/gardens.go:120` | 4/5 | error-handling, maintainability |
| 🟠 | Client-side copy-name derivation diverges from server truncation logic | `web/src/components/gardens/CopyGardenModal.tsx:20` | 3/5 | correctness, error-handling, maintainability |
| 🟡 | Unused variable suppressed with _ = kept instead of using _ in original assignment | `internal/service/gardens_test.go:364` | 3/5 | maintainability |
| 🟠 | CopyGarden inserts objects/plantings one row at a time inside a single write transaction, serializing all other writers under WAL for O(n) round-trips | `internal/store/gardens.go:216` | 2/5 | maintainability, performance |
<details><summary>2 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟡 | Misleadingly-named dead variable 'kept' in TestCopyGardenSkipsRemovedPlantings — it is actually cleared, not kept | `internal/service/gardens_test.go:348` | claude-code/sonnet | maintainability |
| ⚪ | Generic queryer interface declared in users.go rather than a shared store file | `internal/store/users.go:22` | 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>
All static constants — no injection surface. The security-relevant design (owner-only copy, no token/share inheritance, parameterized SQL throughout) checks out and is well-tested. Nothing material in my lens.
**VERDICT: No material issues found**
- Authorization: `CopyGarden` (internal/service/gardens.go:172) correctly gates on `roleOwner` via `requireGardenRole`, which returns `ErrNotFound` for no-access and `ErrForbidden` for insufficient access (viewer/editor) — verified against `requireGardenRole`/`effectiveGardenRole` (internal/service/gardens.go:69-105) and exercised by `TestCopyGardenPermissions`.
- IDOR/cross-tenant reference risk: the PR's stated rationale for owner-only copying (avoiding a copy whose plops reference another user's `plant_id`s, and avoiding pinning those plants against `ON DELETE RESTRICT`) is actually enforced in code, not just asserted in comments.
- SQL injection: all queries in `internal/store/gardens.go` `CopyGarden` use parameterized `?` placeholders; `gardenColumns`/`objectColumns`/`plantingColumns`/`qualifyColumns` are static Go constants, not built from request input — no injection surface.
- Secret/capability leakage: `public_token` is deliberately excluded from the `INSERT ... SELECT` column list in `CopyGarden` (internal/store/gardens.go:172), and the column has no default other than `NULL`, so a copy starts with no public link — confirmed via migration `0004_garden_public_token.sql` and `TestCopyGardenDoesNotInheritPublicLink`. `garden_shares` is likewise never touched, confirmed by `TestCopyGardenDoesNotInheritShares`.
- CSRF: the new route sits under the existing `v1` group with `h.csrfGuard()` applied at the group level (internal/api/api.go), so it isn't missing CSRF protection that other mutating endpoints have.
- Input handling: `name` is length-capped (`maxGardenNameLen`) and rejected rather than silently truncated when explicitly supplied; the request body is optional and a non-string `name` is rejected by `ShouldBindJSON` with 400. The `ContentLength != 0` check (internal/api/gardens.go) can mis-trigger on chunked requests with `ContentLength == -1`, but that's a correctness/robustness nit, not a security issue (worst case is a spurious 400), so it's outside this lens.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Confirms GardenFormModal (create/edit) also has no maxLength cap, so a garden name up to 200 bytes is indeed creatable through the normal UI, then reachable via copy. The finding fully checks out.
## Corrected review
**VERDICT: Minor issues**
- `web/src/components/gardens/CopyGardenModal.tsx:20` — the modal prefills the name field with `` `${garden.name} (copy)` `` with no length cap, but the server (`internal/service/gardens.go:175-181`) only applies the graceful `copyName` truncation when the incoming name is **blank**; a non-blank name over `maxGardenNameLen` (200 bytes, `internal/service/gardens.go:20`) hits the strict `domain.ErrInvalidInput` path instead. Since `GardenFormModal.tsx` also has no `maxLength` on the name field (confirmed no `maxLength` prop anywhere in `web/src/components/gardens/GardenFormModal.tsx` or `web/src/components/ui/TextField.tsx`), a garden name legitimately created near the 200-byte cap becomes uncopyable without the user first editing the prefilled field — appending " (copy)" (7 bytes) pushes it over the cap and the request is rejected with a 400, surfaced to the user as "Could not copy the garden."
- Fix: cap the client-side default to the same room `copyName` leaves (mirroring its truncation logic), or have the service truncate rather than reject over-cap names on this endpoint.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
VERDICT: Minor issues
- `internal/service/gardens_test.go:348` (`TestCopyGardenSkipsRemovedPlantings`) — the local var `kept` (declared line 348, silenced via `_ = kept` at line 364) is misleadingly named and is dead code. It's created, never asserted on, and then discarded just to silence "declared and not used." Worse, the name actively misleads: `ClearObjectPlantings` (`internal/store/plantings.go:76-92`), invoked via `ClearObject` (`internal/service/ops.go:221-227`), soft-removes *every* active plop on the object with no per-row selectivity — so `kept` is cleared right along with the other, unnamed planting at line 357's `ClearObject` call. It is not "kept" at all. A future reader skimming this test could easily conclude one plop survives the clear while the other doesn't, which isn't what's being tested. Drop the variable (mirror the `if _, err := s.CreatePlanting(...)` form already used two lines below for the plop that's supposed to be cleared) or rename it to something that doesn't imply survival, e.g. `clearedA`/`clearedB`.
- `internal/store/gardens.go:216-230` and `internal/store/gardens.go:246-252` — `CopyGarden`'s per-row inserts duplicate the exact INSERT SQL and argument list already living in `CreateObject` (`internal/store/objects.go:38-46`) and `CreatePlanting`/`CreatePlantings` (`internal/store/plantings.go:110-121`, `:135-137`), just issued against `tx` instead of `d.sql`. The PR already introduces exactly the abstraction that would fix this on the *read* side (`queryer` in `internal/store/users.go:24-26`, shared by `queryObjects`/`queryPlantings`); doing the analogous thing for these inserts (an `execer`-style interface satisfied by both `*sql.DB` and `*sql.Tx`, with `CreateObject`/`CreatePlanting` parameterized over it) would have let `CopyGarden` call the existing helpers instead of re-typing the SQL. This does follow a pre-existing precedent in the codebase (`CreatePlantings` already duplicates `CreatePlanting`'s INSERT for its tx-batch case), so it's not a new pattern this PR invented, but the PR had the ideal opportunity — and the tooling already in hand — to close that gap instead of extending it to a third copy.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
All claims check out: the loop structure (one `INSERT...RETURNING` per object, one `INSERT` per planting, lines 216-253), the WAL + `busy_timeout(5000)` pragmas (sqlite.go:20-21), the absence of any per-garden object/planting cap, `maxGardensListed` only bounding the list endpoint (gardens.go:50), and the indexes existing in the migration. The finding is accurate.
VERDICT: Minor issues
- `internal/store/gardens.go:216-253` — `CopyGarden` executes one `INSERT ... RETURNING` per source object and one `INSERT` per active planting, sequentially, inside a single write transaction. There is no cap on how many objects/plantings a garden can hold (`maxGardensListed` at `internal/store/gardens.go:50` only bounds the *list* endpoint). The DB is opened in WAL mode with `busy_timeout(5000)` (`internal/store/sqlite.go:20-21`), so only one writer can proceed app-wide; every other user's write queues behind `busy_timeout` while this transaction is open. For a garden with many objects/plantings, the copy becomes N+M sequential round-trips held under one write lock — the copy's own latency scales linearly with garden size, and it serializes all other users' writes for that whole window (a concurrent write from an unrelated user can block or fail with `SQLITE_BUSY`). A multi-row `INSERT ... VALUES (...),(...),... RETURNING ...` (or chunked batching) would cut the round-trip count from O(n) to O(1) for both phases. Severity is moderate since it requires an unusually large garden to bite in practice.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
This confirms the store/service code matches the draft's description exactly — transaction handling, error wrapping, `sql.ErrNoRows` mapping, and `copyName`'s rune-safe truncation are all as claimed. The only actionable finding is the `ContentLength` check in `gardens.go:120`, which I independently verified: Go's server sets `ContentLength == -1` (not `0`) for a request with `Transfer-Encoding: chunked` and no `Content-Length` header, so `c.Request.ContentLength != 0` is true for an unknown-length body, sending it into `ShouldBindJSON` where an empty chunked body yields `io.EOF`, incorrectly rejected as `400`. The sibling handler `createShareLink` (`public.go:56`) already uses the correct `err != nil && !errors.Is(err, io.EOF)` pattern, confirming this is a real regression/inconsistency, not a false positive.
**VERDICT: Minor issues**
- `internal/api/gardens.go:120` — `copyGarden` gates JSON body parsing on `c.Request.ContentLength != 0`, but Go sets `ContentLength` to `-1` (not `0`) when the body length is unknown (e.g. `Transfer-Encoding: chunked` with no body). In that case the handler calls `ShouldBindJSON` on an effectively empty body, which fails with `io.EOF`, and gets turned into a `400 INVALID_INPUT "name must be a string"` — incorrectly rejecting a legitimate no-body copy request. The sibling handler `createShareLink` (`internal/api/public.go:56`) already implements this correctly: `if err := c.ShouldBindJSON(&req); err != nil && !errors.Is(err, io.EOF) { ... }`, which is agnostic to how the empty body was framed. `copyGarden` reinvents the optional-body check with a strictly weaker condition. The existing test (`gardens_test.go`) doesn't catch this because `httptest.NewRequest` always sets `ContentLength` to the exact buffer length (0 for a nil/empty body), so the chunked/`-1` path is untested.
- Fix: mirror the `public.go` pattern — always call `ShouldBindJSON` and ignore only `io.EOF`, rather than gating on `ContentLength`.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**VERDICT: No material issues found**
Through the security lens, I verified the copy feature's authorization, capability-leakage, input-validation, and injection surfaces against the actual code:
- **Authorization (authz)**: `CopyGarden` calls `s.requireGardenRole(ctx, actorID, gardenID, roleOwner)` (`internal/service/gardens.go:171`), so strangers get `ErrNotFound` (→ 404) and non-owner editors get `ErrForbidden` (→ 403), matching the stated owner-only policy. The route is registered under `gardens := v1.Group("/gardens", h.requireAuth())` (`internal/api/api.go:78`), so unauthenticated requests are rejected before the handler runs.
- **Actor identity is not user-controllable**: `mustActor(c).ID` is derived from the session (`internal/api/auth.go:209` via `actorKey`), not from the request body or query; the new garden's `owner_id` is set to this trusted `actorID` in the store (`internal/store/gardens.go:191`, `ownerID` param). No IDOR path.
- **Capability leakage — `public_token`**: The garden-copy `INSERT … SELECT` explicitly enumerates columns (`owner_id, name, width_cm, height_cm, unit_pref, notes, grid_size_cm, snap_to_grid`) and does **not** include `public_token` (`internal/store/gardens.go:192-197`). Since `public_token` defaults to NULL (migration 0004), the copy starts with no public link. Confirmed it cannot silently inherit a live capability.
- **Capability leakage — `garden_shares`**: The store copy transaction never inserts into `garden_shares`, so the copy is private to the copier. Confirmed.
- **Removed-planting history**: The planting scan filters `pl.removed_at IS NULL` (`internal/store/gardens.go:225`), so soft-removed plops aren't carried over — no history/capability bleed.
- **Input validation**: The name is `TrimSpace`'d, blank-derived, and length-capped at `maxGardenNameLen` (200) with `ErrInvalidInput` on overflow (`internal/service/gardens.go:175-180`); `copyName` truncates on a rune boundary via `ToValidUTF8` to keep results valid UTF-8 (`internal/service/gardens.go:193-198`). The handler rejects non-string `name` with 400 (`internal/api/gardens.go:118`). No unbounded/unvalidated string reaches storage.
- **Injection**: All SQL uses parameterized placeholders (`?`) for every user- or source-derived value across the garden, object, and planting inserts (`internal/store/gardens.go:192-237`); no string interpolation of untrusted input into a query. The column-list constants (`gardenColumns`, `objectColumns`) are compile-time string literals, not user input.
No security issues found within this lens.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
I reviewed the change through the correctness lens, verifying the schema, column lists, truncation math, and transaction flow against the checked-out code.
- **`copyName` truncation math** (`internal/service/gardens.go:193`): `room = maxGardenNameLen(200) - len(" (copy)")(7) = 193`. When `len(src) > 193`, it slices `src[:193]`, runs `ToValidUTF8(..., "")` (empty replacement → only shrinks, never grows, and repairs any rune split at the byte boundary) then `TrimSpace` (only shrinks). Result + suffix ≤ 193 + 7 = 200 = cap. Verified against `gardenFromInput`'s `len(name) > maxGardenNameLen` byte-length check — units are consistent (bytes, not runes). The `TestCopyGardenNameRuneBoundary` case (100 × "é" = 200 bytes) truncates to 193 bytes = 96 complete é + a dangling lead byte that `ToValidUTF8` drops → 192 + 7 = 199 ≤ 200, valid UTF-8. Correct.
- **Garden row copy** (`internal/store/gardens.go:189`): `INSERT ... SELECT` copies `width_cm, height_cm, unit_pref, notes, grid_size_cm, snap_to_grid` and supplies `owner_id, name`; `version`, `created_at`, `updated_at`, and `public_token` fall back to defaults (so no inherited share link, fresh version=1). Verified against `migrations/0004` (public_token is a nullable added column, not in `gardenColumns`) and `0001_init` (version default 1). Correct.
- **Object copy** (`internal/store/gardens.go:217`): 17 insert columns match the 17 placeholders and match `CreateObject`'s column list at `objects.go:38`; `version`/timestamps default. `z_index` is copied per-row so the `ORDER BY id` scan (vs. the live list's `z_index, id`) doesn't affect correctness. Correct.
- **Planting copy** (`internal/store/gardens.go:247`): query joins source objects (`o.garden_id = srcID`) and filters `removed_at IS NULL`, so only active plops are read and they reference source object IDs; the `objectIDs` map re-parents each onto the copied object. `removed_at` is intentionally not inserted (defaults NULL = active). `count`/`label`/`planted_at` (all nullable) are passed through. Correct.
- **TOCTOU on source existence**: `requireGardenRole` reads the garden, then `store.CopyGarden` re-checks via `INSERT ... SELECT ... WHERE id = ?`; a vanished source yields `sql.ErrNoRows` → `domain.ErrNotFound`. Correct.
- **`queryer` interface + cursor draining**: `*sql.Tx` and `d.sql` both satisfy `queryer`; `queryObjects`/`queryPlantings` fully drain and `defer rows.Close()` before returning, so the subsequent writes on the same tx connection are safe. Verified at `objects.go:78` and `plantings.go:44`. Correct.
- **`ListActivePlantingsForObject` refactor** (`plantings.go:68`): now routes through `queryPlantings` with unqualified `plantingColumns`; `scanPlanting` scans by position so the unqualified select still lines up. Correct.
No correctness defects found in this lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings are confirmed against the actual code:
1. **gardens.go:217** — The `INSERT INTO garden_objects` column list at gardens.go:217-221 is verbatim-duplicated from `CreateObject` at objects.go:38-42. The plantings INSERT column list at gardens.go:247-248 is duplicated from both `CreatePlanting` (plantings.go:112-114) and `CreatePlantings` (plantings.go:135-137) — three places total. Confirmed.
2. **users.go:22** — The `queryer` interface is declared at users.go:22-26 and is a generic store-wide abstraction. (Note: `scanner` also lives in users.go:18, so the draft's "or the file holding scanner" phrasing is slightly off, but the core point — `queryer` is a shared helper misplaced in a user-specific file — holds.) Confirmed as trivial.
3. **gardens_test.go:364** — `kept` is assigned at line 348 and only referenced via `_ = kept` at line 364. Confirmed.
**Verdict: Minor issues**
- `internal/store/gardens.go:217` and `internal/store/plantings.go:112`/`:135` — the `INSERT INTO garden_objects …` and `INSERT INTO plantings …` column lists are duplicated verbatim between `CreateObject`/`CreatePlanting` and the in-transaction copy in `CopyGarden`. The copy can't call `CreateObject`/`CreatePlanting` directly because those bind to `d.sql` rather than `tx`, so the duplication is a deliberate tradeoff, but it means a future schema/column change must be edited in two (objects) / three (plantings) places. A small shared `insertObjectSQL`/`insertPlantingSQL` constant for the `(cols) VALUES (?,?,…) RETURNING …` text would keep them in sync at zero behavioral cost. Verified by reading both files.
- `internal/store/users.go:22` — the new `queryer` interface is a generic store-wide abstraction but is declared in `users.go`, which is otherwise about user rows. For a shared store helper this is a mildly leaky/misplaced home; a `store.go` would be the natural spot and would make discovery easier. Verified by reading the file.
- `internal/service/gardens_test.go:364` — `kept` is assigned from `CreatePlanting` and then only referenced via `_ = kept` to silence the unused-variable check. Just dropping the assignment (`_, err := s.CreatePlanting(...)`) or naming it `_` would be cleaner than the throwaway `_ = kept` line. Verified by reading the test.
No dead code, no confusing names, no over-long functions, and the new code follows the surrounding patterns (error wrapping prefixes, `defer tx.Rollback()`, `boolToInt`, column constants) consistently.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
I verified the store implementation (`internal/store/gardens.go:179-259`) and the refactored query helpers (`internal/store/objects.go`, `internal/store/plantings.go`).
Through the performance lens:
- The copy does N individual `INSERT ... RETURNING` for objects and M individual `INSERT`s for plantings (`gardens.go:215-253`). This is N+M round-trips rather than a single batched copy, but it's an owner-initiated, one-shot operation (not a hot path), and the per-row `RETURNING` is genuinely needed to build the source→dest `objectIDs` map for planting re-parenting. SQLite offers no clean batched-`RETURNING`-with-remapping alternative here, so this is inherent, not a regression.
- `queryObjects`/`queryPlantings` fully materialize the source rows into slices before any write (`gardens.go:206-237`). That's required by the SQLite single-connection cursor constraint the PR calls out, and the `make(map[int64]int64, len(srcObjects))` pre-sizing avoids reallocation churn. Memory is O(objects + plantings), unbounded in principle, but bounded in practice by a single garden's contents on a user-triggered action.
- The whole copy runs in one transaction, holding SQLite's single write lock for its duration. For a large garden this serializes other writers briefly, but again it's a user-initiated copy, not a read-path or list-path concern.
No hot-path regressions, no N+1 on a list/read path, no missing pagination on a frequently-hit endpoint. Nothing material to flag in this lens.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding's reasoning about Go's `net/http` semantics is correct: `Request.ContentLength` is `-1` for chunked transfers, so `!= 0` is true, and `ShouldBindJSON` on an empty body returns `io.EOF` → the 400 path. The code at line 120 matches the draft's description. This is a legitimate, low-severity edge case.
**VERDICT: Minor issues found**
- `internal/api/gardens.go:120` — The "body absent" detection uses `c.Request.ContentLength != 0`. For a chunked-transfer POST (`Transfer-Encoding: chunked`, no `Content-Length`), Go sets `Request.ContentLength = -1`, so `!= 0` is true and the handler falls into `ShouldBindJSON` on an empty body, which returns `io.EOF` → a `400 "name must be a string"`. A genuinely empty chunked body should behave like "no body" (derive the default name), but instead it's rejected. Low-severity edge case (chunked empty POSTs are unusual, and rejecting one isn't dangerous), but it is an inconsistent unhappy path. The cleanest fix is to attempt the decode only when a body is actually present (e.g. peek `c.Request.Body`), or gate on `ContentLength > 0` so `-1` falls through to the default-name path (noting that would silently swallow a malformed chunked body).
</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**
From the 🔒 Security lens, the change is well-constructed:
- **Authn/Authz**: The `POST /gardens/:id/copy` route sits behind the existing `requireAuth()` middleware. The service layer enforces **owner-only** access via `requireGardenRole(..., roleOwner)` — strangers receive `ErrNotFound` (information hiding) and editors receive `ErrForbidden`, matching the PR's deliberate access policy.
- **SQL injection**: All store queries use `?` placeholders. The column-list constants (`gardenColumns`, `objectColumns`, `plantingColumns`) are hard-coded, never derived from user input.
- **Transaction consistency / TOCTOU**: The entire copy runs inside a single `*sql.Tx`. Reads of source objects and plantings are drained before writes, respecting SQLite's same-connection cursor limitation, so the transaction snapshot stays consistent.
- **Sensitive-data exclusion**: `public_token` and `garden_shares` are intentionally **not** copied, and the store's `INSERT ... SELECT` only copies the explicitly listed non-sensitive metadata columns. The resulting garden starts private.
- **Input validation**: The `parseIDParam` helper rejects non-numeric and non-positive IDs with 400. The name is trimmed and capped at `maxGardenNameLen` bytes; `copyName` uses `strings.ToValidUTF8` to avoid splitting multi-byte runes when trimming for the suffix.
- **No SSRF / deserialization / secret-leakage paths**: The feature is an entirely internal database duplication with no external network calls or unmarshalling of untrusted complex types.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found
The implementation is correct through the correctness lens. I verified:
- **Transaction integrity**: The `CopyGarden` store method uses `BeginTx` with `defer tx.Rollback()` and a final `tx.Commit()`, ensuring no half-populated garden survives a failure.
- **Cursor draining**: `queryObjects` and `queryPlantings` both fully consume and close their `*sql.Rows` via `defer rows.Close()` before returning, satisfying the SQLite constraint that you cannot hold a read cursor open while writing on the same connection within a transaction.
- **Rune-boundary truncation**: `copyName` uses `strings.ToValidUTF8` after byte slicing and before appending the suffix, guaranteeing valid UTF-8. The test `TestCopyGardenNameRuneBoundary` confirms this.
- **Length invariant**: `copyName` always produces `len(result) <= maxGardenNameLen` because it only truncates when `len(src) > (maxGardenNameLen - len(suffix))`, and the final result is at most `room + len(suffix) == maxGardenNameLen`.
- **Version reset**: Neither the garden INSERT...SELECT nor the object/planting INSERTs copy `version`, so the schema defaults (`DEFAULT 1`) give the copy fresh version 1 values, matching the test expectation.
- **Excluded fields**: `public_token` and `garden_shares` are deliberately omitted from copying; `version`, `created_at`, `updated_at` are reset via defaults; only active plantings (`removed_at IS NULL`) are copied.
- **Permission check**: The service calls `requireGardenRole(ctx, actorID, gardenID, roleOwner)`, correctly restricting copying to owners and returning `ErrForbidden` for editors and `ErrNotFound` for strangers, as verified by tests.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `web/src/components/gardens/CopyGardenModal.tsx:20` — The modal pre-derives the copy name with `` `${garden.name} (copy)` ``, duplicating the server's `copyName()` logic but without the byte-cap truncation. If the source name is long, the prefilled default can exceed `maxGardenNameLen` and will be rejected by the service when submitted unchanged. The client and server derivations are out of sync. **Fix:** either share the truncation logic (e.g., a common util) or prefill with a name capped the same way the server does.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
Reviewed through the **Performance** lens. The deep-copy implementation in `internal/store/gardens.go` does materialize all source objects and plantings in memory before writing, and performs individual `INSERT` statements per object and per planting inside a single transaction. While this is technically an N+1 insert pattern, it is bounded by garden size (user-managed, typically small), runs on an explicit user action (not a hot path), and correctly drains cursors before writing to respect SQLite's same-connection limitations. No hot-loop allocations, unbounded growth, missing limits, or blocking call regressions were identified.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`web/src/components/gardens/CopyGardenModal.tsx:20`** — The modal prefills the copy name as `` `${garden.name} (copy)` `` without applying the server's 200-byte name-length cap (`maxGardenNameLen`). If the source garden name is 194–200 bytes, the derived name exceeds the limit and the server rejects it with `ErrInvalidInput` (400). The user opens the modal to a default value that is guaranteed to fail.
**Fix:** Reuse the backend truncation logic in the frontend (cap source name to make room for the 7-byte `" (copy)"` suffix, trimming on a valid UTF-8 boundary), or derive the default name from the API rather than precomputing it.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**VERDICT: No material issues found**
I reviewed this PR strictly through the security lens (authn/authz, injection, secret leakage, input validation, unsafe untrusted data). I verified each concern against the checked-out code, not just the diff:
- **Authz (owner-only)** — `CopyGarden` calls `s.requireGardenRole(ctx, actorID, gardenID, roleOwner)` (`internal/service/gardens.go:171`), and `requireGardenRole` returns `ErrNotFound` for users with no role and `ErrForbidden` for insufficient role (`internal/service/gardens.go:69-86`). Strangers don't even learn the garden exists; the test confirms stranger→404 and editor→403. ✓
- **Route is auth-gated** — `POST /gardens/:id/copy` is registered inside the `gardens` group under `h.requireAuth()` (`internal/api/api.go:78,84`), so anonymous → 401, and POST is covered by `csrfGuard` (`internal/api/api.go:50`). ✓
- **No `public_token` inheritance** — The `INSERT INTO gardens` at `internal/store/gardens.go:190-193` lists columns explicitly and omits `public_token`; the column defaults to NULL (`internal/store/migrations/0004_garden_public_token.sql:5`). `gardenColumns` also excludes it, so it can't leak via the API response. The copy starts with no live share URL. ✓
- **No `garden_shares` inheritance** — Only `gardens`, `garden_objects`, and active `plantings` are inserted; no shares rows. ✓
- **`plant_id` reuse is owner-scoped** — The PR's stated reason for owner-only is that copied plops keep the source's `plant_id`s. Since authorization is enforced as owner-only, the actor copying is the owner of those plants, so this doesn't hand a stranger a reference to someone else's catalog. The `ON DELETE RESTRICT` consequence is a correctness/feature-scoping concern (acknowledged in the design), not a security bypass. ✓
- **SQL injection** — Every query uses `?` placeholders for user-supplied values (`name`, `srcID`, `ownerID`). The string-concatenated bits (`gardenColumns`, `objectColumns`, `plantingColumns`, `qualifyColumns(...)`) are compile-time constants, not request input. ✓
- **ID + body validation** — `parseIDParam` rejects non-numeric ids → 400 (`internal/api/errors.go:77`); the optional body is only decoded when `ContentLength != 0`, and `ShouldBindJSON` rejects a non-string `name` (e.g. `{"name":42}`) → 400, as tested. Over-long names → `ErrInvalidInput` → 400 (`internal/service/gardens.go:179-181`). Trusting `ContentLength` here is a minor optimization, not a vulnerability — an attacker can't exploit it to bypass validation. ✓
- **No mass-assignment** — `owner_id` in the copy is supplied as the `mustActor(c).ID` from the session, not from the request body; `created.MyRole` is set server-side to `roleOwner` (`internal/service/gardens.go:186`). The request body only carries `name`. ✓
- **No XSS in the UI** — `CopyGardenModal.tsx` renders `garden.name` and `created.name` as React text content (no `dangerouslySetInnerHTML`), so the name is escaped. ✓
Nothing in my lens turned up a material issue.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I'll verify the key claims in the draft review against the actual code.
The code matches the draft's claims. Let me verify the supporting functions referenced (`requireGardenRole`, `boolToInt`, `qualifyColumns`, cursor closing).
All supporting functions exist. The draft's verification is accurate. The draft found no material issues and verified each claim against actual code — which I've now independently confirmed. The `gadfly-findings` array is empty, matching the draft.
No material issues found.
- `copyName` truncation math (service/gardens.go:193-199): confirmed `maxGardenNameLen=200`, suffix 7 bytes, room=193. Boundary cases hold; multi-byte rune split repaired by `ToValidUTF8(..., "")` (empty replacement, only shrinks) then `TrimSpace`. Cannot exceed 200 bytes.
- `INSERT ... SELECT` garden copy (store/gardens.go:190-194): copies `width_cm, height_cm, unit_pref, notes, grid_size_cm, snap_to_grid`; omits `public_token` (NULL default), `version` (1), timestamps (now). Owner/name supplied. Correct.
- Object copy (store/gardens.go:216-225): column list matches `objectColumns`; `boolToInt` applied to the two INTEGER boolean columns; `z_index` preserved. `boolToInt` confirmed defined at store/users.go:173.
- Planting copy (store/gardens.go:246-249): inserts `object_id` (remapped via `objectIDs`), `plant_id, x_cm, y_cm, radius_cm, count, label, planted_at`; omits `removed_at` (NULL → active, consistent with the `removed_at IS NULL` filter), `version` (1), timestamps. The "unreachable" missing-object branch is genuinely unreachable given the join.
- Authorization (service/gardens.go:170-171): `requireGardenRole(..., roleOwner)` confirmed at service/gardens.go:69; masks non-owners as `ErrNotFound` and insufficient-role as `ErrForbidden`, matching test expectations.
- Handler body parsing (api/gardens.go:114-122): `ContentLength != 0` skips binding for empty bodies while still binding chunked (`ContentLength == -1`) bodies; wrong-typed `name` → 400.
- Transaction integrity (store/gardens.go:180-258): `defer tx.Rollback()` + explicit `tx.Commit()`; all writes via `tx`. `queryObjects`/`queryPlantings` confirmed defined at store/objects.go:78 and store/plantings.go:44 (drain+close before subsequent writes), respecting SQLite's no-concurrent-cursor constraint.
- `qualifyColumns` confirmed at store/sqlite.go:105.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
I'll verify each finding against the actual code.
All three findings verified. Now let me check the `useCopyGarden` hook to confirm finding #1's claim about the UI always sending `{ name: name ?? '' }`.
Confirmed: `api.post` with `{ name: name ?? '' }` always sends a body, so the API's `ContentLength != 0` branch is always hit for the UI client. The component passes `name.trim()` which is non-empty (button disabled if empty), but the body is sent regardless. Finding #1 verified.
All three findings are confirmed against the code. Let me also verify the test naming concern (finding #3) — `kept` is indeed soft-removed by `ClearObject` (line 357) and never asserted.
Verified. The three findings survive:
1. `internal/api/gardens.go:120` — UI always sends `{ name: name ?? '' }` via `useCopyGarden`, so the `ContentLength != 0` branch is dead for the web client (only the curl/test path exercises the no-body branch).
2. `web/src/components/gardens/CopyGardenModal.tsx:20` — `" (copy)"` suffix hardcoded client-side; `copyName` in `internal/service/gardens.go` defines it server-side. Duplication confirmed.
3. `internal/service/gardens_test.go:364` — `_ = kept` discards the variable; `kept` was created at line 350, then `ClearObject` (line 357) soft-removes all plantings including `kept`, and `kept` is never used in any assertion.
Minor issues
- `internal/api/gardens.go:120` — the `c.Request.ContentLength != 0` branch advertises an optional-body contract, but the only real caller (`useCopyGarden` in `web/src/lib/gardens.ts:98`) always posts `{ name: name ?? '' }`, so the no-body path is only exercised by the curl/test caller. The API surface and the UI client disagree on which contract is authoritative; a future reader will wonder which is canonical. Cosmetic leaky-abstraction smell, not blocking.
- `web/src/components/gardens/CopyGardenModal.tsx:20` — `useState(\`${garden.name} (copy)\`)` reconstructs the server's `copyName` derivation client-side (the `" (copy)"` suffix lives in `internal/service/gardens.go`). The component comment claims the prefill is "the server's own default so what you see is what you get," but no server call fetches it — the suffix is duplicated. If the server's suffix or truncation rule changes, the UI's prefill silently drifts; the test suite asserts the suffix as a contract, so the duplication is a latent maintainability trap.
- `internal/service/gardens_test.go:364` — `_ = kept` discards an assigned-but-unused variable. `kept` is created at line 350, then soft-removed by `ClearObject` at line 357, and never asserted. The name `kept` is misleading (it's actually the cleared plop) and the discard is cosmetic noise. Rename to reflect its fate or drop the assignment and use `_` at creation.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
Verdict: No material issues found
Through the performance lens:
- The copy path is O(objects + plantings) with one round-trip per object and one per planting insert (`internal/store/gardens.go:215-253`). This is linear, not quadratic, and it's a user-initiated one-shot operation rather than a hot path, so the per-row INSERT overhead is acceptable. A bulk/`INSERT ... SELECT` rewrite could reduce round-trips but isn't material here.
- `queryObjects`/`queryPlantings` fully drain their cursors before the transaction issues writes (`internal/store/objects.go:78-97`, `internal/store/plantings.go:44-63`), correctly avoiding the SQLite "open cursor + write on same connection" trap. The reads happen once each, before the insert loops — no cursor-held-during-write regression.
- `requireGardenRole` reads the source garden row, then the store's `INSERT ... SELECT` reads it again (`internal/service/gardens.go:171`, `internal/store/gardens.go:189-195`). One extra single-row read on a non-hot path; not worth changing.
- The two in-memory slices (`srcObjects`, `srcPlantings`) and the `objectIDs` map scale with garden size. Bounded by a single garden's contents on a copy action; no unbounded/paginated-growth concern.
I verified these by reading `internal/store/gardens.go`, `internal/store/objects.go`, `internal/store/plantings.go`, and `internal/service/gardens.go` at the checked-out state.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
Now let me look at the `copyName` rune-boundary truncation carefully. The truncation uses `src[:room]` which slices by bytes, then `ToValidUTF8` cleans up. But it trims to `room` bytes, not on a rune boundary. Let me check the test case.
Let me check the UI error handling for the modal and the API.
Now let me re-examine `copyName` more carefully. `maxGardenNameLen = 200`, `suffix = " (copy)"` = 7 bytes, `room = 193`. If `len(src) > 193`, truncate to `src[:193]` (byte slice, possibly splitting a rune), then `ToValidUTF8` replaces invalid sequences with empty string. But `TrimSpace` is applied AFTER `ToValidUTF8` — wait, no: `strings.TrimSpace(strings.ToValidUTF8(src[:room], ""))`. So ToValidUTF8 cleans partial rune, then TrimSpace trims whitespace. Then `src + suffix`. Length check: result ≤ `room + len(suffix)` = 200. OK.
But there's a subtle issue: if `src[:room]` ends mid-rune AND the rune before is a valid character, ToValidUTF8 drops the partial bytes. Fine. If room falls such that the last byte is part of a multi-byte rune boundary that includes whitespace... edge cases are handled. Tests pass. The truncation logic seems correct.
Let me check the `queryPlantings` reuse — the diff modified `ListActivePlantingsForObject` to use `queryPlantings`, but the original used `plantingColumns` unqualified. Let me verify the new version still works.
Now let me check whether `requireGardenRole` returns a garden with `MyRole` populated, and whether `src.Name` is reliable. Also let me verify the `CopyGarden` store function doesn't check that `ownerID` actually exists (FK violation edge case).
Now let me check whether the `INSERT ... SELECT` from the store can return `sql.ErrNoRows` properly and whether the `scanGarden` handles the `sql.ErrNoRows` correctly when the source is gone.
Now let me check the UI mutation — what does `gardenSchema.parse` do, and is the returned object the right shape (does the copy endpoint return a full Garden)? Also check that `created.id` exists in the schema.
Now let me check the `ContentLength != 0` check in the API handler — what about a request with `Content-Length: 0` but a body, or chunked encoding (no Content-Length)?
Now let me check if there are any unique constraints that could fail on copy (e.g., garden name uniqueness, or public_token uniqueness — but public_token is not copied).
Now let me verify the `Count` field handling in copy. The `plantings` INSERT copies `count` as `p.Count` (a `*int`). If `Count` is NULL in DB, the scan reads it as nil, and the INSERT inserts nil. That works.
Let me look at how `PlantedAt` is handled — it's a `*string`. In the copy, `p.PlantedAt` is passed directly. If NULL, nil passed to INSERT — fine.
Now, a key edge case: the copy INSERT for plantings does NOT copy `removed_at`, `version`, `created_at`, `updated_at`. The defaults handle those. Good — removed_at defaults NULL (active), version defaults 1, timestamps default now. That's correct for a fresh layout.
Let me check whether there's an issue with the `Label` field — it's `*string`, nullable. Same handling. Fine.
Now let me check the `points` column in objects — it's `*string`, nullable. The copy passes `o.Points` directly. Fine.
Let me verify the `Color` and `Props` nullable handling in the object copy. Looking at the INSERT: `o.Color, o.Props` are `*string` passed directly. Fine.
Now, one important edge case in the store CopyGarden: what if `srcID == ownerID`... no, srcID is gardenID, ownerID is userID. Different.
Let me check: is there any validation that `ownerID` (the actor) still exists when the copy is made? The actor is authenticated, so they exist. The INSERT into gardens has `owner_id REFERENCES users(id)`, so FK is satisfied. Fine.
Now, the critical edge case I want to verify: **the `ContentLength != 0` check**. With chunked transfer encoding, `ContentLength` is -1, not 0. So a chunked POST with an actual body would skip decoding, leaving `req.Name` empty, and the service derives the default name. That's a silent behavior difference but not an error handling issue per se — the body would just be ignored. Not a crash, not a swallowed error. The browser always sends Content-Length for POST with body. Minor.
Let me check: what if `ShouldBindJSON` succeeds but the JSON has extra fields or `name` is null? `null` for a string field → Go leaves it as ""? Actually `json.Unmarshal` of `null` into a string leaves it unchanged (empty string default). So `{"name": null}` → `req.Name = ""` → derived name. Fine.
Let me verify the `writeServiceError` mapping for `ErrInvalidInput` → 400.
Now let me double-check the `copyName` truncation edge case where the source name has trailing whitespace. Suppose `src.Name` (after the source was created) has trailing whitespace that was trimmed at creation. Actually `CreateGarden` trims the name. But `src` here comes from `requireGardenRole` → `GetGarden`, which reads the stored (already-trimmed) name. So `src.Name` is already trimmed. The `copyName` function trims again — harmless.
But there's a subtle issue: in `copyName`, if `len(src) > room`, it does `strings.TrimSpace(strings.ToValidUTF8(src[:room], ""))`. The `TrimSpace` could shrink the result further, making `len(src) < room`, then `src + suffix` < maxGardenNameLen. Fine — under cap. But what if after trimming, `src` becomes empty? Then result is just `" (copy)"` = 7 bytes. That's a valid name. Edge case but handled.
Now, is there a scenario where `copyName` returns a name that's empty after... no, suffix is always appended. Fine.
Let me check one more thing: the `ToValidUTF8` is applied to `src[:room]` which is a byte slice. If `src` contains invalid UTF-8 (shouldn't, since it came from DB which stored valid UTF-8), but defensive. Fine.
Let me verify the `queryObjects`/`queryPlantings` defer rows.Close() — they have `defer rows.Close()`. Good, no cursor leak.
Now let me check the `tx.QueryRowContext` for the object INSERT inside the loop — if it returns an error, we return and the deferred `tx.Rollback()` runs. Good. The `defer rows.Close()` in queryObjects runs. Good.
Let me check: in the store CopyGarden, after `queryObjects` drains the cursor and returns, we then do INSERTs in a loop on `tx`. Since the cursor is closed, no conflict. Good.
Now, the one thing I want to verify: **does `tx.QueryRowContext` for the INSERTs conflict with the still-open cursor from `queryPlantings`?** No — `queryPlantings` drains and closes before returning. So when we do `tx.ExecContext` for planting inserts, the cursor is closed. Good.
Let me verify there's no issue with `scanGarden` being called with `tx.QueryRowContext` — `*sql.Tx.QueryRowContext` returns `*sql.Row` which implements `scanner` (has `Scan`). Good.
Good. `*sql.Tx` implements both `QueryContext` (satisfies `queryer`) and `QueryRowContext` returns `*sql.Row` which satisfies `scanner`. Everything compiles.
Now let me consider the error-handling lens more carefully. The `defer tx.Rollback()` with `errcheck` nolint — this is the standard pattern. After commit, rollback returns `sql.ErrTxDone` which is ignored. Fine. On the error paths before commit, rollback runs and cleans up. Good.
One concern: in the store `CopyGarden`, when `queryObjects` or `queryPlantings` returns an error, we return `err` directly (not wrapped). The `queryObjects`/`queryPlantings` already wrap with "store: list objects/plantings". But in the copy context, "list objects" is slightly misleading (it's a copy operation). Minor, not an error-handling bug.
Let me check one potential real issue: **the `INSERT ... SELECT ... FROM gardens WHERE id = ?` with RETURNING**. Does SQLite support `INSERT ... SELECT ... RETURNING`? Modern SQLite (3.35+) does. The project uses it elsewhere (CreateGarden uses RETURNING). So fine.
But a subtle edge: if `srcID` doesn't exist, the SELECT yields 0 rows, INSERT inserts nothing, and `RETURNING` yields no rows. `tx.QueryRowContext` then returns `*sql.Row` whose `.Scan()` returns `sql.ErrNoRows`. The code checks `errors.Is(err, sql.ErrNoRows)` → `domain.ErrNotFound`. Good, verified at line 196.
Now, let me check a potential issue: **the transaction isolation**. We read the source garden, then read its objects, then read its plantings — all within the tx. If another transaction modifies the source concurrently, we get a consistent snapshot (SQLite serializable). Good.
But wait — the `requireGardenRole` call in the SERVICE happens OUTSIDE the store transaction (it's a separate `GetGarden` call). So there's a TOCTOU window: the garden could be deleted between the role check and the store copy. But if deleted, the `INSERT ... SELECT` yields no rows → `ErrNotFound`. So the store handles it. Good — no panic, clean error.
Let me verify there's no nil-pointer issue: `src` from `requireGardenRole` is used for `src.Name`. If `requireGardenRole` returns nil garden with nil error? No — it returns `g, nil` only with a valid garden. Good.
Now let me check the `created` return from store — if `scanGarden` succeeds, `created` is non-nil. Then service sets `created.MyRole`. No nil deref. Good.
Let me check the test file for the `seedBed`, `seedOwnPlant`, `seedGarden`, `seedUser` helpers to make sure they exist.
All helpers exist. Now let me do a final check on the `copyName` function for a tricky edge: what if `maxGardenNameLen - len(suffix)` could be negative? `maxGardenNameLen = 200`, `len(suffix) = 7`, `room = 193`. Always positive. No overflow.
What if `len(src)` is huge? `src[:room]` is a valid slice (room ≤ len(src) since we're in the `len(src) > room` branch). Fine.
I've thoroughly examined the error-handling and edge cases. Let me look at one last thing — the `Count` field in the planting copy. Let me verify the INSERT column list matches the values. The INSERT: `(object_id, plant_id, x_cm, y_cm, radius_cm, count, label, planted_at)` — 8 columns, 8 placeholders `?, ?, ?, ?, ?, ?, ?, ?`. Values: `objectID, p.PlantID, p.XCM, p.YCM, p.RadiusCM, p.Count, p.Label, p.PlantedAt` — 8 values. Matches. Good.
The copy does NOT carry `removed_at`, so the new planting is active (NULL). Good. It does NOT carry `version` (defaults to 1), `created_at`/`updated_at` (default to now). Good — fresh row.
I'm confident the error handling and edge cases are well-covered. Let me verify one potential concern: the UI modal's `onClose` is called BEFORE `navigate`. If `onClose` unmounts the modal/component, does `navigate` still work? `onClose` sets `dialog` to null in the parent, which unmounts `CopyGardenModal`. But `navigate` is called from within the `onSubmit` async function which already holds the `navigate` reference. The `useNavigate` hook returns a stable function. Calling it after unmount is fine (TanStack Router's navigate is not tied to component lifecycle). And `toast.info` also works after unmount. This is a common pattern. No error-handling issue.
Actually, wait — `onClose()` is called, then `navigate()`. If `onClose` triggers a state update that causes re-render, the current execution context (the async function) continues fine. No issue.
Let me check: is there a race where `copy.isPending` stays true and the user can't interact? No — `mutateAsync` resolves, `isPending` becomes false. The error path sets `error` and the user can retry. Good error recovery.
One more: the modal's submit button is `disabled={copy.isPending || name.trim() === ''}`. If the user submits a whitespace-only name, the button is disabled — but they could still press Enter in the TextField (form submit). The `onSubmit` does `name.trim()` and sends it. If empty after trim, the server derives the default name. Wait — `name.trim() === ''` disables the button, so the form can't be submitted via button click. But Enter key in a text field submits the form. The button is `disabled`, so Enter won't trigger it (disabled submit button blocks form submission). Actually, in HTML, if the submit button is disabled, Enter in a text input may still submit the form in some browsers. But `onSubmit` handles it: `name.trim()` → empty string sent → server derives default. No error. Fine.
I've completed my review through the error-handling/edge-cases lens. Everything is robust:
- Transaction rollback via defer handles partial failures
- Cursors are drained before writes (SQLite constraint)
- Missing source → ErrNotFound via sql.ErrNoRows
- TOCTOU between role check and copy is handled by the INSERT...SELECT returning no rows
- Name truncation handles rune boundaries and produces valid UTF-8 under the cap
- Empty/whitespace names fall through to derivation
- Over-long explicit names rejected
- Optional body parsed safely with fallback
- FK constraints satisfied (actor exists)
- No nil derefs on the return paths
No material error-handling or edge-case issues found.
## Verdict: No material issues found
Reviewed the deep-copy change end-to-end through the error-handling & edge-cases lens. The unhappy paths are well-covered:
- **Store `CopyGarden` transaction** (`internal/store/gardens.go:179-259`): `defer tx.Rollback()` runs on every error path and is a no-op after commit; partial failures leave no half-populated garden. Verified the `INSERT ... SELECT ... RETURNING` returns `sql.ErrNoRows` when the source is gone, mapped to `domain.ErrNotFound` (line 196-197) — so the TOCTOU window between the service-level `requireGardenRole` read and the store write degrades cleanly rather than panicking.
- **Cursor discipline** (`internal/store/objects.go:78-97`, `plantings.go:44-63`): both `queryObjects`/`queryPlantings` `defer rows.Close()` and fully drain before returning, so the subsequent `tx.ExecContext`/`tx.QueryRowContext` writes don't hit SQLite's "cursor open while writing on the same connection" limit. Verified the in-memory single-connection rationale in the comments.
- **Name derivation** (`internal/service/gardens.go:193-199`): `copyName` truncates `src[:room]` (byte slice) then `strings.ToValidUTF8(..., "")` repairs any split rune, then `TrimSpace`; result is always ≤ `room + len(suffix)` = `maxGardenNameLen`. Confirmed `room = 200 - 7 = 193 > 0`, no underflow. The dedicated `TestCopyGardenNameRuneBoundary` exercises the multi-byte case and asserts `utf8.ValidString` + length cap.
- **Input bounds**: blank/absent name → derivation; over-long explicit name → `ErrInvalidInput` (line 179); API handler treats `ContentLength != 0` as "a body was sent" and rejects malformed JSON as 400. `{"name": null}` decodes to `""` → derivation, not a crash.
- **Planting copy** (`internal/store/gardens.go:246-252`): 8 columns / 8 placeholders / 8 values match; `count`/`label`/`planted_at` nullable `*` fields pass through as nil for NULL source values; `removed_at`/`version`/timestamps deliberately omitted so defaults yield a fresh active row.
- **Unreachable-planting guard** (line 241-244): the `objectIDs` map miss returns a wrapped error rather than proceeding with a zero `objectID` — defensive against a logic break.
- **UI modal** (`CopyGardenModal.tsx`): `mutateAsync` errors are caught and surfaced via `Alert`, `copy.isPending` disables actions during flight, and `name.trim() === ''` blocks submission. The `created` return is zod-parsed before use.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found through the security lens.
- **AuthZ/owner-only guard verified**: `CopyGarden` calls `requireGardenRole(..., roleOwner)` at `internal/service/gardens.go:171`, so only owners can copy. Confirmed by service tests: strangers get `ErrNotFound`, editors get `ErrForbidden`. The UI correctly only shows the Copy button for owners.
- **No public_token leakage**: The new garden is inserted with `INSERT ... SELECT` that omits `public_token`; `GetPublicShareLink` on the copy returns empty token, and `PublicGarden` with the source token still resolves to the source (tested in service tests).
- **No share inheritance**: `garden_shares` rows are not copied; confirmed by service tests.
- **No SQL injection**: Parameterized queries used throughout; no string concatenation of user-controlled data.
- **No path/template/command injection**: Only integer IDs parsed from params; names are plain strings without template evaluation.
- **Name length cap enforced**: Explicit names over `maxGardenNameLen` are rejected with `ErrInvalidInput`; derived names are truncated safely with `ToValidUTF8`.
- **No SSRF/unsafe deserialization**: Nothing in the diff opens outbound connections or deserializes untrusted data.
- **Transaction safety**: Copy runs in one transaction with rollback on failure; no partial-state exposure.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found. All logic paths, formulas, and edge cases were verified and are correct.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `internal/api/gardens.go:120` — The `copyGarden` handler checks `c.Request.ContentLength != 0` to decide whether to parse an optional JSON body, but the codebase already has an established pattern for this in `internal/api/public.go:56`: try `c.ShouldBindJSON(&req)` and ignore `io.EOF`. The `ContentLength` guard is both inconsistent with surrounding code and less robust (e.g., chunked encoding or HTTP/2 can leave `ContentLength == -1` even with no body, which would cause the handler to incorrectly reject a valid no-body request as `INVALID_INPUT`). **Fix:** Remove the `ContentLength` guard and use `if err := c.ShouldBindJSON(&req); err != nil && !errors.Is(err, io.EOF) { … }`, adding `"io"` to the imports.
- `internal/service/gardens_test.go:364` (in `TestCopyGardenSkipsRemovedPlantings`) — The test assigns a planting to `kept` only to suppress it with `_ = kept`. Using `_, err := s.CreatePlanting(...)` on that line avoids the unused-variable workaround.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
The copy implementation is structurally sound from a performance standpoint:
- **Single transaction**: `CopyGarden` wraps everything in one `BEGIN...COMMIT` block, avoiding partial-state overhead and extra round-trips.
- **No N+1 queries**: Objects and plantings are each fetched with one query (the planting query uses a `JOIN garden_objects` to scope by garden). Reparenting is done in-memory via the `objectIDs` map.
- **Bounded allocations**: The `objectIDs` map is pre-sized (`make(..., len(srcObjects))`). Result slices are drained and closed before writes, which is a deliberate SQLite-safe pattern, not an allocation leak.
- **No hot-path quadratic behavior**: The loops over `srcObjects` and `srcPlantings` are linear and triggered by an explicit user action (`POST /copy`), not a background or high-frequency path.
- **Name length bounded**: `maxGardenNameLen` prevents unbounded input.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**Verdict:** No material issues found
I investigated the copy-garden flow through the error-handling & edge-cases lens and found the unhappy paths are properly covered:
- **Transaction safety** — `CopyGarden` in `internal/store/gardens.go:179` opens a transaction, has `defer tx.Rollback()`, and only commits after all inserts succeed. Any failure mid-way rolls back, leaving no half-copied garden.
- **Empty/whitespace-only name** — `CopyGarden` in `internal/service/gardens.go:175` trims the name and falls back to `copyName(src.Name)` when blank, matching the documented behavior.
- **Over-long name** — Rejected with `ErrInvalidInput` rather than silently truncated (`internal/service/gardens.go:179`).
- **Rune-boundary truncation** — `copyName` (`internal/service/gardens.go:193`) uses `strings.ToValidUTF8` on the sliced prefix so multi-byte runes are never split.
- **Orphaned planting guard** — The `objectIDs` lookup in `internal/store/gardens.go:241` is defensive: if an active planting somehow references an object that wasn’t copied, it returns an explicit error and the deferred rollback cleans up.
- **Optional request body** — The API handler (`internal/api/gardens.go:120`) only binds JSON when `ContentLength != 0`, so a no-body POST correctly triggers the default-name path.
- **Zero objects / zero plantings** — Both `queryObjects` and `queryPlantings` return non-nil empty slices; the loops simply don’t execute and the empty garden commits successfully.
No ignored errors, no missing cleanup, no unhandled nil/empty/zero cases, and no boundary issues were found in this change.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
* api: detect an absent copy body by io.EOF from the decoder rather than
Content-Length. A chunked request reports -1, not 0, so the length test
read an empty chunked body as malformed and 400'd instead of taking the
default-name path. Covered by a new unknown-length test.
* store: hoist the garden_objects/plantings INSERTs into shared
objectInsert/plantingInsert statements with matching *InsertArgs
helpers. CopyGarden had its own copies of both column lists, so a
column added to the table could be wired into Create* and silently
dropped from a copy.
* store: move the queryer interface to sqlite.go, next to the other
shared query plumbing, instead of users.go.
* web: derive a copy's prefilled name via defaultCopyName, mirroring the
server's copyName including its 200-BYTE cap. The inline
`${name} (copy)` both duplicated the suffix and, for a garden whose
name was already at the cap, prefilled an over-long name that the
server rejected with a 400. Unit-tested, including multi-byte
truncation on a code-point boundary.
* test: drop a throwaway `_ = kept`.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Gadfly swarm returned 11 findings (several duplicated across models). Addressed in e1ec93d:
Finding
Action
Empty chunked body → 400 instead of default-name path (ContentLength is -1, not 0)
Fixed — detect an absent body by io.EOF from the decoder; new unknown-length test
Prefilled copy name uncapped → guaranteed 400 for a garden already at the name cap
Fixed — defaultCopyName mirrors the server's copyName, 200-byte cap included; unit-tested with multi-byte truncation
Copy-suffix duplicated client-side, drifts from copyName
Fixed — same helper, with a comment tying it to the server function
INSERT column lists duplicated between Create* and in-tx CopyGarden
Fixed — shared objectInsert/plantingInsert + *InsertArgs; a new column can no longer land in one path and be dropped from the other
queryer declared in users.go
Fixed — moved to sqlite.go beside the other shared query plumbing
Throwaway _ = kept / misleading dead variable
Fixed — removed
Two findings I'm deliberately not acting on:
"Optional body the UI never exercises is contract ambiguity." The no-body form is there for non-UI callers — the internal/agent toolbox is the intended second consumer, and POST .../copy with no body is the natural shape for a tool call. It's covered by two tests now (absent body and unknown-length empty body), so it isn't an untested path.
"Row-at-a-time inserts inside one write transaction serialize other writers." True in principle, but this matches the existing CreatePlantings batch, which loops the same way, and household-scale gardens are tens of objects and hundreds of plops — a few ms under the write lock. Batching would mean multi-row VALUES and giving up RETURNING for the object id→id map. Not worth the complexity here; worth revisiting if garden sizes ever grow by orders of magnitude.
Not graded, per usual.
Gadfly swarm returned 11 findings (several duplicated across models). Addressed in e1ec93d:
| Finding | Action |
|---|---|
| Empty **chunked** body → 400 instead of default-name path (`ContentLength` is `-1`, not `0`) | **Fixed** — detect an absent body by `io.EOF` from the decoder; new unknown-length test |
| Prefilled copy name uncapped → guaranteed 400 for a garden already at the name cap | **Fixed** — `defaultCopyName` mirrors the server's `copyName`, 200-**byte** cap included; unit-tested with multi-byte truncation |
| Copy-suffix duplicated client-side, drifts from `copyName` | **Fixed** — same helper, with a comment tying it to the server function |
| `INSERT` column lists duplicated between `Create*` and in-tx `CopyGarden` | **Fixed** — shared `objectInsert`/`plantingInsert` + `*InsertArgs`; a new column can no longer land in one path and be dropped from the other |
| `queryer` declared in `users.go` | **Fixed** — moved to `sqlite.go` beside the other shared query plumbing |
| Throwaway `_ = kept` / misleading dead variable | **Fixed** — removed |
Two findings I'm deliberately **not** acting on:
- **"Optional body the UI never exercises is contract ambiguity."** The no-body form is there for non-UI callers — the `internal/agent` toolbox is the intended second consumer, and `POST .../copy` with no body is the natural shape for a tool call. It's covered by two tests now (absent body and unknown-length empty body), so it isn't an untested path.
- **"Row-at-a-time inserts inside one write transaction serialize other writers."** True in principle, but this matches the existing `CreatePlantings` batch, which loops the same way, and household-scale gardens are tens of objects and hundreds of plops — a few ms under the write lock. Batching would mean multi-row `VALUES` and giving up `RETURNING` for the object id→id map. Not worth the complexity here; worth revisiting if garden sizes ever grow by orders of magnitude.
Not graded, per usual.
steve
merged commit e22bdd6cab into main2026-07-21 03:58:40 +00:00
steve
deleted branch feat/copy-garden2026-07-21 03:58:40 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Adds a way to duplicate a garden, so a layout can be forked and experimented on without touching the original.
API
POST /gardens/:id/copy— optional{"name": "..."}body; a blank/absent name derives"<source> (copy)". Returns201+ the new garden.Copies the garden's settings, its objects, and each object's currently-planted plops.
Deliberately not carried over:
public_token— the share link is a capability granted to the original; a copy must not silently inherit a live public URL.garden_shares— a copy is private to whoever made it.Owner-only, for now
A copy's plops keep pointing at the source's
plant_ids, and a custom plant is owned by one user. Letting a viewer copy someone else's garden would hand them a garden referencing plants outside their catalog, and would pin the original owner's plants against deletion (plantings.plant_idisON DELETE RESTRICT). Copying a shared garden needs a plant-cloning policy first; the owner's own copy is the case that's unambiguously correct today.Implementation
The whole copy runs in one transaction, so a failure part-way leaves no half-populated garden. The object/planting list scans are factored into
queryObjects/queryPlantingsover a smallqueryerinterface, so the same scan code serves both a plain read and a read inside that transaction — and each drains its cursor before returning, since SQLite can't hold a cursor open and write on the same connection.UI
A
Copyaction on the owner's garden card opens a modal prefilled with the derived name, then lands you in the new garden.Tests
Service: contents duplicated + source untouched, removed plops skipped, naming (explicit / derived / over-cap rejected / rune-boundary truncation), permissions (stranger 404, editor 403, missing 404), no inherited public link, no inherited shares. API: no-body and named copy, list count, 401/404/400 paths.
Verified locally end-to-end in a browser: create → Copy → modal → lands in the copy.
🤖 Generated with Claude Code
Adds POST /gardens/:id/copy: duplicates a garden the actor owns, along with its objects and their currently-planted plops, into a new garden owned by the actor. A blank name derives "<source> (copy)". Deliberately not carried over: * public_token — the share link is a capability granted to the original; a copy must not silently inherit a live public URL * garden_shares — a copy is private to whoever made it * removed plantings — the copy is a fresh layout, not a history Owner-only for now: a copy's plops keep pointing at the SOURCE's plant_ids, so letting a viewer copy someone else's garden would hand them plants outside their catalog and pin the original owner's plants against deletion (plantings.plant_id is ON DELETE RESTRICT). Copying a shared garden needs a plant-cloning policy first. The whole copy runs in one transaction, so a failure part-way leaves no half-populated garden. The object/planting list scans are factored into queryObjects/queryPlantings so the same code serves a plain read and a read inside that transaction. UI: a Copy action on the owner's garden card opens a modal prefilled with the derived name, then lands in the new garden. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-21 03:56:46Z
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 — 6 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -106,0 +117,4 @@// The body is optional — POST with no body means "copy under the default// name" — so only decode when one was actually sent.var req gardenCopyRequestif c.Request.ContentLength != 0 {🟠 Inconsistent optional-body pattern uses ContentLength instead of io.EOF
error-handling, maintainability · flagged by 4 models
internal/api/gardens.go:120— ThecopyGardenhandler checksc.Request.ContentLength != 0to decide whether to parse an optional JSON body, but the codebase already has an established pattern for this ininternal/api/public.go:56: tryc.ShouldBindJSON(&req)and ignoreio.EOF. TheContentLengthguard is both inconsistent with surrounding code and less robust (e.g., chunked encoding or HTTP/2 can leaveContentLength == -1even with no body, which would cause the handler to incorre…🪰 Gadfly · advisory
@@ -252,0 +345,4 @@bed := seedBed(t, s, owner, src.ID)plant := seedOwnPlant(t, s, owner, 20)kept, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{PlantID: plant.ID, RadiusCM: 10})🟡 Misleadingly-named dead variable
keptin TestCopyGardenSkipsRemovedPlantings — it is actually cleared, not keptmaintainability · flagged by 1 model
internal/service/gardens_test.go:348(TestCopyGardenSkipsRemovedPlantings) — the local varkept(declared line 348, silenced via_ = keptat line 364) is misleadingly named and is dead code. It's created, never asserted on, and then discarded just to silence "declared and not used." Worse, the name actively misleads:ClearObjectPlantings(internal/store/plantings.go:76-92), invoked viaClearObject(internal/service/ops.go:221-227), soft-removes every active plop on the object…🪰 Gadfly · advisory
@@ -252,0 +361,4 @@if err != nil {t.Fatalf("CreatePlanting(replanted): %v", err)}_ = kept🟡 Unused variable suppressed with _ = kept instead of using _ in original assignment
maintainability · flagged by 3 models
internal/service/gardens_test.go:364(inTestCopyGardenSkipsRemovedPlantings) — The test assigns a planting tokeptonly to suppress it with_ = kept. Using_, err := s.CreatePlanting(...)on that line avoids the unused-variable workaround.🪰 Gadfly · advisory
@@ -166,0 +213,4 @@// re-parented onto the right new object.objectIDs := make(map[int64]int64, len(srcObjects))for _, o := range srcObjects {copied, err := scanObject(tx.QueryRowContext(ctx,🟠 CopyGarden inserts objects/plantings one row at a time inside a single write transaction, serializing all other writers under WAL for O(n) round-trips
maintainability, performance · flagged by 2 models
internal/store/gardens.go:216-253—CopyGardenexecutes oneINSERT ... RETURNINGper source object and oneINSERTper active planting, sequentially, inside a single write transaction. There is no cap on how many objects/plantings a garden can hold (maxGardensListedatinternal/store/gardens.go:50only bounds the list endpoint). The DB is opened in WAL mode withbusy_timeout(5000)(internal/store/sqlite.go:20-21), so only one writer can proceed app-wide; every other user's wri…🪰 Gadfly · advisory
@@ -19,6 +19,12 @@ type scanner interface {Scan(dest ...any) error}// queryer is the read surface shared by *sql.DB and *sql.Tx, so a list helper can⚪ Generic queryer interface declared in users.go rather than a shared store file
maintainability · flagged by 1 model
internal/store/gardens.go:217andinternal/store/plantings.go:112/:135— theINSERT INTO garden_objects …andINSERT INTO plantings …column lists are duplicated verbatim betweenCreateObject/CreatePlantingand the in-transaction copy inCopyGarden. The copy can't callCreateObject/CreatePlantingdirectly because those bind tod.sqlrather thantx, so the duplication is a deliberate tradeoff, but it means a future schema/column change must be edited in two (objects) / t…🪰 Gadfly · advisory
@@ -0,0 +17,4 @@export function CopyGardenModal({ garden, onClose }: { garden: Garden; onClose: () => void }) {const copy = useCopyGarden()const navigate = useNavigate()const [name, setName] = useState(`${garden.name} (copy)`)🟠 Client-side copy-name derivation diverges from server truncation logic
correctness, error-handling, maintainability · flagged by 3 models
web/src/components/gardens/CopyGardenModal.tsx:20— The modal pre-derives the copy name with`${garden.name} (copy)`, duplicating the server'scopyName()logic but without the byte-cap truncation. If the source name is long, the prefilled default can exceedmaxGardenNameLenand will be rejected by the service when submitted unchanged. The client and server derivations are out of sync. Fix: either share the truncation logic (e.g., a common util) or prefill with a name capped the…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 6 findings (4 with multi-model agreement)
internal/api/gardens.go:120web/src/components/gardens/CopyGardenModal.tsx:20internal/service/gardens_test.go:364internal/store/gardens.go:2162 single-model findings (lower confidence)
internal/service/gardens_test.go:348internal/store/users.go:22Per-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
All static constants — no injection surface. The security-relevant design (owner-only copy, no token/share inheritance, parameterized SQL throughout) checks out and is well-tested. Nothing material in my lens.
VERDICT: No material issues found
CopyGarden(internal/service/gardens.go:172) correctly gates onroleOwnerviarequireGardenRole, which returnsErrNotFoundfor no-access andErrForbiddenfor insufficient access (viewer/editor) — verified againstrequireGardenRole/effectiveGardenRole(internal/service/gardens.go:69-105) and exercised byTestCopyGardenPermissions.plant_ids, and avoiding pinning those plants againstON DELETE RESTRICT) is actually enforced in code, not just asserted in comments.internal/store/gardens.goCopyGardenuse parameterized?placeholders;gardenColumns/objectColumns/plantingColumns/qualifyColumnsare static Go constants, not built from request input — no injection surface.public_tokenis deliberately excluded from theINSERT ... SELECTcolumn list inCopyGarden(internal/store/gardens.go:172), and the column has no default other thanNULL, so a copy starts with no public link — confirmed via migration0004_garden_public_token.sqlandTestCopyGardenDoesNotInheritPublicLink.garden_sharesis likewise never touched, confirmed byTestCopyGardenDoesNotInheritShares.v1group withh.csrfGuard()applied at the group level (internal/api/api.go), so it isn't missing CSRF protection that other mutating endpoints have.nameis length-capped (maxGardenNameLen) and rejected rather than silently truncated when explicitly supplied; the request body is optional and a non-stringnameis rejected byShouldBindJSONwith 400. TheContentLength != 0check (internal/api/gardens.go) can mis-trigger on chunked requests withContentLength == -1, but that's a correctness/robustness nit, not a security issue (worst case is a spurious 400), so it's outside this lens.🎯 Correctness — Minor issues
Confirms GardenFormModal (create/edit) also has no maxLength cap, so a garden name up to 200 bytes is indeed creatable through the normal UI, then reachable via copy. The finding fully checks out.
Corrected review
VERDICT: Minor issues
web/src/components/gardens/CopyGardenModal.tsx:20— the modal prefills the name field with`${garden.name} (copy)`with no length cap, but the server (internal/service/gardens.go:175-181) only applies the gracefulcopyNametruncation when the incoming name is blank; a non-blank name overmaxGardenNameLen(200 bytes,internal/service/gardens.go:20) hits the strictdomain.ErrInvalidInputpath instead. SinceGardenFormModal.tsxalso has nomaxLengthon the name field (confirmed nomaxLengthprop anywhere inweb/src/components/gardens/GardenFormModal.tsxorweb/src/components/ui/TextField.tsx), a garden name legitimately created near the 200-byte cap becomes uncopyable without the user first editing the prefilled field — appending " (copy)" (7 bytes) pushes it over the cap and the request is rejected with a 400, surfaced to the user as "Could not copy the garden."copyNameleaves (mirroring its truncation logic), or have the service truncate rather than reject over-cap names on this endpoint.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
internal/service/gardens_test.go:348(TestCopyGardenSkipsRemovedPlantings) — the local varkept(declared line 348, silenced via_ = keptat line 364) is misleadingly named and is dead code. It's created, never asserted on, and then discarded just to silence "declared and not used." Worse, the name actively misleads:ClearObjectPlantings(internal/store/plantings.go:76-92), invoked viaClearObject(internal/service/ops.go:221-227), soft-removes every active plop on the object with no per-row selectivity — sokeptis cleared right along with the other, unnamed planting at line 357'sClearObjectcall. It is not "kept" at all. A future reader skimming this test could easily conclude one plop survives the clear while the other doesn't, which isn't what's being tested. Drop the variable (mirror theif _, err := s.CreatePlanting(...)form already used two lines below for the plop that's supposed to be cleared) or rename it to something that doesn't imply survival, e.g.clearedA/clearedB.internal/store/gardens.go:216-230andinternal/store/gardens.go:246-252—CopyGarden's per-row inserts duplicate the exact INSERT SQL and argument list already living inCreateObject(internal/store/objects.go:38-46) andCreatePlanting/CreatePlantings(internal/store/plantings.go:110-121,:135-137), just issued againsttxinstead ofd.sql. The PR already introduces exactly the abstraction that would fix this on the read side (queryerininternal/store/users.go:24-26, shared byqueryObjects/queryPlantings); doing the analogous thing for these inserts (anexecer-style interface satisfied by both*sql.DBand*sql.Tx, withCreateObject/CreatePlantingparameterized over it) would have letCopyGardencall the existing helpers instead of re-typing the SQL. This does follow a pre-existing precedent in the codebase (CreatePlantingsalready duplicatesCreatePlanting's INSERT for its tx-batch case), so it's not a new pattern this PR invented, but the PR had the ideal opportunity — and the tooling already in hand — to close that gap instead of extending it to a third copy.⚡ Performance — Minor issues
All claims check out: the loop structure (one
INSERT...RETURNINGper object, oneINSERTper planting, lines 216-253), the WAL +busy_timeout(5000)pragmas (sqlite.go:20-21), the absence of any per-garden object/planting cap,maxGardensListedonly bounding the list endpoint (gardens.go:50), and the indexes existing in the migration. The finding is accurate.VERDICT: Minor issues
internal/store/gardens.go:216-253—CopyGardenexecutes oneINSERT ... RETURNINGper source object and oneINSERTper active planting, sequentially, inside a single write transaction. There is no cap on how many objects/plantings a garden can hold (maxGardensListedatinternal/store/gardens.go:50only bounds the list endpoint). The DB is opened in WAL mode withbusy_timeout(5000)(internal/store/sqlite.go:20-21), so only one writer can proceed app-wide; every other user's write queues behindbusy_timeoutwhile this transaction is open. For a garden with many objects/plantings, the copy becomes N+M sequential round-trips held under one write lock — the copy's own latency scales linearly with garden size, and it serializes all other users' writes for that whole window (a concurrent write from an unrelated user can block or fail withSQLITE_BUSY). A multi-rowINSERT ... VALUES (...),(...),... RETURNING ...(or chunked batching) would cut the round-trip count from O(n) to O(1) for both phases. Severity is moderate since it requires an unusually large garden to bite in practice.🧯 Error handling & edge cases — Minor issues
This confirms the store/service code matches the draft's description exactly — transaction handling, error wrapping,
sql.ErrNoRowsmapping, andcopyName's rune-safe truncation are all as claimed. The only actionable finding is theContentLengthcheck ingardens.go:120, which I independently verified: Go's server setsContentLength == -1(not0) for a request withTransfer-Encoding: chunkedand noContent-Lengthheader, soc.Request.ContentLength != 0is true for an unknown-length body, sending it intoShouldBindJSONwhere an empty chunked body yieldsio.EOF, incorrectly rejected as400. The sibling handlercreateShareLink(public.go:56) already uses the correcterr != nil && !errors.Is(err, io.EOF)pattern, confirming this is a real regression/inconsistency, not a false positive.VERDICT: Minor issues
internal/api/gardens.go:120—copyGardengates JSON body parsing onc.Request.ContentLength != 0, but Go setsContentLengthto-1(not0) when the body length is unknown (e.g.Transfer-Encoding: chunkedwith no body). In that case the handler callsShouldBindJSONon an effectively empty body, which fails withio.EOF, and gets turned into a400 INVALID_INPUT "name must be a string"— incorrectly rejecting a legitimate no-body copy request. The sibling handlercreateShareLink(internal/api/public.go:56) already implements this correctly:if err := c.ShouldBindJSON(&req); err != nil && !errors.Is(err, io.EOF) { ... }, which is agnostic to how the empty body was framed.copyGardenreinvents the optional-body check with a strictly weaker condition. The existing test (gardens_test.go) doesn't catch this becausehttptest.NewRequestalways setsContentLengthto the exact buffer length (0 for a nil/empty body), so the chunked/-1path is untested.public.gopattern — always callShouldBindJSONand ignore onlyio.EOF, rather than gating onContentLength.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Through the security lens, I verified the copy feature's authorization, capability-leakage, input-validation, and injection surfaces against the actual code:
Authorization (authz):
CopyGardencallss.requireGardenRole(ctx, actorID, gardenID, roleOwner)(internal/service/gardens.go:171), so strangers getErrNotFound(→ 404) and non-owner editors getErrForbidden(→ 403), matching the stated owner-only policy. The route is registered undergardens := v1.Group("/gardens", h.requireAuth())(internal/api/api.go:78), so unauthenticated requests are rejected before the handler runs.Actor identity is not user-controllable:
mustActor(c).IDis derived from the session (internal/api/auth.go:209viaactorKey), not from the request body or query; the new garden'sowner_idis set to this trustedactorIDin the store (internal/store/gardens.go:191,ownerIDparam). No IDOR path.Capability leakage —
public_token: The garden-copyINSERT … SELECTexplicitly enumerates columns (owner_id, name, width_cm, height_cm, unit_pref, notes, grid_size_cm, snap_to_grid) and does not includepublic_token(internal/store/gardens.go:192-197). Sincepublic_tokendefaults to NULL (migration 0004), the copy starts with no public link. Confirmed it cannot silently inherit a live capability.Capability leakage —
garden_shares: The store copy transaction never inserts intogarden_shares, so the copy is private to the copier. Confirmed.Removed-planting history: The planting scan filters
pl.removed_at IS NULL(internal/store/gardens.go:225), so soft-removed plops aren't carried over — no history/capability bleed.Input validation: The name is
TrimSpace'd, blank-derived, and length-capped atmaxGardenNameLen(200) withErrInvalidInputon overflow (internal/service/gardens.go:175-180);copyNametruncates on a rune boundary viaToValidUTF8to keep results valid UTF-8 (internal/service/gardens.go:193-198). The handler rejects non-stringnamewith 400 (internal/api/gardens.go:118). No unbounded/unvalidated string reaches storage.Injection: All SQL uses parameterized placeholders (
?) for every user- or source-derived value across the garden, object, and planting inserts (internal/store/gardens.go:192-237); no string interpolation of untrusted input into a query. The column-list constants (gardenColumns,objectColumns) are compile-time string literals, not user input.No security issues found within this lens.
🎯 Correctness — No material issues found
Verdict: No material issues found
I reviewed the change through the correctness lens, verifying the schema, column lists, truncation math, and transaction flow against the checked-out code.
copyNametruncation math (internal/service/gardens.go:193):room = maxGardenNameLen(200) - len(" (copy)")(7) = 193. Whenlen(src) > 193, it slicessrc[:193], runsToValidUTF8(..., "")(empty replacement → only shrinks, never grows, and repairs any rune split at the byte boundary) thenTrimSpace(only shrinks). Result + suffix ≤ 193 + 7 = 200 = cap. Verified againstgardenFromInput'slen(name) > maxGardenNameLenbyte-length check — units are consistent (bytes, not runes). TheTestCopyGardenNameRuneBoundarycase (100 × "é" = 200 bytes) truncates to 193 bytes = 96 complete é + a dangling lead byte thatToValidUTF8drops → 192 + 7 = 199 ≤ 200, valid UTF-8. Correct.Garden row copy (
internal/store/gardens.go:189):INSERT ... SELECTcopieswidth_cm, height_cm, unit_pref, notes, grid_size_cm, snap_to_gridand suppliesowner_id, name;version,created_at,updated_at, andpublic_tokenfall back to defaults (so no inherited share link, fresh version=1). Verified againstmigrations/0004(public_token is a nullable added column, not ingardenColumns) and0001_init(version default 1). Correct.Object copy (
internal/store/gardens.go:217): 17 insert columns match the 17 placeholders and matchCreateObject's column list atobjects.go:38;version/timestamps default.z_indexis copied per-row so theORDER BY idscan (vs. the live list'sz_index, id) doesn't affect correctness. Correct.Planting copy (
internal/store/gardens.go:247): query joins source objects (o.garden_id = srcID) and filtersremoved_at IS NULL, so only active plops are read and they reference source object IDs; theobjectIDsmap re-parents each onto the copied object.removed_atis intentionally not inserted (defaults NULL = active).count/label/planted_at(all nullable) are passed through. Correct.TOCTOU on source existence:
requireGardenRolereads the garden, thenstore.CopyGardenre-checks viaINSERT ... SELECT ... WHERE id = ?; a vanished source yieldssql.ErrNoRows→domain.ErrNotFound. Correct.queryerinterface + cursor draining:*sql.Txandd.sqlboth satisfyqueryer;queryObjects/queryPlantingsfully drain anddefer rows.Close()before returning, so the subsequent writes on the same tx connection are safe. Verified atobjects.go:78andplantings.go:44. Correct.ListActivePlantingsForObjectrefactor (plantings.go:68): now routes throughqueryPlantingswith unqualifiedplantingColumns;scanPlantingscans by position so the unqualified select still lines up. Correct.No correctness defects found in this lens.
🧹 Code cleanliness & maintainability — Minor issues
All three findings are confirmed against the actual code:
gardens.go:217 — The
INSERT INTO garden_objectscolumn list at gardens.go:217-221 is verbatim-duplicated fromCreateObjectat objects.go:38-42. The plantings INSERT column list at gardens.go:247-248 is duplicated from bothCreatePlanting(plantings.go:112-114) andCreatePlantings(plantings.go:135-137) — three places total. Confirmed.users.go:22 — The
queryerinterface is declared at users.go:22-26 and is a generic store-wide abstraction. (Note:scanneralso lives in users.go:18, so the draft's "or the file holding scanner" phrasing is slightly off, but the core point —queryeris a shared helper misplaced in a user-specific file — holds.) Confirmed as trivial.gardens_test.go:364 —
keptis assigned at line 348 and only referenced via_ = keptat line 364. Confirmed.Verdict: Minor issues
internal/store/gardens.go:217andinternal/store/plantings.go:112/:135— theINSERT INTO garden_objects …andINSERT INTO plantings …column lists are duplicated verbatim betweenCreateObject/CreatePlantingand the in-transaction copy inCopyGarden. The copy can't callCreateObject/CreatePlantingdirectly because those bind tod.sqlrather thantx, so the duplication is a deliberate tradeoff, but it means a future schema/column change must be edited in two (objects) / three (plantings) places. A small sharedinsertObjectSQL/insertPlantingSQLconstant for the(cols) VALUES (?,?,…) RETURNING …text would keep them in sync at zero behavioral cost. Verified by reading both files.internal/store/users.go:22— the newqueryerinterface is a generic store-wide abstraction but is declared inusers.go, which is otherwise about user rows. For a shared store helper this is a mildly leaky/misplaced home; astore.gowould be the natural spot and would make discovery easier. Verified by reading the file.internal/service/gardens_test.go:364—keptis assigned fromCreatePlantingand then only referenced via_ = keptto silence the unused-variable check. Just dropping the assignment (_, err := s.CreatePlanting(...)) or naming it_would be cleaner than the throwaway_ = keptline. Verified by reading the test.No dead code, no confusing names, no over-long functions, and the new code follows the surrounding patterns (error wrapping prefixes,
defer tx.Rollback(),boolToInt, column constants) consistently.⚡ Performance — No material issues found
No material issues found
I verified the store implementation (
internal/store/gardens.go:179-259) and the refactored query helpers (internal/store/objects.go,internal/store/plantings.go).Through the performance lens:
INSERT ... RETURNINGfor objects and M individualINSERTs for plantings (gardens.go:215-253). This is N+M round-trips rather than a single batched copy, but it's an owner-initiated, one-shot operation (not a hot path), and the per-rowRETURNINGis genuinely needed to build the source→destobjectIDsmap for planting re-parenting. SQLite offers no clean batched-RETURNING-with-remapping alternative here, so this is inherent, not a regression.queryObjects/queryPlantingsfully materialize the source rows into slices before any write (gardens.go:206-237). That's required by the SQLite single-connection cursor constraint the PR calls out, and themake(map[int64]int64, len(srcObjects))pre-sizing avoids reallocation churn. Memory is O(objects + plantings), unbounded in principle, but bounded in practice by a single garden's contents on a user-triggered action.No hot-path regressions, no N+1 on a list/read path, no missing pagination on a frequently-hit endpoint. Nothing material to flag in this lens.
🧯 Error handling & edge cases — Minor issues
The finding's reasoning about Go's
net/httpsemantics is correct:Request.ContentLengthis-1for chunked transfers, so!= 0is true, andShouldBindJSONon an empty body returnsio.EOF→ the 400 path. The code at line 120 matches the draft's description. This is a legitimate, low-severity edge case.VERDICT: Minor issues found
internal/api/gardens.go:120— The "body absent" detection usesc.Request.ContentLength != 0. For a chunked-transfer POST (Transfer-Encoding: chunked, noContent-Length), Go setsRequest.ContentLength = -1, so!= 0is true and the handler falls intoShouldBindJSONon an empty body, which returnsio.EOF→ a400 "name must be a string". A genuinely empty chunked body should behave like "no body" (derive the default name), but instead it's rejected. Low-severity edge case (chunked empty POSTs are unusual, and rejecting one isn't dangerous), but it is an inconsistent unhappy path. The cleanest fix is to attempt the decode only when a body is actually present (e.g. peekc.Request.Body), or gate onContentLength > 0so-1falls through to the default-name path (noting that would silently swallow a malformed chunked body).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
From the 🔒 Security lens, the change is well-constructed:
POST /gardens/:id/copyroute sits behind the existingrequireAuth()middleware. The service layer enforces owner-only access viarequireGardenRole(..., roleOwner)— strangers receiveErrNotFound(information hiding) and editors receiveErrForbidden, matching the PR's deliberate access policy.?placeholders. The column-list constants (gardenColumns,objectColumns,plantingColumns) are hard-coded, never derived from user input.*sql.Tx. Reads of source objects and plantings are drained before writes, respecting SQLite's same-connection cursor limitation, so the transaction snapshot stays consistent.public_tokenandgarden_sharesare intentionally not copied, and the store'sINSERT ... SELECTonly copies the explicitly listed non-sensitive metadata columns. The resulting garden starts private.parseIDParamhelper rejects non-numeric and non-positive IDs with 400. The name is trimmed and capped atmaxGardenNameLenbytes;copyNameusesstrings.ToValidUTF8to avoid splitting multi-byte runes when trimming for the suffix.🎯 Correctness — No material issues found
No material issues found
The implementation is correct through the correctness lens. I verified:
CopyGardenstore method usesBeginTxwithdefer tx.Rollback()and a finaltx.Commit(), ensuring no half-populated garden survives a failure.queryObjectsandqueryPlantingsboth fully consume and close their*sql.Rowsviadefer rows.Close()before returning, satisfying the SQLite constraint that you cannot hold a read cursor open while writing on the same connection within a transaction.copyNameusesstrings.ToValidUTF8after byte slicing and before appending the suffix, guaranteeing valid UTF-8. The testTestCopyGardenNameRuneBoundaryconfirms this.copyNamealways produceslen(result) <= maxGardenNameLenbecause it only truncates whenlen(src) > (maxGardenNameLen - len(suffix)), and the final result is at mostroom + len(suffix) == maxGardenNameLen.version, so the schema defaults (DEFAULT 1) give the copy fresh version 1 values, matching the test expectation.public_tokenandgarden_sharesare deliberately omitted from copying;version,created_at,updated_atare reset via defaults; only active plantings (removed_at IS NULL) are copied.requireGardenRole(ctx, actorID, gardenID, roleOwner), correctly restricting copying to owners and returningErrForbiddenfor editors andErrNotFoundfor strangers, as verified by tests.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/components/gardens/CopyGardenModal.tsx:20— The modal pre-derives the copy name with`${garden.name} (copy)`, duplicating the server'scopyName()logic but without the byte-cap truncation. If the source name is long, the prefilled default can exceedmaxGardenNameLenand will be rejected by the service when submitted unchanged. The client and server derivations are out of sync. Fix: either share the truncation logic (e.g., a common util) or prefill with a name capped the same way the server does.⚡ Performance — No material issues found
No material issues found
Reviewed through the Performance lens. The deep-copy implementation in
internal/store/gardens.godoes materialize all source objects and plantings in memory before writing, and performs individualINSERTstatements per object and per planting inside a single transaction. While this is technically an N+1 insert pattern, it is bounded by garden size (user-managed, typically small), runs on an explicit user action (not a hot path), and correctly drains cursors before writing to respect SQLite's same-connection limitations. No hot-loop allocations, unbounded growth, missing limits, or blocking call regressions were identified.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/components/gardens/CopyGardenModal.tsx:20— The modal prefills the copy name as`${garden.name} (copy)`without applying the server's 200-byte name-length cap (maxGardenNameLen). If the source garden name is 194–200 bytes, the derived name exceeds the limit and the server rejects it withErrInvalidInput(400). The user opens the modal to a default value that is guaranteed to fail.Fix: Reuse the backend truncation logic in the frontend (cap source name to make room for the 7-byte
" (copy)"suffix, trimming on a valid UTF-8 boundary), or derive the default name from the API rather than precomputing it.opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
I reviewed this PR strictly through the security lens (authn/authz, injection, secret leakage, input validation, unsafe untrusted data). I verified each concern against the checked-out code, not just the diff:
CopyGardencallss.requireGardenRole(ctx, actorID, gardenID, roleOwner)(internal/service/gardens.go:171), andrequireGardenRolereturnsErrNotFoundfor users with no role andErrForbiddenfor insufficient role (internal/service/gardens.go:69-86). Strangers don't even learn the garden exists; the test confirms stranger→404 and editor→403. ✓POST /gardens/:id/copyis registered inside thegardensgroup underh.requireAuth()(internal/api/api.go:78,84), so anonymous → 401, and POST is covered bycsrfGuard(internal/api/api.go:50). ✓public_tokeninheritance — TheINSERT INTO gardensatinternal/store/gardens.go:190-193lists columns explicitly and omitspublic_token; the column defaults to NULL (internal/store/migrations/0004_garden_public_token.sql:5).gardenColumnsalso excludes it, so it can't leak via the API response. The copy starts with no live share URL. ✓garden_sharesinheritance — Onlygardens,garden_objects, and activeplantingsare inserted; no shares rows. ✓plant_idreuse is owner-scoped — The PR's stated reason for owner-only is that copied plops keep the source'splant_ids. Since authorization is enforced as owner-only, the actor copying is the owner of those plants, so this doesn't hand a stranger a reference to someone else's catalog. TheON DELETE RESTRICTconsequence is a correctness/feature-scoping concern (acknowledged in the design), not a security bypass. ✓?placeholders for user-supplied values (name,srcID,ownerID). The string-concatenated bits (gardenColumns,objectColumns,plantingColumns,qualifyColumns(...)) are compile-time constants, not request input. ✓parseIDParamrejects non-numeric ids → 400 (internal/api/errors.go:77); the optional body is only decoded whenContentLength != 0, andShouldBindJSONrejects a non-stringname(e.g.{"name":42}) → 400, as tested. Over-long names →ErrInvalidInput→ 400 (internal/service/gardens.go:179-181). TrustingContentLengthhere is a minor optimization, not a vulnerability — an attacker can't exploit it to bypass validation. ✓owner_idin the copy is supplied as themustActor(c).IDfrom the session, not from the request body;created.MyRoleis set server-side toroleOwner(internal/service/gardens.go:186). The request body only carriesname. ✓CopyGardenModal.tsxrendersgarden.nameandcreated.nameas React text content (nodangerouslySetInnerHTML), so the name is escaped. ✓Nothing in my lens turned up a material issue.
🎯 Correctness — No material issues found
I'll verify the key claims in the draft review against the actual code.
The code matches the draft's claims. Let me verify the supporting functions referenced (
requireGardenRole,boolToInt,qualifyColumns, cursor closing).All supporting functions exist. The draft's verification is accurate. The draft found no material issues and verified each claim against actual code — which I've now independently confirmed. The
gadfly-findingsarray is empty, matching the draft.No material issues found.
copyNametruncation math (service/gardens.go:193-199): confirmedmaxGardenNameLen=200, suffix 7 bytes, room=193. Boundary cases hold; multi-byte rune split repaired byToValidUTF8(..., "")(empty replacement, only shrinks) thenTrimSpace. Cannot exceed 200 bytes.INSERT ... SELECTgarden copy (store/gardens.go:190-194): copieswidth_cm, height_cm, unit_pref, notes, grid_size_cm, snap_to_grid; omitspublic_token(NULL default),version(1), timestamps (now). Owner/name supplied. Correct.objectColumns;boolToIntapplied to the two INTEGER boolean columns;z_indexpreserved.boolToIntconfirmed defined at store/users.go:173.object_id(remapped viaobjectIDs),plant_id, x_cm, y_cm, radius_cm, count, label, planted_at; omitsremoved_at(NULL → active, consistent with theremoved_at IS NULLfilter),version(1), timestamps. The "unreachable" missing-object branch is genuinely unreachable given the join.requireGardenRole(..., roleOwner)confirmed at service/gardens.go:69; masks non-owners asErrNotFoundand insufficient-role asErrForbidden, matching test expectations.ContentLength != 0skips binding for empty bodies while still binding chunked (ContentLength == -1) bodies; wrong-typedname→ 400.defer tx.Rollback()+ explicittx.Commit(); all writes viatx.queryObjects/queryPlantingsconfirmed defined at store/objects.go:78 and store/plantings.go:44 (drain+close before subsequent writes), respecting SQLite's no-concurrent-cursor constraint.qualifyColumnsconfirmed at store/sqlite.go:105.🧹 Code cleanliness & maintainability — Minor issues
I'll verify each finding against the actual code.
All three findings verified. Now let me check the
useCopyGardenhook to confirm finding #1's claim about the UI always sending{ name: name ?? '' }.Confirmed:
api.postwith{ name: name ?? '' }always sends a body, so the API'sContentLength != 0branch is always hit for the UI client. The component passesname.trim()which is non-empty (button disabled if empty), but the body is sent regardless. Finding #1 verified.All three findings are confirmed against the code. Let me also verify the test naming concern (finding #3) —
keptis indeed soft-removed byClearObject(line 357) and never asserted.Verified. The three findings survive:
internal/api/gardens.go:120— UI always sends{ name: name ?? '' }viauseCopyGarden, so theContentLength != 0branch is dead for the web client (only the curl/test path exercises the no-body branch).web/src/components/gardens/CopyGardenModal.tsx:20—" (copy)"suffix hardcoded client-side;copyNameininternal/service/gardens.godefines it server-side. Duplication confirmed.internal/service/gardens_test.go:364—_ = keptdiscards the variable;keptwas created at line 350, thenClearObject(line 357) soft-removes all plantings includingkept, andkeptis never used in any assertion.Minor issues
internal/api/gardens.go:120— thec.Request.ContentLength != 0branch advertises an optional-body contract, but the only real caller (useCopyGardeninweb/src/lib/gardens.ts:98) always posts{ name: name ?? '' }, so the no-body path is only exercised by the curl/test caller. The API surface and the UI client disagree on which contract is authoritative; a future reader will wonder which is canonical. Cosmetic leaky-abstraction smell, not blocking.web/src/components/gardens/CopyGardenModal.tsx:20—useState(\${garden.name} (copy)`)reconstructs the server'scopyNamederivation client-side (the" (copy)"suffix lives ininternal/service/gardens.go`). The component comment claims the prefill is "the server's own default so what you see is what you get," but no server call fetches it — the suffix is duplicated. If the server's suffix or truncation rule changes, the UI's prefill silently drifts; the test suite asserts the suffix as a contract, so the duplication is a latent maintainability trap.internal/service/gardens_test.go:364—_ = keptdiscards an assigned-but-unused variable.keptis created at line 350, then soft-removed byClearObjectat line 357, and never asserted. The namekeptis misleading (it's actually the cleared plop) and the discard is cosmetic noise. Rename to reflect its fate or drop the assignment and use_at creation.⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens:
internal/store/gardens.go:215-253). This is linear, not quadratic, and it's a user-initiated one-shot operation rather than a hot path, so the per-row INSERT overhead is acceptable. A bulk/INSERT ... SELECTrewrite could reduce round-trips but isn't material here.queryObjects/queryPlantingsfully drain their cursors before the transaction issues writes (internal/store/objects.go:78-97,internal/store/plantings.go:44-63), correctly avoiding the SQLite "open cursor + write on same connection" trap. The reads happen once each, before the insert loops — no cursor-held-during-write regression.requireGardenRolereads the source garden row, then the store'sINSERT ... SELECTreads it again (internal/service/gardens.go:171,internal/store/gardens.go:189-195). One extra single-row read on a non-hot path; not worth changing.srcObjects,srcPlantings) and theobjectIDsmap scale with garden size. Bounded by a single garden's contents on a copy action; no unbounded/paginated-growth concern.I verified these by reading
internal/store/gardens.go,internal/store/objects.go,internal/store/plantings.go, andinternal/service/gardens.goat the checked-out state.🧯 Error handling & edge cases — No material issues found
Now let me look at the
copyNamerune-boundary truncation carefully. The truncation usessrc[:room]which slices by bytes, thenToValidUTF8cleans up. But it trims toroombytes, not on a rune boundary. Let me check the test case.Let me check the UI error handling for the modal and the API.
Now let me re-examine
copyNamemore carefully.maxGardenNameLen = 200,suffix = " (copy)"= 7 bytes,room = 193. Iflen(src) > 193, truncate tosrc[:193](byte slice, possibly splitting a rune), thenToValidUTF8replaces invalid sequences with empty string. ButTrimSpaceis applied AFTERToValidUTF8— wait, no:strings.TrimSpace(strings.ToValidUTF8(src[:room], "")). So ToValidUTF8 cleans partial rune, then TrimSpace trims whitespace. Thensrc + suffix. Length check: result ≤room + len(suffix)= 200. OK.But there's a subtle issue: if
src[:room]ends mid-rune AND the rune before is a valid character, ToValidUTF8 drops the partial bytes. Fine. If room falls such that the last byte is part of a multi-byte rune boundary that includes whitespace... edge cases are handled. Tests pass. The truncation logic seems correct.Let me check the
queryPlantingsreuse — the diff modifiedListActivePlantingsForObjectto usequeryPlantings, but the original usedplantingColumnsunqualified. Let me verify the new version still works.Now let me check whether
requireGardenRolereturns a garden withMyRolepopulated, and whethersrc.Nameis reliable. Also let me verify theCopyGardenstore function doesn't check thatownerIDactually exists (FK violation edge case).Now let me check whether the
INSERT ... SELECTfrom the store can returnsql.ErrNoRowsproperly and whether thescanGardenhandles thesql.ErrNoRowscorrectly when the source is gone.Now let me check the UI mutation — what does
gardenSchema.parsedo, and is the returned object the right shape (does the copy endpoint return a full Garden)? Also check thatcreated.idexists in the schema.Now let me check the
ContentLength != 0check in the API handler — what about a request withContent-Length: 0but a body, or chunked encoding (no Content-Length)?Now let me check if there are any unique constraints that could fail on copy (e.g., garden name uniqueness, or public_token uniqueness — but public_token is not copied).
Now let me verify the
Countfield handling in copy. TheplantingsINSERT copiescountasp.Count(a*int). IfCountis NULL in DB, the scan reads it as nil, and the INSERT inserts nil. That works.Let me look at how
PlantedAtis handled — it's a*string. In the copy,p.PlantedAtis passed directly. If NULL, nil passed to INSERT — fine.Now, a key edge case: the copy INSERT for plantings does NOT copy
removed_at,version,created_at,updated_at. The defaults handle those. Good — removed_at defaults NULL (active), version defaults 1, timestamps default now. That's correct for a fresh layout.Let me check whether there's an issue with the
Labelfield — it's*string, nullable. Same handling. Fine.Now let me check the
pointscolumn in objects — it's*string, nullable. The copy passeso.Pointsdirectly. Fine.Let me verify the
ColorandPropsnullable handling in the object copy. Looking at the INSERT:o.Color, o.Propsare*stringpassed directly. Fine.Now, one important edge case in the store CopyGarden: what if
srcID == ownerID... no, srcID is gardenID, ownerID is userID. Different.Let me check: is there any validation that
ownerID(the actor) still exists when the copy is made? The actor is authenticated, so they exist. The INSERT into gardens hasowner_id REFERENCES users(id), so FK is satisfied. Fine.Now, the critical edge case I want to verify: the
ContentLength != 0check. With chunked transfer encoding,ContentLengthis -1, not 0. So a chunked POST with an actual body would skip decoding, leavingreq.Nameempty, and the service derives the default name. That's a silent behavior difference but not an error handling issue per se — the body would just be ignored. Not a crash, not a swallowed error. The browser always sends Content-Length for POST with body. Minor.Let me check: what if
ShouldBindJSONsucceeds but the JSON has extra fields ornameis null?nullfor a string field → Go leaves it as ""? Actuallyjson.Unmarshalofnullinto a string leaves it unchanged (empty string default). So{"name": null}→req.Name = ""→ derived name. Fine.Let me verify the
writeServiceErrormapping forErrInvalidInput→ 400.Now let me double-check the
copyNametruncation edge case where the source name has trailing whitespace. Supposesrc.Name(after the source was created) has trailing whitespace that was trimmed at creation. ActuallyCreateGardentrims the name. Butsrchere comes fromrequireGardenRole→GetGarden, which reads the stored (already-trimmed) name. Sosrc.Nameis already trimmed. ThecopyNamefunction trims again — harmless.But there's a subtle issue: in
copyName, iflen(src) > room, it doesstrings.TrimSpace(strings.ToValidUTF8(src[:room], "")). TheTrimSpacecould shrink the result further, makinglen(src) < room, thensrc + suffix< maxGardenNameLen. Fine — under cap. But what if after trimming,srcbecomes empty? Then result is just" (copy)"= 7 bytes. That's a valid name. Edge case but handled.Now, is there a scenario where
copyNamereturns a name that's empty after... no, suffix is always appended. Fine.Let me check one more thing: the
ToValidUTF8is applied tosrc[:room]which is a byte slice. Ifsrccontains invalid UTF-8 (shouldn't, since it came from DB which stored valid UTF-8), but defensive. Fine.Let me verify the
queryObjects/queryPlantingsdefer rows.Close() — they havedefer rows.Close(). Good, no cursor leak.Now let me check the
tx.QueryRowContextfor the object INSERT inside the loop — if it returns an error, we return and the deferredtx.Rollback()runs. Good. Thedefer rows.Close()in queryObjects runs. Good.Let me check: in the store CopyGarden, after
queryObjectsdrains the cursor and returns, we then do INSERTs in a loop ontx. Since the cursor is closed, no conflict. Good.Now, the one thing I want to verify: does
tx.QueryRowContextfor the INSERTs conflict with the still-open cursor fromqueryPlantings? No —queryPlantingsdrains and closes before returning. So when we dotx.ExecContextfor planting inserts, the cursor is closed. Good.Let me verify there's no issue with
scanGardenbeing called withtx.QueryRowContext—*sql.Tx.QueryRowContextreturns*sql.Rowwhich implementsscanner(hasScan). Good.Good.
*sql.Tximplements bothQueryContext(satisfiesqueryer) andQueryRowContextreturns*sql.Rowwhich satisfiesscanner. Everything compiles.Now let me consider the error-handling lens more carefully. The
defer tx.Rollback()witherrchecknolint — this is the standard pattern. After commit, rollback returnssql.ErrTxDonewhich is ignored. Fine. On the error paths before commit, rollback runs and cleans up. Good.One concern: in the store
CopyGarden, whenqueryObjectsorqueryPlantingsreturns an error, we returnerrdirectly (not wrapped). ThequeryObjects/queryPlantingsalready wrap with "store: list objects/plantings". But in the copy context, "list objects" is slightly misleading (it's a copy operation). Minor, not an error-handling bug.Let me check one potential real issue: the
INSERT ... SELECT ... FROM gardens WHERE id = ?with RETURNING. Does SQLite supportINSERT ... SELECT ... RETURNING? Modern SQLite (3.35+) does. The project uses it elsewhere (CreateGarden uses RETURNING). So fine.But a subtle edge: if
srcIDdoesn't exist, the SELECT yields 0 rows, INSERT inserts nothing, andRETURNINGyields no rows.tx.QueryRowContextthen returns*sql.Rowwhose.Scan()returnssql.ErrNoRows. The code checkserrors.Is(err, sql.ErrNoRows)→domain.ErrNotFound. Good, verified at line 196.Now, let me check a potential issue: the transaction isolation. We read the source garden, then read its objects, then read its plantings — all within the tx. If another transaction modifies the source concurrently, we get a consistent snapshot (SQLite serializable). Good.
But wait — the
requireGardenRolecall in the SERVICE happens OUTSIDE the store transaction (it's a separateGetGardencall). So there's a TOCTOU window: the garden could be deleted between the role check and the store copy. But if deleted, theINSERT ... SELECTyields no rows →ErrNotFound. So the store handles it. Good — no panic, clean error.Let me verify there's no nil-pointer issue:
srcfromrequireGardenRoleis used forsrc.Name. IfrequireGardenRolereturns nil garden with nil error? No — it returnsg, nilonly with a valid garden. Good.Now let me check the
createdreturn from store — ifscanGardensucceeds,createdis non-nil. Then service setscreated.MyRole. No nil deref. Good.Let me check the test file for the
seedBed,seedOwnPlant,seedGarden,seedUserhelpers to make sure they exist.All helpers exist. Now let me do a final check on the
copyNamefunction for a tricky edge: what ifmaxGardenNameLen - len(suffix)could be negative?maxGardenNameLen = 200,len(suffix) = 7,room = 193. Always positive. No overflow.What if
len(src)is huge?src[:room]is a valid slice (room ≤ len(src) since we're in thelen(src) > roombranch). Fine.I've thoroughly examined the error-handling and edge cases. Let me look at one last thing — the
Countfield in the planting copy. Let me verify the INSERT column list matches the values. The INSERT:(object_id, plant_id, x_cm, y_cm, radius_cm, count, label, planted_at)— 8 columns, 8 placeholders?, ?, ?, ?, ?, ?, ?, ?. Values:objectID, p.PlantID, p.XCM, p.YCM, p.RadiusCM, p.Count, p.Label, p.PlantedAt— 8 values. Matches. Good.The copy does NOT carry
removed_at, so the new planting is active (NULL). Good. It does NOT carryversion(defaults to 1),created_at/updated_at(default to now). Good — fresh row.I'm confident the error handling and edge cases are well-covered. Let me verify one potential concern: the UI modal's
onCloseis called BEFOREnavigate. IfonCloseunmounts the modal/component, doesnavigatestill work?onClosesetsdialogto null in the parent, which unmountsCopyGardenModal. Butnavigateis called from within theonSubmitasync function which already holds thenavigatereference. TheuseNavigatehook returns a stable function. Calling it after unmount is fine (TanStack Router's navigate is not tied to component lifecycle). Andtoast.infoalso works after unmount. This is a common pattern. No error-handling issue.Actually, wait —
onClose()is called, thennavigate(). IfonClosetriggers a state update that causes re-render, the current execution context (the async function) continues fine. No issue.Let me check: is there a race where
copy.isPendingstays true and the user can't interact? No —mutateAsyncresolves,isPendingbecomes false. The error path setserrorand the user can retry. Good error recovery.One more: the modal's submit button is
disabled={copy.isPending || name.trim() === ''}. If the user submits a whitespace-only name, the button is disabled — but they could still press Enter in the TextField (form submit). TheonSubmitdoesname.trim()and sends it. If empty after trim, the server derives the default name. Wait —name.trim() === ''disables the button, so the form can't be submitted via button click. But Enter key in a text field submits the form. The button isdisabled, so Enter won't trigger it (disabled submit button blocks form submission). Actually, in HTML, if the submit button is disabled, Enter in a text input may still submit the form in some browsers. ButonSubmithandles it:name.trim()→ empty string sent → server derives default. No error. Fine.I've completed my review through the error-handling/edge-cases lens. Everything is robust:
No material error-handling or edge-case issues found.
Verdict: No material issues found
Reviewed the deep-copy change end-to-end through the error-handling & edge-cases lens. The unhappy paths are well-covered:
CopyGardentransaction (internal/store/gardens.go:179-259):defer tx.Rollback()runs on every error path and is a no-op after commit; partial failures leave no half-populated garden. Verified theINSERT ... SELECT ... RETURNINGreturnssql.ErrNoRowswhen the source is gone, mapped todomain.ErrNotFound(line 196-197) — so the TOCTOU window between the service-levelrequireGardenRoleread and the store write degrades cleanly rather than panicking.internal/store/objects.go:78-97,plantings.go:44-63): bothqueryObjects/queryPlantingsdefer rows.Close()and fully drain before returning, so the subsequenttx.ExecContext/tx.QueryRowContextwrites don't hit SQLite's "cursor open while writing on the same connection" limit. Verified the in-memory single-connection rationale in the comments.internal/service/gardens.go:193-199):copyNametruncatessrc[:room](byte slice) thenstrings.ToValidUTF8(..., "")repairs any split rune, thenTrimSpace; result is always ≤room + len(suffix)=maxGardenNameLen. Confirmedroom = 200 - 7 = 193 > 0, no underflow. The dedicatedTestCopyGardenNameRuneBoundaryexercises the multi-byte case and assertsutf8.ValidString+ length cap.ErrInvalidInput(line 179); API handler treatsContentLength != 0as "a body was sent" and rejects malformed JSON as 400.{"name": null}decodes to""→ derivation, not a crash.internal/store/gardens.go:246-252): 8 columns / 8 placeholders / 8 values match;count/label/planted_atnullable*fields pass through as nil for NULL source values;removed_at/version/timestamps deliberately omitted so defaults yield a fresh active row.objectIDsmap miss returns a wrapped error rather than proceeding with a zeroobjectID— defensive against a logic break.CopyGardenModal.tsx):mutateAsyncerrors are caught and surfaced viaAlert,copy.isPendingdisables actions during flight, andname.trim() === ''blocks submission. Thecreatedreturn is zod-parsed before use.opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found through the security lens.
CopyGardencallsrequireGardenRole(..., roleOwner)atinternal/service/gardens.go:171, so only owners can copy. Confirmed by service tests: strangers getErrNotFound, editors getErrForbidden. The UI correctly only shows the Copy button for owners.INSERT ... SELECTthat omitspublic_token;GetPublicShareLinkon the copy returns empty token, andPublicGardenwith the source token still resolves to the source (tested in service tests).garden_sharesrows are not copied; confirmed by service tests.maxGardenNameLenare rejected withErrInvalidInput; derived names are truncated safely withToValidUTF8.🎯 Correctness — No material issues found
No material issues found. All logic paths, formulas, and edge cases were verified and are correct.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/api/gardens.go:120— ThecopyGardenhandler checksc.Request.ContentLength != 0to decide whether to parse an optional JSON body, but the codebase already has an established pattern for this ininternal/api/public.go:56: tryc.ShouldBindJSON(&req)and ignoreio.EOF. TheContentLengthguard is both inconsistent with surrounding code and less robust (e.g., chunked encoding or HTTP/2 can leaveContentLength == -1even with no body, which would cause the handler to incorrectly reject a valid no-body request asINVALID_INPUT). Fix: Remove theContentLengthguard and useif err := c.ShouldBindJSON(&req); err != nil && !errors.Is(err, io.EOF) { … }, adding"io"to the imports.internal/service/gardens_test.go:364(inTestCopyGardenSkipsRemovedPlantings) — The test assigns a planting tokeptonly to suppress it with_ = kept. Using_, err := s.CreatePlanting(...)on that line avoids the unused-variable workaround.⚡ Performance — No material issues found
No material issues found.
The copy implementation is structurally sound from a performance standpoint:
CopyGardenwraps everything in oneBEGIN...COMMITblock, avoiding partial-state overhead and extra round-trips.JOIN garden_objectsto scope by garden). Reparenting is done in-memory via theobjectIDsmap.objectIDsmap is pre-sized (make(..., len(srcObjects))). Result slices are drained and closed before writes, which is a deliberate SQLite-safe pattern, not an allocation leak.srcObjectsandsrcPlantingsare linear and triggered by an explicit user action (POST /copy), not a background or high-frequency path.maxGardenNameLenprevents unbounded input.🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
I investigated the copy-garden flow through the error-handling & edge-cases lens and found the unhappy paths are properly covered:
CopyGardenininternal/store/gardens.go:179opens a transaction, hasdefer tx.Rollback(), and only commits after all inserts succeed. Any failure mid-way rolls back, leaving no half-copied garden.CopyGardenininternal/service/gardens.go:175trims the name and falls back tocopyName(src.Name)when blank, matching the documented behavior.ErrInvalidInputrather than silently truncated (internal/service/gardens.go:179).copyName(internal/service/gardens.go:193) usesstrings.ToValidUTF8on the sliced prefix so multi-byte runes are never split.objectIDslookup ininternal/store/gardens.go:241is defensive: if an active planting somehow references an object that wasn’t copied, it returns an explicit error and the deferred rollback cleans up.internal/api/gardens.go:120) only binds JSON whenContentLength != 0, so a no-body POST correctly triggers the default-name path.queryObjectsandqueryPlantingsreturn non-nil empty slices; the loops simply don’t execute and the empty garden commits successfully.No ignored errors, no missing cleanup, no unhandled nil/empty/zero cases, and no boundary issues were found in this change.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
* api: detect an absent copy body by io.EOF from the decoder rather than Content-Length. A chunked request reports -1, not 0, so the length test read an empty chunked body as malformed and 400'd instead of taking the default-name path. Covered by a new unknown-length test. * store: hoist the garden_objects/plantings INSERTs into shared objectInsert/plantingInsert statements with matching *InsertArgs helpers. CopyGarden had its own copies of both column lists, so a column added to the table could be wired into Create* and silently dropped from a copy. * store: move the queryer interface to sqlite.go, next to the other shared query plumbing, instead of users.go. * web: derive a copy's prefilled name via defaultCopyName, mirroring the server's copyName including its 200-BYTE cap. The inline `${name} (copy)` both duplicated the suffix and, for a garden whose name was already at the cap, prefilled an over-long name that the server rejected with a 400. Unit-tested, including multi-byte truncation on a code-point boundary. * test: drop a throwaway `_ = kept`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>Gadfly swarm returned 11 findings (several duplicated across models). Addressed in
e1ec93d:ContentLengthis-1, not0)io.EOFfrom the decoder; new unknown-length testdefaultCopyNamemirrors the server'scopyName, 200-byte cap included; unit-tested with multi-byte truncationcopyNameINSERTcolumn lists duplicated betweenCreate*and in-txCopyGardenobjectInsert/plantingInsert+*InsertArgs; a new column can no longer land in one path and be dropped from the otherqueryerdeclared inusers.gosqlite.gobeside the other shared query plumbing_ = kept/ misleading dead variableTwo findings I'm deliberately not acting on:
internal/agenttoolbox is the intended second consumer, andPOST .../copywith no body is the natural shape for a tool call. It's covered by two tests now (absent body and unknown-length empty body), so it isn't an untested path.CreatePlantingsbatch, which loops the same way, and household-scale gardens are tens of objects and hundreds of plops — a few ms under the write lock. Batching would mean multi-rowVALUESand giving upRETURNINGfor the object id→id map. Not worth the complexity here; worth revisiting if garden sizes ever grow by orders of magnitude.Not graded, per usual.