Repository navigation
fix(cli): harden provider command detection and routing - #157
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds cross-platform CLI discovery and resolution (including path/version), a spawnCli helper for safe child execution, updates Claude/Gemini/OpenAI providers to use it, threads explicit providerType from UI through API to the orchestrator, and adds tests verifying CLI resolution and provider factory behavior. ChangesCLI Resolution and Provider Type Selection
🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node-backend/lib/ai.mjs`:
- Around line 13-16: The helper withDetectedCommand currently probes fallback
CLI commands even when the user provided an explicit config.command; change it
so that if config.command is present you immediately return config unchanged (do
not call resolveCliCommand), and only call resolveCliCommand with the fallback
commands (the rest of the variadic commands) when config.command is absent;
update logic in withDetectedCommand to build candidates only from ...commands
and set config.command = detected.path only when config.command was originally
undefined and detected.installed is true.
In `@node-backend/lib/orchestrator.mjs`:
- Around line 34-40: The code lets providerType override saved settings without
validation, which can cause typos to silently fall back to HostedAPI; update the
logic around activeProvider to validate providerType before using it: if
providerType is present, call a validator (or AIService.getSupportedProviders)
to ensure it's a known provider and throw or return an error for unsupported
IDs; alternatively call AIService.createProvider with a candidate and then use
provider.type (or provider.id) to normalize activeProvider from the instantiated
provider instead of trusting the raw providerType; reference the variables
activeProvider, providerType, settings and the call
AIService.createProvider(...) as the locations to implement this
validation/normalization and error handling.
In `@node-backend/lib/providers/gemini.mjs`:
- Around line 70-78: In checkAuthentication, don't unconditionally return true;
instead only allow the CLI fallback when the Gemini CLI is positively detected
(e.g., add or call a detection helper like hasGeminiCLI or run a quick `gemini
--version` probe) and otherwise return false so preflight fails when neither an
API key (this.config.apiKeySecretId / this.secrets['GEMINI_API_KEY'] /
this.secrets['GOOGLE_API_KEY']) nor a confirmed CLI login is present; update the
method (checkAuthentication) to gate the current fallback on that CLI detection
and return false when detection fails so setup issues surface before chat().
In `@node-backend/tests/parity.test.mjs`:
- Around line 57-77: The test mutates PATH but doesn't isolate
HOME/PROJECTS_DIR; update the test around AIService.createProvider to set
process.env.HOME and process.env.PROJECTS_DIR to temporary directories (created
via fs.mkdtemp) before calling AIService.createProvider('geminiCli', ...) and
restore the original values in the finally block (alongside resetting PATH and
cleaning temp dirs); ensure any created temp dirs are removed and that
provider.config.command assertion remains unchanged.
In `@node-backend/tests/services/settings-service.test.mjs`:
- Around line 9-20: Tests sandbox only APP_DATA_DIR and PATH but not HOME and
PROJECTS_DIR, so the suite can still touch real user data; update the
beforeEach/afterEach in this test file to create temp directories (e.g., using
fs.mkdtemp like tempAppData) for HOME and PROJECTS_DIR, stash their original
values (add originalHome and originalProjectsDir alongside originalPath), set
process.env.HOME and process.env.PROJECTS_DIR to those temp dirs in beforeEach,
and in afterEach delete the temp dirs, delete the test env vars and restore
originalHome, originalProjectsDir and originalPath so the environment is fully
isolated for the settings-service.test.mjs suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2668e467-a430-4262-872d-ba16267b6568
📒 Files selected for processing (13)
node-backend/lib/ai.mjsnode-backend/lib/orchestrator.mjsnode-backend/lib/providers/base.mjsnode-backend/lib/providers/claude.mjsnode-backend/lib/providers/gemini.mjsnode-backend/lib/providers/openai.mjsnode-backend/lib/system.mjsnode-backend/server.mjsnode-backend/tests/parity.test.mjsnode-backend/tests/services/settings-service.test.mjssrc/api/runtime.tssrc/api/server.tssrc/components/workspace/ChatPanel.tsx
AssafMiron
previously approved these changes
May 8, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/runtime.ts (1)
543-546: 💤 Low value
productOS:event namespace differs from the existingproductos:convention.All other window events in this file use the
productos:prefix (e.g.'productos:settings-changed'), but the new event uses'productOS:provider-switched'(capital O, capital S). Both currently work because the dispatch and the listener inChatPanel.tsxuse the same literal, but the inconsistency will confuse future contributors.♻️ Proposed alignment
- window.dispatchEvent(new CustomEvent('productOS:provider-switched', { detail: { providerType } })); + window.dispatchEvent(new CustomEvent('productos:provider-switched', { detail: { providerType } }));Corresponding change in
ChatPanel.tsxlines 256-257:- window.addEventListener('productOS:provider-switched', handleProviderSwitched); - return () => window.removeEventListener('productOS:provider-switched', handleProviderSwitched); + window.addEventListener('productos:provider-switched', handleProviderSwitched); + return () => window.removeEventListener('productos:provider-switched', handleProviderSwitched);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/runtime.ts` around lines 543 - 546, The new window event uses an inconsistent namespace "productOS:provider-switched" versus the project's "productos:" convention; update the dispatch to use the existing namespace so listeners match project style. Change the CustomEvent name in the dispatch inside the method that calls saveGlobalSettings (the code that creates new CustomEvent('productOS:provider-switched', { detail: { providerType } })) to use the "productos:" prefix (e.g., 'productos:provider-switched'), and ensure any related listener references (such as the handler in ChatPanel.tsx) are using the same 'productos:provider-switched' string so dispatch and listeners remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/api/runtime.ts`:
- Around line 543-546: The new window event uses an inconsistent namespace
"productOS:provider-switched" versus the project's "productos:" convention;
update the dispatch to use the existing namespace so listeners match project
style. Change the CustomEvent name in the dispatch inside the method that calls
saveGlobalSettings (the code that creates new
CustomEvent('productOS:provider-switched', { detail: { providerType } })) to use
the "productos:" prefix (e.g., 'productos:provider-switched'), and ensure any
related listener references (such as the handler in ChatPanel.tsx) are using the
same 'productos:provider-switched' string so dispatch and listeners remain
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae8a09e8-c825-4f79-ae9c-77ae36a887e4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/api/runtime.tssrc/components/workspace/ChatPanel.tsx
AssafMiron
previously approved these changes
May 8, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node-backend/lib/providers/gemini.mjs`:
- Around line 71-81: checkAuthentication() returning cli.installed can falsely
indicate usable authentication; add a runtime fallback in the chat() method to
detect when the Gemini CLI call fails due to missing auth and surface a clear
error instructing the user to run "gemini auth login" or provide an API key.
Specifically, inside chat() (and any helper that invokes the CLI), catch CLI
invocation errors or parse CLI stderr for authentication-related messages, and
convert them into a user-friendly error that mentions running "gemini auth
login" or setting the apiKeySecretId/GOOGLE_API_KEY/GEMINI_API_KEY; keep
existing checkCli/checkAuthentication() behavior but ensure chat() fails fast
with the clearer guidance when the CLI is present but not logged in.
- Line 93: The metadata() implementation currently sets models to ['default']
when no config is present; replace that fallback so models is either an empty
array or a real Gemini model identifier—e.g., compute a value like const model =
this.config.model_alias || this.config.modelAlias || this.config.model || null
and then set models to model ? [model] : [] (or use a concrete default like
'gemini-2.0-flash' if your app requires a specific fallback) so API/UI consumers
never receive the misleading 'default' identifier.
- Around line 15-20: Add a clarifying inline comment in gemini.mjs next to the
configuredModel check (the block that pushes '--model' onto args) explaining why
the 'pro' value is excluded (e.g., because the Gemini CLI already defaults to
'pro' or to avoid overriding the CLI/account-compatible default), mirroring the
note pattern used in claude.mjs; update the comment near the const args /
configuredModel check to mention the '--model' flag, the 'pro' sentinel value,
and that omitting it preserves the CLI/account default behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ad22681-6af1-4069-855b-d95c115f6876
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
node-backend/lib/ai.mjsnode-backend/lib/orchestrator.mjsnode-backend/lib/providers/gemini.mjsnode-backend/tests/parity.test.mjsnode-backend/tests/services/settings-service.test.mjssrc/api/runtime.tssrc/components/workspace/ChatPanel.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/api/runtime.ts
- node-backend/lib/orchestrator.mjs
- node-backend/tests/parity.test.mjs
- node-backend/lib/ai.mjs
- node-backend/tests/services/settings-service.test.mjs
- src/components/workspace/ChatPanel.tsx
AssafMiron
previously approved these changes
May 9, 2026
…tor provider validation
AssafMiron
approved these changes
May 9, 2026
This was referenced May 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch hardens CLI provider command detection and routing across Codex, Claude, and Gemini, adds Windows-safe CLI spawning, and updates the related tests.
Summary by CodeRabbit
New Features
Bug Fixes
Tests