diff --git a/CLAUDE.md b/CLAUDE.md index b4a2fa9..c8093ae 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -192,6 +192,13 @@ Conventions that follow from it: the argument is what makes the rule visible in the schema, the prompt is what makes the model ask. Neither is a guarantee, and a new outward-facing tool gets the same pair. +- **A turn that changed nothing cannot say it did.** `honestReply` in + `runtime.go` appends a correction when the reply claims a change ("Done — + I've deleted…") and no non-read-only tool call succeeded in the run. Live, + glm-5.2 did this twice in one session (a journal entry, then a seed lot: + "Done", nothing deleted). The prompt rule stays; the guard is for when the + model ignores it. `readOnlyTools` must list every tool that changes nothing + — a new read-only tool left out of it makes a turn look like it acted. - **Garden notes are the assistant's memory.** `systemPrompt` quotes `Garden.Notes` (owner-written, `%q`) as standing context, and `update_garden` is how the model adds "we're in zone 6a" to them. Notes are replaced whole, diff --git a/internal/agent/runtime.go b/internal/agent/runtime.go index 09637ee..4444584 100644 --- a/internal/agent/runtime.go +++ b/internal/agent/runtime.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "log/slog" + "regexp" "strings" "time" @@ -168,6 +169,12 @@ func (r *Runner) Run(ctx context.Context, actorID, gardenID int64, message, toda if result != nil { turn.Reply = result.Output turn.Steps = len(result.Steps) + if corrected := honestReply(turn.Reply, result, tools); corrected != turn.Reply { + // The steps are logged so the mechanism can be read off the log + // next time — which tool it tried, what came back, what it said. + slog.Warn("agent: reply claimed a change no tool made", "run", runID, "garden", gardenID, "steps", describeSteps(result)) + turn.Reply = corrected + } } if turn.Reply == "" { turn.Reply = fallbackReply(turn) @@ -175,6 +182,92 @@ func (r *Runner) Run(ctx context.Context, actorID, gardenID int64, message, toda return turn, nil } +// readOnlyTools are the tools whose success changes nothing — a turn made of +// these alone has not done anything, whatever its reply says. +var readOnlyTools = map[string]bool{ + "list_gardens": true, "describe_garden": true, "list_years": true, "list_plantings": true, + "find_plant": true, "read_journal": true, "read_history": true, "list_seed_lots": true, + "list_shares": true, +} + +// selfReportingTools succeed without necessarily changing anything — +// public_link with action=get reads, and undo_change with nothing left to +// revert reverts nothing — so their results don't count; the adapter says +// whether they changed something (adapter.changed). +var selfReportingTools = map[string]bool{"public_link": true, "undo_change": true} + +// changeVerbs are the past participles a claim of change is made of. One +// list, used by both shapes the claim takes. +const changeVerbs = `deleted|removed|updated|changed|added|saved|moved|planted|filled|cleared|corrected|recorded|marked|shared|renamed|reverted|undone|set|put|pulled|replaced|swapped|rotated|rewrote|rewritten|edited|created|started|attached|restored|made` + +// changeClaim matches a reply that reports a change as made: a "Done"/"Fixed"/ +// "Undone" opener, or a first-person past-tense claim ("I've deleted", "I +// moved"). A question or an offer ("want me to delete it?", "I'll remove it") +// does not match — only a claim of something already done. The opener list is +// short on purpose: "Updated totals:" opening a read-only answer must not +// trip it, and those replies say "I've …" when they mean a change. +var changeClaim = regexp.MustCompile(`(?i)(?:^\s*(?:done|fixed|undone)\b|\bI(?:'ve| have)? (?:just |now |also |already )?(?:` + changeVerbs + `)\b)`) + +// unbackedClaim is what the person reads under a claim no tool backs up. +const unbackedClaim = "\n\n_Correction: nothing actually changed in this turn — no tool call that changes anything succeeded. Ask again and I'll do it properly._" + +// describeSteps summarizes a run for a log line: per step, the tools it +// called (with ! on a failure) and the start of what the model said. +func describeSteps(r *agent.Result) string { + parts := make([]string, 0, len(r.Steps)) + for _, st := range r.Steps { + var b strings.Builder + fmt.Fprintf(&b, "%d:", st.Index) + for _, res := range st.Results { + b.WriteString(" " + res.Name) + if res.IsError { + b.WriteString("!") + } + } + if st.Response != nil { + if text := strings.Join(strings.Fields(st.Response.Text()), " "); text != "" { + // Cut on a rune boundary, like turnSummary: a byte slice can + // split a multibyte character and log invalid UTF-8. + if runes := []rune(text); len(runes) > 80 { + text = string(runes[:80]) + "…" + } + fmt.Fprintf(&b, " %q", text) + } + } + parts = append(parts, b.String()) + } + return strings.Join(parts, " | ") +} + +// acted reports whether the run changed anything: a successful call to a tool +// that is neither read-only nor self-reporting, or a self-reporting tool that +// told the adapter it changed something. +func acted(r *agent.Result, tools *adapter) bool { + for _, st := range r.Steps { + for _, res := range st.Results { + if !res.IsError && !readOnlyTools[res.Name] && !selfReportingTools[res.Name] { + return true + } + } + } + return tools != nil && tools.didChange() +} + +// honestReply appends a correction to a reply that claims a change when no +// tool call in the run made one. The prompt already forbids this, and the +// live model did it anyway: asked to delete a journal entry it answered +// "Done — I've deleted it" having deleted nothing, and the entry was found +// still there a turn later. The person should hear that from the app, not +// discover it. A reply that claims nothing, or a run in which some change +// succeeded, passes through unchanged — this cannot tell a true claim from a +// false one once anything at all was done, so it only speaks when nothing was. +func honestReply(reply string, r *agent.Result, tools *adapter) string { + if r == nil || acted(r, tools) || !changeClaim.MatchString(reply) { + return reply + } + return reply + unbackedClaim +} + // isLoopLimit reports whether an error is one of majordomo's loop guards firing // rather than a genuine failure. Those runs have a partial result worth keeping. func isLoopLimit(err error) bool { @@ -318,7 +411,10 @@ How to work: How to behave: - Only claim what a tool actually did. If a tool failed, or there is no tool for what was asked, say so plainly — never describe a change you did not make, and never say something is undone - unless undo_change did it. + unless undo_change did it. A tool result that is an error means the thing did not happen: say + it failed and why, and what you will try instead. Before you say you deleted, changed or added + something, there must be a successful tool result for it in THIS turn — an earlier turn does + not count, and neither does meaning to. - Every reply of yours that changed the garden has an "Undo this" button under it, and the History panel can revert any change; mention that when it helps. - When a request could mean materially different things — "swap the cucumbers and the melons" diff --git a/internal/agent/runtime_test.go b/internal/agent/runtime_test.go index 5bbff6a..3470142 100644 --- a/internal/agent/runtime_test.go +++ b/internal/agent/runtime_test.go @@ -591,3 +591,81 @@ func TestSystemPromptCarriesTheGardenersNotes(t *testing.T) { t.Error("a garden without notes doesn't say so") } } + +// TestAClaimedChangeNoToolMadeIsCorrected — the live model, asked to delete a +// journal entry, answered "Done — I've deleted it" having called nothing, and +// the entry was found still there a turn later. The prompt forbids that; the +// run now catches it too: a reply that claims a change, in a turn where no +// tool call changed anything, gets a correction the person can read. +func TestAClaimedChangeNoToolMadeIsCorrected(t *testing.T) { + ctx := context.Background() + svc, owner := newAgentTestService(t) + g, err := svc.CreateGarden(ctx, owner, service.GardenInput{Name: "Plot", WidthCM: 2000, HeightCM: 2000}) + if err != nil { + t.Fatalf("garden: %v", err) + } + entry, err := svc.CreateJournalEntry(ctx, owner, g.ID, service.JournalInput{Body: "Aphids on the cucumbers."}) + if err != nil { + t.Fatalf("journal: %v", err) + } + + run := func(steps ...fake.Step) string { + t.Helper() + turn, err := scriptedRunner(t, svc, steps...).Run(ctx, owner, g.ID, "delete that note", "", nil, nil) + if err != nil { + t.Fatalf("Run: %v", err) + } + return turn.Reply + } + corrected := func(reply string) bool { return strings.Contains(reply, "nothing actually changed") } + + // No tool at all, a confident claim: corrected. + if r := run(fake.Reply("Done — I've deleted the journal entry about the aphids.")); !corrected(r) { + t.Errorf("a claim with no tool call passed uncorrected: %q", r) + } + // Only reads, then a claim: corrected. + if r := run(toolCall("read_journal", map[string]any{"gardenId": g.ID}), fake.Reply("I've deleted it.")); !corrected(r) { + t.Errorf("a claim over read-only calls passed uncorrected: %q", r) + } + // A tool that FAILED, then a claim: corrected — and the failure names the + // entry and where the ids come from, so a model that reads it has no + // excuse to guess again. + box := NewToolbox(svc, owner, "") + res := box.Execute(ctx, llm.ToolCall{ID: "1", Name: "delete_journal_entry", Arguments: mustJSON(t, map[string]any{"entryId": 999})}) + if !res.IsError || !strings.Contains(res.Content, "read_journal") || !strings.Contains(res.Content, "nothing was changed") { + t.Errorf("deleting a missing entry = %q, want a refusal naming read_journal and saying nothing changed", res.Content) + } + if r := run(toolCall("delete_journal_entry", map[string]any{"entryId": 999}), fake.Reply("Done — it's gone.")); !corrected(r) { + t.Errorf("a claim over a failed call passed uncorrected: %q", r) + } + // No claim: nothing appended, whatever the tools did. + if r := run(fake.Reply("That note is still there — want me to delete it?")); corrected(r) { + t.Errorf("an offer was corrected as if it were a claim: %q", r) + } + // Reading the public link is not a change, whatever its tool name. + if r := run(toolCall("public_link", map[string]any{"gardenId": g.ID, "action": "get"}), fake.Reply("Done — I've turned the public link on.")); !corrected(r) { + t.Errorf("a claim over public_link get passed uncorrected: %q", r) + } + // An undo that had nothing to revert is not a change either. + history, _, _ := svc.GardenHistory(ctx, owner, g.ID, 1, 0) + if len(history) > 0 { + if _, _, err := svc.RevertChangeSet(ctx, owner, history[0].ID, domain.SourceUI); err != nil { + t.Fatalf("pre-revert: %v", err) + } + if r := run(toolCall("undo_change", map[string]any{"changeSetId": history[0].ID}), fake.Reply("Undone — it's back the way it was.")); !corrected(r) { + t.Errorf("a claim over an undo that reverted nothing passed uncorrected: %q", r) + } + } + // A real deletion: the claim stands. + r := run(toolCall("delete_journal_entry", map[string]any{"entryId": entry.ID}), fake.Reply("Done — I've deleted the journal entry.")) + if corrected(r) { + t.Errorf("a true claim was corrected: %q", r) + } + // A read-only answer that happens to open with a participle is left alone. + if r := run(toolCall("describe_garden", map[string]any{"gardenId": g.ID}), fake.Reply("Updated totals: 0 plantings. Nothing is in the ground.")); corrected(r) { + t.Errorf("an informational reply was corrected: %q", r) + } + if _, _, err := svc.ListJournal(ctx, owner, g.ID, service.JournalQuery{}); err != nil { + t.Fatalf("journal after: %v", err) + } +} diff --git a/internal/agent/tools.go b/internal/agent/tools.go index 2dfc9ce..c232d4a 100644 --- a/internal/agent/tools.go +++ b/internal/agent/tools.go @@ -157,7 +157,8 @@ func newToolbox(svc *service.Service, actorID int64, today string) (*llm.Toolbox a.updateJournalEntry), llm.DefineTool("delete_journal_entry", "Delete a journal entry, by its id from read_journal. This is permanent — the journal is "+ - "not in the undo history — so delete only the entry the user pointed at.", + "not in the undo history — so delete only the entry the user pointed at. Get the id "+ + "from read_journal in this turn: ids from earlier turns are not in front of you.", a.deleteJournalEntry), llm.DefineTool("read_history", "Read the garden's change history: every change anyone made — by hand in the editor, or "+ @@ -227,7 +228,8 @@ func newToolbox(svc *service.Service, actorID int64, today string) (*llm.Toolbox llm.DefineTool("delete_seed_lot", "Delete a seed lot the user recorded, by its id from list_seed_lots. Plantings attributed "+ "to it stay in the garden, just no longer linked to a purchase. Permanent — seed lots "+ - "are not in the undo history — so delete only the lot the user pointed at.", + "are not in the undo history — so delete only the lot the user pointed at. Get the id "+ + "from list_seed_lots in this turn: ids from earlier turns are not in front of you.", a.deleteSeedLot), llm.DefineTool("delete_plant", "Delete a plant from the user's own catalog — a duplicate or a mistake. Refused while "+ @@ -275,6 +277,10 @@ type adapter struct { today string mu sync.Mutex + // changed is set by the tools whose success does not itself mean a change + // — public_link (get reads) and undo_change (nothing left to revert) — when + // they did change something, for Run's honesty check. + changed bool // reverts is every change set undo_change produced this turn. A revert is // its own change set (it points back at the one it undid, and the target is // marked undone), so it never joins the turn's scope — which leaves a turn @@ -283,6 +289,20 @@ type adapter struct { reverts []int64 } +// noteChange records that a self-reporting tool changed something. +func (a *adapter) noteChange() { + a.mu.Lock() + a.changed = true + a.mu.Unlock() +} + +// didChange reports whether a self-reporting tool changed something this turn. +func (a *adapter) didChange() bool { + a.mu.Lock() + defer a.mu.Unlock() + return a.changed +} + // lastRevert is the newest change set undo_change produced this turn, if any. func (a *adapter) lastRevert() *int64 { a.mu.Lock() @@ -391,8 +411,9 @@ func (a *adapter) moveObject(ctx context.Context, args struct { YCM float64 `json:"yCm" description:"new center y in garden cm"` Version int64 `json:"version" description:"the object's current version (from describe_garden)"` }) (any, error) { - return a.svc.UpdateObject(ctx, a.actor, args.ObjectID, + o, err := a.svc.UpdateObject(ctx, a.actor, args.ObjectID, service.ObjectPatch{XCM: &args.XCM, YCM: &args.YCM}, args.Version) + return o, whenMissing(err, "object", args.ObjectID, "describe_garden") } func (a *adapter) placePlanting(ctx context.Context, args struct { @@ -468,8 +489,9 @@ func (a *adapter) movePlanting(ctx context.Context, args struct { YCM float64 `json:"yCm" description:"new center y in the destination object's local frame (cm)"` ToObjectID *int64 `json:"toObjectId" description:"optional: another plantable object in the same garden to move it into; omit to move within its current object"` }) (any, error) { - return a.svc.MovePlanting(ctx, a.actor, args.PlantingID, + pl, err := a.svc.MovePlanting(ctx, a.actor, args.PlantingID, service.MoveInput{ToObjectID: args.ToObjectID, XCM: args.XCM, YCM: args.YCM}, args.Version) + return pl, whenMissing(err, "planting", args.PlantingID, "list_plantings") } func (a *adapter) findPlant(ctx context.Context, args struct { @@ -507,11 +529,12 @@ func (a *adapter) updatePlant(ctx context.Context, args struct { Vendor *string `json:"vendor" description:"optional vendor name"` Notes *string `json:"notes" description:"optional free-text notes"` }) (any, error) { - return a.svc.UpdatePlant(ctx, a.actor, args.PlantID, service.PlantPatch{ + p, err := a.svc.UpdatePlant(ctx, a.actor, args.PlantID, service.PlantPatch{ Name: args.Name, Category: args.Category, SpacingCM: args.SpacingCM, Color: args.Color, SetDays: args.DaysToMaturity != nil, DaysToMaturity: args.DaysToMaturity, SourceURL: args.SourceURL, Vendor: args.Vendor, Notes: args.Notes, }, args.Version) + return p, whenMissing(err, "plant of the user's own", args.PlantID, "find_plant") } func (a *adapter) addJournalEntry(ctx context.Context, args struct { @@ -539,7 +562,7 @@ func (a *adapter) clearObject(ctx context.Context, args struct { } n, err := a.svc.ClearPlantings(ctx, a.actor, args.ObjectID, service.ClearOptions{RemovedAt: on}) if err != nil { - return nil, err + return nil, whenMissing(err, "object", args.ObjectID, "describe_garden") } return map[string]int{"cleared": n}, nil } @@ -560,7 +583,7 @@ func (a *adapter) removePlantings(ctx context.Context, args struct { n, err := a.svc.ClearPlantings(ctx, a.actor, args.ObjectID, service.ClearOptions{PlantID: &args.PlantID, RemovedAt: on}) if err != nil { - return nil, err + return nil, whenMissing(err, "object", args.ObjectID, "describe_garden") } return map[string]int{"removed": n}, nil } @@ -608,7 +631,19 @@ func (a *adapter) updatePlanting(ctx context.Context, args struct { case args.SeedLotID != nil: patch.SetSeedLotID, patch.SeedLotID = true, args.SeedLotID } - return a.svc.UpdatePlanting(ctx, a.actor, args.PlantingID, patch, args.Version) + pl, err := a.svc.UpdatePlanting(ctx, a.actor, args.PlantingID, patch, args.Version) + return pl, whenMissing(err, "planting", args.PlantingID, "list_plantings") +} + +// whenMissing turns the store's bare "not found" into a message that says what +// was not found and where the ids come from, so the model re-reads instead of +// guessing at another id — or, as the live one did, reporting success over +// the error. +func whenMissing(err error, what string, id int64, from string) error { + if errors.Is(err, domain.ErrNotFound) { + return fmt.Errorf("%w: no %s with id %d that you can act on — %s lists the ids; nothing was changed", domain.ErrNotFound, what, id, from) + } + return err } // parseDay checks a date the model typed, so a malformed one fails with a @@ -660,15 +695,16 @@ func (a *adapter) updateJournalEntry(ctx context.Context, args struct { } observed = &on } - return a.svc.UpdateJournalEntry(ctx, a.actor, args.EntryID, + e, err := a.svc.UpdateJournalEntry(ctx, a.actor, args.EntryID, service.JournalPatch{Body: args.Body, ObservedAt: observed}, args.Version) + return e, whenMissing(err, "journal entry", args.EntryID, "read_journal") } func (a *adapter) deleteJournalEntry(ctx context.Context, args struct { EntryID int64 `json:"entryId" description:"journal entry to delete (its id from read_journal)"` }) (any, error) { if err := a.svc.DeleteJournalEntry(ctx, a.actor, args.EntryID); err != nil { - return nil, err + return nil, whenMissing(err, "journal entry", args.EntryID, "read_journal") } return map[string]any{"deleted": args.EntryID}, nil } @@ -694,7 +730,7 @@ func (a *adapter) undoChange(ctx context.Context, args struct { } cs, conflicts, err := a.svc.RevertChangeSet(ctx, a.actor, args.ChangeSetID, domain.SourceAgent) if err != nil { - return nil, err + return nil, whenMissing(err, "change", args.ChangeSetID, "read_history") } res := undoResult{UndoneID: args.ChangeSetID, Conflicts: conflicts} if cs == nil { @@ -711,6 +747,7 @@ func (a *adapter) undoChange(ctx context.Context, args struct { } a.mu.Lock() a.reverts = append(a.reverts, cs.ID) + a.changed = true a.mu.Unlock() return res, nil } @@ -779,17 +816,18 @@ func (a *adapter) updateObject(ctx context.Context, args struct { RotationDeg *float64 `json:"rotationDeg" description:"optional new rotation in degrees"` Plantable *bool `json:"plantable" description:"optional: whether the object can hold plants"` }) (any, error) { - return a.svc.UpdateObject(ctx, a.actor, args.ObjectID, service.ObjectPatch{ + o, err := 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) + return o, whenMissing(err, "object", args.ObjectID, "describe_garden") } 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 nil, whenMissing(err, "object", args.ObjectID, "describe_garden") } return map[string]any{"deleted": args.ObjectID}, nil } @@ -806,7 +844,8 @@ func (a *adapter) removePlanting(ctx context.Context, args struct { if err != nil { return nil, err } - return a.svc.RemovePlanting(ctx, a.actor, args.PlantingID, args.Version, on) + pl, err := a.svc.RemovePlanting(ctx, a.actor, args.PlantingID, args.Version, on) + return pl, whenMissing(err, "planting", args.PlantingID, "list_plantings") } func (a *adapter) listSeedLots(ctx context.Context, args struct { @@ -934,14 +973,15 @@ func (a *adapter) updateSeedLot(ctx context.Context, args struct { SetPackedForYear: args.PackedForYear != nil, PackedForYear: args.PackedForYear, SetGerminationPct: args.GerminationPct != nil, GerminationPct: args.GerminationPct, } - return a.svc.UpdateSeedLot(ctx, a.actor, args.LotID, patch, args.Version) + l, err := a.svc.UpdateSeedLot(ctx, a.actor, args.LotID, patch, args.Version) + return l, whenMissing(err, "seed lot", args.LotID, "list_seed_lots") } func (a *adapter) deleteSeedLot(ctx context.Context, args struct { LotID int64 `json:"lotId" description:"seed lot to delete (its id from list_seed_lots)"` }) (any, error) { if err := a.svc.DeleteSeedLot(ctx, a.actor, args.LotID); err != nil { - return nil, err + return nil, whenMissing(err, "seed lot", args.LotID, "list_seed_lots") } return map[string]any{"deleted": args.LotID}, nil } @@ -956,7 +996,7 @@ func (a *adapter) deletePlant(ctx context.Context, args struct { return nil, fmt.Errorf("%w: the plant is still used — by plantings (past seasons count) or a seed lot — so it stays; tell the user rather than removing those", domain.ErrPlantInUse) } if err != nil { - return nil, err + return nil, whenMissing(err, "plant of the user's own", args.PlantID, "find_plant") } return map[string]any{"deleted": args.PlantID}, nil } @@ -965,7 +1005,7 @@ func (a *adapter) deletePlanting(ctx context.Context, args struct { PlantingID int64 `json:"plantingId" description:"plop to delete outright (its id from describe_garden or list_plantings)"` }) (any, error) { if err := a.svc.DeletePlanting(ctx, a.actor, args.PlantingID); err != nil { - return nil, err + return nil, whenMissing(err, "planting", args.PlantingID, "list_plantings") } return map[string]any{"deleted": args.PlantingID}, nil } @@ -1146,5 +1186,8 @@ func (a *adapter) publicLink(ctx context.Context, args struct { if err != nil { return nil, err } + if action != "get" { + a.noteChange() + } return a.linkOf(link), nil }