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
9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,15 @@ It must support OpenAI-style function tools. Tool calling is required: an
endpoint that rejects tools fails clearly rather than producing a diff-only
review that looks complete.

Each logical non-streaming model call has one fixed ten-minute deadline shared
across all attempts. A timeout or abort is terminal and is never duplicated;
fast connection failures, 429s and 5xx responses retry only while their request
and backoff fit inside the remaining budget. Consumer workflow jobs must leave
enough time for the complete multi-call review, and proxies must not impose a
shorter upstream deadline. The larger bound is intentional for high-reasoning
models; the shared deadline and workflow timeout remain the outer backstops for
a stalled provider.

Structured Outputs are used when supported. Endpoints vary on
`response_format`, `max_tokens` and `temperature`, so commitreview adapts those
optional parameters when an endpoint explicitly rejects them. Tool calling is
Expand Down
2 changes: 1 addition & 1 deletion src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ const LIMITS = {
minSeverity: 'low',
temperature: 0.1,
maxOutputTokens: 16000,
requestTimeoutMs: 180000,
requestTimeoutMs: 600000,
};

export function readConfig() {
Expand Down
46 changes: 40 additions & 6 deletions src/llm.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,14 @@
import * as core from './core.js';

export class LLM {
constructor(config) {
constructor(config, runtime = {}) {
this.config = config;
this.runtime = {
fetch: runtime.fetch ?? ((input, init) => globalThis.fetch(input, init)),
now: runtime.now ?? (() => Date.now()),
sleep: runtime.sleep ?? core.sleep,
timeoutSignal: runtime.timeoutSignal ?? ((ms) => AbortSignal.timeout(ms)),
};
// Identifies this client in findings and in the summary footer.
this.label = config.label || config.model;
this.quirks = {
Expand Down Expand Up @@ -70,32 +76,40 @@ export class LLM {
*/
async send(messages, options = {}) {
const url = `${this.config.baseUrl}/chat/completions`;
const deadline = this.runtime.now() + this.config.requestTimeoutMs;
let attempt = 0;
let networkRetries = 0;

for (;;) {
const remainingMs = deadline - this.runtime.now();
if (remainingMs <= 0) throw requestDeadlineError(this.config.requestTimeoutMs);
const builtAt = this.quirksVersion;
const body = this.buildBody(messages, options);
let res;
try {
res = await fetch(url, {
res = await this.runtime.fetch(url, {
method: 'POST',
headers: {
authorization: `Bearer ${this.config.apiKey}`,
'content-type': 'application/json',
'user-agent': 'commitreview',
},
body: JSON.stringify(body),
signal: AbortSignal.timeout(this.config.requestTimeoutMs),
signal: this.runtime.timeoutSignal(remainingMs),
});
} catch (err) {
if (networkRetries++ >= 3) throw new Error(`Model request failed: ${err.message}`, { cause: err });
await core.sleep(core.backoff(networkRetries));
if (isRequestAbort(err) || this.runtime.now() >= deadline) {
throw requestDeadlineError(this.config.requestTimeoutMs, err);
}
if (networkRetries++ >= 3) throw new Error(`Model request failed: ${errorMessage(err)}`, { cause: err });
await this.waitForRetry(core.backoff(networkRetries), deadline, err);
continue;
}

if (this.runtime.now() >= deadline) throw requestDeadlineError(this.config.requestTimeoutMs);
if (res.ok) {
const data = /** @type {any} */ (await res.json());
if (this.runtime.now() >= deadline) throw requestDeadlineError(this.config.requestTimeoutMs);
this.usage.requests++;
this.usage.prompt += data?.usage?.prompt_tokens || 0;
this.usage.completion += data?.usage?.completion_tokens || 0;
Expand All @@ -112,8 +126,9 @@ export class LLM {
if (res.status === 429 || res.status >= 500) {
if (networkRetries++ >= 3) throw new Error(`Model request failed: ${res.status} ${truncate(text)}`);
const after = Number(res.headers.get('retry-after'));
await core.sleep(
await this.waitForRetry(
Number.isFinite(after) && after > 0 ? Math.min(after, 60) * 1000 : core.backoff(networkRetries),
deadline,
);
continue;
}
Expand Down Expand Up @@ -147,6 +162,13 @@ export class LLM {
}
}

async waitForRetry(delayMs, deadline, cause) {
const remainingMs = deadline - this.runtime.now();
if (remainingMs <= delayMs) throw requestDeadlineError(this.config.requestTimeoutMs, cause);
await this.runtime.sleep(delayMs);
if (this.runtime.now() >= deadline) throw requestDeadlineError(this.config.requestTimeoutMs, cause);
}

/** Drop or rename whatever the endpoint just complained about. @returns {boolean} changed */
adapt(errorText) {
const changed = this.#adapt(errorText || '');
Expand Down Expand Up @@ -226,6 +248,18 @@ export class LLM {
}
}

function isRequestAbort(error) {
return error instanceof Error && (error.name === 'AbortError' || error.name === 'TimeoutError');
}

function errorMessage(error) {
return error instanceof Error ? error.message : String(error);
}

function requestDeadlineError(timeoutMs, cause) {
return new Error(`Model request exceeded its ${timeoutMs}ms logical deadline.`, { cause });
}

/**
* Pull a JSON value out of arbitrary model text. Returns null when there is none.
*
Expand Down
1 change: 1 addition & 0 deletions test/config.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ test('configuration has six public inputs and no URL fallback', () => {
assert.equal(config.baseUrl, 'https://models.example/v1');
assert.equal(config.githubApiUrl, 'https://github.example/api/v3');
assert.equal(config.instructions, 'Use integer pence.');
assert.equal(config.requestTimeoutMs, 600000);
assert.ok(config.ignore.includes('private/**'));

const withoutBase = { ...values };
Expand Down
97 changes: 97 additions & 0 deletions test/llm.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,103 @@ test('request body carries the tunables the endpoint is expected to support', ()
assert.deepEqual(body.response_format, { type: 'json_object' });
});

test('a timed-out logical request is not duplicated', async () => {
let now = 0;
let requests = 0;
const timeout = new Error('request timed out');
timeout.name = 'TimeoutError';
const llm = new LLM(
{ ...base, requestTimeoutMs: 100 },
{
fetch: async () => {
requests++;
now = 100;
throw timeout;
},
now: () => now,
sleep: async () => assert.fail('a timed-out request must not sleep or retry'),
timeoutSignal: () => new AbortController().signal,
},
);

await assert.rejects(() => llm.send([{ role: 'user', content: 'hi' }]), /100ms logical deadline/);
assert.equal(requests, 1);
});

test('a transient network failure retries within the shared deadline', async () => {
let now = 0;
let requests = 0;
const timeoutBudgets = [];
const llm = new LLM(
{ ...base, requestTimeoutMs: 10_000 },
{
fetch: async () => {
requests++;
if (requests === 1) throw new Error('connection reset');
return Response.json({ choices: [{ message: { content: 'ok' } }] });
},
now: () => now,
sleep: async (ms) => {
now += ms;
},
timeoutSignal: (remainingMs) => {
timeoutBudgets.push(remainingMs);
return new AbortController().signal;
},
},
);

assert.deepEqual(await llm.send([{ role: 'user', content: 'hi' }]), {
message: { content: 'ok' },
});
assert.equal(requests, 2);
assert.ok(now > 0 && now < 10_000);
assert.equal(timeoutBudgets.length, 2);
assert.equal(timeoutBudgets[0], 10_000);
assert.ok(timeoutBudgets[1] < timeoutBudgets[0]);
});

test('response decoding must finish inside the shared deadline', async () => {
let now = 0;
const llm = new LLM(
{ ...base, requestTimeoutMs: 100 },
{
fetch: async () => ({
ok: true,
json: async () => {
now = 100;
return { choices: [{ message: { content: 'too late' } }] };
},
}),
now: () => now,
timeoutSignal: () => new AbortController().signal,
},
);

await assert.rejects(() => llm.send([{ role: 'user', content: 'hi' }]), /100ms logical deadline/);
});

test('a retry delay that exceeds the shared deadline fails closed', async () => {
let now = 0;
let requests = 0;
const llm = new LLM(
{ ...base, requestTimeoutMs: 100 },
{
fetch: async () => {
requests++;
now = 99;
return new Response('busy', { status: 503 });
},
now: () => now,
sleep: async () => assert.fail('the retry cannot fit inside the deadline'),
timeoutSignal: () => new AbortController().signal,
},
);

await assert.rejects(() => llm.send([{ role: 'user', content: 'hi' }]), /100ms logical deadline/);
assert.equal(requests, 1);
});

test('sends a json schema as response_format when a schema is passed', () => {
const llm = new LLM(base);
const schema = { type: 'object', properties: { a: { type: 'string' } }, required: ['a'] };
Expand Down
Loading