diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 64c3c57ae..2fe913737 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -19,6 +19,8 @@ ### Fixed +- Fixed `models.yml` schema validation failures being treated as valid config data, so invalid custom provider files now surface a warning instead of silently dropping all custom providers. ([#4305](https://github.com/can1357/oh-my-pi/issues/4305)) + - Fixed stuttering/latency in speech by running synthesis chunks through the player gaplessly - Fixed race condition causing EPIPE errors and broken pipes during speech playback - Fixed interrupted speech audio by ensuring segments queue and drain in order diff --git a/packages/coding-agent/src/cli/models-cli.ts b/packages/coding-agent/src/cli/models-cli.ts index 5bc862ae0..ca43238ec 100644 --- a/packages/coding-agent/src/cli/models-cli.ts +++ b/packages/coding-agent/src/cli/models-cli.ts @@ -84,6 +84,14 @@ function writeLine(line = ""): void { process.stdout.write(`${line}\n`); } +function writeModelsConfigError(error: Error): void { + writeLine(chalk.yellow("Warning: models.yml validation failed — custom providers disabled")); + for (const line of error.message.split("\n")) { + writeLine(` ${line}`); + } + writeLine(); +} + function formatLimit(n: number | null): string { return n === null ? "-" : formatNumber(n); } @@ -187,12 +195,23 @@ function renderProviderModels( } } + const configError = modelRegistry.getError(); + if (json) { + if (configError) { + process.stderr.write( + `Warning: models.yml validation failed — custom providers disabled\n${configError.message}\n`, + ); + } const output: ModelsJson = { models: filtered.slice().sort(byProviderThenId).map(toModelJson) }; writeLine(JSON.stringify(output)); return; } + if (configError) { + writeModelsConfigError(configError); + } + if (available.length === 0) { writeLine("No models available. Set API keys in environment variables."); return; diff --git a/packages/coding-agent/src/config/config-file.ts b/packages/coding-agent/src/config/config-file.ts index 018d25c8f..5c73fc9d9 100644 --- a/packages/coding-agent/src/config/config-file.ts +++ b/packages/coding-agent/src/config/config-file.ts @@ -1,7 +1,7 @@ import * as fs from "node:fs"; import * as path from "node:path"; import { getAgentDir, isEnoent, logger } from "@oh-my-pi/pi-utils"; -import type { Type } from "arktype"; +import { ArkErrors, type Type } from "arktype"; import { JSONC, YAML } from "bun"; /** Minimal subset of the AJV ConfigSchemaError shape this module actually relies on. */ @@ -220,11 +220,11 @@ export class ConfigFile implements IConfigFile { } const checked = this.schema(parsed); - if (checked instanceof Error) { - const schemaErrors: ConfigSchemaError[] = []; - // arktype errors are Error instances with a message property - // Extract the error message as a single schema error - schemaErrors.push({ instancePath: "root", message: checked.message }); + if (checked instanceof ArkErrors) { + const schemaErrors: ConfigSchemaError[] = checked.map(error => ({ + instancePath: error.path.length === 0 ? "root" : error.path.join("."), + message: error.problem, + })); const error = new ConfigError(this.id, schemaErrors); logger.warn("Failed to parse config file", { path: this.path(), error }); return this.#storeCache({ error, status: "error" }); diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index a863e9366..245e10434 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -1383,6 +1383,9 @@ export async function runRootCommand( } if (!isInteractive && !session.model) { + if (modelRegistryError) { + process.stderr.write(`${chalk.red(modelRegistryError.message)}\n\n`); + } if (modelFallbackMessage) { process.stderr.write(`${chalk.red(modelFallbackMessage)}\n`); } else { diff --git a/packages/coding-agent/test/issue-905-repro.test.ts b/packages/coding-agent/test/issue-905-repro.test.ts index 63da70f65..ff665d311 100644 --- a/packages/coding-agent/test/issue-905-repro.test.ts +++ b/packages/coding-agent/test/issue-905-repro.test.ts @@ -85,3 +85,57 @@ test("omp models surfaces extension-registered providers (issue #905)", async () authStorage.close(); } }); + +test("omp models prints invalid models.yml schema errors before listing output", async () => { + const modelsPath = tmp.join("invalid-models.yml"); + await fs.writeFile( + modelsPath, + `providers: + myprovider: + baseUrl: http://localhost:8000/v1 + api: openai-completions + auth: none + compat: + thinkingFormat: deepseek + models: + - id: my-model + name: My Model + reasoning: false + input: [text] + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 } + contextWindow: 8192 + maxTokens: 4096 +`, + ); + + const authStorage = await AuthStorage.create(":memory:"); + try { + const modelRegistry = new ModelRegistry(authStorage, modelsPath); + + const captured: string[] = []; + const originalWrite = process.stdout.write; + Reflect.set(process.stdout, "write", (chunk: string | Uint8Array) => { + captured.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString("utf8")); + return true; + }); + + try { + await runModelsListing({ + modelRegistry, + cwd: tmp.path(), + action: "ls", + pattern: "myprovider", + disableExtensionDiscovery: true, + }); + } finally { + process.stdout.write = originalWrite; + } + + const output = captured.join(""); + expect(output).toContain("Warning: models.yml validation failed — custom providers disabled"); + expect(output).toContain("providers.myprovider.compat.thinkingFormat"); + expect(output).toContain("deepseek"); + } finally { + authStorage.close(); + } +}); diff --git a/packages/coding-agent/test/model-registry.test.ts b/packages/coding-agent/test/model-registry.test.ts index 58f89aa6e..643069d30 100644 --- a/packages/coding-agent/test/model-registry.test.ts +++ b/packages/coding-agent/test/model-registry.test.ts @@ -1230,6 +1230,36 @@ describe("ModelRegistry", () => { expect(nonexistent.getError()).toBeUndefined(); }); + test("invalid models config exposes schema errors instead of silently dropping providers", () => { + writeRawModelsJson({ + myprovider: { + baseUrl: "http://localhost:8000/v1", + api: "openai-completions", + auth: "none", + compat: { thinkingFormat: "deepseek" }, + models: [ + { + id: "my-model", + name: "My Model", + reasoning: false, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 8192, + maxTokens: 4096, + }, + ], + }, + }); + + const invalid = new ModelRegistry(authStorage, modelsJsonPath); + const error = invalid.getError(); + + expect(error?.message).toContain("Failed to load config file models, Schema error"); + expect(error?.message).toContain("providers.myprovider.compat.thinkingFormat"); + expect(error?.message).toContain("deepseek"); + expect(invalid.find("myprovider", "my-model")).toBeUndefined(); + }); + test("model override can change cost fields partially", () => { const sonnet = getModelsForProvider(costPartial, "openrouter").find(m => m.id === "anthropic/claude-sonnet-4"); // Input cost should be overridden