Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question not provided error. #53

Open
shiv810 opened this issue Jan 22, 2025 · 15 comments
Open

Question not provided error. #53

shiv810 opened this issue Jan 22, 2025 · 15 comments

Comments

@shiv810
Copy link
Collaborator

shiv810 commented Jan 22, 2025

The kernel is forwarding, all pull_request_review.comments without processing them, through command router, leading to the Question Not Provided error.

Originally posted by @shiv810 in ubiquity-os-marketplace/text-conversation-rewards#218 (comment)

Copy link

Note

The following contributors may be suitable for this task:

gentlementlegen

78% Match ubiquity-os-marketplace/text-conversation-rewards#95
77% Match ubiquity-os/ubiquity-os-kernel#105

Keyrxng

77% Match ubiquity-os/ubiquity-os-logger#2

@shiv810
Copy link
Collaborator Author

shiv810 commented Jan 22, 2025

Also tracked in #241

@0x4007
Copy link
Member

0x4007 commented Jan 23, 2025

When you open a new issue you need to put a time label on it

Copy link

Important

  • Be sure to link a pull-request before the first reminder to avoid disqualification.
  • Reminders will be sent every 7 days if there is no activity.
  • Assignees will be disqualified after 14 days of inactivity.

@0x4007
Copy link
Member

0x4007 commented Jan 23, 2025

Actually, only time label. I'll handle priority label!

@0x4007
Copy link
Member

0x4007 commented Jan 23, 2025

Also tracked in #241

Why did you post the same problem twice? I don't really understand the specifics of either problem, but these probably should be consolidated into a single task if you're saying that it's "also tracked at..."

As I understand the solution requires changes across two repositories. It should be tracked under a single task because we don't allow for double dipping rewards.

@0x4007
Copy link
Member

0x4007 commented Jan 23, 2025

@gentlementlegen you understand events better in the context of kernel-plugin communications. Is this a regression?

@gentlementlegen
Copy link
Member

Not sure to understand what without processing them means. There is no special case for pull_request_review.comments and the flow is the same for any event.

@shiv810
Copy link
Collaborator Author

shiv810 commented Jan 23, 2025

Not sure to understand what without processing them means. There is no special case for pull_request_review.comments and the flow is the same for any event.

The plugin currently expects the question to be either part of the plugin input from the kernel or have "/ask" included in the comment. If neither of these conditions is met, it will throw a Question not Provided error.

The kernel uses the command router only for the issue_comment.created event, which means that command-ask is invoked selectively. Without this, command-ask will be called for every pull_request_review_comment.created event.

@shiv810
Copy link
Collaborator Author

shiv810 commented Jan 23, 2025

@0x4007 Should I rollback pull_request_review_comment.created support, or update the kernel the support this ?

@0x4007
Copy link
Member

0x4007 commented Jan 23, 2025

@gentlementlegen @whilefoo rfc

Anyways as I understand the plugin should work with the same rules as other plugins. There shouldn't be special accommodations in the kernel for it.

Besides this was working fine up until a couple of days ago.

@gentlementlegen
Copy link
Member

Okay I get it now. I think we should avoid having custom implementation for this ideally. You can filter out the with the payloads you get and check if /ask or Command is populated, otherwise ignore that.

@0x4007
Copy link
Member

0x4007 commented Jan 24, 2025

Seems like a lot of unnecessary calls to the GitHub API if every command runs and then self cancels with filters.

@whilefoo
Copy link
Member

The plugin manifest specifies "ubiquity:listeners": ["issue_comment.created", "pull_request_review_comment.created"] and /ask command.
The kernel skips the plugin if it has a command and it doesn't match the issue comment, but it doesn't skip if the command doesn't match PR review comment which are two separate events.
This should be optimized but the main issue is that the plugin should also do the check and if the command is null and no /ask in the comment, it shouldn't even respond.

What I recommend to avoid calling the plugin unnecessarily:
If a plugin only wants to handle commands but not every event, it should not specify any listeners and only a command. The kernel can then be modified to automatically check the comment and only run the plugin if it matches the command, but if the plugin specifies issue_comment.created in the manifest then it would send the event every time.

@0x4007
Copy link
Member

0x4007 commented Jan 24, 2025

@shiv810 please take whilefoo's suggestion and adjust the plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants