ubiquity-os / ubiquity-os-kernel

1 stars 13 forks source link

Handling `issues.assigned` for `task-xp-guard` #94

Open Keyrxng opened 1 month ago

Keyrxng commented 1 month ago

Related to https://github.com/ubiquibot/task-xp-guard/pull/1

The issue is that after using the /start command an issues.assigned event is fired but this line in the kernel skips the plugin because skipBotEvents defaults to true and the sender of that event is the bot which is valid in regards to issues.assigned/task-xp-guard.

Which is why I could only get it to run if I created a two-step chain passing the issue_comment.created event forward from start-stop to task-xp-guard

My quick workaround below but is there a better way to handle this sort of thing? I have tried using skipBotEvents: false on both plugins with no success. The only thing that seems to work for me is below.

 if (
    pluginChain.skipBotEvents && 
    "sender" in event.payload && 
    event.payload.sender?.type === "Bot" && 
    context.key !== "issues.assigned"
  ) {
    console.log("Skipping plugin chain because sender is a bot");
    return true;
  }
0x4007 commented 1 month ago

@gentlementlegen @whilefoo rfc

gentlementlegen commented 1 month ago

I had a similar issue when using automated-merging with conversation-rewards being skipped due to the bot nature of the action. I tried to use skipBotEvents but no task was closed by automation yet so I cannot confirm if it works or not. I think this is important to solve, which technically should be handled just by setting skipBotEvents to false.

whilefoo commented 1 month ago

@Keyrxng did you try console logging the skipBotEvents? because it should definitely not skip if it's set to false. I will try this myself too

whilefoo commented 3 weeks ago

@Keyrxng can you show me the config you used?

whilefoo commented 3 weeks ago

also I'm thinking that skipBotEvents should be in the manifest because the plugin developer should set it accordingly and not the partner

Keyrxng commented 3 weeks ago

also I'm thinking that skipBotEvents should be in the manifest because the plugin developer should set it accordingly and not the partner

Agreed

can you show me the config you used?

I posted one here. But I tried a variety, the one posted just attempts to show my configuration in installing both:

I tried with skipBotEvents defined and undefined in both plugins and tried to cover all bases.

whilefoo commented 3 weeks ago

Because I'm wondering if you set skipBotEvents correctly on the plugin chain and not on the plugin itself

Keyrxng commented 3 weeks ago

I think we could be doing with better naming conventions for plugin chaining lmao, just me that finds it slightly confusing when discussing chains?

Because I'm wondering if you set skipBotEvents correctly on the plugin chain and not on the plugin itself

I set skipBotEvents on each plugin but your comment reads like I should have placed it before - plugin and after - uses. Is that how it should be, where the comment is? I just tried this and the kernel complains

plugins:
  - uses:
    # skipBotEvents: false
    - plugin: http://localhost:4001
      runsOn: [ "issues.assigned"]
      skipBotEvents: false # seems to have no effect whether true or false
  - uses:
    - plugin: http://localhost:4000
      runsOn:  ["issue_comment.created"]
      skipBotEvents: false
Keyrxng commented 3 weeks ago

This is the kernel logs running /start with task-xp-guard

image

This is an added log inside the conditional showing it hits on issues.assigned.

image

Keyrxng commented 3 weeks ago

@gentlementlegen I can't find the discussion but it was mentioned about the base event such as issues being a valid event but the kernel not accepting it?

It shows in logs above that it's receiving the base event as a payload perhaps that has something to do with this problem @whilefoo?

gentlementlegen commented 3 weeks ago

@Keyrxng the event logged does not represent the real event used, it is very likely issues.some_event

whilefoo commented 3 weeks ago

It should be like this:

plugins:
  - uses:
    - plugin: http://localhost:4001
      runsOn: [ "issues.assigned"]
    skipBotEvents: false
  - uses:
    - plugin: http://localhost:4000
      runsOn:  ["issue_comment.created"]
    skipBotEvents: false
Keyrxng commented 3 weeks ago

It should be like this:

plugins:
  - uses:
    - plugin: http://localhost:4001
      runsOn: [ "issues.assigned"]
    skipBotEvents: false
  - uses:
    - plugin: http://localhost:4000
      runsOn:  ["issue_comment.created"]
    skipBotEvents: false

😓 So it does work with the config set like this. My bad, although it's weird syntax I think because skipBotEvents is an item of the - uses array not an item of the plugin array but if you place skipBotEvents before - plugin it doesn't work despite it still being in the same - uses block.

I think your idea to move this into the manifest is a better idea. I guess this will also be the solution for #98

whilefoo commented 2 weeks ago

skipBotEvents is outside uses even though it may not look like it This is how you do it:

plugins:
  - skipBotEvents: false
    uses:
    - plugin: http://localhost:4001
      runsOn: [ "issues.assigned"]

but it is a bit confusing because there are two nested arrays

Keyrxng commented 2 weeks ago

Thanks for explaining, it makes more sense seeing that.