diff --git a/docs/models.md b/docs/models.md index 5ca430d03..3f1ca6221 100644 --- a/docs/models.md +++ b/docs/models.md @@ -1,4 +1,4 @@ -# Model and Provider Configuration (`models.yml`) +# Model and Provider Configuration (`models.yml` / `models.yaml`) This document describes how the coding-agent currently loads models, applies overrides, resolves credentials, and chooses models at runtime. @@ -14,16 +14,17 @@ Primary implementation files: ## Config file location and legacy behavior -Default config path: +Default config paths, in precedence order: - `~/.omp/agent/models.yml` +- `~/.omp/agent/models.yaml` Legacy behavior still present: -- If `models.yml` is missing and `models.json` exists at the same location, it is migrated to `models.yml`. +- If both YAML files are missing and `models.json` exists at the same location, it is migrated to `models.yml`. - Explicit `.json` / `.jsonc` config paths are still supported when passed programmatically to `ModelRegistry`. -## `models.yml` shape +## `models.yml` / `models.yaml` shape ```yaml providers: @@ -158,7 +159,7 @@ Successful command outputs are cached for the process lifetime so the command is ModelRegistry pipeline (on refresh): 1. Load built-in providers/models from `@oh-my-pi/pi-catalog` (`getBundledProviders` / `getBundledModels`). -2. Load `models.yml` custom config. +2. Load `models.yml` / `models.yaml` custom config. 3. Apply provider overrides (`baseUrl`, `headers`, `disableStrictTools`) to built-in models. 4. Apply `modelOverrides` (per provider + model id). 5. Merge custom `models`: @@ -735,11 +736,11 @@ providers: ## Legacy consumer caveat -Most model configuration now flows through `models.yml` via `ModelRegistry`. Explicit `.json` / `.jsonc` paths remain supported only when passed programmatically to `ModelRegistry`; the default user config is `~/.omp/agent/models.yml`. +Most model configuration now flows through `models.yml` / `models.yaml` via `ModelRegistry`. Explicit `.json` / `.jsonc` paths remain supported only when passed programmatically to `ModelRegistry`; the default user config prefers `~/.omp/agent/models.yml`, then falls back to `~/.omp/agent/models.yaml`. ## Failure mode -If `models.yml` fails schema or validation checks: +If `models.yml` / `models.yaml` fails schema or validation checks: - registry keeps operating with built-in models - error is exposed via `ModelRegistry.getError()` and surfaced in UI/notifications diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d8041c95c..7fb747a94 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed custom model/provider config discovery so `~/.omp/agent/models.yaml` loads when `models.yml` is absent, while preserving `.yml` precedence and only migrating legacy `models.json` when neither YAML file exists. ([#5145](https://github.com/can1357/oh-my-pi/issues/5145)) + ## [16.4.2] - 2026-07-10 ### Fixed diff --git a/packages/coding-agent/src/config/config-file.ts b/packages/coding-agent/src/config/config-file.ts index 5c73fc9d9..e7f4d4d73 100644 --- a/packages/coding-agent/src/config/config-file.ts +++ b/packages/coding-agent/src/config/config-file.ts @@ -11,9 +11,9 @@ interface ConfigSchemaError { } /** - * Module-private cache of (jsonPath, ymlPath) pairs we already migrated this - * process. Prevents `ConfigFile.relocate()` / repeated `tryLoad()` calls from - * re-running the migration over and over on the boot path. + * Module-private cache of JSON → YAML migrations this process already ran. + * Prevents `ConfigFile.relocate()` / repeated `tryLoad()` calls from re-running + * the migration over and over on the boot path. */ const migratedPaths = new Set(); @@ -123,6 +123,7 @@ export type LoadResult = export class ConfigFile implements IConfigFile { readonly #basePath: string; + readonly #yamlFallbackPath: string | null; readonly #jsonMigrationPath: string | null; #cache?: LoadResult; #auxValidate?: (value: T) => void; @@ -134,13 +135,17 @@ export class ConfigFile implements IConfigFile { ) { this.#basePath = configPath; if (configPath.endsWith(".yml")) { + this.#yamlFallbackPath = `${configPath.slice(0, -4)}.yaml`; this.#jsonMigrationPath = `${configPath.slice(0, -4)}.json`; } else if (configPath.endsWith(".yaml")) { + this.#yamlFallbackPath = null; this.#jsonMigrationPath = `${configPath.slice(0, -5)}.json`; } else if (configPath.endsWith(".json") || configPath.endsWith(".jsonc")) { + this.#yamlFallbackPath = null; // JSON configs are still supported without migration. this.#jsonMigrationPath = null; } else { + this.#yamlFallbackPath = null; throw new Error(`Invalid config file path: ${configPath}`); } } @@ -150,9 +155,11 @@ export class ConfigFile implements IConfigFile { * Sync callers (tests, settings init) hit this implicitly via {@link tryLoad}. */ #ensureMigrated(): void { - if (this.#jsonMigrationPath) { - migrateJsonToYml(this.#jsonMigrationPath, this.#basePath); + if (!this.#jsonMigrationPath) return; + if (this.#yamlFallbackPath && !fs.existsSync(this.#basePath) && fs.existsSync(this.#yamlFallbackPath)) { + return; } + migrateJsonToYml(this.#jsonMigrationPath, this.#basePath); } relocate(configPath?: string): ConfigFile { @@ -163,9 +170,19 @@ export class ConfigFile implements IConfigFile { return result; } + #resolveReadPath(): string { + if (fs.existsSync(this.#basePath)) { + return this.#basePath; + } + if (this.#yamlFallbackPath && fs.existsSync(this.#yamlFallbackPath)) { + return this.#yamlFallbackPath; + } + return this.#basePath; + } + getMtimeMs(): number | null { try { - return fs.statSync(this.path()).mtimeMs; + return fs.statSync(this.#resolveReadPath()).mtimeMs; } catch (err) { if (isEnoent(err)) return null; throw err; @@ -211,12 +228,13 @@ export class ConfigFile implements IConfigFile { #parseContent(content: string): LoadResult { try { let parsed: unknown; - if (this.#basePath.endsWith(".json") || this.#basePath.endsWith(".jsonc")) { + const readPath = this.#resolveReadPath(); + if (readPath.endsWith(".json") || readPath.endsWith(".jsonc")) { parsed = JSONC.parse(content); - } else if (this.#basePath.endsWith(".yml") || this.#basePath.endsWith(".yaml")) { + } else if (readPath.endsWith(".yml") || readPath.endsWith(".yaml")) { parsed = YAML.parse(content); } else { - throw new Error(`Invalid config file path: ${this.#basePath}`); + throw new Error(`Invalid config file path: ${readPath}`); } const checked = this.schema(parsed); @@ -255,7 +273,7 @@ export class ConfigFile implements IConfigFile { let content: string; try { - content = fs.readFileSync(this.path(), "utf-8").trim(); + content = fs.readFileSync(this.#resolveReadPath(), "utf-8").trim(); } catch (error) { if (isEnoent(error)) { return this.#storeCache({ status: "not-found" }); @@ -275,7 +293,7 @@ export class ConfigFile implements IConfigFile { let content: string; try { - content = (await Bun.file(this.path()).text()).trim(); + content = (await Bun.file(this.#resolveReadPath()).text()).trim(); } catch (error) { if (isEnoent(error)) { return this.#storeCache({ status: "not-found" }); @@ -306,7 +324,7 @@ export class ConfigFile implements IConfigFile { } path(): string { - return this.#basePath; + return this.#resolveReadPath(); } invalidate() { diff --git a/packages/coding-agent/src/config/model-registry.ts b/packages/coding-agent/src/config/model-registry.ts index 7fb1491b6..79196e090 100644 --- a/packages/coding-agent/src/config/model-registry.ts +++ b/packages/coding-agent/src/config/model-registry.ts @@ -258,7 +258,7 @@ export interface ProviderDiscoveryState { error?: string; } -/** Result of loading custom models from models.json */ +/** Result of loading custom models config. */ interface CustomModelsResult { models?: CustomModelOverlay[]; overrides?: Map; @@ -309,7 +309,7 @@ interface CommandApiKeyResolution { value?: string; } /** - * Resolve a models.yml secret/config value to an actual value. + * Resolve a models.yml/models.yaml secret/config value to an actual value. * `!cmd` runs a shell command and returns trimmed stdout, otherwise env vars are * checked first and the input falls back to a literal value. */ @@ -822,7 +822,7 @@ export class ModelRegistry { } /** - * Reload models from disk (built-in + custom from models.json). + * Reload models from disk (built-in + custom config). */ async refresh(strategy: ModelRefreshStrategy = "online-if-uncached"): Promise { this.#reloadStaticModels(); @@ -938,7 +938,7 @@ export class ModelRegistry { #reloadStaticModels(): void { const currentMtime = this.#modelsConfigFile.getMtimeMs(); if (currentMtime !== null && currentMtime === this.#lastStaticLoadMtime) { - // models.json unchanged since last load; reloading would be redundant. + // Models config unchanged since last load; reloading would be redundant. return; } this.#modelsConfigFile.invalidate(); @@ -962,14 +962,14 @@ export class ModelRegistry { } /** - * Get any error from loading models.json (undefined if no error). + * Get any error from loading custom models config (undefined if no error). */ getError(): ConfigError | undefined { return this.#configError; } #loadModels() { - // Load custom models from models.json first (to know which providers to override) + // Load custom config first (to know which providers to override). const { models: customModels = [], overrides = new Map(), @@ -1909,7 +1909,7 @@ export class ModelRegistry { /** * Get all models (built-in + custom). - * If models.json had errors, returns only built-in models. + * If custom config had errors, returns only built-in models. */ getAll(): Model[] { return this.#models; diff --git a/packages/coding-agent/src/config/models-config.ts b/packages/coding-agent/src/config/models-config.ts index cdf0916cb..04ff71c71 100644 --- a/packages/coding-agent/src/config/models-config.ts +++ b/packages/coding-agent/src/config/models-config.ts @@ -1,5 +1,5 @@ /** - * models.json config file handle and provider configuration validation. + * Custom model/provider config file handle and validation. */ import type { Api, ModelSpec } from "@oh-my-pi/pi-ai/types"; diff --git a/packages/coding-agent/test/model-registry-default-config.test.ts b/packages/coding-agent/test/model-registry-default-config.test.ts new file mode 100644 index 000000000..0d2dba3d8 --- /dev/null +++ b/packages/coding-agent/test/model-registry-default-config.test.ts @@ -0,0 +1,196 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import * as fs from "node:fs"; +import * as path from "node:path"; +import { TempDir } from "@oh-my-pi/pi-utils"; + +const packageRoot = path.resolve(import.meta.dir, ".."); + +let tempDir: TempDir; + +describe("ModelRegistry default custom models config", () => { + beforeEach(() => { + tempDir = TempDir.createSync("@model-registry-default-config-"); + }); + + afterEach(async () => { + await tempDir.remove().catch(() => {}); + }); + + test("loads custom provider models from default models.yaml when models.yml is absent", () => { + writeModelsYaml("models.yaml", { + provider: "yaml-default-only", + modelId: "yaml-model", + modelName: "YAML default model", + baseUrl: "https://yaml-default.example.com/v1", + }); + + const model = loadDefaultRegistryModel({ + provider: "yaml-default-only", + modelId: "yaml-model", + }); + + expect(model?.name).toBe("YAML default model"); + expect(model?.baseUrl).toBe("https://yaml-default.example.com/v1"); + }); + + test("prefers default models.yml over models.yaml when both exist", () => { + writeModelsYaml("models.yml", { + provider: "yaml-precedence", + modelId: "from-yml", + modelName: "YML winner", + baseUrl: "https://yml-winner.example.com/v1", + }); + writeModelsYaml("models.yaml", { + provider: "yaml-precedence", + modelId: "from-yaml", + modelName: "YAML loser", + baseUrl: "https://yaml-loser.example.com/v1", + }); + + const ymlModel = loadDefaultRegistryModel({ + provider: "yaml-precedence", + modelId: "from-yml", + }); + const yamlModel = loadDefaultRegistryModel({ + provider: "yaml-precedence", + modelId: "from-yaml", + }); + + expect(ymlModel?.baseUrl).toBe("https://yml-winner.example.com/v1"); + expect(yamlModel).toBeUndefined(); + }); + + test("prefers default models.yaml over legacy models.json when models.yml is absent", () => { + writeModelsYaml("models.yaml", { + provider: "yaml-json-precedence", + modelId: "from-yaml", + modelName: "YAML winner over JSON", + baseUrl: "https://yaml-over-json.example.com/v1", + }); + writeModelsJson({ + provider: "yaml-json-precedence", + modelId: "from-json", + modelName: "JSON loser", + baseUrl: "https://json-loser.example.com/v1", + }); + + const yamlModel = loadDefaultRegistryModel({ + provider: "yaml-json-precedence", + modelId: "from-yaml", + }); + const jsonModel = loadDefaultRegistryModel({ + provider: "yaml-json-precedence", + modelId: "from-json", + }); + + expect(yamlModel?.baseUrl).toBe("https://yaml-over-json.example.com/v1"); + expect(jsonModel).toBeUndefined(); + }); +}); + +interface ProviderFixture { + provider: string; + modelId: string; + modelName: string; + baseUrl: string; +} + +interface ModelLookup { + provider: string; + modelId: string; +} + +interface ModelSnapshot { + provider: string; + id: string; + name: string; + baseUrl: string | undefined; +} + +function writeModelsYaml(file: "models.yml" | "models.yaml", fixture: ProviderFixture): void { + fs.writeFileSync( + path.join(tempDir.path(), file), + [ + "providers:", + ` ${fixture.provider}:`, + ` baseUrl: ${fixture.baseUrl}`, + " apiKey: TEST_KEY", + " api: anthropic-messages", + " models:", + ` - id: ${fixture.modelId}`, + ` name: ${fixture.modelName}`, + " reasoning: false", + " input: [text]", + " cost:", + " input: 0", + " output: 0", + " cacheRead: 0", + " cacheWrite: 0", + " contextWindow: 100000", + " maxTokens: 8000", + "", + ].join("\n"), + ); +} + +function writeModelsJson(fixture: ProviderFixture): void { + fs.writeFileSync( + path.join(tempDir.path(), "models.json"), + JSON.stringify({ + providers: { + [fixture.provider]: { + baseUrl: fixture.baseUrl, + apiKey: "TEST_KEY", + api: "anthropic-messages", + models: [ + { + id: fixture.modelId, + name: fixture.modelName, + reasoning: false, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 100000, + maxTokens: 8000, + }, + ], + }, + }, + }), + ); +} + +function loadDefaultRegistryModel(lookup: ModelLookup): ModelSnapshot | undefined { + const script = ` + import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; + import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; + + const authStorage = await AuthStorage.create(":memory:"); + try { + const registry = new ModelRegistry(authStorage); + const model = registry.find(${JSON.stringify(lookup.provider)}, ${JSON.stringify(lookup.modelId)}); + process.stdout.write(JSON.stringify(model ? { + provider: model.provider, + id: model.id, + name: model.name, + baseUrl: model.baseUrl, + } : null)); + } finally { + authStorage.close(); + } + `; + const result = Bun.spawnSync([process.execPath, "-e", script], { + cwd: packageRoot, + env: { + ...process.env, + PI_CODING_AGENT_DIR: tempDir.path(), + }, + stdout: "pipe", + stderr: "pipe", + }); + const stdout = new TextDecoder().decode(result.stdout).trim(); + const stderr = new TextDecoder().decode(result.stderr).trim(); + if (result.exitCode !== 0) { + throw new Error(`default ModelRegistry lookup failed: ${stderr || stdout || `exit ${result.exitCode}`}`); + } + return JSON.parse(stdout) ?? undefined; +}