feat(cli): route observability through GuardScan-Monitoring API#30
feat(cli): route observability through GuardScan-Monitoring API#30ntanwir10 wants to merge 7 commits into
Conversation
…g API Centralize the remote API base URL via getGuardscanMonitoringBaseUrl and derive monitoring endpoint defaults from telemetry/offline config so remote observability uses the same GuardScan-Monitoring worker as telemetry. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
guardscan-backend | 88a0f35 | Jun 10 2026, 05:14 AM |
![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 refactors telemetry and monitoring base URL resolution by introducing a shared helper function and dynamically resolving configuration options in the MonitoringManager constructor. The review feedback highlights a critical privacy issue where the application could default to online mode if the configuration file is missing or fails to load, and suggests a cleaner, more idiomatic way to construct the merged configuration object without mutating it post-creation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![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)
cli/src/utils/monitoring.ts (1)
233-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential crash if config file doesn't exist.
The constructor gracefully handles missing YAML config (lines 118-122 with try-catch), but
trackUsageunconditionally callsthis.configManager.load()which throws if the config file is missing (perConfigManager.load()documentation). This inconsistency could causetrackUsageto crash in environments whereguardscan inithasn't been run yet, even though the constructor succeeds.🛡️ Suggested fix
- const userConfig = this.configManager.load(); + let userConfig; + try { + userConfig = this.configManager.load(); + } catch { + // Config not initialized, skip usage tracking + return; + }🤖 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 `@cli/src/utils/monitoring.ts` at line 233, trackUsage calls this.configManager.load() unconditionally which can throw if no config exists; wrap the load call in a try/catch (or check for existence via a provided API) inside trackUsage so a missing config falls back to null/empty config and does not propagate an exception. Specifically, update the trackUsage method to call this.configManager.load() inside a try block, handle/load defaults in the catch, and ensure subsequent logic that reads userConfig (the variable currently assigned from this.configManager.load()) handles the null/default case; reference the trackUsage function and ConfigManager.load for locating the change.
🧹 Nitpick comments (1)
cli/src/utils/monitoring.ts (1)
130-139: ⚡ Quick winConsider restructuring the config merge for clarity.
The current code spreads
customat line 137, then explicitly overwritesendpointat line 139. While functionally correct, this pattern is a bit redundant for theendpointfield—the spread already assignscustom.endpointif it exists, then line 139 reassigns it with the same fallback logic.For better clarity, consider moving the endpoint assignment into the object literal itself:
♻️ Suggested restructure
- const merged: MonitoringConfig = { - enabled: telemetryOn, - endpoint: inferredEndpoint, - errorReportingEnabled: true, - performanceMonitoringEnabled: true, - usageAnalyticsEnabled: true, - sampleRate: 1.0, - ...custom, - }; - merged.endpoint = custom?.endpoint ?? inferredEndpoint; + const merged: MonitoringConfig = { + enabled: telemetryOn, + errorReportingEnabled: true, + performanceMonitoringEnabled: true, + usageAnalyticsEnabled: true, + sampleRate: 1.0, + ...custom, + endpoint: custom?.endpoint ?? inferredEndpoint, + };This makes it explicit that
endpointhas special fallback handling and avoids the redundant assignment.🤖 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 `@cli/src/utils/monitoring.ts` around lines 130 - 139, The merge of MonitoringConfig builds `merged` by spreading `custom` then immediately reassigning `merged.endpoint`; update the object literal for `merged` (the const merged: MonitoringConfig = { ... }) to set `endpoint: custom?.endpoint ?? inferredEndpoint` inside the initial object and remove the subsequent `merged.endpoint = ...` line; this keeps `telemetryOn`, `inferredEndpoint`, `custom`, and the `endpoint` fallback behavior intact while eliminating the redundant reassignment.
🤖 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.
Outside diff comments:
In `@cli/src/utils/monitoring.ts`:
- Line 233: trackUsage calls this.configManager.load() unconditionally which can
throw if no config exists; wrap the load call in a try/catch (or check for
existence via a provided API) inside trackUsage so a missing config falls back
to null/empty config and does not propagate an exception. Specifically, update
the trackUsage method to call this.configManager.load() inside a try block,
handle/load defaults in the catch, and ensure subsequent logic that reads
userConfig (the variable currently assigned from this.configManager.load())
handles the null/default case; reference the trackUsage function and
ConfigManager.load for locating the change.
---
Nitpick comments:
In `@cli/src/utils/monitoring.ts`:
- Around line 130-139: The merge of MonitoringConfig builds `merged` by
spreading `custom` then immediately reassigning `merged.endpoint`; update the
object literal for `merged` (the const merged: MonitoringConfig = { ... }) to
set `endpoint: custom?.endpoint ?? inferredEndpoint` inside the initial object
and remove the subsequent `merged.endpoint = ...` line; this keeps
`telemetryOn`, `inferredEndpoint`, `custom`, and the `endpoint` fallback
behavior intact while eliminating the redundant reassignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79cf18c7-dee6-4ee2-afc0-160f75b3a4ec
📒 Files selected for processing (3)
cli/src/utils/api-client.tscli/src/utils/monitoring-base-url.tscli/src/utils/monitoring.ts
1 participant
Summary
getGuardscanMonitoringBaseUrl()helper (GUARDSCAN_API_URLor defaultapi.guardscancli.com)telemetryEnabled,offlineMode, andGUARDSCAN_NO_TELEMETRYwhen inferring remote endpointsStacks on: #29
Test plan
api.guardscancli.com/api/telemetryand/api/monitoring--no-telemetry/ offline mode, confirm no remote endpoint is configuredSummary by CodeRabbit