diff options
| author | Agando <roduccomm@gmail.com> | 2026-10-27 14:41:20 +0100 |
|---|---|---|
| committer | Agando <roduccomm@gmail.com> | 2026-10-27 14:41:20 +0100 |
| commit | 0ccd2cbc4cc15e0a362b08d76a8bd814abfb9ed9 (patch) | |
| tree | 50020b322d79651c4e70662550b793905708a9ee /modules/core | |
| parent | ea3acceac673b3cb6d84d38f968f057082d8ccb8 (diff) | |
fix(newos4): name the real Ollama failure, bound auto-connect, fix the review findings
Second pass over v4.6.105, driven by an independent review that reproduced each item.
- ollama-manager.js: chat() now reports the captured cause ("Ollama error: Failed to fetch",
"…: HTTP 500") instead of the generic "No provider responded"; _lastChatError is reset per
call so an old reason is never replayed. chatWithTools carries the reason in its error string.
- ollama-manager.js: _ensureConnected() bounds connect() with a deadline (connectTimeoutMs,
default 10 s). connect() uses a bare fetch(), so a blackholed endpoint previously left the
shared in-flight promise pending forever and hung every later message — the old code failed
fast. The attempt is now try/finally, so a throwing terminal.log() can no longer poison
_connecting and suppress the retry on the next message.
- ollama-manager.js: an automatic connect no longer silently rewrites the user's saved model;
the previous choice is restored in memory and in localStorage, and the user is told.
- ollama-manager.js: the DeepSeek empty-content branch returned top-level tool_calls with
content '', so the caller pushed an assistant message with no tool_calls and then a
role:'tool' reply — an orphan tool message, which is the HTTP 400 that branch exists to
prevent. tool_calls now live inside the message as well.
- module-aiassist.js: a no-argument list_files returned the ROOT instead of the cwd (readDir('/')
instead of ls()); it now means the current directory again. Error text keeps the path as the
caller wrote it, and write_file distinguishes a read-only virtual path (/sys/…) from a missing
parent instead of blaming the parent.
Tests: test-ollama-error-text.js (new, 14 asserts, RED 9/5 before the edit), test-ai-fs-tools.js
7 → 16 asserts (write onto a directory, /sys write, list a file, '/', no-arg cwd, relative path),
test-ollama-autoconnect.js 11 → 19 (blackholed endpoint settles + retries, saved model survives;
the old "connecting hint" assertion is now falsifiable — the reviewer showed it also matched
connect()'s pre-existing line). Suite: 10/10 files green. Live on v4.6.106: healthy auto-connect
42 ms with the hint; blackholed endpoint settles in 1242 ms with connectTimeoutMs=1200, prints
"timed out" and "unreachable", _connecting cleared.
Diffstat (limited to 'modules/core')
| -rw-r--r-- | modules/core/core-bundle.js | 91 | ||||
| -rw-r--r-- | modules/core/module-aiassist.js | 12 | ||||
| -rw-r--r-- | modules/core/module-loader.js | 2 | ||||
| -rw-r--r-- | modules/core/ollama-manager.js | 79 |
4 files changed, 141 insertions, 43 deletions
diff --git a/modules/core/core-bundle.js b/modules/core/core-bundle.js index 0d3a783..a057332 100644 --- a/modules/core/core-bundle.js +++ b/modules/core/core-bundle.js @@ -1617,14 +1617,37 @@ class OllamaManager { try { await this._connecting; } catch (e) { /* fall through to the state check */ } return this.connected; } - this.terminal.print('Connecting to Ollama…', 'info'); - this._connecting = this.connect(); - try { await this._connecting; } catch (e) { - this.terminal.log('WARN', 'Ollama', 'Auto-connect failed: ' + e.message); + // An automatic connect must not silently rewrite the model the user picked. + const savedModel = (typeof localStorage !== 'undefined') ? localStorage.getItem('webx11_ollama_model') : null; + this.terminal.print('Ollama: connecting on first use…', 'info'); + let timedOut = false; + try { + this._connecting = this.connect(); + // connect() uses a bare fetch(), so a blackholed endpoint would hang this promise + // forever and — because the promise is shared — every later message with it. + const deadline = this.connectTimeoutMs || 10000; + const outcome = await Promise.race([ + this._connecting.then(() => 'done', e => { throw e; }), + new Promise(resolve => setTimeout(() => resolve('timeout'), deadline)), + ]); + timedOut = outcome === 'timeout' && !this.connected; + } catch (e) { + // A throwing terminal.log() must not leave _connecting poisoned: log defensively. + try { this.terminal.log('WARN', 'Ollama', 'Auto-connect failed: ' + (e && e.message ? e.message : e)); } catch (_) {} + } finally { + this._connecting = null; + } + if (this.connected && savedModel && !(this.models || []).some(m => m.name === savedModel)) { + this.currentModel = savedModel; + if (this.currentProvider) this.currentProvider.model = savedModel; + // connect() may already have overwritten the stored preference; put it back. + try { localStorage.setItem('webx11_ollama_model', savedModel); } catch (_) {} + this.terminal.print('Keeping your saved model "' + savedModel + '" — this endpoint does not list it.', 'warning'); } - this._connecting = null; if (!this.connected) { - this.terminal.print('Auto-connect failed — check the endpoint in the Ollama tab (or run ollamaconnect).', 'warning'); + this.terminal.print(timedOut + ? 'Auto-connect timed out after ' + ((this.connectTimeoutMs || 10000) / 1000) + 's — the endpoint is not answering. Check the Ollama tab or run ollamaconnect.' + : 'Auto-connect failed — check the endpoint in the Ollama tab (or run ollamaconnect).', 'warning'); } return this.connected; } @@ -1632,6 +1655,8 @@ class OllamaManager { // ─── Chat (non-streaming) ──────────────────────────────────────────────── async chat(prompt, model = null) { + // Per-call state: never let a reason captured by an earlier failure be replayed here. + this._lastChatError = null; if (!this.connected && !(await this._ensureConnected())) { this.terminal.print('Ollama not connected: the endpoint is unreachable. Check the Ollama tab or run ollamaconnect.', 'error'); return null; @@ -1661,7 +1686,11 @@ class OllamaManager { } if (lastError === null) { - this.terminal.print(`Ollama error: No provider responded`, 'error'); + // Name the real cause when one was captured; the generic wording is only a fallback. + const reason = this._lastChatError; + this.terminal.print( + reason ? `Ollama error: ${reason}` : 'Ollama error: No provider responded', + 'error'); } return null; } @@ -1787,6 +1816,9 @@ class OllamaManager { return fullText || null; } } catch (error) { + // Remember why this provider failed so chat() can name the real cause + // instead of falling back to the generic "No provider responded". + this._lastChatError = error.message || String(error); if (fullText && fullText.length > 0) { this.terminal.printStreamingResponse(`\n\n[Stream error: ${error.message}]`, true); return fullText; @@ -1974,6 +2006,8 @@ class OllamaManager { } async chatWithTools(messages, tools, model = null) { + // Per-call state: never let a reason captured by an earlier failure be replayed here. + this._lastChatError = null; if (!this.connected && !(await this._ensureConnected())) { this.terminal.print('Ollama not connected: the endpoint is unreachable. Check the Ollama tab or run ollamaconnect.', 'error'); return { message: null, tool_calls: null, error: 'Not connected' }; @@ -2004,7 +2038,12 @@ class OllamaManager { } } - return lastError || { message: null, tool_calls: null, error: 'All providers failed' }; + // Name the real cause when one was captured; the generic wording is only a fallback. + const reason = this._lastChatError; + return lastError || { + message: null, tool_calls: null, + error: reason ? `All providers failed: ${reason}` : 'All providers failed' + }; } async _chatWithToolsWithProvider(provider, messages, tools, model) { @@ -2090,16 +2129,21 @@ class OllamaManager { // DeepSeek: handle null content with tool calls — send empty string if (isDeepSeek && content === '' && toolCalls && toolCalls.length > 0) { + const normalised = toolCalls.map(tc => ({ + id: tc.id, + type: tc.type || 'function', + function: { + name: tc.function?.name || '', + arguments: tc.function?.arguments || '{}' + } + })); return { - message: { role: 'assistant', content: '' }, - tool_calls: toolCalls.map(tc => ({ - id: tc.id, - type: tc.type || 'function', - function: { - name: tc.function?.name || '', - arguments: tc.function?.arguments || '{}' - } - })), + // tool_calls must live INSIDE the assistant message: the caller pushes + // result.message and then a role:'tool' reply, and a tool message whose + // tool_call_id has no matching assistant call is the HTTP 400 this + // branch exists to avoid. + message: { role: 'assistant', content: '', tool_calls: normalised }, + tool_calls: normalised, error: null }; } @@ -2183,6 +2227,7 @@ class OllamaManager { return await this.chatWithToolsFallback(messages, tools, useModel); } this.terminal.print(`${provider.name} /api/chat error: ${error.message}`, 'error'); + this._lastChatError = error.message || String(error); return { message: null, tool_calls: null, error: error.message }; } finally { this._abortController = null; @@ -10849,10 +10894,11 @@ buildSystemPrompt() { // MAINT-011: readDir() resolves the virtual mounts and returns null for a // path that does not exist. The old hand-rolled walk over root.content could // not see /sys, so a directory the UI shows answered "Directory not found". - const entries = fsapi.readDir(p || '/'); + // No argument means "the current directory", which ls() already does. + const entries = p ? fsapi.readDir(p) : fsapi.ls(); fsapi.currentPath = saved; if (entries === null || entries === undefined) { - return JSON.stringify({ error: `Directory not found: ${p || '/'}` }); + return JSON.stringify({ error: `Directory not found: ${originalPath || p}` }); } return JSON.stringify(entries.map(f => ({ name: f.name, type: f.type, size: f.size }))); } @@ -10900,9 +10946,12 @@ buildSystemPrompt() { const wrote = this.terminal.fs.writeFile(path, content); this.terminal.fs.currentPath = savedW; if (!wrote) { - return `Write FAILED: ${path} — parent directory missing or target is a directory; nothing was written`; + const why = (typeof this.terminal.fs._isVirtualPath === 'function' && this.terminal.fs._isVirtualPath(wp)) + ? 'read-only virtual path' + : 'parent directory missing or target is a directory'; + return `Write FAILED: ${wp} — ${why}; nothing was written`; } - return `File written: ${path} (${content.length} chars)`; + return `File written: ${wp} (${content.length} chars)`; } case 'make_directory': { const mp = a.path || ''; diff --git a/modules/core/module-aiassist.js b/modules/core/module-aiassist.js index 303f9c2..ac8d297 100644 --- a/modules/core/module-aiassist.js +++ b/modules/core/module-aiassist.js @@ -2379,10 +2379,11 @@ buildSystemPrompt() { // MAINT-011: readDir() resolves the virtual mounts and returns null for a // path that does not exist. The old hand-rolled walk over root.content could // not see /sys, so a directory the UI shows answered "Directory not found". - const entries = fsapi.readDir(p || '/'); + // No argument means "the current directory", which ls() already does. + const entries = p ? fsapi.readDir(p) : fsapi.ls(); fsapi.currentPath = saved; if (entries === null || entries === undefined) { - return JSON.stringify({ error: `Directory not found: ${p || '/'}` }); + return JSON.stringify({ error: `Directory not found: ${originalPath || p}` }); } return JSON.stringify(entries.map(f => ({ name: f.name, type: f.type, size: f.size }))); } @@ -2430,9 +2431,12 @@ buildSystemPrompt() { const wrote = this.terminal.fs.writeFile(path, content); this.terminal.fs.currentPath = savedW; if (!wrote) { - return `Write FAILED: ${path} — parent directory missing or target is a directory; nothing was written`; + const why = (typeof this.terminal.fs._isVirtualPath === 'function' && this.terminal.fs._isVirtualPath(wp)) + ? 'read-only virtual path' + : 'parent directory missing or target is a directory'; + return `Write FAILED: ${wp} — ${why}; nothing was written`; } - return `File written: ${path} (${content.length} chars)`; + return `File written: ${wp} (${content.length} chars)`; } case 'make_directory': { const mp = a.path || ''; diff --git a/modules/core/module-loader.js b/modules/core/module-loader.js index 1f790e3..365fa55 100644 --- a/modules/core/module-loader.js +++ b/modules/core/module-loader.js @@ -4,7 +4,7 @@ // Core module bundle loaded synchronously in main.html. // This loader only handles version cache-busting, splash, terminal init, and optional ext modules. - const LATEST_VER = 'v4.6.105'; + const LATEST_VER = 'v4.6.106'; const CACHE = '?v=' + LATEST_VER; const stored = localStorage.getItem('webx11_cache_ver'); if (stored !== LATEST_VER) { diff --git a/modules/core/ollama-manager.js b/modules/core/ollama-manager.js index 3d157ba..4512662 100644 --- a/modules/core/ollama-manager.js +++ b/modules/core/ollama-manager.js @@ -382,14 +382,37 @@ class OllamaManager { try { await this._connecting; } catch (e) { /* fall through to the state check */ } return this.connected; } - this.terminal.print('Connecting to Ollama…', 'info'); - this._connecting = this.connect(); - try { await this._connecting; } catch (e) { - this.terminal.log('WARN', 'Ollama', 'Auto-connect failed: ' + e.message); + // An automatic connect must not silently rewrite the model the user picked. + const savedModel = (typeof localStorage !== 'undefined') ? localStorage.getItem('webx11_ollama_model') : null; + this.terminal.print('Ollama: connecting on first use…', 'info'); + let timedOut = false; + try { + this._connecting = this.connect(); + // connect() uses a bare fetch(), so a blackholed endpoint would hang this promise + // forever and — because the promise is shared — every later message with it. + const deadline = this.connectTimeoutMs || 10000; + const outcome = await Promise.race([ + this._connecting.then(() => 'done', e => { throw e; }), + new Promise(resolve => setTimeout(() => resolve('timeout'), deadline)), + ]); + timedOut = outcome === 'timeout' && !this.connected; + } catch (e) { + // A throwing terminal.log() must not leave _connecting poisoned: log defensively. + try { this.terminal.log('WARN', 'Ollama', 'Auto-connect failed: ' + (e && e.message ? e.message : e)); } catch (_) {} + } finally { + this._connecting = null; + } + if (this.connected && savedModel && !(this.models || []).some(m => m.name === savedModel)) { + this.currentModel = savedModel; + if (this.currentProvider) this.currentProvider.model = savedModel; + // connect() may already have overwritten the stored preference; put it back. + try { localStorage.setItem('webx11_ollama_model', savedModel); } catch (_) {} + this.terminal.print('Keeping your saved model "' + savedModel + '" — this endpoint does not list it.', 'warning'); } - this._connecting = null; if (!this.connected) { - this.terminal.print('Auto-connect failed — check the endpoint in the Ollama tab (or run ollamaconnect).', 'warning'); + this.terminal.print(timedOut + ? 'Auto-connect timed out after ' + ((this.connectTimeoutMs || 10000) / 1000) + 's — the endpoint is not answering. Check the Ollama tab or run ollamaconnect.' + : 'Auto-connect failed — check the endpoint in the Ollama tab (or run ollamaconnect).', 'warning'); } return this.connected; } @@ -397,6 +420,8 @@ class OllamaManager { // ─── Chat (non-streaming) ──────────────────────────────────────────────── async chat(prompt, model = null) { + // Per-call state: never let a reason captured by an earlier failure be replayed here. + this._lastChatError = null; if (!this.connected && !(await this._ensureConnected())) { this.terminal.print('Ollama not connected: the endpoint is unreachable. Check the Ollama tab or run ollamaconnect.', 'error'); return null; @@ -426,7 +451,11 @@ class OllamaManager { } if (lastError === null) { - this.terminal.print(`Ollama error: No provider responded`, 'error'); + // Name the real cause when one was captured; the generic wording is only a fallback. + const reason = this._lastChatError; + this.terminal.print( + reason ? `Ollama error: ${reason}` : 'Ollama error: No provider responded', + 'error'); } return null; } @@ -552,6 +581,9 @@ class OllamaManager { return fullText || null; } } catch (error) { + // Remember why this provider failed so chat() can name the real cause + // instead of falling back to the generic "No provider responded". + this._lastChatError = error.message || String(error); if (fullText && fullText.length > 0) { this.terminal.printStreamingResponse(`\n\n[Stream error: ${error.message}]`, true); return fullText; @@ -739,6 +771,8 @@ class OllamaManager { } async chatWithTools(messages, tools, model = null) { + // Per-call state: never let a reason captured by an earlier failure be replayed here. + this._lastChatError = null; if (!this.connected && !(await this._ensureConnected())) { this.terminal.print('Ollama not connected: the endpoint is unreachable. Check the Ollama tab or run ollamaconnect.', 'error'); return { message: null, tool_calls: null, error: 'Not connected' }; @@ -769,7 +803,12 @@ class OllamaManager { } } - return lastError || { message: null, tool_calls: null, error: 'All providers failed' }; + // Name the real cause when one was captured; the generic wording is only a fallback. + const reason = this._lastChatError; + return lastError || { + message: null, tool_calls: null, + error: reason ? `All providers failed: ${reason}` : 'All providers failed' + }; } async _chatWithToolsWithProvider(provider, messages, tools, model) { @@ -855,16 +894,21 @@ class OllamaManager { // DeepSeek: handle null content with tool calls — send empty string if (isDeepSeek && content === '' && toolCalls && toolCalls.length > 0) { + const normalised = toolCalls.map(tc => ({ + id: tc.id, + type: tc.type || 'function', + function: { + name: tc.function?.name || '', + arguments: tc.function?.arguments || '{}' + } + })); return { - message: { role: 'assistant', content: '' }, - tool_calls: toolCalls.map(tc => ({ - id: tc.id, - type: tc.type || 'function', - function: { - name: tc.function?.name || '', - arguments: tc.function?.arguments || '{}' - } - })), + // tool_calls must live INSIDE the assistant message: the caller pushes + // result.message and then a role:'tool' reply, and a tool message whose + // tool_call_id has no matching assistant call is the HTTP 400 this + // branch exists to avoid. + message: { role: 'assistant', content: '', tool_calls: normalised }, + tool_calls: normalised, error: null }; } @@ -948,6 +992,7 @@ class OllamaManager { return await this.chatWithToolsFallback(messages, tools, useModel); } this.terminal.print(`${provider.name} /api/chat error: ${error.message}`, 'error'); + this._lastChatError = error.message || String(error); return { message: null, tool_calls: null, error: error.message }; } finally { this._abortController = null; |