Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions internal/agent/acpx.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ func (a *acpxAgent) Name() string { return "acp:" + a.target }
func (a *acpxAgent) ReportsAgentAttempts() bool { return true }

func (a *acpxAgent) Run(ctx context.Context, opts RunOpts) (*Result, error) {
return runWithRetry(ctx, a.Name(), opts, claudeMaxRetries, classifyTransient, nil, func() (*Result, error) {
return a.runOnce(ctx, opts)
return runWithRetry(ctx, a.Name(), opts, claudeMaxRetries, classifyTransient, nil, func(o RunOpts) (*Result, error) {
return a.runOnce(ctx, o)
})
}

Expand Down
42 changes: 42 additions & 0 deletions internal/agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,17 @@ type Options struct {
Profile agentcfg.Profile
}

// errTurnEndedWithoutJSON is returned when every structured-output candidate
// fails AND the final text shows no attempt at JSON at all: it neither opens
// a JSON object nor carries a ```json fence. That shape means the model ended
// its turn with plain prose - progress narration after an early stop, or a
// turn aborted mid-task by provider stream errors - so leaking the raw
// encoding/json decoder error ("invalid character 'L' looking for beginning
// of value", #811) would only misdirect diagnosis toward format problems.
// Malformed output that DOES attempt JSON keeps the precise decoder error.
// This failure is retryable with a JSON reminder (see runWithRetry).
var errTurnEndedWithoutJSON = errors.New("agent ended its turn without the required JSON result")

func finalizeTextResult(agentName, text string, schema json.RawMessage, usage TokenUsage) (*Result, error) {
if text == "" {
return nil, fmt.Errorf("%s returned no text output", agentName)
Expand Down Expand Up @@ -352,9 +363,40 @@ func parseStructuredTextOutput(text string, schema json.RawMessage) (json.RawMes
if candidateErr != nil {
return nil, candidateErr
}
// No candidate anywhere parsed and the text never attempted JSON: report
// the prose-final-turn condition instead of the whole-text decoder error,
// which only ever named the first character of the narration (#811).
if !outputAttemptsJSON(text) {
return nil, errTurnEndedWithoutJSON
}
return nil, rawErr
}

// outputAttemptsJSON reports whether the final text shows the model tried to
// produce JSON at all: either it opens a JSON object (possibly after leading
// whitespace) or it contains a ```json fence opener anywhere. When neither
// holds, no candidate in parseStructuredTextOutput could ever have parsed,
// so the failure is a prose final turn rather than malformed JSON. The scan
// deliberately mirrors fencedJSONCandidates' opener detection (case-insensitive
// info token) and errs on the side of "attempted JSON" so only genuinely
// prose-shaped output is reclassified away from the decoder error.
func outputAttemptsJSON(text string) bool {
if strings.HasPrefix(strings.TrimLeft(text, " \t\r\n"), "{") {
return true
}
Comment on lines +384 to +386

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Embedded JSON attempts are missed

When narration precedes a truncated JSON object such as Let me finish... {"findings": [, this prefix-only check classifies the response as having made no JSON attempt, causing unnecessary retries and replacing the precise malformed-JSON error with the misleading missing-result sentinel.

Suggested change
if strings.HasPrefix(strings.TrimLeft(text, " \t\r\n"), "{") {
return true
}
if strings.Contains(text, "{") {
return true
}

rest := text
for {
marker := strings.Index(rest, "```")
if marker < 0 {
return false
}
if _, info := fenceContentStart(rest, marker); strings.EqualFold(strings.TrimSpace(info), "json") {
return true
}
rest = rest[marker+3:]
}
}

func textValidationSchema(schema json.RawMessage) (json.RawMessage, error) {
if len(schema) == 0 {
return nil, nil
Expand Down
61 changes: 61 additions & 0 deletions internal/agent/agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package agent
import (
"context"
"encoding/json"
"errors"
"os"
"path/filepath"
"runtime"
Expand Down Expand Up @@ -752,3 +753,63 @@ func TestFinalizeTextResult_WithSchemaParsesProseQuotingFenceExampleThenClosedBl
t.Errorf("expected summary=one issue, got %q", output.Summary)
}
}

func TestFinalizeTextResult_ProseFinalTurnReturnsDedicatedError(t *testing.T) {
// Regression (upstream #811): when the model ends its turn with plain
// progress narration instead of the required JSON object, no-mistakes fed
// that prose to encoding/json and leaked "invalid character 'L' looking
// for beginning of value" as the step error, with no retry.
schema := json.RawMessage(`{"type":"object"}`)
for _, text := range []string{
"Let me check something.",
"\n\nI've completed a thorough review of the diff. Let me verify a couple more details before finalizing.",
} {
_, err := finalizeTextResult("opencode", text, schema, TokenUsage{})
if err == nil {
t.Fatalf("expected error for prose output %q", text)
}
if !errors.Is(err, errTurnEndedWithoutJSON) {
t.Fatalf("expected errTurnEndedWithoutJSON for %q, got %v", text, err)
}
msg := err.Error()
if !strings.Contains(msg, "agent ended its turn without the required JSON result") {
t.Errorf("error should name the missing JSON result, got %v", err)
}
if strings.Contains(msg, "invalid character") {
t.Errorf("raw decoder error must not leak for prose output, got %v", err)
}
if !strings.Contains(msg, "output snippet:") || !strings.Contains(msg, strings.TrimSpace(text)) {
t.Errorf("error should keep the diagnosable output snippet, got %v", err)
}
}
}

func TestFinalizeTextResult_AttemptedJSONKeepsRawParseError(t *testing.T) {
// Output that shows an attempt at JSON - a leading '{', a ```json fence,
// or an embedded bare object - keeps today's precise parse/validation
// error instead of the dedicated prose-final-turn error (#811).
reviewSchema := json.RawMessage(`{
"type":"object",
"properties":{"findings":{"type":"array"},"summary":{"type":"string"}},
"required":["findings","summary"]
}`)
cases := []struct {
name string
text string
}{
{"malformed leading object", `{"findings": [} truncated`},
{"malformed fenced object", "```json\nnot json\n```"},
{"bare object missing required keys", `I inspected the diff and found no issues. {"foo":"bar"}`},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
_, err := finalizeTextResult("codex", tc.text, reviewSchema, TokenUsage{})
if err == nil {
t.Fatal("expected parse failure")
}
if errors.Is(err, errTurnEndedWithoutJSON) {
t.Fatalf("attempted-JSON failure must keep the raw parse error, got %v", err)
}
})
}
}
4 changes: 2 additions & 2 deletions internal/agent/antigravity.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ func (a *antigravityAgent) Name() string { return "antigravity" }
func (a *antigravityAgent) ReportsAgentAttempts() bool { return true }

func (a *antigravityAgent) Run(ctx context.Context, opts RunOpts) (*Result, error) {
return runWithRetry(ctx, "antigravity", opts, claudeMaxRetries, classifyTransient, nil, func() (*Result, error) {
return a.runOnce(ctx, opts)
return runWithRetry(ctx, "antigravity", opts, claudeMaxRetries, classifyTransient, nil, func(o RunOpts) (*Result, error) {
return a.runOnce(ctx, o)
})
}

Expand Down
4 changes: 2 additions & 2 deletions internal/agent/claude.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ func (a *claudeAgent) NeutralizesGateInstructions() bool {
}

func (a *claudeAgent) Run(ctx context.Context, opts RunOpts) (*Result, error) {
return runWithRetry(ctx, "claude", opts, claudeMaxRetries, claudeRetryClassifier, nil, func() (*Result, error) {
return a.runOnce(ctx, opts)
return runWithRetry(ctx, "claude", opts, claudeMaxRetries, claudeRetryClassifier, nil, func(o RunOpts) (*Result, error) {
return a.runOnce(ctx, o)
})
}

Expand Down
4 changes: 2 additions & 2 deletions internal/agent/codex.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ func (a *codexAgent) NeutralizesGateInstructions() bool {
}

func (a *codexAgent) Run(ctx context.Context, opts RunOpts) (*Result, error) {
return runWithRetry(ctx, "codex", opts, claudeMaxRetries, classifyTransient, nil, func() (*Result, error) {
return a.runOnce(ctx, opts)
return runWithRetry(ctx, "codex", opts, claudeMaxRetries, classifyTransient, nil, func(o RunOpts) (*Result, error) {
return a.runOnce(ctx, o)
})
}

Expand Down
4 changes: 2 additions & 2 deletions internal/agent/copilot.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ func (a *copilotAgent) Name() string { return "copilot" }
func (a *copilotAgent) ReportsAgentAttempts() bool { return true }

func (a *copilotAgent) Run(ctx context.Context, opts RunOpts) (*Result, error) {
return runWithRetry(ctx, "copilot", opts, claudeMaxRetries, classifyTransient, nil, func() (*Result, error) {
return a.runOnce(ctx, opts)
return runWithRetry(ctx, "copilot", opts, claudeMaxRetries, classifyTransient, nil, func(o RunOpts) (*Result, error) {
return a.runOnce(ctx, o)
})
}

Expand Down
4 changes: 2 additions & 2 deletions internal/agent/grok.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ func (a *grokAgent) NeutralizesGateInstructions() bool {
}

func (a *grokAgent) Run(ctx context.Context, opts RunOpts) (*Result, error) {
return runWithRetry(ctx, "grok", opts, claudeMaxRetries, classifyTransient, nil, func() (*Result, error) {
return a.runOnce(ctx, opts)
return runWithRetry(ctx, "grok", opts, claudeMaxRetries, classifyTransient, nil, func(o RunOpts) (*Result, error) {
return a.runOnce(ctx, o)
})
}

Expand Down
4 changes: 2 additions & 2 deletions internal/agent/opencode.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ func (a *opencodeAgent) Name() string { return "opencode" }
func (a *opencodeAgent) ReportsAgentAttempts() bool { return true }

func (a *opencodeAgent) Run(ctx context.Context, opts RunOpts) (*Result, error) {
return runWithRetry(ctx, "opencode", opts, claudeMaxRetries, classifyTransient, a.recoverTransientRetry, func() (*Result, error) {
return a.runOnce(ctx, opts)
return runWithRetry(ctx, "opencode", opts, claudeMaxRetries, classifyTransient, a.recoverTransientRetry, func(o RunOpts) (*Result, error) {
return a.runOnce(ctx, o)
})
}

Expand Down
4 changes: 2 additions & 2 deletions internal/agent/pi.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ func (a *piAgent) NeutralizesGateInstructions() bool {
}

func (a *piAgent) Run(ctx context.Context, opts RunOpts) (*Result, error) {
return runWithRetry(ctx, "pi", opts, claudeMaxRetries, classifyTransient, nil, func() (*Result, error) {
return a.runOnce(ctx, opts)
return runWithRetry(ctx, "pi", opts, claudeMaxRetries, classifyTransient, nil, func(o RunOpts) (*Result, error) {
return a.runOnce(ctx, o)
})
}

Expand Down
34 changes: 28 additions & 6 deletions internal/agent/retry.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,20 @@ func transientBackoffBaseDuration(attempt int, base time.Duration) time.Duration
return delay
}

// jsonResultReminder is appended to the prompt (exactly once per invocation)
// when a retry follows errTurnEndedWithoutJSON: the previous attempt ended
// with plain prose instead of the required structured result, so the
// re-invocation restates the final-message contract (#811).
const jsonResultReminder = "\n\nYour previous reply ended without the required JSON result. Return now: your final assistant message must be exactly one bare JSON object matching the given schema - no prose before or after it."

// runWithRetry invokes runOnce up to maxRetries+1 times, retrying when the
// classifier marks the error as retriable. Between retries it sleeps with
// exponential backoff (via transientBackoff) and respects ctx cancellation.
// The retry attempt and classification label are surfaced to opts.OnLifecycle,
// When the previous attempt failed with errTurnEndedWithoutJSON, the next
// attempt is invoked with a reminder appended to the prompt that the final
// message must be the bare JSON object; adapters that resume a durable
// session therefore re-invoke that same session with the reminder. The
// retry attempt and classification label are surfaced to opts.OnLifecycle,
// falling back to opts.OnChunk for older direct callers.
func runWithRetry(
ctx context.Context,
Expand All @@ -61,19 +71,24 @@ func runWithRetry(
maxRetries int,
classify retryClassifier,
recoverRetry func(label string),
runOnce func() (*Result, error),
runOnce func(RunOpts) (*Result, error),
) (*Result, error) {
var lastErr error
var lastLabel string
reminded := false
for attempt := 0; attempt <= maxRetries; attempt++ {
if attempt > 0 {
emitAgentRetry(opts, name, lastLabel, attempt+1, maxRetries+1)
if !reminded && errors.Is(lastErr, errTurnEndedWithoutJSON) {
opts.Prompt += jsonResultReminder
reminded = true
}
if err := transientBackoff(ctx, attempt); err != nil {
return nil, err
}
}
startedAt := time.Now()
result, err := runOnce()
result, err := runOnce(opts)
emitAgentAttempt(opts, name, result, err, startedAt, time.Now())
if err == nil {
return result, nil
Expand Down Expand Up @@ -146,16 +161,23 @@ var transientNeedles = []struct {
{"unexpected eof", "unexpected eof"},
}

// classifyTransient reports whether an error message looks like a transient
// API or network failure. It deliberately ignores ctx cancellation/deadline
// errors so explicit cancellation is never silently retried.
// classifyTransient reports whether an error looks like a transient API or
// network failure, or the prose-final-turn structured-output failure
// (errTurnEndedWithoutJSON), which is recoverable by re-invoking with a JSON
// reminder. It deliberately ignores ctx cancellation/deadline errors so
// explicit cancellation is never silently retried.
func classifyTransient(err error) (string, bool) {
if err == nil {
return "", false
}
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return "", false
}
// Identity check on purpose: only a real parse failure carrying this
// sentinel retries; the same sentence echoed in provider output must not.
if errors.Is(err, errTurnEndedWithoutJSON) {
return "missing JSON result", true
}
msg := strings.ToLower(err.Error())
for _, sig := range transientNeedles {
if strings.Contains(msg, sig.needle) {
Expand Down
Loading
Loading