Skip to content

Commit 8dd34d1

Browse files
committed
address coderabbit review findings
- [critical] thread._fallback_stream: wrap the placeholder-clear edit_message(" ") in try/except so Telegram (which raises ValidationError on whitespace-only text) logs and falls back to upstream's stranded-placeholder behavior instead of propagating a crash up through thread.post(). - [major] pyrefly python-version 3.11 -> 3.10 to match requires-python. Otherwise type-check target drifts ahead of the package's claimed support floor. - [major] test_chat_faithful.py: the fallback-streaming test now verifies the final posted markdown is "Hi", not just that some post happened on the right thread. - [minor] test_thread_faithful.py: three "no empty content" tests were vacuous — they iterated PostableMarkdown calls inside an isinstance guard but never asserted that any calls happened, so the tests passed even if the SDK emitted nothing. Now assert at least one matching call before checking content. - [nit] channel.py / thread.py from_json: replace `or` truthiness chain on camel-to-snake fallback keys with explicit None checks. `""` is a valid-but-falsy value that shouldn't silently fall through to the alias. https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG
1 parent c72a943 commit 8dd34d1

5 files changed

Lines changed: 56 additions & 27 deletions

File tree

pyproject.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ project-excludes = ["**/__pycache__", "**/.pytest_cache"]
9393
disable-project-excludes-heuristics = true
9494
use-ignore-files = false
9595

96-
python-version = "3.11"
96+
python-version = "3.10"
9797
python-platform = "linux"
9898

9999
# Optional adapter deps that aren't in the default install. Treating them as

