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 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
- Rename fillRequest → objectFillRequest to match the package's
<resource><Action>Request convention (objectCreateRequest, gardenCreateRequest…).
- Add the missing anonymous-clear permission case (401), mirroring anonymous fill.
- Reword the makeFillPlant comment to stop referencing external branch state,
which won't make sense once merged — just note the consolidation follow-up.
- clearObject: send no body (undefined) rather than an empty {}, and validate
the {cleared} response with a zod schema instead of a raw type cast.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- Reject a degenerate (zero-area or inverted) fill rect with 400 instead of
silently planting a single plop at the object centre. An empty `"rect": {}`
decodes to all-zeros and is caught the same way. New fillRect.degenerate()
and a table test covering {}, zero-width, zero-height, and inverted.
- Make the rect a named type (fillRect) rather than an anonymous inline struct,
matching the rest of internal/api, and bind the pointer to a local in the
handler so the deref is visibly guarded rather than reading req.Rect.MinX
under an invariant from a line above.
- ops_test: drop gid from the unpack instead of the `_ = gid` shim.
- ClearBedModal: drop the `const n = plopCount` no-op alias.
Not taken: moving createPlantAPI/makeFillPlant to a shared helper — that
consolidation spans this branch and #88 and is cleanest once both land, as
noted in the PR. Reusing an objectPlantingsPath helper: there isn't one in this
package, and the one inline use doesn't earn a new helper on its own.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
FillRegion and ClearObject were reachable only through the agent toolbox, so
on an instance with no model configured the most valuable bulk operation in a
garden planner — and the one carrying the most carefully reasoned geometry in
the codebase — did not exist at all. internal/service/ops.go said these lived
on *Service so "any future REST surface" would inherit the ACL checks; this is
that surface.
POST /objects/:id/fill takes a region EITHER by compass name ("all", "ne",
"south half") or as an explicit rect in the object's local frame, and refuses
both-or-neither: silently preferring one would make a client bug look like a
geometry bug. It answers 200 rather than 201 because a fill can legitimately
create nothing (the region is already planted) and there is no single resource
to point a Location at.
POST /objects/:id/clear replaces a client-side loop of PATCHes. That loop
wrote one change set per plop, so clearing a 40-plop bed took 40 presses of
Undo to put back — while the agent's clear_object, for the identical
user-facing action, undid in one click. CLAUDE.md states the rule it violated:
multi-row operations record together so they undo as one unit. Moving it
server-side also removes the partial-failure case the loop had to reconcile.
TestClearObjectIsOneChangeSetAPI pins that: fill a bed, count change sets,
clear it, assert the count rose by exactly one.
DESIGN.md's API block gains the two new routes, and the four it was already
missing: GET/POST/DELETE /gardens/:id/share-link and the unauthenticated
GET /public/gardens/:token. An unauthenticated surface absent from the
architecture doc is the one worth fixing on sight.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steveandClaude Opus 4.8