feat(thread): port getParticipants + close 8 fidelity gaps#64
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| async def get_participants(self) -> list[Author]: | ||
| """Return unique non-bot, non-self authors who've posted in the thread.""" | ||
| ... |
There was a problem hiding this comment.
False positive — this is the standard typing.Protocol method body per PEP 544. Consistent with every other Protocol method in this file.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_thread_faithful.py (1)
2585-2616: Add an explicit self-only exclusion test (is_me=True,is_bot=False).Current bot-focused case won’t fail if
is_mefiltering regresses while bot filtering remains intact.Suggested test addition
+ `@pytest.mark.asyncio` + async def test_should_exclude_self_messages_even_when_not_bot(self): + adapter = create_mock_adapter() + state = create_mock_state() + + self_human_msg = create_test_message( + "self-1", + "my own message", + author=Author(user_id="U_SELF", user_name="me", full_name="Me", is_bot=False, is_me=True), + ) + other_human_msg = create_test_message( + "other-1", + "other user", + author=Author(user_id="U1", user_name="alice", full_name="Alice", is_bot=False, is_me=False), + ) + + adapter.fetch_messages = AsyncMock( # type: ignore[assignment] + return_value=FetchResult(messages=[self_human_msg, other_human_msg], next_cursor=None) + ) + + thread = _make_thread(adapter, state) + participants = await thread.get_participants() + + assert [p.user_id for p in participants] == ["U1"]As per coding guidelines, "Every test must fail when the code is wrong."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_thread_faithful.py` around lines 2585 - 2616, The test currently only verifies bot exclusion (is_bot=True) but misses a case where a message is from self (is_me=True, is_bot=False), so add an extra assertion within test_should_exclude_bot_messages (or a new small test) that includes a create_test_message with Author(... is_bot=False, is_me=True) in the fetched messages via adapter.fetch_messages and assert that thread.get_participants() still excludes that self message (i.e., participants length remains 1 and the remaining user_id is "U1"); update the AsyncMock return_value to include this self-only message alongside the human message to ensure is_me-based filtering is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_thread_faithful.py`:
- Around line 2585-2616: The test currently only verifies bot exclusion
(is_bot=True) but misses a case where a message is from self (is_me=True,
is_bot=False), so add an extra assertion within test_should_exclude_bot_messages
(or a new small test) that includes a create_test_message with Author(...
is_bot=False, is_me=True) in the fetched messages via adapter.fetch_messages and
assert that thread.get_participants() still excludes that self message (i.e.,
participants length remains 1 and the remaining user_id is "U1"); update the
AsyncMock return_value to include this self-only message alongside the human
message to ensure is_me-based filtering is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04ddb6fb-bd92-4743-bde9-5dc34ca880a1
📒 Files selected for processing (5)
CHANGELOG.mdsrc/chat_sdk/thread.pysrc/chat_sdk/types.pytests/test_chat_faithful.pytests/test_thread_faithful.py
![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: 9ec8020ac9
ℹ️ 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".
| if ( | ||
| self._current_message is not None | ||
| and not self._current_message.author.is_me | ||
| and not self._current_message.author.is_bot |
There was a problem hiding this comment.
Author.is_bot is typed as bool | "unknown", but this check relies on truthiness, so authors with is_bot="unknown" are treated as bots and excluded from participants. That causes get_participants() to silently drop valid users in adapters that emit unknown bot status (for example Telegram fallback authors), so the method can return incomplete results even when non-self humans posted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional upstream parity — in TS, !author.isBot also excludes the string "unknown" because non-empty strings are truthy in JS. Explicit is True would diverge from vercel/chat. See thread.ts:359-384.
![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 the Thread.get_participants() method to retrieve unique, non-bot, and non-self authors from a thread, aligning with the upstream TypeScript SDK. It also includes new tests for the thread factory and participant retrieval. Feedback was provided regarding the truthiness check for bot status, which may inadvertently exclude authors with an "unknown" status if not explicitly checked against a boolean value.
| seen[self._current_message.author.user_id] = self._current_message.author | ||
|
|
||
| async for msg in self.all_messages(): | ||
| if msg.author.is_me or msg.author.is_bot or msg.author.user_id in seen: |
There was a problem hiding this comment.
The check msg.author.is_bot will evaluate to True if the value is the string "unknown", causing authors with unknown bot status to be excluded from the participants list. If the intention is to only exclude confirmed bots, consider using msg.author.is_bot is True. However, if this behavior is intended to maintain strict parity with the TypeScript SDK (where !author.isBot also excludes 'unknown'), then the current implementation is correct.
There was a problem hiding this comment.
Intentional upstream parity — in TS, !author.isBot on is_bot="unknown" also evaluates false (non-empty strings truthy). Python's if ... or author.is_bot: mirrors that. See upstream thread.ts:359-384.
Addresses bot review feedback on PR #64 — the existing bot-exclusion test doesn't isolate the is_me=True, is_bot=False path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review verdict: LGTM. Refreshed and reviewed latest head 431c5b6 against main. No issues found in getParticipants/thread factory changes. Verification: TestGetParticipants + TestThreadFactory passed (9 passed), full faithful chat/thread slice passed (232 passed, 2 skipped), and ruff passed on touched files. Formal GitHub approval is blocked because the authenticated account owns this PR. |
Adds Thread.get_participants() mirroring upstream TS Thread.getParticipants() (vercel-chat/packages/chat/src/thread.ts:359). Returns unique non-bot, non-self authors who've posted in the thread — seeds from current_message.author when eligible, then iterates all_messages() oldest-first and dedupes on user_id. Added to the Thread Protocol so consumers typed against the interface can call it. Also ports the 4 [getParticipants] tests from thread.test.ts and the 4 [thread] factory tests from chat.test.ts (existing-behavior coverage for Chat.thread(id) that had no Python equivalent). Closes 8 fidelity gaps; verify_test_fidelity now shows 32 missing, down from 40. Changelog entry under Unreleased — will batch into 0.4.26.2 alongside other Tier 1/2 parity fixes in flight. Refs #54. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses bot review feedback on PR #64 — the existing bot-exclusion test doesn't isolate the is_me=True, is_bot=False path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
patrick-chinchill
force-pushed
the
get-participants-54
branch
from
431c5b6 to
1523d27
Compare
Per gemini-code-assist review on PR #83. Without the repo prefix, GitHub auto-links the upstream PR numbers to local PRs in chat-sdk-python, which collides with the local refs (#64, #66, #67, #74, #82) elsewhere in the file. Use vercel/chat#NNN so the upstream refs link correctly. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
1 participant
Summary
Thread.get_participants()from upstream TS (vercel-chat/packages/chat/src/thread.ts:359). Returns unique non-bot, non-self authors who've posted in the thread; seeds fromcurrent_message.authorwhen eligible, iteratesall_messages()oldest-first, dedupes onuser_id. Added to theThreadProtocol.[getParticipants]tests fromthread.test.ts+ 4[thread]factory tests fromchat.test.ts(existing-behavior coverage forChat.thread(id)that had no Python equivalent).Refs #54. First of several upstream-parity PRs targeting bundled release
0.4.26.2. CHANGELOG entry lands under## Unreleased.Fidelity impact
verify_test_fidelity.py: 40 missing → 32 missing (closed exactly the 8 covered here). Remaining gaps trace to #50 (Options Load), #52 (rehydrate), and StreamingPlan feature work (#53).Test plan
uv run pytest tests/test_thread_faithful.py::TestGetParticipants -q— 4 passeduv run pytest tests/test_chat_faithful.py::TestThreadFactory -q— 4 passeduv run ruff check src/ tests/ scripts/— cleanuv run ruff format --check src/ tests/ scripts/— cleanuv run python scripts/audit_test_quality.py— 0 hard failuresuv run python scripts/verify_test_fidelity.py— 8 gaps closeduv run pytest tests/ --tb=short -q— 3553 passed, 2 skipped, 0 failedNotes
Thread.getParticipants()method #54's body sketched a simplified implementation that omittedis_mefiltering and usedmessages()(newest-first). I ported the real upstream: filters bothis_meandis_bot, and usesall_messages()(oldest-first) to match the map-seeding order.## Unreleasedper the0.4.26.1-style bundled-release pattern. A separate release PR will flip to## 0.4.26.2 (date)+ bumppyproject.tomlonce 2–3 items land.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
get_participants()method to threads for retrieving unique message participants, automatically excluding bots and the current user.Tests