From d4eb62a2bad6fd291dd3b6668e4dea504b491562 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sun, 23 Aug 2026 02:49:55 -0400 Subject: [PATCH] Address #132 review: one verb list, self-reporting tools, rune-safe log - changeClaim is built from one changeVerbs list; the opener is just done/fixed/undone so an informational "Updated totals:" can't trip it. - public_link (get reads) and undo_change (nothing left to revert) are self-reporting: their success no longer counts as a change by name; the adapter says whether they changed something (noteChange / didChange). - whenMissing covers the object and plant tools too (move/update/delete object, clear/remove plantings by object, update/delete plant). - The step summary cuts on a rune boundary. Co-Authored-By: Claude Fable 5 --- internal/agent/runtime.go | 46 ++++++++++++++++++++++------------ internal/agent/runtime_test.go | 18 +++++++++++++ internal/agent/tools.go | 39 ++++++++++++++++++++++------ 3 files changed, 80 insertions(+), 23 deletions(-) diff --git a/internal/agent/runtime.go b/internal/agent/runtime.go index 470ed5f..4444584 100644 --- a/internal/agent/runtime.go +++ b/internal/agent/runtime.go @@ -169,7 +169,7 @@ 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); corrected != turn.Reply { + 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)) @@ -190,12 +190,23 @@ var readOnlyTools = map[string]bool{ "list_shares": true, } -// changeClaim matches a reply that reports a change as made: a "Done"/"Fixed" -// 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. -var changeClaim = regexp.MustCompile(`(?i)(?:^\s*(?:done|fixed|undone|deleted|removed|updated|changed|added|saved|moved|planted|filled|cleared|corrected|recorded|marked|shared|renamed|reverted)\b` + - `|\bI(?:'ve| have)? (?:just |now |also |already )?(?: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)\b)`) +// 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._" @@ -215,8 +226,10 @@ func describeSteps(r *agent.Result) string { } if st.Response != nil { if text := strings.Join(strings.Fields(st.Response.Text()), " "); text != "" { - if len(text) > 80 { - text = text[:80] + "…" + // 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) } @@ -226,17 +239,18 @@ func describeSteps(r *agent.Result) string { return strings.Join(parts, " | ") } -// acted reports whether any tool call in the run succeeded at something that -// is not read-only. -func acted(r *agent.Result) bool { +// 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] { + if !res.IsError && !readOnlyTools[res.Name] && !selfReportingTools[res.Name] { return true } } } - return false + return tools != nil && tools.didChange() } // honestReply appends a correction to a reply that claims a change when no @@ -247,8 +261,8 @@ func acted(r *agent.Result) bool { // 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) string { - if r == nil || acted(r) || !changeClaim.MatchString(reply) { +func honestReply(reply string, r *agent.Result, tools *adapter) string { + if r == nil || acted(r, tools) || !changeClaim.MatchString(reply) { return reply } return reply + unbackedClaim diff --git a/internal/agent/runtime_test.go b/internal/agent/runtime_test.go index 830f5df..3470142 100644 --- a/internal/agent/runtime_test.go +++ b/internal/agent/runtime_test.go @@ -642,11 +642,29 @@ func TestAClaimedChangeNoToolMadeIsCorrected(t *testing.T) { 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 982f744..c232d4a 100644 --- a/internal/agent/tools.go +++ b/internal/agent/tools.go @@ -277,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 @@ -285,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() @@ -393,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 { @@ -510,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 { @@ -542,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 } @@ -563,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 } @@ -727,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 } @@ -795,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 } @@ -974,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 } @@ -1164,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 }