diff --git a/server/routes/codeReview.js b/server/routes/codeReview.js index 893ba25205..03d4bad386 100644 --- a/server/routes/codeReview.js +++ b/server/routes/codeReview.js @@ -9,7 +9,9 @@ const router = Router() // Body shape for POST /api/code-review/local. `model` and `effort` are optional — // when omitted (or empty) we fall back to the model / reasoning effort configured -// on the Code Review Defaults panel. The diff is sent as-is; agents can pipe +// on the Code Review Defaults panel, and with no configured model either, to the +// model the backend itself reports serving when that is unambiguous (see +// `resolveServedModel`). The diff is sent as-is; agents can pipe // `gh pr diff ` straight into it without preprocessing. // `effort` is checked against the ladder for the REQUESTED backend rather than a // flat union of every local level: the two backends are separate identities in @@ -74,8 +76,11 @@ router.post('/local', asyncHandler(async (req, res) => { timeoutMs: body.timeoutMs, }) if (!result.ok) { + // A model neither the request, the panel, nor the backend's own listing could + // supply is the caller's config gap (400) — the 502 bucket is for a reviewer + // that was actually asked and failed. throw new ServerError(result.error || 'Code review failed', { - status: 502, + status: result.code === 'NO_MODEL' ? 400 : 502, context: { backend: result.backend, model: result.model } }) } diff --git a/server/routes/codeReview.test.js b/server/routes/codeReview.test.js index bec2ddbaed..d125651b8c 100644 --- a/server/routes/codeReview.test.js +++ b/server/routes/codeReview.test.js @@ -183,6 +183,24 @@ describe('POST /api/code-review/local', () => { ) }) + // A model neither the request, the panel, nor the backend's own listing could + // supply is the caller's config gap, not a reviewer that was asked and failed — + // an agent retrying a 502 would retry forever against an unset setting. + it('returns 400 when the service could not resolve a model', async () => { + codeReviewSvc.runLocalCodeReview.mockResolvedValue({ + ok: false, + code: 'NO_MODEL', + error: 'No model configured for mtplx reviewer and mtplx is serving no models — set one on the Settings → Code Reviewers page.', + }) + + const res = await request(makeApp()) + .post('/api/code-review/local') + .send({ backend: 'mtplx', diff: 'diff --git a b' }) + + expect(res.status).toBe(400) + expect(res.body.error).toMatch(/No model configured/) + }) + it('returns 502 when the service returns { ok: false }', async () => { codeReviewSvc.runLocalCodeReview.mockResolvedValue({ ok: false, diff --git a/server/services/codeReview.js b/server/services/codeReview.js index d20f515236..40e1c98749 100644 --- a/server/services/codeReview.js +++ b/server/services/codeReview.js @@ -16,6 +16,8 @@ import { fetchWithTimeout } from '../lib/fetchWithTimeout.js' import { readResponseJson } from '../lib/readResponseJson.js' import { commandExists } from '../lib/commandExists.js' import { extractJson } from '../lib/jsonExtract.js' +import { probeOpenAiModels } from '../lib/openAiModelsProbe.js' +import { normalizeOpenAiBaseUrl } from '../lib/localProviderRuntime.js' import { LOCAL_LLM_REVIEWERS, DEFAULT_REVIEWERS, @@ -387,12 +389,66 @@ async function sendChatCompletion(baseUrl, { model, messages, timeoutMs }, effor return { ok: true, response } } -async function runToolFreeLocalCompletion({ backend, model, messages, effort, timeoutMs, baseUrl: requestedBaseUrl = null }) { +const SERVED_MODEL_PROBE_TIMEOUT_MS = 5_000 + +/** + * The model a local reviewer runs with when the user pinned none: ask the + * backend what it is actually serving, and use it when the answer is + * unambiguous. + * + * A single-model daemon (MTPLX, llama.cpp, vLLM — and LM Studio with one model + * loaded) makes "which model?" a question with exactly one answer, so failing + * the whole review pass over an unset `Model` scalar blocks a review + * loop on a config field that carries no information. An MTPLX reviewer hit + * exactly that: the daemon was up and serving, and the pass returned no verdict + * because nothing had typed the model id into settings. + * + * Ambiguity is NOT resolved by guessing. Ollama lists every installed model, so + * a normal install answers with many — picking one would silently review with a + * model the user never chose (a small embedding or chat model reads a diff very + * differently from a coder model). Several models, none, or an unreadable + * listing all fall through to the "pin one" error. + * + * @returns {Promise<{model: string|null, reason: string|null}>} + */ +async function resolveServedModel(backend, baseUrl) { + // Back to the `/v1` root the probe wants, through the shared normalizer rather + // than a re-typed suffix — the caller collapsed it to the host root for the + // chat-completions path. + const probe = await probeOpenAiModels(normalizeOpenAiBaseUrl(baseUrl), { timeoutMs: SERVED_MODEL_PROBE_TIMEOUT_MS }) + .catch((err) => ({ reachable: false, models: null, error: err.message })) + if (!probe.reachable) return { model: null, reason: `${backend} is not reachable (${probe.error || 'no response'})` } + if (!Array.isArray(probe.models)) return { model: null, reason: `${backend} did not report which models it is serving` } + if (probe.models.length === 0) return { model: null, reason: `${backend} is serving no models` } + if (probe.models.length > 1) return { model: null, reason: `${backend} is serving ${probe.models.length} models, so there is no unambiguous default` } + return { model: probe.models[0], reason: null } +} + +async function runToolFreeLocalCompletion({ backend, model: pinnedModel, messages, effort, timeoutMs, baseUrl: requestedBaseUrl = null }) { if (!isLocalLlmReviewer(backend)) { return { ok: false, error: `Unsupported reviewer backend: ${backend}` } } + + // Local runtime records are normalized to the OpenAI `/v1` root, while the + // legacy backend managers return the host root. Keep both forms compatible + // with the one endpoint suffix below. + const baseUrl = String(requestedBaseUrl || await BACKEND_BASE_URLS[backend]()) + .replace(/\/+$/, '') + .replace(/\/v\d+$/i, '') + + // An unpinned model is recoverable when the backend serves exactly one — see + // `resolveServedModel`. Resolved BEFORE the effort probe below, which is keyed + // by `backend:model`. + let model = pinnedModel if (!model || typeof model !== 'string') { - return { ok: false, error: `No model configured for ${backend} reviewer — set one on the Settings → Code Reviewers page.` } + const served = await resolveServedModel(backend, baseUrl) + if (!served.model) { + // `code` so a caller can tell a config gap from a reviewer that ran and + // failed (a 4xx vs the 502 bucket) without matching on the message text. + return { ok: false, code: 'NO_MODEL', error: `No model configured for ${backend} reviewer and ${served.reason} — set one on the Settings → Code Reviewers page.` } + } + model = served.model + console.log(`🔍 No ${backend} reviewer model configured — using the only model it serves: ${model}`) } // Probe only when there is actually a level to drop — an unpinned effort @@ -401,13 +457,6 @@ async function runToolFreeLocalCompletion({ backend, model, messages, effort, ti let effortUnsupported = requestedEffort ? await modelRejectsThinking(backend, model) : false let resolvedEffort = effortUnsupported ? null : requestedEffort - // Local runtime records are normalized to the OpenAI `/v1` root, while the - // legacy backend managers return the host root. Keep both forms compatible - // with the one endpoint suffix below. - const baseUrl = String(requestedBaseUrl || await BACKEND_BASE_URLS[backend]()) - .replace(/\/+$/, '') - .replace(/\/v\d+$/i, '') - let attempt = await sendChatCompletion(baseUrl, { model, messages, timeoutMs }, resolvedEffort) if (!attempt.ok && attempt.status === 400 && resolvedEffort && /does not support thinking/i.test(attempt.text || '')) { @@ -448,7 +497,10 @@ async function runToolFreeLocalCompletion({ backend, model, messages, effort, ti * * @param {Object} opts * @param {'lmstudio'|'ollama'|'mtplx'} opts.backend - * @param {string} opts.model - Installed model id (e.g. `qwen2.5-coder:7b`). + * @param {string} [opts.model] - Installed model id (e.g. `qwen2.5-coder:7b`). + * Optional: when unset, the model the backend is serving is used, provided it + * is serving exactly one (a single-model daemon like MTPLX). Several, none, or + * an unreadable listing is an error rather than a guess. * @param {string} opts.diff - Unified diff text to review. * @param {string} [opts.effort] - Reasoning effort (`low`/`medium`/`high`), sent * as the OpenAI-compatible `reasoning_effort` field. Omitted from the body @@ -468,9 +520,9 @@ export async function runLocalCodeReview({ backend, model, diff, effort = null, if (!isLocalLlmReviewer(backend)) { return { ok: false, error: `Unsupported reviewer backend: ${backend}` } } - if (!model || typeof model !== 'string') { - return { ok: false, error: `No model configured for ${backend} reviewer — set one on the Settings → Code Reviewers page.` } - } + // No model pre-check here: an unpinned model is resolved from what the backend + // is serving inside `runToolFreeLocalCompletion`, and a second copy of the + // guard would reject the recoverable case before that ever ran. const trimmedDiff = typeof diff === 'string' ? diff.trim() : '' if (!trimmedDiff) { return { ok: false, error: 'Empty diff — nothing to review.' } @@ -499,7 +551,9 @@ export async function runLocalCodeReview({ backend, model, diff, effort = null, return { ok: true, backend, - model, + // The model the pass actually ran with, which is not the argument when it + // was unpinned and resolved from the backend's own listing. + model: result.model, effort: result.effort, ...(result.effortUnsupported ? { effortUnsupported: true } : {}), findings: result.content, @@ -550,17 +604,20 @@ export async function runLocalClaimCommentReview({ backend, model, comments, cur ], }) if (!result.ok) return result + // The model the pass actually ran with, which is not the argument when it was + // unpinned and resolved from the backend's own listing. + const usedModel = result.model const { value: parsed } = extractJson(result.content, { shapePredicate: (value) => value !== null && typeof value === 'object' && !Array.isArray(value), }) if (parsed === undefined) { - return { ok: false, backend, model, error: `${backend} returned malformed claim-comment JSON.` } + return { ok: false, backend, model: usedModel, error: `${backend} returned malformed claim-comment JSON.` } } if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed) || (parsed.claimant !== null && typeof parsed.claimant !== 'string') || typeof parsed.suspicious !== 'boolean') { - return { ok: false, backend, model, error: `${backend} returned an invalid claim-comment verdict.` } + return { ok: false, backend, model: usedModel, error: `${backend} returned an invalid claim-comment verdict.` } } const claimant = parsed.claimant @@ -570,13 +627,13 @@ export async function runLocalClaimCommentReview({ backend, model, comments, cur && comment.login !== String(currentUser || '') )) if (!claimantIsEligibleInput) { - return { ok: false, backend, model, error: `${backend} returned a claimant not present as an eligible human commenter.` } + return { ok: false, backend, model: usedModel, error: `${backend} returned a claimant not present as an eligible human commenter.` } } return { ok: true, backend, - model, + model: usedModel, effort: result.effort, ...(result.effortUnsupported ? { effortUnsupported: true } : {}), claimant, diff --git a/server/services/codeReview.test.js b/server/services/codeReview.test.js index 2a438c11cd..4a4b143062 100644 --- a/server/services/codeReview.test.js +++ b/server/services/codeReview.test.js @@ -21,6 +21,10 @@ vi.mock('./providers.js', () => ({ getActiveProvider: () => Promise.resolve(mockedActiveProvider.current), })) vi.mock('./lmStudioManager.js', () => ({ getBaseUrl: () => 'http://localhost:1234' })) +// MTPLX's endpoint is resolved through a DYNAMIC import in the SUT (its manager +// drags in the managed-daemon/PM2 graph), and it reports the OpenAI `/v1` root +// rather than the host root — both halves of what the reviewer has to tolerate. +vi.mock('./mtplxServerManager.js', () => ({ getMtplxServerEndpoint: () => Promise.resolve('http://127.0.0.1:8000/v1') })) // Ollama's per-model `/api/show` capability probe, which the reviewer now // consults BEFORE attaching `reasoning_effort`. Default `null` = "probe could // not answer", the sentinel that keeps a test on the reactive 400-retry path; @@ -478,10 +482,67 @@ describe('codeReview helpers', () => { expect(r.error).toMatch(/Unsupported reviewer backend/) }) - it('requires a model id', async () => { - const r = await runLocalCodeReview({ backend: 'lmstudio', model: '', diff: 'a' }) - expect(r.ok).toBe(false) - expect(r.error).toMatch(/No model configured/) + describe('with no model pinned', () => { + // A single-model daemon (MTPLX, llama.cpp — or LM Studio with one model + // loaded) answers "which model?" unambiguously, so an unset + // `Model` scalar must not fail the whole review pass: an mtplx + // review loop was blocked with "no verdict" while its daemon was up and + // serving, purely because nothing had typed the id into settings. + const modelListing = (ids) => mockJsonResponse({ data: ids.map((id) => ({ id })) }) + + it('reviews with the only model the backend reports serving', async () => { + global.fetch = vi.fn() + .mockResolvedValueOnce(modelListing(['mlx-community/example-coder'])) + .mockResolvedValueOnce(mockJsonResponse({ choices: [{ message: { content: 'No findings.' } }] })) + + const r = await runLocalCodeReview({ backend: 'mtplx', diff: 'diff --git a b' }) + + expect(r.ok).toBe(true) + // The resolved id is reported back, not the (absent) argument — callers + // record which model produced the verdict. + expect(r.model).toBe('mlx-community/example-coder') + const [probeUrl] = global.fetch.mock.calls[0] + // MTPLX's manager reports the `/v1` root; the probe must not double it. + expect(probeUrl).toBe('http://127.0.0.1:8000/v1/models') + const [chatUrl, chatInit] = global.fetch.mock.calls[1] + expect(chatUrl).toBe('http://127.0.0.1:8000/v1/chat/completions') + expect(JSON.parse(chatInit.body).model).toBe('mlx-community/example-coder') + }) + + it('refuses to guess when the backend serves several models', async () => { + // Ollama lists every INSTALLED model, so picking one would silently + // review with a model the user never chose. + global.fetch = vi.fn().mockResolvedValue(modelListing(['qwen2.5-coder:7b', 'nomic-embed-text'])) + + const r = await runLocalCodeReview({ backend: 'ollama', diff: 'diff --git a b' }) + + expect(r.ok).toBe(false) + expect(r.code).toBe('NO_MODEL') + expect(r.error).toMatch(/serving 2 models/) + // Probe only — no review request went out on an unresolved model. + expect(global.fetch).toHaveBeenCalledTimes(1) + }) + + it('names an unreachable backend rather than reporting a bare config gap', async () => { + global.fetch = vi.fn().mockRejectedValue(Object.assign(new Error('fetch failed'), { code: 'ECONNREFUSED' })) + + const r = await runLocalCodeReview({ backend: 'mtplx', diff: 'diff --git a b' }) + + expect(r.ok).toBe(false) + expect(r.code).toBe('NO_MODEL') + expect(r.error).toMatch(/not reachable/) + }) + + it('still asks for a pin when the backend is up and serving nothing', async () => { + global.fetch = vi.fn().mockResolvedValue(modelListing([])) + + const r = await runLocalCodeReview({ backend: 'lmstudio', model: '', diff: 'diff --git a b' }) + + expect(r.ok).toBe(false) + expect(r.code).toBe('NO_MODEL') + expect(r.error).toMatch(/No model configured/) + expect(r.error).toMatch(/Code Reviewers/) + }) }) it('requires a non-empty diff', async () => { diff --git a/server/services/cosTaskStore.js b/server/services/cosTaskStore.js index 73cad94f2c..de5cd0bccb 100644 --- a/server/services/cosTaskStore.js +++ b/server/services/cosTaskStore.js @@ -1328,7 +1328,7 @@ export async function resolveTaskChallenge(taskId, { outcome, note, resolvedBy } * (→ upheld). This is the cheap confirm/overturn pass that runs BEFORE falling back * to user escalation, closing the gap #2470 left ("this slice resolves manually"). * - * Only the in-process local reviewers (`lmstudio`/`ollama`) are re-run here; CLI + * Only the in-process local reviewers (`LOCAL_LLM_REVIEWERS`) are re-run here; CLI * reviewers are re-run by the follow-up agent itself, which then calls the manual * `resolveTaskChallenge` path with an explicit outcome. * @@ -1354,19 +1354,21 @@ export async function resolveTaskChallengeWithRecheck(taskId, { recheck, resolve // stale level is null by the time it reaches here — same as the model read below. const recheckDefaults = await getCodeReviewDefaults().catch(() => null); const effort = recheckDefaults?.[`${backend}Effort`] || null; - let model = recheck?.model; - if (!model) { - model = backend === 'ollama' ? recheckDefaults?.ollamaModel : recheckDefaults?.lmstudioModel; - } - // A missing model is a config problem (no Code Review Defaults set), not an - // upstream-reviewer failure — surface it as a 4xx (RECHECK_NO_MODEL → 400), not - // the 502 bucket reserved for a reviewer that's actually unreachable. - if (!model) { - return { error: `No model configured for the ${backend} reviewer — set one on the Settings → Code Reviewers page.`, code: 'RECHECK_NO_MODEL' }; - } - console.log(`⚖️ Re-checking challenge on ${taskId} via ${backend} (${model}${effort ? `, ${effort} effort` : ''})`); + // Keyed off the roster's `Model` scalar rather than a per-backend + // branch (matching `POST /api/code-review/local`): the old ollama-or-lmstudio + // ternary read LM STUDIO's model for any third local backend, so an `mtplx` + // re-check ran against a model id from the wrong daemon. + // + // An unset scalar is no longer fatal here: `runLocalCodeReview` falls back to the + // model the backend is actually serving when that answer is unambiguous, so a + // single-model daemon re-checks without one. It reports `code: 'NO_MODEL'` when it + // could not resolve one either, which stays a config problem (4xx) rather than the + // 502 bucket reserved for a reviewer that's actually unreachable. + const model = recheck?.model || recheckDefaults?.[`${backend}Model`] || null; + console.log(`⚖️ Re-checking challenge on ${taskId} via ${backend} (${model || 'model from the backend'}${effort ? `, ${effort} effort` : ''})`); const review = await runLocalCodeReview({ backend, model, effort, diff: recheck?.diff }); if (!review?.ok) { + if (review?.code === 'NO_MODEL') return { error: review.error, code: 'RECHECK_NO_MODEL' }; return { error: `Re-check failed: ${review?.error || 'unknown reviewer error'}`, code: 'RECHECK_FAILED' }; } const outcome = classifyRecheckOutcome(review.findings); @@ -1378,6 +1380,9 @@ export async function resolveTaskChallengeWithRecheck(taskId, { recheck, resolve : `a blocking finding still stands (${backend})`; // The resolution note is auto-generated from the re-check verdict (any caller // `note` is intentionally not threaded here — the machine verdict is the record). - const note = `Auto re-check by ${backend} (${model}): ${verdict}.`; + // `review.model` rather than `model`: the reviewer resolves an unpinned id from + // what the backend is serving, and the record has to name the model that actually + // produced this verdict, not `null`. + const note = `Auto re-check by ${backend} (${review.model || model}): ${verdict}.`; return resolveTaskChallenge(taskId, { outcome, note, resolvedBy: resolvedBy || `recheck:${backend}` }, taskType, { now }); } diff --git a/server/services/cosTaskStore.test.js b/server/services/cosTaskStore.test.js index 83acc80442..10b7e9324a 100644 --- a/server/services/cosTaskStore.test.js +++ b/server/services/cosTaskStore.test.js @@ -1878,13 +1878,35 @@ describe('cosTaskStore.resolveTaskChallengeWithRecheck (#2471)', () => { expect(mock.reviewCalls[0].model).toBe('coder-7b'); }); - it('returns RECHECK_NO_MODEL (config problem, not 502) when no model is configured', async () => { + // The model read is keyed off `Model`, not an ollama-or-lmstudio + // ternary — that ternary handed a third local backend LM STUDIO's model id. + it("reads the re-check model from the challenged backend's own scalar", async () => { + const id = await seedChallenged('mtplx'); + mock.reviewDefaults = { lmstudioModel: 'wrong-model', mtplxModel: 'mtplx-model' }; + mock.review = { ok: true, model: 'mtplx-model', findings: 'No findings.' }; + await resolveTaskChallengeWithRecheck(id, { recheck: { backend: 'mtplx', diff: 'diff' } }); + expect(mock.reviewCalls[0].model).toBe('mtplx-model'); + }); + + it('leaves an unconfigured model to the reviewer, which resolves it from the backend', async () => { + const id = await seedChallenged(); + mock.reviewDefaults = { lmstudioModel: null, ollamaModel: null }; + mock.review = { ok: true, model: 'served-by-the-daemon', findings: 'No findings.' }; + const resolved = await resolveTaskChallengeWithRecheck(id, { recheck: { backend: 'ollama', diff: 'diff' } }); + expect(mock.reviewCalls[0].model).toBeNull(); + expect(resolved.status).toBe('pending'); + // The record names the model that actually produced the verdict, not `null`. + expect(resolved.metadata.challengeResolution.note).toContain('served-by-the-daemon'); + }); + + it('returns RECHECK_NO_MODEL (config problem, not 502) when nothing can name a model', async () => { const id = await seedChallenged(); mock.reviewDefaults = { lmstudioModel: null, ollamaModel: null }; + mock.review = { ok: false, code: 'NO_MODEL', error: 'No model configured for ollama reviewer and ollama is serving no models — set one on the Settings → Code Reviewers page.' }; const result = await resolveTaskChallengeWithRecheck(id, { recheck: { backend: 'ollama', diff: 'diff' } }); expect(result.code).toBe('RECHECK_NO_MODEL'); - // No reviewer call attempted without a model. - expect(mock.reviewCalls.length).toBe(0); + // Surfaced verbatim: the reviewer's message says WHICH gap it is. + expect(result.error).toMatch(/No model configured/); }); it('returns RECHECK_FAILED when the reviewer is unreachable', async () => {