From e0fe09fdaf55e9cda75266d848ca5e9264428fe7 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Mon, 27 Jul 2026 01:51:30 -0700 Subject: [PATCH 1/4] feat: give every reviewer-picker surface a per-reviewer Provider/Model/Optional/Max table (#3133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code Review Defaults rendered reviewers as a chip row plus a stack of panel-local per-backend model selects, so per-reviewer model choice existed only on that one page — TaskAddForm, ScheduleTab, and SlashDoRunDrawer all reuse ReviewerPicker but had no way to set a reviewer's model. Move the model picker into ReviewerPicker as a Model column and lay each reviewer out as a row: Provider | Model | Optional | Max Iterations. The picker stays fetch-free — callers pass resolved option lists via the new useReviewerModelOptions hook, so all four surfaces offer the same models. The pin now travels per task, not just per install: `reviewerModels` is a token-keyed map alongside `reviewerMaxRounds` (same normalizer/resolver/ task-over-default precedence), emitted as slashdo's `[]` selector between the slug and the `~opt`/`~max` suffixes. Local-LLM pins ride the same map because /api/code-review/local resolves its default from global settings and can't see a task-level choice — the follow-up prompt names it in the request body instead. settings.codeReview keeps its `Model` scalars as the persisted encoding (it crosses installs); client/src/lib/reviewerModels.js is the one adapter between those and the map. --- .../src/components/apps/SlashDoRunDrawer.jsx | 24 +- .../components/apps/SlashDoRunDrawer.test.jsx | 3 + client/src/components/cos/ReviewerPicker.jsx | 415 +++++++++++++----- .../components/cos/ReviewerPicker.test.jsx | 138 ++++++ client/src/components/cos/TaskAddForm.jsx | 15 +- .../src/components/cos/TaskAddForm.test.jsx | 7 + client/src/components/cos/constants.js | 16 + .../tabs/schedule/GlobalConfigControls.jsx | 13 +- .../providers/CodeReviewDefaultsPanel.jsx | 168 ++----- client/src/hooks/README.md | 1 + client/src/hooks/index.js | 1 + client/src/hooks/useCodeReviewDefaults.jsx | 8 + client/src/hooks/useReviewerModelOptions.js | 94 ++++ client/src/lib/README.md | 1 + client/src/lib/index.js | 1 + client/src/lib/reviewerModels.js | 37 ++ server/lib/cosValidation.js | 163 ++++++- server/lib/validation.test.js | 169 +++++++ server/routes/cosTaskRoutes.js | 4 +- server/services/agentPromptBuilder.js | 49 ++- server/services/agentWorktreeCleanup.js | 14 +- server/services/codeReview.js | 41 +- server/services/codeReview.test.js | 28 +- server/services/cosTaskGenerator.js | 17 +- server/services/cosTaskGenerator.test.js | 15 +- server/services/cosTaskStore.js | 10 +- 26 files changed, 1122 insertions(+), 330 deletions(-) create mode 100644 client/src/hooks/useReviewerModelOptions.js create mode 100644 client/src/lib/reviewerModels.js diff --git a/client/src/components/apps/SlashDoRunDrawer.jsx b/client/src/components/apps/SlashDoRunDrawer.jsx index c716eaf9a..7f524fc69 100644 --- a/client/src/components/apps/SlashDoRunDrawer.jsx +++ b/client/src/components/apps/SlashDoRunDrawer.jsx @@ -1,10 +1,12 @@ -import { useState, useCallback } from 'react'; +import { useState, useCallback, useMemo } from 'react'; import { Loader2, Terminal, Wand2 } from 'lucide-react'; import Drawer from '../Drawer'; import ProviderModelSelector from '../ProviderModelSelector'; import ReviewerPicker from '../cos/ReviewerPicker'; import EffortSelect from '../cos/EffortSelect'; import useProviderModels from '../../hooks/useProviderModels'; +import useReviewerModelOptions from '../../hooks/useReviewerModelOptions'; +import { reviewerModelsFromDefaults } from '../../lib/reviewerModels'; import { CodeReviewDefaultsProvider, useCodeReviewDefaults } from '../../hooks/useCodeReviewDefaults'; import { isProcessProvider } from '../../utils/providers'; import WorkItemPicker from './WorkItemPicker'; @@ -31,6 +33,9 @@ const SELECT_CLASS = 'w-full px-3 py-2 bg-port-bg border border-port-border roun */ function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, onQueued }) { const codeReviewDefaults = useCodeReviewDefaults(); + // Resolved model lists for the reviewer table's Model column (the picker never + // fetches — see its `modelOptions` prop). + const reviewerModelOptions = useReviewerModelOptions(); const { providers, selectedProviderId, selectedModel, availableModels, selectedProvider, setSelectedProviderId, setSelectedModel @@ -41,7 +46,13 @@ function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, o // Seeded from the install's Code Review Defaults for display. `reviewDirty` // gates whether they're SENT — see the component doc. const [review, setReview] = useState(null); - const reviewValue = review ?? codeReviewDefaults; + // The defaults carry per-reviewer models as `Model` scalars; the picker + // takes the token-keyed map, so fold them in for the seeded (untouched) display. + const seededReview = useMemo( + () => ({ ...codeReviewDefaults, reviewerModels: reviewerModelsFromDefaults(codeReviewDefaults) }), + [codeReviewDefaults] + ); + const reviewValue = review ?? seededReview; const [work, setWork] = useState({ mode: 'auto', target: '', issueAuthorFilter: '' }); const [submitting, setSubmitting] = useState(false); @@ -71,7 +82,8 @@ function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, o reviewers: review.reviewers, usernames: review.usernames, optionalReviewers: review.optionalReviewers, - reviewerMaxRounds: review.reviewerMaxRounds + reviewerMaxRounds: review.reviewerMaxRounds, + reviewerModels: review.reviewerModels } : {}) }, { silent: true }).catch((err) => { setSubmitError(err.message || 'Failed to queue the task'); @@ -135,11 +147,13 @@ function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, o usernames={reviewValue.usernames} optionalReviewers={reviewValue.optionalReviewers} reviewerMaxRounds={reviewValue.reviewerMaxRounds} + reviewerModels={reviewValue.reviewerModels} + modelOptions={reviewerModelOptions} // The claim flows substitute a reviewer CSV into their prompt and have // no slashdo flag string, so stop-mode / reviewer-applies can't be honored. showRunFlags={false} - onChange={({ reviewers, usernames, optionalReviewers, reviewerMaxRounds }) => - setReview({ reviewers, usernames, optionalReviewers, reviewerMaxRounds })} + onChange={({ reviewers, usernames, optionalReviewers, reviewerMaxRounds, reviewerModels }) => + setReview({ reviewers, usernames, optionalReviewers, reviewerMaxRounds, reviewerModels })} />

The claim flow opens and merges its own PR, so these reviewers gate that merge (slashdo --review-with). diff --git a/client/src/components/apps/SlashDoRunDrawer.test.jsx b/client/src/components/apps/SlashDoRunDrawer.test.jsx index bdb6ab0e3..9115a8560 100644 --- a/client/src/components/apps/SlashDoRunDrawer.test.jsx +++ b/client/src/components/apps/SlashDoRunDrawer.test.jsx @@ -6,6 +6,8 @@ import SlashDoRunDrawer from './SlashDoRunDrawer'; const api = vi.hoisted(() => ({ getCodeReviewDefaults: vi.fn(), getProviders: vi.fn(), + // Backs the reviewer table's Model column (useReviewerModelOptions). + getLocalLlmStatus: vi.fn(), getAppWorkItems: vi.fn(), createSlashdoTask: vi.fn() })); @@ -30,6 +32,7 @@ describe('SlashDoRunDrawer', () => { vi.clearAllMocks(); api.getCodeReviewDefaults.mockResolvedValue({ reviewers: ['copilot'], usernames: [], optionalReviewers: [] }); api.getProviders.mockResolvedValue({ providers: [] }); + api.getLocalLlmStatus.mockResolvedValue({ ollama: { models: [] }, lmstudio: { models: [] } }); api.getAppWorkItems.mockResolvedValue({ tracker: 'github', issueAuthorFilter: 'self', diff --git a/client/src/components/cos/ReviewerPicker.jsx b/client/src/components/cos/ReviewerPicker.jsx index 6ad404c93..70792d14c 100644 --- a/client/src/components/cos/ReviewerPicker.jsx +++ b/client/src/components/cos/ReviewerPicker.jsx @@ -6,6 +6,8 @@ import { DEFAULT_REVIEW_STOP_MODE, MAX_REVIEW_USERNAMES, MAX_REVIEWER_MAX_ROUNDS, + MAX_REVIEWER_MODEL_LENGTH, + MODEL_SELECTABLE_REVIEWERS, cleanReviewUsername, normalizeReviewUsernames } from './constants'; @@ -14,21 +16,38 @@ const labelFor = (value) => REVIEWER_OPTIONS.find(o => o.value === value)?.label const normalizeReviewerValue = (value) => value === 'gemini' ? 'antigravity' : value; /** - * Ordered multi-reviewer picker. Click a reviewer to append it (run order = + * Ordered multi-reviewer picker, rendered as one row per reviewer with the four + * per-reviewer controls as columns: **Provider | Model | Optional | Max + * Iterations** (#3133). Click a reviewer in the Add row to append it (run order = * click order), reorder with the arrows, remove with ✕. Maps to slashdo's * `--review-with a,b,c` plus the stop-mode / `--reviewer-applies` flags. * - * A second "GitHub reviewers" row collects arbitrary usernames (e.g. + * A second "GitHub reviewers" table collects arbitrary usernames (e.g. * `@CodeReviewbot`) requested as PR reviewers to gate the merge — appended to - * `--review-with` as `@user` tokens after the keyed reviewers. + * `--review-with` as `@user` tokens after the keyed reviewers. Those rows have no + * Model cell: a username reviewer is a human or a GitHub App, not a model-taking + * backend (slashdo rejects `@login[…]` for the same reason). * - * Each chip carries two per-entry slashdo suffix controls: the `~opt` - * non-blocking badge and a numeric `~max=` round cap (blank = slashdo's - * built-in default, `0` = loop until clean). + * Per-row controls, each mapping to one slashdo per-entry token feature: + * - **Model** → the `[]` selector (or, for a CLI reviewer the follow-up + * agent invokes directly, ` --model `). Only rendered for + * MODEL_SELECTABLE_REVIEWERS. The option lists are OWNED BY THE CALLER (see + * `modelOptions`) so this component does no fetching. + * - **Optional** → the `~opt` non-blocking marker. + * - **Max Iterations** → the numeric `~max=` round cap (blank = slashdo's + * built-in default, `0` = loop until clean). * * Controlled: emits the full next shape via onChange so the parent can store - * `reviewers` / `usernames` / `reviewerMaxRounds` / `reviewStopMode` / - * `reviewerApplies` however it persists them. + * `reviewers` / `usernames` / `optionalReviewers` / `reviewerModels` / + * `reviewerMaxRounds` / `reviewStopMode` / `reviewerApplies` however it persists + * them. + * + * `modelOptions` is the resolved model-picker data, shaped like + * `useReviewerModelOptions()`'s return: `{ optionsByReviewer, freeText, + * unavailable, loaded }`. Callers keep owning their own + * `api.getLocalLlmStatus` / `api.getProviders` fetches (that's what the hook is + * for) — passing nothing degrades every Model cell to a free-text input, which is + * still fully usable, rather than hiding the column. * * `showRunFlags={false}` hides the stop-mode select and the "reviewer applies * fixes" checkbox for surfaces that can't honor them — the `/do:next` claim @@ -41,6 +60,8 @@ export default function ReviewerPicker({ usernames = [], optionalReviewers = [], reviewerMaxRounds = {}, + reviewerModels = {}, + modelOptions = null, stopMode = DEFAULT_REVIEW_STOP_MODE, reviewerApplies = false, onChange, @@ -68,27 +89,39 @@ export default function ReviewerPicker({ const optionalSet = new Set(optionalTokens.map(t => t.toLowerCase())); const isOptional = (token) => optionalSet.has(token.toLowerCase()); const withoutToken = (token) => optionalTokens.filter(t => t.toLowerCase() !== token.toLowerCase()); + // Shared shape for the two token-keyed maps below (`~max=` caps and model + // pins). Both key on the same emitted `--review-with` token and both need the + // same case-insensitive read / key-preserving delete, so the lookup helpers are + // generated once rather than written twice. + const asMap = (value) => (value && typeof value === 'object' && !Array.isArray(value)) ? value : {}; + const keyedLookup = (map) => ({ + get: (token) => { + const key = Object.keys(map).find(k => k.toLowerCase() === token.toLowerCase()); + return key === undefined ? undefined : map[key]; + }, + without: (token) => Object.fromEntries( + Object.entries(map).filter(([k]) => k.toLowerCase() !== token.toLowerCase()) + ) + }); // Per-reviewer `~max=` round caps, keyed by the same emitted token. Absent // key = no cap requested (slashdo's built-in default stands); `0` = loop until // clean. The two must never collapse, so the input renders '' for absent and // clearing it DELETES the key rather than writing 0 (server // `normalizeReviewerMaxRounds` owns the authoritative shape). - const maxRoundsMap = (reviewerMaxRounds && typeof reviewerMaxRounds === 'object' && !Array.isArray(reviewerMaxRounds)) - ? reviewerMaxRounds - : {}; - const maxRoundsFor = (token) => { - const key = Object.keys(maxRoundsMap).find(k => k.toLowerCase() === token.toLowerCase()); - return key === undefined ? undefined : maxRoundsMap[key]; - }; - const maxRoundsWithout = (token) => Object.fromEntries( - Object.entries(maxRoundsMap).filter(([k]) => k.toLowerCase() !== token.toLowerCase()) - ); + const maxRoundsMap = asMap(reviewerMaxRounds); + const maxRounds = keyedLookup(maxRoundsMap); + // Per-reviewer model pins, same token keying. Absent key = "let that reviewer + // pick its own default", which is NOT an empty string — so clearing the field + // DELETES the key rather than persisting `''` (a `--model ` with no id). + const modelsMap = asMap(reviewerModels); + const models = keyedLookup(modelsMap); const emit = (next) => onChange?.({ reviewers: selected, usernames: selectedUsernames, optionalReviewers: optionalTokens, reviewerMaxRounds: maxRoundsMap, + reviewerModels: modelsMap, stopMode, reviewerApplies, ...next @@ -109,17 +142,30 @@ export default function ReviewerPicker({ // budget, never make it unlimited, and it matches the input's `max`. const setMaxRounds = (token, raw) => { if (raw === '') { - emit({ reviewerMaxRounds: maxRoundsWithout(token) }); + emit({ reviewerMaxRounds: maxRounds.without(token) }); return; } const parsed = Number(raw); if (!Number.isInteger(parsed) || parsed < 0) return; const capped = Math.min(parsed, MAX_REVIEWER_MAX_ROUNDS); - emit({ reviewerMaxRounds: { ...maxRoundsWithout(token), [token]: capped } }); + emit({ reviewerMaxRounds: { ...maxRounds.without(token), [token]: capped } }); }; - // The `~opt` non-blocking toggle rendered on every reviewer/username chip. - // `subject` is the human name used in the aria-label; `title` is the (chip- + // Blank (or whitespace-only) clears the pin so the reviewer falls back to its + // own default — the DELETE, not an empty-string write, because `''` is not a + // model id the reviewer could run. Mirrors the server normalizer, which drops a + // blank value rather than persisting it. + const setModel = (token, raw) => { + const model = raw.trim(); + if (!model) { + emit({ reviewerModels: models.without(token) }); + return; + } + emit({ reviewerModels: { ...models.without(token), [token]: model } }); + }; + + // The `~opt` non-blocking toggle rendered on every reviewer/username row. + // `subject` is the human name used in the aria-label; `title` is the (row- // kind-specific) hover copy the caller resolves. const renderOptToggle = (token, subject, title) => ( - - {renderOptToggle(value, labelFor(value), isOptional(value) - ? `${labelFor(value)} is non-blocking (~opt): an inconclusive verdict from it won't block the merge. Click to make it blocking.` - : `${labelFor(value)} gates the merge. Click to make it non-blocking (~opt) — its inconclusive verdicts won't block the merge (a hard failure still does).`)} - {renderMaxRounds(value, labelFor(value))} - - - ))} +

+ Reviewers (in order): + {selected.length > 0 && ( + <> + +
+ {selected.map((value, index) => ( +
o.value === value)?.description} + > +
+ {index + 1}. + + +
+ {labelFor(value)} + Model +
{renderModelCell(value)}
+ Optional +
+ {renderOptToggle(value, labelFor(value), isOptional(value) + ? `${labelFor(value)} is non-blocking (~opt): an inconclusive verdict from it won't block the merge. Click to make it blocking.` + : `${labelFor(value)} gates the merge. Click to make it non-blocking (~opt) — its inconclusive verdicts won't block the merge (a hard failure still does).`)} +
+ Max iterations +
{renderMaxRounds(value, labelFor(value))}
+
+ +
+
+ ))} +
+ + )} {selected.length === 0 && ( none — defaults to Copilot )}
{(selected.length > 0 || selectedUsernames.length > 0) && ( - - Tip: the ~opt badge marks a reviewer non-blocking — it still runs and its findings are still fixed, but an inconclusive verdict (timeout / no result) won't block the merge. A hard failure still does. ~max caps that reviewer's review → fix → re-review rounds (blank = its built-in cap, 0 = loop until clean) — a small cap keeps a slow local model affordable. + + Tip: Model pins the model that reviewer runs (blank = its own default). The ~opt badge marks a reviewer non-blocking — it still runs and its findings are still fixed, but an inconclusive verdict (timeout / no result) won't block the merge. A hard failure still does. Max caps that reviewer's review → fix → re-review rounds (blank = its built-in cap, 0 = loop until clean) — a small cap keeps a slow local model affordable. )} @@ -286,37 +450,48 @@ export default function ReviewerPicker({ )} {/* GitHub reviewer usernames — arbitrary PR reviewers (bots/humans) that - gate the merge. Appended to `--review-with` as `@user` tokens. */} + gate the merge. Appended to `--review-with` as `@user` tokens. Same row + grid as the keyed reviewers so the columns line up, minus reorder (their + order is fixed after the keyed list) and minus a Model cell. */}
-
- GitHub reviewers (gate merge): - {selectedUsernames.map((value) => ( - - @ - {value} - {renderOptToggle(`@${value}`, `@${value}`, isOptional(`@${value}`) - ? `@${value} is non-blocking (~opt): if it never submits a review, the merge isn't blocked. Click to make it blocking.` - : `@${value} gates the merge. Click to make it non-blocking (~opt) — a missing/timed-out review from it won't block the merge.`)} - {renderMaxRounds(`@${value}`, `@${value}`)} - - - ))} - {selectedUsernames.length === 0 && ( - none - )} -
+ @ + {value} + Model +
{renderModelCell(`@${value}`)}
+ Optional +
+ {renderOptToggle(`@${value}`, `@${value}`, isOptional(`@${value}`) + ? `@${value} is non-blocking (~opt): if it never submits a review, the merge isn't blocked. Click to make it blocking.` + : `@${value} gates the merge. Click to make it non-blocking (~opt) — a missing/timed-out review from it won't block the merge.`)} +
+ Max iterations +
{renderMaxRounds(`@${value}`, `@${value}`)}
+
+ +
+
+ ))} + + ) : ( + none + )}
@ { })); }); }); + + describe('per-reviewer Model column', () => { + // Shape mirrors useReviewerModelOptions()' return — the caller owns the fetch. + const modelOptions = { + optionsByReviewer: { + ollama: ['qwen2.5-coder:32b', 'codellama'], + lmstudio: ['local-model-a'], + codex: ['gpt-tier-a', 'gpt-tier-b'], + claude: ['claude-tier-a', 'qwen2.5-coder:32b'], + }, + freeText: { codex: true, claude: true, lmstudio: false, ollama: false }, + unavailable: { lmstudio: false, ollama: false }, + loaded: true, + }; + + it('renders a Model control for each model-taking reviewer', () => { + render( {}} />); + expect(screen.getByLabelText('Model for Ollama')).toBeInTheDocument(); + expect(screen.getByLabelText('Model for Codex')).toBeInTheDocument(); + // Copilot has no CLI and takes no model — no control, just the em dash. + expect(screen.queryByLabelText('Model for Copilot')).not.toBeInTheDocument(); + }); + + it('does not render a Model control for a @username reviewer', () => { + render( {}} />); + expect(screen.queryByLabelText('Model for @flaky-bot')).not.toBeInTheDocument(); + }); + + it('renders a local backend as a closed select of its installed ids', () => { + render( {}} />); + const select = screen.getByLabelText('Model for Ollama'); + expect(select.tagName).toBe('SELECT'); + 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', () => { + render( {}} />); + // An Ollama-backed / Bedrock-form claude id can't be enumerated, so the + // control must accept a typed value rather than only a pick. + expect(screen.getByLabelText('Model for Claude').tagName).toBe('INPUT'); + }); + + it('falls back to free-text when no options resolved (a closed empty select would be dead)', () => { + render( {}} />); + expect(screen.getByLabelText('Model for Ollama').tagName).toBe('INPUT'); + }); + + it('shows an existing pin', () => { + render( {}} />); + expect(screen.getByLabelText('Model for Ollama')).toHaveValue('codellama'); + }); + + it('keeps an option for a pin the probe no longer lists', () => { + render( {}} />); + // Without the synthesized option the select would render blank and read as + // "unset" while the value is in fact stored. + expect(screen.getByLabelText('Model for Ollama')).toHaveValue('uninstalled-model'); + expect(screen.getByRole('option', { name: /uninstalled-model \(not installed\)/ })).toBeInTheDocument(); + }); + + it('pins a model for a local reviewer', () => { + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getByLabelText('Model for Ollama'), { target: { value: 'codellama' } }); + expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ reviewerModels: { ollama: 'codellama' } })); + }); + + it('pins a typed model for a CLI reviewer', () => { + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getByLabelText('Model for Codex'), { target: { value: 'gpt-tier-b' } }); + expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ reviewerModels: { codex: 'gpt-tier-b' } })); + }); + + it('clearing the field DELETES the pin rather than writing an empty id', () => { + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getByLabelText('Model for Codex'), { target: { value: '' } }); + // `''` would emit a `--model ` with no id; absent means "its own default". + expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ reviewerModels: {} })); + }); + + 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: ' ' } }); + expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ reviewerModels: {} })); + }); + + it('prunes the pin when its reviewer is removed', async () => { + const onChange = vi.fn(); + const user = userEvent.setup(); + render(); + await user.click(screen.getByLabelText('Remove Ollama')); + expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ reviewers: ['codex'], reviewerModels: {} })); + }); + + it('keeps the model pin, the ~opt toggle, and the cap independent', async () => { + const onChange = vi.fn(); + const user = userEvent.setup(); + render( + + ); + await user.click(screen.getByLabelText('Make Ollama non-blocking')); + expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ + optionalReviewers: ['ollama'], + reviewerMaxRounds: { ollama: 1 }, + reviewerModels: { ollama: 'codellama' } + })); + }); + + it('preserves an untouched pin when another row changes', () => { + const onChange = vi.fn(); + render( + + ); + fireEvent.change(screen.getByLabelText('Model for Ollama'), { target: { value: 'codellama' } }); + expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ + reviewerModels: { codex: 'gpt-tier-a', ollama: 'codellama' } + })); + }); + + it('reads a pin case-insensitively (legacy/hand-edited key casing)', () => { + render( {}} />); + expect(screen.getByLabelText('Model for Codex')).toHaveValue('gpt-tier-a'); + }); + }); }); diff --git a/client/src/components/cos/TaskAddForm.jsx b/client/src/components/cos/TaskAddForm.jsx index c34dc7841..d7374cd5f 100644 --- a/client/src/components/cos/TaskAddForm.jsx +++ b/client/src/components/cos/TaskAddForm.jsx @@ -12,6 +12,8 @@ import { clickableProps } from '../../lib/a11yKeyboard'; import { slashdoLabel } from '../../lib/slashdoCatalog'; import ReviewerPicker from './ReviewerPicker'; import EffortSelect from './EffortSelect'; +import useReviewerModelOptions from '../../hooks/useReviewerModelOptions'; +import { reviewerModelsFromDefaults } from '../../lib/reviewerModels'; export default function TaskAddForm({ providers, apps, onTaskAdded, compact = false, defaultExpanded = false, defaultApp = '' }) { const [newTask, setNewTask] = useState({ description: '', model: '', provider: '', effort: '', app: defaultApp }); @@ -26,6 +28,7 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa const [reviewUsernames, setReviewUsernames] = useState([]); const [optionalReviewers, setOptionalReviewers] = useState([]); const [reviewerMaxRounds, setReviewerMaxRounds] = useState({}); + const [reviewerModels, setReviewerModels] = useState({}); const [reviewStopMode, setReviewStopMode] = useState(DEFAULT_REVIEW_STOP_MODE); const [reviewerApplies, setReviewerApplies] = useState(false); const [createJiraTicket, setCreateJiraTicket] = useState(false); @@ -43,6 +46,9 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa // Bare slashdo command a quick-template pinned (`plan-task`), never a rendered // `/do:x` string — see server/lib/slashdoInvocation.js for why. const [slashdoCommand, setSlashdoCommand] = useState(''); + // Resolved model lists for the reviewer table's Model column. Owned here (not by + // ReviewerPicker) so the picker stays fetch-free — see its `modelOptions` prop. + const reviewerModelOptions = useReviewerModelOptions(); const submittingRef = useRef(false); const descriptionRef = useRef(null); // Set by applyTemplate only when a template changes BOTH the app and the @@ -70,6 +76,9 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa if (Array.isArray(d.usernames)) setReviewUsernames(d.usernames); if (Array.isArray(d.optionalReviewers)) setOptionalReviewers(d.optionalReviewers); if (d.reviewerMaxRounds && typeof d.reviewerMaxRounds === 'object' && !Array.isArray(d.reviewerMaxRounds)) setReviewerMaxRounds(d.reviewerMaxRounds); + // The defaults persist per-reviewer models as scalars; the picker takes the + // token-keyed map (see client/src/lib/reviewerModels.js). + setReviewerModels(reviewerModelsFromDefaults(d)); if (d.stopMode) setReviewStopMode(d.stopMode); if (d.reviewerApplies === true) setReviewerApplies(true); }) @@ -309,6 +318,7 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa usernames: openPR && prCompletion === 'review-then-merge' ? reviewUsernames : undefined, optionalReviewers: openPR && prCompletion === 'review-then-merge' ? optionalReviewers : undefined, reviewerMaxRounds: openPR && prCompletion === 'review-then-merge' ? reviewerMaxRounds : undefined, + reviewerModels: openPR && prCompletion === 'review-then-merge' ? reviewerModels : undefined, reviewStopMode: openPR && prCompletion === 'review-then-merge' ? reviewStopMode : undefined, reviewerApplies: openPR && prCompletion === 'review-then-merge' ? reviewerApplies : undefined, screenshots: screenshots.length > 0 ? screenshots.map(s => s.path) : undefined, @@ -562,13 +572,16 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa usernames={reviewUsernames} optionalReviewers={optionalReviewers} reviewerMaxRounds={reviewerMaxRounds} + reviewerModels={reviewerModels} + modelOptions={reviewerModelOptions} stopMode={reviewStopMode} reviewerApplies={reviewerApplies} - onChange={({ reviewers: r, usernames: u, optionalReviewers: o, reviewerMaxRounds: m, stopMode, reviewerApplies: ra }) => { + onChange={({ reviewers: r, usernames: u, optionalReviewers: o, reviewerMaxRounds: m, reviewerModels: rm, stopMode, reviewerApplies: ra }) => { setReviewers(r); setReviewUsernames(u); setOptionalReviewers(o); setReviewerMaxRounds(m); + setReviewerModels(rm); setReviewStopMode(stopMode); setReviewerApplies(ra); }} diff --git a/client/src/components/cos/TaskAddForm.test.jsx b/client/src/components/cos/TaskAddForm.test.jsx index 6e6533232..184ccbdeb 100644 --- a/client/src/components/cos/TaskAddForm.test.jsx +++ b/client/src/components/cos/TaskAddForm.test.jsx @@ -6,6 +6,9 @@ import TaskAddForm from './TaskAddForm'; const api = vi.hoisted(() => ({ getCosPopularTemplates: vi.fn(), getCodeReviewDefaults: vi.fn(), + // Back the reviewer table's Model column (useReviewerModelOptions). + getLocalLlmStatus: vi.fn(), + getProviders: vi.fn(), useCosTaskTemplate: vi.fn() })); @@ -16,6 +19,8 @@ describe('TaskAddForm responsive layout', () => { vi.clearAllMocks(); api.getCosPopularTemplates.mockResolvedValue({ templates: [] }); api.getCodeReviewDefaults.mockResolvedValue(null); + api.getLocalLlmStatus.mockResolvedValue({ ollama: { models: [] }, lmstudio: { models: [] } }); + api.getProviders.mockResolvedValue({ providers: [] }); api.useCosTaskTemplate.mockResolvedValue({ success: true }); }); @@ -65,6 +70,8 @@ describe('TaskAddForm quick templates', () => { beforeEach(() => { vi.clearAllMocks(); api.getCodeReviewDefaults.mockResolvedValue(null); + api.getLocalLlmStatus.mockResolvedValue({ ollama: { models: [] }, lmstudio: { models: [] } }); + api.getProviders.mockResolvedValue({ providers: [] }); api.useCosTaskTemplate.mockResolvedValue({ success: true }); }); diff --git a/client/src/components/cos/constants.js b/client/src/components/cos/constants.js index 81cbbc22b..cc6529bfb 100644 --- a/client/src/components/cos/constants.js +++ b/client/src/components/cos/constants.js @@ -191,6 +191,22 @@ export const REVIEWER_OPTIONS = [ ]; export const LOCAL_LLM_REVIEWERS = ['lmstudio', 'ollama']; +// CLI reviewers whose binary takes a `--model ` tier. Client mirror of +// MODEL_CAPABLE_CLI_REVIEWERS in server/lib/cosValidation.js. +export const MODEL_CAPABLE_CLI_REVIEWERS = ['codex', 'claude']; + +// Every reviewer whose model the user can pick per row in ReviewerPicker — the +// model-capable CLIs plus the local-LLM backends. Client mirror of +// MODEL_SELECTABLE_REVIEWERS in server/lib/cosValidation.js; keep in sync so the +// picker only offers a Model cell where the server would keep the pin. +// `copilot` and `@username` reviewers take no model. +export const MODEL_SELECTABLE_REVIEWERS = [...MODEL_CAPABLE_CLI_REVIEWERS, ...LOCAL_LLM_REVIEWERS]; + +// Upper bound on a pinned reviewer model id. Client mirror of +// MAX_REVIEWER_MODEL_LENGTH in server/lib/cosValidation.js — a longer id is +// dropped server-side, so the input must not accept one. +export const MAX_REVIEWER_MODEL_LENGTH = 200; + // pr-watcher author gate (taskMetadata.prAuthorFilter). Mirrors // PR_AUTHOR_FILTERS in server/lib/validation.js. 'self' = PRs opened by the // gh-authenticated operator (or their automation); 'others' = external diff --git a/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx b/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx index c7ccbcfe7..73b1a0371 100644 --- a/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx +++ b/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx @@ -8,6 +8,8 @@ import InfoTooltip from '../../../ui/InfoTooltip'; import { FormField } from '../../../ui/FormField'; import { formatDateTime } from '../../../../utils/formatters'; import { useCodeReviewDefaults } from '../../../../hooks/useCodeReviewDefaults'; +import useReviewerModelOptions from '../../../../hooks/useReviewerModelOptions'; +import { reviewerModelsFromDefaults } from '../../../../lib/reviewerModels'; import ToggleSwitch from '../../../ToggleSwitch'; import { filterSelectableModels } from '../../../../utils/providers'; import EffortSelect from '../../EffortSelect'; @@ -17,6 +19,9 @@ import { INTERVAL_DESCRIPTIONS, toggleMetadataField } from './scheduleConstants' export default function GlobalConfigControls({ taskType, config, onUpdate, onTrigger, onReset, category: _category, providers, apps, updating, setUpdating, allTaskTypes, improvementDisabled }) { const reviewDefaults = useCodeReviewDefaults(); + // Resolved model lists for the reviewer table's Model column (the picker itself + // never fetches — see its `modelOptions` prop). + const reviewerModelOptions = useReviewerModelOptions(); const [selectedType, setSelectedType] = useState(config.type); const [selectedProviderId, setSelectedProviderId] = useState(config.providerId || ''); const [selectedModel, setSelectedModel] = useState(config.model || ''); @@ -435,16 +440,20 @@ export default function GlobalConfigControls({ taskType, config, onUpdate, onTri usernames={config.taskMetadata?.usernames ?? reviewDefaults.usernames} optionalReviewers={config.taskMetadata?.optionalReviewers ?? reviewDefaults.optionalReviewers} reviewerMaxRounds={config.taskMetadata?.reviewerMaxRounds ?? reviewDefaults.reviewerMaxRounds} + // The task type's own pins when it has them, else the install's Code + // Review Defaults (persisted as `Model` scalars). + reviewerModels={config.taskMetadata?.reviewerModels ?? reviewerModelsFromDefaults(reviewDefaults)} + modelOptions={reviewerModelOptions} stopMode={config.taskMetadata?.reviewStopMode || reviewDefaults.stopMode || DEFAULT_REVIEW_STOP_MODE} reviewerApplies={config.taskMetadata?.reviewerApplies !== undefined ? (config.taskMetadata?.reviewerApplies === true || config.taskMetadata?.reviewerApplies === 'true') : reviewDefaults.reviewerApplies} disabled={updating} - onChange={({ reviewers, usernames, optionalReviewers, reviewerMaxRounds, stopMode, reviewerApplies }) => { + onChange={({ reviewers, usernames, optionalReviewers, reviewerMaxRounds, reviewerModels, stopMode, reviewerApplies }) => { // Drop the legacy single `reviewer` key so storage converges on `reviewers`. const { reviewer: _reviewer, ...rest } = config.taskMetadata || {}; onUpdate(taskType, { - taskMetadata: { ...rest, reviewers, usernames, optionalReviewers, reviewerMaxRounds, reviewStopMode: stopMode, reviewerApplies } + taskMetadata: { ...rest, reviewers, usernames, optionalReviewers, reviewerMaxRounds, reviewerModels, reviewStopMode: stopMode, reviewerApplies } }); }} /> diff --git a/client/src/components/providers/CodeReviewDefaultsPanel.jsx b/client/src/components/providers/CodeReviewDefaultsPanel.jsx index 2dcf61844..3d9052aae 100644 --- a/client/src/components/providers/CodeReviewDefaultsPanel.jsx +++ b/client/src/components/providers/CodeReviewDefaultsPanel.jsx @@ -1,9 +1,10 @@ -import { useEffect, useId, useMemo, useState } from 'react'; +import { useEffect, useState } from 'react'; import { ShieldCheck } from 'lucide-react'; import toast from '../ui/Toast'; import * as api from '../../services/api'; -import { filterSelectableModels } from '../../utils/providers'; import ReviewerPicker from '../cos/ReviewerPicker'; +import useReviewerModelOptions from '../../hooks/useReviewerModelOptions'; +import { reviewerModelsFromDefaults, reviewerModelsToDefaults } from '../../lib/reviewerModels'; import { DEFAULT_REVIEWERS, DEFAULT_REVIEW_STOP_MODE, @@ -12,98 +13,45 @@ import { // Global Code Review Defaults — the chain the Review Loop uses when a task or // task-type config didn't pin its own reviewers. Lives at the top of the AI // Providers page so adding a new provider and pointing reviews at it stay in -// the same flow. Per-backend model dropdowns are shown only when the -// corresponding reviewer is in the chain: the local-LLM (LM Studio / Ollama) -// lists come from `/api/local-llm/status` so they reflect what's actually -// installed, while the Codex tier list comes from the provider catalog -// (`/api/providers`) since Codex is a CLI reviewer, not a local backend. +// the same flow. Every per-reviewer control (model, `~opt`, `~max`) now lives in +// the shared ReviewerPicker table (#3133), so this panel owns only the fetch of +// the model option lists (via useReviewerModelOptions) and the save. export default function CodeReviewDefaultsPanel() { - const lmStudioSelectId = useId(); - const ollamaSelectId = useId(); - const codexSelectId = useId(); - const claudeSelectId = useId(); - const claudeListId = useId(); const [loading, setLoading] = useState(true); const [saving, setSaving] = useState(false); const [reviewers, setReviewers] = useState(DEFAULT_REVIEWERS); const [usernames, setUsernames] = useState([]); const [optionalReviewers, setOptionalReviewers] = useState([]); const [reviewerMaxRounds, setReviewerMaxRounds] = useState({}); + const [reviewerModels, setReviewerModels] = useState({}); const [stopMode, setStopMode] = useState(DEFAULT_REVIEW_STOP_MODE); const [reviewerApplies, setReviewerApplies] = useState(false); - const [lmstudioModel, setLmstudioModel] = useState(''); - const [ollamaModel, setOllamaModel] = useState(''); - const [codexModel, setCodexModel] = useState(''); - const [claudeModel, setClaudeModel] = useState(''); - const [localLlmStatus, setLocalLlmStatus] = useState(null); - const [codexProvider, setCodexProvider] = useState(null); - const [claudeProvider, setClaudeProvider] = useState(null); + const modelOptions = useReviewerModelOptions(); useEffect(() => { let cancelled = false; - Promise.all([ - api.getCodeReviewDefaults({ silent: true }).catch(() => null), - api.getLocalLlmStatus({ silent: true }).catch(() => null), - api.getProviders({ silent: true }).catch(() => null), - ]).then(([defaults, status, providers]) => { - if (cancelled) return; - if (defaults) { - setReviewers(Array.isArray(defaults.reviewers) && defaults.reviewers.length ? defaults.reviewers : DEFAULT_REVIEWERS); - setUsernames(Array.isArray(defaults.usernames) ? defaults.usernames : []); - setOptionalReviewers(Array.isArray(defaults.optionalReviewers) ? defaults.optionalReviewers : []); - setReviewerMaxRounds(defaults.reviewerMaxRounds && typeof defaults.reviewerMaxRounds === 'object' && !Array.isArray(defaults.reviewerMaxRounds) - ? defaults.reviewerMaxRounds - : {}); - setStopMode(defaults.stopMode || DEFAULT_REVIEW_STOP_MODE); - setReviewerApplies(defaults.reviewerApplies === true); - setLmstudioModel(defaults.lmstudioModel || ''); - setOllamaModel(defaults.ollamaModel || ''); - setCodexModel(defaults.codexModel || ''); - setClaudeModel(defaults.claudeModel || ''); - } - setLocalLlmStatus(status || null); - // Codex and Claude are CLI reviewers, so their selectable model tiers come - // from the provider catalog (not the local-LLM status probe the others use). - const providerList = providers?.providers || []; - setCodexProvider(providerList.find((p) => p.id === 'codex') || null); - setClaudeProvider(providerList.find((p) => p.id === 'claude-code') || null); - setLoading(false); - }); + api.getCodeReviewDefaults({ silent: true }) + .catch(() => null) + .then((defaults) => { + if (cancelled) return; + if (defaults) { + setReviewers(Array.isArray(defaults.reviewers) && defaults.reviewers.length ? defaults.reviewers : DEFAULT_REVIEWERS); + setUsernames(Array.isArray(defaults.usernames) ? defaults.usernames : []); + setOptionalReviewers(Array.isArray(defaults.optionalReviewers) ? defaults.optionalReviewers : []); + setReviewerMaxRounds(defaults.reviewerMaxRounds && typeof defaults.reviewerMaxRounds === 'object' && !Array.isArray(defaults.reviewerMaxRounds) + ? defaults.reviewerMaxRounds + : {}); + setReviewerModels(reviewerModelsFromDefaults(defaults)); + setStopMode(defaults.stopMode || DEFAULT_REVIEW_STOP_MODE); + setReviewerApplies(defaults.reviewerApplies === true); + } + setLoading(false); + }); return () => { cancelled = true; }; }, []); - const needsLmStudio = reviewers.includes('lmstudio'); - const needsOllama = reviewers.includes('ollama'); - const needsCodex = reviewers.includes('codex'); - const needsClaude = reviewers.includes('claude'); - - const lmStudioModels = useMemo( - () => localLlmStatus?.lmstudio?.models?.map((m) => m.id).filter(Boolean) || [], - [localLlmStatus] - ); - const ollamaModels = useMemo( - () => localLlmStatus?.ollama?.models?.map((m) => m.id).filter(Boolean) || [], - [localLlmStatus] - ); - const codexModels = useMemo( - () => codexProvider ? filterSelectableModels(codexProvider.models || [codexProvider.defaultModel]) : [], - [codexProvider] - ); - // Claude reviewer suggestions span BOTH usage modes: the `claude-code` provider - // tiers (normal Claude) and installed Ollama models (an Ollama-backed `claude` - // reviewer, where `--model` selects the local model). Deduped, order-preserving. - // A datalist (not a hard renders. const payload = { reviewers, usernames, @@ -111,10 +59,7 @@ export default function CodeReviewDefaultsPanel() { reviewerMaxRounds, stopMode, reviewerApplies, - lmstudioModel: lmstudioModel || undefined, - ollamaModel: ollamaModel || undefined, - codexModel: codexModel || undefined, - claudeModel: claudeModel.trim() || undefined, + ...reviewerModelsToDefaults(reviewerModels), }; const ok = await api.updateSettings({ codeReview: payload }, { silent: true }) .then(() => true) @@ -123,35 +68,6 @@ export default function CodeReviewDefaultsPanel() { if (ok) toast.success('Code Review Defaults saved'); }; - // `emptyMessage` overrides the default local-LLM "install a model" hint so the - // Codex picker (a CLI reviewer, not a local backend) shows relevant guidance. - const renderModelPicker = (label, backend, value, setValue, options, selectId, emptyMessage = null) => { - const status = localLlmStatus?.[backend]; - const unavailable = status && status.available === false; - return ( -
- - {options.length > 0 ? ( - - ) : ( -
- {emptyMessage || (unavailable - ? `${label} backend isn't reachable — start it from Settings → Local LLMs to load models.` - : `No ${label} models installed yet — add one in Settings → Local LLMs.`)} -
- )} -
- ); - }; - return (
@@ -159,7 +75,7 @@ export default function CodeReviewDefaultsPanel() {

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 Codex and Claude reviewers invoke their CLI directly. Each runs the model selected below (Claude also supports an Ollama-backed CLI for local-only setups). + 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 Codex and Claude 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).

{loading ? ( @@ -171,43 +87,22 @@ export default function CodeReviewDefaultsPanel() { usernames={usernames} optionalReviewers={optionalReviewers} reviewerMaxRounds={reviewerMaxRounds} + reviewerModels={reviewerModels} + modelOptions={modelOptions} stopMode={stopMode} reviewerApplies={reviewerApplies} disabled={saving} - onChange={({ reviewers: r, usernames: u, optionalReviewers: o, reviewerMaxRounds: m, stopMode: s, reviewerApplies: a }) => { + onChange={({ reviewers: r, usernames: u, optionalReviewers: o, reviewerMaxRounds: m, reviewerModels: dm, stopMode: s, reviewerApplies: a }) => { setReviewers(r); setUsernames(u); setOptionalReviewers(o); setReviewerMaxRounds(m); + setReviewerModels(dm); setStopMode(s); setReviewerApplies(a); }} /> - {needsLmStudio && renderModelPicker('LM Studio', 'lmstudio', lmstudioModel, setLmstudioModel, lmStudioModels, lmStudioSelectId)} - {needsOllama && renderModelPicker('Ollama', 'ollama', ollamaModel, setOllamaModel, ollamaModels, ollamaSelectId)} - {needsCodex && renderModelPicker('Codex', 'codex', codexModel, setCodexModel, codexModels, codexSelectId, 'No selectable Codex models — configure the Codex provider on the AI Providers page (or leave blank to use the Codex CLI default).')} - {needsClaude && ( -
- - setClaudeModel(e.target.value)} - placeholder="Leave blank for the Claude CLI default" - className="px-2 py-1 bg-port-bg border border-port-border rounded text-xs text-gray-300 min-h-[28px] max-w-md" - /> - - {claudeModelSuggestions.map((id) => )} - -

- Passed as claude --model <id>. Pick a Claude tier, or — if you run an Ollama-backed claude — type or pick one of your installed Ollama models. -

-
- )} -