diff --git a/backend/composer.py b/backend/composer.py index c42d99f..34ae3f2 100644 --- a/backend/composer.py +++ b/backend/composer.py @@ -30,6 +30,7 @@ from __future__ import annotations +from collections.abc import Callable from dataclasses import dataclass, field from typing import Any from uuid import uuid4 @@ -69,7 +70,28 @@ class ComposerDocument: """The composer's state for one session.""" draft: ComposerDraft = field(default_factory=ComposerDraft) + # The FALLBACK reasoning level, used only when no persisted-settings + # reader has been wired (a bare ComposerDocument() in a unit test). + # Whenever register_composer got a real SettingsManager, the persisted + # value wins - see reasoning_level_reader below. reasoning_level: str = DEFAULT_REASONING_LEVEL + # R7.5d follow-up: reads the ACTIVE provider's persisted reasoning mode + # ("Thinking"/"Quick"). Legacy's composer never stored a reasoning level + # of its own either - graphlink_composer_bridge.py's _reasoning() derives + # it from the settings manager on every payload build, which is why the + # Qt composer and the Settings dialog could never disagree. + # + # The first pass of R7.5d wired only the WRITE half of that (the toggle + # began genuinely persisting + re-applying to api_provider) and left this + # document's own private reasoning_level as the display source. That made + # the two halves diverge in a way that was worse than the original bug: + # get_ollama_reasoning_mode() defaults to "Thinking", so a user who had + # never touched the setting saw the composer confidently report "Quick + # Mode (No CoT)" while every chat call really ran with think=True, and + # selecting the already-active "Quick" was the only way to reach the + # state the UI already claimed. Deriving instead of mirroring removes the + # second source of truth rather than trying to keep two in sync. + reasoning_level_reader: Callable[[], str] | None = field(default=None, repr=False) # R4: the in-flight agent-dispatch request, if any - set by # backend/agents.py's AgentDispatcher around a real chat call so the # composer UI can reflect generating/idle state and offer cancellation. @@ -93,10 +115,24 @@ def end_request(self) -> None: self.request_id = None self.request_state = "idle" - def _reasoning_label(self) -> str: - return next(o["label"] for o in REASONING_OPTIONS if o["id"] == self.reasoning_level) + def effective_reasoning_level(self) -> str: + """The reasoning level to DISPLAY: the active provider's persisted + setting when one is reachable, else this document's own fallback. + + Normalizes the settings manager's Title-Case vocabulary + ("Thinking"/"Quick") to this payload's lowercase option ids, which + are the two the frontend renders and sends back. + """ + if self.reasoning_level_reader is None: + return self.reasoning_level + raw = str(self.reasoning_level_reader() or "").strip().lower() + return "thinking" if raw == "thinking" else "quick" + + def _reasoning_label(self, level: str) -> str: + return next(o["label"] for o in REASONING_OPTIONS if o["id"] == level) def payload(self) -> dict[str, Any]: + reasoning_level = self.effective_reasoning_level() return { "draft": { "id": self.draft.id, @@ -118,8 +154,8 @@ def payload(self) -> dict[str, Any]: "modelLabel": "", "modelOptions": [], "reasoning": { - "level": self.reasoning_level, - "label": self._reasoning_label(), + "level": reasoning_level, + "label": self._reasoning_label(reasoning_level), "options": list(REASONING_OPTIONS), }, "label": "Ollama (Local)", @@ -159,6 +195,17 @@ def register_composer( # always passes both (mirroring register_settings's own `notifications` # optional-param precedent exactly). document = ComposerDocument() + if settings_manager is not None: + # Derive the displayed level from the SAME persisted setting the + # Settings dialog edits and api_provider actually obeys, exactly as + # legacy's _reasoning() did (graphlink_composer_bridge.py:464-474), + # branching on the live provider for the same reason it did. + def _persisted_reasoning_mode() -> str: + if api_provider.is_local_llama_cpp_mode(): + return settings_manager.get_llama_cpp_reasoning_mode() + return settings_manager.get_ollama_reasoning_mode() + + document.reasoning_level_reader = _persisted_reasoning_mode bus.register_topic("app-composer", document.payload) async def publish(): @@ -211,6 +258,13 @@ async def set_reasoning_level(level): # there (see payload() above), so the UI disables this control # entirely; skip the apply step, never raise. + # The Settings dialog renders the same persisted value on its + # Ollama/Llama.cpp page. Without this it keeps showing the old + # one until something else republishes, which is the mirror + # image of the stale-composer bug this follow-up exists to fix. + if bus.has_topic("app-settings"): + await bus.publish("app-settings") + await publish() bus.register_intent("app-composer", "updateDraft", update_draft) diff --git a/backend/events.py b/backend/events.py index 661d63c..8869443 100644 --- a/backend/events.py +++ b/backend/events.py @@ -91,6 +91,21 @@ def register_intent(self, topic: str, intent: str, handler: IntentHandler) -> No assert key not in self._intents, f"intent {topic}/{intent} registered twice" self._intents[key] = handler + def has_topic(self, name: str) -> bool: + """True when `name` has a registered builder on this bus. + + Cross-topic publishing is an established pattern here (the composer + publishes "token-counter", the canvas publishes "notification"), but + publish() raises UnknownTopicError for an unregistered topic. In + production every topic is registered by _configure_session, so an + unconditional cross-publish is safe there; a focused unit test that + registers only ONE module's topics is where it would blow up. This + lets a cross-publisher say "notify that surface too, if it exists" + without either swallowing a real error or forcing every test to + register unrelated modules. + """ + return name in self._topics + # -- connections ------------------------------------------------------- def attach(self, conn: Connection) -> None: diff --git a/backend/settings.py b/backend/settings.py index 60877ff..04743da 100644 --- a/backend/settings.py +++ b/backend/settings.py @@ -140,6 +140,20 @@ def _locked_llama_cpp_settings(manager: SettingsManager) -> dict[str, Any]: return manager.get_llama_cpp_settings() +async def _republish_composer_reasoning(bus: SessionBus) -> None: + """Push a fresh composer snapshot after a reasoning-mode change here. + + The composer's own Reasoning control displays the SAME persisted value + these intents edit (backend/composer.py derives it rather than keeping a + private copy, matching legacy). It therefore has to be told to rebuild, + or the Settings dialog and the composer disagree until some unrelated + composer event happens to republish. Guarded because a focused + test_settings.py bus registers no composer topic. + """ + if bus.has_topic("app-composer"): + await bus.publish("app-composer") + + async def apply_ollama_reasoning_mode(manager: SettingsManager, mode: str) -> None: """Persist `mode` and, if Ollama is still the live provider, re-apply it to api_provider's module state so the very next chat()/chat_stream() call @@ -719,6 +733,7 @@ async def set_ollama_reasoning_mode(mode: str): return await apply_ollama_reasoning_mode(manager, mode) await bus.publish("app-settings") + await _republish_composer_reasoning(bus) async def set_ollama_model_assignment(task: str, value: str): # Coerced before use, matching every intent in this file post the @@ -934,6 +949,7 @@ async def set_llama_cpp_reasoning_mode(mode: str): # success either); do not "improve" this as a drive-by fix in this # increment. await bus.publish("app-settings") + await _republish_composer_reasoning(bus) async def set_llama_cpp_chat_format(chat_format: str): await asyncio.to_thread(_apply, manager.set_llama_cpp_chat_format, str(chat_format)) diff --git a/backend/tests/test_backend_composer.py b/backend/tests/test_backend_composer.py index b611c78..8e477fa 100644 --- a/backend/tests/test_backend_composer.py +++ b/backend/tests/test_backend_composer.py @@ -11,6 +11,7 @@ from backend.composer import ComposerDocument, ComposerError, register_composer from backend.events import SessionBus from backend.notifications import NotificationState, register_notifications +from backend.settings import register_settings from backend.token_counter import TokenCounterState, estimate_tokens, register_token_counter @@ -307,3 +308,117 @@ async def run(): assert recorder.topics_seen().count("notification") == 1 asyncio.run(run()) + + +# -- R7.5d follow-up: the composer must DISPLAY the same persisted reasoning +# mode it writes. The first R7.5d pass wired only the write half, leaving the +# composer's own private reasoning_level as the display source - and since +# get_ollama_reasoning_mode() defaults to "Thinking" while that field defaults +# to "quick", a user who had never touched the setting saw the composer report +# "Quick Mode (No CoT)" while every chat call really ran with think=True. + + +def _make_bus_with_composer_and_settings(settings_manager): + bus = SessionBus("composer-settings-sync-test") + counter = register_token_counter(bus) + notifications = register_notifications(bus) + composer = register_composer(bus, counter, settings_manager, notifications) + register_settings(bus, settings_manager, notifications) + recorder = Recorder() + bus.attach(recorder) + return bus, composer, recorder + + +def test_composer_reasoning_level_derives_from_the_persisted_setting(tmp_path, monkeypatch): + monkeypatch.setattr(api_provider, "is_local_ollama_mode", lambda: True) + monkeypatch.setattr(api_provider, "is_local_llama_cpp_mode", lambda: False) + manager = SettingsManager(tmp_path / "session.dat") + + # The exact startup case that was wrong: never-touched settings, whose + # getter defaults to "Thinking", against a composer defaulting to "quick". + assert manager.get_ollama_reasoning_mode() == "Thinking" + _, composer, _ = _make_bus_with_composer_and_settings(manager) + assert composer.payload()["route"]["reasoning"]["level"] == "thinking" + assert composer.payload()["route"]["reasoning"]["label"] == "Thinking Mode (Enable CoT)" + + manager.set_ollama_reasoning_mode("Quick") + assert composer.payload()["route"]["reasoning"]["level"] == "quick" + + +def test_composer_reasoning_level_follows_llama_cpp_when_llama_cpp_is_active(tmp_path, monkeypatch): + monkeypatch.setattr(api_provider, "is_local_ollama_mode", lambda: False) + monkeypatch.setattr(api_provider, "is_local_llama_cpp_mode", lambda: True) + manager = SettingsManager(tmp_path / "session.dat") + manager.set_ollama_reasoning_mode("Thinking") + manager.set_llama_cpp_reasoning_mode("Quick") + + _, composer, _ = _make_bus_with_composer_and_settings(manager) + # Must read the ACTIVE provider's setting, not whichever getter is first. + assert composer.payload()["route"]["reasoning"]["level"] == "quick" + + +def test_bare_composer_document_still_uses_its_own_fallback_level(monkeypatch): + monkeypatch.setattr(api_provider, "is_local_ollama_mode", lambda: True) + monkeypatch.setattr(api_provider, "is_local_llama_cpp_mode", lambda: False) + # No reader wired (no SettingsManager) - the local field is the source, + # which is what keeps register_composer(bus, counter) usable in tests. + document = ComposerDocument() + assert document.payload()["route"]["reasoning"]["level"] == "quick" + document.set_reasoning_level("thinking") + assert document.payload()["route"]["reasoning"]["level"] == "thinking" + + +def test_settings_reasoning_change_republishes_the_composer(tmp_path, monkeypatch): + monkeypatch.setattr(api_provider, "is_local_ollama_mode", lambda: True) + monkeypatch.setattr(api_provider, "is_local_llama_cpp_mode", lambda: False) + monkeypatch.setattr(api_provider, "initialize_local_provider", lambda *a, **k: None) + manager = SettingsManager(tmp_path / "session.dat") + manager.set_ollama_reasoning_mode("Quick") + + async def run(): + bus, composer, recorder = _make_bus_with_composer_and_settings(manager) + recorder.messages.clear() + await bus.dispatch_intent("app-settings", "setOllamaReasoningMode", ["Thinking"]) + return composer, recorder + + composer, recorder = asyncio.run(run()) + assert composer.payload()["route"]["reasoning"]["level"] == "thinking" + # Both surfaces render this value, so both have to be told to rebuild. + assert recorder.topics_seen().count("app-settings") == 1 + assert recorder.topics_seen().count("app-composer") == 1 + + +def test_composer_reasoning_change_republishes_settings(tmp_path, monkeypatch): + monkeypatch.setattr(api_provider, "is_local_ollama_mode", lambda: True) + monkeypatch.setattr(api_provider, "is_local_llama_cpp_mode", lambda: False) + monkeypatch.setattr(api_provider, "initialize_local_provider", lambda *a, **k: None) + manager = SettingsManager(tmp_path / "session.dat") + + async def run(): + bus, _, recorder = _make_bus_with_composer_and_settings(manager) + recorder.messages.clear() + await bus.dispatch_intent("app-composer", "setReasoningLevel", ["quick"]) + return recorder + + recorder = asyncio.run(run()) + assert manager.get_ollama_reasoning_mode() == "Quick" + assert recorder.topics_seen().count("app-composer") == 1 + assert recorder.topics_seen().count("app-settings") == 1 + + +def test_composer_reasoning_republish_is_skipped_when_settings_is_not_registered(tmp_path, monkeypatch): + # The guard that keeps focused unit tests (and any future composer-only + # bus) from tripping UnknownTopicError on the cross-topic publish. + monkeypatch.setattr(api_provider, "is_local_ollama_mode", lambda: True) + monkeypatch.setattr(api_provider, "is_local_llama_cpp_mode", lambda: False) + monkeypatch.setattr(api_provider, "initialize_local_provider", lambda *a, **k: None) + manager = SettingsManager(tmp_path / "session.dat") + + async def run(): + bus, _, _, recorder = _make_bus_with_settings(manager) # no register_settings + await bus.dispatch_intent("app-composer", "setReasoningLevel", ["quick"]) + return recorder + + recorder = asyncio.run(run()) + assert recorder.topics_seen().count("app-composer") == 1 + assert "app-settings" not in recorder.topics_seen()