Agent seam: bulk ops (FillRegion/ClearObject/DescribeGarden) + DefineTool wrappers (#19) #38

Merged
steve merged 2 commits from phase-8-agent-seam into main 2026-07-19 05:16:40 +00:00
6 changed files with 897 additions and 0 deletions
Showing only changes of commit d3b7dd06c9 - Show all commits
+18
View File
@@ -0,0 +1,18 @@
// Package agent adapts pansy's service layer to majordomo tools so an agent can
// drive a garden in natural language ("fill the NE corner with garlic, the NW
// with basil, the south half with beans"). Each tool runs as a fixed actor and
// therefore inherits pansy's ACL checks for free — a viewer's fill_region returns
// ErrForbidden, exactly as the REST API would.
//
// Two deliberate separations keep the core server lean:
//
// - cmd/pansy does NOT import this package, so the server binary carries no
// agent/LLM dependencies.
// - the tool wiring (tools.go) sits behind the `majordomo` build tag, so the
// default `go build ./...` / `go test ./...` compiles without the majordomo
// module. Build the agent tools with `-tags majordomo` once majordomo is a
// dependency (`go get gitea.stevedudenhoeffer.com/steve/majordomo`).
//
// The agent harness itself (model loop, chat surface) lives in the
// majordomo/executus stack, outside this repo; this package is only the toolbox.
package agent
+119
View File
@@ -0,0 +1,119 @@
//go:build majordomo
package agent
import (
"context"
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
)
// NewToolbox builds a majordomo toolbox over pansy's service layer, bound to a
// single acting user. Every tool call runs as actorID, so pansy's permission
// checks (requireGardenRole / objectForRole) apply unchanged. Construct one per
// authenticated agent session:
//
// box := agent.NewToolbox(svc, session.UserID)
// agent.Run(ctx, model, box, "fill the NE corner with garlic")
func NewToolbox(svc *service.Service, actorID int64) *llm.Toolbox {
a := &adapter{svc: svc, actor: actorID}
return llm.NewToolbox("pansy",
llm.DefineTool("list_gardens",
"List the gardens the user can see (owned and shared), with the user's role on each.",
a.listGardens),
llm.DefineTool("describe_garden",
"Summarize a garden: its dimensions, objects (with sizes/positions/version), and each object's active plantings with a rough compass location.",
a.describeGarden),
llm.DefineTool("create_object",
"Add an object (bed, grow_bag, container, in_ground, tree, path, structure) to a garden, positioned by its center in garden cm.",
a.createObject),
llm.DefineTool("move_object",
"Move an object to a new center position (garden cm). Needs the object's current version from describe_garden.",
a.moveObject),
llm.DefineTool("place_planting",
"Place one plop of a plant inside a plantable object, positioned in the object's LOCAL frame (0,0 = object center, -y = north).",
a.placePlanting),
llm.DefineTool("fill_region",
"Fill a named region of a plantable object with a plant, hex-packed. Region is one of nw/ne/sw/se (corners), north/south/east/west or top/bottom/left/right (halves), or all.",
a.fillRegion),
llm.DefineTool("clear_object",
"Remove all plants from an object (soft-remove; history is kept).",
a.clearObject),
)
}
// adapter carries the service and the acting user for the tool handlers.
type adapter struct {
svc *service.Service
actor int64
}
func (a *adapter) listGardens(ctx context.Context, _ struct{}) (any, error) {
return a.svc.ListGardens(ctx, a.actor)
}
func (a *adapter) describeGarden(ctx context.Context, args struct {
GardenID int64 `json:"gardenId" description:"id of the garden to describe"`
}) (any, error) {
return a.svc.DescribeGarden(ctx, a.actor, args.GardenID)
}
func (a *adapter) createObject(ctx context.Context, args struct {
GardenID int64 `json:"gardenId" description:"garden to add the object to"`
Kind string `json:"kind" description:"bed | grow_bag | container | in_ground | tree | path | structure"`
Name string `json:"name" description:"optional label for the object"`
Shape string `json:"shape" description:"rect or circle (default rect)"`
XCM float64 `json:"xCm" description:"center x in garden cm"`
YCM float64 `json:"yCm" description:"center y in garden cm"`
WidthCM float64 `json:"widthCm" description:"width in cm (a circle's diameter)"`
HeightCM float64 `json:"heightCm" description:"height in cm"`
}) (any, error) {
return a.svc.CreateObject(ctx, a.actor, args.GardenID, service.ObjectInput{
Kind: args.Kind, Name: args.Name, Shape: args.Shape,
XCM: args.XCM, YCM: args.YCM, WidthCM: args.WidthCM, HeightCM: args.HeightCM,
})
}
func (a *adapter) moveObject(ctx context.Context, args struct {
ObjectID int64 `json:"objectId" description:"object to move"`
XCM float64 `json:"xCm" description:"new center x in garden cm"`
YCM float64 `json:"yCm" description:"new center y in garden cm"`
Version int64 `json:"version" description:"the object's current version (from describe_garden)"`
}) (any, error) {
return a.svc.UpdateObject(ctx, a.actor, args.ObjectID,
service.ObjectPatch{XCM: &args.XCM, YCM: &args.YCM}, args.Version)
}
func (a *adapter) placePlanting(ctx context.Context, args struct {
ObjectID int64 `json:"objectId" description:"plantable object to plant in"`
PlantID int64 `json:"plantId" description:"plant to place"`
XCM float64 `json:"xCm" description:"center x in the object's local frame (cm; 0,0 = center, -y = north)"`
YCM float64 `json:"yCm" description:"center y in the object's local frame (cm)"`
RadiusCM float64 `json:"radiusCm" description:"plop radius in cm"`
Count *int `json:"count" description:"optional explicit plant count; omit to derive from area ÷ spacing²"`
}) (any, error) {
return a.svc.CreatePlanting(ctx, a.actor, args.ObjectID, service.PlantingInput{
PlantID: args.PlantID, XCM: args.XCM, YCM: args.YCM, RadiusCM: args.RadiusCM, Count: args.Count,
})
}
func (a *adapter) fillRegion(ctx context.Context, args struct {
ObjectID int64 `json:"objectId" description:"plantable object to fill"`
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"`
}) (any, error) {
return a.svc.FillNamedRegion(ctx, a.actor, args.ObjectID, args.Region, args.PlantID, args.SpacingOverride)
}
func (a *adapter) clearObject(ctx context.Context, args struct {
ObjectID int64 `json:"objectId" description:"object to remove all plants from"`
}) (any, error) {
n, err := a.svc.ClearObject(ctx, a.actor, args.ObjectID)
if err != nil {
return nil, err
}
return map[string]int{"cleared": n}, nil
}
+133
View File
@@ -0,0 +1,133 @@
//go:build majordomo
package agent
import (
"context"
"encoding/json"
"testing"
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/store"
)
// TestToolboxScenario is the #19 demo harness: it drives the DESIGN scenario
// through the tool layer (JSON args → DefineTool → service) rather than a live
// model — fill the NE corner with garlic, the NW with basil, the south with
// beans — and checks describe_garden reports three groups in the right places.
// It also confirms the toolbox inherits pansy's ACL (a viewer is refused).
//
// Build/run with: go test -tags majordomo ./internal/agent/
func TestToolboxScenario(t *testing.T) {
ctx := context.Background()
db, err := store.Open(":memory:")
if err != nil {
t.Fatalf("open: %v", err)
}
t.Cleanup(func() { db.Close() })
if err := db.Migrate(ctx); err != nil {
t.Fatalf("migrate: %v", err)
}
svc := service.New(db, &config.Config{Registration: config.RegistrationOpen, LocalAuth: true})
owner, err := svc.Register(ctx, service.RegisterInput{Email: "[email protected]", DisplayName: "A", Password: "password123"})
if err != nil {
t.Fatalf("register: %v", err)
}
box := NewToolbox(svc, owner.ID)
call := func(name string, args any) llm.ToolResult {
t.Helper()
raw, _ := json.Marshal(args)
return box.Execute(ctx, llm.ToolCall{ID: "1", Name: name, Arguments: raw})
}
// Garden + plants are set up directly (there are no create_garden/plant tools);
// the agent-facing bits — object + fills + describe — go through the toolbox.
g, err := svc.CreateGarden(ctx, owner.ID, service.GardenInput{Name: "Plot", WidthCM: 2000, HeightCM: 2000})
if err != nil {
t.Fatalf("garden: %v", err)
}
garlic, _ := svc.CreatePlant(ctx, owner.ID, service.PlantInput{Name: "Garlic", Category: "vegetable", SpacingCM: 15, Color: "#d9d2c5", Icon: "🧄"})
Review

🟡 Swallowed CreatePlant errors in test setup

error-handling, maintainability · flagged by 2 models

  • internal/agent/tools_test.go:54-56 — Swallowed errors in test setup CreatePlant returns (*domain.Plant, error) but the test discards the error with _, _ for garlic, basil, and beans. If plant creation fails, the test will later fail with a confusing downstream error (e.g. plantId: 0 rejected) instead of surfacing the root cause. Check the errors like the rest of the test body and like ops_test.go's seedNamedPlant.

🪰 Gadfly · advisory

🟡 **Swallowed CreatePlant errors in test setup** _error-handling, maintainability · flagged by 2 models_ - **`internal/agent/tools_test.go:54-56` — Swallowed errors in test setup** `CreatePlant` returns `(*domain.Plant, error)` but the test discards the error with `_, _` for garlic, basil, and beans. If plant creation fails, the test will later fail with a confusing downstream error (e.g. `plantId: 0` rejected) instead of surfacing the root cause. Check the errors like the rest of the test body and like `ops_test.go`'s `seedNamedPlant`. <sub>🪰 Gadfly · advisory</sub>
basil, _ := svc.CreatePlant(ctx, owner.ID, service.PlantInput{Name: "Basil", Category: "herb", SpacingCM: 25, Color: "#4a7c3f", Icon: "🌿"})
beans, _ := svc.CreatePlant(ctx, owner.ID, service.PlantInput{Name: "Beans", Category: "vegetable", SpacingCM: 10, Color: "#6b8e23", Icon: "🫘"})
// create_object → a 400×400 bed.
res := call("create_object", map[string]any{
"gardenId": g.ID, "kind": "bed", "name": "Bed 1", "xCm": 1000, "yCm": 1000, "widthCm": 400, "heightCm": 400,
})
if res.IsError {
t.Fatalf("create_object: %s", res.Content)
}
var bed struct {
ID int64 `json:"id"`
}
if err := json.Unmarshal([]byte(res.Content), &bed); err != nil {
t.Fatalf("decode created object: %v", err)
}
// fill_region for each corner/half.
for _, f := range []struct {
region string
plantID int64
}{{"ne", garlic.ID}, {"nw", basil.ID}, {"south", beans.ID}} {
if r := call("fill_region", map[string]any{"objectId": bed.ID, "region": f.region, "plantId": f.plantID}); r.IsError {
t.Fatalf("fill_region %s: %s", f.region, r.Content)
}
}
// describe_garden → parse and check locations.
res = call("describe_garden", map[string]any{"gardenId": g.ID})
if res.IsError {
t.Fatalf("describe_garden: %s", res.Content)
}
var desc service.DescribeResult
if err := json.Unmarshal([]byte(res.Content), &desc); err != nil {
t.Fatalf("decode describe: %v", err)
}
if len(desc.Objects) != 1 {
t.Fatalf("objects = %d, want 1", len(desc.Objects))
}
seen := map[string]map[string]bool{}
for _, p := range desc.Objects[0].Plantings {
if seen[p.Plant] == nil {
seen[p.Plant] = map[string]bool{}
}
seen[p.Plant][p.Location] = true
}
if !seen["Garlic"]["NE corner"] {
t.Errorf("garlic at %v, want NE corner", seen["Garlic"])
}
if !seen["Basil"]["NW corner"] {
t.Errorf("basil at %v, want NW corner", seen["Basil"])
}
if len(seen["Beans"]) == 0 {
t.Error("beans produced no plops")
}
for loc := range seen["Beans"] {
if loc == "north" || loc == "NE corner" || loc == "NW corner" || loc == "center" {
t.Errorf("beans at %q, want only southern locations", loc)
}
}
// ACL: a viewer's fill_region is refused (the toolbox runs as that actor).
viewerUser, _ := svc.Register(ctx, service.RegisterInput{Email: "[email protected]", DisplayName: "V", Password: "password123"})
if _, err := svc.AddShare(ctx, owner.ID, g.ID, "[email protected]", "viewer"); err != nil {
Outdated
Review

🟡 String literal role instead of domain.RoleViewer constant

maintainability · flagged by 1 model

  • internal/agent/tools_test.go:118 — String literal role "viewer" instead of constant The rest of the service tests (e.g. ops_test.go:171) use domain.RoleViewer. Using a raw string breaks the project's established pattern and is brittle if the canonical value changes. Import internal/domain and use domain.RoleViewer.

🪰 Gadfly · advisory

🟡 **String literal role instead of domain.RoleViewer constant** _maintainability · flagged by 1 model_ - **`internal/agent/tools_test.go:118` — String literal role `"viewer"` instead of constant** The rest of the service tests (e.g. `ops_test.go:171`) use `domain.RoleViewer`. Using a raw string breaks the project's established pattern and is brittle if the canonical value changes. Import `internal/domain` and use `domain.RoleViewer`. <sub>🪰 Gadfly · advisory</sub>
t.Fatalf("share: %v", err)
}
viewerBox := NewToolbox(svc, viewerUser.ID)
vr := viewerBox.Execute(ctx, llm.ToolCall{ID: "2", Name: "fill_region", Arguments: mustJSON(map[string]any{
"objectId": bed.ID, "region": "all", "plantId": garlic.ID,
})})
if !vr.IsError {
t.Errorf("viewer fill_region succeeded, want a permission error")
}
}
func mustJSON(v any) json.RawMessage {
Review

mustJSON helper swallows marshal errors and lacks t.Helper()

maintainability · flagged by 1 model

  • internal/agent/tools_test.go:130mustJSON swallows errors and lacks t.Helper(). The helper ignores json.Marshal errors and doesn’t mark itself with t.Helper(), so a failure would print the wrong line number. It should panic on error and call t.Helper() for clearer test output.

🪰 Gadfly · advisory

⚪ **mustJSON helper swallows marshal errors and lacks t.Helper()** _maintainability · flagged by 1 model_ - **`internal/agent/tools_test.go:130` — `mustJSON` swallows errors and lacks `t.Helper()`.** The helper ignores `json.Marshal` errors and doesn’t mark itself with `t.Helper()`, so a failure would print the wrong line number. It should panic on error and call `t.Helper()` for clearer test output. <sub>🪰 Gadfly · advisory</sub>
b, _ := json.Marshal(v)
return b
}
+323
View File
@@ -0,0 +1,323 @@
package service
import (
"context"
"math"
"strings"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
)
// This file holds pansy's bulk, "natural-language-shaped" operations — the ones
// an agent drives ("fill the NE corner with garlic", "clear the bed"). They live
// on *Service like every other operation, so agent tools (internal/agent) and any
// future REST surface inherit the same ACL enforcement via objectForRole /
// requireGardenRole. Geometry is in each object's LOCAL frame (origin at the
// object's center, +x east, +y south, so -y is NORTH).
// Region is an area in an object's local frame: an axis-aligned rectangle, or a
// circle when Circle is set. NamedRegion produces the common ones.
type Region struct {
Outdated
Review

🟡 Region.Circle/CX/CY/Radius fields and contains circle branch are dead/speculative code never produced by NamedRegion and unsupported by hexCenters

maintainability · flagged by 4 models

  • internal/service/ops.go:20-24Region.Circle/CX/CY/Radius are dead/speculative fields. NamedRegion (the only Region constructor in the package) always returns rectangles via rect, which never sets Circle. A repo-wide grep for Circle =/Circle:/Circle: true finds no assignment anywhere — Circle is only ever read in the r.Circle branch of Region.contains (ops.go:27-33), which is therefore unreachable from any production path. hexCenters (ops.go:144-166) also d…

🪰 Gadfly · advisory

🟡 **Region.Circle/CX/CY/Radius fields and contains circle branch are dead/speculative code never produced by NamedRegion and unsupported by hexCenters** _maintainability · flagged by 4 models_ - **`internal/service/ops.go:20-24` — `Region.Circle`/`CX`/`CY`/`Radius` are dead/speculative fields.** `NamedRegion` (the only `Region` constructor in the package) always returns rectangles via `rect`, which never sets `Circle`. A repo-wide grep for `Circle =`/`Circle:`/`Circle: true` finds no assignment anywhere — `Circle` is only ever read in the `r.Circle` branch of `Region.contains` (ops.go:27-33), which is therefore unreachable from any production path. `hexCenters` (ops.go:144-166) also d… <sub>🪰 Gadfly · advisory</sub>
MinX, MinY, MaxX, MaxY float64 // rectangle (also the circle's bounding box)
Circle bool
CX, CY, Radius float64 // used when Circle
}
// contains reports whether a local point lies in the region.
func (r Region) contains(x, y float64) bool {
if r.Circle {
dx, dy := x-r.CX, y-r.CY
return dx*dx+dy*dy <= r.Radius*r.Radius
}
return x >= r.MinX && x <= r.MaxX && y >= r.MinY && y <= r.MaxY
}
// rect builds a rectangular region.
func rect(minX, minY, maxX, maxY float64) Region {
return Region{MinX: minX, MinY: minY, MaxX: maxX, MaxY: maxY}
}
// NamedRegion resolves a compass name to a Region in the object's local frame.
// Recognizes the quarter corners "nw|ne|sw|se", the halves
// "north|south|east|west" and their "top|bottom|left|right" synonyms, and "all".
// A trailing "corner"/"half" word is ignored ("NE corner", "south half"). North
// is -y (see the file header). Unknown names return ErrInvalidInput.
func NamedRegion(o *domain.GardenObject, name string) (Region, error) {
hw, hh := o.WidthCM/2, o.HeightCM/2
Review

🔴 NamedRegion nil pointer dereference on nil GardenObject

correctness, error-handling · flagged by 2 models

  • internal/service/ops.go:48-49 (NamedRegion) — the literal strings "corner" or "half" alone silently resolve to "all" instead of being rejected. TrimSuffix doesn't require a preceding separator: input "corner" → strip suffix "corner""", which matches the case "all", "": branch, filling the entire object rather than erroring. Same for "half". Low real-world likelihood (the toolbox description enumerates only compass-qualified values), but it's a genuine mismatch betw…

🪰 Gadfly · advisory

🔴 **NamedRegion nil pointer dereference on nil GardenObject** _correctness, error-handling · flagged by 2 models_ - **`internal/service/ops.go:48-49` (`NamedRegion`) — the literal strings `"corner"` or `"half"` alone silently resolve to `"all"` instead of being rejected.** `TrimSuffix` doesn't require a preceding separator: input `"corner"` → strip suffix `"corner"` → `""`, which matches the `case "all", "":` branch, filling the entire object rather than erroring. Same for `"half"`. Low real-world likelihood (the toolbox description enumerates only compass-qualified values), but it's a genuine mismatch betw… <sub>🪰 Gadfly · advisory</sub>
key := strings.ToLower(strings.TrimSpace(name))
key = strings.TrimSpace(strings.TrimSuffix(key, "corner"))
key = strings.TrimSpace(strings.TrimSuffix(key, "half"))
switch key {
case "all", "":
Review

🟡 Empty/blank region name silently maps to "all" instead of returning ErrInvalidInput, contradicting the doc and surprising bad-input handling

error-handling, security · flagged by 2 models

  • internal/service/ops.go:52 — empty/blank region name silently maps to "all" instead of ErrInvalidInput. The doc comment (line 44) states "Unknown names return ErrInvalidInput," but case "all", "": (line 52) accepts an empty string (after trim) as the whole-object rect. Through the agent tool this means fill_region with region: "" (or whitespace) fills the entire object rather than being rejected — a surprising edge case for bad input. Suggested fix: drop "" from the all cas…

🪰 Gadfly · advisory

🟡 **Empty/blank region name silently maps to "all" instead of returning ErrInvalidInput, contradicting the doc and surprising bad-input handling** _error-handling, security · flagged by 2 models_ - **`internal/service/ops.go:52` — empty/blank region name silently maps to `"all"` instead of `ErrInvalidInput`.** The doc comment (line 44) states "Unknown names return ErrInvalidInput," but `case "all", "":` (line 52) accepts an empty string (after trim) as the whole-object rect. Through the agent tool this means `fill_region` with `region: ""` (or whitespace) fills the entire object rather than being rejected — a surprising edge case for bad input. Suggested fix: drop `""` from the `all` cas… <sub>🪰 Gadfly · advisory</sub>
return rect(-hw, -hh, hw, hh), nil
case "north", "top":
return rect(-hw, -hh, hw, 0), nil
case "south", "bottom":
return rect(-hw, 0, hw, hh), nil
case "east", "right":
return rect(0, -hh, hw, hh), nil
case "west", "left":
return rect(-hw, -hh, 0, hh), nil
case "nw", "northwest":
return rect(-hw, -hh, 0, 0), nil
case "ne", "northeast":
return rect(0, -hh, hw, 0), nil
case "sw", "southwest":
return rect(-hw, 0, 0, hh), nil
case "se", "southeast":
return rect(0, 0, hw, hh), nil
default:
return Region{}, domain.ErrInvalidInput
}
}
// defaultPlopRadius is #15's formula for a placed plop's radius: max(1.5×spacing,
Review

defaultPlopRadius comment says 'Reused here (don't duplicate the constant elsewhere)' but this is the only definition in the codebase — misleading phrasing

