feat: DM per-message ops + attachments + group avatar (3/3)#58
Merged
jackparnell merged 1 commit intoMay 27, 2026
Merged
Conversation
… mock)
Third and final PR of the group-DM coverage series. With this in,
the SDK wraps the full /api/v1/messages/* surface. 15 new methods
plus brand-new multipart-upload + binary-download infrastructure.
Per-message operations (same surface for 1:1 and group):
mark_message_read(message_id)
list_message_reads(message_id)
add_message_reaction(message_id, emoji)
remove_message_reaction(message_id, emoji)
edit_message(message_id, body)
list_message_edits(message_id)
delete_message(message_id)
toggle_star_message(message_id)
list_saved_messages(limit=50, offset=0)
forward_message(message_id, recipient_username, comment="")
Attachments (multipart):
upload_message_attachment(filename, file_bytes, content_type)
delete_message_attachment(attachment_id)
get_message_attachment(attachment_id, variant="full") -> bytes
Group avatar (multipart):
upload_group_avatar(conv_id, filename, file_bytes, content_type)
get_group_avatar(conv_id) -> bytes
Infrastructure:
_raw_multipart_upload — RFC 7578 envelope hand-rolled on the sync
client (urllib has no native multipart); the async client uses
httpx's native files= argument. Filenames are RFC 6266 §4.2-
escaped so backslashes / quotes don't break the envelope.
_raw_request_bytes — GET returning raw bytes for image/file
downloads. Shares auth, hooks, and rate-limit-header tracking
with _raw_request; retry loop is skipped because uploads /
downloads are rarely safe to retry blindly.
Both helpers route errors through _build_api_error so 4xx/5xx
responses surface as ColonyAPIError / ColonyAuthError etc., and
transport failures surface as ColonyNetworkError — identical to
the JSON path.
MockColonyClient records byte-length (not raw bytes) for upload
calls so test assertions stay grep-able for large payloads.
Bytes-returning getters yield a deterministic sentinel by default,
overridable via responses={"get_message_attachment": b"..."}.
67 new tests cover request shape, RFC 6266 filename escaping, the
413 / 403 error envelopes, network-error wrapping, lazy-token
minting on first call, and request/response hook fan-out across
both new transport paths. 100% line coverage preserved on sync,
async, and mock clients. ruff + format + mypy + 661 tests green.
Per the user's instruction, no version bump in this PR either —
the version moves with a dedicated release PR now that all three
group-DM-coverage PRs are in.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ColonistOne
approved these changes
May 27, 2026
Collaborator
ColonistOne
left a comment
There was a problem hiding this comment.
Reviewed end-to-end against main. CI is fully green across 3.10/3.11/3.12/3.13 + lint + typecheck + codecov, and the surface closes out the group-DM coverage cleanly. The new transport infrastructure (multipart upload + binary GET on both clients) is the most interesting part — flagging a few cross-helper asymmetries below.
Strengths
- RFC 6266 §4.2 filename escape pinned at the byte level.
test_upload_message_attachment_escapes_quote_in_filenameassertsb'filename="weird\\"name.png"' in body— not just "has escape" but the exact backslash-quoted form FastAPI'sUploadFileparser expects. The right depth for a hand-rolled multipart envelope. - Emoji URL encoding pinned by exact wire bytes —
%F0%9F%91%8D(sync) and%F0%9F%8E%89(async), not just "contains percent encoding". Catches a future regression where someone passessafe=':/'and the multi-byte sequence sneaks through. - Multipart wire shape inspected directly —
test_upload_message_attachment_builds_multipart_bodyparses out the boundary token from theContent-Typeheader and confirms it appears in the body as--{boundary}--. Verifies the round-trip rather than trusting it. - Error envelope parity through
_build_api_error— 413 + structured{detail: {message, code: "LIMIT_EXCEEDED"}}unwraps asColonyAPIErrorwith.code == "LIMIT_EXCEEDED"; 403 on bytes path becomesColonyAuthError. Same shape as the JSON path. Pinned on both helpers, both clients. ColonyNetworkErrorparity —URLError(sync) andhttpx.ConnectError(async) both wrap withstatus=0so callers branch on type rather than parsing message strings. Pinned on both helpers, both clients.- Lazy-token mint coverage — both new transport paths trigger
_ensure_token()when the client has no token in memory. Pinned on sync + async. - Hook fan-out coverage —
on_request+on_responsecallbacks fire on both multipart-upload and bytes-GET paths. Easy to forget when copy-pasting_raw_requestplumbing; pinned explicitly. - Mock records byte-length, not raw bytes.
test_upload_message_attachment_records_size_not_bytesconfirms this — sensible call-shape choice for large uploads, and grep-friendly. The bytes-returning getters yield a deterministic sentinel by default, overridable viaresponses={"get_message_attachment": b"..."}— pinned bytest_get_message_attachment_custom_bytes_response. os.urandom(16).hex()for the sync boundary token prevents collisions across concurrent uploads from the same client. Async delegates to httpx for the same reason.- PR scope discipline — author explicitly held the version bump for a dedicated release PR consolidating all three changelog entries. Matches the plan committed to in #56.
Minor observations (none blocking)
- Async multipart + bytes helpers skip
self.last_rate_limittracking. The sync counterparts both doself.last_rate_limit = RateLimitInfo.from_headers(...), and the existing async_raw_request(src/colony_sdk/async_client.py:389) also tracks it on every response. The new async helpers don't. Callers readingclient.last_rate_limitafter anawait client.upload_message_attachment(...)would see stale data on async but fresh data on sync. Worth a follow-up to re-sync the shared client state. - Circuit breaker bypassed by both helpers on both clients. Sync
_raw_requestand async_raw_requestboth check_circuit_breaker_thresholdbefore issuing and update_consecutive_failureson failure / reset on success; the new multipart + bytes helpers do none of this. The PR description explains the retry skip ("uploads + downloads are rarely safe to retry blindly"), but the circuit breaker is a pre-flight refusal that wouldn't retry — bypassing it means a tripped breaker still lets uploads through. Probably intentional but the rationale isn't documented; worth a note or a smallif breaker_open: raiseat the top of each helper. Idempotency-Keyheader not supported on multipart. Sync_raw_requestthreadsIdempotency-Key(per the #56 review); the new multipart helpers don't accept one. Uploads are exactly where idempotency matters — network blip during a 7 MB POST and a naive retry creates two attachments. Server-side dedup-by-content_hash (thededuped: boolresponse field) mitigates this case, so it's defensible to defer, but worth a note that the same hash will dedupe a retry rather than the SDK threading the header.field_namekwarg is wired but always"file". Both helpers passfield_name="file"for every callsite. Fine — leaving the param exposed makes a future endpoint with a different field name a one-line change rather than a refactor.
Approval note
Approving — the three observations above are all follow-up work, not blockers. Holding the merge button for a human reviewer per the TheColonyCC/* convention.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Third and final PR of the group-DM coverage series. With this in, the SDK wraps the full
/api/v1/messages/*surface. 15 new methods plus brand-new multipart-upload + binary-download infrastructure.Per-message operations (1:1 + group)
mark_message_read,list_message_reads,add_message_reaction,remove_message_reaction,edit_message,list_message_edits,delete_message,toggle_star_message,list_saved_messages,forward_message.Attachments (multipart)
upload_message_attachment,delete_message_attachment,get_message_attachment(…, variant)→bytes.Group avatar (multipart)
upload_group_avatar,get_group_avatar→bytes.New transport infrastructure
Two new helpers next to
_raw_request:_raw_multipart_upload— RFC 7578 envelope hand-rolled on the sync client (urllib has no native multipart support); the async client uses httpx's nativefiles=argument. Filename quotes + backslashes are RFC 6266 §4.2-escaped so the envelope stays parseable for weird filenames. Pinned bytest_upload_message_attachment_escapes_quote_in_filename._raw_request_bytes— GET returning rawbytes(no JSON parse). Shares auth, hooks, and rate-limit-header tracking with_raw_request; the retry loop is deliberately skipped because uploads + downloads are rarely safe to retry blindly.Both helpers route errors through the existing
_build_api_errorplumbing so 4xx/5xx responses surface asColonyAPIError/ColonyAuthErroretc. and transport failures surface asColonyNetworkError— same shape as the JSON path.Mock client conventions
MockColonyClientrecords byte-length, not raw bytes for upload calls so test assertion shapes stay grep-able for large payloads. Bytes-returning getters yield a deterministic sentinel by default, overridable viaresponses={"get_message_attachment": b\"...\"}.Test plan
TestPerMessageOps,TestAttachments, async + mock counterparts, plus coverage for hooks / lazy-token / network errors)ruff check+ruff format --checkcleanmypy src/cleanNotes for review
edit_messageis enforced server-side — the SDK just forwards the body; a 403 is the signal the window lapsed.remove_message_reactionURL-encodes the emoji in the DELETE path (urllib.parse.quote(emoji, safe='')) because most emoji are multi-byte UTF-8 and would corrupt the URL otherwise. Pinned bytest_remove_message_reaction_url_encodes_emoji.os.urandom(16).hex()for the boundary token so two concurrent uploads from the same client don't collide. The async path delegates boundary generation to httpx.URLError(sync) andhttpx.ConnectError(async). Both wrap toColonyNetworkErrorwithstatus=0so callers can branch on type rather than parsing message strings.pyproject.toml+__init__.py.🤖 Generated with Claude Code