Address #127 review: quote the garden name, validate rectangles, require plantId

- The plan-name line of the system prompt interpolates the garden's name
  with %q like the rest of the prompt: any editor can rename a garden, and a
  name with a newline in it must not read as an instruction.
- fill_region refuses an inverted rectangle with its corners named, and a
  rectangle that misses the bed (or only touches its edge) is an error from
  the service rather than a successful fill of nothing.
- remove_plantings requires plantId; omitted it would remove plant 0 and
  report success.
- historyEntry.Undo → UndoOf (it holds the reverted change set's id).
- remove_planting's description names list_plantings as an id source.
- RemovePlanting takes the removal date itself; the dateless wrapper had no
  callers left.

Co-Authored-By: Claude Fable 5 <[email protected]>

internal/agent/runtime.go

@@ -261,7 +261,7 @@ How to work:
 - fill_region in grid mode lays out individual plants at true spacing, which is what "so I can
   plant from it" means; clump mode is a quick sketch. For an area no compass name describes (a
   middle third, a strip along one edge) give fill_region a rectangle instead of placing plops by hand.
-- A garden named "%s — <year>" is this garden's plan for that year; copy_garden with that name
+- A garden named %s is this garden's plan for that year; copy_garden with that name
   makes one. Never use a different real garden as a scratch space.
 - When a tool refuses (for example, the user only has view access to this garden), explain what
   happened in plain words. Do not retry it.
