Copy a garden (deep duplicate) from the gardens list #46

Merged
steve merged 2 commits from feat/copy-garden into main 2026-07-21 03:58:40 +00:00
Owner

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_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.

🤖 Generated with Claude Code

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)
steve added 1 commit 2026-07-21 03:50:15 +00:00
Copy a garden (deep duplicate) from the gardens list
Build image / build-and-push (push) Successful in 23s
Gadfly review (reusable) / review (pull_request) Successful in 6m30s
Adversarial Review (Gadfly) / review (pull_request) Successful in 6m30s
5dd35c3800
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 — 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

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>
gitea-actions bot reviewed 2026-07-21 03:56:47 +00:00
gitea-actions bot left a comment

🪰 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>
@@ -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 gardenCopyRequest
if 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 — 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>
@@ -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 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>
@@ -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 (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>
@@ -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-253CopyGarden 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>
@@ -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: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>
@@ -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'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>

🪰 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
2 single-model findings (lower confidence)
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
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

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

🔒 Security — No material issues found

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.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.
🎯 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-252CopyGarden'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-253CopyGarden 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:120copyGarden 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.
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): 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.ErrNoRowsdomain.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:

  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:364kept 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:364kept 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).
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:

  • 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.
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:

  • 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-gatedPOST /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 validationparseIDParam 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-assignmentowner_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 UICopyGardenModal.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.
  • 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.
🧹 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:

  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:20useState(\${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 requireGardenRoleGetGarden, 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.
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.

  • 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 safetyCopyGarden 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 nameCopyGarden 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 truncationcopyName (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>
steve added 1 commit 2026-07-21 03:57:46 +00:00
Address Gadfly review on the garden copy
Build image / build-and-push (push) Successful in 8s
e1ec93ddff
* 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]>
Author
Owner

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 FixeddefaultCopyName 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 main 2026-07-21 03:58:40 +00:00
steve deleted branch feat/copy-garden 2026-07-21 03:58:40 +00:00
Sign in to join this conversation.