From 49e73b4f6bf58486a7dc4ce8e9e8f95957ee3d2b Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 7 Jul 2026 22:28:10 +0200 Subject: [PATCH] fix(summarizer): raise on stream failure instead of swallowing it as an empty summary A failed summary stream (Ollama 404, cloud API error, adapter error record) was logged and then swallowed (bare return), so consumers saw an empty-but-successful stream and wrote an empty note + STREAM_COMPLETE + exit 0 with no error surfaced anywhere. The whole downstream error path (STREAM_ERROR + exit 1 in simple_recorder, failure mapping in app/main.js) already existed - the generators just never raised into it. - _stream_completion: all four provider paths (anthropic/bedrock/openai/ ollama) now log + raise the original exception - _adapter_stream: error NDJSON record, HTTPError, and transport errors now raise (query_transcript_streaming catches and shows them inline) - summarize_transcript_streaming: empty-stream guard - a stream that completes without raising but yields no non-whitespace content raises ValueError instead of saving an empty summary (mirrors the existing map-reduce empty-reduce guard) - tests: unit coverage for all provider paths + empty-stream guard; model-free t2 e2e (summarize-failure) drives reprocess against a mock Ollama 404 and asserts honest failure + the existing summary untouched Fixes #301 --- e2e/fixtures/mock-ollama.js | 13 ++ e2e/specs/summarize-failure.t2.spec.ts | 89 +++++++++++ src/summarizer.py | 50 ++++-- tests/test_summarizer_stream_errors.py | 205 +++++++++++++++++++++++++ 4 files changed, 340 insertions(+), 17 deletions(-) create mode 100644 e2e/specs/summarize-failure.t2.spec.ts create mode 100644 tests/test_summarizer_stream_errors.py diff --git a/e2e/fixtures/mock-ollama.js b/e2e/fixtures/mock-ollama.js index bc98b69b..6f8e0177 100644 --- a/e2e/fixtures/mock-ollama.js +++ b/e2e/fixtures/mock-ollama.js @@ -23,6 +23,9 @@ const OLLAMA_PORT = 11434; * @param {object} [opts] * @param {string} [opts.chatReply='ok'] assistant content returned by /api/chat. * @param {string[]} [opts.chatReplyQueue=[]] queue of replies to dequeue per call; falls back to chatReply when exhausted. + * @param {{status:number, message:string}} [opts.chatError] when set, /api/chat responds with this HTTP + * status and JSON body `{"error": message}` (the real Ollama 404 shape) instead of a reply. Off by + * default so existing specs are unaffected. chatCalls still increments so callers can assert it was hit. * @param {string[]} [opts.installedModels=['gemma4:e2b-it-qat','llama3.2:3b']] models /api/tags reports as installed. * @param {number} [opts.pullDelayMs=0] hold the /api/pull response open this long before completing it -- gives a * cancel-mid-download test a real window to call cancel-pull before the mock would otherwise finish first. @@ -31,6 +34,7 @@ const OLLAMA_PORT = 11434; function startMockOllama(opts = {}) { const chatReply = opts.chatReply ?? 'ok'; const chatReplyQueue = Array.isArray(opts.chatReplyQueue) ? [...opts.chatReplyQueue] : []; + const chatError = opts.chatError ?? null; const installedModels = Array.isArray(opts.installedModels) ? opts.installedModels : ['gemma4:e2b-it-qat', 'llama3.2:3b']; @@ -111,6 +115,15 @@ function startMockOllama(opts = {}) { const msgs = Array.isArray(body.messages) ? body.messages : []; lastChatPrompt = msgs.length ? String(msgs[msgs.length - 1].content || '') : ''; + // Opt-in failure mode: reply with the configured HTTP error (the real + // Ollama 404 "model not found" shape) so the summarizer's stream fails + // and must surface it rather than saving an empty summary. + if (chatError) { + res.writeHead(chatError.status, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ error: chatError.message })); + return; + } + // Dequeue reply or fall back to default const reply = chatReplyQueue.length > 0 ? chatReplyQueue.shift() : chatReply; diff --git a/e2e/specs/summarize-failure.t2.spec.ts b/e2e/specs/summarize-failure.t2.spec.ts new file mode 100644 index 00000000..1e3988dd --- /dev/null +++ b/e2e/specs/summarize-failure.t2.spec.ts @@ -0,0 +1,89 @@ +import { test, expect } from '../fixtures/electron'; +import { realUserDataDir, fileSig } from '../fixtures/real-user-data'; +import { writeUserConfig, writeMeetingSummary } from '../fixtures/user-config'; +import { startMockOllama } from '../fixtures/mock-ollama'; +import { readFileSync } from 'fs'; + +/** + * T2 — summarisation FAILURE contract (regression for the swallowed-stream bug / + * GH #301). Drives the real `reprocess` path (re-summarise an existing + * transcript — NO ASR, NO real model) with a mock Ollama that returns a 404 + * "model not found" on /api/chat, and asserts the failure is surfaced honestly: + * + * 1. reprocess reports failure (success:false), NOT a false success. + * 2. the pre-existing summary on disk is byte-for-byte UNCHANGED — the failed + * stream must not clobber it with an empty summary. + * + * Before the fix the streaming generators swallowed the provider error and ended + * the stream silently, so the consumer saw an empty-but-"successful" stream, wrote + * an empty summary, printed STREAM_COMPLETE and exited 0. Model-free: the mock + * never loads a model, so this runs in the model-free t2 lane (no @pipeline tag). + */ + +const TRANSCRIPT = + 'Alice: we ship the release on Friday. Bob: the budget is fifty thousand dollars.'; + +const SEEDED_SUMMARY = 'PRE-EXISTING summary that a failed reprocess must NOT clobber'; + +type ReprocessResult = { success: boolean; error?: string }; +type StenoWindow = Window & { + stenoai: { + meetings: { + reprocess: (summaryFile: string, regenTitle: boolean, name: string) => Promise; + }; + }; +}; + +const readSummary = (file: string) => JSON.parse(readFileSync(file, 'utf8')); + +test('reprocess surfaces a stream failure and leaves the existing summary untouched', async ({ + launchApp, + userDataDir, +}) => { + const realDirBefore = fileSig(realUserDataDir()); + + // Local provider so the summarizer talks to the mock Ollama on 11434. + writeUserConfig(userDataDir, { ai_provider: 'local' }); + const summaryFile = writeMeetingSummary(userDataDir, 'failure', { + name: 'Failure Meeting', + summary: SEEDED_SUMMARY, + key_points: ['seeded point'], + action_items: ['seeded action'], + transcript: TRANSCRIPT, + }); + const summarySigBefore = fileSig(summaryFile); + + // Mock Ollama answers /api/chat with the real Ollama 404 shape so the stream + // fails mid-flight instead of returning content. + const ollama = await startMockOllama({ + chatError: { status: 404, message: "model 'gemma4:e2b-it-qat' not found" }, + }); + try { + const { page } = await launchApp(); + + const res = await page.evaluate( + (f) => (window as StenoWindow).stenoai.meetings.reprocess(f, false, 'Failure Meeting'), + summaryFile, + ); + + // (1) The operation must report failure, not a false success. + expect(res.success).toBe(false); + + // The summariser must have actually attempted the call (proves we exercised + // the streaming path, not an earlier bail-out). + expect(ollama.chatCalls()).toBeGreaterThanOrEqual(1); + + // (2) The pre-existing summary on disk is byte-for-byte unchanged — a failed + // stream must not overwrite it with an empty summary. + expect(fileSig(summaryFile)).toBe(summarySigBefore); + const after = readSummary(summaryFile); + expect(after.summary).toBe(SEEDED_SUMMARY); + expect(after.key_points).toEqual(['seeded point']); + expect(after.action_items).toEqual(['seeded action']); + + // Keystone: the real user-data dir is byte-for-byte untouched. + expect(fileSig(realUserDataDir())).toBe(realDirBefore); + } finally { + await ollama.close(); + } +}); diff --git a/src/summarizer.py b/src/summarizer.py index 8b3b8a25..a7508287 100644 --- a/src/summarizer.py +++ b/src/summarizer.py @@ -853,9 +853,10 @@ def _adapter_stream(self, prompt: str, timeout_seconds: int = 600): ... {"type": "done", "model": "...", "input_tokens": N, "output_tokens": M} or {"type": "error", "error": "..."} on failure. - Yields the text portion of each chunk record. Errors are logged - and the generator returns silently — matches the streaming-error - behaviour of the cloud and ollama paths. + Yields the text portion of each chunk record. On any failure (an + error record, an HTTP error, or a transport error) it logs and then + RAISES so the consumer surfaces the real error via STREAM_ERROR — + matching the streaming-error contract of the cloud and ollama paths. """ import urllib.request import urllib.error @@ -890,7 +891,7 @@ def _adapter_stream(self, prompt: str, timeout_seconds: int = 600): yield text elif kind == "error": logger.error(f"Adapter stream error: {record.get('error')}") - return + raise RuntimeError(f"Adapter stream error: {record.get('error')}") elif kind == "done": return except urllib.error.HTTPError as e: @@ -898,8 +899,10 @@ def _adapter_stream(self, prompt: str, timeout_seconds: int = 600): logger.error("Adapter stream rejected: session expired or unauthorized") else: logger.error(f"Adapter streaming failed: HTTP {e.code}") + raise except Exception as e: logger.error(f"Adapter streaming failed: {e}") + raise def _anthropic_chat(self, prompt: str, timeout_seconds: int = 300) -> str: """Send a chat request via the Anthropic Messages API.""" @@ -1487,7 +1490,7 @@ def _stream_completion(self, prompt: str): yield text except Exception as e: logger.error(f"Anthropic streaming failed: {e}") - return + raise elif self.cloud_provider == "bedrock": # Bedrock's /converse-stream uses amazon eventstream framing # (binary, length-prefixed), which would need a dedicated @@ -1503,7 +1506,7 @@ def _stream_completion(self, prompt: str): yield text except Exception as e: logger.error(f"Bedrock summarisation failed: {e}") - return + raise else: try: response = self.cloud_client.chat.completions.create( @@ -1519,7 +1522,7 @@ def _stream_completion(self, prompt: str): yield content except Exception as e: logger.error(f"OpenAI streaming failed: {e}") - return + raise return # Ollama (local or remote) — shared with the free-form template path. @@ -1527,7 +1530,7 @@ def _stream_completion(self, prompt: str): yield from self._stream_direct(prompt) except Exception as e: logger.error(f"Ollama streaming failed: {e}") - return + raise def summarize_transcript_streaming(self, transcript: str, duration_minutes: int = 0, language: str = "en", notes: str = None, progress_callback=None, template_prompt: Optional[str] = None): """Generator that yields markdown chunks from the LLM. @@ -1553,15 +1556,28 @@ def summarize_transcript_streaming(self, transcript: str, duration_minutes: int # ACTIVE provider — not straight to Ollama, which has no client and # would crash in cloud/adapter mode. prompt = self._create_template_report_prompt(transcript, template_prompt, language, notes) - yield from self._stream_completion(prompt) - return - - if self._needs_chunking(transcript, notes): - yield from self._map_reduce_streaming(transcript, language, notes, progress_callback) - return - - prompt = self._create_markdown_prompt(transcript, language, notes) - yield from self._stream_completion(prompt) + inner = self._stream_completion(prompt) + empty_message = "Model returned an empty report" + elif self._needs_chunking(transcript, notes): + inner = self._map_reduce_streaming(transcript, language, notes, progress_callback) + empty_message = "Model returned an empty summary" + else: + prompt = self._create_markdown_prompt(transcript, language, notes) + inner = self._stream_completion(prompt) + empty_message = "Model returned an empty summary" + + # Defense in depth mirroring _map_reduce_streaming's empty-reduce guard: a + # provider can complete the stream without raising yet yield nothing (or + # only whitespace). Route that through STREAM_ERROR instead of silently + # saving an empty summary/report. (_map_reduce_streaming already raises on + # an empty reduce, so wrapping it here is harmless.) + saw_content = False + for chunk in inner: + if chunk and chunk.strip(): + saw_content = True + yield chunk + if not saw_content: + raise ValueError(empty_message) def test_connection(self) -> bool: """ diff --git a/tests/test_summarizer_stream_errors.py b/tests/test_summarizer_stream_errors.py new file mode 100644 index 00000000..63e792c5 --- /dev/null +++ b/tests/test_summarizer_stream_errors.py @@ -0,0 +1,205 @@ +"""Regression tests for streaming-summary error propagation. + +Before this fix the streaming generators in ``src.summarizer`` swallowed +provider failures (``logger.error(...); return``): a failed stream (e.g. Ollama +404 "model not found", a cloud API error, or an adapter error record) ended the +generator silently, so the consumers in ``simple_recorder`` saw an empty-but- +"successful" stream and wrote an empty summary + printed STREAM_COMPLETE + exit +0. These tests pin the corrected contract — a stream failure RAISES so the +consumer surfaces it via STREAM_ERROR — and add an empty-stream guard mirroring +``_map_reduce_streaming``'s empty-reduce guard. Fixes GH #301. +""" + +import unittest +from unittest import mock + +from src.config import Config +from src.summarizer import OllamaSummarizer + + +def _make_summarizer(model="llama3.2:3b"): + # Mock the readiness check: __init__ calls _ensure_ollama_ready() for the + # local provider, so without this construction would try to start Ollama + # (non-hermetic). Mirrors tests/test_summarizer_template.py::_s. + with mock.patch.object(OllamaSummarizer, "_ensure_ollama_ready", return_value=True): + return OllamaSummarizer(model_name=model, ai_provider="local", config=Config()) + + +def _gen_raising(exc): + """Return a zero-arg-usable generator function that raises ``exc`` on iteration.""" + + def _factory(*args, **kwargs): + raise exc + yield # pragma: no cover - makes this a generator + + return _factory + + +def _gen_yielding(chunks): + def _factory(*args, **kwargs): + for c in chunks: + yield c + + return _factory + + +class _FakeResp: + """Minimal context-manager stand-in for urllib.request.urlopen().""" + + def __init__(self, lines): + self._lines = lines + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + def __iter__(self): + return iter(self._lines) + + +class OllamaStreamErrorTests(unittest.TestCase): + def test_ollama_stream_error_propagates(self): + """Ollama 404 (model not found) must propagate, not yield nothing.""" + s = _make_summarizer() + short = "Speaker: hello.\n" * 3 + err = RuntimeError("model 'x' not found (404)") + with mock.patch.object(s, "_ensure_ollama_ready"): + with mock.patch.object(s, "_stream_direct", side_effect=_gen_raising(err)): + with self.assertRaises(RuntimeError) as ctx: + list(s.summarize_transcript_streaming(short)) + # The ORIGINAL exception surfaces so STREAM_ERROR shows the real message. + self.assertIn("not found", str(ctx.exception)) + self.assertIs(ctx.exception, err) + + def test_empty_stream_raises_valueerror(self): + """A stream that completes without raising but yields only whitespace + must raise ValueError rather than silently save an empty summary.""" + s = _make_summarizer() + short = "Speaker: hello.\n" * 3 + with mock.patch.object(s, "_ensure_ollama_ready"): + with mock.patch.object(s, "_stream_direct", side_effect=_gen_yielding(["", " ", "\n"])): + with self.assertRaises(ValueError) as ctx: + list(s.summarize_transcript_streaming(short)) + self.assertIn("empty", str(ctx.exception).lower()) + + def test_successful_stream_yields_chunks_unchanged(self): + """Regression guard: a normal successful stream is passed through verbatim.""" + s = _make_summarizer() + short = "Speaker: hello.\n" * 3 + with mock.patch.object(s, "_ensure_ollama_ready"): + with mock.patch.object( + s, "_stream_direct", side_effect=_gen_yielding(["## Summary\n", "ok\n"]) + ): + out = list(s.summarize_transcript_streaming(short)) + self.assertEqual(out, ["## Summary\n", "ok\n"]) + + +class TemplatePathStreamErrorTests(unittest.TestCase): + def test_template_path_error_propagates(self): + """The free-form template path must also surface provider errors.""" + s = _make_summarizer() + err = RuntimeError("Ollama streaming failed: connection refused") + with mock.patch.object(s, "_ensure_ollama_ready"): + with mock.patch.object(s, "_stream_direct", side_effect=_gen_raising(err)): + with self.assertRaises(RuntimeError) as ctx: + list( + s.summarize_transcript_streaming( + "Speaker: hi.", template_prompt="Write a status update." + ) + ) + self.assertIs(ctx.exception, err) + + def test_template_path_empty_stream_raises(self): + s = _make_summarizer() + with mock.patch.object(s, "_ensure_ollama_ready"): + with mock.patch.object(s, "_stream_direct", side_effect=_gen_yielding([""])): + with self.assertRaises(ValueError): + list( + s.summarize_transcript_streaming( + "Speaker: hi.", template_prompt="Write a status update." + ) + ) + + +class CloudStreamErrorTests(unittest.TestCase): + def test_openai_compatible_stream_error_propagates(self): + """Cloud (openai-compatible) streaming failure must propagate.""" + s = _make_summarizer() + s.ai_provider = "cloud" + s.cloud_provider = "openai" + s.client = None + s.cloud_client = mock.Mock() + s.cloud_client.chat.completions.create.side_effect = RuntimeError( + "The model `x` does not exist (404)" + ) + with mock.patch.object(s, "_ensure_ollama_ready"): + with self.assertRaises(RuntimeError) as ctx: + list(s.summarize_transcript_streaming("Speaker: hi.")) + self.assertIn("does not exist", str(ctx.exception)) + + def test_anthropic_stream_error_propagates(self): + s = _make_summarizer() + s.ai_provider = "cloud" + s.cloud_provider = "anthropic" + s.anthropic_client = mock.Mock() + s.anthropic_client.messages.stream.side_effect = RuntimeError("overloaded_error") + with mock.patch.object(s, "_ensure_ollama_ready"): + with self.assertRaises(RuntimeError) as ctx: + list(s.summarize_transcript_streaming("Speaker: hi.")) + self.assertIn("overloaded_error", str(ctx.exception)) + + def test_bedrock_error_propagates(self): + s = _make_summarizer() + s.ai_provider = "cloud" + s.cloud_provider = "bedrock" + with mock.patch.object(s, "_ensure_ollama_ready"): + with mock.patch.object( + s, "_bedrock_chat", side_effect=RuntimeError("AccessDeniedException") + ): + with self.assertRaises(RuntimeError) as ctx: + list(s.summarize_transcript_streaming("Speaker: hi.")) + self.assertIn("AccessDeniedException", str(ctx.exception)) + + +class AdapterStreamErrorTests(unittest.TestCase): + def _adapter_summarizer(self): + s = _make_summarizer() + s.ai_provider = "adapter" + s.adapter_url = "https://adapter.example.com" + s.adapter_token = "fake-token" + return s + + def test_adapter_error_record_raises(self): + """An NDJSON {"type":"error"} record must raise, not end silently.""" + s = self._adapter_summarizer() + lines = [ + b'{"type": "chunk", "text": "partial"}\n', + b'{"type": "error", "error": "model not found"}\n', + ] + with mock.patch("urllib.request.urlopen", return_value=_FakeResp(lines)): + with self.assertRaises(RuntimeError) as ctx: + list(s._adapter_stream("prompt")) + self.assertIn("model not found", str(ctx.exception)) + + def test_adapter_httperror_raises(self): + import urllib.error + + s = self._adapter_summarizer() + http_err = urllib.error.HTTPError( + "https://adapter.example.com/ai/chat/stream", 500, "Server Error", {}, None + ) + with mock.patch("urllib.request.urlopen", side_effect=http_err): + with self.assertRaises(urllib.error.HTTPError): + list(s._adapter_stream("prompt")) + + def test_adapter_transport_error_raises(self): + s = self._adapter_summarizer() + with mock.patch("urllib.request.urlopen", side_effect=OSError("connection reset")): + with self.assertRaises(OSError): + list(s._adapter_stream("prompt")) + + +if __name__ == "__main__": + unittest.main()