diff --git a/bun.lock b/bun.lock index 88baf038..4d935448 100644 --- a/bun.lock +++ b/bun.lock @@ -12,16 +12,16 @@ }, "packages/extension": { "name": "@nikolasd/crew", - "version": "0.4.1", + "version": "0.6.0", "devDependencies": { "@nikolasd/crew-protocol": "workspace:*", - "@oh-my-pi/pi-coding-agent": ">=17.0.7 <18", + "@oh-my-pi/pi-coding-agent": ">=17.0.7 <19", "@types/bun": "1.3.14", "ajv": "8.17.1", "zod": "^4", }, "peerDependencies": { - "@oh-my-pi/pi-coding-agent": ">=17.0.7 <18", + "@oh-my-pi/pi-coding-agent": ">=17.0.7 <19", }, }, "packages/protocol-ts": { diff --git a/docs/manual-testing.md b/docs/manual-testing.md index 7e46b8aa..330a7e58 100644 --- a/docs/manual-testing.md +++ b/docs/manual-testing.md @@ -443,18 +443,30 @@ cargo test -p crew-runtime --test approval which drives `ApprovalService` directly, the same way this walkthrough can't. -### 3e. Adapter model selection and persistence (no model call) - -`crew_profile` registered with an adapter but **no model** triggers ask-on-first-use in an -interactive session: the extension asks which model to use, persists the answer to the -repository's `.omp/crew.json`, and later registrations are silent. - -For `claude`, `codex`, and `copilot` (not `ompRpc`, which has no single catalogue provider to -check against), an explicit `model` is resolved against omp's own model catalogue -(`omp models ls --json`, read fresh every call, never refreshed over the network) plus a small -per-adapter table of vendor-defined aliases, before anything else happens -(`packages/extension/src/models.ts`). There are five outcomes; each is deterministic and -free to exercise (no model call, just `crew_profile` calls): +### 3e. Adapter model selection and persistence + +`crew_profile` registered with an adapter that has **no model configured yet** never accepts a +model as a decision, whatever the caller passed: in an interactive session, crew opens the +model-selection dialog itself and asks the human, offering the current models for that adapter +(omp's own catalogue, or the vendor's own model family table when omp has no catalogue entry for +that provider — see below for which one you'll see). A leader-supplied `model` only preselects a +row in that list if it resolves to one of the options shown; the human's own pick is what actually +gets registered and persisted to the repository's `.omp/crew.json`. Later registrations for that +adapter reuse the stored answer silently, no dialog. + +Without an interactive UI attached, or if the dialog times out with no answer, or if there is +nothing to offer (no catalogue entry and no vendor family table for that adapter), registration is +refused with a typed `model-not-configured` error — nothing is registered or persisted. +`ompRpc` and any caller-defined adapter have no catalogue and no dialog either way: an explicit +`model` is used exactly as given (this is unchanged from before the dialog existed), and omitting +it is what triggers the same `model-not-configured` error for them. + +**Once a model IS configured for an adapter**, an explicit `model` on a later call is never a +second decision — it is resolved against omp's own model catalogue (`omp models ls --json`, read +fresh every call, never refreshed over the network) plus a small per-adapter table of +vendor-defined aliases (`packages/extension/src/models.ts`), and the result is only ever *compared* +to the stored answer. There are five outcomes; each is deterministic and free to exercise (write +`.omp/crew.json` yourself first, then just `crew_profile` calls — no model call, no dialog): | You call `crew_profile` with... | Resolves to | What you see | |---|---|---| @@ -464,17 +476,30 @@ free to exercise (no model call, just `crew_profile` calls): | A shorthand matching **several** catalogue ids | Nothing — refused | `"" matches N models for adapter : , , ... -- name one of them exactly.` (a typed `model-ambiguous` error; nothing is registered or persisted) | | A name omp's catalogue does not know (typo, or a genuinely new/unlisted model) | Used as given, **not persisted** | `model: (not in omp's catalogue for ; passing through UNVERIFIED -- the vendor will reject it if it is wrong)` | +The same resolution also decides which dialog row a leader's suggestion preselects on +first use: a suggestion that resolves `exact`, `alias`, or `match` preselects that row; one that +resolves `ambiguous` or `unverified` (or doesn't resolve at all when the catalogue is down and it +isn't an alias) preselects nothing, and the dialog still opens with every real option on offer — +there is no way to preselect, let alone register, a name nothing could confirm. + The alias table itself is small and vendor-sourced (`claude`: `fable`/`opus`/`sonnet`/`haiku`, read from the installed binary's own `latest_per_family` config, not its `--help` text; `codex`: the single documented `gpt-5.6` → `gpt-5.6-sol`; `copilot`: none, since it defines no aliases crew can verify) — see the doc comment on `VENDOR_ALIASES` in `models.ts` before assuming it needs a new -entry for some other shorthand. +entry for some other shorthand. A test in `models.test.ts` reads this table's `claude` half back +out of the installed `claude` binary's own baked catalogue and fails (or skips, visibly, if the +binary isn't present) when they've drifted apart. + +When omp's catalogue has nothing for an adapter's provider (e.g. no `anthropic` credentials in +omp, so `claude` has none, even though the `claude` binary itself works fine), the dialog falls +back to that same alias table's values instead of refusing outright — `currentModels` in +`models.ts` names which source answered, and the dialog says so in its title. **Persistence follows verification, not success.** An `exact`, `alias`, or `match` resolution is confirmed and gets written to `.omp/crew.json` on first use, same as before model resolution was -added. An `unverified` name runs (the vendor gets the final say) but is deliberately **not** -recorded — pass it again next session, or add it to `.omp/crew.json` yourself once a run has proven -it works. +added — and so is a dialog pick, which is a member of a verified list by construction. An +`unverified` name (only reachable once a model is already configured, comparing an explicit +override to it) runs but is deliberately **not** re-recorded. The existing conflict check still applies, but now compares canonical ids, not raw spelling — a stored `claude-opus-5` and an explicit `opus` are recognized as the same model and accepted as a no-op, diff --git a/packages/extension/dist/index.js b/packages/extension/dist/index.js index a0754e1d..d2a64ff6 100644 --- a/packages/extension/dist/index.js +++ b/packages/extension/dist/index.js @@ -13038,6 +13038,14 @@ function resolutionNote(adapter, r) { function isCataloguedAdapter(adapter) { return Object.hasOwn(PROVIDER_FOR_ADAPTER, adapter); } +function currentModels(adapter, catalogue) { + if (catalogue.available) { + return { available: true, models: catalogue.ids, source: "catalogue" }; + } + const aliases = VENDOR_ALIASES[adapter]; + const familyModels = aliases === undefined ? [] : Array.from(new Set(Object.values(aliases))); + return familyModels.length > 0 ? { available: true, models: familyModels, source: "vendorFamilyTable" } : { available: false }; +} function decideModel(adapter, requested, configured, catalogue) { const resolved = resolveModelName(adapter, requested, catalogue); if (resolved.kind === "ambiguous") { @@ -13057,6 +13065,7 @@ function decideModel(adapter, requested, configured, catalogue) { // src/tools/profiles.ts var CREW_PROFILE_TOOL_NAME = "crew_profile"; +var MODEL_DIALOG_TIMEOUT_MS = 5 * 60 * 1000; var RESERVED_ADAPTER_NAMES = ["claude", "codex", "copilot", "ompRpc"]; function injectTuiMode(adapter, startupOptions) { if (!RESERVED_ADAPTER_NAMES.includes(adapter)) { @@ -13080,10 +13089,17 @@ function modelConflictResult(adapter, configuredModel) { isError: true }; } +function modelNotConfiguredResult(adapter, reason, text) { + return { + content: [{ type: "text", text }], + details: { code: "model-not-configured", adapter, reason }, + isError: true + }; +} function registerProfileTool(pi, ctx) { const params = pi.zod.object({ adapter: pi.zod.string().describe("The adapter name this profile launches, e.g. claude, codex, copilot, ompRpc, terminalDegraded."), - model: pi.zod.string().optional().describe("The model identifier this profile uses. Optional: if omitted and no model is already configured for this adapter (in .omp/crew.json), registration is refused with a typed 'model-not-configured' error -- ask the user which model to use, then call crew_profile again with it. For claude, codex and copilot the value you pass is resolved before anything else happens, against omp's own model catalogue and the vendor's aliases: a vendor alias ('opus', 'haiku') and a name matching exactly one model ('sol' -> gpt-5.6-sol) both resolve to the canonical id, and the result is reported back to you. A name matching several models is refused with a typed 'model-ambiguous' error listing them -- name one of them exactly. A name omp's catalogue does not know is still used, but is reported as UNVERIFIED and is NOT recorded in .omp/crew.json: pass it again next session, or record it there yourself once a run has proven it. The first time a confirmed model is given explicitly for an adapter with none configured, its canonical id is persisted into the repository's .omp/crew.json for future sessions to reuse silently. crew_profile never overwrites an already-recorded model, and never silently ignores an explicit value that conflicts with one: passing a value naming a *different* model than the one already configured is refused with a typed 'model-conflict' error naming the stored value -- correct it by editing the repository's .omp/crew.json directly (/crew config path locates it; /crew config has no set/edit subcommand), never by passing a new value here. Passing a value naming the same model as the configured one is a no-op success, whichever of the two is the shorthand."), + model: pi.zod.string().optional().describe("A suggested model identifier, not a decision. Once a model is already configured for this adapter (in .omp/crew.json), it is authoritative: this parameter is only compared to it (a genuine mismatch is refused with a typed 'model-conflict' error naming the stored value; the same model either way is a no-op success), so pass the previously-registered value or omit it. Before any model is configured, for claude, codex and copilot, crew itself asks the user which model to use via an interactive dialog -- it does not accept this value directly, however it is spelled. Passing a value here still helps: if it resolves against omp's catalogue or the vendor's own aliases ('opus', 'haiku', a name matching exactly one catalogue entry), it is preselected in the dialog, but the user's own pick is what gets used and persisted. If no interactive UI is attached, or the dialog times out with no answer, or there is nothing to offer (no catalogue entry and no vendor alias table for this adapter), registration is refused with a typed 'model-not-configured' error -- register a profile from an interactive session once, or write the model into the repository's .omp/crew.json directly (/crew config path locates it). ompRpc and other adapters omp does not catalogue have no dialog to offer either way: pass the model explicitly, or the same 'model-not-configured' error applies."), startupOptions: pi.zod.record(pi.zod.string(), pi.zod.unknown()).optional().describe("Adapter-specific startup options, tagged by adapter kind, e.g. { claude: { mode: 'tui' } }. For a reserved adapter (claude, codex, copilot, ompRpc), an omitted mode is filled in as 'tui' automatically -- headless is retired. Other options depend on the adapter (see crew-orchestration skill)."), environmentAllowlist: pi.zod.array(pi.zod.string()).optional().describe("Environment variable names this profile's process is allowed to read."), permissionEnvelope: pi.zod.record(pi.zod.string(), pi.zod.unknown()).optional() @@ -13091,7 +13107,7 @@ function registerProfileTool(pi, ctx) { pi.registerTool({ name: CREW_PROFILE_TOOL_NAME, label: "Crew Profile", - description: "Register a reusable worker profile (adapter, model, startup options, environment allowlist) before provisioning workers. Call this once per adapter/model combination, then pass the returned profileId to crew_worker { op: 'create', profileId }. model is optional -- if none is configured yet for this adapter, you'll get a typed error telling you to ask the user which model to use and call this again; that answer is remembered for future sessions. For claude, codex and copilot, model names are resolved against omp's catalogue and the vendor's aliases, so a shorthand or a unique partial name is accepted and echoed back as the canonical id; a name the catalogue does not know still runs, but is flagged UNVERIFIED and is not remembered. mode:'tui' is filled in automatically for reserved adapters when omitted. The profile-first flow (crew_profile \u2192 crew_worker \u2192 crew_run) replaces the legacy fingerprint/adapter/model pattern. Registration is permanent for the lifetime of the runtime's database; there is no update or delete operation, so register a new profile rather than mutating an existing one.", + description: "Register a reusable worker profile (adapter, model, startup options, environment allowlist) before provisioning workers. Call this once per adapter/model combination, then pass the returned profileId to crew_worker { op: 'create', profileId }. model is optional and, for claude, codex and copilot, is never accepted as a decision on the first call for an adapter: with no model configured yet, crew opens an interactive dialog and asks the user itself, offering the current models (omp's catalogue, or the vendor's own model family when omp has no catalogue entry for it) with your suggestion (if any) preselected -- the user's pick is what gets used and remembered, not your suggestion. A typed 'model-not-configured' error means there was no way to ask (no interactive UI, the dialog timed out, or nothing to offer) or, for ompRpc and adapters omp does not catalogue, that no model was given at all. Once a model IS configured for an adapter, it is reused silently forever; a value you pass is only compared to it (mismatch: typed 'model-conflict', naming the stored value; match: no-op success). mode:'tui' is filled in automatically for reserved adapters when omitted. The profile-first flow (crew_profile \u2192 crew_worker \u2192 crew_run) replaces the legacy fingerprint/adapter/model pattern. Registration is permanent for the lifetime of the runtime's database; there is no update or delete operation, so register a new profile rather than mutating an existing one.", parameters: params, approval: () => "exec", async execute(_toolCallId, input, _signal, _onUpdate, extCtx) { @@ -13112,46 +13128,70 @@ function registerProfileTool(pi, ctx) { let model; let note; let mayPersist; - if (input.model !== undefined && isCataloguedAdapter(input.adapter)) { - const catalogue = await (ctx.readModelCatalogue ?? readCatalogue)(input.adapter); - const decision = decideModel(input.adapter, input.model, configuredModel, catalogue); - if (decision.kind === "conflict") { - return modelConflictResult(input.adapter, decision.configuredModel); + if (configuredModel !== undefined) { + if (input.model !== undefined && isCataloguedAdapter(input.adapter)) { + const catalogue = await (ctx.readModelCatalogue ?? readCatalogue)(input.adapter); + const decision = decideModel(input.adapter, input.model, configuredModel, catalogue); + if (decision.kind === "conflict") { + return modelConflictResult(input.adapter, decision.configuredModel); + } + if (decision.kind === "ambiguous") { + return { + content: [ + { + type: "text", + text: `"${decision.from}" matches ${decision.candidates.length} models for adapter ${input.adapter}: ${decision.candidates.join(", ")} -- name one of them exactly.` + } + ], + details: { code: "model-ambiguous", adapter: input.adapter, requested: decision.from, candidates: decision.candidates }, + isError: true + }; + } + model = decision.model; + note = decision.note; + mayPersist = decision.verified; + } else { + if (input.model !== undefined && input.model !== configuredModel) { + return modelConflictResult(input.adapter, configuredModel); + } + model = configuredModel; + note = undefined; + mayPersist = true; } - if (decision.kind === "ambiguous") { - return { - content: [ - { - type: "text", - text: `"${decision.from}" matches ${decision.candidates.length} models for adapter ${input.adapter}: ${decision.candidates.join(", ")} -- name one of them exactly.` - } - ], - details: { code: "model-ambiguous", adapter: input.adapter, requested: decision.from, candidates: decision.candidates }, - isError: true - }; + } else if (!isCataloguedAdapter(input.adapter)) { + if (input.model === undefined) { + return modelNotConfiguredResult(input.adapter, "no-model-given", `no model configured for adapter ${input.adapter} -- ask the user which model to use, then call crew_profile again with it; the answer will be persisted for future sessions.`); } - model = decision.model; - note = decision.note; - mayPersist = decision.verified; + model = input.model; + note = undefined; + mayPersist = true; + } else if (!extCtx.hasUI) { + return modelNotConfiguredResult(input.adapter, "no-ui", `no model configured for adapter ${input.adapter}, and no interactive UI is attached to ask which one to use -- register a profile from an interactive session once, or set the model in the repository's .omp/crew.json directly.`); } else { - if (input.model !== undefined && configuredModel !== undefined && input.model !== configuredModel) { - return modelConflictResult(input.adapter, configuredModel); + const catalogue = await (ctx.readModelCatalogue ?? readCatalogue)(input.adapter); + const options = currentModels(input.adapter, catalogue); + if (!options.available) { + return modelNotConfiguredResult(input.adapter, "no-current-models", `no model configured for adapter ${input.adapter}, and there is nothing to offer -- omp's catalogue has no entry for it and it has no reviewed vendor model-family table either.`); + } + const sourceLabel = options.source === "catalogue" ? "omp's model catalogue" : "the vendor's own model family table (omp's catalogue has no entry for this adapter)"; + let initialIndex; + if (input.model !== undefined) { + const suggested = resolveModelName(input.adapter, input.model, catalogue); + const suggestedModel = suggested.kind === "ambiguous" || suggested.kind === "unverified" ? undefined : suggested.model; + const idx = suggestedModel === undefined ? -1 : options.models.indexOf(suggestedModel); + if (idx >= 0) { + initialIndex = idx; + } } - const chosen = input.model ?? configuredModel; - if (chosen === undefined) { - return { - content: [ - { - type: "text", - text: `no model configured for adapter ${input.adapter} -- ask the user which model to use, then call crew_profile again with it; the answer will be persisted for future sessions.` - } - ], - details: { code: "model-not-configured", adapter: input.adapter }, - isError: true - }; + const pick = await extCtx.ui.select(`Model for the ${input.adapter} worker (from ${sourceLabel})`, [...options.models], { + timeout: MODEL_DIALOG_TIMEOUT_MS, + initialIndex + }); + if (pick === undefined) { + return modelNotConfiguredResult(input.adapter, "dialog-timeout", `no model configured for adapter ${input.adapter}, and the model-selection dialog timed out with no answer.`); } - model = chosen; - note = undefined; + model = pick; + note = `model: ${model} (chosen from ${sourceLabel})`; mayPersist = true; } const client = await ctx.getClient(extCtx); @@ -13167,7 +13207,7 @@ function registerProfileTool(pi, ctx) { if (result.isError !== true && note !== undefined) { result.content.push({ type: "text", text: note }); } - if (result.isError !== true && input.model !== undefined && configuredModel === undefined) { + if (result.isError !== true && configuredModel === undefined) { if (!mayPersist) { result.content.push({ type: "text", diff --git a/packages/extension/skills/crew-orchestration/SKILL.md b/packages/extension/skills/crew-orchestration/SKILL.md index 4d44a0ee..76c8379d 100644 --- a/packages/extension/skills/crew-orchestration/SKILL.md +++ b/packages/extension/skills/crew-orchestration/SKILL.md @@ -13,7 +13,7 @@ Crew executes durable worker runs. OMP owns the task graph and the leader's deci To run any task via Claude, Codex, Copilot, or OMP-RPC: -1. **Register a profile** — `crew_profile { adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }`. The profile captures adapter, model, startup options, and environment allowlist. Receives a `profileId`. `model` is optional-with-ask-once: if omitted and none is configured yet for that adapter, you get a typed `model-not-configured` error instead of a guess — ask the user which model to use by offering the vendor's stable aliases (e.g., "sonnet", "opus", "haiku" for Claude), never dated model IDs, then call `crew_profile` again with it; the answer is persisted into the repo's `.omp/crew.json` and reused silently on every later call, for that adapter, for good. Passing a model that *conflicts* with one already configured is refused too, with a typed `model-conflict` error naming the stored value — `crew_profile` never overwrites it and never silently ignores the conflict; the fix is editing the repository's `.omp/crew.json` directly (`/crew config path` locates it — `/crew config` has no set/edit subcommand). `mode: "tui"` is filled in automatically for a reserved adapter (claude, codex, copilot, ompRpc) when omitted — headless is retired, but you never have to remember to spell out the replacement. +1. **Register a profile** — `crew_profile { adapter: "claude", startupOptions: { claude: { mode: "tui" } } }`. The profile captures adapter, model, startup options, and environment allowlist. Receives a `profileId`. `model` is optional and, until one is configured for that adapter, is never a decision you make on the leader's behalf: omit it (or pass a suggestion, e.g. "sonnet") and, in an interactive session, crew opens the model-selection dialog itself and asks the human — your suggestion is only preselected, not accepted. That answer is persisted into the repo's `.omp/crew.json` and reused silently on every later call, for that adapter, for good; once it's set, this parameter is only ever compared to it. A typed `model-not-configured` error means there was no way to ask (no interactive UI, the dialog timed out, or nothing to offer). Passing a model that *conflicts* with one already configured is refused with a typed `model-conflict` error naming the stored value — `crew_profile` never overwrites it and never silently ignores the conflict; the fix is editing the repository's `.omp/crew.json` directly (`/crew config path` locates it — `/crew config` has no set/edit subcommand). `mode: "tui"` is filled in automatically for a reserved adapter (claude, codex, copilot, ompRpc) when omitted — headless is retired, but you never have to remember to spell out the replacement. 2. **Create a worker** — `crew_worker { op: "create", profileId }`. The worker identity binds to that profile permanently. Receives a `workerId`. 3. **Submit a run** — `crew_run { op: "submit", taskId, workerId, prompt }` to execute a task. The prompt is the full instruction; Crew stores identity, not prose. diff --git a/packages/extension/src/models.test.ts b/packages/extension/src/models.test.ts index 4f4d6d6e..2b6ff940 100644 --- a/packages/extension/src/models.test.ts +++ b/packages/extension/src/models.test.ts @@ -10,8 +10,10 @@ // subset fixture can make an ambiguous input look unique, which is the one // thing these tests exist to catch. +import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; import { expect, test } from "bun:test"; -import { type Catalogue, decideModel, isCataloguedAdapter, readCatalogue, resolutionNote, resolveModelName } from "./models"; +import { type Catalogue, VENDOR_ALIASES, currentModels, decideModel, isCataloguedAdapter, readCatalogue, resolutionNote, resolveModelName } from "./models"; /** Every `anthropic` id omp catalogues. */ const ANTHROPIC: Catalogue = { @@ -293,3 +295,116 @@ test("a name that could not be checked because the catalogue was unavailable is const d = decideModel("codex", "gpt-5.6-sol", undefined, { available: false, why: "omp not on PATH" }); expect(d.kind === "use" && d.verified).toBe(false); }); + +// ------------------------------------------------------- currentModels + +test("currentModels offers the catalogue when one is available, and says so", () => { + const result = currentModels("claude", ANTHROPIC); + expect(result).toEqual({ available: true, models: ANTHROPIC.ids, source: "catalogue" }); +}); + +test("currentModels falls back to the vendor's own family table when the catalogue has nothing for this provider", () => { + // The real scenario this exists for: a machine with no `anthropic` + // credentials in omp, so `readCatalogue("claude")` is unavailable even + // though the claude binary itself works fine. + const result = currentModels("claude", { available: false, why: "omp's catalogue lists no models for provider `anthropic`" }); + expect(result.available).toBe(true); + expect(result.available && result.source).toBe("vendorFamilyTable"); + expect(result.available && [...result.models].sort()).toEqual(["claude-fable-5-1", "claude-haiku-4-5", "claude-opus-5", "claude-sonnet-5"]); +}); + +test("currentModels reports unavailable when neither the catalogue nor a family table has anything -- ompRpc", () => { + const result = currentModels("ompRpc" as never, { available: false, why: "no single catalogue provider bounds the ompRpc adapter" }); + expect(result).toEqual({ available: false }); +}); + +test("currentModels reports unavailable for copilot when the catalogue is down -- it has no reviewed aliases", () => { + const result = currentModels("copilot", { available: false, why: "omp not on PATH" }); + expect(result).toEqual({ available: false }); +}); + +// ------------------------------------------- VENDOR_ALIASES drift guard + +/** + * `VENDOR_ALIASES.claude`'s values are also what `currentModels` offers the + * model-ask dialog when omp's catalogue has nothing for `claude` -- so this + * table silently drifting from the installed binary is no longer a + * cosmetic problem, it is the dialog offering models that no longer exist + * or omitting the vendor's current one. + * + * Reads `latest_per_family` out of the INSTALLED claude binary rather than + * trusting the table on faith: that object is baked into the binary's own + * bundled model catalogue (greppable off the raw bytes -- a few hundred + * megabytes read and searched in well under a second, measured). Skips + * visibly, rather than failing, when the binary can't be found or read: + * CI has no vendor CLI, and a test that fails there is a test that gets + * deleted. + * + * This legitimately fails on a machine whose installed claude is older or + * newer than this table -- that is the guard doing its job, not a defect + * in the table, which is what the failure message says. + */ +function findClaudeBinary(): string | undefined { + try { + const path = execFileSync("which", ["claude"], { encoding: "utf8" }).trim(); + return path === "" ? undefined : path; + } catch { + return undefined; + } +} + +function extractLatestPerFamily(source: string): Record | undefined { + const block = /latest_per_family:\{([^}]*)\}/.exec(source); + if (!block) { + return undefined; + } + const entries: Record = {}; + for (const entry of block[1]!.matchAll(/(\w+):"([^"]*)"/g)) { + entries[entry[1]!] = entry[2]!; + } + return Object.keys(entries).length > 0 ? entries : undefined; +} + +const claudeBinaryPath = findClaudeBinary(); +let installedLatestPerFamily: Record | undefined; +let skipReason: string | undefined; +if (claudeBinaryPath === undefined) { + skipReason = "`claude` is not on PATH"; +} else { + try { + installedLatestPerFamily = extractLatestPerFamily(readFileSync(claudeBinaryPath).toString("latin1")); + if (installedLatestPerFamily === undefined) { + skipReason = `could not find a \`latest_per_family\` object in ${claudeBinaryPath}`; + } + } catch (err) { + skipReason = `could not read ${claudeBinaryPath}: ${err instanceof Error ? err.message : String(err)}`; + } +} +if (skipReason !== undefined) { + // Visible, not silent: printed even though the test itself is skipped, + // so a reader of CI output sees the guard was skipped and why, not + // nothing. + console.warn(`SKIPPING VENDOR_ALIASES.claude drift guard: ${skipReason}`); +} + +function sortedEntries(record: Readonly> | undefined): [string, string][] { + return Object.entries(record ?? {}).sort(([a], [b]) => a.localeCompare(b)); +} + +test.skipIf(skipReason !== undefined)("VENDOR_ALIASES.claude matches the installed claude binary's latest_per_family", () => { + const table = VENDOR_ALIASES.claude; + const installed = installedLatestPerFamily; + const tableSorted = sortedEntries(table); + const installedSorted = sortedEntries(installed); + if (JSON.stringify(tableSorted) !== JSON.stringify(installedSorted)) { + throw new Error( + `VENDOR_ALIASES.claude has drifted from the installed claude binary's latest_per_family.\n` + + ` table (models.ts): ${JSON.stringify(table)}\n` + + ` installed (${claudeBinaryPath}): ${JSON.stringify(installed)}\n` + + `Update VENDOR_ALIASES.claude to match, and re-check whether the vendor added a family the ` + + `model-ask dialog should now offer. (If this machine's claude is simply older or newer than ` + + `the table expects, that is this guard working as intended, not a defect in the table.)`, + ); + } + expect(tableSorted).toEqual(installedSorted); +}); diff --git a/packages/extension/src/models.ts b/packages/extension/src/models.ts index 7ffc479a..bae28a2f 100644 --- a/packages/extension/src/models.ts +++ b/packages/extension/src/models.ts @@ -9,8 +9,12 @@ // // VALIDATION comes from omp's own model catalogue (`omp models --json`), // which crew can read for free because crew runs INSIDE omp. It lists -// canonical ids per provider -- 725 models across 7 providers on the -// machine this was written on, three of which are crew's adapters. +// canonical ids per provider, read fresh on every call rather than cached +// -- but it is scoped to providers the local omp installation holds +// credentials for, which is a different question from "does the vendor +// CLI accept this name". A machine with no `anthropic` credentials in omp +// has an empty catalogue for `claude` even though the claude binary itself +// works fine (see `currentModels`'s fallback below for what that implies). // // ALIAS RESOLUTION cannot come from the catalogue, because the catalogue // holds canonical ids and display names and never aliases: `openai-codex` @@ -79,7 +83,7 @@ const PROVIDER_FOR_ADAPTER: Partial> = { * shorthand for copilot would be a mapping we owned and could not verify, * so there is none. */ -const VENDOR_ALIASES: Partial>>> = { +export const VENDOR_ALIASES: Partial>>> = { claude: { fable: "claude-fable-5-1", opus: "claude-opus-5", @@ -271,6 +275,43 @@ export function isCataloguedAdapter(adapter: string): adapter is Adapter { return Object.hasOwn(PROVIDER_FOR_ADAPTER, adapter); } +/** Which source answered a `currentModels()` call. */ +export type ModelListSource = "catalogue" | "vendorFamilyTable"; + +/** The list `crew_profile`'s model-ask dialog offers, and which source built it. */ +export type CurrentModels = { readonly available: true; readonly models: readonly string[]; readonly source: ModelListSource } | { readonly available: false }; + +/** + * The "current" models to offer for `adapter`, in the order the maintainer's + * ruling defines: omp's own catalogue first (it needs no interpretation -- + * its ids are current by construction), the vendor's own family table when + * the catalogue has nothing for this provider, and neither when both come up + * empty. + * + * The family-table fallback exists because omp's catalogue is scoped to + * providers it holds credentials for, which is a different question from + * "does the vendor CLI accept this model" -- on a machine with no anthropic + * credentials in omp, `readCatalogue("claude")` is unavailable even though + * the claude CLI itself works fine and has its own opinion of what's + * current. `VENDOR_ALIASES`' values are exactly that opinion for the + * adapters it lists; deduplicated since more than one alias can name the + * same canonical id (not true today, but the table makes no promise + * against it). + * + * This is a fallback of last resort, not a second catalogue: `ompRpc` and + * any caller-defined adapter have no entry in `VENDOR_ALIASES` at all, and + * `copilot`'s is empty (no aliases it can source), so both correctly report + * unavailable here when the catalogue is also down. + */ +export function currentModels(adapter: Adapter, catalogue: Catalogue): CurrentModels { + if (catalogue.available) { + return { available: true, models: catalogue.ids, source: "catalogue" }; + } + const aliases = VENDOR_ALIASES[adapter]; + const familyModels = aliases === undefined ? [] : Array.from(new Set(Object.values(aliases))); + return familyModels.length > 0 ? { available: true, models: familyModels, source: "vendorFamilyTable" } : { available: false }; +} + /** What `crew_profile` should do with the model it was given. */ export type ModelOutcome = /** diff --git a/packages/extension/src/tools/profiles.test.ts b/packages/extension/src/tools/profiles.test.ts index c158bf0e..8d727668 100644 --- a/packages/extension/src/tools/profiles.test.ts +++ b/packages/extension/src/tools/profiles.test.ts @@ -72,8 +72,32 @@ function fakeClient(handler?: (method: string, params: unknown) => unknown) { return { client, calls }; } -function fakeExtCtx(cwd: string): ExtensionContext { - return { cwd } as unknown as ExtensionContext; +/** + * A UI stub for the model-ask dialog. Defaults to "no UI attached" (the + * fail-closed case most existing tests don't care about); pass `select` to + * simulate an interactive session, and it records every `select()` call so + * a test can assert on the title/options/preselection the dialog was + * actually given. + */ +function fakeUi(select?: (title: string, options: readonly string[], dialogOptions?: { initialIndex?: number }) => string | undefined) { + const calls: Array<{ title: string; options: readonly string[]; initialIndex: number | undefined }> = []; + return { + calls, + select: async (title: string, options: readonly string[], dialogOptions?: { initialIndex?: number }): Promise => { + calls.push({ title, options, initialIndex: dialogOptions?.initialIndex }); + return select?.(title, options, dialogOptions); + }, + }; +} + +function fakeExtCtx(cwd: string, ui?: ReturnType): ExtensionContext { + return (ui === undefined ? { cwd, hasUI: false } : { cwd, hasUI: true, ui }) as unknown as ExtensionContext; +} + +/** A `select` that always confirms whatever row the dialog preselected -- + * simulates a user who accepts the leader's own suggestion verbatim. */ +function confirmPreselected(_title: string, options: readonly string[], dialogOptions?: { initialIndex?: number }): string | undefined { + return dialogOptions?.initialIndex === undefined ? undefined : options[dialogOptions.initialIndex]; } function setupProfileTool(client: CrewClient, readModelCatalogue: (adapter: Adapter) => Promise = stubCatalogue) { @@ -104,22 +128,26 @@ function writeRepoConfig(repository: string, contents: string): string { // ------------------------------------------------------- resolution order -// Resolution changed what these two assert. `sonnet` is claude's own alias, so -// what reaches the daemon is now the canonical id it names -- the adapter -// gets an unambiguous model, and the value crew records is one whose meaning -// cannot move (claude's config carries an `alias_migration` map). -test("crew_profile resolves an explicit alias to the canonical id the daemon receives", async () => { +// `sonnet` is claude's own alias, so it resolves to the canonical id the +// daemon receives -- but only once a human confirms it in the dialog, since +// no model is configured yet for this adapter. The leader's suggestion +// preselects the row; `confirmPreselected` simulates a user accepting it. +test("crew_profile resolves an explicit alias to the canonical id the daemon receives, once the dialog confirms it", async () => { const repository = tempRepo(); const { client, calls } = fakeClient(); + const ui = fakeUi(confirmPreselected); const { tool } = setupProfileTool(client); - const result = await tool({ adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository)); + const result = await tool({ adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository, ui)); expect(result.isError).not.toBe(true); const register = calls.find((c) => c.method === "profile/register"); expect((register!.params as { model: string }).model).toBe("claude-sonnet-5"); // Never silently: the caller is told what their input became. expect(result.content.some((c) => "text" in c && c.text.includes("claude-sonnet-5"))).toBe(true); + // The suggestion was resolved to the row it preselected, not accepted raw. + expect(ui.calls[0]?.options).toContain("claude-sonnet-5"); + expect(ui.calls[0]?.options[ui.calls[0]!.initialIndex!]).toBe("claude-sonnet-5"); }); test("crew_profile treats an explicit model matching the configured one as a no-op success", async () => { @@ -178,14 +206,31 @@ test("crew_profile returns a typed model-not-configured error, and never calls p expect(calls.map((c) => c.method)).not.toContain("profile/register"); }); +// A leader-supplied model must not change this outcome: with no configured +// model and no interactive UI to ask through, crew fails closed regardless +// of what (or whether) the leader suggested -- the ask, not the suggestion, +// is what's missing. +test("crew_profile returns a typed model-not-configured error even with an explicit model, when there is no UI to ask through", async () => { + const repository = tempRepo(); + const { client, calls } = fakeClient(); + const { tool } = setupProfileTool(client); + + const result = await tool({ adapter: "claude", model: "claude-sonnet-4-6-20260215", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository)); + + expect(result.isError).toBe(true); + expect((result.details as { code: string; reason: string }).reason).toBe("no-ui"); + expect(calls.map((c) => c.method)).not.toContain("profile/register"); +}); + // ------------------------------------------------------------- persistence test("crew_profile persists an explicit model into the repo layer when none was configured", async () => { const repository = tempRepo(); const { client } = fakeClient(); + const ui = fakeUi(confirmPreselected); const { tool } = setupProfileTool(client); - await tool({ adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository)); + await tool({ adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository, ui)); const written = JSON.parse(readFileSync(join(repository, ".omp", "crew.json"), "utf8")); // The resolved id, not the spelling passed in: a persisted alias is a @@ -197,9 +242,10 @@ test("crew_profile preserves existing keys when persisting", async () => { const repository = tempRepo(); writeRepoConfig(repository, '{"approval":"auto","adapters":{"codex":{"model":"gpt-5"}}}'); const { client } = fakeClient(); + const ui = fakeUi(confirmPreselected); const { tool } = setupProfileTool(client); - await tool({ adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository)); + await tool({ adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository, ui)); const written = JSON.parse(readFileSync(join(repository, ".omp", "crew.json"), "utf8")); expect(written.approval).toBe("auto"); @@ -229,8 +275,9 @@ test("crew_profile never persists when profile/register itself failed", async () }, } as unknown as CrewClient; const { tool } = setupProfileTool(rejecting); + const ui = fakeUi(confirmPreselected); - const result = await tool({ adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository)); + const result = await tool({ adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository, ui)); expect(result.isError).toBe(true); expect(() => readFileSync(join(repository, ".omp", "crew.json"), "utf8")).toThrow(); @@ -251,8 +298,9 @@ test("crew_profile warns rather than throwing when persistence fails after a suc }, } as unknown as CrewClient; const { tool } = setupProfileTool(racy); + const ui = fakeUi(confirmPreselected); - const result = await tool({ adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository)); + const result = await tool({ adapter: "claude", model: "sonnet", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository, ui)); expect(result.isError).not.toBe(true); expect(result.content.some((c) => "text" in c && c.text.includes("Warning"))).toBe(true); @@ -263,9 +311,10 @@ test("crew_profile warns rather than throwing when persistence fails after a suc test("crew_profile fills in mode: tui for a reserved adapter when the caller omits it", async () => { const repository = tempRepo(); const { client, calls } = fakeClient(); + const ui = fakeUi(confirmPreselected); const { tool } = setupProfileTool(client); - await tool({ adapter: "claude", model: "sonnet" }, fakeExtCtx(repository)); + await tool({ adapter: "claude", model: "sonnet" }, fakeExtCtx(repository, ui)); const register = calls.find((c) => c.method === "profile/register"); const startupOptions = (register!.params as { startupOptions: Record }).startupOptions; @@ -289,12 +338,15 @@ test("injectTuiMode preserves other keys already present on the reserved adapter test("`sol` reaches the daemon as gpt-5.6-sol -- the maintainer's example, end to end", async () => { // No alias table entry exists for bare `sol`. It resolves because it is // the only openai-codex id containing it, which is why crew does not own - // a mapping for the shorthand that motivated the ticket. + // a mapping for the shorthand that motivated the ticket -- and, since + // nothing is configured for codex yet, it only preselects that row; the + // dialog still decides. const repository = tempRepo(); const { client, calls } = fakeClient(); + const ui = fakeUi(confirmPreselected); const { tool } = setupProfileTool(client); - const result = await tool({ adapter: "codex", model: "sol" }, fakeExtCtx(repository)); + const result = await tool({ adapter: "codex", model: "sol" }, fakeExtCtx(repository, ui)); expect(result.isError).not.toBe(true); expect((calls.find((c) => c.method === "profile/register")!.params as { model: string }).model).toBe("gpt-5.6-sol"); @@ -316,8 +368,13 @@ test("a stored shorthand and an explicit canonical id are one model, not a confl expect((calls.find((c) => c.method === "profile/register")!.params as { model: string }).model).toBe("claude-opus-5"); }); -test("an ambiguous model is refused by name, and never registered", async () => { +// Ambiguity is a `decideModel` concern, reachable now only through the +// configured branch (an explicit override compared against a stored +// value) -- with nothing configured, an ambiguous suggestion just fails to +// preselect a dialog row (covered below) rather than refusing outright. +test("an ambiguous model is refused by name, and never registered, when it conflicts with an already-configured one", async () => { const repository = tempRepo(); + writeRepoConfig(repository, '{"adapters":{"codex":{"model":"gpt-5.5"}}}'); const { client, calls } = fakeClient(); const { tool } = setupProfileTool(client); @@ -330,49 +387,87 @@ test("an ambiguous model is refused by name, and never registered", async () => expect(calls.map((c) => c.method)).not.toContain("profile/register"); }); -test("an unverified model registers but is NOT written to crew.json, and says so", async () => { - // This is the ticket's actual symptom: an invented dated id became the - // repository's durable answer. It may run this once -- the vendor is the - // second line of defence -- but nothing confirmed it, so nothing records it. +// The ticket's actual symptom, closed a second way: an invented dated id +// can no longer become the repository's durable answer at all, because it +// can't even be typed into existence -- with nothing configured, a name +// that isn't a real model just fails to preselect anything, and the human +// still has to pick one of the real options shown. +test("an invented model name preselects nothing in the dialog, and the human's real pick -- not the suggestion -- is what registers and persists", async () => { const repository = tempRepo(); const { client, calls } = fakeClient(); + const ui = fakeUi(() => "claude-opus-5"); const { tool } = setupProfileTool(client); - const result = await tool({ adapter: "claude", model: "claude-sonnet-4-6-20260215" }, fakeExtCtx(repository)); + const result = await tool({ adapter: "claude", model: "claude-sonnet-4-6-20260215", startupOptions: { claude: { mode: "tui" } } }, fakeExtCtx(repository, ui)); expect(result.isError).not.toBe(true); - expect((calls.find((c) => c.method === "profile/register")!.params as { model: string }).model).toBe("claude-sonnet-4-6-20260215"); - expect(() => readFileSync(join(repository, ".omp", "crew.json"), "utf8")).toThrow(); - const text = result.content.filter((c) => "text" in c).map((c) => (c as { text: string }).text); - expect(text.some((t) => t.includes("UNVERIFIED"))).toBe(true); - expect(text.some((t) => t.includes("Not persisted"))).toBe(true); + expect(ui.calls[0]?.initialIndex).toBeUndefined(); + expect((calls.find((c) => c.method === "profile/register")!.params as { model: string }).model).toBe("claude-opus-5"); + expect(JSON.parse(readFileSync(join(repository, ".omp", "crew.json"), "utf8")).adapters.claude.model).toBe("claude-opus-5"); }); -test("an unreachable catalogue still registers -- an absent validator never refuses everything", async () => { +test("catalogue unavailable for the provider -- the dialog offers the vendor's own family table instead, and says so", async () => { + // `gpt-5.6` is codex's own documented alias for `gpt-5.6-sol` -- the + // suggestion resolves through the alias table even with the catalogue + // down, so it still preselects the matching family-table row. const repository = tempRepo(); const { client, calls } = fakeClient(); + const ui = fakeUi(confirmPreselected); const { tool } = setupProfileTool(client, NO_CATALOGUE); - const result = await tool({ adapter: "codex", model: "gpt-5.6-sol" }, fakeExtCtx(repository)); + const result = await tool({ adapter: "codex", model: "gpt-5.6" }, fakeExtCtx(repository, ui)); expect(result.isError).not.toBe(true); expect((calls.find((c) => c.method === "profile/register")!.params as { model: string }).model).toBe("gpt-5.6-sol"); - expect(result.content.some((c) => "text" in c && c.text.includes("could not be verified"))).toBe(true); - // Not written down: correct or not, nothing checked it this time. - expect(() => readFileSync(join(repository, ".omp", "crew.json"), "utf8")).toThrow(); + expect(ui.calls[0]?.options).toEqual(["gpt-5.6-sol"]); + expect(ui.calls[0]?.title).toContain("vendor's own model family table"); + expect(result.content.some((c) => "text" in c && c.text.includes("family table"))).toBe(true); + expect(JSON.parse(readFileSync(join(repository, ".omp", "crew.json"), "utf8")).adapters.codex.model).toBe("gpt-5.6-sol"); }); -test("a local alias still resolves when the catalogue is unreachable, and is persisted", async () => { +test("a local alias still resolves when the catalogue is unreachable, preselects, and is persisted once confirmed", async () => { const repository = tempRepo(); const { client, calls } = fakeClient(); + const ui = fakeUi(confirmPreselected); const { tool } = setupProfileTool(client, NO_CATALOGUE); - await tool({ adapter: "claude", model: "haiku" }, fakeExtCtx(repository)); + await tool({ adapter: "claude", model: "haiku" }, fakeExtCtx(repository, ui)); expect((calls.find((c) => c.method === "profile/register")!.params as { model: string }).model).toBe("claude-haiku-4-5"); + expect(ui.calls[0]?.options[ui.calls[0]!.initialIndex!]).toBe("claude-haiku-4-5"); expect(JSON.parse(readFileSync(join(repository, ".omp", "crew.json"), "utf8")).adapters.claude.model).toBe("claude-haiku-4-5"); }); +test("dialog timeout returns a typed model-not-configured error, and never calls profile/register", async () => { + const repository = tempRepo(); + const { client, calls } = fakeClient(); + const ui = fakeUi(() => undefined); // simulates extCtx.ui.select resolving undefined on timeout + const { tool } = setupProfileTool(client); + + const result = await tool({ adapter: "claude", model: "sonnet" }, fakeExtCtx(repository, ui)); + + expect(result.isError).toBe(true); + expect((result.details as { code: string; reason: string }).reason).toBe("dialog-timeout"); + expect(calls.map((c) => c.method)).not.toContain("profile/register"); + expect(() => readFileSync(join(repository, ".omp", "crew.json"), "utf8")).toThrow(); +}); + +test("a leader-supplied model never bypasses the dialog -- the pick wins even when it differs from the suggestion", async () => { + const repository = tempRepo(); + const { client, calls } = fakeClient(); + // The leader suggests opus; the human picks sonnet instead. + const ui = fakeUi(() => "claude-sonnet-5"); + const { tool } = setupProfileTool(client); + + const result = await tool({ adapter: "claude", model: "opus" }, fakeExtCtx(repository, ui)); + + expect(result.isError).not.toBe(true); + // The suggestion was still preselected -- the dialog just wasn't bound to it. + expect(ui.calls[0]?.options[ui.calls[0]!.initialIndex!]).toBe("claude-opus-5"); + expect((calls.find((c) => c.method === "profile/register")!.params as { model: string }).model).toBe("claude-sonnet-5"); + expect(JSON.parse(readFileSync(join(repository, ".omp", "crew.json"), "utf8")).adapters.claude.model).toBe("claude-sonnet-5"); +}); + test("an adapter omp does not catalogue behaves exactly as it did before", async () => { // ompRpc has no provider and no alias source, so resolution could only // annotate a name nothing checked. Withholding persistence there would diff --git a/packages/extension/src/tools/profiles.ts b/packages/extension/src/tools/profiles.ts index 001057da..b226644c 100644 --- a/packages/extension/src/tools/profiles.ts +++ b/packages/extension/src/tools/profiles.ts @@ -8,12 +8,16 @@ import type { AgentToolResult, ExtensionAPI } from "@oh-my-pi/pi-coding-agent"; import { homedir } from "node:os"; import { CrewConfigError, persistConfiguredModel, resolveConfiguredModel } from "../crew-config"; -import { decideModel, isCataloguedAdapter, readCatalogue } from "../models"; +import { currentModels, decideModel, isCataloguedAdapter, readCatalogue, resolveModelName } from "../models"; import type { OrchestrationToolContext } from "./shared"; import { callOrchestration } from "./shared"; export const CREW_PROFILE_TOOL_NAME = "crew_profile"; +/** How long the model-ask dialog waits for a pick before timing out, same + * window `plan.ts`'s and `approval-ui.ts`'s dialogs use. */ +const MODEL_DIALOG_TIMEOUT_MS = 5 * 60 * 1000; + /** The four reserved adapter kinds `crates/runtime/src/adapter/profile.rs`'s * `AdapterKind::RESERVED_NAMES` also declares -- kept in sync manually, * since this list isn't part of the generated protocol types. */ @@ -58,6 +62,21 @@ function modelConflictResult(adapter: string, configuredModel: string): AgentToo }; } +/** + * The refusal for every "no answer exists, and none can be obtained" path. + * One `code` throughout (`model-not-configured`) since a caller's retry + * logic only needs to know registration didn't happen; `reason` in + * `details` and the message text say WHY, since that differs by adapter + * and by whether crew could even try to ask. + */ +function modelNotConfiguredResult(adapter: string, reason: "no-model-given" | "no-ui" | "no-current-models" | "dialog-timeout", text: string): AgentToolResult { + return { + content: [{ type: "text", text }], + details: { code: "model-not-configured", adapter, reason }, + isError: true, + }; +} + export function registerProfileTool(pi: ExtensionAPI, ctx: OrchestrationToolContext): void { const params = pi.zod.object({ adapter: pi.zod.string().describe("The adapter name this profile launches, e.g. claude, codex, copilot, ompRpc, terminalDegraded."), @@ -65,7 +84,7 @@ export function registerProfileTool(pi: ExtensionAPI, ctx: OrchestrationToolCont .string() .optional() .describe( - "The model identifier this profile uses. Optional: if omitted and no model is already configured for this adapter (in .omp/crew.json), registration is refused with a typed 'model-not-configured' error -- ask the user which model to use, then call crew_profile again with it. For claude, codex and copilot the value you pass is resolved before anything else happens, against omp's own model catalogue and the vendor's aliases: a vendor alias ('opus', 'haiku') and a name matching exactly one model ('sol' -> gpt-5.6-sol) both resolve to the canonical id, and the result is reported back to you. A name matching several models is refused with a typed 'model-ambiguous' error listing them -- name one of them exactly. A name omp's catalogue does not know is still used, but is reported as UNVERIFIED and is NOT recorded in .omp/crew.json: pass it again next session, or record it there yourself once a run has proven it. The first time a confirmed model is given explicitly for an adapter with none configured, its canonical id is persisted into the repository's .omp/crew.json for future sessions to reuse silently. crew_profile never overwrites an already-recorded model, and never silently ignores an explicit value that conflicts with one: passing a value naming a *different* model than the one already configured is refused with a typed 'model-conflict' error naming the stored value -- correct it by editing the repository's .omp/crew.json directly (/crew config path locates it; /crew config has no set/edit subcommand), never by passing a new value here. Passing a value naming the same model as the configured one is a no-op success, whichever of the two is the shorthand.", + "A suggested model identifier, not a decision. Once a model is already configured for this adapter (in .omp/crew.json), it is authoritative: this parameter is only compared to it (a genuine mismatch is refused with a typed 'model-conflict' error naming the stored value; the same model either way is a no-op success), so pass the previously-registered value or omit it. Before any model is configured, for claude, codex and copilot, crew itself asks the user which model to use via an interactive dialog -- it does not accept this value directly, however it is spelled. Passing a value here still helps: if it resolves against omp's catalogue or the vendor's own aliases ('opus', 'haiku', a name matching exactly one catalogue entry), it is preselected in the dialog, but the user's own pick is what gets used and persisted. If no interactive UI is attached, or the dialog times out with no answer, or there is nothing to offer (no catalogue entry and no vendor alias table for this adapter), registration is refused with a typed 'model-not-configured' error -- register a profile from an interactive session once, or write the model into the repository's .omp/crew.json directly (/crew config path locates it). ompRpc and other adapters omp does not catalogue have no dialog to offer either way: pass the model explicitly, or the same 'model-not-configured' error applies.", ), startupOptions: pi.zod .record(pi.zod.string(), pi.zod.unknown()) @@ -79,7 +98,7 @@ export function registerProfileTool(pi: ExtensionAPI, ctx: OrchestrationToolCont name: CREW_PROFILE_TOOL_NAME, label: "Crew Profile", description: - "Register a reusable worker profile (adapter, model, startup options, environment allowlist) before provisioning workers. Call this once per adapter/model combination, then pass the returned profileId to crew_worker { op: 'create', profileId }. model is optional -- if none is configured yet for this adapter, you'll get a typed error telling you to ask the user which model to use and call this again; that answer is remembered for future sessions. For claude, codex and copilot, model names are resolved against omp's catalogue and the vendor's aliases, so a shorthand or a unique partial name is accepted and echoed back as the canonical id; a name the catalogue does not know still runs, but is flagged UNVERIFIED and is not remembered. mode:'tui' is filled in automatically for reserved adapters when omitted. The profile-first flow (crew_profile → crew_worker → crew_run) replaces the legacy fingerprint/adapter/model pattern. Registration is permanent for the lifetime of the runtime's database; there is no update or delete operation, so register a new profile rather than mutating an existing one.", + "Register a reusable worker profile (adapter, model, startup options, environment allowlist) before provisioning workers. Call this once per adapter/model combination, then pass the returned profileId to crew_worker { op: 'create', profileId }. model is optional and, for claude, codex and copilot, is never accepted as a decision on the first call for an adapter: with no model configured yet, crew opens an interactive dialog and asks the user itself, offering the current models (omp's catalogue, or the vendor's own model family when omp has no catalogue entry for it) with your suggestion (if any) preselected -- the user's pick is what gets used and remembered, not your suggestion. A typed 'model-not-configured' error means there was no way to ask (no interactive UI, the dialog timed out, or nothing to offer) or, for ompRpc and adapters omp does not catalogue, that no model was given at all. Once a model IS configured for an adapter, it is reused silently forever; a value you pass is only compared to it (mismatch: typed 'model-conflict', naming the stored value; match: no-op success). mode:'tui' is filled in automatically for reserved adapters when omitted. The profile-first flow (crew_profile → crew_worker → crew_run) replaces the legacy fingerprint/adapter/model pattern. Registration is permanent for the lifetime of the runtime's database; there is no update or delete operation, so register a new profile rather than mutating an existing one.", parameters: params, approval: () => "exec", async execute(_toolCallId, input, _signal, _onUpdate, extCtx) { @@ -98,74 +117,110 @@ export function registerProfileTool(pi: ExtensionAPI, ctx: OrchestrationToolCont throw err; } - // A hallucinating leader inventing a model name is the original - // symptom -- crew_profile must never let an explicit param silently - // clobber (nor silently lose to) an already-persisted choice. An - // explicit model that *conflicts* with the stored one is refused, - // named, with the correction path spelled out; the *same* explicit - // value as already stored is a no-op success (nothing to persist, - // nothing to reject). - // - // Resolution goes in front of that comparison, for the - // adapters omp catalogues. `haiku` and a stored `claude-haiku-4-5` are - // one model, so comparing the spellings would refuse a correct call - // with an error telling the user to edit a file that is already right. - // `decideModel` owns the comparison and the ambiguity rule together -- - // see its doc comment for why each ordering is load-bearing. + // The ask is driven by whether a user answer already exists + // (`configuredModel`), never by whether the leader supplied one. A + // hallucinating leader inventing a model name was the original + // symptom, and a leader-supplied model silently bypassing the ask + // when one is due was a second instance of the same shape of bug -- + // both are closed by making the STORED answer, not the caller's + // input, the only thing that skips asking. let model: string; let note: string | undefined; // Whether persistence is allowed to write this model down. Withholding // it is only justified where a check was available and came back - // negative, which is why the non-catalogued path below leaves it true. + // negative, which is why the ompRpc/dialog paths below leave it true: + // an ompRpc model can't be checked at all, and a dialog pick is by + // construction a member of a list something already verified. let mayPersist: boolean; - if (input.model !== undefined && isCataloguedAdapter(input.adapter)) { - const catalogue = await (ctx.readModelCatalogue ?? readCatalogue)(input.adapter); - const decision = decideModel(input.adapter, input.model, configuredModel, catalogue); - if (decision.kind === "conflict") { - return modelConflictResult(input.adapter, decision.configuredModel); + if (configuredModel !== undefined) { + // A user answer already exists. Unchanged from before the ask + // existed: an explicit, catalogued input is resolved and compared + // to the stored value (`decideModel` owns the comparison and the + // ambiguity rule together -- see its doc comment for why each + // ordering is load-bearing); anything else reuses the stored text + // exactly as recorded. + if (input.model !== undefined && isCataloguedAdapter(input.adapter)) { + const catalogue = await (ctx.readModelCatalogue ?? readCatalogue)(input.adapter); + const decision = decideModel(input.adapter, input.model, configuredModel, catalogue); + if (decision.kind === "conflict") { + return modelConflictResult(input.adapter, decision.configuredModel); + } + if (decision.kind === "ambiguous") { + return { + content: [ + { + type: "text", + text: `"${decision.from}" matches ${decision.candidates.length} models for adapter ${input.adapter}: ${decision.candidates.join(", ")} -- name one of them exactly.`, + }, + ], + details: { code: "model-ambiguous", adapter: input.adapter, requested: decision.from, candidates: decision.candidates }, + isError: true, + }; + } + model = decision.model; + note = decision.note; + mayPersist = decision.verified; + } else { + if (input.model !== undefined && input.model !== configuredModel) { + return modelConflictResult(input.adapter, configuredModel); + } + // A model taken from `.omp/crew.json` is used exactly as recorded. It + // is already the repository's answer, and re-resolving it could turn + // a stored value that has always worked into an ambiguity error on a + // call that passed no model at all. + model = configuredModel; + note = undefined; + mayPersist = true; } - if (decision.kind === "ambiguous") { - return { - content: [ - { - type: "text", - text: `"${decision.from}" matches ${decision.candidates.length} models for adapter ${input.adapter}: ${decision.candidates.join(", ")} -- name one of them exactly.`, - }, - ], - details: { code: "model-ambiguous", adapter: input.adapter, requested: decision.from, candidates: decision.candidates }, - isError: true, - }; + } else if (!isCataloguedAdapter(input.adapter)) { + // `ompRpc` and any caller-defined adapter have no catalogue and no + // alias source, so there is no bounded list crew could offer a + // dialog against -- resolution, and the ask, could only annotate a + // name nothing checked. Unchanged from before the ask existed: use + // whatever was given, or fail closed if nothing was. + if (input.model === undefined) { + return modelNotConfiguredResult(input.adapter, "no-model-given", `no model configured for adapter ${input.adapter} -- ask the user which model to use, then call crew_profile again with it; the answer will be persisted for future sessions.`); } - model = decision.model; - note = decision.note; - mayPersist = decision.verified; + model = input.model; + note = undefined; + mayPersist = true; + } else if (!extCtx.hasUI) { + // A catalogued adapter with no configured model needs a user + // answer, and no interactive UI is attached to collect one. Fails + // closed rather than accepting the leader's suggestion as though it + // were that answer -- that silent accept was the bug. + return modelNotConfiguredResult(input.adapter, "no-ui", `no model configured for adapter ${input.adapter}, and no interactive UI is attached to ask which one to use -- register a profile from an interactive session once, or set the model in the repository's .omp/crew.json directly.`); } else { - // No catalogue and no alias source for this adapter (`ompRpc`, or a - // caller-defined one), or no explicit model to resolve: behaviour - // here is exactly what it was before resolution existed. - if (input.model !== undefined && configuredModel !== undefined && input.model !== configuredModel) { - return modelConflictResult(input.adapter, configuredModel); + // The ask: crew opens the dialog itself rather than instructing the + // leader to ask and call back. A leader-supplied `input.model` never + // gets accepted on its own say-so here -- it only preselects a row + // when it resolves to one of the offered models, so the human's own + // pick is what is actually used. + const catalogue = await (ctx.readModelCatalogue ?? readCatalogue)(input.adapter); + const options = currentModels(input.adapter, catalogue); + if (!options.available) { + return modelNotConfiguredResult(input.adapter, "no-current-models", `no model configured for adapter ${input.adapter}, and there is nothing to offer -- omp's catalogue has no entry for it and it has no reviewed vendor model-family table either.`); } - const chosen = input.model ?? configuredModel; - if (chosen === undefined) { - return { - content: [ - { - type: "text", - text: `no model configured for adapter ${input.adapter} -- ask the user which model to use, then call crew_profile again with it; the answer will be persisted for future sessions.`, - }, - ], - details: { code: "model-not-configured", adapter: input.adapter }, - isError: true, - }; + const sourceLabel = options.source === "catalogue" ? "omp's model catalogue" : "the vendor's own model family table (omp's catalogue has no entry for this adapter)"; + let initialIndex: number | undefined; + if (input.model !== undefined) { + const suggested = resolveModelName(input.adapter, input.model, catalogue); + const suggestedModel = suggested.kind === "ambiguous" || suggested.kind === "unverified" ? undefined : suggested.model; + const idx = suggestedModel === undefined ? -1 : options.models.indexOf(suggestedModel); + if (idx >= 0) { + initialIndex = idx; + } } - // A model taken from `.omp/crew.json` is used exactly as recorded. It - // is already the repository's answer, and re-resolving it could turn - // a stored value that has always worked into an ambiguity error on a - // call that passed no model at all. - model = chosen; - note = undefined; + const pick = await extCtx.ui.select(`Model for the ${input.adapter} worker (from ${sourceLabel})`, [...options.models], { + timeout: MODEL_DIALOG_TIMEOUT_MS, + initialIndex, + }); + if (pick === undefined) { + return modelNotConfiguredResult(input.adapter, "dialog-timeout", `no model configured for adapter ${input.adapter}, and the model-selection dialog timed out with no answer.`); + } + model = pick; + note = `model: ${model} (chosen from ${sourceLabel})`; mayPersist = true; } @@ -187,7 +242,12 @@ export function registerProfileTool(pi: ExtensionAPI, ctx: OrchestrationToolCont result.content.push({ type: "text", text: note }); } - if (result.isError !== true && input.model !== undefined && configuredModel === undefined) { + // `configuredModel === undefined` alone is the right gate now: every + // path that reaches here with it undefined either resolved an + // explicit `input.model` (the ompRpc/custom-adapter fallback) or + // collected a dialog pick, and both are exactly the "first time this + // adapter gets an answer" cases persistence exists for. + if (result.isError !== true && configuredModel === undefined) { if (!mayPersist) { // The failure resolution exists for: an invented dated id became the // repository's durable answer in `.omp/crew.json`. A name omp's