The first finding breaks this PR's central promise and is the reason the review was worth running. The run context carries a timeout. When it fired, WithChangeSet's recovery path tried to record what had already committed using that SAME dead context — which fails, losing the history for changes that really happened. So a timed-out turn left its partial work un-undoable, while the user-facing message cheerfully said "anything I'd already changed is in History". That message was a lie in exactly the case it was written for. Both recovery paths (WithChangeSet and RevertChangeSet) now commit with context.WithoutCancel. The commonest reason those paths run at all is a cancelled or timed-out context, so using it to write the record of what it did was self-defeating. There's a test that cancels mid-turn and asserts the partial work is recorded, marked partial, and revertible. turnSummary sliced bytes, so a message whose 120th byte fell inside a multibyte character stored invalid UTF-8 in the history summary. Not hypothetical for text people type. Trimmed by runes now, with a test using emoji. RecordAgentExchange's failure was logged and swallowed, directly under a comment claiming the user would see that their turn wasn't saved. The turn itself succeeded, so Done still goes out — but with a warning saying the exchange wasn't saved and won't survive a reload, because a clean "done" followed by a conversation that has forgotten it is the quieter lie. It also now records with a detached context, since the commonest reason that write fails is the client having gone away, and the exchange is worth keeping either way. AgentRunID was declared, documented as an executus join, and never set by anything. It's set now, from a per-run id that's also logged at run start, so a row in the history list has a thread back to the run that produced it — and the comment describes that rather than a dependency this repo doesn't have. Turn.History was populated and never read, left over from the client-held history design that persistence replaced. Removed. The SSE plumbing moved out of the handler into openEventStream, so agentChat reads like the other decode/call/encode handlers in the package. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
This commit is contained in:
@@ -7,6 +7,8 @@ import (
|
||||
"fmt"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
"unicode/utf8"
|
||||
|
||||
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
|
||||
"gitea.stevedudenhoeffer.com/steve/majordomo/provider/fake"
|
||||
@@ -277,3 +279,81 @@ func TestSystemPromptStatesTheCompassConvention(t *testing.T) {
|
||||
t.Error("system prompt doesn't state the garden's size")
|
||||
}
|
||||
}
|
||||
|
||||
// TestPartialWorkSurvivesATimeout is the finding that mattered most on this PR.
|
||||
//
|
||||
// The run context carries a timeout. When it fires, WithChangeSet's recovery
|
||||
// path has to record what already committed — and doing that with the SAME
|
||||
// dead context would fail, losing the history for changes that really happened.
|
||||
// The user-facing message says "anything I'd already changed is in History", so
|
||||
// this isn't just a gap, it's a promise the code has to keep.
|
||||
func TestPartialWorkSurvivesATimeout(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)
|
||||
}
|
||||
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)
|
||||
}
|
||||
before, _, _ := svc.GardenHistory(ctx, owner, g.ID, 0, 0)
|
||||
|
||||
// A turn that renames the bed, then dies with the context already cancelled.
|
||||
cancelled, cancel := context.WithCancel(ctx)
|
||||
r := scriptedRunner(t, svc,
|
||||
toolCall("move_object", map[string]any{
|
||||
"objectId": bed.ID, "xCm": 600.0, "yCm": 600.0, "version": bed.Version,
|
||||
}),
|
||||
fake.Step{Err: context.DeadlineExceeded},
|
||||
)
|
||||
// Cancel once the first tool call has landed, so the failure path runs with a
|
||||
// dead context — exactly the timeout case.
|
||||
go func() {
|
||||
time.Sleep(50 * time.Millisecond)
|
||||
cancel()
|
||||
}()
|
||||
_, err = r.Run(cancelled, owner, g.ID, "move the bed", nil, nil)
|
||||
if err == nil {
|
||||
t.Fatal("expected the turn to fail")
|
||||
}
|
||||
|
||||
// The move committed, so it must be in history and undoable.
|
||||
after, _, herr := svc.GardenHistory(ctx, owner, g.ID, 0, 0)
|
||||
if herr != nil {
|
||||
t.Fatalf("history: %v", herr)
|
||||
}
|
||||
if len(after) != len(before)+1 {
|
||||
t.Fatalf("the failed turn recorded %d change sets, want 1 — its work is otherwise un-undoable",
|
||||
len(after)-len(before))
|
||||
}
|
||||
if !strings.Contains(after[0].Summary, "failed partway") {
|
||||
t.Errorf("summary = %q, want it marked as partial", after[0].Summary)
|
||||
}
|
||||
if _, conflicts, rerr := svc.RevertChangeSet(ctx, owner, after[0].ID, domain.SourceUI); rerr != nil || len(conflicts) != 0 {
|
||||
t.Fatalf("the partial turn should be undoable: err=%v conflicts=%+v", rerr, conflicts)
|
||||
}
|
||||
o, _ := svc.DescribeGarden(ctx, owner, g.ID)
|
||||
if len(o.Objects) > 0 && o.Objects[0].XCM != bed.XCM {
|
||||
t.Errorf("undo left the bed at %v, want %v", o.Objects[0].XCM, bed.XCM)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTurnSummaryTrimsByRunes — slicing a byte offset would cut a multibyte
|
||||
// character in half and store invalid UTF-8 in the history summary.
|
||||
func TestTurnSummaryTrimsByRunes(t *testing.T) {
|
||||
// 200 multibyte runes: a byte slice at 120 would land mid-character.
|
||||
got := turnSummary(strings.Repeat("🌱", 200))
|
||||
if !utf8.ValidString(got) {
|
||||
t.Errorf("turnSummary produced invalid UTF-8: %q", got)
|
||||
}
|
||||
if !strings.HasSuffix(got, "…") {
|
||||
t.Errorf("long summary should be elided, got %q", got)
|
||||
}
|
||||
if n := utf8.RuneCountInString(got); n > 121 {
|
||||
t.Errorf("summary is %d runes, want it trimmed", n)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user