Skip to content

fix(slack): accept raw bot IDs in trustedBotIds#814

Merged
thepagent merged 1 commit into
mainfrom
fix/slack-trusted-bot-ids
May 14, 2026
Merged

fix(slack): accept raw bot IDs in trustedBotIds#814
thepagent merged 1 commit into
mainfrom
fix/slack-trusted-bot-ids

Conversation

@chaodu-agent
Copy link
Copy Markdown
Collaborator

@chaodu-agent chaodu-agent commented May 13, 2026

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1504239931940409587/1504240354608808170

Summary

Fixes Slack trustedBotIds gating when Slack event payloads provide raw Bot IDs (B...) and operators configure either raw Bot IDs or Bot User IDs (U...).

Changes:

  • Accept raw event.bot_id directly in trusted_bot_ids.
  • Preserve existing B... -> U... resolution path for configs using Slack Bot User IDs.
  • Warn when bots.info fails instead of degrading silently to resolved=None.
  • Document that Slack U... matching requires users:read for bots.info.
  • Add pure helper coverage for raw B..., resolved U..., failed resolution, and empty bot IDs.

Fixes #811

Test plan

  • Not run locally: this execution environment has neither cargo nor rustc in PATH.

@chaodu-agent chaodu-agent requested a review from thepagent as a code owner May 13, 2026 21:58
@github-actions github-actions Bot added closing-soon PR missing Discord Discussion URL — will auto-close in 3 days pending-screening PR awaiting automated screening pending-maintainer and removed closing-soon PR missing Discord Discussion URL — will auto-close in 3 days labels May 13, 2026
@shaun-agent
Copy link
Copy Markdown
Contributor

OpenAB PR Screening

This is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Click 👍 if you find this useful. Human review will be done within 24 hours. We appreciate your support and contribution 🙏

Screening report ## Intent

PR #814 fixes Slack trustedBotIds filtering when Slack events identify bots by raw event.bot_id values like B..., while OpenAB config may contain either raw Bot IDs or resolved Bot User IDs like U....

The operator-visible problem: trusted Slack bots may be incorrectly rejected unless OpenAB can resolve B... -> U... through Slack bots.info, and failures were apparently too quiet.

Feat

Fix.

Behavioral change: Slack bot trust checks now accept raw Slack Bot IDs directly, keep the existing Bot ID to Bot User ID resolution path, warn on bots.info failure, and document the users:read requirement for U... matching.

Who It Serves

Primary beneficiaries: Slack deployers and agent runtime operators.

Secondary beneficiaries: maintainers reviewing Slack auth/gating behavior, because the trusted bot matching rules become explicit and tested.

Rewritten Prompt

Fix Slack trusted bot filtering so trustedBotIds accepts both raw Slack Bot IDs such as B... and resolved Bot User IDs such as U....

Implementation requirements:

  • If event.bot_id is present and directly matches trusted_bot_ids, allow the event.
  • If no raw match exists, preserve the existing bots.info resolution path and allow matches against the resolved Bot User ID.
  • If resolution fails, log a warning with enough context for operators to diagnose missing Slack scopes or API failures.
  • Keep empty or missing bot IDs rejected.
  • Update Slack configuration docs to state that U... matching requires Slack API resolution and the needed scope.
  • Add focused tests for raw B..., resolved U..., failed resolution, and empty bot ID behavior.

Merge Pitch

This is a small, operator-facing correctness fix for Slack deployments. It makes configuration less brittle by supporting the identifier Slack actually sends in event payloads.

Risk profile is low to moderate: the main concern is whether accepting raw B... IDs broadens trust in a way operators do not expect. That risk is acceptable if docs clearly define that trustedBotIds may contain either Slack Bot IDs or Bot User IDs, and tests pin the matching order.

Best-Practice Comparison

OpenClaw principles relevant here:

  • Explicit delivery routing: relevant. Trust gating should be explicit about which Slack bot identity is accepted.
  • Run logs: partially relevant. Warning on failed bot resolution improves operator diagnosis.
  • Durable job persistence, isolated executions, gateway-owned scheduling, retry/backoff: not directly relevant. This PR is about Slack event admission, not scheduled execution.

