fix(slack): trigger monitor on bot mentions - #492
AbarnaaSree wants to merge 1 commit into
Conversation
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
@AbarnaaSree Thanks for the PR. Did you test it? Do you have a video to show that it works properly? |
|
Yes, I tested the change locally with the Slack channel monitor regression suite:
The tests cover direct bot mentions, named bot mentions, non-bot mentions, similar trigger text, and trigger ordering. The existing I don't have a video at the moment, but the regression tests exercise the Slack mention parsing and trigger behavior directly. I can provide a short recording if needed. |
|
I'm on it! AbarnaaSree can track my progress at all-hands.dev |
|
@VascoSch92 I independently verified the testing claim from @AbarnaaSree above. I checked out the PR head ( I also ran the rest of the repository test suite for regressions: 735 passed. The only failure ( The tests cover the exact behaviors in question: direct Note: this comment was generated by an AI agent (OpenHands) on behalf of the maintainers. |
|
I'm on it! AbarnaaSree can track my progress at all-hands.dev |
|
Handled the PR #492 comment by independently verifying the author's testing claim and replying on the PR. What I did:
No code changes were needed, so nothing was pushed. The HUMAN: section flagged by the automated bot check already contains real content (>20 chars), so that check should pass when it re-runs. |
|
Follow-up: I fixed the PR description formatting so the automated I also re-ran the Slack channel monitor regression suite on the PR head ( No code changes were needed. Note: this comment was generated by an AI agent (OpenHands) on behalf of @AbarnaaSree. |
|
Handled the PR #492 comment. The triggering comment was the previous agent's verification report (no code change requested, no open review threads). I independently re-ran the Slack channel monitor regression suite on the PR head c254c5f (9/9 passed), fixed the PR description's broken markdown (unclosed powershell fence that had swallowed the HUMAN: section required by the automated check), and posted a brief follow-up comment with AI disclosure: #492 (comment). No code changes were needed, so nothing was pushed. |
|
I think we want an human to verify that. |
|
Hi @VascoSch92, I understand that you’d like a human verification. I’ve verified the implementation with the regression tests, but I don’t currently have access to a configured Slack workspace/environment to perform the end-to-end manual test. If there is a recommended test workspace or setup I can use, I’d be happy to verify it manually. |
|
@AbarnaaSree You can create your own workspace in slack, and then use it to test the bot. |
|
Thanks, @VascoSch92. I’ll create a Slack workspace and perform the manual end-to-end test there. I’ll verify that the bot mention triggers the Slack channel monitor as expected and report the results here. |
|
Hi @VascoSch92, I’ve created a Slack workspace, installed the test bot, and added it to a test channel. I’m now setting up the Slack channel monitor for the manual E2E test. However, the repository instructions require the Automation backend url_from_agent and OPENHANDS_AUTOMATION_API_KEY from <RUNTIME_SERVICES>, and these aren’t available in my local environment. Could you point me to the recommended way to access or start the Automation backend for this test? |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
The mention-span matching itself is sound: _mention_span anchors on the exact <@ID, requires a closing >, returns the earliest of the literal/mention spans, and _request_after_trigger now strips using the matched span end, so the literal and mention paths stay consistent. The added regression tests pass (uv run --with pytest --with pyyaml pytest tests/test_slack_channel_monitor.py -q -> 9 passed at this head).
Two issues in the wiring around the new helper block the fix from actually working outside the narrow bot-token case:
-
The bot user ID is taken from
auth.testof the resolved token, which is not necessarily the bot._resolve_slack_token()prefersSLACK_USER_TOKEN, and the skill's own prerequisites table (SKILL.md, Step 1) directs users to fall back to a user token when the bot token lackschannels:readfor private channels.auth.testreports the token owner, so for anxoxp-tokenBOT_MENTION_IDbecomes the human user's ID. In that configuration a message that mentions the bot matches nothing, and - worse - a message that mentions the token owner (<@U<human>>) now fires the trigger and creates a conversation._is_human_messagealso uses this same ID, so it already conflates the two; derive the actual bot identity instead (e.g.auth.test->bot_id, thenbots.info/users.info) and feed that intoBOT_MENTION_ID. -
The
search.messagespath never became mention-aware._poll_new_messagesstill callssearch_trigger_messages(slack_token, CHANNEL_IDS, TRIGGER_PHRASE, global_oldest), whose query is"@openhands" in:<#C...>. That path is selected whenever the token is a user token, more than one channel is monitored, andsearch:readis present - i.e. exactly the documented multi-channel user-token configuration. In that configuration a plain<@U...>mention is never even fetched, so the reported bug is only fixed on theconversations.historyfallback; the PR description's claim that mentions are now detected does not hold for that setup. Either include the mention needle in the search query (e.g. an OR-ed<@ID>) or document/limit the fix to the history path.
Non-blocking, worth tidying while here: the prompt sent to the agent still states activated by the trigger phrase: "@openhands" (main.py:957 and main.py:1177) and SKILL.md documents detection as "contains trigger phrase", so operator-visible docs/prompt text now under-describe the actual trigger.
🔄 CHANGES REQUESTED
| # Raises RuntimeError immediately if the token is invalid - no point polling. | ||
| bot_user_id_new, scopes = _slack_auth_test(slack_token) | ||
| state["bot_user_id"] = bot_user_id_new | ||
| BOT_MENTION_ID = bot_user_id_new |
There was a problem hiding this comment.
BOT_MENTION_ID is set from auth.test of the resolved token, but _resolve_slack_token() prefers SLACK_USER_TOKEN, and auth.test reports the token owner, not the app's bot user. In the user-token configuration that SKILL.md itself recommends when the bot token lacks channels:read for private channels, this makes BOT_MENTION_ID the human user's ID: a real <@BOT_ID> mention then never triggers, and a mention of the token owner (<@U<human>>) triggers a conversation instead. Derive the bot identity (e.g. auth.test -> bot_id, then bots.info/users.info) rather than reusing the token owner's user_id.
Slack app mentions are represented in message text as
<@USER_ID>or<@USER_ID|name>, rather than the literal@openhandstrigger phrase.The Slack channel monitor previously only detected the literal
@openhandstrigger, so messages that directly mentioned the bot were ignored and no conversation was created.Summary
auth.testand use it for trigger matching.@openhandstrigger behavior.Issue Number
Closes #431
How to Test
Run the Slack channel monitor test suite:
HUMAN:
Tested the Slack channel monitor regression suite covering bot mentions, named bot mentions, non-bot mentions, similar trigger text, and trigger ordering.