Claude finished @tombii's task —— View job

Pull Request Review

Reviewing PR #916: feat: Add Ollama reasoning mode support for deepseek-r1 and similar models

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze version file changes
• Review LLM config implementation
• Examine default settings structure
• Review progress display fix
• Check for potential bugs and security issues
• Assess performance considerations
• Evaluate test coverage needs
• Provide comprehensive feedback

Review Feedback

✅ Code Quality & Implementation

Strong Points:

• Clean, well-documented implementation that follows existing patterns
• Proper error handling with debug logging in llm_config.py:719-721
• Consistent with existing Ollama parameter handling
• Good separation of concerns - reasoning logic is contained to the Ollama provider

Settings Structure:

• New llm.ollama.reasoning setting follows the established pattern in default_settings.json
• Appropriate UI element (checkbox) and sensible default (true)
• Good description explaining the feature's purpose

🔧 Technical Implementation

LLM Config Changes (src/local_deep_research/config/llm_config.py:709-721):

reasoning_mode = get_setting_from_snapshot(
 "llm.ollama.reasoning",
 True, # Default to True to exclude thinking from responses
 settings_snapshot=settings_snapshot,
)
if reasoning_mode is not None:
 ollama_params["reasoning"] = reasoning_mode

• Implementation is correct and follows existing parameter handling patterns
• Good default value (True) to prevent verbose thinking blocks in responses
• Proper integration with the settings system

Progress Display Fix (src/local_deep_research/web/static/js/components/progress.js:592-593):

const cappedProgress = Math.max(0, Math.min(100, Math.floor(data.progress)));

• Simple and effective fix for the progress display bug
• Prevents display values exceeding 100%

🚨 Potential Issues

1. Missing Category in Settings: The new setting has "category": null in default_settings.json. Consider adding it to a relevant category like "llm" for better organization in the UI.

2. No Input Validation: While reasoning_mode is checked for None, there's no validation that it's a boolean value. Consider adding type validation.

3. Test Coverage: No tests are included for the new functionality. Consider adding:

   • Unit tests for the reasoning parameter being passed correctly
   • Integration tests with a mock Ollama response containing reasoning content

🔒 Security Considerations

• Low Risk: The reasoning parameter is a boolean setting that doesn't involve user input or file operations
• Settings are properly retrieved through the established get_setting_from_snapshot mechanism
• No new attack vectors introduced

⚡ Performance Considerations

• Minimal Impact: Adding one boolean parameter to Ollama requests has negligible performance impact
• Positive Impact: When enabled, this actually improves performance by reducing response size (excluding thinking content)
• Settings retrieval is cached appropriately

📋 Suggested Improvements

1. Add setting category:

"llm.ollama.reasoning": {
 "category": "llm", // Add this
 "description": "...",
 // ... rest of config
}

2. Add type validation in llm_config.py:

reasoning_mode = get_setting_from_snapshot(
 "llm.ollama.reasoning",
 True,
 settings_snapshot=settings_snapshot,
)
if reasoning_mode is not None and isinstance(reasoning_mode, bool):
 ollama_params["reasoning"] = reasoning_mode

3. Consider adding tests for the new functionality, especially testing that:
   • The reasoning parameter is correctly passed to ChatOllama
   • The setting appears in the UI as expected
   • Default behavior works correctly

🎯 Overall Assessment

This is a well-implemented feature that addresses a specific need for reasoning models like deepseek-r1. The code is clean, follows existing patterns, and includes a useful bug fix for progress display.

Recommendation: ✅ Approve with minor suggestions

The implementation is solid and ready for merge. The suggested improvements are minor enhancements that could be addressed in follow-up PRs if desired.

Isnt this done in the wrapper already?

the PR is still draft or?

Isnt this done in the wrapper already?

local-deep-research/src/local_deep_research/config/llm_config.py

Line 874 in b3eb86f

"""Create a wrapper class that processes LLM outputs with remove_think_tags and token counting"""

Yes you are right but Ollama I was still seeing </think> that we need to filter out.