Repository navigation
fix(slack): slash-command turns keep the channel prompt, skill binding and source names (#128720, salvage #128722) - #139858
Conversation
_handle_slash_command built its MessageEvent without channel_prompt and its source without chat_name/user_name, unlike the message path. A /hermes turn ran without the configured channel_prompts and the bot identity line, and as a human turn it re-pinned channel_pin to None and re-keyed the pinned session-context prompt; the next ordinary message flipped both back, so every slash turn cost two agent rebuilds and two prompt-cache misses. Synthetic events copied from it (/retry, /queue, /steer fallback, /goal kickoff) inherited the gap. Resolve the names and the channel prompt with the same helpers the message path uses.
`/hermes <question>` can open a new session, and bound skills load only when a session starts. The slash event carried no auto_skill, so a session opened that way never loaded the channel's slack.channel_skill_bindings, unlike one opened by an ordinary message. Resolve it the same way the message path does.
The message path rejects an unauthorized sender before any lookup (5bb933e); the slash path now resolves the channel and user name, so a sender the gateway was going to reject cost a conversations.info and a users.info first. Ask the same early gate before resolving. In a channel the runner only drops such a sender, so stop there. In a DM it answers per unauthorized_dm_behavior (pairing code or decline), so the event still reaches it, without names, exactly as before this branch. Docs: N/A, slack.md already says an unauthorized DM gets the pairing prompt.
…aits for no Slack lookup The names the slash path resolves (users.info, conversations.info) were awaited before the event reached the active-session dispatcher. With a shared channel session, a second operator's /stop sat behind their uncached users.info while the first operator's worker kept running. /stop, /new, /reset (the interrupt-then-dispatch commands) and /approve, /deny start no turn of their own, so they are handed over before any lookup. Commands that do start a turn (/queue, /retry, /goal, ...) and free-form questions keep the names and the channel prompt.
_enqueue_goal_turn rebuilt the kickoff turn with the message id and channel prompt but not auto_skill, so the turn the agent actually runs for `/goal <text>` carried no channel skill binding. A resume continuation still carries none. Ported from #129046 (5ee2a10) onto #128722 as a credited delta, per the maintainer review of #129046. The code line is unchanged; the docstring line was adjusted to name bound skills when porting. Refs #130316
… prompt Drive message -> slash -> message in one thread session through the real gateway pins (_pinned_channel_inputs / _pinned_session_context_prompt) and the real /goal kickoff enqueue, so the class is judged by the cache outcome, not only by event fields. Co-authored-by: John Paul Soliva <soliva.johnpaul@icloud.com>
Move the two pure slash-payload helpers (gateway text, thread anchor) out of the adapter facade so the slash-turn parity fix keeps adapter.py under its health cap. No behaviour change.
૮ >ﻌ< ა ci reviewran on dc5dee3 — fix(slack): early auth reject checks the session thread's ro debug infoCI timingsCI timings · View report · View jobWall time 9m58s vs 15m21s (-35.1%). 10 job(s) slower, 8 faster, 1 unchanged.
|
[Medium impact] Slash commands now preserve channel prompt and skill binding. Fix the thread-aware access check before merging so valid routed slash commands are not dropped. Findings
Fix with agent prompt### Issue 1
plugins/platforms/slack/adapter.py:6081-6083
The new early access check omits the decoded `thread_id`. If a shared Slack bot has a thread-specific `profile_routes` entry and the caller is approved only in the destination profile's pairing store, this checks the default profile instead. In a channel, the command is then dropped before its correctly routed source is built. Even `/stop` never reaches the runner.
Pass `thread_id` through `_early_reject_unauthorized` to both authorization paths. Add a regression test with a thread route and separate pairing stores.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
| rejected = self._early_reject_unauthorized(user_id, channel_id, is_dm) | ||
| if rejected and not is_dm: | ||
| return |
There was a problem hiding this comment.
The new early access check omits the decoded thread_id. If a shared Slack bot has a thread-specific profile_routes entry and the caller is approved only in the destination profile's pairing store, this checks the default profile instead. In a channel, the command is then dropped before its correctly routed source is built. Even /stop never reaches the runner.
Pass thread_id through _early_reject_unauthorized to both authorization paths. Add a regression test with a thread route and separate pairing stores.
Prompt To Fix With AI
This is a comment left during a code review.
Path: plugins/platforms/slack/adapter.py
Line: 6081-6083
Comment:
**Routed commands get dropped**
The new early access check omits the decoded `thread_id`. If a shared Slack bot has a thread-specific `profile_routes` entry and the caller is approved only in the destination profile's pairing store, this checks the default profile instead. In a channel, the command is then dropped before its correctly routed source is built. Even `/stop` never reaches the runner.
Pass `thread_id` through `_early_reject_unauthorized` to both authorization paths. Add a regression test with a thread route and separate pairing stores.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in dc5dee3: _early_reject_unauthorized takes thread_id and passes it to _is_sender_authorized and the build_source fallback, from both the slash path and the (pre-existing) message path. test_thread_routed_sender_is_checked_against_the_routed_thread drives both entry points with a thread-scoped auth check (red before).
teknium1
left a comment
There was a problem hiding this comment.
P0 rapid-response review — verdict: sound, fold in Greptile's thread fix before the merge-ask.
Slash turns now carry channel_prompt (via _channel_prompt_with_identity, same as the message path), auto_skill, chat_name, user_name, so the pins match the next message. Skipping the Slack lookups for control commands (/stop, /approve, /deny, /pause) is the right call: a cold users.info must not hold an emergency stop.
Real finding (Greptile, confirmed against main): the new early reject at _handle_slash_command calls _early_reject_unauthorized(user_id, channel_id, is_dm) without the decoded thread_id. BasePlatformAdapter._is_sender_authorized and _make_adapter_auth_check both take thread_id precisely so a thread-scoped profile_routes entry checks the routed profile's pairing store. Without it a caller approved only in the routed profile is rejected in a channel and the command is dropped before the runner sees it — /stop included. The message path has the same omission (pre-existing, _handle_slack_message), so fix both: add a thread_id kwarg to _early_reject_unauthorized, pass it through to _is_sender_authorized and the build_source fallback, and add one invariant test with a thread route.
Nothing pushed; review only.
| # A sender the gateway rejects must not cost Slack lookups first (the message path's rule). | ||
| # In a channel the runner only drops them; in a DM it answers per unauthorized_dm_behavior | ||
| # (pairing code or decline), so it still gets the event, without names. | ||
| rejected = self._early_reject_unauthorized(user_id, channel_id, is_dm) |
There was a problem hiding this comment.
Pass thread_id here (and thread it into _is_sender_authorized(..., thread_id=...) / the fallback build_source). Without it a thread-routed profile's paired user is rejected and even /stop is dropped in a channel. The message path's call has the same gap; fix both.
There was a problem hiding this comment.
Done in dc5dee3 (both call sites, regression through _handle_slash_command and _handle_slack_message).
…y in arrival order Names resolved for a slash turn are awaited before the handoff and Bolt runs callbacks concurrently, so a /queue whose cold users.info answered late reached the gateway FIFO after a later one. A slash now reserves its session's slot before any lookup and hands over only after the slash ahead returned from handle_message; a slash cancelled while waiting still chains the one behind it. /stop, /approve, /deny, /pause take no slot. Ported from #128722 (9b71908, eb546b6) into adapter_slash_lane.py. Co-authored-by: John Paul Soliva <soliva.johnpaul@icloud.com>
_early_reject_unauthorized dropped the decoded thread_id, so a thread-scoped profile_routes entry checked the default profile's pairing store: a caller approved only in the routed profile had slash commands (even /stop) and messages in that thread dropped. Pass thread_id from both the slash and the message path to _is_sender_authorized and the build_source fallback.
A Slack slash-command turn (
/hermes <question>, native/<command>) now carries the same channel prompt, bound skills and source names as an ordinary message in that channel, so it no longer dropsslack.channel_promptsor flips the session's prompt pins.SlackAdapter._handle_slash_commandbuilds its event withchannel_prompt=_channel_prompt_with_identity(...),auto_skill=resolve_channel_skills(...), and resolveschat_name/user_namewith the message path's helpers (_build_message_event)./stopand otherinterrupt_then_dispatch,/approve,/deny,/pause) are handed over before anyusers.info/conversations.info, so a cold lookup cannot hold an emergency stop back./goal <text>kickoff turn (gateway/slash_commands_goals.py::_enqueue_goal_turn) keeps the triggering turn'sauto_skillalong with its channel prompt.slash_command_text/slash_thread_idmove unchanged intoplugins/platforms/slack/adapter_slash_payload.pyto keepadapter.pyunder its health cap.website/docs/user-guide/messaging/slack.mdsays slash turns get the channel prompt and binding.Live repro (real
SlackAdapter._handle_slack_message/_handle_slash_command→ realGatewayRunner._pinned_channel_inputs+_pinned_session_context_prompt, tempHERMES_HOME; only the Slack Web API client is stubbed)message →
/hermes what broke?→ message in one thread session, channelC0123OPSwithchannel_prompts: {C0123OPS: "Answer in haiku."}:channel_promptchat_name/user_nameorigin/main1274ab7, threadNoneNone/Noneorigin/main, DM threadNoneNone/Noneops/AliceAlice/AliceRoot cause:
_handle_slash_commandwas the onlyMessageEvent(site in the Slack adapter that skipped the channel-prompt / name / skill-binding helpers; as a non-internal human turn it re-pinnedchannel_pin=(None, …)and re-keyed_ephemeral_change_key, and the next message flipped both back.Tests
tests/plugins/platforms/slack/test_slash_command_channel_prompt.py(9): message → slash → message renders one pinned prompt pair through the real gateway pins (channel, DM,/queue,/goalkickoff via the real_enqueue_goal_turn); rejected/control commands cost no lookup. Red onorigin/main(5 failed); deleting only theauto_skill=line in_enqueue_goal_turnturnsgoal-kickoffred.tests/gateway/test_slack.py+ the new file: 221 passed, 1 skipped.scripts/check --only health: ok.Fixes #128720
Salvages #128722 (@jonpol01), cherry-picked with authorship; includes @JoaoMarcos44's
/goalkickoff commit from that branch.Dropped hunks
9b719083/eb546b6a(per-session slash handoff lane_slash_handoff_tails+ cancellation chaining). Resolving names now awaitsusers.info/conversations.infobefore the handoff, the same as the message path already does. So two slashes of one session fired together, with the first on a cold lookup, can reach the gateway's/queueFIFO swapped. Once the per-(team, user) and per-(team, channel) caches are warm there is no window. That lane is about queue ordering, not the prompt pins this issue covers, and it adds a future-chaining lane to the adapter, so it is left out for a maintainer call. Only its/pauseatom is ported (co-authored). Not re-probed here.Infographic
Review follow-up
811e4b92545. Resolving names before the handoff let two/queueslashes of one session reach the gateway swapped when the first had a coldusers.info. This head now ports fix(slack): carry channel prompt and source names on slash-command turns #1287229b719083/eb546b6a(co-authored with @jonpol01) intoplugins/platforms/slack/adapter_slash_lane.py. A slash takes its session's slot before any lookup and hands over only after the slash ahead of it has returned fromhandle_message. A slash cancelled while waiting still keeps the one behind it waiting./stop,/approve,/denyand/pausetake no slot, so they still go through at once. The first "Dropped hunks" bullet above no longer applies.test_concurrent_slashes_reach_the_gateway_in_arrival_ordergoes through the Bolt listener (_handle_hermes_command). It holds the first/hermes queueon a cold lookup and sends a second, plus a/stopin between./stopis handed over at once, then/queue firstand/queue secondarrive in order. Red onc271b06ee8d(['/queue second', '/stop']), green now. Slack tests: 249 passed, 1 skipped.scripts/check: ok.Review follow-up
_early_reject_unauthorizednow takes the decodedthread_idand passes it to_is_sender_authorizedand to thebuild_sourcefallback. Both call sites pass it: the slash path, and the message path (where the omission predates this PR; it now runs after_session_thread_ts). Before this, a thread-scopedprofile_routesentry was checked against the default profile's pairing store, so a caller approved only in the routed profile had/stopand messages in that thread dropped (Greptile + maintainer review). New testtest_thread_routed_sender_is_checked_against_the_routed_thread, through_handle_slash_commandand_handle_slack_message: fails before (nothing reaches the runner), passes after.