diff --git a/client/src/components/cos/ReviewerPicker.jsx b/client/src/components/cos/ReviewerPicker.jsx index b7a8060885..b3672f02b7 100644 --- a/client/src/components/cos/ReviewerPicker.jsx +++ b/client/src/components/cos/ReviewerPicker.jsx @@ -19,6 +19,13 @@ import { normalizeReviewerSlug } from '../../lib/reviewerPins'; const normalizeReviewerValue = (value) => normalizeReviewerSlug(value); +// The Model dropdown's "type an id instead" entry. Carries a `[`/`]` pair on +// purpose: `sanitizeReviewerModelInput` strips both, so this string can never +// arrive from the free-text input and be mistaken for a stored pin — and the +// server drops any id containing them, so it could not have been persisted by an +// older build either. +const CUSTOM_MODEL_OPTION = '[custom]'; + /** * Ordered multi-reviewer picker, rendered as one row per reviewer with the five * per-reviewer controls as columns: **Provider | Model | Effort | Optional | Max @@ -96,11 +103,24 @@ export default function ReviewerPicker({ const id = useId(); const [usernameInput, setUsernameInput] = useState(''); const [usernameError, setUsernameError] = useState(''); + // Reviewers whose Model cell the user switched to free text by picking the + // "Custom…" entry. Lowercased tokens, matching the case-insensitive keying the + // pin maps use. Purely presentational — nothing is stored until an id is typed, + // so this never has to round-trip through `onChange`. + const [customModelTokens, setCustomModelTokens] = useState(() => new Set()); + const isCustomModel = (token) => customModelTokens.has(token.toLowerCase()); + const setCustomModel = (token, on) => setCustomModelTokens((prev) => { + const next = new Set(prev); + if (on) next.add(token.toLowerCase()); else next.delete(token.toLowerCase()); + return next; + }); // Render the parent's list (de-duped, order-preserving) so display === stored // state for valid input while staying robust to malformed/legacy duplicates — // dupes would otherwise collide on the `key={value}` below and corrupt - // reorder/remove. An empty list shows the "defaults to Copilot" hint and lets - // the user clear copilot; the server/submit layer resolves [] → ['copilot']. + // reorder/remove. An empty list shows the "follows your default AI provider" + // hint and lets the user clear the chain entirely; the server resolves [] to + // the active provider's own reviewer (falling back to copilot when that + // provider maps to none) — see `codeReviewDefaultsFromProvider`. const selected = Array.isArray(reviewers) ? [...new Set(reviewers.map(normalizeReviewerValue))] : []; const available = REVIEWER_OPTIONS.filter(o => !selected.includes(o.value)); const hasNonCopilot = selected.some(r => r !== 'copilot'); @@ -238,7 +258,11 @@ export default function ReviewerPicker({ // the value is in fact stored. `staleSuffix` names why it's absent; `setClass` // differs only so the two columns stay visually distinguishable — passed as a // COMPLETE class string, never interpolated, or Tailwind's scanner won't emit it. - const renderPinSelect = ({ selectId, value, options, onChange, ariaLabel, title, staleSuffix, setClass, maxWidthClass }) => ( + // + // `trailingOption` appends one non-model entry after the list (the Model + // column's "Custom…" escape). It is a UI action, not a pin — the caller's + // `onChange` recognizes its sentinel value and never stores it. + const renderPinSelect = ({ selectId, value, options, onChange, ariaLabel, title, staleSuffix, setClass, maxWidthClass, trailingOption = null }) => ( ); @@ -351,13 +376,25 @@ export default function ReviewerPicker({ // The Model cell. Only MODEL_SELECTABLE_REVIEWERS get one — copilot has no CLI // and a `@username` reviewer is a person/bot, so neither takes a model. // - // A local backend's installed-model list is authoritative (the server probed - // it), so those render a closed ``: the ids are + // known, and a dropdown is how the rest of this table's pins are set. What + // differs between the two reviewer kinds is the ESCAPE HATCH, not the control: + // + // - A probed local backend's installed-model list is authoritative (the server + // asked the running daemon), so its select is closed — an id it doesn't list + // isn't installed. + // - A CLI reviewer's catalog is a stored snapshot, and an Ollama-backed + // `claude` or a Bedrock-form id can be anything the environment provides, so + // its select carries a trailing "Custom…" entry that swaps the cell for a + // free-text input (with the same ids offered as a ``). Clearing + // that input returns the cell to the dropdown. A CLI reviewer whose catalog + // resolved EMPTY (grok/kimi/opencode ship only a configured-default sentinel) + // starts in the free-text form directly — a select of nothing is a dead + // control — and so does a row already pinned to an id outside the catalog, so + // that pin stays editable rather than reading as an unpickable oddity. + // + // `loaded` gates the "nothing installed" messaging so a pre-fetch render + // doesn't accuse a healthy backend of being empty. const renderModelCell = (token) => { if (!MODEL_SELECTABLE_REVIEWERS.includes(token)) return renderNoPinCell(`${reviewerLabel(token)} takes no model`); const subject = reviewerLabel(token); @@ -367,9 +404,8 @@ export default function ReviewerPicker({ const options = modelOptions?.optionsByReviewer?.[token] || []; const inputId = `${id}-model-${token}`; const listId = `${id}-modellist-${token}`; - // No resolved options AND a closed picker would be a dead control, so fall - // back to free-text: better a typed id than no way to set one at all. - const freeText = modelOptions?.freeText?.[token] !== false || options.length === 0; + // Whether this reviewer may carry an id its catalog doesn't list at all. + const acceptsTypedId = modelOptions?.freeText?.[token] !== false; // Why a local backend has no options, so the empty state says the useful // thing instead of a bare "default" placeholder. Only meaningful once the // probe settled — before that, an empty list is "not fetched yet", not a fact. @@ -378,13 +414,23 @@ export default function ReviewerPicker({ ? `${subject} isn't reachable — start it from Models → LLMs to list its models. You can still type an id.` : `No ${subject} models listed — add one in Models → LLMs, or type an id.`) : null; + // A closed select over nothing would be a dead control, so a reviewer with no + // resolved options falls back to free text whichever kind it is: better a + // typed id than no way to set one at all. + const freeText = acceptsTypedId + ? (options.length === 0 || isCustomModel(token) || (Boolean(value) && !options.includes(value))) + : options.length === 0; if (!freeText) { return renderPinSelect({ selectId: inputId, value: value || (defaultModel && options.includes(defaultModel) ? defaultModel : ''), options, - onChange: (model) => setModel(token, model), + onChange: (model) => { + // The escape hatch is a UI mode, not an id — never store the sentinel. + if (model === CUSTOM_MODEL_OPTION) { setCustomModel(token, true); return; } + setModel(token, model); + }, ariaLabel: `Model for ${subject}`, title: value ? `${subject} reviews with ${value}. Choose "default" to let it pick.` @@ -393,27 +439,40 @@ export default function ReviewerPicker({ : `${subject} uses the model configured for its backend. Pick one to pin it for this run.`, staleSuffix: '(not installed)', setClass: 'text-port-accent border-port-accent/50', - maxWidthClass: 'max-w-[190px]' + maxWidthClass: 'max-w-[190px]', + // Only a reviewer that can run an id outside its catalog gets the escape. + trailingOption: acceptsTypedId ? { value: CUSTOM_MODEL_OPTION, label: 'Custom…' } : null }); } + // "Custom…" was picked with nothing pinned yet — start empty so the field + // reads as the blank the user is about to fill, not as an id already in play. + const inputValue = value || (isCustomModel(token) ? '' : defaultModel); return ( <> setModel(token, e.target.value)} + // Leaving the field with nothing pinned is how the user backs out of + // Custom…: with no id to keep, the catalog dropdown is the more useful + // control. Deliberately on blur rather than on an empty onChange — + // clearing the field to retype an id would otherwise swap the control + // out from under the cursor mid-edit. + onBlur={() => { if (!value && options.length) setCustomModel(token, false); }} aria-label={`Model for ${subject}`} title={value ? `${subject} reviews with ${value}. Clear to let it use its own default.` - : (defaultModel - ? `${subject} uses ${defaultModel} by default. Type or pick another id to pin one.` - : (emptyHint || `${subject} uses its own default model. Type or pick an id to pin one.`))} + : (options.length + ? `Type an id ${subject} accepts. Leave it empty to go back to its listed models.` + : (defaultModel + ? `${subject} uses ${defaultModel} by default. Type an id to pin one.` + : (emptyHint || `${subject} uses its own default model. Type an id to pin one.`)))} className={`w-full min-w-0 max-w-[190px] px-1.5 py-0.5 text-[11px] font-mono rounded border bg-port-bg min-h-[28px] disabled:opacity-40 focus:outline-none focus:border-port-accent ${value ? 'text-port-accent border-port-accent/50' : 'text-gray-500 border-port-border/60'}`} @@ -558,7 +617,7 @@ export default function ReviewerPicker({ )} {selected.length === 0 && ( - none — defaults to Copilot + none — follows your default AI provider )} diff --git a/client/src/components/cos/ReviewerPicker.test.jsx b/client/src/components/cos/ReviewerPicker.test.jsx index c21fd2de12..c40ab85c2b 100644 --- a/client/src/components/cos/ReviewerPicker.test.jsx +++ b/client/src/components/cos/ReviewerPicker.test.jsx @@ -41,7 +41,7 @@ describe('ReviewerPicker', () => { it('shows the empty-state hint when no reviewers are selected', () => { render( {}} />); - expect(screen.getByText(/none — defaults to Copilot/)).toBeInTheDocument(); + expect(screen.getByText(/none — follows your default AI provider/)).toBeInTheDocument(); }); it('de-dupes a malformed list with duplicates (order-preserving)', () => { @@ -308,11 +308,50 @@ describe('ReviewerPicker', () => { expect(screen.getByRole('option', { name: 'qwen2.5-coder:32b' })).toBeInTheDocument(); }); - it('renders a CLI reviewer as a free-text input so an env-specific id can be typed', () => { + it('renders a CLI reviewer as a dropdown of its catalog', () => { render( {}} />); + expect(screen.getByLabelText('Model for Claude').tagName).toBe('SELECT'); + expect(screen.getByRole('option', { name: 'claude-tier-a' })).toBeInTheDocument(); + }); + + it('offers a CLI reviewer a Custom… escape that swaps in a free-text input', () => { + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getByLabelText('Model for Claude'), { target: { value: '[custom]' } }); // An Ollama-backed / Bedrock-form claude id can't be enumerated, so the - // control must accept a typed value rather than only a pick. + // escape must accept a typed value rather than only a pick. expect(screen.getByLabelText('Model for Claude').tagName).toBe('INPUT'); + // The sentinel is a UI mode, not an id — it must never be stored as a pin. + expect(onChange).not.toHaveBeenCalled(); + }); + + it('does not offer the Custom… escape to a probed local backend', () => { + render( {}} />); + // Ollama's list is the daemon's own answer: an id it doesn't list isn't installed. + expect(screen.queryByRole('option', { name: 'Custom…' })).not.toBeInTheDocument(); + }); + + it('leaving an empty Custom… field returns the cell to the dropdown', () => { + render( {}} />); + fireEvent.change(screen.getByLabelText('Model for Claude'), { target: { value: '[custom]' } }); + fireEvent.blur(screen.getByLabelText('Model for Claude')); + expect(screen.getByLabelText('Model for Claude').tagName).toBe('SELECT'); + }); + + it('keeps the Custom… input mounted while a typed id is being edited', () => { + render( {}} />); + fireEvent.change(screen.getByLabelText('Model for Claude'), { target: { value: '[custom]' } }); + // Clearing the field to retype must not swap the control out mid-edit. + fireEvent.change(screen.getByLabelText('Model for Claude'), { target: { value: 'x' } }); + fireEvent.change(screen.getByLabelText('Model for Claude'), { target: { value: '' } }); + expect(screen.getByLabelText('Model for Claude').tagName).toBe('INPUT'); + }); + + it('keeps a pin outside the catalog editable rather than unpickable', () => { + render( {}} />); + const control = screen.getByLabelText('Model for Claude'); + expect(control.tagName).toBe('INPUT'); + expect(control).toHaveValue('llama3.1:70b'); }); it('falls back to free-text when no options resolved (a closed empty select would be dead)', () => { @@ -358,6 +397,7 @@ describe('ReviewerPicker', () => { it('treats a whitespace-only entry as a clear, not a pin', () => { const onChange = vi.fn(); render(); + fireEvent.change(screen.getByLabelText('Model for Codex'), { target: { value: '[custom]' } }); fireEvent.change(screen.getByLabelText('Model for Codex'), { target: { value: ' ' } }); expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ reviewerModels: {} })); }); @@ -417,6 +457,7 @@ describe('ReviewerPicker', () => { // `foo]~opt` would close the selector early and leave slashdo reading the // rest as a suffix; the server drops such an id, so accepting it here would // show a pin that never persists. + fireEvent.change(screen.getByLabelText('Model for Codex'), { target: { value: '[custom]' } }); fireEvent.change(screen.getByLabelText('Model for Codex'), { target: { value: 'foo]~opt' } }); expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ reviewerModels: { codex: 'foo~opt' } })); }); @@ -424,6 +465,7 @@ describe('ReviewerPicker', () => { it('keeps a space in a typed id (slashdo selectors are free-form)', () => { const onChange = vi.fn(); render(); + fireEvent.change(screen.getByLabelText('Model for Claude'), { target: { value: '[custom]' } }); fireEvent.change(screen.getByLabelText('Model for Claude'), { target: { value: 'Some Model (High)' } }); expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ reviewerModels: { claude: 'Some Model (High)' } })); }); diff --git a/client/src/components/settings/CodeReviewersTab.jsx b/client/src/components/settings/CodeReviewersTab.jsx index 794f655ece..c405ec97b3 100644 --- a/client/src/components/settings/CodeReviewersTab.jsx +++ b/client/src/components/settings/CodeReviewersTab.jsx @@ -105,7 +105,7 @@ export default function CodeReviewersTab() {

