diff --git a/src/context/workspace.ts b/src/context/workspace.ts index 7d642a8..3fae313 100644 --- a/src/context/workspace.ts +++ b/src/context/workspace.ts @@ -512,13 +512,10 @@ export const createContextReader = ( * matching or traceRelatedFiles. Budget-tracked like readChangedFiles. * excludePaths carries every path a higher-priority channel already claimed * (changed files, related files, the conventions file when its section - * carries the whole file) so a doc's full text - * is never rendered twice. Skipping on path presence rather than on - * successful inclusion is lossless: this budget is what survives all changed - * files, so a file demoted to diff-only there can never fit here, and an - * unreadable or binary file fails the second read for the same reason it - * failed the first. The one path that could still succeed is a lockfile, - * force-demoted upstream precisely so its full text stays out. */ + * carries the whole file) so a doc's full text is never rendered twice. + * Presence-based exclusion is lossless — every excluded path already has + * its full text in the prompt via a higher-priority channel, so re-reading + * it here would only duplicate content and waste budget. */ const readPriorityDocs = async ({ priorityDocs, budgetTokens, diff --git a/src/openrouter/__tests__/client.test.ts b/src/openrouter/__tests__/client.test.ts index 34306a8..1a6d5c5 100644 --- a/src/openrouter/__tests__/client.test.ts +++ b/src/openrouter/__tests__/client.test.ts @@ -1,5 +1,5 @@ import { readFileSync } from "node:fs" -import { describe, expect, it } from "vitest" +import { describe, expect, it, vi } from "vitest" import { createTestLogger } from "../../__tests__/test-logger.js" import { reviewResponseJsonSchema } from "../../review/finding.js" import { @@ -46,11 +46,11 @@ const makeSdkStub = ({ }) => { const sendCalls: { chatRequest: ChatRequestSubset - options: { timeoutMs?: number } | undefined + options: { signal?: AbortSignal } | undefined }[] = [] const generationCalls: { id: string - options: { timeoutMs?: number } | undefined + options: { signal?: AbortSignal } | undefined }[] = [] const takeNext = ( @@ -133,7 +133,10 @@ describe("requestReview", () => { }, stream: false, }, - options: { timeoutMs: 45_000, retries: { strategy: "none" } }, + options: { + retries: { strategy: "none" }, + signal: expect.any(AbortSignal), + }, }, ]) }) @@ -305,8 +308,8 @@ describe("requestReview", () => { stub.sendCalls.map((sendCall) => sendCall.chatRequest.model), ).toEqual(["openai/gpt-5-mini", "openai/gpt-5-mini"]) expect(stub.sendCalls[0]?.options).toEqual({ - timeoutMs: 45_000, retries: { strategy: "none" }, + signal: expect.any(AbortSignal), }) expect(result.attempts[0]).toEqual({ model: "openai/gpt-5-mini", @@ -328,19 +331,103 @@ describe("requestReview", () => { }) }) + it("passes a live (non-aborted) AbortSignal to the SDK call", async () => { + const stub = makeSdkStub({ + sendResponses: [{ value: acceptedChatResult }], + generationResponses: [{ value: { data: { totalCost: 0.01 } } }], + }) + const { client } = makeClient(stub) + + await client.requestReview(requestParams) + + const signal = stub.sendCalls[0]?.options?.signal + expect(signal).toBeInstanceOf(AbortSignal) + expect(signal?.aborted).toBe(false) + }) + + it("aborts the signal when the request exceeds the configured timeout", async () => { + vi.useFakeTimers() + try { + const stub = makeSdkStub({ sendResponses: [] }) + stub.sdk.chat.send = async (_request, options) => { + stub.sendCalls.push({ ..._request, options }) + return new Promise((_resolve, reject) => { + options?.signal?.addEventListener("abort", () => + reject(Object.assign(new Error("aborted"), { name: "AbortError" })), + ) + }) + } + const logger = createTestLogger() + const client = createOpenRouterClient( + { sdk: stub.sdk, requestTimeoutMs: 45_000, retryDelayMs: 0 }, + logger, + ) + + // Attach the rejection handler BEFORE advancing timers so the + // second attempt's async abort rejection is caught immediately. + const reviewPromise = client.requestReview({ + ...requestParams, + fallbackModel: null, + }) + void reviewPromise.catch(() => undefined) + + // Two attempts × 45s each — advance past both timeouts + await vi.advanceTimersByTimeAsync(90_000) + + await expect(reviewPromise).rejects.toThrow("review request failed") + expect(stub.sendCalls[0]?.options?.signal?.aborted).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it("clears the chat attempt timeout timer after a successful response", async () => { + const stub = makeSdkStub({ + sendResponses: [{ value: acceptedChatResult }], + }) + delete stub.sdk.generations + const { client } = makeClient(stub) + + const clearTimeoutSpy = vi.spyOn(globalThis, "clearTimeout") + try { + await client.requestReview(requestParams) + expect(clearTimeoutSpy).toHaveBeenCalled() + } finally { + clearTimeoutSpy.mockRestore() + } + }) + it("does not sleep after the final attempt when retries are exhausted", async () => { + const retryDelayMs = 50 const stub = makeSdkStub({ sendResponses: [ { error: makeStatusError(429) }, { error: makeStatusError(429) }, ], }) - const { client } = makeClient(stub) + const logger = createTestLogger() + const client = createOpenRouterClient( + { sdk: stub.sdk, requestTimeoutMs: 45_000, retryDelayMs }, + logger, + ) - await expect( - client.requestReview({ ...requestParams, fallbackModel: null }), - ).rejects.toThrow("review request failed") - expect(stub.sendCalls).toHaveLength(2) + const setTimeoutSpy = vi.spyOn(globalThis, "setTimeout") + try { + await expect( + client.requestReview({ ...requestParams, fallbackModel: null }), + ).rejects.toThrow("review request failed") + + // retryDelayMs sleeps happen between retryable failures; the guard + // `attemptNumber <= MAX_ATTEMPTS_PER_MODEL` prevents an extra sleep + // after the final attempt. Only one sleep (between attempts 1 and 2). + const retrySleepCalls = setTimeoutSpy.mock.calls.filter( + (call) => call[1] === retryDelayMs, + ) + expect(retrySleepCalls).toHaveLength(1) + expect(stub.sendCalls).toHaveLength(2) + } finally { + setTimeoutSpy.mockRestore() + } }) it("truncates an error message longer than 200 characters in the attempt summary", async () => { @@ -465,7 +552,10 @@ describe("requestReview", () => { expect(stub.generationCalls).toEqual([ { id: "gen-no-cost", - options: { timeoutMs: 45_000, retries: { strategy: "none" } }, + options: { + retries: { strategy: "none" }, + signal: expect.any(AbortSignal), + }, }, ]) expect(result.attempts[0]?.costUsd).toBe(0.0399) diff --git a/src/openrouter/client.ts b/src/openrouter/client.ts index d42d867..8934396 100644 --- a/src/openrouter/client.ts +++ b/src/openrouter/client.ts @@ -58,8 +58,8 @@ export type OpenRouterLike = { send( request: { chatRequest: ChatRequestSubset }, options?: { - timeoutMs?: number retries?: { strategy: "backoff" | "none" } + signal?: AbortSignal }, ): Promise } @@ -67,8 +67,8 @@ export type OpenRouterLike = { getGeneration( request: { id: string }, options?: { - timeoutMs?: number retries?: { strategy: "backoff" | "none" } + signal?: AbortSignal }, ): Promise } @@ -224,12 +224,19 @@ export const createOpenRouterClient = ( chatRequest: ChatRequestSubset model: string }): Promise => { - const sendResult = await toResult( - sdk.chat.send( - { chatRequest }, - { timeoutMs: requestTimeoutMs, retries: { strategy: "none" } }, - ), - ) + const controller = new AbortController() + const timer = setTimeout(() => controller.abort(), requestTimeoutMs) + let sendResult: { ok: true; value: unknown } | { ok: false; error: unknown } + try { + sendResult = await toResult( + sdk.chat.send( + { chatRequest }, + { retries: { strategy: "none" }, signal: controller.signal }, + ), + ) + } finally { + clearTimeout(timer) + } if (!sendResult.ok) { const statusCode = errorStatusCode(sendResult.error) const abort = statusCode !== undefined && ABORT_STATUSES.has(statusCode) @@ -327,12 +334,19 @@ export const createOpenRouterClient = ( generationId: string, ): Promise => { if (sdk.generations === undefined) return null - const lookup = await toResult( - sdk.generations.getGeneration( - { id: generationId }, - { timeoutMs: requestTimeoutMs, retries: { strategy: "none" } }, - ), - ) + const controller = new AbortController() + const timer = setTimeout(() => controller.abort(), requestTimeoutMs) + let lookup: { ok: true; value: unknown } | { ok: false; error: unknown } + try { + lookup = await toResult( + sdk.generations.getGeneration( + { id: generationId }, + { retries: { strategy: "none" }, signal: controller.signal }, + ), + ) + } finally { + clearTimeout(timer) + } if (!lookup.ok) { logger.warn("generation cost lookup failed", { error: summarizeError(lookup.error),