Add attachment listing for items#296
Conversation
github-actions
bot
added
commands
There was a problem hiding this comment.
Pull request overview
Adds a new basecamp attachments list <id|url> command that lists embedded file attachments for arbitrary Basecamp recordings by parsing <bc-attachment> tags from the item’s HTML content, with both styled TTY output and structured JSON output.
Changes:
- Introduces a new
attachmentscommand group with alistsubcommand that fetches an item’s content (with recording-type refetch) and renders attachments. - Adds
richtext.ParseAttachments()+ParsedAttachmenthelpers (with unit tests) to extract attachment metadata and provide display helpers. - Updates command catalog/registration, surface snapshot, docs, API coverage notes, and adds an e2e smoke test.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/attachments.go |
New command implementation for listing attachments (fetch + parse + output). |
internal/richtext/richtext.go |
Adds attachment parsing and display helper methods. |
internal/richtext/richtext_test.go |
Adds unit tests for parsing and display helpers. |
internal/commands/commands.go |
Adds attachments to the command catalog under Communication. |
internal/cli/root.go |
Registers the new attachments command. |
internal/commands/commands_test.go |
Ensures the new command is included in catalog parity tests. |
e2e/smoke/smoke_attachments.bats |
Adds smoke coverage for attachments list (with/without attachments). |
skills/basecamp/SKILL.md |
Documents the new command in the quick reference. |
API-COVERAGE.md |
Notes attachment listing coverage via parsing embedded tags. |
.surface |
Updates surface snapshot with new command and flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/richtext/richtext.go">
<violation number="1" location="internal/richtext/richtext.go:813">
P2: `regexp.MustCompile` is called inside `extractAttr`, which runs 8 times per attachment. Every other regex in this file is pre-compiled at package level. Compile once and reuse — for example, use a single pre-compiled regex that captures any attribute name and value, then filter by name in code.</violation>
</file>
<file name="internal/commands/attachments.go">
<violation number="1" location="internal/commands/attachments.go:192">
P2: `typeToEndpoint` is missing several types that `show.go`'s equivalent switch supports (`todolist`, `card-table`, `schedule-entry`, `checkin`, `forward`). A user passing `--type forward` gets an "Unknown type" error here but not in `show`. Consider extracting the shared switch into a helper both commands call to prevent further drift.</violation>
</file>
<file name="e2e/smoke/smoke_attachments.bats">
<violation number="1" location="e2e/smoke/smoke_attachments.bats:52">
P2: Test asserts `.ok` is true but never verifies that the attachment actually appears in `.data`. The test would pass even if listing returned an empty array, making the "shows results" claim untested. Add an assertion like `assert_json_value '.data | length > 0' 'true'` (or check for the filename) so the test actually validates the attachment was returned.</violation>
<violation number="2" location="e2e/smoke/smoke_attachments.bats:54">
P3: The temp-file writes for `attach_todo_id` and `attach_comment_id` are never read by any subsequent test. Either remove them or add a cleanup/follow-up test that uses the stored IDs.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/attachments.go">
<violation number="1" location="internal/commands/attachments.go:157">
P2: The usage hint for the 204 No Content case lists only a subset of the supported types, missing the five types added in this same PR (`todolist`, `card-table`, `schedule-entry`, `checkin`, `forward`). The two other occurrences in this file were updated but this one was not.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Adds a new basecamp attachments list <id|url> command to enumerate file attachments embedded in Basecamp item rich text by parsing <bc-attachment> tags, with styled TTY output and structured JSON output.
Changes:
- Introduces
attachmentscommand group withlistsubcommand, including recording-type discovery/refetch for full content. - Adds rich text attachment parsing (
ParseAttachments) plus display helpers and unit tests. - Updates command catalog/registration, docs, API coverage notes, and smoke tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/attachments.go |
New CLI command implementation for listing embedded attachments on an item. |
internal/richtext/richtext.go |
Adds attachment parsing and presentation helpers for <bc-attachment> tags. |
internal/richtext/richtext_test.go |
Adds unit tests covering parsing, mention filtering, and display helpers. |
internal/cli/root.go |
Registers the new attachments command in the root CLI. |
internal/commands/commands.go |
Adds attachments to the command catalog under “communication”. |
internal/commands/commands_test.go |
Ensures the new command is registered in command-parity tests. |
e2e/smoke/smoke_attachments.bats |
Adds smoke coverage for attachments list behavior. |
skills/basecamp/SKILL.md |
Documents attachments list in the skill quick reference. |
API-COVERAGE.md |
Updates attachment coverage note to include listing embedded attachments. |
.surface |
Updates surfaced command/flag inventory to include attachments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/attachments.go">
<violation number="1" location="internal/commands/attachments.go:73">
P2: `id` is unconditionally overwritten with `parsed.RecordingID` which can be empty for URL patterns that match but lack a recording ID (e.g., project URLs). This discards the valid fallback from `extractID(arg)` on the line above. Guard the assignment like `urlarg.ExtractID` does.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Nice work @brianevanmiller. I'll merge in related work and address #290, #326, and #283 (comment) here as well. |
There was a problem hiding this comment.
Pull request overview
Adds a new CLI capability to list embedded file attachments on Basecamp items by parsing <bc-attachment> tags from an item’s HTML content, with both styled TTY output and structured JSON output. This extends the CLI’s “show any recording by ID/URL” pattern to provide quick attachment discovery across many item types.
Changes:
- Introduces
basecamp attachments list <id|url>with type discovery / type-hint support and styled + JSON output. - Adds rich-text attachment parsing utilities (
ParsedAttachment,ParseAttachments, display helpers) plus unit tests. - Registers the new command across the CLI (root registration, command catalog/tests), updates docs/coverage notes, and adds an e2e smoke test.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/attachments.go |
New attachments command group and list implementation (fetch + parse + output). |
internal/richtext/richtext.go |
Attachment parsing and display helpers for <bc-attachment> tags. |
internal/richtext/richtext_test.go |
New test coverage for parsing edge cases and helper methods. |
internal/commands/commands.go |
Adds attachments to the “communication” command category catalog. |
internal/cli/root.go |
Registers attachments command with the root CLI. |
internal/commands/commands_test.go |
Ensures command registration parity includes attachments. |
e2e/smoke/smoke_attachments.bats |
Smoke tests for help output and JSON results for items with/without attachments. |
skills/basecamp/SKILL.md |
Quick reference update to include attachments list. |
API-COVERAGE.md |
Updates coverage note to reflect new attachment listing behavior. |
.surface |
Surface/command index updated to include the new command and flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Thanks @jeremy! I'm excited about this new I also squashed the commits & rebased so it's clean and portable |
brianevanmiller
force-pushed
the
brianevanmiller/feature-gap-analysis
branch
from
32928e4 to
bd52d32
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new basecamp attachments list <id|url> CLI feature to enumerate file attachments embedded in Basecamp item rich text content by parsing <bc-attachment> tags, with styled TTY output and structured JSON output.
Changes:
- Introduces a new
attachmentscommand group with alistsubcommand, including type discovery and breadcrumbs. - Adds rich text parsing helpers (
ParsedAttachment,ParseAttachments, display helpers) plus unit tests. - Updates command catalog/registration, docs, surface metadata, and smoke tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/attachments.go |
Implements basecamp attachments list, endpoint selection, refetch via type endpoint, and output formatting. |
internal/richtext/richtext.go |
Adds attachment parsing into structured metadata extracted from <bc-attachment> tags. |
internal/richtext/richtext_test.go |
Adds unit tests covering attachment parsing edge cases and display helpers. |
internal/commands/commands.go |
Registers attachments in the command catalog under “communication”. |
internal/commands/commands_test.go |
Ensures the new command is included in the “all commands” root used by tests. |
internal/cli/root.go |
Registers the new command in the CLI root command. |
e2e/smoke/smoke_attachments.bats |
Adds smoke coverage for listing attachments with/without results. |
skills/basecamp/SKILL.md |
Documents attachments list in the skill quick reference. |
API-COVERAGE.md |
Updates the coverage note to include attachments list. |
.surface |
Updates surfaced commands/flags inventory for the new command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/attachments.go">
<violation number="1" location="internal/commands/attachments.go:70">
P2: When the user passes an explicit `--type` that isn't `comment`, the code still overrides `id` with `parsed.CommentID`, causing a type/ID mismatch. Only prefer `CommentID` when the type is unset or already "comment"; otherwise fall through to `RecordingID`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
This PR adds a new basecamp attachments list <id|url> command to list <bc-attachment>-embedded files/images on Basecamp recordings, using new rich-text parsing helpers and covering behavior via unit + smoke tests.
Changes:
- Introduces an
attachmentscommand group with alistsubcommand, including styled TTY output and structured JSON output with breadcrumbs. - Adds
richtext.ParseAttachments()and aParsedAttachmentmodel to extract attachment metadata from item HTML while filtering out@mentionattachments. - Registers the command across CLI/root + command catalog and adds unit/E2E coverage plus docs/surface updates.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/attachments.go |
Implements the new attachments list command (fetch, parse, render, breadcrumbs). |
internal/richtext/richtext.go |
Adds attachment tag parsing + display helpers used by the command. |
internal/richtext/richtext_test.go |
Adds unit tests for attachment parsing edge-cases and helpers. |
internal/commands/attachments_test.go |
Adds command-focused tests for URL/type resolution helpers. |
internal/cli/root.go |
Registers the new command in the CLI root. |
internal/commands/commands.go |
Adds attachments to the command catalog (“Communication”). |
internal/commands/commands_test.go |
Ensures the command is included in the “all commands” test root. |
e2e/smoke/smoke_attachments.bats |
Adds smoke coverage for listing attachments on items/comments. |
skills/basecamp/SKILL.md |
Documents attachments list in the quick reference table. |
API-COVERAGE.md |
Updates coverage notes to include attachment listing via parsing. |
.surface |
Updates CLI surface snapshot for new command/flags/subcommands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/attachments.go">
<violation number="1" location="internal/commands/attachments.go:256">
P2: Breadcrumb maps `answer`/`question_answers` to `checkin`, but `show --type checkin` fetches `/questions/{id}` (the parent question), not `/question_answers/{id}` (the answer). Since `show` has no type alias for question_answers, return `""` to let generic recording lookup find the correct endpoint.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Adds a new basecamp attachments list <id|url> command to list inline file attachments embedded in Basecamp item rich-text HTML, with styled TTY output and structured JSON output.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Introduces a new
attachmentscommand group with alistsubcommand that fetches an item (generic recording discovery + type-specific refetch) and renders extracted attachments. - Adds rich-text parsing helpers to extract
<bc-attachment>metadata while filtering out Basecamp mention attachments. - Adds unit tests, smoke tests, CLI surface/docs updates, and command catalog/registration entries.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/attachments.go |
Implements basecamp attachments list, including URL/type resolution, fetching, styled output, and JSON breadcrumbs. |
internal/richtext/richtext.go |
Adds ParsedAttachment and ParseAttachments() with attribute extraction + display helpers. |
internal/richtext/richtext_test.go |
Adds parsing and helper tests for attachments/mentions/quoting/entities/image detection. |
internal/commands/attachments_test.go |
Adds unit tests for URL fragment handling and type normalization/endpoint mapping. |
internal/commands/commands.go |
Adds attachments to the command catalog under “communication”. |
internal/commands/commands_test.go |
Ensures the new command is registered in the “all commands” test root. |
internal/cli/root.go |
Registers NewAttachmentsCmd() with the root CLI. |
e2e/smoke/smoke_attachments.bats |
Adds smoke coverage for help output and listing attachments via comment/todo flows. |
skills/basecamp/SKILL.md |
Updates quick reference to include attachments list. |
API-COVERAGE.md |
Updates attachments coverage note to include listing embedded attachments. |
.surface |
Updates CLI surface snapshot for the new commands/flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Adds a new basecamp attachments command group with a list <id|url> subcommand to extract and display embedded <bc-attachment> tags from item HTML content, supporting styled TTY output and structured JSON output.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Introduces
basecamp attachments list <id|url>with type discovery/refetch behavior similar toshow. - Adds rich-text parsing helpers for extracting attachment metadata from
<bc-attachment>tags (with tests). - Wires the new command into the CLI surface, docs, and smoke tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/attachments.go |
New attachments command group and list implementation (fetch + parse + output). |
internal/commands/attachments_test.go |
Unit tests for URL/type resolution and endpoint mapping. |
internal/richtext/richtext.go |
Adds ParsedAttachment model and ParseAttachments + display helpers. |
internal/richtext/richtext_test.go |
Adds parsing/display helper test coverage. |
internal/commands/commands.go |
Registers attachments in command catalog categories. |
internal/commands/commands_test.go |
Ensures the new command is registered in the root command used by tests. |
internal/cli/root.go |
Registers the new command in the CLI root. |
e2e/smoke/smoke_attachments.bats |
Adds smoke coverage for listing attachments on comments/items. |
skills/basecamp/SKILL.md |
Updates skill quick reference with attachments list. |
API-COVERAGE.md |
Notes coverage for listing embedded attachments. |
.surface |
Updates CLI surface snapshot for the new command/flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Introduces `basecamp attachments list <id|url>` to enumerate file attachments embedded in Basecamp item rich text by parsing <bc-attachment> 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 <noreply@anthropic.com>
extractAttr returned raw HTML-encoded strings, so filenames like O&basecamp#39;Brien displayed with visible entities. Apply html.UnescapeString on the read path to match the encoding done by escapeAttr on writes.
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".
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.
- 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
… IsImage - 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
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.
- 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 <bc-attachment-custom> to match - Update typeToEndpoint comment to note it's a superset of show.go
The --in persistent flag from main was missing from the attachments surface entries after the merge.
The regex required at least one char in [\s/>] after the tag name, which failed on bare <bc-attachment> 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.
jeremy
force-pushed
the
brianevanmiller/feature-gap-analysis
branch
from
9e3033b to
8ec9dc6
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new basecamp attachments list <id|url> command to enumerate <bc-attachment> embeds on Basecamp recordings (todos/messages/cards/comments/etc.), producing styled TTY output and structured JSON output.
Changes:
- Introduces a new
attachmentscommand group with alistsubcommand, including type auto-discovery and breadcrumbs. - Adds rich-text parsing utilities to extract attachment metadata from
<bc-attachment>HTML (with mention filtering) plus unit tests. - Registers the new command across CLI wiring, surface snapshot, docs, and adds an e2e smoke test.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/attachments.go |
Implements attachments list, including type resolution, fetch/refetch logic, and output formatting. |
internal/richtext/richtext.go |
Adds ParsedAttachment + ParseAttachments() and helper methods for display/image detection. |
internal/richtext/richtext_test.go |
Adds parsing and helper-behavior test coverage for attachment extraction. |
internal/commands/attachments_test.go |
Adds unit tests for URL/type resolution and endpoint mapping helpers. |
internal/cli/root.go |
Registers the new top-level attachments command. |
internal/commands/commands.go |
Adds attachments to the “communication” command catalog category. |
internal/commands/commands_test.go |
Ensures the command is included in the “all commands” test root. |
e2e/smoke/smoke_attachments.bats |
Adds smoke coverage for listing attachments and empty results. |
skills/basecamp/SKILL.md |
Updates skill quick-reference to include attachments list. |
API-COVERAGE.md |
Updates coverage notes to include the new listing capability. |
.surface |
Updates CLI surface snapshot for the new command and flags. |
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 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 <id|url> instead of <id>
Summary
basecamp attachments list <id|url>to list file attachments on any recording (todos, messages, cards, etc.)<bc-attachment>tags from item HTML content using bc4's proven regex pattern, filtering out @mentionsapp.OK()with breadcrumbsrecordingTypeEndpointpattern fromshow.gofor type-specific endpoint discoveryFiles
internal/commands/attachments.goattachmentscommand group withlistsubcommandinternal/richtext/richtext.goParsedAttachmenttype,ParseAttachments(),IsImage(),DisplayName(),DisplayURL()internal/richtext/richtext_test.gointernal/commands/commands.gointernal/cli/root.gointernal/commands/commands_test.goe2e/smoke/smoke_attachments.batsskills/basecamp/SKILL.mdattachments listto quick referenceAPI-COVERAGE.mdTest plan
make fmt-check— passesmake vet— passesgo test ./internal/richtext/...— 10 new subtests passgo test ./internal/commands/...— catalog parity passesscripts/check-bare-groups.sh— passesscripts/check-skill-drift.sh— passes (121 commands, 307 flags)make smoke— verifyattachments listagainst live projectSummary by cubic
Adds
basecamp attachments list <id|url>to list file attachments embedded in any item by parsing<bc-attachment>tags. Supports styled TTY (icons and image sizes) and JSON with structured metadata and breadcrumbs.New Features
--type; auto-detects type from Basecamp URLs and re-fetches via the type endpoint. Treatsrecording/recordingsas aliases.showhint (e.g.,schedule_entries→schedule-entry,card_tables→card-table), omit--typefor answers (question_answers) so generic lookup targets the answer, and omit thecommentaction for comment items.--inas an alias for--project.Bug Fixes
data-url), supports single/double quotes, bare and uppercase tags, decodes HTML entities and normalizes NBSPs, case-insensitive attribute names and mention filtering; stricter tag boundary so<bc-attachment-custom>doesn’t match.--typemapping (answer/question_answers) with clearer “type required” hints and deduped type validation.IsImage()and removal of variable shadowing in both parser and command code; SKILL quick reference updated to show<id|url>.Written for commit 98a20f8. Summary will update on new commits.