REST fill/clear on objects; clear-bed becomes one change set #89

Merged
steve merged 3 commits from feat/fill-clear-rest into main 2026-07-22 02:53:28 +00:00
Owner

Closes #82.

Two problems, one fix

1. Bed filling was agent-only. internal/service/ops.go's header says these bulk ops live on *Service so "agent tools and any future REST surface inherit the same ACL enforcement". The future REST surface never arrived, so on an instance with no OLLAMA_CLOUD_API_KEY the most valuable bulk operation in a garden planner — and the one carrying the most carefully reasoned geometry in the codebase (hexCenters, the half-spacing edge rule from #75) — did not exist at all.

2. The UI's clear-bed wrote N change sets. useClearObject fired a Promise.allSettled of PATCHes, and since every service mutation auto-scopes its own change set, clearing a 40-plop bed wrote 40 of them — 40 presses of Undo to put the bed back. Meanwhile the agent's clear_object, for the identical user-facing action, undid in one click. CLAUDE.md states the rule directly: multi-row operations record together so they undo as one unit.

API

POST /objects/:id/fill    {plantId, region|rect, spacingOverrideCm?}
POST /objects/:id/clear

Both are thin adapters over the service methods the agent already calls, so permissions and the one-change-set guarantee come along unchanged.

Region is by compass name OR rect, never both. region: "all" | "ne" | "south half" | … is what a person means and what the agent uses; rect (object-local frame) is for a future drag-a-box affordance. Supplying both or neither is a 400 — accepting both and silently preferring one would make a client bug look like a geometry bug.

Fill answers 200, not 201. A fill can legitimately create nothing (the region is already planted, and coveredByExisting skips it), and there's no single resource to point a Location at.

Frontend

useClearObject now takes an objectId and makes one request; the loop, its partial-failure reconciliation, and the "N of M couldn't be cleared" error are all gone because the failure mode is gone. ClearBedModal takes objectId + plopCount instead of the full plop array.

Tests

  • TestClearObjectIsOneChangeSetAPI is the one that matters: fill a bed, count change sets, clear it, assert the count rose by exactly one. That's the behaviour being restored.
  • TestFillAndClearAPI — fill by name, clear, and clear again (a no-op, not an error).
  • TestFillRegionSelectionAPI — neither/both/unknown-name all 400; a rect confined to the NE corner produces plops only there.
  • TestFillClearPermissionsAPI — viewer gets 403 (visible but not permitted), stranger gets 404 (existence masked), anonymous gets 401. That distinction is a stated project convention and worth pinning at the boundary.

Note on a helper name

makeFillPlant duplicates what #88 adds as createPlantAPI in seed_lots_test.go. Deliberately named apart: the two branches are independent, and two files declaring the same helper in package api would collide on whichever merged second. Worth consolidating into one shared helper once both have landed — I'd rather do that as a follow-up than couple the branches.

Docs

DESIGN.md's API block gains the two new routes and the four it was already missing: GET/POST/DELETE /gardens/:id/share-link and the unauthenticated GET /public/gardens/:token. That last one is arguably the most security-relevant route in the app and it was absent from the architecture doc — #85 flagged it, but it's the kind of thing to fix on sight while editing the same block.

GOWORK=off go test ./... green; gofmt -l internal/ clean; tsc --noEmit and 87 vitest tests pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

