[Bug-Fix] Enhance database directory checks for writability in InstallationHistory and SemanticCache; add Ollama model availability checks in tests #518
Conversation
…ory and SemanticCache; add Ollama model availability checks in tests
📝 WalkthroughWalkthroughReorders API key/provider detection to prefer explicit Ollama mode and environment API keys, adds saved-Ollama preference persistence, introduces file-locking atomic writes for env updates, and adds explicit writability checks for system DB/cache directories with fallback to user-home. Tests add dynamic Ollama runtime checks and model selection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Cortex CLI
participant Detector as APIKeyDetector
participant Env as OS Env
participant Saved as "~/.cortex/.env"
participant Cache as Cached Keys
participant FS as Filesystem
participant DB as DB Init
rect rgb(230,240,255)
User->>CLI: run command (e.g., install)
CLI->>Detector: detect provider
Detector->>Env: check CORTEX_PROVIDER & env API keys
Env-->>Detector: provider present? (yes/no)
end
alt env provider found
Detector-->>CLI: return provider (Ollama or API key)
else no env provider
Detector->>Cache: check cached keys
Cache-->>Detector: cached result
alt cache hit
Detector-->>CLI: return cached provider
else
Detector->>Saved: read ~/.cortex/.env for saved Ollama
Saved-->>Detector: saved Ollama? (yes/no)
alt saved Ollama
Detector-->>CLI: return Ollama
else
Detector->>CLI: prompt user for provider
User-->>CLI: choose provider
CLI->>Detector: prompt_for_key result
alt chosen == "ollama"
Detector->>User: ask to save Ollama preference
User-->>Detector: yes/no
alt yes
Detector->>Saved: _save_provider_to_env("ollama") (locked atomic write)
Saved-->>Detector: write success/fail
end
end
end
end
end
CLI->>DB: _ensure_db_directory()
DB->>FS: create/check /var/lib/cortex
FS-->>DB: exists but writable? (no -> raise PermissionError)
alt PermissionError
DB->>FS: fallback create ~/.cortex
FS-->>DB: user-home db dir ready
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes three key issues related to database initialization, Ollama provider persistence, and Ollama test reliability.
Key Changes:
- Added write permission checks to database initialization in
InstallationHistoryandSemanticCacheto properly fallback to~/.cortex/when system directories lack write access - Implemented Ollama provider preference persistence via
CORTEX_PROVIDERenvironment variable and~/.cortex/.envconfiguration file with proper priority handling - Enhanced Ollama integration tests to dynamically detect available models instead of hardcoding model names
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/test_ollama_integration.py | Added dynamic Ollama model detection and improved test skip conditions to check for installation, service status, and available models |
| cortex/semantic_cache.py | Added write permission check using os.access() to ensure proper fallback to user directory when system directory is not writable |
| cortex/installation_history.py | Added write permission check using os.access() to ensure proper fallback to user directory when system directory is not writable |
| cortex/api_key_detector.py | Implemented Ollama provider preference detection and persistence with proper priority order: environment variables → cached keys → saved preferences → file locations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @cortex/installation_history.py:
- Line 84: Remove the local "import os" from inside the permission-checking
function in installation_history.py and add "import os" to the module-level
imports at the top of the file with the other standard-library imports; ensure
no other code changes are made other than removing the in-function import and
adding the module-level import so usages (e.g., os.stat/os.access) continue to
work.
🧹 Nitpick comments (4)
tests/test_ollama_integration.py (2)
28-46: Strengthen the model name parsing logic.The current implementation assumes the model name is in the first column of the second line and uses
.split()[0]without validation. If theollama listoutput format changes or contains unexpected data, this could fail silently or return invalid model names.🔎 Proposed improvement
def get_available_ollama_model() -> str | None: """Get the first available Ollama model, or None if none available.""" try: result = subprocess.run( ["ollama", "list"], capture_output=True, text=True, timeout=5, ) if result.returncode == 0: # Parse output: skip header line, get first model name lines = result.stdout.strip().split("\n") if len(lines) > 1: - # Model name is the first column - model_name = lines[1].split()[0] - return model_name + # Model name is the first column (before any whitespace or colon) + parts = lines[1].split() + if parts: + # Handle format like "llama3.2:latest" by taking everything before whitespace + model_name = parts[0] + if model_name and not model_name.startswith("NAME"): # Skip if still header + return model_name except Exception: pass return None
44-45: Consider narrowing the exception handling.The bare
except Exception:catches all exceptions, includingKeyboardInterruptandSystemExit(though Python 3 treats these specially). For robustness, consider catching specific exceptions likesubprocess.SubprocessError,OSError, andValueError.🔎 Proposed refinement
- except Exception: + except (subprocess.SubprocessError, OSError, ValueError, IndexError): pass return Nonecortex/api_key_detector.py (2)
102-108: Consider adding a docstring note about priority.While the method implementation is correct, it would be helpful to document in the docstring that this check has higher priority than cached keys and saved preferences, making it clear why this method exists separately from the existing
_check_environment_variablemethod.🔎 Proposed enhancement
def _check_environment_api_keys(self) -> tuple[bool, str, str, str] | None: - """Check for API keys in environment variables.""" + """Check for API keys in environment variables. + + This has higher priority than cached keys and saved preferences, + ensuring environment variables always take precedence. + """ for env_var, provider in ENV_VAR_PROVIDERS.items():
110-124: Add error context to exception handling.The
except OSError:at line 122 silently catches all OS-level errors when reading the.envfile. While silent failure is acceptable here (returningNoneis the right behavior), it would be helpful for debugging to at least log the error or add a comment explaining why silent failure is intended.🔎 Proposed improvement
try: content = env_file.read_text() for line in content.splitlines(): line = line.strip() if line.startswith("CORTEX_PROVIDER="): value = line.split("=", 1)[1].strip().strip("\"'").lower() if value == "ollama": return (True, "ollama-local", "ollama", str(env_file)) - except OSError: + except OSError: + # Silently ignore read errors - saved preference is optional pass return None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/api_key_detector.pycortex/installation_history.pycortex/semantic_cache.pytests/test_ollama_integration.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/semantic_cache.pycortex/api_key_detector.pytests/test_ollama_integration.pycortex/installation_history.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_ollama_integration.py
**/*install*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*install*.py: Dry-run by default for all installations in command execution
No silent sudo execution - require explicit user confirmation
Implement audit logging to ~/.cortex/history.db for all package operations
Files:
cortex/installation_history.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/installation_history.py
🧬 Code graph analysis (1)
cortex/api_key_detector.py (1)
cortex/branding.py (1)
cx_print(49-69)
⏰ 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). (4)
- GitHub Check: Agent
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (5)
cortex/installation_history.py (1)
89-91: LGTM! Writability check correctly addresses the permission issue.The explicit write permission check using
os.access(db_dir, os.W_OK)properly detects when/var/lib/cortex/exists but is not writable, allowing the fallback to~/.cortex/to work correctly. This directly addresses the bug described in issue #514.cortex/semantic_cache.py (1)
83-85: LGTM! Writability check is correctly implemented.The explicit write permission check using
os.access(db_dir, os.W_OK)mirrors the fix ininstallation_history.pyand ensures that the cache directory is actually writable before attempting to create the database. The fallback to~/.cortex/cache.dbwhen the system directory is not writable is appropriate.tests/test_ollama_integration.py (1)
69-85: LGTM! Dynamic model selection improves test flexibility.The environment variable override (
OLLAMA_TEST_MODEL) combined with automatic model discovery viaget_available_ollama_model()makes the tests much more flexible. The three skip conditions ensure tests only run when Ollama is properly set up with available models.cortex/api_key_detector.py (2)
78-96: LGTM! Detection priority order is well-designed.The new detection flow correctly prioritizes configuration sources:
- Explicit
CORTEX_PROVIDER=ollamaenvironment variable (highest priority)- API keys in environment variables
- Cached key locations
- Saved Ollama provider preference in
~/.cortex/.env- Other file locations
This ensures environment variables always take precedence over saved preferences, which is the expected behavior. The code correctly addresses the PR objective of persisting Ollama provider selection.
225-237: LGTM! User confirmation flow is well-implemented.The method correctly handles user input with proper error handling for
EOFErrorandKeyboardInterrupt. The default behavior (treating interrupts as "no") is appropriate, and the success message provides clear feedback to the user.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
CLA Verification PassedAll contributors have signed the CLA.
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ile locking mechanisms
|
@divanshu-go @Anshgrover23 @Suyashd999 |
|
I want to clarify the attribution and scope here, since this PR addresses three independent concerns that originate from different places:
this is a filesystem permission issue when /var/lib/cortex exists but is not writable by the current user. This behavior predates PR #436 and is unrelated to zero-config API key detection.
@sujay-d07 This item is not attributable to PR #436 and was outside the scope of Issue #255. That PR was limited to zero-config API key auto-detection and validation, not provider persistence across runs. Selecting Ollama via CORTEX_PROVIDER=ollama was already a supported and explicit mechanism. Persisting the provider choice represents a UX improvement, not a regression or correctness issue related to #436. I tested that on 
these stem from tests hardcoding a specific model (llama3.2), which may not exist in many environments. This is test fragility, not behavior introduced by #436. The fixes themselves look good, but I think it’s important we separate responsibility from remediation here. Grouping multiple unrelated issues into a single PR is fine, but attributing all of them back to PR #436 isn’t accurate based on scope or timelines. Appreciate the work on tightening these areas up. |
|
@divanshu-go My bad, my previous comment didn’t explicitly mention that the Ollama persistence issue originated from PR #436. The database initialization issue and the Ollama integration test failure were separate concerns. The earlier implementation for Ollama choosen was incomplete: while API keys for Claude/OpenAI were correctly persisted. The Ollama provider selection was returned immediately without saving to config, unlike API keys which prompted the user to save to ~/.cortex/.env. |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@sujay-d07 Update your branch.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_ollama_integration.py (1)
91-127: Consider refactoring to reduce code duplication.The
check_ollama_installed()andcheck_ollama_running()functions duplicate logic from the newer helper functions (is_ollama_installed()andis_ollama_running()at lines 52-68). While they serve different purposes (interactive diagnostics vs. pytest guards), consider refactoring them to reuse the helpers and add only the print statements for user feedback.🔎 Example refactor
def check_ollama_installed(): """Check if Ollama is installed.""" print("1. Checking Ollama installation...") - result = subprocess.run(["which", "ollama"], capture_output=True) - if result.returncode == 0: + if is_ollama_installed(): print(" ✓ Ollama is installed") return True else: print(" ✗ Ollama is not installed") print(" Run: python scripts/setup_ollama.py") return FalseSimilar refactoring could apply to
check_ollama_running().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/api_key_detector.pycortex/installation_history.pytests/test_ollama_integration.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/installation_history.pytests/test_ollama_integration.pycortex/api_key_detector.py
**/*install*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*install*.py: Dry-run by default for all installations in command execution
No silent sudo execution - require explicit user confirmation
Implement audit logging to ~/.cortex/history.db for all package operations
Files:
cortex/installation_history.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_ollama_integration.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/installation_history.py
🧬 Code graph analysis (1)
tests/test_ollama_integration.py (1)
cortex/llm_router.py (1)
LLMRouter(76-846)
⏰ 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). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (14)
cortex/installation_history.py (2)
12-12: LGTM! Import placement follows Python best practices.The
osmodule is now correctly imported at the module level alongside other standard library imports, addressing the previous review feedback.
84-90: Effective fix for database initialization failure when directory exists but is not writable.The explicit write permission check using
os.access(db_dir, os.W_OK)correctly addresses issue #514. When the system directory exists but is not writable, raisingPermissionErrortriggers the existing fallback mechanism to use~/.cortex/, ensuring the application doesn't crash during database initialization.tests/test_ollama_integration.py (4)
28-49: LGTM! Dynamic model detection is well-implemented.The function properly handles edge cases with the
if parts:check (line 43) and includes an explanatory comment for the exception handler (lines 46-48), addressing previous review feedback. The timeout and error handling make this a robust helper for test setup.
52-68: Good separation of concerns for test prerequisites.The
is_ollama_installed()andis_ollama_running()helpers provide clear, focused checks that improve test skip logic readability and maintainability.
72-72: Excellent test configuration with environment override and granular skip conditions.The environment variable override on line 72 provides flexibility for CI/CD environments, while the three separate skip conditions (lines 75-88) offer clear diagnostics when tests cannot run, making it easier for developers to understand and resolve issues.
Also applies to: 75-88
133-133: Consistent use of dynamic model selection across tests.The tests now use
OLLAMA_TEST_MODELconsistently, replacing the previous hardcoded "llama3.2" reference. This makes the tests more robust and adaptable to different Ollama installations.Also applies to: 139-139, 180-180, 210-210
cortex/api_key_detector.py (8)
4-19: LGTM! Module docstring accurately reflects the updated detection logic.The docstring now comprehensively documents all detection steps including the new Ollama provider checks and file locking mechanisms, addressing previous review feedback.
84-106: Well-structured detection flow with clear prioritization.The updated detection sequence properly prioritizes explicit environment variables, then API keys, then cached values, then saved preferences, and finally file-based detection. This logical ordering ensures that user intent is respected while providing a smooth fallback chain.
108-114: Good refactoring for clarity.Extracting the environment API key check into a dedicated method improves code organization and readability. The type hints are complete and correct.
116-131: Saved Ollama preference detection is well-implemented.The method correctly reads the config file and parses the
CORTEX_PROVIDERsetting. The exception handling on lines 128-130 now includes an explanatory comment, addressing previous review feedback. The parsing logic appropriately handles quoted and unquoted values.
232-244: User-friendly Ollama preference persistence.The prompt properly handles user interruption and defaults to saving the preference (any response except "n"), providing a smooth UX that implements the PR objective of persisting Ollama provider selection.
246-254: Provider persistence with proper concurrency protection.The method correctly uses
_locked_read_modify_writeto prevent race conditions, and gracefully handles write failures without crashing the application.
481-513: Robust file locking implementation prevents race conditions.This method properly addresses the TOCTOU (Time-Of-Check-Time-Of-Use) race condition raised in previous reviews. The exclusive lock (line 501), combined with atomic writes and guaranteed lock release in the finally block, ensures safe concurrent access on Unix systems.
595-596: Consistent adoption of locking mechanism for API key persistence.The method now uses the file locking infrastructure, ensuring safe concurrent updates. The docstring accurately reflects this behavioral change.
Also applies to: 605-605
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
LGTM! Thanks @sujay-d07 .
…lationHistory and SemanticCache; add Ollama model availability checks in tests (cortexlinux#518) * Enhance database directory checks for writability in InstallationHistory and SemanticCache; add Ollama model availability checks in tests * feat: Enhance API key detection with Ollama provider support * Apply suggestion from @Copilot for the Except Clause Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor: Improve API key detection logic with enhanced caching and file locking mechanisms --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Ansh Grover <168731971+Anshgrover23@users.noreply.github.com>
3 participants
Related Issue
Closes #514
Summary
1. Database initialization fails when /var/lib/cortex/ is not writable:
Added write permission check (os.access(db_dir, os.W_OK)) in _ensure_db_directory() to properly fallback to ~/.cortex/ when the system directory exists but user lacks write access.
Files: cortex/installation_history.py, cortex/semantic_cache.py
2. Ollama provider choice not persisted between runs:
Added option to save CORTEX_PROVIDER=ollama to ~/.cortex/.env when user selects Ollama
Subsequent runs auto-detect saved preference without re-prompting
Environment variable API keys take priority over saved Ollama preference
File: cortex/api_key_detector.py
3. Ollama integration tests fail with hardcoded model
Tests now dynamically detect available Ollama models instead of hardcoding llama3.2
File: tests/test_ollama_integration.py
Type of Change
AI Disclosure
Used Cursor with Opus 4.5 to fix this error and add the automatic detection of ollama model in test_ollama_integration.py
Checklist
Before
Screencast.from.2026-01-05.18-51-12.webm
After Changes
Screencast.from.2026-01-05.18-45-32.webm
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.