fix: fail closed on unavailable llm review

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,
+};

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');
+  });
+});