Add explicit Assistant listener registration mode#1519
Conversation
ee9fb84 to
18fae4a
Compare
WilliamBergamin
left a comment
There was a problem hiding this comment.
Hi @Cr1stal thanks for your time working on this issue & interest in our project 💯
Unfortunately I can't accept these changes, they introduce more complexity to the already complex assistant logic, this increases the maintenance burden on our team 😅 and can slow down future feature implementation
The ideal solution to #1346 might be to make the assistant middleware a none global middleware, but this might require some server side changes
All this to say we'd prefer to solve #1346 by simplifying the the assistant logic rather then increasing it's complexity
|
@WilliamBergamin Thanks, that makes sense. I understand the main concern is not the goal of #1346, but that this PR adds a second Assistant dispatch path and listener ordering machinery, which increases maintenance cost. When you say the ideal solution might be making the Assistant middleware non-global, do you mean moving Assistant handling closer to the normal app listener pipeline while avoiding a new public registration mode and priority registry? Also, could you clarify what server-side change you have in mind? My current read is that assistant user messages are hard to distinguish from broad message listeners without either server-side classification or client-side ordering rules. |
Yes, the goal would be to dissolve the assistant abstraction into the normal app listener pipeline. Users would configure their Bolt app as an 'assistant' and use the same app listener pipeline, rather than having a dedicated one for this abstraction.
Your current read is accurate. The biggest challenge in this space is around context handling; it's hard to distinguish assistant and user messages from broader messages. Server-side classification would be the ideal solution for this. |
Summary
Addresses #1346 by adding an explicit Assistant registration mode to
App.assistant()andAsyncApp.assistant().By default,
app.assistant(assistant)continues to behave as it does today: it registers Assistant as middleware. This preserves the existing Assistant middleware dispatch path and keepsapp.use(assistant)/app.middleware(assistant)behavior unchanged.Apps can now opt into listener-based registration:
In this mode, Assistant handlers are registered as normal App listeners. This lets assistant-specific handlers participate in the App listener pipeline and inherit app-level middleware such as authorization, auditing, rate limiting, or request enrichment.
The PR also adds an internal listener registry so Assistant listeners can be evaluated before broad catch-all listeners without exposing a general listener priority API. Registration order is preserved within Assistant listeners and ordinary App listeners, and sync/async behavior is kept mirrored.
Testing
./scripts/format.sh --no-install./scripts/lint.sh --no-install./scripts/run_tests.sh tests/slack_bolt/app/test_app_assistant_middleware.py(14 passed)./scripts/run_mypy.sh --no-install(Success: no issues found in 235 source files)./scripts/install_all_and_run_tests.sh(924 passed,81 warnings; mypy:Success: no issues found in 235 source files)Category
slack_bolt.Appand/or its core componentsslack_bolt.async_app.AsyncAppand/or its core componentsslack_bolt.adapter/docsRequirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.
./scripts/install_all_and_run_tests.shafter making the changes.