Agent: add the corrective tools the toolbox was missing (#85 item 3) #122
@@ -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), "+
|
||||
|
|
||||
"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}
|
||||
|
gitea-actions
commented
🟠 read_journal exposes hasMore without offset parameter, making pagination impossible correctness, maintainability · flagged by 3 models
🪰 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)
|
||||
|
gitea-actions
commented
🟠 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) {
|
||||
|
gitea-actions
commented
🟡 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.
|
||||
|
gitea-actions
commented
🟠 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
🪰 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)
|
||||
}
|
||||
|
gitea-actions
commented
🟡 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,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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) {
|
||||
|
gitea-actions
commented
⚪ TestCorrectiveTools bundles five unrelated tool checks into one fat test; a t.Fatalf in any step skips the rest maintainability · flagged by 1 model
🪰 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()
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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.
|
||||
|
||||
⚪ 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_objectdescription is a plausible-magic-number trap.update_objectsets absolute dimensions whilemove_objectis separate. A model conflating "make 60cm wider" with "set widthCm=60" gets a 60cm bed instead of a 560cm one, andfinalizeObjectonly 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