Closes #82. ## Two problems, one fix **1. Bed filling was agent-only.** `internal/service/ops.go`'s header says these bulk ops live on `*Service` so "agent tools and any future REST surface inherit the same ACL enforcement". The future REST surface never arrived, so on an instance with no `OLLAMA_CLOUD_API_KEY` the most valuable bulk operation in a garden planner — and the one carrying the most carefully reasoned geometry in the codebase (`hexCenters`, the half-spacing edge rule from #75) — did not exist at all. **2. The UI's clear-bed wrote N change sets.** `useClearObject` fired a `Promise.allSettled` of PATCHes, and since every service mutation auto-scopes its own change set, clearing a 40-plop bed wrote 40 of them — 40 presses of Undo to put the bed back. Meanwhile the agent's `clear_object`, for the *identical user-facing action*, undid in one click. CLAUDE.md states the rule directly: multi-row operations record together so they undo as one unit. ## API ``` POST /objects/:id/fill {plantId, region|rect, spacingOverrideCm?} POST /objects/:id/clear ``` Both are thin adapters over the service methods the agent already calls, so permissions and the one-change-set guarantee come along unchanged. **Region is by compass name OR rect, never both.** `region: "all" | "ne" | "south half" | …` is what a person means and what the agent uses; `rect` (object-local frame) is for a future drag-a-box affordance. Supplying both or neither is a 400 — accepting both and silently preferring one would make a client bug look like a geometry bug. **Fill answers 200, not 201.** A fill can legitimately create nothing (the region is already planted, and `coveredByExisting` skips it), and there's no single resource to point a `Location` at. ## Frontend `useClearObject` now takes an `objectId` and makes one request; the loop, its partial-failure reconciliation, and the "N of M couldn't be cleared" error are all gone because the failure mode is gone. `ClearBedModal` takes `objectId` + `plopCount` instead of the full plop array. ## Tests - **`TestClearObjectIsOneChangeSetAPI`** is the one that matters: fill a bed, count change sets, clear it, assert the count rose by **exactly one**. That's the behaviour being restored. - `TestFillAndClearAPI` — fill by name, clear, and clear again (a no-op, not an error). - `TestFillRegionSelectionAPI` — neither/both/unknown-name all 400; a rect confined to the NE corner produces plops only there. - `TestFillClearPermissionsAPI` — viewer gets **403** (visible but not permitted), stranger gets **404** (existence masked), anonymous gets 401. That distinction is a stated project convention and worth pinning at the boundary. ## Note on a helper name `makeFillPlant` duplicates what #88 adds as `createPlantAPI` in `seed_lots_test.go`. Deliberately named apart: the two branches are independent, and two files declaring the same helper in package `api` would collide on whichever merged second. Worth consolidating into one shared helper once both have landed — I'd rather do that as a follow-up than couple the branches. ## Docs DESIGN.md's API block gains the two new routes **and the four it was already missing**: `GET/POST/DELETE /gardens/:id/share-link` and the unauthenticated `GET /public/gardens/:token`. That last one is arguably the most security-relevant route in the app and it was absent from the architecture doc — #85 flagged it, but it's the kind of thing to fix on sight while editing the same block. `GOWORK=off go test ./...` green; `gofmt -l internal/` clean; `tsc --noEmit` and 87 vitest tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve added 1 commit 2026-07-21 22:21:18 +00:00
REST fill/clear on objects; clear-bed becomes one change set (#82)
Build image / build-and-push (push) Successful in 18s
Gadfly review (reusable) / review (pull_request) Successful in 5m18s
Adversarial Review (Gadfly) / review (pull_request) Successful in 5m19s
3cfa72cb25
FillRegion and ClearObject were reachable only through the agent toolbox, so
on an instance with no model configured the most valuable bulk operation in a
garden planner — and the one carrying the most carefully reasoned geometry in
the codebase — did not exist at all. internal/service/ops.go said these lived
on *Service so "any future REST surface" would inherit the ACL checks; this is
that surface.

POST /objects/:id/fill takes a region EITHER by compass name ("all", "ne",
"south half") or as an explicit rect in the object's local frame, and refuses
both-or-neither: silently preferring one would make a client bug look like a
geometry bug. It answers 200 rather than 201 because a fill can legitimately
create nothing (the region is already planted) and there is no single resource
to point a Location at.

POST /objects/:id/clear replaces a client-side loop of PATCHes. That loop
wrote one change set per plop, so clearing a 40-plop bed took 40 presses of
Undo to put back — while the agent's clear_object, for the identical
user-facing action, undid in one click. CLAUDE.md states the rule it violated:
multi-row operations record together so they undo as one unit. Moving it
server-side also removes the partial-failure case the loop had to reconcile.

TestClearObjectIsOneChangeSetAPI pins that: fill a bed, count change sets,
clear it, assert the count rose by exactly one.

DESIGN.md's API block gains the two new routes, and the four it was already
missing: GET/POST/DELETE /gardens/:id/share-link and the unauthenticated
GET /public/gardens/:token. An unauthenticated surface absent from the
architecture doc is the one worth fixing on sight.

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

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-22 02:33:00Z

claude-code/sonnet · claude-code — done

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

glm-5.2:cloud · ollama-cloud — done

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

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

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

opencode/glm-5.2:cloud · opencode — done

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

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

  • security — No material issues found
  • ⚠️ correctness — could not complete
  • maintainability — No material issues found
  • 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-22 02:33:00Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ⚠️ **security** — could not complete - ⚠️ **correctness** — could not complete - ⚠️ **maintainability** — could not complete - ✅ **performance** — No material issues found - ⚠️ **error-handling** — could not complete #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ⚠️ **security** — could not complete - ✅ **correctness** — No material issues found - ⚠️ **maintainability** — could not complete - ⚠️ **performance** — could not complete - ⚠️ **error-handling** — could not complete #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Reviewed - ⚠️ **maintainability** — could not complete - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ⚠️ **correctness** — could not complete - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: No material issues found · 0 findings (0 with multi-model agreement)

No material issues found by consensus.

Per-model detail
claude-code/sonnet (claude-code) — No material issues found

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

🔒 Security — No material issues found

VERDICT: No material issues found

I verified the security-relevant path end to end rather than trusting the diff/description:

  • Authn: objects.POST("/:id/fill") and .../clear are registered inside the objects group behind h.requireAuth() (internal/api/api.go:120-124), so mustActor(c) in ops.go can't panic on an unauthenticated request — confirmed by reading mustActor (internal/api/auth.go:207-211) and the route group wiring.
  • CSRF: the v1 group applies h.csrfGuard() before any route registration (internal/api/api.go:51), so both new POST endpoints inherit the origin check, same as every other mutating route.
  • Authz: FillRegion, FillNamedRegion, and ClearObject all call s.objectForRole(ctx, actorID, objectID, roleEditor) (internal/service/ops.go:112,318,335), which masks existence as ErrNotFound for gardens the actor can't see and returns ErrForbidden for insufficient role (internal/service/gardens.go:78-83), and writeServiceError maps those to 404/403 correctly (internal/api/errors.go:26-29). The permissions test (TestFillClearPermissionsAPI) exercises exactly this and I traced it against real code, not just the fixture's assumptions.
  • IDOR on the plant reference: fillLoaded resolves plantId via visiblePlant, which rejects a plant owned by another user or a nonexistent one as ErrInvalidInput rather than leaking whether it exists (internal/service/plantings.go:217-229) — this closes off using the fill endpoint to probe/reference another user's private plants.
  • Untrusted geometry (rect): the handler's degenerate() check rejects zero/negative-area rects, and server-side fillLoaded independently rejects non-finite region bounds and clamps to the object's actual bounds (internal/service/ops.go:141-158) before hexCenters ever runs — so a malicious/malformed rect can't plant outside the object or poison the row with NaN/Inf.
  • DoS via huge fill: hexCenters computes the total plop count before allocating and refuses over maxFillPlops = 5000 (internal/service/ops.go:20,231-239), so an attacker requesting region: "all" on a huge object with tiny spacing gets a 400, not an unbounded insert.
  • Injection: ClearObjectPlantings builds its IN (...) clause with placeholders(len(ids)) and passes ids as bind args, not string-concatenated (internal/store/plantings.go:174-189) — no SQL injection surface from the new bulk-clear path.

Frontend changes (ClearBedModal.tsx, objects.ts) are a straightforward request-shape simplification with no new rendering of untrusted content or new URL/host construction — nothing in scope for this lens.

Outside my lens: none worth flagging.

🎯 Correctness — No material issues found

VERDICT: No material issues found

I reviewed the diff (new internal/api/ops.go handlers, route registration in internal/api/api.go, ops_test.go, and the frontend ClearBedModal.tsx/objects.ts/GardenEditorPage.tsx changes) against the actual checked-out repo. Notes on what I verified:

  • The service-layer geometry this PR wraps (hexCenters, NamedRegion, FillRegion, ClearObject in internal/service/ops.go) already exists on main and is untouched by this diff — I read it to sanity-check the half-spacing edge rule and hex-lattice math against CLAUDE.md's #75 explanation; it's internally consistent (inset = radius - spacing/2, clamped at 0) and out of scope for this PR's changes.
  • objectFillRequest's exclusivity check (named == hasRect → 400) correctly enforces "exactly one of region/rect", verified against TestFillRegionSelectionAPI's neither/both/unknown-name cases.
  • fillRect.degenerate() (MaxX <= MinX || MaxY <= MinY) correctly catches the zero-area, inverted, and empty-{} cases the test enumerates.
  • writeServiceError (internal/api/errors.go:24) maps ErrInvalidInput→400, ErrForbidden→403, ErrNotFound→404 — matches the permissions test's expected viewer/stranger/anonymous codes, and objects := v1.Group("/objects", h.requireAuth()) (internal/api/api.go:118) confirms anonymous requests 401 before reaching the handler.
  • NE-corner rect test (minY:-200, maxY:0 → assert x>=0, y<=0) is consistent with the documented local frame (+x east, +y south, so -y is north) — correct, not inverted.
  • Frontend: useClearObject's mutationFn signature (objectId: number) matches ClearBedModal's clear.mutate(objectId, ...) call and GardenEditorPage.tsx's prop wiring (objectId={focusedObject.id}, plopCount={focusedPlops.length}); objectErrorMessage and focusedPlops are pre-existing symbols, confirmed via grep, so no broken references.
  • api.post(path) with no body (used by useClearObject) matches post's optional-body signature in web/src/lib/api.ts:139.

Nothing in this diff's own logic — request validation, region-selection branching, error mapping, response shape, or the frontend plumbing — is incorrect.

🧹 Code cleanliness & maintainability — No material issues found

Not critical — the code is visibly well-formatted from direct reading. I have enough to finalize the review.

VERDICT: No material issues found

The new REST handlers (internal/api/ops.go) follow the existing package conventions closely: same parseIDParam/writeAPIError/writeServiceError/mustActor patterns as plantings.go and agent.go, thin adapters with no logic duplicated from the service layer, and comments that explain non-obvious why (200 vs 201, degenerate-rect rejection, the rect-pointer rebinding) rather than restating the code. The objects.ts/ClearBedModal.tsx changes correctly delete the now-dead reconciliation logic (partial-failure handling, the today date computation, the loop) rather than leaving it stubbed out, and useClearObject matches the error-handling idiom (objectErrorMessage) used by every other mutation in that file. focusedPlops in GardenEditorPage.tsx remains genuinely used (count display), not leftover dead state.

The one duplication in this diff — makeFillPlant in ops_test.go overlapping with createPlantAPI from the still-unmerged #88 — is explicitly called out and justified in both the PR description and a code comment as a deliberate, temporary branch-collision avoidance, with a stated follow-up to consolidate. That's the kind of tech debt worth accepting as written rather than flagging.

No dead code, no oversized/deeply-nested functions, no copy-paste that should already be shared, no naming that fights the surrounding package.

Performance — No material issues found

This all looks clean from a performance standpoint — no N+1 patterns, no hot-loop issues, and the change is a net perf improvement (N PATCH requests collapsed to 1 POST, plus the corresponding drop from N change-set writes to 1 on the server). The API handlers are thin single-call wrappers with no added overhead. focusedPlops filtering in GardenEditorPage.tsx:281 is pre-existing and unchanged by this diff.

Verdict: No material issues found

Through the performance lens, this diff has no regressions:

  • internal/api/ops.go handlers (fillObject, clearObject) are thin single-call adapters over existing service methods — no loops, no redundant queries, no extra allocations of note.
  • The frontend change is a clear performance win: useClearObject (web/src/lib/objects.ts:342) replaced a Promise.allSettled loop of N PATCH requests with a single POST, eliminating N network round-trips and N separate change-set writes (verified server-side in internal/service/ops.go:331ClearObject does one snapshot read, one bulk UPDATE via ClearObjectPlantings, and one history read, not per-row writes).
  • FillRegion/fillLoaded (unchanged, pre-existing code at internal/service/ops.go:120+) already batches inserts in one transaction and caps output at maxFillPlops — not part of this diff, but confirms the endpoint being exposed doesn't newly introduce quadratic or unbounded work.
  • ClearBedModal/GardenEditorPage passing plopCount instead of a mapped plops array is a minor allocation reduction, not a regression.

No N+1 queries, unbounded growth, or hot-loop concerns introduced by this PR.

🧯 Error handling & edge cases — No material issues found

VERDICT: No material issues found

I reviewed the new REST handlers (internal/api/ops.go), route wiring (internal/api/api.go), the frontend useClearObject/ClearBedModal changes, and the underlying service methods they call (internal/service/ops.go), focused specifically on ignored errors, missing cleanup, and edge/boundary conditions.

What I verified:

  • fillObject/clearObject (internal/api/ops.go:44-116) correctly check ShouldBindJSON errors, use parseIDParam (which rejects non-positive/malformed ids, internal/api/errors.go:109), and route every service error through writeServiceError, which has an explicit default: branch that logs and 500s rather than silently dropping unmapped errors (internal/api/errors.go:56-59).
  • The region/rect XOR validation and the degenerate() zero/inverted-rect check are exercised by TestFillRegionSelectionAPI, including the "rect": {} all-zeros decode case — I confirmed fillRect{} (JSON {}) does decode to a non-nil pointer with zero fields, so degenerate() correctly catches it.
  • The service layer the new routes call (fillLoaded, hexCenters, ClearObject) already has deliberate, well-commented guards for non-finite regions/spacing, maxFillPlops overflow protection computed before allocating, and a documented decision to treat a post-clear history-write failure as a logged-but-successful clear rather than reporting a false failure to the caller (internal/service/ops.go:140-150, 253-255, 354-362). None of this is modified by the diff, and the new handlers don't bypass or duplicate it.
  • useClearObject (web/src/lib/objects.ts:328-352) no longer swallows partial failures (the old Promise.allSettled loop is gone); a thrown error from api.post or a clearResultSchema.parse mismatch propagates to React Query's onError, which is still wired to a toast, and onSettled still invalidates the cache in both outcomes so the UI can't go stale on a failure.
  • ClearBedModal/GardenEditorPage edge cases (focusedPlops defaults to [], plopCount === 0 still disables the button) are unchanged in spirit from before and don't introduce a new null/empty-collection hazard.

Nothing in this diff ignores an error, skips cleanup, or introduces a new unhandled boundary condition. Outside my lens: none worth flagging (nothing severe I noticed elsewhere).

glm-5.2:cloud (ollama-cloud) — No material issues found

Verdict: No material issues found · ⚠️ 4/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security⚠️ could not complete

⚠️ This reviewer failed to complete: agent: step 3: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests

🎯 Correctness⚠️ could not complete

⚠️ This reviewer failed to complete: agent: step 4: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests

🧹 Code cleanliness & maintainability⚠️ could not complete

⚠️ This reviewer failed to complete: agent: step 3: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests

Performance — No material issues found

I've verified the service methods and store layer. This PR's performance impact is straightforwardly positive — let me confirm the key points.

The new fillObject/clearObject handlers are thin adapters over pre-existing service methods (FillRegion/FillNamedRegion/ClearObject), which I read in internal/service/ops.go. The geometry, batching, and one-change-set logic are all unchanged from the agent path:

  • fillLoaded (ops.go:120) caps at maxFillPlops = 5000, builds the lattice with a count-before-allocate optimization in hexCenters (ops.go:224), and inserts the batch in a single transaction via CreatePlantings (plantings.go:240) — one commit, not one round-trip per plop.
  • ClearObject (ops.go:331) snapshots active rows once, issues a single bulk UPDATE ... id IN (...) (plantings.go:174), and re-reads once to build revisions. No per-row queries.

The frontend change is the actual performance story, and it's a strict win: useClearObject (objects.ts) went from Promise.allSettled over N PATCH /plantings/:id requests (N network round-trips, N change-set writes, N version-guard conflicts possible) to a single POST /objects/:id/clear. Fewer requests, one transaction, one change set, no partial-failure reconciliation loop.

Nothing the diff introduces is a regression. The only pre-existing hot-path concern in scope — coveredByExisting (ops.go:302) is an O(centers × existing) nested loop that grows further because existing is appended to during the fill (ops.go:170), making a maxed fill O(N²) in hypot calls — lives entirely in the unchanged service code, not in this PR's diff, so it isn't this change's regression to flag.

Verdict: No material issues found

🧯 Error handling & edge cases⚠️ could not complete

⚠️ This reviewer failed to complete: agent: step 1: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests

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

Verdict: No material issues found · ⚠️ 4/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security⚠️ could not complete

⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ skipped (backed off until 02:18:18.304)

🎯 Correctness — No material issues found

No material issues found

I verified the new fillObject and clearObject handlers in internal/api/ops.go, traced their calls through to the existing service methods (FillRegion, FillNamedRegion, ClearObject), and checked the router setup in internal/api/api.go. The geometry in NamedRegion (NE = rect(0, -hh, hw, 0)) matches exactly what the rect-based test in TestFillRegionSelectionAPI asserts (x ≥ 0, y ≤ 0 for NE). The degenerate() check correctly rejects zero-area and inverted rects with <= on both axes. The one-change-set guarantee is both implemented in the service layer (ClearObject snapshots then bulk-updates by IDs) and pinned by the regression test TestClearObjectIsOneChangeSetAPI. Both endpoints are behind the existing requireAuth middleware and inherit the service's role-based permissions (roleEditor), so the 403/404/401 distinctions are handled by the existing objectForRole flow. No constants, thresholds, or formulas needed independent re-derivation — they all delegate to the same service methods the agent already uses.

🧹 Code cleanliness & maintainability⚠️ could not complete

⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ skipped (backed off until 02:18:18.304)

Performance⚠️ could not complete

⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ skipped (backed off until 02:18:18.304)

🧯 Error handling & edge cases⚠️ could not complete

⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests

opencode/glm-5.2:cloud (opencode) — No material issues found

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

🔒 Security — No material issues found

The service layer enforces roleEditor via objectForRole (line 109/315/332), so authz is handled. Let me verify the error mapping and the region name validation for any injection/path concerns, plus check that the rect coordinates aren't used unsafely downstream.
Everything checks out cleanly through the security lens. The authz flows through the existing objectForRole (requiring roleEditor), existence masking (ErrNotFound for both not-found and no-access) is preserved via writeServiceError, region names are validated against a strict allowlist switch, rect coordinates are finite-checked and clamped server-side in fillLoaded, and the spacing override is bounded.

One thing worth noting: in fillObject, when a rect is supplied the handler checks degenerate() in the API layer but the service's own fillLoaded only checks isFinite (not inverted-rect). This is fine here because the API layer catches it, but it's a minor defense-in-depth gap. Not material enough to flag as a security issue though — the API gate is sufficient for the only entry point that takes a rect.

VERDICT: No material issues found

  • Verified that fillObject and clearObject delegate to FillRegion/FillNamedRegion/ClearObject, all of which call s.objectForRole(..., roleEditor) as their first step (internal/service/ops.go:109, 315, 332). The authz check happens before any geometry or DB work, so a viewer/stranger/anonymous is rejected at the same boundary the agent tools use.
  • Verified ErrNotFound maps to 404 (masking existence for no-access) and ErrForbidden to 403 in writeServiceError (internal/api/errors.go:26-29), matching the TestFillClearPermissionsAPI assertions.
  • Verified region (compass name) is validated against a fixed switch allowlist in NamedRegion (internal/service/ops.go:70-91), defaulting to ErrInvalidInput — no string interpolation, no SQL/template/path use of the name.
  • Verified caller-supplied rect coordinates flow into fillLoaded, which checks isFinite on each (ops.go:147-150) and clamps to object bounds (ops.go:152), and spacingOverride is bounded to [minPlantSpacingCM, maxPlantSpacingCM] (ops.go:130). No tainted-float reaches SQL unguarded.
  • No secrets/credentials touched; no new unauthenticated routes (the GET /public/gardens/:token addition is a docs-only change to DESIGN.md, not new code in this PR).
🎯 Correctness — Reviewed

I'll verify the service-layer methods the new handlers call, and the geometry/semantics around them.
Now let me check the api.post without a body to confirm it doesn't send a JSON body that would fail binding. The server's clearObject doesn't bind JSON, so that's fine. But let me verify the api.post sends no content-type/body when body is undefined.
Now let me verify the fillObject handler: the binding:"required" on PlantID. When region is empty AND rect is nil, binding succeeds (neither field has binding:"required"), and then the named/hasRect check catches it. But there's a subtle case: what about binding:"required" — does gin's required validator treat 0 as missing? Let me check.

The PlantID int64 ... binding:"required" — gin's required validator treats 0 as missing for non-pointer types. Plant ID 0 is never a valid plant ID (auto-increment), so this is fine. But let me verify this doesn't break a legitimate test.
Now let me look at the clearObject handler more closely. The frontend sends no body. clearObject doesn't call ShouldBindJSON — good, no EOF issue. Let me double-check the test: TestFillClearPermissionsAPI sends nil cookie for anonymous — let me verify doJSON with nil cookie gives 401 (the route is behind requireAuth).
The permission funnel through objectForRolerequireGardenRole returns ErrNotFound for no role and ErrForbidden for visible-but-not-editor. So writeServiceError correctly maps these to 404/403 — the test's expectations (viewer 403, stranger 404, anonymous 401) align with actual behavior. Verified the full path.

Now let me verify one subtle thing in the fill test: the NE rect in a 400×400 bed. The object's local frame origin is at center, so the object spans x∈[-200,200], y∈[-200,200]. NE = +x east, -y north. The rect {minX: 0, minY: -200, maxX: 200, maxY: 0} is the NE quadrant. The assertion checks x < 0 || y > 0 — i.e. plop must have x≥0 and y≤0, which matches the NE quadrant. But note: the rect itself gets clamped to object bounds in fillLoaded via clampTo, and then hexCenters uses inset so centers stay within [MinX+inset, MaxX-inset]. With radius = max(1.5×20, 15) = 30, inset = max(0, 30 - 10) = 20. So centers are in x∈[20, 180], y∈[-180, -20]. All satisfy x≥0 and y≤0. ✓. The test logic is sound.

Now let me double-check the degenerate() check ordering vs the named/hasRect XOR. The XOR check happens first (line 64-69), then degenerate inside the rect branch (line 73-76). For a request with only rect: {} (empty), named=false, hasRect=true, XOR passes, then rect.degenerate() is true (0≤0), returns 400. ✓.

Let me verify the SpacingOverrideCM field name — the JSON tag is spacingOverrideCm but the field is SpacingOverrideCM. Go field names don't matter for JSON binding; only the tag matters. ✓.

Let me verify the api.post with no body actually sends no body (so the server's clearObject which doesn't bind JSON is fine), and that it works. Already confirmed in api.ts lines 90, 100: body !== undefined ? ... : undefined, and content-type only set when body is defined. Good.

Now let me check ClearBedModal callers to make sure all pass the new props correctly.
Let me verify whether plantings (the source of focusedPlops) is already filtered to active plops, since ClearBedModal uses plopCount which should be the count of active plops. The clear endpoint clears active plops server-side.
Confirmed: /full returns active plantings, so focusedPlops (active count) matches what the server clears. The cleared count should match plopCount displayed. ✓

Now let me check the test's claim that the second clear returns cleared: 0. Looking at ClearObject line 350-351: if err != nil || n == 0 { return n, err }. So when bed is empty, before is empty slice, ids is empty, ClearObjectPlantings returns n=0, returns (0, nil). Status 200, body {"cleared": 0}. ✓

Now let me check the record call happens only when n != 0. Looking at the test TestClearObjectIsOneChangeSetAPI: it does a fill (creates 1 change set), then clears (should create 1 change set). Let me verify ClearObject records once. Line 350-352 returns early if n==0; otherwise line 358+ builds changes and calls s.record. So yes, one record call → one change set. ✓

Let me verify that s.record creates exactly one change set, not that it auto-scopes via commitScope.
Let me verify the plant creation endpoint accepts the fields the test helper sends.
The test helper makeFillPlant sends name, category, spacingCm, color, icon — all match. Good. Let me verify the createGardenAPI and object creation match the test helper's expectations.
Now let me verify the object creation accepts kind: bed, widthCm, heightCm, plantable.
All verified. Let me now look at whether there's an issue with the fill handler: when Region is supplied as an unknown compass name like "middle-ish", ShouldBindJSON succeeds (no binding required on Region), then FillNamedRegionNamedRegion returns domain.ErrInvalidInput, which writeServiceError maps to 400. ✓ The test TestFillRegionSelectionAPI expects 400 for bad name — confirmed.

Now let me double-check one subtle correctness concern: the objectFillRequest uses binding:"required" on PlantID which is int64. In gin's validator, required on a numeric zero value fails. Plant IDs start at 1 (auto-increment), so 0 is never valid. The test always uses real plant IDs. Fine.

Let me verify the share creation path used by the permissions test (sharesPath POST with email/role).
All verified and consistent. The change is a clean, thin REST adapter over already-tested service methods. Through the correctness lens, I traced every path:

  • fillObject: parse ID, bind (plantId required), XOR check region/rect, degenerate rect check, dispatch to FillRegion/FillNamedRegion, 200 on success. Errors flow through writeServiceError → correct status codes (404 stranger, 403 viewer, 400 invalid input, 401 anonymous via requireAuth).
  • clearObject: parse ID, call ClearObject, 200 with count. Empty bed returns (0, nil) → 200 {"cleared":0}.
  • One-change-set guarantee: ClearObject calls s.record exactly once (after the n==0 early return), auto-scoping one change set. FillRegion likewise calls s.record once with all created plops.
  • Frontend useClearObject: single POST, validates cleared count, invalidates queries. api.post with no body sends no content-type/body — server's clearObject doesn't bind JSON, so no EOF issue.
  • ClearBedModal props updated consistently at the call site; plopCount derived from focusedPlops which is the active-plantings list (matches server clear semantics).
  • Geometry: the NE-rect test (minX:0, minY:-200, maxX:200, maxY:0 in a 400×400 bed) asserts x>=0 && y<=0, which correctly describes the NE quadrant in the local frame (+x east, -y north). clampTo + inset keep centers inside, so no plop falls outside the asserted bounds.

No material correctness issues found.

🧹 Code cleanliness & maintainability⚠️ could not complete

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

Performance — No material issues found

No material issues found.

This PR is strictly a performance win in my lane. The frontend change replaces an N+1 client-side loop (one PATCH per plop, each its own round-trip and its own change-set write) with a single POST that hits one bulk UPDATE server-side — fewer requests, fewer store round-trips, fewer history writes. The new handlers are thin adapters over the existing FillRegion/FillNamedRegion/ClearObject service methods, which I read (internal/service/ops.go:108-379); the work they do is unchanged from what the agent path already did, so no new hot-path cost is introduced.

Checked specifics:

  • ClearObject (ops.go:331-379) does a snapshot read, one bulk UPDATE, then a second ListPlantingsForObject to build revision entries. That second query is unbounded over history (it returns removed rows too), but it is pre-existing behavior, not introduced here, and runs once per clear — not per plop.
  • FillRegion is capped at maxFillPlops = 5000 (ops.go:23) and counts-before-building to avoid allocating a huge lattice just to refuse it (hexCenters, ops.go:224-272). The API response serializes up to 5000 Planting objects — bounded, and no worse than the agent already received.
  • The degenerate rect check and the region/rect mutual-exclusion check are O(1) input validation, not hot-path concerns.
🧯 Error handling & edge cases — No material issues found

VERDICT: No material issues found

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

  • fillObject (internal/api/ops.go:56-98): All unhappy paths handled. parseIDParam → 400 on bad id; ShouldBindJSON → 400 on malformed/missing-required; the named == hasRect guard catches both-neither and both; rect.degenerate() catches zero-area/inverted rects (including the all-zeros {} case) before they reach the service. Service errors flow through writeServiceError. The nil-rect deref is guarded inside if rect := req.Rect; rect != nil. ✓
  • clearObject (internal/api/ops.go:105-116): parseIDParam + writeServiceError cover the bad paths. The service's ClearObject (internal/service/ops.go:331-379) handles the empty-bed case (n==0, err==nil → returns (0,nil) → handler emits {"cleared":0}), and the narrow "history write failed after clear applied" race is deliberately swallowed and logged (slog.Error + return success) — consistent with the project's stated "history is best-effort, the row is already written" rule. ✓
  • useClearObject (web/src/lib/objects.ts:342-354): clearResultSchema.parse validates the response (a shape mismatch throws → onError → toast). api.post with no body sends undefined (no content-type header, no body) per apiFetch (web/src/lib/api.ts:90,100), matching the handler which doesn't read a body. ApiError from non-2xx propagates to objectErrorMessage. ✓
  • ClearBedModal (web/src/editor/ClearBedModal.tsx): plopCount === 0 disables the confirm button; the server side also treats an empty bed as a no-op (200, cleared: 0), so a stale count from a concurrent clear can't cause a bad action. ✓
  • Numeric edge cases: SpacingOverrideCM is a pointer (absent → nil → no validation; 0 → rejected by service as < minPlantSpacingCM); PlantID binding:"required" rejects 0/negative/non-numeric; JSON can't carry NaN/Inf into fillRect, and the service re-checks isFinite on the region anyway. No overflow path — maxFillPlops caps the lattice before allocation. ✓

One trivially-misleading (not material) item I noted but am not reporting as a finding: the ShouldBindJSON error message at ops.go:63 says "a plantId and a region are required," but region is not a binding-required field — it's enforced by the subsequent named == hasRect check. A client sending valid JSON with only plantId gets the more accurate "supply exactly one of region or rect" message instead, so the misleading message only surfaces for genuinely malformed JSON / missing plantId.

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

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

🔒 Security — No material issues found

No material issues found

  • Authn/Authz: Both POST /objects/:id/fill and POST /objects/:id/clear are registered under the existing /objects group in internal/api/api.go:118, which carries h.requireAuth(). Each handler uses mustActor(c) and delegates immediately to service.FillRegion / FillNamedRegion / ClearObject, all of which call objectForRole(..., roleEditor). Verified by reading internal/service/ops.go:108-334 and internal/service/objects.go:148. The permission checks (viewer → 403, stranger → 404, anonymous → 401) are exercised in the new tests and match the existing project convention.
  • Input validation: The handler validates that exactly one of region or rect is supplied (internal/api/ops.go:66-70). Degenerate rectangles are rejected (ops.go:82-84). The service layer already enforces finite floats, clamps to object bounds, caps total plops at maxFillPlops, validates spacing overrides against minPlantSpacingCM/maxPlantSpacingCM, and rejects non-plantable objects. No new injection surface is introduced — the handler does not construct SQL or commands.
  • Race condition in clear: ClearObject snapshots active plop IDs before the bulk UPDATE (internal/service/ops.go:340-347) and passes explicit IDs to ClearObjectPlantings, preventing a newly-created plop from being silently removed without a revision. The history-write failure is handled by logging and returning success (ops.go:354-362), matching the documented commitScope / detached-history rule from CLAUDE.md. This is a deliberate safety choice, not a bug.
  • CSRF: The routes sit behind the global csrfGuard() (internal/api/api.go:54), which rejects cross-origin state-changing requests when PANSY_BASE_URL is set.

Nothing in this diff raises a security concern.

🎯 Correctness⚠️ could not complete

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

🧹 Code cleanliness & maintainability — No material issues found

Verdict: No material issues found

I read the full contents of internal/api/ops.go, internal/api/ops_test.go, web/src/lib/objects.ts, web/src/editor/ClearBedModal.tsx, and the relevant surrounding files. Through the code-cleanliness lens, the change is well-structured and consistent with existing patterns:

  • Handler structure: fillObject and clearObject in ops.go follow the established thin-adapter pattern (decode → call service → encode). The new fillRect type with its degenerate() method is appropriately extracted rather than inlined, matching how other request structs in internal/api/ carry their own validation.
  • Naming: ops.go/ops_test.go mirror the service-layer naming. The SpacingOverrideCM field follows the unit-explicit convention used elsewhere (SpacingCM in plant structs, etc.).
  • Frontend refactor: useClearObject simplifies from a client-side Promise.allSettled loop to a single mutation, which is a clear net reduction in complexity. The clearResultSchema is a good addition for runtime validation.
  • Test helpers: makeFillPlant is intentionally named apart from createPlantAPI to avoid a merge collision, with an explicit TODO comment — the right call for a cross-branch situation.

No dead code, no confusing names, no overly long functions, and no abstractions that leak. An honest "nothing in my area."

Performance — No material issues found

No material issues found

Through the Performance lens, this PR is clean. In fact, it fixes a notable efficiency regression:

  • The old useClearObject was N+1 over the network. A Promise.allSettled of per-plop PATCHes meant clearing a 40-plop bed cost 40 round-trips, 40 change-set writes, and 40 undo actions. The new single POST /objects/:id/clear eliminates all of that.
  • The new REST adapters are thin wrappers over existing service methods (FillRegion, FillNamedRegion, ClearObject) and do not introduce new allocations, extra queries, or unbounded growth. The fill path already caps plops at maxFillPlops = 5000 and pre-counts via hexCenters before allocating.
  • No new loops, no missing pagination, no blocking hot-path additions. The endpoints do not stream or paginate, but they don't need to: the service layer's bounds make the payloads bounded by design.
🧯 Error handling & edge cases — No material issues found

No material issues found

Through the 🧯 Error handling & edge cases lens, the PR is clean.

Verified checks:

  • fillObject: parseIDParam guards malformed IDs; ShouldBindJSON rejects missing plantId; the named == hasRect check correctly enforces exactly-one-of region/rect; degenerate() catches zero-area rects (including the all-zeros {} case); non-finite coordinates are rejected by the service's isFinite guard; spacing override is bounded by [minPlantSpacingCM, maxPlantSpacingCM]; FillNamedRegion rejects unknown region names via ErrInvalidInput; the fill path clamps to object bounds and legitimately returns 200 with zero creations for already-covered regions.
  • clearObject: id parsing is guarded; the service snapshots active plops before updating and falls back gracefully when n == 0 (already-empty bed is a no-op, not an error); history-write failures are logged and swallowed rather than reported as a failed clear (matching the existing record() best-effort design); permissions flow through the existing objectForRoleErrNotFound/ErrForbidden mapping.
  • Frontend: useClearObject validates the response with zod before returning; onError surfaces failures via toast.error(objectErrorMessage(...)); the mutation invalidates the cache on settled regardless of outcome.
  • Tests: TestFillRegionSelectionAPI covers neither/both/unknown-name/degenerate-rect; TestClearObjectIsOneChangeSetAPI verifies the one-change-set guarantee; TestFillClearPermissionsAPI pins 403 vs 404 vs 401.

One theoretical race exists in the service-layer ClearObject history snapshot (concurrent removals between the read and the UPDATE can cause over-reporting in the recorded changes), but that is pre-existing in internal/service/ops.go and not introduced or worsened by this PR.

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: No material issues found** · 0 findings (0 with multi-model agreement) No material issues found by consensus. <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — No material issues found</summary> **Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **VERDICT: No material issues found** I verified the security-relevant path end to end rather than trusting the diff/description: - **Authn**: `objects.POST("/:id/fill")` and `.../clear` are registered inside the `objects` group behind `h.requireAuth()` (`internal/api/api.go:120-124`), so `mustActor(c)` in `ops.go` can't panic on an unauthenticated request — confirmed by reading `mustActor` (`internal/api/auth.go:207-211`) and the route group wiring. - **CSRF**: the `v1` group applies `h.csrfGuard()` before any route registration (`internal/api/api.go:51`), so both new POST endpoints inherit the origin check, same as every other mutating route. - **Authz**: `FillRegion`, `FillNamedRegion`, and `ClearObject` all call `s.objectForRole(ctx, actorID, objectID, roleEditor)` (`internal/service/ops.go:112,318,335`), which masks existence as `ErrNotFound` for gardens the actor can't see and returns `ErrForbidden` for insufficient role (`internal/service/gardens.go:78-83`), and `writeServiceError` maps those to 404/403 correctly (`internal/api/errors.go:26-29`). The permissions test (`TestFillClearPermissionsAPI`) exercises exactly this and I traced it against real code, not just the fixture's assumptions. - **IDOR on the plant reference**: `fillLoaded` resolves `plantId` via `visiblePlant`, which rejects a plant owned by another user or a nonexistent one as `ErrInvalidInput` rather than leaking whether it exists (`internal/service/plantings.go:217-229`) — this closes off using the fill endpoint to probe/reference another user's private plants. - **Untrusted geometry (`rect`)**: the handler's `degenerate()` check rejects zero/negative-area rects, and server-side `fillLoaded` independently rejects non-finite region bounds and clamps to the object's actual bounds (`internal/service/ops.go:141-158`) before `hexCenters` ever runs — so a malicious/malformed rect can't plant outside the object or poison the row with NaN/Inf. - **DoS via huge fill**: `hexCenters` computes the total plop count *before* allocating and refuses over `maxFillPlops = 5000` (`internal/service/ops.go:20,231-239`), so an attacker requesting `region: "all"` on a huge object with tiny spacing gets a 400, not an unbounded insert. - **Injection**: `ClearObjectPlantings` builds its `IN (...)` clause with `placeholders(len(ids))` and passes ids as bind args, not string-concatenated (`internal/store/plantings.go:174-189`) — no SQL injection surface from the new bulk-clear path. Frontend changes (`ClearBedModal.tsx`, `objects.ts`) are a straightforward request-shape simplification with no new rendering of untrusted content or new URL/host construction — nothing in scope for this lens. Outside my lens: none worth flagging. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **VERDICT: No material issues found** I reviewed the diff (new `internal/api/ops.go` handlers, route registration in `internal/api/api.go`, `ops_test.go`, and the frontend `ClearBedModal.tsx`/`objects.ts`/`GardenEditorPage.tsx` changes) against the actual checked-out repo. Notes on what I verified: - The service-layer geometry this PR wraps (`hexCenters`, `NamedRegion`, `FillRegion`, `ClearObject` in `internal/service/ops.go`) already exists on `main` and is untouched by this diff — I read it to sanity-check the half-spacing edge rule and hex-lattice math against CLAUDE.md's #75 explanation; it's internally consistent (inset = `radius - spacing/2`, clamped at 0) and out of scope for this PR's changes. - `objectFillRequest`'s exclusivity check (`named == hasRect` → 400) correctly enforces "exactly one of region/rect", verified against `TestFillRegionSelectionAPI`'s neither/both/unknown-name cases. - `fillRect.degenerate()` (`MaxX <= MinX || MaxY <= MinY`) correctly catches the zero-area, inverted, and empty-`{}` cases the test enumerates. - `writeServiceError` (`internal/api/errors.go:24`) maps `ErrInvalidInput`→400, `ErrForbidden`→403, `ErrNotFound`→404 — matches the permissions test's expected viewer/stranger/anonymous codes, and `objects := v1.Group("/objects", h.requireAuth())` (`internal/api/api.go:118`) confirms anonymous requests 401 before reaching the handler. - NE-corner rect test (`minY:-200, maxY:0` → assert `x>=0, y<=0`) is consistent with the documented local frame (`+x east, +y south`, so `-y` is north) — correct, not inverted. - Frontend: `useClearObject`'s `mutationFn` signature (`objectId: number`) matches `ClearBedModal`'s `clear.mutate(objectId, ...)` call and `GardenEditorPage.tsx`'s prop wiring (`objectId={focusedObject.id}`, `plopCount={focusedPlops.length}`); `objectErrorMessage` and `focusedPlops` are pre-existing symbols, confirmed via grep, so no broken references. - `api.post(path)` with no body (used by `useClearObject`) matches `post`'s optional-`body` signature in `web/src/lib/api.ts:139`. Nothing in this diff's own logic — request validation, region-selection branching, error mapping, response shape, or the frontend plumbing — is incorrect. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> Not critical — the code is visibly well-formatted from direct reading. I have enough to finalize the review. VERDICT: No material issues found The new REST handlers (`internal/api/ops.go`) follow the existing package conventions closely: same `parseIDParam`/`writeAPIError`/`writeServiceError`/`mustActor` patterns as `plantings.go` and `agent.go`, thin adapters with no logic duplicated from the service layer, and comments that explain non-obvious *why* (200 vs 201, degenerate-rect rejection, the rect-pointer rebinding) rather than restating the code. The `objects.ts`/`ClearBedModal.tsx` changes correctly delete the now-dead reconciliation logic (partial-failure handling, the `today` date computation, the loop) rather than leaving it stubbed out, and `useClearObject` matches the error-handling idiom (`objectErrorMessage`) used by every other mutation in that file. `focusedPlops` in `GardenEditorPage.tsx` remains genuinely used (count display), not leftover dead state. The one duplication in this diff — `makeFillPlant` in `ops_test.go` overlapping with `createPlantAPI` from the still-unmerged #88 — is explicitly called out and justified in both the PR description and a code comment as a deliberate, temporary branch-collision avoidance, with a stated follow-up to consolidate. That's the kind of tech debt worth accepting as written rather than flagging. No dead code, no oversized/deeply-nested functions, no copy-paste that should already be shared, no naming that fights the surrounding package. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> This all looks clean from a performance standpoint — no N+1 patterns, no hot-loop issues, and the change is a net perf improvement (N PATCH requests collapsed to 1 POST, plus the corresponding drop from N change-set writes to 1 on the server). The API handlers are thin single-call wrappers with no added overhead. `focusedPlops` filtering in `GardenEditorPage.tsx:281` is pre-existing and unchanged by this diff. ## Verdict: No material issues found Through the performance lens, this diff has no regressions: - `internal/api/ops.go` handlers (`fillObject`, `clearObject`) are thin single-call adapters over existing service methods — no loops, no redundant queries, no extra allocations of note. - The frontend change is a clear performance win: `useClearObject` (`web/src/lib/objects.ts:342`) replaced a `Promise.allSettled` loop of N PATCH requests with a single POST, eliminating N network round-trips and N separate change-set writes (verified server-side in `internal/service/ops.go:331` — `ClearObject` does one snapshot read, one bulk `UPDATE` via `ClearObjectPlantings`, and one history read, not per-row writes). - `FillRegion`/`fillLoaded` (unchanged, pre-existing code at `internal/service/ops.go:120`+) already batches inserts in one transaction and caps output at `maxFillPlops` — not part of this diff, but confirms the endpoint being exposed doesn't newly introduce quadratic or unbounded work. - `ClearBedModal`/`GardenEditorPage` passing `plopCount` instead of a mapped `plops` array is a minor allocation reduction, not a regression. No N+1 queries, unbounded growth, or hot-loop concerns introduced by this PR. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **VERDICT: No material issues found** I reviewed the new REST handlers (`internal/api/ops.go`), route wiring (`internal/api/api.go`), the frontend `useClearObject`/`ClearBedModal` changes, and the underlying service methods they call (`internal/service/ops.go`), focused specifically on ignored errors, missing cleanup, and edge/boundary conditions. What I verified: - `fillObject`/`clearObject` (`internal/api/ops.go:44-116`) correctly check `ShouldBindJSON` errors, use `parseIDParam` (which rejects non-positive/malformed ids, `internal/api/errors.go:109`), and route every service error through `writeServiceError`, which has an explicit `default:` branch that logs and 500s rather than silently dropping unmapped errors (`internal/api/errors.go:56-59`). - The `region`/`rect` XOR validation and the `degenerate()` zero/inverted-rect check are exercised by `TestFillRegionSelectionAPI`, including the `"rect": {}` all-zeros decode case — I confirmed `fillRect{}` (JSON `{}`) does decode to a non-nil pointer with zero fields, so `degenerate()` correctly catches it. - The service layer the new routes call (`fillLoaded`, `hexCenters`, `ClearObject`) already has deliberate, well-commented guards for non-finite regions/spacing, `maxFillPlops` overflow protection computed *before* allocating, and a documented decision to treat a post-clear history-write failure as a logged-but-successful clear rather than reporting a false failure to the caller (`internal/service/ops.go:140-150, 253-255, 354-362`). None of this is modified by the diff, and the new handlers don't bypass or duplicate it. - `useClearObject` (`web/src/lib/objects.ts:328-352`) no longer swallows partial failures (the old `Promise.allSettled` loop is gone); a thrown error from `api.post` or a `clearResultSchema.parse` mismatch propagates to React Query's `onError`, which is still wired to a toast, and `onSettled` still invalidates the cache in both outcomes so the UI can't go stale on a failure. - `ClearBedModal`/`GardenEditorPage` edge cases (`focusedPlops` defaults to `[]`, `plopCount === 0` still disables the button) are unchanged in spirit from before and don't introduce a new null/empty-collection hazard. Nothing in this diff ignores an error, skips cleanup, or introduces a new unhandled boundary condition. Outside my lens: none worth flagging (nothing severe I noticed elsewhere). </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found · ⚠️ 4/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 3: all chain targets failed ollama-cloud/glm-5.2:cloud: ollama-cloud/glm-5.2:cloud: HTTP 429: too many concurrent requests </details> <details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 4: all chain targets failed ollama-cloud/glm-5.2:cloud: ollama-cloud/glm-5.2:cloud: HTTP 429: too many concurrent requests </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 3: all chain targets failed ollama-cloud/glm-5.2:cloud: ollama-cloud/glm-5.2:cloud: HTTP 429: too many concurrent requests </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I've verified the service methods and store layer. This PR's performance impact is straightforwardly positive — let me confirm the key points. The new `fillObject`/`clearObject` handlers are thin adapters over pre-existing service methods (`FillRegion`/`FillNamedRegion`/`ClearObject`), which I read in `internal/service/ops.go`. The geometry, batching, and one-change-set logic are all unchanged from the agent path: - `fillLoaded` (ops.go:120) caps at `maxFillPlops = 5000`, builds the lattice with a count-before-allocate optimization in `hexCenters` (ops.go:224), and inserts the batch in a single transaction via `CreatePlantings` (plantings.go:240) — one commit, not one round-trip per plop. - `ClearObject` (ops.go:331) snapshots active rows once, issues a single bulk `UPDATE ... id IN (...)` (plantings.go:174), and re-reads once to build revisions. No per-row queries. The frontend change is the actual performance story, and it's a strict win: `useClearObject` (objects.ts) went from `Promise.allSettled` over N `PATCH /plantings/:id` requests (N network round-trips, N change-set writes, N version-guard conflicts possible) to a single `POST /objects/:id/clear`. Fewer requests, one transaction, one change set, no partial-failure reconciliation loop. Nothing the diff *introduces* is a regression. The only pre-existing hot-path concern in scope — `coveredByExisting` (ops.go:302) is an O(centers × existing) nested loop that grows further because `existing` is appended to during the fill (ops.go:170), making a maxed fill O(N²) in hypot calls — lives entirely in the unchanged service code, not in this PR's diff, so it isn't this change's regression to flag. **Verdict: No material issues found** </details> <details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 1: all chain targets failed ollama-cloud/glm-5.2:cloud: ollama-cloud/glm-5.2:cloud: HTTP 429: too many concurrent requests </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found · ⚠️ 4/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed ollama-cloud/kimi-k2.6:cloud: skipped (backed off until 02:18:18.304) </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** I verified the new `fillObject` and `clearObject` handlers in `internal/api/ops.go`, traced their calls through to the existing service methods (`FillRegion`, `FillNamedRegion`, `ClearObject`), and checked the router setup in `internal/api/api.go`. The geometry in `NamedRegion` (NE = `rect(0, -hh, hw, 0)`) matches exactly what the rect-based test in `TestFillRegionSelectionAPI` asserts (x ≥ 0, y ≤ 0 for NE). The `degenerate()` check correctly rejects zero-area and inverted rects with `<=` on both axes. The one-change-set guarantee is both implemented in the service layer (`ClearObject` snapshots then bulk-updates by IDs) and pinned by the regression test `TestClearObjectIsOneChangeSetAPI`. Both endpoints are behind the existing `requireAuth` middleware and inherit the service's role-based permissions (`roleEditor`), so the 403/404/401 distinctions are handled by the existing `objectForRole` flow. No constants, thresholds, or formulas needed independent re-derivation — they all delegate to the same service methods the agent already uses. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed ollama-cloud/kimi-k2.6:cloud: skipped (backed off until 02:18:18.304) </details> <details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed ollama-cloud/kimi-k2.6:cloud: skipped (backed off until 02:18:18.304) </details> <details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed ollama-cloud/kimi-k2.6:cloud: ollama-cloud/kimi-k2.6:cloud: HTTP 429: too many concurrent requests </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — No material issues found</summary> **Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> The service layer enforces `roleEditor` via `objectForRole` (line 109/315/332), so authz is handled. Let me verify the error mapping and the region name validation for any injection/path concerns, plus check that the rect coordinates aren't used unsafely downstream. Everything checks out cleanly through the security lens. The authz flows through the existing `objectForRole` (requiring `roleEditor`), existence masking (`ErrNotFound` for both not-found and no-access) is preserved via `writeServiceError`, region names are validated against a strict allowlist switch, rect coordinates are finite-checked and clamped server-side in `fillLoaded`, and the spacing override is bounded. One thing worth noting: in `fillObject`, when a rect is supplied the handler checks `degenerate()` in the API layer but the service's own `fillLoaded` only checks `isFinite` (not inverted-rect). This is fine here because the API layer catches it, but it's a minor defense-in-depth gap. Not material enough to flag as a security issue though — the API gate is sufficient for the only entry point that takes a rect. **VERDICT: No material issues found** - Verified that `fillObject` and `clearObject` delegate to `FillRegion`/`FillNamedRegion`/`ClearObject`, all of which call `s.objectForRole(..., roleEditor)` as their first step (`internal/service/ops.go:109, 315, 332`). The authz check happens before any geometry or DB work, so a viewer/stranger/anonymous is rejected at the same boundary the agent tools use. - Verified `ErrNotFound` maps to 404 (masking existence for no-access) and `ErrForbidden` to 403 in `writeServiceError` (`internal/api/errors.go:26-29`), matching the `TestFillClearPermissionsAPI` assertions. - Verified `region` (compass name) is validated against a fixed `switch` allowlist in `NamedRegion` (`internal/service/ops.go:70-91`), defaulting to `ErrInvalidInput` — no string interpolation, no SQL/template/path use of the name. - Verified caller-supplied rect coordinates flow into `fillLoaded`, which checks `isFinite` on each (`ops.go:147-150`) and clamps to object bounds (`ops.go:152`), and `spacingOverride` is bounded to `[minPlantSpacingCM, maxPlantSpacingCM]` (`ops.go:130`). No tainted-float reaches SQL unguarded. - No secrets/credentials touched; no new unauthenticated routes (the `GET /public/gardens/:token` addition is a docs-only change to DESIGN.md, not new code in this PR). </details> <details><summary><b>🎯 Correctness</b> — Reviewed</summary> I'll verify the service-layer methods the new handlers call, and the geometry/semantics around them. Now let me check the `api.post` without a body to confirm it doesn't send a JSON body that would fail binding. The server's `clearObject` doesn't bind JSON, so that's fine. But let me verify the `api.post` sends no content-type/body when body is undefined. Now let me verify the `fillObject` handler: the `binding:"required"` on PlantID. When `region` is empty AND `rect` is nil, binding succeeds (neither field has `binding:"required"`), and then the named/hasRect check catches it. But there's a subtle case: what about `binding:"required"` — does gin's required validator treat `0` as missing? Let me check. The `PlantID int64 ... binding:"required"` — gin's `required` validator treats `0` as missing for non-pointer types. Plant ID 0 is never a valid plant ID (auto-increment), so this is fine. But let me verify this doesn't break a legitimate test. Now let me look at the `clearObject` handler more closely. The frontend sends no body. `clearObject` doesn't call `ShouldBindJSON` — good, no EOF issue. Let me double-check the test: `TestFillClearPermissionsAPI` sends `nil` cookie for anonymous — let me verify `doJSON` with `nil` cookie gives 401 (the route is behind `requireAuth`). The permission funnel through `objectForRole` → `requireGardenRole` returns `ErrNotFound` for no role and `ErrForbidden` for visible-but-not-editor. So `writeServiceError` correctly maps these to 404/403 — the test's expectations (viewer 403, stranger 404, anonymous 401) align with actual behavior. Verified the full path. Now let me verify one subtle thing in the fill test: the NE rect in a 400×400 bed. The object's local frame origin is at center, so the object spans x∈[-200,200], y∈[-200,200]. NE = +x east, -y north. The rect `{minX: 0, minY: -200, maxX: 200, maxY: 0}` is the NE quadrant. The assertion checks `x < 0 || y > 0` — i.e. plop must have x≥0 and y≤0, which matches the NE quadrant. But note: the rect itself gets clamped to object bounds in `fillLoaded` via `clampTo`, and then `hexCenters` uses `inset` so centers stay within `[MinX+inset, MaxX-inset]`. With radius = max(1.5×20, 15) = 30, inset = max(0, 30 - 10) = 20. So centers are in x∈[20, 180], y∈[-180, -20]. All satisfy x≥0 and y≤0. ✓. The test logic is sound. Now let me double-check the `degenerate()` check ordering vs the named/hasRect XOR. The XOR check happens first (line 64-69), then degenerate inside the rect branch (line 73-76). For a request with only `rect: {}` (empty), `named=false`, `hasRect=true`, XOR passes, then `rect.degenerate()` is true (0≤0), returns 400. ✓. Let me verify the `SpacingOverrideCM` field name — the JSON tag is `spacingOverrideCm` but the field is `SpacingOverrideCM`. Go field names don't matter for JSON binding; only the tag matters. ✓. Let me verify the `api.post` with no body actually sends no body (so the server's `clearObject` which doesn't bind JSON is fine), and that it works. Already confirmed in api.ts lines 90, 100: `body !== undefined ? ... : undefined`, and content-type only set when body is defined. Good. Now let me check `ClearBedModal` callers to make sure all pass the new props correctly. Let me verify whether `plantings` (the source of `focusedPlops`) is already filtered to active plops, since `ClearBedModal` uses `plopCount` which should be the count of active plops. The clear endpoint clears active plops server-side. Confirmed: `/full` returns active plantings, so `focusedPlops` (active count) matches what the server clears. The `cleared` count should match `plopCount` displayed. ✓ Now let me check the test's claim that the second clear returns `cleared: 0`. Looking at `ClearObject` line 350-351: `if err != nil || n == 0 { return n, err }`. So when bed is empty, `before` is empty slice, `ids` is empty, `ClearObjectPlantings` returns `n=0`, returns `(0, nil)`. Status 200, body `{"cleared": 0}`. ✓ Now let me check the `record` call happens only when `n != 0`. Looking at the test `TestClearObjectIsOneChangeSetAPI`: it does a fill (creates 1 change set), then clears (should create 1 change set). Let me verify `ClearObject` records once. Line 350-352 returns early if n==0; otherwise line 358+ builds changes and calls `s.record`. So yes, one record call → one change set. ✓ Let me verify that `s.record` creates exactly one change set, not that it auto-scopes via commitScope. Let me verify the plant creation endpoint accepts the fields the test helper sends. The test helper `makeFillPlant` sends `name, category, spacingCm, color, icon` — all match. Good. Let me verify the `createGardenAPI` and object creation match the test helper's expectations. Now let me verify the object creation accepts `kind: bed, widthCm, heightCm, plantable`. All verified. Let me now look at whether there's an issue with the fill handler: when `Region` is supplied as an unknown compass name like "middle-ish", `ShouldBindJSON` succeeds (no binding required on Region), then `FillNamedRegion` → `NamedRegion` returns `domain.ErrInvalidInput`, which `writeServiceError` maps to 400. ✓ The test `TestFillRegionSelectionAPI` expects 400 for bad name — confirmed. Now let me double-check one subtle correctness concern: the `objectFillRequest` uses `binding:"required"` on PlantID which is `int64`. In gin's validator, `required` on a numeric zero value fails. Plant IDs start at 1 (auto-increment), so 0 is never valid. The test always uses real plant IDs. Fine. Let me verify the share creation path used by the permissions test (sharesPath POST with email/role). All verified and consistent. The change is a clean, thin REST adapter over already-tested service methods. Through the correctness lens, I traced every path: - `fillObject`: parse ID, bind (plantId required), XOR check region/rect, degenerate rect check, dispatch to `FillRegion`/`FillNamedRegion`, 200 on success. Errors flow through `writeServiceError` → correct status codes (404 stranger, 403 viewer, 400 invalid input, 401 anonymous via `requireAuth`). - `clearObject`: parse ID, call `ClearObject`, 200 with count. Empty bed returns `(0, nil)` → 200 `{"cleared":0}`. - One-change-set guarantee: `ClearObject` calls `s.record` exactly once (after the `n==0` early return), auto-scoping one change set. `FillRegion` likewise calls `s.record` once with all created plops. - Frontend `useClearObject`: single POST, validates `cleared` count, invalidates queries. `api.post` with no body sends no content-type/body — server's `clearObject` doesn't bind JSON, so no EOF issue. - `ClearBedModal` props updated consistently at the call site; `plopCount` derived from `focusedPlops` which is the active-plantings list (matches server clear semantics). - Geometry: the NE-rect test (`minX:0, minY:-200, maxX:200, maxY:0` in a 400×400 bed) asserts `x>=0 && y<=0`, which correctly describes the NE quadrant in the local frame (+x east, -y north). `clampTo` + `inset` keep centers inside, so no plop falls outside the asserted bounds. No material correctness issues found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run context deadline exceeded </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found. This PR is strictly a performance win in my lane. The frontend change replaces an N+1 client-side loop (one PATCH per plop, each its own round-trip and its own change-set write) with a single POST that hits one bulk `UPDATE` server-side — fewer requests, fewer store round-trips, fewer history writes. The new handlers are thin adapters over the existing `FillRegion`/`FillNamedRegion`/`ClearObject` service methods, which I read (`internal/service/ops.go:108-379`); the work they do is unchanged from what the agent path already did, so no new hot-path cost is introduced. Checked specifics: - `ClearObject` (`ops.go:331-379`) does a snapshot read, one bulk `UPDATE`, then a second `ListPlantingsForObject` to build revision entries. That second query is unbounded over history (it returns removed rows too), but it is pre-existing behavior, not introduced here, and runs once per clear — not per plop. - `FillRegion` is capped at `maxFillPlops = 5000` (`ops.go:23`) and counts-before-building to avoid allocating a huge lattice just to refuse it (`hexCenters`, `ops.go:224-272`). The API response serializes up to 5000 `Planting` objects — bounded, and no worse than the agent already received. - The `degenerate` rect check and the region/rect mutual-exclusion check are O(1) input validation, not hot-path concerns. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> VERDICT: No material issues found I reviewed the diff through the error-handling & edge-case lens, verifying against the checked-out code: - **`fillObject` (`internal/api/ops.go:56-98`)**: All unhappy paths handled. `parseIDParam` → 400 on bad id; `ShouldBindJSON` → 400 on malformed/missing-required; the `named == hasRect` guard catches both-neither and both; `rect.degenerate()` catches zero-area/inverted rects (including the all-zeros `{}` case) before they reach the service. Service errors flow through `writeServiceError`. The nil-rect deref is guarded inside `if rect := req.Rect; rect != nil`. ✓ - **`clearObject` (`internal/api/ops.go:105-116`)**: `parseIDParam` + `writeServiceError` cover the bad paths. The service's `ClearObject` (`internal/service/ops.go:331-379`) handles the empty-bed case (`n==0, err==nil` → returns `(0,nil)` → handler emits `{"cleared":0}`), and the narrow "history write failed after clear applied" race is deliberately swallowed and logged (`slog.Error` + return success) — consistent with the project's stated "history is best-effort, the row is already written" rule. ✓ - **`useClearObject` (`web/src/lib/objects.ts:342-354`)**: `clearResultSchema.parse` validates the response (a shape mismatch throws → `onError` → toast). `api.post` with no body sends `undefined` (no `content-type` header, no body) per `apiFetch` (`web/src/lib/api.ts:90,100`), matching the handler which doesn't read a body. `ApiError` from non-2xx propagates to `objectErrorMessage`. ✓ - **`ClearBedModal` (`web/src/editor/ClearBedModal.tsx`)**: `plopCount === 0` disables the confirm button; the server side also treats an empty bed as a no-op (200, `cleared: 0`), so a stale count from a concurrent clear can't cause a bad action. ✓ - **Numeric edge cases**: `SpacingOverrideCM` is a pointer (absent → nil → no validation; `0` → rejected by service as `< minPlantSpacingCM`); `PlantID` `binding:"required"` rejects 0/negative/non-numeric; JSON can't carry NaN/Inf into `fillRect`, and the service re-checks `isFinite` on the region anyway. No overflow path — `maxFillPlops` caps the lattice before allocation. ✓ One trivially-misleading (not material) item I noted but am not reporting as a finding: the `ShouldBindJSON` error message at `ops.go:63` says "a plantId and a region are required," but `region` is not a binding-required field — it's enforced by the subsequent `named == hasRect` check. A client sending valid JSON with only `plantId` gets the more accurate "supply exactly one of region or rect" message instead, so the misleading message only surfaces for genuinely malformed JSON / missing `plantId`. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — No material issues found</summary> **Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** - **Authn/Authz:** Both `POST /objects/:id/fill` and `POST /objects/:id/clear` are registered under the existing `/objects` group in `internal/api/api.go:118`, which carries `h.requireAuth()`. Each handler uses `mustActor(c)` and delegates immediately to `service.FillRegion` / `FillNamedRegion` / `ClearObject`, all of which call `objectForRole(..., roleEditor)`. Verified by reading `internal/service/ops.go:108-334` and `internal/service/objects.go:148`. The permission checks (viewer → 403, stranger → 404, anonymous → 401) are exercised in the new tests and match the existing project convention. - **Input validation:** The handler validates that exactly one of `region` or `rect` is supplied (`internal/api/ops.go:66-70`). Degenerate rectangles are rejected (`ops.go:82-84`). The service layer already enforces finite floats, clamps to object bounds, caps total plops at `maxFillPlops`, validates spacing overrides against `minPlantSpacingCM`/`maxPlantSpacingCM`, and rejects non-plantable objects. No new injection surface is introduced — the handler does not construct SQL or commands. - **Race condition in clear:** `ClearObject` snapshots active plop IDs before the bulk UPDATE (`internal/service/ops.go:340-347`) and passes explicit IDs to `ClearObjectPlantings`, preventing a newly-created plop from being silently removed without a revision. The history-write failure is handled by logging and returning success (`ops.go:354-362`), matching the documented `commitScope` / detached-history rule from `CLAUDE.md`. This is a deliberate safety choice, not a bug. - **CSRF:** The routes sit behind the global `csrfGuard()` (`internal/api/api.go:54`), which rejects cross-origin state-changing requests when `PANSY_BASE_URL` is set. Nothing in this diff raises a security concern. </details> <details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run context deadline exceeded </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> **Verdict:** No material issues found I read the full contents of `internal/api/ops.go`, `internal/api/ops_test.go`, `web/src/lib/objects.ts`, `web/src/editor/ClearBedModal.tsx`, and the relevant surrounding files. Through the code-cleanliness lens, the change is well-structured and consistent with existing patterns: - **Handler structure:** `fillObject` and `clearObject` in `ops.go` follow the established thin-adapter pattern (decode → call service → encode). The new `fillRect` type with its `degenerate()` method is appropriately extracted rather than inlined, matching how other request structs in `internal/api/` carry their own validation. - **Naming:** `ops.go`/`ops_test.go` mirror the service-layer naming. The `SpacingOverrideCM` field follows the unit-explicit convention used elsewhere (`SpacingCM` in plant structs, etc.). - **Frontend refactor:** `useClearObject` simplifies from a client-side `Promise.allSettled` loop to a single mutation, which is a clear net reduction in complexity. The `clearResultSchema` is a good addition for runtime validation. - **Test helpers:** `makeFillPlant` is intentionally named apart from `createPlantAPI` to avoid a merge collision, with an explicit TODO comment — the right call for a cross-branch situation. No dead code, no confusing names, no overly long functions, and no abstractions that leak. An honest "nothing in my area." </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** Through the **Performance** lens, this PR is clean. In fact, it fixes a notable efficiency regression: - **The old `useClearObject` was N+1 over the network.** A `Promise.allSettled` of per-plop PATCHes meant clearing a 40-plop bed cost 40 round-trips, 40 change-set writes, and 40 undo actions. The new single `POST /objects/:id/clear` eliminates all of that. - **The new REST adapters are thin wrappers over existing service methods** (`FillRegion`, `FillNamedRegion`, `ClearObject`) and do not introduce new allocations, extra queries, or unbounded growth. The fill path already caps plops at `maxFillPlops = 5000` and pre-counts via `hexCenters` before allocating. - **No new loops, no missing pagination, no blocking hot-path additions.** The endpoints do not stream or paginate, but they don't need to: the service layer's bounds make the payloads bounded by design. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> No material issues found Through the 🧯 **Error handling & edge cases** lens, the PR is clean. **Verified checks:** - **`fillObject`**: `parseIDParam` guards malformed IDs; `ShouldBindJSON` rejects missing `plantId`; the `named == hasRect` check correctly enforces exactly-one-of region/rect; `degenerate()` catches zero-area rects (including the all-zeros `{}` case); non-finite coordinates are rejected by the service's `isFinite` guard; spacing override is bounded by `[minPlantSpacingCM, maxPlantSpacingCM]`; `FillNamedRegion` rejects unknown region names via `ErrInvalidInput`; the fill path clamps to object bounds and legitimately returns 200 with zero creations for already-covered regions. - **`clearObject`**: id parsing is guarded; the service snapshots active plops before updating and falls back gracefully when `n == 0` (already-empty bed is a no-op, not an error); history-write failures are logged and swallowed rather than reported as a failed clear (matching the existing `record()` best-effort design); permissions flow through the existing `objectForRole` → `ErrNotFound`/`ErrForbidden` mapping. - **Frontend**: `useClearObject` validates the response with `zod` before returning; `onError` surfaces failures via `toast.error(objectErrorMessage(...))`; the mutation invalidates the cache on settled regardless of outcome. - **Tests**: `TestFillRegionSelectionAPI` covers neither/both/unknown-name/degenerate-rect; `TestClearObjectIsOneChangeSetAPI` verifies the one-change-set guarantee; `TestFillClearPermissionsAPI` pins 403 vs 404 vs 401. One theoretical race exists in the service-layer `ClearObject` history snapshot (concurrent removals between the read and the UPDATE can cause over-reporting in the recorded changes), but that is pre-existing in `internal/service/ops.go` and not introduced or worsened by this PR. </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-22 00:36:47 +00:00
Address Gadfly findings on #89
Build image / build-and-push (push) Successful in 23s
7a6f6963f9
- Reject a degenerate (zero-area or inverted) fill rect with 400 instead of
  silently planting a single plop at the object centre. An empty `"rect": {}`
  decodes to all-zeros and is caught the same way. New fillRect.degenerate()
  and a table test covering {}, zero-width, zero-height, and inverted.
- Make the rect a named type (fillRect) rather than an anonymous inline struct,
  matching the rest of internal/api, and bind the pointer to a local in the
  handler so the deref is visibly guarded rather than reading req.Rect.MinX
  under an invariant from a line above.
- ops_test: drop gid from the unpack instead of the `_ = gid` shim.
- ClearBedModal: drop the `const n = plopCount` no-op alias.

Not taken: moving createPlantAPI/makeFillPlant to a shared helper — that
consolidation spans this branch and #88 and is cleanest once both land, as
noted in the PR. Reusing an objectPlantingsPath helper: there isn't one in this
package, and the one inline use doesn't earn a new helper on its own.

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

@gadfly review

Findings addressed:

  • Degenerate rect (zero-area or inverted, including an empty "rect": {} that decodes to all-zeros) is now a 400 instead of silently planting one plop at the object centre. New fillRect.degenerate() and a table test covering {}, zero-width, zero-height, and inverted.
  • Named fillRect type instead of the anonymous inline struct, matching the rest of internal/api. The handler binds the pointer to a local so the deref is visibly guarded rather than reading req.Rect.MinX under an invariant from a line above — which also closes the nil-deref-readability finding.
  • Dropped the _ = gid shim (unpack with _) and the const n = plopCount no-op alias.

Not taken: the createPlantAPI/makeFillPlant consolidation spans this branch and #88 — cleanest once both land, noted on both. There's no existing objectPlantingsPath helper in the package, and one inline use doesn't earn a new one.

@gadfly review Findings addressed: - **Degenerate rect** (zero-area or inverted, including an empty `"rect": {}` that decodes to all-zeros) is now a **400** instead of silently planting one plop at the object centre. New `fillRect.degenerate()` and a table test covering `{}`, zero-width, zero-height, and inverted. - **Named `fillRect` type** instead of the anonymous inline struct, matching the rest of `internal/api`. The handler binds the pointer to a local so the deref is visibly guarded rather than reading `req.Rect.MinX` under an invariant from a line above — which also closes the nil-deref-readability finding. - Dropped the `_ = gid` shim (unpack with `_`) and the `const n = plopCount` no-op alias. Not taken: the `createPlantAPI`/`makeFillPlant` consolidation spans this branch and #88 — cleanest once both land, noted on both. There's no existing `objectPlantingsPath` helper in the package, and one inline use doesn't earn a new one.
Author
Owner

@gadfly review

(Re-requesting after the CI infra recovery — earlier runs died on a docker.gitea.com outage. Current head has the degenerate-rect 400, the named fillRect type, and the two style nits.)

@gadfly review (Re-requesting after the CI infra recovery — earlier runs died on a `docker.gitea.com` outage. Current head has the degenerate-rect 400, the named `fillRect` type, and the two style nits.)
gitea-actions bot reviewed 2026-07-22 01:58:40 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 4 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +44,4 @@
// means and what the agent uses; the rect is for a future drag-a-box affordance.
// Exactly one must be supplied — accepting both and silently preferring one
// would make a client bug look like a geometry bug.
type fillRequest struct {

🟡 fillRequest naming doesn't follow resourceActionRequest convention

maintainability · flagged by 1 model

  • internal/api/ops.go:47fillRequest (and its nested fillRect) should follow the established resourceActionRequest naming convention used by every other request struct in internal/api/ (e.g. gardenCreateRequest, plantCreateRequest, objectCreateRequest, journalCreateRequest). The comment says the type is named "to match the rest of internal/api", but the name itself doesn't match that pattern. Future readers have to hunt for fillRequest instead of guessing `objectFillRequest…

🪰 Gadfly · advisory

🟡 **fillRequest naming doesn't follow resourceActionRequest convention** _maintainability · flagged by 1 model_ * `internal/api/ops.go:47` — `fillRequest` (and its nested `fillRect`) should follow the established `resourceActionRequest` naming convention used by every other request struct in `internal/api/` (e.g. `gardenCreateRequest`, `plantCreateRequest`, `objectCreateRequest`, `journalCreateRequest`). The comment says the type is named "to match the rest of internal/api", but the name itself doesn't match that pattern. Future readers have to hunt for `fillRequest` instead of guessing `objectFillRequest… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +10,4 @@
func fillPath(id int64) string { return objectPath(id) + "/fill" }
func clearPath(id int64) string { return objectPath(id) + "/clear" }
// makeFillPlant creates a custom plant and returns its id.

🟡 makeFillPlant comment references non-existent file and external branch state

maintainability · flagged by 1 model

  • internal/api/ops_test.go:13-17 — The comment justifying makeFillPlant's name references a seed_lots_test.go that does not exist in this branch and an external PR (#88). Once this PR lands, the comment becomes misleading stale context for anyone reading the file. Also, the package already has createGardenAPI (verb create); makeFillPlant introduces a new make prefix for the same pattern. Suggested fix: either name it createPlantAPI to match createGardenAPI, or replace the branc…

🪰 Gadfly · advisory

🟡 **makeFillPlant comment references non-existent file and external branch state** _maintainability · flagged by 1 model_ * `internal/api/ops_test.go:13-17` — The comment justifying `makeFillPlant`'s name references a `seed_lots_test.go` that does not exist in this branch and an external PR (#88). Once this PR lands, the comment becomes misleading stale context for anyone reading the file. Also, the package already has `createGardenAPI` (verb `create`); `makeFillPlant` introduces a new `make` prefix for the same pattern. Suggested fix: either name it `createPlantAPI` to match `createGardenAPI`, or replace the branc… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +218,4 @@
if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, stranger); w.Code != http.StatusNotFound {
t.Errorf("stranger clear = %d, want 404", w.Code)
}
if w := doJSON(t, r, http.MethodPost, fillPath(objID), fillBody, nil); w.Code != http.StatusUnauthorized {

🟡 Missing anonymous clear permission test — only fill is tested, not clear

error-handling · flagged by 1 model

  • internal/api/ops_test.go:221TestFillClearPermissionsAPI tests anonymous fill (expects 401) but never tests anonymous clear. The description says "anonymous gets 401" for both operations; the test should cover both endpoints to prevent a future regression where one route loses its auth middleware. The route is behind requireAuth() in api.go, so the behaviour is correct — this is a test-coverage gap, not a runtime bug.

🪰 Gadfly · advisory

🟡 **Missing anonymous clear permission test — only fill is tested, not clear** _error-handling · flagged by 1 model_ - **`internal/api/ops_test.go:221`** — `TestFillClearPermissionsAPI` tests anonymous `fill` (expects 401) but never tests anonymous `clear`. The description says "anonymous gets 401" for both operations; the test should cover both endpoints to prevent a future regression where one route loses its auth middleware. The route is behind `requireAuth()` in `api.go`, so the behaviour is correct — this is a test-coverage gap, not a runtime bug. <sub>🪰 Gadfly · advisory</sub>
@@ -347,2 +343,2 @@
throw new Error(`${failed} of ${plops.length} plants couldn't be cleared — refresh and try again.`)
}
mutationFn: async (objectId: number): Promise<number> => {
const res = (await api.post(`/objects/${objectId}/clear`, {})) as { cleared?: number }

🟡 clearObject POST sends empty body {} instead of undefined and uses raw type cast

maintainability · flagged by 1 model

  • web/src/lib/objects.ts:344api.post(\/objects/${objectId}/clear`, {}) sends an empty JSON object body when the endpoint doesn't read a body at all. The codebase's other no-body POST (useRevertChangeSetinhistory.ts) passes undefined, which omits the body and the content-typeheader. The{}is inconsistent and slightly misleading about the API contract. Also, the response is cast withas { cleared?: number }` rather than parsed through a zod schema, which the codebase increa…

🪰 Gadfly · advisory

🟡 **clearObject POST sends empty body {} instead of undefined and uses raw type cast** _maintainability · flagged by 1 model_ * `web/src/lib/objects.ts:344` — `api.post(\`/objects/${objectId}/clear\`, {})` sends an empty JSON object body when the endpoint doesn't read a body at all. The codebase's other no-body POST (`useRevertChangeSet` in `history.ts`) passes `undefined`, which omits the body and the `content-type` header. The `{}` is inconsistent and slightly misleading about the API contract. Also, the response is cast with `as { cleared?: number }` rather than parsed through a zod schema, which the codebase increa… <sub>🪰 Gadfly · advisory</sub>
steve added 1 commit 2026-07-22 02:12:03 +00:00
Address second round of Gadfly findings on #89
Build image / build-and-push (push) Successful in 16s
33e048cea9
- Rename fillRequest → objectFillRequest to match the package's
  <resource><Action>Request convention (objectCreateRequest, gardenCreateRequest…).
- Add the missing anonymous-clear permission case (401), mirroring anonymous fill.
- Reword the makeFillPlant comment to stop referencing external branch state,
  which won't make sense once merged — just note the consolidation follow-up.
- clearObject: send no body (undefined) rather than an empty {}, and validate
  the {cleared} response with a zod schema instead of a raw type cast.

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

@gadfly review

All four taken: fillRequestobjectFillRequest (convention), added the anonymous-clear 401 case, reworded the branch-referencing makeFillPlant comment, and clearObject now sends no body and validates the response with a zod schema instead of a raw cast.

@gadfly review All four taken: `fillRequest`→`objectFillRequest` (convention), added the anonymous-clear 401 case, reworded the branch-referencing `makeFillPlant` comment, and `clearObject` now sends no body and validates the response with a zod schema instead of a raw cast.
steve merged commit 15734c9195 into main 2026-07-22 02:53:28 +00:00
Sign in to join this conversation.