Skip to content

Hide Gemma models reasoning tags#7948

Merged
qnixsynapse merged 3 commits into
janhq:mainfrom
kirsenn:hide-gemma4-reasoning-tags
Apr 16, 2026
Merged

Hide Gemma models reasoning tags#7948
qnixsynapse merged 3 commits into
janhq:mainfrom
kirsenn:hide-gemma4-reasoning-tags

Conversation

@kirsenn
Copy link
Copy Markdown
Contributor

@kirsenn kirsenn commented Apr 11, 2026

Describe Your Changes

Fixed an issue where reasoning/thought tags (e.g., <thought>...</thought>) remained visible in the chat UI during the streaming of a new response, despite being correctly hidden in the message history.

The issue was caused by the extractReasoningMiddleware not correctly utilizing the model-specific tagName during the streaming process. I updated the logic to ensure that reasoning tags are properly detected and extracted based on the current model configuration in real-time, ensuring a consistent user experience between active streaming and message history.

Before: <thought> tags were rendered as plain text in the UI during the first response of a new chat.
After: <thought> tags are correctly extracted and hidden/collapsed during streaming, matching the behavior of the message history.

Fixes Issues

image

@tokamak-pm
Copy link
Copy Markdown

tokamak-pm Bot commented Apr 12, 2026

Code Review — first review

Summary

Fixes an issue where Gemma models' <thought> reasoning tags were rendered as plain text during streaming. The PR extends the reasoning tag parser to handle both <think> and <thought> tags, and dynamically selects the correct tag in extractReasoningMiddleware based on the model ID.

What works well

  • Regex approach is correct: Using <(think|thought)> with backreference correctly matches opening/closing pairs. Capture group index updates ([1][2]) are properly handled throughout.
  • Targeted fix: Two files, 22 additions — well-scoped for the problem.

