From b20d847a24a963a614c55b75af1b81b06db16cec Mon Sep 17 00:00:00 2001 From: DBarr3 <143002219+DBarr3@users.noreply.github.com> Date: Thu, 20 Aug 2026 06:01:27 -0400 Subject: [PATCH] fix(ollama): accept Ollama's own OLLAMA_HOST format, and keep the timeout armed through the body read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects on the offline on-ramp, both on the path a signed-out user hits first. 1. Ollama's convention for OLLAMA_HOST is a scheme-less host:port — that is what `ollama serve` prints and binds. The client concatenated the value raw, so OLLAMA_HOST=127.0.0.1:11434 produced fetch("127.0.0.1:11434/v1/chat/completions") -> "Failed to parse URL", which the unreachable handler then reported as "Cannot reach Ollama ... or set OLLAMA_HOST" — the exact thing the user had just done. Every local turn failed, and `smoke` plus the handoff demo were equally unnormalized, so they echoed the same broken value instead of diagnosing it. Adds one exported pure function, normalizeOllamaHost(): trims, adds http:// when there is no scheme, maps the 0.0.0.0 bind address to a connectable 127.0.0.1, strips trailing slashes, and rejects anything unusable with a message naming the bad value. All three call sites route through it, and smokeMain now reports a bad host as its own FAIL rather than as "Ollama is down". Accepted forms are documented in COMMANDS.md. 2. The AbortController timer was cleared in the finally attached to the fetch, i.e. as soon as response headers arrived. Because stream:false puts the whole completion in the body, both body reads then ran with no timer and no signal: a server that answered 200 and stalled mid-body — a loaded local GPU, a dropped SSH tunnel — hung the turn forever instead of failing at the timeout. Request and parse now share one try/finally, and the AbortError branch is reachable from the body-read path, so a stalled body surfaces as the same "timed out after Ns" message the connect path already produced. Co-Authored-By: Claude Opus 5 (1M context) --- COMMANDS.md | 20 +++++- scripts/handoff-demo.ts | 5 +- src/core/ollama.ts | 155 +++++++++++++++++++++++++++++----------- src/core/smoke.ts | 20 ++++-- test/ollama.test.ts | 87 ++++++++++++++++++++++ 5 files changed, 240 insertions(+), 47 deletions(-) diff --git a/COMMANDS.md b/COMMANDS.md index c1cf306..bf83bae 100644 --- a/COMMANDS.md +++ b/COMMANDS.md @@ -446,13 +446,31 @@ Requires an active orchestrator — switch with `/agent neo` or `/agent kronus` | `AETHER_LOG_DIR` | `~/.aether-agent/logs` | Where session logs (and therefore `aether resume`) live. | | `AETHER_BACKEND` | `auto` | `local` \| `cloud` \| `auto` — overrides the config `backend`. | | `AETHER_LOCAL_BRAIN` | *(unset)* | `python` runs the separately-installed Unlimited-Context brain instead of the built-in Ollama one. | -| `OLLAMA_HOST` | `http://localhost:11434` | Where the offline brain looks for Ollama. | +| `OLLAMA_HOST` | `http://localhost:11434` | Where the offline brain looks for Ollama. Accepts Ollama's own scheme-less form (`127.0.0.1:11434`) as well as a full URL — see below. | | `AETHER_STREAM_TIMEOUT_MS` | `120000` | Stream open/idle timeout (ms). `0` disables it. | | `AETHER_NO_ANIM` | *(unset)* | `1` disables all animated status lines and the thinking pulse. | | `NO_COLOR` | *(unset)* | Any value disables ANSI colors (https://no-color.org). | See [`.env.example`](.env.example). +### `OLLAMA_HOST` accepted forms + +`ollama serve` prints and binds a **scheme-less** `host:port`, and that is what most +people paste into `OLLAMA_HOST`. Every accepted form below is normalized to a full +base URL before any request is built: + +| You set | Aether uses | Note | +|---|---|---| +| *(unset or empty)* | `http://localhost:11434` | The default. | +| `127.0.0.1:11434` | `http://127.0.0.1:11434` | Scheme-less — `http://` is added. | +| `localhost:11434` | `http://localhost:11434` | Scheme-less. | +| `0.0.0.0:11434` | `http://127.0.0.1:11434` | `0.0.0.0` is a *bind* address, not a *connect* address. | +| `http://localhost:11434/` | `http://localhost:11434` | Trailing slashes are stripped. | +| `https://ollama.example.com` | `https://ollama.example.com` | A remote/proxied Ollama. | + +Anything that still will not parse as an `http`/`https` URL is rejected up front with +an error naming the bad value, instead of failing later as "cannot reach Ollama". + --- ## Exit codes diff --git a/scripts/handoff-demo.ts b/scripts/handoff-demo.ts index a74ac10..c1038ad 100644 --- a/scripts/handoff-demo.ts +++ b/scripts/handoff-demo.ts @@ -24,6 +24,7 @@ import { cpSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } f import { tmpdir } from "node:os"; import { dirname, join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; +import { normalizeOllamaHost } from "../src/core/ollama.js"; const here = dirname(fileURLToPath(import.meta.url)); const repoRoot = resolve(here, "..", ".."); @@ -240,7 +241,9 @@ async function main(): Promise { const state: StubState = { turns: new Map(), firstPrompt: new Map(), failures: [] }; let server: Server | null = null; - let ollamaHost = process.env["OLLAMA_HOST"] ?? "http://localhost:11434"; + // Normalize so Ollama's own scheme-less OLLAMA_HOST convention ("127.0.0.1:11434") + // is a usable base URL for the child CLI instead of an unparseable string. + let ollamaHost = normalizeOllamaHost(process.env["OLLAMA_HOST"]); if (!REAL) { const stub = await startStub(state); server = stub.server; diff --git a/src/core/ollama.ts b/src/core/ollama.ts index 384e835..0e0954f 100644 --- a/src/core/ollama.ts +++ b/src/core/ollama.ts @@ -89,6 +89,60 @@ const PROFILES: readonly Profile[] = [ // Safe default for an unknown tag: deterministic, like Qwen coding. const DEFAULT_SAMPLING: Sampling = { temperature: 0.2, top_p: 0.9, top_k: 40 }; +// --- host normalization ------------------------------------------------------ +// Ollama's own convention for OLLAMA_HOST is a scheme-LESS host:port — that is +// exactly what `ollama serve` prints and binds ("127.0.0.1:11434", "0.0.0.0:11434"). +// Concatenating "/v1/chat/completions" onto that produces a string fetch() cannot +// parse, so every local turn died with "Failed to parse URL" and an error hint +// telling the user to set the very variable they had just set. Normalize once, +// here, and route every call site through it. + +/** Matches an explicit `scheme://` prefix (http, https, or anything else). */ +const SCHEME_RE = /^[a-z][a-z0-9+.-]*:\/\//i; + +/** + * Canonicalize an Ollama base URL. Accepts every form users actually type: + * `127.0.0.1:11434` · `localhost:11434` · `0.0.0.0:11434` · + * `http://localhost:11434` · `https://ollama.example.com` · + * `http://localhost:11434/` (trailing slashes stripped) · `` (→ default). + * + * A bare `0.0.0.0` is a *bind* address, not a *connect* address, so it maps to + * `127.0.0.1` for the client side. Throws an Error naming the offending value + * when the result still will not parse. + */ +export function normalizeOllamaHost(raw?: string | null): string { + const trimmed = (raw ?? "").trim(); + if (trimmed === "") return DEFAULT_OLLAMA_HOST; + + const candidate = SCHEME_RE.test(trimmed) ? trimmed : `http://${trimmed}`; + let url: URL; + try { + url = new URL(candidate); + } catch { + throw new Error( + `Invalid Ollama host ${JSON.stringify(trimmed)}. Use 'host:port' (e.g. 127.0.0.1:11434) ` + + `or a full URL (e.g. http://localhost:11434).`, + ); + } + if (url.protocol !== "http:" && url.protocol !== "https:") { + throw new Error( + `Invalid Ollama host ${JSON.stringify(trimmed)}: unsupported scheme '${url.protocol.replace(/:$/, "")}'. ` + + `Ollama speaks http (or https behind a proxy).`, + ); + } + if (url.hostname === "") { + throw new Error( + `Invalid Ollama host ${JSON.stringify(trimmed)}: no hostname. Use 'host:port' (e.g. 127.0.0.1:11434).`, + ); + } + // 0.0.0.0 / :: mean "bind every interface"; you cannot connect to them portably. + if (url.hostname === "0.0.0.0") url.hostname = "127.0.0.1"; + else if (url.hostname === "[::]") url.hostname = "[::1]"; + + const path = url.pathname.replace(/\/+$/, ""); + return `${url.protocol}//${url.host}${path}`; +} + /** The sampling knobs for a model tag (first substring match; safe default). */ export function samplingFor(model: string): Sampling { const low = (model || "").toLowerCase(); @@ -236,7 +290,7 @@ export async function ollamaChat( messages: readonly ChatMessage[], opts: ChatOptions = {}, ): Promise { - const host = (opts.host || process.env["OLLAMA_HOST"] || DEFAULT_OLLAMA_HOST).replace(/\/+$/, ""); + const host = normalizeOllamaHost(opts.host || process.env["OLLAMA_HOST"] || DEFAULT_OLLAMA_HOST); const model = opts.model || DEFAULT_OLLAMA_MODEL; const sampling = samplingFor(model); const temperature = opts.temperature ?? sampling.temperature; @@ -255,51 +309,72 @@ export async function ollamaChat( const controller = new AbortController(); const timeoutMs = opts.timeoutMs ?? DEFAULT_TIMEOUT_MS; const timer = setTimeout(() => controller.abort(), timeoutMs); - let res: Response; - try { - res = await fetch(url, { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify(body), - signal: controller.signal, - }); - } catch (err) { + const timedOut = (): Error => + new Error(`Ollama request timed out after ${Math.round(timeoutMs / 1000)}s (${url}).`); + const isAbort = (err: unknown): boolean => { const msg = err instanceof Error ? err.message : String(err); - if ((err instanceof Error && err.name === "AbortError") || msg.includes("aborted")) { - throw new Error(`Ollama request timed out after ${Math.round(timeoutMs / 1000)}s (${url}).`); - } - throw new Error( - `Cannot reach Ollama at ${host}. Is it running? Start it with 'ollama serve' ` + - `(default port 11434), or set OLLAMA_HOST. Underlying error: ${msg}`, - ); - } finally { - clearTimeout(timer); - } + return (err instanceof Error && err.name === "AbortError") || msg.includes("aborted"); + }; - if (!res.ok) { - const text = await res.text().catch(() => ""); - if (res.status === 404) { + // The timer must stay armed until the BODY is read, not just until headers + // arrive: stream:false means the whole completion is in the body, so a server + // that answers 200 and then stalls mid-body would otherwise hang forever. + try { + let res: Response; + try { + res = await fetch(url, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + signal: controller.signal, + }); + } catch (err) { + if (isAbort(err)) throw timedOut(); + const msg = err instanceof Error ? err.message : String(err); throw new Error( - `Ollama model '${model}' not found (404). Pull it first: 'ollama pull ${model}'. ` + - `Server said: ${text.slice(0, 300)}`, + `Cannot reach Ollama at ${host}. Is it running? Start it with 'ollama serve' ` + + `(default port 11434), or set OLLAMA_HOST. Underlying error: ${msg}`, ); } - throw new Error(`Ollama error ${res.status} at ${url}: ${text.slice(0, 300)}`); - } - const json = (await res.json().catch(() => ({}))) as Record; - const choices = json["choices"]; - const first = Array.isArray(choices) && isRecord(choices[0]) ? (choices[0] as Record) : {}; - const message = isRecord(first["message"]) ? (first["message"] as Record) : {}; - const content = typeof message["content"] === "string" ? message["content"] : ""; + if (!res.ok) { + let text = ""; + try { + text = await res.text(); + } catch (err) { + if (isAbort(err)) throw timedOut(); + } + if (res.status === 404) { + throw new Error( + `Ollama model '${model}' not found (404). Pull it first: 'ollama pull ${model}'. ` + + `Server said: ${text.slice(0, 300)}`, + ); + } + throw new Error(`Ollama error ${res.status} at ${url}: ${text.slice(0, 300)}`); + } + + let json: Record = {}; + try { + json = (await res.json()) as Record; + } catch (err) { + if (isAbort(err)) throw timedOut(); + json = {}; + } + const choices = json["choices"]; + const first = Array.isArray(choices) && isRecord(choices[0]) ? (choices[0] as Record) : {}; + const message = isRecord(first["message"]) ? (first["message"] as Record) : {}; + const content = typeof message["content"] === "string" ? message["content"] : ""; - const structured = normalizeToolCalls(message["tool_calls"]); - const toolCalls = structured.length > 0 ? structured : extractToolCalls(content); + const structured = normalizeToolCalls(message["tool_calls"]); + const toolCalls = structured.length > 0 ? structured : extractToolCalls(content); - const reply: ChatReply = { - role: "assistant", - content, - ...(toolCalls.length > 0 ? { tool_calls: toolCalls } : {}), - }; - return reply; + const reply: ChatReply = { + role: "assistant", + content, + ...(toolCalls.length > 0 ? { tool_calls: toolCalls } : {}), + }; + return reply; + } finally { + clearTimeout(timer); + } } diff --git a/src/core/smoke.ts b/src/core/smoke.ts index 3a06163..3739315 100644 --- a/src/core/smoke.ts +++ b/src/core/smoke.ts @@ -12,7 +12,7 @@ import { isSafeUrl, webFetch, webSearch } from "./web.js"; import { OllamaBrain } from "./brain_ollama.js"; import { CloudBrain } from "./brain_cloud.js"; import { ToolExecutor, type ToolResult } from "./tool_executor.js"; -import { DEFAULT_OLLAMA_HOST, DEFAULT_OLLAMA_MODEL } from "./ollama.js"; +import { DEFAULT_OLLAMA_HOST, DEFAULT_OLLAMA_MODEL, normalizeOllamaHost } from "./ollama.js"; import { defaultTokenStore, type TokenStore } from "./auth.js"; import { loadConfig } from "./config.js"; import { ApiClient } from "./transport.js"; @@ -40,7 +40,7 @@ export async function ollamaUp(host: string, timeoutMs = 4000): Promise const ctl = new AbortController(); const timer = setTimeout(() => ctl.abort(), timeoutMs); try { - const res = await fetch(host.replace(/\/$/, "") + "/api/tags", { signal: ctl.signal }); + const res = await fetch(normalizeOllamaHost(host) + "/api/tags", { signal: ctl.signal }); return res.ok; } catch { return false; @@ -210,7 +210,16 @@ export async function smokeMain(): Promise { const cfg = loadConfig(); const baseUrl = cfg.baseUrl || "https://api.aethersystems.net/cloud"; const model = cfg.defaultModel || DEFAULT_OLLAMA_MODEL; - const host = process.env["OLLAMA_HOST"] || DEFAULT_OLLAMA_HOST; + // Normalize here so a scheme-less OLLAMA_HOST (Ollama's own convention) is + // diagnosed as a bad value instead of being reported as "Ollama is down". + const rawHost = process.env["OLLAMA_HOST"] || DEFAULT_OLLAMA_HOST; + let host = DEFAULT_OLLAMA_HOST; + let hostError = ""; + try { + host = normalizeOllamaHost(rawHost); + } catch (err) { + hostError = err instanceof Error ? err.message : String(err); + } const safe = async (name: string, fn: () => Promise): Promise => { try { @@ -219,11 +228,12 @@ export async function smokeMain(): Promise { return { name, status: "FAIL", detail: `crashed: ${err instanceof Error ? err.message : String(err)}` }; } }; + const badHost = (name: string): Check => ({ name, status: "FAIL", detail: hostError }); const results: Check[] = [ checkSsrf(), - await safe("ollama", () => checkOllama(host)), - await safe("local turn", () => checkLocalTurn(host, model)), + hostError ? badHost("ollama") : await safe("ollama", () => checkOllama(host)), + hostError ? badHost("local turn") : await safe("local turn", () => checkLocalTurn(host, model)), await safe("web_search", () => checkWebSearch()), await safe("web_fetch", () => checkWebFetch()), await safe("auth", () => checkAuth(baseUrl)), diff --git a/test/ollama.test.ts b/test/ollama.test.ts index 00083a7..e9154d3 100644 --- a/test/ollama.test.ts +++ b/test/ollama.test.ts @@ -7,6 +7,8 @@ import { ollamaChat, extractToolCalls, samplingFor, + normalizeOllamaHost, + DEFAULT_OLLAMA_HOST, DEFAULT_OLLAMA_MODEL, type ChatMessage, type ToolSchema, @@ -213,3 +215,88 @@ test("ollamaChat throws a model-not-pulled hint on a 404", async () => { test("DEFAULT_OLLAMA_MODEL is the universal small coder default", () => { assert.equal(DEFAULT_OLLAMA_MODEL, "qwen2.5-coder:7b"); }); + +// --- normalizeOllamaHost: Ollama's own scheme-less OLLAMA_HOST convention ---- +test("normalizeOllamaHost accepts the scheme-less host:port ollama serve prints", () => { + assert.equal(normalizeOllamaHost("127.0.0.1:11434"), "http://127.0.0.1:11434"); + assert.equal(normalizeOllamaHost("localhost:11434"), "http://localhost:11434"); +}); + +test("normalizeOllamaHost maps the 0.0.0.0 bind address to a connectable one", () => { + assert.equal(normalizeOllamaHost("0.0.0.0:11434"), "http://127.0.0.1:11434"); + assert.equal(normalizeOllamaHost("http://0.0.0.0:11434"), "http://127.0.0.1:11434"); +}); + +test("normalizeOllamaHost keeps explicit schemes and strips trailing slashes", () => { + assert.equal(normalizeOllamaHost("http://localhost:11434/"), "http://localhost:11434"); + assert.equal(normalizeOllamaHost("http://localhost:11434///"), "http://localhost:11434"); + assert.equal(normalizeOllamaHost("https://ollama.example.com"), "https://ollama.example.com"); + assert.equal(normalizeOllamaHost("https://ollama.example.com/proxy/"), "https://ollama.example.com/proxy"); +}); + +test("normalizeOllamaHost falls back to the default for empty/whitespace/undefined", () => { + assert.equal(normalizeOllamaHost(undefined), DEFAULT_OLLAMA_HOST); + assert.equal(normalizeOllamaHost(""), DEFAULT_OLLAMA_HOST); + assert.equal(normalizeOllamaHost(" "), DEFAULT_OLLAMA_HOST); + assert.equal(normalizeOllamaHost(" 127.0.0.1:11434 "), "http://127.0.0.1:11434"); +}); + +test("normalizeOllamaHost rejects unusable values and names the bad value", () => { + assert.throws(() => normalizeOllamaHost("ftp://localhost:11434"), /ftp/); + assert.throws(() => normalizeOllamaHost("http://"), /"http:\/\/"|no hostname|Invalid Ollama host/); + assert.throws(() => normalizeOllamaHost(":::"), /Invalid Ollama host/); +}); + +// The bug this fixes: a scheme-less host was concatenated raw, so fetch() got +// "127.0.0.1:11434/v1/chat/completions" -> "Failed to parse URL", which the +// unreachable-handler then reported as "set OLLAMA_HOST" — the thing the user did. +test("ollamaChat completes a turn against a scheme-less host:port", async () => { + const { base, server } = await stub((_req, _body, res) => { + res.end(JSON.stringify({ choices: [{ message: { role: "assistant", content: "pong" } }] })); + }); + try { + const schemeless = base.replace(/^https?:\/\//, ""); + assert.ok(!schemeless.includes("://"), "test really is exercising the scheme-less form"); + const reply = await ollamaChat([{ role: "user", content: "ping" }], { host: schemeless }); + assert.equal(reply.content, "pong"); + } finally { + server.close(); + } +}); + +test("ollamaChat surfaces a bad OLLAMA_HOST as a host error, not 'cannot reach Ollama'", async () => { + await assert.rejects( + () => ollamaChat([{ role: "user", content: "x" }], { host: "ftp://localhost:11434" }), + /Invalid Ollama host/, + ); +}); + +// --- the timeout must cover the BODY, not just the headers ------------------ +// stream:false means the whole completion arrives in the body. A server that +// answers 200 and then stalls mid-body used to hang forever, because the timer +// was cleared as soon as headers arrived. +test("ollamaChat times out when the server stalls mid-body after 200 headers", async () => { + const server = createServer((req, res) => { + req.on("data", () => {}); + req.on("end", () => { + res.writeHead(200, { "content-type": "application/json" }); + res.write('{"choices":[{"message":{"role":"assistant","content":"'); + // ...and then never finishes. No res.end(). + }); + }); + await new Promise((r) => server.listen(0, "127.0.0.1", () => r())); + const port = (server.address() as AddressInfo).port; + try { + await assert.rejects( + () => + ollamaChat([{ role: "user", content: "x" }], { + host: `http://127.0.0.1:${port}`, + timeoutMs: 400, + }), + /timed out after/, + ); + } finally { + server.closeAllConnections(); + server.close(); + } +});