fix(gateway): bypass trigger router for in-thread @mentions + drop dead button code#109
Conversation
When a channel is bound to a slack_trigger (e.g. feedback-intake on #brix-feedback-sandbox), an in-thread human @mention of the bot was being lost: the router's STATUS_SKIPPED_NOT_TOP_LEVEL path returned False to fall through to the normal flow, but in practice the operator's command never produced a response. Adds an explicit bypass at the top of _maybe_dispatch_slack_trigger: when the message is a human thread reply AND the channel is trigger-bound AND the text contains the bot's @mention, log status=bypassed_in_thread_mention and return False outright so the normal agent path handles it. Scope is thread-only by design. Top-level messages in a trigger-bound channel are feedback content, not operator commands; the @mention bypass would otherwise hijack the trigger. Enables the feedback-intake filing flow: operators reply `@babydidier file <N>` in the proposal thread, the agent reads the proposal post and invokes inverter-customer-request per index. 3 new tests in TestTriggerDispatch (in-thread @mention bypasses, in-thread non-mention still skips, top-level @mention still fires the trigger). 10/10 trigger-dispatch tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
feedback-intake migrated to plain-text proposals + in-thread @mention filing in hermes-state #12. With no remaining caller for any of the Block Kit machinery, removes the dead code rather than leaving it behind as a latent surface skills could accidentally re-light. Removed: - gateway/platforms/slack.py: the generic skill-invoke Block Kit action registration (^skill:<name>$ regex) and the _handle_skill_invoke_action handler. - tools/send_message_tool.py: the interactive_actions schema property with its full nested items shape; _validate_interactive_actions and _build_slack_blocks_from_interactive_actions; _INTERACTIVE_ACTIONS_MAX_COUNT, _SKILL_ACTION_ID_RE, _INTERACTIVE_ACTION_STYLES, _SLACK_TEXT_FALLBACK_MAX_CHARS constants; slack_interactive_actions plumbing through _handle_send, _send_to_platform, and _send_slack (the blocks kwarg too). - tests/tools/test_send_message_tool.py: TestValidateInteractiveActions, TestBuildSlackBlocksFromInteractiveActions, and the routing/payload tests that exercised slack_interactive_actions / blocks. Kept: - thread_ts on send_message (cross-platform-useful for threading). - TestSendMessageSchemaShape regression guard (array properties must declare items). - The slash-confirm Block Kit handlers (hermes_confirm_*); these are a separate flow tied to slash_confirm tool and stay in use. Net: 426 deletions, 6 insertions. 402 tests pass across Slack + send_message suites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
072d401 to
f3b46b7
Compare
Two regression-guard tests on the in-thread @mention bypass added in this PR's first commit, addressing review feedback: - test_in_thread_bot_authored_mention_does_not_bypass: a bot's own in-thread reply that contains <@U_BOT> as literal text (cross-bot loop or self-quote) MUST NOT trigger the bypass. The is_bot_message guard catches it before the substring check runs. - test_in_thread_mention_with_unresolved_bot_uid_no_match: when _bot_user_id is empty (identity not yet resolved at startup), the `bot_uid and` truthy guard prevents the substring check against a literal "<@>" pattern. Pins the safe fallback. Also trims a comment that was narrating what the code does (per project-standards reviewer): the bypass branch's intent rationale is already in the function docstring. 12/12 trigger-dispatch tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lafawnduh1966
left a comment
There was a problem hiding this comment.
Review Findings
Blocking
None.
Non-blocking
🟡 [1] In-thread mention test still does not prove normal agent dispatch
test_slack_trigger_dispatch.py:175 only calls _maybe_dispatch_slack_trigger() and asserts False. That was already the old result for human thread replies, so it does not prove @bot file 1 reaches handle_message. Add one full _handle_slack_message() test that asserts handle_message is awaited and receives file 1.
Per Fabi's review: the existing router-level test only asserted _maybe_dispatch_slack_trigger returns False for in-thread @mentions, which was already the case before this PR for any human thread reply. That assertion doesn't prove the message actually reaches the agent dispatch. Adds test_in_thread_mention_reaches_agent_dispatch that calls the full _handle_slack_message path and asserts: - handle_message is awaited exactly once. - The MessageEvent text contains "file 1" (operator command survives). - The bot mention <@U_BOT> is stripped per the existing normal-flow contract. - reply_to_message_id carries the thread root. - No trigger envelope is prepended (we bypassed the router). - auto_skill is None (no skill auto-load on the general agent path). 13/13 trigger-dispatch tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed in c0bda2ac. New Kept the existing router-level |
…aml (#112) cmd_render_gateway_runtime_env grows three new values-driven env vars: - SLACK_ALLOWED_CHANNELS from slack.runtime.allowed_channels - GATEWAY_ALLOW_ALL_USERS from gateway.allow_all_users (bool) - (SLACK_ALLOWED_USERS empty-list semantics already in place) All three land in <auth>/gateway-runtime.env which is loaded by the z-runtime-env.conf systemd drop-in (lexical sort: loads LAST, wins). That means values.yaml is now the single source of truth for slack allowlists; stale assignments in legacy auth/slack.env or /etc/default/hermes-gateway are overridden on every install. Motivation: the krustentier rails deploy went stale because PR #109 never rendered into the running tree, but even after re-rendering the gateway dropped in-thread @mentions because (a) SLACK_ALLOWED_CHANNELS in auth/slack.env was an older single-channel value masking the 3- channel list in config.yaml, and (b) SLACK_ALLOWED_USERS was empty so the gateway denied every user in normal flow. Routing both through the existing values-driven render-gateway-runtime-env step makes re-running setup-hermes.sh sufficient to propagate channel/user allowlist edits across all hosts. deploy.values.yaml additions: - gateway.allow_all_users: true (the deployment trusts channel-level authz via slack.runtime.allowed_channels; per-user gate is redundant) - schema-doc entries for slack.runtime.allowed_channels and gateway.allow_all_users. 8 new tests across TestRenderGatewayRuntimeEnv covering write/empty/ non-list/non-bool/absent cases for both new fields. 181 total in test_values_helper.py pass. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
What
Two changes:
_maybe_dispatch_slack_triggerbypass. When the channel is bound to aslack_triggerAND the message is a human in-thread reply AND the text contains the bot's @mention, logstatus=bypassed_in_thread_mentionand return False so the normal agent path handles it.feedback-intakemigrated to plain-text proposals + in-thread @mention filing (hermes-state slack: wire BabyDidier(dev) into the Inverter workspace (ITRY-1288) #12), the generic^skill:<name>$Block Kit action handler and theinteractive_actionstool surface have no caller. Delete them rather than leaving dead surface.Net: ~420 deletions.
Why
(1) In a trigger-bound channel like
#brix-feedback-sandbox, in-thread @mentions of the bot were being lost. The router'sSTATUS_SKIPPED_NOT_TOP_LEVELbranch should fall through, but in practice the operator's@bot file 1command never produced a response. The explicit bypass routes the event around the router so the agent's general response path picks it up.(2)
feedback-intakewas the only Block Kit caller. After migrating to the @mention flow (hermes-state #12), nothing dispatchesskill:<name>button clicks. Leaving the machinery behind invites future skills to reach for it and re-introduce the fragility (Slack interactivity callback wiring, schema regressions, recursive payload validation).Behavior matrix
#brix-feedback-sandboxfeedback-intakefeedback-intakeScope is deliberately thread-only on the bypass. Top-level @mentions in a trigger-bound channel are feedback content; bypassing on top-level @mentions would let any @mention hijack the trigger.
Removed
gateway/platforms/slack.py:^skill:<name>$regex)._handle_skill_invoke_actionmethod.tools/send_message_tool.py:interactive_actionsschema property + nesteditemsshape._validate_interactive_actions,_build_slack_blocks_from_interactive_actions._INTERACTIVE_ACTIONS_MAX_COUNT,_SKILL_ACTION_ID_RE,_INTERACTIVE_ACTION_STYLES,_SLACK_TEXT_FALLBACK_MAX_CHARSconstants.slack_interactive_actionsplumbing through_handle_send,_send_to_platform, and_send_slack(theblockskwarg too).tests/tools/test_send_message_tool.py:TestValidateInteractiveActions,TestBuildSlackBlocksFromInteractiveActions, and the routing/payload tests that exercisedslack_interactive_actions/blocks.Kept
thread_tsonsend_message(cross-platform useful for threading; not button-specific).TestSendMessageSchemaShaperegression guard (array properties must declareitems).hermes_confirm_*); separate flow tied toslash_confirmtool.Unblocks
feedback-intakeskill's new filing flow (hermes-state #12): operators reply@babydidier file <N>in the proposal thread, the agent re-reads the proposal post, parses the numbered[N]anchors, and dispatchesinverter-customer-requestper index.How to verify
python3 -m pytest tests/gateway/test_slack_trigger_dispatch.py -q(10 pass).python3 -m pytest tests/tools/test_send_message_tool.py -q(100 pass).#brix-feedback-sandbox, wait for the bot's plain-text proposal reply, then reply in-thread with@babydidier file 1. Confirm the bot reads the thread and files proposal 1 to Linear. Log line for the @mention should showstatus=bypassed_in_thread_mention.Follow-up
hermes-statecleanup PR: delete the dormantskills/file-cr/.