Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 168 additions & 16 deletions src/__tests__/orchestrate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type { ContextReader } from "../context/workspace.js"
import {
ReviewRequestError,
createOpenRouterClient,
type ChatRequestSubset,
type ModelAttempt,
type OpenRouterClient,
type StructuredReviewResult,
Expand Down Expand Up @@ -3939,14 +3940,11 @@ describe("staged phases", () => {
choices: [{ message: { content: JSON.stringify(fixtureReviewResponse) } }],
usage: { promptTokens: 10, completionTokens: 20 },
}))
const client = createOpenRouterClient(
{
sdk: { chat: { send }, generations: { getGeneration } },
requestTimeoutMs: 900_000,
remainingReviewMs,
},
createTestLogger(),
)
const client = createOpenRouterClient({
sdk: { chat: { send }, generations: { getGeneration } },
requestTimeoutMs: 900_000,
remainingReviewMs,
})
const stubs = makeOrchestrateDeps({
remainingReviewMs,
generateFindings: createPromptedGenerateFindings(
Expand Down Expand Up @@ -4033,14 +4031,11 @@ describe("staged phases", () => {
.mockResolvedValueOnce(response)
.mockResolvedValueOnce(response)
.mockImplementation(() => lateResponse.promise)
const client = createOpenRouterClient(
{
sdk: { chat: { send } },
requestTimeoutMs: 900_000,
remainingReviewMs,
},
createTestLogger(),
)
const client = createOpenRouterClient({
sdk: { chat: { send } },
requestTimeoutMs: 900_000,
remainingReviewMs,
})
const stubs = makeOrchestrateDeps({
config: { phases: "parallel" },
remainingReviewMs,
Expand Down Expand Up @@ -4119,6 +4114,90 @@ describe("staged phases", () => {
}
})

it("tags each parallel phase's client log lines with that phase", async () => {
const response = {
id: "completed",
model: "test/model",
choices: [{ message: { content: JSON.stringify(fixtureReviewResponse) } }],
usage: { promptTokens: 10, completionTokens: 20, cost: 0.01 },
}

// correctness-security is dispatched first, so a logger shared across the
// concurrent calls would already carry a later phase when its failure logs.
// The failure is keyed on that phase's prompt text, not on call order
const correctnessSecurityPassScope = "this pass covers correctness & security"
const failedPrompts = new Set<string>()
const send = vi.fn(async ({ chatRequest }: { chatRequest: ChatRequestSubset }) => {
const systemPrompt = first(chatRequest.messages).content
const isFirstCorrectnessSecurityRequest =
systemPrompt.includes(correctnessSecurityPassScope) && !failedPrompts.has(systemPrompt)

if (isFirstCorrectnessSecurityRequest) {
failedPrompts.add(systemPrompt)
throw Object.assign(new Error("HTTP 500"), { statusCode: 500 })
}
return response
})
const client = createOpenRouterClient({
sdk: { chat: { send } },
requestTimeoutMs: 900_000,
remainingReviewMs: () => Infinity,
retryDelayMs: 0,
})
const generateLogger = createTestLogger()
const stubs = makeOrchestrateDeps({
config: { phases: "parallel" },
generateFindings: createPromptedGenerateFindings(
{ openrouterClient: client, model: "test/model", fallbackModel: null },
generateLogger,
),
})

const result = await orchestrate(stubs.deps, createTestLogger())

// Completion order depends on scheduling; which phase each line names is
// what this test checks, so the accepted lines are compared by phase
const acceptedByPhase = logsWithMessage(generateLogger, "review response accepted").toSorted(
(left, right) => String(left.data.phase).localeCompare(String(right.data.phase)),
)
const acceptedEntry = (phase: string, attemptCount: number) => ({
level: "info",
message: "review response accepted",
data: {
phase,
model: "test/model",
routedModel: "test/model",
generationId: "completed",
attemptCount,
},
})

expect(result.phases).toEqual([
{ phase: "correctness-security", status: "completed" },
{ phase: "conventions-tests", status: "completed" },
{ phase: "subtle-bugs", status: "completed" },
])
expect(send).toHaveBeenCalledTimes(4)
expect(logsWithMessage(generateLogger, "review attempt failed")).toEqual([
{
level: "warn",
message: "review attempt failed",
data: {
phase: "correctness-security",
model: "test/model",
attemptNumber: 1,
outcome: "api_error",
errorSummary: "HTTP 500: HTTP 500",
},
},
])
expect(acceptedByPhase).toEqual([
acceptedEntry("conventions-tests", 1),
acceptedEntry("correctness-security", 2),
acceptedEntry("subtle-bugs", 1),
])
})

it("posts the surviving phases' findings when one phase fails, naming the gap on the status comment and the check run", async () => {
const timeoutAttempt: ModelAttempt = {
model: "test/model",
Expand Down Expand Up @@ -4550,6 +4629,79 @@ describe("createPromptedGenerateFindings", () => {
).toEqual([{ model: "test/primary", fallbackModel: "test/fallback" }])
})

it("passes requestReview a logger tagged with the review phase", async () => {
const stubClient: OpenRouterClient = {
requestReview: async (_params, logger) => {
logger.info("stub client line")
return { review: { analysis: "", findings: [] }, modelUsed: "m", attempts: [] }
},
}
const logger = createTestLogger()
const generate = createPromptedGenerateFindings(
{ openrouterClient: stubClient, model: "m", fallbackModel: null },
logger,
)
const reviewContext: Omit<ReviewContext, "phase"> = {
prContext: fixturePrContext,
conventions: null,
conventionsFile: "AGENTS.md",
conventionsBudgetTokens: 8_000,
changedFiles: [],
relatedFiles: [],
relatedDocs: [],
annotatedDiff: annotateDiff(parseDiff(sampleDiff)),
priorFindings: [],
priorBotComments: [],
}

await generate({ ...reviewContext, phase: COMBINED_PHASE })
await generate({ ...reviewContext, phase: SUBTLE_BUGS_PHASE })

expect(logsWithMessage(logger, "stub client line")).toEqual([
{ level: "info", message: "stub client line", data: { phase: "combined" } },
{ level: "info", message: "stub client line", data: { phase: "subtle-bugs" } },
])
})

it("logs the review request with its phase and ladder models", async () => {
const stubClient: OpenRouterClient = {
requestReview: async () => {
return { review: { analysis: "", findings: [] }, modelUsed: "test/primary", attempts: [] }
},
}
const logger = createTestLogger()
const generate = createPromptedGenerateFindings(
{ openrouterClient: stubClient, model: "test/primary", fallbackModel: "test/fallback" },
logger,
)

await generate({
prContext: fixturePrContext,
phase: SUBTLE_BUGS_PHASE,
conventions: null,
conventionsFile: "AGENTS.md",
conventionsBudgetTokens: 8_000,
changedFiles: [],
relatedFiles: [],
relatedDocs: [],
annotatedDiff: annotateDiff(parseDiff(sampleDiff)),
priorFindings: [],
priorBotComments: [],
})

expect(logsWithMessage(logger, "requesting review")).toEqual([
{
level: "info",
message: "requesting review",
data: {
phase: "subtle-bugs",
model: "test/primary",
fallbackModel: "test/fallback",
},
},
])
})

it("includes annotated diff in the user prompt", async () => {
const requestReviewCalls: RequestReviewParams[] = []
const stubClient: OpenRouterClient = {
Expand Down
15 changes: 7 additions & 8 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,13 @@ try {
),
generateFindings: createPromptedGenerateFindings(
{
openrouterClient: createOpenRouterClient(
{
sdk: new OpenRouter({ apiKey: config.openrouterApiKey }),
requestTimeoutMs: config.requestTimeoutSeconds * 1000,
remainingReviewMs,
},
logger,
),
// createOpenRouterClient takes no logger because each review phase
// passes its own phase-tagged logger to requestReview
openrouterClient: createOpenRouterClient({
sdk: new OpenRouter({ apiKey: config.openrouterApiKey }),
requestTimeoutMs: config.requestTimeoutSeconds * 1000,
remainingReviewMs,
}),
model: config.model,
fallbackModel: config.fallbackModel === "" ? null : config.fallbackModel,
},
Expand Down
Loading
Loading