diff --git a/internal/agent/runtime.go b/internal/agent/runtime.go index 2073737..1692869 100644 --- a/internal/agent/runtime.go +++ b/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 — " 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+" — ")) } diff --git a/internal/agent/tools.go b/internal/agent/tools.go index 6a09c03..1d907ac 100644 --- a/internal/agent/tools.go +++ b/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 "+ - "basil out of the corner\".", + "bed does. Needs the plop's id and version (describe_garden or list_plantings). Use "+ + "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. - Undo *int64 `json:"undoOf,omitempty"` + // UndoOf is the earlier entry this one reverted, when it is itself an undo. + 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 { diff --git a/internal/agent/tools_test.go b/internal/agent/tools_test.go index c1a0862..02b5803 100644 --- a/internal/agent/tools_test.go +++ b/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]) } diff --git a/internal/service/ops.go b/internal/service/ops.go index 53d2b93..f53da2c 100644 --- a/internal/service/ops.go +++ b/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 diff --git a/internal/service/ops_test.go b/internal/service/ops_test.go index a803ae8..2599277 100644 --- a/internal/service/ops_test.go +++ b/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) + } } diff --git a/internal/service/plantings.go b/internal/service/plantings.go index 4c7a01f..d8708d5 100644 --- a/internal/service/plantings.go +++ b/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 -// UTC day ClearObject and the fill path default to; RemovePlantingOn is the -// form for a caller that knows the gardener's local day. Both delegate to -// UpdatePlanting for the editor-role check, version guard and history record. -func (s *Service) RemovePlanting(ctx context.Context, actorID, plantingID, version int64) (*domain.Planting, error) { - return s.RemovePlantingOn(ctx, actorID, plantingID, version, nil) -} - -// 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) { +// ClearObject, used by the agent's remove_planting tool. removedAt (YYYY-MM-DD) +// is the day the caller knows it happened — the gardener's local day; nil +// stamps the service clock's UTC today, the same default ClearObject and the +// fill path use. Delegates to UpdatePlanting for the editor-role check, version +// guard and history record. +func (s *Service) RemovePlanting(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) }