Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 80 additions & 4 deletions src/chat_sdk/adapters/google_chat/adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
EphemeralMessage,
FetchOptions,
FetchResult,
FileUpload,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

FormattedContent,
ListThreadsOptions,
ListThreadsResult,
Expand Down Expand Up @@ -430,6 +431,72 @@ async def _gchat_api_request(
)
return result

# =========================================================================
# Media upload
# =========================================================================

async def _gchat_media_upload(
self,
files: list[FileUpload],
space_name: str,
thread_name: str | None,
label: str,
) -> None:
"""Upload files to a GChat space via the multipart media upload endpoint.

One request is sent per file -- the GChat upload API does not support batching.
Per-file errors are logged but never raised so a transport hiccup cannot kill
a turn whose text response already landed.
"""
if not files:
return

token = await self._get_access_token()
session = await self._get_http_session()
Comment on lines +454 to +455
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Authenticate media uploads as a user

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 👍 / 👎.

upload_url = f"https://chat.googleapis.com/upload/v1/{space_name}/messages"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Use the attachments upload flow

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 👍 / 👎.


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},
)
Comment on lines +458 to +498
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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
  1. When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use is not None instead of a truthiness check to avoid silently ignoring them.


# =========================================================================
# Lifecycle
# =========================================================================
Expand Down Expand Up @@ -1346,13 +1413,22 @@ async def post_message(
thread_name = decoded.thread_name

try:
# Check for files - currently not implemented for GChat
files = extract_files(message)
if files:
self._logger.warn(
"File uploads are not yet supported for Google Chat. Files will be ignored.",
{"fileCount": len(files)},
await self._gchat_media_upload(files, space_name, thread_name, "")
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))
)
Comment on lines +1419 to 1424
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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).

Suggested change
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
  1. When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use is not None instead of a truthiness check to avoid silently ignoring them.

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)
Expand Down
135 changes: 135 additions & 0 deletions tests/test_google_chat_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,3 +284,138 @@ def test_is_trusted_gchat_download_url_allowlist(self):
assert not GoogleChatAdapter._is_trusted_gchat_download_url("https://attacker.example/x")
# Rejects look-alikes
assert not GoogleChatAdapter._is_trusted_gchat_download_url("https://chat.googleapis.com.attacker.tld/x")


# ---------------------------------------------------------------------------
# _gchat_media_upload
# ---------------------------------------------------------------------------


class TestGChatMediaUpload:
"""GChat file delivery via the media upload endpoint, exercised through post_message."""

@staticmethod
def _make_fake_session():
"""Return (session, calls_list). calls_list accumulates every request kwargs dict."""
calls: list[dict] = []

class _FakeResponse:
status = 200

async def json(self) -> dict:
return {"name": "spaces/TEST/messages/1"}

async def text(self) -> str:
return ""

async def __aenter__(self):
return self

async def __aexit__(self, *args):
pass

class _FakeSession:
def request(self, method: str, url: str, **kwargs) -> _FakeResponse:
calls.append({"method": method, "url": url, **kwargs})
return _FakeResponse()

return _FakeSession(), calls

@pytest.mark.asyncio
async def test_post_message_sends_one_multipart_upload_per_file(self):
"""Two files produce two POSTs to the GChat media upload endpoint with
uploadType=multipart -- the upload API does not support batching."""
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]
session, calls = self._make_fake_session()
adapter._get_http_session = AsyncMock(return_value=session) # type: ignore[method-assign]

thread_id = encode_thread_id(GoogleChatThreadId(space_name="spaces/TEST"))
message = PostableMarkdown(
markdown="",
files=[
FileUpload(data=b"csv", filename="data.csv", mime_type="text/csv"),
FileUpload(data=b"\x89PNG", filename="chart.png", mime_type="image/png"),
],
)

await adapter.post_message(thread_id, message)

upload_calls = [call for call in calls if "/upload/v1/" in call["url"]]
assert len(upload_calls) == 2, (
f"expected one media upload call per file; got {len(upload_calls)}. "
"GChat's upload endpoint does not batch -- see _gchat_media_upload in google_chat/adapter.py."
)
for call in upload_calls:
assert call.get("params", {}).get("uploadType") == "multipart", (
"uploadType=multipart query param required for GChat media upload; "
f"got params={call.get('params')}"
)

@pytest.mark.asyncio
async def test_files_only_post_message_does_not_emit_text_message(self):
"""post_message with files and no text/card returns early after uploading --
a separate empty text message must not be posted to the standard endpoint."""
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]
session, calls = self._make_fake_session()
adapter._get_http_session = AsyncMock(return_value=session) # type: ignore[method-assign]

thread_id = encode_thread_id(GoogleChatThreadId(space_name="spaces/TEST"))
message = PostableMarkdown(
markdown="",
files=[FileUpload(data=b"hello", filename="report.txt", mime_type="text/plain")],
)

result = await adapter.post_message(thread_id, message)

standard_api_calls = [call for call in calls if "/upload/v1/" not in call["url"]]
assert standard_api_calls == [], (
"post_message must return early after file upload when there is no text or card -- "
"an empty text message alongside files is unwanted. "
f"unexpected standard-API calls: {standard_api_calls}"
)
assert result is not None

@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)
Comment on lines +390 to +421
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Loading