diff --git a/extensions/auto-router-classify.ts b/extensions/auto-router-classify.ts index f1b05fc..08452c4 100644 --- a/extensions/auto-router-classify.ts +++ b/extensions/auto-router-classify.ts @@ -4,6 +4,21 @@ import type { ModelRegistry } from "@earendil-works/pi-coding-agent"; import type { AutoRouterEffortLevel } from "./auto-router-settings.js"; const CLASSIFY_TIMEOUT_MS = 15_000; +/** + * Generous compared to the single word this call actually needs, but bounded: a reasoning-capable + * classifier model can spend real output budget on reasoning content before ever emitting the + * answer, and the previous fixed cap of 20 starved that to nothing (verified). Uncapped isn't the + * right fix either though - `CLASSIFY_TIMEOUT_MS` only bounds wall-clock time, not tokens, so a + * model that reasons at length but still finishes quickly could otherwise run up real cost on what + * is supposed to be a cheap per-turn triage call. There's no explicit `reasoningEffort` set (see + * below), so a reasoning model's default reasoning depth for this trivial a prompt is unmeasured - + * sized generously to hedge against that uncertainty rather than tuned from real data. Still tiny + * next to a real agent turn's own token usage (input context there dwarfs this call's output cap + * by orders of magnitude - the two aren't comparable). If replies still come back empty/truncated + * at this size, that's a signal to control reasoning effort per-provider instead of just raising + * this further. + */ +export const CLASSIFY_MAX_TOKENS = 8_000; const VALID_LEVELS: readonly AutoRouterEffortLevel[] = [ "minimal", "low", @@ -14,6 +29,22 @@ const VALID_LEVELS: readonly AutoRouterEffortLevel[] = [ ]; const DEFAULT_LEVEL: AutoRouterEffortLevel = "medium"; +/** + * APIs whose raw `reasoningEffort` field is verified (against each module's own type + * declaration) to accept the full `AutoRouterEffortLevel` vocabulary (minus "off", handled + * separately below). Passing it to any other API either does nothing - most providers (Anthropic, + * Z.ai, MiniMax, OpenCode Go, ...) have no such field at all - or, worse, sends a value invalid + * for that API's own narrower enum: Mistral's `reasoningEffort` only accepts "none" | "high", so + * "medium" would be exactly the kind of invalid-value bug this allowlist exists to avoid + * repeating. + */ +const REASONING_EFFORT_SAFE_APIS: ReadonlySet = new Set([ + "openai-completions", + "openai-responses", + "azure-openai-responses", + "openai-codex-responses", +]); + const SYSTEM_PROMPT = `You triage the complexity of a single upcoming coding-agent turn so it can be routed to an appropriately capable model. Reply with exactly one word, lowercase, no punctuation: minimal, low, medium, high, xhigh, or max. - minimal: rote, no real reasoning needed. A one-word answer, a pure formatting pass, a trivial rename, echoing back something already known. @@ -41,6 +72,14 @@ export type ClassificationResult = { * reply" after the fact, since the model call itself is never persisted anywhere else. */ reply: string; + /** + * True when `level` is the `medium` default because the classifier call errored, timed out, or + * came back with no recognizable level word - not because the model actually judged the turn to + * be medium complexity. Callers should surface this (a notification, a log line) rather than let + * it pass as an ordinary classification: silently defaulting with no visible signal is exactly + * what made a real, sustained classifier failure indistinguishable from normal routing. + */ + failed: boolean; }; function numeric(value: unknown): number { @@ -48,15 +87,20 @@ function numeric(value: unknown): number { } /** - * Classify a turn's complexity using the given (default/medium-tier) model. Never throws and - * never blocks indefinitely: a bounded timeout, a provider error, or an unparseable reply all - * fall back to `medium` so classification can never stall or break the user's turn. + * Classify a turn's complexity using the given (default/medium-tier) model, reasoning at + * `reasoningEffort` - the same effort this model is actually dispatched at for real work (its own + * configured override, or its tier's name), so the classify call doesn't reason at some unrelated + * provider default. Never throws and never blocks indefinitely: a bounded timeout, a provider + * error, or an unparseable reply all fall back to `medium` so classification can never stall or + * break the user's turn - but that fallback is reported via `failed: true` rather than silently, + * so a caller can still tell a real judgment apart from a classifier that never actually answered. */ export async function classifyTurnComplexity( modelRegistry: ModelRegistry, model: Model, prompt: string, hasImages: boolean, + reasoningEffort: AutoRouterEffortLevel, ): Promise { const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), CLASSIFY_TIMEOUT_MS); @@ -64,6 +108,13 @@ export async function classifyTurnComplexity( const text = hasImages ? `${prompt}\n\n(This turn also includes attached images.)` : prompt; + // "off" isn't a valid raw reasoningEffort value on any API observed (that's the bug this + // whole thing started from), and only route it through at all on APIs verified to accept our + // effort vocabulary - see REASONING_EFFORT_SAFE_APIS. + const rawReasoningEffort = + reasoningEffort !== "off" && REASONING_EFFORT_SAFE_APIS.has(model.api) + ? reasoningEffort + : undefined; const response = await modelRegistry.complete( model, { @@ -78,16 +129,12 @@ export async function classifyTurnComplexity( }, { signal: controller.signal, - reasoningEffort: "off", cacheRetention: "none", sessionId: uuidv7(), - // No maxTokens cap: a small fixed budget (previously 20) is plenty for a non-reasoning - // model's one-word reply, but a reasoning-capable model can spend the entire budget on - // reasoning content before ever emitting the answer - `reasoningEffort: "off"` should - // suppress that, but isn't honored the same way by every provider/model, and when it - // isn't, a tight cap starves the visible reply to nothing rather than just some reasoning - // tokens (verified: this is what was happening). `CLASSIFY_TIMEOUT_MS` above is the real - // bound on how long/expensive a stuck classification call can get. + maxTokens: CLASSIFY_MAX_TOKENS, + ...(rawReasoningEffort !== undefined + ? { reasoningEffort: rawReasoningEffort } + : {}), }, ); const reply = response.content @@ -115,10 +162,30 @@ export async function classifyTurnComplexity( cost: numeric(response.usage.cost?.total), } : undefined; - return { level, usage, reply: reply || "(empty reply)" }; + // `String.match()` returns `null`, not `undefined`, when nothing matches. + const failed = match === null; + // `stopReason: "length"` means the model hit CLASSIFY_MAX_TOKENS before finishing - distinct + // from a model that finished cleanly but just didn't say a recognizable level word. Naming + // the actual cause here means a diagnosis doesn't have to be guessed at: it's directly + // actionable (raise CLASSIFY_MAX_TOKENS, or this model needs less reasoning effort) rather + // than indistinguishable from any other reason the reply came back empty. + const reasonSuffix = + failed && response.stopReason === "length" + ? ` (hit CLASSIFY_MAX_TOKENS=${CLASSIFY_MAX_TOKENS} before answering)` + : ""; + return { + level, + usage, + reply: `${reply || "(empty reply)"}${reasonSuffix}`, + failed, + }; } catch (error) { const message = error instanceof Error ? error.message : String(error); - return { level: DEFAULT_LEVEL, reply: `(classification failed: ${message})` }; + return { + level: DEFAULT_LEVEL, + reply: `(classification failed: ${message})`, + failed: true, + }; } finally { clearTimeout(timeout); } diff --git a/extensions/auto-router-health.ts b/extensions/auto-router-health.ts index fdd1c2e..73e4748 100644 --- a/extensions/auto-router-health.ts +++ b/extensions/auto-router-health.ts @@ -57,11 +57,15 @@ export type ClassificationLogEntry = { model: ModelIdentity; }; -function truncateForLog(text: string): string { +/** Collapses whitespace and bounds length - shared with the live "classifier failed" UI + * notification, since an unusable reply can now run up to `CLASSIFY_MAX_TOKENS` long and would + * otherwise flood a one-line notification the same way it would this log. */ +export function truncateForLog( + text: string, + limit: number = CLASSIFICATION_LOG_TEXT_LIMIT, +): string { const collapsed = text.replace(/\s+/g, " ").trim(); - return collapsed.length > CLASSIFICATION_LOG_TEXT_LIMIT - ? `${collapsed.slice(0, CLASSIFICATION_LOG_TEXT_LIMIT)}…` - : collapsed; + return collapsed.length > limit ? `${collapsed.slice(0, limit)}…` : collapsed; } export type UsageDelta = { input: number; output: number; cost: number }; diff --git a/extensions/auto-router.ts b/extensions/auto-router.ts index 35486c7..b394aae 100644 --- a/extensions/auto-router.ts +++ b/extensions/auto-router.ts @@ -16,6 +16,7 @@ import { type ModelHealthEntry, type ModelIdentity, modelKey, + truncateForLog, } from "./auto-router-health.js"; import { normalizeModelId, reconcileProviderQuota } from "./auto-router-quota.js"; import { @@ -352,10 +353,8 @@ export default async function autoRouter(pi: ExtensionAPI): Promise { level = "(pinned)"; classifierReply = `pinned to ${pinnedTier} - not classified`; } else { - const classifierPool = resolveAvailableModels( - ctx.modelRegistry, - settings.efforts.medium?.models ?? allConfiguredModels(settings), - ); + const classifierRefs = settings.efforts.medium?.models ?? allConfiguredModels(settings); + const classifierPool = resolveAvailableModels(ctx.modelRegistry, classifierRefs); const classifierRef = healthStore.pickHealthy(classifierPool); const classifierModel = classifierRef ? classifierPool.find( @@ -367,17 +366,35 @@ export default async function autoRouter(pi: ExtensionAPI): Promise { let classifiedLevel: AutoRouterEffortLevel = "medium"; classifierReply = "(no classifier available)"; if (classifierModel) { + // Same effort this model would actually be dispatched at for real work in the medium + // tier - its own configured override, or "medium" itself - so the classify call reasons + // at the level the user configured for it rather than an unrelated provider default. + const classifierEffort = resolveEffort(classifierRefs, classifierModel, "medium"); const result = await classifyTurnComplexity( ctx.modelRegistry, classifierModel, prompt, hasImages, + classifierEffort, ); classifiedLevel = result.level; classifierReply = result.reply; if (result.usage) { healthStore.recordSuccess(modelKey(classifierModel), result.usage); } + // The classifier defaulting to `medium` isn't a real judgment of this turn's complexity + // when it failed to answer at all - that's silently indistinguishable from a genuine + // medium verdict otherwise, which is exactly what let a sustained classifier failure go + // unnoticed. Surface it visibly rather than let it pass as ordinary routing. + if (result.failed && ctx.hasUI) { + // A failed reply can now run up to CLASSIFY_MAX_TOKENS long (e.g. a reasoning model + // that never got to its answer) - bound it here the same way it's already bounded for + // /usage below, so one long reply can't flood this notification and bury the warning. + ctx.ui.notify( + `Auto: classifier gave no usable answer (${truncateForLog(result.reply, 500)}); defaulting to ${classifiedLevel} for this turn. Check /usage for details.`, + "warning", + ); + } } level = classifiedLevel; tier = resolveEffortTier(settings, classifiedLevel); diff --git a/tests/auto-router-classify.test.ts b/tests/auto-router-classify.test.ts index 6e417b8..79b0175 100644 --- a/tests/auto-router-classify.test.ts +++ b/tests/auto-router-classify.test.ts @@ -1,10 +1,20 @@ import { expect, test } from "bun:test"; import type { Api, Model } from "@earendil-works/pi-ai"; import type { ModelRegistry } from "@earendil-works/pi-coding-agent"; -import { classifyTurnComplexity } from "../extensions/auto-router-classify.ts"; +import { CLASSIFY_MAX_TOKENS, classifyTurnComplexity } from "../extensions/auto-router-classify.ts"; import type { AutoRouterEffortLevel } from "../extensions/auto-router-settings.ts"; const MODEL = { provider: "prov", id: "classifier" } as unknown as Model; +const CODEX_MODEL = { + provider: "openai-codex", + id: "classifier", + api: "openai-codex-responses", +} as unknown as Model; +const UNLISTED_API_MODEL = { + provider: "anthropic", + id: "classifier", + api: "anthropic-messages", +} as unknown as Model; function registryReplying( text: string, @@ -18,6 +28,40 @@ function registryReplying( } as unknown as ModelRegistry; } +function registryStoppingAt( + text: string, + stopReason: string, +): ModelRegistry { + return { + complete: async () => ({ + content: [{ type: "text", text }], + usage: undefined, + stopReason, + }), + } as unknown as ModelRegistry; +} + +/** Captures the raw options object passed to `complete()`, so a test can assert on the actual + * request shape rather than just the parsed result - the only way to catch a regression like the + * invalid `reasoningEffort: "off"` this suite didn't previously guard against. */ +function registryCapturingOptions(text: string): { + registry: ModelRegistry; + options: () => Record | undefined; +} { + let captured: Record | undefined; + const registry = { + complete: async ( + _model: Model, + _context: unknown, + options: Record, + ) => { + captured = options; + return { content: [{ type: "text", text }], usage: undefined }; + }, + } as unknown as ModelRegistry; + return { registry, options: () => captured }; +} + function throwingRegistry(): ModelRegistry { return { complete: async () => { @@ -30,7 +74,7 @@ const ALL_LEVELS: AutoRouterEffortLevel[] = ["minimal", "low", "medium", "high", for (const level of ALL_LEVELS) { test(`classifyTurnComplexity parses a bare "${level}" reply`, async () => { - const result = await classifyTurnComplexity(registryReplying(level), MODEL, "do something", false); + const result = await classifyTurnComplexity(registryReplying(level), MODEL, "do something", false, "medium"); expect(result.level).toBe(level); }); } @@ -41,6 +85,7 @@ test("classifyTurnComplexity parses the reply case-insensitively with surroundin MODEL, "do something", false, + "medium", ); expect(result.level).toBe("max"); }); @@ -53,6 +98,7 @@ test("classifyTurnComplexity picks the level word the model actually led with, n MODEL, "do something", false, + "medium", ); expect(result.level).toBe("high"); }); @@ -63,20 +109,73 @@ test("classifyTurnComplexity picks the level word the model actually led with, e MODEL, "do something", false, + "medium", + ); + expect(result.level).toBe("medium"); +}); + +test("classifyTurnComplexity falls back to medium on an unparseable reply, and flags it as failed", async () => { + const result = await classifyTurnComplexity( + registryReplying("uh, tricky one"), + MODEL, + "do something", + false, + "medium", ); expect(result.level).toBe("medium"); + // `failed: true` is what lets a caller tell "the model actually said medium" apart from "the + // model said nothing usable, so this is just the fallback" - a distinction that matters because + // the two look identical if only `level` is checked, which is exactly what let a real, sustained + // classifier failure pass as ordinary routing undetected. + expect(result.failed).toBe(true); +}); + +test("classifyTurnComplexity flags a completely empty reply as failed too, not just unparseable text", async () => { + const result = await classifyTurnComplexity(registryReplying(""), MODEL, "do something", false, "medium"); + expect(result.level).toBe("medium"); + expect(result.reply).toBe("(empty reply)"); + expect(result.failed).toBe(true); +}); + +test("classifyTurnComplexity names CLASSIFY_MAX_TOKENS truncation specifically, not just \"(empty reply)\"", async () => { + // stopReason "length" means the model hit the token cap before finishing - directly actionable + // (raise the cap, or this model needs less reasoning effort) rather than indistinguishable from + // any other reason the reply came back unusable. + const result = await classifyTurnComplexity( + registryStoppingAt("", "length"), + MODEL, + "do something", + false, + "medium", + ); + expect(result.failed).toBe(true); + expect(result.reply).toContain(`CLASSIFY_MAX_TOKENS=${CLASSIFY_MAX_TOKENS}`); }); -test("classifyTurnComplexity falls back to medium on an unparseable reply", async () => { - const result = await classifyTurnComplexity(registryReplying("uh, tricky one"), MODEL, "do something", false); +test("classifyTurnComplexity does not claim truncation when the reply is just empty for some other reason", async () => { + const result = await classifyTurnComplexity( + registryStoppingAt("", "stop"), + MODEL, + "do something", + false, + "medium", + ); + expect(result.failed).toBe(true); + expect(result.reply).not.toContain("CLASSIFY_MAX_TOKENS"); +}); + +test("classifyTurnComplexity does not flag a genuine parsed verdict as failed", async () => { + const result = await classifyTurnComplexity(registryReplying("medium"), MODEL, "do something", false, "medium"); expect(result.level).toBe("medium"); + expect(result.failed).toBe(false); }); test("classifyTurnComplexity falls back to medium when the provider call throws, and records why in reply", async () => { - const result = await classifyTurnComplexity(throwingRegistry(), MODEL, "do something", false); + const result = await classifyTurnComplexity(throwingRegistry(), MODEL, "do something", false, "medium"); expect(result.level).toBe("medium"); expect(result.usage).toBeUndefined(); expect(result.reply).toContain("provider down"); + expect(result.failed).toBe(true); }); test("classifyTurnComplexity surfaces the raw reply text alongside the parsed level, for diagnosing mismatches later", async () => { @@ -85,6 +184,7 @@ test("classifyTurnComplexity surfaces the raw reply text alongside the parsed le MODEL, "do something", false, + "medium", ); expect(result.level).toBe("high"); expect(result.reply).toBe("high complexity, more than a medium task"); @@ -96,6 +196,7 @@ test("classifyTurnComplexity surfaces usage from the response when present", asy MODEL, "do something", false, + "medium", ); expect(result.level).toBe("low"); expect(result.usage).toEqual({ input: 42, output: 7, cost: 0.002 }); @@ -113,6 +214,50 @@ test("classifyTurnComplexity notes attached images in the classification prompt" }, } as unknown as ModelRegistry; - await classifyTurnComplexity(registry, MODEL, "describe this screenshot", true); + await classifyTurnComplexity(registry, MODEL, "describe this screenshot", true, "medium"); expect(capturedText).toContain("attached images"); }); + +test("classifyTurnComplexity caps output at CLASSIFY_MAX_TOKENS", async () => { + const { registry, options } = registryCapturingOptions("medium"); + + await classifyTurnComplexity(registry, MODEL, "do something", false, "medium"); + + // Bounded, but not the old fixed 20 that starved reasoning-capable models to an empty reply. + expect(options()?.maxTokens).toBe(CLASSIFY_MAX_TOKENS); +}); + +test("classifyTurnComplexity passes the requested reasoningEffort through for a model on a known-safe API", async () => { + const { registry, options } = registryCapturingOptions("medium"); + + await classifyTurnComplexity(registry, CODEX_MODEL, "do something", false, "max"); + + // This is the whole point of threading an effort through at all: a model configured with + // effort: "max" for its tier should actually reason at max here too, not some unrelated + // provider default. + expect(options()?.reasoningEffort).toBe("max"); +}); + +test("classifyTurnComplexity never sends reasoningEffort \"off\", even for a known-safe API", async () => { + const { registry, options } = registryCapturingOptions("medium"); + + await classifyTurnComplexity(registry, CODEX_MODEL, "do something", false, "off"); + + // "off" isn't a valid reasoningEffort value for any OpenAI-family API - regression coverage for + // the bug that started all this: the field must be entirely absent, not just falsy, since some + // raw request builders treat "any value present" as "send a reasoning object" regardless of + // its content. + expect(options()).not.toHaveProperty("reasoningEffort"); +}); + +test("classifyTurnComplexity does not send reasoningEffort for a model on an unlisted API", async () => { + const { registry, options } = registryCapturingOptions("medium"); + + // "high" is a perfectly valid AutoRouterEffortLevel, but this model's API was never verified to + // accept it as a raw reasoningEffort value - Mistral's own enum, for example, is only + // "none" | "high", so blindly forwarding an untested value risks repeating the exact bug this + // allowlist exists to prevent. + await classifyTurnComplexity(registry, UNLISTED_API_MODEL, "do something", false, "high"); + + expect(options()).not.toHaveProperty("reasoningEffort"); +}); diff --git a/tests/auto-router-extension.test.ts b/tests/auto-router-extension.test.ts index 8e63183..51d3ddc 100644 --- a/tests/auto-router-extension.test.ts +++ b/tests/auto-router-extension.test.ts @@ -378,6 +378,33 @@ test("before_agent_start routes to the classified tier, and the picker shows Aut expect(lastFooterBadge(fake.footerEvents)).toBe("🔀 Auto (auto)"); }); +test("before_agent_start notifies when the classifier gives no usable answer, instead of silently defaulting", async () => { + const medium = model("prov", "medium-model"); + await writeConfig({ + efforts: { medium: { models: [{ provider: "prov", id: "medium-model" }] } }, + }); + + const fake = createFakePi(); + await autoRouter(fake.pi); + // An empty classifier reply parses to no level word at all - exactly what happened for real + // when a codex-family model was sent an invalid reasoningEffort value and came back with no + // text content. + const registry = fakeModelRegistry({ models: [medium], classify: () => "" }); + const ctx = fakeCtx({ modelRegistry: registry, currentModel: fake.currentModel }); + + await fake.fire("session_start", {}, ctx); + await selectAuto(fake, ctx); + await fake.fire("before_agent_start", { prompt: "anything" }, ctx); + + // Still routes (defaulting to medium) rather than blocking the turn... + expect(fake.setModelCalls).toEqual([medium]); + // ...but the failure is visible, not silent. + const warning = ctx.notifications.find( + (n) => n.type === "warning" && n.message.includes("classifier"), + ); + expect(warning?.message).toContain("(empty reply)"); +}); + test("a model's `effort` override sets its own thinking level, independent of the tier that routed to it", async () => { const high = model("opencode-go", "kimi-k3"); await writeConfig({