ubiquity-os-marketplace / command-ask

0 stars 5 forks source link

refactor: proxy callbacks #9

Closed Keyrxng closed 1 month ago

Keyrxng commented 1 month ago

Resolves #10 Required by https://github.com/ubiquity-os/plugins-wishlist/issues/45

This PR adds the proxyCallback logic that we use in other plugins. I'm adding it because it's the cleanest most effective way to handle type-safe payloads across every webhook event and I need it for handling pull_request events.


I've noticed a separate issue regarding Logs which is that sometimes we get stack traces, sometimes we don't which is not good.

 if (!issue.body || !issue.html_url) {
    throw logger.error("Issue body or URL not found", { issueUrl: issue.html_url });
  }

This should generate a stack but instead we get:

image

  if (!issue.body || !issue.html_url) {
    throw logger.error("Issue body or URL not found", { issueUrl: issue.html_url, error: new Error("Issue body or URL not found") });
  }

If we manually create an error in the metadata we get a stack trace but you can clearly see that error is an empty object which is wrong which is where my suspicion comes from that the logger is wiping out that property under certain conditions. image

Will open a task to get it addressed

github-actions[bot] commented 1 month ago

Unused types (1)

Filename types
src/types/github-types.ts IssueWithUser
Keyrxng commented 1 month ago

QA: https://github.com/ubq-testing/ask-plugin/issues/7

Keyrxng commented 1 month ago

I'm unable to request review for some reason so gonna tag instead

@gentlementlegen @0x4007 @sshivaditya2019 @sshivaditya @rndquu

0x4007 commented 1 month ago

I've never got my own kernel/plugin set up so I don't have any context on this problem and can't add any value in the form of a review.

Keyrxng commented 1 month ago

I've never got my own kernel/plugin set up so I don't have any context on this problem and can't add any value in the form of a review.

It's the solution to this https://github.com/ubiquity-os/ubiquity-os-kernel/issues/10

Keyrxng commented 1 month ago

QA: https://github.com/ubq-testing/ask-plugin/issues/7#issuecomment-2437802271

Keyrxng commented 1 month ago

QA: https://github.com/ubq-testing/ask-plugin/issues/11#issuecomment-2442253913

Good to merge? @sshivaditya2019 @gentlementlegen @whilefoo

0x4007 commented 1 month ago

It's extremely redundant. This can all be expressed in a sentence or two.

Keyrxng commented 1 month ago

It's extremely redundant. This can all be expressed in a sentence or two.

I don't understand what you are referring to

QA demonstrates that the plugin is working that's all it has nothing to do with the content in the LLM response. This PR is about adding the proxy so we can handle multiple webhook event payloads. If you are referring to ground truths there a spec to improve them #17

Keyrxng commented 1 month ago

Don't hesitate to move that to the SDK, it makes things much simpler!

Yeah we definitely should build our webhook handler logic into the SDK

I have a couple of tasks I'll close out first but I won't forget about it

Keyrxng commented 1 month ago

I can't merge if it's me we are waiting on for this fellas