Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions server/routes/codeReview.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <N>` 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
Expand Down Expand Up @@ -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 }
})
}
Expand Down
18 changes: 18 additions & 0 deletions server/routes/codeReview.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
93 changes: 75 additions & 18 deletions server/services/codeReview.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 `<backend>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
Expand All @@ -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 || '')) {
Expand Down Expand Up @@ -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
Expand All @@ -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.' }
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down
69 changes: 65 additions & 4 deletions server/services/codeReview.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
// `<backend>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 () => {
Expand Down
31 changes: 18 additions & 13 deletions server/services/cosTaskStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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 `<reviewer>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);
Expand All @@ -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 });
}
Loading