fix(attachments): download real bytes and hint --type for bucket-scoped recordings

internal/commands/attachments.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -16,12 +17,26 @@ import (
 	"charm.land/lipgloss/v2"
 	"github.com/spf13/cobra"
 
+	"github.com/basecamp/basecamp-sdk/go/pkg/basecamp"
+
 	"github.com/basecamp/basecamp-cli/internal/appctx"
 	"github.com/basecamp/basecamp-cli/internal/output"
 	"github.com/basecamp/basecamp-cli/internal/richtext"
 	"github.com/basecamp/basecamp-cli/internal/urlarg"
 )
 
+// genericLookupTypeHint is the usage hint emitted when the generic
+// /recordings/<id>.json lookup cannot resolve the recording — either
+// because the endpoint returns 204 (some recording types) or 404 (cards,
+// and any other type whose recording is not addressable without bucket
+// scope). Callers need to specify --type (or pass a URL, from which the
+// type is parsed) so the typed endpoint can be used instead.
+//
+// Kept command-agnostic: fetchItemContent is shared by `attachments list`
+// and `attachments download`, so the hint must not tell a download caller
+// to re-run the list command (or vice versa).
+const genericLookupTypeHint = "Re-run with --type todo|todolist|message|comment|card|card-table|document|schedule-entry|checkin|answer|forward|upload, or pass a URL (which encodes the type)"
+
 // NewAttachmentsCmd creates the attachments command group.
 func NewAttachmentsCmd() *cobra.Command {
 	cmd := &cobra.Command{
@@ -401,6 +416,20 @@ func isGenericType(recordType string) bool {
 	}
 }
 
+// shouldSuggestType reports whether the --type usage hint is appropriate
+// for the current recordType. It fires only when no explicit type was
+// provided. The "line"/"lines"/"replies" aliases route through the same
+// generic /recordings lookup for parent discovery, but the user *did*
+// pass --type — telling those callers to "specify --type" is misleading.
+func shouldSuggestType(recordType string) bool {
+	switch recordType {
+	case "", "recording", "recordings":
+		return true
+	default:
+		return false
+	}
+}
+
 // fetchItemContent retrieves the HTML content field from a Basecamp item.
 // Uses the same recording-type discovery pattern as show.go.
 func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string) (string, error) {
@@ -416,13 +445,29 @@ func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string
 
 	resp, err := app.Account().Get(cmd.Context(), endpoint)
 	if err != nil {
+		// The generic /recordings/<id>.json endpoint returns 404 for
+		// recording types that require bucket scope (e.g. Kanban::Card).
+		// Convert to the same usage hint the 204 branch below emits so
+		// the user is told to pass --type instead of seeing a bare
+		// "Resource not found". Only fires when no explicit type was
+		// provided — a 404 under --type line|replies means the ID is
+		// wrong, not that --type is missing.
+		if shouldSuggestType(recordType) {
+			var sdkErr *basecamp.Error
+			if errors.As(err, &sdkErr) && sdkErr.Code == basecamp.CodeNotFound {
+				return "", output.ErrUsageHint(
+					fmt.Sprintf("Item %s not found or type required", id),
+					genericLookupTypeHint,
+				)
+			}
+		}
 		return "", convertSDKError(err)
 	}
 	if resp.StatusCode == http.StatusNoContent {
-		if isGenericType(recordType) {
+		if shouldSuggestType(recordType) {
 			return "", output.ErrUsageHint(
 				fmt.Sprintf("Item %s not found or type required", id),
-				"Specify a type: basecamp attachments list <id> --type todo|todolist|message|comment|card|card-table|document|schedule-entry|checkin|answer|forward|upload",
+				genericLookupTypeHint,
 			)
 		}
 		return "", output.ErrNotFound("item", id)

internal/commands/attachments_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"os"
 	"path/filepath"
 	"strings"
@@ -13,6 +14,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/basecamp/basecamp-cli/internal/appctx"
+	"github.com/basecamp/basecamp-cli/internal/output"
 	"github.com/basecamp/basecamp-cli/internal/richtext"
 )
 
@@ -300,16 +302,16 @@ func TestWithAttachmentMeta(t *testing.T) {
 func TestAttachmentMeta(t *testing.T) {
 	t.Run("uses DisplayURL", func(t *testing.T) {
 		atts := []richtext.ParsedAttachment{
-			{URL: "https://example.com/a.png", Href: "https://example.com/a-href.png", Filename: "a.png", ContentType: "image/png", Filesize: "1024"},
-			{Href: "https://example.com/b.txt"},
+			{URL: "https://preview.example.com/a-preview", Href: "https://storage.example.com/download/a.png", Filename: "a.png", ContentType: "image/png", Filesize: "1024"},
+			{URL: "https://preview.example.com/b-preview"},
 		}
 		result := attachmentMeta(atts, nil)
 		assert.Len(t, result, 2)
 		assert.Equal(t, "a.png", result[0]["filename"])
 		assert.Equal(t, "image/png", result[0]["content_type"])
 		assert.Equal(t, "1024", result[0]["filesize"])
-		assert.Equal(t, "https://example.com/a.png", result[0]["url"])
-		assert.Equal(t, "https://example.com/b.txt", result[1]["url"])
+		assert.Equal(t, "https://storage.example.com/download/a.png", result[0]["url"])
+		assert.Equal(t, "https://preview.example.com/b-preview", result[1]["url"])
 		_, hasFilename := result[1]["filename"]
 		assert.False(t, hasFilename)
 	})
@@ -509,3 +511,77 @@ func TestFetchItemContentRefetchesLineWithLargeParentID(t *testing.T) {
 	assert.Contains(t, reqs[0], "/recordings/111.json")
 	assert.Contains(t, reqs[1], "/chats/"+largeID+"/lines/111.json")
 }
+
+// TestAttachmentsList404GenericPathSuggestsType verifies that when the
+// generic /recordings/<id>.json endpoint returns 404 and no --type is
+// provided, the CLI surfaces the "Specify a type" usage hint rather than
+// a bare "Resource not found". Cards are the concrete case: their
+// recording is only addressable via the bucket-scoped card_tables
+// endpoint, so the generic lookup returns 404.
+func TestAttachmentsList404GenericPathSuggestsType(t *testing.T) {
+	const cardID = "12345"
+
+	transport := &showTrackingTransport{
+		responder: func(path string) (int, string) {
+			if strings.Contains(path, "/recordings/"+cardID) {
+				return 404, `{"error":"not found"}`
+			}
+			return 200, `{}`
+		},
+	}
+	app := showTestApp(t, transport)
+
+	cmd := NewAttachmentsCmd()
+	cmd.SetArgs([]string{"list", cardID})
+	ctx := appctx.WithApp(context.Background(), app)
+	cmd.SetContext(ctx)
+	cmd.SetOut(&bytes.Buffer{})
+	cmd.SetErr(&bytes.Buffer{})
+
+	err := cmd.Execute()
+	require.Error(t, err)
+
+	var e *output.Error
+	require.True(t, errors.As(err, &e), "expected output.Error, got %T: %v", err, err)
+	assert.Contains(t, e.Message, cardID)
+	assert.Contains(t, e.Message, "type required")
+	assert.Contains(t, e.Hint, "--type")
+	assert.Contains(t, e.Hint, "card")
+}
+
+// TestAttachmentsList404WithExplicitTypeDoesNotSuggestType verifies the
+// hint gate is narrow: when the user already passed --type line (or any
+// non-suggest-appropriate type that routes through the generic /recordings
+// lookup for parent discovery), a 404 must not produce the "Specify a
+// type" hint — they already did.
+func TestAttachmentsList404WithExplicitTypeDoesNotSuggestType(t *testing.T) {
+	const lineID = "12345"
+
+	transport := &showTrackingTransport{
+		responder: func(path string) (int, string) {
+			if strings.Contains(path, "/recordings/"+lineID) {
+				return 404, `{"error":"not found"}`
+			}
+			return 200, `{}`
+		},
+	}
+	app := showTestApp(t, transport)
+
+	cmd := NewAttachmentsCmd()
+	cmd.SetArgs([]string{"list", lineID, "--type", "line"})
+	ctx := appctx.WithApp(context.Background(), app)
+	cmd.SetContext(ctx)
+	cmd.SetOut(&bytes.Buffer{})
+	cmd.SetErr(&bytes.Buffer{})
+
+	err := cmd.Execute()
+	require.Error(t, err)
+
+	var e *output.Error
+	if errors.As(err, &e) {
+		assert.NotContains(t, e.Hint, "--type",
+			"line callers already provided --type; hint would be misleading")
+		assert.NotContains(t, e.Message, "type required",
+			"should not claim type is required when it was provided")
+	}
+}

internal/richtext/richtext.go

@@ -1248,9 +1248,16 @@ func (a *ParsedAttachment) DisplayName() string {
 }
 
 // DisplayURL returns the best available URL for the attachment.
+// Href is preferred because it points at the real blob download endpoint
+// (storage.3.basecamp.com/.../download/<filename>). URL points at the
+// preview endpoint (preview.3.basecamp.com/.../previews/full), which for
+// non-image content types returns a generic SVG file-type icon instead of
+// the real file. Every internal caller is a download path, so preferring
+// Href is correct; URL is retained as a fallback for the rare case where
+// an attachment has no downloadable blob (e.g. externally hosted images).
 func (a *ParsedAttachment) DisplayURL() string {
-	if a.URL != "" {
-		return a.URL
+	if a.Href != "" {
+		return a.Href
 	}
-	return a.Href
+	return a.URL
 }

internal/richtext/richtext_test.go

@@ -2200,8 +2200,9 @@ func TestParsedAttachmentDisplayURL(t *testing.T) {
 		att      ParsedAttachment
 		expected string
 	}{
-		{"URL wins", ParsedAttachment{URL: "https://a.com", Href: "https://b.com"}, "https://a.com"},
-		{"href fallback", ParsedAttachment{Href: "https://b.com"}, "https://b.com"},
+		{"Href wins over preview URL", ParsedAttachment{URL: "https://preview.example.com/icon", Href: "https://storage.example.com/download/file.csv"}, "https://storage.example.com/download/file.csv"},
+		{"URL fallback when Href missing", ParsedAttachment{URL: "https://preview.example.com/icon"}, "https://preview.example.com/icon"},
+		{"Href only", ParsedAttachment{Href: "https://storage.example.com/download/file.csv"}, "https://storage.example.com/download/file.csv"},
 		{"empty", ParsedAttachment{}, ""},
 	}
 	for _, tt := range tests {