fix(agent): replace raw Go errors with user-friendly messages in chat
Raw Go error chains (HTTP status codes, wrapped contexts, internal paths) were sent directly to end users in chat when LLM calls failed. This exposed internal details like API keys and file paths. Add userFriendlyError() that reuses the existing error_classifier.go patterns to map errors to actionable, plain-language messages: - Auth errors -> check API key / run auth login - Rate limits -> try again in a moment - Timeouts -> check internet connection - Billing -> check account balance - Format errors -> run picoclaw doctor Raw errors are still logged server-side via logger.ErrorCF. Fixes #564
This commit is contained in:
parent
2f4f45080b
commit
c0c6781461
3 changed files with 305 additions and 1 deletions
61
pkg/agent/errors.go
Normal file
61
pkg/agent/errors.go
Normal file
|
|
@ -0,0 +1,61 @@
|
||||||
|
package agent
|
||||||
|
|
||||||
|
import (
|
||||||
|
"errors"
|
||||||
|
|
||||||
|
"github.com/sipeed/picoclaw/pkg/providers"
|
||||||
|
)
|
||||||
|
|
||||||
|
// userFriendlyError converts a raw Go error into a message safe to display
|
||||||
|
// to end users in chat. Internal details (HTTP status codes, wrapped error
|
||||||
|
// chains, Go formatting) are replaced with actionable, plain-language
|
||||||
|
// guidance. The original error is still logged server-side by the caller.
|
||||||
|
func userFriendlyError(err error) string {
|
||||||
|
if err == nil {
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check if it's already a classified FailoverError from the fallback chain.
|
||||||
|
var foErr *providers.FailoverError
|
||||||
|
if errors.As(err, &foErr) {
|
||||||
|
return reasonToUserMessage(foErr.Reason)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Classify the raw error using the same patterns the fallback chain uses.
|
||||||
|
classified := providers.ClassifyError(err, "", "")
|
||||||
|
if classified != nil {
|
||||||
|
return reasonToUserMessage(classified.Reason)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Unclassified error -- return a generic message.
|
||||||
|
// Never expose the raw error string (it may contain API keys, internal
|
||||||
|
// paths, or Go stack traces).
|
||||||
|
return genericErrorMessage
|
||||||
|
}
|
||||||
|
|
||||||
|
// reasonToUserMessage maps a FailoverReason to a user-facing message.
|
||||||
|
func reasonToUserMessage(reason providers.FailoverReason) string {
|
||||||
|
switch reason {
|
||||||
|
case providers.FailoverAuth:
|
||||||
|
return "I couldn't authenticate with the AI provider. " +
|
||||||
|
"Please check your API key in ~/.picoclaw/config.json or run 'picoclaw auth login'."
|
||||||
|
case providers.FailoverRateLimit:
|
||||||
|
return "The AI provider is rate-limiting requests. Please try again in a moment."
|
||||||
|
case providers.FailoverBilling:
|
||||||
|
return "The AI provider rejected the request due to billing. " +
|
||||||
|
"Please check your account balance or plan."
|
||||||
|
case providers.FailoverTimeout:
|
||||||
|
return "The request to the AI provider timed out. " +
|
||||||
|
"Please check your internet connection and try again."
|
||||||
|
case providers.FailoverOverloaded:
|
||||||
|
return "The AI provider is currently overloaded. Please try again in a moment."
|
||||||
|
case providers.FailoverFormat:
|
||||||
|
return "The request format was rejected by the AI provider. " +
|
||||||
|
"Run 'picoclaw doctor' to diagnose."
|
||||||
|
default:
|
||||||
|
return genericErrorMessage
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
const genericErrorMessage = "Something went wrong processing your message. " +
|
||||||
|
"Run 'picoclaw doctor' to diagnose."
|
||||||
241
pkg/agent/errors_test.go
Normal file
241
pkg/agent/errors_test.go
Normal file
|
|
@ -0,0 +1,241 @@
|
||||||
|
package agent
|
||||||
|
|
||||||
|
import (
|
||||||
|
"errors"
|
||||||
|
"fmt"
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"github.com/sipeed/picoclaw/pkg/providers"
|
||||||
|
)
|
||||||
|
|
||||||
|
func TestUserFriendlyError_NilError(t *testing.T) {
|
||||||
|
result := userFriendlyError(nil)
|
||||||
|
if result != "" {
|
||||||
|
t.Errorf("expected empty string for nil error, got %q", result)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestUserFriendlyError_AuthErrors(t *testing.T) {
|
||||||
|
cases := []string{
|
||||||
|
"API request failed:\n Status: 401\n Body: {\"error\":{\"code\":\"401\",\"message\":\"Unauthorized\"}}",
|
||||||
|
"API request failed:\n Status: 403\n Body: Forbidden",
|
||||||
|
"invalid api key provided",
|
||||||
|
"token has expired",
|
||||||
|
"no credentials found for anthropic",
|
||||||
|
"oauth token refresh failed",
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, errMsg := range cases {
|
||||||
|
result := userFriendlyError(errors.New(errMsg))
|
||||||
|
if result == "" {
|
||||||
|
t.Errorf("expected non-empty result for auth error %q", errMsg)
|
||||||
|
}
|
||||||
|
if result == genericErrorMessage {
|
||||||
|
t.Errorf("expected auth-specific message for %q, got generic", errMsg)
|
||||||
|
}
|
||||||
|
// Should mention API key or auth login
|
||||||
|
if !contains(result, "API key") && !contains(result, "auth") && !contains(result, "authenticate") {
|
||||||
|
t.Errorf("expected auth guidance in message for %q, got %q", errMsg, result)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestUserFriendlyError_RateLimitErrors(t *testing.T) {
|
||||||
|
cases := []string{
|
||||||
|
"rate limit exceeded",
|
||||||
|
"too many requests",
|
||||||
|
"API request failed:\n Status: 429\n Body: rate limited",
|
||||||
|
"exceeded your current quota",
|
||||||
|
"resource_exhausted",
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, errMsg := range cases {
|
||||||
|
result := userFriendlyError(errors.New(errMsg))
|
||||||
|
if result == genericErrorMessage {
|
||||||
|
t.Errorf("expected rate-limit-specific message for %q, got generic", errMsg)
|
||||||
|
}
|
||||||
|
if !contains(result, "rate") && !contains(result, "try again") {
|
||||||
|
t.Errorf("expected rate-limit guidance in message for %q, got %q", errMsg, result)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestUserFriendlyError_TimeoutErrors(t *testing.T) {
|
||||||
|
cases := []string{
|
||||||
|
"failed to send request: context deadline exceeded (Client.Timeout exceeded while awaiting headers)",
|
||||||
|
"timeout waiting for response",
|
||||||
|
"request timed out",
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, errMsg := range cases {
|
||||||
|
result := userFriendlyError(errors.New(errMsg))
|
||||||
|
if result == genericErrorMessage {
|
||||||
|
t.Errorf("expected timeout-specific message for %q, got generic", errMsg)
|
||||||
|
}
|
||||||
|
if !contains(result, "timed out") && !contains(result, "internet") {
|
||||||
|
t.Errorf("expected timeout guidance in message for %q, got %q", errMsg, result)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestUserFriendlyError_BillingErrors(t *testing.T) {
|
||||||
|
cases := []string{
|
||||||
|
"API request failed:\n Status: 402\n Body: Payment Required",
|
||||||
|
"insufficient credits",
|
||||||
|
"insufficient balance",
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, errMsg := range cases {
|
||||||
|
result := userFriendlyError(errors.New(errMsg))
|
||||||
|
if result == genericErrorMessage {
|
||||||
|
t.Errorf("expected billing-specific message for %q, got generic", errMsg)
|
||||||
|
}
|
||||||
|
if !contains(result, "billing") && !contains(result, "balance") && !contains(result, "plan") {
|
||||||
|
t.Errorf("expected billing guidance in message for %q, got %q", errMsg, result)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestUserFriendlyError_FormatErrors(t *testing.T) {
|
||||||
|
cases := []string{
|
||||||
|
"string should match pattern",
|
||||||
|
"invalid request format",
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, errMsg := range cases {
|
||||||
|
result := userFriendlyError(errors.New(errMsg))
|
||||||
|
if result == genericErrorMessage {
|
||||||
|
t.Errorf("expected format-specific message for %q, got generic", errMsg)
|
||||||
|
}
|
||||||
|
if !contains(result, "doctor") {
|
||||||
|
t.Errorf("expected doctor guidance in message for %q, got %q", errMsg, result)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestUserFriendlyError_UnknownErrors(t *testing.T) {
|
||||||
|
cases := []string{
|
||||||
|
"some completely random internal error",
|
||||||
|
"unexpected nil pointer dereference",
|
||||||
|
"goroutine stack overflow",
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, errMsg := range cases {
|
||||||
|
result := userFriendlyError(errors.New(errMsg))
|
||||||
|
if result != genericErrorMessage {
|
||||||
|
t.Errorf("expected generic message for unclassified error %q, got %q", errMsg, result)
|
||||||
|
}
|
||||||
|
// Must NOT contain the raw error text
|
||||||
|
if contains(result, errMsg) {
|
||||||
|
t.Errorf("generic message should not contain raw error text %q", errMsg)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestUserFriendlyError_FailoverError(t *testing.T) {
|
||||||
|
// When the fallback chain wraps errors in FailoverError, we should
|
||||||
|
// still produce a user-friendly message.
|
||||||
|
foErr := &providers.FailoverError{
|
||||||
|
Reason: providers.FailoverAuth,
|
||||||
|
Provider: "openai",
|
||||||
|
Model: "gpt-4",
|
||||||
|
Wrapped: errors.New("status 401: Unauthorized"),
|
||||||
|
}
|
||||||
|
|
||||||
|
result := userFriendlyError(foErr)
|
||||||
|
if result == genericErrorMessage {
|
||||||
|
t.Errorf("expected auth-specific message for FailoverError, got generic")
|
||||||
|
}
|
||||||
|
if !contains(result, "API key") && !contains(result, "authenticate") {
|
||||||
|
t.Errorf("expected auth guidance for FailoverError, got %q", result)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestUserFriendlyError_WrappedErrors(t *testing.T) {
|
||||||
|
// Errors wrapped with fmt.Errorf %w should still be classified
|
||||||
|
inner := errors.New("rate limit exceeded")
|
||||||
|
wrapped := fmt.Errorf("LLM call failed after retries: %w", inner)
|
||||||
|
|
||||||
|
result := userFriendlyError(wrapped)
|
||||||
|
if result == genericErrorMessage {
|
||||||
|
t.Errorf("expected rate-limit message for wrapped error, got generic")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestUserFriendlyError_NeverLeaksRawError(t *testing.T) {
|
||||||
|
// The key security property: raw error text should never appear
|
||||||
|
// in the user-facing message.
|
||||||
|
sensitiveErrors := []string{
|
||||||
|
"API request failed:\n Status: 401\n Body: {\"error\":{\"code\":\"401\"}}",
|
||||||
|
"failed to send request: dial tcp 10.0.0.1:443: connect: connection refused",
|
||||||
|
"codex API call: stream ended without completed response",
|
||||||
|
"loading auth credentials: open /root/.picoclaw/auth.json: permission denied",
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, errMsg := range sensitiveErrors {
|
||||||
|
result := userFriendlyError(errors.New(errMsg))
|
||||||
|
// The result should NOT contain any of the raw technical details
|
||||||
|
if contains(result, "Status:") ||
|
||||||
|
contains(result, "dial tcp") ||
|
||||||
|
contains(result, "stream ended") ||
|
||||||
|
contains(result, "/root/") ||
|
||||||
|
contains(result, "permission denied") ||
|
||||||
|
contains(result, "connection refused") {
|
||||||
|
t.Errorf("user-friendly message leaked raw error for %q: got %q", errMsg, result)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestReasonToUserMessage_AllReasons(t *testing.T) {
|
||||||
|
reasons := []providers.FailoverReason{
|
||||||
|
providers.FailoverAuth,
|
||||||
|
providers.FailoverRateLimit,
|
||||||
|
providers.FailoverBilling,
|
||||||
|
providers.FailoverTimeout,
|
||||||
|
providers.FailoverOverloaded,
|
||||||
|
providers.FailoverFormat,
|
||||||
|
providers.FailoverUnknown,
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, reason := range reasons {
|
||||||
|
msg := reasonToUserMessage(reason)
|
||||||
|
if msg == "" {
|
||||||
|
t.Errorf("expected non-empty message for reason %q", reason)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// contains is a case-insensitive helper for test assertions.
|
||||||
|
func contains(s, substr string) bool {
|
||||||
|
return len(s) >= len(substr) &&
|
||||||
|
len(substr) > 0 &&
|
||||||
|
(s == substr || containsLower(s, substr))
|
||||||
|
}
|
||||||
|
|
||||||
|
func containsLower(s, substr string) bool {
|
||||||
|
for i := 0; i <= len(s)-len(substr); i++ {
|
||||||
|
if eqFoldSlice(s[i:i+len(substr)], substr) {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
|
||||||
|
func eqFoldSlice(a, b string) bool {
|
||||||
|
if len(a) != len(b) {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
for i := 0; i < len(a); i++ {
|
||||||
|
ca, cb := a[i], b[i]
|
||||||
|
if ca >= 'A' && ca <= 'Z' {
|
||||||
|
ca += 'a' - 'A'
|
||||||
|
}
|
||||||
|
if cb >= 'A' && cb <= 'Z' {
|
||||||
|
cb += 'a' - 'A'
|
||||||
|
}
|
||||||
|
if ca != cb {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
|
@ -188,7 +188,9 @@ func (al *AgentLoop) Run(ctx context.Context) error {
|
||||||
|
|
||||||
response, err := al.processMessage(ctx, msg)
|
response, err := al.processMessage(ctx, msg)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
response = fmt.Sprintf("Error processing message: %v", err)
|
logger.ErrorCF("agent", "Error processing message",
|
||||||
|
map[string]any{"error": err.Error()})
|
||||||
|
response = userFriendlyError(err)
|
||||||
}
|
}
|
||||||
|
|
||||||
if response != "" {
|
if response != "" {
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue