Agent: add the corrective tools the toolbox was missing (#85 item 3) #122

Merged
steve merged 2 commits from feat/agent-corrective-tools into main 2026-07-23 01:41:38 +00:00
4 changed files with 249 additions and 0 deletions
+111
View File
@@ -64,6 +64,40 @@ func NewToolbox(svc *service.Service, actorID int64) *llm.Toolbox {
"(\"powdery mildew on the west bed\", \"first frost\"), not for descriptions of what a "+
"thing is. observedAt defaults to today; set it to backdate.",
a.addJournalEntry),
llm.DefineTool("read_journal",
"Read back the garden's grow journal — the observations add_journal_entry wrote. "+
"Narrow it with objectId (one bed), or a from/to date range (YYYY-MM-DD). Most "+
"recently observed first. Use this to answer \"what did I note about the west bed?\" "+
"or \"what happened last spring?\".",
a.readJournal),
llm.DefineTool("update_object",
"Change an existing object: resize it (widthCm/heightCm), rotate it (rotationDeg), "+
Review

update_object sets absolute dimensions; a model conflating 'make 60cm wider' with 'set widthCm=60' passes finalizeObject's [1cm,100m] check and silently shrinks the bed

correctness · flagged by 1 model

  • internal/agent/tools.go:74update_object description is a plausible-magic-number trap. update_object sets absolute dimensions while move_object is separate. A model conflating "make 60cm wider" with "set widthCm=60" gets a 60cm bed instead of a 560cm one, and finalizeObject only rejects dimensions outside [1cm, 100m], so 60cm passes silently. The description does warn with the correct procedure (read width, add 60, pass sum), so this is a prompt-design risk, not a code bug;…

🪰 Gadfly · advisory

⚪ **update_object sets absolute dimensions; a model conflating 'make 60cm wider' with 'set widthCm=60' passes finalizeObject's [1cm,100m] check and silently shrinks the bed** _correctness · flagged by 1 model_ - **`internal/agent/tools.go:74` — `update_object` description is a plausible-magic-number trap.** `update_object` sets *absolute* dimensions while `move_object` is separate. A model conflating "make 60cm wider" with "set widthCm=60" gets a 60cm bed instead of a 560cm one, and `finalizeObject` only rejects dimensions outside `[1cm, 100m]`, so 60cm passes silently. The description does warn with the correct procedure (read width, add 60, pass sum), so this is a prompt-design risk, not a code bug;… <sub>🪰 Gadfly · advisory</sub>
"rename it (name), or toggle whether it can hold plants (plantable). Only the fields "+
"you pass change. Needs the object's current version from describe_garden. Example: "+
"\"make that bed 60cm wider\" — read its widthCm from describe_garden, add 60, pass the "+
"sum.",
a.updateObject),
llm.DefineTool("delete_object",
"Delete an object from a garden entirely, along with its plantings. This is the "+
"counterpart to create_object — use it for \"remove the old grow bag\". Permanent (not "+
"the same as clearing a bed's plants); prefer clear_object when the bed itself stays.",
a.deleteObject),
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\".",
a.removePlanting),
llm.DefineTool("list_seed_lots",
"List the seed lots (purchases) the user has recorded — vendor, quantity, and what's "+
"left — optionally for one plant via plantId. This is the detail behind the \"seed "+
"remaining\" number find_plant reports.",
a.listSeedLots),
llm.DefineTool("record_seed_lot",
"Record a seed purchase for a plant the user owns, so pansy can track how much is left. "+
"Get the plantId from find_plant first. quantity + unit is what was bought (e.g. 2 "+
"\"packets\", or 500 \"seeds\"). Use for \"I bought two packets of Cherokee Purple\".",
a.recordSeedLot),
)
}
@@ -177,3 +211,80 @@ func (a *adapter) clearObject(ctx context.Context, args struct {
}
return map[string]int{"cleared": n}, nil
}
func (a *adapter) readJournal(ctx context.Context, args struct {
GardenID int64 `json:"gardenId" description:"garden whose journal to read"`
ObjectID *int64 `json:"objectId" description:"optional bed to narrow to; omit for the whole garden"`
From string `json:"from" description:"optional earliest observed date, YYYY-MM-DD"`
To string `json:"to" description:"optional latest observed date, YYYY-MM-DD"`
Offset int `json:"offset" description:"how many entries to skip; pass the count you've already seen to page when hasMore is true"`
}) (any, error) {
q := service.JournalQuery{ObjectID: args.ObjectID, Limit: 50, Offset: args.Offset}
Outdated
Review

🟠 read_journal exposes hasMore without offset parameter, making pagination impossible

correctness, maintainability · flagged by 3 models

  • internal/agent/tools.go:222 — The read_journal tool hardcodes Limit: 50 and returns hasMore to the LLM, but does not expose the offset parameter that the underlying service.JournalQuery supports. Because the tool adapter omits offset, an LLM that sees hasMore: true has no way to request the next page, making the pagination signal functionally useless. The tool should either accept an offset argument so the LLM can page, or not return hasMore at all.

🪰 Gadfly · advisory

🟠 **read_journal exposes hasMore without offset parameter, making pagination impossible** _correctness, maintainability · flagged by 3 models_ - `internal/agent/tools.go:222` — The `read_journal` tool hardcodes `Limit: 50` and returns `hasMore` to the LLM, but does not expose the `offset` parameter that the underlying `service.JournalQuery` supports. Because the tool adapter omits `offset`, an LLM that sees `hasMore: true` has no way to request the next page, making the pagination signal functionally useless. The tool should either accept an `offset` argument so the LLM can page, or not return `hasMore` at all. <sub>🪰 Gadfly · advisory</sub>
if args.From != "" {
q.From = &args.From
}
if args.To != "" {
q.To = &args.To
}
entries, hasMore, err := a.svc.ListJournal(ctx, a.actor, args.GardenID, q)
Review

🟠 read_journal returns hasMore but exposes no pagination (offset/cursor), so journals over 50 entries are only partially readable via the agent

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟠 **read_journal returns hasMore but exposes no pagination (offset/cursor), so journals over 50 entries are only partially readable via the agent** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
if err != nil {
return nil, err
}
// hasMore is actionable now: re-call with offset += len(entries) to page.
return map[string]any{"entries": entries, "hasMore": hasMore}, nil
}
func (a *adapter) updateObject(ctx context.Context, args struct {
ObjectID int64 `json:"objectId" description:"object to change"`
Version int64 `json:"version" description:"the object's current version (from describe_garden)"`
Name *string `json:"name" description:"optional new label"`
WidthCM *float64 `json:"widthCm" description:"optional new width in cm (a circle's diameter)"`
HeightCM *float64 `json:"heightCm" description:"optional new height in cm"`
RotationDeg *float64 `json:"rotationDeg" description:"optional new rotation in degrees"`
Plantable *bool `json:"plantable" description:"optional: whether the object can hold plants"`
}) (any, error) {
Review

🟡 update_object overlaps move_object — two partial-patch tools over the same UpdateObject service method

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **update_object overlaps move_object — two partial-patch tools over the same UpdateObject service method** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
return a.svc.UpdateObject(ctx, a.actor, args.ObjectID, service.ObjectPatch{
Name: args.Name, WidthCM: args.WidthCM, HeightCM: args.HeightCM,
RotationDeg: args.RotationDeg, Plantable: args.Plantable,
}, args.Version)
}
func (a *adapter) deleteObject(ctx context.Context, args struct {
ObjectID int64 `json:"objectId" description:"object to delete (with its plantings)"`
}) (any, error) {
if err := a.svc.DeleteObject(ctx, a.actor, args.ObjectID); err != nil {
return nil, err
}
return map[string]any{"deleted": args.ObjectID}, nil
}
func (a *adapter) removePlanting(ctx context.Context, args struct {
PlantingID int64 `json:"plantingId" description:"plop to remove (its id from describe_garden)"`
Version int64 `json:"version" description:"the plop's current version (from describe_garden)"`
}) (any, error) {
// Soft-remove via the service, so removed_at is stamped from the same
// (injectable) clock clear_object uses rather than the adapter's wall clock.
Outdated
Review

🟠 remove_planting stamps removed_at from wall-clock time.Now() instead of the service's injectable s.now(), diverging from the clear_object path it claims to mirror

correctness, error-handling, maintainability · flagged by 3 models

  • internal/agent/tools.go:266removePlanting computes today := time.Now().UTC().Format("2006-01-02") from the wall clock, but every other date-stamping path in the service uses the injectable clock (s.now().UTC().Format(dateLayout)): CreatePlanting at internal/service/plantings.go:104, ClearObjectPlantings's soft-remove path at internal/service/ops.go:413, and the fill path at internal/service/ops.go:238. now is an injectable func() time.Time on Service (`internal/s…

🪰 Gadfly · advisory

🟠 **remove_planting stamps removed_at from wall-clock time.Now() instead of the service's injectable s.now(), diverging from the clear_object path it claims to mirror** _correctness, error-handling, maintainability · flagged by 3 models_ - `internal/agent/tools.go:266` — `removePlanting` computes `today := time.Now().UTC().Format("2006-01-02")` from the **wall clock**, but every other date-stamping path in the service uses the injectable clock (`s.now().UTC().Format(dateLayout)`): `CreatePlanting` at `internal/service/plantings.go:104`, `ClearObjectPlantings`'s soft-remove path at `internal/service/ops.go:413`, and the fill path at `internal/service/ops.go:238`. `now` is an injectable `func() time.Time` on `Service` (`internal/s… <sub>🪰 Gadfly · advisory</sub>
return a.svc.RemovePlanting(ctx, a.actor, args.PlantingID, args.Version)
}
func (a *adapter) listSeedLots(ctx context.Context, args struct {
PlantID *int64 `json:"plantId" description:"optional: only lots for this plant"`
}) (any, error) {
return a.svc.ListSeedLots(ctx, a.actor, args.PlantID)
}
Review

🟡 list_seed_lots returns a bare array while read_journal wraps in {entries,hasMore}; inconsistent tool return shapes in the same PR

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **list_seed_lots returns a bare array while read_journal wraps in {entries,hasMore}; inconsistent tool return shapes in the same PR** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
func (a *adapter) recordSeedLot(ctx context.Context, args struct {
PlantID int64 `json:"plantId" description:"plant the seed is for (from find_plant); must be the user's own or a built-in"`
Quantity float64 `json:"quantity" description:"how much was bought, in the given unit"`
Unit string `json:"unit" description:"what quantity counts, e.g. packets | seeds | grams"`
Vendor string `json:"vendor" description:"optional vendor name"`
SourceURL string `json:"sourceUrl" description:"optional http(s) link to where it was bought"`
PackedForYear *int `json:"packedForYear" description:"optional 'packed for' year from the packet"`
Notes string `json:"notes" description:"optional free-text notes"`
}) (any, error) {
return a.svc.CreateSeedLot(ctx, a.actor, service.SeedLotInput{
PlantID: args.PlantID, Quantity: args.Quantity, Unit: args.Unit,
Vendor: args.Vendor, SourceURL: args.SourceURL,
PackedForYear: args.PackedForYear, Notes: args.Notes,
})
}
+121
View File
@@ -347,6 +347,127 @@ func TestJournalToolWritesADatedObservation(t *testing.T) {
}
}
// TestCorrectiveTools covers the #85 gaps: the agent can now read the journal it
// could only write, resize and delete an object it could only create and move,
// pull a single plop instead of clearing the whole bed, and record/read seed
// lots. Each is driven through the tool layer the way a model would run it.
func TestCorrectiveTools(t *testing.T) {
Review

TestCorrectiveTools bundles five unrelated tool checks into one fat test; a t.Fatalf in any step skips the rest

maintainability · flagged by 1 model

  • internal/agent/tools_test.go:354TestCorrectiveTools is one ~125-line function covering five unrelated tools. It's readable thanks to the call/describe helpers, and the per-step comments are good, but it conflates resize, single-plop-remove, journal read, seed-lot record/list, and delete into a single test. If one assertion fails early, the rest are skipped (t.Fatalf). Splitting into subtests (t.Run) would localize failures and keep each gap's coverage independent. Trivial, n…

🪰 Gadfly · advisory

⚪ **TestCorrectiveTools bundles five unrelated tool checks into one fat test; a t.Fatalf in any step skips the rest** _maintainability · flagged by 1 model_ - **`internal/agent/tools_test.go:354` — `TestCorrectiveTools` is one ~125-line function covering five unrelated tools.** It's readable thanks to the `call`/`describe` helpers, and the per-step comments are good, but it conflates resize, single-plop-remove, journal read, seed-lot record/list, and delete into a single test. If one assertion fails early, the rest are skipped (`t.Fatalf`). Splitting into subtests (`t.Run`) would localize failures and keep each gap's coverage independent. Trivial, n… <sub>🪰 Gadfly · advisory</sub>
ctx := context.Background()
svc, owner := newAgentTestService(t)
box := NewToolbox(svc, owner)
var gid int64 // set once the garden exists; the describe closure reads it.
call := func(name string, args any) llm.ToolResult {
t.Helper()
return box.Execute(ctx, llm.ToolCall{ID: "1", Name: name, Arguments: mustJSON(t, args)})
}
describe := func() service.DescribeResult {
t.Helper()
res := call("describe_garden", map[string]any{"gardenId": gid})
if res.IsError {
t.Fatalf("describe_garden: %s", res.Content)
}
var d service.DescribeResult
if err := json.Unmarshal([]byte(res.Content), &d); err != nil {
t.Fatalf("decode describe: %v", err)
}
return d
}
g, err := svc.CreateGarden(ctx, owner, service.GardenInput{Name: "Plot", WidthCM: 2000, HeightCM: 2000})
if err != nil {
t.Fatalf("garden: %v", err)
}
gid = g.ID
basil := mustPlant(t, svc, owner, "Basil", 25, "🌿")
bed, err := svc.CreateObject(ctx, owner, g.ID, service.ObjectInput{
Kind: domain.KindBed, Name: "Bed", XCM: 1000, YCM: 1000, WidthCM: 400, HeightCM: 400,
})
if err != nil {
t.Fatalf("bed: %v", err)
}
// update_object: "make that bed 100cm wider" — read the version, pass a new width.
d := describe()
if r := call("update_object", map[string]any{
"objectId": bed.ID, "version": d.Objects[0].Version, "widthCm": 500.0,
}); r.IsError {
t.Fatalf("update_object: %s", r.Content)
}
if w := describe().Objects[0].WidthCM; w != 500 {
t.Errorf("width = %v after update_object, want 500", w)
}
// place a plop, then remove_planting it by id+version — one plop, not the bed.
if r := call("place_planting", map[string]any{
"objectId": bed.ID, "plantId": basil.ID, "xCm": 0, "yCm": 0, "radiusCm": 30,
}); r.IsError {
t.Fatalf("place_planting: %s", r.Content)
}
d = describe()
if len(d.Objects[0].Plantings) != 1 {
t.Fatalf("want 1 plop before removal, got %d", len(d.Objects[0].Plantings))
}
plop := d.Objects[0].Plantings[0]
if r := call("remove_planting", map[string]any{"plantingId": plop.ID, "version": plop.Version}); r.IsError {
t.Fatalf("remove_planting: %s", r.Content)
}
if n := len(describe().Objects[0].Plantings); n != 0 {
t.Errorf("want 0 active plops after remove_planting, got %d", n)
}
// add then read the journal — the write/read asymmetry the issue flagged.
if r := call("add_journal_entry", map[string]any{
"gardenId": g.ID, "objectId": bed.ID, "body": "aphids", "observedAt": "2026-06-01",
}); r.IsError {
t.Fatalf("add_journal_entry: %s", r.Content)
}
res := call("read_journal", map[string]any{"gardenId": g.ID, "objectId": bed.ID})
if res.IsError {
t.Fatalf("read_journal: %s", res.Content)
}
var jr struct {
Entries []struct {
Body string `json:"body"`
} `json:"entries"`
}
if err := json.Unmarshal([]byte(res.Content), &jr); err != nil {
t.Fatalf("decode read_journal: %v (%s)", err, res.Content)
}
if len(jr.Entries) != 1 || jr.Entries[0].Body != "aphids" {
t.Errorf("read_journal = %+v, want the one aphids entry", jr.Entries)
}
// record then list a seed lot — the detail behind find_plant's "remaining".
if r := call("record_seed_lot", map[string]any{
"plantId": basil.ID, "quantity": 2.0, "unit": "packets", "vendor": "Johnny's",
}); r.IsError {
t.Fatalf("record_seed_lot: %s", r.Content)
}
res = call("list_seed_lots", map[string]any{"plantId": basil.ID})
if res.IsError {
t.Fatalf("list_seed_lots: %s", res.Content)
}
var lots []struct {
Quantity float64 `json:"quantity"`
Unit string `json:"unit"`
}
if err := json.Unmarshal([]byte(res.Content), &lots); err != nil {
t.Fatalf("decode list_seed_lots: %v (%s)", err, res.Content)
}
if len(lots) != 1 || lots[0].Quantity != 2 || lots[0].Unit != "packets" {
t.Errorf("list_seed_lots = %+v, want one lot of 2 packets", lots)
}
// delete_object: the counterpart to create_object.
if r := call("delete_object", map[string]any{"objectId": bed.ID}); r.IsError {
t.Fatalf("delete_object: %s", r.Content)
}
if n := len(describe().Objects); n != 0 {
t.Errorf("want 0 objects after delete_object, got %d", n)
}
}
// newAgentTestService spins up an in-memory pansy with one registered user.
func newAgentTestService(t *testing.T) (*service.Service, int64) {
t.Helper()
+6
View File
@@ -471,7 +471,11 @@ type DescribeObject struct {
}
// DescribePlanting is one plop with a rough compass location, for DescribeResult.
// ID + Version are included so an agent can address a single plop — remove it or
// move it — the same way DescribeObject.Version lets it edit an object.
type DescribePlanting struct {
ID int64 `json:"id"`
Version int64 `json:"version"`
PlantID int64 `json:"plantId"`
Plant string `json:"plant"`
Count int `json:"count"`
@@ -518,6 +522,8 @@ func (s *Service) DescribeGarden(ctx context.Context, actorID, gardenID int64) (
count = *pl.Count
}
do.Plantings = append(do.Plantings, DescribePlanting{
ID: pl.ID,
Version: pl.Version,
PlantID: pl.PlantID,
Plant: plantByID[pl.PlantID].Name,
Count: count,
+11
View File
@@ -178,6 +178,17 @@ func (s *Service) UpdatePlanting(ctx context.Context, actorID, plantingID int64,
return updated, nil
}
// RemovePlanting soft-removes a single plop — the one-plop counterpart to
// ClearObject, used by the agent's remove_planting tool. It stamps removed_at
// from the service clock (s.now()), same as ClearObject and the fill path, so the
// removal date can't diverge by which caller set it; then delegates 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) {
today := s.now().UTC().Format(dateLayout)
return s.UpdatePlanting(ctx, actorID, plantingID,
PlantingPatch{SetRemovedAt: true, RemovedAt: &today}, version)
}
// plantingEditSummary describes a plop edit for the history list. Soft-removal
// ("clear bed", harvested) is the one edit worth naming specifically — it reads
// as a removal to the person who did it, not as an edit.