From 8d701b924813b7ec2898277be96a2086115056a2 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 15:43:44 +0000 Subject: [PATCH] fall back to a local reviewer's served model so an unpinned mtplx review isn't blocked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review loop configured with `mtplx` and no `mtplxModel` scalar failed every pass with "No model configured for mtplx reviewer", which the agent's local-review script maps to `no-verdict` — review-blocked, while the MTPLX daemon was up and serving exactly one model. The model id carried no information there: a single-model daemon (MTPLX, llama.cpp, vLLM, or LM Studio with one model loaded) answers "which model?" unambiguously. `runToolFreeLocalCompletion` now probes the backend's own `/v1/models` (via the shared `probeOpenAiModels`) when no model is pinned and uses the answer when the backend serves exactly one. Ambiguity is never guessed: several models (an ordinary Ollama install lists every one it has), none, or an unreadable listing still error, now naming which of those it was. Results report the model the pass actually ran with. The failure also carries `code: 'NO_MODEL'`, so a config gap maps to 400 at `POST /api/code-review/local` and to `RECHECK_NO_MODEL` in the challenge re-check, instead of the 502 bucket meant for a reviewer that ran and failed. Also fixes the re-check's model read, which used an ollama-or-lmstudio ternary and so handed any third local backend LM Studio's model id. --- server/routes/codeReview.js | 9 ++- server/routes/codeReview.test.js | 18 ++++++ server/services/codeReview.js | 93 ++++++++++++++++++++++------ server/services/codeReview.test.js | 69 +++++++++++++++++++-- server/services/cosTaskStore.js | 31 ++++++---- server/services/cosTaskStore.test.js | 28 ++++++++- 6 files changed, 208 insertions(+), 40 deletions(-) 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 () => {