fix: add interactive action registry for button/select callbacks#2108
Conversation
#2104) - Created InteractiveRegistry and dispatch protocol in core SDK - Added encode_action/decode_callback for typed action handling - Updated Telegram, Discord, and Slack adapters to use registry - Registered handlers for 'command' and 'pair' namespaces - Maintains backward compatibility with existing pairing callbacks Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds ChangesInteractive Callback Dispatch System
Sequence DiagramsequenceDiagram
participant User as User (taps button)
participant Adapter as Platform Adapter
participant InteractiveContext
participant InteractiveRegistry
participant NSHandler as Namespace Handler
User->>Adapter: button callback (raw callback_data)
Adapter->>InteractiveContext: build(callback_data, user_id, platform_data)
Adapter->>InteractiveRegistry: dispatch(ctx)
InteractiveRegistry->>InteractiveRegistry: decode_callback(callback_data) → (namespace, payload)
InteractiveRegistry->>InteractiveRegistry: inject namespace/payload into ctx.platform_data
InteractiveRegistry->>NSHandler: await handler(ctx)
NSHandler-->>Adapter: result / edited message
alt registry did not handle
Adapter->>Adapter: legacy pair: fallback
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR adds a symmetric inbound dispatch system for interactive UI callbacks (buttons/selects) across Discord, Slack, and Telegram adapters, complementing the existing presentation rendering layer with a new
Confidence Score: 3/5The callback dispatch path has null-dereference bugs in the new command-button handlers for Discord and Telegram that silently swallow user feedback on every button-triggered command when the message object is unavailable. Both the Discord and Telegram command handlers access discord.py (handle_command_callback success and error branches) and telegram.py (handle_command_callback permission-denied, success, and error branches) both need null guards before accessing the message object. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant U as User
participant P as Platform (TG/Discord/Slack)
participant A as Bot Adapter
participant R as InteractiveRegistry
participant H as Namespace Handler
participant F as Fallback Path
U->>P: Click button / select
P->>A: Callback event (pair:CODE / cmd:foo)
A->>A: Build InteractiveContext
A->>R: dispatch(ctx)
R->>R: decode_callback(ctx.callback_data)
note over R: pair:... namespace=pair, cmd:... namespace=command
R->>H: handler(ctx)
alt handler returns truthy
H-->>R: Command foo executed
R-->>A: True (handled)
else handler returns None or raises
H-->>R: None
R->>R: try fallback_handler
R-->>A: False (not handled)
A->>F: legacy pair: check
F->>A: handle_approval_callback()
end
A->>P: edit_original_response / edit_message_text
P-->>U: Updated message
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant U as User
participant P as Platform (TG/Discord/Slack)
participant A as Bot Adapter
participant R as InteractiveRegistry
participant H as Namespace Handler
participant F as Fallback Path
U->>P: Click button / select
P->>A: Callback event (pair:CODE / cmd:foo)
A->>A: Build InteractiveContext
A->>R: dispatch(ctx)
R->>R: decode_callback(ctx.callback_data)
note over R: pair:... namespace=pair, cmd:... namespace=command
R->>H: handler(ctx)
alt handler returns truthy
H-->>R: Command foo executed
R-->>A: True (handled)
else handler returns None or raises
H-->>R: None
R->>R: try fallback_handler
R-->>A: False (not handled)
A->>F: legacy pair: check
F->>A: handle_approval_callback()
end
A->>P: edit_original_response / edit_message_text
P-->>U: Updated message
Reviews (3): Last reviewed commit: "fix: remove duplicate BotOSProtocol impo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/praisonai/praisonai/bots/slack.py (1)
882-887: 💤 Low valueConsider adding error handling around
chat_updatecalls in handlers.Both the command and pair handlers call
self._client.chat_update()without explicit error handling. If the message was deleted or the API call fails, the exception propagates todispatch()which catches it but loses context about which operation failed.This is a minor concern since
dispatch()does catch and log exceptions, but explicit handling would provide better diagnostics.💡 Optional: Add error handling for better diagnostics
await self._client.chat_update( channel=body["channel"]["id"], ts=body["message"]["ts"], blocks=blocks, text=body["message"]["text"] ) + except Exception as e: + logger.error(f"Failed to update message after command execution: {e}") return f"Command {cmd_name} executed"Also applies to: 925-930
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/praisonai/bots/slack.py` around lines 882 - 887, The chat_update calls made via self._client.chat_update() in both the command and pair handlers lack explicit error handling, causing exceptions to propagate to the dispatch() method where they are caught but lose context about which specific operation failed. Wrap each chat_update call in a try-except block to catch any exceptions and log them with context-specific information that identifies which handler and operation failed, such as including details about the channel, message timestamp, or handler type in the error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai/praisonai/bots/discord.py`:
- Around line 732-740: The BotMessage instantiation is passing three invalid
keyword arguments (chat_id, command, and command_args) that are not defined
fields in the BotMessage dataclass from praisonaiagents.bots.protocols. Remove
these three invalid parameters from the BotMessage constructor call. If this
data needs to be preserved for later use, store it in the metadata field
instead, which is the appropriate parameter for holding additional contextual
information.
In `@src/praisonai/praisonai/bots/telegram.py`:
- Around line 279-284: The special case for `cmd_name == "approve"` in the
command routing logic only logs and returns without performing any actual work,
meaning any registered approval command handlers in `_command_handlers` will
never be invoked for this command. Either remove this conditional block entirely
if approval is handled through the `pair` namespace handler, or replace the
no-op logging with actual routing logic that invokes the appropriate approval
system handler, or add a detailed comment explaining why this specific case
needs special handling that differs from normal command processing.
---
Nitpick comments:
In `@src/praisonai/praisonai/bots/slack.py`:
- Around line 882-887: The chat_update calls made via self._client.chat_update()
in both the command and pair handlers lack explicit error handling, causing
exceptions to propagate to the dispatch() method where they are caught but lose
context about which specific operation failed. Wrap each chat_update call in a
try-except block to catch any exceptions and log them with context-specific
information that identifies which handler and operation failed, such as
including details about the channel, message timestamp, or handler type in the
error message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4eced2e-843c-4dff-8ed3-8bd79d8904aa
📒 Files selected for processing (5)
src/praisonai-agents/praisonaiagents/bots/__init__.pysrc/praisonai-agents/praisonaiagents/bots/interactive.pysrc/praisonai/praisonai/bots/discord.pysrc/praisonai/praisonai/bots/slack.pysrc/praisonai/praisonai/bots/telegram.py
| from praisonaiagents.bots import BotMessage, BotUser | ||
| message = BotMessage( | ||
| message_id=str(interaction.message.id) if interaction.message else "", | ||
| content=f"/{command}", | ||
| sender=BotUser(user_id=ctx.user_id), | ||
| chat_id=str(interaction.channel_id) if hasattr(interaction, "channel_id") else "", | ||
| command=cmd_name, | ||
| command_args=cmd_args | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for BotMessage class definition to verify it has chat_id and command_args fields
ast-grep --pattern $'class BotMessage:
$$$
'
# Also search with dataclass decorator
ast-grep --pattern $'`@dataclass`
class BotMessage:
$$$
'
# Fallback: grep for field definitions
rg -n 'chat_id|command_args' --type py -g '**/protocols.py' -g '**/bots/*.py' -A2 -B2Repository: MervinPraison/PraisonAI
Length of output: 50379
Remove invalid chat_id, command, and command_args keyword arguments from BotMessage instantiation.
The BotMessage dataclass at src/praisonai-agents/praisonaiagents/bots/protocols.py only accepts these fields: message_id, content, message_type, sender, channel, timestamp, reply_to, thread_id, attachments, and metadata. The instantiation at lines 732–740 passes chat_id, command, and command_args, which are not valid constructor parameters. This will raise a TypeError at runtime. Remove these invalid kwargs or store them in metadata if they need to be preserved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/bots/discord.py` around lines 732 - 740, The
BotMessage instantiation is passing three invalid keyword arguments (chat_id,
command, and command_args) that are not defined fields in the BotMessage
dataclass from praisonaiagents.bots.protocols. Remove these three invalid
parameters from the BotMessage constructor call. If this data needs to be
preserved for later use, store it in the metadata field instead, which is the
appropriate parameter for holding additional contextual information.
| if cmd_name == "approve": | ||
| # Handle approval command (from approval buttons) | ||
| # This is a special case that should be handled by the approval system | ||
| # For now, log it | ||
| logger.info(f"Approval command via button: {command}") | ||
| return "Approval handled" |
There was a problem hiding this comment.
Clarify the purpose of the approve command special case or remove dead code.
The special case for cmd_name == "approve" just logs and returns without performing any action. If approval handling is done elsewhere (e.g., through the pair namespace handler), this code path never does useful work. If there's a registered approve command handler in _command_handlers, it will never be invoked due to this early return.
Consider either:
- Removing this special case if approval is handled via the
pairnamespace - Adding a comment explaining why this no-op exists
- Actually routing to the approval system if that's the intent
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/bots/telegram.py` around lines 279 - 284, The special
case for `cmd_name == "approve"` in the command routing logic only logs and
returns without performing any actual work, meaning any registered approval
command handlers in `_command_handlers` will never be invoked for this command.
Either remove this conditional block entirely if approval is handled through the
`pair` namespace handler, or replace the no-op logging with actual routing logic
that invokes the appropriate approval system handler, or add a detailed comment
explaining why this specific case needs special handling that differs from
normal command processing.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job ✅ Final Architecture Review Complete - PR #2108All critical issues have been fixed and pushed to the branch. Todo List:
Fixes Implemented1. Fixed dispatch() return value (P1)Issue: 2. Fixed global registry singleton (P1)Issue: Multiple adapters shared the same global registry, causing handler conflicts
3. Fixed invalid BotMessage constructorIssue: Discord, Slack, and Telegram passed invalid
4. Cleaned up redundant importsIssue: Interactive symbols imported twice in 5. Removed dead codeIssue: Special case for 6. Fixed dataclass field defaultIssue: Using Architecture Compliance✅ Protocol-driven design: Core SDK has minimal protocol definitions, heavy implementations in wrapper Files Modified
Final Verdict✅ PR APPROVED - All critical issues have been resolved. The implementation now correctly:
The PR successfully implements the interactive action registry for button/select callbacks as intended in #2104. |
- Fix dispatch() to properly return False when handlers return None - Replace global registry singleton with create_registry() for adapter-specific instances - Fix invalid BotMessage constructor arguments (chat_id, command, command_args moved to metadata) - Remove redundant TYPE_CHECKING imports - Remove dead code for 'approve' command special case - Use field(default_factory=dict) for proper dataclass default Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude validate the fix, if its correct and follow guidenlines ? also see if it valid to remove this , are you doing it right ? |
|
Claude finished @MervinPraison's task —— View job ✅ Validation Complete & Fix AppliedI've reviewed the fixes from the previous commit and addressed the duplicate imports issue. Todo List:
1. Previous Fixes Validation ✅All critical issues have been properly addressed: ✅ Fixed: dispatch() return value
✅ Fixed: Global registry singleton issue
✅ Fixed: BotMessage constructor issues
✅ Fixed: Dataclass field defaults
2. Duplicate Import Fixed ✅Removed the redundant import in commit
3. False Positives from ReviewersTwo incorrect issues that don't need fixing: ❌ Greptile's "static_select issue" - Already Correct
❌ Dead code in Telegram - Not Found
4. AGENTS.md Compliance ✅The implementation follows all guidelines:
SummaryAll issues have been validated and the duplicate import has been removed. The PR is ready for merge with all critical issues resolved and full backward compatibility maintained. |
The import was redundant as BotOSProtocol is already imported at runtime (line 23) and needs to be available for the __all__ export list.
| # Update the message to show command was executed | ||
| await interaction.edit_original_response( | ||
| content=f"{interaction.message.content}\n\n✅ Command executed: /{cmd_name}", | ||
| view=None | ||
| ) | ||
| return f"Command {cmd_name} executed" | ||
| except Exception as e: | ||
| logger.error(f"Command handler error: {e}") | ||
| await interaction.edit_original_response( | ||
| content=f"{interaction.message.content}\n\n❌ Error executing command", | ||
| view=None | ||
| ) |
There was a problem hiding this comment.
interaction.message.content is accessed without a None guard in both the success and error response paths. Discord sets interaction.message to None for ephemeral or certain app-owned messages; when that happens, the f-string evaluation raises AttributeError before edit_original_response is called, the exception is swallowed by dispatch()'s catch block, and the user receives no feedback that the command ran (or failed).
| # Update the message to show command was executed | |
| await interaction.edit_original_response( | |
| content=f"{interaction.message.content}\n\n✅ Command executed: /{cmd_name}", | |
| view=None | |
| ) | |
| return f"Command {cmd_name} executed" | |
| except Exception as e: | |
| logger.error(f"Command handler error: {e}") | |
| await interaction.edit_original_response( | |
| content=f"{interaction.message.content}\n\n❌ Error executing command", | |
| view=None | |
| ) | |
| # Update the message to show command was executed | |
| original_content = interaction.message.content if interaction.message else "" | |
| await interaction.edit_original_response( | |
| content=f"{original_content}\n\n✅ Command executed: /{cmd_name}", | |
| view=None | |
| ) | |
| return f"Command {cmd_name} executed" | |
| except Exception as e: | |
| logger.error(f"Command handler error: {e}") | |
| original_content = interaction.message.content if interaction.message else "" | |
| await interaction.edit_original_response( | |
| content=f"{original_content}\n\n❌ Error executing command", | |
| view=None | |
| ) |
| # Check permissions | ||
| if not self._command_policy.can_run(ctx.user_id, cmd_name): | ||
| await query.edit_message_text( | ||
| text=f"{query.message.text}\n\n⛔ You are not permitted to run /{cmd_name}", | ||
| parse_mode="Markdown" | ||
| ) | ||
| return "Permission denied" |
There was a problem hiding this comment.
query.message.text is embedded in three f-strings without first verifying that query.message is not None. In Telegram, message is None for callbacks originating from inline-mode results or from keyboards forwarded across bots. When it is None, evaluating the f-string raises AttributeError before edit_message_text is invoked — so the permission-denied notice, the success confirmation, and the error notice are all silently lost.
| # Check permissions | |
| if not self._command_policy.can_run(ctx.user_id, cmd_name): | |
| await query.edit_message_text( | |
| text=f"{query.message.text}\n\n⛔ You are not permitted to run /{cmd_name}", | |
| parse_mode="Markdown" | |
| ) | |
| return "Permission denied" | |
| # Check permissions | |
| if not self._command_policy.can_run(ctx.user_id, cmd_name): | |
| original_text = query.message.text if query.message else "" | |
| await query.edit_message_text( | |
| text=f"{original_text}\n\n⛔ You are not permitted to run /{cmd_name}", | |
| parse_mode="Markdown" | |
| ) | |
| return "Permission denied" |
| # Update the message to show command was executed | ||
| await query.edit_message_text( | ||
| text=f"{query.message.text}\n\n✅ Command executed: /{cmd_name}", | ||
| parse_mode="Markdown" | ||
| ) | ||
| return f"Command {cmd_name} executed" | ||
| except Exception as e: | ||
| logger.error(f"Command handler error: {e}") | ||
| await query.edit_message_text( | ||
| text=f"{query.message.text}\n\n❌ Error executing command", | ||
| parse_mode="Markdown" | ||
| ) |
There was a problem hiding this comment.
query.message.text null-dereference in the success and error paths of the command handler — see the companion comment on the permission-check branch above.
| # Update the message to show command was executed | |
| await query.edit_message_text( | |
| text=f"{query.message.text}\n\n✅ Command executed: /{cmd_name}", | |
| parse_mode="Markdown" | |
| ) | |
| return f"Command {cmd_name} executed" | |
| except Exception as e: | |
| logger.error(f"Command handler error: {e}") | |
| await query.edit_message_text( | |
| text=f"{query.message.text}\n\n❌ Error executing command", | |
| parse_mode="Markdown" | |
| ) | |
| # Update the message to show command was executed | |
| original_text = query.message.text if query.message else "" | |
| await query.edit_message_text( | |
| text=f"{original_text}\n\n✅ Command executed: /{cmd_name}", | |
| parse_mode="Markdown" | |
| ) | |
| return f"Command {cmd_name} executed" | |
| except Exception as e: | |
| logger.error(f"Command handler error: {e}") | |
| original_text = query.message.text if query.message else "" | |
| await query.edit_message_text( | |
| text=f"{original_text}\n\n❌ Error executing command", | |
| parse_mode="Markdown" | |
| ) |
* fix: add interactive action registry for button/select callbacks (fixes #2104) - Created InteractiveRegistry and dispatch protocol in core SDK - Added encode_action/decode_callback for typed action handling - Updated Telegram, Discord, and Slack adapters to use registry - Registered handlers for 'command' and 'pair' namespaces - Maintains backward compatibility with existing pairing callbacks Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com> * fix: address critical issues in interactive registry implementation - Fix dispatch() to properly return False when handlers return None - Replace global registry singleton with create_registry() for adapter-specific instances - Fix invalid BotMessage constructor arguments (chat_id, command, command_args moved to metadata) - Remove redundant TYPE_CHECKING imports - Remove dead code for 'approve' command special case - Use field(default_factory=dict) for proper dataclass default Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com> * fix: remove duplicate BotOSProtocol import from TYPE_CHECKING block The import was redundant as BotOSProtocol is already imported at runtime (line 23) and needs to be available for the __all__ export list. --------- Co-authored-by: praisonai-triage-agent[bot] <272766704+praisonai-triage-agent[bot]@users.noreply.github.com> Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
1 participant
Fixes #2104
Summary
This PR implements a symmetric inbound dispatch system for interactive UI elements (buttons/selects) that were previously rendering correctly but not handling callbacks except for pairing cases.
Changes
Core SDK (praisonaiagents/bots/):
Channel Adapters (praisonai/bots/):
Design
The solution follows protocol-driven design principles:
Testing
Summary by CodeRabbit