Closes#19. Part of epic #20 (Phase 8 — Agent seam). The final v1 issue.
What
Bulk service operations — internal/service/ops.go
All on *Service, so they funnel through objectForRole/requireGardenRole and inherit ACL enforcement (a viewer's FillRegion → ErrForbidden). Geometry is in each object's local frame (origin at center, +x east, +y south, so −y is north).
Region + NamedRegion — resolves nw|ne|sw|se (corners), north|south|east|west and top|bottom|left|right (halves), and all; trailing "corner"/"half" ignored.
FillRegion — hex-packs plops at 2×radius pitch (radius from #15's max(1.5·spacing, 15cm)), clipped to the region, skipping any candidate that would sit entirely inside an existing active plop (idempotent-ish re-fills). FillNamedRegion is the agent-friendly by-name form. Returns what it created.
ClearObject — soft-removes all active plops in one UPDATE; returns the count.
DescribeGarden — structured summary (dims, objects + version, plantings with plant / effective count / rough compass location) for prompting.
doc.go (untagged) keeps the package in the default build; tools.go is behind the majordomo build tag and majordomo is not in go.mod, so go build/test ./... and the server binary carry no majordomo/LLM deps — the acceptance's "core server still builds without majordomo". Built + tested locally against real majordomo (go test -tags majordomo ./internal/agent/).
NewToolbox(svc, actorID) registers list_gardens, describe_garden, create_object, move_object, place_planting, fill_region, clear_object as llm.DefineTool[Args] wrappers — thin typed adapters over the service, each running as the bound actor.
Tests
NamedRegion for every name (+ unknown → ErrInvalidInput); defaultPlopRadius.
Deterministic packing: a 60×60 bed with a spacing-10 plant → exactly 4 plops; a re-fill of the same region creates 0 (all covered).
Rotated bed fills the correct local NE corner (x ≥ 0, y ≤ 0) regardless of rotation.
ClearObject; viewer → ErrForbidden on fill/clear but can DescribeGarden.
The DESIGN scenario (TestFillScenario): garlic NE + basil NW + beans south → DescribeGarden shows three groups in the right locations.
Tagged demo (TestToolboxScenario, -tags majordomo): drives the same scenario through the toolbox (JSON args → tool → service) and confirms a viewer is refused.
GOWORK=off go build/vet/test ./internal/... green (majordomo-free).
Acceptance criteria
✅ The scenario executes (fill NE garlic / NW basil / south beans) → DescribeGarden shows three distinct groups in the right places, counts consistent with spacing.
✅FillRegion on a rotated bed still fills the correct local-frame corner.
✅ A viewer calling fill_region gets ErrForbidden; unit tests cover NamedRegion for all names + deterministic packing.
✅go test ./... green; the core server builds without majordomo (build-tag separation kept).
Notes / choices
The optional mort-style HTTP agent surface is not included — the issue marks it optional and direct package import (NewToolbox) is the clean path; adding it later is straightforward.
The demo is the integration test form (the issue accepts "or an integration test") rather than a live-model cmd/pansy-agent-demo, since a deterministic test can't drive a real LLM loop.
Closes #19. Part of epic #20 (Phase 8 — Agent seam). The final v1 issue.
## What
### Bulk service operations — `internal/service/ops.go`
All on `*Service`, so they funnel through `objectForRole`/`requireGardenRole` and inherit ACL enforcement (a viewer's `FillRegion` → `ErrForbidden`). Geometry is in each object's **local frame** (origin at center, +x east, +y south, so **−y is north**).
- **`Region` + `NamedRegion`** — resolves `nw|ne|sw|se` (corners), `north|south|east|west` and `top|bottom|left|right` (halves), and `all`; trailing "corner"/"half" ignored.
- **`FillRegion`** — hex-packs plops at 2×radius pitch (radius from #15's `max(1.5·spacing, 15cm)`), clipped to the region, **skipping any candidate that would sit entirely inside an existing active plop** (idempotent-ish re-fills). `FillNamedRegion` is the agent-friendly by-name form. Returns what it created.
- **`ClearObject`** — soft-removes all active plops in one UPDATE; returns the count.
- **`DescribeGarden`** — structured summary (dims, objects + version, plantings with plant / effective count / rough compass location) for prompting.
- store: `ListActivePlantingsForObject`, `ClearObjectPlantings`.
### Agent toolbox — `internal/agent` (deliberately isolated)
- `doc.go` (untagged) keeps the package in the default build; **`tools.go` is behind the `majordomo` build tag and majordomo is *not* in `go.mod`**, so `go build/test ./...` and the **server binary carry no majordomo/LLM deps** — the acceptance's "core server still builds without majordomo". Built + tested locally against real majordomo (`go test -tags majordomo ./internal/agent/`).
- `NewToolbox(svc, actorID)` registers `list_gardens`, `describe_garden`, `create_object`, `move_object`, `place_planting`, `fill_region`, `clear_object` as `llm.DefineTool[Args]` wrappers — thin typed adapters over the service, each running as the bound actor.
## Tests
- `NamedRegion` for every name (+ unknown → `ErrInvalidInput`); `defaultPlopRadius`.
- **Deterministic packing**: a 60×60 bed with a spacing-10 plant → exactly 4 plops; a re-fill of the same region creates 0 (all covered).
- **Rotated bed** fills the correct *local* NE corner (x ≥ 0, y ≤ 0) regardless of rotation.
- `ClearObject`; **viewer → `ErrForbidden`** on fill/clear but can `DescribeGarden`.
- **The DESIGN scenario** (`TestFillScenario`): garlic NE + basil NW + beans south → `DescribeGarden` shows three groups in the right locations.
- Tagged demo (`TestToolboxScenario`, `-tags majordomo`): drives the same scenario **through the toolbox** (JSON args → tool → service) and confirms a viewer is refused.
`GOWORK=off go build/vet/test ./internal/...` green (majordomo-free).
## Acceptance criteria
- ✅ The scenario executes (fill NE garlic / NW basil / south beans) → `DescribeGarden` shows three distinct groups in the right places, counts consistent with spacing.
- ✅ `FillRegion` on a rotated bed still fills the correct local-frame corner.
- ✅ A viewer calling `fill_region` gets `ErrForbidden`; unit tests cover `NamedRegion` for all names + deterministic packing.
- ✅ `go test ./...` green; the core server builds without majordomo (build-tag separation kept).
## Notes / choices
- The optional mort-style HTTP agent surface is **not** included — the issue marks it optional and direct package import (`NewToolbox`) is the clean path; adding it later is straightforward.
- The demo is the integration test form (the issue accepts "or an integration test") rather than a live-model `cmd/pansy-agent-demo`, since a deterministic test can't drive a real LLM loop.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Bulk, natural-language-shaped service operations (ACL-enforced like every other
op, so agent tools inherit permissions for free):
- ops.go: Region + NamedRegion (nw/ne/sw/se corners, north/south/east/west and
top/bottom/left/right halves, "all"; -y is north in the local frame).
FillRegion hex-packs plops at 2×radius pitch (radius = #15's max(1.5·spacing,
15cm)) clipped to the region, skipping any candidate that would sit entirely
inside an existing active plop. ClearObject soft-removes all active plops in one
UPDATE. DescribeGarden returns a structured summary (dims, objects+version,
plantings with plant/effective-count/rough compass location).
- store/plantings.go: ListActivePlantingsForObject + ClearObjectPlantings.
Agent toolbox (internal/agent), deliberately isolated:
- doc.go (untagged) keeps the package in the default build; tools.go is behind
the `majordomo` build tag and NOT in go.mod, so `go build/test ./...` and the
server binary carry no majordomo/LLM deps. Built + tested locally against real
majordomo (go test -tags majordomo ./internal/agent/).
- NewToolbox(svc, actorID) exposes list_gardens, describe_garden, create_object,
move_object, place_planting, fill_region, clear_object as llm.DefineTool
wrappers — thin typed adapters over the service, each running as the bound
actor.
Tests: NamedRegion for all names + unknown→ErrInvalidInput; deterministic
hex-packing count (60×60 bed → 4 plops) + re-fill skips covered; rotated bed
fills the correct LOCAL corner; ClearObject; viewer→ErrForbidden on fill/clear
but can describe; and the DESIGN corner/half scenario (garlic NE, basil NW, beans
south) verified via DescribeGarden. A tagged demo drives the same scenario
through the toolbox (JSON args → tool → service) and confirms a viewer is refused.
GOWORK=off go build/vet/test ./internal/... green (majordomo-free).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
🪰Gadfly consensus review — 20 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** — 20 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
error-handling, maintainability · flagged by 2 models
internal/agent/tools_test.go:54-56 — Swallowed errors in test setupCreatePlant returns (*domain.Plant, error) but the test discards the error with _, _ for garlic, basil, and beans. If plant creation fails, the test will later fail with a confusing downstream error (e.g. plantId: 0 rejected) instead of surfacing the root cause. Check the errors like the rest of the test body and like ops_test.go's seedNamedPlant.
🪰 Gadfly · advisory
🟡 **Swallowed CreatePlant errors in test setup**
_error-handling, maintainability · flagged by 2 models_
- **`internal/agent/tools_test.go:54-56` — Swallowed errors in test setup** `CreatePlant` returns `(*domain.Plant, error)` but the test discards the error with `_, _` for garlic, basil, and beans. If plant creation fails, the test will later fail with a confusing downstream error (e.g. `plantId: 0` rejected) instead of surfacing the root cause. Check the errors like the rest of the test body and like `ops_test.go`'s `seedNamedPlant`.
<sub>🪰 Gadfly · advisory</sub>
🟡String literal role instead of domain.RoleViewer constant
maintainability · flagged by 1 model
internal/agent/tools_test.go:118 — String literal role "viewer" instead of constant The rest of the service tests (e.g. ops_test.go:171) use domain.RoleViewer. Using a raw string breaks the project's established pattern and is brittle if the canonical value changes. Import internal/domain and use domain.RoleViewer.
🪰 Gadfly · advisory
🟡 **String literal role instead of domain.RoleViewer constant**
_maintainability · flagged by 1 model_
- **`internal/agent/tools_test.go:118` — String literal role `"viewer"` instead of constant** The rest of the service tests (e.g. `ops_test.go:171`) use `domain.RoleViewer`. Using a raw string breaks the project's established pattern and is brittle if the canonical value changes. Import `internal/domain` and use `domain.RoleViewer`.
<sub>🪰 Gadfly · advisory</sub>
⚪mustJSON helper swallows marshal errors and lacks t.Helper()
maintainability · flagged by 1 model
internal/agent/tools_test.go:130 — mustJSON swallows errors and lacks t.Helper(). The helper ignores json.Marshal errors and doesn’t mark itself with t.Helper(), so a failure would print the wrong line number. It should panic on error and call t.Helper() for clearer test output.
🪰 Gadfly · advisory
⚪ **mustJSON helper swallows marshal errors and lacks t.Helper()**
_maintainability · flagged by 1 model_
- **`internal/agent/tools_test.go:130` — `mustJSON` swallows errors and lacks `t.Helper()`.** The helper ignores `json.Marshal` errors and doesn’t mark itself with `t.Helper()`, so a failure would print the wrong line number. It should panic on error and call `t.Helper()` for clearer test output.
<sub>🪰 Gadfly · advisory</sub>
🟡Region.Circle/CX/CY/Radius fields and contains circle branch are dead/speculative code never produced by NamedRegion and unsupported by hexCenters
maintainability · flagged by 4 models
internal/service/ops.go:20-24 — Region.Circle/CX/CY/Radius are dead/speculative fields.NamedRegion (the only Region constructor in the package) always returns rectangles via rect, which never sets Circle. A repo-wide grep for Circle =/Circle:/Circle: true finds no assignment anywhere — Circle is only ever read in the r.Circle branch of Region.contains (ops.go:27-33), which is therefore unreachable from any production path. hexCenters (ops.go:144-166) also d…
🪰 Gadfly · advisory
🟡 **Region.Circle/CX/CY/Radius fields and contains circle branch are dead/speculative code never produced by NamedRegion and unsupported by hexCenters**
_maintainability · flagged by 4 models_
- **`internal/service/ops.go:20-24` — `Region.Circle`/`CX`/`CY`/`Radius` are dead/speculative fields.** `NamedRegion` (the only `Region` constructor in the package) always returns rectangles via `rect`, which never sets `Circle`. A repo-wide grep for `Circle =`/`Circle:`/`Circle: true` finds no assignment anywhere — `Circle` is only ever read in the `r.Circle` branch of `Region.contains` (ops.go:27-33), which is therefore unreachable from any production path. `hexCenters` (ops.go:144-166) also d…
<sub>🪰 Gadfly · advisory</sub>
🔴NamedRegion nil pointer dereference on nil GardenObject
correctness, error-handling · flagged by 2 models
internal/service/ops.go:48-49 (NamedRegion) — the literal strings "corner" or "half" alone silently resolve to "all" instead of being rejected.TrimSuffix doesn't require a preceding separator: input "corner" → strip suffix "corner" → "", which matches the case "all", "": branch, filling the entire object rather than erroring. Same for "half". Low real-world likelihood (the toolbox description enumerates only compass-qualified values), but it's a genuine mismatch betw…
🪰 Gadfly · advisory
🔴 **NamedRegion nil pointer dereference on nil GardenObject**
_correctness, error-handling · flagged by 2 models_
- **`internal/service/ops.go:48-49` (`NamedRegion`) — the literal strings `"corner"` or `"half"` alone silently resolve to `"all"` instead of being rejected.** `TrimSuffix` doesn't require a preceding separator: input `"corner"` → strip suffix `"corner"` → `""`, which matches the `case "all", "":` branch, filling the entire object rather than erroring. Same for `"half"`. Low real-world likelihood (the toolbox description enumerates only compass-qualified values), but it's a genuine mismatch betw…
<sub>🪰 Gadfly · advisory</sub>
🟡Empty/blank region name silently maps to "all" instead of returning ErrInvalidInput, contradicting the doc and surprising bad-input handling
error-handling, security · flagged by 2 models
internal/service/ops.go:52 — empty/blank region name silently maps to "all" instead of ErrInvalidInput. The doc comment (line 44) states "Unknown names return ErrInvalidInput," but case "all", "": (line 52) accepts an empty string (after trim) as the whole-object rect. Through the agent tool this means fill_region with region: "" (or whitespace) fills the entire object rather than being rejected — a surprising edge case for bad input. Suggested fix: drop "" from the all cas…
🪰 Gadfly · advisory
🟡 **Empty/blank region name silently maps to "all" instead of returning ErrInvalidInput, contradicting the doc and surprising bad-input handling**
_error-handling, security · flagged by 2 models_
- **`internal/service/ops.go:52` — empty/blank region name silently maps to `"all"` instead of `ErrInvalidInput`.** The doc comment (line 44) states "Unknown names return ErrInvalidInput," but `case "all", "":` (line 52) accepts an empty string (after trim) as the whole-object rect. Through the agent tool this means `fill_region` with `region: ""` (or whitespace) fills the entire object rather than being rejected — a surprising edge case for bad input. Suggested fix: drop `""` from the `all` cas…
<sub>🪰 Gadfly · advisory</sub>
⚪defaultPlopRadius comment says 'Reused here (don't duplicate the constant elsewhere)' but this is the only definition in the codebase — misleading phrasing
maintainability · flagged by 1 model
internal/service/ops.go:75-79 — misleading "Reused here" comment on defaultPlopRadius. The comment says "defaultPlopRadius is #15's formula... Reused here (don't duplicate the constant elsewhere)." But this is the only definition of the function in the codebase (grep for defaultPlopRadius returns exactly this definition plus its one call site and two tests). The phrasing reads as if it's being imported/duplicated from #15's code, when it's actually the canonical definition. Minor; th…
🪰 Gadfly · advisory
⚪ **defaultPlopRadius comment says 'Reused here (don't duplicate the constant elsewhere)' but this is the only definition in the codebase — misleading phrasing**
_maintainability · flagged by 1 model_
- **`internal/service/ops.go:75-79` — misleading "Reused here" comment on `defaultPlopRadius`.** The comment says "defaultPlopRadius is #15's formula... Reused here (don't duplicate the constant elsewhere)." But this is the only definition of the function in the codebase (grep for `defaultPlopRadius` returns exactly this definition plus its one call site and two tests). The phrasing reads as if it's being imported/duplicated from #15's code, when it's actually the canonical definition. Minor; th…
<sub>🪰 Gadfly · advisory</sub>
🟠FillRegion silent empty fill when computed radius is NaN/Inf
error-handling · flagged by 2 models
internal/service/ops.go:108-134 — FillRegion's read-then-write is unguarded, so concurrent fills on the same object can create duplicate/overlapping plops.FillRegion loads existing plantings once via ListActivePlantingsForObject (line 108), computes hex-lattice candidates against that snapshot, then inserts one row at a time (line 125) with no transaction, row lock, or version check tying the read to the writes. By contrast, UpdateObject (internal/service/objects.go:136-149)…
🪰 Gadfly · advisory
🟠 **FillRegion silent empty fill when computed radius is NaN/Inf**
_error-handling · flagged by 2 models_
- **`internal/service/ops.go:108-134` — `FillRegion`'s read-then-write is unguarded, so concurrent fills on the same object can create duplicate/overlapping plops.** `FillRegion` loads `existing` plantings once via `ListActivePlantingsForObject` (line 108), computes hex-lattice candidates against that snapshot, then inserts one row at a time (line 125) with no transaction, row lock, or version check tying the read to the writes. By contrast, `UpdateObject` (`internal/service/objects.go:136-149`)…
<sub>🪰 Gadfly · advisory</sub>
🔴FillRegion performs one INSERT per lattice candidate (N+1 writes); worst case ~10^5 sequential round-trips on a max-sized bed with min spacing
correctness, error-handling, performance, security · flagged by 5 models
internal/service/ops.go:116-134 — FillRegion issues one INSERT per candidate with no batching (N+1 writes). Each surviving lattice point calls s.store.CreatePlanting individually — a separate INSERT … RETURNING round-trip to SQLite (verified at store/plantings.go:122). There is no bulk insert helper anywhere in internal/store (grep for BatchInsert|BulkCreate|batch returns nothing). Objects are bounded only by validDimensionCM = maxGardenCM = 10_000 cm (gardens.go:18, ap…
🪰 Gadfly · advisory
🔴 **FillRegion performs one INSERT per lattice candidate (N+1 writes); worst case ~10^5 sequential round-trips on a max-sized bed with min spacing**
_correctness, error-handling, performance, security · flagged by 5 models_
- **`internal/service/ops.go:116-134` — `FillRegion` issues one INSERT per candidate with no batching (N+1 writes).** Each surviving lattice point calls `s.store.CreatePlanting` individually — a separate `INSERT … RETURNING` round-trip to SQLite (verified at `store/plantings.go:122`). There is no bulk insert helper anywhere in `internal/store` (grep for `BatchInsert|BulkCreate|batch` returns nothing). Objects are bounded only by `validDimensionCM` = `maxGardenCM = 10_000` cm (`gardens.go:18`, ap…
<sub>🪰 Gadfly · advisory</sub>
⚪localPoint is a one-off struct with no reuse; minor parallel abstraction over plain (x,y) coords
maintainability · flagged by 1 model
internal/service/ops.go:138 — localPoint is a one-off internal struct defined separately from Region and used only by hexCenters/FillRegion. Since Region.contains already takes (x, y float64), localPoint adds a small parallel abstraction with no reuse elsewhere (grep confirms no other references). A plain (x, y float64) return would be one fewer type to track. Trivial; leave if you prefer the named clarity.
🪰 Gadfly · advisory
⚪ **localPoint is a one-off struct with no reuse; minor parallel abstraction over plain (x,y) coords**
_maintainability · flagged by 1 model_
- **`internal/service/ops.go:138` — `localPoint` is a one-off internal struct** defined separately from `Region` and used only by `hexCenters`/`FillRegion`. Since `Region.contains` already takes `(x, y float64)`, `localPoint` adds a small parallel abstraction with no reuse elsewhere (grep confirms no other references). A plain `(x, y float64)` return would be one fewer type to track. Trivial; leave if you prefer the named clarity.
<sub>🪰 Gadfly · advisory</sub>
🟡hexCenters generates the full lattice before the bounds guard filters it; an oversized caller-supplied Region (not clamped to the object) can cause a very long loop / large allocation
error-handling · flagged by 1 model
hexCenters can loop a very long time on an oversized caller-supplied Region — internal/service/ops.go:144-166. FillRegion takes a caller-supplied Region that is never validated to lie within the object or clamped to [-halfW,halfW]×[-halfH,halfH]. hexCenters generates the full lattice before the math.Abs(c.x) > halfW guard at ops.go:118 filters each point, so a region like MinX=-1e9, MaxX=1e9 with a tiny radius would generate a huge lattice only to discard every point.…
🪰 Gadfly · advisory
🟡 **hexCenters generates the full lattice before the bounds guard filters it; an oversized caller-supplied Region (not clamped to the object) can cause a very long loop / large allocation**
_error-handling · flagged by 1 model_
- **`hexCenters` can loop a very long time on an oversized caller-supplied `Region`** — `internal/service/ops.go:144-166`. `FillRegion` takes a caller-supplied `Region` that is never validated to lie within the object or clamped to `[-halfW,halfW]×[-halfH,halfH]`. `hexCenters` generates the full lattice *before* the `math.Abs(c.x) > halfW` guard at `ops.go:118` filters each point, so a region like `MinX=-1e9, MaxX=1e9` with a tiny radius would generate a huge lattice only to discard every point.…
<sub>🪰 Gadfly · advisory</sub>
🟠coveredByExisting is O(N) linear scan making fill O(M*N)
performance · flagged by 1 model
internal/service/ops.go:170 — coveredByExisting is an O(N) linear scan, making the fill O(M·N). For every lattice candidate, the code iterates over all existing plops (and appends newly created ones to the same slice, so the scan grows as the fill proceeds). Impact: Coverage checking degrades quadratically. A fill that creates 100 plops performs ~5 000 distance checks; a 500-plop fill performs ~125 000. Fix: Partition existing into a coarse spatial grid (e.g. keyed by `floo…
🪰 Gadfly · advisory
🟠 **coveredByExisting is O(N) linear scan making fill O(M*N)**
_performance · flagged by 1 model_
* **`internal/service/ops.go:170` — `coveredByExisting` is an O(N) linear scan, making the fill O(M·N).** For every lattice candidate, the code iterates over *all* existing plops (and appends newly created ones to the same slice, so the scan grows as the fill proceeds). **Impact:** Coverage checking degrades quadratically. A fill that creates 100 plops performs ~5 000 distance checks; a 500-plop fill performs ~125 000. **Fix:** Partition `existing` into a coarse spatial grid (e.g. keyed by `floo…
<sub>🪰 Gadfly · advisory</sub>
🟠FillNamedRegion calls objectForRole twice (once itself, once via FillRegion)
correctness, error-handling, maintainability, performance · flagged by 5 models
internal/service/ops.go:182-191 — Redundant objectForRole call in FillNamedRegion.FillNamedRegion fetches the object via objectForRole, resolves the region, then delegates to FillRegion, which calls objectForRoleagain. This doubles the same permission/DB work for every named-region fill. Clean fix: extract a private fillRegion that accepts the already-resolved *domain.GardenObject, so FillNamedRegion fetches once and FillRegion can still enforce its own ACL withou…
🪰 Gadfly · advisory
🟠 **FillNamedRegion calls objectForRole twice (once itself, once via FillRegion)**
_correctness, error-handling, maintainability, performance · flagged by 5 models_
- **`internal/service/ops.go:182-191` — Redundant `objectForRole` call in `FillNamedRegion`.** `FillNamedRegion` fetches the object via `objectForRole`, resolves the region, then delegates to `FillRegion`, which calls `objectForRole` *again*. This doubles the same permission/DB work for every named-region fill. Clean fix: extract a private `fillRegion` that accepts the already-resolved `*domain.GardenObject`, so `FillNamedRegion` fetches once and `FillRegion` can still enforce its own ACL withou…
<sub>🪰 Gadfly · advisory</sub>
🟡ClearObject inconsistent Plantable check compared to FillRegion
correctness, error-handling · flagged by 2 models
internal/service/ops.go:196-202 — ClearObject does not check o.Plantable.FillRegion rejects non-plantable objects (trees, paths, etc.) with ErrInvalidInput, but ClearObject allows the call and simply clears 0 rows. This is harmless but inconsistent. Fix: either return ErrInvalidInput for non-plantable objects, or document that clearing them is a no-op.
🪰 Gadfly · advisory
🟡 **ClearObject inconsistent Plantable check compared to FillRegion**
_correctness, error-handling · flagged by 2 models_
- **`internal/service/ops.go:196-202` — `ClearObject` does not check `o.Plantable`.** `FillRegion` rejects non-plantable objects (`trees`, `paths`, etc.) with `ErrInvalidInput`, but `ClearObject` allows the call and simply clears 0 rows. This is harmless but inconsistent. *Fix:* either return `ErrInvalidInput` for non-plantable objects, or document that clearing them is a no-op.
<sub>🪰 Gadfly · advisory</sub>
🟡DescribeGarden emits an empty plant name when a planting's plant is missing from the (independently-read) referenced-plants map
error-handling · flagged by 1 model
DescribeGarden emits an empty plant name when a planting's plant isn't in plantByID — internal/service/ops.go:280 (Plant: plantByID[pl.PlantID].Name). full.Plants comes from ListReferencedPlants and full.Plantings from ListActivePlantingsForGarden (objects.go:171-178); both filter removed_at IS NULL (plants.go:36, plantings.go:36) but are independent reads with no transaction. If a planting is soft-removed between the two reads, it appears in full.Plantings while…
🪰 Gadfly · advisory
🟡 **DescribeGarden emits an empty plant name when a planting's plant is missing from the (independently-read) referenced-plants map**
_error-handling · flagged by 1 model_
- **`DescribeGarden` emits an empty plant name when a planting's plant isn't in `plantByID`** — `internal/service/ops.go:280` (`Plant: plantByID[pl.PlantID].Name`). `full.Plants` comes from `ListReferencedPlants` and `full.Plantings` from `ListActivePlantingsForGarden` (`objects.go:171-178`); both filter `removed_at IS NULL` (`plants.go:36`, `plantings.go:36`) but are independent reads with no transaction. If a planting is soft-removed between the two reads, it appears in `full.Plantings` while…
<sub>🪰 Gadfly · advisory</sub>
⚪describeLocation's final default silently maps to 'west'; an explicit ew=="W" case would make the switch's exhaustiveness obvious
maintainability · flagged by 1 model
internal/service/ops.go:293-323 — describeLocation's final switch is slightly leaky. The function builds ns/ew strings then switches on their specific values (case ns == "N", case ns == "S", case ew == "E", default: return "west"). It works, but the trailing default: return "west" silently encodes "ew == W" — a future edit that adds a fourth ns/ew state would get "west" by accident. Minor readability nit; an explicit case ew == "W": with a real default: that retur…
🪰 Gadfly · advisory
⚪ **describeLocation's final default silently maps to 'west'; an explicit ew=="W" case would make the switch's exhaustiveness obvious**
_maintainability · flagged by 1 model_
- **`internal/service/ops.go:293-323` — `describeLocation`'s final switch is slightly leaky.** The function builds `ns`/`ew` strings then switches on their specific values (`case ns == "N"`, `case ns == "S"`, `case ew == "E"`, `default: return "west"`). It works, but the trailing `default: return "west"` silently encodes "ew == W" — a future edit that adds a fourth `ns`/`ew` state would get "west" by accident. Minor readability nit; an explicit `case ew == "W":` with a real `default:` that retur…
<sub>🪰 Gadfly · advisory</sub>
🟡Duplicate seedFillBed helper; seedBed already exists in same package
maintainability · flagged by 1 model
internal/service/ops_test.go:56 / internal/service/plantings_test.go:12 — Duplicate test helper seedFillBed vs seedBed. Same package, same purpose (create a plantable bed), only the dimensions/position differ. Parameterize the existing seedBed helper instead of adding a second one.
🪰 Gadfly · advisory
🟡 **Duplicate seedFillBed helper; seedBed already exists in same package**
_maintainability · flagged by 1 model_
- **`internal/service/ops_test.go:56` / `internal/service/plantings_test.go:12` — Duplicate test helper `seedFillBed` vs `seedBed`.** Same package, same purpose (create a plantable bed), only the dimensions/position differ. Parameterize the existing `seedBed` helper instead of adding a second one.
<sub>🪰 Gadfly · advisory</sub>
⚪2000x2000 garden setup duplicated verbatim across 4 tests in two files instead of a shared seed helper
maintainability · flagged by 1 model
🪰 Gadfly · advisory
⚪ **2000x2000 garden setup duplicated verbatim across 4 tests in two files instead of a shared seed helper**
_maintainability · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
🟡Duplicate seedNamedPlant helper; seedOwnPlant already exists in same package
maintainability · flagged by 2 models
internal/service/ops_test.go:248 / internal/service/plantings_test.go:24 — Duplicate test helper seedNamedPlant vs seedOwnPlant. Both are in package service and do the same thing (create a plant for an owner), differing only by whether the name is parameterized. Having two helpers for the same seeding operation is unnecessary copy-paste; consolidate into one helper with an optional name parameter.
🪰 Gadfly · advisory
🟡 **Duplicate seedNamedPlant helper; seedOwnPlant already exists in same package**
_maintainability · flagged by 2 models_
- **`internal/service/ops_test.go:248` / `internal/service/plantings_test.go:24` — Duplicate test helper `seedNamedPlant` vs `seedOwnPlant`.** Both are in package `service` and do the same thing (create a plant for an owner), differing only by whether the name is parameterized. Having two helpers for the same seeding operation is unnecessary copy-paste; consolidate into one helper with an optional name parameter.
<sub>🪰 Gadfly · advisory</sub>
🟠ListActivePlantingsForObject loads unbounded rows with no composite index on (object_id, removed_at)
performance · flagged by 1 model
internal/store/plantings.go:64 — ListActivePlantingsForObject is unbounded and lacks a composite index. The query is SELECT … WHERE object_id = ? AND removed_at IS NULL. There is only a single-column index on object_id; because ClearObjectPlantings soft-removes rows (sets removed_at), an object that is cleared and refilled accumulates dead rows that are still reached by the object_id index and then filtered in-memory. There is no LIMIT. Impact: Memory usage and query ti…
🪰 Gadfly · advisory
🟠 **ListActivePlantingsForObject loads unbounded rows with no composite index on (object_id, removed_at)**
_performance · flagged by 1 model_
* **`internal/store/plantings.go:64` — `ListActivePlantingsForObject` is unbounded and lacks a composite index.** The query is `SELECT … WHERE object_id = ? AND removed_at IS NULL`. There is only a single-column index on `object_id`; because `ClearObjectPlantings` soft-removes rows (sets `removed_at`), an object that is cleared and refilled accumulates dead rows that are still reached by the `object_id` index and then filtered in-memory. There is no `LIMIT`. **Impact:** Memory usage and query ti…
<sub>🪰 Gadfly · advisory</sub>
Region.Circle/CX/CY/Radius fields and contains circle branch are dead/speculative code never produced by NamedRegion and unsupported by hexCenters
internal/service/ops.go:20
4/5
maintainability
🔴
NamedRegion nil pointer dereference on nil GardenObject
internal/service/ops.go:46
2/5
correctness, error-handling
🟠
FillRegion silent empty fill when computed radius is NaN/Inf
internal/service/ops.go:106
2/5
error-handling
🟡
Swallowed CreatePlant errors in test setup
internal/agent/tools_test.go:54
2/5
error-handling, maintainability
🟡
Empty/blank region name silently maps to "all" instead of returning ErrInvalidInput, contradicting the doc and surprising bad-input handling
internal/service/ops.go:52
2/5
error-handling, security
🟡
ClearObject inconsistent Plantable check compared to FillRegion
internal/service/ops.go:196
2/5
correctness, error-handling
🟡
Duplicate seedNamedPlant helper; seedOwnPlant already exists in same package
internal/service/ops_test.go:248
2/5
maintainability
11 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
coveredByExisting is O(N) linear scan making fill O(M*N)
internal/service/ops.go:170
opencode/kimi-k2.6:cloud
performance
🟠
ListActivePlantingsForObject loads unbounded rows with no composite index on (object_id, removed_at)
internal/store/plantings.go:64
opencode/kimi-k2.6:cloud
performance
🟡
String literal role instead of domain.RoleViewer constant
internal/agent/tools_test.go:118
opencode/kimi-k2.6:cloud
maintainability
🟡
hexCenters generates the full lattice before the bounds guard filters it; an oversized caller-supplied Region (not clamped to the object) can cause a very long loop / large allocation
internal/service/ops.go:144
opencode/glm-5.2:cloud
error-handling
🟡
DescribeGarden emits an empty plant name when a planting's plant is missing from the (independently-read) referenced-plants map
internal/service/ops.go:280
opencode/glm-5.2:cloud
error-handling
🟡
Duplicate seedFillBed helper; seedBed already exists in same package
internal/service/ops_test.go:56
kimi-k2.6:cloud
maintainability
⚪
mustJSON helper swallows marshal errors and lacks t.Helper()
internal/agent/tools_test.go:130
kimi-k2.6:cloud
maintainability
⚪
defaultPlopRadius comment says 'Reused here (don't duplicate the constant elsewhere)' but this is the only definition in the codebase — misleading phrasing
internal/service/ops.go:75
opencode/glm-5.2:cloud
maintainability
⚪
localPoint is a one-off struct with no reuse; minor parallel abstraction over plain (x,y) coords
internal/service/ops.go:138
glm-5.2:cloud
maintainability
⚪
describeLocation's final default silently maps to 'west'; an explicit ew=="W" case would make the switch's exhaustiveness obvious
internal/service/ops.go:293
opencode/glm-5.2:cloud
maintainability
⚪
2000x2000 garden setup duplicated verbatim across 4 tests in two files instead of a shared seed helper
internal/service/ops_test.go:71
claude-code/sonnet
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Blocking issues found
The draft did not contain any actual findings — it only contains meta-commentary about a missing ExitPlanMode tool and a stray reference to "the DoS finding" location without ever stating what that finding is, where it is, or its impact. There is nothing concrete here to confirm against the code (no file:line claim, no described defect), so nothing carries forward.
🎯 Correctness — Minor issues
Both findings verified directly against the code and store layer.
Finding 1 — confirmed. internal/service/plantings.go:18 sets maxRadiusCM = 10_000; internal/service/plants.go:19 sets maxPlantSpacingCM = 10_000. FillRegion (ops.go:106) computes radius := defaultPlopRadius(spacing) = max(1.5·spacing, 15), so any spacing above 10000/1.5 ≈ 6667cm (legal per maxPlantSpacingCM, and reachable directly via spacingOverrideCm which is only checked against minPlantSpacingCM/maxPlantSpacingCM at ops.go:100-104) yields a radius > maxRadiusCM. ops.go:125 calls s.store.CreatePlanting — the raw store method, not s.CreatePlanting — which I confirmed in internal/store/plantings.go:119-133 performs zero validation (its own comment: "fields already validated by the service"). So finalizePlanting's radius cap (plantings.go:235) is never applied on this path. I also confirmed finalizePlanting's radius check (plantings.go:235) sits above and outside the checkBounds gate, so it runs unconditionally on every UpdatePlanting call regardless of what's being patched — a row created this way permanently fails ErrInvalidInput on any future edit.
Finding 2 — confirmed by tracing NamedRegion (ops.go:45-73) by hand: input "corner" → TrimSuffix(key, "corner") → "" → TrimSuffix("", "half") → "", which hits case "all", "": and returns the full-object rectangle instead of erroring. Same for "half". This is a genuine behavior/intent mismatch, though real-world exposure is limited since the toolbox's fill_region description (internal/agent/tools.go) enumerates only compass-qualified values.
VERDICT: Minor issues
internal/service/ops.go:106,125 — FillRegion bypasses the maxRadiusCM invariant, allowing an unfixable-later planting.FillRegion computes radius := defaultPlopRadius(spacing) (max(1.5·spacing, 15)) and inserts via s.store.CreatePlanting directly (the raw store call), skipping finalizePlanting — the validator every other write path (CreatePlanting/UpdatePlanting in internal/service/plantings.go:101,145,235) goes through, which rejects RadiusCM > maxRadiusCM (10,000cm, internal/service/plantings.go:18). Since maxPlantSpacingCM is also 10,000cm (internal/service/plants.go:19), a plant with spacing above maxRadiusCM/1.5 ≈ 6667cm — a legal, in-bounds spacing value — produces a stored radius >10,000cm that finalizePlanting would normally reject. Worse, finalizePlanting's radius check runs unconditionally on every future UpdatePlanting call (not gated by checkBounds), so once such a row exists, any subsequent edit to that planting (even just a label change) permanently fails with ErrInvalidInput. spacingOverride makes this trivially reachable (pass spacingOverrideCm: 7000, which only hits the existing min/max spacing bounds at ops.go:101).
Fix: after computing radius, cap/validate it against maxRadiusCM before entering the loop, or route creation through finalizePlanting itself.
internal/service/ops.go:48-49 (NamedRegion) — the literal strings "corner" or "half" alone silently resolve to "all" instead of being rejected.TrimSuffix doesn't require a preceding separator: input "corner" → strip suffix "corner" → "", which matches the case "all", "": branch, filling the entire object rather than erroring. Same for "half". Low real-world likelihood (the toolbox description enumerates only compass-qualified values), but it's a genuine mismatch between intent (unknown/degenerate name → ErrInvalidInput) and behavior (silently expands scope to the whole object).
Fix: only trim the suffix when it's preceded by a space (e.g. strings.TrimSuffix(key, " corner")), or explicitly reject key == "corner" || key == "half" before the switch.
🧹 Code cleanliness & maintainability — Minor issues
Both findings independently verified against the actual source. Finding 1: objectForRole performs store.GetObject + requireGardenRole (objects.go:124-134), and it's called once in FillRegion (ops.go:88) and once more in FillNamedRegion (ops.go:183) before delegating to FillRegion, which repeats the lookup — confirmed redundant. Finding 2: the GardenInput{..., WidthCM: 2000, HeightCM: 2000} + CreateGarden pattern is duplicated verbatim in ops_test.go:71,111,194 and tools_test.go:50, while seedGarden (objects_test.go:13, default 1000×1000) already exists as the shared-helper precedent — confirmed.
VERDICT: Minor issues
internal/service/ops.go:183 — FillNamedRegion calls objectForRole to resolve the object, then immediately calls FillRegion, which calls objectForRole again for the same objectID/actorID (verified both call sites at ops.go:88 and ops.go:183, going through the same s.store.GetObject + s.requireGardenRole in objects.go:124). This is the sole caller of FillRegion from within the package that already has o in hand but throws it away. Every agent fill_region call goes through FillNamedRegion, so this redundant fetch-and-recheck is on the hot path. An unexported fillRegion(ctx, actorID, o *domain.GardenObject, region, plantID, spacingOverride) core that both FillRegion and FillNamedRegion call after their own single objectForRole would remove the duplicate authorization/DB round trip without changing either public signature.
Test setup duplication: the literal GardenInput{Name: "Big"/"Plot", WidthCM: 2000, HeightCM: 2000} + CreateGarden pair is copy-pasted across TestFillRegionDeterministicPacking (ops_test.go:71), TestFillRegionRotatedBedUsesLocalFrame (ops_test.go:111), TestFillScenario (ops_test.go:194), and again in TestToolboxScenario (internal/agent/tools_test.go:50). The package already has a seedGarden helper (internal/service/objects_test.go:13) for the default-size (1000×1000) case; a second small helper (e.g. seedBigGarden) alongside it would remove four copies of the same three-line setup.
⚡ Performance — Blocking issues found
Both findings check out against the actual code. All confirmed: ops.go:116-135, hexCenters, coveredByExisting, defaultPlopRadius, the minPlantSpacingCM/maxGardenCM/validDimensionCM constants and their reuse for objects, and CreatePlanting's unbatched single-row INSERT (plantings.go:122-133) with no transaction usage anywhere else in the service layer besides migrations.
One correction: the draft's claim that SetMaxOpenConns(1) (sqlite.go:50) "blocks all other DB access" is wrong for the reachable production path — that line only fires if memory (a bare :memory: DSN, used in tests per the doc comment at sqlite.go:37-39). Production (cmd/pansy/main.go:40) opens cfg.DBPath, a real file, so the pool is NOT pinned to one connection there; WAL mode allows concurrent readers. I dropped that clause but kept the core finding, since the unbounded O(N²) work still hangs the request itself regardless of connection pooling.
Corrected review
VERDICT: Blocking issues found
internal/service/ops.go:116-135 (FillRegion) has unbounded, quadratic cost with no cap on candidate/region size. hexCenters (ops.go:144-166) generates a candidate grid over the whole region at 2×radius pitch, and radius floors at 15cm regardless of spacing (defaultPlopRadius, ops.go:77-78, combined with minPlantSpacingCM = 0.1 in plants.go:18). Objects can be up to maxGardenCM = 10_000 cm (100m) per side (gardens.go:18, enforced for objects too via validDimensionCM at objects.go:243). Filling "all" on a 100m×100m object at the 15cm radius floor yields on the order of ~1.3×10⁵ hex candidates. For every one of those, coveredByExisting (ops.go:170-176) does a linear scan over existing, which itself grows by one for every accepted candidate (existing = append(existing, *stored) at ops.go:133) — an O(N²) scan, ~10¹⁰ operations in the worst case. This is reachable through completely normal usage (an agent told to "fill the whole bed with basil," or fill_region/"all" on any large plantable object), not just adversarial input — it will hang the handling goroutine for the request long before it produces a response. Suggest bucketing existing into a spatial grid (cell size ≈ 2×radius) so coverage checks are O(1) average instead of O(N), and/or capping total region area or candidate count with a clear ErrInvalidInput when exceeded.
internal/service/ops.go:124-133 — each accepted candidate triggers its own s.store.CreatePlanting call, and CreatePlanting (internal/store/plantings.go:122-133) is a single INSERT ... RETURNING with no surrounding transaction — and no transaction/batching pattern exists anywhere else in the service or store layers to model this on (the only BeginTx in the codebase is in the migration runner, store/migrate.go:79). For a fill producing hundreds of plops (a realistic case even without hitting the pathological size above), this is N sequential DB round trips/autocommits instead of one batched insert, so cost scales with plop count purely from per-statement overhead rather than being amortized in a single transaction. Suggest wrapping the fill loop in one BeginTx/Commit (or a multi-row insert) so the whole region fill is one write.
Everything else in this lens is fine: ClearObject/ClearObjectPlantings is a single UPDATE (store/plantings.go:88-104) as advertised; DescribeGarden reuses GardenFull's existing bulk queries plus O(1) map lookups (ops.go:243-289) — no N+1 there.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
internal/service/ops.go:108-134 — FillRegion's read-then-write is unguarded, so concurrent fills on the same object can create duplicate/overlapping plops.FillRegion loads existing plantings once via ListActivePlantingsForObject (line 108), computes hex-lattice candidates against that snapshot, then inserts one row at a time (line 125) with no transaction, row lock, or version check tying the read to the writes. By contrast, UpdateObject (internal/service/objects.go:136-149) and UpdatePlanting (internal/service/plantings.go:112+) are explicitly version-guarded (WHERE id = ? AND version = ?). There's no Tx/BeginTx usage anywhere in internal/store except the one-time schema migration (internal/store/migrate.go:79), confirming ops.go's writes are unprotected. The server is a standard net/http.Server (cmd/pansy/main.go:60-73), and for a real (non-:memory:) SQLite path the connection pool is not pinned to 1 (internal/store/sqlite.go:46-51 only pins :memory: DBs, used in tests), so genuine concurrent List/Insert interleaving across goroutines is possible in production. Two concurrent fill_region calls (or a client retry racing the original) on the same object can each see the same stale existing set and insert plops at the same lattice positions, defeating the "skip if covered" idempotency and stacking overlapping plops.
internal/service/ops.go:116-134 — a mid-loop CreatePlanting failure leaves a partial fill silently committed with no rollback, and the caller only sees an error, not what was already written. Confirmed no transaction wraps the loop (same absence of Tx noted above). For N hex-packed candidates, if the k-th store.CreatePlanting call fails, plops 1..k-1 are already committed individually (each is its own INSERT ... RETURNING, internal/store/plantings.go:122-133) but FillRegion returns (nil, err) at line 127 — the caller gets no indication of the partial write. There is no other multi-row write path in this codebase to compare against for precedent; ClearObjectPlantings is a single UPDATE statement.
internal/agent/tools_test.go:54-56 — svc.CreatePlant errors are swallowed (garlic, _ := ...), inconsistent with the rest of the test's error checking, risking a nil-pointer panic instead of a clear failure message. Every other setup call in this test checks the error with t.Fatalf (garden creation at lines 50-53, and create_object/describe_garden calls throughout). CreatePlant returns (*domain.Plant, error) and returns nil on error (internal/service/plants.go:68-83). If any of the three CreatePlant calls fails, garlic/basil/beans are nil, and the subsequent garlic.ID/basil.ID/beans.ID field accesses (used building the fill-region argument list at line 76) dereference a nil pointer, panicking instead of producing a readable test failure.
Through the security lens, the change is sound. I verified the authz and input-handling paths against the actual code:
Authz enforced on every mutation.FillRegion/FillNamedRegion/ClearObject all route through objectForRole(..., roleEditor) (internal/service/objects.go:124), and DescribeGarden goes through GardenFull → requireGardenRole(..., roleViewer) (internal/service/objects.go:162). A viewer is correctly refused on fill/clear and allowed on describe (confirmed by TestOpsForbiddenForViewer and the tagged toolbox test). The toolbox binds a fixed actorID and reuses these same service methods, so it inherits the ACLs unchanged.
No SQL injection.ListActivePlantingsForObject and ClearObjectPlantings (internal/store/plantings.go) use ?-parameterized queries; objectID/date are bound, not interpolated. plantingColumns is a package constant, not user input.
Untrusted inputs validated. The region name is resolved by pure Go string-matching in NamedRegion with an ErrInvalidInput fallback for unknown names — never reaches SQL. spacingOverride is checked with isFinite + the [minPlantSpacingCM, maxPlantSpacingCM] bounds (internal/service/ops.go:101, constants at internal/service/plants.go:18). plantID is gated by visiblePlant (internal/service/plantings.go:178), which masks non-existent/non-owned plants as ErrInvalidInput (no existence oracle). CreateObject/CreatePlanting tool wrappers delegate to the existing finalizeObject/finalizePlanting validation.
No secret/credential surface is introduced; the agent package is a thin adapter with no auth of its own.
One borderline note (outside my lens, correctness more than security): FillRegion writes plops via s.store.CreatePlanting directly, bypassing finalizePlanting's maxRadiusCM/count validation. In practice this is bounded — the radius is server-derived from a validated spacing (max(1.5·spacing, 15) with spacing ≤ 10000cm), centers are server-computed, and the center-in-object guard is reimplemented inline — so it does not create an exploitable input path. Noting it only for completeness; it is not a security finding.
🎯 Correctness — No material issues found
VERDICT: No material issues found
Through the correctness lens, I re-derived the key formulas and packing geometry against the checked-out code and they hold:
defaultPlopRadius (ops.go:77): max(1.5·spacing, 15). spacing=4 → 6<15 → 15 ✓; spacing=20 → 30 ✓. Matches the existing #15 formula and the test expectations.
derivedCount (plantings.go:28): max(1, round(π·r²/spacing²)) with the maxExplicitCount cap — reused correctly in FillRegion (ops.go:129) and in DescribeGarden count reporting (ops.go:274).
Hex packing (hexCenters, ops.go:144): pitch=2·radius, rowH=pitch·√3/2. For the 60×60 bed / radius-15 test case I walked the lattice by hand: row 0 (y=−15) → x=−15,15; row 1 (y≈+10.98) → x=0,30; row 2 (y≈+36.96) out of range ⇒ exactly 4 plops, matching the asserted count and the in-bounds assertion.
Re-fill idempotency (coveredByExisting, ops.go:170): dist + radius <= e.RadiusCM with <= means an exact-on-top candidate (dist=0) is covered ⇒ second fill yields 0. Verified.
Local-frame geometry (NamedRegion/describeLocation): north = −y consistently in both the forward map (ops.go:51-69) and the inverse map (ops.go:293-322). NE corner (x>0, y<0) → "NE corner"; south-half beans start at y=MinY+radius>0 ⇒ never "center"/"north". The rotated-bed test relies on FillRegion using o.WidthCM/HeightCM (local) and ignoring RotationDeg, which it does. Verified.
ACL: FillRegion/ClearObject go through objectForRole(..., roleEditor), FillNamedRegion double-checks via the same path, and DescribeGarden through GardenFull→requireGardenRole(..., roleViewer). The viewer-forbidden / viewer-can-describe behavior follows. Verified against objects.go:124-189.
Minor (non-blocking, not strictly a bug): FillRegion builds the domain.Planting and calls store.CreatePlanting directly, bypassing finalizePlanting (plantings.go:234) — so the maxRadiusCM ceiling and explicit validDatePtr checks aren't applied to bulk plops. In practice the radius is bounded by defaultPlopRadius over a spacing that is itself validated to [minPlantSpacingCM, maxPlantSpacingCM], and PlantedAt is set to s.now().Format(dateLayout), so this gap isn't currently reachable. Flagging only as a latent invariant (bulk path diverges from the single-plant path's validation), not as a live correctness defect.
The Region.Circle fields are dead code (no NamedRegion branch nor any public caller ever sets Circle), but that's an unused-code observation, not incorrect behavior.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
Through the code-cleanliness & maintainability lens, the change is well-structured and follows the surrounding service patterns. A few genuine dead-code/duplication nits survive verification:
internal/service/ops.go:20-24 — Region.Circle/CX/CY/Radius are dead/speculative fields.NamedRegion (the only Region constructor in the package) always returns rectangles via rect, which never sets Circle. A repo-wide grep for Circle =/Circle:/Circle: true finds no assignment anywhere — Circle is only ever read in the r.Circle branch of Region.contains (ops.go:27-33), which is therefore unreachable from any production path. hexCenters (ops.go:144-166) also doesn't honor Circle — it iterates the bounding box (r.MinX/MaxX/MinY/MaxY), so even if a circle were constructed the packer would fill the bounding rectangle, not the disk. Either drop the four fields and the contains circle branch (YAGNI), or implement circle support in hexCenters too. As-is it's misleading surface area.
internal/service/ops_test.go:248-257 — seedNamedPlant is a near-duplicate of seedOwnPlant (internal/service/plantings_test.go:24-31). Both verified via read_file: they differ only in Name (param vs hardcoded "Testroot"), Category (CategoryVegetable vs CategoryHerb), and icon (🌱 vs 🌿). A single seedPlant(t, s, owner, name, category, spacing, icon) would cover both. Minor duplication, worth consolidating before more variants accrete.
internal/service/ops.go:138 — localPoint is a one-off internal struct defined separately from Region and used only by hexCenters/FillRegion. Since Region.contains already takes (x, y float64), localPoint adds a small parallel abstraction with no reuse elsewhere (grep confirms no other references). A plain (x, y float64) return would be one fewer type to track. Trivial; leave if you prefer the named clarity.
Nothing else in this lane: the adapter/toolbox wiring in internal/agent/tools.go is thin and consistent, the store additions mirror existing plantings.go patterns, and the build-tag separation is cleanly done.
⚡ Performance — Minor issues
Verdict: Minor issues
internal/service/ops.go:125 (FillRegion) inserts plops one-at-a-time inside the loop — each candidate issues its own s.store.CreatePlanting INSERT/RETURNING round-trip. There is no batch insert and no transaction wrapping the set, so an agent fill_region over a large region with a small-spacing plant is N serial DB writes. A single large bed (e.g. a few hundred plops) becomes a few hundred sequential round-trips. Confirmed by reading CreatePlanting (internal/store/plantings.go:122), which is a single-row INSERT ... RETURNING. Suggested fix: batch the inserts in one transaction, or expose a bulk store method. Impact: performance degradation on large fills (medium).
internal/service/ops.go:121 / internal/service/ops.go:170 — coveredByExisting is O(existing) per candidate, and existing is appended to on every placed plop (internal/service/ops.go:133). Within one FillRegion call the coverage scan is therefore O(candidates × placed) ≈ quadratic in the number of plops being laid down, with no spatial index. For the small fills in the tests this is trivial, but a whole-garden all fill on a big bed degrades quadratically. Confirmed that existing grows every iteration. Suggested fix: keep the existing-plop check O(N) only against pre-existing plops and use a uniform-grid/spatial hash for the "what I just placed" set, or accept overlap among same-fill siblings. Impact: quadratic scaling on large fills (small).
internal/service/ops.go:183 (FillNamedRegion) calls objectForRole and then delegates to FillRegion, which calls objectForRole again (internal/service/ops.go:88). Each objectForRole is a GetObject + requireGardenRole round-trip (verified at internal/service/objects.go:124 and internal/service/gardens.go:66; requireGardenRole itself does GetGarden + effectiveGardenRole), so every agent fill_region does the role/object lookups twice. Minor duplicated-query cost; suggest resolving the region in the caller and calling FillRegion once, or caching the loaded object. Impact: redundant queries per fill (small).
🧯 Error handling & edge cases — Minor issues
All three findings are confirmed against the actual code.
Verdict: Minor issues
internal/service/ops.go:116-134 — FillRegion is non-atomic; a mid-loop store error leaves orphaned plops. The bulk operation creates plops one CreatePlanting at a time (line 125) and return nil, err on the first failure (line 126-128), discarding the created slice. There is no transaction wrapper — I confirmed the service package has no Tx/BeginTx helper (only internal/store/migrate.go and internal/api/oidc_test.go mention transactions, and the latter is a test). A bulk fill widens the partial-failure window: an actor asking "fill the south half" can end up with some plops persisted and an error returned, with no partial-success signal and no rollback. Re-running won't fix it cleanly because the skipped-candidate logic (coveredByExisting, line 121) treats the already-placed plops as "existing" (line 121 + 133 appends stored plops to existing). Suggested fix: wrap the loop in a transaction and rollback on the first insert error, or return the created slice alongside the error so the caller can recover.
internal/service/ops.go:52 — empty/blank region name silently maps to "all" instead of ErrInvalidInput. The doc comment (line 44) states "Unknown names return ErrInvalidInput," but case "all", "": (line 52) accepts an empty string (after trim) as the whole-object rect. Through the agent tool this means fill_region with region: "" (or whitespace) fills the entire object rather than being rejected — a surprising edge case for bad input. Suggested fix: drop "" from the all case so it falls through to default → ErrInvalidInput, or explicitly validate key == "" before the switch.
internal/service/ops.go:182-192 — FillNamedRegion fetches the object twice via objectForRole.FillNamedRegion calls s.objectForRole (line 183) to load the object, then calls s.FillRegion (line 191), which itself calls s.objectForRole again (line 88). The redundant ACL/GetObject lookup doubles the store round-trips per fill and widens the delete-race window (an object deleted between the two lookups surfaces as a confusing error from the second call). Low-impact; introduced by the bulk op. Suggested fix: have FillNamedRegion resolve the region from the object it already fetched and call an internal fill that doesn't re-fetch, or pass the already-loaded object through.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
I audited the change through the Security lens. Here is what I verified:
Auth / AuthZ — All new operations (FillRegion, FillNamedRegion, ClearObject, DescribeGarden) correctly gate on objectForRole / requireGardenRole with the expected minimum role (roleEditor for mutations, roleViewer for reads). No bypass path was found.
Injection — All SQL in the new store methods (ListActivePlantingsForObject, ClearObjectPlantings) uses parameterised queries (? placeholders). No string concatenation of user input into SQL. The date passed to ClearObjectPlantings is server-generated (s.now().UTC().Format(dateLayout)), not user-controlled.
Input validation — NamedRegion rejects unknown names with ErrInvalidInput. FillRegion validates spacingOverride against the same minPlantSpacingCM/maxPlantSpacingCM bounds used elsewhere, and checks isFinite. The Region coordinates are derived only from the already-validated object dimensions, never from raw user input.
Unsafe deserialization / SSRF / secret leakage — None present in this diff. The majordomo integration uses statically-typed JSON struct tags; there is no reflection-based unmarshaling of untrusted Go types and no HTTP client usage.
DoS / resource exhaustion — The minimum plop radius is 15 cm (defaultPlopRadius floors at 15), so hexCenters cannot generate an unbounded number of iterations. On the maximum 100 m garden the lattice yields at most ~10⁵ candidates, and coveredByExisting prevents duplicate insertion on re-fill. A garden editor could already achieve comparable load via individual CreatePlanting calls, so the bulk path does not materially expand the attack surface.
Outside my lens: The radius generated by defaultPlopRadius(maxPlantSpacingCM) exceeds maxRadiusCM, which FillRegion does not catch because it bypasses finalizePlanting. This is a correctness / validation consistency issue, not a security vulnerability (it results in zero placements, not an integrity breach).
🎯 Correctness — Minor issues
Verdict: Minor issues
internal/service/ops.go:118 — Circular object clipping bug: The FillRegion guard math.Abs(c.x) > halfW || math.Abs(c.y) > halfH clips candidates to the axis-aligned bounding square of the object. Since create_object (and the domain) already supports "circle" shapes (diameter = WidthCM), a circular bed with e.g. 400 cm diameter will incorrectly accept plop centers at (200, 200) — outside the actual circle (distance 282 > radius 200) — as long as they are inside the bounding square. The region and hex lattice are both rectangular, so plops end up in the corners of circular objects. The fix is to branch on o.Shape == "circle" (or "circle") and use math.Hypot(c.x, c.y) > halfW for the bound check instead of the box guard.
internal/service/ops.go:183 — FillNamedRegion redundant ACL / object fetch: FillNamedRegion calls objectForRole with roleEditor, then calls FillRegion which calls objectForRole again. This double-fetches the object and double-checks permissions. If the two calls race (rare but possible under concurrent role changes), the second check could spuriously fail after the first passed. More importantly, the extra query is unnecessary overhead. Pass the already-resolved object into a package-private variant, or drop the first check and let FillRegion be the sole gate.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
internal/service/ops.go:182-191 — Redundant objectForRole call in FillNamedRegion. FillNamedRegion fetches the object via objectForRole, resolves the region, then delegates to FillRegion, which calls objectForRoleagain. This doubles the same permission/DB work for every named-region fill. Clean fix: extract a private fillRegion that accepts the already-resolved *domain.GardenObject, so FillNamedRegion fetches once and FillRegion can still enforce its own ACL without duplicating the round-trip.
internal/service/ops.go:20-24 — Dead fields on Region. Circle, CX, CY, and Radius are never constructed or exercised in this PR. contains handles them, but every call site (including tests) only uses rectangles. Either remove the circle support until it’s needed, or add at least one constructor/test so the struct doesn’t carry unused API surface.
internal/service/ops_test.go:248 / internal/service/plantings_test.go:24 — Duplicate test helper seedNamedPlant vs seedOwnPlant.
Both are in package service and do the same thing (create a plant for an owner), differing only by whether the name is parameterized. Having two helpers for the same seeding operation is unnecessary copy-paste; consolidate into one helper with an optional name parameter.
internal/service/ops_test.go:56 / internal/service/plantings_test.go:12 — Duplicate test helper seedFillBed vs seedBed.
Same package, same purpose (create a plantable bed), only the dimensions/position differ. Parameterize the existing seedBed helper instead of adding a second one.
internal/agent/tools_test.go:130 — mustJSON swallows errors and lacks t.Helper().
The helper ignores json.Marshal errors and doesn’t mark itself with t.Helper(), so a failure would print the wrong line number. It should panic on error and call t.Helper() for clearer test output.
⚡ Performance — Blocking issues found
Blocking issues found
FillNamedRegion performs duplicate ACL work — it calls objectForRole (ops.go:183), resolves the name, then delegates to FillRegion, which calls objectForRole again (ops.go:88). Each call hits GetObject + requireGardenRole (which may do GetGarden + GetShareRole), so a named-region fill costs roughly double the DB round-trips of a raw FillRegion for the same permission check.
FillRegion emits one INSERT per plop — inside the hexCenters loop it calls store.CreatePlanting individually (ops.go:125). For even a modest 2 m × 2 m bed with 15 cm radius plops this is dozens of SQLite round-trips; for a large bed near the 100 m cap it can be thousands. The store has no batch-insert API, so this N+1 pattern is an inherent regression for a method whose entire purpose is bulk creation.
coveredByExisting is O(N) per candidate and N grows during the fill — the loop at ops.go:116-134 scans every existing (and already-created-in-this-fill) plop for each candidate. For a re-fill on a bed with many plops, this compounds with the per-insert DB calls above. The existing slice also undergoes repeated append without pre-sized capacity, causing reallocations and copies as the fill proceeds.
🧯 Error handling & edge cases — Minor issues
Minor issues
internal/service/ops.go:118 — The object-bounds guard math.Abs(c.x) > halfW || math.Abs(c.y) > halfH uses strict > instead of >= (or a radius-aware check). When a hex-lattice center lands exactly on the object boundary (e.g., a 60 cm-wide bed with a 20 cm radius plop produces a center at x = 30), the plop is allowed even though it extends outside the object. The comment says “center must be inside”, but the check permits boundary centers, and with positive radius half the plop protrudes past the edge. A radius-aware guard like math.Abs(c.x)+radius > halfW would keep the entire plop within bounds.
internal/service/ops.go:116-135 — FillRegion silently returns an empty slice when the region is too small to fit even one plop (e.g., a tiny bed or a very large spacing override). The caller gets success with zero items created and no indication that the fill was effectively a no-op. An explicit check after hexCenters—or inside it—should return an error such as ErrInvalidInput when the region dimensions are smaller than the computed plop diameter so the agent/user knows why nothing was planted.
opencode/glm-5.2:cloud (opencode) — Blocking issues found
Reviewed through the security lens (authn/authz, injection, input validation, info leakage, untrusted-data handling). I verified the key claims against the checked-out code:
ACL enforcement: FillRegion/FillNamedRegion/ClearObject all gate on objectForRole(..., roleEditor) (ops.go:88, 183, 197), and DescribeGarden routes through GardenFull → requireGardenRole(..., roleViewer) (objects.go:162-163). A viewer is denied mutations and only granted describe, matching the test in ops_test.go. Confirmed by reading objectForRole/requireGardenRole.
SQL injection: Both new store functions (ListActivePlantingsForObject, ClearObjectPlantings) use QueryContext/ExecContext with ? placeholders; no string interpolation of user input. The date arg into ClearObjectPlantings originates from s.now().UTC().Format(dateLayout) inside the service, not from the caller.
Input validation: spacingOverride is bounds-checked with isFinite and [minPlantSpacingCM, maxPlantSpacingCM] before use (ops.go:101-104). NamedRegion maps free-form region names through a fixed switch — unknown names return ErrInvalidInput; no path/eval surface.
Toolbox actor binding: NewToolbox(svc, actorID) binds a single actor and every handler runs through service methods that re-check roles per call, so the actor identity is not trusted from the tool layer. The doc.go note that this must be constructed per authenticated session is the right contract; authn itself is correctly left to the caller (the majordomo/executus harness outside this repo).
Info leakage: Error paths return sentinel domain errors (ErrForbidden, ErrInvalidInput, ErrNotFound), not detailed internal messages. DescribeGarden only exposes objects/plantings of a garden the actor can already view via GardenFull; Version is intentionally surfaced as the mutation guard, not sensitive material.
Nothing material in my lens.
🎯 Correctness — Minor issues
Review
Verdict: Minor issues
Findings (all verified against the actual checked-out code):
internal/service/ops.go:124-129 — FillRegion bypasses the maxRadiusCM invariant enforced by CreatePlanting.CreatePlanting validates every plop via finalizePlanting, which rejects RadiusCM > maxRadiusCM (10000, plantings.go:235). FillRegion instead calls s.store.CreatePlanting directly (ops.go:125) with radius = defaultPlopRadius(spacing) = max(1.5*spacing, 15) (ops.go:106/77-79). A plant with spacing > ~6666.67 cm (valid: maxPlantSpacingCM = 10000, plants.go:19), or a spacingOverride near the upper limit (validated only against maxPlantSpacingCM at ops.go:101), yields a radius > 10000 that CreatePlanting would refuse but FillRegion happily inserts. The schema has no CHECK on radius_cm (0001_init.sql:129), so the row is persisted. Suggested fix: gate radius against maxRadiusCM in FillRegion, or cap the spacing range so 1.5*spacing ≤ maxRadiusCM.
internal/service/ops.go:129 vs internal/service/objects.go:186 — DerivedCount mismatch when spacingOverride is used.FillRegion computes DerivedCount with the override spacing (ops.go:129), but GardenFull (which backs DescribeGarden) recomputes it from plant.SpacingCM (objects.go:186), the un-overridden value. So an agent that fills with an override and then calls describe_garden sees a different count than fill_region returned. The persisted Count is unaffected (always nil/derived here), so this is only a transient display inconsistency. Minor.
internal/service/ops.go:196-201 — ClearObject skips the removed_at >= planted_at ordering check enforced by finalizePlanting for single updates.UpdatePlanting rejects removed < planted (plantings.go:256-261); ClearObject's bulk UPDATE sets removed_at = today for all active plops regardless of their planted_at. A plop with a future planted_at (allowed by validDatePtr, plantings.go:267-273) would end up with removed_at < planted_at. Edge case (future plantings are unusual) and per-row validation in a bulk UPDATE is impractical; trivial severity — noted for completeness.
The hex-packing math, NamedRegion compass mapping, re-fill idempotency (the 4-plop lattice lands exactly on existing centers), rotated-bed local-frame behavior, ACL enforcement (viewer → ErrForbidden via objectForRole(...,roleEditor)), and the describeLocation inverse mapping all check out against first-principles re-derivation.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues found
internal/service/ops.go:20-24 — vestigial circle support on Region. The Region struct exposes Circle, CX, CY, Radius fields and contains() (ops.go:27-33) has a Circle branch, but nothing in the repo ever sets Circle = true. I grepped the whole tree: the only Region{...} literals are via rect() (ops.go:36-38, which produces rectangles) and NamedRegion's default arm returns a zero Region with Circle false. The only other Circle matches are an unrelated domain.ShapeCircle object-shape constant and a test that creates a circle object — neither touches Region.Circle. The exported type's doc ("...or a circle when Circle is set") advertises a shape mode that is unreachable in practice. This is dead API surface that future readers will have to reason about. Suggested fix: drop the four circle fields and the if r.Circle { ... } branch, or actually wire the circle case (e.g. for Shape: "circle" objects) if that's intended.
internal/service/ops.go:75-79 — misleading "Reused here" comment on defaultPlopRadius. The comment says "defaultPlopRadius is #15's formula... Reused here (don't duplicate the constant elsewhere)." But this is the only definition of the function in the codebase (grep for defaultPlopRadius returns exactly this definition plus its one call site and two tests). The phrasing reads as if it's being imported/duplicated from #15's code, when it's actually the canonical definition. Minor; the "don't duplicate the constant elsewhere" directive is oddly aimed at the very file that holds the original. Suggested fix: drop "Reused here" or reword to "This is the canonical definition; don't re-derive the 1.5×spacing / 15 cm floor elsewhere."
internal/service/ops.go:293-323 — describeLocation's final switch is slightly leaky. The function builds ns/ew strings then switches on their specific values (case ns == "N", case ns == "S", case ew == "E", default: return "west"). It works, but the trailing default: return "west" silently encodes "ew == W" — a future edit that adds a fourth ns/ew state would get "west" by accident. Minor readability nit; an explicit case ew == "W": with a real default: that returns "" (or panics) would make the exhaustive intent obvious. Not blocking.
⚡ Performance — Blocking issues found
I'll verify each finding against the actual code.
All three findings are confirmed against the actual code:
N+1 INSERTs (ops.go:116-134): Confirmed — the loop calls s.store.CreatePlanting once per surviving candidate, no batch helper exists in the store.
O(N²) coveredByExisting (ops.go:121,133): Confirmed — coveredByExisting is a linear scan (lines 170-177), and existing = append(existing, *stored) grows the slice on each insert.
Double objectForRole in FillNamedRegion (ops.go:182-191): Confirmed — FillNamedRegion calls s.objectForRole (line 183), then delegates to FillRegion which calls s.objectForRole again (line 88) on the same objectID. objectForRole (objects.go:124) does a GetObject + requireGardenRole DB read each call.
The bounds reasoning is also valid: validDimensionCM caps at maxGardenCM = 10_000 (gardens.go:18), and minPlantSpacingCM = 0.1 (plants.go:18).
VERDICT: Blocking issues found
internal/service/ops.go:116-134 — FillRegion issues one INSERT per candidate with no batching (N+1 writes). Each surviving lattice point calls s.store.CreatePlanting individually — a separate INSERT … RETURNING round-trip to SQLite (verified at store/plantings.go:122). There is no bulk insert helper anywhere in internal/store (grep for BatchInsert|BulkCreate|batch returns nothing). Objects are bounded only by validDimensionCM = maxGardenCM = 10_000 cm (gardens.go:18, applied via finalizeObject at objects.go:243), and the minimum plant spacing is minPlantSpacingCM = 0.1 (plants.go:18). The plop radius floor is max(1.5·spacing, 15) = 15 cm, giving a hex pitch of 30 cm and row height ~26 cm. On a maximally-sized 100 m × 100 m bed that is ~128,000 candidates → up to ~128k sequential INSERTs on a single agent call. Suggested fix: collect the surviving candidates and insert them in one transactional batch (the store already uses BeginTx in migrate.go), or expose a CreatePlantings(ctx, []*Planting) bulk path.
internal/service/ops.go:121,133 — coveredByExisting makes FillRegion O(N²) in created plops. Each candidate scans the entire existing slice, and the loop appends each just-stored plop back into existing (existing = append(existing, *stored)). Verified coveredByExisting (ops.go:170-177) is a linear scan with no spatial index. With N survivors this is N existing-scans per candidate → ~N²/2 distance comparisons. Suggested fix: build a spatial filter keyed by hex lattice cell (a map[localPoint]struct{} of taken cells) so the "already covered" check is O(1).
internal/service/ops.go:182-191 — FillNamedRegion performs the ACL/object fetch twice.FillNamedRegion calls s.objectForRole(...) (a DB read + role check) and then delegates to FillRegion, which calls s.objectForRole(...) again on the same objectID (ops.go:88). Verified objectForRole at objects.go:124 loads the object and does a share/role lookup each call — so this is a doubled DB round-trip on every agent fill_region call. Suggested fix: have FillNamedRegion resolve the name and call an internal fillRegion that accepts the already-loaded *domain.GardenObject, or have FillRegion accept a pre-fetched object.
🧯 Error handling & edge cases — Minor issues
I've verified all three findings against the actual code.
Finding 1 (FillRegion bypasses finalizePlanting): Confirmed. ops.go:124-129 builds the plop by hand and calls s.store.CreatePlanting directly. plantings.go:101 shows the normal create path calls finalizePlanting(p, o, true). plantings.go:235 enforces p.RadiusCM > maxRadiusCM (10000). defaultPlopRadius = max(1.5*spacing, 15); with spacingOverride validated only to ≤ maxPlantSpacingCM = 10000 (plants.go:19, ops.go:101), radius can reach 15000, exceeding the 10000 ceiling the REST path rejects. PlantedAt is well-formed today (s.now().UTC().Format(dateLayout)), so only the radius cap is currently exercisable. Keep.
Finding 2 (DescribeGarden empty plant name): Confirmed structurally. ops.go:280 does Plant: plantByID[pl.PlantID].Name with no fallback. objects.go:171-178 runs ListActivePlantingsForGarden and ListReferencedPlants as independent queries with no transaction. plants.go:31-57 and plantings.go:32-57 both filter removed_at IS NULL, so a planting soft-removed between the two reads would appear in full.Plantings but its plant would be absent from full.Plants, yielding "". Plant deletion is blocked by ON DELETE RESTRICT (0001_init.sql:126) and CountPlantingsForPlant (plants.go:128-134), so the realistic trigger is the soft-remove race, not plant deletion — but the empty-field impact stands. Keep (impact is cosmetic, as the draft notes).
Finding 3 (hexCenters unbounded loop on oversized region): Confirmed. ops.go:144-166 generates the full lattice before ops.go:118 filters by math.Abs(c.x) > halfW. FillRegion (ops.go:87) takes a caller-supplied Region with no clamping/intersection with object bounds. Current reachability: only FillNamedRegion (ops.go:182) calls it, which builds the region via NamedRegion from the object's own WidthCM/HeightCM (ops.go:45-73) — safe. No REST endpoint exposes FillRegion with an arbitrary region (grep of internal/api found none). So this is latent, not active — matching the draft's "latent rather than active" framing. Keep as small/medium.
All three findings survive verification. Verdict remains "Minor issues".
Review (lens: Error handling & edge cases)
Verdict: Minor issues
FillRegion bypasses finalizePlanting validation — internal/service/ops.go:124-129. The plop is built by hand and inserted via s.store.CreatePlanting directly, skipping the finalizePlanting(p, o, true) gate that every other create path (CreatePlanting at plantings.go:101) uses. The local-frame center is bounded by the math.Abs(c.x) > halfW guard, and radius/today are well-formed by construction today, but two invariants are lost: the radius upper bound (maxRadiusCM = 10_000, plantings.go:18,235) and the PlantedAt date-format check (validDatePtr, plantings.go:252,267-273). The radius cap is currently exercisable: spacingOverride is validated only to ≤ maxPlantSpacingCM = 10_000 (plants.go:19, ops.go:101) and defaultPlopRadius = max(1.5*spacing, 15) (ops.go:77-79), so a spacing of 10 000 yields radius 15 000 — over the 10 000 ceiling finalizePlanting would reject, but FillRegion accepts and inserts. The PlantedAt bypass is latent (today is produced via s.now().UTC().Format(dateLayout) so it is well-formed today), but a future change to how today is built would silently create rows the REST create path rejects. Suggested fix: call finalizePlanting(p, o, false) before CreatePlanting (bounds already checked above, so checkBounds=false is fine), or at minimum assert radius <= maxRadiusCM.
DescribeGarden emits an empty plant name when a planting's plant isn't in plantByID — internal/service/ops.go:280 (Plant: plantByID[pl.PlantID].Name). full.Plants comes from ListReferencedPlants and full.Plantings from ListActivePlantingsForGarden (objects.go:171-178); both filter removed_at IS NULL (plants.go:36, plantings.go:36) but are independent reads with no transaction. If a planting is soft-removed between the two reads, it appears in full.Plantings while its plant is absent from full.Plants, producing "plant": "". (Hard plant deletion is blocked by ON DELETE RESTRICT — 0001_init.sql:126 — plus CountPlantingsForPlant in plants.go:128-134, so the realistic trigger is the soft-remove race, not deletion.) Impact is cosmetic (an empty JSON field in the agent-facing output, not a crash). Suggested fix: fall back to "unknown" or fmt.Sprintf("plant#%d", pl.PlantID) when the map lookup misses.
hexCenters can loop a very long time on an oversized caller-supplied Region — internal/service/ops.go:144-166. FillRegion takes a caller-supplied Region that is never validated to lie within the object or clamped to [-halfW,halfW]×[-halfH,halfH]. hexCenters generates the full lattice before the math.Abs(c.x) > halfW guard at ops.go:118 filters each point, so a region like MinX=-1e9, MaxX=1e9 with a tiny radius would generate a huge lattice only to discard every point. There is no upper bound on the returned slice. Today the only caller is FillNamedRegion (ops.go:182), which builds the region from the object's own WidthCM/HeightCM via NamedRegion (ops.go:45-73) — safe — and no REST endpoint exposes FillRegion with an arbitrary region, so this is latent rather than active. Suggested fix: clamp the region to the object's bounds at the top of FillRegion before generating centers.
opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
internal/service/ops.go:124 — FillRegion creates plantings with a computed RadiusCM = defaultPlopRadius(spacing) but never enforces the maxRadiusCM = 10_000 ceiling that finalizePlanting applies in the single-plop CreatePlanting path. Since spacing (or spacingOverride) can be as large as maxPlantSpacingCM = 10_000, defaultPlopRadius produces 15_000, which exceeds the domain maximum. This creates plantings that violate the radius invariant; subsequent edits via UpdatePlanting would fail validation. Fix: add if radius > maxRadiusCM { return nil, domain.ErrInvalidInput } before the hex-centers loop (or call finalizePlanting on each candidate).
internal/service/ops.go:48-52 — NamedRegion strips a trailing "corner" or "half" suffix. If the input is literally "corner" or "half" (or variants like " corner "), the stripped remainder is empty string, which matches case "all", "" and returns the full-object region instead of ErrInvalidInput. An LLM hallucinating an ambiguous region name could inadvertently trigger a full-object fill. Fix: after suffix stripping, return ErrInvalidInput when the remaining string is empty and the original input was not "all".
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
internal/service/ops.go:182-191 — Duplicate objectForRole call between FillNamedRegion and FillRegion FillNamedRegion fetches and authorizes the object, resolves the region, then calls s.FillRegion(...) which fetches and authorizes the same object again. This doubles the ACL query on every named fill and means two code sites enforce the same role rule. A cleaner pattern is an unexported fillRegion that takes the already-loaded *domain.GardenObject, with both exported methods calling it after their respective auth/geometry steps.
internal/agent/tools_test.go:118 — String literal role "viewer" instead of constant
The rest of the service tests (e.g. ops_test.go:171) use domain.RoleViewer. Using a raw string breaks the project's established pattern and is brittle if the canonical value changes. Import internal/domain and use domain.RoleViewer.
internal/agent/tools_test.go:54-56 — Swallowed errors in test setup CreatePlant returns (*domain.Plant, error) but the test discards the error with _, _ for garlic, basil, and beans. If plant creation fails, the test will later fail with a confusing downstream error (e.g. plantId: 0 rejected) instead of surfacing the root cause. Check the errors like the rest of the test body and like ops_test.go's seedNamedPlant.
internal/service/ops.go:20-24 — Unused circle fields in Region
The Region struct carries Circle, CX, CY, and Radius (documented as "used when Circle"), but nothing in this PR ever sets Circle = true and there is no circle(...) constructor. It's dead code that adds cognitive overhead. Either add a constructor + a code path that exercises it, or drop the circle fields until they are actually needed.
⚡ Performance — Minor issues
Verdict: Minor issues
internal/service/ops.go:125 — FillRegion issues one INSERT … RETURNING per plop.
The loop over hexCenters calls s.store.CreatePlanting for every candidate, which is a single-row insert. There is no transaction wrapping or batching in the store layer (no BeginTx / bulk-insert API exists). For a large fill this is classic N+1 query behaviour — each plop is a separate DB round-trip and SQLite commit. Impact: Filling a large region (e.g. a 2 m bed with 10 cm spacing) becomes proportionally slower and hammered by write overhead. Fix: Wrap the fill in a transaction and add a batched InsertPlantings store method that inserts multiple rows in one statement.
internal/service/ops.go:170 — coveredByExisting is an O(N) linear scan, making the fill O(M·N).
For every lattice candidate, the code iterates over all existing plops (and appends newly created ones to the same slice, so the scan grows as the fill proceeds). Impact: Coverage checking degrades quadratically. A fill that creates 100 plops performs ~5 000 distance checks; a 500-plop fill performs ~125 000. Fix: Partition existing into a coarse spatial grid (e.g. keyed by floor(x/radius), floor(y/radius)) so only neighbours are checked.
internal/store/plantings.go:64 — ListActivePlantingsForObject is unbounded and lacks a composite index.
The query is SELECT … WHERE object_id = ? AND removed_at IS NULL. There is only a single-column index on object_id; because ClearObjectPlantings soft-removes rows (sets removed_at), an object that is cleared and refilled accumulates dead rows that are still reached by the object_id index and then filtered in-memory. There is no LIMIT. Impact: Memory usage and query time grow with the total historical plop count for the object, not just active ones. Fix: Add CREATE INDEX idx_plantings_object_active ON plantings (object_id, removed_at); and consider a LIMIT if objects can hold thousands of plantings.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
internal/service/ops.go:116-135 — FillRegion performs a non-atomic bulk insert.
It loops over lattice candidates and calls s.store.CreatePlanting one at a time. If any intermediate call fails (DB constraint error, context cancellation, transient failure), the function returns nil, err but leaves the previously-created plantings already persisted. The garden ends up partially filled with no way to roll back. Fix: wrap the loop in a DB transaction (begin → commit/rollback), or document the at-least-once semantics in the method comment.
internal/service/ops.go:45-73 — NamedRegion dereferences o without a nil guard.
Line 46 does hw, hh := o.WidthCM/2, o.HeightCM/2. If o is nil, this panics. The function is exported and test code (and any future caller) can pass a nil object directly. Fix: add if o == nil { return Region{}, domain.ErrInvalidInput } at the top of NamedRegion.
internal/service/ops.go:99-106 — FillRegion does not validate the computed plop radius. plant.SpacingCM is trusted from the DB (validated on insert), but if a bad value somehow exists (manual insertion, future bug, corrupted row), defaultPlopRadius could return NaN or +Inf. hexCenters only checks radius <= 0; a NaN radius causes the loop to silently produce zero plantings with no error returned. Fix: after computing radius, add if !isFinite(radius) || radius <= 0 { return nil, domain.ErrInvalidInput }.
Minor issues
internal/service/ops.go:196-202 — ClearObject does not check o.Plantable. FillRegion rejects non-plantable objects (trees, paths, etc.) with ErrInvalidInput, but ClearObject allows the call and simply clears 0 rows. This is harmless but inconsistent. Fix: either return ErrInvalidInput for non-plantable objects, or document that clearing them is a no-op.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Blocking issues found** · 20 findings (9 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | FillRegion performs one INSERT per lattice candidate (N+1 writes); worst case ~10^5 sequential round-trips on a max-sized bed with min spacing | `internal/service/ops.go:116` | 5/5 | correctness, error-handling, performance, security |
| 🟠 | FillNamedRegion calls objectForRole twice (once itself, once via FillRegion) | `internal/service/ops.go:182` | 5/5 | correctness, error-handling, maintainability, performance |
| 🟡 | Region.Circle/CX/CY/Radius fields and contains circle branch are dead/speculative code never produced by NamedRegion and unsupported by hexCenters | `internal/service/ops.go:20` | 4/5 | maintainability |
| 🔴 | NamedRegion nil pointer dereference on nil GardenObject | `internal/service/ops.go:46` | 2/5 | correctness, error-handling |
| 🟠 | FillRegion silent empty fill when computed radius is NaN/Inf | `internal/service/ops.go:106` | 2/5 | error-handling |
| 🟡 | Swallowed CreatePlant errors in test setup | `internal/agent/tools_test.go:54` | 2/5 | error-handling, maintainability |
| 🟡 | Empty/blank region name silently maps to "all" instead of returning ErrInvalidInput, contradicting the doc and surprising bad-input handling | `internal/service/ops.go:52` | 2/5 | error-handling, security |
| 🟡 | ClearObject inconsistent Plantable check compared to FillRegion | `internal/service/ops.go:196` | 2/5 | correctness, error-handling |
| 🟡 | Duplicate seedNamedPlant helper; seedOwnPlant already exists in same package | `internal/service/ops_test.go:248` | 2/5 | maintainability |
<details><summary>11 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | coveredByExisting is O(N) linear scan making fill O(M*N) | `internal/service/ops.go:170` | opencode/kimi-k2.6:cloud | performance |
| 🟠 | ListActivePlantingsForObject loads unbounded rows with no composite index on (object_id, removed_at) | `internal/store/plantings.go:64` | opencode/kimi-k2.6:cloud | performance |
| 🟡 | String literal role instead of domain.RoleViewer constant | `internal/agent/tools_test.go:118` | opencode/kimi-k2.6:cloud | maintainability |
| 🟡 | hexCenters generates the full lattice before the bounds guard filters it; an oversized caller-supplied Region (not clamped to the object) can cause a very long loop / large allocation | `internal/service/ops.go:144` | opencode/glm-5.2:cloud | error-handling |
| 🟡 | DescribeGarden emits an empty plant name when a planting's plant is missing from the (independently-read) referenced-plants map | `internal/service/ops.go:280` | opencode/glm-5.2:cloud | error-handling |
| 🟡 | Duplicate seedFillBed helper; seedBed already exists in same package | `internal/service/ops_test.go:56` | kimi-k2.6:cloud | maintainability |
| ⚪ | mustJSON helper swallows marshal errors and lacks t.Helper() | `internal/agent/tools_test.go:130` | kimi-k2.6:cloud | maintainability |
| ⚪ | defaultPlopRadius comment says 'Reused here (don't duplicate the constant elsewhere)' but this is the only definition in the codebase — misleading phrasing | `internal/service/ops.go:75` | opencode/glm-5.2:cloud | maintainability |
| ⚪ | localPoint is a one-off struct with no reuse; minor parallel abstraction over plain (x,y) coords | `internal/service/ops.go:138` | glm-5.2:cloud | maintainability |
| ⚪ | describeLocation's final default silently maps to 'west'; an explicit ew=="W" case would make the switch's exhaustiveness obvious | `internal/service/ops.go:293` | opencode/glm-5.2:cloud | maintainability |
| ⚪ | 2000x2000 garden setup duplicated verbatim across 4 tests in two files instead of a shared seed helper | `internal/service/ops_test.go:71` | claude-code/sonnet | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
The draft did not contain any actual findings — it only contains meta-commentary about a missing `ExitPlanMode` tool and a stray reference to "the DoS finding" location without ever stating what that finding is, where it is, or its impact. There is nothing concrete here to confirm against the code (no file:line claim, no described defect), so nothing carries forward.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings verified directly against the code and store layer.
**Finding 1** — confirmed. `internal/service/plantings.go:18` sets `maxRadiusCM = 10_000`; `internal/service/plants.go:19` sets `maxPlantSpacingCM = 10_000`. `FillRegion` (`ops.go:106`) computes `radius := defaultPlopRadius(spacing)` = `max(1.5·spacing, 15)`, so any spacing above `10000/1.5 ≈ 6667cm` (legal per `maxPlantSpacingCM`, and reachable directly via `spacingOverrideCm` which is only checked against `minPlantSpacingCM`/`maxPlantSpacingCM` at `ops.go:100-104`) yields a radius > `maxRadiusCM`. `ops.go:125` calls `s.store.CreatePlanting` — the raw store method, not `s.CreatePlanting` — which I confirmed in `internal/store/plantings.go:119-133` performs zero validation (its own comment: "fields already validated by the service"). So `finalizePlanting`'s radius cap (`plantings.go:235`) is never applied on this path. I also confirmed `finalizePlanting`'s radius check (`plantings.go:235`) sits above and outside the `checkBounds` gate, so it runs unconditionally on every `UpdatePlanting` call regardless of what's being patched — a row created this way permanently fails `ErrInvalidInput` on any future edit.
**Finding 2** — confirmed by tracing `NamedRegion` (`ops.go:45-73`) by hand: input `"corner"` → `TrimSuffix(key, "corner")` → `""` → `TrimSuffix("", "half")` → `""`, which hits `case "all", "":` and returns the full-object rectangle instead of erroring. Same for `"half"`. This is a genuine behavior/intent mismatch, though real-world exposure is limited since the toolbox's `fill_region` description (`internal/agent/tools.go`) enumerates only compass-qualified values.
## VERDICT: Minor issues
- **`internal/service/ops.go:106,125` — `FillRegion` bypasses the `maxRadiusCM` invariant, allowing an unfixable-later planting.** `FillRegion` computes `radius := defaultPlopRadius(spacing)` (`max(1.5·spacing, 15)`) and inserts via `s.store.CreatePlanting` directly (the raw store call), skipping `finalizePlanting` — the validator every other write path (`CreatePlanting`/`UpdatePlanting` in `internal/service/plantings.go:101,145,235`) goes through, which rejects `RadiusCM > maxRadiusCM` (10,000cm, `internal/service/plantings.go:18`). Since `maxPlantSpacingCM` is also 10,000cm (`internal/service/plants.go:19`), a plant with spacing above `maxRadiusCM/1.5 ≈ 6667cm` — a legal, in-bounds spacing value — produces a stored radius >10,000cm that `finalizePlanting` would normally reject. Worse, `finalizePlanting`'s radius check runs unconditionally on every future `UpdatePlanting` call (not gated by `checkBounds`), so once such a row exists, any subsequent edit to that planting (even just a label change) permanently fails with `ErrInvalidInput`. `spacingOverride` makes this trivially reachable (pass `spacingOverrideCm: 7000`, which only hits the existing min/max spacing bounds at `ops.go:101`).
- Fix: after computing `radius`, cap/validate it against `maxRadiusCM` before entering the loop, or route creation through `finalizePlanting` itself.
- **`internal/service/ops.go:48-49` (`NamedRegion`) — the literal strings `"corner"` or `"half"` alone silently resolve to `"all"` instead of being rejected.** `TrimSuffix` doesn't require a preceding separator: input `"corner"` → strip suffix `"corner"` → `""`, which matches the `case "all", "":` branch, filling the entire object rather than erroring. Same for `"half"`. Low real-world likelihood (the toolbox description enumerates only compass-qualified values), but it's a genuine mismatch between intent (unknown/degenerate name → `ErrInvalidInput`) and behavior (silently expands scope to the whole object).
- Fix: only trim the suffix when it's preceded by a space (e.g. `strings.TrimSuffix(key, " corner")`), or explicitly reject `key == "corner" || key == "half"` before the switch.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings independently verified against the actual source. Finding 1: `objectForRole` performs `store.GetObject` + `requireGardenRole` (objects.go:124-134), and it's called once in `FillRegion` (ops.go:88) and once more in `FillNamedRegion` (ops.go:183) before delegating to `FillRegion`, which repeats the lookup — confirmed redundant. Finding 2: the `GardenInput{..., WidthCM: 2000, HeightCM: 2000}` + `CreateGarden` pattern is duplicated verbatim in `ops_test.go:71,111,194` and `tools_test.go:50`, while `seedGarden` (objects_test.go:13, default 1000×1000) already exists as the shared-helper precedent — confirmed.
VERDICT: Minor issues
- `internal/service/ops.go:183` — `FillNamedRegion` calls `objectForRole` to resolve the object, then immediately calls `FillRegion`, which calls `objectForRole` again for the same `objectID`/`actorID` (verified both call sites at ops.go:88 and ops.go:183, going through the same `s.store.GetObject` + `s.requireGardenRole` in `objects.go:124`). This is the sole caller of `FillRegion` from within the package that already has `o` in hand but throws it away. Every agent `fill_region` call goes through `FillNamedRegion`, so this redundant fetch-and-recheck is on the hot path. An unexported `fillRegion(ctx, actorID, o *domain.GardenObject, region, plantID, spacingOverride)` core that both `FillRegion` and `FillNamedRegion` call after their own single `objectForRole` would remove the duplicate authorization/DB round trip without changing either public signature.
- Test setup duplication: the literal `GardenInput{Name: "Big"/"Plot", WidthCM: 2000, HeightCM: 2000}` + `CreateGarden` pair is copy-pasted across `TestFillRegionDeterministicPacking` (ops_test.go:71), `TestFillRegionRotatedBedUsesLocalFrame` (ops_test.go:111), `TestFillScenario` (ops_test.go:194), and again in `TestToolboxScenario` (`internal/agent/tools_test.go:50`). The package already has a `seedGarden` helper (`internal/service/objects_test.go:13`) for the default-size (1000×1000) case; a second small helper (e.g. `seedBigGarden`) alongside it would remove four copies of the same three-line setup.
</details>
<details><summary><b>⚡ Performance</b> — Blocking issues found</summary>
Both findings check out against the actual code. All confirmed: `ops.go:116-135`, `hexCenters`, `coveredByExisting`, `defaultPlopRadius`, the `minPlantSpacingCM`/`maxGardenCM`/`validDimensionCM` constants and their reuse for objects, and `CreatePlanting`'s unbatched single-row INSERT (`plantings.go:122-133`) with no transaction usage anywhere else in the service layer besides migrations.
One correction: the draft's claim that `SetMaxOpenConns(1)` (`sqlite.go:50`) "blocks all other DB access" is wrong for the reachable production path — that line only fires `if memory` (a bare `:memory:` DSN, used in tests per the doc comment at `sqlite.go:37-39`). Production (`cmd/pansy/main.go:40`) opens `cfg.DBPath`, a real file, so the pool is NOT pinned to one connection there; WAL mode allows concurrent readers. I dropped that clause but kept the core finding, since the unbounded O(N²) work still hangs the request itself regardless of connection pooling.
## Corrected review
**VERDICT: Blocking issues found**
- `internal/service/ops.go:116-135` (`FillRegion`) has unbounded, quadratic cost with no cap on candidate/region size. `hexCenters` (`ops.go:144-166`) generates a candidate grid over the whole region at `2×radius` pitch, and `radius` floors at 15cm regardless of spacing (`defaultPlopRadius`, `ops.go:77-78`, combined with `minPlantSpacingCM = 0.1` in `plants.go:18`). Objects can be up to `maxGardenCM = 10_000` cm (100m) per side (`gardens.go:18`, enforced for objects too via `validDimensionCM` at `objects.go:243`). Filling `"all"` on a 100m×100m object at the 15cm radius floor yields on the order of ~1.3×10⁵ hex candidates. For every one of those, `coveredByExisting` (`ops.go:170-176`) does a linear scan over `existing`, which itself grows by one for every accepted candidate (`existing = append(existing, *stored)` at `ops.go:133`) — an O(N²) scan, ~10¹⁰ operations in the worst case. This is reachable through completely normal usage (an agent told to "fill the whole bed with basil," or `fill_region`/`"all"` on any large plantable object), not just adversarial input — it will hang the handling goroutine for the request long before it produces a response. Suggest bucketing `existing` into a spatial grid (cell size ≈ `2×radius`) so coverage checks are O(1) average instead of O(N), and/or capping total region area or candidate count with a clear `ErrInvalidInput` when exceeded.
- `internal/service/ops.go:124-133` — each accepted candidate triggers its own `s.store.CreatePlanting` call, and `CreatePlanting` (`internal/store/plantings.go:122-133`) is a single `INSERT ... RETURNING` with no surrounding transaction — and no transaction/batching pattern exists anywhere else in the service or store layers to model this on (the only `BeginTx` in the codebase is in the migration runner, `store/migrate.go:79`). For a fill producing hundreds of plops (a realistic case even without hitting the pathological size above), this is N sequential DB round trips/autocommits instead of one batched insert, so cost scales with plop count purely from per-statement overhead rather than being amortized in a single transaction. Suggest wrapping the fill loop in one `BeginTx`/`Commit` (or a multi-row insert) so the whole region fill is one write.
Everything else in this lens is fine: `ClearObject`/`ClearObjectPlantings` is a single `UPDATE` (`store/plantings.go:88-104`) as advertised; `DescribeGarden` reuses `GardenFull`'s existing bulk queries plus O(1) map lookups (`ops.go:243-289`) — no N+1 there.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
VERDICT: Minor issues
- **`internal/service/ops.go:108-134` — `FillRegion`'s read-then-write is unguarded, so concurrent fills on the same object can create duplicate/overlapping plops.** `FillRegion` loads `existing` plantings once via `ListActivePlantingsForObject` (line 108), computes hex-lattice candidates against that snapshot, then inserts one row at a time (line 125) with no transaction, row lock, or version check tying the read to the writes. By contrast, `UpdateObject` (`internal/service/objects.go:136-149`) and `UpdatePlanting` (`internal/service/plantings.go:112+`) are explicitly version-guarded (`WHERE id = ? AND version = ?`). There's no `Tx`/`BeginTx` usage anywhere in `internal/store` except the one-time schema migration (`internal/store/migrate.go:79`), confirming ops.go's writes are unprotected. The server is a standard `net/http.Server` (`cmd/pansy/main.go:60-73`), and for a real (non-`:memory:`) SQLite path the connection pool is not pinned to 1 (`internal/store/sqlite.go:46-51` only pins `:memory:` DBs, used in tests), so genuine concurrent `List`/`Insert` interleaving across goroutines is possible in production. Two concurrent `fill_region` calls (or a client retry racing the original) on the same object can each see the same stale `existing` set and insert plops at the same lattice positions, defeating the "skip if covered" idempotency and stacking overlapping plops.
- **`internal/service/ops.go:116-134` — a mid-loop `CreatePlanting` failure leaves a partial fill silently committed with no rollback, and the caller only sees an error, not what was already written.** Confirmed no transaction wraps the loop (same absence of `Tx` noted above). For N hex-packed candidates, if the k-th `store.CreatePlanting` call fails, plops 1..k-1 are already committed individually (each is its own `INSERT ... RETURNING`, `internal/store/plantings.go:122-133`) but `FillRegion` returns `(nil, err)` at line 127 — the caller gets no indication of the partial write. There is no other multi-row write path in this codebase to compare against for precedent; `ClearObjectPlantings` is a single UPDATE statement.
- **`internal/agent/tools_test.go:54-56` — `svc.CreatePlant` errors are swallowed (`garlic, _ := ...`), inconsistent with the rest of the test's error checking, risking a nil-pointer panic instead of a clear failure message.** Every other setup call in this test checks the error with `t.Fatalf` (garden creation at lines 50-53, and `create_object`/`describe_garden` calls throughout). `CreatePlant` returns `(*domain.Plant, error)` and returns `nil` on error (`internal/service/plants.go:68-83`). If any of the three `CreatePlant` calls fails, `garlic`/`basil`/`beans` are `nil`, and the subsequent `garlic.ID`/`basil.ID`/`beans.ID` field accesses (used building the fill-region argument list at line 76) dereference a nil pointer, panicking instead of producing a readable test failure.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the security lens, the change is sound. I verified the authz and input-handling paths against the actual code:
- **Authz enforced on every mutation.** `FillRegion`/`FillNamedRegion`/`ClearObject` all route through `objectForRole(..., roleEditor)` (`internal/service/objects.go:124`), and `DescribeGarden` goes through `GardenFull` → `requireGardenRole(..., roleViewer)` (`internal/service/objects.go:162`). A viewer is correctly refused on fill/clear and allowed on describe (confirmed by `TestOpsForbiddenForViewer` and the tagged toolbox test). The toolbox binds a fixed `actorID` and reuses these same service methods, so it inherits the ACLs unchanged.
- **No SQL injection.** `ListActivePlantingsForObject` and `ClearObjectPlantings` (`internal/store/plantings.go`) use `?`-parameterized queries; `objectID`/`date` are bound, not interpolated. `plantingColumns` is a package constant, not user input.
- **Untrusted inputs validated.** The `region` name is resolved by pure Go string-matching in `NamedRegion` with an `ErrInvalidInput` fallback for unknown names — never reaches SQL. `spacingOverride` is checked with `isFinite` + the `[minPlantSpacingCM, maxPlantSpacingCM]` bounds (`internal/service/ops.go:101`, constants at `internal/service/plants.go:18`). `plantID` is gated by `visiblePlant` (`internal/service/plantings.go:178`), which masks non-existent/non-owned plants as `ErrInvalidInput` (no existence oracle). `CreateObject`/`CreatePlanting` tool wrappers delegate to the existing `finalizeObject`/`finalizePlanting` validation.
- **No secret/credential surface** is introduced; the agent package is a thin adapter with no auth of its own.
One borderline note (outside my lens, correctness more than security): `FillRegion` writes plops via `s.store.CreatePlanting` directly, bypassing `finalizePlanting`'s `maxRadiusCM`/count validation. In practice this is bounded — the radius is server-derived from a validated spacing (`max(1.5·spacing, 15)` with spacing ≤ 10000cm), centers are server-computed, and the center-in-object guard is reimplemented inline — so it does not create an exploitable input path. Noting it only for completeness; it is not a security finding.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT: No material issues found**
Through the correctness lens, I re-derived the key formulas and packing geometry against the checked-out code and they hold:
- **`defaultPlopRadius`** (`ops.go:77`): `max(1.5·spacing, 15)`. spacing=4 → 6<15 → 15 ✓; spacing=20 → 30 ✓. Matches the existing #15 formula and the test expectations.
- **`derivedCount`** (`plantings.go:28`): `max(1, round(π·r²/spacing²))` with the `maxExplicitCount` cap — reused correctly in `FillRegion` (`ops.go:129`) and in `DescribeGarden` count reporting (`ops.go:274`).
- **Hex packing** (`hexCenters`, `ops.go:144`): pitch=2·radius, rowH=pitch·√3/2. For the 60×60 bed / radius-15 test case I walked the lattice by hand: row 0 (y=−15) → x=−15,15; row 1 (y≈+10.98) → x=0,30; row 2 (y≈+36.96) out of range ⇒ exactly 4 plops, matching the asserted count and the in-bounds assertion.
- **Re-fill idempotency** (`coveredByExisting`, `ops.go:170`): `dist + radius <= e.RadiusCM` with `<=` means an exact-on-top candidate (dist=0) is covered ⇒ second fill yields 0. Verified.
- **Local-frame geometry** (`NamedRegion`/`describeLocation`): north = −y consistently in both the forward map (`ops.go:51-69`) and the inverse map (`ops.go:293-322`). NE corner (x>0, y<0) → "NE corner"; south-half beans start at y=MinY+radius>0 ⇒ never "center"/"north". The rotated-bed test relies on `FillRegion` using `o.WidthCM/HeightCM` (local) and ignoring `RotationDeg`, which it does. Verified.
- **ACL**: `FillRegion`/`ClearObject` go through `objectForRole(..., roleEditor)`, `FillNamedRegion` double-checks via the same path, and `DescribeGarden` through `GardenFull`→`requireGardenRole(..., roleViewer)`. The viewer-forbidden / viewer-can-describe behavior follows. Verified against `objects.go:124-189`.
Minor (non-blocking, not strictly a bug): `FillRegion` builds the `domain.Planting` and calls `store.CreatePlanting` directly, bypassing `finalizePlanting` (`plantings.go:234`) — so the `maxRadiusCM` ceiling and explicit `validDatePtr` checks aren't applied to bulk plops. In practice the radius is bounded by `defaultPlopRadius` over a spacing that is itself validated to `[minPlantSpacingCM, maxPlantSpacingCM]`, and `PlantedAt` is set to `s.now().Format(dateLayout)`, so this gap isn't currently reachable. Flagging only as a latent invariant (bulk path diverges from the single-plant path's validation), not as a live correctness defect.
The `Region.Circle` fields are dead code (no `NamedRegion` branch nor any public caller ever sets `Circle`), but that's an unused-code observation, not incorrect behavior.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues**
Through the code-cleanliness & maintainability lens, the change is well-structured and follows the surrounding service patterns. A few genuine dead-code/duplication nits survive verification:
- **`internal/service/ops.go:20-24` — `Region.Circle`/`CX`/`CY`/`Radius` are dead/speculative fields.** `NamedRegion` (the only `Region` constructor in the package) always returns rectangles via `rect`, which never sets `Circle`. A repo-wide grep for `Circle =`/`Circle:`/`Circle: true` finds no assignment anywhere — `Circle` is only ever read in the `r.Circle` branch of `Region.contains` (ops.go:27-33), which is therefore unreachable from any production path. `hexCenters` (ops.go:144-166) also doesn't honor `Circle` — it iterates the bounding box (`r.MinX/MaxX/MinY/MaxY`), so even if a circle were constructed the packer would fill the bounding rectangle, not the disk. Either drop the four fields and the `contains` circle branch (YAGNI), or implement circle support in `hexCenters` too. As-is it's misleading surface area.
- **`internal/service/ops_test.go:248-257` — `seedNamedPlant` is a near-duplicate of `seedOwnPlant` (`internal/service/plantings_test.go:24-31`).** Both verified via read_file: they differ only in `Name` (param vs hardcoded `"Testroot"`), `Category` (`CategoryVegetable` vs `CategoryHerb`), and icon (`🌱` vs `🌿`). A single `seedPlant(t, s, owner, name, category, spacing, icon)` would cover both. Minor duplication, worth consolidating before more variants accrete.
- **`internal/service/ops.go:138` — `localPoint` is a one-off internal struct** defined separately from `Region` and used only by `hexCenters`/`FillRegion`. Since `Region.contains` already takes `(x, y float64)`, `localPoint` adds a small parallel abstraction with no reuse elsewhere (grep confirms no other references). A plain `(x, y float64)` return would be one fewer type to track. Trivial; leave if you prefer the named clarity.
Nothing else in this lane: the adapter/toolbox wiring in `internal/agent/tools.go` is thin and consistent, the store additions mirror existing `plantings.go` patterns, and the build-tag separation is cleanly done.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Verdict: Minor issues**
- `internal/service/ops.go:125` (`FillRegion`) inserts plops one-at-a-time inside the loop — each candidate issues its own `s.store.CreatePlanting` INSERT/RETURNING round-trip. There is no batch insert and no transaction wrapping the set, so an agent `fill_region` over a large region with a small-spacing plant is N serial DB writes. A single large bed (e.g. a few hundred plops) becomes a few hundred sequential round-trips. Confirmed by reading `CreatePlanting` (`internal/store/plantings.go:122`), which is a single-row `INSERT ... RETURNING`. Suggested fix: batch the inserts in one transaction, or expose a bulk store method. Impact: performance degradation on large fills (medium).
- `internal/service/ops.go:121` / `internal/service/ops.go:170` — `coveredByExisting` is O(existing) per candidate, and `existing` is appended to on every placed plop (`internal/service/ops.go:133`). Within one `FillRegion` call the coverage scan is therefore O(candidates × placed) ≈ quadratic in the number of plops being laid down, with no spatial index. For the small fills in the tests this is trivial, but a whole-garden `all` fill on a big bed degrades quadratically. Confirmed that `existing` grows every iteration. Suggested fix: keep the existing-plop check O(N) only against pre-existing plops and use a uniform-grid/spatial hash for the "what I just placed" set, or accept overlap among same-fill siblings. Impact: quadratic scaling on large fills (small).
- `internal/service/ops.go:183` (`FillNamedRegion`) calls `objectForRole` and then delegates to `FillRegion`, which calls `objectForRole` again (`internal/service/ops.go:88`). Each `objectForRole` is a `GetObject` + `requireGardenRole` round-trip (verified at `internal/service/objects.go:124` and `internal/service/gardens.go:66`; `requireGardenRole` itself does `GetGarden` + `effectiveGardenRole`), so every agent `fill_region` does the role/object lookups twice. Minor duplicated-query cost; suggest resolving the region in the caller and calling `FillRegion` once, or caching the loaded object. Impact: redundant queries per fill (small).
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
All three findings are confirmed against the actual code.
## Verdict: Minor issues
- **`internal/service/ops.go:116-134` — `FillRegion` is non-atomic; a mid-loop store error leaves orphaned plops.** The bulk operation creates plops one `CreatePlanting` at a time (line 125) and `return nil, err` on the first failure (line 126-128), discarding the `created` slice. There is no transaction wrapper — I confirmed the service package has no `Tx`/`BeginTx` helper (only `internal/store/migrate.go` and `internal/api/oidc_test.go` mention transactions, and the latter is a test). A bulk fill widens the partial-failure window: an actor asking "fill the south half" can end up with some plops persisted and an error returned, with no partial-success signal and no rollback. Re-running won't fix it cleanly because the skipped-candidate logic (`coveredByExisting`, line 121) treats the already-placed plops as "existing" (line 121 + 133 appends stored plops to `existing`). Suggested fix: wrap the loop in a transaction and rollback on the first insert error, or return the `created` slice alongside the error so the caller can recover.
- **`internal/service/ops.go:52` — empty/blank region name silently maps to `"all"` instead of `ErrInvalidInput`.** The doc comment (line 44) states "Unknown names return ErrInvalidInput," but `case "all", "":` (line 52) accepts an empty string (after trim) as the whole-object rect. Through the agent tool this means `fill_region` with `region: ""` (or whitespace) fills the entire object rather than being rejected — a surprising edge case for bad input. Suggested fix: drop `""` from the `all` case so it falls through to `default` → `ErrInvalidInput`, or explicitly validate `key == ""` before the switch.
- **`internal/service/ops.go:182-192` — `FillNamedRegion` fetches the object twice via `objectForRole`.** `FillNamedRegion` calls `s.objectForRole` (line 183) to load the object, then calls `s.FillRegion` (line 191), which itself calls `s.objectForRole` again (line 88). The redundant ACL/`GetObject` lookup doubles the store round-trips per fill and widens the delete-race window (an object deleted between the two lookups surfaces as a confusing error from the second call). Low-impact; introduced by the bulk op. Suggested fix: have `FillNamedRegion` resolve the region from the object it already fetched and call an internal fill that doesn't re-fetch, or pass the already-loaded object through.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found.
I audited the change through the **Security** lens. Here is what I verified:
**Auth / AuthZ** — All new operations (`FillRegion`, `FillNamedRegion`, `ClearObject`, `DescribeGarden`) correctly gate on `objectForRole` / `requireGardenRole` with the expected minimum role (`roleEditor` for mutations, `roleViewer` for reads). No bypass path was found.
**Injection** — All SQL in the new store methods (`ListActivePlantingsForObject`, `ClearObjectPlantings`) uses parameterised queries (`?` placeholders). No string concatenation of user input into SQL. The `date` passed to `ClearObjectPlantings` is server-generated (`s.now().UTC().Format(dateLayout)`), not user-controlled.
**Input validation** — `NamedRegion` rejects unknown names with `ErrInvalidInput`. `FillRegion` validates `spacingOverride` against the same `minPlantSpacingCM`/`maxPlantSpacingCM` bounds used elsewhere, and checks `isFinite`. The `Region` coordinates are derived only from the already-validated object dimensions, never from raw user input.
**Unsafe deserialization / SSRF / secret leakage** — None present in this diff. The majordomo integration uses statically-typed JSON struct tags; there is no reflection-based unmarshaling of untrusted Go types and no HTTP client usage.
**DoS / resource exhaustion** — The minimum plop radius is 15 cm (`defaultPlopRadius` floors at 15), so `hexCenters` cannot generate an unbounded number of iterations. On the maximum 100 m garden the lattice yields at most ~10⁵ candidates, and `coveredByExisting` prevents duplicate insertion on re-fill. A garden editor could already achieve comparable load via individual `CreatePlanting` calls, so the bulk path does not materially expand the attack surface.
**Outside my lens:** The radius generated by `defaultPlopRadius(maxPlantSpacingCM)` exceeds `maxRadiusCM`, which `FillRegion` does not catch because it bypasses `finalizePlanting`. This is a correctness / validation consistency issue, not a security vulnerability (it results in zero placements, not an integrity breach).
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`internal/service/ops.go:118` — Circular object clipping bug**: The `FillRegion` guard `math.Abs(c.x) > halfW || math.Abs(c.y) > halfH` clips candidates to the *axis-aligned bounding square* of the object. Since `create_object` (and the domain) already supports `"circle"` shapes (diameter = `WidthCM`), a circular bed with e.g. 400 cm diameter will incorrectly accept plop centers at `(200, 200)` — outside the actual circle (distance 282 > radius 200) — as long as they are inside the bounding square. The region and hex lattice are both rectangular, so plops end up in the corners of circular objects. The fix is to branch on `o.Shape == "circle"` (or `"circle"`) and use `math.Hypot(c.x, c.y) > halfW` for the bound check instead of the box guard.
- **`internal/service/ops.go:183` — `FillNamedRegion` redundant ACL / object fetch**: `FillNamedRegion` calls `objectForRole` with `roleEditor`, then calls `FillRegion` which calls `objectForRole` again. This double-fetches the object and double-checks permissions. If the two calls race (rare but possible under concurrent role changes), the second check could spuriously fail after the first passed. More importantly, the extra query is unnecessary overhead. Pass the already-resolved object into a package-private variant, or drop the first check and let `FillRegion` be the sole gate.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`internal/service/ops.go:182-191` — Redundant `objectForRole` call in `FillNamedRegion`.**
`FillNamedRegion` fetches the object via `objectForRole`, resolves the region, then delegates to `FillRegion`, which calls `objectForRole` *again*. This doubles the same permission/DB work for every named-region fill. Clean fix: extract a private `fillRegion` that accepts the already-resolved `*domain.GardenObject`, so `FillNamedRegion` fetches once and `FillRegion` can still enforce its own ACL without duplicating the round-trip.
- **`internal/service/ops.go:20-24` — Dead fields on `Region`.**
`Circle`, `CX`, `CY`, and `Radius` are never constructed or exercised in this PR. `contains` handles them, but every call site (including tests) only uses rectangles. Either remove the circle support until it’s needed, or add at least one constructor/test so the struct doesn’t carry unused API surface.
- **`internal/service/ops_test.go:248` / `internal/service/plantings_test.go:24` — Duplicate test helper `seedNamedPlant` vs `seedOwnPlant`.**
Both are in package `service` and do the same thing (create a plant for an owner), differing only by whether the name is parameterized. Having two helpers for the same seeding operation is unnecessary copy-paste; consolidate into one helper with an optional name parameter.
- **`internal/service/ops_test.go:56` / `internal/service/plantings_test.go:12` — Duplicate test helper `seedFillBed` vs `seedBed`.**
Same package, same purpose (create a plantable bed), only the dimensions/position differ. Parameterize the existing `seedBed` helper instead of adding a second one.
- **`internal/agent/tools_test.go:130` — `mustJSON` swallows errors and lacks `t.Helper()`.**
The helper ignores `json.Marshal` errors and doesn’t mark itself with `t.Helper()`, so a failure would print the wrong line number. It should panic on error and call `t.Helper()` for clearer test output.
</details>
<details><summary><b>⚡ Performance</b> — Blocking issues found</summary>
**Blocking issues found**
- **`FillNamedRegion` performs duplicate ACL work** — it calls `objectForRole` (ops.go:183), resolves the name, then delegates to `FillRegion`, which calls `objectForRole` again (ops.go:88). Each call hits `GetObject` + `requireGardenRole` (which may do `GetGarden` + `GetShareRole`), so a named-region fill costs roughly double the DB round-trips of a raw `FillRegion` for the same permission check.
- **`FillRegion` emits one `INSERT` per plop** — inside the `hexCenters` loop it calls `store.CreatePlanting` individually (ops.go:125). For even a modest 2 m × 2 m bed with 15 cm radius plops this is dozens of SQLite round-trips; for a large bed near the 100 m cap it can be thousands. The store has no batch-insert API, so this N+1 pattern is an inherent regression for a method whose entire purpose is bulk creation.
- **`coveredByExisting` is O(N) per candidate and N grows during the fill** — the loop at ops.go:116-134 scans every existing (and already-created-in-this-fill) plop for each candidate. For a re-fill on a bed with many plops, this compounds with the per-insert DB calls above. The `existing` slice also undergoes repeated `append` without pre-sized capacity, causing reallocations and copies as the fill proceeds.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- **`internal/service/ops.go:118`** — The object-bounds guard `math.Abs(c.x) > halfW || math.Abs(c.y) > halfH` uses strict `>` instead of `>=` (or a radius-aware check). When a hex-lattice center lands exactly on the object boundary (e.g., a 60 cm-wide bed with a 20 cm radius plop produces a center at x = 30), the plop is allowed even though it extends outside the object. The comment says “center must be inside”, but the check permits boundary centers, and with positive radius half the plop protrudes past the edge. A radius-aware guard like `math.Abs(c.x)+radius > halfW` would keep the entire plop within bounds.
- **`internal/service/ops.go:116-135`** — `FillRegion` silently returns an empty slice when the region is too small to fit even one plop (e.g., a tiny bed or a very large spacing override). The caller gets success with zero items created and no indication that the fill was effectively a no-op. An explicit check after `hexCenters`—or inside it—should return an error such as `ErrInvalidInput` when the region dimensions are smaller than the computed plop diameter so the agent/user knows why nothing was planted.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking 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
Reviewed through the security lens (authn/authz, injection, input validation, info leakage, untrusted-data handling). I verified the key claims against the checked-out code:
- **ACL enforcement**: `FillRegion`/`FillNamedRegion`/`ClearObject` all gate on `objectForRole(..., roleEditor)` (ops.go:88, 183, 197), and `DescribeGarden` routes through `GardenFull` → `requireGardenRole(..., roleViewer)` (objects.go:162-163). A viewer is denied mutations and only granted describe, matching the test in `ops_test.go`. Confirmed by reading `objectForRole`/`requireGardenRole`.
- **SQL injection**: Both new store functions (`ListActivePlantingsForObject`, `ClearObjectPlantings`) use `QueryContext`/`ExecContext` with `?` placeholders; no string interpolation of user input. The `date` arg into `ClearObjectPlantings` originates from `s.now().UTC().Format(dateLayout)` inside the service, not from the caller.
- **Input validation**: `spacingOverride` is bounds-checked with `isFinite` and `[minPlantSpacingCM, maxPlantSpacingCM]` before use (ops.go:101-104). `NamedRegion` maps free-form region names through a fixed `switch` — unknown names return `ErrInvalidInput`; no path/eval surface.
- **Toolbox actor binding**: `NewToolbox(svc, actorID)` binds a single actor and every handler runs through service methods that re-check roles per call, so the actor identity is not trusted from the tool layer. The `doc.go` note that this must be constructed per authenticated session is the right contract; authn itself is correctly left to the caller (the majordomo/executus harness outside this repo).
- **Info leakage**: Error paths return sentinel domain errors (`ErrForbidden`, `ErrInvalidInput`, `ErrNotFound`), not detailed internal messages. `DescribeGarden` only exposes objects/plantings of a garden the actor can already view via `GardenFull`; `Version` is intentionally surfaced as the mutation guard, not sensitive material.
Nothing material in my lens.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
## Review
**Verdict: Minor issues**
Findings (all verified against the actual checked-out code):
- **`internal/service/ops.go:124-129` — `FillRegion` bypasses the `maxRadiusCM` invariant enforced by `CreatePlanting`.** `CreatePlanting` validates every plop via `finalizePlanting`, which rejects `RadiusCM > maxRadiusCM` (10000, plantings.go:235). `FillRegion` instead calls `s.store.CreatePlanting` directly (ops.go:125) with `radius = defaultPlopRadius(spacing) = max(1.5*spacing, 15)` (ops.go:106/77-79). A plant with spacing > ~6666.67 cm (valid: `maxPlantSpacingCM = 10000`, plants.go:19), or a `spacingOverride` near the upper limit (validated only against `maxPlantSpacingCM` at ops.go:101), yields a radius > 10000 that `CreatePlanting` would refuse but `FillRegion` happily inserts. The schema has no CHECK on `radius_cm` (0001_init.sql:129), so the row is persisted. Suggested fix: gate `radius` against `maxRadiusCM` in `FillRegion`, or cap the spacing range so `1.5*spacing ≤ maxRadiusCM`.
- **`internal/service/ops.go:129` vs `internal/service/objects.go:186` — `DerivedCount` mismatch when `spacingOverride` is used.** `FillRegion` computes `DerivedCount` with the override `spacing` (ops.go:129), but `GardenFull` (which backs `DescribeGarden`) recomputes it from `plant.SpacingCM` (objects.go:186), the un-overridden value. So an agent that fills with an override and then calls `describe_garden` sees a different count than `fill_region` returned. The persisted `Count` is unaffected (always nil/derived here), so this is only a transient display inconsistency. Minor.
- **`internal/service/ops.go:196-201` — `ClearObject` skips the `removed_at >= planted_at` ordering check enforced by `finalizePlanting` for single updates.** `UpdatePlanting` rejects `removed < planted` (plantings.go:256-261); `ClearObject`'s bulk UPDATE sets `removed_at = today` for all active plops regardless of their `planted_at`. A plop with a future `planted_at` (allowed by `validDatePtr`, plantings.go:267-273) would end up with `removed_at < planted_at`. Edge case (future plantings are unusual) and per-row validation in a bulk UPDATE is impractical; trivial severity — noted for completeness.
The hex-packing math, `NamedRegion` compass mapping, re-fill idempotency (the 4-plop lattice lands exactly on existing centers), rotated-bed local-frame behavior, ACL enforcement (viewer → `ErrForbidden` via `objectForRole(...,roleEditor)`), and the `describeLocation` inverse mapping all check out against first-principles re-derivation.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
## VERDICT: Minor issues found
- **`internal/service/ops.go:20-24` — vestigial circle support on `Region`.** The `Region` struct exposes `Circle`, `CX`, `CY`, `Radius` fields and `contains()` (ops.go:27-33) has a `Circle` branch, but nothing in the repo ever sets `Circle = true`. I grepped the whole tree: the only `Region{...}` literals are via `rect()` (ops.go:36-38, which produces rectangles) and `NamedRegion`'s `default` arm returns a zero `Region` with `Circle` false. The only other `Circle` matches are an unrelated `domain.ShapeCircle` object-shape constant and a test that creates a circle *object* — neither touches `Region.Circle`. The exported type's doc ("...or a circle when Circle is set") advertises a shape mode that is unreachable in practice. This is dead API surface that future readers will have to reason about. Suggested fix: drop the four circle fields and the `if r.Circle { ... }` branch, or actually wire the circle case (e.g. for `Shape: "circle"` objects) if that's intended.
- **`internal/service/ops.go:75-79` — misleading "Reused here" comment on `defaultPlopRadius`.** The comment says "defaultPlopRadius is #15's formula... Reused here (don't duplicate the constant elsewhere)." But this is the only definition of the function in the codebase (grep for `defaultPlopRadius` returns exactly this definition plus its one call site and two tests). The phrasing reads as if it's being imported/duplicated from #15's code, when it's actually the canonical definition. Minor; the "don't duplicate the constant elsewhere" directive is oddly aimed at the very file that holds the original. Suggested fix: drop "Reused here" or reword to "This is the canonical definition; don't re-derive the 1.5×spacing / 15 cm floor elsewhere."
- **`internal/service/ops.go:293-323` — `describeLocation`'s final switch is slightly leaky.** The function builds `ns`/`ew` strings then switches on their specific values (`case ns == "N"`, `case ns == "S"`, `case ew == "E"`, `default: return "west"`). It works, but the trailing `default: return "west"` silently encodes "ew == W" — a future edit that adds a fourth `ns`/`ew` state would get "west" by accident. Minor readability nit; an explicit `case ew == "W":` with a real `default:` that returns `""` (or panics) would make the exhaustive intent obvious. Not blocking.
</details>
<details><summary><b>⚡ Performance</b> — Blocking issues found</summary>
I'll verify each finding against the actual code.
All three findings are confirmed against the actual code:
1. **N+1 INSERTs** (ops.go:116-134): Confirmed — the loop calls `s.store.CreatePlanting` once per surviving candidate, no batch helper exists in the store.
2. **O(N²) coveredByExisting** (ops.go:121,133): Confirmed — `coveredByExisting` is a linear scan (lines 170-177), and `existing = append(existing, *stored)` grows the slice on each insert.
3. **Double objectForRole in FillNamedRegion** (ops.go:182-191): Confirmed — `FillNamedRegion` calls `s.objectForRole` (line 183), then delegates to `FillRegion` which calls `s.objectForRole` again (line 88) on the same `objectID`. `objectForRole` (objects.go:124) does a `GetObject` + `requireGardenRole` DB read each call.
The bounds reasoning is also valid: `validDimensionCM` caps at `maxGardenCM = 10_000` (gardens.go:18), and `minPlantSpacingCM = 0.1` (plants.go:18).
**VERDICT: Blocking issues found**
- **`internal/service/ops.go:116-134` — `FillRegion` issues one INSERT per candidate with no batching (N+1 writes).** Each surviving lattice point calls `s.store.CreatePlanting` individually — a separate `INSERT … RETURNING` round-trip to SQLite (verified at `store/plantings.go:122`). There is no bulk insert helper anywhere in `internal/store` (grep for `BatchInsert|BulkCreate|batch` returns nothing). Objects are bounded only by `validDimensionCM` = `maxGardenCM = 10_000` cm (`gardens.go:18`, applied via `finalizeObject` at `objects.go:243`), and the minimum plant spacing is `minPlantSpacingCM = 0.1` (`plants.go:18`). The plop radius floor is `max(1.5·spacing, 15) = 15` cm, giving a hex pitch of 30 cm and row height ~26 cm. On a maximally-sized 100 m × 100 m bed that is ~128,000 candidates → up to ~128k sequential INSERTs on a single agent call. Suggested fix: collect the surviving candidates and insert them in one transactional batch (the store already uses `BeginTx` in `migrate.go`), or expose a `CreatePlantings(ctx, []*Planting)` bulk path.
- **`internal/service/ops.go:121,133` — `coveredByExisting` makes `FillRegion` O(N²) in created plops.** Each candidate scans the entire `existing` slice, and the loop appends each just-stored plop back into `existing` (`existing = append(existing, *stored)`). Verified `coveredByExisting` (ops.go:170-177) is a linear scan with no spatial index. With N survivors this is N existing-scans per candidate → ~N²/2 distance comparisons. Suggested fix: build a spatial filter keyed by hex lattice cell (a `map[localPoint]struct{}` of taken cells) so the "already covered" check is O(1).
- **`internal/service/ops.go:182-191` — `FillNamedRegion` performs the ACL/object fetch twice.** `FillNamedRegion` calls `s.objectForRole(...)` (a DB read + role check) and then delegates to `FillRegion`, which calls `s.objectForRole(...)` again on the same `objectID` (`ops.go:88`). Verified `objectForRole` at `objects.go:124` loads the object and does a share/role lookup each call — so this is a doubled DB round-trip on every agent `fill_region` call. Suggested fix: have `FillNamedRegion` resolve the name and call an internal `fillRegion` that accepts the already-loaded `*domain.GardenObject`, or have `FillRegion` accept a pre-fetched object.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
I've verified all three findings against the actual code.
**Finding 1 (FillRegion bypasses finalizePlanting):** Confirmed. `ops.go:124-129` builds the plop by hand and calls `s.store.CreatePlanting` directly. `plantings.go:101` shows the normal create path calls `finalizePlanting(p, o, true)`. `plantings.go:235` enforces `p.RadiusCM > maxRadiusCM` (10000). `defaultPlopRadius` = `max(1.5*spacing, 15)`; with `spacingOverride` validated only to `≤ maxPlantSpacingCM = 10000` (`plants.go:19`, `ops.go:101`), radius can reach 15000, exceeding the 10000 ceiling the REST path rejects. `PlantedAt` is well-formed today (`s.now().UTC().Format(dateLayout)`), so only the radius cap is currently exercisable. Keep.
**Finding 2 (DescribeGarden empty plant name):** Confirmed structurally. `ops.go:280` does `Plant: plantByID[pl.PlantID].Name` with no fallback. `objects.go:171-178` runs `ListActivePlantingsForGarden` and `ListReferencedPlants` as independent queries with no transaction. `plants.go:31-57` and `plantings.go:32-57` both filter `removed_at IS NULL`, so a planting soft-removed between the two reads would appear in `full.Plantings` but its plant would be absent from `full.Plants`, yielding `""`. Plant *deletion* is blocked by `ON DELETE RESTRICT` (`0001_init.sql:126`) and `CountPlantingsForPlant` (`plants.go:128-134`), so the realistic trigger is the soft-remove race, not plant deletion — but the empty-field impact stands. Keep (impact is cosmetic, as the draft notes).
**Finding 3 (hexCenters unbounded loop on oversized region):** Confirmed. `ops.go:144-166` generates the full lattice before `ops.go:118` filters by `math.Abs(c.x) > halfW`. `FillRegion` (`ops.go:87`) takes a caller-supplied `Region` with no clamping/intersection with object bounds. Current reachability: only `FillNamedRegion` (`ops.go:182`) calls it, which builds the region via `NamedRegion` from the object's own `WidthCM/HeightCM` (`ops.go:45-73`) — safe. No REST endpoint exposes `FillRegion` with an arbitrary region (grep of `internal/api` found none). So this is latent, not active — matching the draft's "latent rather than active" framing. Keep as small/medium.
All three findings survive verification. Verdict remains "Minor issues".
## Review (lens: Error handling & edge cases)
**Verdict: Minor issues**
- **`FillRegion` bypasses `finalizePlanting` validation** — `internal/service/ops.go:124-129`. The plop is built by hand and inserted via `s.store.CreatePlanting` directly, skipping the `finalizePlanting(p, o, true)` gate that every other create path (`CreatePlanting` at `plantings.go:101`) uses. The local-frame center *is* bounded by the `math.Abs(c.x) > halfW` guard, and `radius`/`today` are well-formed by construction today, but two invariants are lost: the **radius upper bound** (`maxRadiusCM = 10_000`, `plantings.go:18,235`) and the **`PlantedAt` date-format check** (`validDatePtr`, `plantings.go:252,267-273`). The radius cap is currently exercisable: `spacingOverride` is validated only to `≤ maxPlantSpacingCM = 10_000` (`plants.go:19`, `ops.go:101`) and `defaultPlopRadius = max(1.5*spacing, 15)` (`ops.go:77-79`), so a spacing of 10 000 yields radius 15 000 — over the 10 000 ceiling `finalizePlanting` would reject, but `FillRegion` accepts and inserts. The `PlantedAt` bypass is latent (today is produced via `s.now().UTC().Format(dateLayout)` so it is well-formed today), but a future change to how `today` is built would silently create rows the REST create path rejects. Suggested fix: call `finalizePlanting(p, o, false)` before `CreatePlanting` (bounds already checked above, so `checkBounds=false` is fine), or at minimum assert `radius <= maxRadiusCM`.
- **`DescribeGarden` emits an empty plant name when a planting's plant isn't in `plantByID`** — `internal/service/ops.go:280` (`Plant: plantByID[pl.PlantID].Name`). `full.Plants` comes from `ListReferencedPlants` and `full.Plantings` from `ListActivePlantingsForGarden` (`objects.go:171-178`); both filter `removed_at IS NULL` (`plants.go:36`, `plantings.go:36`) but are independent reads with no transaction. If a planting is soft-removed between the two reads, it appears in `full.Plantings` while its plant is absent from `full.Plants`, producing `"plant": ""`. (Hard plant deletion is blocked by `ON DELETE RESTRICT` — `0001_init.sql:126` — plus `CountPlantingsForPlant` in `plants.go:128-134`, so the realistic trigger is the soft-remove race, not deletion.) Impact is cosmetic (an empty JSON field in the agent-facing output, not a crash). Suggested fix: fall back to `"unknown"` or `fmt.Sprintf("plant#%d", pl.PlantID)` when the map lookup misses.
- **`hexCenters` can loop a very long time on an oversized caller-supplied `Region`** — `internal/service/ops.go:144-166`. `FillRegion` takes a caller-supplied `Region` that is never validated to lie within the object or clamped to `[-halfW,halfW]×[-halfH,halfH]`. `hexCenters` generates the full lattice *before* the `math.Abs(c.x) > halfW` guard at `ops.go:118` filters each point, so a region like `MinX=-1e9, MaxX=1e9` with a tiny radius would generate a huge lattice only to discard every point. There is no upper bound on the returned slice. Today the only caller is `FillNamedRegion` (`ops.go:182`), which builds the region from the object's own `WidthCM/HeightCM` via `NamedRegion` (`ops.go:45-73`) — safe — and no REST endpoint exposes `FillRegion` with an arbitrary region, so this is latent rather than active. Suggested fix: clamp the region to the object's bounds at the top of `FillRegion` before generating centers.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
**Minor issues**
- **`internal/service/ops.go:124`** — `FillRegion` creates plantings with a computed `RadiusCM = defaultPlopRadius(spacing)` but never enforces the `maxRadiusCM = 10_000` ceiling that `finalizePlanting` applies in the single-plop `CreatePlanting` path. Since `spacing` (or `spacingOverride`) can be as large as `maxPlantSpacingCM = 10_000`, `defaultPlopRadius` produces `15_000`, which exceeds the domain maximum. This creates plantings that violate the radius invariant; subsequent edits via `UpdatePlanting` would fail validation.
*Fix:* add `if radius > maxRadiusCM { return nil, domain.ErrInvalidInput }` before the hex-centers loop (or call `finalizePlanting` on each candidate).
- **`internal/service/ops.go:48-52`** — `NamedRegion` strips a trailing `"corner"` or `"half"` suffix. If the input is literally `"corner"` or `"half"` (or variants like `" corner "`), the stripped remainder is empty string, which matches `case "all", ""` and returns the full-object region instead of `ErrInvalidInput`. An LLM hallucinating an ambiguous region name could inadvertently trigger a full-object fill.
*Fix:* after suffix stripping, return `ErrInvalidInput` when the remaining string is empty and the original input was not `"all"`.
</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> — Minor issues</summary>
**Verdict:** Minor issues
- **`internal/service/ops.go:182-191` — Duplicate `objectForRole` call between `FillNamedRegion` and `FillRegion`**
`FillNamedRegion` fetches and authorizes the object, resolves the region, then calls `s.FillRegion(...)` which fetches and authorizes the **same object again**. This doubles the ACL query on every named fill and means two code sites enforce the same role rule. A cleaner pattern is an unexported `fillRegion` that takes the already-loaded `*domain.GardenObject`, with both exported methods calling it after their respective auth/geometry steps.
- **`internal/agent/tools_test.go:118` — String literal role `"viewer"` instead of constant**
The rest of the service tests (e.g. `ops_test.go:171`) use `domain.RoleViewer`. Using a raw string breaks the project's established pattern and is brittle if the canonical value changes. Import `internal/domain` and use `domain.RoleViewer`.
- **`internal/agent/tools_test.go:54-56` — Swallowed errors in test setup**
`CreatePlant` returns `(*domain.Plant, error)` but the test discards the error with `_, _` for garlic, basil, and beans. If plant creation fails, the test will later fail with a confusing downstream error (e.g. `plantId: 0` rejected) instead of surfacing the root cause. Check the errors like the rest of the test body and like `ops_test.go`'s `seedNamedPlant`.
- **`internal/service/ops.go:20-24` — Unused circle fields in `Region`**
The `Region` struct carries `Circle`, `CX`, `CY`, and `Radius` (documented as "used when Circle"), but nothing in this PR ever sets `Circle = true` and there is no `circle(...)` constructor. It's dead code that adds cognitive overhead. Either add a constructor + a code path that exercises it, or drop the circle fields until they are actually needed.
---
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
## Verdict: Minor issues
* **`internal/service/ops.go:125` — `FillRegion` issues one `INSERT … RETURNING` per plop.**
The loop over `hexCenters` calls `s.store.CreatePlanting` for every candidate, which is a single-row insert. There is no transaction wrapping or batching in the store layer (no `BeginTx` / bulk-insert API exists). For a large fill this is classic N+1 query behaviour — each plop is a separate DB round-trip and SQLite commit.
**Impact:** Filling a large region (e.g. a 2 m bed with 10 cm spacing) becomes proportionally slower and hammered by write overhead.
**Fix:** Wrap the fill in a transaction and add a batched `InsertPlantings` store method that inserts multiple rows in one statement.
* **`internal/service/ops.go:170` — `coveredByExisting` is an O(N) linear scan, making the fill O(M·N).**
For every lattice candidate, the code iterates over *all* existing plops (and appends newly created ones to the same slice, so the scan grows as the fill proceeds).
**Impact:** Coverage checking degrades quadratically. A fill that creates 100 plops performs ~5 000 distance checks; a 500-plop fill performs ~125 000.
**Fix:** Partition `existing` into a coarse spatial grid (e.g. keyed by `floor(x/radius), floor(y/radius)`) so only neighbours are checked.
* **`internal/store/plantings.go:64` — `ListActivePlantingsForObject` is unbounded and lacks a composite index.**
The query is `SELECT … WHERE object_id = ? AND removed_at IS NULL`. There is only a single-column index on `object_id`; because `ClearObjectPlantings` soft-removes rows (sets `removed_at`), an object that is cleared and refilled accumulates dead rows that are still reached by the `object_id` index and then filtered in-memory. There is no `LIMIT`.
**Impact:** Memory usage and query time grow with the total historical plop count for the object, not just active ones.
**Fix:** Add `CREATE INDEX idx_plantings_object_active ON plantings (object_id, removed_at);` and consider a `LIMIT` if objects can hold thousands of plantings.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- **`internal/service/ops.go:116-135` — `FillRegion` performs a non-atomic bulk insert.**
It loops over lattice candidates and calls `s.store.CreatePlanting` one at a time. If any intermediate call fails (DB constraint error, context cancellation, transient failure), the function returns `nil, err` but leaves the previously-created plantings already persisted. The garden ends up partially filled with no way to roll back.
*Fix:* wrap the loop in a DB transaction (begin → commit/rollback), or document the at-least-once semantics in the method comment.
- **`internal/service/ops.go:45-73` — `NamedRegion` dereferences `o` without a nil guard.**
Line 46 does `hw, hh := o.WidthCM/2, o.HeightCM/2`. If `o` is nil, this panics. The function is exported and test code (and any future caller) can pass a nil object directly.
*Fix:* add `if o == nil { return Region{}, domain.ErrInvalidInput }` at the top of `NamedRegion`.
- **`internal/service/ops.go:99-106` — `FillRegion` does not validate the computed plop radius.**
`plant.SpacingCM` is trusted from the DB (validated on insert), but if a bad value somehow exists (manual insertion, future bug, corrupted row), `defaultPlopRadius` could return `NaN` or `+Inf`. `hexCenters` only checks `radius <= 0`; a `NaN` radius causes the loop to silently produce zero plantings with no error returned.
*Fix:* after computing `radius`, add `if !isFinite(radius) || radius <= 0 { return nil, domain.ErrInvalidInput }`.
**Minor issues**
- **`internal/service/ops.go:196-202` — `ClearObject` does not check `o.Plantable`.**
`FillRegion` rejects non-plantable objects (`trees`, `paths`, etc.) with `ErrInvalidInput`, but `ClearObject` allows the call and simply clears 0 rows. This is harmless but inconsistent.
*Fix:* either return `ErrInvalidInput` for non-plantable objects, or document that clearing them is a no-op.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- FillRegion: insert the whole batch in one transaction (store.CreatePlantings)
instead of one round-trip per plop, and refuse fills over maxFillPlops (5000)
so a max-sized bed with tiny spacing can't generate ~10^5 sequential writes.
Guards the computed radius is finite/positive and clamps the region to the
object's bounds before packing (bounds hexCenters).
- FillNamedRegion + FillRegion share a fillLoaded body, so the object is loaded
and authorized once (no double objectForRole).
- Region is now rect-only — dropped the speculative circle fields/branch that
NamedRegion never produced and hexCenters didn't pack (circles are post-v1).
- NamedRegion guards a nil object and no longer maps a blank name to "all"
(blank → ErrInvalidInput, matching the doc).
- ClearObject documents why it deliberately doesn't require plantable.
Tests: empty/nil region name → ErrInvalidInput; an oversized fill → ErrInvalidInput
(over the cap). Tagged demo tidied (checked errors, domain.RoleViewer, t.Helper).
GOWORK=off go build/vet/test ./internal/... green; tagged agent test green against
majordomo (go.mod stays majordomo-free).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve
merged commit 18e4a299c2 into main2026-07-19 05:16:40 +00:00
steve
deleted branch phase-8-agent-seam2026-07-19 05:16:40 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #19. Part of epic #20 (Phase 8 — Agent seam). The final v1 issue.
What
Bulk service operations —
internal/service/ops.goAll on
*Service, so they funnel throughobjectForRole/requireGardenRoleand inherit ACL enforcement (a viewer'sFillRegion→ErrForbidden). Geometry is in each object's local frame (origin at center, +x east, +y south, so −y is north).Region+NamedRegion— resolvesnw|ne|sw|se(corners),north|south|east|westandtop|bottom|left|right(halves), andall; trailing "corner"/"half" ignored.FillRegion— hex-packs plops at 2×radius pitch (radius from #15'smax(1.5·spacing, 15cm)), clipped to the region, skipping any candidate that would sit entirely inside an existing active plop (idempotent-ish re-fills).FillNamedRegionis the agent-friendly by-name form. Returns what it created.ClearObject— soft-removes all active plops in one UPDATE; returns the count.DescribeGarden— structured summary (dims, objects + version, plantings with plant / effective count / rough compass location) for prompting.ListActivePlantingsForObject,ClearObjectPlantings.Agent toolbox —
internal/agent(deliberately isolated)doc.go(untagged) keeps the package in the default build;tools.gois behind themajordomobuild tag and majordomo is not ingo.mod, sogo build/test ./...and the server binary carry no majordomo/LLM deps — the acceptance's "core server still builds without majordomo". Built + tested locally against real majordomo (go test -tags majordomo ./internal/agent/).NewToolbox(svc, actorID)registerslist_gardens,describe_garden,create_object,move_object,place_planting,fill_region,clear_objectasllm.DefineTool[Args]wrappers — thin typed adapters over the service, each running as the bound actor.Tests
NamedRegionfor every name (+ unknown →ErrInvalidInput);defaultPlopRadius.ClearObject; viewer →ErrForbiddenon fill/clear but canDescribeGarden.TestFillScenario): garlic NE + basil NW + beans south →DescribeGardenshows three groups in the right locations.TestToolboxScenario,-tags majordomo): drives the same scenario through the toolbox (JSON args → tool → service) and confirms a viewer is refused.GOWORK=off go build/vet/test ./internal/...green (majordomo-free).Acceptance criteria
DescribeGardenshows three distinct groups in the right places, counts consistent with spacing.FillRegionon a rotated bed still fills the correct local-frame corner.fill_regiongetsErrForbidden; unit tests coverNamedRegionfor all names + deterministic packing.go test ./...green; the core server builds without majordomo (build-tag separation kept).Notes / choices
NewToolbox) is the clean path; adding it later is straightforward.cmd/pansy-agent-demo, since a deterministic test can't drive a real LLM loop.🤖 Generated with Claude Code
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-19 05:08:03Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 20 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +51,4 @@if err != nil {t.Fatalf("garden: %v", err)}garlic, _ := svc.CreatePlant(ctx, owner.ID, service.PlantInput{Name: "Garlic", Category: "vegetable", SpacingCM: 15, Color: "#d9d2c5", Icon: "🧄"})🟡 Swallowed CreatePlant errors in test setup
error-handling, maintainability · flagged by 2 models
internal/agent/tools_test.go:54-56— Swallowed errors in test setupCreatePlantreturns(*domain.Plant, error)but the test discards the error with_, _for garlic, basil, and beans. If plant creation fails, the test will later fail with a confusing downstream error (e.g.plantId: 0rejected) instead of surfacing the root cause. Check the errors like the rest of the test body and likeops_test.go'sseedNamedPlant.🪰 Gadfly · advisory
@@ -0,0 +115,4 @@// ACL: a viewer's fill_region is refused (the toolbox runs as that actor).viewerUser, _ := svc.Register(ctx, service.RegisterInput{Email: "[email protected]", DisplayName: "V", Password: "password123"})if _, err := svc.AddShare(ctx, owner.ID, g.ID, "[email protected]", "viewer"); err != nil {🟡 String literal role instead of domain.RoleViewer constant
maintainability · flagged by 1 model
internal/agent/tools_test.go:118— String literal role"viewer"instead of constant The rest of the service tests (e.g.ops_test.go:171) usedomain.RoleViewer. Using a raw string breaks the project's established pattern and is brittle if the canonical value changes. Importinternal/domainand usedomain.RoleViewer.🪰 Gadfly · advisory
@@ -0,0 +127,4 @@}}func mustJSON(v any) json.RawMessage {⚪ mustJSON helper swallows marshal errors and lacks t.Helper()
maintainability · flagged by 1 model
internal/agent/tools_test.go:130—mustJSONswallows errors and lackst.Helper(). The helper ignoresjson.Marshalerrors and doesn’t mark itself witht.Helper(), so a failure would print the wrong line number. It should panic on error and callt.Helper()for clearer test output.🪰 Gadfly · advisory
@@ -0,0 +17,4 @@// Region is an area in an object's local frame: an axis-aligned rectangle, or a// circle when Circle is set. NamedRegion produces the common ones.type Region struct {🟡 Region.Circle/CX/CY/Radius fields and contains circle branch are dead/speculative code never produced by NamedRegion and unsupported by hexCenters
maintainability · flagged by 4 models
internal/service/ops.go:20-24—Region.Circle/CX/CY/Radiusare dead/speculative fields.NamedRegion(the onlyRegionconstructor in the package) always returns rectangles viarect, which never setsCircle. A repo-wide grep forCircle =/Circle:/Circle: truefinds no assignment anywhere —Circleis only ever read in ther.Circlebranch ofRegion.contains(ops.go:27-33), which is therefore unreachable from any production path.hexCenters(ops.go:144-166) also d…🪰 Gadfly · advisory
@@ -0,0 +43,4 @@// A trailing "corner"/"half" word is ignored ("NE corner", "south half"). North// is -y (see the file header). Unknown names return ErrInvalidInput.func NamedRegion(o *domain.GardenObject, name string) (Region, error) {hw, hh := o.WidthCM/2, o.HeightCM/2🔴 NamedRegion nil pointer dereference on nil GardenObject
correctness, error-handling · flagged by 2 models
internal/service/ops.go:48-49(NamedRegion) — the literal strings"corner"or"half"alone silently resolve to"all"instead of being rejected.TrimSuffixdoesn't require a preceding separator: input"corner"→ strip suffix"corner"→"", which matches thecase "all", "":branch, filling the entire object rather than erroring. Same for"half". Low real-world likelihood (the toolbox description enumerates only compass-qualified values), but it's a genuine mismatch betw…🪰 Gadfly · advisory
@@ -0,0 +49,4 @@key = strings.TrimSpace(strings.TrimSuffix(key, "half"))switch key {case "all", "":🟡 Empty/blank region name silently maps to "all" instead of returning ErrInvalidInput, contradicting the doc and surprising bad-input handling
error-handling, security · flagged by 2 models
internal/service/ops.go:52— empty/blank region name silently maps to"all"instead ofErrInvalidInput. The doc comment (line 44) states "Unknown names return ErrInvalidInput," butcase "all", "":(line 52) accepts an empty string (after trim) as the whole-object rect. Through the agent tool this meansfill_regionwithregion: ""(or whitespace) fills the entire object rather than being rejected — a surprising edge case for bad input. Suggested fix: drop""from theallcas…🪰 Gadfly · advisory
@@ -0,0 +72,4 @@}}// defaultPlopRadius is #15's formula for a placed plop's radius: max(1.5×spacing,⚪ defaultPlopRadius comment says 'Reused here (don't duplicate the constant elsewhere)' but this is the only definition in the codebase — misleading phrasing
maintainability · flagged by 1 model
internal/service/ops.go:75-79— misleading "Reused here" comment ondefaultPlopRadius. The comment says "defaultPlopRadius is #15's formula... Reused here (don't duplicate the constant elsewhere)." But this is the only definition of the function in the codebase (grep fordefaultPlopRadiusreturns exactly this definition plus its one call site and two tests). The phrasing reads as if it's being imported/duplicated from #15's code, when it's actually the canonical definition. Minor; th…🪰 Gadfly · advisory
@@ -0,0 +103,4 @@}spacing = *spacingOverride}radius := defaultPlopRadius(spacing)🟠 FillRegion silent empty fill when computed radius is NaN/Inf
error-handling · flagged by 2 models
internal/service/ops.go:108-134—FillRegion's read-then-write is unguarded, so concurrent fills on the same object can create duplicate/overlapping plops.FillRegionloadsexistingplantings once viaListActivePlantingsForObject(line 108), computes hex-lattice candidates against that snapshot, then inserts one row at a time (line 125) with no transaction, row lock, or version check tying the read to the writes. By contrast,UpdateObject(internal/service/objects.go:136-149)…🪰 Gadfly · advisory
@@ -0,0 +113,4 @@today := s.now().UTC().Format(dateLayout)halfW, halfH := o.WidthCM/2, o.HeightCM/2created := []domain.Planting{}for _, c := range hexCenters(region, radius) {🔴 FillRegion performs one INSERT per lattice candidate (N+1 writes); worst case ~10^5 sequential round-trips on a max-sized bed with min spacing
correctness, error-handling, performance, security · flagged by 5 models
internal/service/ops.go:116-134—FillRegionissues one INSERT per candidate with no batching (N+1 writes). Each surviving lattice point callss.store.CreatePlantingindividually — a separateINSERT … RETURNINGround-trip to SQLite (verified atstore/plantings.go:122). There is no bulk insert helper anywhere ininternal/store(grep forBatchInsert|BulkCreate|batchreturns nothing). Objects are bounded only byvalidDimensionCM=maxGardenCM = 10_000cm (gardens.go:18, ap…🪰 Gadfly · advisory
@@ -0,0 +135,4 @@return created, nil}type localPoint struct{ x, y float64 }⚪ localPoint is a one-off struct with no reuse; minor parallel abstraction over plain (x,y) coords
maintainability · flagged by 1 model
internal/service/ops.go:138—localPointis a one-off internal struct defined separately fromRegionand used only byhexCenters/FillRegion. SinceRegion.containsalready takes(x, y float64),localPointadds a small parallel abstraction with no reuse elsewhere (grep confirms no other references). A plain(x, y float64)return would be one fewer type to track. Trivial; leave if you prefer the named clarity.🪰 Gadfly · advisory
@@ -0,0 +141,4 @@// Rows are spaced radius·√3 apart and every other row is offset by radius, the// standard hexagonal packing at a 2×radius pitch. The lattice is anchored one// radius inside the region's min corner so the first plop sits inside it.func hexCenters(r Region, radius float64) []localPoint {🟡 hexCenters generates the full lattice before the bounds guard filters it; an oversized caller-supplied Region (not clamped to the object) can cause a very long loop / large allocation
error-handling · flagged by 1 model
hexCenterscan loop a very long time on an oversized caller-suppliedRegion—internal/service/ops.go:144-166.FillRegiontakes a caller-suppliedRegionthat is never validated to lie within the object or clamped to[-halfW,halfW]×[-halfH,halfH].hexCentersgenerates the full lattice before themath.Abs(c.x) > halfWguard atops.go:118filters each point, so a region likeMinX=-1e9, MaxX=1e9with a tiny radius would generate a huge lattice only to discard every point.…🪰 Gadfly · advisory
@@ -0,0 +167,4 @@// coveredByExisting reports whether a new plop (center, radius) would sit// entirely inside some existing active plop.func coveredByExisting(x, y, radius float64, existing []domain.Planting) bool {🟠 coveredByExisting is O(N) linear scan making fill O(M*N)
performance · flagged by 1 model
internal/service/ops.go:170—coveredByExistingis an O(N) linear scan, making the fill O(M·N). For every lattice candidate, the code iterates over all existing plops (and appends newly created ones to the same slice, so the scan grows as the fill proceeds). Impact: Coverage checking degrades quadratically. A fill that creates 100 plops performs ~5 000 distance checks; a 500-plop fill performs ~125 000. Fix: Partitionexistinginto a coarse spatial grid (e.g. keyed by `floo…🪰 Gadfly · advisory
@@ -0,0 +179,4 @@// FillNamedRegion is FillRegion addressed by a compass name ("ne", "south half")// instead of a resolved Region — the ergonomic form for agent tools, which don't// hold the object's geometry. It resolves the name against the object, then fills.func (s *Service) FillNamedRegion(ctx context.Context, actorID, objectID int64, regionName string, plantID int64, spacingOverride *float64) ([]domain.Planting, error) {🟠 FillNamedRegion calls objectForRole twice (once itself, once via FillRegion)
correctness, error-handling, maintainability, performance · flagged by 5 models
internal/service/ops.go:182-191— RedundantobjectForRolecall inFillNamedRegion.FillNamedRegionfetches the object viaobjectForRole, resolves the region, then delegates toFillRegion, which callsobjectForRoleagain. This doubles the same permission/DB work for every named-region fill. Clean fix: extract a privatefillRegionthat accepts the already-resolved*domain.GardenObject, soFillNamedRegionfetches once andFillRegioncan still enforce its own ACL withou…🪰 Gadfly · advisory
@@ -0,0 +193,4 @@// ClearObject soft-removes every active plop in an object the actor can edit (one// UPDATE), returning how many were cleared. Distinct from deleting the object.func (s *Service) ClearObject(ctx context.Context, actorID, objectID int64) (int, error) {🟡 ClearObject inconsistent Plantable check compared to FillRegion
correctness, error-handling · flagged by 2 models
internal/service/ops.go:196-202—ClearObjectdoes not checko.Plantable.FillRegionrejects non-plantable objects (trees,paths, etc.) withErrInvalidInput, butClearObjectallows the call and simply clears 0 rows. This is harmless but inconsistent. Fix: either returnErrInvalidInputfor non-plantable objects, or document that clearing them is a no-op.🪰 Gadfly · advisory
@@ -0,0 +277,4 @@}do.Plantings = append(do.Plantings, DescribePlanting{PlantID: pl.PlantID,Plant: plantByID[pl.PlantID].Name,🟡 DescribeGarden emits an empty plant name when a planting's plant is missing from the (independently-read) referenced-plants map
error-handling · flagged by 1 model
DescribeGardenemits an empty plant name when a planting's plant isn't inplantByID—internal/service/ops.go:280(Plant: plantByID[pl.PlantID].Name).full.Plantscomes fromListReferencedPlantsandfull.PlantingsfromListActivePlantingsForGarden(objects.go:171-178); both filterremoved_at IS NULL(plants.go:36,plantings.go:36) but are independent reads with no transaction. If a planting is soft-removed between the two reads, it appears infull.Plantingswhile…🪰 Gadfly · advisory
@@ -0,0 +290,4 @@// describeLocation reverse-maps a local point to a rough compass location — the// inverse of NamedRegion's quarters/halves ("NE corner", "south", "center").func describeLocation(x, y float64) string {⚪ describeLocation's final default silently maps to 'west'; an explicit ew=="W" case would make the switch's exhaustiveness obvious
maintainability · flagged by 1 model
internal/service/ops.go:293-323—describeLocation's final switch is slightly leaky. The function buildsns/ewstrings then switches on their specific values (case ns == "N",case ns == "S",case ew == "E",default: return "west"). It works, but the trailingdefault: return "west"silently encodes "ew == W" — a future edit that adds a fourthns/ewstate would get "west" by accident. Minor readability nit; an explicitcase ew == "W":with a realdefault:that retur…🪰 Gadfly · advisory
@@ -0,0 +53,4 @@}// seedFillBed makes a plantable bed of the given size centered in a big garden.func seedFillBed(t *testing.T, s *Service, owner, gardenID int64, w, h float64) *domain.GardenObject {🟡 Duplicate seedFillBed helper; seedBed already exists in same package
maintainability · flagged by 1 model
internal/service/ops_test.go:56/internal/service/plantings_test.go:12— Duplicate test helperseedFillBedvsseedBed. Same package, same purpose (create a plantable bed), only the dimensions/position differ. Parameterize the existingseedBedhelper instead of adding a second one.🪰 Gadfly · advisory
@@ -0,0 +68,4 @@ctx := context.Background()s := newTestService(t, openConfig())owner := seedUser(t, s, "[email protected]")g, err := s.CreateGarden(ctx, owner, GardenInput{Name: "Big", WidthCM: 2000, HeightCM: 2000})⚪ 2000x2000 garden setup duplicated verbatim across 4 tests in two files instead of a shared seed helper
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +245,4 @@}// seedNamedPlant creates a custom plant with a specific name + spacing.func seedNamedPlant(t *testing.T, s *Service, owner int64, name string, spacingCM float64) *domain.Plant {🟡 Duplicate seedNamedPlant helper; seedOwnPlant already exists in same package
maintainability · flagged by 2 models
internal/service/ops_test.go:248/internal/service/plantings_test.go:24— Duplicate test helperseedNamedPlantvsseedOwnPlant. Both are in packageserviceand do the same thing (create a plant for an owner), differing only by whether the name is parameterized. Having two helpers for the same seeding operation is unnecessary copy-paste; consolidate into one helper with an optional name parameter.🪰 Gadfly · advisory
@@ -59,0 +61,4 @@// stacking new plops inside existing ones.func (d *DB) ListActivePlantingsForObject(ctx context.Context, objectID int64) ([]domain.Planting, error) {rows, err := d.sql.QueryContext(ctx,`SELECT `+plantingColumns+` FROM plantings WHERE object_id = ? AND removed_at IS NULL ORDER BY id`,🟠 ListActivePlantingsForObject loads unbounded rows with no composite index on (object_id, removed_at)
performance · flagged by 1 model
internal/store/plantings.go:64—ListActivePlantingsForObjectis unbounded and lacks a composite index. The query isSELECT … WHERE object_id = ? AND removed_at IS NULL. There is only a single-column index onobject_id; becauseClearObjectPlantingssoft-removes rows (setsremoved_at), an object that is cleared and refilled accumulates dead rows that are still reached by theobject_idindex and then filtered in-memory. There is noLIMIT. Impact: Memory usage and query ti…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 20 findings (9 with multi-model agreement)
internal/service/ops.go:116internal/service/ops.go:182internal/service/ops.go:20internal/service/ops.go:46internal/service/ops.go:106internal/agent/tools_test.go:54internal/service/ops.go:52internal/service/ops.go:196internal/service/ops_test.go:24811 single-model findings (lower confidence)
internal/service/ops.go:170internal/store/plantings.go:64internal/agent/tools_test.go:118internal/service/ops.go:144internal/service/ops.go:280internal/service/ops_test.go:56internal/agent/tools_test.go:130internal/service/ops.go:75internal/service/ops.go:138internal/service/ops.go:293internal/service/ops_test.go:71Per-model detail
claude-code/sonnet (claude-code) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The draft did not contain any actual findings — it only contains meta-commentary about a missing
ExitPlanModetool and a stray reference to "the DoS finding" location without ever stating what that finding is, where it is, or its impact. There is nothing concrete here to confirm against the code (no file:line claim, no described defect), so nothing carries forward.🎯 Correctness — Minor issues
Both findings verified directly against the code and store layer.
Finding 1 — confirmed.
internal/service/plantings.go:18setsmaxRadiusCM = 10_000;internal/service/plants.go:19setsmaxPlantSpacingCM = 10_000.FillRegion(ops.go:106) computesradius := defaultPlopRadius(spacing)=max(1.5·spacing, 15), so any spacing above10000/1.5 ≈ 6667cm(legal permaxPlantSpacingCM, and reachable directly viaspacingOverrideCmwhich is only checked againstminPlantSpacingCM/maxPlantSpacingCMatops.go:100-104) yields a radius >maxRadiusCM.ops.go:125callss.store.CreatePlanting— the raw store method, nots.CreatePlanting— which I confirmed ininternal/store/plantings.go:119-133performs zero validation (its own comment: "fields already validated by the service"). SofinalizePlanting's radius cap (plantings.go:235) is never applied on this path. I also confirmedfinalizePlanting's radius check (plantings.go:235) sits above and outside thecheckBoundsgate, so it runs unconditionally on everyUpdatePlantingcall regardless of what's being patched — a row created this way permanently failsErrInvalidInputon any future edit.Finding 2 — confirmed by tracing
NamedRegion(ops.go:45-73) by hand: input"corner"→TrimSuffix(key, "corner")→""→TrimSuffix("", "half")→"", which hitscase "all", "":and returns the full-object rectangle instead of erroring. Same for"half". This is a genuine behavior/intent mismatch, though real-world exposure is limited since the toolbox'sfill_regiondescription (internal/agent/tools.go) enumerates only compass-qualified values.VERDICT: Minor issues
internal/service/ops.go:106,125—FillRegionbypasses themaxRadiusCMinvariant, allowing an unfixable-later planting.FillRegioncomputesradius := defaultPlopRadius(spacing)(max(1.5·spacing, 15)) and inserts vias.store.CreatePlantingdirectly (the raw store call), skippingfinalizePlanting— the validator every other write path (CreatePlanting/UpdatePlantingininternal/service/plantings.go:101,145,235) goes through, which rejectsRadiusCM > maxRadiusCM(10,000cm,internal/service/plantings.go:18). SincemaxPlantSpacingCMis also 10,000cm (internal/service/plants.go:19), a plant with spacing abovemaxRadiusCM/1.5 ≈ 6667cm— a legal, in-bounds spacing value — produces a stored radius >10,000cm thatfinalizePlantingwould normally reject. Worse,finalizePlanting's radius check runs unconditionally on every futureUpdatePlantingcall (not gated bycheckBounds), so once such a row exists, any subsequent edit to that planting (even just a label change) permanently fails withErrInvalidInput.spacingOverridemakes this trivially reachable (passspacingOverrideCm: 7000, which only hits the existing min/max spacing bounds atops.go:101).radius, cap/validate it againstmaxRadiusCMbefore entering the loop, or route creation throughfinalizePlantingitself.internal/service/ops.go:48-49(NamedRegion) — the literal strings"corner"or"half"alone silently resolve to"all"instead of being rejected.TrimSuffixdoesn't require a preceding separator: input"corner"→ strip suffix"corner"→"", which matches thecase "all", "":branch, filling the entire object rather than erroring. Same for"half". Low real-world likelihood (the toolbox description enumerates only compass-qualified values), but it's a genuine mismatch between intent (unknown/degenerate name →ErrInvalidInput) and behavior (silently expands scope to the whole object).strings.TrimSuffix(key, " corner")), or explicitly rejectkey == "corner" || key == "half"before the switch.🧹 Code cleanliness & maintainability — Minor issues
Both findings independently verified against the actual source. Finding 1:
objectForRoleperformsstore.GetObject+requireGardenRole(objects.go:124-134), and it's called once inFillRegion(ops.go:88) and once more inFillNamedRegion(ops.go:183) before delegating toFillRegion, which repeats the lookup — confirmed redundant. Finding 2: theGardenInput{..., WidthCM: 2000, HeightCM: 2000}+CreateGardenpattern is duplicated verbatim inops_test.go:71,111,194andtools_test.go:50, whileseedGarden(objects_test.go:13, default 1000×1000) already exists as the shared-helper precedent — confirmed.VERDICT: Minor issues
internal/service/ops.go:183—FillNamedRegioncallsobjectForRoleto resolve the object, then immediately callsFillRegion, which callsobjectForRoleagain for the sameobjectID/actorID(verified both call sites at ops.go:88 and ops.go:183, going through the sames.store.GetObject+s.requireGardenRoleinobjects.go:124). This is the sole caller ofFillRegionfrom within the package that already hasoin hand but throws it away. Every agentfill_regioncall goes throughFillNamedRegion, so this redundant fetch-and-recheck is on the hot path. An unexportedfillRegion(ctx, actorID, o *domain.GardenObject, region, plantID, spacingOverride)core that bothFillRegionandFillNamedRegioncall after their own singleobjectForRolewould remove the duplicate authorization/DB round trip without changing either public signature.Test setup duplication: the literal
GardenInput{Name: "Big"/"Plot", WidthCM: 2000, HeightCM: 2000}+CreateGardenpair is copy-pasted acrossTestFillRegionDeterministicPacking(ops_test.go:71),TestFillRegionRotatedBedUsesLocalFrame(ops_test.go:111),TestFillScenario(ops_test.go:194), and again inTestToolboxScenario(internal/agent/tools_test.go:50). The package already has aseedGardenhelper (internal/service/objects_test.go:13) for the default-size (1000×1000) case; a second small helper (e.g.seedBigGarden) alongside it would remove four copies of the same three-line setup.⚡ Performance — Blocking issues found
Both findings check out against the actual code. All confirmed:
ops.go:116-135,hexCenters,coveredByExisting,defaultPlopRadius, theminPlantSpacingCM/maxGardenCM/validDimensionCMconstants and their reuse for objects, andCreatePlanting's unbatched single-row INSERT (plantings.go:122-133) with no transaction usage anywhere else in the service layer besides migrations.One correction: the draft's claim that
SetMaxOpenConns(1)(sqlite.go:50) "blocks all other DB access" is wrong for the reachable production path — that line only firesif memory(a bare:memory:DSN, used in tests per the doc comment atsqlite.go:37-39). Production (cmd/pansy/main.go:40) openscfg.DBPath, a real file, so the pool is NOT pinned to one connection there; WAL mode allows concurrent readers. I dropped that clause but kept the core finding, since the unbounded O(N²) work still hangs the request itself regardless of connection pooling.Corrected review
VERDICT: Blocking issues found
internal/service/ops.go:116-135(FillRegion) has unbounded, quadratic cost with no cap on candidate/region size.hexCenters(ops.go:144-166) generates a candidate grid over the whole region at2×radiuspitch, andradiusfloors at 15cm regardless of spacing (defaultPlopRadius,ops.go:77-78, combined withminPlantSpacingCM = 0.1inplants.go:18). Objects can be up tomaxGardenCM = 10_000cm (100m) per side (gardens.go:18, enforced for objects too viavalidDimensionCMatobjects.go:243). Filling"all"on a 100m×100m object at the 15cm radius floor yields on the order of ~1.3×10⁵ hex candidates. For every one of those,coveredByExisting(ops.go:170-176) does a linear scan overexisting, which itself grows by one for every accepted candidate (existing = append(existing, *stored)atops.go:133) — an O(N²) scan, ~10¹⁰ operations in the worst case. This is reachable through completely normal usage (an agent told to "fill the whole bed with basil," orfill_region/"all"on any large plantable object), not just adversarial input — it will hang the handling goroutine for the request long before it produces a response. Suggest bucketingexistinginto a spatial grid (cell size ≈2×radius) so coverage checks are O(1) average instead of O(N), and/or capping total region area or candidate count with a clearErrInvalidInputwhen exceeded.internal/service/ops.go:124-133— each accepted candidate triggers its owns.store.CreatePlantingcall, andCreatePlanting(internal/store/plantings.go:122-133) is a singleINSERT ... RETURNINGwith no surrounding transaction — and no transaction/batching pattern exists anywhere else in the service or store layers to model this on (the onlyBeginTxin the codebase is in the migration runner,store/migrate.go:79). For a fill producing hundreds of plops (a realistic case even without hitting the pathological size above), this is N sequential DB round trips/autocommits instead of one batched insert, so cost scales with plop count purely from per-statement overhead rather than being amortized in a single transaction. Suggest wrapping the fill loop in oneBeginTx/Commit(or a multi-row insert) so the whole region fill is one write.Everything else in this lens is fine:
ClearObject/ClearObjectPlantingsis a singleUPDATE(store/plantings.go:88-104) as advertised;DescribeGardenreusesGardenFull's existing bulk queries plus O(1) map lookups (ops.go:243-289) — no N+1 there.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
internal/service/ops.go:108-134—FillRegion's read-then-write is unguarded, so concurrent fills on the same object can create duplicate/overlapping plops.FillRegionloadsexistingplantings once viaListActivePlantingsForObject(line 108), computes hex-lattice candidates against that snapshot, then inserts one row at a time (line 125) with no transaction, row lock, or version check tying the read to the writes. By contrast,UpdateObject(internal/service/objects.go:136-149) andUpdatePlanting(internal/service/plantings.go:112+) are explicitly version-guarded (WHERE id = ? AND version = ?). There's noTx/BeginTxusage anywhere ininternal/storeexcept the one-time schema migration (internal/store/migrate.go:79), confirming ops.go's writes are unprotected. The server is a standardnet/http.Server(cmd/pansy/main.go:60-73), and for a real (non-:memory:) SQLite path the connection pool is not pinned to 1 (internal/store/sqlite.go:46-51only pins:memory:DBs, used in tests), so genuine concurrentList/Insertinterleaving across goroutines is possible in production. Two concurrentfill_regioncalls (or a client retry racing the original) on the same object can each see the same staleexistingset and insert plops at the same lattice positions, defeating the "skip if covered" idempotency and stacking overlapping plops.internal/service/ops.go:116-134— a mid-loopCreatePlantingfailure leaves a partial fill silently committed with no rollback, and the caller only sees an error, not what was already written. Confirmed no transaction wraps the loop (same absence ofTxnoted above). For N hex-packed candidates, if the k-thstore.CreatePlantingcall fails, plops 1..k-1 are already committed individually (each is its ownINSERT ... RETURNING,internal/store/plantings.go:122-133) butFillRegionreturns(nil, err)at line 127 — the caller gets no indication of the partial write. There is no other multi-row write path in this codebase to compare against for precedent;ClearObjectPlantingsis a single UPDATE statement.internal/agent/tools_test.go:54-56—svc.CreatePlanterrors are swallowed (garlic, _ := ...), inconsistent with the rest of the test's error checking, risking a nil-pointer panic instead of a clear failure message. Every other setup call in this test checks the error witht.Fatalf(garden creation at lines 50-53, andcreate_object/describe_gardencalls throughout).CreatePlantreturns(*domain.Plant, error)and returnsnilon error (internal/service/plants.go:68-83). If any of the threeCreatePlantcalls fails,garlic/basil/beansarenil, and the subsequentgarlic.ID/basil.ID/beans.IDfield accesses (used building the fill-region argument list at line 76) dereference a nil pointer, panicking instead of producing a readable test failure.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
Through the security lens, the change is sound. I verified the authz and input-handling paths against the actual code:
FillRegion/FillNamedRegion/ClearObjectall route throughobjectForRole(..., roleEditor)(internal/service/objects.go:124), andDescribeGardengoes throughGardenFull→requireGardenRole(..., roleViewer)(internal/service/objects.go:162). A viewer is correctly refused on fill/clear and allowed on describe (confirmed byTestOpsForbiddenForViewerand the tagged toolbox test). The toolbox binds a fixedactorIDand reuses these same service methods, so it inherits the ACLs unchanged.ListActivePlantingsForObjectandClearObjectPlantings(internal/store/plantings.go) use?-parameterized queries;objectID/dateare bound, not interpolated.plantingColumnsis a package constant, not user input.regionname is resolved by pure Go string-matching inNamedRegionwith anErrInvalidInputfallback for unknown names — never reaches SQL.spacingOverrideis checked withisFinite+ the[minPlantSpacingCM, maxPlantSpacingCM]bounds (internal/service/ops.go:101, constants atinternal/service/plants.go:18).plantIDis gated byvisiblePlant(internal/service/plantings.go:178), which masks non-existent/non-owned plants asErrInvalidInput(no existence oracle).CreateObject/CreatePlantingtool wrappers delegate to the existingfinalizeObject/finalizePlantingvalidation.One borderline note (outside my lens, correctness more than security):
FillRegionwrites plops vias.store.CreatePlantingdirectly, bypassingfinalizePlanting'smaxRadiusCM/count validation. In practice this is bounded — the radius is server-derived from a validated spacing (max(1.5·spacing, 15)with spacing ≤ 10000cm), centers are server-computed, and the center-in-object guard is reimplemented inline — so it does not create an exploitable input path. Noting it only for completeness; it is not a security finding.🎯 Correctness — No material issues found
VERDICT: No material issues found
Through the correctness lens, I re-derived the key formulas and packing geometry against the checked-out code and they hold:
defaultPlopRadius(ops.go:77):max(1.5·spacing, 15). spacing=4 → 6<15 → 15 ✓; spacing=20 → 30 ✓. Matches the existing #15 formula and the test expectations.derivedCount(plantings.go:28):max(1, round(π·r²/spacing²))with themaxExplicitCountcap — reused correctly inFillRegion(ops.go:129) and inDescribeGardencount reporting (ops.go:274).hexCenters,ops.go:144): pitch=2·radius, rowH=pitch·√3/2. For the 60×60 bed / radius-15 test case I walked the lattice by hand: row 0 (y=−15) → x=−15,15; row 1 (y≈+10.98) → x=0,30; row 2 (y≈+36.96) out of range ⇒ exactly 4 plops, matching the asserted count and the in-bounds assertion.coveredByExisting,ops.go:170):dist + radius <= e.RadiusCMwith<=means an exact-on-top candidate (dist=0) is covered ⇒ second fill yields 0. Verified.NamedRegion/describeLocation): north = −y consistently in both the forward map (ops.go:51-69) and the inverse map (ops.go:293-322). NE corner (x>0, y<0) → "NE corner"; south-half beans start at y=MinY+radius>0 ⇒ never "center"/"north". The rotated-bed test relies onFillRegionusingo.WidthCM/HeightCM(local) and ignoringRotationDeg, which it does. Verified.FillRegion/ClearObjectgo throughobjectForRole(..., roleEditor),FillNamedRegiondouble-checks via the same path, andDescribeGardenthroughGardenFull→requireGardenRole(..., roleViewer). The viewer-forbidden / viewer-can-describe behavior follows. Verified againstobjects.go:124-189.Minor (non-blocking, not strictly a bug):
FillRegionbuilds thedomain.Plantingand callsstore.CreatePlantingdirectly, bypassingfinalizePlanting(plantings.go:234) — so themaxRadiusCMceiling and explicitvalidDatePtrchecks aren't applied to bulk plops. In practice the radius is bounded bydefaultPlopRadiusover a spacing that is itself validated to[minPlantSpacingCM, maxPlantSpacingCM], andPlantedAtis set tos.now().Format(dateLayout), so this gap isn't currently reachable. Flagging only as a latent invariant (bulk path diverges from the single-plant path's validation), not as a live correctness defect.The
Region.Circlefields are dead code (noNamedRegionbranch nor any public caller ever setsCircle), but that's an unused-code observation, not incorrect behavior.🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
Through the code-cleanliness & maintainability lens, the change is well-structured and follows the surrounding service patterns. A few genuine dead-code/duplication nits survive verification:
internal/service/ops.go:20-24—Region.Circle/CX/CY/Radiusare dead/speculative fields.NamedRegion(the onlyRegionconstructor in the package) always returns rectangles viarect, which never setsCircle. A repo-wide grep forCircle =/Circle:/Circle: truefinds no assignment anywhere —Circleis only ever read in ther.Circlebranch ofRegion.contains(ops.go:27-33), which is therefore unreachable from any production path.hexCenters(ops.go:144-166) also doesn't honorCircle— it iterates the bounding box (r.MinX/MaxX/MinY/MaxY), so even if a circle were constructed the packer would fill the bounding rectangle, not the disk. Either drop the four fields and thecontainscircle branch (YAGNI), or implement circle support inhexCenterstoo. As-is it's misleading surface area.internal/service/ops_test.go:248-257—seedNamedPlantis a near-duplicate ofseedOwnPlant(internal/service/plantings_test.go:24-31). Both verified via read_file: they differ only inName(param vs hardcoded"Testroot"),Category(CategoryVegetablevsCategoryHerb), and icon (🌱vs🌿). A singleseedPlant(t, s, owner, name, category, spacing, icon)would cover both. Minor duplication, worth consolidating before more variants accrete.internal/service/ops.go:138—localPointis a one-off internal struct defined separately fromRegionand used only byhexCenters/FillRegion. SinceRegion.containsalready takes(x, y float64),localPointadds a small parallel abstraction with no reuse elsewhere (grep confirms no other references). A plain(x, y float64)return would be one fewer type to track. Trivial; leave if you prefer the named clarity.Nothing else in this lane: the adapter/toolbox wiring in
internal/agent/tools.gois thin and consistent, the store additions mirror existingplantings.gopatterns, and the build-tag separation is cleanly done.⚡ Performance — Minor issues
Verdict: Minor issues
internal/service/ops.go:125(FillRegion) inserts plops one-at-a-time inside the loop — each candidate issues its owns.store.CreatePlantingINSERT/RETURNING round-trip. There is no batch insert and no transaction wrapping the set, so an agentfill_regionover a large region with a small-spacing plant is N serial DB writes. A single large bed (e.g. a few hundred plops) becomes a few hundred sequential round-trips. Confirmed by readingCreatePlanting(internal/store/plantings.go:122), which is a single-rowINSERT ... RETURNING. Suggested fix: batch the inserts in one transaction, or expose a bulk store method. Impact: performance degradation on large fills (medium).internal/service/ops.go:121/internal/service/ops.go:170—coveredByExistingis O(existing) per candidate, andexistingis appended to on every placed plop (internal/service/ops.go:133). Within oneFillRegioncall the coverage scan is therefore O(candidates × placed) ≈ quadratic in the number of plops being laid down, with no spatial index. For the small fills in the tests this is trivial, but a whole-gardenallfill on a big bed degrades quadratically. Confirmed thatexistinggrows every iteration. Suggested fix: keep the existing-plop check O(N) only against pre-existing plops and use a uniform-grid/spatial hash for the "what I just placed" set, or accept overlap among same-fill siblings. Impact: quadratic scaling on large fills (small).internal/service/ops.go:183(FillNamedRegion) callsobjectForRoleand then delegates toFillRegion, which callsobjectForRoleagain (internal/service/ops.go:88). EachobjectForRoleis aGetObject+requireGardenRoleround-trip (verified atinternal/service/objects.go:124andinternal/service/gardens.go:66;requireGardenRoleitself doesGetGarden+effectiveGardenRole), so every agentfill_regiondoes the role/object lookups twice. Minor duplicated-query cost; suggest resolving the region in the caller and callingFillRegiononce, or caching the loaded object. Impact: redundant queries per fill (small).🧯 Error handling & edge cases — Minor issues
All three findings are confirmed against the actual code.
Verdict: Minor issues
internal/service/ops.go:116-134—FillRegionis non-atomic; a mid-loop store error leaves orphaned plops. The bulk operation creates plops oneCreatePlantingat a time (line 125) andreturn nil, erron the first failure (line 126-128), discarding thecreatedslice. There is no transaction wrapper — I confirmed the service package has noTx/BeginTxhelper (onlyinternal/store/migrate.goandinternal/api/oidc_test.gomention transactions, and the latter is a test). A bulk fill widens the partial-failure window: an actor asking "fill the south half" can end up with some plops persisted and an error returned, with no partial-success signal and no rollback. Re-running won't fix it cleanly because the skipped-candidate logic (coveredByExisting, line 121) treats the already-placed plops as "existing" (line 121 + 133 appends stored plops toexisting). Suggested fix: wrap the loop in a transaction and rollback on the first insert error, or return thecreatedslice alongside the error so the caller can recover.internal/service/ops.go:52— empty/blank region name silently maps to"all"instead ofErrInvalidInput. The doc comment (line 44) states "Unknown names return ErrInvalidInput," butcase "all", "":(line 52) accepts an empty string (after trim) as the whole-object rect. Through the agent tool this meansfill_regionwithregion: ""(or whitespace) fills the entire object rather than being rejected — a surprising edge case for bad input. Suggested fix: drop""from theallcase so it falls through todefault→ErrInvalidInput, or explicitly validatekey == ""before the switch.internal/service/ops.go:182-192—FillNamedRegionfetches the object twice viaobjectForRole.FillNamedRegioncallss.objectForRole(line 183) to load the object, then callss.FillRegion(line 191), which itself callss.objectForRoleagain (line 88). The redundant ACL/GetObjectlookup doubles the store round-trips per fill and widens the delete-race window (an object deleted between the two lookups surfaces as a confusing error from the second call). Low-impact; introduced by the bulk op. Suggested fix: haveFillNamedRegionresolve the region from the object it already fetched and call an internal fill that doesn't re-fetch, or pass the already-loaded object through.kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
I audited the change through the Security lens. Here is what I verified:
Auth / AuthZ — All new operations (
FillRegion,FillNamedRegion,ClearObject,DescribeGarden) correctly gate onobjectForRole/requireGardenRolewith the expected minimum role (roleEditorfor mutations,roleViewerfor reads). No bypass path was found.Injection — All SQL in the new store methods (
ListActivePlantingsForObject,ClearObjectPlantings) uses parameterised queries (?placeholders). No string concatenation of user input into SQL. Thedatepassed toClearObjectPlantingsis server-generated (s.now().UTC().Format(dateLayout)), not user-controlled.Input validation —
NamedRegionrejects unknown names withErrInvalidInput.FillRegionvalidatesspacingOverrideagainst the sameminPlantSpacingCM/maxPlantSpacingCMbounds used elsewhere, and checksisFinite. TheRegioncoordinates are derived only from the already-validated object dimensions, never from raw user input.Unsafe deserialization / SSRF / secret leakage — None present in this diff. The majordomo integration uses statically-typed JSON struct tags; there is no reflection-based unmarshaling of untrusted Go types and no HTTP client usage.
DoS / resource exhaustion — The minimum plop radius is 15 cm (
defaultPlopRadiusfloors at 15), sohexCenterscannot generate an unbounded number of iterations. On the maximum 100 m garden the lattice yields at most ~10⁵ candidates, andcoveredByExistingprevents duplicate insertion on re-fill. A garden editor could already achieve comparable load via individualCreatePlantingcalls, so the bulk path does not materially expand the attack surface.Outside my lens: The radius generated by
defaultPlopRadius(maxPlantSpacingCM)exceedsmaxRadiusCM, whichFillRegiondoes not catch because it bypassesfinalizePlanting. This is a correctness / validation consistency issue, not a security vulnerability (it results in zero placements, not an integrity breach).🎯 Correctness — Minor issues
Verdict: Minor issues
internal/service/ops.go:118— Circular object clipping bug: TheFillRegionguardmath.Abs(c.x) > halfW || math.Abs(c.y) > halfHclips candidates to the axis-aligned bounding square of the object. Sincecreate_object(and the domain) already supports"circle"shapes (diameter =WidthCM), a circular bed with e.g. 400 cm diameter will incorrectly accept plop centers at(200, 200)— outside the actual circle (distance 282 > radius 200) — as long as they are inside the bounding square. The region and hex lattice are both rectangular, so plops end up in the corners of circular objects. The fix is to branch ono.Shape == "circle"(or"circle") and usemath.Hypot(c.x, c.y) > halfWfor the bound check instead of the box guard.internal/service/ops.go:183—FillNamedRegionredundant ACL / object fetch:FillNamedRegioncallsobjectForRolewithroleEditor, then callsFillRegionwhich callsobjectForRoleagain. This double-fetches the object and double-checks permissions. If the two calls race (rare but possible under concurrent role changes), the second check could spuriously fail after the first passed. More importantly, the extra query is unnecessary overhead. Pass the already-resolved object into a package-private variant, or drop the first check and letFillRegionbe the sole gate.🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
internal/service/ops.go:182-191— RedundantobjectForRolecall inFillNamedRegion.FillNamedRegionfetches the object viaobjectForRole, resolves the region, then delegates toFillRegion, which callsobjectForRoleagain. This doubles the same permission/DB work for every named-region fill. Clean fix: extract a privatefillRegionthat accepts the already-resolved*domain.GardenObject, soFillNamedRegionfetches once andFillRegioncan still enforce its own ACL without duplicating the round-trip.internal/service/ops.go:20-24— Dead fields onRegion.Circle,CX,CY, andRadiusare never constructed or exercised in this PR.containshandles them, but every call site (including tests) only uses rectangles. Either remove the circle support until it’s needed, or add at least one constructor/test so the struct doesn’t carry unused API surface.internal/service/ops_test.go:248/internal/service/plantings_test.go:24— Duplicate test helperseedNamedPlantvsseedOwnPlant.Both are in package
serviceand do the same thing (create a plant for an owner), differing only by whether the name is parameterized. Having two helpers for the same seeding operation is unnecessary copy-paste; consolidate into one helper with an optional name parameter.internal/service/ops_test.go:56/internal/service/plantings_test.go:12— Duplicate test helperseedFillBedvsseedBed.Same package, same purpose (create a plantable bed), only the dimensions/position differ. Parameterize the existing
seedBedhelper instead of adding a second one.internal/agent/tools_test.go:130—mustJSONswallows errors and lackst.Helper().The helper ignores
json.Marshalerrors and doesn’t mark itself witht.Helper(), so a failure would print the wrong line number. It should panic on error and callt.Helper()for clearer test output.⚡ Performance — Blocking issues found
Blocking issues found
FillNamedRegionperforms duplicate ACL work — it callsobjectForRole(ops.go:183), resolves the name, then delegates toFillRegion, which callsobjectForRoleagain (ops.go:88). Each call hitsGetObject+requireGardenRole(which may doGetGarden+GetShareRole), so a named-region fill costs roughly double the DB round-trips of a rawFillRegionfor the same permission check.FillRegionemits oneINSERTper plop — inside thehexCentersloop it callsstore.CreatePlantingindividually (ops.go:125). For even a modest 2 m × 2 m bed with 15 cm radius plops this is dozens of SQLite round-trips; for a large bed near the 100 m cap it can be thousands. The store has no batch-insert API, so this N+1 pattern is an inherent regression for a method whose entire purpose is bulk creation.coveredByExistingis O(N) per candidate and N grows during the fill — the loop at ops.go:116-134 scans every existing (and already-created-in-this-fill) plop for each candidate. For a re-fill on a bed with many plops, this compounds with the per-insert DB calls above. Theexistingslice also undergoes repeatedappendwithout pre-sized capacity, causing reallocations and copies as the fill proceeds.🧯 Error handling & edge cases — Minor issues
Minor issues
internal/service/ops.go:118— The object-bounds guardmath.Abs(c.x) > halfW || math.Abs(c.y) > halfHuses strict>instead of>=(or a radius-aware check). When a hex-lattice center lands exactly on the object boundary (e.g., a 60 cm-wide bed with a 20 cm radius plop produces a center at x = 30), the plop is allowed even though it extends outside the object. The comment says “center must be inside”, but the check permits boundary centers, and with positive radius half the plop protrudes past the edge. A radius-aware guard likemath.Abs(c.x)+radius > halfWwould keep the entire plop within bounds.internal/service/ops.go:116-135—FillRegionsilently returns an empty slice when the region is too small to fit even one plop (e.g., a tiny bed or a very large spacing override). The caller gets success with zero items created and no indication that the fill was effectively a no-op. An explicit check afterhexCenters—or inside it—should return an error such asErrInvalidInputwhen the region dimensions are smaller than the computed plop diameter so the agent/user knows why nothing was planted.opencode/glm-5.2:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
Reviewed through the security lens (authn/authz, injection, input validation, info leakage, untrusted-data handling). I verified the key claims against the checked-out code:
FillRegion/FillNamedRegion/ClearObjectall gate onobjectForRole(..., roleEditor)(ops.go:88, 183, 197), andDescribeGardenroutes throughGardenFull→requireGardenRole(..., roleViewer)(objects.go:162-163). A viewer is denied mutations and only granted describe, matching the test inops_test.go. Confirmed by readingobjectForRole/requireGardenRole.ListActivePlantingsForObject,ClearObjectPlantings) useQueryContext/ExecContextwith?placeholders; no string interpolation of user input. Thedatearg intoClearObjectPlantingsoriginates froms.now().UTC().Format(dateLayout)inside the service, not from the caller.spacingOverrideis bounds-checked withisFiniteand[minPlantSpacingCM, maxPlantSpacingCM]before use (ops.go:101-104).NamedRegionmaps free-form region names through a fixedswitch— unknown names returnErrInvalidInput; no path/eval surface.NewToolbox(svc, actorID)binds a single actor and every handler runs through service methods that re-check roles per call, so the actor identity is not trusted from the tool layer. Thedoc.gonote that this must be constructed per authenticated session is the right contract; authn itself is correctly left to the caller (the majordomo/executus harness outside this repo).ErrForbidden,ErrInvalidInput,ErrNotFound), not detailed internal messages.DescribeGardenonly exposes objects/plantings of a garden the actor can already view viaGardenFull;Versionis intentionally surfaced as the mutation guard, not sensitive material.Nothing material in my lens.
🎯 Correctness — Minor issues
Review
Verdict: Minor issues
Findings (all verified against the actual checked-out code):
internal/service/ops.go:124-129—FillRegionbypasses themaxRadiusCMinvariant enforced byCreatePlanting.CreatePlantingvalidates every plop viafinalizePlanting, which rejectsRadiusCM > maxRadiusCM(10000, plantings.go:235).FillRegioninstead callss.store.CreatePlantingdirectly (ops.go:125) withradius = defaultPlopRadius(spacing) = max(1.5*spacing, 15)(ops.go:106/77-79). A plant with spacing > ~6666.67 cm (valid:maxPlantSpacingCM = 10000, plants.go:19), or aspacingOverridenear the upper limit (validated only againstmaxPlantSpacingCMat ops.go:101), yields a radius > 10000 thatCreatePlantingwould refuse butFillRegionhappily inserts. The schema has no CHECK onradius_cm(0001_init.sql:129), so the row is persisted. Suggested fix: gateradiusagainstmaxRadiusCMinFillRegion, or cap the spacing range so1.5*spacing ≤ maxRadiusCM.internal/service/ops.go:129vsinternal/service/objects.go:186—DerivedCountmismatch whenspacingOverrideis used.FillRegioncomputesDerivedCountwith the overridespacing(ops.go:129), butGardenFull(which backsDescribeGarden) recomputes it fromplant.SpacingCM(objects.go:186), the un-overridden value. So an agent that fills with an override and then callsdescribe_gardensees a different count thanfill_regionreturned. The persistedCountis unaffected (always nil/derived here), so this is only a transient display inconsistency. Minor.internal/service/ops.go:196-201—ClearObjectskips theremoved_at >= planted_atordering check enforced byfinalizePlantingfor single updates.UpdatePlantingrejectsremoved < planted(plantings.go:256-261);ClearObject's bulk UPDATE setsremoved_at = todayfor all active plops regardless of theirplanted_at. A plop with a futureplanted_at(allowed byvalidDatePtr, plantings.go:267-273) would end up withremoved_at < planted_at. Edge case (future plantings are unusual) and per-row validation in a bulk UPDATE is impractical; trivial severity — noted for completeness.The hex-packing math,
NamedRegioncompass mapping, re-fill idempotency (the 4-plop lattice lands exactly on existing centers), rotated-bed local-frame behavior, ACL enforcement (viewer →ErrForbiddenviaobjectForRole(...,roleEditor)), and thedescribeLocationinverse mapping all check out against first-principles re-derivation.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues found
internal/service/ops.go:20-24— vestigial circle support onRegion. TheRegionstruct exposesCircle,CX,CY,Radiusfields andcontains()(ops.go:27-33) has aCirclebranch, but nothing in the repo ever setsCircle = true. I grepped the whole tree: the onlyRegion{...}literals are viarect()(ops.go:36-38, which produces rectangles) andNamedRegion'sdefaultarm returns a zeroRegionwithCirclefalse. The only otherCirclematches are an unrelateddomain.ShapeCircleobject-shape constant and a test that creates a circle object — neither touchesRegion.Circle. The exported type's doc ("...or a circle when Circle is set") advertises a shape mode that is unreachable in practice. This is dead API surface that future readers will have to reason about. Suggested fix: drop the four circle fields and theif r.Circle { ... }branch, or actually wire the circle case (e.g. forShape: "circle"objects) if that's intended.internal/service/ops.go:75-79— misleading "Reused here" comment ondefaultPlopRadius. The comment says "defaultPlopRadius is #15's formula... Reused here (don't duplicate the constant elsewhere)." But this is the only definition of the function in the codebase (grep fordefaultPlopRadiusreturns exactly this definition plus its one call site and two tests). The phrasing reads as if it's being imported/duplicated from #15's code, when it's actually the canonical definition. Minor; the "don't duplicate the constant elsewhere" directive is oddly aimed at the very file that holds the original. Suggested fix: drop "Reused here" or reword to "This is the canonical definition; don't re-derive the 1.5×spacing / 15 cm floor elsewhere."internal/service/ops.go:293-323—describeLocation's final switch is slightly leaky. The function buildsns/ewstrings then switches on their specific values (case ns == "N",case ns == "S",case ew == "E",default: return "west"). It works, but the trailingdefault: return "west"silently encodes "ew == W" — a future edit that adds a fourthns/ewstate would get "west" by accident. Minor readability nit; an explicitcase ew == "W":with a realdefault:that returns""(or panics) would make the exhaustive intent obvious. Not blocking.⚡ Performance — Blocking issues found
I'll verify each finding against the actual code.
All three findings are confirmed against the actual code:
s.store.CreatePlantingonce per surviving candidate, no batch helper exists in the store.coveredByExistingis a linear scan (lines 170-177), andexisting = append(existing, *stored)grows the slice on each insert.FillNamedRegioncallss.objectForRole(line 183), then delegates toFillRegionwhich callss.objectForRoleagain (line 88) on the sameobjectID.objectForRole(objects.go:124) does aGetObject+requireGardenRoleDB read each call.The bounds reasoning is also valid:
validDimensionCMcaps atmaxGardenCM = 10_000(gardens.go:18), andminPlantSpacingCM = 0.1(plants.go:18).VERDICT: Blocking issues found
internal/service/ops.go:116-134—FillRegionissues one INSERT per candidate with no batching (N+1 writes). Each surviving lattice point callss.store.CreatePlantingindividually — a separateINSERT … RETURNINGround-trip to SQLite (verified atstore/plantings.go:122). There is no bulk insert helper anywhere ininternal/store(grep forBatchInsert|BulkCreate|batchreturns nothing). Objects are bounded only byvalidDimensionCM=maxGardenCM = 10_000cm (gardens.go:18, applied viafinalizeObjectatobjects.go:243), and the minimum plant spacing isminPlantSpacingCM = 0.1(plants.go:18). The plop radius floor ismax(1.5·spacing, 15) = 15cm, giving a hex pitch of 30 cm and row height ~26 cm. On a maximally-sized 100 m × 100 m bed that is ~128,000 candidates → up to ~128k sequential INSERTs on a single agent call. Suggested fix: collect the surviving candidates and insert them in one transactional batch (the store already usesBeginTxinmigrate.go), or expose aCreatePlantings(ctx, []*Planting)bulk path.internal/service/ops.go:121,133—coveredByExistingmakesFillRegionO(N²) in created plops. Each candidate scans the entireexistingslice, and the loop appends each just-stored plop back intoexisting(existing = append(existing, *stored)). VerifiedcoveredByExisting(ops.go:170-177) is a linear scan with no spatial index. With N survivors this is N existing-scans per candidate → ~N²/2 distance comparisons. Suggested fix: build a spatial filter keyed by hex lattice cell (amap[localPoint]struct{}of taken cells) so the "already covered" check is O(1).internal/service/ops.go:182-191—FillNamedRegionperforms the ACL/object fetch twice.FillNamedRegioncallss.objectForRole(...)(a DB read + role check) and then delegates toFillRegion, which callss.objectForRole(...)again on the sameobjectID(ops.go:88). VerifiedobjectForRoleatobjects.go:124loads the object and does a share/role lookup each call — so this is a doubled DB round-trip on every agentfill_regioncall. Suggested fix: haveFillNamedRegionresolve the name and call an internalfillRegionthat accepts the already-loaded*domain.GardenObject, or haveFillRegionaccept a pre-fetched object.🧯 Error handling & edge cases — Minor issues
I've verified all three findings against the actual code.
Finding 1 (FillRegion bypasses finalizePlanting): Confirmed.
ops.go:124-129builds the plop by hand and callss.store.CreatePlantingdirectly.plantings.go:101shows the normal create path callsfinalizePlanting(p, o, true).plantings.go:235enforcesp.RadiusCM > maxRadiusCM(10000).defaultPlopRadius=max(1.5*spacing, 15); withspacingOverridevalidated only to≤ maxPlantSpacingCM = 10000(plants.go:19,ops.go:101), radius can reach 15000, exceeding the 10000 ceiling the REST path rejects.PlantedAtis well-formed today (s.now().UTC().Format(dateLayout)), so only the radius cap is currently exercisable. Keep.Finding 2 (DescribeGarden empty plant name): Confirmed structurally.
ops.go:280doesPlant: plantByID[pl.PlantID].Namewith no fallback.objects.go:171-178runsListActivePlantingsForGardenandListReferencedPlantsas independent queries with no transaction.plants.go:31-57andplantings.go:32-57both filterremoved_at IS NULL, so a planting soft-removed between the two reads would appear infull.Plantingsbut its plant would be absent fromfull.Plants, yielding"". Plant deletion is blocked byON DELETE RESTRICT(0001_init.sql:126) andCountPlantingsForPlant(plants.go:128-134), so the realistic trigger is the soft-remove race, not plant deletion — but the empty-field impact stands. Keep (impact is cosmetic, as the draft notes).Finding 3 (hexCenters unbounded loop on oversized region): Confirmed.
ops.go:144-166generates the full lattice beforeops.go:118filters bymath.Abs(c.x) > halfW.FillRegion(ops.go:87) takes a caller-suppliedRegionwith no clamping/intersection with object bounds. Current reachability: onlyFillNamedRegion(ops.go:182) calls it, which builds the region viaNamedRegionfrom the object's ownWidthCM/HeightCM(ops.go:45-73) — safe. No REST endpoint exposesFillRegionwith an arbitrary region (grep ofinternal/apifound none). So this is latent, not active — matching the draft's "latent rather than active" framing. Keep as small/medium.All three findings survive verification. Verdict remains "Minor issues".
Review (lens: Error handling & edge cases)
Verdict: Minor issues
FillRegionbypassesfinalizePlantingvalidation —internal/service/ops.go:124-129. The plop is built by hand and inserted vias.store.CreatePlantingdirectly, skipping thefinalizePlanting(p, o, true)gate that every other create path (CreatePlantingatplantings.go:101) uses. The local-frame center is bounded by themath.Abs(c.x) > halfWguard, andradius/todayare well-formed by construction today, but two invariants are lost: the radius upper bound (maxRadiusCM = 10_000,plantings.go:18,235) and thePlantedAtdate-format check (validDatePtr,plantings.go:252,267-273). The radius cap is currently exercisable:spacingOverrideis validated only to≤ maxPlantSpacingCM = 10_000(plants.go:19,ops.go:101) anddefaultPlopRadius = max(1.5*spacing, 15)(ops.go:77-79), so a spacing of 10 000 yields radius 15 000 — over the 10 000 ceilingfinalizePlantingwould reject, butFillRegionaccepts and inserts. ThePlantedAtbypass is latent (today is produced vias.now().UTC().Format(dateLayout)so it is well-formed today), but a future change to howtodayis built would silently create rows the REST create path rejects. Suggested fix: callfinalizePlanting(p, o, false)beforeCreatePlanting(bounds already checked above, socheckBounds=falseis fine), or at minimum assertradius <= maxRadiusCM.DescribeGardenemits an empty plant name when a planting's plant isn't inplantByID—internal/service/ops.go:280(Plant: plantByID[pl.PlantID].Name).full.Plantscomes fromListReferencedPlantsandfull.PlantingsfromListActivePlantingsForGarden(objects.go:171-178); both filterremoved_at IS NULL(plants.go:36,plantings.go:36) but are independent reads with no transaction. If a planting is soft-removed between the two reads, it appears infull.Plantingswhile its plant is absent fromfull.Plants, producing"plant": "". (Hard plant deletion is blocked byON DELETE RESTRICT—0001_init.sql:126— plusCountPlantingsForPlantinplants.go:128-134, so the realistic trigger is the soft-remove race, not deletion.) Impact is cosmetic (an empty JSON field in the agent-facing output, not a crash). Suggested fix: fall back to"unknown"orfmt.Sprintf("plant#%d", pl.PlantID)when the map lookup misses.hexCenterscan loop a very long time on an oversized caller-suppliedRegion—internal/service/ops.go:144-166.FillRegiontakes a caller-suppliedRegionthat is never validated to lie within the object or clamped to[-halfW,halfW]×[-halfH,halfH].hexCentersgenerates the full lattice before themath.Abs(c.x) > halfWguard atops.go:118filters each point, so a region likeMinX=-1e9, MaxX=1e9with a tiny radius would generate a huge lattice only to discard every point. There is no upper bound on the returned slice. Today the only caller isFillNamedRegion(ops.go:182), which builds the region from the object's ownWidthCM/HeightCMviaNamedRegion(ops.go:45-73) — safe — and no REST endpoint exposesFillRegionwith an arbitrary region, so this is latent rather than active. Suggested fix: clamp the region to the object's bounds at the top ofFillRegionbefore generating centers.opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Minor issues
internal/service/ops.go:124—FillRegioncreates plantings with a computedRadiusCM = defaultPlopRadius(spacing)but never enforces themaxRadiusCM = 10_000ceiling thatfinalizePlantingapplies in the single-plopCreatePlantingpath. Sincespacing(orspacingOverride) can be as large asmaxPlantSpacingCM = 10_000,defaultPlopRadiusproduces15_000, which exceeds the domain maximum. This creates plantings that violate the radius invariant; subsequent edits viaUpdatePlantingwould fail validation.Fix: add
if radius > maxRadiusCM { return nil, domain.ErrInvalidInput }before the hex-centers loop (or callfinalizePlantingon each candidate).internal/service/ops.go:48-52—NamedRegionstrips a trailing"corner"or"half"suffix. If the input is literally"corner"or"half"(or variants like" corner "), the stripped remainder is empty string, which matchescase "all", ""and returns the full-object region instead ofErrInvalidInput. An LLM hallucinating an ambiguous region name could inadvertently trigger a full-object fill.Fix: after suffix stripping, return
ErrInvalidInputwhen the remaining string is empty and the original input was not"all".🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
internal/service/ops.go:182-191— DuplicateobjectForRolecall betweenFillNamedRegionandFillRegionFillNamedRegionfetches and authorizes the object, resolves the region, then callss.FillRegion(...)which fetches and authorizes the same object again. This doubles the ACL query on every named fill and means two code sites enforce the same role rule. A cleaner pattern is an unexportedfillRegionthat takes the already-loaded*domain.GardenObject, with both exported methods calling it after their respective auth/geometry steps.internal/agent/tools_test.go:118— String literal role"viewer"instead of constantThe rest of the service tests (e.g.
ops_test.go:171) usedomain.RoleViewer. Using a raw string breaks the project's established pattern and is brittle if the canonical value changes. Importinternal/domainand usedomain.RoleViewer.internal/agent/tools_test.go:54-56— Swallowed errors in test setupCreatePlantreturns(*domain.Plant, error)but the test discards the error with_, _for garlic, basil, and beans. If plant creation fails, the test will later fail with a confusing downstream error (e.g.plantId: 0rejected) instead of surfacing the root cause. Check the errors like the rest of the test body and likeops_test.go'sseedNamedPlant.internal/service/ops.go:20-24— Unused circle fields inRegionThe
Regionstruct carriesCircle,CX,CY, andRadius(documented as "used when Circle"), but nothing in this PR ever setsCircle = trueand there is nocircle(...)constructor. It's dead code that adds cognitive overhead. Either add a constructor + a code path that exercises it, or drop the circle fields until they are actually needed.⚡ Performance — Minor issues
Verdict: Minor issues
internal/service/ops.go:125—FillRegionissues oneINSERT … RETURNINGper plop.The loop over
hexCenterscallss.store.CreatePlantingfor every candidate, which is a single-row insert. There is no transaction wrapping or batching in the store layer (noBeginTx/ bulk-insert API exists). For a large fill this is classic N+1 query behaviour — each plop is a separate DB round-trip and SQLite commit.Impact: Filling a large region (e.g. a 2 m bed with 10 cm spacing) becomes proportionally slower and hammered by write overhead.
Fix: Wrap the fill in a transaction and add a batched
InsertPlantingsstore method that inserts multiple rows in one statement.internal/service/ops.go:170—coveredByExistingis an O(N) linear scan, making the fill O(M·N).For every lattice candidate, the code iterates over all existing plops (and appends newly created ones to the same slice, so the scan grows as the fill proceeds).
Impact: Coverage checking degrades quadratically. A fill that creates 100 plops performs ~5 000 distance checks; a 500-plop fill performs ~125 000.
Fix: Partition
existinginto a coarse spatial grid (e.g. keyed byfloor(x/radius), floor(y/radius)) so only neighbours are checked.internal/store/plantings.go:64—ListActivePlantingsForObjectis unbounded and lacks a composite index.The query is
SELECT … WHERE object_id = ? AND removed_at IS NULL. There is only a single-column index onobject_id; becauseClearObjectPlantingssoft-removes rows (setsremoved_at), an object that is cleared and refilled accumulates dead rows that are still reached by theobject_idindex and then filtered in-memory. There is noLIMIT.Impact: Memory usage and query time grow with the total historical plop count for the object, not just active ones.
Fix: Add
CREATE INDEX idx_plantings_object_active ON plantings (object_id, removed_at);and consider aLIMITif objects can hold thousands of plantings.🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
internal/service/ops.go:116-135—FillRegionperforms a non-atomic bulk insert.It loops over lattice candidates and calls
s.store.CreatePlantingone at a time. If any intermediate call fails (DB constraint error, context cancellation, transient failure), the function returnsnil, errbut leaves the previously-created plantings already persisted. The garden ends up partially filled with no way to roll back.Fix: wrap the loop in a DB transaction (begin → commit/rollback), or document the at-least-once semantics in the method comment.
internal/service/ops.go:45-73—NamedRegiondereferencesowithout a nil guard.Line 46 does
hw, hh := o.WidthCM/2, o.HeightCM/2. Ifois nil, this panics. The function is exported and test code (and any future caller) can pass a nil object directly.Fix: add
if o == nil { return Region{}, domain.ErrInvalidInput }at the top ofNamedRegion.internal/service/ops.go:99-106—FillRegiondoes not validate the computed plop radius.plant.SpacingCMis trusted from the DB (validated on insert), but if a bad value somehow exists (manual insertion, future bug, corrupted row),defaultPlopRadiuscould returnNaNor+Inf.hexCentersonly checksradius <= 0; aNaNradius causes the loop to silently produce zero plantings with no error returned.Fix: after computing
radius, addif !isFinite(radius) || radius <= 0 { return nil, domain.ErrInvalidInput }.Minor issues
internal/service/ops.go:196-202—ClearObjectdoes not checko.Plantable.FillRegionrejects non-plantable objects (trees,paths, etc.) withErrInvalidInput, butClearObjectallows the call and simply clears 0 rows. This is harmless but inconsistent.Fix: either return
ErrInvalidInputfor non-plantable objects, or document that clearing them is a no-op.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.