feat(ui): add multi-provider embedder credential validation #560
Conversation
- Add biome.json configuration file - Configure import sorting, formatting rules, and linter settings - Add ConsoleLogsPanel component for debug output - Add console-store for console message management Note: Run `npx @biomejs/biome format --write .` to apply formatting
- Enable important linting rules as warnings (noUnusedVariables, useExhaustiveDependencies, noExplicitAny, noArrayIndexKey, noDangerouslySetInnerHtml) - Fix handleSourceToggle logic bug when unchecking from 'all selected' state - Move getFilteredLogs to component with useMemo for performance - Optimize errorCount/warnCount to single iteration with useMemo - Add shared ALL_SOURCES const for type safety
Document the decision to use npm over bun for this Electron project, including technical justification and when to reconsider.
Implements explicit Claude CLI path detection and passing via environment: - Add getClaudeCliPath() to python-env-manager.ts with multi-location search - Pass CLAUDE_CLI_PATH env var from agent-process.ts - Read CLAUDE_CLI_PATH in client.py and pass to ClaudeAgentOptions Fixes AndyMik90#529
Implements Phase 1 of implicit conflict detection using string matching: - Detect when one task removes a function/import that another task references - Extend detect_implicit_conflicts() with basic pattern matching - Track removed entities and check for references in other tasks This provides basic implicit conflict detection without requiring full AST parsing. More sophisticated detection can be added in Phase 2.
Adds get_enhanced_path() function to include common Node.js/npm/npx locations for GUI-launched Electron apps that don't inherit shell PATH modifications. Fixes AndyMik90#523
Implements _record_merge_completion() function to record which files were merged by which spec, enabling the Evolution Tracker to maintain merge history for incremental sync operations. Fixes AndyMik90#498
- Add Approve Plan button for tasks awaiting plan review - Updates review_state.json with current spec hash - Sets plan status to pending/backlog for task execution - Add Recreate Task button to reset stuck tasks - Shows confirmation dialog before proceeding - Cleans up worktree if exists - Clears spec directory while preserving task description - Add IPC handlers: TASK_RECREATE and TASK_APPROVE_PLAN - Add preload API methods and type definitions - Add browser mock implementations for testing
- Add refresh button in KanbanBoard header to manually refresh tasks - Shows spinning animation while refreshing - Integrate ConsoleLogsToggle button in app header (next to settings) - Add ConsoleLogsPanel at bottom of main content area - Panel shows/hides based on console-store state These components existed from a previous branch but weren't integrated into the main layout.
- Add ConsoleLogsPanel component for displaying console logs - Add console-store for console message management - Integrate ConsoleLogsToggle in app header - Add ConsoleLogsPanel at bottom of main content area - Support filtering by log level and source - Show error/warning count badge in toggle button This provides visibility into background operations during development.
- Add Refresh button in Kanban Board header - Show spinning animation while refreshing - Add handleRefreshTasks handler in App component - Pass onRefresh and isRefreshing props to KanbanBoard This helps when the UI doesn't update or when tasks are stuck, giving users a way to manually refresh task state.
- Use Tailwind class h-[250px] instead of inline style - Use crypto.randomUUID() for unique log IDs - Remove unused getFilteredLogs action from store
Apply code review suggestion to improve layout flexibility by using ml-auto on the right-side controls instead of justify-between on parent.
Extends the memory status system to validate credentials for all
supported Graphiti embedder providers, not just OpenAI.
Changes:
- Add getEmbedderProviderStatus() function to utils.ts
- Checks GRAPHITI_EMBEDDER_PROVIDER env var (defaults to 'openai')
- Validates credentials per provider:
* openai: OPENAI_API_KEY
* voyage: VOYAGE_API_KEY
* azure_openai: API key + base URL + embedding deployment
* ollama: OLLAMA_EMBEDDING_MODEL
* google: GOOGLE_API_KEY
- Returns provider name, configured status, and reason if not configured
- Update GlobalSettings interface with new API key fields:
- globalAnthropicApiKey
- globalAzureOpenAIApiKey
- globalVoyageApiKey
- globalGoogleApiKey
- Update buildMemoryStatus() to use new provider-aware validation
- Shows specific provider name in error messages
- Indicates which credential is missing for the configured provider
This allows users to configure alternative embedder providers (Voyage,
Azure, Ollama, Google) and get proper status feedback in the UI.
|
Warning Rate limit exceeded@adryserage has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (21)
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 @adryserage, 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 UI's capability to manage and provide feedback on Graphiti embedder configurations by implementing multi-provider credential validation. It also refines the underlying Python environment setup for greater robustness and tool discoverability. Furthermore, new task lifecycle management features like task recreation and explicit plan approval are introduced, alongside a dedicated console log panel for better operational visibility. These changes collectively improve the user experience, stability, and debuggability of the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 significantly expands the application's capabilities on multiple fronts. The primary feature, adding multi-provider embedder credential validation, is well-implemented and makes the memory status system much more robust. Beyond that, this PR introduces several other valuable features: a new console logging UI, task recreation and plan approval workflows, and substantial improvements to Python environment and path handling, which will increase reliability, especially in packaged app environments. The initial implementation of implicit conflict detection is also a promising step forward for the merge system. While the PR is quite large, the changes are of high quality. My feedback focuses on opportunities for refactoring to improve long-term maintainability.
| switch (embedderProvider) { | ||
| case 'openai': | ||
| if (projectEnvVars['OPENAI_API_KEY'] || globalSettings.globalOpenAIApiKey || process.env.OPENAI_API_KEY) { | ||
| return { provider: 'openai', configured: true }; | ||
| } | ||
| return { provider: 'openai', configured: false, reason: 'OPENAI_API_KEY not set' }; | ||
|
|
||
| case 'voyage': | ||
| if (projectEnvVars['VOYAGE_API_KEY'] || globalSettings.globalVoyageApiKey || process.env.VOYAGE_API_KEY) { | ||
| return { provider: 'voyage', configured: true }; | ||
| } | ||
| return { provider: 'voyage', configured: false, reason: 'VOYAGE_API_KEY not set' }; | ||
|
|
||
| case 'azure_openai': | ||
| const hasAzureKey = projectEnvVars['AZURE_OPENAI_API_KEY'] || globalSettings.globalAzureOpenAIApiKey || process.env.AZURE_OPENAI_API_KEY; | ||
| const hasAzureUrl = projectEnvVars['AZURE_OPENAI_BASE_URL'] || process.env.AZURE_OPENAI_BASE_URL; | ||
| const hasAzureDeployment = projectEnvVars['AZURE_OPENAI_EMBEDDING_DEPLOYMENT'] || process.env.AZURE_OPENAI_EMBEDDING_DEPLOYMENT; | ||
| if (hasAzureKey && hasAzureUrl && hasAzureDeployment) { | ||
| return { provider: 'azure_openai', configured: true }; | ||
| } | ||
| return { provider: 'azure_openai', configured: false, reason: 'Azure OpenAI requires API key, base URL, and embedding deployment' }; | ||
|
|
||
| case 'ollama': | ||
| const hasOllamaModel = projectEnvVars['OLLAMA_EMBEDDING_MODEL'] || process.env.OLLAMA_EMBEDDING_MODEL; | ||
| if (hasOllamaModel) { | ||
| return { provider: 'ollama', configured: true }; | ||
| } | ||
| return { provider: 'ollama', configured: false, reason: 'OLLAMA_EMBEDDING_MODEL not set' }; | ||
|
|
||
| case 'google': | ||
| if (projectEnvVars['GOOGLE_API_KEY'] || globalSettings.globalGoogleApiKey || process.env.GOOGLE_API_KEY) { | ||
| return { provider: 'google', configured: true }; | ||
| } | ||
| return { provider: 'google', configured: false, reason: 'GOOGLE_API_KEY not set' }; | ||
|
|
||
| default: | ||
| return { provider: embedderProvider, configured: false, reason: `Unknown embedder provider: ${embedderProvider}` }; | ||
| } |
There was a problem hiding this comment.
The logic for checking credentials for each provider is quite repetitive. Each case block follows the same pattern of checking projectEnvVars, globalSettings, and process.env. To improve maintainability and reduce code duplication, you could extract this logic into a helper function.
For example, a helper like const getCredential = (key: string, globalKey: keyof GlobalSettings) => ... could encapsulate the lookup priority.
| const result = require('node:child_process').spawnSync( | ||
| pythonPath, | ||
| ['-c', `import ${pkg}`], | ||
| { | ||
| stdio: 'pipe', | ||
| timeout: 10000, | ||
| } |
There was a problem hiding this comment.
The use of require('node:child_process').spawnSync inside the checkDepsForPathSync function is a bit unconventional. For better code style, consistency, and to make dependencies clearer at a glance, it's recommended to move all imports to the top of the file. You can add spawnSync to the existing import from node:child_process at the top.
| const result = require('node:child_process').spawnSync( | |
| pythonPath, | |
| ['-c', `import ${pkg}`], | |
| { | |
| stdio: 'pipe', | |
| timeout: 10000, | |
| } | |
| const result = spawnSync( | |
| pythonPath, | |
| ['-c', `import ${pkg}`], | |
| { | |
| stdio: 'pipe', | |
| timeout: 10000, | |
| } | |
| ) |
| def get_enhanced_path() -> str: | ||
| """ | ||
| Get an enhanced PATH that includes common Node.js/npm/npx locations. | ||
| Electron apps launched from GUI don't inherit shell PATH modifications | ||
| from .bashrc/.zshrc or Node version managers (nvm, fnm, volta, asdf). | ||
| This function adds common locations to ensure npx/npm are found. | ||
| Fixes Issue #523: Cannot add custom mcp servers using npx | ||
| Returns: | ||
| Enhanced PATH string with additional Node.js locations | ||
| """ | ||
| current_path = os.environ.get("PATH", "") | ||
| home = Path.home() | ||
| additional_paths: list[str] = [] | ||
|
|
||
| if platform.system() == "Windows": | ||
| # Windows common locations | ||
| appdata = os.environ.get("APPDATA", "") | ||
| programfiles = os.environ.get("PROGRAMFILES", "C:\\Program Files") | ||
| programfiles_x86 = os.environ.get( | ||
| "PROGRAMFILES(X86)", "C:\\Program Files (x86)" | ||
| ) | ||
| nvm_home = os.environ.get("NVM_HOME", "") | ||
|
|
||
| candidates = [ | ||
| Path(appdata) / "npm" if appdata else None, | ||
| Path(programfiles) / "nodejs", | ||
| Path(programfiles_x86) / "nodejs", | ||
| home / "AppData" / "Roaming" / "npm", | ||
| home / "AppData" / "Local" / "fnm_multishells", | ||
| home / ".volta" / "bin", | ||
| ] | ||
|
|
||
| # Add nvm-windows paths | ||
| if nvm_home: | ||
| nvm_path = Path(nvm_home) | ||
| if nvm_path.exists(): | ||
| # Find installed node versions | ||
| for version_dir in nvm_path.iterdir(): | ||
| if version_dir.is_dir() and version_dir.name.startswith("v"): | ||
| candidates.append(version_dir) | ||
|
|
||
| else: | ||
| # macOS / Linux common locations | ||
| candidates = [ | ||
| Path("/usr/local/bin"), | ||
| Path("/usr/bin"), | ||
| Path("/opt/homebrew/bin"), # Apple Silicon Homebrew | ||
| home / ".local" / "bin", | ||
| home / ".volta" / "bin", | ||
| home / ".fnm" / "current" / "bin", | ||
| home / ".asdf" / "shims", | ||
| home / ".npm-global" / "bin", | ||
| home / "node_modules" / ".bin", | ||
| ] | ||
|
|
||
| # Add nvm paths (find installed versions) | ||
| nvm_dir = home / ".nvm" / "versions" / "node" | ||
| if nvm_dir.exists(): | ||
| for version_dir in nvm_dir.iterdir(): | ||
| if version_dir.is_dir(): | ||
| candidates.append(version_dir / "bin") | ||
|
|
||
| # Add fnm paths | ||
| fnm_dir = home / ".fnm" / "node-versions" | ||
| if fnm_dir.exists(): | ||
| for version_dir in fnm_dir.iterdir(): | ||
| if version_dir.is_dir(): | ||
| candidates.append(version_dir / "installation" / "bin") | ||
|
|
||
| # Filter to existing directories not already in PATH | ||
| path_entries = set(current_path.split(os.pathsep)) | ||
| for candidate in candidates: | ||
| if candidate and candidate.exists() and str(candidate) not in path_entries: | ||
| additional_paths.append(str(candidate)) | ||
|
|
||
| # Prepend additional paths (higher priority than existing) | ||
| if additional_paths: | ||
| return os.pathsep.join(additional_paths) + os.pathsep + current_path | ||
|
|
||
| return current_path |
There was a problem hiding this comment.
The get_enhanced_path function is quite long and contains complex, platform-specific logic. To improve readability and maintainability, consider refactoring this into smaller helper functions. For example, you could have separate functions like _get_windows_node_paths and _get_unix_node_paths to encapsulate the platform-specific path discovery.
| # Check if any removed symbol appears in other task's new code | ||
| for removed_symbol in removals_a: | ||
| for added_code in additions_b: | ||
| if removed_symbol in added_code: |
There was a problem hiding this comment.
The check if removed_symbol in added_code: is a good first step for detecting implicit conflicts, but it could lead to false positives. For instance, if a function getUser is removed, this check would flag any occurrence of the substring "getUser", including in variable names like getUsers or comments.
To make this more robust, consider using a regular expression with word boundaries to ensure you're matching the whole symbol, like re.search(rf'\b{re.escape(removed_symbol)}\b', added_code).
AlexMadera
left a comment
AlexMadera
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
10 issue(s) must be addressed (2 required, 8 recommended), 2 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | Low | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- High: 2 issue(s)
- Medium: 8 issue(s)
- Low: 2 issue(s)
Generated by Auto Claude PR Review
Findings (12 selected of 12 total)
🟠 [HIGH] Substring matching causes false positives in conflict detection
📁 auto-claude/merge/conflict_analysis.py:325
The detect_implicit_conflicts() function uses if removed_symbol in added_code which is simple substring matching. This causes false positives when a removed symbol name is a substring of another identifier (e.g., removing 'get' matches 'getUser', 'getConfig', 'forget'). This was flagged by Gemini and validated by both logic-reviewer and ai-triage-reviewer.
Suggested fix:
Use word boundary matching with regex: `import re; if re.search(rf'\b{re.escape(removed_symbol)}\b', added_code)` to ensure the symbol is matched as a complete identifier, not as a substring.
🟠 [HIGH] Biome config conflicts with existing ESLint and codebase style
📁 auto-claude-ui/biome.json:1
Adding Biome creates a dual-linter scenario with ESLint. Critical conflict: Biome uses 'semicolons: asNeeded' but existing codebase consistently uses explicit semicolons everywhere (task-store.ts, settings-store.ts, project-store.ts, Terminal.tsx). This causes visual inconsistency between newly formatted code (console-store.ts, agent-process.ts) and existing code.
Suggested fix:
Either: (1) Remove Biome and keep ESLint, (2) Fully migrate to Biome and format entire codebase, or (3) Change Biome config to 'semicolons: always' to match existing style. Document the decision.
🟡 [MEDIUM] Empty/whitespace strings treated as valid API credentials
📁 auto-claude-ui/src/main/ipc-handlers/context/utils.ts:142
The provider validation uses truthy checks that pass for whitespace-only strings like ' ' or empty strings from env vars that exist but have no value. The hasOpenAIKey, hasAzureKey checks use simple || operators which accept any truthy string including whitespace.
Suggested fix:
Add explicit validation with .trim().length > 0 check: `const key = projectEnvVars['OPENAI_API_KEY']?.trim() || globalSettings.globalOpenAIApiKey?.trim() || process.env.OPENAI_API_KEY?.trim(); if (key && key.length > 0) { ... }`
🟡 [MEDIUM] Git commands with string interpolation allow potential injection
📁 auto-claude-ui/src/main/ipc-handlers/task/worktree-handlers.ts:87
Multiple execSync calls use string interpolation with branch variables: git rev-list --count ${baseBranch}..HEAD. If baseBranch could be controlled by malicious input (e.g., crafted branch name with shell metacharacters), this could allow command injection. Similar issues at lines 104, 189, 196, 847, 943, 960.
Suggested fix:
Use spawnSync with array arguments instead of execSync with string interpolation: spawnSync('git', ['rev-list', '--count', `${baseBranch}..HEAD`], {...})
🟡 [MEDIUM] TOCTOU race condition in TASK_RECREATE handler
📁 auto-claude-ui/src/main/ipc-handlers/task/execution-handlers.ts:896
The handler checks agentManager.isRunning(taskId) at line 896, but another IPC message (like TASK_START) could start the task between this check and subsequent worktree/spec deletion operations. This could cause partial deletion while a task is executing.
Suggested fix:
Use a mutex/lock or call agentManager.killTask(taskId) unconditionally before deletion to ensure no process is running. Alternatively, implement atomic 'recreate' operation in AgentManager.
🟡 [MEDIUM] Wildcard tool permissions may be overly permissive
📁 auto-claude/core/client.py:246
The security_settings permissions include wildcards like 'WebSearch()', 'WebFetch()', 'Task(*)'. While Bash has a security hook for validation, the wildcard permissions for web tools could allow unintended operations - WebSearch/WebFetch could potentially be used to exfiltrate data or access internal resources.
Suggested fix:
Consider restricting WebFetch to specific domains or implementing a URL allowlist/blocklist. Document the security implications of these permissions.
🟡 [MEDIUM] Duplicate .env parsing logic across files
📁 auto-claude-ui/src/main/agent/agent-process.ts:107
The loadAutoBuildEnv method (lines 108-152) implements the exact same .env parsing logic that exists in utils.ts parseEnvFile function. This duplication means bug fixes or improvements need to be applied in multiple places.
Suggested fix:
Import and use parseEnvFile from '../ipc-handlers/context/utils' instead of duplicating the parsing logic. This reduces maintenance burden and ensures consistent parsing behavior.
🟡 [MEDIUM] Async methods resolve(false) instead of rejecting on failure
📁 auto-claude-ui/src/main/python-env-manager.ts:156
Methods createVenv, bootstrapPip, and installDeps are marked async but resolve(false) on failure instead of rejecting. The initialize method wraps them in try/catch but they will never throw. This anti-pattern mixes error-as-value with try/catch semantics.
Suggested fix:
Either: (1) Change methods to reject on failure instead of resolve(false), OR (2) Make methods non-async and return Promise<boolean> directly with proper error handling in the caller.
🟡 [MEDIUM] ScrollArea ref may not work as expected
📁 auto-claude-ui/src/renderer/components/ConsoleLogsPanel.tsx:216
The ScrollArea component receives a ref at line 216, but the scroll behavior in useEffect accesses scrollRef.current.scrollTop. shadcn/ui ScrollArea component may not forward refs to the scrollable element by default, which could cause the auto-scroll feature to fail.
Suggested fix:
Verify that ScrollArea properly forwards the ref to the scrollable viewport. If not, use ScrollArea's viewport slot or wrap content in a div with the ref and handle scrolling on that element.
🔵 [LOW] Inline require() usage inconsistent with top-level imports
📁 auto-claude-ui/src/main/python-env-manager.ts:435
Line 435 uses require('node:child_process').spawnSync when the file already imports spawn and execSync from child_process at line 1. This inconsistency makes the codebase harder to maintain.
Suggested fix:
Add spawnSync to the existing import: import { execFileSync, execSync, spawn, spawnSync } from 'node:child_process' and use it directly instead of require.
🟡 [MEDIUM] Rule lookup only checks one direction of change type pair
📁 auto-claude/merge/conflict_analysis.py:160
The rule lookup uses rule_index.get((type_a, type_b)) but doesn't check the reverse order (type_b, type_a). Valid compatible pairs might be incorrectly marked as incompatible if rules are only stored in one direction.
Suggested fix:
Check both directions: `rule = rule_index.get((type_a, type_b)) or rule_index.get((type_b, type_a))`. Alternatively, ensure the rule_index is populated symmetrically during initialization.
🔵 [LOW] Deprecated getVenvPipPath method should be removed
📁 auto-claude-ui/src/main/python-env-manager.ts:66
The method getVenvPipPath at line 66 is marked @deprecated and simply returns null. Dead code that serves no purpose should be removed to improve maintainability.
Suggested fix:
Remove the deprecated getVenvPipPath method entirely since it always returns null and is not used.
This review was generated by Auto Claude.
MikeeBuilds
added
feature
|
Similar to PR #559, this PR has a complex git history with commits from other feature branches merged in (claude-cli-path, biome, console-logs, etc.). Issues during rebase:
Recommendation: |
Analysis CompleteAfter reviewing the current Already in develop:
This PR's additional changes:
Since the core multi-provider embedding validation is already in develop, this PR is superseded. Closing as the functionality has been implemented in the codebase restructure. |
|
Closing as superseded - the multi-provider embedding validation functionality is already in develop. See analysis comment above. |
3 participants
Summary
Extends the memory status system in the UI to validate credentials for all supported Graphiti embedder providers, not just OpenAI. This enables proper user feedback when using alternative embedding providers.
Changes
utils.tsAdded
getEmbedderProviderStatus()function that:GRAPHITI_EMBEDDER_PROVIDERenv var (defaults to 'openai')OPENAI_API_KEYVOYAGE_API_KEYOLLAMA_EMBEDDING_MODELGOOGLE_API_KEYExtended
GlobalSettingsinterface with new API key fields:globalAnthropicApiKeyglobalAzureOpenAIApiKeyglobalVoyageApiKeyglobalGoogleApiKeymemory-status-handlers.tsbuildMemoryStatus()to use provider-aware validationRelated PRs
Test Plan
GRAPHITI_ENABLED=truewithout any API keys → should show provider-specific errorGRAPHITI_EMBEDDER_PROVIDER=voyagewithVOYAGE_API_KEY→ should show availableGRAPHITI_EMBEDDER_PROVIDER=ollamawithOLLAMA_EMBEDDING_MODEL→ should show availableOPENAI_API_KEY→ should work as before