Address Gadfly review on #19: batch fills + tighten Region

- FillRegion: insert the whole batch in one transaction (store.CreatePlantings)
  instead of one round-trip per plop, and refuse fills over maxFillPlops (5000)
  so a max-sized bed with tiny spacing can't generate ~10^5 sequential writes.
  Guards the computed radius is finite/positive and clamps the region to the
  object's bounds before packing (bounds hexCenters).
- FillNamedRegion + FillRegion share a fillLoaded body, so the object is loaded
  and authorized once (no double objectForRole).
- Region is now rect-only — dropped the speculative circle fields/branch that
  NamedRegion never produced and hexCenters didn't pack (circles are post-v1).
- NamedRegion guards a nil object and no longer maps a blank name to "all"
  (blank → ErrInvalidInput, matching the doc).
- ClearObject documents why it deliberately doesn't require plantable.

Tests: empty/nil region name → ErrInvalidInput; an oversized fill → ErrInvalidInput
(over the cap). Tagged demo tidied (checked errors, domain.RoleViewer, t.Helper).

GOWORK=off go build/vet/test ./internal/... green; tagged agent test green against
majordomo (go.mod stays majordomo-free).

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

internal/agent/tools_test.go

@@ -10,6 +10,7 @@ import (
 	"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
 
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
+	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/store"
 )
@@ -51,9 +52,9 @@ func TestToolboxScenario(t *testing.T) {
 	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: "🧄"})