Hermes Agent principles relevant here:

  • Self-contained prompts for scheduled tasks: not relevant.
  • Fresh session per scheduled run, daemon tick model, file locking, atomic writes: not relevant.
  • The closest relevant principle is operational clarity: failed identity resolution should be visible instead of silently degrading.

Implementation Options

Conservative option: accept raw B... IDs before resolution, preserve all existing behavior, add tests and docs only.

Balanced option: introduce a small pure helper for Slack trusted bot matching that takes raw bot ID, resolved user ID, and configured trusted IDs, then use it from the event gate. This keeps behavior testable without over-touching Slack runtime code.

Ambitious option: normalize Slack bot identity handling into a typed identity model, e.g. SlackBotIdentity { bot_id, bot_user_id }, and use it consistently across Slack routing, trust checks, logging, and future permissions logic.

Comparison Table

Option Speed to ship Complexity Reliability Maintainability User impact Fit for OpenAB right now
Conservative raw B... match High Low Good Good High for affected Slack deployers Strong
Balanced pure helper Medium Medium-low Very good Very good High Best
Typed Slack identity model Low Medium-high Very good long-term Strong if Slack surface grows Medium now, higher later Probably premature

Recommendation

Advance with the balanced option if the PR already trends that way: a small pure helper plus focused tests gives reviewers a clean behavioral surface and avoids burying identity rules inside Slack event handling.

Do not expand this into a larger Slack identity refactor during this PR. Merge the correctness fix first, then open a follow-up only if maintainers see more Slack identity checks spreading through the codebase.

@antigenius0910
Copy link
Copy Markdown
Contributor

Verified raw B… matching against an actual bots.info / users:read failure mode on a 0.8.3-beta.9-equivalent rig; behaves selectively (Slackbot still rejected). Pairs cleanly with #808 for the full relay path.

@chaodu-agent
Copy link
Copy Markdown
Collaborator Author

LGTM ✅ — Clean fix for raw Bot ID matching in trustedBotIds.

What This PR Does

Fixes trusted_bot_ids gating when Slack events provide raw Bot IDs (B...) but operators configure either raw Bot IDs or Bot User IDs (U...). Previously only the resolved U... path worked, silently rejecting trusted bots when bots.info failed.

How It Works

  • New bot_id_matches_trusted() pure helper checks both raw event B... ID and resolved U... ID against the trusted set.
  • New trusted_bot_ids_contains() async method: checks raw ID first (fast path), then falls back to bots.info resolution.
  • Empty event_bot_id short-circuits to false (guard against empty-string matching).
  • resolve_bot_user_id now warns on bots.info failure instead of silently returning None.
  • Docs updated to clarify that U... matching requires users:read scope.

Findings

# Severity Finding Location
1 🟢 Raw B... ID fast-path eliminates dependency on bots.info availability src/slack.rs:243
2 🟢 Pure helper is easily testable with 4 unit tests covering accept/reject/empty cases src/slack.rs tests
3 🟢 Documentation accurately describes the U... vs B... behavior and scope requirement docs/
4 🟢 Empty bot_id guard prevents accidental empty-string set membership match src/slack.rs:1199
Baseline Check
  • PR opened: 2026-05-13
  • Main has: Only resolves B...U... via bots.info, then checks trusted_bot_ids.contains(uid)
  • Net-new value: Direct B... matching + graceful fallback when resolution fails
What's Good (🟢)
  • Two-tier matching (raw first, resolved second) is the correct design — fast path avoids unnecessary API calls
  • Deduplicates the trusted-bot check logic from two inline blocks into one reusable method
  • Warning on bots.info failure adds observability for operators debugging trust issues
  • All CI checks pass including smoke tests across all agent backends

Note: This PR and #808 both modify src/slack.rs. Whichever merges second will need a trivial rebase.

@thepagent thepagent merged commit d06bf26 into main May 14, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pending-maintainer pending-screening PR awaiting automated screening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(slack): trustedBotIds check always fails — bot_id resolution returns None

4 participants