fix(coding-agent): surfaced invalid models config errors
Converted ArkType validation failures into ConfigError results and surfaced models.yml validation failures in CLI model listing and noninteractive startup. Fixes #4305
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<T> implements IConfigFile<T> {
|
||||
}
|
||||
|
||||
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" });
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
});
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user