From dacdddbfa1f6386ada70d2eaa6d225ef9bf1661a Mon Sep 17 00:00:00 2001 From: bilogic <946010+bilogic@users.noreply.github.com> Date: Sun, 2 Aug 2026 09:30:05 +0800 Subject: [PATCH] =?UTF-8?q?don't=20react=20with=20=F0=9F=92=94=20on=20tele?= =?UTF-8?q?gram=20(#1492)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/agent/loop_pipeline_callbacks.go | 17 ++++++ .../agent/loop_pipeline_callbacks_test.go | 56 +++++++++++++++++++ internal/channels/errors.go | 28 ++++++++++ internal/channels/errors_test.go | 31 ++++++++++ internal/channels/events.go | 13 ++++- 5 files changed, 144 insertions(+), 1 deletion(-) diff --git a/internal/agent/loop_pipeline_callbacks.go b/internal/agent/loop_pipeline_callbacks.go index 6c9e0773..d6a0973c 100644 --- a/internal/agent/loop_pipeline_callbacks.go +++ b/internal/agent/loop_pipeline_callbacks.go @@ -7,6 +7,7 @@ import ( "fmt" "log/slog" "slices" + "strconv" "strings" "time" @@ -369,6 +370,22 @@ func (l *Loop) makeCallLLM(req *RunRequest, emitRun func(AgentEvent)) func(ctx c provider := state.Provider model := state.Model + // Issue 3: surface transient provider retries to the user ("Provider busy, + // retrying...") instead of a silent failure ending in a 💔 reaction. The + // providers' internal RetryDo / codex loops fire this hook before each retry + // attempt; the channel layer turns run.retrying into a placeholder update. + ctx = providers.WithRetryHook(ctx, func(attempt, maxAttempts int, _ error) { + emitRun(AgentEvent{ + Type: protocol.AgentEventRunRetrying, + AgentID: l.id, + RunID: req.RunID, + Payload: map[string]string{ + "attempt": strconv.Itoa(attempt), + "maxAttempts": strconv.Itoa(maxAttempts), + }, + }) + }) + // Enrich ChatRequest options to match v2 (providers need these for caching, routing, audit). if chatReq.Options == nil { chatReq.Options = make(map[string]any) diff --git a/internal/agent/loop_pipeline_callbacks_test.go b/internal/agent/loop_pipeline_callbacks_test.go index 8f4e0765..21072bce 100644 --- a/internal/agent/loop_pipeline_callbacks_test.go +++ b/internal/agent/loop_pipeline_callbacks_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/google/uuid" @@ -55,6 +56,61 @@ func (s *recordingSessionStore) AddMessage(_ context.Context, _ string, msg prov s.added = append(s.added, msg) } +// retryOnceProvider fails the first Chat call with a transient 429 via RetryDo +// (which fires the injected retry hook), then succeeds — mimicking a flaky +// provider whose internal RetryDo consumes the run's retry hook. +type retryOnceProvider struct { + calls int + cfg providers.RetryConfig +} + +func (p *retryOnceProvider) Chat(ctx context.Context, _ providers.ChatRequest) (*providers.ChatResponse, error) { + return providers.RetryDo(ctx, p.cfg, func() (*providers.ChatResponse, error) { + p.calls++ + if p.calls == 1 { + return nil, &providers.HTTPError{Status: 429, Body: "rate limited"} + } + return &providers.ChatResponse{Content: "ok"}, nil + }) +} + +func (p *retryOnceProvider) ChatStream(ctx context.Context, _ providers.ChatRequest, _ func(providers.StreamChunk)) (*providers.ChatResponse, error) { + return p.Chat(ctx, providers.ChatRequest{}) +} + +func (p *retryOnceProvider) DefaultModel() string { return "test-model" } +func (p *retryOnceProvider) Name() string { return "test-provider" } + +func TestMakeCallLLMEmitsRetryingOnTransientProviderError(t *testing.T) { + col := &eventCollector{} + loop := &Loop{id: "test-agent", onEvent: col.onEvent} + req := &RunRequest{RunID: "run-1", SessionKey: "sess-1", Channel: "telegram"} + state := &pipeline.RunState{ + Provider: &retryOnceProvider{ + cfg: providers.RetryConfig{Attempts: 2, MinDelay: time.Millisecond, MaxDelay: time.Millisecond}, + }, + Model: "test-model", + Iteration: 0, + } + + resp, err := loop.makeCallLLM(req, col.onEvent)(context.Background(), state, providers.ChatRequest{}) + if err != nil { + t.Fatalf("makeCallLLM returned error: %v", err) + } + if resp == nil || resp.Content != "ok" { + t.Fatalf("unexpected response: %+v", resp) + } + + retrying := col.filter(protocol.AgentEventRunRetrying) + if len(retrying) != 1 { + t.Fatalf("retrying events = %+v, want exactly one", retrying) + } + payload, ok := retrying[0].Payload.(map[string]string) + if !ok || payload["attempt"] != "1" || payload["maxAttempts"] != "2" { + t.Fatalf("retrying payload = %+v, want attempt=1 maxAttempts=2", retrying[0].Payload) + } +} + func TestMakeCallLLM_StreamsFinalThinkingWhenNoThinkingChunkArrives(t *testing.T) { col := &eventCollector{} loop := &Loop{id: "test-agent", onEvent: col.onEvent} diff --git a/internal/channels/errors.go b/internal/channels/errors.go index b33fe7f9..ee9a476f 100644 --- a/internal/channels/errors.go +++ b/internal/channels/errors.go @@ -1,11 +1,39 @@ package channels import ( + "errors" "strings" "github.com/nextlevelbuilder/goclaw/internal/providers" ) +// isTransientFailureMessage reports whether an agent run failure string is a +// transient provider error (rate limit, timeout, overload, connection drop) +// that the user can simply retry — as opposed to a permanent error (auth, +// context overflow). Used to decide whether a failure should surface a +// terminal 💔 reaction (see events.go reaction forwarding). +func isTransientFailureMessage(errStr string) bool { + if errStr == "" { + return false + } + lower := strings.ToLower(errStr) + // Reuse the provider retryability classifier (429/5xx typed errors, + // connection reset, timeouts, rate limits, EOF) on the raw message. + if providers.IsRetryableError(errors.New(errStr)) { + return true + } + // Mirror FormatAgentError's transient matchers (rate limit, timeout, + // overload) plus HTTP 5xx status codes, which IsRetryableError only matches + // on typed HTTPError values — the string seen here has been re-wrapped. + return strings.Contains(lower, "rate limit") || + strings.Contains(lower, "too many requests") || + strings.Contains(lower, "429") || + strings.Contains(lower, "timeout") || + strings.Contains(lower, "deadline exceeded") || + strings.Contains(lower, "overload") || + strings.Contains(lower, "http 5") +} + // FormatAgentError converts internal error to user-friendly message. // Issue 958: Send user-friendly error on RunFailed instead of silent "...". func FormatAgentError(errStr string) string { diff --git a/internal/channels/errors_test.go b/internal/channels/errors_test.go index 84d14a9f..ff7c79a7 100644 --- a/internal/channels/errors_test.go +++ b/internal/channels/errors_test.go @@ -63,3 +63,34 @@ func TestFormatAgentError_Empty(t *testing.T) { t.Errorf("expected empty string for empty error, got %q", result) } } + +func TestIsTransientFailureMessage(t *testing.T) { + t.Parallel() + transient := []string{ + "llm call: HTTP 429: rate limited", + "request timeout", + "deadline exceeded", + "service overloaded", + "connection reset by peer", + "llm call: HTTP 503: service unavailable", + "too many requests", + } + for _, tc := range transient { + if !isTransientFailureMessage(tc) { + t.Errorf("expected %q to be transient", tc) + } + } + + permanent := []string{ + "unauthorized access", + "invalid api key", + "context length exceeded", + "request context budget exceeded", + "", + } + for _, tc := range permanent { + if isTransientFailureMessage(tc) { + t.Errorf("expected %q to be permanent", tc) + } + } +} diff --git a/internal/channels/events.go b/internal/channels/events.go index 776cdfce..da8ea501 100644 --- a/internal/channels/events.go +++ b/internal/channels/events.go @@ -417,7 +417,18 @@ func (m *Manager) HandleAgentEvent(eventType, runID string, payload any) { case protocol.AgentEventRunCompleted: status = "done" case protocol.AgentEventRunFailed: - status = "error" + // Issue 3: transient provider failures (rate limit, timeout, overload) + // already surface a friendly retry message via FormatAgentError — a + // terminal 💔 reaction would be misleading. Clear any interim reaction + // instead; permanent errors keep the 💔. + if isTransientFailureMessage(extractPayloadString(payload, "error")) { + if err := reactionCh.ClearReaction(ctx, rc.ChatID, rc.MessageID); err != nil { + slog.Debug("reaction clear failed", "channel", rc.ChannelName, "error", err) + } + status = "" + } else { + status = "error" + } case protocol.AgentEventRunCancelled: status = "done" }