Add attachments download and show-command inline attachment discovery#360
Add attachments download and show-command inline attachment discovery#360
Conversation
github-actions
bot
added
commands
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
This PR expands inline attachment support across the Basecamp CLI by adding an attachments download subcommand, surfacing inline_attachments in show-style commands, and enabling stdout streaming for files download to support piping workflows.
Changes:
- Add
basecamp attachments download <id|url>with parallel downloads, filename de-duping, and optional stdout streaming (--out -). - Enhance multiple
showcommands (and genericshow) to detect and exposeinline_attachments, including notices/breadcrumbs for follow-up download. - Add documentation and workflow guidance (SKILL.md) plus an API proposal communique for first-class inline attachment metadata.
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 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/basecamp/SKILL.md | Updates skill docs with new inline attachment discovery/download workflows and stdout streaming examples. |
| internal/commands/attachments.go | Adds attachments download, extends generic-type handling, and introduces shared helpers for inline-attachment integration. |
| internal/commands/attachments_test.go | Adds unit tests covering download helpers, inline attachment metadata helpers, and content field selection logic. |
| internal/commands/show.go | Generic show now detects inline attachments and adds inline_attachments + notices/breadcrumbs. |
| internal/commands/todos.go | todos show now surfaces inline attachments and download breadcrumb/notice. |
| internal/commands/messages.go | messages show now surfaces inline attachments and download breadcrumb/notice. |
| internal/commands/cards.go | cards show now surfaces inline attachments; also removes --attach support from card create/update/shortcut flows. |
| internal/commands/files.go | files show (documents) surfaces inline attachments; files download --out - streams to stdout. |
| internal/commands/commands.go | Updates command catalog entry for attachments to include download. |
| e2e/smoke/smoke_communication.bats | Adds a smoke test around attachments download behavior in the communication suite. |
| COMMUNIQUE-inline-attachments-api.md | Adds an API proposal document for server-side inline_attachments metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2142700882
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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 12 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="e2e/smoke/smoke_communication.bats">
<violation number="1" location="e2e/smoke/smoke_communication.bats:135">
P2: `ensure_message` already marks unverifiable on failure; calling `mark_unverifiable` again here can duplicate smoke trace entries. Use `|| return 0` at the call site.
(Based on your team's feedback about avoiding duplicate mark_unverifiable calls after ensure_* helpers.) [FEEDBACK_USED]</violation>
</file>
<file name="internal/commands/attachments.go">
<violation number="1" location="internal/commands/attachments.go:217">
P2: Validate `--type` before fetching item content so unsupported values fail with a usage error. Without this guard, invalid types proceed into fetch logic and return a confusing downstream API/SDK error instead of an immediate `Unknown type` message.</violation>
<violation number="2" location="internal/commands/attachments.go:648">
P2: Path traversal check fails when the output directory is the root directory.</violation>
</file>
<file name="internal/commands/cards.go">
<violation number="1" location="internal/commands/cards.go:562">
P1: Cards create/update no longer support `--attach`, causing a breaking regression in card attachment workflows.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
jeremy
force-pushed
the
download-attachments
branch
from
2142700 to
a9eb065
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9eb06595c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jeremy
force-pushed
the
download-attachments
branch
from
a9eb065 to
489b6f9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeremy
force-pushed
the
download-attachments
branch
from
489b6f9 to
fd9e59c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd9e59cfce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd1479da7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8771dd11d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Updates the SDK dependency from v0.6.1-pre (fe6154c8b320) to v0.7.0 (a3d967605ac6). Fixes two breaking changes: - Cards().Move() now takes *MoveCardOptions (pass nil for existing behavior) - Todos().Reposition() now takes *int64 parentID (pass nil) v0.7.0 adds four new services: Gauges, MyAssignments, MyNotifications, and Account — commands for these follow in subsequent commits.
…reaming New `basecamp attachments download <id|url>` subcommand alongside the existing `list`. Downloads inline <bc-attachment> files from any recording type in parallel (concurrency 5) with filename dedup. - --out <dir> for output directory, --out - for stdout streaming - --file <name> to filter by filename, --index <n> for disambiguation (indexes match list numbering; errors on non-downloadable selection) - --type hint with early validation (same guard as list) - writeBodyToFile bypasses SDK filename override for deduped names - isGenericType extended for lines/replies (parent-dependent types) - extractContentField prefers HTML field over plain text (fixes todo content/description ambiguity) - list and download both use ParseAttachments for consistent index numbering; download filters to downloadable subset - Styled list output includes download hint - Path traversal check handles root directory edge case Issue #290
todos show, messages show, cards show, files show (document), and generic show now extract inline attachments from rich text content and add an inline_attachments field to the response data. When attachments are found, a notice and download breadcrumb guide users to `attachments download`. The field is in data (not envelope), so it survives all output modes: --json, --agent/--quiet, --jq, styled, and markdown.
SKILL.md: workflow recipe for discovering and downloading inline attachments, --out - streaming, --index selection. Quick reference entries for attachments download and files download --out -. Communique for BC3 Rails team proposing inline_attachments as a first-class API field to replace client-side HTML parsing.
…ision Three fixes: 1. withAttachmentMeta now takes a field parameter and writes <field>_attachments (e.g. content_attachments, description_attachments) instead of a flat "attachments" key. This avoids colliding with native API attachments on CampfireLine records and keeps collections distinct when a recording has multiple rich-text attributes. 2. Show handlers compose a single notice string instead of calling WithNotice twice (which was last-write-wins, losing the attachment hint when downloads partially failed). 3. The struct-to-map conversion in withAttachmentMeta and the generic show command now use json.NewDecoder with UseNumber() to preserve integer precision through the round-trip. parentRecordingID handles both json.Number and float64.
Follows the established pattern from comment.go and todos.go: when every download in a batch fails, return an error instead of app.OK so automation sees a non-zero exit code. Partial failures still return OK with per-file status in the results array.
…alignment - todos show: add Args: cobra.ExactArgs(1) to match cards show, messages show, and other show commands. Extra args were silently ignored. - smoke test: add --out "$BATS_FILE_TMPDIR" to attachments download test to avoid polluting cwd if the message happens to have attachments. - PR description: align field name references from stale inline_attachments to content_attachments/description_attachments.
fetchItemContent had the same json.Unmarshal precision issue as show.go: the recording response was decoded with plain Unmarshal, so parent.id values above 2^53 would round to float64 before recordingTypeEndpoint built the refetch URL. Now uses json.NewDecoder with UseNumber() at both decode sites, matching the show.go pattern. Adds regression test: TestFetchItemContentRefetchesLineWithLargeParentID verifies a chat line with parent.id = 2^53+1 correctly refetches via /chats/9007199254740993/lines/111.json.
Two fixes in attachments download: 1. After filtering/selection, check that at least one attachment has a download URL. Previously, recordings with <bc-attachment> tags that had metadata but no url/href would proceed, mark everything as "skipped", and exit 0. Now returns the same "No downloadable attachments found" error. 2. On DownloadURL failure, the error result and progress output now use the deduped filename (name) instead of the original (fname), consistent with successful downloads and write-error results.
…e URLs Two fixes: 1. NoOptDefVal was set to "" which pflag treats as "flag needs an argument". Now uses os.TempDir() so --download-attachments (bare) works as documented. 2. --file filtering now matches against the full attachment list first, then checks for download URLs. If the name exists but has no URL, returns "Attachment <name> has no download URL" (same contract as --index). Previously it fell through to "No attachment matching" because it only searched the downloadable subset.
… test Three review fixes: 1. attachments list: download breadcrumb now only shown when at least one attachment has a download URL, avoiding misleading hints. 2. writeBodyToFile: path traversal error now returns output.ErrUsage for structured JSON/agent output, consistent with files.go. 3. TestShowInjectsFieldScopedAttachments: verifies generic show injects content_attachments/description_attachments (not flat "attachments") when content has bc-attachment tags.
- .surface: add --download-attachments flag entries for show commands - commands.go: catalog alignment - richtext: simplify after attachment parsing consolidation
…nario - TestFilesDownloadStdoutStreamsStorageURL: verifies storage URL body is streamed directly to stdout with no files created - TestFilesDownloadStdoutStreamsUploadID: exercises the full upload ID path (metadata fetch → signed download → stdout stream) - TestShowPreservesNativeAttachmentsField: locks the collision fix — a record with a native API "attachments" array retains it while content_attachments is added alongside
- TestShowPreservesNativeAttachmentsField: now parses JSON and asserts the "attachments" key exists by name, not just its values appearing somewhere in the output - files download: extract resolveDownloadProject helper, eliminating duplicated 5-step project resolution between --out - and normal paths - assignments/notifications: add Args: cobra.NoArgs so typos like "basecamp assignments completd" error instead of silently listing
jeremy
force-pushed
the
download-attachments
branch
from
8771dd1 to
7af2ada
Compare
- go.mod: restore SDK v0.7.1 (was v0.7.0 from pre-rebase history) - typeToEndpoint: add inbox_forwards as alias for forward endpoint, so inbox forward URLs work without --type override - normalizeShowType: add inbox_forwards → forward mapping for breadcrumb show command hints
github-actions
bot
removed
sdk
Summary
basecamp attachments download <id|url>— download inline<bc-attachment>files from any recording type, parallel (concurrency 5), with filename dedup and stdout streaming (--out -)todos show,messages show,cards show,files show, and genericshownow surface field-scopedcontent_attachments/description_attachmentsin response data with notice and download breadcrumbfiles download --out -— stdout streaming for single-file pipinginline_attachmentsas a first-class API fieldBuilds on #296 (attachments list). Both
listanddownloaduseParseAttachmentsfor consistent index numbering. Download filters to the downloadable subset (attachments with URLs).Closes #290
Design decisions
content_attachments,description_attachments) avoid colliding with native APIattachmentson CampfireLine records; each rich-text attribute gets its own collectionextractContentFieldprefers HTML over plain text when bothcontentanddescriptionare present — fixes todo content/description ambiguity wherecontentis the plain-text titleisGenericTypeextended forlines/replies— parent-dependent types route through generic recording lookup + refetch, same asshowwriteBodyToFilebypasses SDK'sresult.Filenameoverride so filename dedup actually works--indexflag for disambiguation when--filematches multiple attachments with the same nameUseNumberto preserve integer precision through the struct→map round-tripWithNoticecalls (last-write-wins)Test plan
bin/cipasses (all 262 leaf commands covered)basecamp attachments download <id-with-attachments> --out /tmp/test-att— comment #9713996624 PNG downloaded (6514 bytes, valid PNG)basecamp attachments download <id> --index 1 --out -— streams valid PNG to stdout (piped tofile -confirms image/png).--filehas a Unicode normalization issue on macOS (NFD vs NFC) — pre-existing, not from this PR.basecamp show <id-with-attachments> --json | jq '.data.content_attachments'— comment #9713996624 returnscontent_attachmentsarray with filename, sgid, url, dimensionsbasecamp show <id-with-attachments> --agent—content_attachmentspresent in agent output alongside full recording databasecamp attachments list <id> && basecamp attachments download <id> --index 1— list finds 1 attachment, download by index succeeds with deduped filenamebasecamp attachments list <chat-line-url>— URL parsing works (type=lines, recording_id extracted), but tested recording was 404 at generic endpoint (data availability, not code). Refetch path is covered byTestShowLineRefetchesViaParentandTestFetchItemContentRefetchesLineWithLargeParentID.