mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-11 18:00:54 -10:00
* fix(agent): don't end a run as success on a no-tool-call turn mid-task
A turn that produced text but no tool call was always accepted as the final answer, so the loop reported success even when the model stopped mid-task (e.g. ended on "...Let me check the SSH configuration:" with plan steps still pending).
Add an opt-in completion gate (Options.RequireCompletionSignal): when a turn has no tool call and work clearly remains -- pending update_plan items, or the message ends on a continuation cue -- re-prompt the model to continue instead of finalizing. Bounded by maxContinueNudges and still by MaxTurns/the deadline; once the budget is spent the run finalizes as INCOMPLETE (Result.Incomplete) rather than success. Default off, so the interactive path is byte-identical.
Genuine single-turn completions (no pending plan, no cue) still finalize as success. Covered by internal/agent/completion_gate_test.go.
* feat(exec): report stalled headless runs as INCOMPLETE (exit 4)
Enable the agent completion gate for headless exec (RequireCompletionSignal) and map Result.Incomplete to run_end status "incomplete" with a new exit code 4, so a run that stalled mid-task (model stopped without a tool call while work remained, continue budget exhausted) is no longer reported as success. Interactive callers are unaffected.
* fix(agent): downgrade self-reported non-completion; advisory task-grounded acceptance
Reduce -- not eliminate -- false-success on headless runs. Two DETERMINISTIC, unit-tested gates (with the plan gate from the prior commit):
(a) self-report downgrade: if the final message admits the model guessed or could not meet the objective, finalize INCOMPLETE (exit 4), never success. Inability is matched by first-person STEMS generalized over verb/tense ("I cannot/can't/could not/am unable to/do not have/unable to ...") plus guess/fallback/uncertainty phrases, with a guard so success-y negations ("could not find any issues", "cannot reproduce") are not misread. (b7bc0b8's plan gate already forces INCOMPLETE on pending/in_progress update_plan items at termination.)
(b) task-grounded acceptance is ADVISORY, not a guarantee. When --self-correct is on it demands one bounded acceptance pass that re-derives the task's stated criterion and runs a concrete check, discouraging three false-success patterns (well-formed==correct, existing-tests-pass==objective-met, result==baseline-it-was-told-to-beat). But it is a prompt: a model that ignores it and confidently claims "PASS, all requirements met" still slips, because ZERO has no general oracle to verify correctness against a task's hidden criterion. Empirically (TB-2, qwen3-coder:480b) this reliably catches admissions and incomplete plans and REDUCES false-success, but a confident false PASS on a model-ceiling task is a residual, fundamental gap -- not a tuning miss.
Default off (RequireCompletionSignal); interactive callers are byte-identical. Covered by internal/agent/{acceptance_gate_test.go,completion_gate_test.go}.
* feat(exec): surface the INCOMPLETE reason in run_end and logs
When a headless run finalizes as INCOMPLETE, include Result.IncompleteReason in the session error event and a stderr warning so an honestly-incomplete run (e.g. "the final message admits the objective was not met") is debuggable rather than an opaque exit 4.
* fix(agent): harden completion gate per PR review
Addresses the correctness findings from the PR review.
self-report (#1): drop behavior-describing phrases ("fall back to",
"placeholder value", bare "best guess", "as a fallback", "without proper")
that also match legitimate final answers; keep first-person/uncertainty
admissions only, since these are matched without a context guard.
self-report (#5): scan every occurrence of each inability stem so an early
success-negation ("could not find any examples") no longer masks a later
genuine admission with the same stem ("could not implement it").
continuation cue (#6): require a trailing colon AND an action lead-in on the
final clause; stop flagging recommendations, plain summary colons, and
sign-offs. Still catches the mid-line "...Let me check the config:".
gate order (#8): check the self-report admission BEFORE pending-plan, so an
admitted-impossible task downgrades immediately with the accurate reason
instead of burning continue-nudges.
pending plan (#3): treat a pending/in_progress update_plan item as a
NUDGE-only weak signal -- it no longer forces INCOMPLETE on its own (a
completed run that left stale plan bookkeeping is trusted). Only a
continuation cue or a self-report admission finalizes INCOMPLETE.
max-turns (#4): a run cut off at the MaxTurns ceiling now finalizes
INCOMPLETE under the gate instead of being reported as success.
exec json/cron (#2, #7): for -o json, emit the terminal done with exit 4 on
an incomplete run (final() pre-emits a success done:0 for json that would
otherwise mask it); emit an error event -- not just a warning -- so the cron
failure extractor can recover the reason.
Deferred (noted on the PR): acceptance-only-when-mutated (#9, cost) and
reusing tools.normalizePlanStatus (#10, would widen scope to internal/tools).
Tests: add TestContinuationCueMatching and TestMaxTurnsCutoffIsIncompleteUnderGate,
extend TestSelfReportedIncompletionMatching with the #1/#5 cases, and replace
the in_progress=>incomplete test with TestPendingPlanAloneDoesNotForceIncomplete.
make build / go vet / make lint / go test ./... -race all green.
229 lines
9.6 KiB
Go
229 lines
9.6 KiB
Go
package agent
|
|
|
|
import (
|
|
"context"
|
|
"strings"
|
|
"testing"
|
|
|
|
"github.com/Gitlawb/zero/internal/tools"
|
|
"github.com/Gitlawb/zero/internal/zeroruntime"
|
|
)
|
|
|
|
func enabledSelfCorrector(t *testing.T) *SelfCorrector {
|
|
t.Helper()
|
|
// checker/verifier nil → no LSP / project-test pass runs; we only need a
|
|
// non-nil SelfCorrector to enable the task-grounded acceptance gate.
|
|
return NewSelfCorrector(t.TempDir(), nil, nil, SelfCorrectConfig{Enabled: true})
|
|
}
|
|
|
|
// BUG #2(a): a final message that admits the model guessed / could not meet the
|
|
// objective must be reported as INCOMPLETE, never success — and immediately
|
|
// (an admitted guess is not worth re-prompting).
|
|
func TestAcceptanceSelfReportAdmissionIsIncomplete(t *testing.T) {
|
|
provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{
|
|
textTurn("I couldn't analyze the board image, so I wrote the common opening move e2e4 as my best guess."),
|
|
}}
|
|
|
|
result, err := Run(context.Background(), "write the best chess move", provider, Options{
|
|
Registry: tools.NewRegistry(),
|
|
MaxTurns: 10,
|
|
RequireCompletionSignal: true,
|
|
SelfCorrect: enabledSelfCorrector(t),
|
|
})
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if !result.Incomplete {
|
|
t.Fatalf("admitted guess must be Incomplete, got success; final=%q", result.FinalAnswer)
|
|
}
|
|
if !strings.Contains(result.IncompleteReason, "objective was not met") {
|
|
t.Fatalf("IncompleteReason = %q, want it to cite the admission", result.IncompleteReason)
|
|
}
|
|
if len(provider.requests) != 1 {
|
|
t.Fatalf("an admission should downgrade immediately (1 turn), got %d", len(provider.requests))
|
|
}
|
|
}
|
|
|
|
// BUG #2(a): the inability detector must generalize over the verb the model uses
|
|
// (so re-phrasing can't defeat it) while NOT misreading success-y negations
|
|
// ("could not find any issues") as admissions.
|
|
func TestSelfReportedIncompletionMatching(t *testing.T) {
|
|
cases := []struct {
|
|
text string
|
|
admit bool
|
|
}{
|
|
// real chess re-run wording that slipped a fixed phrase list:
|
|
{"…which I cannot do without proper image analysis capabilities.", true},
|
|
{"I cannot visually analyze the chess board image.", true},
|
|
{"I could not recover the original values, so this is my best guess.", true},
|
|
{"I was unable to complete the optimization.", true},
|
|
{"The result may not be correct.", true},
|
|
// multi-occurrence: an early guarded use ("could not find any") must NOT mask a
|
|
// later genuine admission with the same stem (review #5):
|
|
{"I could not find any examples in the codebase, so I could not implement the feature.", true},
|
|
// genuine successes that must NOT be flagged:
|
|
{"I implemented the function and all tests pass.", false},
|
|
{"I verified the fix; I could not find any remaining issues.", false},
|
|
{"I cannot reproduce the bug, so the fix holds.", false},
|
|
{"Done — the output matches the required format and values.", false},
|
|
// behavior descriptions that must NOT be flagged — these phrases describe
|
|
// implemented behavior, not an admission (review #1, removed from the list):
|
|
{"The parser will fall back to UTF-8 when no encoding is set.", false},
|
|
{"I replaced the placeholder value with the computed key.", false},
|
|
{"The retry uses exponential backoff as a fallback.", false},
|
|
{"Without proper error handling the function would crash, so I added it.", false},
|
|
}
|
|
for _, c := range cases {
|
|
got := selfReportedIncompletion(c.text) != ""
|
|
if got != c.admit {
|
|
t.Errorf("selfReportedIncompletion(%q) admitted=%v, want %v", c.text, got, c.admit)
|
|
}
|
|
}
|
|
}
|
|
|
|
// BUG #2(a): a "cannot analyze / cannot determine" admission (present tense) must
|
|
// be caught even when the model also stamps a bogus "PASS" based on a shape check.
|
|
// Mirrors the chess-best-move final ("I cannot analyze the position… PASS: I wrote
|
|
// the move and verified by reading it back").
|
|
func TestAcceptanceCannotPhrasingIsIncomplete(t *testing.T) {
|
|
provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{
|
|
textTurn("I cannot analyze the specific position from the image, so I cannot determine the best move. PASS: I wrote e2e4 to the file and verified it by reading it back."),
|
|
}}
|
|
|
|
result, err := Run(context.Background(), "write the best move", provider, Options{
|
|
Registry: tools.NewRegistry(),
|
|
MaxTurns: 10,
|
|
RequireCompletionSignal: true,
|
|
SelfCorrect: enabledSelfCorrector(t),
|
|
})
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if !result.Incomplete {
|
|
t.Fatalf("a 'cannot analyze/determine' admission must be Incomplete despite the bogus PASS; final=%q", result.FinalAnswer)
|
|
}
|
|
}
|
|
|
|
// A pending/in_progress update_plan item is an AMBIGUOUS signal (the model may have
|
|
// finished without re-marking the step), so it must NOT by itself force INCOMPLETE.
|
|
// The gate nudges, but if the model keeps claiming completion with no continuation
|
|
// cue and no admission, the run finalizes as SUCCESS — trusting the completion claim
|
|
// over stale plan bookkeeping. (Only a continuation cue or self-report admission
|
|
// forces INCOMPLETE; see TestCompletionGate* and TestAcceptance*Admission*.)
|
|
func TestPendingPlanAloneDoesNotForceIncomplete(t *testing.T) {
|
|
registry := tools.NewRegistry()
|
|
registry.Register(tools.NewUpdatePlanTool())
|
|
done := "All set." // confident; no cue, no admission — only the plan is stale
|
|
provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{
|
|
planTurn("completed", "in_progress"),
|
|
textTurn(done), textTurn(done), textTurn(done), textTurn(done), textTurn(done),
|
|
}}
|
|
|
|
result, err := Run(context.Background(), "do the multi-step task", provider, Options{
|
|
Registry: registry,
|
|
MaxTurns: 10,
|
|
RequireCompletionSignal: true,
|
|
SelfCorrect: enabledSelfCorrector(t),
|
|
})
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if result.Incomplete {
|
|
t.Fatalf("pending-plan alone must NOT force Incomplete (stale bookkeeping != unfinished); reason=%q", result.IncompleteReason)
|
|
}
|
|
if result.FinalAnswer != done {
|
|
t.Fatalf("final answer = %q, want the completion claim %q", result.FinalAnswer, done)
|
|
}
|
|
if !someRequestContains(provider.requests, continueNudgeMarker) {
|
|
t.Fatalf("expected at least one continue-nudge before accepting completion")
|
|
}
|
|
}
|
|
|
|
// BUG #2(b): when self-correct is on, the run must pass a one-time task-grounded
|
|
// acceptance check before success. A model that, when challenged, confirms its
|
|
// work meets the criterion (no admission) then finalizes as success — no regression.
|
|
func TestAcceptanceGroundedCompletionSucceeds(t *testing.T) {
|
|
provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{
|
|
textTurn("I implemented the function."),
|
|
textTurn("PASS. I ran the verification and it passes; the output meets the task requirement."),
|
|
}}
|
|
|
|
result, err := Run(context.Background(), "implement the function", provider, Options{
|
|
Registry: tools.NewRegistry(),
|
|
MaxTurns: 10,
|
|
RequireCompletionSignal: true,
|
|
SelfCorrect: enabledSelfCorrector(t),
|
|
})
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if result.Incomplete {
|
|
t.Fatalf("a verified completion must succeed, got Incomplete (%q)", result.IncompleteReason)
|
|
}
|
|
if result.FinalAnswer != "PASS. I ran the verification and it passes; the output meets the task requirement." {
|
|
t.Fatalf("final answer = %q", result.FinalAnswer)
|
|
}
|
|
// Exactly one acceptance pass: the first "done" turn was challenged, the second confirmed.
|
|
if len(provider.requests) != 2 {
|
|
t.Fatalf("expected 2 turns (1 acceptance challenge + 1 confirmation), got %d", len(provider.requests))
|
|
}
|
|
if !someRequestContains(provider.requests, acceptanceNudgeMarker) {
|
|
t.Fatalf("expected the task-grounded acceptance check to be demanded")
|
|
}
|
|
}
|
|
|
|
// With the gate OFF (interactive/TUI default), even an admission is accepted as the
|
|
// final answer exactly as before — proving no behavior change for non-headless callers.
|
|
func TestAcceptanceGateOffLeavesAdmissionAsSuccess(t *testing.T) {
|
|
admission := "I couldn't determine the value, so this is a guess."
|
|
provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{
|
|
textTurn(admission),
|
|
}}
|
|
|
|
result, err := Run(context.Background(), "do a thing", provider, Options{
|
|
Registry: tools.NewRegistry(),
|
|
MaxTurns: 10,
|
|
// RequireCompletionSignal false, SelfCorrect nil — the legacy path.
|
|
})
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if result.Incomplete {
|
|
t.Fatalf("legacy path must never set Incomplete; final=%q", result.FinalAnswer)
|
|
}
|
|
if result.FinalAnswer != admission {
|
|
t.Fatalf("final answer = %q, want the text verbatim (legacy behavior)", result.FinalAnswer)
|
|
}
|
|
if len(provider.requests) != 1 {
|
|
t.Fatalf("legacy path must not re-prompt; got %d turns", len(provider.requests))
|
|
}
|
|
}
|
|
|
|
// The acceptance check is scoped to runs with self-correct on: a headless run
|
|
// WITHOUT a SelfCorrector accepts a confident completion immediately (the (b) gate
|
|
// is inert), so enabling RequireCompletionSignal alone adds no acceptance turn.
|
|
func TestAcceptanceGateRequiresSelfCorrect(t *testing.T) {
|
|
provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{
|
|
textTurn("Done. Implemented and it works."),
|
|
}}
|
|
|
|
result, err := Run(context.Background(), "implement it", provider, Options{
|
|
Registry: tools.NewRegistry(),
|
|
MaxTurns: 10,
|
|
RequireCompletionSignal: true,
|
|
// SelfCorrect deliberately nil → acceptance gate (b) must not fire.
|
|
})
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if result.Incomplete {
|
|
t.Fatalf("confident completion without self-correct must succeed; reason=%q", result.IncompleteReason)
|
|
}
|
|
if len(provider.requests) != 1 {
|
|
t.Fatalf("acceptance gate must not fire without self-correct; got %d turns", len(provider.requests))
|
|
}
|
|
if someRequestContains(provider.requests, acceptanceNudgeMarker) {
|
|
t.Fatalf("acceptance nudge must not be injected when self-correct is off")
|
|
}
|
|
}
|