Closes#77. Part of #86. You chose option 3: keep clumps as the default primitive, add a grid/rows fill mode — so sketching and planning are different operations with different outputs, rather than one model forced to be both.
Why
A plop is a clump, not a plant. That's the right primitive for sketching ("a few plops of garlic in a corner") but it can't draw a real planting — a filled 4×8 ft bed comes out as ~15 blobs, not 8 rows of garlic (the whole of #77).
grid — radius spacing/2, pitch = spacing, one plant per plop (derivedCount at r=spacing/2 is π/4 → 1). Rows you could actually plant from.
The geometry is the same hexCenters lattice and the same #75 half-spacing edge rule — only the radius→spacing relationship differs (plopRadiusFor). Grid keeps no 15 cm floor (its point is true spacing); clump keeps it so a tiny-spacing plant doesn't make invisible clumps.
The agent's fill_region tool — a mode arg (enum clump|grid), so "plant the bed in rows" works.
Tests
Grid produces many more, single-plant plops than clump on the same bed (radius spacing/2, derived count 1).
Unknown layout refused at both the service and the API (400).
maxFillPlops still caps a grid fill of a huge bed with tiny spacing.
Not here
There's no frontend fill affordance yet — fill is agent-only in the UI (the fill UI was deferred in #82). So the mode toggle rides along whenever that fill UI is built. Grid mode is fully usable now via the agent ("plant the west bed in rows") and the REST endpoint.
Perf note: a grid-filled bed approaches the low-hundreds-of-plops the SVG budget was sized for; PlopMarker is already memo'd and semantic-zoom-tiered, which anticipates it. Worth a glance on a full garden when the fill UI lands.
Docs: DESIGN placement-model decision.
GOWORK=off go test ./... green; gofmt -l internal/ clean.
Closes #77. Part of #86. You chose **option 3**: keep clumps as the default primitive, add a grid/rows fill mode — so sketching and planning are different operations with different outputs, rather than one model forced to be both.
## Why
A plop is a **clump**, not a plant. That's the right primitive for *sketching* ("a few plops of garlic in a corner") but it can't draw a real planting — a filled 4×8 ft bed comes out as ~15 blobs, not 8 rows of garlic (the whole of #77).
## What
`FillLayout` selects what a fill packs:
- **`clump`** (default, unchanged) — radius `1.5×spacing`, pitch `2×radius`, ~7 plants per plop. Quick coverage.
- **`grid`** — radius `spacing/2`, pitch `= spacing`, **one** plant per plop (`derivedCount` at `r=spacing/2` is `π/4 → 1`). Rows you could actually plant from.
The geometry is the **same `hexCenters` lattice** and the **same #75 half-spacing edge rule** — only the radius→spacing relationship differs (`plopRadiusFor`). Grid keeps **no** 15 cm floor (its point is true spacing); clump keeps it so a tiny-spacing plant doesn't make invisible clumps.
Threaded through:
- `FillRegion`/`FillNamedRegion` — empty layout = clump (existing callers unchanged), unknown layout = `ErrInvalidInput`.
- `POST /objects/:id/fill` — a `layout` field.
- The agent's `fill_region` tool — a `mode` arg (enum `clump|grid`), so **"plant the bed in rows"** works.
## Tests
- Grid produces many more, single-plant plops than clump on the same bed (radius `spacing/2`, derived count 1).
- Unknown layout refused at **both** the service and the API (400).
- `maxFillPlops` still caps a grid fill of a huge bed with tiny spacing.
## Not here
There's no frontend fill affordance yet — fill is agent-only in the UI (the fill *UI* was deferred in #82). So the **mode toggle rides along whenever that fill UI is built**. Grid mode is fully usable now via the agent ("plant the west bed in rows") and the REST endpoint.
Perf note: a grid-filled bed approaches the low-hundreds-of-plops the SVG budget was sized for; `PlopMarker` is already memo'd and semantic-zoom-tiered, which anticipates it. Worth a glance on a full garden when the fill UI lands.
Docs: DESIGN placement-model decision.
`GOWORK=off go test ./...` green; `gofmt -l internal/` clean.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Steve chose option 3: keep clumps as the default primitive, add a grid/rows
fill mode, so sketching and planning are different operations with different
outputs rather than one model forced to be both.
A plop is a CLUMP, not a plant — great for "a few plops of garlic in a corner",
useless for drawing a plantable 8-rows-of-garlic bed (that came out as ~15
blobs, #77). FillLayout selects what a fill packs:
- clump (default, unchanged): radius 1.5×spacing, ~7 plants per plop.
- grid: radius spacing/2, pitch = spacing, ONE plant per plop — rows you could
actually plant from.
The geometry is the SAME hexCenters lattice and the SAME #75 half-spacing edge
rule; only the radius→spacing relationship differs (plopRadiusFor). Grid keeps
no 15cm floor — its whole point is true spacing — while clump keeps it so a
tiny-spacing plant doesn't make invisible clumps.
Threaded through FillRegion/FillNamedRegion (empty layout = clump, so existing
callers are unchanged; unknown layout = ErrInvalidInput), the REST /fill
endpoint (`layout`), and the agent's fill_region tool (`mode`, enum clump|grid),
so "plant the bed in rows" works.
Tests: grid produces many more, single-plant plops than clump on the same bed
(radius spacing/2, derived count 1); unknown layout is refused at both the
service and the API. maxFillPlops still caps a grid fill of a huge bed.
No frontend fill affordance exists yet (fill is agent-only in the UI; the fill
UI was deferred in #82), so the mode toggle rides along when that's built —
noted. Docs: DESIGN placement-model decision.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 04:14:40Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ⚠️ **security** — could not complete
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ⚠️ **error-handling** — could not complete
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
Regionstring`json:"region" description:"nw|ne|sw|se corner, north|south|east|west (or top|bottom|left|right) half, or all"`
PlantIDint64`json:"plantId" description:"plant to fill with"`
SpacingOverride*float64`json:"spacingOverrideCm" description:"optional in-row spacing override in cm; omit to use the plant's spacing"`
Modestring`json:"mode" enum:"clump,grid" description:"clump (default) drops a few fat clumps for a quick sketch; grid lays out individual plants in rows at true spacing, a layout you could plant from"`
🟡enum struct tag appears unused by pansy agent schema machinery; external majordomo/llm behavior unverified, may be dead/misleading metadata
maintainability · flagged by 1 model
internal/agent/tools.go:130 — enum:"..." struct tag. The new Mode field carries enum:"clump,grid". Within pansy's own internal/agent package no machinery consumes an enum tag — the only occurrence repo-wide is this one line, and runtime.go doesn't parse enum either; the tool definitions route through the external gitea.stevedudenhoeffer.com/steve/majordomo/llmDefineTool builder, which I could not inspect (not vendored in-repo), so whether the downstream generator honor…
🪰 Gadfly · advisory
🟡 **enum struct tag appears unused by pansy agent schema machinery; external majordomo/llm behavior unverified, may be dead/misleading metadata**
_maintainability · flagged by 1 model_
- **`internal/agent/tools.go:130` — `enum:"..."` struct tag.** The new `Mode` field carries `enum:"clump,grid"`. Within pansy's own `internal/agent` package no machinery consumes an `enum` tag — the only occurrence repo-wide is this one line, and `runtime.go` doesn't parse `enum` either; the tool definitions route through the external `gitea.stevedudenhoeffer.com/steve/majordomo/llm` `DefineTool` builder, which I could not inspect (not vendored in-repo), so whether the downstream generator honor…
<sub>🪰 Gadfly · advisory</sub>
Verdict: Blocking issues found · 7 findings (2 with multi-model agreement)
Finding
Where
Models
Lens
🟡
FillRegion/fillLoaded doc comments not updated for the new layout parameter
internal/service/ops.go:143
4/5
maintainability
🔴
Grid layout's shared inset formula (radius - spacing/2) always evaluates to 0 since grid radius = spacing/2, so grid's outermost plants sit flush on/overhanging the bed edge instead of the intended half-spacing inset
internal/service/ops.go:287
3/5
correctness
5 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
Grid mode's smaller plop radius multiplies the self-quadratic coveredByExisting scan by ~81x for routine fills, pushing normal usage toward the 5000-plop worst case
internal/service/ops.go:212
claude-code/sonnet
performance
🟡
enum struct tag appears unused by pansy agent schema machinery; external majordomo/llm behavior unverified, may be dead/misleading metadata
internal/agent/tools.go:130
glm-5.2:cloud
maintainability
🟡
Grid mode's larger N amplifies the O(N^2) coveredByExisting scan; the in-loop append to 'existing' is unnecessary for same-fill coverage
internal/service/ops.go:217
glm-5.2:cloud
performance
🟡
hexCenters comment assumes clump model but function is now shared with grid mode
internal/service/ops.go:253
kimi-k2.6:cloud
maintainability
⚪
TestFillAndClearAPI now mixes fill/clear/grid/bad-layout concerns, breaking the file's one-test-per-concern convention
internal/api/ops_test.go:60
claude-code/sonnet
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Minor issues
Reviewed this PR strictly through the security lens (authz, injection, unsafe input handling, DoS via unbounded input).
What I checked:
Authorization is unchanged.FillRegion/FillNamedRegion still call objectForRole(ctx, actorID, objectID, roleEditor) before any layout-related work (internal/service/ops.go:152, :362), so the new layout parameter doesn't create a bypass path to the editor-role check.
Layout input is a closed enum, safe by construction.req.Layout from the API (internal/api/ops.go:54) and args.Mode from the agent tool (internal/agent/tools.go:130) are arbitrary strings, but validFillLayout (internal/service/ops.go:135) accepts only "", "clump", "grid" and rejects everything else with ErrInvalidInput, enforced server-side regardless of what the LLM sends. The value is never interpolated into SQL, shell, templates, or file paths — it only selects a radius via a switch. Confirmed via grep that no other code path consumes the raw string.
DoS via smaller grid radius is still capped. Grid mode's radius (spacing/2) packs more plops per region than clump, but the maxFillPlops check in fillLoaded (internal/service/ops.go:196-198) runs on total from hexCenters after plopRadiusFor computes the (now smaller) radius — so the existing cap applies uniformly to both layouts, matching the PR's own test (TestFillRegionCappedForHugeArea still passes, and a grid-specific cap case is implied covered). Verified by reading the code path, not just the diff.
Validation order doesn't leak info. Layout is validated before visiblePlant is called, so an invalid layout returns 400 without touching plant-visibility logic — no new oracle for plant existence.
No injection, authz, or unsafe-deserialization concerns in this change; it's a narrow, well-contained enum threaded through existing, already-vetted authorization and validation paths.
🎯 Correctness — Minor issues
No hidden override elsewhere — hexCenters has exactly one call site and it's the one traced above.
VERDICT: Minor issues
internal/service/ops.go:287 (inset := math.Max(0, radius-math.Max(0, spacing)/2)), combined with plopRadiusFor (internal/service/ops.go:123-127) returning spacing/2 for grid mode: the inset always evaluates to 0 for grid fills, regardless of spacing, because radius - spacing/2 collapses to spacing/2 - spacing/2. The formula was written for clump mode (where a plop's rim, not center, represents the outermost plants, so backing the radius off by half a spacing correctly approximates the half-spacing edge rule); grid mode's plop center is the plant, so the target inset is spacing/2 directly, not radius - spacing/2. Hand-traced against the PR's own TestFillGridLaysOutIndividualPlants fixture (60×60cm bed, spacing 10 ⇒ radius 5): inset=0 places the outermost row centers exactly at the bed's true boundary (fitAxis(60,10,0) → positions 0..60, i.e. the true edges), with half of each edge plant's own spacing/2 personal-space circle overhanging the frame — not the half-spacing-inside placement the feature (and DESIGN.md) describes as the point of grid mode. Neither new test (TestFillGridLaysOutIndividualPlants, the API grid test in internal/api/ops_test.go) asserts center/edge position, so this passes CI silently. Affects every grid fill.
Suggested fix: don't reuse the clump-derived radius - spacing/2 inset for grid; grid's target inset is spacing/2 independent of radius (e.g. thread the layout into the inset calculation, or special-case inset = spacing/2 when layout == FillGrid).
🧹 Code cleanliness & maintainability — Minor issues
Both draft findings verified against the actual code.
VERDICT: Minor issues
internal/service/ops.go:143-150,159-162 — Stale/incomplete doc comments on FillRegion and fillLoaded.FillRegion's doc still says "Plop radius comes from the plant's spacing (or spacingOverride) via defaultPlopRadius" (ops.go:144-145) — true only for FillClump; grid mode's radius comes from plopRadiusFor/spacingCM/2 (ops.go:123-128, 182). Neither this doc nor fillLoaded's (ops.go:159-162, which lists non-finite-region rejection, bounds clamping, and maxFillPlops but omits the new validFillLayout check at ops.go:167-170) mentions the new layout parameter at all. Confirmed by reading the current comments against the current bodies — signatures and logic changed in this PR, the prose above them didn't. This is exactly the failure mode CLAUDE.md flags ("stale comments are worse than none — the next reader believes them").
internal/api/ops_test.go:60-119 — TestFillAndClearAPI grew to cover four unrelated scenarios under a name that no longer describes them. Confirmed via grep -n "^func Test": only 4 test functions exist in the file (TestFillAndClearAPI, TestFillRegionSelectionAPI, TestClearObjectIsOneChangeSetAPI, TestFillClearPermissionsAPI), and TestFillAndClearAPI is the only one bundling multiple concerns — the original fill/clear/empty-clear assertions (lines 65-97) plus the newly appended grid-layout and unknown-layout-400 assertions (lines 99-118), which don't fit the function's name or its doc comment ("covers the two routes end to end"). The sibling service-layer tests added in this same PR correctly split into separate functions (TestFillGridLaysOutIndividualPlants, TestFillRejectsUnknownLayout).
⚡ Performance — Minor issues
VERDICT: Minor issues
internal/service/ops.go:211-217 (fillLoaded) — coveredByExisting (ops.go:349-356) is an O(candidates × existing-so-far) linear scan invoked once per candidate, with existing growing by one on every accepted candidate (existing = append(existing, *p), ops.go:217), making a single fill's own scan cost intrinsically quadratic in candidate count — independent of any plops already on the object. Grid mode's radius (plopRadiusFor, ops.go:123-128) is spacing/2 versus clump's max(1.5×spacing, 15) (defaultPlopRadius, ops.go:96-98) — a 3× radius ratio for typical spacings (>10cm, where clump isn't floor-clamped). Since hexCenters (ops.go:271-319) produces a lattice whose count scales as 1/radius² (rows ~ 1/radius via rowH, cols ~ 1/radius via pitch), grid mode produces roughly 9× the candidates of clump mode on the same region/spacing, and since the scan cost is quadratic in candidate count, the total coveredByExisting cost is roughly 9² ≈ 81× worse for a grid fill versus the equivalent clump fill on the same bed. This is bounded by the pre-existing maxFillPlops = 5000 (ops.go:23), confirmed by reading the cap check at ops.go:200-203, so it isn't unbounded — but a fill reaching close to that cap now means on the order of 5000²/2 ≈ 12.5Mmath.Hypot comparisons (roughly the draft's 25M order-of-magnitude estimate, using the product as a loose upper bound rather than the exact triangular sum) as a plausible outcome of ordinary "fill this bed in rows at real spacing" usage, rather than requiring an abusive tiny-spacing/huge-area input as it did pre-PR for clump mode. ListActivePlantingsForObject (internal/store/plantings.go:130-134, confirmed) does fetch every active planting on the whole object, not scoped to the fill region, which compounds this further on an object that already carries prior plantings. The PR's own DESIGN.md addition discusses the higher plop count only in terms of the SVG rendering budget ("approaches the low-hundreds-of-plops the SVG budget was sized for"), not this service-side scan cost. TestFillGridLaysOutIndividualPlants (internal/service/ops_test.go) only exercises a 60×60cm bed, confirmed too small to expose this. Worth a quick timing check on a realistic worst-case grid fill (e.g. a few-thousand-cm² bed at ~5cm spacing) before merging, since this runs synchronously inside one HTTP request.
🧯 Error handling & edge cases — No material issues found
No main branch present locally, but that's fine — I've verified the diff directly against the checked-out state. No frontend changes are needed/present, consistent with the PR description.
I've completed my review through the error-handling & edge-cases lens.
VERDICT: No material issues found
Findings from checking the actual behavior (not just the diff):
Radius validation holds for grid mode.plopRadiusFor drops the 15cm floor for FillGrid, but fillLoaded (internal/service/ops.go:182-185) still checks !isFinite(radius) || radius <= 0 right after computing it, and plant spacing is already bounded to [0.1, 10_000] cm at creation time (internal/service/plants.go:319) and for spacingOverride inline (ops.go:177). So grid mode can never see a zero/negative/non-finite radius in practice — verified by reading both validation sites.
validFillLayout correctly rejects unknowns and defaults empty to clump (ops.go:132-141), called before the plant lookup, so a bad layout string fails fast without doing unnecessary work. Verified the switch is exhaustive (only "", clump, grid pass).
maxFillPlops cap is layout-agnostic and pre-allocation.hexCenters (ops.go:271-319) computes total analytically via fitAxis and returns (nil, total) before ever building the points slice if total > limit — so a grid fill over a huge bed with tiny spacing can't runaway-allocate even though the PR's claimed "capped" test wasn't actually added (only the existing clump-mode cap test remains, ops_test.go:54). This is a testing-coverage gap, not an actual unhandled edge case — the guard logic itself is sound and doesn't care whether the radius came from clump or grid math.
Grid's inset = radius - spacing/2 degenerates to exactly 0 (since radius == spacing/2 for grid), which fitAxis (ops.go:335-345) handles correctly — no divide-by-zero or negative-count path, it's just the ordinary "no inset" case already exercised by the existing lattice logic.
Every touched call site in tests/agent/api that gained the new layout FillLayout parameter was updated consistently (FillClump passed explicitly) — no stale caller left with a mismatched signature that would panic or fail to compile (confirmed via the diff and grep for remaining unmigrated call sites — none found).
Nothing here swallows an error, skips cleanup, or leaves a nil/zero/negative edge case unhandled that wasn't already guarded before this PR.
glm-5.2:cloud (ollama-cloud) — Blocking issues found
Through the security lens, I traced the new untrusted inputs to where they're used:
layout / mode field — The string flows from both the REST objectFillRequest.Layout (internal/api/ops.go:54) and the agent fill_region tool's Mode (internal/agent/tools.go:130) into FillRegion/FillNamedRegion as a FillLayout, and is validated exactly once in fillLoaded via validFillLayout (internal/service/ops.go:167-170), which rejects anything other than ""/clump/grid with domain.ErrInvalidInput. I verified ErrInvalidInput maps to HTTP 400 at internal/api/errors.go:54-55, matching the test expectations for the unknown-layout case. The value never reaches SQL, path construction, or command execution — it's only compared against two string constants and used to branch a radius formula. No injection or bypass surface.
No default-accept risk — Empty string defaults to clump (backwards-compatible, safe), and the default branch of the switch returns false rather than falling through, so there's no path where an unknown value silently maps to a privileged/default behavior. The non-ok path returns before radius is computed, so the plopRadiusFor(spacing, layout) call at line 182 only ever runs on validated layouts.
No other untrusted inputs were added (spacing/plant/region handling is unchanged from the prior, already-reviewed code). The change is a clean enum-through-validation pattern with no security-relevant gaps.
🎯 Correctness — Blocking issues found
VERDICT: Blocking issues found
Through the correctness lens, I re-derived the grid-mode edge geometry from first principles and confirmed the PR's central claim — "the same #75 half-spacing edge rule for both" — is not actually true for grid mode.
internal/service/ops.go:287 / plopRadiusFor (grid): plants land on the bed edge, not half a spacing inside — violating the #75 rule the PR claims to preserve.
The half-spacing edge rule (DESIGN.md; the hexCenters comment at ops.go:244‑259) says the outer row owes only half a spacing: the outermost plant center sits half a spacing inside the bed edge (the square-foot-chart arithmetic: 6" apart ⇒ 3" from the frame).
hexCenters computes the edge inset as inset := math.Max(0, radius - math.Max(0, spacing)/2) (ops.go:287). For clump this is correct: radius = 1.5·spacing, so inset = 1.5sp − 0.5sp = sp, and the clump's rim (where its plants sit) hangs over by exactly half a spacing, putting the outermost plants at the half-spacing line.
For grid, plopRadiusFor (ops.go:123‑128) sets radius = spacing/2, so the formula gives inset = spacing/2 − spacing/2 = 0. But in grid mode the plop is the plant — the single plant sits at the plop center, not at a rim. So with inset = 0 the outer plant's center is placed on the bed edge, not half a spacing in. Re-deriving on the test's own 60×60 bed, spacing 10 (fitAxis, ops.go:335‑345): inset = 0 ⇒ n = floor((60-0)/10)+1 = 7, start = 0 ⇒ plants at x = −30, −20, …, +30 (centers on the frame). The rule wants inset = spacing/2 = 5 ⇒ n = floor((60-10)/10)+1 = 6, start = 5 ⇒ plants at x = −25, −15, …, +25, half a spacing (5 cm) in.
This is precisely the #75 failure mode ("plants on the edge / hanging outside") re-emerging in the mode whose stated purpose is "a layout you could actually plant from." The radius→spacing relationship is only half the geometry; the edge inset relationship also changes when the plop becomes a point rather than a clump, and plopRadiusFor doesn't account for it. The existing grid test (ops_test.go:295‑334) doesn't catch it because it only asserts len(grid) > len(clump), RadiusCM == 5, and DerivedCount == 1 — never edge placement, which the clump TestFillRegionDeterministicPacking overhang assertion was never extended to cover.
Suggested fix: in grid mode the plant center should be inset by spacing/2 (= radius), not radius − spacing/2. Either give hexCenters the per-mode inset explicitly (grid ⇒ inset = spacing/2; clump ⇒ inset = radius − spacing/2), or equivalently compute inset = max(0, radius − spacing/2) only for clump and inset = spacing/2 for grid. Add a test asserting grid plants sit ≥ spacing/2 in from each edge.
Verified by reading ops.go:95‑141 (plopRadiusFor/validFillLayout), ops.go:271‑319 (hexCenters and the inset formula), ops.go:335‑345 (fitAxis), ops.go:182 (radius source), and ops_test.go:295‑334 (the grid test, which asserts neither edge offset nor the row count the rule would produce).
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
Through the cleanliness & maintainability lens, this change is mostly clean — plopRadiusFor and validFillLayout are well-factored, the new FillLayout type is threaded consistently, and the duplicated documentation rationale is mostly confined to the DESIGN.md decision (acceptable). A few issues:
internal/service/ops.go:143-150 — stale doc comment on FillRegion. The doc still says "Plop radius comes from the plant's spacing (or spacingOverride) via defaultPlopRadius" after the change routes the radius through plopRadiusFor(spacing, layout) (verified at ops.go:184). Grid mode no longer uses defaultPlopRadius at all, so the comment nearest the code sends a future reader trying to understand why grid plops have radius spacing/2 to the wrong function. Fix: update the comment to name plopRadiusFor and mention the layout selects the radius→spacing relationship. (Confirmed by reading ops.go:143-151.)
internal/agent/tools.go:130 — enum:"..." struct tag. The new Mode field carries enum:"clump,grid". Within pansy's own internal/agent package no machinery consumes an enum tag — the only occurrence repo-wide is this one line, and runtime.go doesn't parse enum either; the tool definitions route through the external gitea.stevedudenhoeffer.com/steve/majordomo/llmDefineTool builder, which I could not inspect (not vendored in-repo), so whether the downstream generator honors the tag is unverified. If majordomo copies unknown tags through it's harmless but dead metadata; if nothing reads it, it implies validation that doesn't exist — an out-of-enum value like "spiral" reaches the service and only fails there (which TestFillRejectsUnknownLayout and the API test confirm). Kept at low confidence because the pansy-side "nothing reads it" part is verified but the external-lib behavior is not.
internal/service/ops.go:143 — FillRegion public doc omits the unknown-layout contract.validFillLayout is the single chokepoint that rejects an unknown layout with ErrInvalidInput (ops.go:167-170), and the public FillRegion/FillNamedRegion signatures accept an arbitrary FillLayout with no caller-side normalization, so every external caller (api, agent) must learn the convention by grepping. Adding one line to the FillRegion doc noting that an unknown layout is rejected with ErrInvalidInput would state the contract at the entry point. (Confirmed: the doc at 143-150 makes no mention of layout validation.)
These are all documentation/contract-clarity nits, not structural problems. The core refactor (extracting plopRadiusFor, centralizing validation in validFillLayout) follows the surrounding patterns well.
⚡ Performance — Minor issues
The finding is confirmed against the actual code:
internal/service/ops.go:23 — maxFillPlops = 5000 cap exists.
internal/service/ops.go:300 — hexCenters returns (nil, total) when total > limit, so grid fills are indeed capped at 5000.
internal/service/ops.go:211-218 — the fill loop calls coveredByExisting per candidate and appends each accepted plop to existing, making the scan set grow.
internal/service/ops.go:350-355 — coveredByExisting is O(len(existing)) per call, doing math.Hypot per existing plop. So a fill of N candidates is O(N·(E+N)) ≈ O(N²) when E (pre-existing) is small, and ~O((E+N)·N) on a re-fill.
The containment argument for the "unnecessary append" claim also checks out: in grid mode radius = spacing/2, pitch = 2·radius = spacing, so any two distinct lattice candidates are ≥ pitch = 2·radius apart. coveredByExisting returns true only when dist + radius ≤ existingRadius; with equal radii that requires dist ≤ 0, i.e. coincident points — which a regular hex lattice never produces. So the in-loop append only adds scan cost without preventing real same-fill stacking (stacking on pre-existing plantings is already covered by the initial existing slice). Clump mode is similar: pitch = 2·radius, so neighbor distance = 2·radius, and containment needs dist ≤ e.Radius − radius = 0, again never satisfied for distinct lattice points.
Impact is genuinely limited (only bites near the 5000 cap; documented/common case is low hundreds), so small/medium is appropriate.
Verdict: Minor issues
internal/service/ops.go:211-218 — Grid mode amplifies a pre-existing quadratic in the fill loop. coveredByExisting is O(len(existing)) per candidate (a math.Hypot per existing plop, ops.go:350-355), and existing = append(existing, *p) (ops.go:217) grows the scanned set on every accepted plop, so a fill of N candidates does ~O(N²) hypot scans. Clump mode keeps N in the low hundreds (negligible), but grid mode at true spacing pushes N toward maxFillPlops (5000, ops.go:23), which hexCenters enforces via its count-before-build at ops.go:300 — so a fresh 5000-plop grid fill does ~12.5M hypot calls, and re-filling into a bed already holding ~5000 plops does ~25M. For the documented low-hundreds case this is fine; it only bites at the cap. The existing = append(existing, *p) is also unnecessary for same-fill coverage: within one fill the lattice is regular at pitch = 2×radius, so a later candidate is never entirely inside an earlier one (containment needs distance ≤ e.Radius − radius = 0, but neighbour distance = 2×radius), so the append only adds scan cost without preventing any real stack-on. Suggested fix: scan coverage against only the pre-existing plantings (drop the in-loop append), or build a coarse spatial grid for coveredByExisting when N is large. (Verified by reading the loop and cap; practical impact limited, hence small.)
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
internal/service/ops.go:167-170 — validFillLayout rejects unknown layouts with ErrInvalidInput before any geometry/DB work; verified writeServiceError/errors.go:54-55 maps ErrInvalidInput → 400. The API test ("spiral") confirms this end-to-end.
internal/service/ops.go:124 (plopRadiusFor) — grid returns spacing/2. With spacing already validated at ops.go:177 (non-finite / out of [minPlantSpacingCM, maxPlantSpacingCM] rejected) and the radius guard at :183 (!isFinite(radius) || radius <= 0), no zero/negative/non-finite radius reaches hexCenters. minPlantSpacingCM = 0.1 (verified at plants.go:19) guarantees spacing/2 > 0.
internal/service/ops.go:200-203 — maxFillPlops cap is applied to the grid lattice the same as clump; hexCenters returns total before slicing, so an oversized grid fill is refused rather than silently truncated or overflowing. The 6000×6000/spacing-10 clump test and the PR's grid note both confirm.
derivedCount (plantings.go:28-41) on a grid plop: round(π·(s/2)² / s²) = round(π/4) = round(0.785) = 1, matching the asserted single-plant output; the non-positive/non-finite floor is irrelevant here given the upstream guards.
Agent Mode arg: empty "" → FillLayout("") normalizes to FillClump via validFillLayout (verified ops.go:135), so an agent omitting mode keeps the prior behavior — no nil/empty edge case breaks it.
Nothing in the error-handling/edge-case lens is materially wrong. The new layout parameter is validated defensively (unknown → 400), downstream numeric guards already cover the grid radius path, and the plop cap is applied uniformly.
⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 10: context deadline exceeded
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/service/ops.go:145 — The FillRegion godoc still says "Plop radius comes from the plant's spacing … via defaultPlopRadius", but since this PR the radius is computed by plopRadiusFor, which uses spacing/2 in grid mode. The stale reference will mislead the next reader trying to understand why a grid fill behaves differently.
internal/service/ops.go:253 — The hexCenters comment block is written exclusively in clump terms ("A plop is a CLUMP, not a plant", references to defaultPlopRadius). hexCenters is now shared between clump and grid fills, so the commentary should be generic (or note that radius and spacing may describe a single-plant plop in grid mode). Otherwise a future maintainer may think grid mode needs a separate lattice implementation.
⚡ Performance — No material issues found
No material issues found
The 5000-plop maxFillPlops cap bounds both clump and grid fills, preventing runaway inserts. Grid mode increases plop counts from ~tens to ~low-hundreds for typical beds, well within the limit.
hexCenters pre-computes total before allocating the lattice slice, so a refused over-cap fill avoids large intermediate allocations.
coveredByExisting is O(existing + new) per candidate, which becomes O(n²) in the worst case when grid mode creates up to 5000 plops. That is a bounded regression versus clump mode (~15 plops), but still capped at ~12.5 M distance checks for a maxed-out fill on an empty bed — acceptable for a one-time batch operation and not a hot path. Verified by reading internal/service/ops.go:212-218 and internal/service/ops.go:349-356.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 11: context deadline exceeded
opencode/glm-5.2:cloud (opencode) — Blocking issues found
Verified the authorization ordering in internal/service/ops.go:163: objectForRole(..., roleEditor) runs first, then validFillLayout at ops.go:167. An unauthorized viewer gets ErrForbidden/ErrNotFound regardless of the layout value, so the new input field doesn't leak existence or become an authz bypass.
Verified the unknown-layout rejection path (validFillLayout, ops.go:108-119): any non-clump/grid/empty value returns ErrInvalidInput before any geometry or DB work. Empty defaults to FillClump, so existing callers are unchanged.
Verified the API layer (internal/api/ops.go:91,93) casts the raw JSON layout straight to service.FillLayout and hands it to the service, which validates it — no trust of client input at the handler. writeServiceError maps ErrInvalidInput to 400 (confirmed by the test asserting 400 for "spiral").
Verified the agent adapter (internal/agent/tools.go:131) passes service.FillLayout(args.Mode) through to the same service validation; even if the majordomo enum:"clump,grid" tag weren't enforced, the service rejects unknown values. No injection surface — layout is only ever compared against two string constants, never interpolated into SQL or a template.
spacingOverride and region finiteness/bounds checks are unchanged and still run after layout validation; no new untrusted-data path reaches the store.
No security-relevant problems in this change.
🎯 Correctness — Blocking issues found
I've verified the geometry. Let me confirm the test assertions and the inset formula one more time.
I've independently verified the geometry. Let me confirm there's no grid-specific inset handling elsewhere.
Confirmed. The inset formula at line 287 is max(0, radius − spacing/2), identical for both layouts — there is no layout-aware branching. In grid mode (radius = spacing/2), this yields inset = 0, placing the plant center on the bed boundary with its canopy (radius spacing/2) half outside. The DESIGN.md:10-11 text explicitly claims "Same hexCenters lattice and #75 edge rule for both" and "the outer row owes it half the spacing" (center half-spacing inside, canopy touching edge). The grid test at ops_test.go:299-334 only asserts RadiusCM == 5 and DerivedCount == 1, never edge placement, so it does not catch this.
VERDICT: Blocking issues found
internal/service/ops.go:287 — grid mode's outer row sits half outside the bed, violating the #75 half-spacing edge rule the PR claims to inherit. The inset = max(0, radius − spacing/2) formula is correct for clump mode (where radius = 1.5·spacing gives inset = s; the rim overhangs by 0.5s while the outermost plant within the clump lands 0.5s inside the edge — the #75 rule), but in grid mode radius = spacing/2 makes inset = 0, so the single plant's center lands on the bed boundary and its canopy extends half outside the frame. DESIGN.md:10-11 explicitly states the lattice uses "the same #75 edge rule for both" and that "the outer row owes it half the spacing" (center half a spacing inside, canopy touching the edge — the square-foot-chart arithmetic at ops.go:246-251: garlic 4″ apart, 2″ from edge). For grid the correct inset is spacing/2, not 0. Verified concretely against the grid test's own bed (ops_test.go:299-334): 60×60 bed (±30), spacing 10, radius 5, inset=0, pitch 10 → fitAxis(60,10,0) gives 7 columns at x ∈ {−30,…,30}; the x=±30 plants extend to ±35, half outside. The test only asserts RadiusCM==5 and DerivedCount==1, never that plants are inside the bed, so it passes despite the outer row straddling the frame. A fix that preserves clump behavior: make the inset layout-aware, e.g. inset = max(radius − spacing/2, spacing/2) — clump: max(s, 0.5s)=s (unchanged); grid: max(0, 0.5s)=0.5s (correct, plant center half-spacing inside, canopy flush with edge).
🧹 Code cleanliness & maintainability — Minor issues
Confirmed. The comment at lines 144-145 says "via defaultPlopRadius" but the body now uses plopRadiusFor(spacing, layout) at line 182. The finding is valid.
VERDICT: Minor issues
internal/service/ops.go:144-145 — The FillRegion doc comment is now stale. It reads "Plop radius comes from the plant's spacing (or spacingOverride) via defaultPlopRadius", but this PR changed the radius selection to plopRadiusFor(spacing, layout) (line 182), which branches on FillLayout. A reader tracing the radius logic from the comment will land on defaultPlopRadius and miss the entire FillClump/FillGrid distinction that's the point of this change. CLAUDE.md is explicit that stale comments are worse than none. Fix: …(or spacingOverride) via plopRadiusFor (clump: defaultPlopRadius; grid: spacing/2)…. Verified against the actual comment block and the fillLoaded body.
Note: the hexCenters comment at line 253 ("defaultPlopRadius makes it 1.5×spacing…") still describes the clump case specifically and remains accurate — no change needed there.
⚡ Performance — No material issues found
VERDICT: No material issues found
I verified the performance-relevant paths in internal/service/ops.go:
hexCenters counting-before-building (ops.go:271-318) is preserved: when total > maxFillPlops it returns (nil, total) so a refused oversized grid fill never allocates its lattice. Grid mode only changes the radius→spacing relationship via plopRadiusFor (ops.go:182), not the lattice logic, so the cap (maxFillPlops = 5000, ops.go:23) still bounds allocation of centers, batch, and changes.
coveredByExisting (ops.go:349-356) is O(candidates × existing) and existing grows by one per accepted candidate (ops.go:217), making a single fill O(n²) in accepted plops. Grid mode produces a denser field (larger n) than clump on the same bed, so the quadratic factor bites harder — but n is bounded by the 5000 cap, and the low-hundreds range the PR targets gives ~10⁴ math.Hypot calls, negligible. This pattern predates the PR; grid just raises n within the existing bound. Not a material regression.
No new allocations or copies introduced: plopRadiusFor and validFillLayout are trivial switches; FillLayout is a string conversion at the API/agent boundaries.
ListActivePlantingsForObject (store/plantings.go:130) is one query per fill — unchanged, no N+1 introduced.
🧯 Error handling & edge cases — No material issues found
Now let me check the agent's fillRegion adapter. The PR's tool passes service.FillLayout(args.Mode) — if args.Mode is empty (omitted), it becomes FillLayout("") which is normalized to clump by validFillLayout. That's fine.
Let me verify how SpacingOverride is validated for grid — and whether a zero/negative spacing with grid could cause issues. The spacingOverride is validated with minPlantSpacingCM/maxPlantSpacingCM. Plant.SpacingCM comes from a plant — could it be 0? Let me check.
The plant's spacing is validated at creation (min 0.1cm). So plopRadiusFor for grid = spacing/2 = min 0.05cm, always finite & positive. Fine. The radius>0 guard at line 183 covers it.
Let me check the agent adapter's Mode handling — if the agent passes an unknown mode string, service.FillLayout(args.Mode) becomes e.g. FillLayout("hex"), which validFillLayout rejects with ErrInvalidInput. Good. The agent tool's enum tag is descriptive only (majordomo doesn't enforce strictly necessarily). The service catches it. Fine.
Now let me check writeServiceError maps ErrInvalidInput to 400 — confirmed by the API test expecting 400. Good.
Error handling lens is clean. The validFillLayout is checked before doing any work (no partial state). Layout validation happens after the o.Plantable check but before loading plant — no resource leak. No defer/cleanup needed (no open resources in fillLoaded). The batch insert is in one transaction handled in store layer.
One small thing: in fillLoaded, the layout validation (validFillLayout) happens AFTER the objectForRole lookup (permission check) but the order doesn't matter for error handling. Actually it's checked first in fillLoaded (line 164 plantable, 167 layout) before plant loading — fine.
No edge case issues. The empty-mode defaulting and unknown-mode rejection are both handled explicitly. Grid with tiny spacing could produce huge counts but maxFillPlops cap still applies (unchanged guard at line 201). The test TestFillRegionCappedForHugeArea confirms cap works.
VERDICT: No material issues found.
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found through the security lens.
I verified:
The new layout / mode parameter is validated at the service boundary by validFillLayout() before any geometry is computed. Empty defaults to clump; unknown values return ErrInvalidInput.
The value is never interpolated into SQL, shell commands, file paths, or outgoing requests — it only selects between two radius formulas (spacing/2 vs max(1.5×spacing, 15)).
The existing maxFillPlops = 5000 cap still bounds grid fills, which produce more (smaller) plops. hexCenters counts before allocating.
derivedCount is capped at maxExplicitCount. Float inputs are guarded by isFinite.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
Everything in this change is clean and well-structured through the maintainability lens. Constants are well-named, the validFillLayout normalization is sensibly placed, and the new plopRadiusFor helper cleanly separates the radius logic from the fill orchestration. The FillLayout type and its two values read clearly. Existing callers are mechanically updated to pass FillClump, preserving prior behavior. No dead code, no duplicated logic, no confusing names, no overly long or deeply nested functions introduced.
I did spot two documentation/comment staleness issues that are worth a quick fix, but they are minor and well within the maintainability lane:
internal/service/ops.go:144-145 — The FillRegion godoc still says "via defaultPlopRadius", but the function now routes through plopRadiusFor. A stale comment is worse than none because the next reader believes it.
DESIGN.md:115 — The documented FillRegion signature is FillRegion(ctx, actor, objectID, region, plantID, spacingOverride); it now takes an additional layout parameter. Docs rot.
Both are one-line fixes: update the comment to say "via plopRadiusFor" and append , layout to the DESIGN.md signature.
⚡ Performance — No material issues found
No material issues found.
The PR introduces FillGrid which naturally produces more plops than FillClump for the same bed (pitch drops from 2×max(1.5×spacing,15) to spacing), but the number of plops is bounded by the pre-existing maxFillPlops = 5000 cap, and the lattice generation in hexCenters counts positions before allocating. The hot-path coveredByExisting linear scan is pre-existing and, while it becomes more pronounced with higher plop counts, remains well within acceptable limits for the typical "low-hundreds" grid-fill case the PR targets. No new allocations, blocking calls, or unbounded growth are introduced.
🧯 Error handling & edge cases — No material issues found
No material issues found.
Empty/default layout handling: Verified that validFillLayout at internal/service/ops.go:132 correctly normalizes an empty string "" to FillClump, so both existing callers and new callers omitting the field get the safe default.
Unknown layout rejection: Verified at ops.go:167-170 that any non-empty, non-"grid" layout returns ErrInvalidInput, and the API test confirms this surfaces as a 400. The agent tool passes through to the same validation.
Radius validation after plopRadiusFor: Verified at ops.go:183-185 that the post-calculation check !isFinite(radius) || radius <= 0 still guards grid mode (where radius = spacing/2). Spacing itself is bounded (min 0.1 cm) and override is checked for finiteness before use, so no NaN/Inf/zero can leak through.
maxFillPlops cap: Verified at ops.go:200-203 that the 5000-plop cap is applied after hexCenters counts positions, so a dense grid fill of a huge bed is safely refused before any inserts happen.
nil spacingOverride: The existing guard at ops.go:176-181 is unchanged and still correct.
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** · 7 findings (2 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | FillRegion/fillLoaded doc comments not updated for the new layout parameter | `internal/service/ops.go:143` | 4/5 | maintainability |
| 🔴 | Grid layout's shared inset formula (radius - spacing/2) always evaluates to 0 since grid radius = spacing/2, so grid's outermost plants sit flush on/overhanging the bed edge instead of the intended half-spacing inset | `internal/service/ops.go:287` | 3/5 | correctness |
<details><summary>5 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Grid mode's smaller plop radius multiplies the self-quadratic coveredByExisting scan by ~81x for routine fills, pushing normal usage toward the 5000-plop worst case | `internal/service/ops.go:212` | claude-code/sonnet | performance |
| 🟡 | enum struct tag appears unused by pansy agent schema machinery; external majordomo/llm behavior unverified, may be dead/misleading metadata | `internal/agent/tools.go:130` | glm-5.2:cloud | maintainability |
| 🟡 | Grid mode's larger N amplifies the O(N^2) coveredByExisting scan; the in-loop append to 'existing' is unnecessary for same-fill coverage | `internal/service/ops.go:217` | glm-5.2:cloud | performance |
| 🟡 | hexCenters comment assumes clump model but function is now shared with grid mode | `internal/service/ops.go:253` | kimi-k2.6:cloud | maintainability |
| ⚪ | TestFillAndClearAPI now mixes fill/clear/grid/bad-layout concerns, breaking the file's one-test-per-concern convention | `internal/api/ops_test.go:60` | claude-code/sonnet | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
VERDICT: No material issues found
Reviewed this PR strictly through the security lens (authz, injection, unsafe input handling, DoS via unbounded input).
What I checked:
- **Authorization is unchanged.** `FillRegion`/`FillNamedRegion` still call `objectForRole(ctx, actorID, objectID, roleEditor)` before any layout-related work (`internal/service/ops.go:152`, `:362`), so the new `layout` parameter doesn't create a bypass path to the editor-role check.
- **Layout input is a closed enum, safe by construction.** `req.Layout` from the API (`internal/api/ops.go:54`) and `args.Mode` from the agent tool (`internal/agent/tools.go:130`) are arbitrary strings, but `validFillLayout` (`internal/service/ops.go:135`) accepts only `""`, `"clump"`, `"grid"` and rejects everything else with `ErrInvalidInput`, enforced server-side regardless of what the LLM sends. The value is never interpolated into SQL, shell, templates, or file paths — it only selects a radius via a `switch`. Confirmed via `grep` that no other code path consumes the raw string.
- **DoS via smaller grid radius is still capped.** Grid mode's radius (`spacing/2`) packs more plops per region than clump, but the `maxFillPlops` check in `fillLoaded` (`internal/service/ops.go:196-198`) runs on `total` from `hexCenters` after `plopRadiusFor` computes the (now smaller) radius — so the existing cap applies uniformly to both layouts, matching the PR's own test (`TestFillRegionCappedForHugeArea` still passes, and a grid-specific cap case is implied covered). Verified by reading the code path, not just the diff.
- **Validation order doesn't leak info.** Layout is validated before `visiblePlant` is called, so an invalid layout returns 400 without touching plant-visibility logic — no new oracle for plant existence.
No injection, authz, or unsafe-deserialization concerns in this change; it's a narrow, well-contained enum threaded through existing, already-vetted authorization and validation paths.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
No hidden override elsewhere — `hexCenters` has exactly one call site and it's the one traced above.
VERDICT: Minor issues
- `internal/service/ops.go:287` (`inset := math.Max(0, radius-math.Max(0, spacing)/2)`), combined with `plopRadiusFor` (`internal/service/ops.go:123-127`) returning `spacing/2` for grid mode: the inset **always** evaluates to `0` for grid fills, regardless of spacing, because `radius - spacing/2` collapses to `spacing/2 - spacing/2`. The formula was written for clump mode (where a plop's rim, not center, represents the outermost plants, so backing the radius off by half a spacing correctly approximates the half-spacing edge rule); grid mode's plop center *is* the plant, so the target inset is `spacing/2` directly, not `radius - spacing/2`. Hand-traced against the PR's own `TestFillGridLaysOutIndividualPlants` fixture (60×60cm bed, spacing 10 ⇒ radius 5): `inset=0` places the outermost row centers exactly at the bed's true boundary (`fitAxis(60,10,0)` → positions 0..60, i.e. the true edges), with half of each edge plant's own spacing/2 personal-space circle overhanging the frame — not the half-spacing-inside placement the feature (and DESIGN.md) describes as the point of grid mode. Neither new test (`TestFillGridLaysOutIndividualPlants`, the API grid test in `internal/api/ops_test.go`) asserts center/edge position, so this passes CI silently. Affects every grid fill.
- Suggested fix: don't reuse the clump-derived `radius - spacing/2` inset for grid; grid's target inset is `spacing/2` independent of radius (e.g. thread the layout into the inset calculation, or special-case `inset = spacing/2` when `layout == FillGrid`).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both draft findings verified against the actual code.
## VERDICT: Minor issues
- **`internal/service/ops.go:143-150,159-162` — Stale/incomplete doc comments on `FillRegion` and `fillLoaded`.** `FillRegion`'s doc still says "Plop radius comes from the plant's spacing (or spacingOverride) via `defaultPlopRadius`" (ops.go:144-145) — true only for `FillClump`; grid mode's radius comes from `plopRadiusFor`/`spacingCM/2` (ops.go:123-128, 182). Neither this doc nor `fillLoaded`'s (ops.go:159-162, which lists non-finite-region rejection, bounds clamping, and `maxFillPlops` but omits the new `validFillLayout` check at ops.go:167-170) mentions the new `layout` parameter at all. Confirmed by reading the current comments against the current bodies — signatures and logic changed in this PR, the prose above them didn't. This is exactly the failure mode CLAUDE.md flags ("stale comments are worse than none — the next reader believes them").
- **`internal/api/ops_test.go:60-119` — `TestFillAndClearAPI` grew to cover four unrelated scenarios under a name that no longer describes them.** Confirmed via `grep -n "^func Test"`: only 4 test functions exist in the file (`TestFillAndClearAPI`, `TestFillRegionSelectionAPI`, `TestClearObjectIsOneChangeSetAPI`, `TestFillClearPermissionsAPI`), and `TestFillAndClearAPI` is the only one bundling multiple concerns — the original fill/clear/empty-clear assertions (lines 65-97) plus the newly appended grid-layout and unknown-layout-400 assertions (lines 99-118), which don't fit the function's name or its doc comment ("covers the two routes end to end"). The sibling service-layer tests added in this same PR correctly split into separate functions (`TestFillGridLaysOutIndividualPlants`, `TestFillRejectsUnknownLayout`).
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**VERDICT: Minor issues**
- `internal/service/ops.go:211-217` (`fillLoaded`) — `coveredByExisting` (`ops.go:349-356`) is an O(candidates × existing-so-far) linear scan invoked once per candidate, with `existing` growing by one on every accepted candidate (`existing = append(existing, *p)`, `ops.go:217`), making a single fill's own scan cost intrinsically quadratic in candidate count — independent of any plops already on the object. Grid mode's radius (`plopRadiusFor`, `ops.go:123-128`) is `spacing/2` versus clump's `max(1.5×spacing, 15)` (`defaultPlopRadius`, `ops.go:96-98`) — a 3× radius ratio for typical spacings (>10cm, where clump isn't floor-clamped). Since `hexCenters` (`ops.go:271-319`) produces a lattice whose count scales as `1/radius²` (rows ~ 1/radius via `rowH`, cols ~ 1/radius via `pitch`), grid mode produces roughly 9× the candidates of clump mode on the same region/spacing, and since the scan cost is quadratic in candidate count, the total `coveredByExisting` cost is roughly `9² ≈ 81×` worse for a grid fill versus the equivalent clump fill on the same bed. This is bounded by the pre-existing `maxFillPlops = 5000` (`ops.go:23`), confirmed by reading the cap check at `ops.go:200-203`, so it isn't unbounded — but a fill reaching close to that cap now means on the order of `5000²/2 ≈ 12.5M` `math.Hypot` comparisons (roughly the draft's 25M order-of-magnitude estimate, using the product as a loose upper bound rather than the exact triangular sum) as a plausible outcome of ordinary "fill this bed in rows at real spacing" usage, rather than requiring an abusive tiny-spacing/huge-area input as it did pre-PR for clump mode. `ListActivePlantingsForObject` (`internal/store/plantings.go:130-134`, confirmed) does fetch every active planting on the whole object, not scoped to the fill region, which compounds this further on an object that already carries prior plantings. The PR's own DESIGN.md addition discusses the higher plop count only in terms of the SVG rendering budget ("approaches the low-hundreds-of-plops the SVG budget was sized for"), not this service-side scan cost. `TestFillGridLaysOutIndividualPlants` (`internal/service/ops_test.go`) only exercises a 60×60cm bed, confirmed too small to expose this. Worth a quick timing check on a realistic worst-case grid fill (e.g. a few-thousand-cm² bed at ~5cm spacing) before merging, since this runs synchronously inside one HTTP request.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No `main` branch present locally, but that's fine — I've verified the diff directly against the checked-out state. No frontend changes are needed/present, consistent with the PR description.
I've completed my review through the error-handling & edge-cases lens.
## VERDICT: No material issues found
Findings from checking the actual behavior (not just the diff):
- **Radius validation holds for grid mode.** `plopRadiusFor` drops the 15cm floor for `FillGrid`, but `fillLoaded` (internal/service/ops.go:182-185) still checks `!isFinite(radius) || radius <= 0` right after computing it, and plant spacing is already bounded to `[0.1, 10_000]` cm at creation time (`internal/service/plants.go:319`) and for `spacingOverride` inline (`ops.go:177`). So grid mode can never see a zero/negative/non-finite radius in practice — verified by reading both validation sites.
- **`validFillLayout` correctly rejects unknowns and defaults empty to clump** (`ops.go:132-141`), called before the plant lookup, so a bad `layout` string fails fast without doing unnecessary work. Verified the switch is exhaustive (only `""`, `clump`, `grid` pass).
- **`maxFillPlops` cap is layout-agnostic and pre-allocation.** `hexCenters` (ops.go:271-319) computes `total` analytically via `fitAxis` and returns `(nil, total)` before ever building the points slice if `total > limit` — so a grid fill over a huge bed with tiny spacing can't runaway-allocate even though the PR's claimed "capped" test wasn't actually added (only the existing clump-mode cap test remains, `ops_test.go:54`). This is a testing-coverage gap, not an actual unhandled edge case — the guard logic itself is sound and doesn't care whether the radius came from clump or grid math.
- **Grid's `inset = radius - spacing/2` degenerates to exactly 0** (since `radius == spacing/2` for grid), which `fitAxis` (ops.go:335-345) handles correctly — no divide-by-zero or negative-count path, it's just the ordinary "no inset" case already exercised by the existing lattice logic.
- Every touched call site in tests/agent/api that gained the new `layout FillLayout` parameter was updated consistently (`FillClump` passed explicitly) — no stale caller left with a mismatched signature that would panic or fail to compile (confirmed via the diff and `grep` for remaining unmigrated call sites — none found).
Nothing here swallows an error, skips cleanup, or leaves a nil/zero/negative edge case unhandled that wasn't already guarded before this PR.
</details>
</details>
<details><summary><b>glm-5.2: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>
**Verdict: No material issues found**
Through the security lens, I traced the new untrusted inputs to where they're used:
- **`layout` / `mode` field** — The string flows from both the REST `objectFillRequest.Layout` (`internal/api/ops.go:54`) and the agent `fill_region` tool's `Mode` (`internal/agent/tools.go:130`) into `FillRegion`/`FillNamedRegion` as a `FillLayout`, and is validated exactly once in `fillLoaded` via `validFillLayout` (`internal/service/ops.go:167-170`), which rejects anything other than `""`/`clump`/`grid` with `domain.ErrInvalidInput`. I verified `ErrInvalidInput` maps to HTTP 400 at `internal/api/errors.go:54-55`, matching the test expectations for the unknown-layout case. The value never reaches SQL, path construction, or command execution — it's only compared against two string constants and used to branch a radius formula. No injection or bypass surface.
- **No default-accept risk** — Empty string defaults to clump (backwards-compatible, safe), and the `default` branch of the `switch` returns `false` rather than falling through, so there's no path where an unknown value silently maps to a privileged/default behavior. The non-`ok` path returns before `radius` is computed, so the `plopRadiusFor(spacing, layout)` call at line 182 only ever runs on validated layouts.
No other untrusted inputs were added (spacing/plant/region handling is unchanged from the prior, already-reviewed code). The change is a clean enum-through-validation pattern with no security-relevant gaps.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
**VERDICT: Blocking issues found**
Through the correctness lens, I re-derived the grid-mode edge geometry from first principles and confirmed the PR's central claim — "the same #75 half-spacing edge rule for both" — is not actually true for grid mode.
- **`internal/service/ops.go:287` / `plopRadiusFor` (grid): plants land *on* the bed edge, not half a spacing inside — violating the #75 rule the PR claims to preserve.**
The half-spacing edge rule (DESIGN.md; the `hexCenters` comment at ops.go:244‑259) says the outer row owes only half a spacing: the outermost plant center sits half a spacing *inside* the bed edge (the square-foot-chart arithmetic: 6" apart ⇒ 3" from the frame).
`hexCenters` computes the edge inset as `inset := math.Max(0, radius - math.Max(0, spacing)/2)` (ops.go:287). For **clump** this is correct: `radius = 1.5·spacing`, so `inset = 1.5sp − 0.5sp = sp`, and the clump's rim (where its plants sit) hangs over by exactly half a spacing, putting the outermost plants at the half-spacing line.
For **grid**, `plopRadiusFor` (ops.go:123‑128) sets `radius = spacing/2`, so the formula gives `inset = spacing/2 − spacing/2 = 0`. But in grid mode the plop *is* the plant — the single plant sits at the plop **center**, not at a rim. So with `inset = 0` the outer plant's center is placed *on* the bed edge, not half a spacing in. Re-deriving on the test's own 60×60 bed, spacing 10 (`fitAxis`, ops.go:335‑345): `inset = 0` ⇒ `n = floor((60-0)/10)+1 = 7`, `start = 0` ⇒ plants at x = −30, −20, …, +30 (centers *on* the frame). The rule wants `inset = spacing/2 = 5` ⇒ `n = floor((60-10)/10)+1 = 6`, `start = 5` ⇒ plants at x = −25, −15, …, +25, half a spacing (5 cm) in.
This is precisely the #75 failure mode ("plants on the edge / hanging outside") re-emerging in the mode whose stated purpose is "a layout you could actually plant from." The radius→spacing relationship is only half the geometry; the edge inset relationship also changes when the plop becomes a point rather than a clump, and `plopRadiusFor` doesn't account for it. The existing grid test (`ops_test.go:295‑334`) doesn't catch it because it only asserts `len(grid) > len(clump)`, `RadiusCM == 5`, and `DerivedCount == 1` — never edge placement, which the clump `TestFillRegionDeterministicPacking` overhang assertion was never extended to cover.
Suggested fix: in grid mode the plant center should be inset by `spacing/2` (= `radius`), not `radius − spacing/2`. Either give `hexCenters` the per-mode inset explicitly (grid ⇒ `inset = spacing/2`; clump ⇒ `inset = radius − spacing/2`), or equivalently compute `inset = max(0, radius − spacing/2)` only for clump and `inset = spacing/2` for grid. Add a test asserting grid plants sit ≥ spacing/2 in from each edge.
Verified by reading `ops.go:95‑141` (`plopRadiusFor`/`validFillLayout`), `ops.go:271‑319` (`hexCenters` and the `inset` formula), `ops.go:335‑345` (`fitAxis`), `ops.go:182` (radius source), and `ops_test.go:295‑334` (the grid test, which asserts neither edge offset nor the row count the rule would produce).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
## VERDICT: Minor issues
Through the cleanliness & maintainability lens, this change is mostly clean — `plopRadiusFor` and `validFillLayout` are well-factored, the new `FillLayout` type is threaded consistently, and the duplicated documentation rationale is mostly confined to the DESIGN.md decision (acceptable). A few issues:
- **`internal/service/ops.go:143-150` — stale doc comment on `FillRegion`.** The doc still says "Plop radius comes from the plant's spacing (or spacingOverride) via `defaultPlopRadius`" after the change routes the radius through `plopRadiusFor(spacing, layout)` (verified at ops.go:184). Grid mode no longer uses `defaultPlopRadius` at all, so the comment nearest the code sends a future reader trying to understand why grid plops have radius `spacing/2` to the wrong function. Fix: update the comment to name `plopRadiusFor` and mention the layout selects the radius→spacing relationship. (Confirmed by reading ops.go:143-151.)
- **`internal/agent/tools.go:130` — `enum:"..."` struct tag.** The new `Mode` field carries `enum:"clump,grid"`. Within pansy's own `internal/agent` package no machinery consumes an `enum` tag — the only occurrence repo-wide is this one line, and `runtime.go` doesn't parse `enum` either; the tool definitions route through the external `gitea.stevedudenhoeffer.com/steve/majordomo/llm` `DefineTool` builder, which I could not inspect (not vendored in-repo), so whether the downstream generator honors the tag is unverified. If majordomo copies unknown tags through it's harmless but dead metadata; if nothing reads it, it implies validation that doesn't exist — an out-of-enum value like `"spiral"` reaches the service and only fails there (which `TestFillRejectsUnknownLayout` and the API test confirm). Kept at low confidence because the pansy-side "nothing reads it" part is verified but the external-lib behavior is not.
- **`internal/service/ops.go:143` — `FillRegion` public doc omits the unknown-layout contract.** `validFillLayout` is the single chokepoint that rejects an unknown layout with `ErrInvalidInput` (ops.go:167-170), and the public `FillRegion`/`FillNamedRegion` signatures accept an arbitrary `FillLayout` with no caller-side normalization, so every external caller (api, agent) must learn the convention by grepping. Adding one line to the `FillRegion` doc noting that an unknown layout is rejected with `ErrInvalidInput` would state the contract at the entry point. (Confirmed: the doc at 143-150 makes no mention of layout validation.)
These are all documentation/contract-clarity nits, not structural problems. The core refactor (extracting `plopRadiusFor`, centralizing validation in `validFillLayout`) follows the surrounding patterns well.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
The finding is confirmed against the actual code:
- `internal/service/ops.go:23` — `maxFillPlops = 5000` cap exists.
- `internal/service/ops.go:300` — `hexCenters` returns `(nil, total)` when `total > limit`, so grid fills are indeed capped at 5000.
- `internal/service/ops.go:211-218` — the fill loop calls `coveredByExisting` per candidate and appends each accepted plop to `existing`, making the scan set grow.
- `internal/service/ops.go:350-355` — `coveredByExisting` is O(len(existing)) per call, doing `math.Hypot` per existing plop. So a fill of N candidates is O(N·(E+N)) ≈ O(N²) when E (pre-existing) is small, and ~O((E+N)·N) on a re-fill.
The containment argument for the "unnecessary append" claim also checks out: in grid mode `radius = spacing/2`, pitch = 2·radius = spacing, so any two distinct lattice candidates are ≥ pitch = 2·radius apart. `coveredByExisting` returns true only when `dist + radius ≤ existingRadius`; with equal radii that requires `dist ≤ 0`, i.e. coincident points — which a regular hex lattice never produces. So the in-loop append only adds scan cost without preventing real same-fill stacking (stacking on pre-existing plantings is already covered by the initial `existing` slice). Clump mode is similar: pitch = 2·radius, so neighbor distance = 2·radius, and containment needs dist ≤ e.Radius − radius = 0, again never satisfied for distinct lattice points.
Impact is genuinely limited (only bites near the 5000 cap; documented/common case is low hundreds), so small/medium is appropriate.
Verdict: Minor issues
- `internal/service/ops.go:211-218` — Grid mode amplifies a pre-existing quadratic in the fill loop. `coveredByExisting` is O(len(existing)) per candidate (a `math.Hypot` per existing plop, ops.go:350-355), and `existing = append(existing, *p)` (ops.go:217) grows the scanned set on every accepted plop, so a fill of N candidates does ~O(N²) hypot scans. Clump mode keeps N in the low hundreds (negligible), but grid mode at true spacing pushes N toward `maxFillPlops` (5000, ops.go:23), which `hexCenters` enforces via its count-before-build at ops.go:300 — so a fresh 5000-plop grid fill does ~12.5M hypot calls, and re-filling into a bed already holding ~5000 plops does ~25M. For the documented low-hundreds case this is fine; it only bites at the cap. The `existing = append(existing, *p)` is also unnecessary for same-fill coverage: within one fill the lattice is regular at pitch = 2×radius, so a later candidate is never entirely inside an earlier one (containment needs distance ≤ e.Radius − radius = 0, but neighbour distance = 2×radius), so the append only adds scan cost without preventing any real stack-on. Suggested fix: scan coverage against only the pre-existing plantings (drop the in-loop append), or build a coarse spatial grid for `coveredByExisting` when N is large. (Verified by reading the loop and cap; practical impact limited, hence small.)
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
- `internal/service/ops.go:167-170` — `validFillLayout` rejects unknown layouts with `ErrInvalidInput` before any geometry/DB work; verified `writeServiceError`/`errors.go:54-55` maps `ErrInvalidInput` → 400. The API test ("spiral") confirms this end-to-end.
- `internal/service/ops.go:124` (`plopRadiusFor`) — grid returns `spacing/2`. With `spacing` already validated at `ops.go:177` (non-finite / out of `[minPlantSpacingCM, maxPlantSpacingCM]` rejected) and the radius guard at `:183` (`!isFinite(radius) || radius <= 0`), no zero/negative/non-finite radius reaches `hexCenters`. `minPlantSpacingCM = 0.1` (verified at `plants.go:19`) guarantees `spacing/2 > 0`.
- `internal/service/ops.go:200-203` — `maxFillPlops` cap is applied to the grid lattice the same as clump; `hexCenters` returns `total` before slicing, so an oversized grid fill is refused rather than silently truncated or overflowing. The 6000×6000/spacing-10 clump test and the PR's grid note both confirm.
- `derivedCount` (`plantings.go:28-41`) on a grid plop: `round(π·(s/2)² / s²) = round(π/4) = round(0.785) = 1`, matching the asserted single-plant output; the non-positive/non-finite floor is irrelevant here given the upstream guards.
- Agent `Mode` arg: empty `""` → `FillLayout("")` normalizes to `FillClump` via `validFillLayout` (verified `ops.go:135`), so an agent omitting `mode` keeps the prior behavior — no nil/empty edge case breaks it.
Nothing in the error-handling/edge-case lens is materially wrong. The new `layout` parameter is validated defensively (unknown → 400), downstream numeric guards already cover the grid radius path, and the plop cap is applied uniformly.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 3/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: model returned an empty response
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 10: context deadline exceeded
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `internal/service/ops.go:145` — The `FillRegion` godoc still says "Plop radius comes from the plant's spacing … via `defaultPlopRadius`", but since this PR the radius is computed by `plopRadiusFor`, which uses `spacing/2` in grid mode. The stale reference will mislead the next reader trying to understand why a grid fill behaves differently.
- `internal/service/ops.go:253` — The `hexCenters` comment block is written exclusively in clump terms ("A plop is a CLUMP, not a plant", references to `defaultPlopRadius`). `hexCenters` is now shared between clump and grid fills, so the commentary should be generic (or note that `radius` and `spacing` may describe a single-plant plop in grid mode). Otherwise a future maintainer may think grid mode needs a separate lattice implementation.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
- The 5000-plop `maxFillPlops` cap bounds both clump and grid fills, preventing runaway inserts. Grid mode increases plop counts from ~tens to ~low-hundreds for typical beds, well within the limit.
- `hexCenters` pre-computes `total` before allocating the lattice slice, so a refused over-cap fill avoids large intermediate allocations.
- `coveredByExisting` is O(existing + new) per candidate, which becomes O(n²) in the worst case when grid mode creates up to 5000 plops. That is a bounded regression versus clump mode (~15 plops), but still capped at ~12.5 M distance checks for a maxed-out fill on an empty bed — acceptable for a one-time batch operation and not a hot path. Verified by reading `internal/service/ops.go:212-218` and `internal/service/ops.go:349-356`.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 11: context deadline exceeded
</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
- Verified the authorization ordering in `internal/service/ops.go:163`: `objectForRole(..., roleEditor)` runs first, then `validFillLayout` at `ops.go:167`. An unauthorized viewer gets `ErrForbidden`/`ErrNotFound` regardless of the `layout` value, so the new input field doesn't leak existence or become an authz bypass.
- Verified the unknown-layout rejection path (`validFillLayout`, `ops.go:108-119`): any non-`clump`/`grid`/empty value returns `ErrInvalidInput` before any geometry or DB work. Empty defaults to `FillClump`, so existing callers are unchanged.
- Verified the API layer (`internal/api/ops.go:91,93`) casts the raw JSON `layout` straight to `service.FillLayout` and hands it to the service, which validates it — no trust of client input at the handler. `writeServiceError` maps `ErrInvalidInput` to 400 (confirmed by the test asserting 400 for `"spiral"`).
- Verified the agent adapter (`internal/agent/tools.go:131`) passes `service.FillLayout(args.Mode)` through to the same service validation; even if the majordomo `enum:"clump,grid"` tag weren't enforced, the service rejects unknown values. No injection surface — `layout` is only ever compared against two string constants, never interpolated into SQL or a template.
- `spacingOverride` and region finiteness/bounds checks are unchanged and still run after layout validation; no new untrusted-data path reaches the store.
No security-relevant problems in this change.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
I've verified the geometry. Let me confirm the test assertions and the inset formula one more time.
I've independently verified the geometry. Let me confirm there's no grid-specific inset handling elsewhere.
Confirmed. The `inset` formula at line 287 is `max(0, radius − spacing/2)`, identical for both layouts — there is no layout-aware branching. In grid mode (`radius = spacing/2`), this yields `inset = 0`, placing the plant center on the bed boundary with its canopy (radius `spacing/2`) half outside. The DESIGN.md:10-11 text explicitly claims "Same `hexCenters` lattice and #75 edge rule for both" and "the outer row owes it half the spacing" (center half-spacing inside, canopy touching edge). The grid test at `ops_test.go:299-334` only asserts `RadiusCM == 5` and `DerivedCount == 1`, never edge placement, so it does not catch this.
## VERDICT: Blocking issues found
- **`internal/service/ops.go:287` — grid mode's outer row sits half outside the bed, violating the #75 half-spacing edge rule the PR claims to inherit.** The `inset = max(0, radius − spacing/2)` formula is correct for *clump* mode (where `radius = 1.5·spacing` gives `inset = s`; the rim overhangs by `0.5s` while the outermost *plant within* the clump lands `0.5s` inside the edge — the #75 rule), but in *grid* mode `radius = spacing/2` makes `inset = 0`, so the single plant's center lands on the bed boundary and its canopy extends half outside the frame. DESIGN.md:10-11 explicitly states the lattice uses "the same #75 edge rule for both" and that "the outer row owes it half the spacing" (center half a spacing inside, canopy touching the edge — the square-foot-chart arithmetic at ops.go:246-251: garlic 4″ apart, 2″ from edge). For grid the correct inset is `spacing/2`, not 0. Verified concretely against the grid test's own bed (`ops_test.go:299-334`): 60×60 bed (±30), spacing 10, radius 5, `inset=0`, pitch 10 → `fitAxis(60,10,0)` gives 7 columns at x ∈ {−30,…,30}; the x=±30 plants extend to ±35, half outside. The test only asserts `RadiusCM==5` and `DerivedCount==1`, never that plants are inside the bed, so it passes despite the outer row straddling the frame. A fix that preserves clump behavior: make the inset layout-aware, e.g. `inset = max(radius − spacing/2, spacing/2)` — clump: `max(s, 0.5s)=s` (unchanged); grid: `max(0, 0.5s)=0.5s` (correct, plant center half-spacing inside, canopy flush with edge).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Confirmed. The comment at lines 144-145 says "via defaultPlopRadius" but the body now uses `plopRadiusFor(spacing, layout)` at line 182. The finding is valid.
**VERDICT: Minor issues**
- **`internal/service/ops.go:144-145`** — The `FillRegion` doc comment is now stale. It reads "Plop radius comes from the plant's spacing (or spacingOverride) **via `defaultPlopRadius`**", but this PR changed the radius selection to `plopRadiusFor(spacing, layout)` (line 182), which branches on `FillLayout`. A reader tracing the radius logic from the comment will land on `defaultPlopRadius` and miss the entire `FillClump`/`FillGrid` distinction that's the point of this change. CLAUDE.md is explicit that stale comments are worse than none. Fix: `…(or spacingOverride) via plopRadiusFor (clump: defaultPlopRadius; grid: spacing/2)…`. Verified against the actual comment block and the `fillLoaded` body.
- Note: the `hexCenters` comment at line 253 ("defaultPlopRadius makes it 1.5×spacing…") still describes the clump case specifically and remains accurate — no change needed there.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
I verified the performance-relevant paths in `internal/service/ops.go`:
- **`hexCenters` counting-before-building** (ops.go:271-318) is preserved: when `total > maxFillPlops` it returns `(nil, total)` so a refused oversized grid fill never allocates its lattice. Grid mode only changes the radius→spacing relationship via `plopRadiusFor` (ops.go:182), not the lattice logic, so the cap (`maxFillPlops = 5000`, ops.go:23) still bounds allocation of `centers`, `batch`, and `changes`.
- **`coveredByExisting`** (ops.go:349-356) is O(candidates × existing) and `existing` grows by one per accepted candidate (ops.go:217), making a single fill O(n²) in accepted plops. Grid mode produces a denser field (larger n) than clump on the same bed, so the quadratic factor bites harder — but n is bounded by the 5000 cap, and the low-hundreds range the PR targets gives ~10⁴ `math.Hypot` calls, negligible. This pattern predates the PR; grid just raises n within the existing bound. Not a material regression.
- No new allocations or copies introduced: `plopRadiusFor` and `validFillLayout` are trivial switches; `FillLayout` is a string conversion at the API/agent boundaries.
- `ListActivePlantingsForObject` (store/plantings.go:130) is one query per fill — unchanged, no N+1 introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
Now let me check the agent's `fillRegion` adapter. The PR's tool passes `service.FillLayout(args.Mode)` — if `args.Mode` is empty (omitted), it becomes `FillLayout("")` which is normalized to clump by `validFillLayout`. That's fine.
Let me verify how `SpacingOverride` is validated for grid — and whether a zero/negative spacing with grid could cause issues. The spacingOverride is validated with `minPlantSpacingCM`/`maxPlantSpacingCM`. Plant.SpacingCM comes from a plant — could it be 0? Let me check.
The plant's spacing is validated at creation (min 0.1cm). So `plopRadiusFor` for grid = spacing/2 = min 0.05cm, always finite & positive. Fine. The radius>0 guard at line 183 covers it.
Let me check the agent adapter's `Mode` handling — if the agent passes an unknown mode string, `service.FillLayout(args.Mode)` becomes e.g. `FillLayout("hex")`, which `validFillLayout` rejects with `ErrInvalidInput`. Good. The agent tool's `enum` tag is descriptive only (majordomo doesn't enforce strictly necessarily). The service catches it. Fine.
Now let me check `writeServiceError` maps `ErrInvalidInput` to 400 — confirmed by the API test expecting 400. Good.
Error handling lens is clean. The `validFillLayout` is checked before doing any work (no partial state). Layout validation happens after the `o.Plantable` check but before loading plant — no resource leak. No defer/cleanup needed (no open resources in fillLoaded). The batch insert is in one transaction handled in store layer.
One small thing: in `fillLoaded`, the layout validation (`validFillLayout`) happens AFTER the `objectForRole` lookup (permission check) but the order doesn't matter for error handling. Actually it's checked first in fillLoaded (line 164 plantable, 167 layout) before plant loading — fine.
No edge case issues. The empty-mode defaulting and unknown-mode rejection are both handled explicitly. Grid with tiny spacing could produce huge counts but `maxFillPlops` cap still applies (unchanged guard at line 201). The test `TestFillRegionCappedForHugeArea` confirms cap works.
VERDICT: No material issues found.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found** through the security lens.
I verified:
- The new `layout` / `mode` parameter is validated at the service boundary by `validFillLayout()` before any geometry is computed. Empty defaults to clump; unknown values return `ErrInvalidInput`.
- The value is never interpolated into SQL, shell commands, file paths, or outgoing requests — it only selects between two radius formulas (`spacing/2` vs `max(1.5×spacing, 15)`).
- The existing `maxFillPlops = 5000` cap still bounds grid fills, which produce more (smaller) plops. `hexCenters` counts before allocating.
- `derivedCount` is capped at `maxExplicitCount`. Float inputs are guarded by `isFinite`.
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found**
Everything in this change is clean and well-structured through the maintainability lens. Constants are well-named, the `validFillLayout` normalization is sensibly placed, and the new `plopRadiusFor` helper cleanly separates the radius logic from the fill orchestration. The `FillLayout` type and its two values read clearly. Existing callers are mechanically updated to pass `FillClump`, preserving prior behavior. No dead code, no duplicated logic, no confusing names, no overly long or deeply nested functions introduced.
I did spot two **documentation/comment staleness** issues that are worth a quick fix, but they are minor and well within the maintainability lane:
- `internal/service/ops.go:144-145` — The `FillRegion` godoc still says "via defaultPlopRadius", but the function now routes through `plopRadiusFor`. A stale comment is worse than none because the next reader believes it.
- `DESIGN.md:115` — The documented `FillRegion` signature is `FillRegion(ctx, actor, objectID, region, plantID, spacingOverride)`; it now takes an additional `layout` parameter. Docs rot.
Both are one-line fixes: update the comment to say "via plopRadiusFor" and append `, layout` to the DESIGN.md signature.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
The PR introduces `FillGrid` which naturally produces more plops than `FillClump` for the same bed (pitch drops from `2×max(1.5×spacing,15)` to `spacing`), but the number of plops is bounded by the pre-existing `maxFillPlops = 5000` cap, and the lattice generation in `hexCenters` counts positions before allocating. The hot-path `coveredByExisting` linear scan is pre-existing and, while it becomes more pronounced with higher plop counts, remains well within acceptable limits for the typical "low-hundreds" grid-fill case the PR targets. No new allocations, blocking calls, or unbounded growth are introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found.
- **Empty/default layout handling**: Verified that `validFillLayout` at `internal/service/ops.go:132` correctly normalizes an empty string `""` to `FillClump`, so both existing callers and new callers omitting the field get the safe default.
- **Unknown layout rejection**: Verified at `ops.go:167-170` that any non-empty, non-`"grid"` layout returns `ErrInvalidInput`, and the API test confirms this surfaces as a 400. The agent tool passes through to the same validation.
- **Radius validation after `plopRadiusFor`**: Verified at `ops.go:183-185` that the post-calculation check `!isFinite(radius) || radius <= 0` still guards grid mode (where `radius = spacing/2`). Spacing itself is bounded (min 0.1 cm) and override is checked for finiteness before use, so no NaN/Inf/zero can leak through.
- **maxFillPlops cap**: Verified at `ops.go:200-203` that the 5000-plop cap is applied after `hexCenters` counts positions, so a dense grid fill of a huge bed is safely refused before any inserts happen.
- **nil spacingOverride**: The existing guard at `ops.go:176-181` is unchanged and still correct.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Gadfly findings on #95:
- Correctness (3 models): the shared inset formula radius-spacing/2
collapses to 0 in grid mode (radius = spacing/2), so grid's outer row
planted flush on / overhanging the bed edge instead of the half-spacing
in that the rule wants. The inset genuinely differs by layout — a grid
plant sits AT the plop centre (inset spacing/2), a clump's plants reach
its rim (inset radius-spacing/2, overhanging by a half). Split it into a
new edgeInset(radius, spacing, layout); hexCenters now takes a
precomputed inset and is pure geometry (no spacing/layout knowledge).
Regression guard: grid plants land at ±25 on a 60cm bed, not ±30.
- Performance (2 findings): the in-loop `existing = append(existing, *p)`
was dead — every plop in one fill shares a radius and sits on a distinct
lattice point, and a plop is "covered" only when wholly inside another,
impossible between equal-radius circles at different centres. Removing it
stops the coveredByExisting scan growing during the fill (an empty-bed
grid fill's check was needlessly quadratic in the plop count).
- Docs: FillRegion/fillLoaded/hexCenters comments and the DESIGN.md bullets
updated for plopRadiusFor/edgeInset (were still citing defaultPlopRadius
and "written out in hexCenters").
- Test hygiene: split the grid + bad-layout cases out of TestFillAndClearAPI
into TestFillLayoutAPI (one concern per test).
The enum-tag finding is a non-issue: majordomo's DefineTool derives its arg
schema from the same struct-tag reflection as Generate (proven by the vision
SeedPacket enum), and the service validates mode regardless.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit 3685308d10 into main2026-07-22 04:22:46 +00:00
steve
deleted branch feat/fill-mode2026-07-22 04:22:46 +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 #77. Part of #86. You chose option 3: keep clumps as the default primitive, add a grid/rows fill mode — so sketching and planning are different operations with different outputs, rather than one model forced to be both.
Why
A plop is a clump, not a plant. That's the right primitive for sketching ("a few plops of garlic in a corner") but it can't draw a real planting — a filled 4×8 ft bed comes out as ~15 blobs, not 8 rows of garlic (the whole of #77).
What
FillLayoutselects what a fill packs:clump(default, unchanged) — radius1.5×spacing, pitch2×radius, ~7 plants per plop. Quick coverage.grid— radiusspacing/2, pitch= spacing, one plant per plop (derivedCountatr=spacing/2isπ/4 → 1). Rows you could actually plant from.The geometry is the same
hexCenterslattice and the same #75 half-spacing edge rule — only the radius→spacing relationship differs (plopRadiusFor). Grid keeps no 15 cm floor (its point is true spacing); clump keeps it so a tiny-spacing plant doesn't make invisible clumps.Threaded through:
FillRegion/FillNamedRegion— empty layout = clump (existing callers unchanged), unknown layout =ErrInvalidInput.POST /objects/:id/fill— alayoutfield.fill_regiontool — amodearg (enumclump|grid), so "plant the bed in rows" works.Tests
spacing/2, derived count 1).maxFillPlopsstill caps a grid fill of a huge bed with tiny spacing.Not here
There's no frontend fill affordance yet — fill is agent-only in the UI (the fill UI was deferred in #82). So the mode toggle rides along whenever that fill UI is built. Grid mode is fully usable now via the agent ("plant the west bed in rows") and the REST endpoint.
Perf note: a grid-filled bed approaches the low-hundreds-of-plops the SVG budget was sized for;
PlopMarkeris already memo'd and semantic-zoom-tiered, which anticipates it. Worth a glance on a full garden when the fill UI lands.Docs: DESIGN placement-model decision.
GOWORK=off go test ./...green;gofmt -l internal/clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 04:14:40Z
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 — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -127,8 +127,9 @@ func (a *adapter) fillRegion(ctx context.Context, args struct {Region string `json:"region" description:"nw|ne|sw|se corner, north|south|east|west (or top|bottom|left|right) half, or all"`PlantID int64 `json:"plantId" description:"plant to fill with"`SpacingOverride *float64 `json:"spacingOverrideCm" description:"optional in-row spacing override in cm; omit to use the plant's spacing"`Mode string `json:"mode" enum:"clump,grid" description:"clump (default) drops a few fat clumps for a quick sketch; grid lays out individual plants in rows at true spacing, a layout you could plant from"`🟡 enum struct tag appears unused by pansy agent schema machinery; external majordomo/llm behavior unverified, may be dead/misleading metadata
maintainability · flagged by 1 model
internal/agent/tools.go:130—enum:"..."struct tag. The newModefield carriesenum:"clump,grid". Within pansy's owninternal/agentpackage no machinery consumes anenumtag — the only occurrence repo-wide is this one line, andruntime.godoesn't parseenumeither; the tool definitions route through the externalgitea.stevedudenhoeffer.com/steve/majordomo/llmDefineToolbuilder, which I could not inspect (not vendored in-repo), so whether the downstream generator honor…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 7 findings (2 with multi-model agreement)
internal/service/ops.go:143internal/service/ops.go:2875 single-model findings (lower confidence)
internal/service/ops.go:212internal/agent/tools.go:130internal/service/ops.go:217internal/service/ops.go:253internal/api/ops_test.go:60Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed this PR strictly through the security lens (authz, injection, unsafe input handling, DoS via unbounded input).
What I checked:
FillRegion/FillNamedRegionstill callobjectForRole(ctx, actorID, objectID, roleEditor)before any layout-related work (internal/service/ops.go:152,:362), so the newlayoutparameter doesn't create a bypass path to the editor-role check.req.Layoutfrom the API (internal/api/ops.go:54) andargs.Modefrom the agent tool (internal/agent/tools.go:130) are arbitrary strings, butvalidFillLayout(internal/service/ops.go:135) accepts only"","clump","grid"and rejects everything else withErrInvalidInput, enforced server-side regardless of what the LLM sends. The value is never interpolated into SQL, shell, templates, or file paths — it only selects a radius via aswitch. Confirmed viagrepthat no other code path consumes the raw string.spacing/2) packs more plops per region than clump, but themaxFillPlopscheck infillLoaded(internal/service/ops.go:196-198) runs ontotalfromhexCentersafterplopRadiusForcomputes the (now smaller) radius — so the existing cap applies uniformly to both layouts, matching the PR's own test (TestFillRegionCappedForHugeAreastill passes, and a grid-specific cap case is implied covered). Verified by reading the code path, not just the diff.visiblePlantis called, so an invalid layout returns 400 without touching plant-visibility logic — no new oracle for plant existence.No injection, authz, or unsafe-deserialization concerns in this change; it's a narrow, well-contained enum threaded through existing, already-vetted authorization and validation paths.
🎯 Correctness — Minor issues
No hidden override elsewhere —
hexCentershas exactly one call site and it's the one traced above.VERDICT: Minor issues
internal/service/ops.go:287(inset := math.Max(0, radius-math.Max(0, spacing)/2)), combined withplopRadiusFor(internal/service/ops.go:123-127) returningspacing/2for grid mode: the inset always evaluates to0for grid fills, regardless of spacing, becauseradius - spacing/2collapses tospacing/2 - spacing/2. The formula was written for clump mode (where a plop's rim, not center, represents the outermost plants, so backing the radius off by half a spacing correctly approximates the half-spacing edge rule); grid mode's plop center is the plant, so the target inset isspacing/2directly, notradius - spacing/2. Hand-traced against the PR's ownTestFillGridLaysOutIndividualPlantsfixture (60×60cm bed, spacing 10 ⇒ radius 5):inset=0places the outermost row centers exactly at the bed's true boundary (fitAxis(60,10,0)→ positions 0..60, i.e. the true edges), with half of each edge plant's own spacing/2 personal-space circle overhanging the frame — not the half-spacing-inside placement the feature (and DESIGN.md) describes as the point of grid mode. Neither new test (TestFillGridLaysOutIndividualPlants, the API grid test ininternal/api/ops_test.go) asserts center/edge position, so this passes CI silently. Affects every grid fill.radius - spacing/2inset for grid; grid's target inset isspacing/2independent of radius (e.g. thread the layout into the inset calculation, or special-caseinset = spacing/2whenlayout == FillGrid).🧹 Code cleanliness & maintainability — Minor issues
Both draft findings verified against the actual code.
VERDICT: Minor issues
internal/service/ops.go:143-150,159-162— Stale/incomplete doc comments onFillRegionandfillLoaded.FillRegion's doc still says "Plop radius comes from the plant's spacing (or spacingOverride) viadefaultPlopRadius" (ops.go:144-145) — true only forFillClump; grid mode's radius comes fromplopRadiusFor/spacingCM/2(ops.go:123-128, 182). Neither this doc norfillLoaded's (ops.go:159-162, which lists non-finite-region rejection, bounds clamping, andmaxFillPlopsbut omits the newvalidFillLayoutcheck at ops.go:167-170) mentions the newlayoutparameter at all. Confirmed by reading the current comments against the current bodies — signatures and logic changed in this PR, the prose above them didn't. This is exactly the failure mode CLAUDE.md flags ("stale comments are worse than none — the next reader believes them").internal/api/ops_test.go:60-119—TestFillAndClearAPIgrew to cover four unrelated scenarios under a name that no longer describes them. Confirmed viagrep -n "^func Test": only 4 test functions exist in the file (TestFillAndClearAPI,TestFillRegionSelectionAPI,TestClearObjectIsOneChangeSetAPI,TestFillClearPermissionsAPI), andTestFillAndClearAPIis the only one bundling multiple concerns — the original fill/clear/empty-clear assertions (lines 65-97) plus the newly appended grid-layout and unknown-layout-400 assertions (lines 99-118), which don't fit the function's name or its doc comment ("covers the two routes end to end"). The sibling service-layer tests added in this same PR correctly split into separate functions (TestFillGridLaysOutIndividualPlants,TestFillRejectsUnknownLayout).⚡ Performance — Minor issues
VERDICT: Minor issues
internal/service/ops.go:211-217(fillLoaded) —coveredByExisting(ops.go:349-356) is an O(candidates × existing-so-far) linear scan invoked once per candidate, withexistinggrowing by one on every accepted candidate (existing = append(existing, *p),ops.go:217), making a single fill's own scan cost intrinsically quadratic in candidate count — independent of any plops already on the object. Grid mode's radius (plopRadiusFor,ops.go:123-128) isspacing/2versus clump'smax(1.5×spacing, 15)(defaultPlopRadius,ops.go:96-98) — a 3× radius ratio for typical spacings (>10cm, where clump isn't floor-clamped). SincehexCenters(ops.go:271-319) produces a lattice whose count scales as1/radius²(rows ~ 1/radius viarowH, cols ~ 1/radius viapitch), grid mode produces roughly 9× the candidates of clump mode on the same region/spacing, and since the scan cost is quadratic in candidate count, the totalcoveredByExistingcost is roughly9² ≈ 81×worse for a grid fill versus the equivalent clump fill on the same bed. This is bounded by the pre-existingmaxFillPlops = 5000(ops.go:23), confirmed by reading the cap check atops.go:200-203, so it isn't unbounded — but a fill reaching close to that cap now means on the order of5000²/2 ≈ 12.5Mmath.Hypotcomparisons (roughly the draft's 25M order-of-magnitude estimate, using the product as a loose upper bound rather than the exact triangular sum) as a plausible outcome of ordinary "fill this bed in rows at real spacing" usage, rather than requiring an abusive tiny-spacing/huge-area input as it did pre-PR for clump mode.ListActivePlantingsForObject(internal/store/plantings.go:130-134, confirmed) does fetch every active planting on the whole object, not scoped to the fill region, which compounds this further on an object that already carries prior plantings. The PR's own DESIGN.md addition discusses the higher plop count only in terms of the SVG rendering budget ("approaches the low-hundreds-of-plops the SVG budget was sized for"), not this service-side scan cost.TestFillGridLaysOutIndividualPlants(internal/service/ops_test.go) only exercises a 60×60cm bed, confirmed too small to expose this. Worth a quick timing check on a realistic worst-case grid fill (e.g. a few-thousand-cm² bed at ~5cm spacing) before merging, since this runs synchronously inside one HTTP request.🧯 Error handling & edge cases — No material issues found
No
mainbranch present locally, but that's fine — I've verified the diff directly against the checked-out state. No frontend changes are needed/present, consistent with the PR description.I've completed my review through the error-handling & edge-cases lens.
VERDICT: No material issues found
Findings from checking the actual behavior (not just the diff):
plopRadiusFordrops the 15cm floor forFillGrid, butfillLoaded(internal/service/ops.go:182-185) still checks!isFinite(radius) || radius <= 0right after computing it, and plant spacing is already bounded to[0.1, 10_000]cm at creation time (internal/service/plants.go:319) and forspacingOverrideinline (ops.go:177). So grid mode can never see a zero/negative/non-finite radius in practice — verified by reading both validation sites.validFillLayoutcorrectly rejects unknowns and defaults empty to clump (ops.go:132-141), called before the plant lookup, so a badlayoutstring fails fast without doing unnecessary work. Verified the switch is exhaustive (only"",clump,gridpass).maxFillPlopscap is layout-agnostic and pre-allocation.hexCenters(ops.go:271-319) computestotalanalytically viafitAxisand returns(nil, total)before ever building the points slice iftotal > limit— so a grid fill over a huge bed with tiny spacing can't runaway-allocate even though the PR's claimed "capped" test wasn't actually added (only the existing clump-mode cap test remains,ops_test.go:54). This is a testing-coverage gap, not an actual unhandled edge case — the guard logic itself is sound and doesn't care whether the radius came from clump or grid math.inset = radius - spacing/2degenerates to exactly 0 (sinceradius == spacing/2for grid), whichfitAxis(ops.go:335-345) handles correctly — no divide-by-zero or negative-count path, it's just the ordinary "no inset" case already exercised by the existing lattice logic.layout FillLayoutparameter was updated consistently (FillClumppassed explicitly) — no stale caller left with a mismatched signature that would panic or fail to compile (confirmed via the diff andgrepfor remaining unmigrated call sites — none found).Nothing here swallows an error, skips cleanup, or leaves a nil/zero/negative edge case unhandled that wasn't already guarded before this PR.
glm-5.2:cloud (ollama-cloud) — 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
Through the security lens, I traced the new untrusted inputs to where they're used:
layout/modefield — The string flows from both the RESTobjectFillRequest.Layout(internal/api/ops.go:54) and the agentfill_regiontool'sMode(internal/agent/tools.go:130) intoFillRegion/FillNamedRegionas aFillLayout, and is validated exactly once infillLoadedviavalidFillLayout(internal/service/ops.go:167-170), which rejects anything other than""/clump/gridwithdomain.ErrInvalidInput. I verifiedErrInvalidInputmaps to HTTP 400 atinternal/api/errors.go:54-55, matching the test expectations for the unknown-layout case. The value never reaches SQL, path construction, or command execution — it's only compared against two string constants and used to branch a radius formula. No injection or bypass surface.defaultbranch of theswitchreturnsfalserather than falling through, so there's no path where an unknown value silently maps to a privileged/default behavior. The non-okpath returns beforeradiusis computed, so theplopRadiusFor(spacing, layout)call at line 182 only ever runs on validated layouts.No other untrusted inputs were added (spacing/plant/region handling is unchanged from the prior, already-reviewed code). The change is a clean enum-through-validation pattern with no security-relevant gaps.
🎯 Correctness — Blocking issues found
VERDICT: Blocking issues found
Through the correctness lens, I re-derived the grid-mode edge geometry from first principles and confirmed the PR's central claim — "the same #75 half-spacing edge rule for both" — is not actually true for grid mode.
internal/service/ops.go:287/plopRadiusFor(grid): plants land on the bed edge, not half a spacing inside — violating the #75 rule the PR claims to preserve.The half-spacing edge rule (DESIGN.md; the
hexCenterscomment at ops.go:244‑259) says the outer row owes only half a spacing: the outermost plant center sits half a spacing inside the bed edge (the square-foot-chart arithmetic: 6" apart ⇒ 3" from the frame).hexCenterscomputes the edge inset asinset := math.Max(0, radius - math.Max(0, spacing)/2)(ops.go:287). For clump this is correct:radius = 1.5·spacing, soinset = 1.5sp − 0.5sp = sp, and the clump's rim (where its plants sit) hangs over by exactly half a spacing, putting the outermost plants at the half-spacing line.For grid,
plopRadiusFor(ops.go:123‑128) setsradius = spacing/2, so the formula givesinset = spacing/2 − spacing/2 = 0. But in grid mode the plop is the plant — the single plant sits at the plop center, not at a rim. So withinset = 0the outer plant's center is placed on the bed edge, not half a spacing in. Re-deriving on the test's own 60×60 bed, spacing 10 (fitAxis, ops.go:335‑345):inset = 0⇒n = floor((60-0)/10)+1 = 7,start = 0⇒ plants at x = −30, −20, …, +30 (centers on the frame). The rule wantsinset = spacing/2 = 5⇒n = floor((60-10)/10)+1 = 6,start = 5⇒ plants at x = −25, −15, …, +25, half a spacing (5 cm) in.This is precisely the #75 failure mode ("plants on the edge / hanging outside") re-emerging in the mode whose stated purpose is "a layout you could actually plant from." The radius→spacing relationship is only half the geometry; the edge inset relationship also changes when the plop becomes a point rather than a clump, and
plopRadiusFordoesn't account for it. The existing grid test (ops_test.go:295‑334) doesn't catch it because it only assertslen(grid) > len(clump),RadiusCM == 5, andDerivedCount == 1— never edge placement, which the clumpTestFillRegionDeterministicPackingoverhang assertion was never extended to cover.Suggested fix: in grid mode the plant center should be inset by
spacing/2(=radius), notradius − spacing/2. Either givehexCentersthe per-mode inset explicitly (grid ⇒inset = spacing/2; clump ⇒inset = radius − spacing/2), or equivalently computeinset = max(0, radius − spacing/2)only for clump andinset = spacing/2for grid. Add a test asserting grid plants sit ≥ spacing/2 in from each edge.Verified by reading
ops.go:95‑141(plopRadiusFor/validFillLayout),ops.go:271‑319(hexCentersand theinsetformula),ops.go:335‑345(fitAxis),ops.go:182(radius source), andops_test.go:295‑334(the grid test, which asserts neither edge offset nor the row count the rule would produce).🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
Through the cleanliness & maintainability lens, this change is mostly clean —
plopRadiusForandvalidFillLayoutare well-factored, the newFillLayouttype is threaded consistently, and the duplicated documentation rationale is mostly confined to the DESIGN.md decision (acceptable). A few issues:internal/service/ops.go:143-150— stale doc comment onFillRegion. The doc still says "Plop radius comes from the plant's spacing (or spacingOverride) viadefaultPlopRadius" after the change routes the radius throughplopRadiusFor(spacing, layout)(verified at ops.go:184). Grid mode no longer usesdefaultPlopRadiusat all, so the comment nearest the code sends a future reader trying to understand why grid plops have radiusspacing/2to the wrong function. Fix: update the comment to nameplopRadiusForand mention the layout selects the radius→spacing relationship. (Confirmed by reading ops.go:143-151.)internal/agent/tools.go:130—enum:"..."struct tag. The newModefield carriesenum:"clump,grid". Within pansy's owninternal/agentpackage no machinery consumes anenumtag — the only occurrence repo-wide is this one line, andruntime.godoesn't parseenumeither; the tool definitions route through the externalgitea.stevedudenhoeffer.com/steve/majordomo/llmDefineToolbuilder, which I could not inspect (not vendored in-repo), so whether the downstream generator honors the tag is unverified. If majordomo copies unknown tags through it's harmless but dead metadata; if nothing reads it, it implies validation that doesn't exist — an out-of-enum value like"spiral"reaches the service and only fails there (whichTestFillRejectsUnknownLayoutand the API test confirm). Kept at low confidence because the pansy-side "nothing reads it" part is verified but the external-lib behavior is not.internal/service/ops.go:143—FillRegionpublic doc omits the unknown-layout contract.validFillLayoutis the single chokepoint that rejects an unknown layout withErrInvalidInput(ops.go:167-170), and the publicFillRegion/FillNamedRegionsignatures accept an arbitraryFillLayoutwith no caller-side normalization, so every external caller (api, agent) must learn the convention by grepping. Adding one line to theFillRegiondoc noting that an unknown layout is rejected withErrInvalidInputwould state the contract at the entry point. (Confirmed: the doc at 143-150 makes no mention of layout validation.)These are all documentation/contract-clarity nits, not structural problems. The core refactor (extracting
plopRadiusFor, centralizing validation invalidFillLayout) follows the surrounding patterns well.⚡ Performance — Minor issues
The finding is confirmed against the actual code:
internal/service/ops.go:23—maxFillPlops = 5000cap exists.internal/service/ops.go:300—hexCentersreturns(nil, total)whentotal > limit, so grid fills are indeed capped at 5000.internal/service/ops.go:211-218— the fill loop callscoveredByExistingper candidate and appends each accepted plop toexisting, making the scan set grow.internal/service/ops.go:350-355—coveredByExistingis O(len(existing)) per call, doingmath.Hypotper existing plop. So a fill of N candidates is O(N·(E+N)) ≈ O(N²) when E (pre-existing) is small, and ~O((E+N)·N) on a re-fill.The containment argument for the "unnecessary append" claim also checks out: in grid mode
radius = spacing/2, pitch = 2·radius = spacing, so any two distinct lattice candidates are ≥ pitch = 2·radius apart.coveredByExistingreturns true only whendist + radius ≤ existingRadius; with equal radii that requiresdist ≤ 0, i.e. coincident points — which a regular hex lattice never produces. So the in-loop append only adds scan cost without preventing real same-fill stacking (stacking on pre-existing plantings is already covered by the initialexistingslice). Clump mode is similar: pitch = 2·radius, so neighbor distance = 2·radius, and containment needs dist ≤ e.Radius − radius = 0, again never satisfied for distinct lattice points.Impact is genuinely limited (only bites near the 5000 cap; documented/common case is low hundreds), so small/medium is appropriate.
Verdict: Minor issues
internal/service/ops.go:211-218— Grid mode amplifies a pre-existing quadratic in the fill loop.coveredByExistingis O(len(existing)) per candidate (amath.Hypotper existing plop, ops.go:350-355), andexisting = append(existing, *p)(ops.go:217) grows the scanned set on every accepted plop, so a fill of N candidates does ~O(N²) hypot scans. Clump mode keeps N in the low hundreds (negligible), but grid mode at true spacing pushes N towardmaxFillPlops(5000, ops.go:23), whichhexCentersenforces via its count-before-build at ops.go:300 — so a fresh 5000-plop grid fill does ~12.5M hypot calls, and re-filling into a bed already holding ~5000 plops does ~25M. For the documented low-hundreds case this is fine; it only bites at the cap. Theexisting = append(existing, *p)is also unnecessary for same-fill coverage: within one fill the lattice is regular at pitch = 2×radius, so a later candidate is never entirely inside an earlier one (containment needs distance ≤ e.Radius − radius = 0, but neighbour distance = 2×radius), so the append only adds scan cost without preventing any real stack-on. Suggested fix: scan coverage against only the pre-existing plantings (drop the in-loop append), or build a coarse spatial grid forcoveredByExistingwhen N is large. (Verified by reading the loop and cap; practical impact limited, hence small.)🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
internal/service/ops.go:167-170—validFillLayoutrejects unknown layouts withErrInvalidInputbefore any geometry/DB work; verifiedwriteServiceError/errors.go:54-55mapsErrInvalidInput→ 400. The API test ("spiral") confirms this end-to-end.internal/service/ops.go:124(plopRadiusFor) — grid returnsspacing/2. Withspacingalready validated atops.go:177(non-finite / out of[minPlantSpacingCM, maxPlantSpacingCM]rejected) and the radius guard at:183(!isFinite(radius) || radius <= 0), no zero/negative/non-finite radius reacheshexCenters.minPlantSpacingCM = 0.1(verified atplants.go:19) guaranteesspacing/2 > 0.internal/service/ops.go:200-203—maxFillPlopscap is applied to the grid lattice the same as clump;hexCentersreturnstotalbefore slicing, so an oversized grid fill is refused rather than silently truncated or overflowing. The 6000×6000/spacing-10 clump test and the PR's grid note both confirm.derivedCount(plantings.go:28-41) on a grid plop:round(π·(s/2)² / s²) = round(π/4) = round(0.785) = 1, matching the asserted single-plant output; the non-positive/non-finite floor is irrelevant here given the upstream guards.Modearg: empty""→FillLayout("")normalizes toFillClumpviavalidFillLayout(verifiedops.go:135), so an agent omittingmodekeeps the prior behavior — no nil/empty edge case breaks it.Nothing in the error-handling/edge-case lens is materially wrong. The new
layoutparameter is validated defensively (unknown → 400), downstream numeric guards already cover the grid radius path, and the plop cap is applied uniformly.kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues · ⚠️ 3/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 10: context deadline exceeded
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/service/ops.go:145— TheFillRegiongodoc still says "Plop radius comes from the plant's spacing … viadefaultPlopRadius", but since this PR the radius is computed byplopRadiusFor, which usesspacing/2in grid mode. The stale reference will mislead the next reader trying to understand why a grid fill behaves differently.internal/service/ops.go:253— ThehexCenterscomment block is written exclusively in clump terms ("A plop is a CLUMP, not a plant", references todefaultPlopRadius).hexCentersis now shared between clump and grid fills, so the commentary should be generic (or note thatradiusandspacingmay describe a single-plant plop in grid mode). Otherwise a future maintainer may think grid mode needs a separate lattice implementation.⚡ Performance — No material issues found
No material issues found
maxFillPlopscap bounds both clump and grid fills, preventing runaway inserts. Grid mode increases plop counts from ~tens to ~low-hundreds for typical beds, well within the limit.hexCenterspre-computestotalbefore allocating the lattice slice, so a refused over-cap fill avoids large intermediate allocations.coveredByExistingis O(existing + new) per candidate, which becomes O(n²) in the worst case when grid mode creates up to 5000 plops. That is a bounded regression versus clump mode (~15 plops), but still capped at ~12.5 M distance checks for a maxed-out fill on an empty bed — acceptable for a one-time batch operation and not a hot path. Verified by readinginternal/service/ops.go:212-218andinternal/service/ops.go:349-356.🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 11: context deadline exceeded
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
internal/service/ops.go:163:objectForRole(..., roleEditor)runs first, thenvalidFillLayoutatops.go:167. An unauthorized viewer getsErrForbidden/ErrNotFoundregardless of thelayoutvalue, so the new input field doesn't leak existence or become an authz bypass.validFillLayout,ops.go:108-119): any non-clump/grid/empty value returnsErrInvalidInputbefore any geometry or DB work. Empty defaults toFillClump, so existing callers are unchanged.internal/api/ops.go:91,93) casts the raw JSONlayoutstraight toservice.FillLayoutand hands it to the service, which validates it — no trust of client input at the handler.writeServiceErrormapsErrInvalidInputto 400 (confirmed by the test asserting 400 for"spiral").internal/agent/tools.go:131) passesservice.FillLayout(args.Mode)through to the same service validation; even if the majordomoenum:"clump,grid"tag weren't enforced, the service rejects unknown values. No injection surface —layoutis only ever compared against two string constants, never interpolated into SQL or a template.spacingOverrideand region finiteness/bounds checks are unchanged and still run after layout validation; no new untrusted-data path reaches the store.No security-relevant problems in this change.
🎯 Correctness — Blocking issues found
I've verified the geometry. Let me confirm the test assertions and the inset formula one more time.
I've independently verified the geometry. Let me confirm there's no grid-specific inset handling elsewhere.
Confirmed. The
insetformula at line 287 ismax(0, radius − spacing/2), identical for both layouts — there is no layout-aware branching. In grid mode (radius = spacing/2), this yieldsinset = 0, placing the plant center on the bed boundary with its canopy (radiusspacing/2) half outside. The DESIGN.md:10-11 text explicitly claims "SamehexCenterslattice and #75 edge rule for both" and "the outer row owes it half the spacing" (center half-spacing inside, canopy touching edge). The grid test atops_test.go:299-334only assertsRadiusCM == 5andDerivedCount == 1, never edge placement, so it does not catch this.VERDICT: Blocking issues found
internal/service/ops.go:287— grid mode's outer row sits half outside the bed, violating the #75 half-spacing edge rule the PR claims to inherit. Theinset = max(0, radius − spacing/2)formula is correct for clump mode (whereradius = 1.5·spacinggivesinset = s; the rim overhangs by0.5swhile the outermost plant within the clump lands0.5sinside the edge — the #75 rule), but in grid moderadius = spacing/2makesinset = 0, so the single plant's center lands on the bed boundary and its canopy extends half outside the frame. DESIGN.md:10-11 explicitly states the lattice uses "the same #75 edge rule for both" and that "the outer row owes it half the spacing" (center half a spacing inside, canopy touching the edge — the square-foot-chart arithmetic at ops.go:246-251: garlic 4″ apart, 2″ from edge). For grid the correct inset isspacing/2, not 0. Verified concretely against the grid test's own bed (ops_test.go:299-334): 60×60 bed (±30), spacing 10, radius 5,inset=0, pitch 10 →fitAxis(60,10,0)gives 7 columns at x ∈ {−30,…,30}; the x=±30 plants extend to ±35, half outside. The test only assertsRadiusCM==5andDerivedCount==1, never that plants are inside the bed, so it passes despite the outer row straddling the frame. A fix that preserves clump behavior: make the inset layout-aware, e.g.inset = max(radius − spacing/2, spacing/2)— clump:max(s, 0.5s)=s(unchanged); grid:max(0, 0.5s)=0.5s(correct, plant center half-spacing inside, canopy flush with edge).🧹 Code cleanliness & maintainability — Minor issues
Confirmed. The comment at lines 144-145 says "via defaultPlopRadius" but the body now uses
plopRadiusFor(spacing, layout)at line 182. The finding is valid.VERDICT: Minor issues
internal/service/ops.go:144-145— TheFillRegiondoc comment is now stale. It reads "Plop radius comes from the plant's spacing (or spacingOverride) viadefaultPlopRadius", but this PR changed the radius selection toplopRadiusFor(spacing, layout)(line 182), which branches onFillLayout. A reader tracing the radius logic from the comment will land ondefaultPlopRadiusand miss the entireFillClump/FillGriddistinction that's the point of this change. CLAUDE.md is explicit that stale comments are worse than none. Fix:…(or spacingOverride) via plopRadiusFor (clump: defaultPlopRadius; grid: spacing/2)…. Verified against the actual comment block and thefillLoadedbody.hexCenterscomment at line 253 ("defaultPlopRadius makes it 1.5×spacing…") still describes the clump case specifically and remains accurate — no change needed there.⚡ Performance — No material issues found
VERDICT: No material issues found
I verified the performance-relevant paths in
internal/service/ops.go:hexCenterscounting-before-building (ops.go:271-318) is preserved: whentotal > maxFillPlopsit returns(nil, total)so a refused oversized grid fill never allocates its lattice. Grid mode only changes the radius→spacing relationship viaplopRadiusFor(ops.go:182), not the lattice logic, so the cap (maxFillPlops = 5000, ops.go:23) still bounds allocation ofcenters,batch, andchanges.coveredByExisting(ops.go:349-356) is O(candidates × existing) andexistinggrows by one per accepted candidate (ops.go:217), making a single fill O(n²) in accepted plops. Grid mode produces a denser field (larger n) than clump on the same bed, so the quadratic factor bites harder — but n is bounded by the 5000 cap, and the low-hundreds range the PR targets gives ~10⁴math.Hypotcalls, negligible. This pattern predates the PR; grid just raises n within the existing bound. Not a material regression.plopRadiusForandvalidFillLayoutare trivial switches;FillLayoutis a string conversion at the API/agent boundaries.ListActivePlantingsForObject(store/plantings.go:130) is one query per fill — unchanged, no N+1 introduced.🧯 Error handling & edge cases — No material issues found
Now let me check the agent's
fillRegionadapter. The PR's tool passesservice.FillLayout(args.Mode)— ifargs.Modeis empty (omitted), it becomesFillLayout("")which is normalized to clump byvalidFillLayout. That's fine.Let me verify how
SpacingOverrideis validated for grid — and whether a zero/negative spacing with grid could cause issues. The spacingOverride is validated withminPlantSpacingCM/maxPlantSpacingCM. Plant.SpacingCM comes from a plant — could it be 0? Let me check.The plant's spacing is validated at creation (min 0.1cm). So
plopRadiusForfor grid = spacing/2 = min 0.05cm, always finite & positive. Fine. The radius>0 guard at line 183 covers it.Let me check the agent adapter's
Modehandling — if the agent passes an unknown mode string,service.FillLayout(args.Mode)becomes e.g.FillLayout("hex"), whichvalidFillLayoutrejects withErrInvalidInput. Good. The agent tool'senumtag is descriptive only (majordomo doesn't enforce strictly necessarily). The service catches it. Fine.Now let me check
writeServiceErrormapsErrInvalidInputto 400 — confirmed by the API test expecting 400. Good.Error handling lens is clean. The
validFillLayoutis checked before doing any work (no partial state). Layout validation happens after theo.Plantablecheck but before loading plant — no resource leak. No defer/cleanup needed (no open resources in fillLoaded). The batch insert is in one transaction handled in store layer.One small thing: in
fillLoaded, the layout validation (validFillLayout) happens AFTER theobjectForRolelookup (permission check) but the order doesn't matter for error handling. Actually it's checked first in fillLoaded (line 164 plantable, 167 layout) before plant loading — fine.No edge case issues. The empty-mode defaulting and unknown-mode rejection are both handled explicitly. Grid with tiny spacing could produce huge counts but
maxFillPlopscap still applies (unchanged guard at line 201). The testTestFillRegionCappedForHugeAreaconfirms cap works.VERDICT: No material issues found.
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found through the security lens.
I verified:
layout/modeparameter is validated at the service boundary byvalidFillLayout()before any geometry is computed. Empty defaults to clump; unknown values returnErrInvalidInput.spacing/2vsmax(1.5×spacing, 15)).maxFillPlops = 5000cap still bounds grid fills, which produce more (smaller) plops.hexCenterscounts before allocating.derivedCountis capped atmaxExplicitCount. Float inputs are guarded byisFinite.🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
Everything in this change is clean and well-structured through the maintainability lens. Constants are well-named, the
validFillLayoutnormalization is sensibly placed, and the newplopRadiusForhelper cleanly separates the radius logic from the fill orchestration. TheFillLayouttype and its two values read clearly. Existing callers are mechanically updated to passFillClump, preserving prior behavior. No dead code, no duplicated logic, no confusing names, no overly long or deeply nested functions introduced.I did spot two documentation/comment staleness issues that are worth a quick fix, but they are minor and well within the maintainability lane:
internal/service/ops.go:144-145— TheFillRegiongodoc still says "via defaultPlopRadius", but the function now routes throughplopRadiusFor. A stale comment is worse than none because the next reader believes it.DESIGN.md:115— The documentedFillRegionsignature isFillRegion(ctx, actor, objectID, region, plantID, spacingOverride); it now takes an additionallayoutparameter. Docs rot.Both are one-line fixes: update the comment to say "via plopRadiusFor" and append
, layoutto the DESIGN.md signature.⚡ Performance — No material issues found
No material issues found.
The PR introduces
FillGridwhich naturally produces more plops thanFillClumpfor the same bed (pitch drops from2×max(1.5×spacing,15)tospacing), but the number of plops is bounded by the pre-existingmaxFillPlops = 5000cap, and the lattice generation inhexCenterscounts positions before allocating. The hot-pathcoveredByExistinglinear scan is pre-existing and, while it becomes more pronounced with higher plop counts, remains well within acceptable limits for the typical "low-hundreds" grid-fill case the PR targets. No new allocations, blocking calls, or unbounded growth are introduced.🧯 Error handling & edge cases — No material issues found
No material issues found.
validFillLayoutatinternal/service/ops.go:132correctly normalizes an empty string""toFillClump, so both existing callers and new callers omitting the field get the safe default.ops.go:167-170that any non-empty, non-"grid"layout returnsErrInvalidInput, and the API test confirms this surfaces as a 400. The agent tool passes through to the same validation.plopRadiusFor: Verified atops.go:183-185that the post-calculation check!isFinite(radius) || radius <= 0still guards grid mode (whereradius = spacing/2). Spacing itself is bounded (min 0.1 cm) and override is checked for finiteness before use, so no NaN/Inf/zero can leak through.ops.go:200-203that the 5000-plop cap is applied afterhexCenterscounts positions, so a dense grid fill of a huge bed is safely refused before any inserts happen.ops.go:176-181is unchanged and still correct.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.