maintainability · flagged by 1 model

  • internal/service/ops.go:75-79 — misleading "Reused here" comment on defaultPlopRadius. The comment says "defaultPlopRadius is #15's formula... Reused here (don't duplicate the constant elsewhere)." But this is the only definition of the function in the codebase (grep for defaultPlopRadius returns exactly this definition plus its one call site and two tests). The phrasing reads as if it's being imported/duplicated from #15's code, when it's actually the canonical definition. Minor; th…

🪰 Gadfly · advisory

⚪ **defaultPlopRadius comment says 'Reused here (don't duplicate the constant elsewhere)' but this is the only definition in the codebase — misleading phrasing** _maintainability · flagged by 1 model_ - **`internal/service/ops.go:75-79` — misleading "Reused here" comment on `defaultPlopRadius`.** The comment says "defaultPlopRadius is #15's formula... Reused here (don't duplicate the constant elsewhere)." But this is the only definition of the function in the codebase (grep for `defaultPlopRadius` returns exactly this definition plus its one call site and two tests). The phrasing reads as if it's being imported/duplicated from #15's code, when it's actually the canonical definition. Minor; th… <sub>🪰 Gadfly · advisory</sub>
// 15cm). Reused here (don't duplicate the constant elsewhere).
func defaultPlopRadius(spacingCM float64) float64 {
return math.Max(1.5*spacingCM, 15)
}
// FillRegion lays a hex-packed field of plops of one plant across a region of a
// plantable object the actor can edit. Plop radius comes from the plant's spacing
// (or spacingOverride) via defaultPlopRadius; centers sit on a hex lattice at 2×
// radius pitch, kept where the center is inside the region. A candidate is
// skipped when its plop would sit entirely inside an existing active plop (so
// re-filling doesn't stack duplicates). Returns the plops it created.
func (s *Service) FillRegion(ctx context.Context, actorID, objectID int64, region Region, plantID int64, spacingOverride *float64) ([]domain.Planting, error) {
o, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor)
if err != nil {
return nil, err
}
if !o.Plantable {
return nil, domain.ErrInvalidInput
}
plant, err := s.visiblePlant(ctx, actorID, plantID)
if err != nil {
return nil, err
}
spacing := plant.SpacingCM
if spacingOverride != nil {
if !isFinite(*spacingOverride) || *spacingOverride < minPlantSpacingCM || *spacingOverride > maxPlantSpacingCM {
return nil, domain.ErrInvalidInput
}
spacing = *spacingOverride
}
radius := defaultPlopRadius(spacing)
Outdated
Review

🟠 FillRegion silent empty fill when computed radius is NaN/Inf

error-handling · flagged by 2 models

  • internal/service/ops.go:108-134FillRegion's read-then-write is unguarded, so concurrent fills on the same object can create duplicate/overlapping plops. FillRegion loads existing plantings once via ListActivePlantingsForObject (line 108), computes hex-lattice candidates against that snapshot, then inserts one row at a time (line 125) with no transaction, row lock, or version check tying the read to the writes. By contrast, UpdateObject (internal/service/objects.go:136-149)…

🪰 Gadfly · advisory

🟠 **FillRegion silent empty fill when computed radius is NaN/Inf** _error-handling · flagged by 2 models_ - **`internal/service/ops.go:108-134` — `FillRegion`'s read-then-write is unguarded, so concurrent fills on the same object can create duplicate/overlapping plops.** `FillRegion` loads `existing` plantings once via `ListActivePlantingsForObject` (line 108), computes hex-lattice candidates against that snapshot, then inserts one row at a time (line 125) with no transaction, row lock, or version check tying the read to the writes. By contrast, `UpdateObject` (`internal/service/objects.go:136-149`)… <sub>🪰 Gadfly · advisory</sub>
existing, err := s.store.ListActivePlantingsForObject(ctx, objectID)
if err != nil {
return nil, err
}
today := s.now().UTC().Format(dateLayout)
halfW, halfH := o.WidthCM/2, o.HeightCM/2
created := []domain.Planting{}
for _, c := range hexCenters(region, radius) {
Review

🔴 FillRegion performs one INSERT per lattice candidate (N+1 writes); worst case ~10^5 sequential round-trips on a max-sized bed with min spacing

correctness, error-handling, performance, security · flagged by 5 models

  • internal/service/ops.go:116-134FillRegion issues one INSERT per candidate with no batching (N+1 writes). Each surviving lattice point calls s.store.CreatePlanting individually — a separate INSERT … RETURNING round-trip to SQLite (verified at store/plantings.go:122). There is no bulk insert helper anywhere in internal/store (grep for BatchInsert|BulkCreate|batch returns nothing). Objects are bounded only by validDimensionCM = maxGardenCM = 10_000 cm (gardens.go:18, ap…

🪰 Gadfly · advisory

🔴 **FillRegion performs one INSERT per lattice candidate (N+1 writes); worst case ~10^5 sequential round-trips on a max-sized bed with min spacing** _correctness, error-handling, performance, security · flagged by 5 models_ - **`internal/service/ops.go:116-134` — `FillRegion` issues one INSERT per candidate with no batching (N+1 writes).** Each surviving lattice point calls `s.store.CreatePlanting` individually — a separate `INSERT … RETURNING` round-trip to SQLite (verified at `store/plantings.go:122`). There is no bulk insert helper anywhere in `internal/store` (grep for `BatchInsert|BulkCreate|batch` returns nothing). Objects are bounded only by `validDimensionCM` = `maxGardenCM = 10_000` cm (`gardens.go:18`, ap… <sub>🪰 Gadfly · advisory</sub>
// The center must be inside the object (region ⊆ bounds already, but guard).
if math.Abs(c.x) > halfW || math.Abs(c.y) > halfH {
continue
}
if coveredByExisting(c.x, c.y, radius, existing) {
continue
}
p := &domain.Planting{ObjectID: objectID, PlantID: plantID, XCM: c.x, YCM: c.y, RadiusCM: radius, PlantedAt: &today}
stored, err := s.store.CreatePlanting(ctx, p)
if err != nil {
return nil, err
}
stored.DerivedCount = derivedCount(stored.RadiusCM, spacing)
created = append(created, *stored)
// Count what we just placed so later candidates in this same fill don't
// stack on top of it.
existing = append(existing, *stored)
}
return created, nil
}
type localPoint struct{ x, y float64 }
Review

localPoint is a one-off struct with no reuse; minor parallel abstraction over plain (x,y) coords

maintainability · flagged by 1 model

  • internal/service/ops.go:138localPoint is a one-off internal struct defined separately from Region and used only by hexCenters/FillRegion. Since Region.contains already takes (x, y float64), localPoint adds a small parallel abstraction with no reuse elsewhere (grep confirms no other references). A plain (x, y float64) return would be one fewer type to track. Trivial; leave if you prefer the named clarity.

🪰 Gadfly · advisory

⚪ **localPoint is a one-off struct with no reuse; minor parallel abstraction over plain (x,y) coords** _maintainability · flagged by 1 model_ - **`internal/service/ops.go:138` — `localPoint` is a one-off internal struct** defined separately from `Region` and used only by `hexCenters`/`FillRegion`. Since `Region.contains` already takes `(x, y float64)`, `localPoint` adds a small parallel abstraction with no reuse elsewhere (grep confirms no other references). A plain `(x, y float64)` return would be one fewer type to track. Trivial; leave if you prefer the named clarity. <sub>🪰 Gadfly · advisory</sub>
// hexCenters returns hex-packed lattice centers whose center lies in the region.
// Rows are spaced radius·√3 apart and every other row is offset by radius, the
// standard hexagonal packing at a 2×radius pitch. The lattice is anchored one
// radius inside the region's min corner so the first plop sits inside it.
func hexCenters(r Region, radius float64) []localPoint {
Review

🟡 hexCenters generates the full lattice before the bounds guard filters it; an oversized caller-supplied Region (not clamped to the object) can cause a very long loop / large allocation

error-handling · flagged by 1 model

  • hexCenters can loop a very long time on an oversized caller-supplied Regioninternal/service/ops.go:144-166. FillRegion takes a caller-supplied Region that is never validated to lie within the object or clamped to [-halfW,halfW]×[-halfH,halfH]. hexCenters generates the full lattice before the math.Abs(c.x) > halfW guard at ops.go:118 filters each point, so a region like MinX=-1e9, MaxX=1e9 with a tiny radius would generate a huge lattice only to discard every point.…

🪰 Gadfly · advisory

🟡 **hexCenters generates the full lattice before the bounds guard filters it; an oversized caller-supplied Region (not clamped to the object) can cause a very long loop / large allocation** _error-handling · flagged by 1 model_ - **`hexCenters` can loop a very long time on an oversized caller-supplied `Region`** — `internal/service/ops.go:144-166`. `FillRegion` takes a caller-supplied `Region` that is never validated to lie within the object or clamped to `[-halfW,halfW]×[-halfH,halfH]`. `hexCenters` generates the full lattice *before* the `math.Abs(c.x) > halfW` guard at `ops.go:118` filters each point, so a region like `MinX=-1e9, MaxX=1e9` with a tiny radius would generate a huge lattice only to discard every point.… <sub>🪰 Gadfly · advisory</sub>
if radius <= 0 {
return nil
}
pitch := 2 * radius
rowH := pitch * math.Sqrt(3) / 2
const eps = 1e-6
var pts []localPoint
row := 0
for y := r.MinY + radius; y <= r.MaxY+eps; y += rowH {
xStart := r.MinX + radius
if row%2 == 1 {
xStart += radius
}
for x := xStart; x <= r.MaxX+eps; x += pitch {
if r.contains(x, y) {
pts = append(pts, localPoint{x, y})
}
}
row++
}
return pts
}
// coveredByExisting reports whether a new plop (center, radius) would sit
// entirely inside some existing active plop.
func coveredByExisting(x, y, radius float64, existing []domain.Planting) bool {
Review

🟠 coveredByExisting is O(N) linear scan making fill O(M*N)

performance · flagged by 1 model

  • internal/service/ops.go:170coveredByExisting is an O(N) linear scan, making the fill O(M·N). For every lattice candidate, the code iterates over all existing plops (and appends newly created ones to the same slice, so the scan grows as the fill proceeds). Impact: Coverage checking degrades quadratically. A fill that creates 100 plops performs ~5 000 distance checks; a 500-plop fill performs ~125 000. Fix: Partition existing into a coarse spatial grid (e.g. keyed by `floo…

🪰 Gadfly · advisory

🟠 **coveredByExisting is O(N) linear scan making fill O(M*N)** _performance · flagged by 1 model_ * **`internal/service/ops.go:170` — `coveredByExisting` is an O(N) linear scan, making the fill O(M·N).** For every lattice candidate, the code iterates over *all* existing plops (and appends newly created ones to the same slice, so the scan grows as the fill proceeds). **Impact:** Coverage checking degrades quadratically. A fill that creates 100 plops performs ~5 000 distance checks; a 500-plop fill performs ~125 000. **Fix:** Partition `existing` into a coarse spatial grid (e.g. keyed by `floo… <sub>🪰 Gadfly · advisory</sub>
for _, e := range existing {
if math.Hypot(x-e.XCM, y-e.YCM)+radius <= e.RadiusCM {
return true
}
}
return false
}
// FillNamedRegion is FillRegion addressed by a compass name ("ne", "south half")
// instead of a resolved Region — the ergonomic form for agent tools, which don't
// hold the object's geometry. It resolves the name against the object, then fills.
func (s *Service) FillNamedRegion(ctx context.Context, actorID, objectID int64, regionName string, plantID int64, spacingOverride *float64) ([]domain.Planting, error) {
Review

🟠 FillNamedRegion calls objectForRole twice (once itself, once via FillRegion)

correctness, error-handling, maintainability, performance · flagged by 5 models

  • internal/service/ops.go:182-191 — Redundant objectForRole call in FillNamedRegion. FillNamedRegion fetches the object via objectForRole, resolves the region, then delegates to FillRegion, which calls objectForRole again. This doubles the same permission/DB work for every named-region fill. Clean fix: extract a private fillRegion that accepts the already-resolved *domain.GardenObject, so FillNamedRegion fetches once and FillRegion can still enforce its own ACL withou…

🪰 Gadfly · advisory

🟠 **FillNamedRegion calls objectForRole twice (once itself, once via FillRegion)** _correctness, error-handling, maintainability, performance · flagged by 5 models_ - **`internal/service/ops.go:182-191` — Redundant `objectForRole` call in `FillNamedRegion`.** `FillNamedRegion` fetches the object via `objectForRole`, resolves the region, then delegates to `FillRegion`, which calls `objectForRole` *again*. This doubles the same permission/DB work for every named-region fill. Clean fix: extract a private `fillRegion` that accepts the already-resolved `*domain.GardenObject`, so `FillNamedRegion` fetches once and `FillRegion` can still enforce its own ACL withou… <sub>🪰 Gadfly · advisory</sub>
o, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor)
if err != nil {
return nil, err
}
region, err := NamedRegion(o, regionName)
if err != nil {
return nil, err
}
return s.FillRegion(ctx, actorID, objectID, region, plantID, spacingOverride)
}
// ClearObject soft-removes every active plop in an object the actor can edit (one
// UPDATE), returning how many were cleared. Distinct from deleting the object.
func (s *Service) ClearObject(ctx context.Context, actorID, objectID int64) (int, error) {
Review

🟡 ClearObject inconsistent Plantable check compared to FillRegion

correctness, error-handling · flagged by 2 models

  • internal/service/ops.go:196-202ClearObject does not check o.Plantable. FillRegion rejects non-plantable objects (trees, paths, etc.) with ErrInvalidInput, but ClearObject allows the call and simply clears 0 rows. This is harmless but inconsistent. Fix: either return ErrInvalidInput for non-plantable objects, or document that clearing them is a no-op.

🪰 Gadfly · advisory

🟡 **ClearObject inconsistent Plantable check compared to FillRegion** _correctness, error-handling · flagged by 2 models_ - **`internal/service/ops.go:196-202` — `ClearObject` does not check `o.Plantable`.** `FillRegion` rejects non-plantable objects (`trees`, `paths`, etc.) with `ErrInvalidInput`, but `ClearObject` allows the call and simply clears 0 rows. This is harmless but inconsistent. *Fix:* either return `ErrInvalidInput` for non-plantable objects, or document that clearing them is a no-op. <sub>🪰 Gadfly · advisory</sub>
if _, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor); err != nil {
return 0, err
}
today := s.now().UTC().Format(dateLayout)
return s.store.ClearObjectPlantings(ctx, objectID, today)
}
// DescribeResult is a structured summary of a garden for prompting an agent.
type DescribeResult struct {
GardenID int64 `json:"gardenId"`
Name string `json:"name"`
WidthCM float64 `json:"widthCm"`
HeightCM float64 `json:"heightCm"`
UnitPref string `json:"unitPref"`
Objects []DescribeObject `json:"objects"`
}
// DescribeObject is one object plus its active plantings, for DescribeResult.
// Version is included so an agent can move/edit the object (the mutation guard).
type DescribeObject struct {
ID int64 `json:"id"`
Kind string `json:"kind"`
Name string `json:"name"`
Shape string `json:"shape"`
WidthCM float64 `json:"widthCm"`
HeightCM float64 `json:"heightCm"`
XCM float64 `json:"xCm"`
YCM float64 `json:"yCm"`
RotationDeg float64 `json:"rotationDeg"`
Plantable bool `json:"plantable"`
Version int64 `json:"version"`
Plantings []DescribePlanting `json:"plantings"`
}
// DescribePlanting is one plop with a rough compass location, for DescribeResult.
type DescribePlanting struct {
PlantID int64 `json:"plantId"`
Plant string `json:"plant"`
Count int `json:"count"`
Location string `json:"location"`
RadiusCM float64 `json:"radiusCm"`
}
// DescribeGarden returns a structured summary — dimensions, objects, and each
// object's active plantings (plant, effective count, rough location) — for a
// garden the actor can view. Built on GardenFull so it inherits the ACL check.
func (s *Service) DescribeGarden(ctx context.Context, actorID, gardenID int64) (*DescribeResult, error) {
full, err := s.GardenFull(ctx, actorID, gardenID)
if err != nil {
return nil, err
}
plantByID := make(map[int64]domain.Plant, len(full.Plants))
for _, p := range full.Plants {
plantByID[p.ID] = p
}
plopsByObject := make(map[int64][]domain.Planting)
for _, pl := range full.Plantings {
plopsByObject[pl.ObjectID] = append(plopsByObject[pl.ObjectID], pl)
}
res := &DescribeResult{
GardenID: full.Garden.ID,
Name: full.Garden.Name,
WidthCM: full.Garden.WidthCM,
HeightCM: full.Garden.HeightCM,
UnitPref: full.Garden.UnitPref,
Objects: make([]DescribeObject, 0, len(full.Objects)),
}
for _, o := range full.Objects {
do := DescribeObject{
ID: o.ID, Kind: o.Kind, Name: o.Name, Shape: o.Shape,
WidthCM: o.WidthCM, HeightCM: o.HeightCM, XCM: o.XCM, YCM: o.YCM,
RotationDeg: o.RotationDeg, Plantable: o.Plantable, Version: o.Version,
Plantings: []DescribePlanting{},
}
for _, pl := range plopsByObject[o.ID] {
count := pl.DerivedCount
if pl.Count != nil {
count = *pl.Count
}
do.Plantings = append(do.Plantings, DescribePlanting{
PlantID: pl.PlantID,
Plant: plantByID[pl.PlantID].Name,
Review

🟡 DescribeGarden emits an empty plant name when a planting's plant is missing from the (independently-read) referenced-plants map

error-handling · flagged by 1 model

  • DescribeGarden emits an empty plant name when a planting's plant isn't in plantByIDinternal/service/ops.go:280 (Plant: plantByID[pl.PlantID].Name). full.Plants comes from ListReferencedPlants and full.Plantings from ListActivePlantingsForGarden (objects.go:171-178); both filter removed_at IS NULL (plants.go:36, plantings.go:36) but are independent reads with no transaction. If a planting is soft-removed between the two reads, it appears in full.Plantings while…

🪰 Gadfly · advisory

🟡 **DescribeGarden emits an empty plant name when a planting's plant is missing from the (independently-read) referenced-plants map** _error-handling · flagged by 1 model_ - **`DescribeGarden` emits an empty plant name when a planting's plant isn't in `plantByID`** — `internal/service/ops.go:280` (`Plant: plantByID[pl.PlantID].Name`). `full.Plants` comes from `ListReferencedPlants` and `full.Plantings` from `ListActivePlantingsForGarden` (`objects.go:171-178`); both filter `removed_at IS NULL` (`plants.go:36`, `plantings.go:36`) but are independent reads with no transaction. If a planting is soft-removed between the two reads, it appears in `full.Plantings` while… <sub>🪰 Gadfly · advisory</sub>
Count: count,
Location: describeLocation(pl.XCM, pl.YCM),
RadiusCM: pl.RadiusCM,
})
}
res.Objects = append(res.Objects, do)
}
return res, nil
}
// describeLocation reverse-maps a local point to a rough compass location — the
// inverse of NamedRegion's quarters/halves ("NE corner", "south", "center").
func describeLocation(x, y float64) string {
Review

describeLocation's final default silently maps to 'west'; an explicit ew=="W" case would make the switch's exhaustiveness obvious

maintainability · flagged by 1 model

  • internal/service/ops.go:293-323describeLocation's final switch is slightly leaky. The function builds ns/ew strings then switches on their specific values (case ns == "N", case ns == "S", case ew == "E", default: return "west"). It works, but the trailing default: return "west" silently encodes "ew == W" — a future edit that adds a fourth ns/ew state would get "west" by accident. Minor readability nit; an explicit case ew == "W": with a real default: that retur…

🪰 Gadfly · advisory

⚪ **describeLocation's final default silently maps to 'west'; an explicit ew=="W" case would make the switch's exhaustiveness obvious** _maintainability · flagged by 1 model_ - **`internal/service/ops.go:293-323` — `describeLocation`'s final switch is slightly leaky.** The function builds `ns`/`ew` strings then switches on their specific values (`case ns == "N"`, `case ns == "S"`, `case ew == "E"`, `default: return "west"`). It works, but the trailing `default: return "west"` silently encodes "ew == W" — a future edit that adds a fourth `ns`/`ew` state would get "west" by accident. Minor readability nit; an explicit `case ew == "W":` with a real `default:` that retur… <sub>🪰 Gadfly · advisory</sub>
const eps = 1e-6
ns := ""
switch {
case y < -eps:
ns = "N"
case y > eps:
ns = "S"
}
ew := ""
switch {
case x < -eps:
ew = "W"
case x > eps:
ew = "E"
}
switch {
case ns == "" && ew == "":
return "center"
case ns != "" && ew != "":
return ns + ew + " corner"
case ns == "N":
return "north"
case ns == "S":
return "south"
case ew == "E":
return "east"
default:
return "west"
}
}
+257
View File
@@ -0,0 +1,257 @@
package service
import (
"context"
"errors"
"testing"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
)
func TestNamedRegion(t *testing.T) {
o := &domain.GardenObject{WidthCM: 200, HeightCM: 100} // hw=100, hh=50
cases := []struct {
name string
minX, minY, maxX, maxY float64
}{
{"all", -100, -50, 100, 50},
{"north", -100, -50, 100, 0},
{"top half", -100, -50, 100, 0},
{"south", -100, 0, 100, 50},
{"bottom", -100, 0, 100, 50},
{"east", 0, -50, 100, 50},
{"right half", 0, -50, 100, 50},
{"west", -100, -50, 0, 50},
{"nw", -100, -50, 0, 0},
{"NE corner", 0, -50, 100, 0},
{"northeast", 0, -50, 100, 0},
{"sw", -100, 0, 0, 50},
{"se corner", 0, 0, 100, 50},
}
for _, c := range cases {
r, err := NamedRegion(o, c.name)
if err != nil {
t.Errorf("%q: unexpected error %v", c.name, err)
continue
}
if r.MinX != c.minX || r.MinY != c.minY || r.MaxX != c.maxX || r.MaxY != c.maxY {
t.Errorf("%q = [%v,%v,%v,%v], want [%v,%v,%v,%v]", c.name, r.MinX, r.MinY, r.MaxX, r.MaxY, c.minX, c.minY, c.maxX, c.maxY)
}
}
if _, err := NamedRegion(o, "middle-ish"); !errors.Is(err, domain.ErrInvalidInput) {
t.Errorf("unknown region err = %v, want ErrInvalidInput", err)
}
}
func TestDefaultPlopRadius(t *testing.T) {
if got := defaultPlopRadius(4); got != 15 { // 1.5*4=6 → floored to 15
t.Errorf("radius(4) = %v, want 15", got)
}
if got := defaultPlopRadius(20); got != 30 { // 1.5*20
t.Errorf("radius(20) = %v, want 30", got)
}
}
// seedFillBed makes a plantable bed of the given size centered in a big garden.
func seedFillBed(t *testing.T, s *Service, owner, gardenID int64, w, h float64) *domain.GardenObject {
Outdated
Review

🟡 Duplicate seedFillBed helper; seedBed already exists in same package

maintainability · flagged by 1 model

  • internal/service/ops_test.go:56 / internal/service/plantings_test.go:12 — Duplicate test helper seedFillBed vs seedBed. Same package, same purpose (create a plantable bed), only the dimensions/position differ. Parameterize the existing seedBed helper instead of adding a second one.

🪰 Gadfly · advisory

🟡 **Duplicate seedFillBed helper; seedBed already exists in same package** _maintainability · flagged by 1 model_ - **`internal/service/ops_test.go:56` / `internal/service/plantings_test.go:12` — Duplicate test helper `seedFillBed` vs `seedBed`.** Same package, same purpose (create a plantable bed), only the dimensions/position differ. Parameterize the existing `seedBed` helper instead of adding a second one. <sub>🪰 Gadfly · advisory</sub>
t.Helper()
o, err := s.CreateObject(context.Background(), owner, gardenID, ObjectInput{
Kind: domain.KindBed, XCM: 1000, YCM: 1000, WidthCM: w, HeightCM: h,
})
if err != nil {
t.Fatalf("seed bed %vx%v: %v", w, h, err)
}
return o
}
func TestFillRegionDeterministicPacking(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g, err := s.CreateGarden(ctx, owner, GardenInput{Name: "Big", WidthCM: 2000, HeightCM: 2000})
Review

2000x2000 garden setup duplicated verbatim across 4 tests in two files instead of a shared seed helper

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **2000x2000 garden setup duplicated verbatim across 4 tests in two files instead of a shared seed helper** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
if err != nil {
t.Fatalf("garden: %v", err)
}
bed := seedFillBed(t, s, owner, g.ID, 60, 60) // hw=hh=30
plant := seedOwnPlant(t, s, owner, 10) // radius = max(15,15) = 15
region, _ := NamedRegion(bed, "all")
created, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil)
if err != nil {
t.Fatalf("FillRegion: %v", err)
}
// Hex lattice on [-30,30]² at pitch 30, rows ~26 apart → 4 plops (2 rows × 2).
if len(created) != 4 {
t.Fatalf("filled %d plops, want 4 (60×60 bed, radius 15)", len(created))
}
for _, p := range created {
if p.RadiusCM != 15 || p.PlantedAt == nil || p.DerivedCount < 1 {
t.Errorf("unexpected created plop: %+v", p)
}
if p.XCM < -30 || p.XCM > 30 || p.YCM < -30 || p.YCM > 30 {
t.Errorf("plop center out of bed bounds: %+v", p)
}
}
// Re-filling the same region skips everything (each candidate sits exactly on
// an existing plop → entirely inside it).
again, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil)
if err != nil {
t.Fatalf("second FillRegion: %v", err)
}
if len(again) != 0 {
t.Errorf("re-fill created %d plops, want 0 (all covered)", len(again))
}
}
func TestFillRegionRotatedBedUsesLocalFrame(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g, _ := s.CreateGarden(ctx, owner, GardenInput{Name: "Big", WidthCM: 2000, HeightCM: 2000})
bed := seedFillBed(t, s, owner, g.ID, 400, 400)
// Rotate the bed 45°; the fill must still target the LOCAL NE corner.
rot := 45.0
bed, _ = s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{RotationDeg: &rot}, bed.Version)
plant := seedOwnPlant(t, s, owner, 20)
region, _ := NamedRegion(bed, "ne")
created, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil)
if err != nil {
t.Fatalf("FillRegion: %v", err)
}
if len(created) == 0 {
t.Fatal("expected some plops in the NE corner")
}
for _, p := range created {
// NE corner in local coords: x ≥ 0 (east), y ≤ 0 (north).
if p.XCM < 0 || p.YCM > 0 {
t.Errorf("plop not in local NE corner: %+v", p)
}
}
}
func TestClearObject(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g := seedGarden(t, s, owner)
bed := seedBed(t, s, owner, g.ID)
plant := seedOwnPlant(t, s, owner, 10)
region, _ := NamedRegion(bed, "all")
if _, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil); err != nil {
t.Fatalf("fill: %v", err)
}
n, err := s.ClearObject(ctx, owner, bed.ID)
if err != nil {
t.Fatalf("ClearObject: %v", err)
}
if n < 1 {
t.Fatalf("cleared %d, want ≥ 1", n)
}
full, _ := s.GardenFull(ctx, owner, g.ID)
if len(full.Plantings) != 0 {
t.Errorf("plantings after clear = %d, want 0", len(full.Plantings))
}
// Clearing an already-empty object clears 0.
if again, _ := s.ClearObject(ctx, owner, bed.ID); again != 0 {
t.Errorf("second clear = %d, want 0", again)
}
}
func TestOpsForbiddenForViewer(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
viewer := seedUser(t, s, "[email protected]")
g := seedGarden(t, s, owner)
bed := seedBed(t, s, owner, g.ID)
plant := seedOwnPlant(t, s, owner, 10)
if _, err := s.AddShare(ctx, owner, g.ID, "[email protected]", domain.RoleViewer); err != nil {
t.Fatalf("share: %v", err)
}
region, _ := NamedRegion(bed, "all")
if _, err := s.FillRegion(ctx, viewer, bed.ID, region, plant.ID, nil); !errors.Is(err, domain.ErrForbidden) {
t.Errorf("viewer fill = %v, want ErrForbidden", err)
}
if _, err := s.ClearObject(ctx, viewer, bed.ID); !errors.Is(err, domain.ErrForbidden) {
t.Errorf("viewer clear = %v, want ErrForbidden", err)
}
// But a viewer can DescribeGarden (read).
if _, err := s.DescribeGarden(ctx, viewer, g.ID); err != nil {
t.Errorf("viewer describe = %v, want ok", err)
}
}
// TestFillScenario is the DESIGN scenario: garlic in the NE corner, basil in the
// NW, beans across the south — three distinct groups in the right places.
func TestFillScenario(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g, _ := s.CreateGarden(ctx, owner, GardenInput{Name: "Plot", WidthCM: 2000, HeightCM: 2000})
bed := seedFillBed(t, s, owner, g.ID, 400, 400)
garlic := seedNamedPlant(t, s, owner, "Garlic", 15)
basil := seedNamedPlant(t, s, owner, "Basil", 25)
beans := seedNamedPlant(t, s, owner, "Beans", 10)
fill := func(name string, plantID int64) {
t.Helper()
region, err := NamedRegion(bed, name)
if err != nil {
t.Fatalf("region %q: %v", name, err)
}
if _, err := s.FillRegion(ctx, owner, bed.ID, region, plantID, nil); err != nil {
t.Fatalf("fill %q: %v", name, err)
}
}
fill("ne", garlic.ID)
fill("nw", basil.ID)
fill("south", beans.ID)
desc, err := s.DescribeGarden(ctx, owner, g.ID)
if err != nil {
t.Fatalf("DescribeGarden: %v", err)
}
if len(desc.Objects) != 1 {
t.Fatalf("objects = %d, want 1", len(desc.Objects))
}
// Tally plant → the set of rough locations it appears in.
locs := map[string]map[string]bool{}
for _, p := range desc.Objects[0].Plantings {
if locs[p.Plant] == nil {
locs[p.Plant] = map[string]bool{}
}
locs[p.Plant][p.Location] = true
}
if len(locs["Garlic"]) == 0 || !locs["Garlic"]["NE corner"] {
t.Errorf("garlic locations = %v, want NE corner", locs["Garlic"])
}
if !locs["Basil"]["NW corner"] {
t.Errorf("basil locations = %v, want NW corner", locs["Basil"])
}
// Beans fill the south half → their plops read as "south" (and possibly the
// SE/SW corners at the edges), never north.
for loc := range locs["Beans"] {
if loc == "north" || loc == "NE corner" || loc == "NW corner" || loc == "center" {
t.Errorf("beans appeared in %q, want only southern locations", loc)
}
}
if len(locs["Beans"]) == 0 {
t.Error("beans produced no plops")
}
}
// seedNamedPlant creates a custom plant with a specific name + spacing.
func seedNamedPlant(t *testing.T, s *Service, owner int64, name string, spacingCM float64) *domain.Plant {
Review

🟡 Duplicate seedNamedPlant helper; seedOwnPlant already exists in same package

maintainability · flagged by 2 models

  • internal/service/ops_test.go:248 / internal/service/plantings_test.go:24 — Duplicate test helper seedNamedPlant vs seedOwnPlant. Both are in package service and do the same thing (create a plant for an owner), differing only by whether the name is parameterized. Having two helpers for the same seeding operation is unnecessary copy-paste; consolidate into one helper with an optional name parameter.

🪰 Gadfly · advisory

🟡 **Duplicate seedNamedPlant helper; seedOwnPlant already exists in same package** _maintainability · flagged by 2 models_ - **`internal/service/ops_test.go:248` / `internal/service/plantings_test.go:24` — Duplicate test helper `seedNamedPlant` vs `seedOwnPlant`.** Both are in package `service` and do the same thing (create a plant for an owner), differing only by whether the name is parameterized. Having two helpers for the same seeding operation is unnecessary copy-paste; consolidate into one helper with an optional name parameter. <sub>🪰 Gadfly · advisory</sub>
t.Helper()
p, err := s.CreatePlant(context.Background(), owner, PlantInput{
Name: name, Category: domain.CategoryVegetable, SpacingCM: spacingCM, Color: "#4a7c3f", Icon: "🌱",
})
if err != nil {
t.Fatalf("seed plant %s: %v", name, err)
}
return p
}
+47
View File
@@ -56,6 +56,53 @@ func (d *DB) ListActivePlantingsForGarden(ctx context.Context, gardenID int64) (
return plantings, nil
}
// ListActivePlantingsForObject returns an object's currently-planted plops
// (removed_at IS NULL). Always a non-nil slice. Used by FillRegion to avoid
// stacking new plops inside existing ones.
func (d *DB) ListActivePlantingsForObject(ctx context.Context, objectID int64) ([]domain.Planting, error) {
rows, err := d.sql.QueryContext(ctx,
`SELECT `+plantingColumns+` FROM plantings WHERE object_id = ? AND removed_at IS NULL ORDER BY id`,
Review

🟠 ListActivePlantingsForObject loads unbounded rows with no composite index on (object_id, removed_at)

performance · flagged by 1 model

  • internal/store/plantings.go:64ListActivePlantingsForObject is unbounded and lacks a composite index. The query is SELECT … WHERE object_id = ? AND removed_at IS NULL. There is only a single-column index on object_id; because ClearObjectPlantings soft-removes rows (sets removed_at), an object that is cleared and refilled accumulates dead rows that are still reached by the object_id index and then filtered in-memory. There is no LIMIT. Impact: Memory usage and query ti…

🪰 Gadfly · advisory

🟠 **ListActivePlantingsForObject loads unbounded rows with no composite index on (object_id, removed_at)** _performance · flagged by 1 model_ * **`internal/store/plantings.go:64` — `ListActivePlantingsForObject` is unbounded and lacks a composite index.** The query is `SELECT … WHERE object_id = ? AND removed_at IS NULL`. There is only a single-column index on `object_id`; because `ClearObjectPlantings` soft-removes rows (sets `removed_at`), an object that is cleared and refilled accumulates dead rows that are still reached by the `object_id` index and then filtered in-memory. There is no `LIMIT`. **Impact:** Memory usage and query ti… <sub>🪰 Gadfly · advisory</sub>
objectID,
)
if err != nil {
return nil, fmt.Errorf("store: list object plantings: %w", err)
}
defer rows.Close()
plantings := []domain.Planting{}
for rows.Next() {
p, err := scanPlanting(rows)
if err != nil {
return nil, fmt.Errorf("store: scan planting: %w", err)
}
plantings = append(plantings, *p)
}
if err := rows.Err(); err != nil {
return nil, fmt.Errorf("store: iterate plantings: %w", err)
}
return plantings, nil
}
// ClearObjectPlantings soft-removes every active plop in an object in one UPDATE
// (sets removed_at=date, bumps version) and returns how many rows it affected.
func (d *DB) ClearObjectPlantings(ctx context.Context, objectID int64, date string) (int, error) {
res, err := d.sql.ExecContext(ctx,
`UPDATE plantings
SET removed_at = ?, version = version + 1,
updated_at = strftime('%Y-%m-%dT%H:%M:%SZ', 'now')
WHERE object_id = ? AND removed_at IS NULL`,
date, objectID,
)
if err != nil {
return 0, fmt.Errorf("store: clear object plantings: %w", err)
}
n, err := res.RowsAffected()
if err != nil {
return 0, fmt.Errorf("store: clear plantings rows: %w", err)
}
return int(n), nil
}
// GetPlanting returns the planting with the given id, or domain.ErrNotFound.
func (d *DB) GetPlanting(ctx context.Context, id int64) (*domain.Planting, error) {
p, err := scanPlanting(d.sql.QueryRowContext(ctx,