-
Notifications
You must be signed in to change notification settings - Fork 235
feat: add conversations_unreads and conversations_mark tools (rebased #146) #171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for cherry picking. One of the blockers on #146 was to manually test |
|
Tested with both ✅ Working
Details
Test Environment
RecommendationThe
For Let me know if you'd like us to add these notes to the README or if you want to test differently! |
|
@Flare576 please could you figure out, case by case basis what kind of issues we have here and what needs to be fixed before we merge it. Because according to your last message, we can't merge it as is. |
|
Did more digging. Here's the actual status: ✅
|
| Tool | xoxp | xoxb | xoxc/xoxd |
|---|---|---|---|
conversations_mark |
✅ | ❓ untested | ✅ |
conversations_unreads |
❌ not_allowed_token_type |
❌ | ✅ |
The PR is safe to merge if we document that conversations_unreads requires stealth mode. Want me to add that to the README?
|
I was able to successfully build a method in python to get a list of unread conversations using an def is_read(conv: Conversation) -> bool:
logfire.info("Cache miss for is_read")
client = get_slack_client()
response: ConversationInfoResponse = client.conversations_info(
channel=conv.id
)
# Check unread_count directly from the typed response
return (
response.channel.unread_count == 0
# Some channels don't have an unread count, not because they are read
# but because the channel doesn't support unread counts (lack of permissions, etc.)
if response.channel.unread_count is not None
else False
)
def fetch_conversations(
# Direct messages only (mpim = multi-person IMs disabled)
types: str = "im",
unread_only: bool = False,
) -> Generator[Conversation, Any, None]:
"""
Fetches direct messages and group DMs from Slack.
Returns:
Iterator[Conversation]: Iterator of conversation objects.
"""
cursor: str | None = None
while tupled_result := get_conversations(cursor, types):
channels, cursor = tupled_result
logfire.info(f"next_cursor: {cursor}")
yield from (
channel
for channel in channels
if not unread_only or not is_read(channel)
)
logfire.info(f"Yielded {len(channels)} channels")
if not cursor:
break |
5b36534 to
ddc3d78
Compare
|
I made a PR stacked on top of this one that adds support for XOXP #199 |
|
Thanks for the suggestion @niebloomj! We've implemented a fallback for How it works:
The fallback respects the Verified working with both token types:
Commit: b94448d |
Hey @Flare576 ! Thanks for implementing the xoxp fallback - glad the approach worked! Would you be open to adding a co-author credit for the implementation? You can amend the commit (b94448d) to include: Co-authored-by: @niebloomj [email protected] No worries if not - happy to help either way! 🙂 |
b94448d to
dab94ff
Compare
|
Done! Added the co-author credit - thanks again for the approach @niebloomj! 🙌 Updated commit: dab94ff |
Thank you! I am very excited about this PR :) |
|
hey, been testing this on a production workspace with 100+ channels/DMs using both found an issue where the xoxp fallback path silently never triggers. tl;dr: the root cause is in the edge client — fwiw this pattern extends to other edge methods too — only one-line fix in r := ClientCountsResponse{}
if err := cl.ParseResponse(&r, resp); err != nil {
return ClientCountsResponse{}, err
}
+ if err := r.validate("client.counts"); err != nil {
+ return ClientCountsResponse{}, err
+ }
return r, nilverified by testing both paths end-to-end through the MCP tool:
also safe for the other caller of separate follow-up note on the xoxp fallback's scalability: |
|
couple more things from testing this on the same workspace: 1. bug: range-by-value in the message-fetch loop the message-fetch loop uses 2. filtering muted channels this was the biggest noise source in practice — the majority of channels showing as "unread" were actually muted. mute state isn't in 3. backfilling real unread counts for channels
i've got all three working and tested — happy to open a PR against your branch if useful |
…rieval - Uses ClientUserBoot to get all channels with LastRead/Latest in one API call - Filters channels where Latest > LastRead to find unreads - Prioritizes: DMs > group DMs > partner channels (ext-*) > internal - Only fetches message history for channels with actual unreads - Supports filtering by channel type and configurable limits Addresses issue korotovsky#114
- Switch from ClientUserBoot to ClientCounts API - ClientCounts returns HasUnreads boolean for all channels - Add ClientCounts to SlackAPI interface - Process Channels, MPIMs, and IMs separately
- Strip existing # prefix before adding to avoid ##name - Use stripped name for ext-/shared- prefix checks for partner type
- Add mentions_only parameter to conversations_unreads to filter channels to only those with @mentions (priority inbox) - Add conversations_mark tool to mark channels/DMs as read - Supports channel IDs, #channel names, and @username - If no timestamp provided, marks all messages as read
…nels - Add IsExtShared field to Channel struct in cache - Pass IsExtShared through mapChannel function - Use cached.IsExtShared to identify external/partner channels instead of checking for ext-/shared- name prefixes Note: Users may need to delete their channels cache file to repopulate with the new IsExtShared field.
- Extract handler params to structs with parsing functions (unreadsParams, markParams) following existing patterns - Add SLACK_MCP_MARK_TOOL env var guard for conversations_mark (disabled by default, requires explicit opt-in) - Remove unused categorizeChannel and getChannelDisplayName functions - Update README with conversations_mark safety note and env var docs
…ations.info Co-authored-by: niebloomj <[email protected]>
dab94ff to
e238772
Compare
Summary
Rebases @saoudrizwan's work from #146 onto current master, as requested by @korotovsky.
Original PR: #146 by @saoudrizwan - all credit for the feature design and implementation goes to them.
What this PR does:
New Tools (from #146)
conversations_unreadsEfficiently retrieves unread messages across all channels using a single API call.
conversations_markMarks a channel or DM as read.
SLACK_MCP_MARK_TOOLenv var (disabled by default)#channel-name, and@usernameConflict Resolutions
README.mdconversations.go(structs)addReactionParams,filesGetParams,unreadsParams,markParamsconversations.go(parsers)Testing
conversations_unreadsreturns valid CSVattachment_get_datastill works after rebaseReady for review and merge when you are, @korotovsky!