From 7c3a4da87c1b35e81d73b29553282582aebdf73b Mon Sep 17 00:00:00 2001 From: dovvnloading Date: Sun, 26 Jul 2026 17:35:26 -0400 Subject: [PATCH] R7.5d second pass: the composer must DISPLAY what it writes An adversarial re-audit of the shipped R7.5d diff found the fix was half a fix. The first pass wired the write path (composer toggle -> SettingsManager -> api_provider) but left the composer's own private reasoning_level field as the display source, so the two halves disagreed. That divergence was arguably worse than the original bug. SettingsManager.get_ollama_reasoning_mode() defaults to "Thinking" while DEFAULT_REASONING_LEVEL is "quick", so any user who had never touched the setting - the default state, not an edge case - saw the composer confidently report "Quick Mode (No CoT)" while every chat call really ran with think=True. Reproduced against the real session.dat: persisted Thinking, live OLLAMA_REASONING_MODE Thinking, composer displaying quick. The first pass's own live browser check looked directly at that wrong label and recorded it as evidence the increment worked. Both sync directions were broken too, measured rather than assumed. A composer change persisted correctly but published only app-composer, so an open Settings dialog stayed stale. A Settings change did not move the composer's label at all, and no amount of republishing could have fixed it, because the composer read a private field rather than the shared setting. The fix removes the second source of truth instead of trying to keep two in sync. ComposerDocument gains an optional reasoning_level_reader that register_composer wires to the active provider's persisted getter, branching on is_local_llama_cpp_mode() exactly as legacy's _reasoning() did - legacy never stored a composer-local reasoning level either, which is precisely why the Qt composer and Settings dialog could never diverge. The private field survives only as the fallback for a bare ComposerDocument() with no manager, keeping register_composer(bus, counter) valid in unit tests. Both reasoning intents now cross-publish the other surface's topic, guarded by a new SessionBus.has_topic(). Cross-topic publishing is already an established pattern here - the composer publishes token-counter, the canvas publishes notification - but publish() raises UnknownTopicError on an unregistered topic, which a focused single-module test bus would hit. Re-verified: all three reproductions now pass. Startup label matches disk, composer -> settings publishes both topics, settings -> composer moves the composer's level. Confirmed live in the browser as well - the very reading that was wrong before now shows "Thinking Mode (Enable CoT)" against a persisted Thinking. 6 new regression tests: derivation from the persisted setting, the llama.cpp-active branch, the bare-document fallback, both cross-publish directions, and the has_topic guard. 880 backend / 1204 frontend, lint/typecheck/build clean, burn-down gate unchanged at 152/84/68. Process note worth keeping: the first pass had a design-synthesis agent, an adversarial reviewer, and a live browser drive, and all three missed this. Each was scoped to "is the write path correct?" - the question the increment posed - rather than "is the feature correct?". A bidirectional binding needs both directions named in the review scope explicitly, or the unexamined direction survives every check. Co-Authored-By: Claude Opus 5 --- backend/composer.py | 62 ++++++++++++- backend/events.py | 15 ++++ backend/settings.py | 16 ++++ backend/tests/test_backend_composer.py | 115 +++++++++++++++++++++++++ 4 files changed, 204 insertions(+), 4 deletions(-) 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()