From 62c3f1c100adfc3962d22799cb0a2e436dc97f9c Mon Sep 17 00:00:00 2001 From: ParthSareen Date: Sun, 25 Jan 2026 16:15:48 -0800 Subject: [PATCH] fix: prefer explicit config contextWindow/maxTokens over auto-discovered values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: When a model is found in the pi-ai registry (auto-discovered from Ollama), the explicit config in clawdbot.json was completely ignored. This caused models to use the registry's contextWindow (e.g., 8192 from Ollama's /api/show) instead of the user's configured value (e.g., 131072). Error seen: low context window: ollama/glm-4.7:cloud ctx=8192 (warn<32000) source=model blocked model (context window too small): ollama/glm-4.7:cloud ctx=8192 Fix: When a model exists in both the registry AND explicit config, merge the config's contextWindow/maxTokens into the registry model. Only positive numbers are accepted as overrides. Logic: - Model not in registry → use config (existing behavior) - Model in registry + valid config contextWindow/maxTokens → merge config values - Model in registry + no valid overrides → use registry (existing behavior) test: add tests for config contextWindow/maxTokens override behavior Tests cover: - Registry model used when no config override - Config contextWindow/maxTokens override registry values - Invalid values (0, negative) are ignored, registry used instead - Config model used when not in registry - Error returned when model not found anywhere --- src/agents/pi-embedded-runner/model.test.ts | 128 +++++++++++++++++++- src/agents/pi-embedded-runner/model.ts | 58 ++++++--- 2 files changed, 164 insertions(+), 22 deletions(-) diff --git a/src/agents/pi-embedded-runner/model.test.ts b/src/agents/pi-embedded-runner/model.test.ts index b59735623..80780c7bc 100644 --- a/src/agents/pi-embedded-runner/model.test.ts +++ b/src/agents/pi-embedded-runner/model.test.ts @@ -1,15 +1,45 @@ -import { describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; -import { buildInlineProviderModels } from "./model.js"; +vi.mock("@mariozechner/pi-coding-agent", () => ({ + discoverAuthStorage: () => ({ profiles: {} }), + discoverModels: () => ({ + find: (provider: string, modelId: string) => { + if (provider === "ollama" && modelId === "test-model") { + return { + id: "test-model", + name: "test-model", + provider: "ollama", + api: "openai-completions", + reasoning: false, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 8192, + maxTokens: 4096, + }; + } + return null; + }, + }), +})); -const makeModel = (id: string) => ({ +vi.mock("../agent-paths.js", () => ({ + resolveClawdbotAgentDir: () => "/tmp/test-agent", +})); + +vi.mock("../model-compat.js", () => ({ + normalizeModelCompat: (model: T) => model, +})); + +import { buildInlineProviderModels, resolveModel } from "./model.js"; + +const makeModel = (id: string, overrides?: { contextWindow?: number; maxTokens?: number }) => ({ id, name: id, reasoning: false, input: ["text"] as const, cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, - contextWindow: 1, - maxTokens: 1, + contextWindow: overrides?.contextWindow ?? 1, + maxTokens: overrides?.maxTokens ?? 1, }); describe("buildInlineProviderModels", () => { @@ -27,3 +57,91 @@ describe("buildInlineProviderModels", () => { ]); }); }); + +describe("resolveModel", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("uses registry model when no config override exists", () => { + const result = resolveModel("ollama", "test-model"); + + expect(result.model).toBeDefined(); + expect(result.model?.contextWindow).toBe(8192); + expect(result.model?.maxTokens).toBe(4096); + }); + + it("overrides registry contextWindow with valid config value", () => { + const cfg = { + models: { + providers: { + ollama: { + models: [makeModel("test-model", { contextWindow: 131072, maxTokens: 32000 })], + }, + }, + }, + }; + + const result = resolveModel("ollama", "test-model", undefined, cfg); + + expect(result.model).toBeDefined(); + expect(result.model?.contextWindow).toBe(131072); + expect(result.model?.maxTokens).toBe(32000); + }); + + it("ignores config contextWindow of 0", () => { + const cfg = { + models: { + providers: { + ollama: { + models: [makeModel("test-model", { contextWindow: 0 })], + }, + }, + }, + }; + + const result = resolveModel("ollama", "test-model", undefined, cfg); + + expect(result.model?.contextWindow).toBe(8192); // registry value + }); + + it("ignores config contextWindow of negative number", () => { + const cfg = { + models: { + providers: { + ollama: { + models: [makeModel("test-model", { contextWindow: -1 })], + }, + }, + }, + }; + + const result = resolveModel("ollama", "test-model", undefined, cfg); + + expect(result.model?.contextWindow).toBe(8192); // registry value + }); + + it("uses config model when not in registry", () => { + const cfg = { + models: { + providers: { + ollama: { + models: [makeModel("not-in-registry", { contextWindow: 65536 })], + }, + }, + }, + }; + + const result = resolveModel("ollama", "not-in-registry", undefined, cfg); + + expect(result.model).toBeDefined(); + expect(result.model?.contextWindow).toBe(65536); + }); + + it("returns error when model not in registry and no config", () => { + const result = resolveModel("ollama", "unknown-model"); + + expect(result.error).toContain("Unknown model"); + expect(result.model).toBeUndefined(); + }); +}); diff --git a/src/agents/pi-embedded-runner/model.ts b/src/agents/pi-embedded-runner/model.ts index 15248aeaa..dcbe7a7f3 100644 --- a/src/agents/pi-embedded-runner/model.ts +++ b/src/agents/pi-embedded-runner/model.ts @@ -35,6 +35,20 @@ export function buildModelAliasLines(cfg?: ClawdbotConfig) { .map((entry) => `- ${entry.alias}: ${entry.model}`); } +/** Find explicit model config matching provider/modelId */ +function findInlineModelConfig( + cfg: ClawdbotConfig | undefined, + provider: string, + modelId: string, +): InlineModelEntry | undefined { + const providers = cfg?.models?.providers ?? {}; + const inlineModels = buildInlineProviderModels(providers); + const normalizedProvider = normalizeProviderId(provider); + return inlineModels.find( + (entry) => normalizeProviderId(entry.provider) === normalizedProvider && entry.id === modelId, + ); +} + export function resolveModel( provider: string, modelId: string, @@ -49,22 +63,19 @@ export function resolveModel( const resolvedAgentDir = agentDir ?? resolveClawdbotAgentDir(); const authStorage = discoverAuthStorage(resolvedAgentDir); const modelRegistry = discoverModels(authStorage, resolvedAgentDir); - const model = modelRegistry.find(provider, modelId) as Model | null; - if (!model) { - const providers = cfg?.models?.providers ?? {}; - const inlineModels = buildInlineProviderModels(providers); - const normalizedProvider = normalizeProviderId(provider); - const inlineMatch = inlineModels.find( - (entry) => normalizeProviderId(entry.provider) === normalizedProvider && entry.id === modelId, - ); - if (inlineMatch) { - const normalized = normalizeModelCompat(inlineMatch as Model); + const registryModel = modelRegistry.find(provider, modelId) as Model | null; + const inlineConfig = findInlineModelConfig(cfg, provider, modelId); + + // Case 1: Model not in registry - use inline config or fallback + if (!registryModel) { + if (inlineConfig) { return { - model: normalized, + model: normalizeModelCompat(inlineConfig as Model), authStorage, modelRegistry, }; } + const providers = cfg?.models?.providers ?? {}; const providerCfg = providers[provider]; if (providerCfg || modelId.startsWith("mock-")) { const fallbackModel: Model = normalizeModelCompat({ @@ -80,11 +91,24 @@ export function resolveModel( } as Model); return { model: fallbackModel, authStorage, modelRegistry }; } - return { - error: `Unknown model: ${provider}/${modelId}`, - authStorage, - modelRegistry, - }; + return { error: `Unknown model: ${provider}/${modelId}`, authStorage, modelRegistry }; } - return { model: normalizeModelCompat(model), authStorage, modelRegistry }; + + // Case 2: Model in registry - override contextWindow/maxTokens from config if valid + if (inlineConfig) { + const ctx = inlineConfig.contextWindow; + const max = inlineConfig.maxTokens; + const useCtx = typeof ctx === "number" && ctx > 0; + const useMax = typeof max === "number" && max > 0; + if (useCtx || useMax) { + const model: Model = { + ...registryModel, + ...(useCtx && { contextWindow: ctx }), + ...(useMax && { maxTokens: max }), + }; + return { model: normalizeModelCompat(model), authStorage, modelRegistry }; + } + } + + return { model: normalizeModelCompat(registryModel), authStorage, modelRegistry }; }