Skip to content

feat(attachments): support uploading files on replies and thread creation#13

Merged
scottlovegrove merged 3 commits into
mainfrom
scottl/attachments
May 31, 2026
Merged

feat(attachments): support uploading files on replies and thread creation#13
scottlovegrove merged 3 commits into
mainfrom
scottl/attachments

Conversation

@scottlovegrove
Copy link
Copy Markdown
Collaborator

What

Ports the twist-cli file-attachment feature to comms-cli, using the attachments client added in @doist/comms-sdk 0.4.1.

tdc thread reply 12345 "See attached" --file ./diagram.png
tdc conversation reply 12345 "Photos" --file ./a.jpg --file ./b.jpg
tdc thread create CH123 "Release notes" --file ./report.pdf   # file-only, no body

How

  • SDK bump @doist/comms-sdk 0.3.0 → 0.4.1.
  • src/lib/local-file.tsopenLocalFileAsBlob resolves a path, verifies readability, returns a file-backed Blob; structured FILE_NOT_FOUND / FILE_READ_ERROR.
  • src/lib/attachments.tsuploadAttachments(files) validates every path before uploading any (no orphaned uploads), then uploads each concurrently (Promise.all, order preserved).
  • --file is repeatable on thread reply, conversation reply, and thread create; a file-only post with no text is allowed (skips the editor prompt).
  • conversation reply preflights the conversation (getConversation) before uploading so an invalid/forbidden target fails before orphaning an upload.
  • --file + --close/--reopen on thread replyCONFLICTING_OPTIONS.

Auth / scopes (403 handling)

Uploading needs the new attachments:write scope. attachments.upload gets a spinner entry so it routes through wrapResult; a token from before the scope change gets a 403 that's translated to INSUFFICIENT_SCOPE with the tdc auth login re-login prompt. Added attachments:read / attachments:write to the OAuth scope lists (read-write stays a superset of read-only).

Verification

  • type-check, full suite (722 tests, incl. new local-file / attachments unit tests, --file suites for all three surfaces, and an attachments.upload 403 → INSUFFICIENT_SCOPE test), oxlint + oxfmt, and check:skill-sync all pass.

Mirrors Doist/twist-cli#260.

🤖 Generated with Claude Code

…tion

Ports the twist-cli attachment feature to comms-cli, backed by the
`attachments` client in @doist/comms-sdk 0.4.1.

- Bump @doist/comms-sdk 0.3.0 -> 0.4.1
- `src/lib/local-file.ts` (`openLocalFileAsBlob`) reads a path into a
  file-backed Blob with structured FILE_NOT_FOUND / FILE_READ_ERROR errors
- `src/lib/attachments.ts` (`uploadAttachments`) validates every path up
  front, then uploads each concurrently (order preserved)
- `tdc thread reply`, `tdc conversation reply`, and `tdc thread create`
  gain a repeatable `--file`; a file-only post (no text) is allowed
- `conversation reply` preflights the conversation before uploading so an
  invalid/forbidden target fails before orphaning an upload
- `--file` + `--close`/`--reopen` on thread reply -> CONFLICTING_OPTIONS
- Add an `attachments.upload` spinner entry so uploads route through
  `wrapResult` for the 403 -> INSUFFICIENT_SCOPE re-login prompt
- Add `attachments:read` / `attachments:write` to the OAuth scopes
- Skill content + regenerated SKILL.md; tests for all of the above

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@scottlovegrove scottlovegrove self-assigned this May 31, 2026
@doistbot doistbot requested a review from nvignola May 31, 2026 13:27
Copy link
Copy Markdown
Member

@doistbot doistbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks scottlovegrove for your contribution 😇. The new file attachment feature integrates cleanly with the existing command structures and thoughtfully handles edge cases like preflighting conversations to prevent orphaned uploads.

Few things worth tightening:

  • Restore type safety for thread replies by building a separately typed request object rather than casting the payload after the fact.
  • Streamline file reads and uploads by using openAsBlob() directly instead of probing with an initial open/close, and cap the upload concurrency to prevent I/O spikes.
  • Extend the permissions mock to ensure attachments.upload is properly tested through the mutating API path.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (4)
  • [P3] src/commands/thread/create.ts:75: This new --file dry-run path never validates the local files. For example, tdc thread create CH100 "Title" --file ./missing.png --dry-run prints a preview here, but the real command fails with FILE_NOT_FOUND once it reaches uploadAttachments(). Please validate the file paths in dry-run as well (without uploading) so the preview matches actual execution; the same issue exists in the other new --file dry-runs.
  • [P3] src/lib/options.ts:25: collect() is now a shared helper, but it only works if every caller remembers to pass [] as the Commander default. If a future repeatable option forgets that, previous is undefined and [...previous, value] throws at runtime. Make it defensive (previous: string[] = [] or previous ?? []) so the helper’s contract is enforced in code instead of comments.
  • [P3] src/commands/conversation/conversation.test.ts:863: The PR description highlights that conversation reply preflights the conversation to prevent orphaned uploads on invalid targets. Consider adding a test where client.conversations.getConversation rejects (e.g., 404 Not Found) to verify that client.attachments.upload is indeed skipped.
  • [P3] src/lib/local-file.test.ts:34: This only covers the FILE_NOT_FOUND branch. The helper also adds a separate FILE_READ_ERROR path for unreadable/non-file inputs, and that structured error is easy to lose without a test. Please add one deterministic non-ENOENT case here (for example a mocked EACCES/EPERM failure).

Share FeedbackReview Logs

Comment thread src/commands/thread/reply.ts Outdated
Comment thread src/lib/local-file.ts Outdated
Comment thread src/lib/attachments.ts Outdated
Comment thread src/lib/api.test.ts
scottlovegrove and others added 2 commits May 31, 2026 14:41
- thread reply: build a `satisfies`-typed createComment request so the
  `attachments` contract is compiler-checked again; only `recipients`
  (EVERYONE sentinels) keeps the assertion
- local-file: try `openAsBlob` first (happy path = one open, no TOCTOU);
  fall back to an `fs.open` probe only on failure to recover the real
  errno (openAsBlob masks it as ERR_INVALID_ARG_VALUE)
- attachments: cap upload concurrency (4) while preserving input order
- options: `collect` defaults `previous` to `[]` so the contract holds
  even if a caller forgets the Commander default
- dry-run now validates attachment paths (without uploading) so the
  preview fails on a bad path exactly as a real run would
- tests: attachments.upload exercised through the mutating write-guard
  (incl. read-only block), local-file FILE_READ_ERROR branch, and a
  conversation preflight-failure-skips-upload case

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@scottlovegrove scottlovegrove merged commit 44dc3b7 into main May 31, 2026
5 checks passed
@scottlovegrove scottlovegrove deleted the scottl/attachments branch May 31, 2026 13:45
doist-release-bot Bot added a commit that referenced this pull request May 31, 2026
## [1.4.0](v1.3.2...v1.4.0) (2026-05-31)

### Features

* **attachments:** support uploading files on replies and thread creation ([#13](#13)) ([44dc3b7](44dc3b7))
@doist-release-bot
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 1.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants