diff --git a/run/executor.go b/run/executor.go index 63de88a..4176447 100644 --- a/run/executor.go +++ b/run/executor.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "strings" "time" "gitea.stevedudenhoeffer.com/steve/majordomo/agent" @@ -500,6 +501,15 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio // small fixed step cap so a misread nudge can't start a new work spree. // The nudge round's failure is non-fatal — the original successful result // stands (any attachments it queued before failing still deliver). + // + // The nudge round's job is to QUEUE deliveries (tool calls); its TEXT is + // meta-commentary ("answer above stands", "sent!") that must never replace + // the answer the user asked for — hosts deliver Output text + separately + // drained attachments, so replacing Output with the nudge round's closer + // LOSES the real answer (live regression: run d5ec39f4, a research run's + // page-cache false positive whose nudge reply "my answer above stands + // as-is" was delivered INSTEAD of the answer). Keep the original Output + // unless it was blank; log what was discarded so hosts can audit. if runErr == nil && runRes != nil && e.cfg.Ports.FinalGuard != nil { state := FinalState{ Output: runRes.Output, @@ -529,7 +539,20 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio slog.Warn("run: final-nudge round failed; keeping original result", "run_id", inv.RunID, "error", nudgeErr) } else if nudgeRes != nil { - runRes.Output = nudgeRes.Output + if strings.TrimSpace(runRes.Output) == "" { + // Degenerate original (blank stop turn): the nudge round's + // text is the only answer there is. + runRes.Output = nudgeRes.Output + } else if rec != nil { + // Discarded meta-text stays observable: a nudge round that + // DECLINED to queue while the original claims a send shows + // up here, not in the user's reply. + rec.LogEvent("final_nudge_result", map[string]any{ + "discarded_output": nudgeRes.Output, + }) + } + // The transcript keeps the nudge round either way (audit, + // PostRun, checkpoint continuity). runRes.Messages = nudgeRes.Messages } } diff --git a/run/final_guard.go b/run/final_guard.go index 6ea813e..4ae4c11 100644 --- a/run/final_guard.go +++ b/run/final_guard.go @@ -20,10 +20,17 @@ import ( // A non-empty nudge sends the loop back for ONE bounded extra round: the nudge // is appended to the SAME conversation as the next user turn and the agent // runs again with the same toolbox, capped at finalNudgeMaxSteps steps — enough -// to deliver a stranded artifact (or correct the final text), not enough to -// start a new project. The guard is consulted at most ONCE per run; the nudge -// round's own stop turn is final. Multi-phase runs are not guarded (their -// output contract is the phase pipeline's, not a delivery surface's). +// to deliver a stranded artifact, not enough to start a new project. The guard +// is consulted at most ONCE per run; the nudge round's own stop turn is final. +// Multi-phase runs are not guarded (their output contract is the phase +// pipeline's, not a delivery surface's). +// +// OUTPUT CONTRACT: the nudge round exists to make TOOL CALLS (queue the +// delivery); its final TEXT is discarded — the run's Output stays the original +// answer unless that answer was blank. A host's nudge message should say so +// explicitly ("any text you write here is not shown to the user"), and must +// not instruct the model to compose a corrected reply. Discarded nudge text is +// recorded as a final_nudge_result audit event. // // The call runs on the run goroutine, inside the run's deadline, and is // panic-isolated: a guard panic is logged and treated as "" (the run's diff --git a/run/final_guard_test.go b/run/final_guard_test.go index ecce145..c2b472b 100644 --- a/run/final_guard_test.go +++ b/run/final_guard_test.go @@ -41,17 +41,21 @@ func (r *eventRecorder) LogEvent(eventType string, _ map[string]any) { // TestFinalGuardNudgeRunsOneExtraRound is the core announce-then-stop fix: the // model stops with "Done — sent!" (nothing delivered), the guard returns a -// nudge, and the SAME conversation runs one more round — here a tool call then -// a corrected final answer. The guard is consulted exactly once (the nudge -// round's own stop is final), and the audit log records the nudge. +// nudge, and the SAME conversation runs one more round — here a tool call +// (queueing the delivery) then a closing text turn. The guard is consulted +// exactly once (the nudge round's own stop is final), the audit log records +// the nudge, and the ORIGINAL answer text survives: the nudge round's closer +// is discarded (recorded as final_nudge_result), because hosts deliver Output +// text + separately-drained attachments — replacing Output with the closer is +// the run-d5ec39f4 lost-answer regression. func TestFinalGuardNudgeRunsOneExtraRound(t *testing.T) { fp := fake.New("fake") fp.Enqueue("m", fake.Reply("Done — sent you the file!"), // Nudge round: an (unregistered → tolerated error result) delivery call, - // then the corrected final answer. + // then a meta closer that must NOT become the run output. fake.ReplyWith(llm.Response{ToolCalls: []llm.ToolCall{{ID: "c1", Name: "send_attachments", Arguments: []byte(`{}`)}}}), - fake.Reply("delivered for real"), + fake.Reply("queued the file — answer above stands"), ) m, _ := fp.Model("m") rec := &eventRecorder{} @@ -72,8 +76,11 @@ func TestFinalGuardNudgeRunsOneExtraRound(t *testing.T) { if res.Err != nil { t.Fatalf("run error: %v", res.Err) } - if res.Output != "delivered for real" { - t.Fatalf("output = %q, want the nudge round's answer", res.Output) + if res.Output != "Done — sent you the file!" { + t.Fatalf("output = %q, want the ORIGINAL answer preserved (nudge closer discarded)", res.Output) + } + if n := fp.CallCount("m"); n != 3 { + t.Fatalf("model called %d times, want 3 (the nudge round must still run)", n) } if guard.calls != 1 { t.Fatalf("guard consulted %d times, want exactly 1 (no re-guard of the nudge round)", guard.calls) @@ -84,17 +91,51 @@ func TestFinalGuardNudgeRunsOneExtraRound(t *testing.T) { if guard.state.Output != "Done — sent you the file!" { t.Errorf("guard state.Output = %q, want the pre-nudge final text", guard.state.Output) } - var sawNudgeEvent bool + var sawNudge, sawResult bool for _, ev := range rec.events { - if ev == "final_nudge" { - sawNudgeEvent = true + switch ev { + case "final_nudge": + sawNudge = true + case "final_nudge_result": + sawResult = true } } - if !sawNudgeEvent { + if !sawNudge { t.Errorf("audit events %v missing final_nudge", rec.events) } - if rec.stats.Output != "delivered for real" { - t.Errorf("audit close output = %q, want the nudged output", rec.stats.Output) + if !sawResult { + t.Errorf("audit events %v missing final_nudge_result (the discarded closer must stay observable)", rec.events) + } + if rec.stats.Output != "Done — sent you the file!" { + t.Errorf("audit close output = %q, want the original answer", rec.stats.Output) + } +} + +// TestFinalGuardNudgeFillsEmptyOriginal: when the ORIGINAL stop turn was blank +// (a degenerate run), the nudge round's text is the only answer there is — it +// becomes the output instead of being discarded. +func TestFinalGuardNudgeFillsEmptyOriginal(t *testing.T) { + fp := fake.New("fake") + fp.Enqueue("m", + fake.Reply(" \n"), + fake.Reply("actual answer from the nudge round"), + ) + m, _ := fp.Model("m") + guard := &guardFunc{nudge: func(RunInfo, FinalState) string { return "deliver or correct" }} + ex := New(Config{ + Registry: tool.NewRegistry(), + Models: func(ctx context.Context, _ string) (context.Context, llm.Model, error) { return ctx, m, nil }, + Ports: Ports{FinalGuard: guard}, + }) + + res := ex.Run(context.Background(), + RunnableAgent{ModelTier: "m"}, tool.Invocation{RunID: "r"}, "hi") + + if res.Err != nil { + t.Fatalf("run error: %v", res.Err) + } + if res.Output != "actual answer from the nudge round" { + t.Fatalf("output = %q, want the nudge round's text to fill a blank original", res.Output) } }