Skip to content

Commit 079e022

Browse files
autogame-17autogame-17
authored
Merge pull request #614 from EvoMap/claude/recursing-carson-ca2ddf
fix: fail closed on unavailable llm review
2 parents d9df8fb + 3effd6d commit 079e022

2 files changed

Lines changed: 303 additions & 30 deletions

File tree

‎src/gep/llmReview.js‎

Lines changed: 116 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ const { getRepoRoot } = require('./paths');
88

99
const REVIEW_ENABLED_KEY = 'EVOLVER_LLM_REVIEW';
1010
const REVIEW_TIMEOUT_MS = 30000;
11+
const REVIEW_MAX_ATTEMPTS = 2;
1112

1213
function isLlmReviewEnabled() {
1314
return String(process.env[REVIEW_ENABLED_KEY] || '').toLowerCase() === 'true';
@@ -48,45 +49,130 @@ Respond with a JSON object:
4849
}`;
4950
}
5051

51-
function runLlmReview({ diff, gene, signals, mutation }) {
52-
if (!isLlmReviewEnabled()) return null;
52+
function failureResult(reason, summary, trace) {
53+
return {
54+
approved: false,
55+
confidence: 0,
56+
concerns: [summary],
57+
summary,
58+
status: 'unavailable',
59+
reason,
60+
retryable: true,
61+
attempts: trace.length,
62+
trace,
63+
};
64+
}
5365

54-
const prompt = buildReviewPrompt({ diff, gene, signals, mutation });
66+
function parseReviewResponse(output) {
67+
const text = typeof output === 'string' ? output.trim() : '';
68+
if (!text) {
69+
return { ok: false, reason: 'empty_output', summary: 'review returned empty output' };
70+
}
5571

72+
let parsed;
5673
try {
57-
const repoRoot = getRepoRoot();
74+
parsed = JSON.parse(text);
75+
} catch (_) {
76+
const partial = /^[{[]/.test(text) && !/[}\]]\s*$/.test(text);
77+
return {
78+
ok: false,
79+
reason: partial ? 'partial_response' : 'malformed_output',
80+
summary: partial ? 'review returned a partial response' : 'review returned malformed output',
81+
};
82+
}
83+
84+
if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed) ||
85+
typeof parsed.approved !== 'boolean' ||
86+
!Number.isFinite(parsed.confidence) || parsed.confidence < 0 || parsed.confidence > 1 ||
87+
!Array.isArray(parsed.concerns) || !parsed.concerns.every(item => typeof item === 'string') ||
88+
typeof parsed.summary !== 'string' || !parsed.summary.trim()) {
89+
return { ok: false, reason: 'partial_response', summary: 'review response was incomplete' };
90+
}
91+
92+
return {
93+
ok: true,
94+
value: {
95+
approved: parsed.approved,
96+
confidence: parsed.confidence,
97+
concerns: parsed.concerns,
98+
summary: parsed.summary.trim(),
99+
status: parsed.approved ? 'approved' : 'rejected',
100+
reason: parsed.approved ? 'review_approved' : 'review_rejected',
101+
retryable: false,
102+
},
103+
};
104+
}
105+
106+
function classifyExecutionError(error) {
107+
const message = error && error.message ? String(error.message) : String(error);
108+
const timedOut = Boolean(error && (error.code === 'ETIMEDOUT' || error.signal === 'SIGTERM')) || /timed?\s*out|ETIMEDOUT/i.test(message);
109+
return {
110+
reason: timedOut ? 'timeout' : 'runner_error',
111+
summary: timedOut ? 'review timed out' : 'review execution failed',
112+
};
113+
}
58114

59-
// Write prompt to a temp file to avoid shell quoting issues entirely.
60-
const tmpFile = path.join(os.tmpdir(), 'evolver_review_prompt_' + process.pid + '.txt');
115+
function defaultExecute({ prompt, timeoutMs }) {
116+
const repoRoot = getRepoRoot();
117+
let tmpDir;
118+
try {
119+
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'evolver-review-'));
120+
const tmpFile = path.join(tmpDir, 'prompt.txt');
61121
fs.writeFileSync(tmpFile, prompt, 'utf8');
122+
const reviewScript = `
123+
const fs = require('fs');
124+
const prompt = fs.readFileSync(process.argv[1], 'utf8');
125+
console.log(JSON.stringify({ approved: true, confidence: 0.7, concerns: [], summary: 'auto-approved (no external LLM configured)' }));
126+
`;
127+
return execFileSync(process.execPath, ['-e', reviewScript, tmpFile], {
128+
cwd: repoRoot,
129+
encoding: 'utf8',
130+
timeout: timeoutMs,
131+
stdio: ['ignore', 'pipe', 'pipe'],
132+
windowsHide: true,
133+
});
134+
} finally {
135+
if (tmpDir) {
136+
try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch (_) {}
137+
}
138+
}
139+
}
140+
141+
function runLlmReview({ diff, gene, signals, mutation }, options) {
142+
if (!isLlmReviewEnabled()) return null;
143+
144+
const opts = options || {};
145+
const execute = typeof opts.execute === 'function' ? opts.execute : defaultExecute;
146+
const timeoutMs = Number.isFinite(opts.timeoutMs) && opts.timeoutMs > 0 ? opts.timeoutMs : REVIEW_TIMEOUT_MS;
147+
const maxAttempts = Number.isInteger(opts.maxAttempts) && opts.maxAttempts > 0 ? opts.maxAttempts : REVIEW_MAX_ATTEMPTS;
148+
const prompt = buildReviewPrompt({ diff, gene, signals, mutation });
149+
const trace = [];
62150

151+
for (let attempt = 1; attempt <= maxAttempts; attempt += 1) {
152+
let failure;
63153
try {
64-
// Use execFileSync to bypass shell interpretation (no quoting issues).
65-
const reviewScript = `
66-
const fs = require('fs');
67-
const prompt = fs.readFileSync(process.argv[1], 'utf8');
68-
console.log(JSON.stringify({ approved: true, confidence: 0.7, concerns: [], summary: 'auto-approved (no external LLM configured)' }));
69-
`;
70-
const result = execFileSync(process.execPath, ['-e', reviewScript, tmpFile], {
71-
cwd: repoRoot,
72-
encoding: 'utf8',
73-
timeout: REVIEW_TIMEOUT_MS,
74-
stdio: ['ignore', 'pipe', 'pipe'],
75-
windowsHide: true,
76-
});
77-
78-
try {
79-
return JSON.parse(result.trim());
80-
} catch (_) {
81-
return { approved: true, confidence: 0.5, concerns: ['failed to parse review response'], summary: 'review parse error' };
154+
const parsed = parseReviewResponse(execute({ prompt, timeoutMs, attempt }));
155+
if (parsed.ok) {
156+
trace.push({ attempt, status: parsed.value.status, reason: parsed.value.reason });
157+
return Object.assign({}, parsed.value, { attempts: attempt, trace });
82158
}
83-
} finally {
84-
try { fs.unlinkSync(tmpFile); } catch (_) {}
159+
failure = parsed;
160+
} catch (error) {
161+
failure = classifyExecutionError(error);
162+
}
163+
164+
trace.push({ attempt, status: 'unavailable', reason: failure.reason });
165+
if (attempt === maxAttempts) {
166+
console.log('[LLMReview] Unavailable: ' + failure.reason + ' after ' + attempt + ' attempt(s)');
167+
return failureResult(failure.reason, failure.summary, trace);
85168
}
86-
} catch (e) {
87-
console.log('[LLMReview] Execution failed (non-fatal): ' + (e && e.message ? e.message : e));
88-
return { approved: true, confidence: 0.5, concerns: ['review execution failed'], summary: 'review timeout or error' };
89169
}
90170
}
91171

92-
module.exports = { isLlmReviewEnabled, runLlmReview, buildReviewPrompt };
172+
module.exports = {
173+
isLlmReviewEnabled,
174+
runLlmReview,
175+
buildReviewPrompt,
176+
parseReviewResponse,
177+
classifyExecutionError,
178+
};

‎test/llmReview.test.js‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
'use strict';
2+
3+
const { describe, it, beforeEach, afterEach } = require('node:test');
4+
const assert = require('node:assert/strict');
5+
const fs = require('fs');
6+
const os = require('os');
7+
8+
const { runLlmReview, parseReviewResponse } = require('../src/gep/llmReview');
9+
10+
const input = {
11+
diff: 'diff --git a/example.js b/example.js\n+const safe = true;',
12+
gene: { id: 'gene_llm_review_test', category: 'repair' },
13+
signals: ['review-boundary'],
14+
mutation: { rationale: 'exercise the review boundary' },
15+
};
16+
17+
let previousEnabled;
18+
beforeEach(function () {
19+
previousEnabled = process.env.EVOLVER_LLM_REVIEW;
20+
process.env.EVOLVER_LLM_REVIEW = 'true';
21+
});
22+
afterEach(function () {
23+
if (previousEnabled === undefined) delete process.env.EVOLVER_LLM_REVIEW;
24+
else process.env.EVOLVER_LLM_REVIEW = previousEnabled;
25+
});
26+
27+
function assertUnavailable(result, reason, attempts) {
28+
assert.equal(result.approved, false);
29+
assert.equal(result.status, 'unavailable');
30+
assert.equal(result.reason, reason);
31+
assert.equal(result.retryable, true);
32+
assert.equal(result.attempts, attempts);
33+
assert.equal(result.trace.length, attempts);
34+
assert.ok(result.trace.every(entry => entry.status === 'unavailable'));
35+
}
36+
37+
describe('llmReview fail-closed boundary', function () {
38+
it('does not approve malformed output after retry exhaustion', function () {
39+
const result = runLlmReview(input, { execute: () => 'not-json', maxAttempts: 2 });
40+
assertUnavailable(result, 'malformed_output', 2);
41+
});
42+
43+
it('does not approve empty output after retry exhaustion', function () {
44+
const result = runLlmReview(input, { execute: () => ' \n', maxAttempts: 2 });
45+
assertUnavailable(result, 'empty_output', 2);
46+
});
47+
48+
it('does not approve runner errors after retry exhaustion', function () {
49+
const result = runLlmReview(input, {
50+
execute: () => { const error = new Error('runner failed'); error.code = 'EIO'; throw error; },
51+
maxAttempts: 2,
52+
});
53+
assertUnavailable(result, 'runner_error', 2);
54+
});
55+
56+
it('does not approve timeouts after retry exhaustion', function () {
57+
const result = runLlmReview(input, {
58+
execute: () => { const error = new Error('spawnSync node ETIMEDOUT'); error.code = 'ETIMEDOUT'; throw error; },
59+
maxAttempts: 2,
60+
});
61+
assertUnavailable(result, 'timeout', 2);
62+
});
63+
64+
it('does not approve truncated or structurally partial responses', function () {
65+
const truncated = runLlmReview(input, {
66+
execute: () => '{"approved":true,"confidence":',
67+
maxAttempts: 1,
68+
});
69+
assertUnavailable(truncated, 'partial_response', 1);
70+
71+
const incomplete = runLlmReview(input, {
72+
execute: () => JSON.stringify({ approved: true, confidence: 0.8 }),
73+
maxAttempts: 1,
74+
});
75+
assertUnavailable(incomplete, 'partial_response', 1);
76+
});
77+
78+
it('retries a recoverable failure and preserves the valid success path', function () {
79+
let calls = 0;
80+
const result = runLlmReview(input, {
81+
execute: () => {
82+
calls += 1;
83+
if (calls === 1) return '';
84+
return JSON.stringify({ approved: true, confidence: 0.9, concerns: [], summary: 'verified' });
85+
},
86+
maxAttempts: 2,
87+
});
88+
89+
assert.equal(result.approved, true);
90+
assert.equal(result.status, 'approved');
91+
assert.equal(result.reason, 'review_approved');
92+
assert.equal(result.retryable, false);
93+
assert.equal(result.attempts, 2);
94+
assert.deepEqual(result.trace, [
95+
{ attempt: 1, status: 'unavailable', reason: 'empty_output' },
96+
{ attempt: 2, status: 'approved', reason: 'review_approved' },
97+
]);
98+
});
99+
100+
it('preserves explicit rejection and compatibility fields', function () {
101+
const result = runLlmReview(input, {
102+
execute: () => JSON.stringify({
103+
approved: false,
104+
confidence: 0.95,
105+
concerns: ['unsafe mutation'],
106+
summary: 'reject unsafe mutation',
107+
}),
108+
maxAttempts: 1,
109+
});
110+
111+
assert.deepEqual(
112+
{
113+
approved: result.approved,
114+
confidence: result.confidence,
115+
concerns: result.concerns,
116+
summary: result.summary,
117+
status: result.status,
118+
reason: result.reason,
119+
},
120+
{
121+
approved: false,
122+
confidence: 0.95,
123+
concerns: ['unsafe mutation'],
124+
summary: 'reject unsafe mutation',
125+
status: 'rejected',
126+
reason: 'review_rejected',
127+
}
128+
);
129+
});
130+
131+
it('is idempotent for the same input and deterministic runner output', function () {
132+
const prompts = [];
133+
const execute = ({ prompt }) => {
134+
prompts.push(prompt);
135+
return JSON.stringify({ approved: true, confidence: 0.8, concerns: [], summary: 'stable review' });
136+
};
137+
138+
const first = runLlmReview(input, { execute, maxAttempts: 1 });
139+
const second = runLlmReview(input, { execute, maxAttempts: 1 });
140+
141+
assert.deepEqual(second, first);
142+
assert.equal(prompts.length, 2);
143+
assert.equal(prompts[1], prompts[0]);
144+
});
145+
146+
it('records a real subprocess trace and removes its temporary directory', function () {
147+
const before = new Set(fs.readdirSync(os.tmpdir()).filter(name => name.startsWith('evolver-review-')));
148+
const result = runLlmReview(input, { maxAttempts: 1, timeoutMs: 5000 });
149+
const after = fs.readdirSync(os.tmpdir()).filter(name => name.startsWith('evolver-review-'));
150+
151+
assert.equal(result.approved, true);
152+
assert.deepEqual(result.trace, [{ attempt: 1, status: 'approved', reason: 'review_approved' }]);
153+
assert.deepEqual(after.filter(name => !before.has(name)), []);
154+
});
155+
156+
it('does not leak a temp directory when prompt write fails', function () {
157+
const before = new Set(fs.readdirSync(os.tmpdir()).filter(name => name.startsWith('evolver-review-')));
158+
const originalWrite = fs.writeFileSync;
159+
fs.writeFileSync = function (file) {
160+
if (String(file).includes('evolver-review-') && String(file).endsWith('prompt.txt')) {
161+
throw new Error('ENOSPC: no space left on device');
162+
}
163+
return originalWrite.apply(this, arguments);
164+
};
165+
try {
166+
const result = runLlmReview(input, { maxAttempts: 1, timeoutMs: 5000 });
167+
assertUnavailable(result, 'runner_error', 1);
168+
} finally {
169+
fs.writeFileSync = originalWrite;
170+
}
171+
const after = fs.readdirSync(os.tmpdir()).filter(name => name.startsWith('evolver-review-'));
172+
assert.deepEqual(after.filter(name => !before.has(name)), []);
173+
});
174+
175+
it('returns null when review is disabled', function () {
176+
process.env.EVOLVER_LLM_REVIEW = 'false';
177+
assert.equal(runLlmReview(input, { execute: () => { throw new Error('must not run'); } }), null);
178+
});
179+
});
180+
181+
describe('parseReviewResponse', function () {
182+
it('rejects out-of-range confidence', function () {
183+
const invalid = parseReviewResponse(JSON.stringify({ approved: true, confidence: 2, concerns: [], summary: 'bad' }));
184+
assert.equal(invalid.ok, false);
185+
assert.equal(invalid.reason, 'partial_response');
186+
});
187+
});

0 commit comments

Comments
 (0)