Issues

  1. Tag-name logic duplicated 4×modelId.toLowerCase().includes('gemma') ? 'thought' : 'think' is copy-pasted across four factory methods in model-factory.ts. Extract to a helper:

    function getReasoningTagName(modelId: string): string {
      return modelId.toLowerCase().includes('gemma') ? 'thought' : 'think'
    }
  2. Fragile model detection — Hard-coding 'gemma' means any future model that uses <thought> tags (or a different tag) will need another condition. Consider making the tag name part of model metadata or a configurable mapping instead of string-matching the model ID.

  3. No testsparseReasoning and convertThreadMessageToUIMessage now handle a new tag variant but have no test coverage for <thought> inputs. Adding unit tests would prevent regressions.

  4. New middleware on previously-unwrapped path — The last change in model-factory.ts adds extractReasoningMiddleware + wrapLanguageModel to a code path that previously returned the model unwrapped. Verify that non-reasoning models served through this path (e.g., embedding models, non-Gemma models that don't emit <think>) aren't affected by the middleware intercepting their output.

Recommendation: improve needed

The fix is correct and addresses a real user-facing bug, but the duplication and hardcoded detection should be cleaned up before merging. Tests should be added to cover the new <thought> tag handling.

@kirsenn kirsenn force-pushed the hide-gemma4-reasoning-tags branch 2 times, most recently from 0d069b7 to 0d585d5 Compare April 14, 2026 13:40
@kirsenn
Copy link
Copy Markdown
Contributor Author

kirsenn commented Apr 14, 2026

Code Review — first review

Recommendation: improve needed

The fix is correct and addresses a real user-facing bug, but the duplication and hardcoded detection should be cleaned up before merging. Tests should be added to cover the new <thought> tag handling.

Please check new 2 commits with no hardcode and tests added

@tokamak-pm
Copy link
Copy Markdown

tokamak-pm Bot commented Apr 15, 2026

Code Review — follow-up review

New commits since last review

  • aa4c5b4c — Update reasoning parser and UI message converter for <think> and <thought> tags
  • 1a515210 — refactor(model-factory): extract reasoning tag logic to helper function
  • 0d585d59 — test(web-app): add unit tests for parseReasoning

The PR was fully rewritten as three clean atomic commits. Analysis below.

Previous feedback addressed

# Concern Status
1 Tag-name logic duplicated 4x Resolved1a515210 introduces REASONING_TAG_MAP, DEFAULT_REASONING_TAG, and getReasoningTagName() helper. All four call sites now use getReasoningTagName(modelId) instead of the inline ternary. This is exactly the refactoring suggested.
2 Fragile model detection (hardcoded 'gemma') Partially addressed — The REASONING_TAG_MAP is a clear improvement: adding a new model-tag mapping is now a single-line addition. However, the mapping is still in-code rather than in model metadata. This is acceptable for now — the map is easy to extend, and the data-driven approach is a reasonable middle ground.
3 No tests Resolved0d585d59 adds comprehensive parseReasoning tests covering: no-tag text, open <think>, open <thought>, closed <think>, closed <thought>, text-before-tag, analysis channel (open + closed), and multiple-tag edge case. Good coverage.
4 New middleware on previously-unwrapped path Acknowledged — The last factory method (openAICompatible.languageModel) now wraps with extractReasoningMiddleware. This is intentional per the commit message in aa4c5b4c. The middleware only activates when it encounters the configured tag in the stream, so non-reasoning models should pass through unaffected. Acceptable.

New observations

  1. DEFAULT_REASONING_TAG constant — Nice touch in 0d585d59. The magic string 'think' is now a named constant, improving readability.

  2. Regex correctness — The <(think|thought)> patterns with \1 backreference are correct. Capture group indices are properly updated from [1] to [2] in all three regex match sites in messages.ts.

  3. Missing newline at end of messages.test.ts — The test file is missing a trailing newline (the diff shows \ No newline at end of file). Minor nit but worth fixing to keep linters happy.

  4. No test for getReasoningTagName helper — The new helper function itself has no dedicated unit test. The parseReasoning tests cover the parser side, but a quick test for getReasoningTagName('gemma-3-27b') returning 'thought' and getReasoningTagName('llama-4') returning 'think' would nail down the mapping behavior directly. Non-blocking but recommended.

Recommendation: can merge

@qnixsynapse
Copy link
Copy Markdown
Contributor

Please rebase

kirsenn added 3 commits April 16, 2026 09:22
…h `<think>` and `<thought>` tags. This ensures that reasoning blocks from models using the `<thought>` format are correctly identified and displayed in the user interface.
Introduces a `REASONING_TAG_MAP` and a `getReasoningTagName` helper function to centralize the logic for determining reasoning tags based on model IDs. This replaces repeated inline ternary checks across the `ModelFactory` class, improving maintainability and making it easier to add support for new models with specific reasoning tags.
Add comprehensive tests for the `parseReasoning` utility to ensure correct extraction of reasoning segments across various formats, including `<think>`, `<thought>`, and analysis channel tags.

Additionally, refactor `model-factory.ts` to extract the default reasoning tag into a constant `DEFAULT_REASONING_TAG` and add descriptive documentation for better maintainability.
@kirsenn kirsenn force-pushed the hide-gemma4-reasoning-tags branch from 0d585d5 to 09680d3 Compare April 16, 2026 06:33
@kirsenn
Copy link
Copy Markdown
Contributor Author

kirsenn commented Apr 16, 2026

Please rebase

done

@qnixsynapse qnixsynapse merged commit def32f8 into janhq:main Apr 16, 2026
18 checks passed
@github-project-automation github-project-automation Bot moved this to QA in Jan Apr 16, 2026
fansilas pushed a commit to fansilas/jan that referenced this pull request Apr 23, 2026
* Update the reasoning parser and UI message converter to recognize both `<think>` and `<thought>` tags. This ensures that reasoning blocks from models using the `<thought>` format are correctly identified and displayed in the user interface.

* refactor(model-factory): extract reasoning tag logic to helper function

Introduces a `REASONING_TAG_MAP` and a `getReasoningTagName` helper function to centralize the logic for determining reasoning tags based on model IDs. This replaces repeated inline ternary checks across the `ModelFactory` class, improving maintainability and making it easier to add support for new models with specific reasoning tags.

* test(web-app): add unit tests for parseReasoning

Add comprehensive tests for the `parseReasoning` utility to ensure correct extraction of reasoning segments across various formats, including `<think>`, `<thought>`, and analysis channel tags.

Additionally, refactor `model-factory.ts` to extract the default reasoning tag into a constant `DEFAULT_REASONING_TAG` and add descriptive documentation for better maintainability.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: QA

Development

Successfully merging this pull request may close these issues.

bug: Reasoning tags visible during streaming in new chats for Gemma models

2 participants