diff --git a/.github/workflows/ci-agent.yml b/.github/workflows/ci-agent.yml index 42aab7d..f20b2e9 100644 --- a/.github/workflows/ci-agent.yml +++ b/.github/workflows/ci-agent.yml @@ -23,8 +23,10 @@ on: permissions: pull-requests: write contents: read + checks: write jobs: + # Magic string: the `ci-agent` job id below must not change as the agent looks for a job with this name to update reviews with result details ci-agent: # if triggered by an issue_comment, the issue must be a Pull Request comment and the commentor must be semi-trusted (not internet rando) if: github.event_name != 'issue_comment' || (github.event.issue.pull_request && contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR", "CONTRIBUTOR"]'), github.event.comment.author_association)) diff --git a/Dockerfile b/Dockerfile index 7c66437..0c3f8ec 100644 --- a/Dockerfile +++ b/Dockerfile @@ -30,8 +30,10 @@ RUN bun run setup COPY agents/ /ci-agent/agents/ COPY source/ /ci-agent/source/ COPY tests/ /ci-agent/tests/ -# copy the dockerfile because we have some test covareg that ensures it stays in-sync with code +# copy the dockerfile because we have test that ensures it stays in-sync with code COPY Dockerfile /ci-agent/Dockerfile +# copy the workflow file because we have a test that ensures it stays in-sync with code +COPY .github/workflows/ci-agent.yml /ci-agent/.github/workflows/ci-agent.yml # Seed a git history so diff tests have commits to work with RUN < Promise +function buildRepoUrl(configuration: GitHubConfiguration, pathSegments: readonly string[], query?: Readonly>): string { + const { apiUrl, owner, repositoryName } = configuration + let url = `${apiUrl}/repos/${owner}/${repositoryName}/${pathSegments.join("/")}` + if (query) { + const queryString = Object.entries(query).map(([key, value]) => `${encodeURIComponent(key)}=${encodeURIComponent(value)}`).join("&") + url += `?${queryString}` + } + return url +} + export function createGithubFetch(logger: Logger): GitHubFetch { return async function githubFetch(url: string, options: RequestInit): Promise { // Retries rate limiting (429) and request timeouts; everything else fails fast. @@ -60,9 +71,10 @@ export function createGithubFetch(logger: Logger): GitHubFetch { } export async function fetchPullRequestDiff(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration): Promise { - const { apiUrl, token, owner, repositoryName, pullRequestNumber } = configuration + const { token } = configuration + const url = buildRepoUrl(configuration, ["pulls", String(configuration.pullRequestNumber)]) - const response = await dependencies.githubFetch(`${apiUrl}/repos/${owner}/${repositoryName}/pulls/${pullRequestNumber}`, { + const response = await dependencies.githubFetch(url, { method: "GET", headers: { Authorization: `token ${token}`, Accept: "application/vnd.github.diff" }, }) @@ -77,9 +89,10 @@ export async function fetchPullRequestDiff(dependencies: { githubFetch: GitHubFe export type SubmitReviewResult = { ok: true } | { ok: false, status: number, body: string } export async function submitReview(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration, review: GitHubReviewPayload): Promise { - const { apiUrl, token, owner, repositoryName, pullRequestNumber } = configuration + const { token } = configuration + const url = buildRepoUrl(configuration, ["pulls", String(configuration.pullRequestNumber), "reviews"]) - const response = await dependencies.githubFetch(`${apiUrl}/repos/${owner}/${repositoryName}/pulls/${pullRequestNumber}/reviews`, { + const response = await dependencies.githubFetch(url, { method: "POST", headers: { Authorization: `token ${token}`, Accept: "application/vnd.github.v3+json", "Content-Type": "application/json" }, body: JSON.stringify(review), @@ -92,29 +105,100 @@ export async function submitReview(dependencies: { githubFetch: GitHubFetch }, c return { ok: true } } -function isValidPrMetadata(data: unknown): data is { base: { sha: string } } { - if (typeof data !== "object") return false - if (data === null) return false - if (!("base" in data) || typeof data.base !== "object" || data.base === null) return false - if (!("sha" in data.base) || typeof data.base.sha !== "string") return false - return true +const isPrRef = guard({ sha: isString }) +const isValidPrMetadata = guard({ + base: isPrRef, + head: isPrRef, +}) + +async function fetchPrShaField(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration, field: "base" | "head"): Promise { + const { token } = configuration + const url = buildRepoUrl(configuration, ["pulls", String(configuration.pullRequestNumber)]) + + const response = await dependencies.githubFetch(url, { + method: "GET", + headers: { Authorization: `token ${token}`, Accept: "application/vnd.github.v3+json" }, + }) + + if (!response.ok) { + const body = await response.text().catch(() => "") + throw new Error(`Failed to fetch PR ${field} commit: ${response.status} ${response.statusText}${body ? `\n${body}` : ""}`) + } + + const data: unknown = await response.json() + if (!isValidPrMetadata(data)) throw new Error(`Invalid PR response: missing ${field}.sha`) + return data[field].sha } export async function fetchPullRequestBaseCommit(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration): Promise { - const { apiUrl, token, owner, repositoryName, pullRequestNumber } = configuration + return fetchPrShaField(dependencies, configuration, "base") +} + +export async function fetchPullRequestHeadSha(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration): Promise { + return fetchPrShaField(dependencies, configuration, "head") +} + +export interface CheckRunOutput { + title: string + summary: string + text?: string +} + +const isValidCheckRunResponse = guard({ id: isInteger }) +const isCheckRunSummary = guard({ id: isInteger, name: isString, status: isString }) +const isValidCheckRunsList = guard({ check_runs: isArrayOf(isCheckRunSummary) }) - const response = await dependencies.githubFetch(`${apiUrl}/repos/${owner}/${repositoryName}/pulls/${pullRequestNumber}`, { +export async function findActiveCheckRunByName(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration, headSha: string, name: string): Promise { + const { token } = configuration + const url = buildRepoUrl(configuration, ["commits", headSha, "check-runs"], { check_name: name }) + + const response = await dependencies.githubFetch(url, { method: "GET", headers: { Authorization: `token ${token}`, Accept: "application/vnd.github.v3+json" }, }) + if (!response.ok) return null + + const data: unknown = await response.json() + if (!isValidCheckRunsList(data)) return null + + const active = data.check_runs.find(run => run.name === name && run.status === "in_progress") + return active?.id ?? null +} + +export async function createCheckRun(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration, headSha: string, name: string, output: CheckRunOutput): Promise { + const { token } = configuration + const url = buildRepoUrl(configuration, ["check-runs"]) + + const response = await dependencies.githubFetch(url, { + method: "POST", + headers: { Authorization: `token ${token}`, Accept: "application/vnd.github.v3+json", "Content-Type": "application/json" }, + body: JSON.stringify({ name, head_sha: headSha, status: "in_progress", output }), + }) + if (!response.ok) { const body = await response.text().catch(() => "") - throw new Error(`Failed to fetch PR base commit: ${response.status} ${response.statusText}${body ? `\n${body}` : ""}`) + throw new Error(`Failed to create check run: ${response.status} ${response.statusText}${body ? `\n${body}` : ""}`) } const data: unknown = await response.json() - if (!isValidPrMetadata(data)) throw new Error("Invalid PR response: missing base.sha") + if (!isValidCheckRunResponse(data)) throw new Error("Invalid check run response: missing id") + + return data.id +} + +export async function updateCheckRun(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration, checkRunId: number, conclusion: "success" | "failure" | "cancelled", output: CheckRunOutput): Promise { + const { token } = configuration + const url = buildRepoUrl(configuration, ["check-runs", String(checkRunId)]) + + const response = await dependencies.githubFetch(url, { + method: "PATCH", + headers: { Authorization: `token ${token}`, Accept: "application/vnd.github.v3+json", "Content-Type": "application/json" }, + body: JSON.stringify({ status: "completed", conclusion, output }), + }) - return data.base.sha + if (!response.ok) { + const body = await response.text().catch(() => "") + throw new Error(`Failed to update check run: ${response.status} ${response.statusText}${body ? `\n${body}` : ""}`) + } } diff --git a/source/orchestrator.mts b/source/orchestrator.mts index dcac9b8..9c35665 100644 --- a/source/orchestrator.mts +++ b/source/orchestrator.mts @@ -8,8 +8,8 @@ import type { DebugWriter } from "./debug.mts" import type { SpawnGit } from "./diff.mts" import { ensureCommitAvailable, generateLocalDiff, validateGitEnvironment } from "./diff.mts" import type { GitHubConfiguration } from "./github-types.mts" -import type { GitHubFetch } from "./github.mts" -import { fetchPullRequestBaseCommit, fetchPullRequestDiff, submitReview } from "./github.mts" +import type { CheckRunOutput, GitHubFetch } from "./github.mts" +import { createCheckRun, fetchPullRequestBaseCommit, fetchPullRequestDiff, fetchPullRequestHeadSha, findActiveCheckRunByName, submitReview, updateCheckRun } from "./github.mts" import type { Logger } from "./logger.mts" import type { ProviderProfile } from "./provider-profiles.mts" import type { AiReviewResult } from "./review.mts" @@ -26,6 +26,48 @@ type OrchestratorDependencies = { debugWriter: DebugWriter } +const COMMENT_REVIEW_CHECK_RUN_NAME = "CI Agent Comment Reviewer" +const PULL_REQUEST_REVIEW_CHECK_RUN_NAME = "CI Agent Pull Request Reviewer" + +// Magic string: must match the `id:` of the job in .github/workflows/ci-agent.yml that creates a check run before invoking CI Agent. The integration test in tests/integration.test.mts enforces this. +export const EXISTING_CHECK_RUN_JOB_NAME = "ci-agent" + +type CheckRunIdSource = (dependencies: OrchestratorDependencies, githubConfiguration: GitHubConfiguration, headSha: string) => Promise + +function extractErrorMessage(error: unknown): string { + if (error instanceof Error) return error.message + return String(error) +} + +function formatCheckRunErrorOutput(error: unknown, name: string): CheckRunOutput { + return { + title: `${name} failed`, + summary: "The CI Agent encountered an error while reviewing this pull request. See details below or check the GitHub Actions logs for more context.", + text: `## Error\n\n> ${extractErrorMessage(error)}`, + } +} + +async function runWithCheckRun(dependencies: OrchestratorDependencies, githubConfiguration: GitHubConfiguration, name: string, getCheckRunId: CheckRunIdSource, operation: () => Promise): Promise { + const headSha = await fetchPullRequestHeadSha(dependencies, githubConfiguration) + const checkRunId = await getCheckRunId(dependencies, githubConfiguration, headSha) + + try { + await operation() + } catch (error) { + await updateCheckRun(dependencies, githubConfiguration, checkRunId, "failure", formatCheckRunErrorOutput(error, name)).catch(reportingError => { + dependencies.logger.log(`Failed to update check run ${checkRunId} with error: ${reportingError}`) + }) + throw error + } + + await updateCheckRun(dependencies, githubConfiguration, checkRunId, "success", { + title: name, + summary: "Review submitted successfully.", + }).catch(reportingError => { + dependencies.logger.log(`Failed to update check run ${checkRunId} with success status: ${reportingError}`) + }) +} + async function runAnalysis(dependencies: OrchestratorDependencies, agentNames: AgentNames, userAgentsDirectory: string, builtinAgentsDirectory: string, diffText: string, baseCommit: string, model: string, profile: ProviderProfile, submit?: (result: AiReviewResult) => Promise): Promise { const agents = await loadAgents(dependencies, userAgentsDirectory, builtinAgentsDirectory, agentNames) const aggregator = await loadAggregator(dependencies, userAgentsDirectory, builtinAgentsDirectory) @@ -66,11 +108,32 @@ export async function runOnCommentTrigger(dependencies: OrchestratorDependencies return } - await submitPrReview(dependencies, triggerResult, userAgentsDirectory, builtinAgentsDirectory, configuration.github, model, profile) + await runWithCheckRun( + dependencies, + configuration.github, + COMMENT_REVIEW_CHECK_RUN_NAME, + async (deps, config, headSha) => createCheckRun(deps, config, headSha, COMMENT_REVIEW_CHECK_RUN_NAME, { + title: COMMENT_REVIEW_CHECK_RUN_NAME, + summary: "Review is in progress.", + }), + () => submitPrReview(dependencies, triggerResult, userAgentsDirectory, builtinAgentsDirectory, configuration.github, model, profile), + ) } export async function runOnPullRequest(dependencies: OrchestratorDependencies, configuration: PullRequestConfiguration, userAgentsDirectory: string, builtinAgentsDirectory: string, model: string, profile: ProviderProfile): Promise { - await submitPrReview(dependencies, configuration.agents, userAgentsDirectory, builtinAgentsDirectory, configuration.github, model, profile) + await runWithCheckRun( + dependencies, + configuration.github, + PULL_REQUEST_REVIEW_CHECK_RUN_NAME, + async (deps, config, headSha) => { + const existingId = await findActiveCheckRunByName(deps, config, headSha, EXISTING_CHECK_RUN_JOB_NAME) + if (existingId === null) { + throw new Error(`No active check run named "${EXISTING_CHECK_RUN_JOB_NAME}" found for head SHA ${headSha}. The workflow must create a check run with this name before invoking CI Agent.`) + } + return existingId + }, + () => submitPrReview(dependencies, configuration.agents, userAgentsDirectory, builtinAgentsDirectory, configuration.github, model, profile), + ) } export async function runOnLocalDiff(dependencies: OrchestratorDependencies, configuration: LocalDiffConfiguration, userAgentsDirectory: string, builtinAgentsDirectory: string, model: string, workspaceDirectory: string, profile: ProviderProfile): Promise { diff --git a/tests/github.test.mts b/tests/github.test.mts index 37868f9..703e551 100644 --- a/tests/github.test.mts +++ b/tests/github.test.mts @@ -1,5 +1,5 @@ import { describe, it, expect } from "bun:test" -import { fetchPullRequestDiff, fetchPullRequestBaseCommit, submitReview, type GitHubFetch } from "../source/github.mts" +import { createCheckRun, fetchPullRequestDiff, fetchPullRequestBaseCommit, fetchPullRequestHeadSha, findActiveCheckRunByName, submitReview, updateCheckRun, type GitHubFetch } from "../source/github.mts" import { makeGitHubConfiguration } from "./helpers.mts" import type { GitHubReviewPayload } from "../source/github-types.mts" @@ -65,7 +65,7 @@ describe("submitReview", () => { describe("fetchPullRequestBaseCommit", () => { it("fetches base commit sha from GitHub API", async () => { - const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({ base: { sha: "abc123def" } }), { status: 200 }) + const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({ base: { sha: "abc123def" }, head: { sha: "def456ghi" } }), { status: 200 }) const configuration = makeGitHubConfiguration() const baseCommit = await fetchPullRequestBaseCommit({ githubFetch }, configuration) @@ -88,9 +88,184 @@ describe("fetchPullRequestBaseCommit", () => { }) it("throws when base.sha is not a string", async () => { - const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({ base: { sha: 123 } }), { status: 200 }) + const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({ base: { sha: 123 }, head: { sha: "def" } }), { status: 200 }) const configuration = makeGitHubConfiguration() expect(fetchPullRequestBaseCommit({ githubFetch }, configuration)).rejects.toThrow("Invalid PR response") }) }) + +describe("fetchPullRequestHeadSha", () => { + it("fetches head commit sha from GitHub API", async () => { + const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({ base: { sha: "abc123def" }, head: { sha: "def456ghi" } }), { status: 200 }) + const configuration = makeGitHubConfiguration() + + const headSha = await fetchPullRequestHeadSha({ githubFetch }, configuration) + + expect(headSha).toBe("def456ghi") + }) + + it("throws on non-ok response", async () => { + const githubFetch: GitHubFetch = async () => new Response("Not found", { status: 404, statusText: "Not Found" }) + const configuration = makeGitHubConfiguration() + + expect(fetchPullRequestHeadSha({ githubFetch }, configuration)).rejects.toThrow("Failed to fetch PR head commit") + }) + + it("throws when response is missing head.sha", async () => { + const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({ base: { sha: "abc" } }), { status: 200 }) + const configuration = makeGitHubConfiguration() + + expect(fetchPullRequestHeadSha({ githubFetch }, configuration)).rejects.toThrow("Invalid PR response") + }) +}) + +describe("createCheckRun", () => { + it("creates a check run and returns its id", async () => { + const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({ id: 12345 }), { status: 201 }) + const configuration = makeGitHubConfiguration() + + const id = await createCheckRun({ githubFetch }, configuration, "headSha123", "Test Check", { + title: "Test", + summary: "Test summary", + }) + + expect(id).toBe(12345) + }) + + it("sends POST to /check-runs with correct payload", async () => { + let capturedUrl = "" + let capturedBody = "" + const githubFetch: GitHubFetch = async (url, options) => { + capturedUrl = url + capturedBody = options.body as string + return new Response(JSON.stringify({ id: 1 }), { status: 201 }) + } + const configuration = makeGitHubConfiguration() + + await createCheckRun({ githubFetch }, configuration, "headSha123", "Test Check", { + title: "Test", + summary: "Test summary", + text: "Detail", + }) + + expect(capturedUrl).toBe("https://api.github.com/repos/owner/repo/check-runs") + const payload = JSON.parse(capturedBody) + expect(payload.name).toBe("Test Check") + expect(payload.head_sha).toBe("headSha123") + expect(payload.status).toBe("in_progress") + expect(payload.output.title).toBe("Test") + expect(payload.output.summary).toBe("Test summary") + expect(payload.output.text).toBe("Detail") + }) + + it("throws on non-ok response", async () => { + const githubFetch: GitHubFetch = async () => new Response("Forbidden", { status: 403, statusText: "Forbidden" }) + const configuration = makeGitHubConfiguration() + + expect(createCheckRun({ githubFetch }, configuration, "headSha123", "Test", { title: "T", summary: "S" })).rejects.toThrow("Failed to create check run") + }) + + it("throws when response is missing id", async () => { + const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({}), { status: 201 }) + const configuration = makeGitHubConfiguration() + + expect(createCheckRun({ githubFetch }, configuration, "headSha123", "Test", { title: "T", summary: "S" })).rejects.toThrow("Invalid check run response") + }) +}) + +describe("updateCheckRun", () => { + it("sends PATCH to /check-runs/{id} with correct payload", async () => { + let capturedUrl = "" + let capturedBody = "" + const githubFetch: GitHubFetch = async (url, options) => { + capturedUrl = url + capturedBody = options.body as string + return new Response(null, { status: 200 }) + } + const configuration = makeGitHubConfiguration() + + await updateCheckRun({ githubFetch }, configuration, 999, "failure", { + title: "Failed", + summary: "It broke", + text: "Stack trace here", + }) + + expect(capturedUrl).toBe("https://api.github.com/repos/owner/repo/check-runs/999") + const payload = JSON.parse(capturedBody) + expect(payload.status).toBe("completed") + expect(payload.conclusion).toBe("failure") + expect(payload.output.title).toBe("Failed") + }) + + it("throws on non-ok response", async () => { + const githubFetch: GitHubFetch = async () => new Response("Not found", { status: 404, statusText: "Not Found" }) + const configuration = makeGitHubConfiguration() + + expect(updateCheckRun({ githubFetch }, configuration, 1, "success", { title: "T", summary: "S" })).rejects.toThrow("Failed to update check run") + }) +}) + +describe("findActiveCheckRunByName", () => { + it("returns the id of the in-progress check run matching the name", async () => { + const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({ + check_runs: [ + { id: 100, name: "ci-agent", status: "completed" }, + { id: 200, name: "ci-agent", status: "in_progress" }, + { id: 300, name: "other-job", status: "in_progress" }, + ], + }), { status: 200 }) + const configuration = makeGitHubConfiguration() + + const id = await findActiveCheckRunByName({ githubFetch }, configuration, "headSha123", "ci-agent") + + expect(id).toBe(200) + }) + + it("returns null when no in-progress check run matches the name", async () => { + const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({ + check_runs: [ + { id: 100, name: "ci-agent", status: "completed" }, + { id: 300, name: "other-job", status: "in_progress" }, + ], + }), { status: 200 }) + const configuration = makeGitHubConfiguration() + + const id = await findActiveCheckRunByName({ githubFetch }, configuration, "headSha123", "ci-agent") + + expect(id).toBeNull() + }) + + it("returns null on non-ok response", async () => { + const githubFetch: GitHubFetch = async () => new Response("Not found", { status: 404, statusText: "Not Found" }) + const configuration = makeGitHubConfiguration() + + const id = await findActiveCheckRunByName({ githubFetch }, configuration, "headSha123", "ci-agent") + + expect(id).toBeNull() + }) + + it("returns null when response has invalid structure", async () => { + const githubFetch: GitHubFetch = async () => new Response(JSON.stringify({ unexpected: "shape" }), { status: 200 }) + const configuration = makeGitHubConfiguration() + + const id = await findActiveCheckRunByName({ githubFetch }, configuration, "headSha123", "ci-agent") + + expect(id).toBeNull() + }) + + it("includes the check name as a query parameter", async () => { + let capturedUrl = "" + const githubFetch: GitHubFetch = async (url) => { + capturedUrl = url + return new Response(JSON.stringify({ check_runs: [] }), { status: 200 }) + } + const configuration = makeGitHubConfiguration() + + await findActiveCheckRunByName({ githubFetch }, configuration, "headSha123", "ci-agent") + + expect(capturedUrl).toContain("/commits/headSha123/check-runs") + expect(capturedUrl).toContain("check_name=ci-agent") + }) +}) + diff --git a/tests/integration.test.mts b/tests/integration.test.mts index 67d9eaf..de1a211 100644 --- a/tests/integration.test.mts +++ b/tests/integration.test.mts @@ -1,6 +1,7 @@ import { describe, it, expect } from "bun:test" import { existsSync, readFileSync } from "node:fs" import { join } from "node:path" +import { EXISTING_CHECK_RUN_JOB_NAME } from "../source/orchestrator.mts" import { BUILTIN_AGENTS_DIRECTORY, DEBUG_DIRECTORY, getWorkspaceDirectory } from "../source/paths.mts" const PROJECT_ROOT = join(import.meta.dir, "..") @@ -23,4 +24,11 @@ describe("integration", () => { expect(dockerfile).toContain(`VOLUME ${DEBUG_DIRECTORY}`) expect(dockerfile).toContain(`WORKDIR ${getWorkspaceDirectory({})}`) }) + + it("workflow job id matches the EXISTING_CHECK_RUN_JOB_NAME constant", () => { + const workflow = readFileSync(join(PROJECT_ROOT, ".github", "workflows", "ci-agent.yml"), "utf-8") + // Match the job id line; the trailing colon disambiguates from comment mentions and the constant name itself. + const jobIdPattern = new RegExp(`^\\s*${EXISTING_CHECK_RUN_JOB_NAME}:\\s*$`, "m") + expect(workflow).toMatch(jobIdPattern) + }) }) diff --git a/tests/orchestrator.test.mts b/tests/orchestrator.test.mts index bde14e4..569e891 100644 --- a/tests/orchestrator.test.mts +++ b/tests/orchestrator.test.mts @@ -39,41 +39,78 @@ function makeSpawnGitWithDiff(diffText: string): SpawnGit { } } -function makeGithubFetch(options: { diffText?: string; baseCommit?: string; diffError?: Error; submitStatus?: number; submitResponses?: readonly { status: number; body?: string }[] } = {}): { githubFetch: GitHubFetch; submitCallCount: () => number; submittedReviews: () => readonly GitHubReviewPayload[] } { +type CheckRunCreated = { name: string; headSha: string; output: { title: string; summary: string; text?: string } } +type CheckRunUpdated = { id: number; conclusion: string; output: { title: string; summary: string; text?: string } } + +function makeGithubFetch(options: { diffText?: string, baseCommit?: string, headCommit?: string, diffError?: Error, submitStatus?: number, submitResponses?: readonly { status: number; body?: string }[], checkRunCreateStatus?: number, checkRunUpdateStatus?: number, checkRunList?: readonly { id: number; name: string; status: string }[] } = {}): { githubFetch: GitHubFetch, submitCallCount: () => number, submittedReviews: () => readonly GitHubReviewPayload[], createdCheckRuns: () => readonly CheckRunCreated[], updatedCheckRuns: () => readonly CheckRunUpdated[] } { let submitIndex = 0 const reviewBodies: GitHubReviewPayload[] = [] - const githubFetch: GitHubFetch = async (url: string, init: RequestInit) => { - if (options.diffError && url.includes("/pulls/") && !url.includes("/reviews")) { - const headers = new Headers(init.headers) - if (headers.get("Accept") === "application/vnd.github.diff") { - throw options.diffError - } - return new Response(JSON.stringify({ base: { sha: options.baseCommit ?? "base123" } }), { status: 200 }) + const createdRuns: CheckRunCreated[] = [] + const updatedRuns: CheckRunUpdated[] = [] + + const handleCheckRunUpdate = (url: string, init: RequestInit): Response => { + const idMatch = /\/check-runs\/(\d+)/.exec(url) + if (idMatch === null) throw new Error(`Invalid PATCH URL for check-runs: ${url}`) + const id = Number.parseInt(idMatch[1]!, 10) + if (options.checkRunUpdateStatus !== undefined && options.checkRunUpdateStatus !== 200) { + return new Response(null, { status: options.checkRunUpdateStatus, statusText: "Error" }) } - if (url.includes("/reviews")) { - submitIndex++ - if (options.submitResponses) { - const response = options.submitResponses[Math.min(submitIndex - 1, options.submitResponses.length - 1)]! - if (response.status === 200 && typeof init.body === "string") { - reviewBodies.push(JSON.parse(init.body)) - } - return new Response(response.body ?? null, { status: response.status }) - } - if (options.submitStatus && options.submitStatus !== 200) { - return new Response("Error", { status: options.submitStatus, statusText: "Error" }) - } - if (typeof init.body === "string") { + const payload = JSON.parse(init.body as string) + updatedRuns.push({ id, conclusion: payload.conclusion, output: payload.output }) + return new Response(null, { status: 200 }) + } + + const handleCheckRunCreate = (init: RequestInit): Response => { + if (options.checkRunCreateStatus !== undefined && options.checkRunCreateStatus !== 201) { + return new Response(null, { status: options.checkRunCreateStatus, statusText: "Error" }) + } + const payload = JSON.parse(init.body as string) + createdRuns.push({ name: payload.name, headSha: payload.head_sha, output: payload.output }) + return new Response(JSON.stringify({ id: createdRuns.length }), { status: 201 }) + } + + const handleCheckRunList = (): Response => { + const runs = options.checkRunList ?? [{ id: 555, name: "ci-agent", status: "in_progress" }] + return new Response(JSON.stringify({ check_runs: runs }), { status: 200 }) + } + + const handlePullRequest = (init: RequestInit): Response => { + const headers = new Headers(init.headers) + const isDiffRequest = headers.get("Accept") === "application/vnd.github.diff" + if (options.diffError && isDiffRequest) throw options.diffError + if (isDiffRequest) return new Response(options.diffText ?? "", { status: 200 }) + return new Response(JSON.stringify({ base: { sha: options.baseCommit ?? "base123" }, head: { sha: options.headCommit ?? "head456" } }), { status: 200 }) + } + + const handleReviews = (init: RequestInit): Response => { + submitIndex++ + if (options.submitResponses) { + const response = options.submitResponses[Math.min(submitIndex - 1, options.submitResponses.length - 1)]! + if (response.status === 200 && typeof init.body === "string") { reviewBodies.push(JSON.parse(init.body)) } - return new Response(null, { status: 200 }) + return new Response(response.body ?? null, { status: response.status }) } - const headers = new Headers(init.headers) - if (headers.get("Accept") === "application/vnd.github.diff") { - return new Response(options.diffText ?? "", { status: 200 }) + if (options.submitStatus !== undefined && options.submitStatus !== 200) { + return new Response("Error", { status: options.submitStatus, statusText: "Error" }) } - return new Response(JSON.stringify({ base: { sha: options.baseCommit ?? "base123" } }), { status: 200 }) + if (typeof init.body === "string") { + reviewBodies.push(JSON.parse(init.body)) + } + return new Response(null, { status: 200 }) + } + + const githubFetch: GitHubFetch = async (url, init) => { + const method = init.method ?? "GET" + if (method === "PATCH" && /\/check-runs\/\d+/.test(url)) return handleCheckRunUpdate(url, init) + if (method === "POST" && /\/reviews/.test(url)) return handleReviews(init) + if (method === "POST" && /\/check-runs$/.test(url)) return handleCheckRunCreate(init) + if (method === "GET" && /\/commits\/[^/]+\/check-runs/.test(url)) return handleCheckRunList() + if (url.includes("/pulls/")) return handlePullRequest(init) + throw new Error(`Unexpected githubFetch call: ${method} ${url}`) } - return { githubFetch, submitCallCount: () => submitIndex, submittedReviews: () => reviewBodies } + + return { githubFetch, submitCallCount: () => submitIndex, submittedReviews: () => reviewBodies, createdCheckRuns: () => createdRuns, updatedCheckRuns: () => updatedRuns } } const SAMPLE_DIFF = [ @@ -156,14 +193,41 @@ describe("runOnCommentTrigger", () => { expect(submittedReviews()[0]!.body).toContain("Good work") }) - it("propagates error when readAgents throws", async () => { + it("creates a check run named 'CI Agent Comment Reviewer' on success and updates it to success", async () => { + const { githubFetch, createdCheckRuns, updatedCheckRuns } = makeGithubFetch({ diffText: SAMPLE_DIFF }) + + await runOnCommentTrigger( + { + spawnGit: makeSpawnGitOk(), + readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), + githubFetch, + fetch: makeFetch("Good work"), + logger: silentLogger, + debugWriter: noopDebugWriter, + }, + makeCommentTriggerConfiguration(), + userAgentsDirectory, + builtinAgentsDirectory, + "test-model", + IDENTITY_PROFILE, + ) + + expect(createdCheckRuns()).toHaveLength(1) + expect(createdCheckRuns()[0]!.name).toBe("CI Agent Comment Reviewer") + expect(createdCheckRuns()[0]!.headSha).toBe("head456") + expect(updatedCheckRuns()).toHaveLength(1) + expect(updatedCheckRuns()[0]!.conclusion).toBe("success") + }) + + it("updates check run with failure output and re-throws when the review fails", async () => { const readAgents: AgentReader = async () => { throw new Error("Read failure") } + const { githubFetch, createdCheckRuns, updatedCheckRuns } = makeGithubFetch({ diffText: SAMPLE_DIFF }) - expect(runOnCommentTrigger( + await expect(runOnCommentTrigger( { spawnGit: makeSpawnGitOk(), readAgents, - githubFetch: makeGithubFetch({ diffText: SAMPLE_DIFF }).githubFetch, + githubFetch, fetch: makeFetch(""), logger: silentLogger, debugWriter: noopDebugWriter, @@ -174,6 +238,39 @@ describe("runOnCommentTrigger", () => { "test-model", IDENTITY_PROFILE, )).rejects.toThrow("Read failure") + + expect(createdCheckRuns()).toHaveLength(1) + expect(updatedCheckRuns()).toHaveLength(1) + expect(updatedCheckRuns()[0]!.conclusion).toBe("failure") + expect(updatedCheckRuns()[0]!.output.title).toBe("CI Agent Comment Reviewer failed") + expect(updatedCheckRuns()[0]!.output.text).toContain("Read failure") + }) + + it("propagates the createCheckRun error when check run creation fails", async () => { + const { githubFetch, createdCheckRuns, updatedCheckRuns, submittedReviews } = makeGithubFetch({ + diffText: SAMPLE_DIFF, + checkRunCreateStatus: 500, + }) + + await expect(runOnCommentTrigger( + { + spawnGit: makeSpawnGitOk(), + readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), + githubFetch, + fetch: makeFetch("Good work"), + logger: silentLogger, + debugWriter: noopDebugWriter, + }, + makeCommentTriggerConfiguration(), + userAgentsDirectory, + builtinAgentsDirectory, + "test-model", + IDENTITY_PROFILE, + )).rejects.toThrow("Failed to create check run") + + expect(createdCheckRuns()).toHaveLength(0) + expect(updatedCheckRuns()).toHaveLength(0) + expect(submittedReviews()).toHaveLength(0) }) it("propagates error when fetchPullRequestDiff throws", async () => { @@ -212,6 +309,32 @@ describe("runOnCommentTrigger", () => { )).rejects.toThrow("Failed to submit review") }) + it("does not throw when updateCheckRun fails on the success path", async () => { + const { githubFetch, updatedCheckRuns, submittedReviews } = makeGithubFetch({ + diffText: SAMPLE_DIFF, + checkRunUpdateStatus: 500, + }) + + await runOnCommentTrigger( + { + spawnGit: makeSpawnGitOk(), + readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), + githubFetch, + fetch: makeFetch("Good work"), + logger: silentLogger, + debugWriter: noopDebugWriter, + }, + makeCommentTriggerConfiguration(), + userAgentsDirectory, + builtinAgentsDirectory, + "test-model", + IDENTITY_PROFILE, + ) + + expect(submittedReviews()).toHaveLength(1) + expect(updatedCheckRuns()).toHaveLength(0) + }) + it("retries submit when GitHub returns 422 and accepts the corrected output", async () => { const capturedBodies: string[] = [] let fetchCount = 0 @@ -341,14 +464,47 @@ describe("runOnPullRequest", () => { expect(submittedReviews()[0]!.body).toContain("Great work") }) - it("propagates error when readAgents throws", async () => { + it("finds the existing 'ci-agent' check run and updates it to success", async () => { + const { githubFetch, createdCheckRuns, updatedCheckRuns } = makeGithubFetch({ + diffText: SAMPLE_DIFF, + checkRunList: [{ id: 555, name: "ci-agent", status: "in_progress" }], + }) + + await runOnPullRequest( + { + spawnGit: makeSpawnGitOk(), + readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), + githubFetch, + fetch: makeFetch("Great work"), + logger: silentLogger, + debugWriter: noopDebugWriter, + }, + makePullRequestConfiguration(), + userAgentsDirectory, + builtinAgentsDirectory, + "test-model", + IDENTITY_PROFILE, + ) + + expect(createdCheckRuns()).toHaveLength(0) + expect(updatedCheckRuns()).toHaveLength(1) + expect(updatedCheckRuns()[0]!.id).toBe(555) + expect(updatedCheckRuns()[0]!.conclusion).toBe("success") + expect(updatedCheckRuns()[0]!.output.title).toBe("CI Agent Pull Request Reviewer") + }) + + it("updates existing check run with failure output and re-throws on error", async () => { const readAgents: AgentReader = async () => { throw new Error("Read failure") } + const { githubFetch, createdCheckRuns, updatedCheckRuns } = makeGithubFetch({ + diffText: SAMPLE_DIFF, + checkRunList: [{ id: 555, name: "ci-agent", status: "in_progress" }], + }) - expect(runOnPullRequest( + await expect(runOnPullRequest( { spawnGit: makeSpawnGitOk(), readAgents, - githubFetch: makeGithubFetch({ diffText: SAMPLE_DIFF }).githubFetch, + githubFetch, fetch: makeFetch(""), logger: silentLogger, debugWriter: noopDebugWriter, @@ -359,6 +515,39 @@ describe("runOnPullRequest", () => { "test-model", IDENTITY_PROFILE, )).rejects.toThrow("Read failure") + + expect(createdCheckRuns()).toHaveLength(0) + expect(updatedCheckRuns()).toHaveLength(1) + expect(updatedCheckRuns()[0]!.id).toBe(555) + expect(updatedCheckRuns()[0]!.conclusion).toBe("failure") + expect(updatedCheckRuns()[0]!.output.text).toContain("Read failure") + }) + + it("throws when no existing 'ci-agent' check run is found", async () => { + const { githubFetch, createdCheckRuns, updatedCheckRuns, submittedReviews } = makeGithubFetch({ + diffText: SAMPLE_DIFF, + checkRunList: [{ id: 555, name: "other-job", status: "in_progress" }], + }) + + await expect(runOnPullRequest( + { + spawnGit: makeSpawnGitOk(), + readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), + githubFetch, + fetch: makeFetch("Great work"), + logger: silentLogger, + debugWriter: noopDebugWriter, + }, + makePullRequestConfiguration(), + userAgentsDirectory, + builtinAgentsDirectory, + "test-model", + IDENTITY_PROFILE, + )).rejects.toThrow(`No active check run named "ci-agent" found for head SHA head456`) + + expect(createdCheckRuns()).toHaveLength(0) + expect(updatedCheckRuns()).toHaveLength(0) + expect(submittedReviews()).toHaveLength(0) }) it("propagates error when fetchPullRequestDiff throws", async () => {