src/chat_sdk/channel.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -389,7 +389,7 @@ def to_json(self) -> dict[str, Any]:
389389
return {
390390
"_type": "chat:Channel",
391391
"id": self._id,
392-
"adapterName": self._adapter_name or self.adapter.name,
392+
"adapterName": self._adapter_name if self._adapter_name else self.adapter.name,
393393
"channelVisibility": self._channel_visibility,
394394
"isDM": self._is_dm,
395395
}
@@ -418,11 +418,18 @@ def from_json(
418418
"""
419419
if isinstance(data, ChannelImpl):
420420
return data
421+
# Explicit None-checks (not `or`) to avoid the truthiness trap:
422+
# `""` is a valid-but-falsy value that shouldn't silently fall
423+
# through to the snake_case alias. See UPSTREAM_SYNC.md hazard #1.
424+
raw_adapter_name = data["adapterName"] if "adapterName" in data else data.get("adapter_name")
425+
raw_channel_visibility = (
426+
data["channelVisibility"] if "channelVisibility" in data else data.get("channel_visibility")
427+
)
421428
channel = cls(
422429
_ChannelImplConfigLazy(
423430
id=data["id"],
424-
adapter_name=data.get("adapterName") or data.get("adapter_name", ""),
425-
channel_visibility=data.get("channelVisibility") or data.get("channel_visibility", "unknown"),
431+
adapter_name=raw_adapter_name if raw_adapter_name is not None else "",
432+
channel_visibility=raw_channel_visibility if raw_channel_visibility is not None else "unknown",
426433
is_dm=data.get("isDM") if "isDM" in data else data.get("is_dm", False),
427434
)
428435
)

src/chat_sdk/thread.py

Lines changed: 32 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -680,12 +680,23 @@ async def _edit_loop() -> None:
680680
# "..." on the message forever. We replace it with " " so the
681681
# placeholder is cleared consistently with the no-placeholder branch
682682
# (which also posts " " in this case). See docs/UPSTREAM_SYNC.md.
683-
await self.adapter.edit_message(
684-
thread_id_for_edits,
685-
msg.id,
686-
PostableMarkdown(markdown=" "),
687-
)
688-
last_edit_content = " "
683+
#
684+
# Some adapters (e.g. Telegram) reject whitespace-only content with
685+
# a ValidationError. If the edit fails, we log and fall back to
686+
# upstream's "leave placeholder visible" behavior for that adapter.
687+
try:
688+
await self.adapter.edit_message(
689+
thread_id_for_edits,
690+
msg.id,
691+
PostableMarkdown(markdown=" "),
692+
)
693+
last_edit_content = " "
694+
except Exception as exc:
695+
if self._logger:
696+
self._logger.warn(
697+
"fallbackStream placeholder-clear edit failed; placeholder will remain visible",
698+
exc,
699+
)
689700

690701
sent = self._create_sent_message(
691702
msg.id,
@@ -733,7 +744,7 @@ def to_json(self) -> dict[str, Any]:
733744
"channelVisibility": self._channel_visibility,
734745
"currentMessage": self._current_message.to_json() if self._current_message else None,
735746
"isDM": self._is_dm,
736-
"adapterName": self._adapter_name or self.adapter.name,
747+
"adapterName": self._adapter_name if self._adapter_name else self.adapter.name,
737748
}
738749

739750
@classmethod
@@ -762,17 +773,26 @@ def from_json(
762773
"""
763774
if isinstance(data, ThreadImpl):
764775
return data
765-
current_msg_raw = data.get("currentMessage") or data.get("current_message")
776+
# Explicit None-checks (not `or`) to avoid the truthiness trap:
777+
# `""` is a valid-but-falsy value that shouldn't silently fall
778+
# through to the snake_case alias. See UPSTREAM_SYNC.md hazard #1.
779+
raw_current = data["currentMessage"] if "currentMessage" in data else data.get("current_message")
766780
# ``object_hook`` revives nested dicts first, so ``currentMessage`` may
767781
# already be a Message instance by the time this runs.
768-
current_msg = Message.from_json(current_msg_raw) if current_msg_raw else None
782+
current_msg = Message.from_json(raw_current) if raw_current is not None else None
783+
784+
raw_adapter_name = data["adapterName"] if "adapterName" in data else data.get("adapter_name")
785+
raw_channel_id = data["channelId"] if "channelId" in data else data.get("channel_id")
786+
raw_channel_visibility = (
787+
data["channelVisibility"] if "channelVisibility" in data else data.get("channel_visibility")
788+
)
769789

770790
thread = cls(
771791
_ThreadImplConfig(
772792
id=data["id"],
773-
adapter_name=data.get("adapterName") or data.get("adapter_name", ""),
774-
channel_id=data.get("channelId") or data.get("channel_id", ""),
775-
channel_visibility=data.get("channelVisibility") or data.get("channel_visibility", "unknown"),
793+
adapter_name=raw_adapter_name if raw_adapter_name is not None else "",
794+
channel_id=raw_channel_id if raw_channel_id is not None else "",
795+
channel_visibility=raw_channel_visibility if raw_channel_visibility is not None else "unknown",
776796
current_message=current_msg,
777797
is_dm=data.get("isDM") if "isDM" in data else data.get("is_dm", False),
778798
)

tests/test_chat_faithful.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -348,8 +348,10 @@ async def _stream():
348348
# guard defers the first commit until stream end, so the final content
349349
# arrives via post rather than an intermediate edit.
350350
assert len(adapter._post_calls) >= 1
351-
last_post = adapter._post_calls[-1]
352-
assert last_post[0] == "slack:C123:1234.5678"
351+
last_thread_id, last_content = adapter._post_calls[-1]
352+
assert last_thread_id == "slack:C123:1234.5678"
353+
last_markdown = last_content.markdown if hasattr(last_content, "markdown") else last_content
354+
assert last_markdown == "Hi"
353355

354356

355357
# ============================================================================

tests/test_thread_faithful.py

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -439,9 +439,9 @@ async def test_should_not_post_empty_content_when_table_is_buffered_with_null_pl
439439
text_stream = _create_text_stream(["| A | B |\n", "|---|---|\n", "| 1 | 2 |\n"])
440440
await thread.post(text_stream)
441441

442-
for _, content in adapter._post_calls:
443-
if isinstance(content, PostableMarkdown):
444-
assert len(content.markdown.strip()) > 0
442+
markdown_posts = [content for _, content in adapter._post_calls if isinstance(content, PostableMarkdown)]
443+
assert markdown_posts, "expected at least one PostableMarkdown post"
444+
assert all(p.markdown.strip() for p in markdown_posts)
445445

446446
# it("should not edit placeholder to empty during LLM warm-up")
447447
@pytest.mark.asyncio
@@ -453,9 +453,9 @@ async def test_should_not_edit_placeholder_to_empty_during_llm_warmup(self):
453453
text_stream = _create_text_stream(["Hello world"])
454454
await thread.post(text_stream)
455455

456-
for _, _, content in adapter._edit_calls:
457-
if isinstance(content, PostableMarkdown):
458-
assert len(content.markdown.strip()) > 0
456+
markdown_edits = [content for _, _, content in adapter._edit_calls if isinstance(content, PostableMarkdown)]
457+
assert markdown_edits, "expected at least one PostableMarkdown edit"
458+
assert all(e.markdown.strip() for e in markdown_edits)
459459

460460
# it("should not post empty content during streaming with whitespace chunks")
461461
@pytest.mark.asyncio
@@ -467,9 +467,9 @@ async def test_should_not_post_empty_content_during_streaming_with_whitespace_ch
467467
text_stream = _create_text_stream([" ", "\n", " \n"])
468468
await thread.post(text_stream)
469469

470-
for _, content in adapter._post_calls:
471-
if isinstance(content, PostableMarkdown):
472-
assert len(content.markdown) > 0
470+
markdown_posts = [content for _, content in adapter._post_calls if isinstance(content, PostableMarkdown)]
471+
assert markdown_posts, "expected at least one PostableMarkdown post"
472+
assert all(len(p.markdown) > 0 for p in markdown_posts)
473473

474474
# it("should preserve newlines in streamed text (native path)")
475475
@pytest.mark.asyncio

0 commit comments

Comments
 (0)