diff --git a/src/agent/agent-loop-local-gate.test.ts b/src/agent/agent-loop-local-gate.test.ts new file mode 100644 index 00000000..8c8674de --- /dev/null +++ b/src/agent/agent-loop-local-gate.test.ts @@ -0,0 +1,458 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { AgentLoop } from "./agent-loop.js"; +import { buildDefaultToolRegistry } from "../tools/index.js"; +import { SlotManager } from "../llm/slot-manager.js"; +import { createEmptySessionState } from "../session/session-state.js"; +import type { + CompletionResult, + LlamaServerClient, +} from "../llm/llama-server-client.js"; +import { ModelProfileManager } from "../llm/model-profile-manager.js"; +import { GEMMA4_PROPS, QWEN3_PROPS } from "../llm/model-profile.fixtures.js"; +import { QWEN_THINK_PROFILE } from "../llm/model-profile.js"; +import { buildGrammar } from "../llm/grammar/build-grammar.js"; +import { + createLocalLinkPreparer, + DeferredLocalBackendProbes, +} from "../llm/local-backend-gate.js"; +import { ProviderFallbackChain } from "../llm/fallback/index.js"; +import { DEFAULT_FALLBACK_TIMING } from "../llm/fallback/fallback-config.js"; +import { providerIdIsLlamaServer } from "../llm/provider/registry/active-text-provider.js"; +import type { ResolvedLlmConfig } from "../llm/provider/registry/provider-registry.js"; +import type { LlmProvider } from "../llm/provider/llm-provider.js"; +import type { StreamChunk } from "../llm/provider/completion-types.js"; +import { OpenAiHttpError } from "../llm/provider/openai/openai-http.js"; +import { openAiToolCallAdapter } from "../llm/provider/openai/openai-tool-call-adapter.js"; +import { createFallbackCompleter } from "../runtime/llm-fallback-seam.js"; +import type { + CapabilitiesSummary, + SkillCatalogEntry, + ToolDescriptor, +} from "../prompt/stable-prefix.js"; + +/** + * Issue #112 — the loop's two `ModelProfileManager` probes are + * llama-server traffic, and a cloud turn must produce none of it. + * + * `fetchProps` is the counted local request: it IS the `/props` call, + * one layer below the HTTP client. Counts are exact — the pre-fix + * behaviour was one probe per turn plus one per stale step, so a + * `not.toHaveBeenCalled()` would not catch a regression that merely + * moved the probe. + */ + +function makeCompletion( + content: string, + modelId: string = "mock", +): CompletionResult { + return { + content, + reasoningContent: "", + stop: true, + truncated: false, + timing: { promptMs: 1, predictedMs: 1, promptTokens: 10, predictedTokens: 5 }, + cacheHitTokens: 0, + slotId: 0, + modelId, + }; +} + +const TOOLS: ToolDescriptor[] = [ + { + name: "finish", + summary: "Finish the session with a summary.", + argsSchema: '{"summary": string}', + }, +]; + +const CAPS: CapabilitiesSummary = { + platform: "darwin", + arch: "arm64", + browserChannel: "chrome", + workingDir: "/work", + hasClipboard: true, + hasWmctrl: false, + hasNotifications: true, +}; + +const SKILLS: SkillCatalogEntry[] = []; + +describe("AgentLoop — local profile probes are gated on the active route", () => { + let workingDir: string; + + beforeEach(() => { + workingDir = mkdtempSync(join(tmpdir(), "atomic-loop-gate-")); + }); + + afterEach(() => { + rmSync(workingDir, { recursive: true, force: true }); + }); + + const buildLoop = async ( + localBackend: ConstructorParameters[0]["localBackend"], + ) => { + const grammar = await buildGrammar(QWEN_THINK_PROFILE); + const fetchProps = vi.fn<[], Promise>>(); + fetchProps.mockResolvedValue(GEMMA4_PROPS); + const profileManager = new ModelProfileManager({ + llama: { fetchProps } as unknown as LlamaServerClient, + initialProfile: QWEN_THINK_PROFILE, + initialGrammar: grammar, + initialModelId: "qwen3-30b-a3b-instruct-2507", + }); + const loop = new AgentLoop({ + registry: buildDefaultToolRegistry(), + slotManager: new SlotManager(2), + grammar, + profile: QWEN_THINK_PROFILE, + profileManager, + ...(localBackend ? { localBackend } : {}), + // Pre-closed reasoning channel so the reply parses under either + // profile — what is under test is the probe count, not parsing. + // The completion echoes the model the manager already believes is + // loaded, so nothing here marks it stale: staleness has its own + // reactive-refresh tests, and letting it leak in would add probes + // that the gate is not responsible for. + llmComplete: async () => + makeCompletion( + `${JSON.stringify({ + tool: "finish", + args: { summary: "done" }, + })}`, + "qwen3-30b-a3b-instruct-2507", + ), + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + }); + return { loop, fetchProps, profileManager }; + }; + + const runTurn = async (loop: AgentLoop, id: string) => + loop.runTurn(createEmptySessionState({ id, workingDir }), { + userMessage: "go", + maxSteps: 2, + signal: new AbortController().signal, + }); + + it("makes zero /props requests on a cloud turn", async () => { + const { loop, fetchProps, profileManager } = await buildLoop({ + isActive: () => false, + ensureProbed: async () => false, + }); + + const result = await runTurn(loop, "s-cloud"); + + expect(result.reason).toBe("finish"); + expect(fetchProps).toHaveBeenCalledTimes(0); + // ...and the turn ran on the plain/non-local profile it started on, + // rather than one detected from a llama-server that is not serving. + expect(profileManager.getProfile().id).toBe(QWEN_THINK_PROFILE.id); + }); + + it("probes exactly once per local turn", async () => { + const { loop, fetchProps, profileManager } = await buildLoop({ + isActive: () => true, + ensureProbed: async () => false, + }); + + await runTurn(loop, "s-local"); + + // One turn-start refresh. The between-steps `refreshIfStale` is a + // no-op on a manager that is not stale, exactly as before #112. + expect(fetchProps).toHaveBeenCalledTimes(1); + expect(profileManager.getProfile().id).toBe("gemma4-think"); + }); + + it("behaves as before the gate when no gate is wired (legacy deps)", async () => { + const { loop, fetchProps } = await buildLoop(undefined); + await runTurn(loop, "s-legacy"); + expect(fetchProps).toHaveBeenCalledTimes(1); + }); + + it("lazily restores local state on the first turn after a switch back to local", async () => { + // Boot was cloud, so the probes were deferred; the operator has + // since switched the active provider to a llama-server link. + let active = false; + const restore = vi.fn(async () => {}); + const gate = new DeferredLocalBackendProbes( + { isActive: () => active, restore }, + /* probedAtBoot */ false, + ); + const { loop, fetchProps } = await buildLoop(gate); + + await runTurn(loop, "s-still-cloud"); + expect(restore).toHaveBeenCalledTimes(0); + expect(fetchProps).toHaveBeenCalledTimes(0); + + active = true; + await runTurn(loop, "s-switched"); + // The restore ran instead of the loop's own refresh — it already + // carries a fresh `/props`, so the turn does not probe twice. + expect(restore).toHaveBeenCalledTimes(1); + expect(fetchProps).toHaveBeenCalledTimes(0); + + // Every later local turn is back on the ordinary refresh. + await runTurn(loop, "s-local-again"); + expect(restore).toHaveBeenCalledTimes(1); + expect(fetchProps).toHaveBeenCalledTimes(1); + }); +}); + +/** + * Issue #112 review, F1 — the SUSTAINED cloud→local fallover. + * + * `fallback.appendLocal` defaults to `true`, so a rate-limited or down + * cloud primary falls over to the llama-server link on every turn under + * the default config shape. The active text provider stays cloud for the + * whole outage, which is what the loop's turn-start gate reads — so once + * `ensureProbed()` had latched on the first fallover, nothing refreshed + * the profile ever again and the prompt kept being built with the first + * model's template. `main` refreshed unconditionally at every turn start + * and did not have that hole. + * + * These tests drive the REAL `createFallbackCompleter` seam and the REAL + * `DeferredLocalBackendProbes` through a real `AgentLoop`, with + * `prepareLink` wired exactly as `bootstrap.ts` wires it, and hot-swap + * the model behind the fake llama-server between turns 2 and 3. + */ +describe("AgentLoop — sustained cloud→local fallover keeps the profile live", () => { + let workingDir: string; + + beforeEach(() => { + workingDir = mkdtempSync(join(tmpdir(), "atomic-loop-fallover-")); + }); + + afterEach(() => { + rmSync(workingDir, { recursive: true, force: true }); + }); + + /** Cloud primary + llama-server tail, as `resolveFallbackChain` builds it. */ + const LLM: ResolvedLlmConfig = { + activeTextProvider: "cloud", + activeEmbeddingProvider: "cloud", + providers: [ + { id: "cloud", kind: "openai" }, + { id: "local", kind: "llama-server" }, + ] as ResolvedLlmConfig["providers"], + toolTransport: "auto", + }; + + function fakeProvider( + id: string, + transport: "grammar" | "native_tools", + serve: () => Promise, + ): LlmProvider { + return { + id, + name: id, + capabilities: { + vision: false, + visionSource: "absent", + toolTransport: transport, + contextWindow: 128_000, + supportsParallelTools: transport === "native_tools", + supportsSlotAffinity: transport === "grammar", + supportsPromptCache: false, + reasoningFormat: "none", + }, + toolCallAdapter: + transport === "native_tools" ? openAiToolCallAdapter : null, + streamConsumer: null, + complete: serve, + async *completeStream() { + const result = await serve(); + yield { + delta: result.content, + reasoningDelta: "", + done: true, + } as StreamChunk; + return result; + }, + async describeImage() { + throw new Error("no vision"); + }, + async health() { + return { reachable: true, status: 200, error: null, latencyMs: 1 }; + }, + async close() {}, + } as unknown as LlmProvider; + } + + const buildFalloverLoop = async () => { + const grammar = await buildGrammar(QWEN_THINK_PROFILE); + // What the fake llama-server currently has loaded. Swapped mid-test. + let loaded = { + props: GEMMA4_PROPS as Record, + modelId: "gemma-4-it", + }; + const fetchProps = vi.fn(async () => loaded.props); + const healthProbes = vi.fn(); + + const profileManager = new ModelProfileManager({ + llama: { fetchProps } as unknown as LlamaServerClient, + initialProfile: QWEN_THINK_PROFILE, + initialGrammar: grammar, + // Boot was cloud, so nothing probed: the manager runs on the + // synthesized default until something warms it. + initialModelId: null, + }); + + const gate = new DeferredLocalBackendProbes( + { + // The active provider is cloud for the whole outage — the + // fallover never changes it. This is the exact condition that + // used to freeze the profile. + isActive: () => false, + restore: async () => { + healthProbes(); + await profileManager.refresh(); + }, + }, + /* probedAtBoot */ false, + ); + + // The profile the prompt was actually built with, per local + // completion. This is the assertion that matters: a stale profile + // here means a stale chat template and a stale GBNF grammar. + const servedWithProfile: string[] = []; + const localServe = async (): Promise => { + servedWithProfile.push(profileManager.getProfile().id); + return makeCompletion( + `${JSON.stringify({ + tool: "finish", + args: { summary: "done" }, + })}`, + loaded.modelId, + ); + }; + const providers = new Map([ + [ + "cloud", + fakeProvider("cloud", "native_tools", async () => { + throw new OpenAiHttpError( + "rate limited", + 429, + "http://cloud", + false, + null, + "cloud", + ); + }), + ], + ["local", fakeProvider("local", "grammar", localServe)], + ]); + + const llmComplete = createFallbackCompleter({ + fallbackChain: new ProviderFallbackChain({ + resolve: () => ({ + chain: ["cloud", "local"], + timing: DEFAULT_FALLBACK_TIMING, + }), + }), + resolveSlice: (providerId) => { + const provider = providers.get(providerId)!; + return { provider, transport: provider.capabilities.toolTransport }; + }, + // The REAL preparer `bootstrap.ts` wires into its seam deps, with + // the same three collaborators — not a re-implementation of it. + prepareLink: createLocalLinkPreparer({ + gate, + isLocalLink: (providerId) => providerIdIsLlamaServer(LLM, providerId), + refreshIfStale: () => profileManager.refreshIfStale(), + }), + recordUnaryUsage: () => {}, + recordStreamUsage: () => {}, + }); + + const loop = new AgentLoop({ + registry: buildDefaultToolRegistry(), + slotManager: new SlotManager(2), + grammar, + profile: QWEN_THINK_PROFILE, + profileManager, + localBackend: gate, + llmComplete, + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + }); + + return { + loop, + fetchProps, + healthProbes, + profileManager, + servedWithProfile, + swapModel: () => { + loaded = { + props: QWEN3_PROPS as Record, + modelId: "qwen3-30b-a3b-instruct-2507", + }; + }, + }; + }; + + const runTurn = async (loop: AgentLoop, id: string) => + loop.runTurn(createEmptySessionState({ id, workingDir }), { + userMessage: "go", + maxSteps: 2, + signal: new AbortController().signal, + }); + + it("re-probes on every turn the local link serves, and follows a hot swap on turn 3", async () => { + const t = await buildFalloverLoop(); + const propsAfter: number[] = []; + + // Turn 1 — cloud 429s, the chain falls over, the seam restores. + await runTurn(t.loop, "s1"); + propsAfter.push(t.fetchProps.mock.calls.length); + expect(t.healthProbes).toHaveBeenCalledTimes(1); + + // Turn 2 — still cloud-active, still falling over. The turn-start + // refresh must run again: `ensureProbed()` has latched, so before + // this fix nothing did. + await runTurn(t.loop, "s2"); + propsAfter.push(t.fetchProps.mock.calls.length); + + // The operator swaps the model behind llama-server mid-outage. + t.swapModel(); + + // Turn 3 — the swap must be picked up BEFORE the prompt is built. + await runTurn(t.loop, "s3"); + propsAfter.push(t.fetchProps.mock.calls.length); + + // One `/props` per turn, matching `main`'s unconditional turn-start + // refresh. Cumulative: 1, 2, 3. + expect(propsAfter).toEqual([1, 2, 3]); + // The restore is still one-shot — turns 2 and 3 refresh, they do not + // replay `/health`. + expect(t.healthProbes).toHaveBeenCalledTimes(1); + + // The load-bearing assertion. Turn 3's completion was built with the + // profile of the model llama-server is NOW serving. Pinned to + // `gemma4-think` before this fix. + expect(t.servedWithProfile).toEqual([ + "gemma4-think", + "gemma4-think", + "qwen-think", + ]); + expect(t.profileManager.getModelId()).toBe("qwen3-30b-a3b-instruct-2507"); + }); + + it("goes quiet again within one turn of the cloud primary recovering", async () => { + // The other half of take-and-clear: the refresh must not become a + // permanent per-turn `/props` just because one fallover happened. + const gate = new DeferredLocalBackendProbes( + { isActive: () => false, restore: async () => {} }, + /* probedAtBoot */ false, + ); + gate.noteLinkServed(); + expect(gate.takeLinkServed()).toBe(true); + expect(gate.takeLinkServed()).toBe(false); + }); +}); diff --git a/src/agent/agent-loop.ts b/src/agent/agent-loop.ts index 3bdf8f3c..b7ea8bb1 100644 --- a/src/agent/agent-loop.ts +++ b/src/agent/agent-loop.ts @@ -10,6 +10,7 @@ import { type ModelProfile, } from "../llm/model-profile.js"; import type { ModelProfileManager } from "../llm/model-profile-manager.js"; +import type { LocalBackendGate } from "../llm/local-backend-gate.js"; import type { ToolRegistry } from "../tools/tool-registry.js"; import { CancelledError, @@ -108,6 +109,19 @@ export interface AgentLoopDependencies { * for the lifetime of the loop (test-mode wiring). */ profileManager?: ModelProfileManager; + /** + * Gate for the `profileManager` probes above (issue #112). The manager + * talks to the local llama-server, so on a cloud turn its refreshes + * are pure `/props` noise against a backend nothing is routed to — + * `isActive()` false skips them. `ensureProbed()` covers the reverse + * case: the operator switched back to a local provider after a cloud + * boot that deferred the probes, and this turn is the first local one. + * It returns `true` when it just ran them, which already includes a + * fresh `/props` — the loop then skips its own refresh rather than + * probing twice. Absent (test / legacy wiring) means "always local", + * preserving the pre-#112 behaviour. + */ + localBackend?: LocalBackendGate; /** Skill catalog (name + description only), rebuilt on install/uninstall. */ skillCatalog: readonly SkillCatalogEntry[]; /** @@ -394,6 +408,15 @@ export interface RunTurnResult { export class AgentLoop { constructor(private readonly deps: AgentLoopDependencies) {} + /** + * Whether the local llama-server is the route this turn takes. No gate + * wired (test / legacy deps) reads as `true` so the profile manager + * behaves exactly as it did before issue #112. + */ + private localBackendActive(): boolean { + return this.deps.localBackend?.isActive() ?? true; + } + /** * Drive one macro-turn: * user message → 0..N tool steps → `reply` (or `finish` / max_steps). @@ -480,9 +503,27 @@ export class AgentLoop { // Proactively sync with the live `llama-server` before the first // step. Catches the case where the operator swapped the model // between turns — without this, step 0 would still build the prompt - // with the previous model's template. + // with the previous model's template. Skipped whole on a cloud turn + // (issue #112): there is no llama-server behind the prompt to sync + // with, and the probe would fail against a backend nobody is using. + // + // ...unless the previous turn was actually SERVED by a local link + // through the fallback chain. `appendLocal` defaults to `true`, so a + // rate-limited cloud primary falls over to llama-server on every + // turn while the active provider stays cloud; without this second + // arm the profile and grammar would stay pinned to whatever the + // first fallover probed for the whole outage. Take-and-clear, so a + // recovered primary quiets the probes again after one turn. + const localLinkServedLastTurn = + this.deps.localBackend?.takeLinkServed?.() ?? false; if (this.deps.profileManager) { - await this.deps.profileManager.refresh(); + if (this.localBackendActive()) { + if (!(await this.deps.localBackend?.ensureProbed())) { + await this.deps.profileManager.refresh(); + } + } else if (localLinkServedLastTurn) { + await this.deps.profileManager.refresh(); + } } let reason: AgentLoopReason = "max_steps"; @@ -545,8 +586,17 @@ export class AgentLoop { // Reactive refresh between steps: if the previous completion // observed a foreign `modelId`, rebuild profile + grammar so the // next prompt matches what `llama-server` is actually serving. - if (this.deps.profileManager) { - await this.deps.profileManager.refreshIfStale(); + // Same cloud-turn gate as the turn-start refresh (issue #112). + // Nothing is lost on a cloud turn that falls over: the fallback + // seam's `prepareLink` runs this same `refreshIfStale` for a + // `llama-server` link at the point the link is picked, which is + // strictly later than here and strictly closer to the request — + // the completion that flagged the manager stale may not even have + // happened yet when this line runs. + if (this.deps.profileManager && this.localBackendActive()) { + if (!(await this.deps.localBackend?.ensureProbed())) { + await this.deps.profileManager.refreshIfStale(); + } } this.deps.onEvent?.({ type: "step_started", stepIndex: i }); const started = Date.now(); diff --git a/src/llm/local-backend-gate.test.ts b/src/llm/local-backend-gate.test.ts new file mode 100644 index 00000000..61001ec0 --- /dev/null +++ b/src/llm/local-backend-gate.test.ts @@ -0,0 +1,189 @@ +import { describe, expect, it, vi } from "vitest"; + +import { + createLocalLinkPreparer, + DeferredLocalBackendProbes, +} from "./local-backend-gate.js"; + +describe("DeferredLocalBackendProbes", () => { + it("never restores when boot already probed (local-from-boot run)", async () => { + const restore = vi.fn(async () => {}); + const gate = new DeferredLocalBackendProbes( + { isActive: () => true, restore }, + true, + ); + expect(await gate.ensureProbed()).toBe(false); + expect(await gate.ensureProbed()).toBe(false); + expect(restore).toHaveBeenCalledTimes(0); + }); + + it("restores exactly once, and only the winner may skip its own refresh", async () => { + const restore = vi.fn(async () => {}); + const gate = new DeferredLocalBackendProbes( + { isActive: () => true, restore }, + false, + ); + expect(await gate.ensureProbed()).toBe(true); + expect(await gate.ensureProbed()).toBe(false); + expect(await gate.ensureProbed()).toBe(false); + expect(restore).toHaveBeenCalledTimes(1); + }); + + it("a concurrent caller waits for the restore but does not claim it", async () => { + // Turn start racing a mid-turn fallover: both must see warm state + // when they proceed, and only one may report "a fresh /props landed". + let release!: () => void; + const started = vi.fn(); + const gate = new DeferredLocalBackendProbes( + { + isActive: () => true, + restore: () => + new Promise((resolve) => { + started(); + release = resolve; + }), + }, + false, + ); + + const first = gate.ensureProbed(); + const second = gate.ensureProbed(); + expect(started).toHaveBeenCalledTimes(1); + release(); + + expect(await first).toBe(true); + expect(await second).toBe(false); + }); + + it("latches after a throwing restore so the probes cannot re-arm every step", async () => { + const restore = vi.fn(async () => { + throw new Error("llama-server is down"); + }); + const gate = new DeferredLocalBackendProbes( + { isActive: () => true, restore }, + false, + ); + await expect(gate.ensureProbed()).rejects.toThrow("llama-server is down"); + expect(await gate.ensureProbed()).toBe(false); + expect(restore).toHaveBeenCalledTimes(1); + }); + + it("latches after a SYNCHRONOUSLY throwing restore too", async () => { + // The async-throw test above passes even with the `restore()` call + // outside the try: the rejection is produced after `inFlight` has + // been assigned. A sync throw escapes before the assignment, so the + // latch never armed and every later call re-ran the probes — three + // `ensureProbed()` calls, three `restore()` calls. + const restore = vi.fn((): Promise => { + throw new Error("config read blew up"); + }); + const gate = new DeferredLocalBackendProbes( + { isActive: () => true, restore }, + false, + ); + await expect(gate.ensureProbed()).rejects.toThrow("config read blew up"); + expect(await gate.ensureProbed()).toBe(false); + expect(await gate.ensureProbed()).toBe(false); + expect(restore).toHaveBeenCalledTimes(1); + }); + + it("take-and-clear reports whether a local link served since the last read", () => { + const gate = new DeferredLocalBackendProbes( + { isActive: () => false, restore: async () => {} }, + false, + ); + // Nothing served yet: a pure cloud turn must not refresh anything. + expect(gate.takeLinkServed()).toBe(false); + + gate.noteLinkServed(); + expect(gate.takeLinkServed()).toBe(true); + // Cleared — one refresh per fallover, not one per turn forever. + expect(gate.takeLinkServed()).toBe(false); + + gate.noteLinkServed(); + gate.noteLinkServed(); + expect(gate.takeLinkServed()).toBe(true); + expect(gate.takeLinkServed()).toBe(false); + }); + + it("reads `isActive` per call so a hot switch is observed", () => { + let active = false; + const gate = new DeferredLocalBackendProbes( + { isActive: () => active, restore: async () => {} }, + false, + ); + expect(gate.isActive()).toBe(false); + active = true; + expect(gate.isActive()).toBe(true); + }); +}); + +describe("createLocalLinkPreparer", () => { + /** + * The three decisions bootstrap's `prepareLink` makes. Covered here + * because deleting any one of them from an inline closure inside + * `buildRuntime` used to survive every test in the tree. + */ + const build = (opts: { + isLocalLink?: (id: string) => boolean; + probedAtBoot?: boolean; + } = {}) => { + const restore = vi.fn(async () => {}); + const refreshIfStale = vi.fn(async () => {}); + const gate = new DeferredLocalBackendProbes( + { isActive: () => false, restore }, + opts.probedAtBoot ?? false, + ); + const prepare = createLocalLinkPreparer({ + gate, + isLocalLink: opts.isLocalLink ?? ((id) => id === "local"), + refreshIfStale, + }); + return { gate, prepare, restore, refreshIfStale }; + }; + + it("does nothing at all for a link that is not llama-server", async () => { + const { prepare, gate, restore, refreshIfStale } = build(); + await prepare("cloudy"); + expect(restore).toHaveBeenCalledTimes(0); + expect(refreshIfStale).toHaveBeenCalledTimes(0); + // The zero-request criterion in one assertion: a cloud attempt does + // not even record that a local link served. + expect(gate.takeLinkServed()).toBe(false); + }); + + it("marks the link served so the loop's turn-start refresh reopens", async () => { + const { prepare, gate } = build(); + await prepare("local"); + expect(gate.takeLinkServed()).toBe(true); + }); + + it("restores on the first local attempt and does not also refresh", async () => { + const { prepare, restore, refreshIfStale } = build(); + await prepare("local"); + expect(restore).toHaveBeenCalledTimes(1); + // The restore's own `/props` just landed; refreshing again would + // probe twice for one attempt. + expect(refreshIfStale).toHaveBeenCalledTimes(0); + }); + + it("falls through to refreshIfStale on every later local attempt", async () => { + // The reactive path the loop's between-steps refresh cannot serve + // while the active provider is cloud. + const { prepare, restore, refreshIfStale } = build(); + await prepare("local"); + await prepare("local"); + await prepare("local"); + expect(restore).toHaveBeenCalledTimes(1); + expect(refreshIfStale).toHaveBeenCalledTimes(2); + }); + + it("refreshes from the very first attempt on a local-from-boot run", async () => { + // Boot already probed, so there is nothing to restore — but the + // staleness flag still needs a consumer. + const { prepare, restore, refreshIfStale } = build({ probedAtBoot: true }); + await prepare("local"); + expect(restore).toHaveBeenCalledTimes(0); + expect(refreshIfStale).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/llm/local-backend-gate.ts b/src/llm/local-backend-gate.ts new file mode 100644 index 00000000..9f6c35c1 --- /dev/null +++ b/src/llm/local-backend-gate.ts @@ -0,0 +1,190 @@ +/** + * Gate for every probe aimed at the managed/external llama-server text + * backend (issue #112). + * + * A cloud-backed session used to open with `/health` + `/props` against + * `http://127.0.0.1:8080`, warn that nothing answered, and then run the + * whole session on a cloud provider that was healthy all along — the + * warning reads as an active-backend failure on the one screen where the + * operator has the least context to judge it. + * + * Boot skips those probes when the active text provider is not a + * `llama-server` link. That leaves the local state cold, which is only + * safe if it can be warmed again before local inference. Two paths reach + * local inference after a cloud boot and both call {@link + * LocalBackendGate.ensureProbed} first: + * + * - the operator switching the active text provider to a llama-server + * link (the agent loop's turn-start refresh); and + * - the fallback chain falling over from a cloud link to a + * `llama-server` link mid-turn (`createFallbackCompleter` / + * `createFallbackStreamer` prepare each link before the attempt). + * + * The second path is not a one-off: a rate-limited or down cloud primary + * falls over on *every* turn, and `appendLocal` defaults to `true`, so + * that is the shape of the default config under an outage. Restoring + * once is not enough there — the operator can still swap the model + * behind `llama-server` mid-outage, and the active provider stays cloud + * the whole time, so the loop's own turn-start refresh never re-opens. + * {@link LocalBackendGate.noteLinkServed} / {@link + * LocalBackendGate.takeLinkServed} carry that fact from the seam to the + * loop so the profile keeps tracking the live server for as long as the + * local link keeps serving — and stops within one turn of it stopping. + */ + +export interface LocalBackendGate { + /** Is the active text provider a `llama-server` link right now? */ + isActive(): boolean; + /** + * Run the probes boot skipped — `/health` logging, the `/props` + * profile + slot refresh, and the context-window advice — exactly + * once. + * + * Returns `true` only for the call that performed them, so a caller + * whose next act would be its own `/props` refresh can skip it: one + * just landed. `false` means boot already probed (a local-from-boot + * run) or another caller got there first, and the caller owns its + * usual refresh. + */ + ensureProbed(): Promise; + /** + * Record that a `llama-server` link just served — or is about to serve + * — an attempt while the *active* text provider is something else: a + * cloud→local fallover. + * + * The agent loop's turn-start refresh keys off the active provider, + * which stays cloud for the whole outage, so without this signal the + * profile and grammar would stay pinned to whatever the first fallover + * probed (issue #112 review, F1). Optional so legacy / test wiring that + * implements only the two original members still type-checks. + */ + noteLinkServed?(): void; + /** + * Take-and-clear the {@link noteLinkServed} flag: `true` when a local + * link served since the last call. Read once per turn by the agent + * loop, which then refreshes the profile even though the active + * provider is cloud. Clearing is what keeps this self-limiting — once + * the cloud primary recovers, exactly one more turn refreshes and then + * the local probes go quiet again. + */ + takeLinkServed?(): boolean; +} + +export interface LocalBackendGateDeps { + /** Live predicate — re-read per call so a hot switch is observed. */ + isActive: () => boolean; + /** The deferred boot probes, in boot order. Must not throw. */ + restore: () => Promise; +} + +/** + * `LocalBackendGate` with a one-shot restore. Not reset when the + * operator switches back to cloud: the state the restore rebuilds + * (profile, grammar, slot pool) stays valid and the profile manager + * keeps it fresh from then on, so re-arming would only buy a second + * round of the same probes. + */ +export class DeferredLocalBackendProbes implements LocalBackendGate { + private restored: boolean; + private inFlight: Promise | null = null; + private linkServed = false; + + /** + * @param probedAtBoot `true` when bootstrap already ran the probes + * (the active provider was local at boot), which makes `ensureProbed` + * a pure no-op for the life of the runtime. + */ + constructor( + private readonly deps: LocalBackendGateDeps, + probedAtBoot: boolean, + ) { + this.restored = probedAtBoot; + } + + isActive(): boolean { + return this.deps.isActive(); + } + + async ensureProbed(): Promise { + if (this.restored) return false; + // A concurrent caller (turn start racing a mid-turn fallover) waits + // on the same restore but reports `false`: it did not produce the + // fresh `/props` and must not claim the winner's right to skip. + if (this.inFlight !== null) { + await this.inFlight; + return false; + } + try { + // Inside the `try` so a SYNCHRONOUS throw from `restore` latches + // too. With the call outside it, the throw escaped before + // `inFlight` was even assigned and `restored` stayed `false` — the + // probes then re-armed on every single call, contradicting the + // contract below. Bootstrap's `restore` is `async` with a + // catch-all, so this was latent there, but the class is exported. + this.inFlight = this.deps.restore(); + await this.inFlight; + } finally { + // Latched even on failure. `restore` swallows its own errors, but + // a hard throw must not re-arm the probes on every step — the + // profile manager's refresh already owns retrying `/props`, and a + // dead backend fails the completion itself a moment later. + this.restored = true; + this.inFlight = null; + } + return true; + } + + noteLinkServed(): void { + this.linkServed = true; + } + + takeLinkServed(): boolean { + const served = this.linkServed; + this.linkServed = false; + return served; + } +} + +export interface LocalLinkPreparerDeps { + gate: LocalBackendGate; + /** Is `providerId` a `llama-server` link? Live, re-read per attempt. */ + isLocalLink: (providerId: string) => boolean; + /** + * `ModelProfileManager.refreshIfStale`, bound. A no-op unless a + * completion reported a model the manager does not believe is loaded. + */ + refreshIfStale: () => Promise; +} + +/** + * The fallback seam's `prepareLink`, as bootstrap wires it. A named + * function rather than an inline closure in `buildRuntime` so the three + * decisions it makes are testable on their own — inline, the only way to + * reach them was to boot a whole runtime and drive a real fallover. + * + * For a `llama-server` link, in order: + * + * 1. `noteLinkServed()` — tell the loop a local link is serving, so its + * turn-start refresh reopens for the duration of the outage even + * though the ACTIVE provider stays cloud. + * 2. `ensureProbed()` — replay the probes boot deferred. `true` means + * they just ran and a fresh `/props` already landed; nothing more to + * do for this attempt. + * 3. otherwise `refreshIfStale()` — on a cloud-active turn the loop's + * own between-steps refresh is gated off, which leaves this the only + * consumer of the staleness `observeCompletionModelId` flagged, and + * it sits closer to the request than the line it replaces. + * + * Every other link kind returns on the first line: one predicate call, + * and no local request on a turn the local backend never serves. + */ +export function createLocalLinkPreparer( + deps: LocalLinkPreparerDeps, +): (providerId: string) => Promise { + return async (providerId: string): Promise => { + if (!deps.isLocalLink(providerId)) return; + deps.gate.noteLinkServed?.(); + if (await deps.gate.ensureProbed()) return; + await deps.refreshIfStale(); + }; +} diff --git a/src/llm/provider/registry/active-text-provider.ts b/src/llm/provider/registry/active-text-provider.ts new file mode 100644 index 00000000..bf20c162 --- /dev/null +++ b/src/llm/provider/registry/active-text-provider.ts @@ -0,0 +1,47 @@ +import type { ResolvedLlmConfig } from "./provider-registry.js"; + +/** + * KIND-based local detection, mirroring `selectComposerBackend`: any + * `llama-server` entry is the local route, because `LlamaServerProvider` + * accepts a custom id (`options.id`) — keying on the literal + * `local-llama` id would leave a renamed entry ungated. An id that + * resolves to no entry reads as local too, matching the composer's + * no-active-row rule (and the no-`llm`-block default, which + * `resolveLlmConfig` synthesizes as a `llama-server` entry anyway). + * + * The conservative direction matters: every caller uses this to decide + * whether the local llama backend is worth probing, and an unrecognised + * id costs one probe against a backend nobody is using — while the + * opposite mistake runs inference on an unprobed profile. + * + * Not the only "is the route local?" predicate in the tree, and the two + * are NOT equivalent: `LocalModelsOrchestrator.autoStartIfReady` asks the + * same question by **id** (`activeTextProvider !== "local-llama"`). For a + * `llama-server` entry under a custom id the two disagree — this one + * calls it local, the orchestrator does not, so the managed daemon is + * not auto-started for it. That is pre-existing and errs toward less + * local activity (a custom-id local entry is almost always an external + * server the operator runs themselves), so it is left alone here rather + * than folded into this change; it is a divergence, not a shared + * default. + * + * Lives beside `resolveLlmConfig` rather than under `src/tui/` because + * `resolveLlmConfig` is a pure function of config with no I/O: the + * answer is available at the very top of `buildRuntime`, long before a + * `ProviderRegistry` exists. `src/tui/local-turn-gate.ts` re-exports it + * for its original callers. + */ +export function providerIdIsLlamaServer( + llm: ResolvedLlmConfig, + providerId: string, +): boolean { + const entry = llm.providers.find((p) => p.id === providerId); + return entry === undefined || entry.kind === "llama-server"; +} + +/** {@link providerIdIsLlamaServer} for the active text provider. */ +export function activeTextProviderIsLlamaServer( + llm: ResolvedLlmConfig, +): boolean { + return providerIdIsLlamaServer(llm, llm.activeTextProvider); +} diff --git a/src/llm/provider/registry/index.ts b/src/llm/provider/registry/index.ts index 33eb85fa..fda0c992 100644 --- a/src/llm/provider/registry/index.ts +++ b/src/llm/provider/registry/index.ts @@ -7,5 +7,9 @@ export { type UserModelConfigEntry, type ResolvedLlmConfig, } from "./provider-registry.js"; +export { + activeTextProviderIsLlamaServer, + providerIdIsLlamaServer, +} from "./active-text-provider.js"; export { registerBuiltInProviderKinds } from "./register-built-in-providers.js"; export { resolveActiveToolTransport } from "./resolve-tool-transport.js"; diff --git a/src/runtime/bootstrap.ts b/src/runtime/bootstrap.ts index 143bcc05..df87191b 100644 --- a/src/runtime/bootstrap.ts +++ b/src/runtime/bootstrap.ts @@ -71,6 +71,14 @@ import { resolveLlmConfig, } from "../llm/provider/index.js"; import { resolveActiveToolTransport } from "../llm/provider/registry/resolve-tool-transport.js"; +import { + activeTextProviderIsLlamaServer, + providerIdIsLlamaServer, +} from "../llm/provider/registry/active-text-provider.js"; +import { + createLocalLinkPreparer, + DeferredLocalBackendProbes, +} from "../llm/local-backend-gate.js"; import { catalogForProvider } from "../llm/provider/catalog-for-provider.js"; import { CostAccumulator } from "../llm/provider/cost-accumulator.js"; import { @@ -898,34 +906,59 @@ export async function createAgentRuntime( approvalRequired: true, }; - if ( - !options.overrides?.skipLlamaHealthCheck && - !options.overrides?.deferLlamaHealthCheck && - !options.overrides?.llamaComplete - ) { - // One attempt, not the retry ladder: this probe exists to log a line, - // and with llama down the default ladder (5 attempts, exponential - // backoff) stalled every boot for 15.5 s before the loop then failed - // fast anyway. The first real completion is the retry. - const health = await checkLlamaServer({ retries: 0 }); - if (!health.reachable) { - logger.warn("llama-server health check failed", { - error: health.error, - url: config.localModels.url, - }); - if (config.localModels.mode === "managed") { - logger.warn(managedLocalLlmHealthFailureHint(config.localModels.managed.port), { - mode: "managed", + // Issue #112. Every local text probe below hangs off this one answer, + // and it is available here — hundreds of lines before + // `ProviderRegistry.fromConfig` resolves the active provider — + // because `resolveLlmConfig` is a pure function of config with no + // I/O. A cloud-backed boot must not open `/health` or `/props` + // against a llama-server nobody is routed to: the warnings it prints + // read as an active-backend failure while the real provider is fine. + const localTextActiveAtBoot = activeTextProviderIsLlamaServer( + resolveLlmConfig(config), + ); + + // The boot-time local `/health` line. Skipped whole when the route is + // cloud — including the "deferred" notice, which is advice about a + // backend this session never talks to. + const runBootHealthProbe = async (): Promise => { + if ( + !options.overrides?.skipLlamaHealthCheck && + !options.overrides?.deferLlamaHealthCheck && + !options.overrides?.llamaComplete + ) { + // One attempt, not the retry ladder: this probe exists to log a line, + // and with llama down the default ladder (5 attempts, exponential + // backoff) stalled every boot for 15.5 s before the loop then failed + // fast anyway. The first real completion is the retry. + const health = await checkLlamaServer({ retries: 0 }); + if (!health.reachable) { + logger.warn("llama-server health check failed", { + error: health.error, + url: config.localModels.url, + }); + if (config.localModels.mode === "managed") { + logger.warn(managedLocalLlmHealthFailureHint(config.localModels.managed.port), { + mode: "managed", + }); + } + } else { + logger.info("llama-server reachable", { + url: config.localModels.url, + latencyMs: health.latencyMs, }); } - } else { - logger.info("llama-server reachable", { + } else if (options.overrides?.deferLlamaHealthCheck) { + logger.info("llama-server health check deferred; runtime will refresh on first turn", { url: config.localModels.url, - latencyMs: health.latencyMs, }); } - } else if (options.overrides?.deferLlamaHealthCheck) { - logger.info("llama-server health check deferred; runtime will refresh on first turn", { + }; + + if (localTextActiveAtBoot) { + await runBootHealthProbe(); + } else { + logger.info("local llama probes skipped; active text provider is not local", { + activeTextProvider: resolveLlmConfig(config).activeTextProvider, url: config.localModels.url, }); } @@ -936,6 +969,7 @@ export async function createAgentRuntime( llama, logger, config.localModels.url, + localTextActiveAtBoot, ); const slotManager = new SlotManager(totalSlots ?? undefined); if (totalSlots !== null) { @@ -958,19 +992,31 @@ export async function createAgentRuntime( // generation budget makes every step come back `truncated` — the model // burns its remaining tokens and never closes a tool-call array. Loud // at startup because the failure mode downstream is silent. - const minUsableCtx = minUsableContextWindow( - config.localModels.completionMaxTokens, - ); - if (profile.contextWindow && profile.contextWindow < minUsableCtx) { - logger.warn("context window too small for the agent prompt", { - contextWindow: profile.contextWindow, - required: minUsableCtx, - completionMaxTokens: config.localModels.completionMaxTokens, - hint: - config.localModels.mode === "managed" - ? "raise localModels.managed.contextSize, lower localModels.completionMaxTokens, or pick a model that fits VRAM" - : "start llama-server with a larger --ctx-size, or lower localModels.completionMaxTokens", - }); + // + // Advice about the LOCAL server's `--ctx-size` only, so it rides the + // same gate as the probe that produced the number (issue #112): on a + // cloud route there is no `/props` reading to judge, and the hints it + // prints name flags a cloud provider does not have. + const warnOnSmallContextWindow = ( + candidate: ReturnType, + ): void => { + const minUsableCtx = minUsableContextWindow( + config.localModels.completionMaxTokens, + ); + if (candidate.contextWindow && candidate.contextWindow < minUsableCtx) { + logger.warn("context window too small for the agent prompt", { + contextWindow: candidate.contextWindow, + required: minUsableCtx, + completionMaxTokens: config.localModels.completionMaxTokens, + hint: + config.localModels.mode === "managed" + ? "raise localModels.managed.contextSize, lower localModels.completionMaxTokens, or pick a model that fits VRAM" + : "start llama-server with a larger --ctx-size, or lower localModels.completionMaxTokens", + }); + } + }; + if (localTextActiveAtBoot) { + warnOnSmallContextWindow(profile); } const browserBackend: BrowserBackend = @@ -1290,6 +1336,42 @@ export async function createAgentRuntime( const getLiveProfile = () => profileManager?.getProfile() ?? profile; + // Issue #112. The manager above is built either way — construction is + // pure field assignment, no I/O — because deleting it on a cloud boot + // would leave a mid-turn fallover to a `llama-server` link running on + // a frozen `plain-instruct` profile with no way back. What is gated is + // its *probing*: the boot probes are deferred here and replayed once, + // lazily, by whichever path reaches local inference first (a provider + // switch, via the agent loop's turn-start gate, or a cloud→local + // fallover, via the fallback seam's `prepareLink`). + const localBackend = new DeferredLocalBackendProbes( + { + isActive: () => + activeTextProviderIsLlamaServer(resolveLlmConfig(getConfig())), + restore: async () => { + logger.info("restoring local llama backend state", { + url: config.localModels.url, + }); + try { + await runBootHealthProbe(); + // `refresh()` is the deferred `/props`: profile, grammar and + // the slot pool (via `onTotalSlots`) in one round trip. It + // swallows its own failures and keeps the prior profile. + await profileManager?.refresh(); + warnOnSmallContextWindow(getLiveProfile()); + } catch (err) { + // The seam awaits this before a fallover attempt: a throw here + // would fail the link and advance the chain over a diagnostic. + // The completion itself is the real verdict on the backend. + logger.warn("local llama backend restore failed; continuing", { + error: err instanceof Error ? err.message : String(err), + }); + } + }, + }, + localTextActiveAtBoot, + ); + const providerRegistry = await ProviderRegistry.fromConfig(config, { config, llamaClient: llama, @@ -1591,6 +1673,20 @@ export async function createAgentRuntime( const { provider, transport } = resolveActiveLlmSlice(providerId); return { provider, transport }; }, + // Issue #112. The one place that knows a cloud→local fallover is + // about to happen: the chain has already picked the link and the + // completion has not been sent. A `llama-server` link reached from a + // cloud boot runs on deferred state (plain profile, one-slot pool, + // no `/props`), so warm it here rather than infer against it. + // No-op on every other attempt — one boolean after the first call. + prepareLink: createLocalLinkPreparer({ + gate: localBackend, + isLocalLink: (providerId) => + providerIdIsLlamaServer(resolveLlmConfig(getConfig()), providerId), + refreshIfStale: async () => { + await profileManager?.refreshIfStale(); + }, + }), recordUnaryUsage, recordStreamUsage, }; @@ -1981,6 +2077,9 @@ export async function createAgentRuntime( profile, contextWindow: resolveCatalogContextWindow, ...(profileManager ? { profileManager } : {}), + // Gates the two `/props` refreshes the loop owns, and carries the + // lazy restore for a switch back to a local provider (issue #112). + localBackend, ...(config.memory.profile.enabled ? { profileFactsProvider: () => profileStore.list() } : {}), @@ -2801,7 +2900,23 @@ async function resolveModelProfile( llama: LlamaServerClient, logger: StructuredLogger, llamaUrl: string, + /** + * `false` when the active text provider is not a `llama-server` link: + * the `/props` probe is skipped entirely and the run starts on the + * plain profile (issue #112). Deliberately silent — the cloud route is + * not a failed probe, and the "using plain fallback" warning below + * would say it was. A later switch or fallover to a local link warms + * the real profile through `DeferredLocalBackendProbes`. + */ + probeLocal: boolean, ): Promise { + if (!probeLocal) { + return { + profile: PLAIN_INSTRUCT_PROFILE, + modelAlias: null, + totalSlots: null, + }; + } if (overrides?.llamaPropsError) { logger.warn("model profile probe failed; using plain fallback", { error: overrides.llamaPropsError.message, diff --git a/src/runtime/llm-fallback-seam.test.ts b/src/runtime/llm-fallback-seam.test.ts index 51c0ee82..cad1284a 100644 --- a/src/runtime/llm-fallback-seam.test.ts +++ b/src/runtime/llm-fallback-seam.test.ts @@ -299,3 +299,115 @@ describe("per-link prompt substitution (grammarPrompt)", () => { expect(localPrompts).toEqual(["shared prompt"]); }); }); + +/** + * Issue #112. Boot skips the local `/health` + `/props` probes while a + * cloud provider is active, which leaves a `llama-server` link running + * on a deferred profile, a one-slot pool and no health reading. A + * cloud→local FALLOVER reaches that link without any config change and + * without the agent loop's turn-start refresh (it saw a cloud route when + * the turn began), so the seam is the last point at which the state can + * still be warmed. These tests pin the ordering: `prepareLink` for the + * link that is about to serve, before its completion is sent. + */ +describe("prepareLink — warming a link before it serves (issue #112)", () => { + function tracingDeps( + providers: Map, + trace: string[], + ): FallbackSeamDeps { + const deps = seamDeps(providers); + deps.prepareLink = async (providerId) => { + trace.push(`prepare:${providerId}`); + }; + return deps; + } + + it("prepares the local link before the fallover attempt is sent", async () => { + const trace: string[] = []; + const providers = new Map([ + [ + "cloud", + fakeProvider("cloud", "native_tools", async () => { + trace.push("serve:cloud"); + throw new OpenAiHttpError( + "rate limited", + 429, + "http://cloud", + false, + null, + "cloud", + ); + }), + ], + [ + "local", + fakeProvider("local", "grammar", async () => { + trace.push("serve:local"); + return answer("local"); + }), + ], + ]); + const result = await createFallbackCompleter( + tracingDeps(providers, trace), + )(baseParams); + + expect(result.modelId).toBe("local-model"); + // The load-bearing ordering: `prepare:local` sits BEFORE + // `serve:local`. Without the hook the local link would answer with + // its profile, grammar and slot pool never probed. + expect(trace).toEqual([ + "prepare:cloud", + "serve:cloud", + "prepare:local", + "serve:local", + ]); + }); + + it("streaming: prepares the local link before the stream is opened", async () => { + const trace: string[] = []; + const providers = new Map([ + [ + "cloud", + fakeProvider("cloud", "native_tools", async () => { + trace.push("serve:cloud"); + throw new OpenAiHttpError( + "rate limited", + 429, + "http://cloud", + false, + null, + "cloud", + ); + }), + ], + [ + "local", + fakeProvider("local", "grammar", async () => { + trace.push("serve:local"); + return answer("local"); + }), + ], + ]); + const streamer = createFallbackStreamer(tracingDeps(providers, trace)); + const gen = streamer(baseParams); + let next = await gen.next(); + while (!next.done) next = await gen.next(); + + expect(next.value.servedTransport).toBe("grammar"); + expect(trace).toEqual([ + "prepare:cloud", + "serve:cloud", + "prepare:local", + "serve:local", + ]); + }); + + it("is optional — an unwired seam behaves exactly as before", async () => { + const providers = new Map([ + ["cloud", fakeProvider("cloud", "native_tools", async () => answer("cloud"))], + ["local", fakeProvider("local", "grammar", async () => answer("local"))], + ]); + const result = await createFallbackCompleter(seamDeps(providers))(baseParams); + expect(result.modelId).toBe("cloud-model"); + }); +}); diff --git a/src/runtime/llm-fallback-seam.ts b/src/runtime/llm-fallback-seam.ts index 2d47e002..69559d7d 100644 --- a/src/runtime/llm-fallback-seam.ts +++ b/src/runtime/llm-fallback-seam.ts @@ -34,6 +34,31 @@ export interface FallbackSeamDeps { fallbackChain: ProviderFallbackChain; /** Resolve the served link's provider + transport for `providerId`. */ resolveSlice: (providerId: string) => ResolvedLinkSlice; + /** + * Awaited once per attempt, before the completion is sent, with the + * link the chain picked. Exists for the state a link may need warmed + * before it can serve: a `llama-server` link reached by fallover from + * a cloud primary boots with its `/health` + `/props` probes deferred + * (issue #112), and this is the last point at which they can still + * run. Kept as a hook rather than folded into `resolveSlice` because + * that seam is synchronous, and rather than into the attempt body + * because only bootstrap knows what "warm" means for a link kind. + * + * Must not throw for a reachable link: a rejection here fails the + * attempt and advances the chain, same as a failed completion. + * + * The `providerId` says WHICH link is about to serve, not where it + * lives. Bootstrap's implementation warms the one local backend the + * runtime owns — the `ModelProfileManager` built over the shared + * `LlamaServerClient`, which reads `localModels.url` per request — so + * a second `llama-server` entry pointed at a different host is + * announced here but warmed against the configured URL. That is a + * pre-existing `ModelProfileManager` limitation (it is a singleton + * over one client, not a per-link cache), not something this hook + * introduces; multi-endpoint local links would need a manager per + * link before it could mean anything more. + */ + prepareLink?: (providerId: string) => Promise; /** Fold a unary completion's usage into cost + meter (no-op when absent). */ recordUnaryUsage: (params: LlmStreamParams, result: CompletionResult) => void; /** Fold a streamed completion's usage into the meter. */ @@ -79,6 +104,7 @@ export function createFallbackCompleter( runWithFallback( deps.fallbackChain, async (providerId) => { + await deps.prepareLink?.(providerId); const { provider, transport } = deps.resolveSlice(providerId); const base = { prompt: promptFor(params, transport), @@ -142,6 +168,7 @@ export function createFallbackStreamer( primed: PrimedStream; transport: ToolCallTransport; }> => { + await deps.prepareLink?.(providerId); const { provider, transport } = deps.resolveSlice(providerId); const base = { prompt: promptFor(params, transport), diff --git a/src/runtime/local-probe-gating.test.ts b/src/runtime/local-probe-gating.test.ts new file mode 100644 index 00000000..0a1d6fca --- /dev/null +++ b/src/runtime/local-probe-gating.test.ts @@ -0,0 +1,453 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { createAgentRuntime } from "./bootstrap.js"; +import { + getUserConfigPath, + resetConfigCache, + USER_CONFIG_DEFAULTS, + writeUserConfigFileSync, +} from "../config/index.js"; +import type { UserConfigFile } from "../config/index.js"; +import { GEMMA4_PROPS } from "../llm/model-profile.fixtures.js"; +import { DEFAULT_EMBEDDING_MODEL_ID } from "../local-llm/index.js"; +import { FakeBrowserBackend } from "../http/test-harness.js"; +import type { LogRecord } from "../tracing/structured-logger.js"; + +/** + * Issue #112 — a cloud-backed session must not probe the local + * llama-server. + * + * The assertions are exact request COUNTS against the two local ports, + * not "was it called": the bug was a fixed number of probes (`/health` + * once, `/props` once) firing on a route that never uses them, and a + * boolean would pass again the moment one of them came back. + */ + +const TEXT_PORT = "127.0.0.1:8080"; +const EMBED_PORT = "127.0.0.1:19092"; + +interface LocalTraffic { + /** Every URL the process asked for, in order. */ + urls: string[]; + countTo(hostPort: string, path?: string): number; + firstIndexOf(hostPort: string, path: string): number; + reset(): void; + /** Flip the fake cloud provider to rate-limiting, to force a fallover. */ + rateLimitCloud(): void; +} + +/** + * Count every outbound request. Answers the local endpoints with real + * llama.cpp shapes so the *local* control cases probe successfully — + * a stub that failed every probe would make "zero requests" and "all + * requests failed" indistinguishable. + */ +function installCountingFetch(): LocalTraffic { + const urls: string[] = []; + let cloudRateLimited = false; + const impl: typeof fetch = async (input) => { + const url = + typeof input === "string" + ? input + : input instanceof URL + ? input.toString() + : (input as Request).url; + urls.push(url); + if (url.includes(TEXT_PORT) || url.includes(EMBED_PORT)) { + if (url.includes("/completion")) { + // Answers with a 200 the client accepts as a completion (the + // content parses to nothing useful, which is fine — the lazy + // restore assertions are about the probes, not the reply). + return new Response(JSON.stringify({ content: "", stop: true }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + if (url.includes("/props")) { + return new Response(JSON.stringify(GEMMA4_PROPS), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + if (url.includes("/health")) { + return new Response(JSON.stringify({ status: "ok" }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + } + // A WORKING cloud provider, so a "cloud turn" is a turn the cloud + // link actually SERVES. Answering it flatly would make the turn fail + // before the fallback chain's local tail is ever consulted, which is + // the one thing a zero-request assertion over cloud turns must not + // do. + if (url.includes("cloud.invalid") && url.includes("completions")) { + if (cloudRateLimited) { + // The shape that makes `runWithFallback` advance to the next + // link rather than fail the turn. + return new Response(JSON.stringify({ error: { message: "slow down" } }), { + status: 429, + headers: { "content-type": "application/json" }, + }); + } + return new Response( + JSON.stringify({ + id: "c1", + object: "chat.completion", + created: 1, + model: "cloudy-1", + choices: [ + { + index: 0, + message: { + role: "assistant", + content: '{"tool":"finish","args":{"summary":"ok"}}', + }, + finish_reason: "stop", + }, + ], + usage: { prompt_tokens: 10, completion_tokens: 5, total_tokens: 15 }, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + // Everything else (analytics, update check, provider catalogues) + // is answered flatly so no test ever reaches the network. + return new Response("{}", { + status: 404, + headers: { "content-type": "application/json" }, + }); + }; + vi.stubGlobal("fetch", impl); + return { + urls, + countTo: (hostPort, path) => + urls.filter((u) => u.includes(hostPort) && (!path || u.includes(path))) + .length, + firstIndexOf: (hostPort, path) => + urls.findIndex((u) => u.includes(hostPort) && u.includes(path)), + reset: () => { + urls.length = 0; + }, + rateLimitCloud: () => { + cloudRateLimited = true; + }, + }; +} + +/** A cloud text provider that needs no network to construct. */ +const CLOUD_PROVIDER = { + id: "cloudy", + kind: "openai-compatible" as const, + baseUrl: "https://cloud.invalid", + defaultChatModel: "cloudy-1", + apiKey: "sk-test", +}; + +function writeConfig(stateDir: string, over: Partial): void { + writeUserConfigFileSync(getUserConfigPath(stateDir), { + ...USER_CONFIG_DEFAULTS, + // Keep the runtime off the network for everything unrelated. + analytics: { enabled: false }, + ...over, + }); + resetConfigCache(); +} + +/** The embedding half of the registry, left on the local default. */ +const LOCAL_EMBED_PROVIDER = { + id: "local-llama-embed", + kind: "llama-server" as const, + url: "http://127.0.0.1:19092", +}; + +const cloudLlm = { + activeTextProvider: "cloudy", + activeEmbeddingProvider: "local-llama-embed", + providers: [CLOUD_PROVIDER, LOCAL_EMBED_PROVIDER], + toolTransport: "auto" as const, +}; + +describe("issue #112 — local probe gating at CLI bootstrap", () => { + let stateDir: string; + let workingDir: string; + let traffic: LocalTraffic; + + beforeEach(() => { + stateDir = mkdtempSync(join(tmpdir(), "atomic-gate-")); + workingDir = mkdtempSync(join(tmpdir(), "atomic-gate-cwd-")); + mkdirSync(join(workingDir, ".atomic-agent", "skills"), { recursive: true }); + process.env.ATOMIC_AGENT_STATE_DIR = stateDir; + process.env.ATOMIC_AGENT_GRAMMARS_DIR = join(process.cwd(), "grammars"); + resetConfigCache(); + traffic = installCountingFetch(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + rmSync(stateDir, { recursive: true, force: true }); + rmSync(workingDir, { recursive: true, force: true }); + delete process.env.ATOMIC_AGENT_STATE_DIR; + delete process.env.ATOMIC_AGENT_GRAMMARS_DIR; + resetConfigCache(); + }); + + const boot = async (logs: LogRecord[] = []) => + createAgentRuntime({ + workingDir, + approvalLevel: 5, + handlers: { logSinks: [(record) => logs.push(record)] }, + overrides: { + browserBackend: new FakeBrowserBackend(), + // Unary seam only: the streaming path's SSE shape is beside the + // point here, and the two share `prepareLink`. + disableStreaming: true, + }, + }); + + it("makes zero local text requests with a cloud text provider", async () => { + writeConfig(stateDir, { llm: cloudLlm }); + const logs: LogRecord[] = []; + const runtime = await boot(logs); + try { + expect(traffic.countTo(TEXT_PORT)).toBe(0); + expect(traffic.countTo(TEXT_PORT, "/health")).toBe(0); + expect(traffic.countTo(TEXT_PORT, "/props")).toBe(0); + // ...and says nothing alarming about the backend it skipped. + const complaints = logs.filter( + (r) => + (r.level === "warn" || r.level === "error") && + /llama|context window/i.test(r.message), + ); + expect(complaints).toEqual([]); + } finally { + await runtime.shutdown(); + } + }); + + /** + * Issue #112 review, F1. The gating is no longer a single latch: the + * loop refreshes the profile again whenever a `llama-server` link + * SERVED the previous turn, so that a sustained cloud->local fallover + * does not freeze the profile. That arm must stay shut on a session + * where the local link never serves — including turns 2 and 3, which a + * boot-only assertion would never reach. + * + * The config is the default fallover shape: `appendLocal` defaults to + * `true` and a `llama-server` text entry is configured, so the chain + * really is `[cloudy, local-llama]`; the cloud link simply keeps + * answering, and nothing behind it is touched. + */ + it("makes zero local text requests across three cloud TURNS, not just at boot", async () => { + writeConfig(stateDir, { + llm: { + ...cloudLlm, + providers: [ + CLOUD_PROVIDER, + { id: "local-llama", kind: "llama-server", url: "http://127.0.0.1:8080" }, + LOCAL_EMBED_PROVIDER, + ], + }, + }); + const runtime = await boot(); + try { + expect(traffic.countTo(TEXT_PORT)).toBe(0); + const session = runtime.createSession(); + for (let i = 0; i < 3; i += 1) { + await runtime + .executeTurn(session, `hello ${i}`, { + maxSteps: 2, + signal: new AbortController().signal, + }) + .catch(() => undefined); + // Reported with the turn index so a failure names the turn. + expect({ turn: i, local: traffic.countTo(TEXT_PORT) }).toEqual({ + turn: i, + local: 0, + }); + } + // ...and the turns really were served by the cloud link. + expect( + traffic.urls.filter( + (u) => u.includes("cloud.invalid") && u.includes("completions"), + ).length, + ).toBe(3); + } finally { + await runtime.shutdown(); + } + }); + + it("still probes when the active text provider IS a llama-server link", async () => { + // The control for the case above: same code path, local route. + writeConfig(stateDir, {}); + const runtime = await boot(); + try { + expect(traffic.countTo(TEXT_PORT, "/health")).toBe(1); + expect(traffic.countTo(TEXT_PORT, "/props")).toBe(1); + } finally { + await runtime.shutdown(); + } + }); + + it("gates on provider KIND, not the `local-llama` id", async () => { + // A llama-server entry under a custom id is still the local route. + writeConfig(stateDir, { + llm: { + activeTextProvider: "my-box", + activeEmbeddingProvider: "local-llama-embed", + providers: [ + { id: "my-box", kind: "llama-server", url: "http://127.0.0.1:8080" }, + LOCAL_EMBED_PROVIDER, + ], + toolTransport: "auto", + }, + }); + const runtime = await boot(); + try { + expect(traffic.countTo(TEXT_PORT, "/health")).toBe(1); + expect(traffic.countTo(TEXT_PORT, "/props")).toBe(1); + } finally { + await runtime.shutdown(); + } + }); + + it("lazily restores the local backend when the operator switches to it", async () => { + // The whole point of deferring rather than deleting: the state boot + // skipped has to come back before local inference, not at the next + // process start. + writeConfig(stateDir, { + llm: { + ...cloudLlm, + providers: [ + CLOUD_PROVIDER, + { id: "local-llama", kind: "llama-server", url: "http://127.0.0.1:8080" }, + LOCAL_EMBED_PROVIDER, + ], + }, + }); + const runtime = await boot(); + try { + expect(traffic.countTo(TEXT_PORT)).toBe(0); + + // Exactly what the LLM tab does: registry first, then config. + await runtime.providerRegistry.setActive("local-llama"); + writeConfig(stateDir, { + llm: { + ...cloudLlm, + activeTextProvider: "local-llama", + providers: [ + CLOUD_PROVIDER, + { id: "local-llama", kind: "llama-server", url: "http://127.0.0.1:8080" }, + LOCAL_EMBED_PROVIDER, + ], + }, + }); + + const session = runtime.createSession(); + await runtime + .executeTurn(session, "hello", { + maxSteps: 1, + signal: new AbortController().signal, + }) + .catch(() => undefined); + + // Health, profile/`/props` and the slot pool are warm before the + // first local completion — none of which boot had done. + expect(traffic.countTo(TEXT_PORT, "/health")).toBe(1); + expect(traffic.countTo(TEXT_PORT, "/props")).toBe(1); + } finally { + await runtime.shutdown(); + } + }); + + /** + * Issue #112 review, F1 + F7 — the fallover, end to end through a + * booted runtime rather than at the seam. + * + * Boot is cloud, so the local link starts with no `/health`, no + * `/props`, a `plain-instruct` profile and a one-slot pool. Then the + * cloud primary starts returning 429 and `appendLocal` (default + * `true`) routes every turn onto the llama-server link. Two things + * have to hold, and neither was covered end-to-end before: the link is + * warmed BEFORE its first completion, and it keeps being refreshed on + * later turns even though the active provider never stops being cloud. + */ + it("a cloud->local FALLOVER warms the link before it serves, turn after turn", async () => { + writeConfig(stateDir, { + llm: { + ...cloudLlm, + providers: [ + CLOUD_PROVIDER, + { id: "local-llama", kind: "llama-server", url: "http://127.0.0.1:8080" }, + LOCAL_EMBED_PROVIDER, + ], + }, + }); + const runtime = await boot(); + try { + expect(traffic.countTo(TEXT_PORT)).toBe(0); + traffic.rateLimitCloud(); + const session = runtime.createSession(); + + const turn = async (n: number) => + runtime + .executeTurn(session, `hello ${n}`, { + maxSteps: 1, + signal: new AbortController().signal, + }) + .catch(() => undefined); + + await turn(0); + // The deferred boot probes were replayed by the seam... + expect(traffic.countTo(TEXT_PORT, "/health")).toBe(1); + expect(traffic.countTo(TEXT_PORT, "/props")).toBe(1); + // ...and BEFORE the local link was asked to serve. Ordering is the + // whole point: a `/props` that lands after the completion has + // already been sent on a plain profile buys nothing. + const props = traffic.firstIndexOf(TEXT_PORT, "/props"); + const completion = traffic.firstIndexOf(TEXT_PORT, "/completion"); + expect(completion).toBeGreaterThan(-1); + expect(props).toBeLessThan(completion); + + // Turns 2 and 3: the profile keeps tracking the live server. The + // active provider is still cloud, so before the F1 fix these were + // both zero and the profile stayed frozen for the whole outage. + await turn(1); + expect(traffic.countTo(TEXT_PORT, "/props")).toBe(2); + await turn(2); + expect(traffic.countTo(TEXT_PORT, "/props")).toBe(3); + // The restore stays one-shot: `/health` is not replayed per turn. + expect(traffic.countTo(TEXT_PORT, "/health")).toBe(1); + } finally { + await runtime.shutdown(); + } + }); + + it("keeps probing local embeddings while the text route is cloud", async () => { + writeConfig(stateDir, { + llm: cloudLlm, + localModels: { + ...USER_CONFIG_DEFAULTS.localModels, + embeddings: { + ...USER_CONFIG_DEFAULTS.localModels.embeddings, + enabled: true, + modelId: DEFAULT_EMBEDDING_MODEL_ID, + }, + }, + memory: { + ...USER_CONFIG_DEFAULTS.memory, + embeddings: { ...USER_CONFIG_DEFAULTS.memory.embeddings, enabled: true }, + }, + }); + const runtime = await boot(); + try { + expect(traffic.countTo(EMBED_PORT, "/health")).toBe(1); + expect(traffic.countTo(TEXT_PORT)).toBe(0); + } finally { + await runtime.shutdown(); + } + }); +}); diff --git a/src/sidecar/local-probe-gating.test.ts b/src/sidecar/local-probe-gating.test.ts new file mode 100644 index 00000000..a0b1e851 --- /dev/null +++ b/src/sidecar/local-probe-gating.test.ts @@ -0,0 +1,210 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { bootstrapSidecar } from "./main.js"; +import { + getUserConfigPath, + resetConfigCache, + USER_CONFIG_DEFAULTS, + writeUserConfigFileSync, +} from "../config/index.js"; +import type { UserConfigFile } from "../config/index.js"; +import { GEMMA4_PROPS } from "../llm/model-profile.fixtures.js"; +import type { SidecarMessage } from "./sidecar-events.js"; + +/** + * Issue #112 — `start_session` ran an unconditional local `/health` + * probe after `buildRuntime` and emitted `llm_unavailable` when nothing + * answered. On a cloud-backed session that event tells the desktop shell + * the backend is down for a session that is about to run perfectly. + * + * The sidecar is driven the way the host drives it: an NDJSON request + * pushed at the real stdin stream, the response read off stdout. + */ + +const TEXT_PORT = "127.0.0.1:8080"; + +describe("sidecar start_session — local probe gating", () => { + let stateDir: string; + let workingDir: string; + let previousStateDir: string | undefined; + let urls: string[]; + let stdout: string[]; + + beforeEach(() => { + stateDir = mkdtempSync(join(tmpdir(), "atomic-sidecar-gate-")); + workingDir = mkdtempSync(join(tmpdir(), "atomic-sidecar-cwd-")); + mkdirSync(join(workingDir, ".atomic-agent", "skills"), { recursive: true }); + previousStateDir = process.env.ATOMIC_AGENT_STATE_DIR; + process.env.ATOMIC_AGENT_STATE_DIR = stateDir; + process.env.ATOMIC_AGENT_GRAMMARS_DIR = join(process.cwd(), "grammars"); + resetConfigCache(); + + urls = []; + vi.stubGlobal("fetch", async (input: unknown) => { + const url = + typeof input === "string" + ? input + : input instanceof URL + ? input.toString() + : (input as Request).url; + urls.push(url); + if (url.includes(TEXT_PORT) && url.includes("/props")) { + return new Response(JSON.stringify(GEMMA4_PROPS), { status: 200 }); + } + if (url.includes(TEXT_PORT) && url.includes("/health")) { + return new Response(JSON.stringify({ status: "ok" }), { status: 200 }); + } + return new Response("{}", { status: 404 }); + }); + + // The sidecar speaks NDJSON on the real stdout; capture it instead + // of letting it interleave with the reporter's output. + stdout = []; + vi.spyOn(process.stdout, "write").mockImplementation((chunk: unknown) => { + stdout.push(String(chunk)); + return true; + }); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + rmSync(stateDir, { recursive: true, force: true }); + rmSync(workingDir, { recursive: true, force: true }); + if (previousStateDir === undefined) { + delete process.env.ATOMIC_AGENT_STATE_DIR; + } else { + process.env.ATOMIC_AGENT_STATE_DIR = previousStateDir; + } + delete process.env.ATOMIC_AGENT_GRAMMARS_DIR; + resetConfigCache(); + }); + + const writeConfig = (llm: UserConfigFile["llm"]): void => { + writeUserConfigFileSync(getUserConfigPath(stateDir), { + ...USER_CONFIG_DEFAULTS, + analytics: { enabled: false }, + ...(llm ? { llm } : {}), + }); + resetConfigCache(); + }; + + /** + * Boot the sidecar, push one `start_session`, wait for its response. + * + * `bootstrapSidecar` attaches to the process-wide stdin/stdout, and a + * listener left behind would make the NEXT test's request run through + * two sidecars at once (two runtimes seeding the same skills dir, and + * whichever answered first winning the response). So the listeners it + * adds are recorded and removed on the way out. + */ + const startSession = async (): Promise<{ + messages: SidecarMessage[]; + shutdown: () => Promise; + }> => { + const before = new Map([ + ["stdin:data", process.stdin.listeners("data").slice()], + ["stdin:end", process.stdin.listeners("end").slice()], + ["stdout:error", process.stdout.listeners("error").slice()], + ["stdout:close", process.stdout.listeners("close").slice()], + ]); + const detach = (): void => { + for (const [key, kept] of before) { + const [target, event] = key.split(":") as ["stdin" | "stdout", string]; + const emitter = target === "stdin" ? process.stdin : process.stdout; + for (const listener of emitter.listeners(event)) { + if (!kept.includes(listener)) { + emitter.removeListener(event, listener as () => void); + } + } + } + }; + const { shutdown } = await bootstrapSidecar(); + process.stdin.emit( + "data", + `${JSON.stringify({ + kind: "request", + id: "req-1", + type: "start_session", + payload: { workingDir }, + })}\n`, + ); + const deadline = Date.now() + 10_000; + const parsed = (): SidecarMessage[] => + stdout + .join("") + .split("\n") + .filter((line) => line.trim().length > 0) + .flatMap((line) => { + try { + return [JSON.parse(line) as SidecarMessage]; + } catch { + return []; + } + }); + while ( + Date.now() < deadline && + !parsed().some((m) => m.kind === "response") + ) { + await new Promise((r) => setTimeout(r, 20)); + } + return { + messages: parsed(), + shutdown: async () => { + detach(); + await shutdown(); + }, + }; + }; + + it("emits no local health probe or llm_unavailable on a cloud route", async () => { + writeConfig({ + activeTextProvider: "cloudy", + activeEmbeddingProvider: "local-llama-embed", + providers: [ + { + id: "cloudy", + kind: "openai-compatible", + baseUrl: "https://cloud.invalid", + defaultChatModel: "cloudy-1", + apiKey: "sk-test", + }, + { id: "local-llama-embed", kind: "llama-server", url: "http://127.0.0.1:19092" }, + ], + toolTransport: "auto", + }); + + const { messages, shutdown } = await startSession(); + try { + expect( + messages.some((m) => m.kind === "response" && m.ok), + ).toBe(true); + expect(urls.filter((u) => u.includes(TEXT_PORT))).toEqual([]); + expect( + messages.filter( + (m) => m.kind === "event" && m.type === "llm_unavailable", + ), + ).toEqual([]); + } finally { + await shutdown(); + } + }); + + it("still probes on a local route (the control)", async () => { + writeConfig(undefined); + const { messages, shutdown } = await startSession(); + try { + expect(messages.some((m) => m.kind === "response" && m.ok)).toBe(true); + // Boot's `/health` + `/props`, then start_session's own `/health`. + expect( + urls.filter((u) => u.includes(TEXT_PORT) && u.includes("/health")) + .length, + ).toBe(2); + } finally { + await shutdown(); + } + }); +}); diff --git a/src/sidecar/main.ts b/src/sidecar/main.ts index 30d54dda..2bdb96ad 100644 --- a/src/sidecar/main.ts +++ b/src/sidecar/main.ts @@ -5,6 +5,8 @@ import { MessageRouter } from "./message-router.js"; import { StdioProtocol } from "./stdio-protocol.js"; import { getConfig } from "../config/index.js"; import { checkLlamaServer } from "../llm/llama-server-health.js"; +import { activeTextProviderIsLlamaServer } from "../llm/provider/registry/active-text-provider.js"; +import { resolveLlmConfig } from "../llm/provider/registry/provider-registry.js"; import { createAgentRuntime } from "../runtime/bootstrap.js"; import type { AgentRuntime } from "../runtime/bootstrap.js"; import type { AgentLoopEvent } from "../agent/agent-loop.js"; @@ -250,18 +252,25 @@ export async function bootstrapSidecar(): Promise<{ const runtime = await buildRuntime(workingDir); // Status probe for the desktop shell — one attempt; the retry // ladder only delayed the `llm_unavailable` event by 15.5 s. - const health = await checkLlamaServer({ retries: 0 }); - if (!health.reachable) { - const hint = - config.localModels.mode === "managed" - ? "run atomic-agent models start" - : "check localModels.url or ATOMIC_AGENT_LLAMA_URL"; - protocol.emitEvent("llm_unavailable", { - url: config.localModels.url, - error: health.error, - mode: config.localModels.mode, - hint, - }); + // + // Only when the local backend is the route (issue #112). Config is + // re-read rather than closed over: the shell can rewrite it + // between sessions, and `llm_unavailable` about a llama-server the + // session never talks to is a failure report for a healthy run. + if (activeTextProviderIsLlamaServer(resolveLlmConfig(getConfig()))) { + const health = await checkLlamaServer({ retries: 0 }); + if (!health.reachable) { + const hint = + config.localModels.mode === "managed" + ? "run atomic-agent models start" + : "check localModels.url or ATOMIC_AGENT_LLAMA_URL"; + protocol.emitEvent("llm_unavailable", { + url: config.localModels.url, + error: health.error, + mode: config.localModels.mode, + hint, + }); + } } const session = runtime.createSession({ ...(request.payload.metadata diff --git a/src/tui/llm-health/llm-health-poller.test.ts b/src/tui/llm-health/llm-health-poller.test.ts index 8ec18ff1..2c5af199 100644 --- a/src/tui/llm-health/llm-health-poller.test.ts +++ b/src/tui/llm-health/llm-health-poller.test.ts @@ -1,4 +1,15 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { + getUserConfigPath, + resetConfigCache, + USER_CONFIG_DEFAULTS, + writeUserConfigFileSync, +} from "../../config/index.js"; +import type { UserConfigFile } from "../../config/index.js"; import * as healthModule from "../../llm/llama-server-health.js"; import type { HealthResult } from "../../llm/llama-server-health.js"; @@ -435,4 +446,207 @@ describe("LlmHealthPoller", () => { capture.actions.map((a) => a.type).filter((t) => t.includes("rss")), ).toEqual([]); }); -}); \ No newline at end of file +}); +/** + * Issue #112 — the footer poller is the noisiest local prober in the + * process: `/health` every 3 s plus a one-shot `/props`, from the moment + * the TUI mounts, whatever the route. On a cloud session that is a + * request every three seconds against a server nobody is running, and a + * `down` badge plus an `n_ctx` reading about a backend that is not + * serving the turn. + */ +describe("LlmHealthPoller — gated on the active text provider", () => { + const stateDir = mkdtempSync(join(tmpdir(), "atomic-poller-gate-")); + let previousStateDir: string | undefined; + let spy: ReturnType; + + /** A cloud primary with a local EMBEDDING entry — the shape #112 is about. */ + const CLOUD_LLM: UserConfigFile["llm"] = { + activeTextProvider: "cloudy", + activeEmbeddingProvider: "local-llama-embed", + providers: [ + { + id: "cloudy", + kind: "openai-compatible", + baseUrl: "https://cloud.invalid", + defaultChatModel: "cloudy-1", + apiKey: "sk-test", + }, + { + id: "local-llama-embed", + kind: "llama-server", + url: "http://127.0.0.1:19092", + }, + ], + toolTransport: "auto", + }; + + const writeLlm = (llm: UserConfigFile["llm"]): void => { + writeUserConfigFileSync(getUserConfigPath(stateDir), { + ...USER_CONFIG_DEFAULTS, + ...(llm ? { llm } : {}), + }); + resetConfigCache(); + }; + + beforeEach(() => { + previousStateDir = process.env.ATOMIC_AGENT_STATE_DIR; + process.env.ATOMIC_AGENT_STATE_DIR = stateDir; + spy = vi.spyOn(healthModule, "checkLlamaServer"); + }); + + afterEach(() => { + vi.restoreAllMocks(); + if (previousStateDir === undefined) { + delete process.env.ATOMIC_AGENT_STATE_DIR; + } else { + process.env.ATOMIC_AGENT_STATE_DIR = previousStateDir; + } + rmSync(getUserConfigPath(stateDir), { force: true }); + resetConfigCache(); + }); + + it("probes nothing at TUI startup when a cloud provider is active", async () => { + writeLlm({ + activeTextProvider: "cloudy", + activeEmbeddingProvider: "local-llama-embed", + providers: [ + { + id: "cloudy", + kind: "openai-compatible", + baseUrl: "https://cloud.invalid", + defaultChatModel: "cloudy-1", + apiKey: "sk-test", + }, + { id: "local-llama-embed", kind: "llama-server", url: "http://127.0.0.1:19092" }, + ], + toolTransport: "auto", + }); + const props = vi.fn(stubProps); + const capture = makeCapture(); + const poller = new LlmHealthPoller(capture, "http://127.0.0.1:8080", 10, props); + poller.start(); + await sleep(40); + poller.stop(); + + // Exact counts: several tick windows elapsed, and every one of them + // must have cost zero requests. + expect(spy).toHaveBeenCalledTimes(0); + expect(props).toHaveBeenCalledTimes(0); + expect(capture.actions).toEqual([]); + }); + + it("probes on the same schedule as before when the route is local", async () => { + // The control: same poller, same timings, default (llama-server) + // config — one `/health` per tick and the one-shot `/props`. + writeLlm(undefined); + spy.mockResolvedValue({ + reachable: true, + status: 200, + error: null, + latencyMs: 1, + } satisfies HealthResult); + const props = vi.fn(stubProps); + const capture = makeCapture(); + const poller = new LlmHealthPoller(capture, "http://127.0.0.1:8080", 10_000, props); + poller.start(); + await sleep(30); + poller.stop(); + + expect(spy).toHaveBeenCalledTimes(1); + expect(props).toHaveBeenCalledTimes(1); + }); + + /** + * Issue #112 review, F4. `updateUrl` reset its bookkeeping and then + * emitted `llm_model_updated {model: null, contextWindow: null}` + * unconditionally — no request, but still a statement about the + * session's model, published on a route where this poller's backend is + * not the one serving. `/llama ` on a cloud session would blank + * the tray label the active provider had put there. + */ + it("emits nothing from updateUrl while a cloud provider is active", async () => { + writeLlm(CLOUD_LLM); + const props = vi.fn(stubProps); + const capture = makeCapture(); + const poller = new LlmHealthPoller( + capture, + "http://127.0.0.1:8080", + 10_000, + props, + ); + poller.start(); + poller.updateUrl("http://127.0.0.1:9090"); + await poller.refreshModelLabel(); + await sleep(20); + poller.stop(); + + expect(capture.actions).toEqual([]); + expect(spy).toHaveBeenCalledTimes(0); + expect(props).toHaveBeenCalledTimes(0); + }); + + it("still announces a URL change on a local route (control)", async () => { + writeLlm(undefined); + spy.mockResolvedValue({ + reachable: true, + status: 200, + error: null, + latencyMs: 1, + } satisfies HealthResult); + const capture = makeCapture(); + const poller = new LlmHealthPoller( + capture, + "http://127.0.0.1:8080", + 10_000, + stubProps, + ); + poller.updateUrl("http://127.0.0.1:9090"); + await sleep(20); + poller.stop(); + + expect( + capture.actions.filter((a) => a.type === "llm_model_updated"), + ).toContainEqual({ + type: "llm_model_updated", + model: null, + contextWindow: null, + }); + }); + + it("resumes within one tick after a hot switch back to a local provider", async () => { + writeLlm({ + activeTextProvider: "cloudy", + activeEmbeddingProvider: "local-llama-embed", + providers: [ + { + id: "cloudy", + kind: "openai-compatible", + baseUrl: "https://cloud.invalid", + defaultChatModel: "cloudy-1", + apiKey: "sk-test", + }, + { id: "local-llama-embed", kind: "llama-server", url: "http://127.0.0.1:19092" }, + ], + toolTransport: "auto", + }); + spy.mockResolvedValue({ + reachable: true, + status: 200, + error: null, + latencyMs: 1, + } satisfies HealthResult); + const capture = makeCapture(); + const poller = new LlmHealthPoller(capture, "http://127.0.0.1:8080", 10, stubProps); + poller.start(); + await sleep(40); + expect(spy).toHaveBeenCalledTimes(0); + + // The operator picks the local backend again. No start/stop call + // reaches the poller — it re-reads config on its own tick. + writeLlm(undefined); + await sleep(40); + poller.stop(); + expect(spy.mock.calls.length).toBeGreaterThan(0); + }); +}); diff --git a/src/tui/llm-health/llm-health-poller.ts b/src/tui/llm-health/llm-health-poller.ts index a90f364c..74d3e66d 100644 --- a/src/tui/llm-health/llm-health-poller.ts +++ b/src/tui/llm-health/llm-health-poller.ts @@ -1,5 +1,7 @@ import { getConfig } from "../../config/index.js"; import { checkLlamaServer } from "../../llm/llama-server-health.js"; +import { activeTextProviderIsLlamaServer } from "../../llm/provider/registry/active-text-provider.js"; +import { resolveLlmConfig } from "../../llm/provider/registry/provider-registry.js"; import { llamaEndpointUrl } from "../../llm/llama-endpoint-url.js"; import type { TuiAction } from "../tui-action.js"; @@ -89,6 +91,14 @@ export class LlmHealthPoller { this.url = nextUrl; this.hasSettledResult = false; this.modelFetchedForUrl = false; + // Same gate as `tick` / `refreshModelLabel` (issue #112). The reset + // above is bookkeeping and always runs, but the *emit* is a claim + // about the session's model — and on a cloud route it would clear + // the label and window for a backend the session is not talking to. + // The follow-up `tick()` already returns early on a cloud route, so + // only this emit was ungated; leaving it in made "on a cloud route + // the poller emits nothing at all" untrue for `/llama `. + if (!this.localTextActive()) return; this.emitter.emit({ type: "llm_model_updated", model: null, @@ -114,11 +124,35 @@ export class LlmHealthPoller { async refreshModelLabel(): Promise { this.modelFetchedForUrl = false; if (this.stopped) return; + // Same gate as `tick`: the Models tab can restart a managed daemon + // while the route is cloud, and the label this would fetch belongs + // to a backend that is not serving the session. + if (!this.localTextActive()) return; await this.fetchModelLabel(); } + /** + * Whether the local backend this poller watches is the route the + * operator is actually on. Read per tick from config rather than + * latched at construction: the active provider changes from the LLM + * tab, the composer switch and the provider wizard, and a poller that + * had to be told about each of them would miss the one that was added + * last (issue #112). + */ + private localTextActive(): boolean { + return activeTextProviderIsLlamaServer(resolveLlmConfig(getConfig())); + } + private async tick(): Promise { if (this.probing || this.stopped) return; + // Cloud route: probe nothing and emit nothing. The alternative — + // poll and hide — still costs a request every 3 s against a server + // nobody is running, and leaves a `down` reading in state that the + // context gauge (`select-context-usage`) would read as the active + // model's window. The interval keeps ticking so a switch back to a + // local provider resumes within one period, with no start/stop + // wiring on every switch path. + if (!this.localTextActive()) return; this.probing = true; if (!this.hasSettledResult) { this.emitter.emit({ diff --git a/src/tui/local-turn-gate.ts b/src/tui/local-turn-gate.ts index ded9348f..ce7f8d38 100644 --- a/src/tui/local-turn-gate.ts +++ b/src/tui/local-turn-gate.ts @@ -6,8 +6,8 @@ import { } from "../local-llm/index.js"; import { resolveFallbackChain } from "../llm/fallback/index.js"; import { + activeTextProviderIsLlamaServer, resolveLlmConfig, - type ResolvedLlmConfig, } from "../llm/provider/registry/index.js"; import { formatBytes } from "./hooks/use-transfer-rate.js"; import type { LocalModelsPullState } from "./local-models/local-models-panel-state.js"; @@ -53,20 +53,12 @@ export type LocalTurnGateDecision = | { kind: "block"; text: string }; /** - * KIND-based local detection, mirroring `selectComposerBackend`: any - * `llama-server` entry is the local route, because `LlamaServerProvider` - * accepts a custom id (`options.id`) — keying on the literal - * `local-llama` id would leave a renamed entry ungated. An active id - * that resolves to no entry reads as local too, matching the composer's - * no-active-row rule (and the no-`llm`-block default, which - * `resolveLlmConfig` synthesizes as a `llama-server` entry anyway). + * Moved beside `resolveLlmConfig` (issue #112): `src/runtime/` and + * `src/sidecar/` gate their local probes on the same predicate and must + * not import from `src/tui/`. Re-exported here so the gate's original + * callers keep working. */ -export function activeTextProviderIsLlamaServer( - llm: ResolvedLlmConfig, -): boolean { - const active = llm.providers.find((p) => p.id === llm.activeTextProvider); - return active === undefined || active.kind === "llama-server"; -} +export { activeTextProviderIsLlamaServer }; /** * Read the live facts from config + disk. Cheap on the happy path: the diff --git a/src/tui/select-context-usage.test.ts b/src/tui/select-context-usage.test.ts index 8b50671e..cc7acbe3 100644 --- a/src/tui/select-context-usage.test.ts +++ b/src/tui/select-context-usage.test.ts @@ -176,6 +176,52 @@ describe("resolving the model's context window", () => { expect(view?.contextWindow).toBe(32_768); }); + /** + * Issue #112. The poller's window is a llama-server `n_ctx`, and after + * a local→cloud switch the last local reading is still sitting in + * `llmHealth`: `agent-event-reducer` deliberately PRESERVES + * `contextWindow` when an `llm_model_updated` omits it (which is + * exactly the shape `notifyCatalogModel` emits), so the stale value is + * reachable by design, not only by a lost race. + * + * Without the `localActive &&` guard in `resolveWindow` this returns + * 4096 — a local server's slot size drawn as a cloud model's context + * gauge. The row's model is deliberately uncatalogued so the guard is + * the ONLY thing standing between the poller's number and the result. + */ + it("ignores the local poller's n_ctx while a cloud provider is the active route", () => { + const base = createInitialTuiState(fakeSession()); + const cloudRow = providerRow({ + kind: "openai-compatible", + chatModel: "vendor/never-heard-of-it", + }); + const view = selectContextUsage({ + ...base, + contextUsage: usage({ contextWindow: null }), + llmHealth: { ...base.llmHealth, contextWindow: 4096 }, + providersPanel: { ...base.providersPanel, rows: [cloudRow] }, + }); + expect(view?.contextWindow).toBeNull(); + expect(view?.percent).toBeNull(); + }); + + it("still uses the poller's n_ctx when the active row IS the local backend", () => { + // The control: the guard must not cost the local route its window. + const base = createInitialTuiState(fakeSession()); + const localRow = providerRow({ + id: "local-llama", + kind: "llama-server", + chatModel: null, + }); + const view = selectContextUsage({ + ...base, + contextUsage: usage({ contextWindow: null }), + llmHealth: { ...base.llmHealth, contextWindow: 4096 }, + providersPanel: { ...base.providersPanel, rows: [localRow] }, + }); + expect(view?.contextWindow).toBe(4096); + }); + it("falls back to the active provider's catalogue on a cloud turn", () => { const view = selectContextUsage(withRow(usage(), providerRow())); // `openai/gpt-5.5-2026-04-23`, from the bundled aimlapi catalogue. diff --git a/src/tui/select-context-usage.ts b/src/tui/select-context-usage.ts index ef5a8e5d..20bb4dec 100644 --- a/src/tui/select-context-usage.ts +++ b/src/tui/select-context-usage.ts @@ -65,7 +65,11 @@ const CONVERSATION_CAP_FLOOR = 512; * 2. The health poller's reading of the same endpoint. Not redundant: * `localModels.mode: "managed"` *defers* the boot probe, so a local * turn can build its prompt with no window while the poller already - * has one. + * has one. Only consulted while a local backend is the active route + * (issue #112) — after a local→cloud switch the poller's last local + * reading is still in state, and drawing the cloud model's gauge + * against a llama-server `n_ctx` is a fabrication with a number + * attached. * 3. The active cloud provider's catalogue. Read here, at render time, * rather than resolved once into a `ProviderRow`: the live catalogue * arrives from an async fetch at start-up, so anything baked into a @@ -80,9 +84,10 @@ const CONVERSATION_CAP_FLOOR = 512; function resolveWindow(state: TuiState): number | null { const fromPrompt = state.contextUsage.contextWindow; if (fromPrompt !== null && fromPrompt > 0) return fromPrompt; - const fromPoller = state.llmHealth.contextWindow; - if (fromPoller !== null && fromPoller > 0) return fromPoller; const active = state.providersPanel.rows.find((row) => row.isActiveText); + const localActive = active === undefined || active.kind === "llama-server"; + const fromPoller = state.llmHealth.contextWindow; + if (localActive && fromPoller !== null && fromPoller > 0) return fromPoller; if (!active?.chatModel) return null; const lookup = catalogEntryLookupForKind(active.kind); const entry = lookup?.(active.chatModel);