Qt-removal R7.5d follow-up: the composer must DISPLAY what it writes#153
Merged
Merged
Conversation
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 <noreply@anthropic.com>
This was referenced Jul 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes #152, which wired the composer's Thinking Mode toggle to persist and re-apply to
api_providerbut left the display side reading a separate, unsynchronized field.Problem
ComposerDocumentkept its ownreasoning_level(default"quick") whileSettingsManager.get_ollama_reasoning_mode()defaults to"Thinking". The composer therefore showed the wrong mode until the user changed it:ollama_reasoning_modeThinkingapi_provider.OLLAMA_REASONING_MODEThinkingQuick Mode (No CoT)The user sees "Quick" while the model runs chain-of-thought, and selecting the already-active "Quick" is the only way to reach the displayed state.
The two surfaces also never notified each other: a composer change published only
app-composer, leaving an open Settings dialog stale; a Settings change did not update the composer at all.Change
ComposerDocumenttakes an optionalreasoning_level_reader, wired byregister_composerto the active provider's persisted getter (branching onis_local_llama_cpp_mode()). This matches legacy_reasoning()(graphlink_composer_bridge.py:464-474), which derived the level rather than storing one — the reason the Qt composer and Settings dialog could not diverge. The local field remains as the fallback when noSettingsManageris supplied, keepingregister_composer(bus, counter)valid in tests.SessionBus.has_topic(). Cross-topic publishing is already used here (composer →token-counter, canvas →notification), butpublish()raisesUnknownTopicErrorfor unregistered topics, which single-module test buses would hit.Test plan
has_topicguard)~/.graphlink/session.dat: startup label matches the persisted value; composer → settings publishes['app-settings','app-composer']; settings → composer updates the composer's levelThinkingpersisted