From 476462afbd583c47e8d4bc7a17ef6e82be9f5b59 Mon Sep 17 00:00:00 2001 From: Micah Zoltu Date: Fri, 12 Jun 2026 05:59:35 +0000 Subject: [PATCH 1/3] Use output validator to retry aggregator on parse failures and GitHub 422s --- AGENTS.md | 2 +- source/agent-loop.mts | 13 ++- source/ai.mts | 58 +++++++----- source/configuration.mts | 2 +- source/github.mts | 6 +- source/orchestrator.mts | 30 +++--- tests/agent-loop.test.mts | 119 ++++++++++++++++++++++- tests/ai.test.mts | 171 +++++++++++++++++++++++---------- tests/github.test.mts | 11 ++- tests/orchestrator.test.mts | 183 +++++++++++++++++++++++++++--------- 10 files changed, 455 insertions(+), 140 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1eb9104..d4cbec5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -208,7 +208,7 @@ Examples: `runAnalysis`, `runOnCommentTrigger`, `runOnLocalDiff`. Pure helper functions contain parsing, validation, formatting, transformation, and decision-making logic. They are directly imported wherever needed and never injected. -Examples: `parseAggregatorOutput`, `buildAgentPrompt`, `formatReviewForConsole`, `isPrFile`. +Examples: `tryParseAggregatorOutput`, `buildAgentPrompt`, `formatReviewForConsole`, `isPrFile`. ### Important Rules diff --git a/source/agent-loop.mts b/source/agent-loop.mts index 5cf0dde..39b9578 100644 --- a/source/agent-loop.mts +++ b/source/agent-loop.mts @@ -1,9 +1,12 @@ -import { completions, type CompletionsMessage, type CompletionsToolCall, type CompletionDelta, type CompletionUsage, type CompletionResult, type CompletionsRequest } from './completions.mts' +import { completions, type CompletionDelta, type CompletionResult, type CompletionsMessage, type CompletionsRequest, type CompletionsToolCall, type CompletionUsage } from './completions.mts' import type { ProviderProfile } from './provider-profiles.mts' import { isArrayOf, isRecord, isString } from './typescript-helpers.mts' export type Fetch = (signal: AbortSignal, body: string, headers?: Record) => Promise +// Return a string to feed it back to the model as a user turn and continue the loop; return null to terminate. +export type OutputValidator = (content: string) => Promise + export interface Tool { readonly name: string readonly description?: string @@ -83,7 +86,7 @@ function toWireTools(tools: readonly Tool[]): CompletionsRequest['tools'] { }) } -export async function* agentLoop(dependencies: { fetch: Fetch }, model: string, messages: readonly CompletionsMessage[], tools: readonly Tool[], profile: ProviderProfile, signal?: AbortSignal, idleTimeoutMilliseconds: number = 300_000): AsyncGenerator { +export async function* agentLoop(dependencies: { fetch: Fetch }, model: string, messages: readonly CompletionsMessage[], tools: readonly Tool[], profile: ProviderProfile, signal?: AbortSignal, outputValidator?: OutputValidator, idleTimeoutMilliseconds: number = 300_000): AsyncGenerator { const toolMap = new Map(tools.map(tool => [tool.name, tool])) const wireTools = toWireTools(tools) const mutableMessages: CompletionsMessage[] = [...messages] @@ -180,6 +183,12 @@ export async function* agentLoop(dependencies: { fetch: Fetch }, model: string, continue } + const feedback = await outputValidator?.(message.content) + if (feedback) { + mutableMessages.push({ role: 'user', content: feedback }) + continue + } + return { finishReason: completionResult.finishReason, usage: completionResult.usage, diff --git a/source/ai.mts b/source/ai.mts index 73ed9d1..b17cae5 100644 --- a/source/ai.mts +++ b/source/ai.mts @@ -1,19 +1,22 @@ +import { agentLoop, type AgentLoopResult, type Fetch, type OutputValidator } from "./agent-loop.mts" import { buildAgentPrompt, type Agent } from "./agents.mts" -import { agentLoop, type Fetch, type AgentLoopResult } from "./agent-loop.mts" -import type { CompletionsMessage } from "./completions.mts" -import type { ProviderProfile } from "./provider-profiles.mts" import type { BaseCommitContext } from "./base-commit.mts" -import type { SpawnGit } from "./diff.mts" +import type { CompletionsMessage } from "./completions.mts" +import type { TryResult } from "./configuration.mts" import type { DebugWriter } from "./debug.mts" +import type { SpawnGit } from "./diff.mts" import type { LineComment } from "./github-types.mts" import { SIDES } from "./github-types.mts" import type { Logger } from "./logger.mts" +import type { ProviderProfile } from "./provider-profiles.mts" import { readReasoningFromDelta } from "./reasoning.mts" import type { AiReviewResult } from "./review.mts" import { createTools } from "./tool-executor.mts" import { createTraceWriter } from "./trace-writer.mts" import { includes, isReadonlyArray, sleepWithSignal } from "./typescript-helpers.mts" +export type AggregatorSubmitResult = { kind: "ok" } | { kind: "retry"; feedback: string } | { kind: "fatal"; message: string } + export interface AiConfiguration { apiUrl: string model: string @@ -72,7 +75,7 @@ export function createFetch(configuration: AiConfiguration): Fetch { } } -async function runAgent(dependencies: { fetch: Fetch; spawnGit: SpawnGit; logger: Logger; debugWriter: DebugWriter }, agent: Agent, baseCommit: string, baseCommitContext: BaseCommitContext, diffText: string, model: string, profile: ProviderProfile, agentInputs?: Map): Promise { +async function runAgent(dependencies: { fetch: Fetch; spawnGit: SpawnGit; logger: Logger; debugWriter: DebugWriter }, agent: Agent, baseCommit: string, baseCommitContext: BaseCommitContext, diffText: string, model: string, profile: ProviderProfile, agentInputs?: Map, outputValidator?: OutputValidator): Promise { dependencies.logger.log(`Building prompt for ${agent.name}`) const promptMessages = buildAgentPrompt(agent, baseCommitContext, diffText, agentInputs) const debugText = promptMessages.map(m => `--- ${m.role} ---\n${m.content}`).join("\n\n") @@ -81,9 +84,9 @@ async function runAgent(dependencies: { fetch: Fetch; spawnGit: SpawnGit; logger const tools = createTools(dependencies, baseCommit) const messages: CompletionsMessage[] = [...promptMessages] - const writer = createTraceWriter(dependencies.debugWriter, agent.name) + const traceWriter = createTraceWriter(dependencies.debugWriter, agent.name) - const generator = agentLoop(dependencies, model, messages, tools, profile, undefined) + const generator = agentLoop(dependencies, model, messages, tools, profile, undefined, outputValidator) let result: AgentLoopResult while (true) { @@ -96,16 +99,16 @@ async function runAgent(dependencies: { fetch: Fetch; spawnGit: SpawnGit; logger const event = iteratorResult.value switch (event.type) { case "delta": - await writer.delta(readReasoningFromDelta(event.delta, profile), event.delta.content ?? undefined) + await traceWriter.delta(readReasoningFromDelta(event.delta, profile), event.delta.content ?? undefined) break case "tool_call": - await writer.toolCall(event.toolCall.function.name, event.toolCall.function.arguments) + await traceWriter.toolCall(event.toolCall.function.name, event.toolCall.function.arguments) break case "tool_result": - await writer.toolResult(event.name, event.result) + await traceWriter.toolResult(event.name, event.result) break case "completion": - await writer.completion(event.finishReason) + await traceWriter.completion(event.finishReason) break } } @@ -143,26 +146,39 @@ function isValidAiReviewResult(data: unknown): data is AiReviewResult { return true } -function parseAggregatorOutput(output: string): AiReviewResult { - const stripped = output.replace(/^```(?:json)?\s*\n?/i, "").replace(/\n?```\s*$/, "") - let parsed: unknown +function tryParseAggregatorOutput(output: string): TryResult { + const stripped = output.trim().replace(/^```(?:json)?\s*\n?/i, "").replace(/\n?```\s*$/, "") + let parsed try { parsed = JSON.parse(stripped) } catch (error) { - const originalMessage = error instanceof Error ? error.message : String(error) - throw new Error(`Failed to parse aggregator output as JSON: ${originalMessage}\nAggregator output:\n${output}`, { cause: error }) + return { ok: false, reason: error instanceof Error ? error.message : typeof error === 'string' ? error : JSON.stringify(error) } + } + if (!isValidAiReviewResult(parsed)) return { ok: false, reason: `Parsed output does not match expected shape:\n${output}` } + return { ok: true, value: parsed } +} + +async function aggregatorOutputValidator(submit: ((result: AiReviewResult) => Promise) | undefined, content: string): Promise { + const parseResult = tryParseAggregatorOutput(content) + if (!parseResult.ok) { + return `Your previous output failed JSON parsing and validation:\n${parseResult.reason}` } - if (!isValidAiReviewResult(parsed)) throw new Error(`Parsed output does not match expected AiReviewResult shape: ${output}`) - return parsed + if (submit === undefined) return null + const outcome = await submit(parseResult.value) + if (outcome.kind === "ok") return null + if (outcome.kind === "retry") return outcome.feedback + throw new Error(outcome.message) } -export async function analyze(dependencies: { fetch: Fetch; spawnGit: SpawnGit; logger: Logger; debugWriter: DebugWriter }, baseCommitContext: BaseCommitContext, diffText: string, agents: Agent[], aggregator: Agent, baseCommit: string, model: string, profile: ProviderProfile): Promise { +export async function analyze(dependencies: { fetch: Fetch; spawnGit: SpawnGit; logger: Logger; debugWriter: DebugWriter }, baseCommitContext: BaseCommitContext, diffText: string, agents: Agent[], aggregator: Agent, baseCommit: string, model: string, profile: ProviderProfile, submit?: (result: AiReviewResult) => Promise): Promise { dependencies.logger.log(`Using agents: ${agents.length > 0 ? agents.map(a => a.name).join(", ") : "Default"}`) const agentOutputs = await runAgents(dependencies, baseCommit, baseCommitContext, diffText, agents, model, profile) - const finalOutput = await runAgent(dependencies, aggregator, baseCommit, baseCommitContext, diffText, model, profile, agentOutputs) + const finalOutput = await runAgent(dependencies, aggregator, baseCommit, baseCommitContext, diffText, model, profile, agentOutputs, content => aggregatorOutputValidator(submit, content)) dependencies.logger.log("Agent analysis complete") - return parseAggregatorOutput(finalOutput) + const result = tryParseAggregatorOutput(finalOutput) + if (!result.ok) throw new Error(result.reason) + return result.value } diff --git a/source/configuration.mts b/source/configuration.mts index 499c630..7f2a1da 100644 --- a/source/configuration.mts +++ b/source/configuration.mts @@ -23,7 +23,7 @@ export interface LocalDiffConfiguration { export type Configuration = CommentTriggerConfiguration | PullRequestConfiguration | LocalDiffConfiguration -type TryResult = { ok: true; value: T } | { ok: false; reason: string } +export type TryResult = { ok: true; value: T } | { ok: false; reason: string } function tryParseGitHubConfiguration(environment: Record): TryResult { const token = environment.GITHUB_TOKEN diff --git a/source/github.mts b/source/github.mts index c449464..fc05232 100644 --- a/source/github.mts +++ b/source/github.mts @@ -75,7 +75,8 @@ export async function fetchPullRequestDiff(dependencies: { githubFetch: GitHubFe return response.text() } -export async function submitReview(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration, review: GitHubReviewPayload): Promise { +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 response = await dependencies.githubFetch(`${apiUrl}/repos/${owner}/${repositoryName}/pulls/${pullRequestNumber}/reviews`, { @@ -86,8 +87,9 @@ export async function submitReview(dependencies: { githubFetch: GitHubFetch }, c if (!response.ok) { const body = await response.text().catch(() => "") - throw new Error(`Failed to submit review: ${response.status} ${response.statusText}${body ? `\n${body}` : ""}`) + return { ok: false, status: response.status, body } } + return { ok: true } } function isValidPrMetadata(data: unknown): data is { base: { sha: string } } { diff --git a/source/orchestrator.mts b/source/orchestrator.mts index f38915d..dcac9b8 100644 --- a/source/orchestrator.mts +++ b/source/orchestrator.mts @@ -1,20 +1,21 @@ +import type { Fetch } from "./agent-loop.mts" import type { AgentNames, AgentReader } from "./agents.mts" import { loadAgents, loadAggregator } from "./agents.mts" import { analyze } from "./ai.mts" -import type { Fetch } from "./agent-loop.mts" -import type { ProviderProfile } from "./provider-profiles.mts" import { getBaseCommitContext } from "./base-commit.mts" import type { CommentTriggerConfiguration, LocalDiffConfiguration, PullRequestConfiguration } from "./configuration.mts" -import { ensureCommitAvailable, generateLocalDiff, validateGitEnvironment } from "./diff.mts" -import type { SpawnGit } from "./diff.mts" import type { DebugWriter } from "./debug.mts" -import { fetchPullRequestDiff, fetchPullRequestBaseCommit, submitReview } from "./github.mts" -import type { GitHubFetch } from "./github.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 { Logger } from "./logger.mts" +import type { ProviderProfile } from "./provider-profiles.mts" import type { AiReviewResult } from "./review.mts" import { buildReviewPayload, formatReviewForConsole } from "./review.mts" import { getAgentsFromComment } from "./trigger.mts" +import type { AggregatorSubmitResult } from "./ai.mts" type OrchestratorDependencies = { spawnGit: SpawnGit @@ -25,12 +26,12 @@ type OrchestratorDependencies = { debugWriter: DebugWriter } -async function runAnalysis(dependencies: OrchestratorDependencies, agentNames: AgentNames, userAgentsDirectory: string, builtinAgentsDirectory: string, diffText: string, baseCommit: string, model: string, profile: ProviderProfile): Promise { +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) await ensureCommitAvailable(dependencies, baseCommit) const baseCommitContext = await getBaseCommitContext(dependencies, baseCommit) - return analyze(dependencies, baseCommitContext, diffText, agents, aggregator, baseCommit, model, profile) + return analyze(dependencies, baseCommitContext, diffText, agents, aggregator, baseCommit, model, profile, submit) } async function submitPrReview(dependencies: OrchestratorDependencies, agentNames: AgentNames, userAgentsDirectory: string, builtinAgentsDirectory: string, githubConfiguration: GitHubConfiguration, model: string, profile: ProviderProfile): Promise { @@ -44,9 +45,16 @@ async function submitPrReview(dependencies: OrchestratorDependencies, agentNames return } - const aiResult = await runAnalysis(dependencies, agentNames, userAgentsDirectory, builtinAgentsDirectory, diffText, baseCommit, model, profile) - const reviewPayload = buildReviewPayload(aiResult) - await submitReview(dependencies, githubConfiguration, reviewPayload) + const submit = async (result: AiReviewResult): Promise => { + const outcome = await submitReview(dependencies, githubConfiguration, buildReviewPayload(result)) + if (outcome.ok) return { kind: "ok" } + if (outcome.status === 422) { + dependencies.logger.log(`GitHub rejected aggregator output with ${outcome.status}, sending feedback to aggregator`) + return { kind: "retry", feedback: `Your previous output failed on submission to GitHub:\nHTTP Status: ${outcome.status}\n${outcome.body}` } + } + return { kind: "fatal", message: `Failed to submit review: ${outcome.status} ${outcome.body}` } + } + await runAnalysis(dependencies, agentNames, userAgentsDirectory, builtinAgentsDirectory, diffText, baseCommit, model, profile, submit) dependencies.logger.log("PR review submitted successfully") } diff --git a/tests/agent-loop.test.mts b/tests/agent-loop.test.mts index 374ead5..a687320 100644 --- a/tests/agent-loop.test.mts +++ b/tests/agent-loop.test.mts @@ -1,5 +1,5 @@ import { describe, it, expect } from "bun:test" -import { agentLoop, type AgentLoopEvent, type AgentLoopResult, type Fetch, type Tool } from "../source/agent-loop.mts" +import { agentLoop, type AgentLoopEvent, type AgentLoopResult, type Fetch, type OutputValidator, type Tool } from "../source/agent-loop.mts" import type { CompletionsMessage } from "../source/completions.mts" import { IDENTITY_PROFILE } from "../source/provider-profiles.mts" @@ -705,12 +705,12 @@ describe("agentLoop", () => { describe("idle timeout", () => { it("throws when no deltas arrive within idle timeout", async () => { const fetch = createHangingFetchWithSignal() - expect(collectLoop(agentLoop({ fetch }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE, undefined, 50))).rejects.toThrow("Agent loop timed out due to inactivity") + expect(collectLoop(agentLoop({ fetch }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE, undefined, undefined, 50))).rejects.toThrow("Agent loop timed out due to inactivity") }) it("includes timeout duration in error message", async () => { const fetch = createHangingFetchWithSignal() - expect(collectLoop(agentLoop({ fetch }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE, undefined, 123))).rejects.toThrow("123 milliseconds") + expect(collectLoop(agentLoop({ fetch }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE, undefined, undefined, 123))).rejects.toThrow("123 milliseconds") }) it("resets idle timer on each delta", async () => { @@ -736,7 +736,7 @@ describe("agentLoop", () => { }) return new Response(stream, { status: 200 }) } - const { result } = await collectLoop(agentLoop({ fetch: fetchWithSignal }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE, undefined, 50)) + const { result } = await collectLoop(agentLoop({ fetch: fetchWithSignal }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE, undefined, undefined, 50)) expect(result.finishReason).toBe("stop") }) }) @@ -1002,6 +1002,117 @@ describe("agentLoop", () => { expect(result.finishReason).toBe("stop") }) }) + + describe("outputValidator callback", () => { + it("terminates normally when callback returns null", async () => { + const outputValidator: OutputValidator = async () => null + const { fetch } = createFetchWithSignal(() => + buildSse([chunk({ content: "done" }), chunk({}, "stop")]), + ) + const { result } = await collectLoop(agentLoop({ fetch }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE, undefined, outputValidator)) + expect(result.finishReason).toBe("stop") + expect(result.messages.at(-1)).toEqual({ role: "assistant", content: "done" }) + }) + + it("continues the loop and feeds back the callback's string as a user turn", async () => { + const responses = [ + buildSse([ + chunk({ content: "first attempt" }), + chunk({}, "stop"), + ]), + buildSse([ + chunk({ content: "second attempt" }), + chunk({}, "stop"), + ]), + ] + const { fetch, callCount } = createFetchWithSignal(({ callIndex }) => responses[callIndex]!) + + const observedContents: string[] = [] + let callCount$ = 0 + const outputValidator: OutputValidator = async (content) => { + observedContents.push(content) + callCount$++ + if (callCount$ === 1) return "your previous output was invalid; please correct it" + return null + } + + const { result } = await collectLoop(agentLoop({ fetch }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE, undefined, outputValidator)) + + expect(observedContents).toEqual(["first attempt", "second attempt"]) + expect(callCount()).toBe(2) + expect(result.finishReason).toBe("stop") + expect(result.messages.at(-1)).toEqual({ role: "assistant", content: "second attempt" }) + expect(result.messages.at(-2)).toEqual({ role: "user", content: "your previous output was invalid; please correct it" }) + expect(result.messages.at(-3)).toEqual({ role: "assistant", content: "first attempt" }) + }) + + it("includes the injected user turn in the next completions request", async () => { + const capturedBodies: string[] = [] + const responses = [ + buildSse([chunk({ content: "first" }), chunk({}, "stop")]), + buildSse([chunk({ content: "second" }), chunk({}, "stop")]), + ] + const fetchWithSignal: Fetch = async (_signal, body, _headers) => { + capturedBodies.push(body) + const callIndex = capturedBodies.length - 1 + const sseText = responses[callIndex] ?? responses[responses.length - 1]! + return createMockFetchResponse(sseText) + } + + let callbackCalls = 0 + const outputValidator: OutputValidator = async () => { + callbackCalls++ + if (callbackCalls === 1) return "feedback message" + return null + } + + await collectLoop(agentLoop({ fetch: fetchWithSignal }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE, undefined, outputValidator)) + + const secondRequest = JSON.parse(capturedBodies[1]!) + expect(secondRequest.messages).toEqual([ + { role: "user", content: "hi" }, + { role: "assistant", content: "first" }, + { role: "user", content: "feedback message" }, + ]) + }) + + it("propagates throws from the callback", async () => { + const outputValidator: OutputValidator = async () => { throw new Error("validator crashed") } + const { fetch } = createFetchWithSignal(() => + buildSse([chunk({ content: "done" }), chunk({}, "stop")]), + ) + expect(collectLoop(agentLoop({ fetch }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE, undefined, outputValidator))).rejects.toThrow("validator crashed") + }) + + it("skips the callback on tool-call turns, invokes it on content turns", async () => { + let callbackCalls = 0 + const outputValidator: OutputValidator = async () => { callbackCalls++; return null } + const responses = [ + buildSse([ + chunk({ + tool_calls: [{ + index: 0, id: "call_1", type: "function", function: { name: "read_file", arguments: '{"path":"a.ts"}' }, + }], + }, "tool_calls"), + ]), + buildSse([chunk({ content: "final" }), chunk({}, "stop")]), + ] + const { fetch } = createFetchWithSignal(({ callIndex }) => responses[callIndex]!) + + await collectLoop(agentLoop({ fetch }, "test-model", [{ role: "user", content: "hi" }], TOOLS, IDENTITY_PROFILE, undefined, outputValidator)) + + expect(callbackCalls).toBe(1) + }) + + it("behaves identically to no-callback when callback is omitted", async () => { + const { fetch } = createFetchWithSignal(() => + buildSse([chunk({ content: "ok" }), chunk({}, "stop")]), + ) + const { result } = await collectLoop(agentLoop({ fetch }, "test-model", [{ role: "user", content: "hi" }], [], IDENTITY_PROFILE)) + expect(result.finishReason).toBe("stop") + expect(result.messages.at(-1)).toEqual({ role: "assistant", content: "ok" }) + }) + }) }) function createMockFetchResponse(sseText: string): Response { diff --git a/tests/ai.test.mts b/tests/ai.test.mts index 89df68c..eeb34fd 100644 --- a/tests/ai.test.mts +++ b/tests/ai.test.mts @@ -20,13 +20,16 @@ const noopDebugWriter: DebugWriter = { writePrompt: async () => {}, writeTrace: const noopSpawnGit: SpawnGit = async () => ({ stdout: "", stderr: "", exitCode: 0, signalCode: null } satisfies GitDiffResult) -function makeFetchWithAggregatorOutput(aggregatorOutput: string): Fetch { - let callCount = 0 - return async (_signal: AbortSignal, _body: string, _headers?: Record) => { - callCount++ - const content = callCount <= 1 +function makeFetchWithAggregatorOutput(aggregatorOutputs: readonly string[]): { fetch: Fetch; capturedBodies: () => readonly string[] } { + const capturedBodies: string[] = [] + const fetch: Fetch = async (_signal: AbortSignal, body: string, _headers?: Record) => { + capturedBodies.push(body) + const callIndex = capturedBodies.length - 1 + const isAgentCall = callIndex === 0 + const aggregatorIndex = callIndex - 1 + const content = isAgentCall ? JSON.stringify({ body: "Agent result", comments: [] }) - : aggregatorOutput + : aggregatorOutputs[Math.min(aggregatorIndex, aggregatorOutputs.length - 1)]! const encoder = new TextEncoder() const sseText = buildContentSse(content) const stream = new ReadableStream({ @@ -37,6 +40,7 @@ function makeFetchWithAggregatorOutput(aggregatorOutput: string): Fetch { }) return new Response(stream, { status: 200 }) } + return { fetch, capturedBodies: () => capturedBodies } } describe("parseAiConfiguration", () => { @@ -148,11 +152,13 @@ describe("analyze", () => { }) describe("aggregator output validation", () => { + const validOutput = JSON.stringify({ + body: "Looks good", + comments: [{ path: "src/file.ts", line: 10, side: "RIGHT", body: "Fix this" }], + }) + it("returns result when aggregator output is valid with comments", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ - body: "Looks good", - comments: [{ path: "src/file.ts", line: 10, side: "RIGHT", body: "Fix this" }], - })) + const { fetch } = makeFetchWithAggregatorOutput([validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } @@ -164,10 +170,10 @@ describe("analyze", () => { }) it("returns result when aggregator output has empty comments", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ + const { fetch } = makeFetchWithAggregatorOutput([JSON.stringify({ body: "No issues found", comments: [], - })) + })]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } @@ -177,104 +183,171 @@ describe("analyze", () => { expect(result.comments).toEqual([]) }) - it("throws Error with aggregator output when output is not valid JSON", async () => { - const fetch = makeFetchWithAggregatorOutput("not json") + it("feeds back the parse error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput(["not json", validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow(/Failed to parse aggregator output as JSON[\s\S]*not json/) + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + expect(capturedBodies().length).toBe(3) + const secondRequest = JSON.parse(capturedBodies()[2]!) + const lastUserMessage = secondRequest.messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Your previous output failed JSON parsing and validation:\nJSON Parse error: Unexpected identifier \"not\"") }) - it("throws when aggregator output does not match expected shape", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ wrong: "shape" })) + it("feeds back the shape error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput([JSON.stringify({ wrong: "shape" }), validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow("Parsed output does not match expected AiReviewResult shape") + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + const secondRequest = JSON.parse(capturedBodies()[2]!) + const lastUserMessage = secondRequest.messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Your previous output failed JSON parsing and validation:\nParsed output does not match expected shape:\n{\"wrong\":\"shape\"}") }) - it("throws when aggregator body is not a string", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ body: 123, comments: [] })) + it("feeds back the body-type error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput([JSON.stringify({ body: 123, comments: [] }), validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow("Parsed output does not match expected AiReviewResult shape") + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + expect(capturedBodies().length).toBe(3) + const lastUserMessage = JSON.parse(capturedBodies()[2]!).messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Parsed output does not match expected shape") }) - it("throws when aggregator comments is not an array", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ body: "test", comments: "not array" })) + it("feeds back the comments-type error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput([JSON.stringify({ body: "test", comments: "not array" }), validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow("Parsed output does not match expected AiReviewResult shape") + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + expect(capturedBodies().length).toBe(3) + const lastUserMessage = JSON.parse(capturedBodies()[2]!).messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Parsed output does not match expected shape") }) - it("throws when aggregator comments is missing", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ body: "test" })) + it("feeds back the missing-comments error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput([JSON.stringify({ body: "test" }), validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow("Parsed output does not match expected AiReviewResult shape") + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + expect(capturedBodies().length).toBe(3) + const lastUserMessage = JSON.parse(capturedBodies()[2]!).messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Parsed output does not match expected shape") }) - it("throws when aggregator body is empty string", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ body: "", comments: [] })) + it("feeds back the empty-body error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput([JSON.stringify({ body: "", comments: [] }), validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow("Parsed output does not match expected AiReviewResult shape") + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + expect(capturedBodies().length).toBe(3) + const lastUserMessage = JSON.parse(capturedBodies()[2]!).messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Parsed output does not match expected shape") }) - it("throws when aggregator body is missing", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ comments: [] })) + it("feeds back the missing-body error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput([JSON.stringify({ comments: [] }), validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow("Parsed output does not match expected AiReviewResult shape") + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + expect(capturedBodies().length).toBe(3) + const lastUserMessage = JSON.parse(capturedBodies()[2]!).messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Parsed output does not match expected shape") }) - it("rejects line number zero in aggregator comments", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ + it("feeds back the zero-line-number error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput([JSON.stringify({ body: "test", comments: [{ path: "src/file.ts", line: 0, side: "RIGHT", body: "comment" }], - })) + }), validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow("Parsed output does not match expected AiReviewResult shape") + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + expect(capturedBodies().length).toBe(3) + const lastUserMessage = JSON.parse(capturedBodies()[2]!).messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Parsed output does not match expected shape") }) - it("rejects negative line number in aggregator comments", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ + it("feeds back the negative-line-number error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput([JSON.stringify({ body: "test", comments: [{ path: "src/file.ts", line: -1, side: "RIGHT", body: "comment" }], - })) + }), validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow("Parsed output does not match expected AiReviewResult shape") + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + expect(capturedBodies().length).toBe(3) + const lastUserMessage = JSON.parse(capturedBodies()[2]!).messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Parsed output does not match expected shape") }) - it("rejects non-integer line number in aggregator comments", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ + it("feeds back the non-integer-line-number error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput([JSON.stringify({ body: "test", comments: [{ path: "src/file.ts", line: 1.5, side: "RIGHT", body: "comment" }], - })) + }), validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow("Parsed output does not match expected AiReviewResult shape") + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + expect(capturedBodies().length).toBe(3) + const lastUserMessage = JSON.parse(capturedBodies()[2]!).messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Parsed output does not match expected shape") }) - it("rejects empty comment body in aggregator comments", async () => { - const fetch = makeFetchWithAggregatorOutput(JSON.stringify({ + it("feeds back the empty-comment-body error and accepts the corrected output", async () => { + const { fetch, capturedBodies } = makeFetchWithAggregatorOutput([JSON.stringify({ body: "test", comments: [{ path: "src/file.ts", line: 1, side: "RIGHT", body: "" }], - })) + }), validOutput]) const agents: Agent[] = [{ name: "TestAgent", prompt: "Test" }] const aggregator: Agent = { name: "Aggregator", prompt: "Aggregate" } - expect(analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE)).rejects.toThrow("Parsed output does not match expected AiReviewResult shape") + const result = await analyze({ fetch, spawnGit: noopSpawnGit, logger: createMockLogger(), debugWriter: noopDebugWriter }, makeBaseCommitContext(), SAMPLE_DIFF, agents, aggregator, "abc123", "test-model", IDENTITY_PROFILE) + + expect(result.body).toBe("Looks good") + expect(capturedBodies().length).toBe(3) + const lastUserMessage = JSON.parse(capturedBodies()[2]!).messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Parsed output does not match expected shape") }) }) }) diff --git a/tests/github.test.mts b/tests/github.test.mts index 8830328..37868f9 100644 --- a/tests/github.test.mts +++ b/tests/github.test.mts @@ -48,15 +48,18 @@ describe("submitReview", () => { const configuration = makeGitHubConfiguration() const review: GitHubReviewPayload = { event: "COMMENT", body: "Great work", comments: [] } - await submitReview({ githubFetch }, configuration, review) + const result = await submitReview({ githubFetch }, configuration, review) + expect(result).toEqual({ ok: true }) }) - it("throws on non-ok response", async () => { - const githubFetch: GitHubFetch = async () => new Response("Error", { status: 422, statusText: "Unprocessable Entity" }) + it("returns a result with status and body on non-2xx response", async () => { + const body = JSON.stringify({ message: "Unprocessable Entity", errors: ["Line could not be resolved"] }) + const githubFetch: GitHubFetch = async () => new Response(body, { status: 422, statusText: "Unprocessable Entity" }) const configuration = makeGitHubConfiguration() const review: GitHubReviewPayload = { event: "COMMENT", body: "Great work", comments: [] } - expect(submitReview({ githubFetch }, configuration, review)).rejects.toThrow("Failed to submit review") + const result = await submitReview({ githubFetch }, configuration, review) + expect(result).toEqual({ ok: false, status: 422, body }) }) }) diff --git a/tests/orchestrator.test.mts b/tests/orchestrator.test.mts index e844c93..bde14e4 100644 --- a/tests/orchestrator.test.mts +++ b/tests/orchestrator.test.mts @@ -39,8 +39,10 @@ function makeSpawnGitWithDiff(diffText: string): SpawnGit { } } -function makeGithubFetch(options: { diffText?: string; baseCommit?: string; diffError?: Error; submitStatus?: number } = {}): GitHubFetch { - return async (url: string, init: RequestInit) => { +function makeGithubFetch(options: { diffText?: string; baseCommit?: string; diffError?: Error; submitStatus?: number; submitResponses?: readonly { status: number; body?: string }[] } = {}): { githubFetch: GitHubFetch; submitCallCount: () => number; submittedReviews: () => readonly GitHubReviewPayload[] } { + 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") { @@ -49,9 +51,20 @@ function makeGithubFetch(options: { diffText?: string; baseCommit?: string; diff return new Response(JSON.stringify({ base: { sha: options.baseCommit ?? "base123" } }), { status: 200 }) } 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") { + reviewBodies.push(JSON.parse(init.body)) + } return new Response(null, { status: 200 }) } const headers = new Headers(init.headers) @@ -60,6 +73,7 @@ function makeGithubFetch(options: { diffText?: string; baseCommit?: string; diff } return new Response(JSON.stringify({ base: { sha: options.baseCommit ?? "base123" } }), { status: 200 }) } + return { githubFetch, submitCallCount: () => submitIndex, submittedReviews: () => reviewBodies } } const SAMPLE_DIFF = [ @@ -103,7 +117,7 @@ describe("runOnCommentTrigger", () => { { spawnGit: makeSpawnGitOk(), readAgents: makeReadAgents([]), - githubFetch: makeGithubFetch({ diffText: "" }), + githubFetch: makeGithubFetch({ diffText: "" }).githubFetch, fetch: makeFetch(""), logger: silentLogger, debugWriter: noopDebugWriter, @@ -119,24 +133,13 @@ describe("runOnCommentTrigger", () => { }) it("submits review when triggered and files exist", async () => { - let submittedReview: GitHubReviewPayload + const { githubFetch, submittedReviews } = makeGithubFetch({ diffText: SAMPLE_DIFF }) await runOnCommentTrigger( { spawnGit: makeSpawnGitOk(), readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), - githubFetch: async (url: string, init: RequestInit) => { - if (url.includes("/reviews")) { - const body = init.body - if (typeof body === "string") submittedReview = JSON.parse(body) - return new Response(null, { status: 200 }) - } - const headers = new Headers(init.headers) - if (headers.get("Accept") === "application/vnd.github.diff") { - return new Response(SAMPLE_DIFF, { status: 200 }) - } - return new Response(JSON.stringify({ base: { sha: "base123" } }), { status: 200 }) - }, + githubFetch, fetch: makeFetch("Good work"), logger: silentLogger, debugWriter: noopDebugWriter, @@ -148,9 +151,9 @@ describe("runOnCommentTrigger", () => { IDENTITY_PROFILE, ) - expect(submittedReview!).toBeDefined() - expect(submittedReview!.event).toBe("COMMENT") - expect(submittedReview!.body).toContain("Good work") + expect(submittedReviews()).toHaveLength(1) + expect(submittedReviews()[0]!.event).toBe("COMMENT") + expect(submittedReviews()[0]!.body).toContain("Good work") }) it("propagates error when readAgents throws", async () => { @@ -160,7 +163,7 @@ describe("runOnCommentTrigger", () => { { spawnGit: makeSpawnGitOk(), readAgents, - githubFetch: makeGithubFetch({ diffText: SAMPLE_DIFF }), + githubFetch: makeGithubFetch({ diffText: SAMPLE_DIFF }).githubFetch, fetch: makeFetch(""), logger: silentLogger, debugWriter: noopDebugWriter, @@ -178,7 +181,7 @@ describe("runOnCommentTrigger", () => { { spawnGit: makeSpawnGitOk(), readAgents: makeReadAgents([]), - githubFetch: makeGithubFetch({ diffError: new Error("API error") }), + githubFetch: makeGithubFetch({ diffError: new Error("API error") }).githubFetch, fetch: makeFetch(""), logger: silentLogger, debugWriter: noopDebugWriter, @@ -191,12 +194,12 @@ describe("runOnCommentTrigger", () => { )).rejects.toThrow("API error") }) - it("propagates error when submitReview throws", async () => { + it("propagates error when submitReview throws with a non-422 status", async () => { expect(runOnCommentTrigger( { spawnGit: makeSpawnGitOk(), readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), - githubFetch: makeGithubFetch({ diffText: SAMPLE_DIFF, submitStatus: 500 }), + githubFetch: makeGithubFetch({ diffText: SAMPLE_DIFF, submitStatus: 500 }).githubFetch, fetch: makeFetch("Good work"), logger: silentLogger, debugWriter: noopDebugWriter, @@ -208,6 +211,88 @@ describe("runOnCommentTrigger", () => { IDENTITY_PROFILE, )).rejects.toThrow("Failed to submit review") }) + + it("retries submit when GitHub returns 422 and accepts the corrected output", async () => { + const capturedBodies: string[] = [] + let fetchCount = 0 + const fetch: Fetch = async (_signal, body, _headers) => { + capturedBodies.push(body) + fetchCount++ + const content = fetchCount === 1 + ? JSON.stringify({ body: "Agent result", comments: [] }) + : (fetchCount === 2 + ? JSON.stringify({ body: "First try", comments: [{ path: "src/file.ts", line: 999, side: "RIGHT", body: "wrong line" }] }) + : JSON.stringify({ body: "Second try", comments: [{ path: "src/file.ts", line: 1, side: "RIGHT", body: "correct line" }] })) + const encoder = new TextEncoder() + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(encoder.encode(buildContentSse(content))) + controller.close() + } + }) + return new Response(stream, { status: 200 }) + } + + const { githubFetch, submitCallCount, submittedReviews } = makeGithubFetch({ + diffText: SAMPLE_DIFF, + submitResponses: [ + { status: 422, body: JSON.stringify({ message: "Unprocessable Entity", errors: ["Line could not be resolved"] }) }, + { status: 200 }, + ], + }) + + await runOnPullRequest( + { + spawnGit: makeSpawnGitOk(), + readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), + githubFetch, + fetch, + logger: silentLogger, + debugWriter: noopDebugWriter, + }, + makePullRequestConfiguration(), + userAgentsDirectory, + builtinAgentsDirectory, + "test-model", + IDENTITY_PROFILE, + ) + + expect(submitCallCount()).toBe(2) + expect(submittedReviews()).toHaveLength(1) + expect(submittedReviews()[0]!.body).toContain("Second try") + expect(submittedReviews()[0]!.comments).toHaveLength(1) + expect(submittedReviews()[0]!.comments[0]!.line).toBe(1) + + const thirdAggregatorRequest = JSON.parse(capturedBodies[2]!) + const lastUserMessage = thirdAggregatorRequest.messages.at(-1) + expect(lastUserMessage.role).toBe("user") + expect(lastUserMessage.content).toContain("Your previous output failed on submission to GitHub:\nHTTP Status: 422\n{\"message\":\"Unprocessable Entity\",\"errors\":[\"Line could not be resolved\"]}") + expect(lastUserMessage.content).toContain("Line could not be resolved") + }) + + it("does not retry on 401/403/404/429 submit errors", async () => { + for (const status of [401, 403, 404, 429]) { + const { githubFetch, submitCallCount } = makeGithubFetch({ diffText: SAMPLE_DIFF, submitStatus: status }) + + await expect(runOnPullRequest( + { + spawnGit: makeSpawnGitOk(), + readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), + githubFetch, + fetch: makeFetch("Good work"), + logger: silentLogger, + debugWriter: noopDebugWriter, + }, + makePullRequestConfiguration(), + userAgentsDirectory, + builtinAgentsDirectory, + "test-model", + IDENTITY_PROFILE, + )).rejects.toThrow() + + expect(submitCallCount()).toBe(1) + } + }) }) describe("runOnPullRequest", () => { @@ -218,7 +303,7 @@ describe("runOnPullRequest", () => { { spawnGit: makeSpawnGitOk(), readAgents: makeReadAgents([]), - githubFetch: makeGithubFetch({ diffText: "" }), + githubFetch: makeGithubFetch({ diffText: "" }).githubFetch, fetch: makeFetch(""), logger: silentLogger, debugWriter: noopDebugWriter, @@ -234,24 +319,13 @@ describe("runOnPullRequest", () => { }) it("submits review after analysis", async () => { - let submittedReview: GitHubReviewPayload + const { githubFetch, submittedReviews } = makeGithubFetch({ diffText: SAMPLE_DIFF }) await runOnPullRequest( { spawnGit: makeSpawnGitOk(), readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), - githubFetch: async (url: string, init: RequestInit) => { - if (url.includes("/reviews")) { - const body = init.body - if (typeof body === "string") submittedReview = JSON.parse(body) - return new Response(null, { status: 200 }) - } - const headers = new Headers(init.headers) - if (headers.get("Accept") === "application/vnd.github.diff") { - return new Response(SAMPLE_DIFF, { status: 200 }) - } - return new Response(JSON.stringify({ base: { sha: "base123" } }), { status: 200 }) - }, + githubFetch, fetch: makeFetch("Great work"), logger: silentLogger, debugWriter: noopDebugWriter, @@ -263,8 +337,8 @@ describe("runOnPullRequest", () => { IDENTITY_PROFILE, ) - expect(submittedReview!).toBeDefined() - expect(submittedReview!.body).toContain("Great work") + expect(submittedReviews()).toHaveLength(1) + expect(submittedReviews()[0]!.body).toContain("Great work") }) it("propagates error when readAgents throws", async () => { @@ -274,7 +348,7 @@ describe("runOnPullRequest", () => { { spawnGit: makeSpawnGitOk(), readAgents, - githubFetch: makeGithubFetch({ diffText: SAMPLE_DIFF }), + githubFetch: makeGithubFetch({ diffText: SAMPLE_DIFF }).githubFetch, fetch: makeFetch(""), logger: silentLogger, debugWriter: noopDebugWriter, @@ -292,7 +366,7 @@ describe("runOnPullRequest", () => { { spawnGit: makeSpawnGitOk(), readAgents: makeReadAgents([]), - githubFetch: makeGithubFetch({ diffError: new Error("API error") }), + githubFetch: makeGithubFetch({ diffError: new Error("API error") }).githubFetch, fetch: makeFetch(""), logger: silentLogger, debugWriter: noopDebugWriter, @@ -399,10 +473,26 @@ describe("runOnLocalDiff", () => { )).rejects.toThrow("Failed to get diff") }) - it("propagates error when fetch returns invalid JSON", async () => { - const fetch = createMockAgentFetch(buildContentSse("not json")) + it("feeds back the parse error and accepts the corrected output in local-diff mode", async () => { + let callCount = 0 + const fetch: Fetch = async (_signal, _body, _headers) => { + callCount++ + const content = callCount === 1 + ? JSON.stringify({ body: "Agent result", comments: [] }) + : (callCount === 2 + ? "not json" + : JSON.stringify({ body: "Looks good", comments: [] })) + const encoder = new TextEncoder() + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(encoder.encode(buildContentSse(content))) + controller.close() + } + }) + return new Response(stream, { status: 200 }) + } - expect(runOnLocalDiff( + const result = await runOnLocalDiff( { spawnGit: makeSpawnGitWithDiff(SAMPLE_DIFF), readAgents: makeReadAgents([makeAgent(), { name: "Aggregator", prompt: "Aggregate." }]), @@ -417,6 +507,9 @@ describe("runOnLocalDiff", () => { "test-model", "/test/workspace", IDENTITY_PROFILE, - )).rejects.toThrow(/Failed to parse aggregator output as JSON/) + ) + + expect(callCount).toBe(3) + expect(result).toContain("Looks good") }) }) From 6ca4032bf121f0fdd66ff5893de8efa7258f79d3 Mon Sep 17 00:00:00 2001 From: Micah Zoltu Date: Fri, 12 Jun 2026 07:43:03 +0000 Subject: [PATCH 2/3] PR Feedback. --- source/ai.mts | 2 +- source/configuration.mts | 3 +-- source/typescript-helpers.mts | 2 ++ 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/source/ai.mts b/source/ai.mts index b17cae5..a0467d8 100644 --- a/source/ai.mts +++ b/source/ai.mts @@ -2,7 +2,7 @@ import { agentLoop, type AgentLoopResult, type Fetch, type OutputValidator } fro import { buildAgentPrompt, type Agent } from "./agents.mts" import type { BaseCommitContext } from "./base-commit.mts" import type { CompletionsMessage } from "./completions.mts" -import type { TryResult } from "./configuration.mts" +import type { TryResult } from "./typescript-helpers.mts" import type { DebugWriter } from "./debug.mts" import type { SpawnGit } from "./diff.mts" import type { LineComment } from "./github-types.mts" diff --git a/source/configuration.mts b/source/configuration.mts index 7f2a1da..c1267ce 100644 --- a/source/configuration.mts +++ b/source/configuration.mts @@ -1,5 +1,6 @@ import type { AgentNames } from "./agents.mts" import type { GitHubConfiguration } from "./github-types.mts" +import type { TryResult } from "./typescript-helpers.mts" import { parseCommaSeparatedList } from "./typescript-helpers.mts" export interface CommentTriggerConfiguration { @@ -23,8 +24,6 @@ export interface LocalDiffConfiguration { export type Configuration = CommentTriggerConfiguration | PullRequestConfiguration | LocalDiffConfiguration -export type TryResult = { ok: true; value: T } | { ok: false; reason: string } - function tryParseGitHubConfiguration(environment: Record): TryResult { const token = environment.GITHUB_TOKEN const pullRequestNumberString = environment.PR_NUMBER diff --git a/source/typescript-helpers.mts b/source/typescript-helpers.mts index 2e5749d..727fad9 100644 --- a/source/typescript-helpers.mts +++ b/source/typescript-helpers.mts @@ -29,6 +29,8 @@ export function isRecord(value: unknown): value is Record { return true } +export type TryResult = { ok: true; value: T } | { ok: false; reason: string } + export type Guard = (value: unknown) => value is T export interface OptionalMarker { __optional: true From debd12217b3b357d4fc43d7dc30145ff86efb620 Mon Sep 17 00:00:00 2001 From: Micah Zoltu Date: Fri, 12 Jun 2026 10:06:13 +0000 Subject: [PATCH 3/3] PR Feedback --- source/agent-loop.mts | 1 + 1 file changed, 1 insertion(+) diff --git a/source/agent-loop.mts b/source/agent-loop.mts index 39b9578..9a9ee18 100644 --- a/source/agent-loop.mts +++ b/source/agent-loop.mts @@ -185,6 +185,7 @@ export async function* agentLoop(dependencies: { fetch: Fetch }, model: string, const feedback = await outputValidator?.(message.content) if (feedback) { + emptyTurnCount = 0 mutableMessages.push({ role: 'user', content: feedback }) continue }