diff --git a/CHANGELOG.md b/CHANGELOG.md index a2deed8..a077dbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,11 @@ workspace subtree) and never enumerate blocked system roots. (`api/routes.py`, `api/workspace.py`, `static/panels.js`, `static/style.css`) (partial for #616) +## [v0.50.182] — 2026-04-24 + +### Fixed +- **Auxiliary title model now respected** — when `auxiliary.title_generation` is explicitly configured in `config.yaml`, the WebUI now routes title generation through that dedicated model instead of silently using the chat session's model. Adds `_aux_title_configured()` to detect meaningful auxiliary config, `_aux_title_timeout()` to respect the configured per-task timeout (was hardcoded to 15.0 s), and adds the missing `llm_invalid_aux` fallback path so invalid auxiliary outputs trigger the agent-model fallback. (`api/streaming.py`, `tests/test_title_aux_routing.py`) Co-authored by @starship-s. + ## [v0.50.181] — 2026-04-24 ### Changed diff --git a/api/streaming.py b/api/streaming.py index 95e9b83..c5a50ff 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -233,6 +233,43 @@ def _is_minimax_route(provider: str = '', model: str = '', base_url: str = '') - return 'minimax' in text or 'minimaxi.com' in text +def _aux_title_configured() -> bool: + """Return True when any auxiliary title_generation config field is meaningfully set.""" + try: + from agent.auxiliary_client import _get_auxiliary_task_config + tg = _get_auxiliary_task_config('title_generation') + provider = tg.get('provider', '') or '' + model = tg.get('model', '') or '' + base_url = tg.get('base_url', '') or '' + return bool(model or base_url or (provider and provider.lower() != 'auto')) + except Exception: + return False + +def _aux_title_timeout(default: float = 15.0) -> float: + """Return the configured timeout (seconds) for auxiliary title generation. + + Only accepts positive numeric values. Falls back to *default* when the + value is ``None``, non-numeric, zero, or negative, and emits a debug log + so mis-configurations are visible in server output. + """ + try: + from agent.auxiliary_client import _get_auxiliary_task_config + tg = _get_auxiliary_task_config('title_generation') + raw = tg.get('timeout') + if raw is None: + return default + try: + value = float(raw) + except (ValueError, TypeError): + logger.debug("aux title timeout: non-numeric value %r, falling back to %s", raw, default) + return default + if value > 0: + return value + logger.debug("aux title timeout: non-positive value %s, falling back to %s", value, default) + return default + except Exception: + return default + def _title_completion_budget(provider: str = '', model: str = '', base_url: str = '') -> int: if _is_minimax_route(provider, model, base_url): return 384 @@ -255,6 +292,7 @@ def generate_title_raw_via_aux( if _is_minimax_route(provider, model, base_url): reasoning_extra["reasoning_split"] = True try: + _timeout = _aux_title_timeout() from agent.auxiliary_client import call_llm for idx, prompt in enumerate(prompts): messages = [ @@ -270,7 +308,7 @@ def generate_title_raw_via_aux( messages=messages, max_tokens=max_tokens, temperature=0.2, - timeout=15.0, + timeout=_timeout, extra_body=reasoning_extra, ) raw = '' @@ -388,14 +426,29 @@ def _generate_llm_session_title_for_agent(agent, user_text: str, assistant_text: return None, 'llm_invalid', str(raw)[:120] -def _generate_llm_session_title_via_aux(user_text: str, assistant_text: str, agent=None) -> tuple[Optional[str], str, str]: - """Generate a title via dedicated auxiliary LLM route, then sanitize/validate result.""" +def _generate_llm_session_title_via_aux(user_text: str, assistant_text: str, agent=None, *, use_agent_model: bool = False) -> tuple[Optional[str], str, str]: + """Generate a title via dedicated auxiliary LLM route, then sanitize/validate result. + + When use_agent_model is False (default), the auxiliary client resolves + provider/model/base_url from config.yaml auxiliary.title_generation, which + prevents the session's chat model (e.g. a Chinese model) from overriding + the dedicated title model. When True, the agent's attrs are passed through + (legacy fallback behaviour). + """ + if use_agent_model and agent: + provider = getattr(agent, 'provider', '') + model = getattr(agent, 'model', '') + base_url = getattr(agent, 'base_url', '') + else: + provider = '' + model = '' + base_url = '' raw, status = generate_title_raw_via_aux( user_text, assistant_text, - provider=getattr(agent, 'provider', '') if agent else '', - model=getattr(agent, 'model', '') if agent else '', - base_url=getattr(agent, 'base_url', '') if agent else '', + provider=provider, + model=model, + base_url=base_url, ) if not raw: return None, status, '' @@ -522,14 +575,15 @@ def _run_background_title_update(session_id: str, user_text: str, assistant_text if not still_auto: _put_title_status(put_event, session_id, 'skipped', 'manual_title', current) return - # Prefer the active session model when available so title generation - # matches the user's chosen runtime and can use provider-specific fixes. - if agent: + aux_title_configured = _aux_title_configured() + if agent and not aux_title_configured: next_title, llm_status, raw_preview = _generate_llm_session_title_for_agent(agent, user_text, assistant_text) if not next_title and llm_status in ('llm_error', 'llm_invalid'): - next_title, llm_status, raw_preview = _generate_llm_session_title_via_aux(user_text, assistant_text, agent=agent) + next_title, llm_status, raw_preview = _generate_llm_session_title_via_aux(user_text, assistant_text, agent=agent, use_agent_model=True) else: - next_title, llm_status, raw_preview = _generate_llm_session_title_via_aux(user_text, assistant_text, agent=agent) + next_title, llm_status, raw_preview = _generate_llm_session_title_via_aux(user_text, assistant_text) + if not next_title and agent and llm_status in ('llm_error_aux', 'llm_invalid_aux'): + next_title, llm_status, raw_preview = _generate_llm_session_title_for_agent(agent, user_text, assistant_text) source = llm_status if not next_title: next_title = _fallback_title_from_exchange(user_text, assistant_text) @@ -539,14 +593,7 @@ def _run_background_title_update(session_id: str, user_text: str, assistant_text wrote_title = False effective_title = current if next_title: - # Hold _agent_lock only for in-memory mutation + save so title write - # is serialized with checkpoint saves, cancel_stream, and other - # session-mutating endpoints. The LLM round-trip above ran outside - # the lock to avoid blocking other writers. with _get_session_agent_lock(session_id): - # Stale-object guard: rebind to the canonical cached Session - # instance under LOCK before checking whether a user rename - # landed while the LLM title request was in-flight. with LOCK: s = SESSIONS.get(session_id, s) effective_title = str(s.title or '').strip() diff --git a/tests/test_title_aux_routing.py b/tests/test_title_aux_routing.py new file mode 100644 index 0000000..f11bf8b --- /dev/null +++ b/tests/test_title_aux_routing.py @@ -0,0 +1,295 @@ +"""Regression tests for auxiliary title-generation config routing. + +Covers: + - _aux_title_configured() broad detection (provider, model, base_url) + - generate_title_raw_via_aux() reads timeout from config instead of hardcoding 15.0 + - aux→agent fallback triggers on 'llm_invalid_aux' status (Comment 1) + - _aux_title_timeout rejects zero, negative, and non-numeric values (Comment 4) +""" +import sys +import types +import unittest +from unittest.mock import MagicMock, patch + +# Stub agent.auxiliary_client so it is importable in the test environment +# (the real package lives in hermes-agent, which is not installed here). +_agent_stub = types.ModuleType('agent') +_aux_stub = types.ModuleType('agent.auxiliary_client') +sys.modules.setdefault('agent', _agent_stub) +sys.modules.setdefault('agent.auxiliary_client', _aux_stub) +_agent_stub.auxiliary_client = _aux_stub + + +def _patch_tg_config(config_dict): + """Return a patch context manager that makes _get_auxiliary_task_config return config_dict.""" + return patch('agent.auxiliary_client._get_auxiliary_task_config', return_value=config_dict, create=True) + + +class TestAuxTitleConfigured(unittest.TestCase): + def _call(self, tg_config): + from api.streaming import _aux_title_configured + with _patch_tg_config(tg_config): + return _aux_title_configured() + + def test_model_set_returns_true(self): + self.assertTrue(self._call({'provider': '', 'model': 'gpt-4o-mini', 'base_url': ''})) + + def test_base_url_set_returns_true(self): + self.assertTrue(self._call({'provider': '', 'model': '', 'base_url': 'http://localhost:1234'})) + + def test_provider_set_non_auto_returns_true(self): + self.assertTrue(self._call({'provider': 'openai', 'model': '', 'base_url': ''})) + + def test_provider_auto_returns_false(self): + self.assertFalse(self._call({'provider': 'auto', 'model': '', 'base_url': ''})) + + def test_provider_auto_case_insensitive_returns_false(self): + self.assertFalse(self._call({'provider': 'AUTO', 'model': '', 'base_url': ''})) + + def test_all_empty_returns_false(self): + self.assertFalse(self._call({'provider': '', 'model': '', 'base_url': ''})) + + def test_empty_dict_returns_false(self): + self.assertFalse(self._call({})) + + def test_provider_configured_model_blank_returns_true(self): + """Regression: provider set + blank model must still be treated as configured.""" + self.assertTrue(self._call({'provider': 'anthropic', 'model': '', 'base_url': ''})) + + def test_base_url_only_returns_true(self): + """Regression: base_url alone (no model) must still be treated as configured.""" + self.assertTrue(self._call({'provider': '', 'model': '', 'base_url': 'https://api.example.com'})) + + def test_import_error_returns_false(self): + from api.streaming import _aux_title_configured + with patch('agent.auxiliary_client._get_auxiliary_task_config', side_effect=ImportError("no module"), create=True): + self.assertFalse(_aux_title_configured()) + + +class TestGenerateTitleRawViaAuxTimeout(unittest.TestCase): + """Verify generate_title_raw_via_aux() reads timeout from config rather than hardcoding 15.0.""" + + def _run_with_config(self, tg_config, expected_timeout): + from api.streaming import generate_title_raw_via_aux + + mock_resp = MagicMock() + mock_resp.choices = [MagicMock()] + mock_resp.choices[0].message.content = 'Test Title' + + captured = {} + + def fake_call_llm(**kwargs): + captured['timeout'] = kwargs.get('timeout') + return mock_resp + + with _patch_tg_config(tg_config): + with patch('agent.auxiliary_client.call_llm', side_effect=fake_call_llm, create=True): + result, status = generate_title_raw_via_aux( + user_text='What is the weather?', + assistant_text='It is sunny.', + ) + + self.assertEqual(result, 'Test Title') + self.assertAlmostEqual(captured['timeout'], expected_timeout) + + def test_default_timeout_when_not_set(self): + """No timeout in config → uses 15.0 default.""" + self._run_with_config({'provider': '', 'model': 'gpt-4o', 'base_url': ''}, 15.0) + + def test_custom_timeout_from_config(self): + """Regression: timeout set in config must be used instead of hardcoded 15.0.""" + self._run_with_config( + {'provider': '', 'model': 'gpt-4o', 'base_url': '', 'timeout': 30.0}, + 30.0, + ) + + def test_integer_timeout_from_config(self): + """Config timeout as int is coerced to float.""" + self._run_with_config( + {'provider': '', 'model': 'gpt-4o', 'base_url': '', 'timeout': 5}, + 5.0, + ) + + def test_timeout_none_in_config_falls_back_to_default(self): + """Explicit None in config falls back to 15.0.""" + self._run_with_config( + {'provider': '', 'model': 'gpt-4o', 'base_url': '', 'timeout': None}, + 15.0, + ) + + +class TestAuxTitleTimeoutEdgeCases(unittest.TestCase): + """Comment 4: _aux_title_timeout must reject zero, negative, and non-numeric values.""" + + def _call(self, tg_config, default=15.0): + from api.streaming import _aux_title_timeout + with _patch_tg_config(tg_config): + return _aux_title_timeout(default=default) + + def test_timeout_zero_falls_back_to_default(self): + """timeout: 0 is not strictly positive → fall back to default.""" + result = self._call({'timeout': 0}, default=15.0) + self.assertEqual(result, 15.0) + + def test_timeout_negative_falls_back_to_default(self): + """timeout: -1 is not strictly positive → fall back to default.""" + result = self._call({'timeout': -1}, default=15.0) + self.assertEqual(result, 15.0) + + def test_timeout_non_numeric_string_falls_back_to_default(self): + """timeout: 'abc' cannot be coerced to float → fall back to default.""" + result = self._call({'timeout': 'abc'}, default=15.0) + self.assertEqual(result, 15.0) + + def test_timeout_empty_string_falls_back_to_default(self): + """timeout: '' cannot be coerced to a positive float → fall back to default.""" + result = self._call({'timeout': ''}, default=15.0) + self.assertEqual(result, 15.0) + + def test_timeout_positive_passes_through(self): + """A valid positive timeout is returned as-is.""" + result = self._call({'timeout': 25.0}, default=15.0) + self.assertEqual(result, 25.0) + + def test_custom_default_used_on_invalid(self): + """When the value is invalid, the caller-supplied *default* is returned.""" + result = self._call({'timeout': 0}, default=20.0) + self.assertEqual(result, 20.0) + + +class TestAuxInvalidAuxTriggersAgentFallback(unittest.TestCase): + """Comment 1: when aux returns llm_invalid_aux, the agent route must be tried as fallback. + + Pins the behaviour so the fallback tuple in _run_background_title_update + stays synchronised with the statuses that _generate_llm_session_title_via_aux + actually emits. + """ + + @patch('api.streaming._aux_title_configured', return_value=True) + @patch('api.streaming._generate_llm_session_title_via_aux') + @patch('api.streaming._generate_llm_session_title_for_agent') + @patch('api.streaming.get_session') + def test_llm_invalid_aux_triggers_agent_fallback( + self, mock_get_session, mock_agent_title, mock_aux_title, mock_configured, + ): + """Simulate aux returning (None, 'llm_invalid_aux', '...') and verify agent fallback fires.""" + from api.streaming import _run_background_title_update + + # Build a mock session that passes all the pre-checks + mock_session = MagicMock() + mock_session.title = 'Untitled' + mock_session.llm_title_generated = False + mock_session.messages = [ + {'role': 'user', 'content': 'What is the weather?'}, + {'role': 'assistant', 'content': 'It is sunny and warm.'}, + ] + mock_get_session.return_value = mock_session + + # aux route returns invalid title + mock_aux_title.return_value = (None, 'llm_invalid_aux', 'bad thinking preamble') + + # agent route succeeds + mock_agent_title.return_value = ('Weather Report', 'llm', '') + + events = [] + + def fake_put_event(event_type, data): + events.append((event_type, data)) + + _run_background_title_update( + session_id='test-session', + user_text='What is the weather?', + assistant_text='It is sunny and warm.', + placeholder_title='Untitled', + put_event=fake_put_event, + agent=MagicMock(), + ) + + # The agent fallback must have been invoked + mock_agent_title.assert_called_once() + + # A title must have been produced via the agent route + title_events = [(e, d) for e, d in events if e == 'title'] + self.assertTrue(len(title_events) > 0, "Expected a 'title' event to be emitted") + self.assertEqual(title_events[0][1]['title'], 'Weather Report') + + @patch('api.streaming._aux_title_configured', return_value=True) + @patch('api.streaming._generate_llm_session_title_via_aux') + @patch('api.streaming._generate_llm_session_title_for_agent') + @patch('api.streaming.get_session') + def test_llm_error_aux_triggers_agent_fallback( + self, mock_get_session, mock_agent_title, mock_aux_title, mock_configured, + ): + """Simulate aux returning (None, 'llm_error_aux', '') and verify agent fallback fires.""" + from api.streaming import _run_background_title_update + + mock_session = MagicMock() + mock_session.title = 'Untitled' + mock_session.llm_title_generated = False + mock_session.messages = [ + {'role': 'user', 'content': 'Tell me a joke.'}, + {'role': 'assistant', 'content': 'Why did the chicken cross the road?'}, + ] + mock_get_session.return_value = mock_session + + mock_aux_title.return_value = (None, 'llm_error_aux', '') + mock_agent_title.return_value = ('Chicken Joke', 'llm', '') + + events = [] + + def fake_put_event(event_type, data): + events.append((event_type, data)) + + _run_background_title_update( + session_id='test-session-2', + user_text='Tell me a joke.', + assistant_text='Why did the chicken cross the road?', + placeholder_title='Untitled', + put_event=fake_put_event, + agent=MagicMock(), + ) + + mock_agent_title.assert_called_once() + + @patch('api.streaming._aux_title_configured', return_value=True) + @patch('api.streaming._generate_llm_session_title_via_aux') + @patch('api.streaming._generate_llm_session_title_for_agent') + @patch('api.streaming.get_session') + def test_success_status_does_not_trigger_agent_fallback( + self, mock_get_session, mock_agent_title, mock_aux_title, mock_configured, + ): + """When aux succeeds, the agent route must NOT be called.""" + from api.streaming import _run_background_title_update + + mock_session = MagicMock() + mock_session.title = 'Untitled' + mock_session.llm_title_generated = False + mock_session.messages = [ + {'role': 'user', 'content': 'Hello'}, + {'role': 'assistant', 'content': 'Hi there'}, + ] + mock_get_session.return_value = mock_session + + # aux succeeds on first try + mock_aux_title.return_value = ('Greeting', 'llm_aux', '') + + events = [] + + def fake_put_event(event_type, data): + events.append((event_type, data)) + + _run_background_title_update( + session_id='test-session-3', + user_text='Hello', + assistant_text='Hi there', + placeholder_title='Untitled', + put_event=fake_put_event, + agent=MagicMock(), + ) + + # Agent route must NOT have been invoked + mock_agent_title.assert_not_called() + + +if __name__ == '__main__': + unittest.main()