fix(channels): decide what opens a turn, and make a DM one conversation - #6647
fix(channels): decide what opens a turn, and make a DM one conversation#6647mmabrouk wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 SummarySummary by CodeRabbit
WalkthroughSlack system notices are filtered before ingestion. Inbound events now carry addressing metadata. Channel routing applies trigger rules, private-conversation keys, fresh sessions, and thread-scoped forwardfill. ChangesChannel routing
Priority: ➖ Normal — Schedule the channel conversation-semantics change because it alters DM continuity, turn triggering, Slack message handling, and thread-scoped history across the channels surface. Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to Slack 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
|
Codex (astra, medium, simplify skill) reviewed this lane. Verdict: "request changes" on two points, both real, both fixed in the last commit:
Also applied: the dispatcher passes the capabilities it already holds into @coderabbitai review |
|
|
319b8e1 to
9b41cba
Compare
Three defects from the live QA, one seam: - Every message in a channel ran a paid turn, the bot's own invitation included (F83, F94). The trigger policy was computed and never read. The channel defaults now admit a mention, a command, or a button, and resolve() applies them at dispatch: every message is still stored (the fill reads it back as context), but a turn opens only for a DM, a reply inside a thread the agent already holds, an answer to a pending choice, a mention, or a command. The adapter's addressed verdict is carried into the stored event. Slack's system notices (channel_join and the like) are dropped in the adapter; thread_broadcast and file_share still carry a person's words and stay. - A fresh thread inherited the whole channel's history, and a DM obeyed the oldest instruction in it (F84). The forwardfill range is now the thread's own: everything in a DM, the same thread key elsewhere, the addressing event alone under message scope. - A DM was keyed per message, so the agent forgot each message as soon as it answered it, and every answer threaded under the message (F88). A DM is now one conversation keyed on the space, and its replies go out top-level. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
- A new thread row is a new conversation: it gets a fresh session id, so `!new` in a DM no longer resumes the closed row's session, and a DM's fill keeps only what arrived since its current thread began. - Message scope wins over the DM rule in the fill filter: one message per turn there, whatever the space kind. - The reply locator drops the platform's own thread-only fields instead of Slack's by name, so a bridge or the Agenta channel gets the same top-level DM reply. - compose_input takes the capabilities the dispatcher already holds and reads the connection only when a caller has none. - Tests: two-message DM continuity, `!new` then a message, a mention inside a held thread, and message-scope isolation in a DM. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
9b41cba to
cc30cdd
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 4e510b23-87c9-4f35-b10d-e547df2ecc2a
📒 Files selected for processing (7)
api/oss/src/apis/fastapi/channels/ingress.pyapi/oss/src/core/channels/adapters/slack/adapter.pyapi/oss/src/core/channels/dtos.pyapi/oss/src/core/channels/service.pyapi/oss/src/tasks/asyncio/channels/inbox.pyapi/oss/tests/pytest/unit/channels/slack/test_slack_adapter.pyapi/oss/tests/pytest/unit/channels/test_channels_service_routing.py
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
|
|
||
| # Message subtypes that are still a person's message: a reply also sent to the | ||
| # channel, and a file shared with a comment. Every other subtype is a notice. | ||
| _MESSAGE_SUBTYPES = {"thread_broadcast", "file_share"} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🌐 Web query:
Does Slack's current Events API define the me_messagesubtype as a user-authored/memessage event withchannel, user, text, and ts fields?
💡 Result:
Yes, Slack's Events API defines the me_message subtype for messages sent using the /me command, and it includes the channel, user, text, and ts fields [1][2]. When a user performs an action via the /me command, the Events API delivers a message event with the subtype field set to me_message [1][3]. The event object typically includes: - type: Always "message" [1][4]. - subtype: Always "me_message" [1][4]. - channel: The ID of the channel where the message was sent [1][4]. - user: The ID of the user who sent the message [1][4]. - text: The content of the message [1][4]. - ts: The timestamp of the message [1][4]. These fields are standard components of the me_message event structure as documented in Slack's developer resources [1][4].
Citations:
- 1: https://docs.slack.dev/reference/events/message/me_message.md
- 2: http://docs.slack.dev/reference/events/message/me_message
- 3: https://docs.slack.dev/reference/events/message
- 4: https://docs.slack.dev/tools/node-slack-sdk/reference/types/interfaces/MeMessageEvent
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- adapter.py relevant sections ---'
sed -n '1,90p;240,295p' api/oss/src/core/channels/adapters/slack/adapter.py
printf '%s\n' '--- references to subtype handling ---'
rg -n -C 3 '_MESSAGE_SUBTYPES|subtype|me_message' api/oss/src/core/channels/adapters/slackRepository: Agenta-AI/agenta
Length of output: 9253
🤖 get_repo_knowledge executed:
get_repo_knowledge Agenta-AI/agenta /tmp/coderabbit-repo-knowledge/agenta-ai-agenta-4b53879a/conventions /tmp/coderabbit-repo-knowledge/agenta-ai-agenta-4b53879a/learnings
Length of output: 43911
Allow Slack me_message events.
When Slack emits a /me message, it uses subtype me_message with the fields consumed by this parser. The subtype filter returns None for it before storage or routing. Add me_message to _MESSAGE_SUBTYPES.
| if capabilities is None: | ||
| connection = await self.channels_dao.fetch_connection( | ||
| project_id=project_id, | ||
| connection_id=resolution.space.connection_id, | ||
| ) | ||
| capabilities = await self.fetch_capabilities( | ||
| channel=connection.channel, connection=connection | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the service before inspecting callers and DAO contracts.
ast-grep outline api/oss/src/core/channels/service.py --items all --match 'compose_input|fetch_connection'
# Find every composition call and identify callers that use the fallback.
rg -n -C 5 --glob '*.py' '\bcompose_input\s*\(' api/oss
# Find the DAO interface and implementation declarations for fetch_connection.
fd -t f -e py . api/oss | xargs rg -n -C 4 'async def fetch_connection\s*\('Repository: Agenta-AI/agenta
Length of output: 14140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- service fetch_connection and compose_input ---'
sed -n '340,410p;1490,1555p' api/oss/src/core/channels/service.py
printf '%s\n' '--- DAO contract and implementation ---'
sed -n '45,75p' api/oss/src/core/channels/interfaces.py
sed -n '108,145p' api/oss/src/dbs/postgres/channels/dao.py
printf '%s\n' '--- relevant compose_input callers ---'
sed -n '330,360p' api/oss/src/tasks/asyncio/channels/inbox.py
sed -n '155,178p' api/oss/tests/pytest/integration/channels/test_channels_service_routing_dao.pyRepository: Agenta-AI/agenta
Length of output: 9130
Guard the optional connection before reading channel.
channels_dao.fetch_connection returns None when the connection is absent. The fallback then reads connection.channel, which raises AttributeError. Use self.fetch_connection(...) for the hydrated result, check for None, and apply the path’s domain error before calling fetch_capabilities.
1 participant
Context
Stacked on #6646. Three defects from the August live QA share one seam: what a stored message does once it arrives. Findings F83, F94, F84, F88 in the ledger.
Changes
channel_joinare dropped in the adapter.Decisions taken here
Two product calls, recorded in the decision record on the docs lane: the default trigger set above, and "a DM is one conversation, replies top-level". Both follow what a person expects from a Slack bot. The alternative for DMs, one thread per message, is what the branch did and is the behavior the QA called unusable.
Tests
https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT