From bef6e33a7ba7162950c8c914de43fa80776ffed6 Mon Sep 17 00:00:00 2001 From: Keyrxng <106303466+Keyrxng@users.noreply.github.com> Date: Wed, 23 Oct 2024 23:40:18 +0100 Subject: [PATCH 1/4] feat: type-safe proxy callbacks --- .github/workflows/compute.yml | 10 ++-- .github/workflows/update-configuration.yml | 2 +- package.json | 2 +- src/handlers/comment-created-callback.ts | 41 ++++++++++++++ src/helpers/callback-proxy.ts | 65 ++++++++++++++++++++++ src/helpers/errors.ts | 29 ++++++++++ src/plugin.ts | 51 +---------------- src/types/proxy.ts | 24 ++++++++ tests/main.test.ts | 4 +- 9 files changed, 171 insertions(+), 57 deletions(-) create mode 100644 src/handlers/comment-created-callback.ts create mode 100644 src/helpers/callback-proxy.ts create mode 100644 src/helpers/errors.ts create mode 100644 src/types/proxy.ts diff --git a/.github/workflows/compute.yml b/.github/workflows/compute.yml index 285665e..533c5ec 100644 --- a/.github/workflows/compute.yml +++ b/.github/workflows/compute.yml @@ -45,8 +45,8 @@ jobs: run: yarn tsx ./src/main.ts id: command-ask env: - SUPABASE_URL: ${{ secrets.SUPABASE_URL }} - SUPABASE_KEY: ${{ secrets.SUPABASE_KEY }} - VOYAGEAI_API_KEY: ${{ secrets.VOYAGEAI_API_KEY }} - OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} - UBIQUITY_OS_APP_NAME: ${{ secrets.UBIQUITY_OS_APP_NAME }} \ No newline at end of file + SUPABASE_URL: ${{ secrets.SUPABASE_URL }} + SUPABASE_KEY: ${{ secrets.SUPABASE_KEY }} + VOYAGEAI_API_KEY: ${{ secrets.VOYAGEAI_API_KEY }} + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + UBIQUITY_OS_APP_NAME: ${{ secrets.UBIQUITY_OS_APP_NAME }} diff --git a/.github/workflows/update-configuration.yml b/.github/workflows/update-configuration.yml index 2d366d6..b92a487 100644 --- a/.github/workflows/update-configuration.yml +++ b/.github/workflows/update-configuration.yml @@ -18,4 +18,4 @@ jobs: commitMessage: "chore: updated manifest.json and dist build" nodeVersion: "20.10.0" env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} \ No newline at end of file + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/package.json b/package.json index 94d1357..32061fa 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "knip-ci": "knip --no-exit-code --reporter json --config .github/knip.ts", "prepare": "husky install", "test": "jest --setupFiles dotenv/config --coverage", - "worker": "wrangler dev --env dev --port 5000" + "worker": "wrangler dev --env dev --port 4000" }, "keywords": [ "typescript", diff --git a/src/handlers/comment-created-callback.ts b/src/handlers/comment-created-callback.ts new file mode 100644 index 0000000..8301a2a --- /dev/null +++ b/src/handlers/comment-created-callback.ts @@ -0,0 +1,41 @@ +import { Context, SupportedEvents } from "../types"; +import { addCommentToIssue } from "./add-comment"; +import { askQuestion } from "./ask-llm"; +import { CallbackResult } from "../types/proxy"; +import { bubbleUpErrorComment } from "../helpers/errors"; + +export async function issueCommentCreatedCallback( + context: Context<"issue_comment.created", SupportedEvents["issue_comment.created"]> +): Promise { + const { + logger, + env: { UBIQUITY_OS_APP_NAME }, + } = context; + const question = context.payload.comment.body; + const slugRegex = new RegExp(`@${UBIQUITY_OS_APP_NAME} `, "gi"); + if (!question.match(slugRegex)) { + return { status: 204, reason: logger.info("Comment does not mention the app. Skipping.").logMessage.raw }; + } + if (context.payload.comment.user?.type === "Bot") { + return { status: 204, reason: logger.info("Comment is from a bot. Skipping.").logMessage.raw }; + } + if (question.replace(slugRegex, "").trim().length === 0) { + return { status: 204, reason: logger.info("Comment is empty. Skipping.").logMessage.raw }; + } + logger.info(`Asking question: ${question}`); + let commentToPost; + try { + const response = await askQuestion(context, question); + const { answer, tokenUsage } = response; + if (!answer) { + throw logger.error(`No answer from OpenAI`); + } + logger.info(`Answer: ${answer}`, { tokenUsage }); + const tokens = `\n\n`; + commentToPost = answer + tokens; + await addCommentToIssue(context, commentToPost); + return { status: 200, reason: logger.info("Comment posted successfully").logMessage.raw }; + } catch (err) { + await bubbleUpErrorComment(context, err, false) + } +} diff --git a/src/helpers/callback-proxy.ts b/src/helpers/callback-proxy.ts new file mode 100644 index 0000000..ffbb810 --- /dev/null +++ b/src/helpers/callback-proxy.ts @@ -0,0 +1,65 @@ +import { issueCommentCreatedCallback } from "../handlers/comment-created-callback"; +import { Context, SupportedEventsU } from "../types"; +import { ProxyCallbacks } from "../types/proxy"; +import { bubbleUpErrorComment } from "./errors"; + +/** + * The `callbacks` object defines an array of callback functions for each supported event type. + * + * Since multiple callbacks might need to be executed for a single event, we store each + * callback in an array. This design allows for extensibility and flexibility, enabling + * us to add more callbacks for a particular event without modifying the core logic. + */ +const callbacks = { + "issue_comment.created": [issueCommentCreatedCallback], +} as ProxyCallbacks; + +/** + * The `proxyCallbacks` function returns a Proxy object that intercepts access to the + * `callbacks` object. This Proxy enables dynamic handling of event callbacks, including: + * + * - **Event Handling:** When an event occurs, the Proxy looks up the corresponding + * callbacks in the `callbacks` object. If no callbacks are found for the event, + * it returns a `skipped` status. + * + * - **Error Handling:** If an error occurs while processing a callback, the Proxy + * logs the error and returns a `failed` status. + * + * The Proxy uses the `get` trap to intercept attempts to access properties on the + * `callbacks` object. This trap allows us to asynchronously execute the appropriate + * callbacks based on the event type, ensuring that the correct context is passed to + * each callback. + */ +export function proxyCallbacks(context: Context): ProxyCallbacks { + return new Proxy(callbacks, { + get(target, prop: SupportedEventsU) { + if (!target[prop]) { + context.logger.info(`No callbacks found for event ${prop}`); + return { status: 204, reason: "skipped" }; + } + return (async () => { + try { + return await Promise.all(target[prop].map((callback) => handleCallback(callback, context))); + } catch (er) { + return { status: 500, reason: await bubbleUpErrorComment(context, er) }; + } + })(); + }, + }); +} + +/** + * Why do we need this wrapper function? + * + * By using a generic `Function` type for the callback parameter, we bypass strict type + * checking temporarily. This allows us to pass a standard `Context` object, which we know + * contains the correct event and payload types, to the callback safely. + * + * We can trust that the `ProxyCallbacks` type has already ensured that each callback function + * matches the expected event and payload types, so this function provides a safe and + * flexible way to handle callbacks without introducing type or logic errors. + */ +// eslint-disable-next-line @typescript-eslint/ban-types +export function handleCallback(callback: Function, context: Context) { + return callback(context); +} diff --git a/src/helpers/errors.ts b/src/helpers/errors.ts new file mode 100644 index 0000000..a8a5839 --- /dev/null +++ b/src/helpers/errors.ts @@ -0,0 +1,29 @@ +import { LogReturn, Logs } from "@ubiquity-dao/ubiquibot-logger"; +import { Context } from "../types"; +import { addCommentToIssue } from "../handlers/add-comment"; +const logger = new Logs("debug"); + +export function handleUncaughtError(err: unknown) { + logger.error("An uncaught error occurred", { err }); + return new Response(JSON.stringify({ err }), { status: 500, headers: { "content-type": "application/json" } }); +} +export function sanitizeMetadata(obj: LogReturn["metadata"]): string { + return JSON.stringify(obj, null, 2).replace(//g, ">").replace(/--/g, "--"); +} + +export async function bubbleUpErrorComment(context: Context, err: unknown, post = true): Promise { + let errorMessage; + if (err instanceof LogReturn) { + errorMessage = err; + } else if (err instanceof Error) { + errorMessage = context.logger.error(err.message, { error: err }); + } else { + errorMessage = context.logger.error("An error occurred", { err }); + } + + if (post) { + await addCommentToIssue(context, `${errorMessage?.logMessage.diff}\n`); + } + + return errorMessage; +} diff --git a/src/plugin.ts b/src/plugin.ts index 8eab234..bdfc3e8 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -1,14 +1,13 @@ import { Octokit } from "@octokit/rest"; import { PluginInputs } from "./types"; import { Context } from "./types"; -import { askQuestion } from "./handlers/ask-llm"; -import { addCommentToIssue } from "./handlers/add-comment"; -import { LogLevel, LogReturn, Logs } from "@ubiquity-dao/ubiquibot-logger"; +import { LogLevel, Logs } from "@ubiquity-dao/ubiquibot-logger"; import { Env } from "./types/env"; import { createAdapters } from "./adapters"; import { createClient } from "@supabase/supabase-js"; import { VoyageAIClient } from "voyageai"; import OpenAI from "openai"; +import { proxyCallbacks } from "./helpers/callback-proxy"; export async function plugin(inputs: PluginInputs, env: Env) { const octokit = new Octokit({ auth: inputs.authToken }); @@ -35,49 +34,5 @@ export async function plugin(inputs: PluginInputs, env: Env) { } export async function runPlugin(context: Context) { - const { - logger, - env: { UBIQUITY_OS_APP_NAME }, - } = context; - const question = context.payload.comment.body; - const slugRegex = new RegExp(`@${UBIQUITY_OS_APP_NAME} `, "gi"); - if (!question.match(slugRegex)) { - logger.info("Comment does not mention the app. Skipping."); - return; - } - if (context.payload.comment.user?.type === "Bot") { - logger.info("Comment is from a bot. Skipping."); - return; - } - if (question.replace(slugRegex, "").trim().length === 0) { - logger.info("Comment is empty. Skipping."); - return; - } - logger.info(`Asking question: ${question}`); - let commentToPost; - try { - const response = await askQuestion(context, question); - const { answer, tokenUsage } = response; - if (!answer) { - throw logger.error(`No answer from OpenAI`); - } - logger.info(`Answer: ${answer}`, { tokenUsage }); - const tokens = `\n\n`; - commentToPost = answer + tokens; - } catch (err) { - let errorMessage; - if (err instanceof LogReturn) { - errorMessage = err; - } else if (err instanceof Error) { - errorMessage = context.logger.error(err.message, { error: err, stack: err.stack }); - } else { - errorMessage = context.logger.error("An error occurred", { err }); - } - commentToPost = `${errorMessage?.logMessage.diff}\n`; - } - await addCommentToIssue(context, commentToPost); -} - -function sanitizeMetadata(obj: LogReturn["metadata"]): string { - return JSON.stringify(obj, null, 2).replace(//g, ">").replace(/--/g, "--"); + return proxyCallbacks(context)[context.eventName]; } diff --git a/src/types/proxy.ts b/src/types/proxy.ts new file mode 100644 index 0000000..b770913 --- /dev/null +++ b/src/types/proxy.ts @@ -0,0 +1,24 @@ +import { Context, SupportedEvents, SupportedEventsU } from "./context"; + +export type CallbackResult = { status: 200 | 201 | 204 | 404 | 500; reason: string; content?: string | Record }; + +/** + * The `Context` type is a generic type defined as `Context`, + * where `TEvent` is a string representing the event name (e.g., "issues.labeled") + * and `TPayload` is the webhook payload type for that event, derived from + * the `SupportedEvents` type map. + * + * The `ProxyCallbacks` object is cast to allow optional callbacks + * for each event type. This is useful because not all events may have associated callbacks. + * As opposed to Partial which could mean an undefined object. + * + * The expected function signature for callbacks looks like this: + * + * ```typescript + * fn(context: Context<"issues.labeled", SupportedEvents["issues.labeled"]>): Promise + * ``` + */ + +export type ProxyCallbacks = { + [K in SupportedEventsU]: Array<(context: Context) => Promise>; +}; diff --git a/tests/main.test.ts b/tests/main.test.ts index 57c0e72..935a113 100644 --- a/tests/main.test.ts +++ b/tests/main.test.ts @@ -106,7 +106,7 @@ describe("Ask plugin tests", () => { createComments([transformCommentTemplate(1, 1, TEST_QUESTION, "ubiquity", "test-repo", true)]); await runPlugin(ctx); - expect(infoSpy).toHaveBeenCalledTimes(3); + expect(infoSpy).toHaveBeenCalledTimes(4); expect(infoSpy).toHaveBeenNthCalledWith(1, `Asking question: @UbiquityOS ${TEST_QUESTION}`); expect(infoSpy).toHaveBeenNthCalledWith(3, "Answer: This is a mock answer for the chat", { caller: LOG_CALLER, @@ -130,7 +130,7 @@ describe("Ask plugin tests", () => { await runPlugin(ctx); - expect(infoSpy).toHaveBeenCalledTimes(3); + expect(infoSpy).toHaveBeenCalledTimes(4); expect(infoSpy).toHaveBeenNthCalledWith(1, `Asking question: @UbiquityOS ${TEST_QUESTION}`); From 02d867d1cc68a575922b6ed75de4dfd90571b290 Mon Sep 17 00:00:00 2001 From: Keyrxng <106303466+Keyrxng@users.noreply.github.com> Date: Wed, 23 Oct 2024 23:49:31 +0100 Subject: [PATCH 2/4] chore: fix catch-all error handler --- src/handlers/comment-created-callback.ts | 2 +- src/helpers/callback-proxy.ts | 2 +- src/helpers/errors.ts | 2 +- src/helpers/issue-fetching.ts | 5 +++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/handlers/comment-created-callback.ts b/src/handlers/comment-created-callback.ts index 8301a2a..471a0bc 100644 --- a/src/handlers/comment-created-callback.ts +++ b/src/handlers/comment-created-callback.ts @@ -36,6 +36,6 @@ export async function issueCommentCreatedCallback( await addCommentToIssue(context, commentToPost); return { status: 200, reason: logger.info("Comment posted successfully").logMessage.raw }; } catch (err) { - await bubbleUpErrorComment(context, err, false) + throw err; } } diff --git a/src/helpers/callback-proxy.ts b/src/helpers/callback-proxy.ts index ffbb810..a50da5e 100644 --- a/src/helpers/callback-proxy.ts +++ b/src/helpers/callback-proxy.ts @@ -41,7 +41,7 @@ export function proxyCallbacks(context: Context): ProxyCallbacks { try { return await Promise.all(target[prop].map((callback) => handleCallback(callback, context))); } catch (er) { - return { status: 500, reason: await bubbleUpErrorComment(context, er) }; + return { status: 500, reason: (await bubbleUpErrorComment(context, er)).logMessage.raw }; } })(); }, diff --git a/src/helpers/errors.ts b/src/helpers/errors.ts index a8a5839..5372863 100644 --- a/src/helpers/errors.ts +++ b/src/helpers/errors.ts @@ -1,7 +1,7 @@ import { LogReturn, Logs } from "@ubiquity-dao/ubiquibot-logger"; import { Context } from "../types"; import { addCommentToIssue } from "../handlers/add-comment"; -const logger = new Logs("debug"); +export const logger = new Logs("debug"); export function handleUncaughtError(err: unknown) { logger.error("An uncaught error occurred", { err }); diff --git a/src/helpers/issue-fetching.ts b/src/helpers/issue-fetching.ts index fc1df5a..c3d9249 100644 --- a/src/helpers/issue-fetching.ts +++ b/src/helpers/issue-fetching.ts @@ -3,6 +3,7 @@ import { Context } from "../types"; import { IssueWithUser, SimplifiedComment, User } from "../types/github-types"; import { FetchParams, Issue, Comments, LinkedIssues } from "../types/github-types"; import { StreamlinedComment } from "../types/llm"; +import { logger } from "./errors"; import { dedupeStreamlinedComments, fetchCodeLinkedFromIssue, @@ -41,11 +42,11 @@ export async function fetchLinkedIssues(params: FetchParams) { return { streamlinedComments: {}, linkedIssues: [], specAndBodies: {}, seen: new Set() }; } if (!issue.body || !issue.html_url) { - throw new Error("Issue body or URL not found"); + throw logger.error("Issue body or URL not found"); } if (!params.owner || !params.repo) { - throw new Error("Owner, repo, or issue number not found"); + throw logger.error("Owner or repo not found"); } const issueKey = createKey(issue.html_url); const [owner, repo, issueNumber] = splitKey(issueKey); From dd203454c146dcec38c738030d79b02e3781479e Mon Sep 17 00:00:00 2001 From: Keyrxng <106303466+Keyrxng@users.noreply.github.com> Date: Thu, 24 Oct 2024 00:17:44 +0100 Subject: [PATCH 3/4] chore: error handling --- src/handlers/comment-created-callback.ts | 8 ++++---- src/handlers/comments.ts | 6 +++++- src/helpers/errors.ts | 2 +- src/helpers/issue-fetching.ts | 2 +- src/helpers/issue.ts | 3 ++- src/main.ts | 2 ++ 6 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/handlers/comment-created-callback.ts b/src/handlers/comment-created-callback.ts index 471a0bc..b366f70 100644 --- a/src/handlers/comment-created-callback.ts +++ b/src/handlers/comment-created-callback.ts @@ -23,7 +23,7 @@ export async function issueCommentCreatedCallback( return { status: 204, reason: logger.info("Comment is empty. Skipping.").logMessage.raw }; } logger.info(`Asking question: ${question}`); - let commentToPost; + try { const response = await askQuestion(context, question); const { answer, tokenUsage } = response; @@ -32,10 +32,10 @@ export async function issueCommentCreatedCallback( } logger.info(`Answer: ${answer}`, { tokenUsage }); const tokens = `\n\n`; - commentToPost = answer + tokens; + const commentToPost = answer + tokens; await addCommentToIssue(context, commentToPost); return { status: 200, reason: logger.info("Comment posted successfully").logMessage.raw }; - } catch (err) { - throw err; + } catch (error) { + throw await bubbleUpErrorComment(context, error, false); } } diff --git a/src/handlers/comments.ts b/src/handlers/comments.ts index d7e2d0f..b033686 100644 --- a/src/handlers/comments.ts +++ b/src/handlers/comments.ts @@ -1,3 +1,4 @@ +import { logger } from "../helpers/errors"; import { splitKey } from "../helpers/issue"; import { LinkedIssues, SimplifiedComment } from "../types/github-types"; import { StreamlinedComment } from "../types/llm"; @@ -53,7 +54,10 @@ export function createKey(issueUrl: string, issue?: number) { } if (!key) { - throw new Error("Invalid issue url"); + throw logger.error("Invalid issue URL", { + issueUrl, + issueNumber: issue, + }); } if (key.includes("#")) { diff --git a/src/helpers/errors.ts b/src/helpers/errors.ts index 5372863..e53a425 100644 --- a/src/helpers/errors.ts +++ b/src/helpers/errors.ts @@ -16,7 +16,7 @@ export async function bubbleUpErrorComment(context: Context, err: unknown, post if (err instanceof LogReturn) { errorMessage = err; } else if (err instanceof Error) { - errorMessage = context.logger.error(err.message, { error: err }); + errorMessage = context.logger.error(err.message, { stack: err.stack }); } else { errorMessage = context.logger.error("An error occurred", { err }); } diff --git a/src/helpers/issue-fetching.ts b/src/helpers/issue-fetching.ts index c3d9249..744e74e 100644 --- a/src/helpers/issue-fetching.ts +++ b/src/helpers/issue-fetching.ts @@ -42,7 +42,7 @@ export async function fetchLinkedIssues(params: FetchParams) { return { streamlinedComments: {}, linkedIssues: [], specAndBodies: {}, seen: new Set() }; } if (!issue.body || !issue.html_url) { - throw logger.error("Issue body or URL not found"); + throw logger.error("Issue body or URL not found", { issueUrl: issue.html_url }); } if (!params.owner || !params.repo) { diff --git a/src/helpers/issue.ts b/src/helpers/issue.ts index 68fcda7..0effb4a 100644 --- a/src/helpers/issue.ts +++ b/src/helpers/issue.ts @@ -2,6 +2,7 @@ import { createKey } from "../handlers/comments"; import { FetchedCodes, FetchParams, LinkedIssues } from "../types/github-types"; import { StreamlinedComment } from "../types/llm"; import { Context } from "../types/context"; // Import Context type +import { logger } from "./errors"; /** * Removes duplicate streamlined comments based on their body content. @@ -239,7 +240,7 @@ export async function pullReadmeFromRepoForIssue(params: FetchParams): Promise Date: Thu, 24 Oct 2024 00:40:38 +0100 Subject: [PATCH 4/4] chore: knip --- src/helpers/errors.ts | 8 +++++--- src/worker.ts | 7 +------ 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/helpers/errors.ts b/src/helpers/errors.ts index e53a425..84da985 100644 --- a/src/helpers/errors.ts +++ b/src/helpers/errors.ts @@ -3,10 +3,12 @@ import { Context } from "../types"; import { addCommentToIssue } from "../handlers/add-comment"; export const logger = new Logs("debug"); -export function handleUncaughtError(err: unknown) { - logger.error("An uncaught error occurred", { err }); - return new Response(JSON.stringify({ err }), { status: 500, headers: { "content-type": "application/json" } }); +export function handleUncaughtError(error: unknown) { + logger.error("An uncaught error occurred", { err: error }); + const status = 500; + return new Response(JSON.stringify({ error }), { status: status, headers: { "content-type": "application/json" } }); } + export function sanitizeMetadata(obj: LogReturn["metadata"]): string { return JSON.stringify(obj, null, 2).replace(//g, ">").replace(/--/g, "--"); } diff --git a/src/worker.ts b/src/worker.ts index b713c77..a8dcee2 100644 --- a/src/worker.ts +++ b/src/worker.ts @@ -3,6 +3,7 @@ import { pluginSettingsSchema, pluginSettingsValidator } from "./types"; import { Env, envValidator } from "./types/env"; import manifest from "../manifest.json"; import { plugin } from "./plugin"; +import { handleUncaughtError } from "./helpers/errors"; export default { async fetch(request: Request, env: Env): Promise { @@ -62,9 +63,3 @@ export default { } }, }; - -function handleUncaughtError(error: unknown) { - console.error(error); - const status = 500; - return new Response(JSON.stringify({ error }), { status: status, headers: { "content-type": "application/json" } }); -}