Code Review Defaults

- Default Review Loop reviewer chain — used by ad-hoc CoS tasks and task-type schedules that haven't pinned their own. Local-LLM reviewers route the diff through PortOS's local code-review endpoint; the {CLI_REVIEWER_LIST} reviewers invoke their CLI directly. Each runs the model pinned on its row (Claude also supports an Ollama-backed CLI for local-only setups — type one of your installed Ollama models). + Default Review Loop reviewer chain — used by ad-hoc CoS tasks and task-type schedules that haven't pinned their own. Leave it empty and reviews follow your default AI provider, at its own model and reasoning effort. Local-LLM reviewers route the diff through PortOS's local code-review endpoint; the {CLI_REVIEWER_LIST} reviewers invoke their CLI directly. Each runs the model picked on its row — choose Custom… to type an id its catalog doesn't list, such as an installed Ollama model for an Ollama-backed Claude.

{loadError && ( diff --git a/client/src/hooks/useReviewerModelOptions.js b/client/src/hooks/useReviewerModelOptions.js index a4e7bba48d..b1e8b2de92 100644 --- a/client/src/hooks/useReviewerModelOptions.js +++ b/client/src/hooks/useReviewerModelOptions.js @@ -31,10 +31,12 @@ const PROBED_LOCAL_BACKENDS = LOCAL_LLM_BACKENDS.map((b) => b.id); * the installed Ollama ids (an Ollama-backed `claude` CLI, where `--model` selects * the local model). Deduped, order-preserving. * - * `freeText` marks a reviewer whose picker must accept a typed id, not just a - * pick: an Ollama-backed `claude` can run any locally-installed id, and a + * `freeText` marks a reviewer whose picker must ALSO accept a typed id, not only + * a pick: an Ollama-backed `claude` can run any locally-installed id, and a * Bedrock/Vertex install needs its environment's own id form, neither of which a - * catalog can enumerate. Consumers render a `` for those. + * catalog can enumerate. Those still render a dropdown of the catalog — the flag + * adds a "Custom…" escape to it (see `ReviewerPicker`'s Model cell); a reviewer + * marked `false` gets a closed list, because its options came from a live probe. * * `unavailable` distinguishes "backend is down" from "backend has no models" so * the empty state can say the useful thing. Absent = not probed (every reviewer diff --git a/server/lib/reviewerConfig.js b/server/lib/reviewerConfig.js index a2015cf11e..6b13e18781 100644 --- a/server/lib/reviewerConfig.js +++ b/server/lib/reviewerConfig.js @@ -11,7 +11,7 @@ * This module must stay Zod-free — it is pure reviewer domain vocabulary. */ import { isPlainObject } from './objects.js'; -import { EFFORT_LEVELS, effortLevelsForProvider, buildEffortArgs, foldCursorEffortIntoModel, splitAntigravityModel } from './providerModels.js'; +import { EFFORT_LEVELS, effortLevelsForProvider, buildEffortArgs, foldCursorEffortIntoModel, splitAntigravityModel, commandBasename, isConfiguredDefaultModel } from './providerModels.js'; import { ANTIGRAVITY_COMMAND } from './antigravity.js'; import { CURSOR_COMMAND } from './cursor.js'; @@ -1217,3 +1217,67 @@ export function buildReviewWithArgs(reviewers, { if (reviewerApplies && hasNonCopilot) parts.push('--reviewer-applies'); return parts.join(' '); } + +/** + * The reviewer slug an AI provider config would review as, or `null` when the + * provider is nothing the Review Loop can run (a hosted API provider with no + * spawnable CLI, an unrecognized binary). + * + * Two ways in, matching how the two reviewer kinds are actually identified: + * a local-LLM reviewer is named by PROVIDER ID (`ollama`/`lmstudio`/`mtplx` — + * it has no binary; `POST /api/code-review/local` talks to the daemon), and a + * CLI reviewer is named by the BINARY its provider spawns, looked up through + * `REVIEWER_CLI_BINARIES` so the slug↔executable mapping stays in one table + * (`antigravity` is the stored slug, `agy` the command — see that constant). + * + * An Ollama/SGLang-backed `claude` or `opencode` wrapper resolves to the + * `claude` / `opencode` reviewer on purpose: the reviewer runs the same binary + * against the same environment, and its model pin is free text precisely so a + * locally-served id can be named. + * + * @param {{id?:string, command?:string}|null|undefined} provider + * @returns {string|null} + */ +export function reviewerForProvider(provider) { + if (!isPlainObject(provider)) return null; + const id = typeof provider.id === 'string' ? provider.id.trim().toLowerCase() : ''; + if (LOCAL_LLM_REVIEWERS.includes(id)) return id; + const command = commandBasename(provider.command); + if (!command) return null; + return Object.entries(REVIEWER_CLI_BINARIES).find(([, binary]) => binary === command)?.[0] || null; +} + +/** + * Code Review Defaults derived from the install's DEFAULT AI provider — the + * reviewer chain an install gets before anyone opens Settings › Code Reviewers. + * + * The historical fallback was a hardcoded `['copilot']`, which is wrong on two + * counts: an install with no GitHub Copilot subscription gets a review that + * never arrives, and an install that has already told PortOS which agent it + * wants to run gets a different one for review with no way to have known. So + * the fallback follows the active provider instead — same vendor, same model, + * same reasoning effort — and only falls back to `DEFAULT_REVIEWERS` when the + * provider maps to no reviewer at all (a hosted API provider, or none set). + * + * The model is dropped when it is a `*-configured-default` sentinel: that + * string is a marker meaning "whatever the CLI is configured for", not an id + * the reviewer's `--model` could take. The effort is dropped when it falls + * outside that reviewer's own ladder, the same drop-don't-clamp rule + * `normalizeReviewerEffort` applies everywhere else. + * + * Returns `null` (not a partial object) when there is nothing to derive, so the + * caller can tell "no provider-derived default" from "derived, with no pins". + * + * @param {{id?:string, command?:string, defaultModel?:string, effort?:string}|null|undefined} provider + * @returns {{reviewer: string, model: string|null, effort: string|null}|null} + */ +export function codeReviewDefaultsFromProvider(provider) { + const reviewer = reviewerForProvider(provider); + if (!reviewer) return null; + const rawModel = provider.defaultModel; + return { + reviewer, + model: isConfiguredDefaultModel(rawModel) ? null : (normalizeReviewerModel(rawModel, reviewer) ?? null), + effort: normalizeReviewerEffort(provider.effort, reviewer) ?? null, + }; +} diff --git a/server/lib/reviewerConfig.test.js b/server/lib/reviewerConfig.test.js index 2a2066bf4b..e14d087148 100644 --- a/server/lib/reviewerConfig.test.js +++ b/server/lib/reviewerConfig.test.js @@ -38,6 +38,8 @@ import { MAX_REVIEW_USERNAMES, MAX_REVIEWER_MAX_ROUNDS, normalizeReviewUsernames, + reviewerForProvider, + codeReviewDefaultsFromProvider, } from './reviewerConfig.js'; // The Zod half of the old cosValidation.js — these cases assert that a reviewer // pin survives the schema that persists it, so they need both modules. @@ -726,3 +728,64 @@ describe('claim reviewer round-trip (prompt CSV ↔ persisted metadata)', () => expect(reviewerConfigMetadata({ reviewers: ['bogus'] })).toEqual({}); }); }); + +describe('reviewerForProvider', () => { + it('names a CLI reviewer by the binary its provider spawns', () => { + expect(reviewerForProvider({ id: 'claude-code-tui', command: 'claude' })).toBe('claude'); + expect(reviewerForProvider({ id: 'codex', command: '/opt/homebrew/bin/codex' })).toBe('codex'); + // The stored slug is `antigravity`; the executable is `agy`. + expect(reviewerForProvider({ id: 'antigravity-cli', command: 'agy' })).toBe('antigravity'); + expect(reviewerForProvider({ id: 'cursor-cli', command: 'cursor-agent' })).toBe('cursor'); + }); + + it('names a local-LLM reviewer by provider id (it has no binary at all)', () => { + expect(reviewerForProvider({ id: 'ollama', type: 'api' })).toBe('ollama'); + expect(reviewerForProvider({ id: 'lmstudio', type: 'api' })).toBe('lmstudio'); + expect(reviewerForProvider({ id: 'mtplx', type: 'api' })).toBe('mtplx'); + }); + + it('follows the wrapper binary for a locally-served CLI provider', () => { + // A locally-served `claude` reviews as `claude`: same binary, same env, and + // its model pin is free text precisely so a local id can be named. + expect(reviewerForProvider({ id: 'claude-ollama', command: 'claude', ollamaBacked: true })).toBe('claude'); + expect(reviewerForProvider({ id: 'opencode-vllm', command: 'opencode' })).toBe('opencode'); + }); + + it('returns null for a provider the Review Loop cannot run', () => { + // A hosted API provider spawns nothing and is not a local backend. + expect(reviewerForProvider({ id: 'openrouter', type: 'api' })).toBeNull(); + expect(reviewerForProvider({ id: 'mystery', command: 'some-unknown-agent' })).toBeNull(); + expect(reviewerForProvider(null)).toBeNull(); + expect(reviewerForProvider('claude')).toBeNull(); + }); +}); + +describe('codeReviewDefaultsFromProvider', () => { + it('carries the provider model and effort onto its reviewer', () => { + expect(codeReviewDefaultsFromProvider({ + id: 'claude-code', command: 'claude', defaultModel: 'claude-opus-5', effort: 'high', + })).toEqual({ reviewer: 'claude', model: 'claude-opus-5', effort: 'high' }); + }); + + it('drops a configured-default sentinel — it is a marker, not a model id', () => { + const out = codeReviewDefaultsFromProvider({ id: 'grok-cli', command: 'grok', defaultModel: 'grok-configured-default' }); + expect(out).toEqual({ reviewer: 'grok', model: null, effort: null }); + }); + + it('drops an effort the reviewer\'s own ladder rejects', () => { + // agy really does reject `--effort max`; reviewing at a silently different + // level than the one configured is worse than using its own default. + expect(codeReviewDefaultsFromProvider({ + id: 'antigravity-cli', command: 'agy', defaultModel: 'gemini-3.6-flash', effort: 'max', + }).effort).toBeNull(); + // A provider with no effort set at all leaves the reviewer at its own default. + expect(codeReviewDefaultsFromProvider({ id: 'codex', command: 'codex' }).effort).toBeNull(); + // ...as does a value that is not an effort level in the first place. + expect(codeReviewDefaultsFromProvider({ id: 'codex', command: 'codex', effort: 'turbo' }).effort).toBeNull(); + }); + + it('returns null when the provider maps to no reviewer', () => { + expect(codeReviewDefaultsFromProvider({ id: 'openrouter', type: 'api' })).toBeNull(); + expect(codeReviewDefaultsFromProvider(null)).toBeNull(); + }); +}); diff --git a/server/services/codeReview.js b/server/services/codeReview.js index dcf3a1367c..baca0b4e26 100644 --- a/server/services/codeReview.js +++ b/server/services/codeReview.js @@ -32,6 +32,7 @@ import { normalizeReviewerMaxRounds, resolveReviewerMaxRounds, reviewerEffortsFromDefaults, + codeReviewDefaultsFromProvider, resolveReviewerPins, normalizeReviewerEffort, prioritizeToolFreeReviewers, @@ -39,6 +40,7 @@ import { MODEL_SELECTABLE_REVIEWERS, } from '../lib/validation.js' import { getSettings, settingsEvents } from './settings.js' +import { getActiveProvider } from './providers.js' import { getBaseUrl as getLmStudioBaseUrl } from './lmStudioManager.js' import { getBaseUrl as getOllamaBaseUrl } from './ollamaManager.js' @@ -66,22 +68,51 @@ export function isLocalLlmReviewer(backend) { return LOCAL_LLM_REVIEWERS.includes(backend) } +/** + * The reviewer chain the user actually configured, with aliases mapped and + * unknown enum values dropped — empty when they have configured none. + * + * Its own function because "did the user choose a chain?" is asked twice and the + * two answers must agree exactly: `pickCodeReviewDefaults` uses it to decide + * whether to derive defaults from the active AI provider, and + * `getCodeReviewDefaults` uses it to decide whether it may memoize the result. + * A settings.json holding only junk (`reviewers: ['bogus']`) has configured + * nothing, and both callers have to see that the same way. + */ +function configuredReviewers(settings) { + const raw = settings && typeof settings === 'object' ? settings.codeReview : null + if (!Array.isArray(raw?.reviewers)) return [] + return Array.from(new Set(raw.reviewers.map((r) => REVIEWER_ALIASES[r] || r).filter((r) => REVIEWER_VALUES.includes(r)))) +} + /** * Resolve the global Code Review Defaults from `settings.codeReview`, falling - * back to the hardcoded `['copilot']` / `all` / `false` defaults when the user - * hasn't configured them yet. Filters out invalid enum values so a hand-edited - * settings.json can't smuggle in bogus reviewer names. Returns a value-only - * shape (no I/O) so the spawner and `GET /api/code-review/defaults` can share. + * back to the install's own defaults when the user hasn't configured them yet. + * Filters out invalid enum values so a hand-edited settings.json can't smuggle + * in bogus reviewer names. Returns a value-only shape (no I/O) so the spawner + * and `GET /api/code-review/defaults` can share. + * + * `activeProvider` is the install's DEFAULT AI provider (the caller's, because + * this function does no I/O). With no configured reviewer chain the defaults + * follow that provider — its reviewer slug, its default model, its reasoning + * effort — rather than the hardcoded `copilot`, which reviews through a GitHub + * subscription the install may not have and ignores the agent the user already + * chose. `DEFAULT_REVIEWERS` remains the last resort, for a provider that maps + * to no reviewer (a hosted API provider) or none being set at all. + * + * A provider-derived model/effort is only a DEFAULT: a stored `Model` + * / `Effort` scalar still wins, so pinning one reviewer's model does + * not silently un-derive the rest. */ -export function pickCodeReviewDefaults(settings) { +export function pickCodeReviewDefaults(settings, { activeProvider = null } = {}) { const raw = settings && typeof settings === 'object' ? settings.codeReview : null const effortDefaults = reviewerEffortsFromDefaults(raw) - const reviewersIn = Array.isArray(raw?.reviewers) ? raw.reviewers : null - const reviewers = reviewersIn - ? Array.from(new Set(reviewersIn.map((r) => REVIEWER_ALIASES[r] || r).filter((r) => REVIEWER_VALUES.includes(r)))) - : [] + const reviewers = configuredReviewers(settings) + // Only consulted when the user has configured no chain of their own — a saved + // chain is an explicit choice and must not be re-derived from the provider. + const derived = reviewers.length ? null : codeReviewDefaultsFromProvider(activeProvider) return { - reviewers: reviewers.length ? reviewers : [...DEFAULT_REVIEWERS], + reviewers: reviewers.length ? reviewers : (derived ? [derived.reviewer] : [...DEFAULT_REVIEWERS]), // Arbitrary GitHub reviewer usernames appended to `--review-with` to gate the // merge. Normalized so a hand-edited settings.json can't smuggle in unsafe // tokens. Empty array = none configured (distinct from the copilot fallback @@ -110,7 +141,8 @@ export function pickCodeReviewDefaults(settings) { ...Object.fromEntries( MODEL_SELECTABLE_REVIEWERS.map((reviewer) => { const stored = raw?.[`${reviewer}Model`] - return [`${reviewer}Model`, typeof stored === 'string' && stored ? stored : null] + if (typeof stored === 'string' && stored) return [`${reviewer}Model`, stored] + return [`${reviewer}Model`, derived?.reviewer === reviewer ? derived.model : null] }) ), // Per-reviewer reasoning-effort defaults. Unlike the model scalars above these @@ -124,7 +156,10 @@ export function pickCodeReviewDefaults(settings) { // (an open-coded check missed the normalizer's case-folding, so a settings.json // holding `"High"` resolved one way here and another there). ...Object.fromEntries( - EFFORT_SELECTABLE_REVIEWERS.map((reviewer) => [`${reviewer}Effort`, effortDefaults[reviewer] ?? null]) + EFFORT_SELECTABLE_REVIEWERS.map((reviewer) => [ + `${reviewer}Effort`, + effortDefaults[reviewer] ?? (derived?.reviewer === reviewer ? derived.effort : null), + ]) ), } } @@ -139,16 +174,29 @@ export function pickCodeReviewDefaults(settings) { * cache invalidates on any `settings:updated` event so the panel's save * takes effect immediately without a restart. */ +let cachedSettings = null let cachedDefaults = null -settingsEvents.on('settings:updated', () => { cachedDefaults = null }) +settingsEvents.on('settings:updated', () => { cachedSettings = null; cachedDefaults = null }) /** Test-only: reset the memoized defaults cache to its uninitialized sentinel. */ -export function __resetCodeReviewDefaultsCache() { cachedDefaults = null } +export function __resetCodeReviewDefaultsCache() { cachedSettings = null; cachedDefaults = null } export async function getCodeReviewDefaults() { if (cachedDefaults) return cachedDefaults - cachedDefaults = pickCodeReviewDefaults(await getSettings()) - return cachedDefaults + if (!cachedSettings) cachedSettings = await getSettings() + const configured = configuredReviewers(cachedSettings).length > 0 + // `getActiveProvider` needs an initialized AI toolkit, which an early-boot + // caller (or a unit-test process) may not have — a failed read just means no + // provider-derived default, never a failed resolve. It reads the toolkit's own + // in-memory provider cache, so an unconfigured install pays no disk I/O for it. + const activeProvider = configured ? null : await getActiveProvider().catch(() => null) + const defaults = pickCodeReviewDefaults(cachedSettings, { activeProvider }) + // Only a settings-derived answer is memoized: `settings:updated` invalidates it + // completely. A provider-derived one has no such event — the active provider + // lives in its own store — so it is re-resolved per call rather than pinned to + // whichever vendor happened to be active when the cache was first filled. + if (configured) cachedDefaults = defaults + return defaults } /** diff --git a/server/services/codeReview.test.js b/server/services/codeReview.test.js index 2d3087b8c4..f6f545ce53 100644 --- a/server/services/codeReview.test.js +++ b/server/services/codeReview.test.js @@ -13,6 +13,13 @@ vi.mock('./settings.js', () => ({ // Same one-liner stub for the two backend managers — `getCodeReviewDefaults` // + `pickCodeReviewDefaults` don't touch them, only `runLocalCodeReview` // does, and those tests stub `global.fetch` directly. +// The install's default AI provider, which the unconfigured reviewer fallback +// now follows. Mutable holder so each test picks the vendor it is asserting on; +// `null` reproduces an install with no provider (or an uninitialized toolkit). +const mockedActiveProvider = { current: null } +vi.mock('./providers.js', () => ({ + getActiveProvider: () => Promise.resolve(mockedActiveProvider.current), +})) vi.mock('./lmStudioManager.js', () => ({ getBaseUrl: () => 'http://localhost:1234' })) vi.mock('./ollamaManager.js', () => ({ getBaseUrl: () => 'http://localhost:11434' })) // Reviewer-CLI-installed probe: stub the shared execFile-based helper so the @@ -45,6 +52,7 @@ const testDeps = { describe('codeReview helpers', () => { afterEach(() => { mockedSettings.current = {} + mockedActiveProvider.current = null __resetCodeReviewDefaultsCache() __resetReviewerCliInstalledCache() commandExistsMock.impl = async () => true @@ -184,6 +192,64 @@ describe('codeReview helpers', () => { it('defaults usernames to an empty array when absent', () => { expect(pickCodeReviewDefaults({ codeReview: { reviewers: ['copilot'] } }).usernames).toEqual([]) }) + + describe('unconfigured fallback follows the default AI provider', () => { + const claudeProvider = { id: 'claude-code-tui', command: 'claude', defaultModel: 'claude-opus-5', effort: 'high' } + + it('reviews with the active provider, its model, and its effort', () => { + const out = pickCodeReviewDefaults(null, { activeProvider: claudeProvider }) + expect(out.reviewers).toEqual(['claude']) + expect(out.claudeModel).toBe('claude-opus-5') + expect(out.claudeEffort).toBe('high') + // Only the derived reviewer gets pins — the rest stay at "its own default". + expect(out.codexModel).toBeNull() + expect(out.codexEffort).toBeNull() + }) + + it('leaves a configured chain alone', () => { + const out = pickCodeReviewDefaults( + { codeReview: { reviewers: ['copilot'] } }, + { activeProvider: claudeProvider } + ) + expect(out.reviewers).toEqual(['copilot']) + expect(out.claudeModel).toBeNull() + }) + + it('lets a stored pin win over the provider-derived one', () => { + const out = pickCodeReviewDefaults( + { codeReview: { claudeModel: 'claude-sonnet-5', claudeEffort: 'low' } }, + { activeProvider: claudeProvider } + ) + expect(out.reviewers).toEqual(['claude']) + expect(out.claudeModel).toBe('claude-sonnet-5') + expect(out.claudeEffort).toBe('low') + }) + + it('keeps copilot for a provider that maps to no reviewer', () => { + // A hosted API provider spawns no binary and is not a local backend. + const out = pickCodeReviewDefaults(null, { activeProvider: { id: 'openrouter', type: 'api', defaultModel: 'stealth/ox-alpha' } }) + expect(out.reviewers).toEqual(['copilot']) + }) + + it('drops a configured-default sentinel rather than pinning it as a model', () => { + const out = pickCodeReviewDefaults(null, { + activeProvider: { id: 'antigravity-cli', command: 'agy', defaultModel: 'antigravity-configured-default' }, + }) + expect(out.reviewers).toEqual(['antigravity']) + // The sentinel means "whatever agy is configured for" — `agy --model + // antigravity-configured-default` is not a runnable invocation. + expect(out.antigravityModel).toBeNull() + }) + + it('drops an effort outside the derived reviewer\'s own ladder', () => { + // agy rejects `--effort max`, so a provider pinned there must not + // silently review at a level its CLI refuses. + const out = pickCodeReviewDefaults(null, { + activeProvider: { id: 'antigravity-cli', command: 'agy', defaultModel: 'gemini-3.6-flash', effort: 'max' }, + }) + expect(out.antigravityEffort).toBeNull() + }) + }) }) describe('getCodeReviewDefaults', () => { @@ -196,6 +262,14 @@ describe('codeReview helpers', () => { expect(out.ollamaModel).toBe('codellama') expect(out.stopMode).toBe('all') }) + + it('falls back to the active provider when nothing is configured', async () => { + mockedSettings.current = {} + mockedActiveProvider.current = { id: 'codex', command: 'codex', defaultModel: 'gpt-5.6-terra' } + const out = await getCodeReviewDefaults() + expect(out.reviewers).toEqual(['codex']) + expect(out.codexModel).toBe('gpt-5.6-terra') + }) }) describe('getReviewerCliInstalled', () => {