From feff28cd4225caf1523ba8bfe773f055540dee89 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 15:09:47 +0000 Subject: [PATCH] give the OpenCode reviewer a model dropdown and hide reviewers this machine can't run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer picker's Model cell was a bare text input for OpenCode, and its Add row offered every reviewer in the roster regardless of whether the machine could run one. - OpenCode's options now come from the seeded OpenCode Zen CLI/TUI records, whose namespaced `opencode/*` ids the Harnesses page refreshes from `opencode models`. The local-runtime wrappers stay excluded: their ids resolve only under the config a PortOS-spawned provider injects, and the reviewer runs a bare `opencode` against the user's own config. - Each reviewer's options now UNION every provider record fronting its binary instead of reading one. Sourcing from a single record is what hid `claude-sonnet-5` behind the CLI record's retired `claude-sonnet-4-6` while `claude-code-tui` already listed the current tier. - The Add row folds a reviewer whose CLI is missing, or whose provider records are all switched off, behind a `+N unavailable` toggle. Hidden rather than dropped: both checks are local-machine-only and the reviewer list is federation-wide config, so the toggle reveals them badged with which signal fired, and an already-selected reviewer always renders its row. Migration 337 repairs the underlying data: `claude` has no `models` subcommand, so nothing in the app can refresh a stale Claude record. It inserts `claude-sonnet-5` after a retired `claude-sonnet-4-6` even on a curated list, which migration 153's exact-match policy skipped. Additive on purpose — the retired id and every tier pointer survive, since 4-6 still resolves and the defect is the new tier being absent, not the old one being present. --- client/src/components/cos/ReviewerPicker.jsx | 111 +++++++--- .../components/cos/ReviewerPicker.test.jsx | 62 +++++- client/src/hooks/useReviewerModelOptions.js | 197 +++++++++++++----- .../hooks/useReviewerModelOptions.test.jsx | 110 +++++++++- .../337-claude-sonnet-5-additive.js | 82 ++++++++ .../337-claude-sonnet-5-additive.test.js | 125 +++++++++++ 6 files changed, 598 insertions(+), 89 deletions(-) create mode 100644 scripts/migrations/337-claude-sonnet-5-additive.js create mode 100644 scripts/migrations/337-claude-sonnet-5-additive.test.js diff --git a/client/src/components/cos/ReviewerPicker.jsx b/client/src/components/cos/ReviewerPicker.jsx index b3672f02b7..118f10a6d1 100644 --- a/client/src/components/cos/ReviewerPicker.jsx +++ b/client/src/components/cos/ReviewerPicker.jsx @@ -65,10 +65,10 @@ const CUSTOM_MODEL_OPTION = '[custom]'; * * `modelOptions` is the resolved model-picker data, shaped like * `useReviewerModelOptions()`'s return: `{ optionsByReviewer, defaultModels, - * 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. + * freeText, unavailable, providerDisabled, 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 @@ -79,11 +79,16 @@ const CUSTOM_MODEL_OPTION = '[custom]'; * `installed` is a per-reviewer-slug install probe from the Code Review * Defaults endpoint (`GET /api/code-review/defaults`'s `installed` field, * #3606) — `{ claude: true, antigravity: false, ... }`. Only an explicit - * `false` renders a "not installed" badge; `undefined` (not a CLI reviewer, - * or the caller didn't fetch it) renders nothing. Warn-only: a reviewer stays - * selectable and selected even when flagged not-installed, since the CLI - * check is local-machine-only and a federated peer (or a later install) may - * satisfy it. + * `false` counts as missing; `undefined` (not a CLI reviewer, or the caller + * didn't fetch it) says nothing. + * + * Together with `modelOptions.providerDisabled`, that decides which reviewers + * the **Add** row offers up front: one whose CLI is missing here, or whose + * provider records are all switched off, is folded behind a `+N unavailable` + * toggle. Warn-only either way — the toggle reveals them with a badge and they + * stay selectable, and an ALREADY-SELECTED reviewer always renders its row + * (badged), since both checks are local-machine-only and the reviewer list is + * federation-wide config a peer may satisfy. */ export default function ReviewerPicker({ reviewers = [], @@ -108,6 +113,9 @@ export default function ReviewerPicker({ // 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()); + // Whether the Add row also lists the reviewers this machine can't run (see + // `hiddenAddable`). Presentational only — nothing about it is stored. + const [showUnavailable, setShowUnavailable] = useState(false); const isCustomModel = (token) => customModelTokens.has(token.toLowerCase()); const setCustomModel = (token, on) => setCustomModelTokens((prev) => { const next = new Set(prev); @@ -122,7 +130,7 @@ export default function ReviewerPicker({ // 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 addable = REVIEWER_OPTIONS.filter(o => !selected.includes(o.value)); const hasNonCopilot = selected.some(r => r !== 'copilot'); const selectedUsernames = normalizeReviewUsernames(usernames); const atMaxUsernames = selectedUsernames.length >= MAX_REVIEW_USERNAMES; @@ -165,20 +173,53 @@ export default function ReviewerPicker({ // normally does", so clearing the select DELETES the key rather than writing `''`. const effortsMap = asMap(reviewerEfforts); const efforts = keyedLookup(effortsMap); - // Only an explicit `false` counts — `undefined` covers both "not a CLI - // reviewer" (copilot/lmstudio/ollama/@username) and "caller didn't fetch - // `installed`", neither of which should render a warning badge. - const notInstalled = (token) => installed?.[token] === false; - const renderInstalledBadge = (token) => notInstalled(token) && ( - - not installed - - ); + // Why this reviewer can't run here, or null when nothing says it can't. + // + // Two independent signals, both warn-only and both reported only when the + // caller actually fetched them — a reviewer stays selectable and selected + // either way, since the checks are local-machine-only and a federated peer + // (or a later install / a flip in Settings) may satisfy them: + // + // - `installed[token] === false` — the CLI binary isn't on PATH. Only an + // explicit `false` counts; `undefined` covers both "not a CLI reviewer" + // (copilot/@username) and "caller didn't fetch `installed`". + // - `providerDisabled[token]` — every provider record fronting that binary is + // switched off on this install, so the user has said they don't use it. A + // `/api/providers` that failed or hasn't landed reports nothing (see the + // hook), so this never fires on a slow page. + const unavailability = (token) => { + if (installed?.[token] === false) { + return { + label: 'not installed', + title: `${reviewerLabel(token)}'s CLI binary wasn't found on this machine. It still runs (federation-wide config), but the review loop here will report it unsatisfied until it's installed.` + }; + } + if (modelOptions?.providerDisabled?.[token]) { + return { + label: 'disabled', + title: `${reviewerLabel(token)}'s provider records are all switched off in Settings → AI Providers, so this machine isn't set up to use it. Adding it still works — the review loop spawns its CLI directly, and a federated peer may have it enabled.` + }; + } + return null; + }; + const renderUnavailableBadge = (token) => { + const reason = unavailability(token); + return reason && ( + + {reason.label} + + ); + }; + // The Add row lists what this machine can actually run, so a reviewer whose + // CLI is missing or whose providers are all switched off is folded behind a + // count instead of padding the row with things the review loop would report + // unsatisfied. HIDDEN, not dropped: the checks are local-machine-only and the + // reviewer list is federation-wide config, so the toggle reveals them (badged) + // rather than making a peer's reviewer unconfigurable from here. + const hiddenAddable = addable.filter(opt => unavailability(opt.value)); + const addOptions = showUnavailable + ? addable + : addable.filter(opt => !hiddenAddable.includes(opt)); const emit = (next) => onChange?.({ reviewers: selected, @@ -586,7 +627,7 @@ export default function ReviewerPicker({ {reviewerLabel(value)} - {renderInstalledBadge(value)} + {renderUnavailableBadge(value)} Model
{renderModelCell(value)}
@@ -630,10 +671,10 @@ export default function ReviewerPicker({ )} - {available.length > 0 && ( + {addable.length > 0 && (
Add: - {available.map(opt => ( + {addOptions.map(opt => ( ))} + {hiddenAddable.length > 0 && ( + + )}
)} diff --git a/client/src/components/cos/ReviewerPicker.test.jsx b/client/src/components/cos/ReviewerPicker.test.jsx index c40ab85c2b..427144c5be 100644 --- a/client/src/components/cos/ReviewerPicker.test.jsx +++ b/client/src/components/cos/ReviewerPicker.test.jsx @@ -32,10 +32,66 @@ describe('ReviewerPicker', () => { expect(screen.queryByText('not installed')).not.toBeInTheDocument(); }); - it('flags an unselected reviewer in the Add row too', () => { + it('flags an unselected reviewer once the Add row reveals it', async () => { + const user = userEvent.setup(); render( {}} />); - const addButton = screen.getByRole('button', { name: /Antigravity/ }); - expect(addButton).toHaveTextContent('not installed'); + await user.click(screen.getByRole('button', { name: /1 unavailable/ })); + expect(screen.getByRole('button', { name: /Antigravity/ })).toHaveTextContent('not installed'); + }); + }); + + // The Add row lists what this machine can actually run. Hidden, not dropped: + // both signals are local-machine-only and the reviewer list is + // federation-wide config, so a peer's reviewer stays configurable from here. + describe('unavailable reviewers in the Add row', () => { + const modelOptions = { providerDisabled: { kimi: true, cursor: true } }; + + it('hides a missing CLI and an all-off provider behind one count', () => { + render( + {}} + /> + ); + expect(screen.getByRole('button', { name: /3 unavailable/ })).toBeInTheDocument(); + for (const hidden of [/Antigravity/, /Kimi/, /Cursor Agent/]) { + expect(screen.queryByRole('button', { name: hidden })).not.toBeInTheDocument(); + } + // An available reviewer is still offered up front. + expect(screen.getByRole('button', { name: /Codex/ })).toBeInTheDocument(); + }); + + it('reveals them, badged with which signal fired, and adds them normally', async () => { + const onChange = vi.fn(); + const user = userEvent.setup(); + render( + + ); + await user.click(screen.getByRole('button', { name: /3 unavailable/ })); + expect(screen.getByRole('button', { name: /Kimi/ })).toHaveTextContent('disabled'); + expect(screen.getByRole('button', { name: /Antigravity/ })).toHaveTextContent('not installed'); + await user.click(screen.getByRole('button', { name: /Kimi/ })); + expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ reviewers: ['copilot', 'kimi'] })); + }); + + it('keeps an already-selected unavailable reviewer visible, badged', () => { + render( + {}} /> + ); + expect(screen.getByText('Kimi').parentElement).toHaveTextContent('disabled'); + }); + + it('offers the whole roster when neither signal was fetched', () => { + render( {}} />); + expect(screen.queryByRole('button', { name: /unavailable/ })).not.toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Kimi/ })).toBeInTheDocument(); }); }); diff --git a/client/src/hooks/useReviewerModelOptions.js b/client/src/hooks/useReviewerModelOptions.js index b1e8b2de92..e69792de4b 100644 --- a/client/src/hooks/useReviewerModelOptions.js +++ b/client/src/hooks/useReviewerModelOptions.js @@ -1,6 +1,6 @@ import { useEffect, useMemo, useState } from 'react'; import * as api from '../services/api'; -import { filterSelectableModels, selectableModelsForProvider, isAntigravityProvider, isGrokBuildCli, isKimiProvider, antigravityModelEffortLevels } from '../utils/providers'; +import { filterSelectableModels, selectableModelsForProvider, isAntigravityProvider, isCursorProvider, isGrokBuildCli, isKimiProvider, antigravityModelEffortLevels } from '../utils/providers'; import { MODEL_SELECTABLE_REVIEWERS } from '../components/cos/constants'; import { reviewerEffortLevels, normalizeReviewerSlug } from '../lib/reviewerPins'; import { LOCAL_LLM_BACKENDS } from '../lib/localLlmBackends'; @@ -11,6 +11,55 @@ import { LOCAL_LLM_BACKENDS } from '../lib/localLlmBackends'; // listing runs the `mtplx` wrapper, so it stays catalog-sourced and free-text. const PROBED_LOCAL_BACKENDS = LOCAL_LLM_BACKENDS.map((b) => b.id); +/** + * Every provider record that fronts a reviewer's binary, as predicates in + * PREFERENCE ORDER. Two reductions run over each list and they answer different + * questions: + * + * - the **option list** unions every matching record's catalog, because a + * reviewer runs one binary and any record fronting that binary lists ids that + * binary accepts. Sourcing from a single record made the picker hostage to + * that record's staleness — `claude-code` (CLI) listing `claude-sonnet-4-6` + * while `claude-code-tui` already listed `claude-sonnet-5` showed the reviewer + * the retired tier and hid the current one. + * - the **shown default** takes the FIRST match, so a reviewer spawned + * non-interactively reports the CLI record's default rather than the TUI's. + * + * A predicate rather than a bare id wherever the app already recognizes a + * provider by more than its shipped id (an `agy` configured by path), so this + * classifies the same records the rest of the UI does. + * + * What is deliberately NOT matched matters as much as what is: + * - **No Bedrock/Vertex record.** `claude-code-bedrock` lists + * `us.anthropic.*` ids that resolve only under that record's own environment. + * - **No `opencode-` preset.** Those enumerate ids that resolve + * only under the `OPENCODE_CONFIG_CONTENT` a PortOS-spawned provider injects, + * and the reviewer runs a bare `opencode` against the user's OWN config. The + * Zen CLI/TUI records are the exception and ARE matched: their ids are the + * namespaced `opencode/*` spellings that bare `opencode models` prints, and + * the Harnesses page's model refresh fills them from exactly that probe (see + * `server/services/harnesses.js#usesHarnessCatalog`), so they are the live + * catalog for the account the reviewer will bill. + * - **Not `opencode-zen` itself.** That is the HTTP-API record; its bare ids + * (`claude-opus-5`) are Zen's API model names, which `opencode -m` cannot + * resolve. + */ +const REVIEWER_PROVIDER_MATCHERS = Object.freeze({ + claude: [(p) => p.id === 'claude-code', (p) => p.id === 'claude-code-tui'], + codex: [(p) => p.id === 'codex', (p) => p.id === 'codex-tui'], + antigravity: [isAntigravityProvider], + // `grok` names one binary that ships as BOTH a `cli` and a `tui` provider, and + // the reviewer is spawned non-interactively, so the CLI's record wins the + // default — the broad predicate follows it for an install that only kept the TUI. + grok: [(p) => p.id === 'grok-cli', isGrokBuildCli], + cursor: [(p) => p.id === 'cursor-cli', isCursorProvider], + kimi: [(p) => p.id === 'kimi-cli', isKimiProvider], + opencode: [(p) => p.id === 'opencode-zen-cli', (p) => p.id === 'opencode-zen-tui'], + mtplx: [(p) => p.id === 'mtplx'], + lmstudio: [(p) => p.id === 'lmstudio'], + ollama: [(p) => p.id === 'ollama'], +}); + /** * Selectable model ids per model-taking reviewer, for `ReviewerPicker`'s Model * column. One hook so all four picker surfaces (Code Review Defaults, TaskAddForm, @@ -21,15 +70,16 @@ const PROBED_LOCAL_BACKENDS = LOCAL_LLM_BACKENDS.map((b) => b.id); * - `lmstudio` / `ollama` ids come from `/api/local-llm/status`, so they reflect * what's actually installed rather than a provider's stale stored `models`. * - Every other reviewer's tiers come from the provider catalog - * (`/api/providers`). That includes `mtplx`: its installed checkpoints live - * behind `/api/local-llm/mtplx/status`, which INVOKES the `mtplx` wrapper (a - * several-hundred-MB venv bootstrap on a cold version — see + * (`/api/providers`), unioned across the records listed in + * `REVIEWER_PROVIDER_MATCHERS`. That includes `mtplx`: its installed + * checkpoints live behind `/api/local-llm/mtplx/status`, which INVOKES the + * `mtplx` wrapper (a several-hundred-MB venv bootstrap on a cold version — see * `server/lib/mtplxRuntime.js`), and a picker mount must never pay that. The * shipped provider's catalog plus a free-text field is the honest trade. * - * The `claude` list spans BOTH usage modes: the `claude-code` provider tiers and - * the installed Ollama ids (an Ollama-backed `claude` CLI, where `--model` selects - * the local model). Deduped, order-preserving. + * The `claude` list spans BOTH usage modes: the `claude-code`/`claude-code-tui` + * provider tiers and 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 ALSO accept a typed id, not only * a pick: an Ollama-backed `claude` can run any locally-installed id, and a @@ -41,6 +91,14 @@ const PROBED_LOCAL_BACKENDS = LOCAL_LLM_BACKENDS.map((b) => b.id); * `unavailable` distinguishes "backend is down" from "backend has no models" so * the empty state can say the useful thing. Absent = not probed (every reviewer * outside PROBED_LOCAL_BACKENDS, `mtplx` included). + * + * `providerDisabled[reviewer]` is true when the reviewer HAS provider records on + * this install and every one of them is switched off — the signal + * `ReviewerPicker` uses to drop it from the Add row, so a machine that never + * enabled Kimi or Cursor isn't offered them. Only ever true from a landed fetch: + * a null/failed `/api/providers` matches no record, which reads as "nothing + * known", never as "switched off". + * * `loaded` flips once both fetches settle, so a consumer can tell "no options * yet" from "genuinely no options" (an empty list is a real answer, not a * pre-fetch placeholder). @@ -51,7 +109,7 @@ const PROBED_LOCAL_BACKENDS = LOCAL_LLM_BACKENDS.map((b) => b.id); * reviewer returns its static ladder. Lives here rather than in the picker so the * picker keeps doing no fetching of its own. * - * @returns {{ optionsByReviewer: Record, defaultModels: Record, freeText: Record, unavailable: Record, modelEffortLevels: (reviewer: string, model?: string|null) => readonly string[]|null, loaded: boolean, reviewers: string[] }} + * @returns {{ optionsByReviewer: Record, defaultModels: Record, freeText: Record, unavailable: Record, providerDisabled: Record, modelEffortLevels: (reviewer: string, model?: string|null) => readonly string[]|null, loaded: boolean, reviewers: string[] }} */ export default function useReviewerModelOptions() { const [localStatus, setLocalStatus] = useState(null); @@ -78,34 +136,49 @@ export default function useReviewerModelOptions() { const localIds = (backend) => (localStatus?.[backend]?.models || []) .map((m) => m.id || m.name) .filter(Boolean); - // `match` is a predicate rather than an id so a reviewer whose provider can be - // recognized by more than its shipped id (an `agy` configured by path) uses the - // same predicate the rest of the app does. Several matchers = preference order: - // `grok` names one binary that ships as BOTH a `cli` and a `tui` provider, and - // the reviewer is spawned non-interactively, so the CLI's catalog wins — the - // broad predicate is the fallback for an install that only kept the TUI. - const providerFor = (...matchers) => - matchers.reduce((found, match) => found || (providers || []).find(match), null); - const providerTiers = (...matchers) => { - const provider = providerFor(...matchers); - if (!provider) return []; + // Every record fronting each reviewer's binary, in matcher-preference order + // (not `providers` array order) so `[0]` is the record whose default the + // picker shows. De-duped by identity: two matchers commonly overlap + // (`grok-cli` is also an `isGrokBuildCli`). + // + // Resolved ONCE for the whole roster rather than per lookup — the option + // list, the shown default, the agy raw catalog and `providerDisabled` all + // ask the same question, and a per-call helper re-walked the provider array + // for every one of them. + const providersByReviewer = Object.fromEntries( + Object.entries(REVIEWER_PROVIDER_MATCHERS).map(([reviewer, matchers]) => { + const matched = []; + for (const match of matchers) { + for (const provider of providers || []) { + if (match(provider) && !matched.includes(provider)) matched.push(provider); + } + } + return [reviewer, matched]; + }) + ); + const providersFor = (reviewer) => providersByReviewer[reviewer] || []; + + // `selectableModelsForProvider` owns the per-provider normalization (today: + // agy's one-id-per-effort-tier catalog collapsed to base ids, so the row's + // separate Effort cell stays the effort control). Going through it rather + // than special-casing agy here keeps the rule in one place. + const selectableModels = (provider) => { // `models` may be empty on a CLI provider configured with only a // defaultModel — `[]` is truthy, so a bare `||` wouldn't fall through. const models = provider.models?.length ? provider.models : [provider.defaultModel]; - // `selectableModelsForProvider` owns the per-provider normalization (today: - // agy's one-id-per-effort-tier catalog collapsed to base ids, so the row's - // separate Effort cell stays the effort control). Going through it rather - // than special-casing agy here keeps the rule in one place. return filterSelectableModels(selectableModelsForProvider(provider, models)); }; + const providerTiers = (reviewer) => + Array.from(new Set(providersFor(reviewer).flatMap(selectableModels))); + // Show the configured provider default in the picker even when the user has // not saved a per-reviewer override. A concrete default is useful context; // configured-default sentinels intentionally resolve to null because the CLI // owns the choice and there is no model id PortOS can honestly display. - const providerDefault = (...matchers) => { - const provider = providerFor(...matchers); + const providerDefault = (reviewer) => { + const provider = providersFor(reviewer)[0]; if (!provider?.defaultModel) return null; return filterSelectableModels( selectableModelsForProvider(provider, [provider.defaultModel]) @@ -115,7 +188,7 @@ export default function useReviewerModelOptions() { // Local backend model lists come from the live runtime probe, so only show a // provider default when it is present in that authoritative list. const localDefault = (backend) => { - const candidate = providerFor((p) => p.id === backend)?.defaultModel; + const candidate = providersFor(backend)[0]?.defaultModel; return candidate && localIds(backend).includes(candidate) ? candidate : null; }; @@ -123,52 +196,58 @@ export default function useReviewerModelOptions() { const optionsByReviewer = { lmstudio: localIds('lmstudio'), ollama, - codex: providerTiers((p) => p.id === 'codex'), + codex: providerTiers('codex'), // Claude tiers first (the common case), then installed Ollama ids for an // Ollama-backed `claude`. Deduped, order-preserving. - claude: Array.from(new Set([...providerTiers((p) => p.id === 'claude-code'), ...ollama].filter(Boolean))), - antigravity: providerTiers(isAntigravityProvider), + claude: Array.from(new Set([...providerTiers('claude'), ...ollama].filter(Boolean))), + antigravity: providerTiers('antigravity'), // The shipped grok provider carries only the configured-default sentinel, // which `filterSelectableModels` strips — so this is legitimately `[]` until // the user lists real ids on the provider. The Model cell stays useful // regardless because grok, like every CLI reviewer, is free-text. - grok: providerTiers((p) => p.id === 'grok-cli', isGrokBuildCli), - cursor: providerTiers((p) => p.id === 'cursor-cli', (p) => p.id === 'cursor-tui'), + grok: providerTiers('grok'), + cursor: providerTiers('cursor'), // Legitimately empty, for grok's documented reason: the shipped kimi // provider carries only the configured-default sentinel, which // `filterSelectableModels` strips. Free-text keeps the cell usable. - kimi: providerTiers((p) => p.id === 'kimi-cli', isKimiProvider), - // Deliberately NOT sourced from the `opencode-` presets. Those - // enumerate ids that only resolve under the `OPENCODE_CONFIG_CONTENT` a - // PortOS-spawned provider injects, and the reviewer runs a bare `opencode` - // against the user's OWN config — so listing them would offer picks that - // silently fail. `opencode -m` takes a `provider/model` id the user types. - opencode: [], - mtplx: providerTiers((p) => p.id === 'mtplx'), + kimi: providerTiers('kimi'), + // The namespaced `opencode/*` ids the seeded Zen CLI/TUI records carry, + // which the Harnesses page refreshes from `opencode models` — so the cell + // is a dropdown of what this account can actually run instead of the plain + // text input it used to be. Still free-text underneath: `opencode -m` takes + // any `provider/model` the user's own config resolves. + opencode: providerTiers('opencode'), + mtplx: providerTiers('mtplx'), }; const defaultModels = { lmstudio: localDefault('lmstudio'), ollama: localDefault('ollama'), - codex: providerDefault((p) => p.id === 'codex'), - claude: providerDefault((p) => p.id === 'claude-code'), - antigravity: providerDefault(isAntigravityProvider), - grok: providerDefault((p) => p.id === 'grok-cli', isGrokBuildCli), - cursor: providerDefault((p) => p.id === 'cursor-cli', (p) => p.id === 'cursor-tui'), - kimi: providerDefault((p) => p.id === 'kimi-cli', isKimiProvider), + codex: providerDefault('codex'), + claude: providerDefault('claude'), + antigravity: providerDefault('antigravity'), + grok: providerDefault('grok'), + cursor: providerDefault('cursor'), + kimi: providerDefault('kimi'), + // Deliberately null even though the Zen records carry one: the reviewer + // spawns a BARE `opencode`, which falls back to whatever the user's own + // config names — not to the PortOS record's default. Naming a model here + // would claim a default the run won't use. opencode: null, - mtplx: providerDefault((p) => p.id === 'mtplx'), + mtplx: providerDefault('mtplx'), }; - // The agy provider's RAW catalog — one id per effort tier + // The agy providers' RAW catalog — one id per effort tier // (`gemini-3.6-flash-low|-medium|-high`), which is exactly what the narrowing // reads. Deliberately NOT `optionsByReviewer.antigravity`: that list has // already had the suffixes collapsed away, so it carries no tier information. - const antigravityCatalog = (providers || []).find(isAntigravityProvider)?.models || []; + const antigravityCatalog = Array.from(new Set( + providersFor('antigravity').flatMap((provider) => provider.models || []) + )); // The effort ladder a reviewer offers ONCE ITS MODEL IS PINNED. `agy` validates // the pair, so a model with no `-medium` sibling must not offer `medium` - // (#3733). `antigravityModelEffortLevels` returns null for "can't tell" — empty - // catalog, unset model, or the configured-default sentinel — and the full + // (#3733). `antigravityModelEffortLevels` returns null for "can't tell" — + // empty catalog, unset model, or the configured-default sentinel — and the full // static ladder stands there, the same null-means-fall-back contract // `effortLevelsForProvider` uses. `[]` is a real answer: that model has no // effort tiers at all. @@ -191,8 +270,9 @@ export default function useReviewerModelOptions() { // because their catalogs are stored snapshots that can lag a newly-released // tier — grok's shipped catalog holds no real id at all, so a typed id is the // ONLY way to pin one (an agy pin may also be typed effort-suffixed — the - // server splits it). Derived from the rosters so a reviewer added to either - // one can't silently default to the wrong control. + // server splits it) — and `opencode` because a user's own config can declare + // namespaces the Zen catalog never lists. Derived from the rosters so a + // reviewer added to either one can't silently default to the wrong control. freeText: Object.fromEntries( MODEL_SELECTABLE_REVIEWERS.map((r) => [r, !PROBED_LOCAL_BACKENDS.includes(r)]) ), @@ -200,6 +280,19 @@ export default function useReviewerModelOptions() { lmstudio: localStatus?.lmstudio?.available === false, ollama: localStatus?.ollama?.available === false, }, + // `every` over a NON-EMPTY match list, so the two ways to have no enabled + // record stay apart: an install that switched every Kimi record off is + // `true` (hide it), while a reviewer with no records at all — or a fetch + // that failed or hasn't landed — is `false` (nothing is known, so hide + // nothing). `enabled === false` rather than falsiness, which deliberately + // reads a record with no `enabled` key as ON — the opposite of + // `providerCardState`'s stricter test, because this answer HIDES a control + // and incomplete data must never do that. + providerDisabled: Object.fromEntries( + Object.entries(providersByReviewer).map(([reviewer, matched]) => + [reviewer, matched.length > 0 && matched.every((p) => p.enabled === false)] + ) + ), loaded, // Exposed so a consumer can assert it covers every model-taking reviewer. reviewers: MODEL_SELECTABLE_REVIEWERS, diff --git a/client/src/hooks/useReviewerModelOptions.test.jsx b/client/src/hooks/useReviewerModelOptions.test.jsx index 06503d1637..394ae6120d 100644 --- a/client/src/hooks/useReviewerModelOptions.test.jsx +++ b/client/src/hooks/useReviewerModelOptions.test.jsx @@ -24,12 +24,21 @@ const providers = [ 'gemini-3.1-pro-high', ], }, - // One `grok` binary ships as both a TUI and a CLI provider; the reviewer is - // spawned non-interactively, so the CLI's catalog is the one it should offer. - { id: 'grok-tui', type: 'tui', command: 'grok', models: ['stale-tui-id'] }, + // One `grok` binary ships as both a TUI and a CLI provider. Both list ids that + // binary accepts, so the picker unions them; the CLI's record still owns the + // shown DEFAULT, since the reviewer is spawned non-interactively. + { id: 'grok-tui', type: 'tui', command: 'grok', models: ['tui-only-id'] }, { id: 'grok-cli', type: 'cli', command: 'grok', models: ['grok-configured-default', 'grok-code-fast-1'] }, { id: 'cursor-cli', type: 'cli', command: 'cursor-agent', models: ['auto', 'gpt-5'] }, { id: 'mtplx', type: 'api', models: ['mtplx-qwen38-27b-optimized-speed'], defaultModel: 'mtplx-qwen38-27b-optimized-speed' }, + // The seeded OpenCode Zen wrappers, whose namespaced ids the Harnesses page + // refreshes from `opencode models` — the reviewer's dropdown source. + { id: 'opencode-zen-cli', type: 'cli', command: 'opencode', models: ['opencode/big-pickle'], defaultModel: 'opencode/big-pickle' }, + { id: 'opencode-zen-tui', type: 'tui', command: 'opencode', models: ['opencode/big-pickle', 'opencode/mimo-v2.5-free'] }, + // An OpenCode wrapper pointed at a local runtime: its ids resolve only under + // the config PortOS injects, so the reviewer (a BARE `opencode`) must not + // offer them. + { id: 'opencode-ollama', type: 'cli', command: 'opencode', ollamaBacked: true, models: ['qwen3-coder:30b'] }, ]; describe('useReviewerModelOptions', () => { @@ -69,6 +78,28 @@ describe('useReviewerModelOptions', () => { expect(result.current.defaultModels.antigravity).toBeNull(); }); + // The reported defect: `claude-code` (CLI) still listed the retired + // `claude-sonnet-4-6` while `claude-code-tui` had moved to `claude-sonnet-5`, + // and sourcing the picker from the CLI record alone showed the retired tier + // and hid the current one. `claude` has no `models` subcommand, so nothing can + // refresh that record in place. + it('unions the Claude CLI and TUI catalogs so one stale record can\'t hide a live tier', async () => { + getProviders.mockResolvedValue({ providers: [ + { id: 'claude-code', type: 'cli', command: 'claude', models: ['claude-haiku-4-5', 'claude-sonnet-4-6'], defaultModel: 'claude-haiku-4-5' }, + { id: 'claude-code-tui', type: 'tui', command: 'claude', models: ['claude-sonnet-5', 'claude-opus-5'] }, + // Bedrock ids resolve only under that record's own environment. + { id: 'claude-code-bedrock', type: 'cli', command: 'claude', models: ['us.anthropic.claude-sonnet-5'] }, + ] }); + const { result } = renderHook(() => useReviewerModelOptions()); + await waitFor(() => expect(result.current.loaded).toBe(true)); + expect(result.current.optionsByReviewer.claude).toEqual([ + 'claude-haiku-4-5', 'claude-sonnet-4-6', 'claude-sonnet-5', 'claude-opus-5', 'qwen2.5:7b', + ]); + // The CLI record still owns the shown default — the reviewer is spawned + // non-interactively. + expect(result.current.defaultModels.claude).toBe('claude-haiku-4-5'); + }); + // #3728: `agy --model ` is real, so the antigravity row gets a Model cell. // Its ids arrive effort-suffixed and would otherwise duplicate the Effort cell // (and hand agy a `--model X-high --effort high` pair it rejects). @@ -82,10 +113,14 @@ describe('useReviewerModelOptions', () => { }); // #3729: `grok --model ` is real, so the grok row gets a Model cell too. - it('sources grok options from the CLI provider, not the TUI, and drops the sentinel', async () => { + // Both records front the same binary, so the option list unions them (CLI + // first) rather than letting one record's staleness hide the other's ids — + // sourcing from a single record is exactly what hid `claude-sonnet-5` behind + // the CLI record's retired `claude-sonnet-4-6`. + it('unions the grok CLI and TUI catalogs, CLI first, and drops the sentinel', async () => { const { result } = renderHook(() => useReviewerModelOptions()); await waitFor(() => expect(result.current.loaded).toBe(true)); - expect(result.current.optionsByReviewer.grok).toEqual(['grok-code-fast-1']); + expect(result.current.optionsByReviewer.grok).toEqual(['grok-code-fast-1', 'tui-only-id']); // Free-text: the shipped grok catalog is sentinel-only, so a typed id is // often the only way to pin one. expect(result.current.freeText.grok).toBe(true); @@ -102,7 +137,70 @@ describe('useReviewerModelOptions', () => { getProviders.mockResolvedValue({ providers: [providers.find((p) => p.id === 'grok-tui')] }); const { result } = renderHook(() => useReviewerModelOptions()); await waitFor(() => expect(result.current.loaded).toBe(true)); - expect(result.current.optionsByReviewer.grok).toEqual(['stale-tui-id']); + expect(result.current.optionsByReviewer.grok).toEqual(['tui-only-id']); + }); + + // The Model cell used to be a bare text input for opencode: the reviewer runs a + // BARE `opencode`, and the only catalogs on hand were the `opencode-` + // presets, whose ids resolve solely under the config a PortOS-spawned provider + // injects. The seeded Zen wrappers are the exception — their namespaced ids are + // what `opencode models` prints, and the Harnesses page refreshes them from + // exactly that probe — so those, and only those, feed the dropdown. + describe('opencode', () => { + it('offers the Zen wrappers\' namespaced ids, unioned across CLI and TUI', async () => { + const { result } = renderHook(() => useReviewerModelOptions()); + await waitFor(() => expect(result.current.loaded).toBe(true)); + expect(result.current.optionsByReviewer.opencode) + .toEqual(['opencode/big-pickle', 'opencode/mimo-v2.5-free']); + // Still free-text underneath: a user's own config can declare namespaces + // the Zen catalog never lists. + expect(result.current.freeText.opencode).toBe(true); + }); + + it('never offers a local-runtime wrapper\'s ids', async () => { + const { result } = renderHook(() => useReviewerModelOptions()); + await waitFor(() => expect(result.current.loaded).toBe(true)); + expect(result.current.optionsByReviewer.opencode).not.toContain('qwen3-coder:30b'); + }); + + // The reviewer spawns a bare `opencode`, which falls back to whatever the + // user's OWN config names — not to the PortOS record's default. Showing the + // record's default would claim a model the run won't use. + it('shows no default even though the Zen record carries one', async () => { + const { result } = renderHook(() => useReviewerModelOptions()); + await waitFor(() => expect(result.current.loaded).toBe(true)); + expect(result.current.defaultModels.opencode).toBeNull(); + }); + }); + + // The Add row hides what this machine can't run, so "every record switched + // off" has to stay apart from "no record at all" and from "fetch failed". + describe('providerDisabled', () => { + it('is true only when every record fronting the reviewer is switched off', async () => { + getProviders.mockResolvedValue({ providers: [ + { id: 'kimi-cli', type: 'cli', command: 'kimi', enabled: false, models: [] }, + { id: 'kimi-tui', type: 'tui', command: 'kimi', enabled: false, models: [] }, + { id: 'grok-cli', type: 'cli', command: 'grok', enabled: false, models: [] }, + { id: 'grok-tui', type: 'tui', command: 'grok', enabled: true, models: [] }, + // No `enabled` key at all — a record written before the flag existed + // must not read as off. + { id: 'codex', type: 'cli', command: 'codex', models: [] }, + ] }); + const { result } = renderHook(() => useReviewerModelOptions()); + await waitFor(() => expect(result.current.loaded).toBe(true)); + expect(result.current.providerDisabled.kimi).toBe(true); + expect(result.current.providerDisabled.grok).toBe(false); + expect(result.current.providerDisabled.codex).toBe(false); + }); + + it('reports nothing disabled when the reviewer has no records, or the fetch failed', async () => { + getProviders.mockResolvedValue({ providers: [] }); + const { result } = renderHook(() => useReviewerModelOptions()); + await waitFor(() => expect(result.current.loaded).toBe(true)); + for (const reviewer of MODEL_SELECTABLE_REVIEWERS) { + expect(result.current.providerDisabled[reviewer], reviewer).toBe(false); + } + }); }); // #3733: `agy` validates the model/effort PAIR, so the Effort cell's ladder has diff --git a/scripts/migrations/337-claude-sonnet-5-additive.js b/scripts/migrations/337-claude-sonnet-5-additive.js new file mode 100644 index 0000000000..d02b6d03af --- /dev/null +++ b/scripts/migrations/337-claude-sonnet-5-additive.js @@ -0,0 +1,82 @@ +/** + * Offer `claude-sonnet-5` on a Claude CLI/TUI record that still lists only the + * retired `claude-sonnet-4-6` sonnet tier. + * + * Migration 153 already made this swap, but ONLY for a `models` array matching + * the prior seeded trio exactly — a user who had appended an id to the list (a + * Fable tier, say) was classified as "customized" and skipped, and their record + * kept the 4-6 tier while the shipped seed and their other Claude records moved + * on. `claude` has no `models` subcommand, so nothing in the app can refresh + * that record: the reviewer/task model pickers reading it offer the retired + * sonnet and cannot offer the current one at all. + * + * ADDITIVE, deliberately — the opposite policy from 153/206 and from + * `makeSeededProviderTierMigration`, because this one runs against lists the + * user curated: + * + * - `claude-sonnet-5` is INSERTED right after `claude-sonnet-4-6`, and the + * retired id is KEPT. `claude-sonnet-4-6` still resolves for the CLI, so + * dropping an id a user chose to list would remove a working pin; the defect + * is the new tier being absent, not the old one being present. + * - Tier pointers (`defaultModel`/`lightModel`/`mediumModel`/`heavyModel`) are + * left ALONE. They point at an id that still works, and a curated list is + * exactly where re-pointing would override a deliberate choice. + * + * Idempotent by the same condition either way: a record already listing + * `claude-sonnet-5` is untouched, so this is a no-op on a seeded install (153/206 + * or a fresh `data.reference` seed already put it there) and on a second run. + */ + +import { readProvidersDoc, writeJsonAtomic } from './_lib.js'; + +const PROVIDERS_REL_PATH = 'data/providers.json'; + +// The four seeded Claude records and the sonnet id each one spells. The Bedrock +// pair uses the region-qualified form its own environment resolves — inserting a +// bare `claude-sonnet-5` there would offer an id that record cannot run. +const TARGETS = [ + { id: 'claude-code', retired: 'claude-sonnet-4-6', current: 'claude-sonnet-5' }, + { id: 'claude-code-tui', retired: 'claude-sonnet-4-6', current: 'claude-sonnet-5' }, + { id: 'claude-code-bedrock', retired: 'us.anthropic.claude-sonnet-4-6', current: 'us.anthropic.claude-sonnet-5' }, + { id: 'claude-code-tui-bedrock', retired: 'us.anthropic.claude-sonnet-4-6', current: 'us.anthropic.claude-sonnet-5' }, +]; + +export default { + async up({ rootDir }) { + const doc = await readProvidersDoc({ rootDir }); + if (!doc.ok) { + if (doc.reason === 'no-file') console.log(`📄 ${PROVIDERS_REL_PATH} not present — skipping (fresh install seeds claude-sonnet-5 from data.reference)`); + else if (doc.reason === 'unreadable') console.log(`⚠️ ${PROVIDERS_REL_PATH}: invalid JSON, skipping (${doc.err.message})`); + else console.log(`⚠️ ${PROVIDERS_REL_PATH}: unexpected shape, skipping`); + return { ok: false, reason: doc.reason, updated: 0 }; + } + + const { config, providers, path: providersPath } = doc; + const touched = []; + + for (const { id, retired, current } of TARGETS) { + const provider = providers[id]; + if (!provider || !Array.isArray(provider.models)) continue; + const at = provider.models.indexOf(retired); + // Nothing to repair unless the retired id is listed AND the current one + // isn't: an already-current record (seeded, or bumped by 153) is a no-op, + // and a record that never listed the retired tier is not this bug. + if (at === -1 || provider.models.includes(current)) continue; + provider.models = [ + ...provider.models.slice(0, at + 1), + current, + ...provider.models.slice(at + 1), + ]; + touched.push(id); + } + + if (touched.length === 0) { + console.log(`✅ ${PROVIDERS_REL_PATH}: Claude sonnet tier already current — no change`); + return { ok: true, reason: 'already-current', updated: 0 }; + } + + await writeJsonAtomic(providersPath, config); + console.log(`📝 ${PROVIDERS_REL_PATH}: offered claude-sonnet-5 on ${touched.join(', ')}`); + return { ok: true, reason: 'updated', updated: touched.length }; + }, +}; diff --git a/scripts/migrations/337-claude-sonnet-5-additive.test.js b/scripts/migrations/337-claude-sonnet-5-additive.test.js new file mode 100644 index 0000000000..47316977fe --- /dev/null +++ b/scripts/migrations/337-claude-sonnet-5-additive.test.js @@ -0,0 +1,125 @@ +/** + * Test for migration 337 — offer `claude-sonnet-5` on a Claude CLI/TUI record + * that still lists only the retired `claude-sonnet-4-6` tier. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, rmSync, writeFileSync, readFileSync, mkdirSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +import migration from './337-claude-sonnet-5-additive.js'; + +const writeJson = (path, value) => writeFileSync(path, JSON.stringify(value, null, 2) + '\n'); +const readJson = (path) => JSON.parse(readFileSync(path, 'utf-8')); + +describe('migration 337 — claude-sonnet-5 additive repair', () => { + let rootDir; + let providersPath; + + beforeEach(() => { + rootDir = mkdtempSync(join(tmpdir(), 'portos-337-')); + mkdirSync(join(rootDir, 'data')); + providersPath = join(rootDir, 'data', 'providers.json'); + }); + + afterEach(() => rmSync(rootDir, { recursive: true, force: true })); + + const seed = (providers) => writeJson(providersPath, { activeProvider: 'claude-code', providers }); + + it('inserts claude-sonnet-5 after the retired tier on a CURATED list 153 skipped', async () => { + seed({ + 'claude-code': { + models: ['claude-haiku-4-5', 'claude-sonnet-4-6', 'claude-opus-5', 'claude-fable-5'], + defaultModel: 'claude-opus-5', + mediumModel: 'claude-sonnet-4-6', + }, + }); + + const result = await migration.up({ rootDir }); + + expect(result).toMatchObject({ ok: true, reason: 'updated', updated: 1 }); + const after = readJson(providersPath).providers['claude-code']; + expect(after.models).toEqual([ + 'claude-haiku-4-5', 'claude-sonnet-4-6', 'claude-sonnet-5', 'claude-opus-5', 'claude-fable-5', + ]); + // Additive: the retired id and every tier pointer survive untouched. + expect(after.mediumModel).toBe('claude-sonnet-4-6'); + expect(after.defaultModel).toBe('claude-opus-5'); + }); + + it('uses each Bedrock record\'s own region-qualified sonnet spelling', async () => { + seed({ + 'claude-code-bedrock': { + models: ['us.anthropic.claude-haiku-4-5', 'us.anthropic.claude-sonnet-4-6', 'global.anthropic.claude-opus-5'], + }, + 'claude-code-tui-bedrock': { + models: ['us.anthropic.claude-sonnet-4-6'], + }, + }); + + const result = await migration.up({ rootDir }); + + expect(result.updated).toBe(2); + const { providers } = readJson(providersPath); + expect(providers['claude-code-bedrock'].models).toEqual([ + 'us.anthropic.claude-haiku-4-5', + 'us.anthropic.claude-sonnet-4-6', + 'us.anthropic.claude-sonnet-5', + 'global.anthropic.claude-opus-5', + ]); + expect(providers['claude-code-tui-bedrock'].models).toEqual([ + 'us.anthropic.claude-sonnet-4-6', + 'us.anthropic.claude-sonnet-5', + ]); + // The bare id must never leak into a Bedrock record — its environment + // resolves only the region-qualified form. + expect(providers['claude-code-bedrock'].models).not.toContain('claude-sonnet-5'); + }); + + it('is a no-op on an already-current record and on a second run', async () => { + seed({ + 'claude-code': { models: ['claude-haiku-4-5', 'claude-sonnet-5', 'claude-opus-5'] }, + 'claude-code-tui': { models: ['claude-haiku-4-5', 'claude-sonnet-4-6', 'claude-sonnet-5'] }, + }); + + const first = await migration.up({ rootDir }); + expect(first).toMatchObject({ ok: true, reason: 'already-current', updated: 0 }); + + // And a record it DID repair stays repaired rather than gaining a duplicate. + seed({ 'claude-code': { models: ['claude-sonnet-4-6'] } }); + expect((await migration.up({ rootDir })).updated).toBe(1); + const second = await migration.up({ rootDir }); + expect(second.updated).toBe(0); + expect(readJson(providersPath).providers['claude-code'].models) + .toEqual(['claude-sonnet-4-6', 'claude-sonnet-5']); + }); + + it('leaves records outside the four seeded Claude ids alone', async () => { + seed({ + 'claude-ollama': { models: ['claude-sonnet-4-6'] }, + 'antigravity-cli': { models: ['claude-sonnet-4-6'] }, + }); + + expect((await migration.up({ rootDir })).updated).toBe(0); + const { providers } = readJson(providersPath); + expect(providers['claude-ollama'].models).toEqual(['claude-sonnet-4-6']); + expect(providers['antigravity-cli'].models).toEqual(['claude-sonnet-4-6']); + }); + + it('skips a missing or malformed providers file without throwing', async () => { + expect(await migration.up({ rootDir })).toMatchObject({ ok: false, reason: 'no-file' }); + + writeFileSync(providersPath, '{not json'); + expect(await migration.up({ rootDir })).toMatchObject({ ok: false, reason: 'unreadable' }); + + writeJson(providersPath, { activeProvider: 'claude-code' }); + expect(await migration.up({ rootDir })).toMatchObject({ ok: false, reason: 'bad-shape' }); + }); + + it('skips a record whose models field is not an array', async () => { + seed({ 'claude-code': { models: 'claude-sonnet-4-6', defaultModel: 'claude-sonnet-4-6' } }); + + expect((await migration.up({ rootDir })).updated).toBe(0); + expect(readJson(providersPath).providers['claude-code'].models).toBe('claude-sonnet-4-6'); + }); +});