diff --git a/src/gep/llmReview.js b/src/gep/llmReview.js index 7f33f9bd..61d5cbf1 100644 --- a/src/gep/llmReview.js +++ b/src/gep/llmReview.js @@ -8,6 +8,7 @@ const { getRepoRoot } = require('./paths'); const REVIEW_ENABLED_KEY = 'EVOLVER_LLM_REVIEW'; const REVIEW_TIMEOUT_MS = 30000; +const REVIEW_MAX_ATTEMPTS = 2; function isLlmReviewEnabled() { return String(process.env[REVIEW_ENABLED_KEY] || '').toLowerCase() === 'true'; @@ -48,45 +49,128 @@ Respond with a JSON object: }`; } -function runLlmReview({ diff, gene, signals, mutation }) { - if (!isLlmReviewEnabled()) return null; +function failureResult(reason, summary, trace) { + return { + approved: false, + confidence: 0, + concerns: [summary], + summary, + status: 'unavailable', + reason, + retryable: true, + attempts: trace.length, + trace, + }; +} - const prompt = buildReviewPrompt({ diff, gene, signals, mutation }); +function parseReviewResponse(output) { + const text = typeof output === 'string' ? output.trim() : ''; + if (!text) { + return { ok: false, reason: 'empty_output', summary: 'review returned empty output' }; + } + let parsed; try { - const repoRoot = getRepoRoot(); + parsed = JSON.parse(text); + } catch (_) { + const partial = /^[{[]/.test(text) && !/[}\]]\s*$/.test(text); + return { + ok: false, + reason: partial ? 'partial_response' : 'malformed_output', + summary: partial ? 'review returned a partial response' : 'review returned malformed output', + }; + } + + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed) || + typeof parsed.approved !== 'boolean' || + !Number.isFinite(parsed.confidence) || parsed.confidence < 0 || parsed.confidence > 1 || + !Array.isArray(parsed.concerns) || !parsed.concerns.every(item => typeof item === 'string') || + typeof parsed.summary !== 'string' || !parsed.summary.trim()) { + return { ok: false, reason: 'partial_response', summary: 'review response was incomplete' }; + } + + return { + ok: true, + value: { + approved: parsed.approved, + confidence: parsed.confidence, + concerns: parsed.concerns, + summary: parsed.summary.trim(), + status: parsed.approved ? 'approved' : 'rejected', + reason: parsed.approved ? 'review_approved' : 'review_rejected', + retryable: false, + }, + }; +} + +function classifyExecutionError(error) { + const message = error && error.message ? String(error.message) : String(error); + const timedOut = Boolean(error && (error.code === 'ETIMEDOUT' || error.signal === 'SIGTERM')) || /timed?\s*out|ETIMEDOUT/i.test(message); + return { + reason: timedOut ? 'timeout' : 'runner_error', + summary: timedOut ? 'review timed out' : 'review execution failed', + }; +} - // Write prompt to a temp file to avoid shell quoting issues entirely. - const tmpFile = path.join(os.tmpdir(), 'evolver_review_prompt_' + process.pid + '.txt'); - fs.writeFileSync(tmpFile, prompt, 'utf8'); +function defaultExecute({ prompt, timeoutMs }) { + const repoRoot = getRepoRoot(); + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'evolver-review-')); + const tmpFile = path.join(tmpDir, 'prompt.txt'); + fs.writeFileSync(tmpFile, prompt, 'utf8'); + try { + const reviewScript = ` + const fs = require('fs'); + const prompt = fs.readFileSync(process.argv[1], 'utf8'); + console.log(JSON.stringify({ approved: true, confidence: 0.7, concerns: [], summary: 'auto-approved (no external LLM configured)' })); + `; + return execFileSync(process.execPath, ['-e', reviewScript, tmpFile], { + cwd: repoRoot, + encoding: 'utf8', + timeout: timeoutMs, + stdio: ['ignore', 'pipe', 'pipe'], + windowsHide: true, + }); + } finally { + try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch (_) {} + } +} + +function runLlmReview({ diff, gene, signals, mutation }, options) { + if (!isLlmReviewEnabled()) return null; + + const opts = options || {}; + const execute = typeof opts.execute === 'function' ? opts.execute : defaultExecute; + const timeoutMs = Number.isFinite(opts.timeoutMs) && opts.timeoutMs > 0 ? opts.timeoutMs : REVIEW_TIMEOUT_MS; + const maxAttempts = Number.isInteger(opts.maxAttempts) && opts.maxAttempts > 0 ? opts.maxAttempts : REVIEW_MAX_ATTEMPTS; + const prompt = buildReviewPrompt({ diff, gene, signals, mutation }); + const trace = []; + + for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { + let failure; try { - // Use execFileSync to bypass shell interpretation (no quoting issues). - const reviewScript = ` - const fs = require('fs'); - const prompt = fs.readFileSync(process.argv[1], 'utf8'); - console.log(JSON.stringify({ approved: true, confidence: 0.7, concerns: [], summary: 'auto-approved (no external LLM configured)' })); - `; - const result = execFileSync(process.execPath, ['-e', reviewScript, tmpFile], { - cwd: repoRoot, - encoding: 'utf8', - timeout: REVIEW_TIMEOUT_MS, - stdio: ['ignore', 'pipe', 'pipe'], - windowsHide: true, - }); - - try { - return JSON.parse(result.trim()); - } catch (_) { - return { approved: true, confidence: 0.5, concerns: ['failed to parse review response'], summary: 'review parse error' }; + const parsed = parseReviewResponse(execute({ prompt, timeoutMs, attempt })); + if (parsed.ok) { + trace.push({ attempt, status: parsed.value.status, reason: parsed.value.reason }); + return Object.assign({}, parsed.value, { attempts: attempt, trace }); } - } finally { - try { fs.unlinkSync(tmpFile); } catch (_) {} + failure = parsed; + } catch (error) { + failure = classifyExecutionError(error); + } + + trace.push({ attempt, status: 'unavailable', reason: failure.reason }); + if (attempt === maxAttempts) { + console.log('[LLMReview] Unavailable: ' + failure.reason + ' after ' + attempt + ' attempt(s)'); + return failureResult(failure.reason, failure.summary, trace); } - } catch (e) { - console.log('[LLMReview] Execution failed (non-fatal): ' + (e && e.message ? e.message : e)); - return { approved: true, confidence: 0.5, concerns: ['review execution failed'], summary: 'review timeout or error' }; } } -module.exports = { isLlmReviewEnabled, runLlmReview, buildReviewPrompt }; +module.exports = { + isLlmReviewEnabled, + runLlmReview, + buildReviewPrompt, + parseReviewResponse, + classifyExecutionError, +}; diff --git a/test/llmReview.test.js b/test/llmReview.test.js new file mode 100644 index 00000000..5ca0201a --- /dev/null +++ b/test/llmReview.test.js @@ -0,0 +1,168 @@ +'use strict'; + +const { describe, it, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const os = require('os'); + +const { runLlmReview, parseReviewResponse } = require('../src/gep/llmReview'); + +const input = { + diff: 'diff --git a/example.js b/example.js\n+const safe = true;', + gene: { id: 'gene_llm_review_test', category: 'repair' }, + signals: ['review-boundary'], + mutation: { rationale: 'exercise the review boundary' }, +}; + +let previousEnabled; +beforeEach(function () { + previousEnabled = process.env.EVOLVER_LLM_REVIEW; + process.env.EVOLVER_LLM_REVIEW = 'true'; +}); +afterEach(function () { + if (previousEnabled === undefined) delete process.env.EVOLVER_LLM_REVIEW; + else process.env.EVOLVER_LLM_REVIEW = previousEnabled; +}); + +function assertUnavailable(result, reason, attempts) { + assert.equal(result.approved, false); + assert.equal(result.status, 'unavailable'); + assert.equal(result.reason, reason); + assert.equal(result.retryable, true); + assert.equal(result.attempts, attempts); + assert.equal(result.trace.length, attempts); + assert.ok(result.trace.every(entry => entry.status === 'unavailable')); +} + +describe('llmReview fail-closed boundary', function () { + it('does not approve malformed output after retry exhaustion', function () { + const result = runLlmReview(input, { execute: () => 'not-json', maxAttempts: 2 }); + assertUnavailable(result, 'malformed_output', 2); + }); + + it('does not approve empty output after retry exhaustion', function () { + const result = runLlmReview(input, { execute: () => ' \n', maxAttempts: 2 }); + assertUnavailable(result, 'empty_output', 2); + }); + + it('does not approve runner errors after retry exhaustion', function () { + const result = runLlmReview(input, { + execute: () => { const error = new Error('runner failed'); error.code = 'EIO'; throw error; }, + maxAttempts: 2, + }); + assertUnavailable(result, 'runner_error', 2); + }); + + it('does not approve timeouts after retry exhaustion', function () { + const result = runLlmReview(input, { + execute: () => { const error = new Error('spawnSync node ETIMEDOUT'); error.code = 'ETIMEDOUT'; throw error; }, + maxAttempts: 2, + }); + assertUnavailable(result, 'timeout', 2); + }); + + it('does not approve truncated or structurally partial responses', function () { + const truncated = runLlmReview(input, { + execute: () => '{"approved":true,"confidence":', + maxAttempts: 1, + }); + assertUnavailable(truncated, 'partial_response', 1); + + const incomplete = runLlmReview(input, { + execute: () => JSON.stringify({ approved: true, confidence: 0.8 }), + maxAttempts: 1, + }); + assertUnavailable(incomplete, 'partial_response', 1); + }); + + it('retries a recoverable failure and preserves the valid success path', function () { + let calls = 0; + const result = runLlmReview(input, { + execute: () => { + calls += 1; + if (calls === 1) return ''; + return JSON.stringify({ approved: true, confidence: 0.9, concerns: [], summary: 'verified' }); + }, + maxAttempts: 2, + }); + + assert.equal(result.approved, true); + assert.equal(result.status, 'approved'); + assert.equal(result.reason, 'review_approved'); + assert.equal(result.retryable, false); + assert.equal(result.attempts, 2); + assert.deepEqual(result.trace, [ + { attempt: 1, status: 'unavailable', reason: 'empty_output' }, + { attempt: 2, status: 'approved', reason: 'review_approved' }, + ]); + }); + + it('preserves explicit rejection and compatibility fields', function () { + const result = runLlmReview(input, { + execute: () => JSON.stringify({ + approved: false, + confidence: 0.95, + concerns: ['unsafe mutation'], + summary: 'reject unsafe mutation', + }), + maxAttempts: 1, + }); + + assert.deepEqual( + { + approved: result.approved, + confidence: result.confidence, + concerns: result.concerns, + summary: result.summary, + status: result.status, + reason: result.reason, + }, + { + approved: false, + confidence: 0.95, + concerns: ['unsafe mutation'], + summary: 'reject unsafe mutation', + status: 'rejected', + reason: 'review_rejected', + } + ); + }); + + it('is idempotent for the same input and deterministic runner output', function () { + const prompts = []; + const execute = ({ prompt }) => { + prompts.push(prompt); + return JSON.stringify({ approved: true, confidence: 0.8, concerns: [], summary: 'stable review' }); + }; + + const first = runLlmReview(input, { execute, maxAttempts: 1 }); + const second = runLlmReview(input, { execute, maxAttempts: 1 }); + + assert.deepEqual(second, first); + assert.equal(prompts.length, 2); + assert.equal(prompts[1], prompts[0]); + }); + + it('records a real subprocess trace and removes its temporary directory', function () { + const before = new Set(fs.readdirSync(os.tmpdir()).filter(name => name.startsWith('evolver-review-'))); + const result = runLlmReview(input, { maxAttempts: 1, timeoutMs: 5000 }); + const after = fs.readdirSync(os.tmpdir()).filter(name => name.startsWith('evolver-review-')); + + assert.equal(result.approved, true); + assert.deepEqual(result.trace, [{ attempt: 1, status: 'approved', reason: 'review_approved' }]); + assert.deepEqual(after.filter(name => !before.has(name)), []); + }); + + it('returns null when review is disabled', function () { + process.env.EVOLVER_LLM_REVIEW = 'false'; + assert.equal(runLlmReview(input, { execute: () => { throw new Error('must not run'); } }), null); + }); +}); + +describe('parseReviewResponse', function () { + it('rejects out-of-range confidence', function () { + const invalid = parseReviewResponse(JSON.stringify({ approved: true, confidence: 2, concerns: [], summary: 'bad' })); + assert.equal(invalid.ok, false); + assert.equal(invalid.reason, 'partial_response'); + }); +});