From 9cc7acb99107a0ac42a9a81d927d57b41b648708 Mon Sep 17 00:00:00 2001 From: Brian Miller Date: Mon, 16 Mar 2026 08:58:52 +0200 Subject: [PATCH 01/11] Add attachment listing for items Introduces `basecamp attachments list ` to enumerate file attachments embedded in Basecamp item rich text by parsing tags. Supports all recording types with auto-detection from URLs, styled TTY and JSON output, and context-aware breadcrumbs. Co-Authored-By: Claude Opus 4.6 --- .surface | 43 +++++ API-COVERAGE.md | 2 +- e2e/smoke/smoke_attachments.bats | 72 ++++++++ internal/cli/root.go | 1 + internal/commands/attachments.go | 257 +++++++++++++++++++++++++++++ internal/commands/commands.go | 1 + internal/commands/commands_test.go | 1 + internal/richtext/richtext.go | 95 +++++++++++ internal/richtext/richtext_test.go | 198 ++++++++++++++++++++++ skills/basecamp/SKILL.md | 1 + 10 files changed, 670 insertions(+), 1 deletion(-) create mode 100644 e2e/smoke/smoke_attachments.bats create mode 100644 internal/commands/attachments.go diff --git a/.surface b/.surface index e414cd55..d4e0f046 100644 --- a/.surface +++ b/.surface @@ -9,6 +9,8 @@ CMD basecamp api post CMD basecamp api put CMD basecamp assign CMD basecamp attach +CMD basecamp attachments +CMD basecamp attachments list CMD basecamp auth CMD basecamp auth login CMD basecamp auth logout @@ -534,6 +536,45 @@ FLAG basecamp attach --stats type=bool FLAG basecamp attach --styled type=bool FLAG basecamp attach --todolist type=string FLAG basecamp attach --verbose type=count +FLAG basecamp attachments --account type=string +FLAG basecamp attachments --agent type=bool +FLAG basecamp attachments --cache-dir type=string +FLAG basecamp attachments --count type=bool +FLAG basecamp attachments --hints type=bool +FLAG basecamp attachments --ids-only type=bool +FLAG basecamp attachments --jq type=string +FLAG basecamp attachments --json type=bool +FLAG basecamp attachments --markdown type=bool +FLAG basecamp attachments --md type=bool +FLAG basecamp attachments --no-hints type=bool +FLAG basecamp attachments --no-stats type=bool +FLAG basecamp attachments --profile type=string +FLAG basecamp attachments --project type=string +FLAG basecamp attachments --quiet type=bool +FLAG basecamp attachments --stats type=bool +FLAG basecamp attachments --styled type=bool +FLAG basecamp attachments --todolist type=string +FLAG basecamp attachments --verbose type=count +FLAG basecamp attachments list --account type=string +FLAG basecamp attachments list --agent type=bool +FLAG basecamp attachments list --cache-dir type=string +FLAG basecamp attachments list --count type=bool +FLAG basecamp attachments list --hints type=bool +FLAG basecamp attachments list --ids-only type=bool +FLAG basecamp attachments list --jq type=string +FLAG basecamp attachments list --json type=bool +FLAG basecamp attachments list --markdown type=bool +FLAG basecamp attachments list --md type=bool +FLAG basecamp attachments list --no-hints type=bool +FLAG basecamp attachments list --no-stats type=bool +FLAG basecamp attachments list --profile type=string +FLAG basecamp attachments list --project type=string +FLAG basecamp attachments list --quiet type=bool +FLAG basecamp attachments list --stats type=bool +FLAG basecamp attachments list --styled type=bool +FLAG basecamp attachments list --todolist type=string +FLAG basecamp attachments list --type type=string +FLAG basecamp attachments list --verbose type=count FLAG basecamp auth --account type=string FLAG basecamp auth --agent type=bool FLAG basecamp auth --cache-dir type=string @@ -7113,6 +7154,8 @@ SUB basecamp api post SUB basecamp api put SUB basecamp assign SUB basecamp attach +SUB basecamp attachments +SUB basecamp attachments list SUB basecamp auth SUB basecamp auth login SUB basecamp auth logout diff --git a/API-COVERAGE.md b/API-COVERAGE.md index 7028b434..bc72c7b8 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -47,7 +47,7 @@ Out-of-scope sections are excluded from parity totals and scripts: chatbots (dif | uploads | 8 | `files`, `uploads` | ✅ | - | list, show | | vaults | 8 | `files`, `vaults` | ✅ | - | list, show, create | | documents | 8 | `files`, `docs` | ✅ | - | list, show, create, update. Create supports `--subscribe`/`--no-subscribe` | -| attachments | 1 | `uploads` | ✅ | - | Attachment metadata | +| attachments | 1 | `uploads`, `attachments` | ✅ | - | Upload via `attach`; list embedded attachments via `attachments list` (parses `` from content) | | **Schedule** | | schedules | 2 | `schedule` | ✅ | - | Schedule container + settings | | schedule_entries | 5 | `schedule` | ✅ | - | list, show, create, update, occurrences. Create supports `--subscribe`/`--no-subscribe` | diff --git a/e2e/smoke/smoke_attachments.bats b/e2e/smoke/smoke_attachments.bats new file mode 100644 index 00000000..06011696 --- /dev/null +++ b/e2e/smoke/smoke_attachments.bats @@ -0,0 +1,72 @@ +#!/usr/bin/env bats +# smoke_attachments.bats - Level 1: Attachment listing on items + +load smoke_helper + +setup_file() { + ensure_token || return 1 + ensure_project || return 1 + ensure_todolist || return 1 +} + +@test "attachments without subcommand shows help" { + run basecamp attachments + assert_success + assert_output_contains "list" +} + +@test "attachments list on comment with attachment shows results" { + # Create a todo to comment on + local todo_out + todo_out=$(basecamp todo "Attach test $(date +%s)" --list "$QA_TODOLIST" \ + -p "$QA_PROJECT" --json 2>/dev/null) || { + mark_unverifiable "Cannot create todo for attachment test" + return + } + local todo_id + todo_id=$(echo "$todo_out" | jq -r '.data.id // empty') + [[ -n "$todo_id" ]] || mark_unverifiable "No todo ID returned" + + # Create a small test file and attach it via comment + local tmpfile="$BATS_FILE_TMPDIR/test-attachment.txt" + echo "smoke test content $(date +%s)" > "$tmpfile" + basecamp comment "$todo_id" "See attached file" --attach "$tmpfile" \ + -p "$QA_PROJECT" --json 2>/dev/null || { + mark_unverifiable "Cannot create comment with attachment" + return + } + + # Get the comment ID (most recent comment on the todo) + local comments_out + comments_out=$(basecamp comments list "$todo_id" -p "$QA_PROJECT" --json 2>/dev/null) || { + mark_unverifiable "Cannot list comments" + return + } + local comment_id + comment_id=$(echo "$comments_out" | jq -r '.data[-1].id // empty') + [[ -n "$comment_id" ]] || mark_unverifiable "No comment ID found" + + # List attachments on the comment + run_smoke basecamp attachments list "$comment_id" --type comment --json + assert_success + assert_json_value '.ok' 'true' + assert_json_value '.data | length > 0' 'true' +} + +@test "attachments list on item with no attachments returns empty" { + # Create a plain todo with no attachments + local todo_out + todo_out=$(basecamp todo "No attach $(date +%s)" --list "$QA_TODOLIST" \ + -p "$QA_PROJECT" --json 2>/dev/null) || { + mark_unverifiable "Cannot create todo" + return + } + local todo_id + todo_id=$(echo "$todo_out" | jq -r '.data.id // empty') + [[ -n "$todo_id" ]] || mark_unverifiable "No todo ID returned" + + run_smoke basecamp attachments list "$todo_id" --type todo --json + assert_success + assert_json_value '.ok' 'true' + assert_json_value '.data' '[]' +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 86bd0cea..407b5e62 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -290,6 +290,7 @@ func Execute() { cmd.AddCommand(commands.NewMigrateCmd()) cmd.AddCommand(commands.NewProfileCmd()) cmd.AddCommand(commands.NewSkillCmd()) + cmd.AddCommand(commands.NewAttachmentsCmd()) cmd.AddCommand(commands.NewAttachCmd()) cmd.AddCommand(commands.NewUploadCmd()) cmd.AddCommand(commands.NewTUICmd()) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go new file mode 100644 index 00000000..581fee18 --- /dev/null +++ b/internal/commands/attachments.go @@ -0,0 +1,257 @@ +package commands + +import ( + "encoding/json" + "fmt" + "net/http" + "strings" + + "charm.land/lipgloss/v2" + "github.com/spf13/cobra" + + "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" +) + +// NewAttachmentsCmd creates the attachments command group (list). +func NewAttachmentsCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "attachments", + Short: "List attachments on items", + Long: "List file attachments embedded in Basecamp items (todos, messages, cards, comments, etc.).", + Annotations: map[string]string{"agent_notes": "Parses tags from item content\nWorks on any recording type — todos, messages, cards, comments\nUse --type to skip the generic recording lookup when you know the type"}, + } + + cmd.AddCommand(newAttachmentsListCmd()) + + return cmd +} + +func newAttachmentsListCmd() *cobra.Command { + var recordType string + + cmd := &cobra.Command{ + Use: "list ", + Short: "List attachments on an item", + Long: `List all file attachments embedded in an item's content. + +Attachments are extracted from the item's rich text content (HTML). This works +on any item type: todos, messages, cards, comments, documents, etc. + +You can pass either an ID or a Basecamp URL: + basecamp attachments list 789 + basecamp attachments list https://3.basecamp.com/123/buckets/456/todos/789`, + Example: ` basecamp attachments list 789 + basecamp attachments list 789 --type todo + basecamp attachments list https://3.basecamp.com/123/buckets/456/todos/789 --json`, + RunE: func(cmd *cobra.Command, args []string) error { + if len(args) == 0 { + return missingArg(cmd, "") + } + return runAttachmentsList(cmd, args[0], recordType) + }, + } + + cmd.Flags().StringVarP(&recordType, "type", "t", "", "Item type hint (todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, forward, upload)") + + return cmd +} + +func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { + app := appctx.FromContext(cmd.Context()) + + id := extractID(arg) + + // Auto-detect type from URL if not specified (mirrors show.go) + if parsed := urlarg.Parse(arg); parsed != nil { + if parsed.RecordingID != "" { + id = parsed.RecordingID + } + if recordType == "" && parsed.Type != "" { + recordType = parsed.Type + } + } + + // Validate type early (before account check) for better error messages + if !isGenericType(recordType) && typeToEndpoint(recordType, id) == "" { + return output.ErrUsageHint( + fmt.Sprintf("Unknown type: %s", recordType), + "Supported: todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, forward, upload", + ) + } + + if err := ensureAccount(cmd, app); err != nil { + return err + } + + // Fetch the item — same two-step pattern as show.go: + // 1. Generic recording lookup to discover type + // 2. Re-fetch via type-specific endpoint for full content + content, err := fetchItemContent(cmd, app, id, recordType) + if err != nil { + return err + } + + attachments := richtext.ParseAttachments(content) + + // Styled TTY output + if app.Output.EffectiveFormat() == output.FormatStyled { + w := cmd.OutOrStdout() + if len(attachments) == 0 { + fmt.Fprintln(w, "No attachments found.") + return nil + } + + bold := lipgloss.NewStyle().Bold(true).Foreground(lipgloss.Color("#888")) + fmt.Fprintln(w, bold.Render("Attachments:")) + + for i, a := range attachments { + icon := "\U0001F4CE" // 📎 + if a.IsImage() { + icon = "\U0001F5BC\uFE0F" // 🖼️ + } + + line := fmt.Sprintf(" %d. %s %s", i+1, icon, a.DisplayName()) + if a.IsImage() && a.Width != "" && a.Height != "" { + line += fmt.Sprintf(" (%s\u00D7%s)", a.Width, a.Height) + } + fmt.Fprintln(w, line) + + if url := a.DisplayURL(); url != "" { + fmt.Fprintf(w, " URL: %s\n", url) + } + } + return nil + } + + // JSON/agent output + showCmd := fmt.Sprintf("basecamp show %s", id) + if !isGenericType(recordType) { + showCmd = fmt.Sprintf("basecamp show %s --type %s", id, recordType) + } + + breadcrumbs := []output.Breadcrumb{ + {Action: "show", Cmd: showCmd, Description: "Show item"}, + } + // Only suggest commenting when the target isn't itself a comment. + if recordType != "comment" && recordType != "comments" { + breadcrumbs = append(breadcrumbs, output.Breadcrumb{ + Action: "comment", + Cmd: fmt.Sprintf("basecamp comment %s ", id), + Description: "Add comment", + }) + } + + respOpts := []output.ResponseOption{ + output.WithEntity("attachment"), + output.WithSummary(fmt.Sprintf("%d attachment(s) on #%s", len(attachments), id)), + output.WithBreadcrumbs(breadcrumbs...), + } + + return app.OK(attachments, respOpts...) +} + +// isGenericType returns true when the record type should use the generic +// recordings endpoint (with auto-discovery re-fetch), mirroring show.go. +func isGenericType(recordType string) bool { + return recordType == "" || recordType == "recording" || recordType == "recordings" +} + +// 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) { + // If type is provided, go directly to the type-specific endpoint + var endpoint string + if !isGenericType(recordType) { + endpoint = typeToEndpoint(recordType, id) + if endpoint == "" { + return "", output.ErrUsageHint( + fmt.Sprintf("Unknown type: %s", recordType), + "Supported: todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, forward, upload", + ) + } + } else { + // Generic recording lookup first + endpoint = fmt.Sprintf("/recordings/%s.json", id) + } + + resp, err := app.Account().Get(cmd.Context(), endpoint) + if err != nil { + return "", convertSDKError(err) + } + if resp.StatusCode == http.StatusNoContent { + if isGenericType(recordType) { + return "", output.ErrUsageHint( + fmt.Sprintf("Item %s not found or type required", id), + "Specify a type: basecamp attachments list --type todo|todolist|message|comment|card|card-table|document|schedule-entry|checkin|forward|upload", + ) + } + return "", output.ErrNotFound("item", id) + } + + var data map[string]any + if err := json.Unmarshal(resp.Data, &data); err != nil { + return "", fmt.Errorf("failed to parse response: %w", err) + } + + // If we used generic recording, re-fetch via type-specific endpoint for full content + if isGenericType(recordType) { + if refetchEndpoint := recordingTypeEndpoint(data, id); refetchEndpoint != "" { + refetchResp, refetchErr := app.Account().Get(cmd.Context(), refetchEndpoint) + if refetchErr == nil && refetchResp.StatusCode != http.StatusNoContent { + var richer map[string]any + if json.Unmarshal(refetchResp.Data, &richer) == nil { + data = richer + } + } + } + } + + // Extract content — different item types use different field names + return extractContentField(data), nil +} + +// extractContentField tries multiple field names to find the HTML content. +// Todos use "description", most others use "content". +func extractContentField(data map[string]any) string { + var parts []string + for _, key := range []string{"content", "description"} { + if v, ok := data[key].(string); ok && v != "" { + parts = append(parts, v) + } + } + return strings.Join(parts, "\n") +} + +// typeToEndpoint maps a user-provided type string to the API endpoint. +// Kept in sync with show.go's isValidRecordType / recordingTypeEndpoint. +func typeToEndpoint(recordType, id string) string { + switch recordType { + case "todo", "todos": + return fmt.Sprintf("/todos/%s.json", id) + case "todolist", "todolists": + return fmt.Sprintf("/todolists/%s.json", id) + case "message", "messages": + return fmt.Sprintf("/messages/%s.json", id) + case "comment", "comments": + return fmt.Sprintf("/comments/%s.json", id) + case "card", "cards": + return fmt.Sprintf("/card_tables/cards/%s.json", id) + case "card-table", "card_table", "cardtable", "card_tables": + return fmt.Sprintf("/card_tables/%s.json", id) + case "document", "documents": + return fmt.Sprintf("/documents/%s.json", id) + case "schedule-entry", "schedule_entry", "schedule_entries": + return fmt.Sprintf("/schedule_entries/%s.json", id) + case "checkin", "check-in", "check_in", "questions": + return fmt.Sprintf("/questions/%s.json", id) + case "forward", "forwards": + return fmt.Sprintf("/forwards/%s.json", id) + case "upload", "uploads": + return fmt.Sprintf("/uploads/%s.json", id) + default: + return "" + } +} diff --git a/internal/commands/commands.go b/internal/commands/commands.go index 552e3408..961c4fdf 100644 --- a/internal/commands/commands.go +++ b/internal/commands/commands.go @@ -96,6 +96,7 @@ func CommandCategories() []CommandCategory { {Name: "forwards", Category: "communication", Description: "Manage email forwards (inbox)", Actions: []string{"list", "show", "inbox", "replies", "reply"}}, {Name: "subscriptions", Category: "communication", Description: "Manage notification subscriptions", Actions: []string{"show", "subscribe", "unsubscribe", "add", "remove"}}, {Name: "comments", Category: "communication", Description: "Manage comments", Actions: []string{"create", "list", "show", "update", "trash", "archive", "restore"}}, + {Name: "attachments", Category: "communication", Description: "List attachments on items", Actions: []string{"list"}}, {Name: "boost", Category: "communication", Description: "Manage boosts (reactions)", Actions: []string{"list", "show", "create", "delete"}}, }, }, diff --git a/internal/commands/commands_test.go b/internal/commands/commands_test.go index 7ee6abef..ed66873f 100644 --- a/internal/commands/commands_test.go +++ b/internal/commands/commands_test.go @@ -112,6 +112,7 @@ func buildRootWithAllCommands() *cobra.Command { root.AddCommand(commands.NewDoctorCmd()) root.AddCommand(commands.NewUpgradeCmd()) root.AddCommand(commands.NewMigrateCmd()) + root.AddCommand(commands.NewAttachmentsCmd()) root.AddCommand(commands.NewAttachCmd()) root.AddCommand(commands.NewUploadCmd()) root.AddCommand(commands.NewSkillCmd()) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 956f816f..3a66a0f0 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -1022,3 +1022,98 @@ func IsHTML(s string) bool { return reSafeTag.MatchString(stripped) } + +// ParsedAttachment holds metadata extracted from a tag in HTML content. +type ParsedAttachment struct { + SGID string `json:"sgid,omitempty"` + Filename string `json:"filename,omitempty"` + ContentType string `json:"content_type,omitempty"` + URL string `json:"url,omitempty"` + Href string `json:"href,omitempty"` + Width string `json:"width,omitempty"` + Height string `json:"height,omitempty"` + Caption string `json:"caption,omitempty"` +} + +// reBcAttachmentTag matches tags, both self-closing and wrapped. +// Group 1 captures the attributes string. +var reBcAttachmentTag = regexp.MustCompile(`(?si)]*)(?:>.*?|/>)`) + +// ParseAttachments extracts file attachment metadata from HTML content. +// It finds all tags and returns their metadata, excluding +// mention attachments (content-type="application/vnd.basecamp.mention"). +func ParseAttachments(html string) []ParsedAttachment { + matches := reBcAttachmentTag.FindAllStringSubmatch(html, -1) + attachments := make([]ParsedAttachment, 0, len(matches)) + + for _, match := range matches { + if len(match) < 2 { + continue + } + attrs := match[1] + + contentType := extractAttr(attrs, "content-type") + if contentType == "application/vnd.basecamp.mention" { + continue + } + + attachments = append(attachments, ParsedAttachment{ + SGID: extractAttr(attrs, "sgid"), + Filename: extractAttr(attrs, "filename"), + ContentType: contentType, + URL: extractAttr(attrs, "url"), + Href: extractAttr(attrs, "href"), + Width: extractAttr(attrs, "width"), + Height: extractAttr(attrs, "height"), + Caption: extractAttr(attrs, "caption"), + }) + } + + return attachments +} + +// reAttrValue matches any HTML attribute as name="value" or name='value'. +// Group 1 = attribute name, group 2 = double-quoted value, group 3 = single-quoted value. +var reAttrValue = regexp.MustCompile(`(?:\s|^)([\w-]+)\s*=\s*(?:"([^"]*)"|'([^']*)')`) + +// extractAttr extracts the value of an HTML attribute from an attribute string. +// Handles both double-quoted and single-quoted values independently so that +// an apostrophe inside a double-quoted value (or vice versa) is not treated +// as a delimiter. The attribute name must match as a whole word to avoid +// partial matches (e.g. "url" won't match "data-url"). +func extractAttr(attrs, name string) string { + for _, m := range reAttrValue.FindAllStringSubmatch(attrs, -1) { + if m[1] != name { + continue + } + if m[3] != "" { + return m[3] + } + return m[2] + } + return "" +} + +// IsImage returns true if the attachment has an image content type. +func (a *ParsedAttachment) IsImage() bool { + return strings.HasPrefix(a.ContentType, "image/") +} + +// DisplayName returns the best display name: caption, then filename, then fallback. +func (a *ParsedAttachment) DisplayName() string { + if a.Caption != "" { + return a.Caption + } + if a.Filename != "" { + return a.Filename + } + return "Unnamed attachment" +} + +// DisplayURL returns the best available URL for the attachment. +func (a *ParsedAttachment) DisplayURL() string { + if a.URL != "" { + return a.URL + } + return a.Href +} diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index 087b90d1..0f21a481 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -1290,3 +1290,201 @@ func TestResolveMentions_ErrMentionSkip(t *testing.T) { } }) } + +func TestParseAttachments(t *testing.T) { + tests := []struct { + name string + html string + expected int + check func(*testing.T, []ParsedAttachment) + }{ + { + name: "no attachments", + html: `

Just some regular HTML content

`, + expected: 0, + }, + { + name: "empty string", + html: "", + expected: 0, + }, + { + name: "single image with nested figure", + html: ` +
My photo
+`, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + a := atts[0] + if a.SGID != "BAh7CEkiCG" { + t.Errorf("SGID = %q, want BAh7CEkiCG", a.SGID) + } + if a.ContentType != "image/jpeg" { + t.Errorf("ContentType = %q, want image/jpeg", a.ContentType) + } + if a.Filename != "photo.jpg" { + t.Errorf("Filename = %q, want photo.jpg", a.Filename) + } + if a.Caption != "My photo" { + t.Errorf("Caption = %q, want My photo", a.Caption) + } + if a.Width != "2560" || a.Height != "1536" { + t.Errorf("Dimensions = %sx%s, want 2560x1536", a.Width, a.Height) + } + if !a.IsImage() { + t.Error("IsImage() = false, want true") + } + if a.DisplayName() != "My photo" { + t.Errorf("DisplayName() = %q, want My photo", a.DisplayName()) + } + }, + }, + { + name: "multiple attachments", + html: `
+ + +
`, + expected: 2, + check: func(t *testing.T, atts []ParsedAttachment) { + if atts[0].Filename != "first.png" { + t.Errorf("first filename = %q, want first.png", atts[0].Filename) + } + if atts[1].Filename != "second.gif" { + t.Errorf("second filename = %q, want second.gif", atts[1].Filename) + } + }, + }, + { + name: "self-closing tag", + html: ``, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + a := atts[0] + if a.ContentType != "application/pdf" { + t.Errorf("ContentType = %q, want application/pdf", a.ContentType) + } + if a.IsImage() { + t.Error("IsImage() = true, want false for PDF") + } + }, + }, + { + name: "mentions filtered out", + html: `@Jane +`, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + if atts[0].Filename != "real.png" { + t.Errorf("Filename = %q, want real.png", atts[0].Filename) + } + }, + }, + { + name: "apostrophe in double-quoted attribute value", + html: ``, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + a := atts[0] + if a.Filename != "Brian's Report.jpg" { + t.Errorf("Filename = %q, want %q", a.Filename, "Brian's Report.jpg") + } + if a.Caption != "It's done" { + t.Errorf("Caption = %q, want %q", a.Caption, "It's done") + } + }, + }, + { + name: "single-quoted attribute values", + html: ``, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + a := atts[0] + if a.SGID != "SQ1" { + t.Errorf("SGID = %q, want SQ1", a.SGID) + } + if a.Filename != "single.png" { + t.Errorf("Filename = %q, want single.png", a.Filename) + } + }, + }, + { + name: "url attr not confused with data-url", + html: ``, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + a := atts[0] + if a.URL != "https://right.com" { + t.Errorf("URL = %q, want https://right.com", a.URL) + } + }, + }, + { + name: "missing attributes handled gracefully", + html: ``, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + a := atts[0] + if a.SGID != "BARE" { + t.Errorf("SGID = %q, want BARE", a.SGID) + } + if a.Filename != "" { + t.Errorf("Filename = %q, want empty", a.Filename) + } + if a.DisplayName() != "Unnamed attachment" { + t.Errorf("DisplayName() = %q, want Unnamed attachment", a.DisplayName()) + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + atts := ParseAttachments(tt.html) + if len(atts) != tt.expected { + t.Fatalf("got %d attachments, want %d", len(atts), tt.expected) + } + if tt.check != nil { + tt.check(t, atts) + } + }) + } +} + +func TestParsedAttachmentDisplayName(t *testing.T) { + tests := []struct { + name string + att ParsedAttachment + expected string + }{ + {"caption wins", ParsedAttachment{Caption: "My Caption", Filename: "file.jpg"}, "My Caption"}, + {"filename fallback", ParsedAttachment{Filename: "document.pdf"}, "document.pdf"}, + {"unnamed fallback", ParsedAttachment{}, "Unnamed attachment"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.att.DisplayName(); got != tt.expected { + t.Errorf("DisplayName() = %q, want %q", got, tt.expected) + } + }) + } +} + +func TestParsedAttachmentDisplayURL(t *testing.T) { + tests := []struct { + name string + 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"}, + {"empty", ParsedAttachment{}, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.att.DisplayURL(); got != tt.expected { + t.Errorf("DisplayURL() = %q, want %q", got, tt.expected) + } + }) + } +} diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 1c049f40..f48ba75f 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -157,6 +157,7 @@ basecamp --page 1 # First page only, no auto-pagination | Post silently | `basecamp message "Title" "Body" --no-subscribe --in --json` | | Post to chat | `basecamp chat post "Message" --in --json` | | Add comment | `basecamp comment "Text" --in --json` | +| List attachments | `basecamp attachments list --json` | | Search | `basecamp search "query" --json` | | Parse URL | `basecamp url parse "" --json` | | Upload file | `basecamp files uploads create [--vault ] --in --json` | From 0272557cc499e4084fd622115e328c6044d0e884 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 17 Mar 2026 17:53:19 -0700 Subject: [PATCH 02/11] Decode HTML entities in attachment attribute values extractAttr returned raw HTML-encoded strings, so filenames like O'Brien displayed with visible entities. Apply html.UnescapeString on the read path to match the encoding done by escapeAttr on writes. --- internal/richtext/richtext.go | 5 +++-- internal/richtext/richtext_test.go | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 3a66a0f0..fa88c269 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -5,6 +5,7 @@ package richtext import ( "errors" "fmt" + "html" "regexp" "slices" "strconv" @@ -1087,9 +1088,9 @@ func extractAttr(attrs, name string) string { continue } if m[3] != "" { - return m[3] + return html.UnescapeString(m[3]) } - return m[2] + return html.UnescapeString(m[2]) } return "" } diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index 0f21a481..b4445725 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -1419,6 +1419,20 @@ func TestParseAttachments(t *testing.T) { } }, }, + { + name: "HTML entities decoded in attributes", + html: ``, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + a := atts[0] + if a.Filename != "O'Brien & Co.png" { + t.Errorf("Filename = %q, want %q", a.Filename, "O'Brien & Co.png") + } + if a.URL != "https://example.com/file?a=1&b=2" { + t.Errorf("URL = %q, want decoded URL", a.URL) + } + }, + }, { name: "missing attributes handled gracefully", html: ``, From dc5e982f952b4b8183dbbdf44d51837bb57d439e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 17 Mar 2026 17:53:51 -0700 Subject: [PATCH 03/11] Prefer comment ID when resolving attachment list URLs Comment URLs like .../todos/111#__recording_789 were using the parent recording ID (111) instead of the comment ID (789). Follow the established convention from comment.go and prefer CommentID when the URL fragment is present, auto-setting type to "comment". --- internal/commands/attachments.go | 10 ++++++-- internal/commands/attachments_test.go | 35 +++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 internal/commands/attachments_test.go diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index 581fee18..a3aa3890 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -64,9 +64,15 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { id := extractID(arg) - // Auto-detect type from URL if not specified (mirrors show.go) + // Auto-detect type from URL if not specified (mirrors show.go). + // Prefer CommentID when present (comment.go convention). if parsed := urlarg.Parse(arg); parsed != nil { - if parsed.RecordingID != "" { + if parsed.CommentID != "" { + id = parsed.CommentID + if recordType == "" { + recordType = "comment" + } + } else if parsed.RecordingID != "" { id = parsed.RecordingID } if recordType == "" && parsed.Type != "" { diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go new file mode 100644 index 00000000..de3d9fc8 --- /dev/null +++ b/internal/commands/attachments_test.go @@ -0,0 +1,35 @@ +package commands + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/basecamp/basecamp-cli/internal/urlarg" +) + +func TestAttachmentsCommentURLPrefersCommentID(t *testing.T) { + // A comment URL with a #__recording_NNN fragment should resolve to the + // comment's recording ID, not the parent item's recording ID. + parsed := urlarg.Parse("https://3.basecamp.com/123/buckets/456/todos/111#__recording_789") + assert.NotNil(t, parsed) + + // Simulate the command's URL resolution logic: prefer CommentID. + var id, recordType string + if parsed.CommentID != "" { + id = parsed.CommentID + recordType = "comment" + } else if parsed.RecordingID != "" { + id = parsed.RecordingID + } + + assert.Equal(t, "789", id) + assert.Equal(t, "comment", recordType) +} + +func TestAttachmentsURLWithoutComment(t *testing.T) { + parsed := urlarg.Parse("https://3.basecamp.com/123/buckets/456/todos/111") + assert.NotNil(t, parsed) + assert.Equal(t, "", parsed.CommentID) + assert.Equal(t, "111", parsed.RecordingID) +} From bc5d48d1e1ef27105bd767653ba52c9a3efe9629 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 17 Mar 2026 17:54:09 -0700 Subject: [PATCH 04/11] Support question_answers type in attachment listing Check-in answer URLs auto-detect as type "question_answers" but typeToEndpoint had no case for it, causing "Unknown type" errors. Add "answer" and "question_answers" aliases and update all hint strings. --- internal/commands/attachments.go | 10 ++++++---- internal/commands/attachments_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index a3aa3890..6acaf8c1 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -54,7 +54,7 @@ You can pass either an ID or a Basecamp URL: }, } - cmd.Flags().StringVarP(&recordType, "type", "t", "", "Item type hint (todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, forward, upload)") + cmd.Flags().StringVarP(&recordType, "type", "t", "", "Item type hint (todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, answer, forward, upload)") return cmd } @@ -84,7 +84,7 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { if !isGenericType(recordType) && typeToEndpoint(recordType, id) == "" { return output.ErrUsageHint( fmt.Sprintf("Unknown type: %s", recordType), - "Supported: todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, forward, upload", + "Supported: todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, answer, forward, upload", ) } @@ -175,7 +175,7 @@ func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string if endpoint == "" { return "", output.ErrUsageHint( fmt.Sprintf("Unknown type: %s", recordType), - "Supported: todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, forward, upload", + "Supported: todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, answer, forward, upload", ) } } else { @@ -191,7 +191,7 @@ func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string if isGenericType(recordType) { return "", output.ErrUsageHint( fmt.Sprintf("Item %s not found or type required", id), - "Specify a type: basecamp attachments list --type todo|todolist|message|comment|card|card-table|document|schedule-entry|checkin|forward|upload", + "Specify a type: basecamp attachments list --type todo|todolist|message|comment|card|card-table|document|schedule-entry|checkin|answer|forward|upload", ) } return "", output.ErrNotFound("item", id) @@ -251,6 +251,8 @@ func typeToEndpoint(recordType, id string) string { return fmt.Sprintf("/documents/%s.json", id) case "schedule-entry", "schedule_entry", "schedule_entries": return fmt.Sprintf("/schedule_entries/%s.json", id) + case "answer", "question_answers": + return fmt.Sprintf("/question_answers/%s.json", id) case "checkin", "check-in", "check_in", "questions": return fmt.Sprintf("/questions/%s.json", id) case "forward", "forwards": diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go index de3d9fc8..a15b0b64 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -33,3 +33,29 @@ func TestAttachmentsURLWithoutComment(t *testing.T) { assert.Equal(t, "", parsed.CommentID) assert.Equal(t, "111", parsed.RecordingID) } + +func TestTypeToEndpointAnswerAliases(t *testing.T) { + assert.Equal(t, "/question_answers/42.json", typeToEndpoint("answer", "42")) + assert.Equal(t, "/question_answers/42.json", typeToEndpoint("question_answers", "42")) + assert.Equal(t, "", typeToEndpoint("question_answer", "42")) +} + +func TestTypeToEndpointKnownTypes(t *testing.T) { + tests := []struct { + typ string + expected string + }{ + {"todo", "/todos/1.json"}, + {"comment", "/comments/1.json"}, + {"message", "/messages/1.json"}, + {"document", "/documents/1.json"}, + {"upload", "/uploads/1.json"}, + {"forward", "/forwards/1.json"}, + {"bogus", ""}, + } + for _, tt := range tests { + t.Run(tt.typ, func(t *testing.T) { + assert.Equal(t, tt.expected, typeToEndpoint(tt.typ, "1")) + }) + } +} From 2752a87d4c9c6fcb94694ef68f4c6856c76c753f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 18 Mar 2026 10:28:40 -0700 Subject: [PATCH 05/11] Address remaining PR review feedback - Guard CommentID preference on compatible recordType to avoid type/ID mismatch when --type is explicitly set - Add \b boundary to reBcAttachmentTag to prevent matching similarly-prefixed tag names like bc-attachment-foo - Use strings.EqualFold for mention content-type filtering and attribute name matching in extractAttr --- internal/commands/attachments.go | 4 ++-- internal/commands/attachments_test.go | 19 +++++++++++++++++++ internal/richtext/richtext.go | 6 +++--- internal/richtext/richtext_test.go | 16 ++++++++++++++++ 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index 6acaf8c1..e55c396f 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -65,9 +65,9 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { id := extractID(arg) // Auto-detect type from URL if not specified (mirrors show.go). - // Prefer CommentID when present (comment.go convention). + // Prefer CommentID when present and type is compatible (comment.go convention). if parsed := urlarg.Parse(arg); parsed != nil { - if parsed.CommentID != "" { + if parsed.CommentID != "" && (recordType == "" || recordType == "comment" || recordType == "comments") { id = parsed.CommentID if recordType == "" { recordType = "comment" diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go index a15b0b64..be6b1103 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -27,6 +27,25 @@ func TestAttachmentsCommentURLPrefersCommentID(t *testing.T) { assert.Equal(t, "comment", recordType) } +func TestAttachmentsCommentURLWithExplicitType(t *testing.T) { + // When --type is something other than comment, CommentID should NOT + // override the recording ID (avoids type/ID mismatch). + parsed := urlarg.Parse("https://3.basecamp.com/123/buckets/456/todos/111#__recording_789") + assert.NotNil(t, parsed) + + recordType := "todo" // explicit --type todo + var id string + if parsed.CommentID != "" && (recordType == "" || recordType == "comment" || recordType == "comments") { + id = parsed.CommentID + recordType = "comment" + } else if parsed.RecordingID != "" { + id = parsed.RecordingID + } + + assert.Equal(t, "111", id) + assert.Equal(t, "todo", recordType) +} + func TestAttachmentsURLWithoutComment(t *testing.T) { parsed := urlarg.Parse("https://3.basecamp.com/123/buckets/456/todos/111") assert.NotNil(t, parsed) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index fa88c269..ea331e11 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -1038,7 +1038,7 @@ type ParsedAttachment struct { // reBcAttachmentTag matches tags, both self-closing and wrapped. // Group 1 captures the attributes string. -var reBcAttachmentTag = regexp.MustCompile(`(?si)]*)(?:>.*?|/>)`) +var reBcAttachmentTag = regexp.MustCompile(`(?si)]*)(?:>.*?|/>)`) // ParseAttachments extracts file attachment metadata from HTML content. // It finds all tags and returns their metadata, excluding @@ -1054,7 +1054,7 @@ func ParseAttachments(html string) []ParsedAttachment { attrs := match[1] contentType := extractAttr(attrs, "content-type") - if contentType == "application/vnd.basecamp.mention" { + if strings.EqualFold(contentType, "application/vnd.basecamp.mention") { continue } @@ -1084,7 +1084,7 @@ var reAttrValue = regexp.MustCompile(`(?:\s|^)([\w-]+)\s*=\s*(?:"([^"]*)"|'([^'] // partial matches (e.g. "url" won't match "data-url"). func extractAttr(attrs, name string) string { for _, m := range reAttrValue.FindAllStringSubmatch(attrs, -1) { - if m[1] != name { + if !strings.EqualFold(m[1], name) { continue } if m[3] != "" { diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index b4445725..98b79a0e 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -1433,6 +1433,22 @@ func TestParseAttachments(t *testing.T) { } }, }, + { + name: "tag boundary prevents false match on bc-attachment-foo", + html: ``, + expected: 0, + }, + { + name: "case-insensitive mention filtering", + html: `@Jane +`, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + if atts[0].Filename != "real.png" { + t.Errorf("Filename = %q, want real.png", atts[0].Filename) + } + }, + }, { name: "missing attributes handled gracefully", html: ``, From fb8a269d6df4ee397eeaa1ea7d8eb0484cae764b Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 18 Mar 2026 10:41:54 -0700 Subject: [PATCH 06/11] Normalize breadcrumb types, fix parameter shadowing, case-insensitive IsImage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Normalize type aliases in show breadcrumb so copied commands work (e.g. question_answers → checkin, schedule_entries → schedule-entry) - Rename ParseAttachments parameter from html to content to avoid shadowing the html import - Make IsImage() case-insensitive with EqualFold prefix check --- internal/commands/attachments.go | 41 +++++++++++++++++++++++++-- internal/commands/attachments_test.go | 24 ++++++++++++++++ internal/richtext/richtext.go | 6 ++-- internal/richtext/richtext_test.go | 10 +++++++ 4 files changed, 76 insertions(+), 5 deletions(-) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index e55c396f..3ee7151a 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -134,8 +134,8 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { // JSON/agent output showCmd := fmt.Sprintf("basecamp show %s", id) - if !isGenericType(recordType) { - showCmd = fmt.Sprintf("basecamp show %s --type %s", id, recordType) + if showType := normalizeShowType(recordType); showType != "" { + showCmd = fmt.Sprintf("basecamp show %s --type %s", id, showType) } breadcrumbs := []output.Breadcrumb{ @@ -231,6 +231,43 @@ func extractContentField(data map[string]any) string { return strings.Join(parts, "\n") } +// normalizeShowType maps attachment type aliases to canonical types that +// basecamp show accepts. Returns "" for generic types (no --type needed). +func normalizeShowType(recordType string) string { + switch recordType { + case "": + return "" + case "todos": + return "todo" + case "todolists": + return "todolist" + case "messages": + return "message" + case "comments": + return "comment" + case "cards": + return "card" + case "card_tables": + return "card-table" + case "documents": + return "document" + case "schedule_entries": + return "schedule-entry" + case "answer", "question_answers": + return "checkin" + case "questions": + return "checkin" + case "forwards": + return "forward" + case "uploads": + return "upload" + case "recording", "recordings": + return "" + default: + return recordType + } +} + // typeToEndpoint maps a user-provided type string to the API endpoint. // Kept in sync with show.go's isValidRecordType / recordingTypeEndpoint. func typeToEndpoint(recordType, id string) string { diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go index be6b1103..e34216b9 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -59,6 +59,30 @@ func TestTypeToEndpointAnswerAliases(t *testing.T) { assert.Equal(t, "", typeToEndpoint("question_answer", "42")) } +func TestNormalizeShowType(t *testing.T) { + tests := []struct { + input string + expected string + }{ + {"", ""}, + {"todo", "todo"}, + {"todos", "todo"}, + {"question_answers", "checkin"}, + {"answer", "checkin"}, + {"questions", "checkin"}, + {"schedule_entries", "schedule-entry"}, + {"card_tables", "card-table"}, + {"recording", ""}, + {"recordings", ""}, + {"comment", "comment"}, + } + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + assert.Equal(t, tt.expected, normalizeShowType(tt.input)) + }) + } +} + func TestTypeToEndpointKnownTypes(t *testing.T) { tests := []struct { typ string diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index ea331e11..53782c88 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -1043,8 +1043,8 @@ var reBcAttachmentTag = regexp.MustCompile(`(?si)]*)(?:>.*?< // ParseAttachments extracts file attachment metadata from HTML content. // It finds all tags and returns their metadata, excluding // mention attachments (content-type="application/vnd.basecamp.mention"). -func ParseAttachments(html string) []ParsedAttachment { - matches := reBcAttachmentTag.FindAllStringSubmatch(html, -1) +func ParseAttachments(content string) []ParsedAttachment { + matches := reBcAttachmentTag.FindAllStringSubmatch(content, -1) attachments := make([]ParsedAttachment, 0, len(matches)) for _, match := range matches { @@ -1097,7 +1097,7 @@ func extractAttr(attrs, name string) string { // IsImage returns true if the attachment has an image content type. func (a *ParsedAttachment) IsImage() bool { - return strings.HasPrefix(a.ContentType, "image/") + return len(a.ContentType) >= 6 && strings.EqualFold(a.ContentType[:6], "image/") } // DisplayName returns the best display name: caption, then filename, then fallback. diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index 98b79a0e..83a8f7e7 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -1449,6 +1449,16 @@ func TestParseAttachments(t *testing.T) { } }, }, + { + name: "mixed-case image content type", + html: ``, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + if !atts[0].IsImage() { + t.Error("IsImage() = false for mixed-case Image/PNG, want true") + } + }, + }, { name: "missing attributes handled gracefully", html: ``, From 52ee05862e7d51444b687fd9632c39b31ff45a94 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 18 Mar 2026 11:23:40 -0700 Subject: [PATCH 07/11] Omit --type for answer/question_answers in show breadcrumb show has no type alias for question_answers, so --type checkin would fetch the parent question instead of the answer. Return "" to let generic recording lookup find the correct endpoint. --- internal/commands/attachments.go | 4 +++- internal/commands/attachments_test.go | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index 3ee7151a..13683d6b 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -254,7 +254,9 @@ func normalizeShowType(recordType string) string { case "schedule_entries": return "schedule-entry" case "answer", "question_answers": - return "checkin" + // show has no type alias for question_answers; omit --type + // so generic recording lookup finds the right endpoint. + return "" case "questions": return "checkin" case "forwards": diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go index e34216b9..60fa3239 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -67,8 +67,8 @@ func TestNormalizeShowType(t *testing.T) { {"", ""}, {"todo", "todo"}, {"todos", "todo"}, - {"question_answers", "checkin"}, - {"answer", "checkin"}, + {"question_answers", ""}, + {"answer", ""}, {"questions", "checkin"}, {"schedule_entries", "schedule-entry"}, {"card_tables", "card-table"}, From 40dcfc5455c4dc09733de045a125d2052f63a466 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 18 Mar 2026 12:38:15 -0700 Subject: [PATCH 08/11] Extract resolveAttachmentTarget helper, fix tag boundary, update comment - Extract URL/type resolution into resolveAttachmentTarget so tests exercise the actual function rather than duplicating logic inline - Replace \b with [\s/>] in reBcAttachmentTag since Go regex treats - as non-word, allowing to match - Update typeToEndpoint comment to note it's a superset of show.go --- internal/commands/attachments.go | 45 ++++++++----- internal/commands/attachments_test.go | 96 +++++++++++++++------------ internal/richtext/richtext.go | 2 +- 3 files changed, 81 insertions(+), 62 deletions(-) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index 13683d6b..da11a8b6 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -62,23 +62,7 @@ You can pass either an ID or a Basecamp URL: func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { app := appctx.FromContext(cmd.Context()) - id := extractID(arg) - - // Auto-detect type from URL if not specified (mirrors show.go). - // Prefer CommentID when present and type is compatible (comment.go convention). - if parsed := urlarg.Parse(arg); parsed != nil { - if parsed.CommentID != "" && (recordType == "" || recordType == "comment" || recordType == "comments") { - id = parsed.CommentID - if recordType == "" { - recordType = "comment" - } - } else if parsed.RecordingID != "" { - id = parsed.RecordingID - } - if recordType == "" && parsed.Type != "" { - recordType = parsed.Type - } - } + id, recordType := resolveAttachmentTarget(arg, recordType) // Validate type early (before account check) for better error messages if !isGenericType(recordType) && typeToEndpoint(recordType, id) == "" { @@ -231,6 +215,30 @@ func extractContentField(data map[string]any) string { return strings.Join(parts, "\n") } +// resolveAttachmentTarget extracts the recording ID and type from a raw +// argument (plain ID or Basecamp URL) and an optional explicit type hint. +// Prefers CommentID when present and type is compatible. +func resolveAttachmentTarget(arg, recordType string) (id, resolvedType string) { + id = extractID(arg) + resolvedType = recordType + + if parsed := urlarg.Parse(arg); parsed != nil { + if parsed.CommentID != "" && (resolvedType == "" || resolvedType == "comment" || resolvedType == "comments") { + id = parsed.CommentID + if resolvedType == "" { + resolvedType = "comment" + } + } else if parsed.RecordingID != "" { + id = parsed.RecordingID + } + if resolvedType == "" && parsed.Type != "" { + resolvedType = parsed.Type + } + } + + return id, resolvedType +} + // normalizeShowType maps attachment type aliases to canonical types that // basecamp show accepts. Returns "" for generic types (no --type needed). func normalizeShowType(recordType string) string { @@ -271,7 +279,8 @@ func normalizeShowType(recordType string) string { } // typeToEndpoint maps a user-provided type string to the API endpoint. -// Kept in sync with show.go's isValidRecordType / recordingTypeEndpoint. +// Superset of show.go's types — includes URL path segment aliases +// (e.g. question_answers, schedule_entries) that show.go doesn't accept. func typeToEndpoint(recordType, id string) string { switch recordType { case "todo", "todos": diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go index 60fa3239..5735f45a 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -4,53 +4,63 @@ import ( "testing" "github.com/stretchr/testify/assert" - - "github.com/basecamp/basecamp-cli/internal/urlarg" ) -func TestAttachmentsCommentURLPrefersCommentID(t *testing.T) { - // A comment URL with a #__recording_NNN fragment should resolve to the - // comment's recording ID, not the parent item's recording ID. - parsed := urlarg.Parse("https://3.basecamp.com/123/buckets/456/todos/111#__recording_789") - assert.NotNil(t, parsed) - - // Simulate the command's URL resolution logic: prefer CommentID. - var id, recordType string - if parsed.CommentID != "" { - id = parsed.CommentID - recordType = "comment" - } else if parsed.RecordingID != "" { - id = parsed.RecordingID +func TestResolveAttachmentTarget(t *testing.T) { + tests := []struct { + name string + arg string + typeHint string + wantID string + wantType string + }{ + { + name: "comment URL prefers CommentID", + arg: "https://3.basecamp.com/123/buckets/456/todos/111#__recording_789", + wantID: "789", + wantType: "comment", + }, + { + name: "comment URL with explicit --type comment", + arg: "https://3.basecamp.com/123/buckets/456/todos/111#__recording_789", + typeHint: "comment", + wantID: "789", + wantType: "comment", + }, + { + name: "comment URL with explicit --type todo uses RecordingID", + arg: "https://3.basecamp.com/123/buckets/456/todos/111#__recording_789", + typeHint: "todo", + wantID: "111", + wantType: "todo", + }, + { + name: "plain URL without comment fragment", + arg: "https://3.basecamp.com/123/buckets/456/todos/111", + wantID: "111", + wantType: "todos", + }, + { + name: "plain ID with no type", + arg: "42", + wantID: "42", + wantType: "", + }, + { + name: "plain ID with explicit type", + arg: "42", + typeHint: "message", + wantID: "42", + wantType: "message", + }, } - - assert.Equal(t, "789", id) - assert.Equal(t, "comment", recordType) -} - -func TestAttachmentsCommentURLWithExplicitType(t *testing.T) { - // When --type is something other than comment, CommentID should NOT - // override the recording ID (avoids type/ID mismatch). - parsed := urlarg.Parse("https://3.basecamp.com/123/buckets/456/todos/111#__recording_789") - assert.NotNil(t, parsed) - - recordType := "todo" // explicit --type todo - var id string - if parsed.CommentID != "" && (recordType == "" || recordType == "comment" || recordType == "comments") { - id = parsed.CommentID - recordType = "comment" - } else if parsed.RecordingID != "" { - id = parsed.RecordingID + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + id, typ := resolveAttachmentTarget(tt.arg, tt.typeHint) + assert.Equal(t, tt.wantID, id) + assert.Equal(t, tt.wantType, typ) + }) } - - assert.Equal(t, "111", id) - assert.Equal(t, "todo", recordType) -} - -func TestAttachmentsURLWithoutComment(t *testing.T) { - parsed := urlarg.Parse("https://3.basecamp.com/123/buckets/456/todos/111") - assert.NotNil(t, parsed) - assert.Equal(t, "", parsed.CommentID) - assert.Equal(t, "111", parsed.RecordingID) } func TestTypeToEndpointAnswerAliases(t *testing.T) { diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 53782c88..303f5850 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -1038,7 +1038,7 @@ type ParsedAttachment struct { // reBcAttachmentTag matches tags, both self-closing and wrapped. // Group 1 captures the attributes string. -var reBcAttachmentTag = regexp.MustCompile(`(?si)]*)(?:>.*?|/>)`) +var reBcAttachmentTag = regexp.MustCompile(`(?si)][^>]*)(?:>.*?|/>)`) // ParseAttachments extracts file attachment metadata from HTML content. // It finds all tags and returns their metadata, excluding From 09d3bd687e010dfae6d81931fcd5cba0fbe401c7 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 18 Mar 2026 13:25:30 -0700 Subject: [PATCH 09/11] Update surface snapshot with --in flag for attachments The --in persistent flag from main was missing from the attachments surface entries after the merge. --- .surface | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.surface b/.surface index d4e0f046..8cf6e9f7 100644 --- a/.surface +++ b/.surface @@ -542,6 +542,7 @@ FLAG basecamp attachments --cache-dir type=string FLAG basecamp attachments --count type=bool FLAG basecamp attachments --hints type=bool FLAG basecamp attachments --ids-only type=bool +FLAG basecamp attachments --in type=string FLAG basecamp attachments --jq type=string FLAG basecamp attachments --json type=bool FLAG basecamp attachments --markdown type=bool @@ -561,6 +562,7 @@ FLAG basecamp attachments list --cache-dir type=string FLAG basecamp attachments list --count type=bool FLAG basecamp attachments list --hints type=bool FLAG basecamp attachments list --ids-only type=bool +FLAG basecamp attachments list --in type=string FLAG basecamp attachments list --jq type=string FLAG basecamp attachments list --json type=bool FLAG basecamp attachments list --markdown type=bool From 8ec9dc6c65cec3fdfff6c735b73a64d1e7f24565 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 18 Mar 2026 13:46:04 -0700 Subject: [PATCH 10/11] Fix reBcAttachmentTag to match bare and uppercase tags The regex required at least one char in [\s/>] after the tag name, which failed on bare with no attributes. Switch to (\s[^>]*|) to allow empty attrs while still requiring whitespace before any attribute names (preventing bc-attachment-foo matches). Add tests for uppercase tag names and bare tags. --- internal/richtext/richtext.go | 2 +- internal/richtext/richtext_test.go | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 303f5850..9ddd95e0 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -1038,7 +1038,7 @@ type ParsedAttachment struct { // reBcAttachmentTag matches tags, both self-closing and wrapped. // Group 1 captures the attributes string. -var reBcAttachmentTag = regexp.MustCompile(`(?si)][^>]*)(?:>.*?|/>)`) +var reBcAttachmentTag = regexp.MustCompile(`(?si)]*|)(?:>.*?|/>)`) // ParseAttachments extracts file attachment metadata from HTML content. // It finds all tags and returns their metadata, excluding diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index 83a8f7e7..8cca0ddc 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -1459,6 +1459,21 @@ func TestParseAttachments(t *testing.T) { } }, }, + { + name: "uppercase tag name", + html: ``, + expected: 1, + check: func(t *testing.T, atts []ParsedAttachment) { + if atts[0].Filename != "upper.png" { + t.Errorf("Filename = %q, want upper.png", atts[0].Filename) + } + }, + }, + { + name: "bare tag with no attributes", + html: `content`, + expected: 1, + }, { name: "missing attributes handled gracefully", html: ``, From 98a20f8d2d842fa82309ad9dda0bdc84fb2f2e11 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 18 Mar 2026 14:16:31 -0700 Subject: [PATCH 11/11] Address review: NBSP normalization, variable shadowing, dedup validation - Normalize NBSP to regular space in extractAttr after html.UnescapeString - Rename shadowed recordType to resolvedType in runAttachmentsList - Remove duplicate type validation from fetchItemContent (caller validates) - Update SKILL.md to show instead of --- internal/commands/attachments.go | 21 ++++++++------------- internal/richtext/richtext.go | 6 ++++-- skills/basecamp/SKILL.md | 2 +- 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index da11a8b6..f029c67b 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -62,12 +62,12 @@ You can pass either an ID or a Basecamp URL: func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { app := appctx.FromContext(cmd.Context()) - id, recordType := resolveAttachmentTarget(arg, recordType) + id, resolvedType := resolveAttachmentTarget(arg, recordType) // Validate type early (before account check) for better error messages - if !isGenericType(recordType) && typeToEndpoint(recordType, id) == "" { + if !isGenericType(resolvedType) && typeToEndpoint(resolvedType, id) == "" { return output.ErrUsageHint( - fmt.Sprintf("Unknown type: %s", recordType), + fmt.Sprintf("Unknown type: %s", resolvedType), "Supported: todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, answer, forward, upload", ) } @@ -79,7 +79,7 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { // Fetch the item — same two-step pattern as show.go: // 1. Generic recording lookup to discover type // 2. Re-fetch via type-specific endpoint for full content - content, err := fetchItemContent(cmd, app, id, recordType) + content, err := fetchItemContent(cmd, app, id, resolvedType) if err != nil { return err } @@ -118,7 +118,7 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { // JSON/agent output showCmd := fmt.Sprintf("basecamp show %s", id) - if showType := normalizeShowType(recordType); showType != "" { + if showType := normalizeShowType(resolvedType); showType != "" { showCmd = fmt.Sprintf("basecamp show %s --type %s", id, showType) } @@ -126,7 +126,7 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { {Action: "show", Cmd: showCmd, Description: "Show item"}, } // Only suggest commenting when the target isn't itself a comment. - if recordType != "comment" && recordType != "comments" { + if resolvedType != "comment" && resolvedType != "comments" { breadcrumbs = append(breadcrumbs, output.Breadcrumb{ Action: "comment", Cmd: fmt.Sprintf("basecamp comment %s ", id), @@ -152,16 +152,11 @@ func isGenericType(recordType string) bool { // 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) { - // If type is provided, go directly to the type-specific endpoint + // If type is provided, go directly to the type-specific endpoint. + // Caller validates the type before calling, so typeToEndpoint won't return "". var endpoint string if !isGenericType(recordType) { endpoint = typeToEndpoint(recordType, id) - if endpoint == "" { - return "", output.ErrUsageHint( - fmt.Sprintf("Unknown type: %s", recordType), - "Supported: todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, answer, forward, upload", - ) - } } else { // Generic recording lookup first endpoint = fmt.Sprintf("/recordings/%s.json", id) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 9ddd95e0..ea207cc5 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -1087,10 +1087,12 @@ func extractAttr(attrs, name string) string { if !strings.EqualFold(m[1], name) { continue } + val := m[2] if m[3] != "" { - return html.UnescapeString(m[3]) + val = m[3] } - return html.UnescapeString(m[2]) + val = html.UnescapeString(val) + return strings.ReplaceAll(val, "\u00A0", " ") } return "" } diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index f48ba75f..569850a4 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -157,7 +157,7 @@ basecamp --page 1 # First page only, no auto-pagination | Post silently | `basecamp message "Title" "Body" --no-subscribe --in --json` | | Post to chat | `basecamp chat post "Message" --in --json` | | Add comment | `basecamp comment "Text" --in --json` | -| List attachments | `basecamp attachments list --json` | +| List attachments | `basecamp attachments list --json` | | Search | `basecamp search "query" --json` | | Parse URL | `basecamp url parse "" --json` | | Upload file | `basecamp files uploads create [--vault ] --in --json` |