+	garlic := mustPlant(t, svc, owner.ID, "Garlic", 15, "🧄")
-	basil, _ := svc.CreatePlant(ctx, owner.ID, service.PlantInput{Name: "Basil", Category: "herb", SpacingCM: 25, Color: "#4a7c3f", Icon: "🌿"})
+	basil := mustPlant(t, svc, owner.ID, "Basil", 25, "🌿")
-	beans, _ := svc.CreatePlant(ctx, owner.ID, service.PlantInput{Name: "Beans", Category: "vegetable", SpacingCM: 10, Color: "#6b8e23", Icon: "🫘"})
+	beans := mustPlant(t, svc, owner.ID, "Beans", 10, "🫘")
 
 	// create_object → a 400×400 bed.
 	res := call("create_object", map[string]any{
@@ -114,12 +115,15 @@ func TestToolboxScenario(t *testing.T) {
 	}
 
 	// 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"})
+	viewerUser, err := 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 {
+	if err != nil {
+		t.Fatalf("register viewer: %v", err)
+	}
+	if _, err := svc.AddShare(ctx, owner.ID, g.ID, "[email protected]", domain.RoleViewer); err != nil {
 		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{
+	vr := viewerBox.Execute(ctx, llm.ToolCall{ID: "2", Name: "fill_region", Arguments: mustJSON(t, map[string]any{
 		"objectId": bed.ID, "region": "all", "plantId": garlic.ID,
 	})})
 	if !vr.IsError {
@@ -127,7 +131,22 @@ func TestToolboxScenario(t *testing.T) {
 	}
 }
 
-func mustJSON(v any) json.RawMessage {
+func mustJSON(t *testing.T, v any) json.RawMessage {
-	b, _ := json.Marshal(v)
+	t.Helper()
+	b, err := json.Marshal(v)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
 	return b
 }
+
+func mustPlant(t *testing.T, svc *service.Service, owner int64, name string, spacing float64, icon string) *domain.Plant {
+	t.Helper()
+	p, err := svc.CreatePlant(context.Background(), owner, service.PlantInput{
+		Name: name, Category: domain.CategoryVegetable, SpacingCM: spacing, Color: "#4a7c3f", Icon: icon,
+	})
+	if err != nil {
+		t.Fatalf("create plant %s: %v", name, err)
+	}
+	return p
+}

internal/service/ops.go

@@ -15,23 +15,31 @@ import (
 // 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
+// maxFillPlops bounds a single FillRegion so a huge bed with tiny spacing can't
-// circle when Circle is set. NamedRegion produces the common ones.
+// generate a runaway number of inserts. A bed with thousands of plops is already
+// far past any real garden; over the cap we refuse rather than silently truncate.
+const maxFillPlops = 5000
+
+// Region is an axis-aligned rectangle in an object's local frame. (Circle/polygon
+// regions are post-v1, like polygon objects; NamedRegion produces only rects.)
 type Region struct {
-	MinX, MinY, MaxX, MaxY float64 // rectangle (also the circle's bounding box)
+	MinX, MinY, MaxX, MaxY float64
-	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
 }
 
+// clampTo intersects the region with an object's local bounds (±halfW, ±halfH),
+// so an oversized caller-supplied region can't make hexCenters loop forever.
+func (r Region) clampTo(halfW, halfH float64) Region {
+	return Region{
+		MinX: math.Max(r.MinX, -halfW), MinY: math.Max(r.MinY, -halfH),
+		MaxX: math.Min(r.MaxX, halfW), MaxY: math.Min(r.MaxY, halfH),
+	}
+}
+
 // rect builds a rectangular region.
 func rect(minX, minY, maxX, maxY float64) Region {
 	return Region{MinX: minX, MinY: minY, MaxX: maxX, MaxY: maxY}
@@ -43,13 +51,16 @@ func rect(minX, minY, maxX, maxY float64) Region {
 // 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) {
+	if o == nil {
+		return Region{}, domain.ErrInvalidInput
+	}
 	hw, hh := o.WidthCM/2, o.HeightCM/2
 	key := strings.ToLower(strings.TrimSpace(name))
 	key = strings.TrimSpace(strings.TrimSuffix(key, "corner"))
 	key = strings.TrimSpace(strings.TrimSuffix(key, "half"))
 
 	switch key {
-	case "all", "":
+	case "all":
 		return rect(-hw, -hh, hw, hh), nil
 	case "north", "top":
 		return rect(-hw, -hh, hw, 0), nil
@@ -72,8 +83,8 @@ func NamedRegion(o *domain.GardenObject, name string) (Region, error) {
 	}
 }
 
-// defaultPlopRadius is #15's formula for a placed plop's radius: max(1.5×spacing,
+// defaultPlopRadius is the radius a freshly-placed plop gets from its plant's
-// 15cm). Reused here (don't duplicate the constant elsewhere).
+// spacing: max(1.5×spacing, 15cm) — matching the editor's placement default (#15).
 func defaultPlopRadius(spacingCM float64) float64 {
 	return math.Max(1.5*spacingCM, 15)
 }
@@ -89,6 +100,14 @@ func (s *Service) FillRegion(ctx context.Context, actorID, objectID int64, regio
 	if err != nil {
 		return nil, err
 	}
+	return s.fillLoaded(ctx, actorID, o, region, plantID, spacingOverride)
+}
+
+// fillLoaded is the shared body of FillRegion/FillNamedRegion given an object
+// already loaded and authorized (roleEditor). It clamps the region to the
+// object's bounds, refuses fills over maxFillPlops, and inserts the whole batch
+// in one transaction rather than one round-trip per plop.
+func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.GardenObject, region Region, plantID int64, spacingOverride *float64) ([]domain.Planting, error) {
 	if !o.Plantable {
 		return nil, domain.ErrInvalidInput
 	}
@@ -104,33 +123,36 @@ func (s *Service) FillRegion(ctx context.Context, actorID, objectID int64, regio
 		spacing = *spacingOverride
 	}
 	radius := defaultPlopRadius(spacing)
+	if !isFinite(radius) || radius <= 0 {
+		return nil, domain.ErrInvalidInput
+	}
 
-	existing, err := s.store.ListActivePlantingsForObject(ctx, objectID)
+	region = region.clampTo(o.WidthCM/2, o.HeightCM/2)
+	centers := hexCenters(region, radius)
+	if len(centers) > maxFillPlops {
+		return nil, domain.ErrInvalidInput // region too large for this spacing; ask for less
+	}
+
+	existing, err := s.store.ListActivePlantingsForObject(ctx, o.ID)
 	if err != nil {
 		return nil, err
 	}
-
 	today := s.now().UTC().Format(dateLayout)
-	halfW, halfH := o.WidthCM/2, o.HeightCM/2
+	batch := make([]*domain.Planting, 0, len(centers))
-	created := []domain.Planting{}
+	for _, c := range centers {
-	for _, c := range hexCenters(region, radius) {
-		// 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}
+		p := &domain.Planting{ObjectID: o.ID, PlantID: plantID, XCM: c.x, YCM: c.y, RadiusCM: radius, PlantedAt: &today}
-		stored, err := s.store.CreatePlanting(ctx, p)
+		batch = append(batch, p)
-		if err != nil {
+		existing = append(existing, *p) // so later candidates in THIS fill don't stack on it
-			return nil, err
+	}
-		}
+	created, err := s.store.CreatePlantings(ctx, batch)
-		stored.DerivedCount = derivedCount(stored.RadiusCM, spacing)
+	if err != nil {
-		created = append(created, *stored)
+		return nil, err
-		// Count what we just placed so later candidates in this same fill don't
+	}
-		// stack on top of it.
+	for i := range created {
-		existing = append(existing, *stored)
+		created[i].DerivedCount = derivedCount(created[i].RadiusCM, spacing)
 	}
 	return created, nil
 }
@@ -188,11 +210,14 @@ func (s *Service) FillNamedRegion(ctx context.Context, actorID, objectID int64,
 	if err != nil {
 		return nil, err
 	}
-	return s.FillRegion(ctx, actorID, objectID, region, plantID, spacingOverride)
+	return s.fillLoaded(ctx, actorID, o, 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.
+// Unlike FillRegion it does NOT require the object be plantable: an object toggled
+// non-plantable after it was planted must still be clearable (you can always
+// remove existing plops, only not add new ones).
 func (s *Service) ClearObject(ctx context.Context, actorID, objectID int64) (int, error) {
 	if _, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor); err != nil {
 		return 0, err

internal/service/ops_test.go

@@ -41,6 +41,25 @@ func TestNamedRegion(t *testing.T) {
 	if _, err := NamedRegion(o, "middle-ish"); !errors.Is(err, domain.ErrInvalidInput) {
 		t.Errorf("unknown region err = %v, want ErrInvalidInput", err)
 	}
+	if _, err := NamedRegion(o, ""); !errors.Is(err, domain.ErrInvalidInput) {
+		t.Errorf("empty region err = %v, want ErrInvalidInput", err)
+	}
+	if _, err := NamedRegion(nil, "all"); !errors.Is(err, domain.ErrInvalidInput) {
+		t.Errorf("nil object err = %v, want ErrInvalidInput", err)
+	}
+}
+
+func TestFillRegionCappedForHugeArea(t *testing.T) {
+	ctx := context.Background()
+	s := newTestService(t, openConfig())
+	owner := seedUser(t, s, "[email protected]")
+	g, _ := s.CreateGarden(ctx, owner, GardenInput{Name: "Huge", WidthCM: 8000, HeightCM: 8000})
+	bed := seedFillBed(t, s, owner, g.ID, 6000, 6000) // ~46k lattice points at radius 15 → over the cap
+	plant := seedOwnPlant(t, s, owner, 10)
+	region, _ := NamedRegion(bed, "all")
+	if _, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil); !errors.Is(err, domain.ErrInvalidInput) {
+		t.Errorf("oversized fill err = %v, want ErrInvalidInput (over maxFillPlops)", err)
+	}
 }
 
 func TestDefaultPlopRadius(t *testing.T) {

internal/store/plantings.go

@@ -132,6 +132,36 @@ func (d *DB) CreatePlanting(ctx context.Context, p *domain.Planting) (*domain.Pl
 	return created, nil
 }
 
+// CreatePlantings inserts many plops in a single transaction (one commit), for
+// bulk fills. Returns the stored rows in order. An empty input is a no-op.
+func (d *DB) CreatePlantings(ctx context.Context, plantings []*domain.Planting) ([]domain.Planting, error) {
+	if len(plantings) == 0 {
+		return []domain.Planting{}, nil
+	}
+	tx, err := d.sql.BeginTx(ctx, nil)
+	if err != nil {
+		return nil, fmt.Errorf("store: begin plantings tx: %w", err)
+	}
+	defer tx.Rollback() //nolint:errcheck // no-op after a successful commit
+
+	const stmt = `INSERT INTO plantings (object_id, plant_id, x_cm, y_cm, radius_cm, count, label, planted_at)
+		 VALUES (?, ?, ?, ?, ?, ?, ?, ?)
+		 RETURNING ` + plantingColumns
+	out := make([]domain.Planting, 0, len(plantings))
+	for _, p := range plantings {
+		created, err := scanPlanting(tx.QueryRowContext(ctx, stmt,
+			p.ObjectID, p.PlantID, p.XCM, p.YCM, p.RadiusCM, p.Count, p.Label, p.PlantedAt))
+		if err != nil {
+			return nil, fmt.Errorf("store: insert planting (batch): %w", err)
+		}
+		out = append(out, *created)
+	}
+	if err := tx.Commit(); err != nil {
+		return nil, fmt.Errorf("store: commit plantings: %w", err)
+	}
+	return out, nil
+}
+
 // UpdatePlanting applies a version-guarded update of all mutable columns (the
 // service merges partial patches first). Returns the updated row, or
 // (current row, ErrVersionConflict) / ErrNotFound — the same contract as the