@@ -280,5 +280,8 @@ How to behave:
   observation — not to narrate your own planting.
 - The gardener is watching the canvas. When you are done, say briefly what you changed and where
   to look; if you changed nothing, say that too.`,
-		g.Name, g.ID, size, today, units, g.Name)
+		// %q throughout for the garden's name: any editor can rename a garden, and
+		// a name is data, not prompt — quoting keeps a newline or a stray quote
+		// in it from reading as a new instruction.
+		g.Name, g.ID, size, today, units, fmt.Sprintf("%q", g.Name+" — <year>"))
 }

internal/agent/tools.go

@@ -72,8 +72,8 @@ func NewToolbox(svc *service.Service, actorID int64, today string) *llm.Toolbox
 		llm.DefineTool("remove_planting",
 			"Remove ONE plop from a bed, leaving the rest — the single-plant answer to clear_object's "+
 				"all-or-nothing. Soft-removes it (kept for planting history, undoable), like clearing a "+
-				"bed does. Needs the plop's id and version from describe_garden. Use for \"pull the "+
+				"bed does. Needs the plop's id and version (describe_garden or list_plantings). Use "+
-				"basil out of the corner\".",
+				"for \"pull the basil out of the corner\".",
 			a.removePlanting),
 		llm.DefineTool("remove_plantings",
 			"Remove every plop of ONE plant from an object, leaving the other plants in it — \"take the "+
@@ -261,6 +261,9 @@ func (a *adapter) fillRegion(ctx context.Context, args struct {
 	}
 	switch {
 	case given == 4 && strings.TrimSpace(args.Region) == "":
+		if !(*args.X0CM < *args.X1CM && *args.Y0CM < *args.Y1CM) {
+			return nil, fmt.Errorf("%w: x0Cm must be west of x1Cm and y0Cm north of y1Cm (-y is north)", domain.ErrInvalidInput)
+		}
 		spec.Region = service.Region{MinX: *args.X0CM, MinY: *args.Y0CM, MaxX: *args.X1CM, MaxY: *args.Y1CM}
 	case given == 0 && strings.TrimSpace(args.Region) != "":
 		// the named region
@@ -352,6 +355,10 @@ func (a *adapter) removePlantings(ctx context.Context, args struct {
 	ObjectID int64 `json:"objectId" description:"object to remove the plant from"`
 	PlantID  int64 `json:"plantId" description:"the plant to remove every plop of (from describe_garden)"`
 }) (any, error) {
+	if args.PlantID == 0 {
+		// Left out, it would "remove" plant 0 — nothing — and report success.
+		return nil, fmt.Errorf("%w: plantId is required — say which plant to remove, or use clear_object for all of them", domain.ErrInvalidInput)
+	}
 	n, err := a.svc.ClearPlantings(ctx, a.actor, args.ObjectID,
 		service.ClearOptions{PlantID: &args.PlantID, RemovedAt: a.day("")})
 	if err != nil {
@@ -392,8 +399,8 @@ type historyEntry struct {
 	Summary string `json:"summary"`
 	Changes string `json:"changes"`
 	Undone  bool   `json:"undone,omitempty"`
-	// Undo is set when this entry is itself an undo of an earlier one.
+	// UndoOf is the earlier entry this one reverted, when it is itself an undo.
-	Undo *int64 `json:"undoOf,omitempty"`
+	UndoOf *int64 `json:"undoOf,omitempty"`
 }
 
 func (a *adapter) readHistory(ctx context.Context, args struct {
@@ -414,7 +421,7 @@ func (a *adapter) readHistory(ctx context.Context, args struct {
 		entries = append(entries, historyEntry{
 			ID: cs.ID, When: cs.CreatedAt, Source: cs.Source, Who: cs.ActorName,
 			Summary: cs.Summary, Changes: describeCounts(cs.Counts),
-			Undone: cs.RevertedByID != nil, Undo: cs.RevertsID,
+			Undone: cs.RevertedByID != nil, UndoOf: cs.RevertsID,
 		})
 	}
 	return map[string]any{"entries": entries, "hasMore": hasMore}, nil
@@ -467,7 +474,7 @@ func (a *adapter) removePlanting(ctx context.Context, args struct {
 }) (any, error) {
 	// Soft-remove via the service, dated the gardener's local day like every
 	// other tool here (the service clock's UTC day when that isn't known).
-	return a.svc.RemovePlantingOn(ctx, a.actor, args.PlantingID, args.Version, a.day(""))
+	return a.svc.RemovePlanting(ctx, a.actor, args.PlantingID, args.Version, a.day(""))
 }
 
 func (a *adapter) listSeedLots(ctx context.Context, args struct {

internal/agent/tools_test.go

@@ -555,6 +555,15 @@ func TestToolsFromTheLiveSweep(t *testing.T) {
 	if r := call("fill_region", map[string]any{"objectId": bed.ID, "plantId": beet.ID}); !r.IsError {
 		t.Error("fill_region with nowhere to fill succeeded")
 	}
+	// An inverted rectangle is refused with its corners named, before the service
+	// sees it — the mistake a model makes is swapping which way is north.
+	if r := call("fill_region", map[string]any{"objectId": bed.ID, "plantId": beet.ID, "x0Cm": 40.0, "y0Cm": -60.0, "x1Cm": -40.0, "y1Cm": 60.0}); !r.IsError || !strings.Contains(r.Content, "west of") {
+		t.Errorf("inverted rectangle: %+v, want a refusal naming the corners", r)
+	}
+	// remove_plantings without a plant would "remove" plant 0 — nothing.
+	if r := call("remove_plantings", map[string]any{"objectId": bed.ID}); !r.IsError || !strings.Contains(r.Content, "plantId") {
+		t.Errorf("remove_plantings with no plant: %+v, want a refusal asking which plant", r)
+	}
 	// place_planting without a radius → one plant at half the spacing; two garlic
 	// cloves along the north edge, dated today.
 	ok("place_planting", map[string]any{"objectId": bed.ID, "plantId": garlic.ID, "xCm": -100, "yCm": -50})
@@ -666,7 +675,7 @@ func TestToolsFromTheLiveSweep(t *testing.T) {
 		t.Fatalf("undo: err=%v conflicts=%+v", err, conflicts)
 	}
 	decode(ok("read_history", map[string]any{"gardenId": g.ID, "limit": 3}), &hist)
-	if hist.Entries[0].Undo == nil || !hist.Entries[2].Undone {
+	if hist.Entries[0].UndoOf == nil || !hist.Entries[2].Undone {
 		t.Errorf("after an undo: newest = %+v, undone = %+v; want the revert to point at the removal, and the removal marked undone", hist.Entries[0], hist.Entries[2])
 	}
 

internal/service/ops.go

@@ -276,6 +276,14 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde
 	}
 
 	region = region.clampTo(o.WidthCM/2, o.HeightCM/2)
+	if region.MaxX <= region.MinX || region.MaxY <= region.MinY {
+		// An explicit rectangle that misses the object, or only touches its edge.
+		// Planting nothing and reporting success would read as "done" to a caller
+		// that aimed at the wrong coordinates (typically the agent mixing up the
+		// garden frame and the object's local one) — and a rectangle clamped to a
+		// line would get hexCenters' one-plop-in-the-middle rule, on the edge.
+		return nil, fmt.Errorf("%w: the region lies outside the object", domain.ErrInvalidInput)
+	}
 	centers, total := hexCenters(region, radius, edgeInset(radius, spacing, layout), maxFillPlops)
 	if total > maxFillPlops {
 		return nil, domain.ErrInvalidInput // region too large for this spacing; ask for less

internal/service/ops_test.go

@@ -797,4 +797,15 @@ func TestFillByRectangleAttributesSeed(t *testing.T) {
 			t.Errorf("empty rectangle %+v: err = %v, want ErrInvalidInput", r, err)
 		}
 	}
+	// A rectangle that misses the bed (it is 240 wide, so ±120) — or only
+	// touches its edge — is an error, not a successful fill of nothing.
+	for _, r := range []Region{{MinX: 200, MinY: -10, MaxX: 300, MaxY: 10}, {MinX: 120, MinY: -10, MaxX: 200, MaxY: 10}} {
+		if _, err := s.Fill(ctx, owner, bed.ID, FillSpec{Region: r, PlantID: garlic.ID}); !errors.Is(err, domain.ErrInvalidInput) {
+			t.Errorf("off-bed rectangle %+v: err = %v, want ErrInvalidInput", r, err)
+		}
+	}
+	// Partly outside is fine: the part inside gets planted.
+	if created, err := s.Fill(ctx, owner, bed.ID, FillSpec{Region: Region{MinX: 80, MinY: -10, MaxX: 300, MaxY: 10}, PlantID: garlic.ID}); err != nil || len(created) == 0 {
+		t.Errorf("overhanging rectangle: %d plops, %v; want some", len(created), err)
+	}
 }

internal/service/plantings.go

@@ -186,18 +186,12 @@ func (s *Service) UpdatePlanting(ctx context.Context, actorID, plantingID int64,
 }
 
 // RemovePlanting soft-removes a single plop — the one-plop counterpart to
-// ClearObject. It stamps removed_at from the service clock (s.now()), the same
+// ClearObject, used by the agent's remove_planting tool. removedAt (YYYY-MM-DD)
-// UTC day ClearObject and the fill path default to; RemovePlantingOn is the
+// is the day the caller knows it happened — the gardener's local day; nil
-// form for a caller that knows the gardener's local day. Both delegate to
+// stamps the service clock's UTC today, the same default ClearObject and the
-// UpdatePlanting for the editor-role check, version guard and history record.
+// fill path use. Delegates to UpdatePlanting for the editor-role check, version
-func (s *Service) RemovePlanting(ctx context.Context, actorID, plantingID, version int64) (*domain.Planting, error) {
+// guard and history record.
-	return s.RemovePlantingOn(ctx, actorID, plantingID, version, nil)
+func (s *Service) RemovePlanting(ctx context.Context, actorID, plantingID, version int64, removedAt *string) (*domain.Planting, error) {
-}
-
-// RemovePlantingOn is RemovePlanting with the removal date supplied (YYYY-MM-DD)
-// by a caller that knows the person's local day. nil keeps the service clock's
-// UTC today.
-func (s *Service) RemovePlantingOn(ctx context.Context, actorID, plantingID, version int64, removedAt *string) (*domain.Planting, error) {
 	if !validDatePtr(removedAt) {
 		return nil, fmt.Errorf("%w: removedAt must be a YYYY-MM-DD date", domain.ErrInvalidInput)
 	}