fix(memory): handle Ollama version errors during model pull #760
Conversation
|
|
📝 WalkthroughWalkthroughAdds Ollama version-awareness: utilities to parse and compare versions, fetch server version, annotate recommended embedding models with minimum required Ollama versions, validate compatibility before pulling models, and surface version-related streaming errors during pulls. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI/CLI
participant Backend as Backend (ollama_model_detector)
participant Ollama as Ollama Server
note over Backend,Ollama `#E6F0FF`: Version checks + streaming error handling
UI->>Backend: request pull model (model_name)
Backend->>Ollama: GET /version (get_ollama_version)
Ollama-->>Backend: returns version or none
alt version known
Backend->>Backend: min_version = get_model_min_version(model_name)
Backend->>Backend: compatible = version_gte(ollama_version, min_version)
alt compatible
Backend->>Ollama: POST /models/pull (start NDJSON stream)
Ollama-->>Backend: NDJSON chunks (progress / success / error)
alt chunk contains "error"
Backend-->>UI: translated upgrade/error message & abort
else progress/completion
Backend-->>UI: forward progress / success
end
else incompatible
Backend-->>UI: return upgrade-required compatibility_note
end
else version unknown
Backend->>Ollama: POST /models/pull (fallback stream)
Ollama-->>Backend: NDJSON chunks
Backend-->>UI: forward progress or translate errors if present
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom Pre-merge Checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)apps/backend/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📓 Common learnings🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @bbopen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's interaction with Ollama, particularly concerning embedding model management. It introduces comprehensive version compatibility checks and improved error reporting, ensuring users are proactively informed about Ollama version requirements for specific models. Beyond Ollama, the PR also includes critical stability fixes for Windows users, such as resolving GPU cache issues and refining terminal shell detection. Furthermore, the release process has been updated to enforce stricter changelog maintenance, contributing to more reliable and transparent future releases. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses silent failures when pulling Ollama models with version incompatibilities. The introduction of version checking, both as a pre-flight check and within the streaming response, is a solid improvement that will enhance user experience by providing clear error messages. The related changes to cmd_check_status and cmd_get_recommended_models are thoughtful additions that help the UI inform users proactively. Additionally, the PR includes several valuable cross-platform compatibility fixes and improvements to the release process. The overall code quality is high. I have one suggestion to improve the robustness of model name matching logic.
| def get_model_min_version(model_name: str) -> str | None: | ||
| """Get the minimum Ollama version required for a model.""" | ||
| name_lower = model_name.lower() | ||
|
|
||
| for known_model, info in KNOWN_EMBEDDING_MODELS.items(): | ||
| if known_model in name_lower: | ||
| return info.get("min_version") | ||
|
|
||
| return None |
There was a problem hiding this comment.
The use of if known_model in name_lower: can be fragile. If one model name is a substring of another (e.g., model and model:large), the iteration order of the dictionary determines which one is matched first. To make this more robust and ensure the most specific model name is matched, you should iterate over the keys sorted by length in descending order. This same logic would also improve the get_embedding_dim and get_embedding_description functions, which are not part of this diff but follow the same pattern.
| def get_model_min_version(model_name: str) -> str | None: | |
| """Get the minimum Ollama version required for a model.""" | |
| name_lower = model_name.lower() | |
| for known_model, info in KNOWN_EMBEDDING_MODELS.items(): | |
| if known_model in name_lower: | |
| return info.get("min_version") | |
| return None | |
| def get_model_min_version(model_name: str) -> str | None: | |
| """Get the minimum Ollama version required for a model.""" | |
| name_lower = model_name.lower() | |
| # Sort keys by length descending to match more specific names first | |
| # e.g., "qwen3-embedding:8b" before "qwen3-embedding" | |
| for known_model in sorted(KNOWN_EMBEDDING_MODELS.keys(), key=len, reverse=True): | |
| if known_model in name_lower: | |
| return KNOWN_EMBEDDING_MODELS[known_model].get("min_version") | |
| return None |
bbopen
force-pushed
the
fix/ollama-version-error-handling
branch
from
18004f2 to
a3a1921
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/release.yml (1)
568-584: Consider DRY improvement for AWK script duplication.The changelog extraction AWK script is duplicated between
release.yml(lines 527-548) andprepare-release.yml(lines 89-111). Consider extracting this to a shared script file to reduce maintenance burden and ensure consistent parsing logic.🔎 Suggested approach
Create
scripts/extract-changelog.sh:#!/bin/bash # Usage: ./scripts/extract-changelog.sh <version> VERSION="$1" awk -v ver="$VERSION" ' BEGIN { found=0; content="" } /^## / { if (found) exit if ($2 == ver || $2 ~ "^"ver"[[:space:]]*-") { found=1 next } } /^---$/ { if (found) exit } found { content = content $0 "\n" } END { if (!found) { print "NOT_FOUND"; exit 1 } gsub(/^[[:space:]]+|[[:space:]]+$/, "", content) print content } ' CHANGELOG.mdThen call from workflows:
CHANGELOG_CONTENT=$(./scripts/extract-changelog.sh "$VERSION")apps/backend/ollama_model_detector.py (1)
255-264: Inconsistent API response structure in fallback path.When falling back to
api/tags, the response omits thesupports_new_modelsfield that's present in the primary response (line 251). This inconsistency could cause issues for API consumers expecting the field.🔎 Proposed fix for consistent response structure
tags = fetch_ollama_api(base_url, "api/tags") if tags: output_json( True, data={ "running": True, "url": base_url, "version": "unknown", + "supports_new_models": None, }, )
🤖 Fix all issues with AI agents
In @apps/backend/merge/semantic_analysis/regex_analyzer.py:
- Around line 97-112: The helper extract_func_names lacks type hints; add
annotations to its signature and intermediate variables to clarify expected
types (e.g., annotate matches as Iterable[Union[str, Tuple[Optional[str],
...]]], return type as Set[str]) and ensure
func_pattern.findall(before_normalized) and
func_pattern.findall(after_normalized) call sites match those types; update
imports (typing: Iterable, Union, Tuple, Optional, Set) as needed and annotate
funcs_before and funcs_after accordingly.
In @apps/backend/ollama_model_detector.py:
- Around line 386-397: If ollama_version is None we should still surface that
compatibility couldn't be verified: when building the dict added to recommended
(the block that currently sets "installed": is_installed, "compatible":
is_compatible, "compatibility_note": ...), set "compatibility_note" to a short
message like "Ollama version unknown — compatibility not verified" whenever
ollama_version is falsy and min_version is present (while keeping existing
behavior when version is known/known-incompatible). Use the same symbols
(min_version, ollama_version, is_compatible, version_gte) so the UI can show a
warning even though cmd_pull_model performs pre-flight checks.
In @apps/frontend/src/main/index.ts:
- Around line 113-118: The current Windows-only workaround disables GPU caches
via process.platform and
app.commandLine.appendSwitch('disable-gpu-shader-disk-cache' /
'disable-gpu-program-cache'), which masks NTFS permission/user-data issues;
instead, implement a root-cause fix by ensuring the Electron user data directory
is writable (use app.setPath('userData', writableDir) early in startup or launch
with --user-data-dir), or proactively remove/recreate the GPU cache folder with
correct permissions before startup (use app.getPath('userData') to locate
cache), and only keep the appendSwitch calls as a documented last-resort
fallback—add a clear comment next to the app.commandLine.appendSwitch lines
explaining why disabling GPU cache is necessary for your deployment if you
choose to retain it.
- Around line 125-130: Remove the blanket session.defaultSession.clearCache()
call in the Windows startup block; since GPU shader/program caching is already
disabled via command-line switches, the full cache clear is redundant and slow.
If you still need targeted cleanup, replace it with session.clearStorageData({
storages: ['shadercache', 'cookies'] }) to limit scope or remove only the
GPUCache directory under app.getPath('userData') (GPUCache) rather than clearing
the whole session. Ensure any cleanup runs asynchronously off the critical
startup path (e.g., after BrowserWindow.show()) to avoid blocking startup.
In @apps/frontend/src/main/terminal/pty-manager.ts:
- Around line 77-79: Wrap the readSettingsFile() call in a try/catch and
validate the returned preferredTerminal before casting: call readSettingsFile()
inside try, extract settings?.preferredTerminal into a local variable, check
typeof pref === 'string' and that it matches a known SupportedTerminal (e.g. by
checking membership in WINDOWS_SHELL_PATHS or a SupportedTerminal enum), only
then assign preferredTerminal = pref as SupportedTerminal; on catch log or warn
(e.g. '[PtyManager] Failed to read terminal preference') and leave
preferredTerminal undefined.
- Around line 18-42: WINDOWS_SHELL_PATHS is currently typed as
Record<string,string[]> which allows typos and mismatches; change its type to
Partial<Record<SupportedTerminal,string[]>> so keys are validated against the
SupportedTerminal union (update the declaration of WINDOWS_SHELL_PATHS
accordingly) and then either add entries for the missing terminals (e.g.,
"conemu" and "cmder") to the map or add a comment/documentation explaining why
those SupportedTerminal values are intentionally omitted.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/beta-release.yml.github/workflows/prepare-release.yml.github/workflows/release.ymlCHANGELOG.mdREADME.mdRELEASE.mdapps/backend/merge/file_merger.pyapps/backend/merge/semantic_analysis/regex_analyzer.pyapps/backend/merge/semantic_analyzer.pyapps/backend/ollama_model_detector.pyapps/frontend/scripts/download-python.cjsapps/frontend/src/main/index.tsapps/frontend/src/main/terminal/pty-manager.tsscripts/bump-version.js
🧰 Additional context used
📓 Path-based instructions (4)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/merge/semantic_analyzer.pyapps/backend/merge/file_merger.pyapps/backend/merge/semantic_analysis/regex_analyzer.pyapps/backend/ollama_model_detector.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/merge/semantic_analyzer.pyapps/backend/merge/file_merger.pyapps/backend/merge/semantic_analysis/regex_analyzer.pyapps/backend/ollama_model_detector.py
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/index.tsapps/frontend/src/main/terminal/pty-manager.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/index.tsapps/frontend/src/main/terminal/pty-manager.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/index.tsapps/frontend/src/main/terminal/pty-manager.ts
🧠 Learnings (1)
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: When submitting PRs to the upstream AndyMik90/Auto-Claude repository, always target the `develop` branch, not `main`
Applied to files:
RELEASE.md
🧬 Code graph analysis (2)
apps/backend/merge/file_merger.py (2)
apps/frontend/scripts/download-python.cjs (2)
lines(568-568)content(567-567)scripts/bump-version.js (2)
content(146-146)content(161-161)
apps/backend/ollama_model_detector.py (1)
scripts/bump-version.js (1)
match(66-66)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~71-~71: Ensure spelling is correct
Context: ... to prefer versioned Python over system python3 - Added support for Bun 1.2.0+ lock file f...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~186-~186: The official name of this software platform is spelled with a capital “H”.
Context: ...inux (#404) by @mitsu in 230de5f - fix(github): pass repo parameter to GHClient for e...
(GITHUB)
[uncategorized] ~194-~194: The official name of this software platform is spelled with a capital “H”.
Context: ...9) by @michael Ludlow in 68548e3 - fix(github): improve PR review with structured out...
(GITHUB)
[uncategorized] ~226-~226: The official name of this software platform is spelled with a capital “H”.
Context: ... path (#308) by @andy in c0a02a4 - fix(github): add augmented PATH env to all gh CLI ...
(GITHUB)
[uncategorized] ~233-~233: The official name of this software platform is spelled with a capital “H”.
Context: ...stage (#293) by @alex in 8416f30 - fix(github): add explicit GET method to gh api com...
(GITHUB)
[uncategorized] ~241-~241: The official name of this software platform is spelled with a capital “H”.
Context: ...) by @dependabot[bot] in 50dd107 - fix(github): resolve follow-up review API issues b...
(GITHUB)
[uncategorized] ~251-~251: The official name of this software platform is spelled with a capital “H”.
Context: ... by @dependabot[bot] in d4cad80 - feat(github): add automated PR review with follow-u...
(GITHUB)
[uncategorized] ~255-~255: The official name of this software platform is spelled with a capital “H”.
Context: ...tsu in f843811 - Revert "Feat/Auto Fix Github issues and do extensive AI PR reviews (...
(GITHUB)
[uncategorized] ~256-~256: The official name of this software platform is spelled with a capital “H”.
Context: ...1) by @andy in 5e8c530 - Feat/Auto Fix Github issues and do extensive AI PR reviews (...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (34)
apps/frontend/scripts/download-python.cjs (1)
613-614: Change appears unrelated to PR objectives.The PR objectives focus on Ollama version error handling and model pull improvements, but this change re-enables pip's cache for Python package installation during build. While the technical change is sound and the rationale is clear (preserving built wheels for packages requiring compilation), it doesn't align with the stated PR goals about Ollama compatibility checking.
Please verify this file should be included in this PR, or update the PR description to reflect build/caching improvements.
apps/backend/merge/semantic_analyzer.py (1)
214-225: LGTM! Cross-platform line-ending normalization is correctly implemented.The normalization ensures tree-sitter byte positions remain accurate across Windows (CRLF), Unix (LF), and old Mac (CR) platforms. The order of replacements is correct (CRLF before CR), and the normalized content is used consistently for both parsing and extraction.
apps/backend/merge/file_merger.py (2)
48-52: Good change!splitlines()correctly handles all line-ending styles.The change from
split("\n")tosplitlines()ensures robust handling of CRLF (Windows), LF (Unix), and CR (old Mac) line endings. Note that the output will always use LF line endings after the"\n".join()on line 52, which aligns with the normalization strategy used elsewhere in this PR (semantic_analyzer.py and regex_analyzer.py).
100-107: Good change! Consistent with the firstsplitlines()usage.This change mirrors the approach at lines 48-52, ensuring all import-insertion logic handles line endings consistently. The output will normalize to LF line endings, which aligns with the broader normalization strategy in this PR.
apps/backend/merge/semantic_analysis/regex_analyzer.py (1)
33-42: LGTM! Normalization aligns with semantic_analyzer.py.The line-ending normalization to LF ensures consistent diff generation across platforms. This approach matches the normalization strategy in semantic_analyzer.py (lines 214-217) and file_merger.py (splitlines usage).
README.md (1)
27-31: LGTM - Version updates are consistent.The stable download links are correctly updated to version 2.7.2 with proper URL patterns and file naming conventions matching the release format.
.github/workflows/beta-release.yml (4)
100-114: LGTM - Well-structured caching strategy for macOS Intel.The pip wheel cache and Python bundle cache are properly configured with:
- Architecture-specific keys (
x64)- Requirements hash for invalidation on dependency changes
- The
rustsuffix correctly reflects the Rust toolchain requirement for this job
192-206: LGTM - ARM64 caching properly differentiated.Cache keys correctly use
arm64suffix to prevent cache collisions between Intel and Apple Silicon builds.
284-298: Verify Windows pip cache path format.The Windows pip cache path uses backslashes (
~\AppData\Local\pip\Cache). While this typically works in GitHub Actions on Windows, forward slashes are more portable. However, sinceactions/cachehandles path normalization, this should work correctly.
362-376: LGTM - Linux caching configuration is correct.The pip wheel cache path
~/.cache/pipis the standard location for pip on Linux systems.CHANGELOG.md (2)
1-2: LGTM - Changelog entry format is correct.The version header
## 2.7.2 - Stability & Performance Enhancementsmatches the expected format that will be parsed by the release workflow validation (matching pattern^## X.Y.Zor^## X.Y.Z -).
149-149: Verify horizontal rule placement for changelog extraction.The
---delimiter at line 149 marks the end of the main changelog content. The AWK script inprepare-release.ymluses this as a termination marker. Ensure this delimiter consistently appears after the main content sections for proper extraction.scripts/bump-version.js (3)
152-171: LGTM - Changelog validation logic is well-implemented.The
checkChangelogEntryfunction correctly:
- Escapes dots in the version string for regex matching
- Uses multiline flag for proper header detection
- Returns early with warning if CHANGELOG.md doesn't exist
- Matches both
## X.Y.Zand## X.Y.Z -formats
222-248: Good user guidance for missing changelog entries.The detailed warning with a sample changelog template helps developers understand exactly what's needed. The visual separators make the warning highly visible without blocking the workflow, which is appropriate since the CI will enforce this validation.
260-281: Clear conditional next steps based on changelog status.The instructions appropriately differentiate between scenarios:
- Without changelog: prompts to update, amend commit, then push
- With changelog: standard review and push flow
The step numbering dynamically adjusts, keeping instructions accurate.
.github/workflows/release.yml (2)
50-64: LGTM - Caching configuration consistent with beta-release workflow.The pip wheel and Python bundle caching follows the same pattern established in
beta-release.yml, ensuring cache consistency across workflows.
509-566: Changelog extraction logic is robust with appropriate fallbacks.The AWK script correctly parses CHANGELOG.md by:
- Matching version headers (both
## X.Y.Zand## X.Y.Z -)- Capturing content until the next version header or
---delimiter- Using file-based output for reliable multiline handling
The fallback to minimal release notes (lines 550-554) is appropriate here since
prepare-release.ymlalready validates the changelog exists before tagging..github/workflows/prepare-release.yml (3)
72-112: Robust changelog validation with clear error messaging.The validation step correctly:
- Fails early if CHANGELOG.md is missing
- Uses AWK to extract version-specific content
- Provides detailed error messages with remediation steps
- Outputs both to console and job summary for visibility
One consideration: The AWK
exit 1on line 106 will cause the command substitution to return a non-zero exit code, but since you're capturing output and checking the string value at line 113, this works correctly. The shell doesn't fail because command substitution doesn't propagate exit codes by default.
113-156: Excellent error handling with actionable guidance.The detailed error block with both
::error::annotations and job summary additions ensures visibility regardless of how users view the workflow results. The "How to fix" section in the summary is particularly helpful.
177-187: Proper gating of downstream steps.Both artifact upload and tag creation are correctly gated on
changelog_valid == 'true', ensuring the release pipeline only proceeds when validation passes. This prevents creating tags for releases that would fail due to missing changelog entries.RELEASE.md (3)
75-101: Clear documentation for the new mandatory changelog requirement.The step-by-step instructions with the code block example and amend workflow are well-documented. The "REQUIRED" emphasis appropriately signals the importance of this step.
146-175: Comprehensive changelog management guidelines.The new section effectively documents:
- Expected format with emoji categories
- Validation behavior (blocks vs uses)
- Best practices for writing release notes
This aligns well with the validation logic in
prepare-release.yml.
207-221: Helpful troubleshooting section for changelog validation failures.The remediation steps are clear and actionable. Users encountering the "CHANGELOG VALIDATION FAILED" error will have a direct path to resolution.
apps/backend/ollama_model_detector.py (7)
27-29: LGTM!The constant is well-documented and the version format is consistent with the parsing logic.
39-42: LGTM!Version metadata is consistently applied to all qwen3-embedding variants.
123-134: Consider documenting the fallback behavior for unparseable versions.The
(0, 0, 0)fallback on parse failure means unparseable versions are treated as very old. This is generally safe since it causesversion_gte()to returnFalsewhen the actual version can't be parsed, preventing incompatible pulls. However, the inverse case (unparseablemin_version) would incorrectly returnTrue.Given that
min_versionvalues are hardcoded constants, this edge case is unlikely. The implementation is acceptable.
170-175: LGTM!Clean implementation with appropriate
Nonereturn for error cases.
225-233: LGTM!Consistent with existing helper patterns (
get_embedding_dim,get_embedding_description).
421-432: LGTM - Good pre-flight version check.Clear error message with actionable upgrade instructions. This prevents unnecessary network traffic for known incompatible models.
447-461: LGTM - Robust streaming error handling.Good approach to detect errors in NDJSON streaming responses. The whitespace normalization and special handling for version-related errors provides a good user experience. This directly addresses the issue of silent failures when pulling models.
apps/frontend/src/main/index.ts (1)
1-1: LGTM: Import addition is correct.The
sessionimport is properly added and used later in the file for cache management.apps/frontend/src/main/terminal/pty-manager.ts (3)
8-13: Imports look appropriate.The new imports support the shell preference logic correctly. However, ensure
readSettingsFile()is called safely (see comment on lines 77-79).
87-87: Enhanced logging is helpful for debugging.The additional logging of the
preferredTerminalsetting will make it easier to diagnose shell selection issues. The fallback to'system'for undefined preferences improves log readability.
47-66: Function logic is sound with proper fallbacks.The
getWindowsShellfunction correctly handles undefined preferences, unknown terminal types, and missing shell executables with appropriate fallbacks toCOMSPEC.
| # For JS/TS patterns with alternation, findall() returns tuples | ||
| # Extract the non-empty match from each tuple | ||
| def extract_func_names(matches): | ||
| names = set() | ||
| for match in matches: | ||
| if isinstance(match, tuple): | ||
| # Get the first non-empty group from the tuple | ||
| name = next((m for m in match if m), None) | ||
| if name: | ||
| names.add(name) | ||
| elif match: | ||
| names.add(match) | ||
| return names | ||
|
|
||
| funcs_before = extract_func_names(func_pattern.findall(before_normalized)) | ||
| funcs_after = extract_func_names(func_pattern.findall(after_normalized)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good fix for regex alternation patterns! Consider adding type hints to the helper.
The extract_func_names helper correctly handles the case where findall() returns tuples for patterns with alternation (e.g., (?:function\s+(\w+)|const\s+(\w+)...)). This is a subtle but important fix for JS/TS function detection.
Consider adding type hints to improve clarity:
🔎 Suggested type hint enhancement
- def extract_func_names(matches):
+ def extract_func_names(matches: list[tuple[str, ...] | str]) -> set[str]:
names = set()
for match in matches:Note: The actual type might be more specific depending on the regex patterns, but this provides a clearer signature.
🤖 Prompt for AI Agents
In @apps/backend/merge/semantic_analysis/regex_analyzer.py around lines 97 -
112, The helper extract_func_names lacks type hints; add annotations to its
signature and intermediate variables to clarify expected types (e.g., annotate
matches as Iterable[Union[str, Tuple[Optional[str], ...]]], return type as
Set[str]) and ensure func_pattern.findall(before_normalized) and
func_pattern.findall(after_normalized) call sites match those types; update
imports (typing: Iterable, Union, Tuple, Optional, Set) as needed and annotate
funcs_before and funcs_after accordingly.
| // Fix Windows GPU cache permission errors (0x5 Access Denied) | ||
| if (process.platform === 'win32') { | ||
| app.commandLine.appendSwitch('disable-gpu-shader-disk-cache'); | ||
| app.commandLine.appendSwitch('disable-gpu-program-cache'); | ||
| console.log('[main] Applied Windows GPU cache fixes'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Search the codebase for GPU cache or related Windows issues
rg -n -C3 "disable-gpu|gpu.*cache|0x5|Access Denied" --type=ts --type=md --type=txtRepository: AndyMik90/Auto-Claude
Length of output: 740
🏁 Script executed:
# Check for any documentation about Windows issues or GPU cache
fd -e md -e txt | xargs rg -l -i "windows.*gpu|gpu.*cache|electron.*windows" 2>/dev/null | head -20Repository: AndyMik90/Auto-Claude
Length of output: 152
🏁 Script executed:
# Look at the broader context of the main/index.ts file to understand PR objectives
wc -l apps/frontend/src/main/index.tsRepository: AndyMik90/Auto-Claude
Length of output: 100
🌐 Web query:
Electron disable-gpu-shader-disk-cache disable-gpu-program-cache Windows 0x5 permission error
💡 Result:
Error 0x5 = ERROR_ACCESS_DENIED. In Electron/Chromium this commonly appears when the GPU/shader cache cannot be moved/created because the app lacks write permission to the cache path (User Data / ShaderCache / GPUCache) or the files are locked by another process/AV. See similar Chrome/Electron errors where cache move/create fails. [1][2][3]
Quick fixes (try in order):
- Delete the shader/GPU cache folder for the app (e.g. %LOCALAPPDATA%\Google\Chrome\User Data\ShaderCache\GPUCache or your Electron app's user-data dir) and relaunch. This forces recreation. [1][3]
- Run the app with a writable user-data directory: start with --user-data-dir="C:\Path\To\Writable\Folder". That avoids protected locations. [1][2]
- Ensure the account can write to the cache folder (adjust NTFS permissions or run as Administrator) or move the app out of Program Files if it's trying to write there. [2][3]
- Temporarily disable software that might lock files (anti‑virus/backups) and retry. [1]
- If you want to avoid shader cache entirely, use flags like --disable-gpu-shader-disk-cache and --disable-gpu-program-cache, but these are workarounds (they may affect rendering/performance) — better to fix permissions. [2]
If you want, tell me the exact Electron app path and the full error log and I’ll give the exact command/permission change to apply.
Sources: [1] StackOverflow (Chrome "Unable to move the cache" / shader_disk_cache error). [2] Electron GitHub issues about GPU-disable behavior. [3] Guides showing ShaderCache/GPUCache locations and deletion steps.
🌐 Web query:
Electron Windows GPU cache permission errors official documentation
💡 Result:
Short answer — there isn’t a single “official” Electron doc titled “Windows GPU cache permission errors,” but the Electron project’s official sources note/cover the problem and workarounds:
- Known GPU-process / GPUCache problems when running from non‑local locations (network shares) — GitHub issue on electron/electron describing GPU process crashes tied to permission/launching locations. [1]
- Electron troubleshooting / installation docs (official site) that cover installation/permissions issues and environment flags (useful when diagnosing cache/permission problems or re-downloading binaries). [2]
- Microsoft / Windows support threads and community posts documenting the “GPU process isn’t usable. Goodbye.” symptom and common mitigations (move app to C: / run with --no-sandbox or --disable-gpu as temporary workarounds). [3]
If you want, I can open those pages and extract the exact error log lines and recommended fixes (e.g., remove GPU cache folder, ensure user-data-dir is writable, avoid running from network shares, or launch with --disable-gpu).
Sources:
[1] electron/electron issue — “GPU process crashes when electron app is started on a network share.”
[2] Electron docs — Installation / Troubleshooting.
[3] Microsoft Q&A thread — “GPU process isn't usable. Goodbye.”
Consider addressing root causes instead of disabling GPU caching.
These Electron switches (disable-gpu-shader-disk-cache and disable-gpu-program-cache) are valid but work only as a workaround for Windows GPU cache permission errors. Official sources and community documentation recommend prioritizing these fixes instead:
- Delete the GPU cache folder and let it recreate with proper permissions
- Use
--user-data-dirpointing to a writable directory (avoids protected locations) - Fix NTFS permissions on the cache directory or move the app outside Program Files
- Disable antivirus/backup software that may lock cache files
Disabling GPU caching is listed as a last-resort option due to potential rendering performance degradation. If this workaround is necessary for your deployment environment, add a comment explaining why the root permission issues cannot be resolved.
🤖 Prompt for AI Agents
In @apps/frontend/src/main/index.ts around lines 113 - 118, The current
Windows-only workaround disables GPU caches via process.platform and
app.commandLine.appendSwitch('disable-gpu-shader-disk-cache' /
'disable-gpu-program-cache'), which masks NTFS permission/user-data issues;
instead, implement a root-cause fix by ensuring the Electron user data directory
is writable (use app.setPath('userData', writableDir) early in startup or launch
with --user-data-dir), or proactively remove/recreate the GPU cache folder with
correct permissions before startup (use app.getPath('userData') to locate
cache), and only keep the appendSwitch calls as a documented last-resort
fallback—add a clear comment next to the app.commandLine.appendSwitch lines
explaining why disabling GPU cache is necessary for your deployment if you
choose to retain it.
| // Clear cache on Windows to prevent permission errors from stale cache | ||
| if (process.platform === 'win32') { | ||
| session.defaultSession.clearCache() | ||
| .then(() => console.log('[main] Cleared cache on startup')) | ||
| .catch((err) => console.warn('[main] Failed to clear cache:', err)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electron session.clearCache() performance impact Windows startup
💡 Result:
Short answer
- Calling session.clearCache() at app startup can add a noticeable startup delay on Windows (depends on cache size, disk I/O and what Chromium needs to delete). session.clearCache() is an async operation that still performs potentially expensive disk work under the hood, so doing it synchronously at launch can slow visible startup. [1][2]
Why
- Electron’s session.clearCache() clears Chromium’s HTTP/disk cache and resolves when that work completes. Large caches or slow disks mean more I/O and longer delays. [1]
- Chromium’s disk cache is designed for performance but clearing it requires file-system work and evictions that are not free; this is why full cache operations can be expensive. [2]
Practical recommendations
- Don’t call clearCache() on the critical path of first window/show. Run it after first paint / app-ready + BrowserWindow.show, or schedule it in background (setImmediate / setTimeout / worker). [1][3]
- If you only need specific data cleared, prefer session.clearStorageData({ storages: [...] }) to target types (cookies, serviceworkers, shadercache, etc.) rather than the whole HTTP cache. [1]
- Before clearing, check size with session.getCacheSize() and skip if small. [1]
- If you must force a fresh cache on first run, consider doing it only on first-launch / opt-in, or clear on a background thread/process so UI isn’t blocked.
- Test on target Windows configurations (HDD vs NVMe, AV scanning enabled) because antivirus/Windows Defender and slow disks amplify the cost.
Sources
- Electron session API (clearCache, clearStorageData, getCacheSize). [1]
- Chromium disk-cache design (explains disk I/O/effects of clearing cache). [2]
- Note about main-process blocking and long synchronous work affecting responsiveness. [3]
References
[1] Electron session docs — session.clearCache(), session.clearStorageData(), session.getCacheSize. [2] Chromium disk-cache design document. [3] Discussion: main-process synchronous work can block UI (Stack Overflow / Electron).
🌐 Web query:
Electron Windows GPU cache permission errors alternatives
💡 Result:
Short answer — if your Electron app on Windows is failing with permission errors for GPUCache, alternatives are:
-
Move the browser cache to a writable location before app ready
- Call app.setPath('sessionData' or 'userData' or 'cache', your chosen writable folder) early (before app 'ready'). This keeps Chromium/GPUCache out of protected dirs. [1]
-
Disable GPU/GL shader disk caching (software fallback)
- Use app.disableHardwareAcceleration() and/or app.commandLine.appendSwitch('disable-gpu') and related switches (e.g. disable-software-rasterizer, disable-gpu-shader-disk-cache) to avoid writing/reading GPU shader caches. Note switches must be appended early. [2][3]
-
Launch with an explicit user-data dir that you control
- Start Electron/your exe with --user-data-dir or set the sessionData path so all Chromium cache lands in a known writable folder. [1][2]
-
Remove or recreate problematic GPUCache on startup
- Detect/remove the app's GPUCache directory at startup (fs.rmdir/rm -rf) so Chromium rebuilds it with correct permissions. This is a common workaround when cache becomes invalid or owned by another user/process. [4]
-
Fix OS permissions / run as appropriate user
- Ensure the Windows account running the app has write permission to the cache/userData path (or install per-user rather than system-wide), or relocate cache to %LOCALAPPDATA%/Temp-like folder.
Notes and references:
- app.setPath and sessionData/cache guidance: Electron docs. [1]
- How to append Chromium flags from Electron (disable-gpu, etc.): Electron commandLine docs; Chromium flags include disable-gpu-shader-disk-cache. [2][3]
- Discussion/issues about Electron writing cache into userData and cache corruption workarounds (delete GPUCache): community/issue threads. [4]
If you want, tell me your Electron version and where GPUCache is being created and I’ll give the exact code to set sessionData or append switches before app ready.
🏁 Script executed:
# Get more context around the cache clearing code
fd "index.ts" apps/frontend/src/main | head -5Repository: AndyMik90/Auto-Claude
Length of output: 269
🏁 Script executed:
# Check the full file to understand the context better
wc -l apps/frontend/src/main/index.tsRepository: AndyMik90/Auto-Claude
Length of output: 100
🏁 Script executed:
# Get context around the cache clearing code (lines 125-130)
sed -n '115,140p' apps/frontend/src/main/index.tsRepository: AndyMik90/Auto-Claude
Length of output: 959
🏁 Script executed:
# Search for any related GPU or cache-related issues in comments
grep -n -i "cache\|gpu\|permission" apps/frontend/src/main/index.ts | head -20Repository: AndyMik90/Auto-Claude
Length of output: 597
Remove or replace the blanket session cache clearing—GPU caching is already disabled, making this redundant and costly.
The code disables GPU shader and program caching via command-line switches (lines 115–116), which already prevents the permission errors this cache-clear is attempting to work around. Calling session.defaultSession.clearCache() on every startup is redundant and adds measurable startup delay on Windows, especially on slower disks or with antivirus scanning enabled.
Instead:
- Remove the clearCache() call entirely if GPU caching is already disabled—the permission issues should not occur.
- If cache clearing is still needed, use
session.clearStorageData({ storages: ['shadercache', 'cookies'] })to target only specific storage types instead of the entire session cache. - If GPUCache corruption is the issue, delete only the GPUCache directory at startup:
// Targeted approach: clear only GPUCache, not entire session const gpuCachePath = path.join(app.getPath('userData'), 'GPUCache'); if (fs.existsSync(gpuCachePath)) { fs.rmSync(gpuCachePath, { recursive: true, force: true }); }
- Avoid blocking startup—if cache clearing is retained, run it after
BrowserWindow.show()rather than in the critical startup path.
🤖 Prompt for AI Agents
In @apps/frontend/src/main/index.ts around lines 125 - 130, Remove the blanket
session.defaultSession.clearCache() call in the Windows startup block; since GPU
shader/program caching is already disabled via command-line switches, the full
cache clear is redundant and slow. If you still need targeted cleanup, replace
it with session.clearStorageData({ storages: ['shadercache', 'cookies'] }) to
limit scope or remove only the GPUCache directory under app.getPath('userData')
(GPUCache) rather than clearing the whole session. Ensure any cleanup runs
asynchronously off the critical startup path (e.g., after BrowserWindow.show())
to avoid blocking startup.
| const WINDOWS_SHELL_PATHS: Record<string, string[]> = { | ||
| powershell: [ | ||
| 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', // PowerShell 7 (Core) | ||
| 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', // Windows PowerShell 5.1 | ||
| ], | ||
| windowsterminal: [ | ||
| 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', // Prefer PowerShell Core in Windows Terminal | ||
| 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', | ||
| ], | ||
| cmd: [ | ||
| 'C:\\Windows\\System32\\cmd.exe', | ||
| ], | ||
| gitbash: [ | ||
| 'C:\\Program Files\\Git\\bin\\bash.exe', | ||
| 'C:\\Program Files (x86)\\Git\\bin\\bash.exe', | ||
| ], | ||
| cygwin: [ | ||
| 'C:\\cygwin64\\bin\\bash.exe', | ||
| 'C:\\cygwin\\bin\\bash.exe', | ||
| ], | ||
| msys2: [ | ||
| 'C:\\msys64\\usr\\bin\\bash.exe', | ||
| 'C:\\msys32\\usr\\bin\\bash.exe', | ||
| ], | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine SupportedTerminal type definition
rg "type SupportedTerminal" --type=ts -A 5Repository: AndyMik90/Auto-Claude
Length of output: 555
🏁 Script executed:
#!/bin/bash
# Examine the pty-manager.ts file to understand imports and type usage
cat -n apps/frontend/src/main/terminal/pty-manager.ts | head -50Repository: AndyMik90/Auto-Claude
Length of output: 2103
🏁 Script executed:
#!/bin/bash
# Search for WINDOWS_SHELL_PATHS usage to understand how it's accessed
rg "WINDOWS_SHELL_PATHS" --type=ts -B 2 -A 2Repository: AndyMik90/Auto-Claude
Length of output: 913
🏁 Script executed:
#!/bin/bash
# Get the full SupportedTerminal type definition
rg "type SupportedTerminal" --type=ts -A 30 apps/frontend/src/shared/types/settings.tsRepository: AndyMik90/Auto-Claude
Length of output: 1374
Refactor WINDOWS_SHELL_PATHS typing to improve type safety and prevent mismatches.
The keys should use Partial<Record<SupportedTerminal, string[]>> instead of Record<string, string[]> to ensure they are validated against the SupportedTerminal type at compile time, preventing potential typos and future misalignment as the type evolves.
Additionally, note that conemu and cmder are Windows terminals in SupportedTerminal but lack entries in this map; consider adding them or documenting why they are unsupported.
Suggested refactor
-const WINDOWS_SHELL_PATHS: Record<string, string[]> = {
+const WINDOWS_SHELL_PATHS: Partial<Record<SupportedTerminal, string[]>> = {
powershell: [This ensures the keys are constrained to valid SupportedTerminal values.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const WINDOWS_SHELL_PATHS: Record<string, string[]> = { | |
| powershell: [ | |
| 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', // PowerShell 7 (Core) | |
| 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', // Windows PowerShell 5.1 | |
| ], | |
| windowsterminal: [ | |
| 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', // Prefer PowerShell Core in Windows Terminal | |
| 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', | |
| ], | |
| cmd: [ | |
| 'C:\\Windows\\System32\\cmd.exe', | |
| ], | |
| gitbash: [ | |
| 'C:\\Program Files\\Git\\bin\\bash.exe', | |
| 'C:\\Program Files (x86)\\Git\\bin\\bash.exe', | |
| ], | |
| cygwin: [ | |
| 'C:\\cygwin64\\bin\\bash.exe', | |
| 'C:\\cygwin\\bin\\bash.exe', | |
| ], | |
| msys2: [ | |
| 'C:\\msys64\\usr\\bin\\bash.exe', | |
| 'C:\\msys32\\usr\\bin\\bash.exe', | |
| ], | |
| }; | |
| const WINDOWS_SHELL_PATHS: Partial<Record<SupportedTerminal, string[]>> = { | |
| powershell: [ | |
| 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', // PowerShell 7 (Core) | |
| 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', // Windows PowerShell 5.1 | |
| ], | |
| windowsterminal: [ | |
| 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', // Prefer PowerShell Core in Windows Terminal | |
| 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', | |
| ], | |
| cmd: [ | |
| 'C:\\Windows\\System32\\cmd.exe', | |
| ], | |
| gitbash: [ | |
| 'C:\\Program Files\\Git\\bin\\bash.exe', | |
| 'C:\\Program Files (x86)\\Git\\bin\\bash.exe', | |
| ], | |
| cygwin: [ | |
| 'C:\\cygwin64\\bin\\bash.exe', | |
| 'C:\\cygwin\\bin\\bash.exe', | |
| ], | |
| msys2: [ | |
| 'C:\\msys64\\usr\\bin\\bash.exe', | |
| 'C:\\msys32\\usr\\bin\\bash.exe', | |
| ], | |
| }; |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/pty-manager.ts around lines 18 - 42,
WINDOWS_SHELL_PATHS is currently typed as Record<string,string[]> which allows
typos and mismatches; change its type to
Partial<Record<SupportedTerminal,string[]>> so keys are validated against the
SupportedTerminal union (update the declaration of WINDOWS_SHELL_PATHS
accordingly) and then either add entries for the missing terminals (e.g.,
"conemu" and "cmder") to the map or add a comment/documentation explaining why
those SupportedTerminal values are intentionally omitted.
| // Read user's preferred terminal setting | ||
| const settings = readSettingsFile(); | ||
| const preferredTerminal = settings?.preferredTerminal as SupportedTerminal | undefined; |
There was a problem hiding this comment.
Add error handling and validate the type cast.
There are two issues here:
- Missing error handling:
readSettingsFile()could throw an exception, which would crash the PTY spawning process. - Unsafe type cast: The cast
as SupportedTerminal | undefinedassumessettings.preferredTerminalis a validSupportedTerminalvalue, but it could be any string from user settings.
🔎 Proposed fix with error handling and validation
- // Read user's preferred terminal setting
- const settings = readSettingsFile();
- const preferredTerminal = settings?.preferredTerminal as SupportedTerminal | undefined;
+ // Read user's preferred terminal setting
+ let preferredTerminal: SupportedTerminal | undefined;
+ try {
+ const settings = readSettingsFile();
+ preferredTerminal = settings?.preferredTerminal as SupportedTerminal | undefined;
+ } catch (error) {
+ console.warn('[PtyManager] Failed to read terminal preference:', error);
+ preferredTerminal = undefined;
+ }Alternatively, if you want to validate the value is actually a valid SupportedTerminal:
let preferredTerminal: SupportedTerminal | undefined;
try {
const settings = readSettingsFile();
const pref = settings?.preferredTerminal;
// Validate it's a known terminal type
if (typeof pref === 'string' && Object.keys(WINDOWS_SHELL_PATHS).includes(pref)) {
preferredTerminal = pref as SupportedTerminal;
}
} catch (error) {
console.warn('[PtyManager] Failed to read terminal preference:', error);
}🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/pty-manager.ts around lines 77 - 79, Wrap
the readSettingsFile() call in a try/catch and validate the returned
preferredTerminal before casting: call readSettingsFile() inside try, extract
settings?.preferredTerminal into a local variable, check typeof pref ===
'string' and that it matches a known SupportedTerminal (e.g. by checking
membership in WINDOWS_SHELL_PATHS or a SupportedTerminal enum), only then assign
preferredTerminal = pref as SupportedTerminal; on catch log or warn (e.g.
'[PtyManager] Failed to read terminal preference') and leave preferredTerminal
undefined.
- Add error handling for streaming response errors in cmd_pull_model - Add version compatibility checking before model pull - Add min_version metadata to known embedding models - Enhanced check-status with supports_new_models flag - Enhanced get-recommended-models with compatibility info Fixes silent failures when Ollama version is too old for newer embedding models like qwen3-embedding:8b. Fixes AndyMik90#758
bbopen
force-pushed
the
fix/ollama-version-error-handling
branch
from
a3a1921 to
124a5be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/ollama_model_detector.py (1)
1-565: Critical: Fix code formatting before merge.The ruff format check has failed. Run
ruff format apps/backend/to automatically fix formatting issues.#!/bin/bash # Run ruff format to fix code style cd apps/backend && ruff format ollama_model_detector.py
🤖 Fix all issues with AI agents
In @apps/backend/ollama_model_detector.py:
- Around line 123-134: parse_version should defensively handle None (or
non-string) inputs to avoid TypeError: check if version_str is falsy or not an
instance of str at the start of parse_version and return (0,0,0) for those
cases; update the type hint to accept Optional[str] if you want explicit typing,
and leave version_gte unchanged since it will now safely compare by calling
parse_version.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/ollama_model_detector.py
🧰 Additional context used
📓 Path-based instructions (1)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/ollama_model_detector.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/ollama_model_detector.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
🪛 GitHub Actions: Lint
apps/backend/ollama_model_detector.py
[error] 1-400: ruff format check failed. 1 file would be reformatted. Run 'ruff format apps/backend/' to fix code style issues in this file.
🔇 Additional comments (6)
apps/backend/ollama_model_detector.py (6)
19-19: LGTM! Clear constant definition.The import and constant are well-documented and follow Python conventions.
Also applies to: 27-29
39-42: LGTM! Consistent version metadata.The min_version metadata is correctly applied to all qwen3 embedding models consistently across both data structures.
Also applies to: 71-71, 79-79, 87-87
170-175: LGTM! Clean version retrieval.The function properly delegates to
fetch_ollama_apiand handles missing version data gracefully.
236-274: LGTM! Proper version checking with guards.The function correctly guards against
"unknown"version before callingversion_gteand setssupports_new_modelstoNonewhen version detection fails, which is appropriate.
421-432: LGTM! Excellent pre-flight version check.The pre-flight compatibility check properly guards against
Nonevalues and provides clear, actionable error messages with upgrade instructions.
447-461: LGTM! Robust streaming error handling.The error detection in the streaming response provides good defense-in-depth:
- Catches version errors that bypass pre-flight checks
- Cleans and enhances error messages for better UX
- Provides upgrade guidance for version-related failures
- Falls back to original errors for other issues
bbopen
force-pushed
the
fix/ollama-version-error-handling
branch
from
6e7a275 to
fbf4f54
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @apps/backend/ollama_model_detector.py:
- Around line 123-131: The type annotation for parse_version's parameter uses
the deprecated Optional[str]; update the signature from def
parse_version(version_str: Optional[str]) -> tuple[int, ...]: to def
parse_version(version_str: str | None) -> tuple[int, ...]: and adjust imports
(remove Optional from typing if no longer used) so type hints follow Python
3.12+ conventions while keeping the behavior for version_str being None
unchanged.
- Line 23: The code currently imports Optional from typing but must use Python
3.12+ union syntax; update the type annotations in parse_version and version_gte
to use "str | None" instead of "Optional[str]" and remove the now-unused
Optional import from the top of the file (leave Any if still used); ensure the
function signatures for parse_version and version_gte are changed to the new
union style and run a quick type check to confirm no other references to
Optional remain.
- Around line 134-136: The type annotations on version_gte should use modern
union syntax instead of typing.Optional to satisfy the pipeline; update the
signature of version_gte(version: Optional[str], min_version: Optional[str]) ->
bool to version_gte(version: str | None, min_version: str | None) -> bool and
remove or adjust any unused Optional import; keep the implementation
(parse_version calls and return type) unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/ollama_model_detector.py
🧰 Additional context used
📓 Path-based instructions (1)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/ollama_model_detector.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/ollama_model_detector.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
🧬 Code graph analysis (1)
apps/backend/ollama_model_detector.py (1)
scripts/bump-version.js (1)
match(66-66)
🪛 GitHub Actions: Lint
apps/backend/ollama_model_detector.py
[error] 123-123: ruff check failed. UP045 Use X | None for type annotations.
🪛 GitHub Check: python
apps/backend/ollama_model_detector.py
[failure] 134-134: Ruff (UP045)
apps/backend/ollama_model_detector.py:134:54: UP045 Use X | None for type annotations
[failure] 134-134: Ruff (UP045)
apps/backend/ollama_model_detector.py:134:26: UP045 Use X | None for type annotations
[failure] 123-123: Ruff (UP045)
apps/backend/ollama_model_detector.py:123:32: UP045 Use X | None for type annotations
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
apps/backend/ollama_model_detector.py (8)
27-29: LGTM!The constant is well-documented and provides a single source of truth for the minimum version requirement.
39-42: LGTM!The
min_versionmetadata is correctly added to all qwen3-embedding variants.
64-87: LGTM!The
min_ollama_versionfield is correctly added only to models that require Ollama 0.10.0+, while older-compatible models remain without version constraints.
172-177: LGTM!Clean implementation that reuses the existing
fetch_ollama_apihelper and properly handles failure cases.
227-237: LGTM!The length-descending sort correctly addresses the substring matching issue (e.g., matching "qwen3-embedding:8b" before "qwen3-embedding").
247-257: LGTM!The
supports_new_modelsflag provides useful information for the UI. The conditional correctly returnsNonewhen version is unknown rather than making assumptions.
369-416: LGTM!The compatibility checking is comprehensive and addresses the previous review feedback. The note when version is unknown provides appropriate user guidance without blocking the operation.
430-470: LGTM!The pre-flight version check and streaming error handling effectively address issue #758. The design correctly falls through to runtime error handling when version cannot be determined (line 434 requires both
min_versionandollama_version), which provides a safety net via the streaming error handler at lines 456-470.
bbopen
force-pushed
the
fix/ollama-version-error-handling
branch
from
fbf4f54 to
9abbf82
Compare
- Add defensive None handling in parse_version() - Sort model keys by length for more specific matching - Add compatibility note when Ollama version is unknown
bbopen
force-pushed
the
fix/ollama-version-error-handling
branch
from
9abbf82 to
bb30f09
Compare
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: ### Merge Verdict: ✅ READY TO MERGE
No blocking issues. 3 non-blocking suggestion(s) to consider
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Low: 3 issue(s)
Generated by Auto Claude PR Review
💡 Suggestions (3)
These are non-blocking suggestions for consideration:
🔵 [f0743c6e9967] [LOW] Substring matching in model lookup could cause false positives
📁 apps/backend/ollama_model_detector.py:249
The check if known_model in name_lower in get_model_min_version() uses substring matching, which could match unintended models if a custom model name contains a known model name as a substring. For example, 'my-custom-qwen3-embedding-variant' would match 'qwen3-embedding'. However, this appears intentional to catch variants like 'qwen3-embedding:latest', and the length-descending sort (line 249) mitigates the risk by matching more specific names first.
Suggested fix:
Document that substring matching is intentional for variant support, or consider word-boundary matching if exact names are preferred.
🔵 [ec857a1ac2ac] [LOW] Code duplication in model lookup functions
📁 apps/backend/ollama_model_detector.py:196
Four functions (is_embedding_model, get_embedding_dim, get_embedding_description, get_model_min_version) iterate over KNOWN_EMBEDDING_MODELS with similar lookup patterns. The only one that sorts by length is get_model_min_version, which could lead to subtly different matching behavior between functions.
Suggested fix:
Consider extracting a helper function like `find_known_model(model_name: str) -> tuple[str, dict] | None` to centralize lookup logic and ensure consistent matching behavior.
🔵 [4d98fc59cd05] [LOW] Silent skip of version check when Ollama version unavailable
📁 apps/backend/ollama_model_detector.py:454
When get_ollama_version() returns None (API failure), the condition if min_version and ollama_version: evaluates to False, silently skipping the pre-flight version check. The pull proceeds and may fail later with a less helpful error message.
Suggested fix:
Consider adding a warning message when version cannot be verified but min_version requirement exists: 'Warning: Could not verify Ollama version compatibility for model requiring {min_version}'
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| match = re.match(r"(\d+)\.(\d+)\.(\d+)", version_str) | ||
| if match: | ||
| return tuple(int(x) for x in match.groups()) | ||
| return (0, 0, 0) |
There was a problem hiding this comment.
Version parsing fails for two-part version strings
Low Severity
The parse_version function uses a regex pattern r"(\d+)\.(\d+)\.(\d+)" that requires exactly three version parts. Version strings with only two parts (like "1.0") won't match and silently fall back to returning (0, 0, 0). This causes version_gte("1.0", "0.10.0") to incorrectly return False, potentially telling users with a newer version that they need to upgrade.
| version, MIN_OLLAMA_VERSION_FOR_NEW_MODELS | ||
| ) | ||
| if version != "unknown" | ||
| else None, |
There was a problem hiding this comment.
Missing supports_new_models in fallback response path
Medium Severity
The cmd_check_status function adds supports_new_models to the response when the version endpoint succeeds, but the fallback path (when version endpoint fails but tags endpoint succeeds) omits this field entirely. This creates an inconsistent API response structure - consumers expecting supports_new_models will get undefined instead of null in JavaScript, which could cause different behavior than intended.
| if not is_compatible: | ||
| compatibility_note = f"Requires Ollama {min_version}+" | ||
| elif min_version and not ollama_version: | ||
| compatibility_note = "Version compatibility could not be verified" |
There was a problem hiding this comment.
Compatible flag true when version unverified
Low Severity
When a model has a min_ollama_version but the Ollama version cannot be fetched, is_compatible remains True while compatibility_note is set to indicate verification failed. This is logically inconsistent - the model is marked as compatible when compatibility is actually unknown. The UI might show a green "compatible" indicator for models that could fail to install.
| version, MIN_OLLAMA_VERSION_FOR_NEW_MODELS | ||
| ) | ||
| if version != "unknown" | ||
| else None, |
There was a problem hiding this comment.
Null version incorrectly reports unsupported new models
Medium Severity
When the Ollama API returns {"version": null}, result.get("version", "unknown") returns None (not the default "unknown"). The condition version != "unknown" evaluates to True for None, causing version_gte(None, ...) to be called. This returns False (treating unknown version as 0.0.0), incorrectly setting supports_new_models to False instead of None. The condition needs to also check that version is truthy, e.g., if version and version != "unknown".
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🔄 Follow-up Review: Needs Revision
Resolution Status
- ✅ Resolved: 2 previous findings addressed
- ❌ Unresolved: 1 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 2 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 2 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: Inconsistent compatibility status when version cannot be determined
Verdict
CI Status: ✅ All 14 checks passing. However, one MEDIUM severity finding (NEW-003) was confirmed valid: when Ollama version cannot be determined, models are marked is_compatible=True despite having version requirements, creating inconsistent UX. Additionally, one LOW finding remains (ec857a1ac2ac): inconsistent sorting across lookup functions where only get_model_min_version sorts by length. Per strict quality gates, MEDIUM severity confirmed findings block merge. Note: These are UX/consistency issues, not runtime bugs. The core version checking functionality works correctly with good fallback error handling. If acceptable to ship with these known inconsistencies, reviewer may override to MERGE_WITH_CHANGES.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (3 selected of 3 total)
🔵 [ec857a1ac2ac] [LOW] [UNRESOLVED] Code duplication in model lookup functions
📁 apps/backend/ollama_model_detector.py:196
Four functions (is_embedding_model, get_embedding_dim, get_embedding_description, get_model_min_version) iterate over KNOWN_EMBEDDING_MODELS with similar lookup patterns. The only one that sorts by length is get_model_min_version, which could lead to subtly different matching behavior between functions.
Resolution note: is_embedding_model: for known_model in KNOWN_EMBEDDING_MODELS (no sort)
get_embedding_dim: for known_model, info in KNOWN_EMBEDDING_MODELS.items() (no sort)
get_embedding_description: for known_model, info in KNOWN_EMBEDDING_MODELS.items() (no sort)
get_model_min_version: for known_model in sorted(..., key=len, reverse=True) (sorted)
Suggested fix:
Consider extracting a helper function like `find_known_model(model_name: str) -> tuple[str, dict] | None` to centralize lookup logic and ensure consistent matching behavior.
🔵 [NEW-001] [LOW] parse_version only handles three-part version strings
📁 apps/backend/ollama_model_detector.py:139
The parse_version() function uses regex strictly requiring three numeric parts (X.Y.Z). If Ollama returns a version like '0.10' (two parts), the function returns (0, 0, 0) instead of properly parsing available parts, potentially causing incorrect version comparisons.
Suggested fix:
Use flexible regex: r"(\d+)\.(\d+)(?:\.(\d+))?" and handle optional third group, or split on dots and convert available parts.
🟡 [NEW-003] [MEDIUM] Inconsistent compatibility status when version cannot be determined
📁 apps/backend/ollama_model_detector.py:410
In cmd_get_recommended_models(), when Ollama version cannot be determined but a model has min_ollama_version requirement, the model is marked is_compatible=True with a compatibility_note saying 'Version compatibility could not be verified'. This inconsistency could mislead UI into showing both compatible status and a warning.
Suggested fix:
Set is_compatible = None when version cannot be determined for models with version requirements, allowing UI to properly handle the unknown state.
This review was generated by Auto Claude.
…0#760) * fix(memory): handle Ollama version errors during model pull - Add error handling for streaming response errors in cmd_pull_model - Add version compatibility checking before model pull - Add min_version metadata to known embedding models - Enhanced check-status with supports_new_models flag - Enhanced get-recommended-models with compatibility info Fixes silent failures when Ollama version is too old for newer embedding models like qwen3-embedding:8b. Fixes AndyMik90#758 * fix: address code review feedback - Add defensive None handling in parse_version() - Sort model keys by length for more specific matching - Add compatibility note when Ollama version is unknown --------- Co-authored-by: Andy <[email protected]>
…0#760) * fix(memory): handle Ollama version errors during model pull - Add error handling for streaming response errors in cmd_pull_model - Add version compatibility checking before model pull - Add min_version metadata to known embedding models - Enhanced check-status with supports_new_models flag - Enhanced get-recommended-models with compatibility info Fixes silent failures when Ollama version is too old for newer embedding models like qwen3-embedding:8b. Fixes AndyMik90#758 * fix: address code review feedback - Add defensive None handling in parse_version() - Sort model keys by length for more specific matching - Add compatibility note when Ollama version is unknown --------- Co-authored-by: Andy <[email protected]>
…0#760) * fix(memory): handle Ollama version errors during model pull - Add error handling for streaming response errors in cmd_pull_model - Add version compatibility checking before model pull - Add min_version metadata to known embedding models - Enhanced check-status with supports_new_models flag - Enhanced get-recommended-models with compatibility info Fixes silent failures when Ollama version is too old for newer embedding models like qwen3-embedding:8b. Fixes AndyMik90#758 * fix: address code review feedback - Add defensive None handling in parse_version() - Sort model keys by length for more specific matching - Add compatibility note when Ollama version is unknown --------- Co-authored-by: Andy <[email protected]>
3 participants
Summary
Fixes silent failures when pulling embedding models that require a newer Ollama version. The UI now properly displays error messages and warns users when their Ollama version is incompatible with certain models.
Changes
Added error handling for streaming response errors (
cmd_pull_model)"error"key in Ollama's NDJSON streaming responseAdded version compatibility checking
parse_version()andversion_gte()helper functionsmin_versionmetadata to known embedding modelsMIN_OLLAMA_VERSION_FOR_NEW_MODELSconstant (0.10.0)Enhanced
cmd_check_statussupports_new_modelsflag indicating if Ollama supports newer embedding modelsEnhanced
cmd_get_recommended_modelscompatibleflag for each model based on current Ollama versioncompatibility_notefor incompatible modelsollama_versionto help UI display version infoTest Plan
check-statusreturnssupports_new_modelsflagget-recommended-modelsreturns compatibility infoRelated Issues
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.