Mirror: fix: reduce console noise for unconfigured services (#5647)#21
Conversation
Prevents error logging when IO Intelligence is not configured by returning empty model record early instead of throwing error.
Prevents 'Invalid URL' error when LiteLLM is not configured by returning empty model record early instead of attempting URL construction.
Prevents 'SAP AI Core service key is required' error when the provider isn't configured by returning empty model/deployment record early.
Prevents console spam by checking CloudService.hasInstance() before accessing CloudService.instance properties in getState(). Since Kilo Code doesn't initialize CloudService (inherited from Roo), these errors are expected and should be silently ignored.
Summary of ChangesHello @jeremylongshore, 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 reduces console noise by implementing more graceful error handling for unconfigured external services. Instead of throwing errors or logging them when API keys or base URLs are missing, the system now returns empty objects or silently skips operations, improving the developer experience and making legitimate issues easier to spot. Additionally, error logging for uninitialized Highlights
Changelog
Activity
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
|
📝 WalkthroughWalkthroughThis PR reduces console noise by implementing early returns in service fetchers to gracefully handle missing credentials or configuration, returning empty defaults instead of throwing errors or attempting network calls. CloudService integration is also silenced in ClineProvider with defaults used instead. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
Review Summary by QodoReduce console noise for unconfigured services and providers
WalkthroughsDescription• Skip model fetching when required credentials are missing - IO Intelligence: return empty object if API key not provided - LiteLLM: return empty object if base URL missing or empty - SAP AI Core: return empty object if service key not provided • Silence CloudService errors by commenting out initialization code - Prevents console spam from uninitialized CloudService in Kilo Code - CloudService is inherited from Roo but never initialized • Update SAP AI Core tests to expect empty objects instead of errors Diagramflowchart LR
A["Missing Credentials"] --> B["Early Return Empty Object"]
C["CloudService Not Initialized"] --> D["Comment Out Error Handling"]
B --> E["Reduce Console Noise"]
D --> E
File Changes1. src/api/providers/fetchers/io-intelligence.ts
|
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/webview/ClineProvider.ts (1)
2338-2358:⚠️ Potential issue | 🟡 Minor
CloudService.instanceaccessed withouthasInstance()guard — same console noise risk.Lines 2341 and 2351 access
CloudService.instancedirectly (.isCloudAgent,.getOrganizationMemberships()). IfCloudServiceis not initialized, this will throw and be silently caught, but it's inconsistent with the pattern applied to the other seven blocks that were explicitly silenced. Consider adding aCloudService.hasInstance()guard or commenting out this block too for consistency.+ if (CloudService.hasInstance()) { try { if (!CloudService.instance.isCloudAgent) { // ...cache logic... cloudOrganizations = await CloudService.instance.getOrganizationMemberships() // ... } } catch (error) { // Ignore this error. } + }
🧹 Nitpick comments (2)
src/core/webview/ClineProvider.ts (2)
2619-2702: Large blocks of commented-out code reduce readability.These seven consecutive commented-out try/catch blocks (for
organizationAllowList,cloudUserInfo,cloudIsAuthenticated,sharingEnabled,publicSharingEnabled,organizationSettingsVersion,taskSyncEnabled) add ~80 lines of dead code. Since the defaults are already assigned on theletdeclarations above each block, consider removing the commented bodies entirely and adding a single explanatory comment, e.g.:// kilocode_change: CloudService is not initialized in this fork. // All cloud-derived state fields use their declared defaults above.This achieves the same intent without the visual noise.
2867-2897: Commented-out IIFE bodies are unnecessarily complex for returningfalse.Both
remoteControlEnabledandfeatureRoomoteControlEnabledare IIFEs that now just returnfalsewith ~10 lines of commented-out code each. Simplify to plainfalse:- remoteControlEnabled: (() => { - // try { - // const cloudSettings = CloudService.instance.getUserSettings() - // return cloudSettings?.settings?.extensionBridgeEnabled ?? false - // } catch (error) { - // ... - return false - // } - })(), + remoteControlEnabled: false, // kilocode_change: CloudService not initializedSame for
featureRoomoteControlEnabled.
Code Review by Qodo
1. SAP models silently skipped
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // kilocode_change start: Skip fetching if service key is not provided | ||
| if (!sapAiCoreServiceKey) { | ||
| return {} | ||
| } |
There was a problem hiding this comment.
1. Sap models silently skipped 🐞 Bug ✧ Quality
getSapAiCoreModels()/getSapAiCoreDeployments() now return {} when the service key is missing;
combined with the SAP AI Core models webview handler not passing the service key into getModels(),
SAP model fetching becomes a silent no-op instead of surfacing a misconfiguration/wiring error.
Users will see no models with no error signal, making the feature appear broken and harder to debug.
Agent Prompt
### Issue description
SAP AI Core fetchers now return `{}` when `sapAiCoreServiceKey` is missing. The webview handler `requestSapAiCoreModels` checks `message.values.sapAiCoreServiceKey`, but then calls `getModels({ provider: "sap-ai-core" })` without passing the service key/resource group/orchestration flag. With the new early-return behavior, this becomes a silent no-op (no models, no error).
### Issue Context
`modelCache.fetchModelsFromProvider()` passes `options.sapAiCoreServiceKey` into `getSapAiCoreModels()`. If the handler does not provide these options, the fetch will be skipped.
### Fix Focus Areas
- src/api/providers/fetchers/sap-ai-core.ts[287-305]
- src/api/providers/fetchers/modelCache.ts[146-152]
- src/core/webview/webviewMessageHandler.ts[1249-1267]
### Implementation notes
- In `requestSapAiCoreModels`, construct `GetModelsOptions` with:
- `sapAiCoreServiceKey: message.values.sapAiCoreServiceKey`
- `sapAiCoreResourceGroup: message.values.sapAiCoreResourceGroup`
- `sapAiCoreUseOrchestration: message.values.sapAiCoreUseOrchestration`
and pass that to both `flushModels()` and `getModels()`.
- Consider posting a webview response even when the result is empty to distinguish “not configured” from “no models”.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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 aims to reduce console noise from unconfigured services. The changes in io-intelligence.ts, litellm.ts, and sap-ai-core.ts correctly implement this by adding early returns. The corresponding test updates are also correct. However, the changes in src/core/webview/ClineProvider.ts are problematic. Instead of just silencing errors, they completely disable several features related to CloudService by commenting out functional code. This is a significant regression. I've added critical comments with suggestions to fix this by checking if CloudService is available before using it, which preserves functionality while still preventing console noise.
| // kilocode_change start: CloudService never initialized, silencing errors | ||
| // try { | ||
| // organizationAllowList = await CloudService.instance.getAllowList() | ||
| // } catch (error) { | ||
| // console.error( | ||
| // `[getState] failed to get organization allow list: ${error instanceof Error ? error.message : String(error)}`, | ||
| // ) | ||
| // } | ||
| // kilocode_change end |
There was a problem hiding this comment.
This change completely disables fetching the organization allow list by commenting out the logic. The goal is to reduce console noise for unconfigured services, but this change breaks the functionality even when the service is configured.
A better approach is to check if CloudService has an instance before attempting to use it, and then silently handle any errors. This preserves the functionality while still achieving the goal of reducing console noise.
| // kilocode_change start: CloudService never initialized, silencing errors | |
| // try { | |
| // organizationAllowList = await CloudService.instance.getAllowList() | |
| // } catch (error) { | |
| // console.error( | |
| // `[getState] failed to get organization allow list: ${error instanceof Error ? error.message : String(error)}`, | |
| // ) | |
| // } | |
| // kilocode_change end | |
| if (CloudService.hasInstance()) { | |
| try { | |
| organizationAllowList = await CloudService.instance.getAllowList() | |
| } catch (error) { | |
| // Silently fail if not configured or error, using the default value. | |
| } | |
| } |
| // kilocode_change start: CloudService never initialized, silencing errors | ||
| // try { | ||
| // cloudUserInfo = CloudService.instance.getUserInfo() | ||
| // } catch (error) { | ||
| // console.error( | ||
| // `[getState] failed to get cloud user info: ${error instanceof Error ? error.message : String(error)}`, | ||
| // ) | ||
| // } | ||
| // kilocode_change end |
There was a problem hiding this comment.
This change completely disables fetching cloud user info by commenting out the logic. While this reduces console noise, it also breaks the feature for users where CloudService is configured and available.
To fix this, the code should first check if CloudService.hasInstance() is true, and only then attempt to get the user info within a try...catch block that silences errors. This will prevent console noise for unconfigured services without removing the functionality for others.
| // kilocode_change start: CloudService never initialized, silencing errors | |
| // try { | |
| // cloudUserInfo = CloudService.instance.getUserInfo() | |
| // } catch (error) { | |
| // console.error( | |
| // `[getState] failed to get cloud user info: ${error instanceof Error ? error.message : String(error)}`, | |
| // ) | |
| // } | |
| // kilocode_change end | |
| if (CloudService.hasInstance()) { | |
| try { | |
| cloudUserInfo = CloudService.instance.getUserInfo() | |
| } catch (error) { | |
| // Silently fail if not configured or error, using the default value. | |
| } | |
| } |
| // kilocode_change start: CloudService never initialized, silencing errors | ||
| // try { | ||
| // cloudIsAuthenticated = CloudService.instance.isAuthenticated() | ||
| // } catch (error) { | ||
| // console.error( | ||
| // `[getState] failed to get cloud authentication state: ${error instanceof Error ? error.message : String(error)}`, | ||
| // ) | ||
| // } | ||
| // kilocode_change end |
There was a problem hiding this comment.
Commenting out this block completely disables checking the cloud authentication state. This is a regression that breaks functionality for users with a configured CloudService.
The correct approach is to wrap this logic in a check for CloudService.hasInstance() and a try...catch block to silently handle errors. This will suppress console noise for unconfigured services while keeping the feature intact for others.
| // kilocode_change start: CloudService never initialized, silencing errors | |
| // try { | |
| // cloudIsAuthenticated = CloudService.instance.isAuthenticated() | |
| // } catch (error) { | |
| // console.error( | |
| // `[getState] failed to get cloud authentication state: ${error instanceof Error ? error.message : String(error)}`, | |
| // ) | |
| // } | |
| // kilocode_change end | |
| if (CloudService.hasInstance()) { | |
| try { | |
| cloudIsAuthenticated = CloudService.instance.isAuthenticated() | |
| } catch (error) { | |
| // Silently fail if not configured or error, using the default value. | |
| } | |
| } |
| // kilocode_change start: CloudService never initialized, silencing errors | ||
| // try { | ||
| // sharingEnabled = await CloudService.instance.canShareTask() | ||
| // } catch (error) { | ||
| // console.error( | ||
| // `[getState] failed to get sharing enabled state: ${error instanceof Error ? error.message : String(error)}`, | ||
| // ) | ||
| // } | ||
| // kilocode_change end |
There was a problem hiding this comment.
This change disables the task sharing feature by commenting out the call to CloudService.instance.canShareTask(). To correctly silence errors from unconfigured services without breaking the feature, you should first check if CloudService.hasInstance() and then wrap the call in a try...catch block to handle potential errors silently.
| // kilocode_change start: CloudService never initialized, silencing errors | |
| // try { | |
| // sharingEnabled = await CloudService.instance.canShareTask() | |
| // } catch (error) { | |
| // console.error( | |
| // `[getState] failed to get sharing enabled state: ${error instanceof Error ? error.message : String(error)}`, | |
| // ) | |
| // } | |
| // kilocode_change end | |
| if (CloudService.hasInstance()) { | |
| try { | |
| sharingEnabled = await CloudService.instance.canShareTask() | |
| } catch (error) { | |
| // Silently fail if not configured or error, using the default value. | |
| } | |
| } |
| // kilocode_change start: CloudService never initialized, silencing errors | ||
| // try { | ||
| // publicSharingEnabled = await CloudService.instance.canSharePublicly() | ||
| // } catch (error) { | ||
| // console.error( | ||
| // `[getState] failed to get public sharing enabled state: ${error instanceof Error ? error.message : String(error)}`, | ||
| // ) | ||
| // } | ||
| // kilocode_change end |
There was a problem hiding this comment.
Commenting out this block disables the public task sharing feature entirely, which is a regression. The intended change is to reduce console noise, not remove functionality.
Please restore this logic, but wrap it in a CloudService.hasInstance() check and a try...catch block to silently handle cases where the service is not configured or unavailable.
| // kilocode_change start: CloudService never initialized, silencing errors | |
| // try { | |
| // publicSharingEnabled = await CloudService.instance.canSharePublicly() | |
| // } catch (error) { | |
| // console.error( | |
| // `[getState] failed to get public sharing enabled state: ${error instanceof Error ? error.message : String(error)}`, | |
| // ) | |
| // } | |
| // kilocode_change end | |
| if (CloudService.hasInstance()) { | |
| try { | |
| publicSharingEnabled = await CloudService.instance.canSharePublicly() | |
| } catch (error) { | |
| // Silently fail if not configured or error, using the default value. | |
| } | |
| } |
| // kilocode_change start: CloudService never initialized, silencing errors | ||
| // try { | ||
| // if (CloudService.hasInstance()) { | ||
| // const settings = CloudService.instance.getOrganizationSettings() | ||
| // organizationSettingsVersion = settings?.version ?? -1 | ||
| // } | ||
| // } catch (error) { | ||
| // console.error( | ||
| // `[getState] failed to get organization settings version: ${error instanceof Error ? error.message : String(error)}`, | ||
| // ) | ||
| // } | ||
| // kilocode_change end |
There was a problem hiding this comment.
This change disables fetching the organization settings version. To fix this while still preventing console noise, you should check for CloudService.hasInstance() before attempting to access the service. This ensures the feature works when configured and fails silently otherwise.
| // kilocode_change start: CloudService never initialized, silencing errors | |
| // try { | |
| // if (CloudService.hasInstance()) { | |
| // const settings = CloudService.instance.getOrganizationSettings() | |
| // organizationSettingsVersion = settings?.version ?? -1 | |
| // } | |
| // } catch (error) { | |
| // console.error( | |
| // `[getState] failed to get organization settings version: ${error instanceof Error ? error.message : String(error)}`, | |
| // ) | |
| // } | |
| // kilocode_change end | |
| if (CloudService.hasInstance()) { | |
| try { | |
| const settings = CloudService.instance.getOrganizationSettings() | |
| organizationSettingsVersion = settings?.version ?? -1 | |
| } catch (error) { | |
| // Silently fail if not configured or error, using the default value. | |
| } | |
| } |
| // kilocode_change start: CloudService never initialized, silencing errors | ||
| // try { | ||
| // taskSyncEnabled = CloudService.instance.isTaskSyncEnabled() | ||
| // } catch (error) { | ||
| // console.error( | ||
| // `[getState] failed to get task sync enabled state: ${error instanceof Error ? error.message : String(error)}`, | ||
| // ) | ||
| // } | ||
| // kilocode_change end |
There was a problem hiding this comment.
Commenting out this block disables the task sync feature. The correct way to silence errors for unconfigured services is to check CloudService.hasInstance() before making the call, and then handle any errors silently within a try...catch block. This preserves the functionality for users who have the service configured.
| // kilocode_change start: CloudService never initialized, silencing errors | |
| // try { | |
| // taskSyncEnabled = CloudService.instance.isTaskSyncEnabled() | |
| // } catch (error) { | |
| // console.error( | |
| // `[getState] failed to get task sync enabled state: ${error instanceof Error ? error.message : String(error)}`, | |
| // ) | |
| // } | |
| // kilocode_change end | |
| if (CloudService.hasInstance()) { | |
| try { | |
| taskSyncEnabled = CloudService.instance.isTaskSyncEnabled() | |
| } catch (error) { | |
| // Silently fail if not configured or error, using the default value. | |
| } | |
| } |
| // kilocode_change start: CloudService never initialized, silencing errors | ||
| remoteControlEnabled: (() => { | ||
| try { | ||
| const cloudSettings = CloudService.instance.getUserSettings() | ||
| return cloudSettings?.settings?.extensionBridgeEnabled ?? false | ||
| } catch (error) { | ||
| console.error( | ||
| `[getState] failed to get remote control setting from cloud: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| return false | ||
| } | ||
| // try { | ||
| // const cloudSettings = CloudService.instance.getUserSettings() | ||
| // return cloudSettings?.settings?.extensionBridgeEnabled ?? false | ||
| // } catch (error) { | ||
| // console.error( | ||
| // `[getState] failed to get remote control setting from cloud: ${error instanceof Error ? error.message : String(error)}`, | ||
| // ) | ||
| return false | ||
| // } | ||
| })(), | ||
| // kilocode_change end |
There was a problem hiding this comment.
This change hardcodes remoteControlEnabled to false, which disables the feature for all users. To correctly suppress console noise for unconfigured services, please check for CloudService.hasInstance() before attempting to get user settings. This will ensure the feature remains functional for configured users while failing silently for others.
remoteControlEnabled: (() => {
if (CloudService.hasInstance()) {
try {
const cloudSettings = CloudService.instance.getUserSettings()
return cloudSettings?.settings?.extensionBridgeEnabled ?? false
} catch (error) {
// Silently fail if not configured or error, using the default value.
}
}
return false
})(),| // kilocode_change start: CloudService never initialized, silencing errors | ||
| featureRoomoteControlEnabled: (() => { | ||
| try { | ||
| const userSettings = CloudService.instance.getUserSettings() | ||
| const hasOrganization = cloudUserInfo?.organizationId != null | ||
| return hasOrganization || (userSettings?.features?.roomoteControlEnabled ?? false) | ||
| } catch (error) { | ||
| console.error( | ||
| `[getState] failed to get featureRoomoteControlEnabled: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| return false | ||
| } | ||
| // try { | ||
| // const userSettings = CloudService.instance.getUserSettings() | ||
| // const hasOrganization = cloudUserInfo?.organizationId != null | ||
| // return hasOrganization || (userSettings?.features?.roomoteControlEnabled ?? false) | ||
| // } catch (error) { | ||
| // console.error( | ||
| // `[getState] failed to get featureRoomoteControlEnabled: ${error instanceof Error ? error.message : String(error)}`, | ||
| // ) | ||
| return false | ||
| // } | ||
| })(), | ||
| // kilocode_change end |
There was a problem hiding this comment.
This change effectively disables featureRoomoteControlEnabled by hardcoding its value to false. The correct approach to silence errors is to check if CloudService is available before trying to access its properties. This prevents breaking the feature for users who have it configured.
featureRoomoteControlEnabled: (() => {
if (CloudService.hasInstance()) {
try {
const userSettings = CloudService.instance.getUserSettings()
const hasOrganization = cloudUserInfo?.organizationId != null
return hasOrganization || (userSettings?.features?.roomoteControlEnabled ?? false)
} catch (error) {
// Silently fail if not configured or error, using the default value.
}
}
return false
})(),
Review: kilocode Kilo-Org#5647
Checklist
FindingsYELLOW - CloudService code commented out instead of guarded (ClineProvider.ts:2619-2900) The PR comments out ~80 lines of CloudService calls in YELLOW - Two unrelated changes bundled together The PR addresses two distinct issues: (1) provider fetchers throwing errors when API keys/URLs are missing (IO Intelligence, LiteLLM, SAP AI Core), and (2) CloudService calls in ClineProvider.getState() producing console errors. These should ideally be separate PRs since the provider changes are clean upstream-compatible fixes while the CloudService changes are Kilo-specific code commenting. GREEN - Provider early returns are clean and correct The IO Intelligence, LiteLLM, and SAP AI Core changes follow a consistent pattern: check if the required configuration is present, return GREEN - IO Intelligence refactoring is a genuine improvement Beyond the early return, the IO Intelligence change removes the else branch that threw an error and simplifies the header construction. Since CI Status
Code SnippetsProvider early return pattern (clean, consistent): // src/api/providers/fetchers/litellm.ts
export async function getLiteLLMModels(apiKey: string, baseUrl: string): Promise<ModelRecord> {
if (!baseUrl || baseUrl.trim() === "") {
return {}
}
// ... rest of function
}CloudService commenting (blunt but functional): // src/core/webview/ClineProvider.ts - getState()
// kilocode_change start: CloudService never initialized, silencing errors
// try {
// organizationAllowList = await CloudService.instance.getAllowList()
// } catch (error) { ... }
// kilocode_change endVerdictCOMMENT - The provider early returns (IO Intelligence, LiteLLM, SAP AI Core) are clean, correct, and improve the codebase. These are worth merging. The CloudService commenting in ClineProvider is functional but crude - using |
Review Journal: kilocode Kilo-Org#5647
SummaryTwo-part PR: (1) clean early-return guards in IO Intelligence, LiteLLM, and SAP AI Core fetchers when required config is missing, and (2) commenting out CloudService calls in ClineProvider.getState() that spam console errors when CloudService is uninitialized. The provider fixes are solid; the CloudService approach is functional but could use First ImpressionsGood PR description. Clear problem statement (console spam on startup), clear solution pattern (return early with empty result). Six files touched across providers and ClineProvider. Author (@markijbema) appears to understand the codebase well. What I Looked At
AnalysisProvider fetcher changes (clean) All three providers follow the same pattern: if required configuration (API key, base URL, service key) is missing, return
The IO Intelligence change also simplifies the header construction by removing a now-unreachable else branch. CloudService commenting (functional but crude) The upstream code wraps each
SAP AI Core test updates Tests correctly changed from VerificationCI is all green: compile, test-extension (ubuntu + windows), test-webview (ubuntu + windows), test-cli, test-jetbrains, build-cli, check-translations all pass. Lessons Learned
Review methodology: AI PR Review Case Studies | Reviewed with GWI + Claude Code |
2 participants
Mirror of Kilo-Org#5647
This PR mirrors the upstream change for multi-AI review analysis.
Bot Review Tracker
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests