From acacaaef87b642c1ca0a35c08c7b146b9e12a242 Mon Sep 17 00:00:00 2001 From: Daniel Campagnoli Date: Tue, 14 Oct 2025 13:56:58 +0800 Subject: [PATCH] Update GitLab code review to only start an agent if there's code review tasks. Updated tsconfig and fixed misc compile issues --- src/functions/confluence.ts | 2 +- src/functions/jira.ts | 2 +- src/functions/scm/gitlabCodeReview.ts | 19 ++++++++-- src/modules/slack/slackConfig.ts | 2 +- src/routes/codeTask/createCodeTaskRoute.ts | 7 ++-- src/routes/codeTask/createPresetRoute.ts | 3 +- src/routes/codeTask/deleteCodeTaskRoute.ts | 3 +- src/routes/codeTask/deletePresetRoute.ts | 3 +- src/routes/codeTask/executeDesignRoute.ts | 5 +-- src/routes/codeTask/generateDesignRoute.ts | 7 ++-- src/routes/codeTask/getCodeTaskByIdRoute.ts | 3 +- src/routes/codeTask/getFileContentRoute.ts | 3 +- src/routes/codeTask/getFileSystemTreeRoute.ts | 3 +- src/routes/codeTask/getRepoBranchesRoute.ts | 3 +- src/routes/codeTask/listCodeTasksRoute.ts | 3 +- src/routes/codeTask/listPresetsRoute.ts | 3 +- src/routes/codeTask/resetSelectionRoute.ts | 5 +-- src/routes/codeTask/updateCodeRoute.ts | 5 +-- src/routes/codeTask/updateCodeTaskRoute.ts | 3 +- .../codeTask/updateDesignPromptRoute.ts | 5 +-- src/routes/codeTask/updateDesignRoute.ts | 5 +-- .../codeTask/updateSelectionPromptRoute.ts | 5 +-- src/routes/llms/llm-call-routes.ts | 1 + src/routes/webhooks/github/github-routes.ts | 36 +++++++++---------- .../webhooks/gitlab/gitlabJobHandler.ts | 2 +- src/routes/webhooks/gitlab/gitlabRoutes.ts | 26 ++++++++------ tsconfig.json | 1 + tsconfig.native.json | 4 +-- 28 files changed, 102 insertions(+), 67 deletions(-) diff --git a/src/functions/confluence.ts b/src/functions/confluence.ts index 96094a98..b120cd72 100644 --- a/src/functions/confluence.ts +++ b/src/functions/confluence.ts @@ -1,9 +1,9 @@ import { promises as fs } from 'node:fs'; import { join } from 'node:path'; import axios, { type AxiosInstance } from 'axios'; -import { getSecretEnvVar } from 'src/config/secretConfig'; import { llms } from '#agent/agentContextLocalStorage'; import { agentStorageDir } from '#app/appDirs'; +import { getSecretEnvVar } from '#config/secretConfig'; import { func, funcClass } from '#functionSchema/functionDecorators'; import { logger } from '#o11y/logger'; import { functionConfig } from '#user/userContext'; diff --git a/src/functions/jira.ts b/src/functions/jira.ts index 3519c813..3156de49 100644 --- a/src/functions/jira.ts +++ b/src/functions/jira.ts @@ -1,9 +1,9 @@ import { promises as fs } from 'node:fs'; import { join } from 'node:path'; import axios, { type AxiosInstance } from 'axios'; -import { getSecretEnvVar } from 'src/config/secretConfig'; import { llms } from '#agent/agentContextLocalStorage'; import { agentStorageDir } from '#app/appDirs'; +import { getSecretEnvVar } from '#config/secretConfig'; import { func, funcClass } from '#functionSchema/functionDecorators'; import { getJiraIssueType } from '#functions/jiraIssueType'; import { logger } from '#o11y/logger'; diff --git a/src/functions/scm/gitlabCodeReview.ts b/src/functions/scm/gitlabCodeReview.ts index 00669763..fd497fd8 100644 --- a/src/functions/scm/gitlabCodeReview.ts +++ b/src/functions/scm/gitlabCodeReview.ts @@ -80,7 +80,7 @@ export class GitLabCodeReview { } @span() - async reviewMergeRequest(gitlabProjectId: string | number, mergeRequestIId: number): Promise { + async createMergeRequestReviewTasks(gitlabProjectId: string | number, mergeRequestIId: number): Promise { const mergeRequest: ExpandedMergeRequestSchema = await this.api().MergeRequests.show(gitlabProjectId, mergeRequestIId); const diffs: MergeRequestDiffSchema[] = await this.getDiffs(gitlabProjectId, mergeRequestIId); const codeReviewService = appContext().codeReviewService; @@ -91,7 +91,9 @@ export class GitLabCodeReview { // Load the hashes of the diffs we've already reviewed const reviewCache: CodeReviewFingerprintCache = await codeReviewService.getMergeRequestReviewCache(gitlabProjectId, mergeRequestIId); - logger.info(`Reviewing MR "${mergeRequest.title}" in project "${projectPath}" (${mergeRequest.web_url}) with ${codeReviewConfigs.length} configs`); + logger.info( + `Checking for review tasks for MR "${mergeRequest.title}" in project "${projectPath}" (${mergeRequest.web_url}) with ${codeReviewConfigs.length} configs`, + ); const codeReviewTasks: CodeReviewTask[] = []; @@ -114,6 +116,19 @@ export class GitLabCodeReview { } } logger.info(`Found ${codeReviewTasks.length} review tasks needing LLM analysis.`); + return codeReviewTasks; + } + + @span() + async processMergeRequestCodeReviewTasks(gitlabProjectId: string | number, mergeRequestIId: number, codeReviewTasks: CodeReviewTask[]): Promise { + const mergeRequest: ExpandedMergeRequestSchema = await this.api().MergeRequests.show(gitlabProjectId, mergeRequestIId); + const codeReviewService = appContext().codeReviewService; + const project = await this.gitlabSCM().getProject(gitlabProjectId); + const projectPath = project.fullPath; + // Load the hashes of the diffs we've already reviewed + const reviewCache: CodeReviewFingerprintCache = await codeReviewService.getMergeRequestReviewCache(gitlabProjectId, mergeRequestIId); + + logger.info(`Reviewing MR "${mergeRequest.title}" in project "${projectPath}" (${mergeRequest.web_url}) with ${codeReviewTasks.length} review tasks`); if (!codeReviewTasks.length) return; // Perform LLM Reviews diff --git a/src/modules/slack/slackConfig.ts b/src/modules/slack/slackConfig.ts index df9db1f0..08de13a9 100644 --- a/src/modules/slack/slackConfig.ts +++ b/src/modules/slack/slackConfig.ts @@ -1,4 +1,4 @@ -import { getSecretEnvVar } from 'src/config/secretConfig'; +import { getSecretEnvVar } from '#config/secretConfig'; import { logger } from '#o11y/logger'; export interface SlackConfig { diff --git a/src/routes/codeTask/createCodeTaskRoute.ts b/src/routes/codeTask/createCodeTaskRoute.ts index b9679ee1..9e7a34d5 100644 --- a/src/routes/codeTask/createCodeTaskRoute.ts +++ b/src/routes/codeTask/createCodeTaskRoute.ts @@ -1,6 +1,7 @@ import type { AppFastifyInstance } from '#app/applicationTypes'; import { CodeTaskServiceImpl } from '#codeTask/codeTaskServiceImpl'; import { sendBadRequest, sendServerError } from '#fastify/responses'; +import { logger } from '#o11y/logger'; import { CODE_TASK_API } from '#shared/codeTask/codeTask.api'; import type { CreateCodeTaskData } from '#shared/codeTask/codeTask.model'; import { currentUser } from '#user/userContext'; @@ -27,9 +28,7 @@ export async function createCodeTaskRoute(fastify: AppFastifyInstance): Promise< if (!effectiveRepositoryPath && repositoryName && (repositorySource === 'github' || repositorySource === 'gitlab')) { effectiveRepositoryPath = repositoryName; - fastify.log.info( - `Code task creation: repositoryFullPath was not provided for source '${repositorySource}', derived from repositoryName '${repositoryName}'.`, - ); + logger.info(`Code task creation: repositoryFullPath was not provided for source '${repositorySource}', derived from repositoryName '${repositoryName}'.`); } if (!effectiveRepositoryPath) { @@ -54,7 +53,7 @@ export async function createCodeTaskRoute(fastify: AppFastifyInstance): Promise< const newCodeTask = await codeTaskService.createCodeTask(userId, createData); return reply.sendJSON(newCodeTask); } catch (error: any) { - fastify.log.error(error, `Error creating Code task for user ${userId}`); + logger.error(error, `Error creating Code task for user ${userId}`); return sendServerError(reply, error.message || 'Failed to create Code task'); } }); diff --git a/src/routes/codeTask/createPresetRoute.ts b/src/routes/codeTask/createPresetRoute.ts index 304f47c0..f13a8291 100644 --- a/src/routes/codeTask/createPresetRoute.ts +++ b/src/routes/codeTask/createPresetRoute.ts @@ -1,6 +1,7 @@ import type { AppFastifyInstance } from '#app/applicationTypes'; import { CodeTaskServiceImpl } from '#codeTask/codeTaskServiceImpl'; import { sendServerError } from '#fastify/responses'; +import { logger } from '#o11y/logger'; import { CODE_TASK_API } from '#shared/codeTask/codeTask.api'; import { currentUser } from '#user/userContext'; import { registerApiRoute } from '../routeUtils'; @@ -15,7 +16,7 @@ export async function createPresetRoute(fastify: AppFastifyInstance): Promise { + // TODO replace with registerApiRoute fastify.get( `${basePath}/calls/agent/:agentId`, { diff --git a/src/routes/webhooks/github/github-routes.ts b/src/routes/webhooks/github/github-routes.ts index 48cf426e..2ee03608 100644 --- a/src/routes/webhooks/github/github-routes.ts +++ b/src/routes/webhooks/github/github-routes.ts @@ -81,7 +81,7 @@ export async function githubRoutes(fastify: AppFastifyInstance): Promise { return reply.code(200).send({ status: 'processed' }); } catch (error) { - fastify.log.error(`Webhook processing failed: ${error.message}`); + logger.error(`Webhook processing failed: ${error.message}`); return reply.code(500).send({ error: 'Processing failed' }); } }, @@ -110,7 +110,7 @@ export async function handleCommentEvent(payload: any, fastify: AppFastifyInstan ); // TODO: Add further AI logic for comment processing here. } else { - fastify.log.info( + logger.info( `GitHub Webhook: Received 'issue_comment' event with action '${payload.action}' for ${commentType} #${issueNumber} in repository '${repositoryFullName}'. Not a 'created' action.`, ); } @@ -118,74 +118,72 @@ export async function handleCommentEvent(payload: any, fastify: AppFastifyInstan async function handleWorkflowRunEvent(payload: WorkflowRunPayload, fastify: AppFastifyInstance) { if (payload.action !== 'completed') { - fastify.log.info(`GitHub Webhook: Received 'workflow_run' event with action '${payload.action}'. Ignoring as it's not 'completed'.`); + logger.info(`GitHub Webhook: Received 'workflow_run' event with action '${payload.action}'. Ignoring as it's not 'completed'.`); return; } const { workflow_run, repository } = payload; const projectPath = repository.full_name; - fastify.log.info( + logger.info( `GitHub Webhook: Workflow run '${workflow_run.name}' (ID: ${workflow_run.id}) in '${projectPath}' completed with conclusion: ${workflow_run.conclusion}.`, ); if (workflow_run.conclusion === 'failure') { - fastify.log.warn(`🔥 Workflow run ${workflow_run.id} failed in '${projectPath}'. Investigating...`); + logger.warn(`🔥 Workflow run ${workflow_run.id} failed in '${projectPath}'. Investigating...`); try { const github = new GitHub(); const jobs = await github.listJobsForWorkflowRun(projectPath, workflow_run.id); const failedJobs = jobs.filter((job) => job.conclusion === 'failure'); if (failedJobs.length === 0) { - fastify.log.info(`No failed jobs found for workflow run ${workflow_run.id}. The failure may be at the workflow level.`); + logger.info(`No failed jobs found for workflow run ${workflow_run.id}. The failure may be at the workflow level.`); return; } - fastify.log.info(`Found ${failedJobs.length} failed jobs for workflow run ${workflow_run.id}:`); + logger.info(`Found ${failedJobs.length} failed jobs for workflow run ${workflow_run.id}:`); for (const job of failedJobs) { - fastify.log.info(`- Job: '${job.name}' (ID: ${job.id})`); + logger.info(`- Job: '${job.name}' (ID: ${job.id})`); const failedSteps = job.steps.filter((step) => step.conclusion === 'failure'); if (failedSteps.length > 0) { const stepNames = failedSteps.map((s) => `'${s.name}'`).join(', '); - fastify.log.info(` - Failed steps: ${stepNames}`); + logger.info(` - Failed steps: ${stepNames}`); } // Fetch and log a snippet of the logs const logs = await github.getJobLogs(projectPath, String(job.id)); - fastify.log.info(` - Fetched ${logs.length} bytes of logs for job ${job.id}.`); + logger.info(` - Fetched ${logs.length} bytes of logs for job ${job.id}.`); } } catch (error) { - fastify.log.error(error, `Error investigating failed workflow run ${workflow_run.id}`); + logger.error(error, `Error investigating failed workflow run ${workflow_run.id}`); } } } async function handleWorkflowJobEvent(payload: WorkflowJobPayload, fastify: AppFastifyInstance) { if (payload.action !== 'completed') { - fastify.log.info(`GitHub Webhook: Received 'workflow_job' event with action '${payload.action}'. Ignoring as it's not 'completed'.`); + logger.info(`GitHub Webhook: Received 'workflow_job' event with action '${payload.action}'. Ignoring as it's not 'completed'.`); return; } const { workflow_job, repository } = payload; const projectPath = repository.full_name; - fastify.log.info( - `GitHub Webhook: Job '${workflow_job.name}' (ID: ${workflow_job.id}) in '${projectPath}' completed with conclusion: ${workflow_job.conclusion}.`, - ); + logger.info(`GitHub Webhook: Job '${workflow_job.name}' (ID: ${workflow_job.id}) in '${projectPath}' completed with conclusion: ${workflow_job.conclusion}.`); if (workflow_job.conclusion === 'failure') { - fastify.log.warn(`🔥 Job '${workflow_job.name}' (ID: ${workflow_job.id}) failed in '${projectPath}'.`); + logger.warn(`🔥 Job '${workflow_job.name}' (ID: ${workflow_job.id}) failed in '${projectPath}'.`); try { const failedSteps = workflow_job.steps.filter((step) => step.conclusion === 'failure'); if (failedSteps.length > 0) { const stepNames = failedSteps.map((s) => `'${s.name}'`).join(', '); - fastify.log.info(` - Failed steps: ${stepNames}`); + logger.info(` - Failed steps: ${stepNames}`); } const github = new GitHub(); const logs = await github.getJobLogs(projectPath, String(workflow_job.id)); - fastify.log.info(` - Fetched ${logs.length} bytes of logs for job ${workflow_job.id}.`); + logger.info(` - Fetched ${logs.length} bytes of logs for job ${workflow_job.id}.`); } catch (error) { - fastify.log.error(error, `Error processing failed job ${workflow_job.id}`); + logger.error(error, `Error processing failed job ${workflow_job.id}`); } } } diff --git a/src/routes/webhooks/gitlab/gitlabJobHandler.ts b/src/routes/webhooks/gitlab/gitlabJobHandler.ts index 8c90d84e..a483c08d 100644 --- a/src/routes/webhooks/gitlab/gitlabJobHandler.ts +++ b/src/routes/webhooks/gitlab/gitlabJobHandler.ts @@ -30,7 +30,7 @@ export async function handleBuildJobEvent(event: WebhookJobEventSchema) { cicdStatsService.saveJobResult(jobInfo).catch((e) => logger.error(e, 'Error saving CICD stats')); } - if (event.build_allow_failure || status === 'success' || status === 'running' || status === 'pending' || status === 'created' || status === 'cancelled') { + if (event.build_allow_failure || status === 'success' || status === 'running' || status === 'pending' || status === 'created' || status === 'canceled') { return; } diff --git a/src/routes/webhooks/gitlab/gitlabRoutes.ts b/src/routes/webhooks/gitlab/gitlabRoutes.ts index b355a456..d586aae3 100644 --- a/src/routes/webhooks/gitlab/gitlabRoutes.ts +++ b/src/routes/webhooks/gitlab/gitlabRoutes.ts @@ -71,9 +71,20 @@ export async function gitlabRoutes(fastify: AppFastifyInstance): Promise { async function handleMergeRequestEvent(event: any) { if (event.object_attributes?.draft) return; - const runAsUser = await getAgentUser(); + // If the MR is approved and there are unchecked checkboxes, then add a comment to the MR to ask for the checkboxes to be checked + // if (event.object_attributes.state === 'approved' && hasUnchecked(event.object_attributes.description)) { + // await new GitLab().addComment(event.project.id, event.object_attributes.iid, 'Please check the task list checkboxes'); + // } - // Code review agent + const mergeRequestId = `project:${event.project.name}, miid:${event.object_attributes.iid}, MR:"${event.object_attributes.title}"`; + const gitlabCodeReview = new GitLabCodeReview(); + + const codeReviewTasks = await gitlabCodeReview.createMergeRequestReviewTasks(event.project.id, event.object_attributes.iid); + + if (!codeReviewTasks.length) return; + + // Start the code review agent + const runAsUser = await getAgentUser(); const config: RunWorkflowConfig = { subtype: 'gitlab-review', @@ -84,17 +95,10 @@ async function handleMergeRequestEvent(event: any) { humanInLoop: envVarHumanInLoopSettings(), }; - // If the MR is approved and there are unchecked checkboxes, then add a comment to the MR to ask for the checkboxes to be checked - // if (event.object_attributes.state === 'approved' && hasUnchecked(event.object_attributes.description)) { - // await new GitLab().addComment(event.project.id, event.object_attributes.iid, 'Please check the checkboxes'); - // } - - const mergeRequestId = `project:${event.project.name}, miid:${event.object_attributes.iid}, MR:"${event.object_attributes.title}"`; - await runWorkflowAgent(config, async (context) => { logger.info(`Agent ${context.agentId} reviewing merge request ${mergeRequestId}`); - return new GitLabCodeReview() - .reviewMergeRequest(event.project.id, event.object_attributes.iid) + return gitlabCodeReview + .processMergeRequestCodeReviewTasks(event.project.id, event.object_attributes.iid, codeReviewTasks) .then(() => { logger.debug(`Competed review of merge request ${mergeRequestId}`); }) diff --git a/tsconfig.json b/tsconfig.json index d0fd723a..21311d8d 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -32,6 +32,7 @@ "#agent/*": ["./src/agent/*"], "#chat/*": ["./src/chat/*"], "#codeTask/*": ["./src/codeTask/*"], + "#config/*": ["./src/config/*"], "#fastify/*": ["./src/fastify/*"], "#firestore/*": ["./src/modules/firestore/*"], "#functionSchema/*": ["./src/functionSchema/*"], diff --git a/tsconfig.native.json b/tsconfig.native.json index c142e194..b81d5360 100644 --- a/tsconfig.native.json +++ b/tsconfig.native.json @@ -33,6 +33,7 @@ "#agent/*": ["./src/agent/*"], "#chat/*": ["./src/chat/*"], "#codeTask/*": ["./src/codeTask/*"], + "#config/*": ["./src/config/*"], "#fastify/*": ["./src/fastify/*"], "#firestore/*": ["./src/modules/firestore/*"], "#functionSchema/*": ["./src/functionSchema/*"], @@ -50,8 +51,7 @@ "#swe/*": ["./src/swe/*"], "#test/*": ["./src/test/*"], "#user/*": ["./src/user/*"], - "#utils/*": ["./src/utils/*"], - "*": ["./*"] + "#utils/*": ["./src/utils/*"] }, "types": ["node", "mocha"] },