diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 001c32bcd..62fd3340c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed session-title generation silently falling back to the online `smol` model (and billing whatever provider held the resolved API key — OpenRouter in the reporter's case) when the user had explicitly configured a **local** `providers.tinyModel`: `generateSessionTitle` raced local against online with a 10s timeout and fired the online request immediately whenever the local worker returned `null` (unknown key, model not downloaded, transformers.js failure). Now an explicit local-model choice is honored end-to-end — on local failure the session is left untitled with a `logger.warn` instead of billing the smol fallback ([#3187](https://github.com/can1357/oh-my-pi/issues/3187)) + ## [16.1.10] - 2026-06-21 ### Added diff --git a/packages/coding-agent/src/utils/title-generator.ts b/packages/coding-agent/src/utils/title-generator.ts index 59866f87f..88a7370d6 100644 --- a/packages/coding-agent/src/utils/title-generator.ts +++ b/packages/coding-agent/src/utils/title-generator.ts @@ -12,7 +12,7 @@ import type { Settings } from "../config/settings"; import titleMarkerInstruction from "../prompts/system/title-marker-instruction.md" with { type: "text" }; import titleSystemPrompt from "../prompts/system/title-system.md" with { type: "text" }; import titleMarkerSystemPrompt from "../prompts/system/title-system-marker.md" with { type: "text" }; -import { ONLINE_TINY_TITLE_MODEL_KEY } from "../tiny/models"; +import { isTinyTitleLocalModelKey, ONLINE_TINY_TITLE_MODEL_KEY } from "../tiny/models"; import { formatTitleUserMessage, isLowSignalTitleInput, normalizeGeneratedTitle } from "../tiny/text"; import { tinyTitleClient } from "../tiny/title-client"; @@ -23,7 +23,6 @@ const TITLE_MARKER_INSTRUCTION = prompt.render(titleMarkerInstruction); const DEFAULT_TERMINAL_TITLE = "π"; const TERMINAL_TITLE_CONTROL_CHARS = /[\u0000-\u001f\u007f-\u009f]/g; -export const TITLE_LOCAL_FALLBACK_DELAY_MS = 10_000; const TITLE_MAX_TOKENS = 30; const REASONING_SAFE_MAX_TOKENS = 1024; const SET_TITLE_TOOL_NAME = "set_title"; @@ -81,78 +80,6 @@ function getTitleModel(registry: ModelRegistry, settings: Settings, currentModel return undefined; } -export async function raceFirstNonNull( - primary: Promise, - startFallback: () => Promise, - delayMs: number = TITLE_LOCAL_FALLBACK_DELAY_MS, - onPrimaryWinAfterFallback?: () => void, -): Promise { - const { promise, resolve } = Promise.withResolvers(); - let resolved = false; - let primarySettled = false; - let fallbackStarted = false; - let fallbackSettled = false; - - const resolveOnce = (value: T | null): void => { - if (resolved) return; - resolved = true; - resolve(value); - }; - const maybeResolveNull = (): void => { - if (primarySettled && fallbackStarted && fallbackSettled) resolveOnce(null); - }; - const startFallbackOnce = (): void => { - if (fallbackStarted || resolved) return; - fallbackStarted = true; - let fallback: Promise; - try { - fallback = startFallback(); - } catch { - fallbackSettled = true; - maybeResolveNull(); - return; - } - void fallback.then( - value => { - fallbackSettled = true; - if (value !== null) resolveOnce(value); - else maybeResolveNull(); - }, - () => { - fallbackSettled = true; - maybeResolveNull(); - }, - ); - }; - - const timer = setTimeout(startFallbackOnce, delayMs); - void primary.then( - value => { - primarySettled = true; - clearTimeout(timer); - if (value !== null) { - if (fallbackStarted) onPrimaryWinAfterFallback?.(); - resolveOnce(value); - return; - } - startFallbackOnce(); - maybeResolveNull(); - }, - () => { - primarySettled = true; - clearTimeout(timer); - startFallbackOnce(); - maybeResolveNull(); - }, - ); - - try { - return await promise; - } finally { - clearTimeout(timer); - } -} - /** * Generate a title for a session based on the first user message. * @@ -200,36 +127,41 @@ export async function generateSessionTitle( ); } - const onlineAbortController = new AbortController(); - const localTitlePromise = titleSystemPrompt - ? tinyTitleClient.generate(tinyModel, firstMessage, { systemPrompt: titleSystemPrompt }) - : tinyTitleClient.generate(tinyModel, firstMessage); - const localTitle = localTitlePromise.then( - title => title || null, - err => { - logger.warn("title-generator: local model error", { + // User explicitly picked a local tiny model. NEVER fall back to the online + // smol path (issue #3187): the smol role resolves through priority.json and + // silently bills whatever provider holds the resolved API key — OpenRouter + // in the reporter's case, leaking real credits without consent. If the + // local worker fails (unknown key, download missing, transformers.js + // crash, abort), leave the session untitled; the next user turn retries. + if (!isTinyTitleLocalModelKey(tinyModel)) { + logger.warn("title-generator: unknown local tiny model; skipping title (will not fall back to online)", { + sessionId, + model: tinyModel, + reason: "unknown-local-model", + }); + return null; + } + try { + const localTitle = titleSystemPrompt + ? await tinyTitleClient.generate(tinyModel, firstMessage, { systemPrompt: titleSystemPrompt }) + : await tinyTitleClient.generate(tinyModel, firstMessage); + if (!localTitle) { + logger.warn("title-generator: local tiny model produced no title; skipping (no online fallback)", { sessionId, model: tinyModel, - error: err instanceof Error ? err.message : String(err), + reason: "local-no-output", }); return null; - }, - ); - const startOnline = (): Promise => - generateTitleOnline( - firstMessage, - registry, - settings, + } + return localTitle; + } catch (err) { + logger.warn("title-generator: local tiny model errored; skipping (no online fallback)", { sessionId, - currentModel, - metadataResolver, - onlineAbortController.signal, - titleSystemPrompt, - ); - - return raceFirstNonNull(localTitle, startOnline, TITLE_LOCAL_FALLBACK_DELAY_MS, () => { - onlineAbortController.abort(); - }); + model: tinyModel, + error: err instanceof Error ? err.message : String(err), + }); + return null; + } } export async function generateTitleOnline( diff --git a/packages/coding-agent/test/tiny-title-generator.test.ts b/packages/coding-agent/test/tiny-title-generator.test.ts index 1994f30e9..20309f9a4 100644 --- a/packages/coding-agent/test/tiny-title-generator.test.ts +++ b/packages/coding-agent/test/tiny-title-generator.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeAll, describe, expect, it, vi } from "bun:test"; -import type { Api, AssistantMessage, Model } from "@oh-my-pi/pi-ai"; +import type { Api, Model } from "@oh-my-pi/pi-ai"; import * as ai from "@oh-my-pi/pi-ai"; import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; import { isSubcommand } from "@oh-my-pi/pi-coding-agent/cli-commands"; @@ -22,17 +22,9 @@ import { TINY_TITLE_MODEL_VALUES, } from "@oh-my-pi/pi-coding-agent/tiny/models"; import { createTinyTitleSubprocess, tinyTitleClient } from "@oh-my-pi/pi-coding-agent/tiny/title-client"; -import { - generateSessionTitle, - raceFirstNonNull, - TITLE_LOCAL_FALLBACK_DELAY_MS, -} from "@oh-my-pi/pi-coding-agent/utils/title-generator"; +import { generateSessionTitle } from "@oh-my-pi/pi-coding-agent/utils/title-generator"; import type { Subprocess } from "bun"; -async function flushMicrotasks(turns = 4): Promise { - for (let i = 0; i < turns; i += 1) await Promise.resolve(); -} - function getModelOrThrow(id: string): Model { const model = getBundledModel("anthropic", id); if (!model) throw new Error(`Expected model ${id}`); @@ -114,102 +106,6 @@ afterEach(() => { vi.restoreAllMocks(); }); -describe("raceFirstNonNull", () => { - it("resolves with local result without starting fallback", async () => { - let fallbackStarted = false; - const title = await raceFirstNonNull( - Promise.resolve("Local Title"), - () => { - fallbackStarted = true; - return Promise.resolve("Online Title"); - }, - TITLE_LOCAL_FALLBACK_DELAY_MS, - ); - - expect(title).toBe("Local Title"); - expect(fallbackStarted).toBe(false); - }); - - it("starts fallback after the hardcoded delay", async () => { - vi.useFakeTimers(); - const local = Promise.withResolvers(); - let fallbackStarted = false; - const result = raceFirstNonNull( - local.promise, - () => { - fallbackStarted = true; - return Promise.resolve("Online Title"); - }, - TITLE_LOCAL_FALLBACK_DELAY_MS, - ); - - await flushMicrotasks(); - expect(fallbackStarted).toBe(false); - vi.advanceTimersByTime(TITLE_LOCAL_FALLBACK_DELAY_MS - 1); - await flushMicrotasks(); - expect(fallbackStarted).toBe(false); - vi.advanceTimersByTime(1); - await flushMicrotasks(); - - expect(fallbackStarted).toBe(true); - await expect(result).resolves.toBe("Online Title"); - local.resolve(null); - }); - - it("starts fallback immediately when local fails", async () => { - let fallbackStarted = false; - const title = await raceFirstNonNull( - Promise.reject(new Error("local failed")), - () => { - fallbackStarted = true; - return Promise.resolve("Online Title"); - }, - TITLE_LOCAL_FALLBACK_DELAY_MS, - ); - - expect(title).toBe("Online Title"); - expect(fallbackStarted).toBe(true); - }); - - it("returns null only after local and fallback return null", async () => { - const title = await raceFirstNonNull( - Promise.resolve(null), - () => Promise.resolve(null), - TITLE_LOCAL_FALLBACK_DELAY_MS, - ); - - expect(title).toBeNull(); - }); - - it("runs the loser-cancel callback when local wins after fallback starts", async () => { - vi.useFakeTimers(); - const local = Promise.withResolvers(); - const fallback = Promise.withResolvers(); - let fallbackStarted = false; - let cancelCount = 0; - const result = raceFirstNonNull( - local.promise, - () => { - fallbackStarted = true; - return fallback.promise; - }, - TITLE_LOCAL_FALLBACK_DELAY_MS, - () => { - cancelCount += 1; - }, - ); - - vi.advanceTimersByTime(TITLE_LOCAL_FALLBACK_DELAY_MS); - await flushMicrotasks(); - expect(fallbackStarted).toBe(true); - - local.resolve("Local Title"); - await expect(result).resolves.toBe("Local Title"); - expect(cancelCount).toBe(1); - fallback.resolve(null); - }); -}); - describe("tiny title generator routing", () => { it("keeps online-only behavior when Tiny Model is Online", async () => { const model = getModelOrThrow("claude-sonnet-4-5"); @@ -264,10 +160,10 @@ describe("tiny title generator routing", () => { expect(online).not.toHaveBeenCalled(); }); - it("starts online fallback immediately when local returns null", async () => { + it("does NOT fall back to online when local returns null (issue #3187)", async () => { const model = getModelOrThrow("claude-sonnet-4-5"); - vi.spyOn(tinyTitleClient, "generate").mockResolvedValue(null); - const online = mockOnlineTitle("Online Title"); + const local = vi.spyOn(tinyTitleClient, "generate").mockResolvedValue(null); + const online = mockOnlineTitle("Billed Online Title"); const title = await generateSessionTitle( "Investigate fallback", @@ -275,63 +171,40 @@ describe("tiny title generator routing", () => { createSettings(model, "lfm2-350m"), ); - expect(title).toBe("Online Title"); - expect(online).toHaveBeenCalledTimes(1); + expect(title).toBeNull(); + expect(local).toHaveBeenCalledTimes(1); + expect(online).not.toHaveBeenCalled(); }); - it("aborts the online request when delayed local generation wins", async () => { - vi.useFakeTimers(); + it("does NOT fall back to online when local throws", async () => { const model = getModelOrThrow("claude-sonnet-4-5"); - const local = Promise.withResolvers(); - const onlineHold = Promise.withResolvers(); - let onlineSignal: AbortSignal | undefined; - vi.spyOn(tinyTitleClient, "generate").mockReturnValue(local.promise); - vi.spyOn(ai, "completeSimple").mockImplementation((_model, _context, options) => { - onlineSignal = options?.signal; - return onlineHold.promise; - }); + vi.spyOn(tinyTitleClient, "generate").mockRejectedValue(new Error("worker crashed")); + const online = mockOnlineTitle("Billed Online Title"); - const result = generateSessionTitle( - "Investigate cancellation", - createRegistry(model), - createSettings(model, "lfm2-350m"), - ); - - vi.advanceTimersByTime(TITLE_LOCAL_FALLBACK_DELAY_MS); - await flushMicrotasks(); - expect(onlineSignal?.aborted).toBe(false); - - local.resolve("Local Title"); - await expect(result).resolves.toBe("Local Title"); - expect(onlineSignal?.aborted).toBe(true); - onlineHold.resolve({ stopReason: "abort", content: [] } as never); - }); - - it("keeps local generation alive when the delayed online fallback wins", async () => { - vi.useFakeTimers(); - const model = getModelOrThrow("claude-sonnet-4-5"); - const local = Promise.withResolvers(); - let localSettled = false; - void local.promise.then(() => { - localSettled = true; - }); - vi.spyOn(tinyTitleClient, "generate").mockReturnValue(local.promise); - mockOnlineTitle("Online Title"); - - const result = generateSessionTitle( - "Investigate background download", + const title = await generateSessionTitle( + "Investigate crash", createRegistry(model), createSettings(model, "lfm2-700m"), ); - vi.advanceTimersByTime(TITLE_LOCAL_FALLBACK_DELAY_MS); - await flushMicrotasks(); - await expect(result).resolves.toBe("Online Title"); - expect(localSettled).toBe(false); + expect(title).toBeNull(); + expect(online).not.toHaveBeenCalled(); + }); - local.resolve("Late Local Title"); - await flushMicrotasks(); - expect(localSettled).toBe(true); + it("does NOT call the local worker or online path for an unknown tinyModel key", async () => { + const model = getModelOrThrow("claude-sonnet-4-5"); + const local = vi.spyOn(tinyTitleClient, "generate").mockResolvedValue("Late Local"); + const online = mockOnlineTitle("Billed Online Title"); + + const title = await generateSessionTitle( + "Investigate unknown", + createRegistry(model), + createSettings(model, "ollama:gpt-oss"), + ); + + expect(title).toBeNull(); + expect(local).not.toHaveBeenCalled(); + expect(online).not.toHaveBeenCalled(); }); });