Address Gadfly review on the agent plant tools

attachSeedRemaining summed Remaining across lots in different units and then
dropped only the unit LABEL, leaving a bare number that reads as a quantity and
isn't one — 50 seeds plus 50 grams reported as "100". Worse for a model than for
a person, since it has nothing else to go on. The total is now omitted with the
label, and SeedLots reports how many lots there are, so "several lots, no single
total" stays distinguishable from "no seed at all".

Also folded TestToolboxScenario's inline setup into the newAgentTestService
helper the new tests use, rather than leaving two copies of the same six lines.

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

internal/agent/tools_test.go

@@ -25,21 +25,8 @@ import (
 // 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)
+	svc, ownerID := newAgentTestService(t)
+	box := NewToolbox(svc, ownerID)
 
 	call := func(name string, args any) llm.ToolResult {
 		t.Helper()
@@ -49,13 +36,13 @@ func TestToolboxScenario(t *testing.T) {
 
 	// 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})
+	g, err := svc.CreateGarden(ctx, ownerID, service.GardenInput{Name: "Plot", WidthCM: 2000, HeightCM: 2000})
 	if err != nil {
 		t.Fatalf("garden: %v", err)
 	}
-	garlic := mustPlant(t, svc, owner.ID, "Garlic", 15, "🧄")
-	basil := mustPlant(t, svc, owner.ID, "Basil", 25, "🌿")
-	beans := mustPlant(t, svc, owner.ID, "Beans", 10, "🫘")
+	garlic := mustPlant(t, svc, ownerID, "Garlic", 15, "🧄")
+	basil := mustPlant(t, svc, ownerID, "Basil", 25, "🌿")
+	beans := mustPlant(t, svc, ownerID, "Beans", 10, "🫘")
 
 	// create_object → a 400×400 bed.
 	res := call("create_object", map[string]any{
@@ -120,7 +107,7 @@ func TestToolboxScenario(t *testing.T) {
 	if err != nil {
 		t.Fatalf("register viewer: %v", err)
 	}
-	if _, err := svc.AddShare(ctx, owner.ID, g.ID, "[email protected]", domain.RoleViewer); err != nil {
+	if _, err := svc.AddShare(ctx, ownerID, g.ID, "[email protected]", domain.RoleViewer); err != nil {
 		t.Fatalf("share: %v", err)
 	}
 	viewerBox := NewToolbox(svc, viewerUser.ID)

internal/service/plants.go

@@ -76,10 +76,14 @@ func (s *Service) ListPlants(ctx context.Context, actorID int64) ([]domain.Plant
 type PlantMatch struct {
 	domain.Plant
 	// SeedRemaining is the total left across the actor's lots of this plant, and
-	// SeedUnit the unit they're counted in — omitted when there are no lots, or
-	// when lots disagree about the unit and summing them would be a lie.
+	// SeedUnit the unit they're counted in. Both are omitted when there are no
+	// lots — and also when the lots disagree about the unit, because adding
+	// grams to packets produces a number that means nothing. SeedLots still
+	// reports how many there are, so "several lots, no single total" is
+	// distinguishable from "no seed at all".
 	SeedRemaining *float64 `json:"seedRemaining,omitempty"`
 	SeedUnit      string   `json:"seedUnit,omitempty"`
+	SeedLots      int      `json:"seedLots,omitempty"`
 }
 
 // maxPlantMatches caps FindPlants. A model given fifty candidates is not being
@@ -162,14 +166,22 @@ func (s *Service) attachSeedRemaining(ctx context.Context, actorID int64, matche
 		if len(ls) == 0 {
 			continue
 		}
-		total := 0.0
+		matches[i].SeedLots = len(ls)
 		unit := ls[0].Unit
+		mixed := false
+		total := 0.0
 		for _, l := range ls {
 			total += l.Remaining
 			if l.Unit != unit {
-				unit = "" // mixed units can't be summed honestly
+				mixed = true
 			}
 		}
+		if mixed {
+			// Dropping only the LABEL would leave a number that reads as a
+			// quantity and isn't one. Drop the total with it; SeedLots still says
+			// there is seed here, just not one figure for it.
+			continue
+		}
 		matches[i].SeedRemaining = &total
 		matches[i].SeedUnit = unit
 	}

internal/service/plants_test.go

@@ -338,3 +338,37 @@ func TestFindPlantsReportsSeedRemaining(t *testing.T) {
 		}
 	}
 }
+
+// TestFindPlantsOmitsATotalItCannotHonestlyGive — lots in different units can't
+// be summed. Dropping only the unit LABEL would leave a number that reads as a
+// quantity and isn't one; the count of lots still says there is seed here.
+func TestFindPlantsOmitsATotalItCannotHonestlyGive(t *testing.T) {
+	s := newTestService(t, openConfig())
+	owner := seedUser(t, s, "[email protected]")
+	plant := seedOwnPlant(t, s, owner, 15)
+	ctx := context.Background()
+
+	for _, u := range []string{domain.UnitSeeds, domain.UnitGrams} {
+		if _, err := s.CreateSeedLot(ctx, owner, SeedLotInput{PlantID: plant.ID, Quantity: 50, Unit: u}); err != nil {
+			t.Fatalf("CreateSeedLot(%s): %v", u, err)
+		}
+	}
+
+	got, err := s.FindPlants(ctx, owner, plant.Name)
+	if err != nil {
+		t.Fatalf("FindPlants: %v", err)
+	}
+	if len(got) == 0 {
+		t.Fatal("plant not found")
+	}
+	if got[0].SeedRemaining != nil {
+		t.Errorf("reported %v across mismatched units; that number means nothing", *got[0].SeedRemaining)
+	}
+	if got[0].SeedUnit != "" {
+		t.Errorf("seedUnit = %q, want empty", got[0].SeedUnit)
+	}
+	// But "several lots, no single total" must stay distinguishable from "none".
+	if got[0].SeedLots != 2 {
+		t.Errorf("seedLots = %d, want 2", got[0].SeedLots)
+	}
+}