Agent: a turn that changed nothing cannot say it did #132
+30
-16
@@ -169,7 +169,7 @@ func (r *Runner) Run(ctx context.Context, actorID, gardenID int64, message, toda
|
|||||||
if result != nil {
|
if result != nil {
|
||||||
turn.Reply = result.Output
|
turn.Reply = result.Output
|
||||||
turn.Steps = len(result.Steps)
|
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
|
// 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.
|
// 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))
|
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,
|
"list_shares": true,
|
||||||
}
|
}
|
||||||
|
|
||||||
// changeClaim matches a reply that reports a change as made: a "Done"/"Fixed"
|
// selfReportingTools succeed without necessarily changing anything —
|
||||||
// opener, or a first-person past-tense claim ("I've deleted", "I moved"). A
|
// public_link with action=get reads, and undo_change with nothing left to
|
||||||
// question or an offer ("want me to delete it?", "I'll remove it") does not
|
// revert reverts nothing — so their results don't count; the adapter says
|
||||||
// match — only a claim of something already done.
|
// whether they changed something (adapter.changed).
|
||||||
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` +
|
var selfReportingTools = map[string]bool{"public_link": true, "undo_change": true}
|
||||||
|
|
|||||||
`|\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)`)
|
|
||||||
|
// 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.
|
// 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._"
|
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 st.Response != nil {
|
||||||
if text := strings.Join(strings.Fields(st.Response.Text()), " "); text != "" {
|
if text := strings.Join(strings.Fields(st.Response.Text()), " "); text != "" {
|
||||||
if len(text) > 80 {
|
// Cut on a rune boundary, like turnSummary: a byte slice can
|
||||||
text = text[:80] + "…"
|
// 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)
|
fmt.Fprintf(&b, " %q", text)
|
||||||
|
gitea-actions
commented
🟠 undo_change that reverts nothing (cs==nil) counts as 'acted', so honestReply misses a false 'I've undone it' claim error-handling · flagged by 1 model
🪰 Gadfly · advisory 🟠 **undo_change that reverts nothing (cs==nil) counts as 'acted', so honestReply misses a false 'I've undone it' claim**
_error-handling · flagged by 1 model_
- **`internal/agent/runtime.go:234` — a successful tool that changed nothing still counts as "acted", so the guard has a hole for exactly the case it targets.** `acted` returns true for any `!IsError && !readOnlyTools[name]` result. But `undo_change` returns a **non-error** result when it reverted nothing: `undoChange` (`tools.go:716`) handles `cs == nil` ("every revision was a conflict, or the set was empty") by returning `(res, nil)` with `res.Changes = "nothing"`. So a turn where the model ca…
<sub>🪰 Gadfly · advisory</sub>
|
|||||||
}
|
}
|
||||||
@@ -226,17 +239,18 @@ func describeSteps(r *agent.Result) string {
|
|||||||
return strings.Join(parts, " | ")
|
return strings.Join(parts, " | ")
|
||||||
}
|
}
|
||||||
|
|
||||||
// acted reports whether any tool call in the run succeeded at something that
|
// acted reports whether the run changed anything: a successful call to a tool
|
||||||
// is not read-only.
|
// that is neither read-only nor self-reporting, or a self-reporting tool that
|
||||||
func acted(r *agent.Result) bool {
|
// told the adapter it changed something.
|
||||||
|
func acted(r *agent.Result, tools *adapter) bool {
|
||||||
for _, st := range r.Steps {
|
for _, st := range r.Steps {
|
||||||
for _, res := range st.Results {
|
for _, res := range st.Results {
|
||||||
if !res.IsError && !readOnlyTools[res.Name] {
|
if !res.IsError && !readOnlyTools[res.Name] && !selfReportingTools[res.Name] {
|
||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
gitea-actions
commented
🟡 changeClaim opener false-positives on read-only informational replies starting with 'Done'/'Updated', appending a bogus correction error-handling · flagged by 1 model
🪰 Gadfly · advisory 🟡 **changeClaim opener false-positives on read-only informational replies starting with 'Done'/'Updated', appending a bogus correction**
_error-handling · flagged by 1 model_
- **`internal/agent/runtime.go:251` — the guard false-positives on a read-only turn whose reply merely opens with a change verb.** The `^\s*(?:done|fixed|updated|…)\b` opener matches replies like *"Done — here's your garden: …"* or *"Updated: you have 12 beds"* to a pure information request answered with only read-only tools (`acted() == false`). Such a turn gets *"nothing actually changed in this turn … Ask again and I'll do it properly"* appended even though the user never asked for a change —…
<sub>🪰 Gadfly · advisory</sub>
|
|||||||
}
|
}
|
||||||
return false
|
return tools != nil && tools.didChange()
|
||||||
}
|
}
|
||||||
|
|
||||||
// honestReply appends a correction to a reply that claims a change when no
|
// 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
|
// 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
|
// 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.
|
// false one once anything at all was done, so it only speaks when nothing was.
|
||||||
func honestReply(reply string, r *agent.Result) string {
|
func honestReply(reply string, r *agent.Result, tools *adapter) string {
|
||||||
if r == nil || acted(r) || !changeClaim.MatchString(reply) {
|
if r == nil || acted(r, tools) || !changeClaim.MatchString(reply) {
|
||||||
return reply
|
return reply
|
||||||
}
|
}
|
||||||
return reply + unbackedClaim
|
return reply + unbackedClaim
|
||||||
|
|||||||
@@ -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) {
|
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)
|
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.
|
// 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."))
|
r := run(toolCall("delete_journal_entry", map[string]any{"entryId": entry.ID}), fake.Reply("Done — I've deleted the journal entry."))
|
||||||
if corrected(r) {
|
if corrected(r) {
|
||||||
t.Errorf("a true claim was corrected: %q", 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 {
|
if _, _, err := svc.ListJournal(ctx, owner, g.ID, service.JournalQuery{}); err != nil {
|
||||||
t.Fatalf("journal after: %v", err)
|
t.Fatalf("journal after: %v", err)
|
||||||
}
|
}
|
||||||
|
|||||||
+32
-7
@@ -277,6 +277,10 @@ type adapter struct {
|
|||||||
today string
|
today string
|
||||||
|
|
||||||
mu sync.Mutex
|
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
|
// 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
|
// 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
|
// marked undone), so it never joins the turn's scope — which leaves a turn
|
||||||
@@ -285,6 +289,20 @@ type adapter struct {
|
|||||||
reverts []int64
|
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.
|
// lastRevert is the newest change set undo_change produced this turn, if any.
|
||||||
func (a *adapter) lastRevert() *int64 {
|
func (a *adapter) lastRevert() *int64 {
|
||||||
a.mu.Lock()
|
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"`
|
YCM float64 `json:"yCm" description:"new center y in garden cm"`
|
||||||
Version int64 `json:"version" description:"the object's current version (from describe_garden)"`
|
Version int64 `json:"version" description:"the object's current version (from describe_garden)"`
|
||||||
}) (any, error) {
|
}) (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)
|
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 {
|
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"`
|
Vendor *string `json:"vendor" description:"optional vendor name"`
|
||||||
Notes *string `json:"notes" description:"optional free-text notes"`
|
Notes *string `json:"notes" description:"optional free-text notes"`
|
||||||
}) (any, error) {
|
}) (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,
|
Name: args.Name, Category: args.Category, SpacingCM: args.SpacingCM, Color: args.Color,
|
||||||
SetDays: args.DaysToMaturity != nil, DaysToMaturity: args.DaysToMaturity,
|
SetDays: args.DaysToMaturity != nil, DaysToMaturity: args.DaysToMaturity,
|
||||||
SourceURL: args.SourceURL, Vendor: args.Vendor, Notes: args.Notes,
|
SourceURL: args.SourceURL, Vendor: args.Vendor, Notes: args.Notes,
|
||||||
}, args.Version)
|
}, 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 {
|
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})
|
n, err := a.svc.ClearPlantings(ctx, a.actor, args.ObjectID, service.ClearOptions{RemovedAt: on})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, whenMissing(err, "object", args.ObjectID, "describe_garden")
|
||||||
}
|
}
|
||||||
return map[string]int{"cleared": n}, nil
|
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,
|
n, err := a.svc.ClearPlantings(ctx, a.actor, args.ObjectID,
|
||||||
service.ClearOptions{PlantID: &args.PlantID, RemovedAt: on})
|
service.ClearOptions{PlantID: &args.PlantID, RemovedAt: on})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, whenMissing(err, "object", args.ObjectID, "describe_garden")
|
||||||
}
|
}
|
||||||
return map[string]int{"removed": n}, nil
|
return map[string]int{"removed": n}, nil
|
||||||
}
|
}
|
||||||
@@ -727,6 +747,7 @@ func (a *adapter) undoChange(ctx context.Context, args struct {
|
|||||||
}
|
}
|
||||||
a.mu.Lock()
|
a.mu.Lock()
|
||||||
a.reverts = append(a.reverts, cs.ID)
|
a.reverts = append(a.reverts, cs.ID)
|
||||||
|
a.changed = true
|
||||||
a.mu.Unlock()
|
a.mu.Unlock()
|
||||||
return res, nil
|
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"`
|
RotationDeg *float64 `json:"rotationDeg" description:"optional new rotation in degrees"`
|
||||||
Plantable *bool `json:"plantable" description:"optional: whether the object can hold plants"`
|
Plantable *bool `json:"plantable" description:"optional: whether the object can hold plants"`
|
||||||
}) (any, error) {
|
}) (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,
|
Name: args.Name, WidthCM: args.WidthCM, HeightCM: args.HeightCM,
|
||||||
RotationDeg: args.RotationDeg, Plantable: args.Plantable,
|
RotationDeg: args.RotationDeg, Plantable: args.Plantable,
|
||||||
}, args.Version)
|
}, args.Version)
|
||||||
|
return o, whenMissing(err, "object", args.ObjectID, "describe_garden")
|
||||||
}
|
}
|
||||||
|
|
||||||
func (a *adapter) deleteObject(ctx context.Context, args struct {
|
func (a *adapter) deleteObject(ctx context.Context, args struct {
|
||||||
ObjectID int64 `json:"objectId" description:"object to delete (with its plantings)"`
|
ObjectID int64 `json:"objectId" description:"object to delete (with its plantings)"`
|
||||||
}) (any, error) {
|
}) (any, error) {
|
||||||
if err := a.svc.DeleteObject(ctx, a.actor, args.ObjectID); err != nil {
|
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
|
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)
|
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 {
|
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
|
return map[string]any{"deleted": args.PlantID}, nil
|
||||||
}
|
}
|
||||||
@@ -1164,5 +1186,8 @@ func (a *adapter) publicLink(ctx context.Context, args struct {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
if action != "get" {
|
||||||
|
a.noteChange()
|
||||||
|
}
|
||||||
return a.linkOf(link), nil
|
return a.linkOf(link), nil
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user
🟠 changeClaim regex duplicates verb lists across two alternations, creating a maintenance hazard
maintainability · flagged by 4 models
internal/agent/runtime.go:197-198—changeClaimduplicates its verb inventory across two alternations. A single source of truth (e.g., a slice composed into the pattern) would be easier to keep consistent and less prone to omissions. As it stands, the two lists already diverge:created,set,made,editedand others appear only in theI've …branch, whiledoneappears only in the sentence-opener branch. That makes the regex harder to maintain and more likely to miss new claim ph…🪰 Gadfly · advisory