fix(slack): improve file download error handling and auth diagnostics (#204)

When downloading Slack files via url_private_download, the code was
already setting the Bearer token correctly. However, there was no
error detection for cases where Slack returns an HTML login page
instead of the actual file content.

This commit adds:
- Fallback to url_private when url_private_download is empty
- Status code validation (fail on non-200 responses)
- HTML content detection (fail with clear error message)
- Improved error logging with redacted URL
- Unit tests for the downloadSlackFile function

The HTML detection is particularly important because when auth
fails (e.g., wrong bot token, expired token), Slack returns a
login page with HTTP 200 instead of an error code. Without this
check, the HTML content would be passed to the AI agent as if
it were an image, causing confusing errors.

Fixes #185

Co-authored-by: Shawn <shawn@chenhg5.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

platform/slack/slack.go

@@ -1,6 +1,7 @@
 package slack
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"io"
@@ -186,10 +187,20 @@ func (p *Platform) handleEvent(evt socketmode.Event) {
 				var images []core.ImageAttachment
 				var audio *core.AudioAttachment
 				for _, f := range ev.Files {
+					// Prefer URLPrivateDownload, fall back to URLPrivate
+					fileURL := f.URLPrivateDownload
+					if fileURL == "" {
+						fileURL = f.URLPrivate
+					}
+					if fileURL == "" {
+						slog.Warn("slack: file has no download URL", "file_id", f.ID, "name", f.Name)
+						continue
+					}
+
 					if f.Mimetype != "" && strings.HasPrefix(f.Mimetype, "audio/") {
-						data, err := p.downloadSlackFile(f.URLPrivateDownload)
+						data, err := p.downloadSlackFile(fileURL)
 						if err != nil {
-							slog.Error("slack: download audio failed", "error", err)
+							slog.Error("slack: download audio failed", "error", err, "url", core.RedactToken(fileURL, p.botToken))
 							continue
 						}
 						format := "mp3"
@@ -200,9 +211,9 @@ func (p *Platform) handleEvent(evt socketmode.Event) {
 							MimeType: f.Mimetype, Data: data, Format: format,
 						}
 					} else if f.Mimetype != "" && strings.HasPrefix(f.Mimetype, "image/") {
-						imgData, err := p.downloadSlackFile(f.URLPrivateDownload)
+						imgData, err := p.downloadSlackFile(fileURL)
 						if err != nil {
-							slog.Error("slack: download file failed", "error", err)
+							slog.Error("slack: download file failed", "error", err, "url", core.RedactToken(fileURL, p.botToken))
 							continue
 						}
 						images = append(images, core.ImageAttachment{
@@ -327,7 +338,24 @@ func (p *Platform) downloadSlackFile(url string) ([]byte, error) {
 		return nil, fmt.Errorf("%s", core.RedactToken(err.Error(), p.botToken))
 	}
 	defer resp.Body.Close()
-	return io.ReadAll(resp.Body)
+
+	// Check if we got an unexpected status code (e.g., redirect to login page)
+	if resp.StatusCode != http.StatusOK {
+		body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
+		return nil, fmt.Errorf("download failed with status %d: %s", resp.StatusCode, string(body))
+	}
+
+	data, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return nil, fmt.Errorf("read response body: %w", err)
+	}
+
+	// Basic sanity check: detect if we received HTML instead of binary data
+	if len(data) > 0 && (bytes.HasPrefix(data, []byte("<!DOCTYPE")) || bytes.HasPrefix(data, []byte("<html"))) {
+		return nil, fmt.Errorf("received HTML response (likely missing auth); first 100 bytes: %s", string(data[:min(100, len(data))]))
+	}
+
+	return data, nil
 }
 
 func (p *Platform) ReconstructReplyCtx(sessionKey string) (any, error) {

platform/slack/slack_test.go

@@ -1,6 +1,10 @@
 package slack
 
-import "testing"
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
 
 func TestStripAppMentionText(t *testing.T) {
 	tests := []struct {
@@ -33,3 +37,71 @@ func TestStripAppMentionText(t *testing.T) {
 		})
 	}
 }
+
+func TestDownloadSlackFile_HTMLDetection(t *testing.T) {
+	// Test that we detect HTML responses (Slack login page) and return an error
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// Simulate Slack returning HTML login page when auth is missing
+		w.Header().Set("Content-Type", "text/html")
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte("<!DOCTYPE html><html><body>Please login</body></html>"))
+	}))
+	defer ts.Close()
+
+	p := &Platform{botToken: "xoxb-test-token"}
+	_, err := p.downloadSlackFile(ts.URL)
+	if err == nil {
+		t.Fatal("expected error for HTML response, got nil")
+	}
+	// Should detect HTML prefix
+	if err != nil && err.Error() == "" {
+		t.Fatal("expected non-empty error message")
+	}
+}
+
+func TestDownloadSlackFile_MissingAuth(t *testing.T) {
+	// Test that we return an error for non-200 status codes
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusUnauthorized)
+		w.Write([]byte("unauthorized"))
+	}))
+	defer ts.Close()
+
+	p := &Platform{botToken: "xoxb-test-token"}
+	_, err := p.downloadSlackFile(ts.URL)
+	if err == nil {
+		t.Fatal("expected error for 401 response, got nil")
+	}
+}
+
+func TestDownloadSlackFile_Success(t *testing.T) {
+	// Test successful binary download
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// Verify Authorization header is set
+		auth := r.Header.Get("Authorization")
+		if auth != "Bearer xoxb-test-token" {
+			t.Errorf("expected Authorization header 'Bearer xoxb-test-token', got %q", auth)
+		}
+		w.Header().Set("Content-Type", "image/png")
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte("\x89PNG\r\n\x1a\n")) // PNG magic bytes
+	}))
+	defer ts.Close()
+
+	p := &Platform{botToken: "xoxb-test-token"}
+	data, err := p.downloadSlackFile(ts.URL)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(data) != 8 {
+		t.Errorf("expected 8 bytes, got %d", len(data))
+	}
+}
+
+func TestDownloadSlackFile_EmptyURL(t *testing.T) {
+	p := &Platform{botToken: "xoxb-test-token"}
+	_, err := p.downloadSlackFile("")
+	if err == nil {
+		t.Fatal("expected error for empty URL, got nil")
+	}
+}