diff --git a/extensions/auto-router-classify.ts b/extensions/auto-router-classify.ts index 08452c4..306183b 100644 --- a/extensions/auto-router-classify.ts +++ b/extensions/auto-router-classify.ts @@ -108,6 +108,12 @@ export async function classifyTurnComplexity( const text = hasImages ? `${prompt}\n\n(This turn also includes attached images.)` : prompt; + // `reasoningEffort` is expected to already be genuinely supported by `model` - the caller + // (auto-router.ts) resolves it through pi-ai's own `getSupportedThinkingLevels`/ + // `clampThinkingLevel` first and warns the user directly if their config asked for something + // this model doesn't actually support, rather than this function quietly substituting + // something else with no visibility into that mismatch. + // // "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. diff --git a/extensions/auto-router-health.ts b/extensions/auto-router-health.ts index 73e4748..bb9f565 100644 --- a/extensions/auto-router-health.ts +++ b/extensions/auto-router-health.ts @@ -301,11 +301,23 @@ export class AutoRouterHealthStore { private state: AutoRouterHealthState = {}; private classifications: ClassificationLogEntry[] = []; private writeTimer: ReturnType | undefined; + /** + * Resolved once, at construction, rather than fresh on every `flush()` - a real Pi process's + * agent dir never changes mid-life, and re-resolving `PI_CODING_AGENT_DIR` on each debounced + * flush instead means *any* code that touches that env var while a save is still pending (not + * just this instance's own caller) silently redirects an in-flight write to wherever the env + * var happens to point at that later moment - verified: this is exactly how a test suite's + * fixture data ended up in the real global `auto-router-state.json` on disk, repeatedly, even + * after each individual test file was fixed to wait out its own debounce window before its own + * teardown - a *different* file's `beforeEach` changing the same process-wide env var while an + * earlier file's save was still in flight was enough on its own. + */ + private readonly path: string = statePath(); async load(): Promise { try { const parsed = parsePersisted( - JSON.parse(await readFile(statePath(), "utf8")), + JSON.parse(await readFile(this.path, "utf8")), ); this.state = parsed.models; this.classifications = parsed.classifications; @@ -392,7 +404,7 @@ export class AutoRouterHealthStore { } async flush(): Promise { - const dir = dirname(statePath()); + const dir = dirname(this.path); await mkdir(dir, { recursive: true }); const tempPath = join( dir, @@ -404,7 +416,7 @@ export class AutoRouterHealthStore { `${JSON.stringify({ models: this.state, classifications: this.classifications }, null, 2)}\n`, "utf8", ); - await rename(tempPath, statePath()); + await rename(tempPath, this.path); } finally { await rm(tempPath, { force: true }).catch(() => undefined); } diff --git a/extensions/auto-router.ts b/extensions/auto-router.ts index b394aae..47ddbfb 100644 --- a/extensions/auto-router.ts +++ b/extensions/auto-router.ts @@ -1,4 +1,9 @@ -import type { Api, Model } from "@earendil-works/pi-ai"; +import { + type Api, + clampThinkingLevel, + getSupportedThinkingLevels, + type Model, +} from "@earendil-works/pi-ai"; import { DynamicBorder, type ExtensionAPI, @@ -229,6 +234,74 @@ export default async function autoRouter(pi: ExtensionAPI): Promise { return ref?.effort ?? tier; } + /** + * `requested` if `model` genuinely supports it, or the closest level it actually does (via + * pi-ai's own `clampThinkingLevel` - the same resolution real turn dispatch already relies on, + * not a guess of our own) - warning the user directly whenever a substitution was needed, since + * a configured `effort` a model silently can't honor is exactly what turned a "medium (max + * effort)" routing decision into an unexplained empty classifier reply, undetected, for two full + * PRs. Applies to both the classifier's own reasoning effort and real per-turn dispatch, so + * fixing (or leaving) the mismatch is the user's informed choice either way, not something Auto + * quietly papers over in only one of the two places it happens. + */ + function resolveSupportedEffort( + ctx: ExtensionContext, + model: Model, + requested: AutoRouterEffortLevel, + ): AutoRouterEffortLevel { + const supported = getSupportedThinkingLevels(model); + if (supported.includes(requested)) return requested; + const clamped = clampThinkingLevel(model, requested) as AutoRouterEffortLevel; + if (ctx.hasUI) { + ctx.ui.notify( + `Auto: ${model.provider}/${model.id} doesn't support "${requested}" effort (configured for it in ~/.pi/agent/settings.json) - it supports ${supported.join(", ")}. Using "${clamped}" instead.`, + "warning", + ); + } + return clamped; + } + + /** + * Whole-config check, once per session start: does every model with a configured `effort` + * override actually support that effort? `resolveSupportedEffort` above only warns about a + * mismatch once a turn happens to route to that specific model - a model configured only in a + * rarely-hit tier (or not yet routed to this session at all) could otherwise sit silently + * misconfigured indefinitely. This surfaces every mismatch in the config up front, in one + * notification, independent of whether anything has actually been routed yet. + */ + function warnAboutUnsupportedConfiguredEfforts( + ctx: ExtensionContext, + settings: AutoRouterSettings, + ): void { + if (!ctx.hasUI) return; + const mismatches: string[] = []; + const seen = new Set(); + for (const tier of AUTO_ROUTER_EFFORT_ORDER) { + for (const ref of settings.efforts[tier]?.models ?? []) { + if (!ref.effort) continue; + const key = `${ref.provider}/${ref.id}:${ref.effort}`; + if (seen.has(key)) continue; + seen.add(key); + // Unresolvable here (no auth configured, wrong id, provider not registered, ...) is a + // separate, pre-existing failure mode already handled elsewhere (pickForTier's own + // fallback/notify path) - not this check's job to also report. + const model = ctx.modelRegistry.find(ref.provider, ref.id); + if (!model) continue; + const supported = getSupportedThinkingLevels(model); + if (supported.includes(ref.effort)) continue; + const clamped = clampThinkingLevel(model, ref.effort); + mismatches.push( + `${ref.provider}/${ref.id}: configured for "${ref.effort}" but only supports ${supported.join(", ")} (will run at "${clamped}")`, + ); + } + } + if (mismatches.length === 0) return; + ctx.ui.notify( + `Auto: ${mismatches.length} configured model effort${mismatches.length === 1 ? "" : "s"} ${mismatches.length === 1 ? "isn't" : "aren't"} actually supported:\n${mismatches.map((line) => ` - ${line}`).join("\n")}`, + "warning", + ); + } + /** Pick the best available (resolved + healthy) model for `tier`, escalating to higher configured tiers when everything in `tier` is unhealthy, then falling back to the first available model anywhere as a last resort. */ function pickForTier( ctx: ExtensionContext, @@ -329,7 +402,7 @@ export default async function autoRouter(pi: ExtensionAPI): Promise { } return; } - await pi.setThinkingLevel(effort); + await pi.setThinkingLevel(resolveSupportedEffort(ctx, model, effort)); } finally { routingInFlight = false; } @@ -369,7 +442,13 @@ export default async function autoRouter(pi: ExtensionAPI): Promise { // 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"); + // resolveSupportedEffort further clamps (and warns) if this model doesn't actually + // support that configured level at all. + const classifierEffort = resolveSupportedEffort( + ctx, + classifierModel, + resolveEffort(classifierRefs, classifierModel, "medium"), + ); const result = await classifyTurnComplexity( ctx.modelRegistry, classifierModel, @@ -491,6 +570,7 @@ export default async function autoRouter(pi: ExtensionAPI): Promise { publishFooter(); } const settings = await readAutoRouterSettings(); + warnAboutUnsupportedConfiguredEfforts(ctx, settings); void reconcileAllProviders(ctx.modelRegistry, settings); void ensureAutoModelScopedInGlobalSettings().catch(() => undefined); }); diff --git a/tests/auto-router-classify.test.ts b/tests/auto-router-classify.test.ts index 79b0175..1c163c9 100644 --- a/tests/auto-router-classify.test.ts +++ b/tests/auto-router-classify.test.ts @@ -230,11 +230,23 @@ test("classifyTurnComplexity caps output at CLASSIFY_MAX_TOKENS", async () => { 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"); + await classifyTurnComplexity(registry, CODEX_MODEL, "do something", false, "high"); // 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 + // effort: "high" for its tier should actually reason at high here too, not some unrelated // provider default. + expect(options()?.reasoningEffort).toBe("high"); +}); + +test("classifyTurnComplexity trusts whatever reasoningEffort it's given verbatim, including \"max\"", async () => { + // Whether a model genuinely supports the requested level is the caller's job to resolve (via + // getSupportedThinkingLevels/clampThinkingLevel in auto-router.ts, which warns the user + // directly if their configured effort doesn't match the model's real capabilities) - not + // something this function should second-guess or silently substitute on its own. + const { registry, options } = registryCapturingOptions("medium"); + + await classifyTurnComplexity(registry, CODEX_MODEL, "do something", false, "max"); + expect(options()?.reasoningEffort).toBe("max"); }); diff --git a/tests/auto-router-extension.test.ts b/tests/auto-router-extension.test.ts index 51d3ddc..2087466 100644 --- a/tests/auto-router-extension.test.ts +++ b/tests/auto-router-extension.test.ts @@ -10,7 +10,6 @@ import type { Theme, } from "@earendil-works/pi-coding-agent"; import autoRouter, { escapeTableCell } from "../extensions/auto-router.ts"; -import { SAVE_DEBOUNCE_MS } from "../extensions/auto-router-health.ts"; import type { AutoRouterSettings } from "../extensions/auto-router-settings.ts"; test("escapeTableCell neutralizes both pipes and line breaks, so one bad reply can't break the rest of the table", () => { @@ -24,13 +23,13 @@ const ENV_VAR = "PI_CODING_AGENT_DIR"; let agentDir: string | undefined; const usedDirs: string[] = []; -// The extension's internal AutoRouterHealthStore debounces its writes (~2s after the last -// record call), so a save scheduled by one test can fire well after that test's own teardown - -// if `afterEach` restored PI_CODING_AGENT_DIR to its prior (usually unset) value in the -// meantime, that late write would land in the real global agent directory instead of a test's -// temp one. So the env var is never restored to anything other than a temp dir for the whole -// run - only ever moved to a new one - and every temp dir used stays on disk until all tests -// finish, so even a very late write can only ever land somewhere harmless. +// Each AutoRouterHealthStore instance pins its target path at construction rather than +// re-resolving PI_CODING_AGENT_DIR on every debounced flush, so a save scheduled by one test +// stays pointed at that test's own temp dir no matter what this (process-wide) env var is set to +// by the time the write actually fires - including by an unrelated later test or file. No +// teardown coordination needed as a result; the temp dirs themselves are still kept around until +// the whole run finishes and cleaned up together, purely so a slightly-delayed write always has +// somewhere valid to land. beforeEach(async () => { agentDir = await mkdtemp(join(tmpdir(), "pi-kit-auto-router-agent-")); usedDirs.push(agentDir); @@ -38,16 +37,6 @@ beforeEach(async () => { }); afterAll(async () => { - // The debounced save timer is unref'd, so it never blocks the process from exiting - but if - // this suite's own run happens to keep the process alive past SAVE_DEBOUNCE_MS anyway (e.g. - // a larger `bun test` invocation still running other files), a timer scheduled by one of this - // file's last tests can still fire *after* this hook would otherwise have already deleted - // PI_CODING_AGENT_DIR and removed its temp dir - at which point `statePath()` falls back to - // the real default `~/.pi/agent`, and the save actually corrupts the developer's real global - // auto-router-state.json with this suite's fixture data (verified: it happened). Waiting out - // the debounce window here first, before touching the env var or any directory, guarantees - // every such timer fires while it's still pointed at a real (about-to-be-removed) temp dir. - await new Promise((resolve) => setTimeout(resolve, SAVE_DEBOUNCE_MS + 500)); delete process.env[ENV_VAR]; await Promise.all(usedDirs.map((dir) => rm(dir, { recursive: true, force: true }))); }); @@ -60,8 +49,17 @@ async function writeConfig(settings: AutoRouterSettings): Promise { ); } +// reasoning: true plus explicit xhigh/max support so fixture models are "fully capable" by +// default - these tests are about routing/escalation/pinning logic, not about effort-support +// clamping specifically (that has its own dedicated tests below), and getSupportedThinkingLevels +// otherwise excludes xhigh/max for any model without an explicit thinkingLevelMap entry for them. function model(provider: string, id: string): Model { - return { provider, id } as unknown as Model; + return { + provider, + id, + reasoning: true, + thinkingLevelMap: { xhigh: "xhigh", max: "max" }, + } as unknown as Model; } const AUTO_PLACEHOLDER = model("auto", "auto"); @@ -439,6 +437,101 @@ test("a model's `effort` override sets its own thinking level, independent of th expect(notified).toContain("at max effort"); // classification log shows the real applied effort }); +test("routing to a model whose effort override it doesn't actually support clamps to what it does, and warns instead of silently substituting", async () => { + // Mirrors the real gpt-5.3-codex-spark case: reasoning-capable, and its own thinkingLevelMap + // confirms "xhigh" support but has no entry for "max" at all - so per pi-ai's own + // getSupportedThinkingLevels, this model does not actually support "max" despite it type-checking + // as a valid AutoRouterEffortLevel. + const spark = { + provider: "openai-codex", + id: "gpt-5.3-codex-spark", + reasoning: true, + thinkingLevelMap: { xhigh: "xhigh", minimal: "low" }, + } as unknown as Model; + await writeConfig({ + efforts: { + medium: { + models: [{ provider: "openai-codex", id: "gpt-5.3-codex-spark", effort: "max" }], + }, + }, + }); + + const fake = createFakePi(); + await autoRouter(fake.pi); + const registry = fakeModelRegistry({ models: [spark], classify: () => "medium" }); + 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 and dispatches - never blocks the turn over this... + expect(fake.setModelCalls).toEqual([spark]); + // ...but clamps to what the model actually supports rather than sending "max" and getting an + // empty/broken response back (verified: this is exactly what was happening for real). + expect(fake.thinkingLevelCalls).toEqual(["xhigh"]); + // ...and the mismatch is surfaced twice, not silently papered over: once as a whole-config + // summary at session start (independent of whether anything routes there yet)... + const startupWarning = ctx.notifications.find( + (n) => n.type === "warning" && n.message.includes("configured model effort"), + ); + expect(startupWarning?.message).toContain("gpt-5.3-codex-spark"); + expect(startupWarning?.message).toContain('configured for "max"'); + // ...and again, specifically, at the point this particular turn actually dispatched there. + const dispatchWarning = ctx.notifications.find( + (n) => n.type === "warning" && n.message.includes('doesn\'t support "max"'), + ); + expect(dispatchWarning?.message).toContain("gpt-5.3-codex-spark"); + expect(dispatchWarning?.message).toContain("xhigh"); +}); + +test("session_start warns once per model+effort pair, even when it's configured in multiple tiers", async () => { + const spark = { + provider: "openai-codex", + id: "gpt-5.3-codex-spark", + reasoning: true, + thinkingLevelMap: { xhigh: "xhigh", minimal: "low" }, + } as unknown as Model; + // Same model, same "max" override, configured in both low and medium - exactly the real + // gpt-5.3-codex-spark case. + await writeConfig({ + efforts: { + low: { models: [{ provider: "openai-codex", id: "gpt-5.3-codex-spark", effort: "max" }] }, + medium: { models: [{ provider: "openai-codex", id: "gpt-5.3-codex-spark", effort: "max" }] }, + }, + }); + + const fake = createFakePi(); + await autoRouter(fake.pi); + const ctx = fakeCtx({ modelRegistry: fakeModelRegistry({ models: [spark] }), currentModel: fake.currentModel }); + + await fake.fire("session_start", {}, ctx); + + const startupWarnings = ctx.notifications.filter((n) => n.message.includes("configured model effort")); + expect(startupWarnings).toHaveLength(1); + expect(startupWarnings[0]?.message).toContain("1 configured model effort isn't"); +}); + +test("session_start does not warn when every configured effort override is genuinely supported", async () => { + const luna = { + provider: "openai-codex", + id: "gpt-5.6-luna", + reasoning: true, + thinkingLevelMap: { xhigh: "xhigh", max: "max", minimal: "low" }, + } as unknown as Model; + await writeConfig({ + efforts: { medium: { models: [{ provider: "openai-codex", id: "gpt-5.6-luna", effort: "max" }] } }, + }); + + const fake = createFakePi(); + await autoRouter(fake.pi); + const ctx = fakeCtx({ modelRegistry: fakeModelRegistry({ models: [luna] }), currentModel: fake.currentModel }); + + await fake.fire("session_start", {}, ctx); + + expect(ctx.notifications.some((n) => n.message.includes("configured model effort"))).toBe(false); +}); + test("a routed turn's classification is logged and shows up in /usage, so a routing decision can be checked against what the classifier actually said", async () => { const medium = model("prov", "medium-model"); const high = model("prov", "high-model"); diff --git a/tests/auto-router-health.test.ts b/tests/auto-router-health.test.ts index d019694..cec8f84 100644 --- a/tests/auto-router-health.test.ts +++ b/tests/auto-router-health.test.ts @@ -20,13 +20,13 @@ const ENV_VAR = "PI_CODING_AGENT_DIR"; let agentDir: string | undefined; const usedDirs: string[] = []; -// AutoRouterHealthStore debounces its writes (~2s after the last record call), so a save -// scheduled by one test can fire well after that test's own teardown - if `afterEach` restored -// PI_CODING_AGENT_DIR to its prior (usually unset) value in the meantime, that late write would -// land in the real global agent directory instead of a test's temp one. So the env var is never -// restored to anything other than a temp dir for the whole run - only ever moved to a new one - -// and every temp dir used stays on disk until all tests finish, so even a very late write can -// only ever land somewhere harmless. +// Each `AutoRouterHealthStore` instance pins its target path at construction rather than +// re-resolving PI_CODING_AGENT_DIR on every debounced flush, so a save scheduled by one test stays +// pointed at that test's own temp dir no matter what this (process-wide) env var is set to by the +// time the write actually fires - including by an unrelated later test or file. No teardown +// coordination needed as a result; the temp dirs themselves are still kept around until the whole +// run finishes and cleaned up together, purely so a slightly-delayed write always has somewhere +// valid to land. beforeEach(async () => { agentDir = await mkdtemp(join(tmpdir(), "pi-kit-auto-router-health-")); usedDirs.push(agentDir);