feat(gchat): deliver files via multipart media upload endpoint#112
feat(gchat): deliver files via multipart media upload endpoint#112tony-chinchill-ai wants to merge 1 commit into
Conversation
Add _gchat_media_upload() to GoogleChatAdapter: POSTs each file as a multipart/related request to the GChat media upload endpoint (upload/v1/.../messages?uploadType=multipart). One request per file -- the GChat upload API does not support batching. Wire into post_message(): uploads files first, then returns early when the message has no text or card (matching the Slack adapter pattern). Per-file errors are logged and swallowed so a transport hiccup cannot kill a turn whose text response already landed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesGoogle Chat File Upload Support
Sequence DiagramsequenceDiagram
participant Client
participant post_message
participant extract_files
participant _gchat_media_upload
participant GChatAPI
Client->>post_message: post_message(message)
post_message->>extract_files: extract_files(message)
extract_files-->>post_message: files list
alt files present
loop per file
post_message->>_gchat_media_upload: upload file with optional thread metadata
_gchat_media_upload->>GChatAPI: POST /upload/v1/ multipart
GChatAPI-->>_gchat_media_upload: response
_gchat_media_upload-->>post_message: log success/failure
end
alt files-only (no text/card)
post_message-->>Client: RawMessage({files})
else has text or card
post_message->>GChatAPI: POST standard card/text API
GChatAPI-->>post_message: response
post_message-->>Client: result
end
else no files
post_message->>GChatAPI: POST standard card/text API
GChatAPI-->>post_message: response
post_message-->>Client: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements file upload support for Google Chat by introducing a multipart media upload method (_gchat_media_upload) and integrating it into post_message. It also adds corresponding unit tests. The review feedback suggests improving the robustness of the text detection logic (has_text) to correctly handle dictionary-based messages and avoid truthiness issues with is not None checks. Additionally, it recommends uploading files concurrently using asyncio.gather to improve performance, alongside ensuring explicit is not None checks for optional parameters.
| has_text = ( | ||
| isinstance(message, str) | ||
| or (hasattr(message, "raw") and getattr(message, "raw", None)) | ||
| or (hasattr(message, "markdown") and getattr(message, "markdown", None)) | ||
| or (hasattr(message, "ast") and getattr(message, "ast", None)) | ||
| ) |
There was a problem hiding this comment.
If message is passed as a dict (which is supported and handled by extract_files and extract_card), the has_text check will evaluate to False because it only checks attributes using hasattr and getattr. This causes any text content in a dict-based message to be silently dropped when files are present, returning early with only the files uploaded. Adding a check for dictionary keys ensures robust behavior. Additionally, use is not None checks instead of truthiness checks to avoid silently ignoring falsy but valid values (like empty strings).
| has_text = ( | |
| isinstance(message, str) | |
| or (hasattr(message, "raw") and getattr(message, "raw", None)) | |
| or (hasattr(message, "markdown") and getattr(message, "markdown", None)) | |
| or (hasattr(message, "ast") and getattr(message, "ast", None)) | |
| ) | |
| has_text = ( | |
| isinstance(message, str) | |
| or (isinstance(message, dict) and any(message.get(k) is not None for k in ("raw", "markdown", "ast"))) | |
| or (hasattr(message, "raw") and getattr(message, "raw", None) is not None) | |
| or (hasattr(message, "markdown") and getattr(message, "markdown", None) is not None) | |
| or (hasattr(message, "ast") and getattr(message, "ast", None) is not None) | |
| ) |
References
- When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use
is not Noneinstead of a truthiness check to avoid silently ignoring them.
| for file in files: | ||
| try: | ||
| boundary = f"boundary_{_random_id()}" | ||
| metadata: dict[str, Any] = {"text": label} | ||
| if thread_name: | ||
| metadata["thread"] = {"name": thread_name} | ||
|
|
||
| body = ( | ||
| f"--{boundary}\r\n" | ||
| f"Content-Type: application/json; charset=UTF-8\r\n\r\n" | ||
| f"{json.dumps(metadata)}\r\n" | ||
| f"--{boundary}\r\n" | ||
| f"Content-Type: {file.mime_type or 'application/octet-stream'}\r\n\r\n" | ||
| ).encode() + file.data + f"\r\n--{boundary}--".encode() | ||
|
|
||
| async with session.request( | ||
| "POST", | ||
| upload_url, | ||
| data=body, | ||
| params={"uploadType": "multipart"}, | ||
| headers={ | ||
| "Authorization": f"Bearer {token}", | ||
| "Content-Type": f"multipart/related; boundary={boundary}", | ||
| }, | ||
| ) as response: | ||
| if response.status >= 400: | ||
| error_text = await response.text() | ||
| self._logger.error( | ||
| f"GChat media upload failed for {file.filename}", | ||
| {"status": response.status, "error": error_text}, | ||
| ) | ||
| else: | ||
| self._logger.debug( | ||
| "GChat media upload succeeded", | ||
| {"filename": file.filename}, | ||
| ) | ||
| except Exception: | ||
| self._logger.error( | ||
| f"GChat media upload failed for {file.filename}", | ||
| {"exc_info": True}, | ||
| ) |
There was a problem hiding this comment.
Uploading files sequentially can be slow, especially when multiple files are attached. Since Google Chat does not support batching, we can perform the uploads concurrently using asyncio.gather to improve performance and efficiency. Additionally, ensure we use explicit is not None checks for optional values like thread_name and mime_type to avoid issues with falsy but valid values.
async def _upload_single_file(file: FileUpload) -> None:
try:
boundary = f"boundary_{_random_id()}"
metadata: dict[str, Any] = {"text": label}
if thread_name is not None:
metadata["thread"] = {"name": thread_name}
body = (
f"--{boundary}\r\n"
f"Content-Type: application/json; charset=UTF-8\r\n\r\n"
f"{json.dumps(metadata)}\r\n"
f"--{boundary}\r\n"
f"Content-Type: {file.mime_type if file.mime_type is not None else 'application/octet-stream'}\r\n\r\n"
).encode() + file.data + f"\r\n--{boundary}--".encode()
async with session.request(
"POST",
upload_url,
data=body,
params={"uploadType": "multipart"},
headers={
"Authorization": f"Bearer {token}",
"Content-Type": f"multipart/related; boundary={boundary}",
},
) as response:
if response.status >= 400:
error_text = await response.text()
self._logger.error(
f"GChat media upload failed for {file.filename}",
{"status": response.status, "error": error_text},
)
else:
self._logger.debug(
"GChat media upload succeeded",
{"filename": file.filename},
)
except Exception:
self._logger.error(
f"GChat media upload failed for {file.filename}",
{"exc_info": True},
)
await asyncio.gather(*(_upload_single_file(file) for file in files))References
- When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use
is not Noneinstead of a truthiness check to avoid silently ignoring them.
![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: 8e1e21a849
ℹ️ 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".
|
|
||
| token = await self._get_access_token() | ||
| session = await self._get_http_session() | ||
| upload_url = f"https://chat.googleapis.com/upload/v1/{space_name}/messages" |
There was a problem hiding this comment.
When any file is posted, this builds https://chat.googleapis.com/upload/v1/{space}/messages, but the Google Chat media API upload URI is /upload/v1/{parent=spaces/*}/attachments:upload and it returns an attachmentDataRef that must be passed in a subsequent spaces.messages.create attachment field. As written, file uploads go to the wrong resource and the response is ignored, so file-only messages can return success while no attachment is delivered.
Useful? React with 👍 / 👎.
| token = await self._get_access_token() | ||
| session = await self._get_http_session() |
There was a problem hiding this comment.
For the default service-account/ADC bot configuration, _get_access_token() mints a token using GCHAT_SCOPES (chat.bot, read/reaction scopes), but Google Chat media upload requires user authentication with chat.messages.create or chat.messages. In those deployments every file upload gets a 403 and is swallowed by the new error handling, so users see a successful post with missing files.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/chat_sdk/adapters/google_chat/adapter.py (1)
1425-1434: 💤 Low valueRedundant
extract_cardcall.
extract_card(message)is called at line 1425 and again at line 1434. Since the function has no side effects, the second call is wasteful.♻️ Remove redundant call
has_text = ( isinstance(message, str) or (hasattr(message, "raw") and getattr(message, "raw", None)) or (hasattr(message, "markdown") and getattr(message, "markdown", None)) or (hasattr(message, "ast") and getattr(message, "ast", None)) ) card = extract_card(message) if not (has_text or card): return RawMessage( id=f"file-{int(time.time() * 1000)}", thread_id=thread_id, raw={"files": files}, ) - # Check if message contains a card - card = extract_card(message) - - if card: + if card: # already computed above when files were presentNote: This fix is incomplete since
cardis only computed inside theif files:block. You'd need to movecard = extract_card(message)outside that block to avoid the redundant call in all paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat_sdk/adapters/google_chat/adapter.py` around lines 1425 - 1434, The code calls extract_card(message) twice (once inside the files-handling branch and again after it), which is redundant; compute card once before the files conditional by moving the extract_card(message) call above the block that checks files, remove the later duplicate call, and ensure all uses reference the single variable card (look for the extract_card function and the card variable in the files-handling branch and the subsequent "Check if message contains a card" section).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chat_sdk/adapters/google_chat/adapter.py`:
- Line 71: The file src/chat_sdk/adapters/google_chat/adapter.py failed ruff
formatting; run the formatter (e.g., `ruff format
src/chat_sdk/adapters/google_chat/adapter.py`) or apply the equivalent styling
changes so the import list (including FileUpload) and surrounding code comply
with ruff rules; ensure the import line containing FileUpload and the adapter
module's formatting are fixed and then re-run `ruff format --check` to verify.
In `@tests/test_google_chat_adapter.py`:
- Around line 390-421: Run ruff format on the test file and add the missing
newline at EOF: run `ruff format tests/test_google_chat_adapter.py` to fix
formatting in the test_post_message_file_upload_failure_does_not_raise test
(which references GoogleChatThreadId, encode_thread_id, PostableMarkdown,
FileUpload, adapter._get_access_token, adapter._get_http_session, and
adapter.post_message) and ensure the file ends with a trailing newline after the
last line.
---
Nitpick comments:
In `@src/chat_sdk/adapters/google_chat/adapter.py`:
- Around line 1425-1434: The code calls extract_card(message) twice (once inside
the files-handling branch and again after it), which is redundant; compute card
once before the files conditional by moving the extract_card(message) call above
the block that checks files, remove the later duplicate call, and ensure all
uses reference the single variable card (look for the extract_card function and
the card variable in the files-handling branch and the subsequent "Check if
message contains a card" section).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78c83cd4-a174-4c9e-963c-76df9c48339e
📒 Files selected for processing (2)
src/chat_sdk/adapters/google_chat/adapter.pytests/test_google_chat_adapter.py
| EphemeralMessage, | ||
| FetchOptions, | ||
| FetchResult, | ||
| FileUpload, |
There was a problem hiding this comment.
Pipeline failure: ruff formatting required.
The CI pipeline reports that ruff format --check failed for this file. Run ruff format src/chat_sdk/adapters/google_chat/adapter.py to fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/adapters/google_chat/adapter.py` at line 71, The file
src/chat_sdk/adapters/google_chat/adapter.py failed ruff formatting; run the
formatter (e.g., `ruff format src/chat_sdk/adapters/google_chat/adapter.py`) or
apply the equivalent styling changes so the import list (including FileUpload)
and surrounding code comply with ruff rules; ensure the import line containing
FileUpload and the adapter module's formatting are fixed and then re-run `ruff
format --check` to verify.
| @pytest.mark.asyncio | ||
| async def test_post_message_file_upload_failure_does_not_raise(self): | ||
| """A file upload network error must not propagate out of post_message -- | ||
| a transport hiccup cannot kill a turn whose text response already landed.""" | ||
| from unittest.mock import AsyncMock | ||
|
|
||
| from chat_sdk.adapters.google_chat.thread_utils import GoogleChatThreadId, encode_thread_id | ||
| from chat_sdk.types import FileUpload, PostableMarkdown | ||
|
|
||
| adapter = _make_adapter() | ||
| adapter._get_access_token = AsyncMock(return_value="tok") # type: ignore[method-assign] | ||
|
|
||
| class _FailResponse: | ||
| async def __aenter__(self): | ||
| raise RuntimeError("network error") | ||
|
|
||
| async def __aexit__(self, *args): | ||
| pass | ||
|
|
||
| class _FailSession: | ||
| def request(self, method: str, url: str, **kwargs) -> _FailResponse: | ||
| return _FailResponse() | ||
|
|
||
| adapter._get_http_session = AsyncMock(return_value=_FailSession()) # type: ignore[method-assign] | ||
|
|
||
| thread_id = encode_thread_id(GoogleChatThreadId(space_name="spaces/TEST")) | ||
| message = PostableMarkdown( | ||
| markdown="", | ||
| files=[FileUpload(data=b"x", filename="file.txt", mime_type="text/plain")], | ||
| ) | ||
|
|
||
| await adapter.post_message(thread_id, message) |
There was a problem hiding this comment.
Pipeline failure: ruff formatting required & missing trailing newline.
The CI reports ruff format --check failed. Run ruff format tests/test_google_chat_adapter.py and ensure the file ends with a newline after line 421.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_google_chat_adapter.py` around lines 390 - 421, Run ruff format on
the test file and add the missing newline at EOF: run `ruff format
tests/test_google_chat_adapter.py` to fix formatting in the
test_post_message_file_upload_failure_does_not_raise test (which references
GoogleChatThreadId, encode_thread_id, PostableMarkdown, FileUpload,
adapter._get_access_token, adapter._get_http_session, and adapter.post_message)
and ensure the file ends with a trailing newline after the last line.
1 participant
Summary
_gchat_media_upload()toGoogleChatAdapter: POSTs each file tohttps://chat.googleapis.com/upload/v1/{space}/messages?uploadType=multipart(one request per file -- the GChat upload API does not support batching)post_message(): uploads files first, then returns early when the message carries no text or card (mirrors the Slack adapter pattern atslack/adapter.py:3261-3277)Test plan
TestGChatMediaUpload::test_post_message_sends_one_multipart_upload_per_file-- two files produce two POSTs to/upload/v1/withuploadType=multipartTestGChatMediaUpload::test_files_only_post_message_does_not_emit_text_message-- files-only message returns early without posting to the standard messages endpointTestGChatMediaUpload::test_post_message_file_upload_failure_does_not_raise-- network error during upload is swallowed;post_messagedoes not raiseuv run pytest tests/test_google_chat_adapter.py)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests