fixing dashboard errors of missing devices from stats cards #2411

Merged

mfreeman451 merged 14 commits from refs/pull/2411/head into main 2025-11-14 18:17:40 +00:00

Conversation 4 Commits 14 Files changed 71 +3907 -1477

mfreeman451 commented 2025-11-12 18:38:59 +00:00 (Migrated from github.com)

Owner

Copy link

Imported from GitHub pull request.

Original GitHub pull request: #1937
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1937
Original created: 2025-11-12T18:38:59Z
Original updated: 2025-11-14T18:18:29Z
Original head: carverauto/serviceradar:1936-bugdocker-agent-detail-showing-clientside-react-error
Original base: main
Original merged: 2025-11-14T18:17:40Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

• I have signed the DCO?
• The build completes without errors?
• All tests are passing when running make test?

---

PR Type

Bug fix, Enhancement

---

Description

• Fixed dashboard errors by improving poller device registration with source IP tracking

• Enhanced stats aggregator to fallback to service components when no canonical records exist

• Added device planner query validation to prevent aggregation queries from using planner

• Fixed React client-side errors in device detail and traces dashboard components

---

Diagram Walkthrough

flowchart LR
  A["Poller Status Updates"] -->|"Pass source IP"| B["storePollerStatus"]
  B -->|"Register with IP"| C["registerPollerAsDevice"]
  C -->|"Resolve IP from metadata"| D["Device Registry"]
  E["Stats Aggregator"] -->|"Fallback logic"| F["Service Components"]
  F -->|"Count when no canonical"| G["Device Stats"]
  H["Query Route"] -->|"Validate query type"| I["Device Planner"]
  I -->|"Skip aggregations"| J["SRQL Backend"]
  K["Device Detail"] -->|"Handle object arrays"| L["Discovery Sources"]
  M["Traces Dashboard"] -->|"Safe null handling"| N["Trace ID Display"]

File Walkthrough

	Relevant files
Bug fix	pollers.go Add source IP tracking to poller device registration          pkg/core/pollers.go Added hostIP parameter to storePollerStatus function to pass source IP from requests Enhanced registerPollerAsDevice to accept and resolve host IP with fallback logic checking ServiceRegistry metadata Removed redundant device registration from storePollerStatus to avoid duplicate registrations Updated all callers to pass req.SourceIp or empty string when registering pollers +27/-20  stats_aggregator.go Improve stats aggregator with service component fallback  pkg/core/stats_aggregator.go Refactored record processing into processRecord closure for better code organization Added nil checks before updating metadata counters to prevent nil pointer dereferences Implemented fallback logic to include service components when no canonical records exist Service components are now counted in stats only when they are the sole available records +49/-24  route.ts Add query validation for device planner routing                    web/src/app/api/query/route.ts Added extractPrimaryStream function to parse stream names from SRQL queries Added shouldUseDevicePlanner function to validate query eligibility for device planner Device planner is now skipped for aggregation queries (containing stats:) and non-device streams Prevents client-side React errors from invalid planner requests +46/-1    DeviceDetail.tsx Fix discovery sources handling for object arrays                  web/src/components/Devices/DeviceDetail.tsx Added DiscoverySource interface to handle structured discovery source objects Updated DeviceRecord type to support discovery sources as object arrays Enhanced ensureArray function to handle both string and object array formats Safely extracts source field from discovery source objects to prevent rendering errors +27/-3    TracesDashboard.tsx Add null safety handling for trace ID display                        web/src/components/Observability/TracesDashboard.tsx Added null/undefined safety checks for traceId parameter in TraceIdCell component Returns placeholder dash when trace ID is missing instead of rendering undefined Prevents clipboard copy operations when trace ID is empty Fixed whitespace formatting issues in component +15/-7
Tests	stats_aggregator_test.go Add test for service component fallback logic                        pkg/core/stats_aggregator_test.go Added new test TestStatsAggregatorFallsBackToServiceComponents to verify service component handling Test validates that service components are counted when they are the only available records Ensures proper device count aggregation for poller and agent service components +40/-0

---

Imported from GitHub pull request. Original GitHub pull request: #1937 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1937 Original created: 2025-11-12T18:38:59Z Original updated: 2025-11-14T18:18:29Z Original head: carverauto/serviceradar:1936-bugdocker-agent-detail-showing-clientside-react-error Original base: main Original merged: 2025-11-14T18:17:40Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Fixed dashboard errors by improving poller device registration with source IP tracking - Enhanced stats aggregator to fallback to service components when no canonical records exist - Added device planner query validation to prevent aggregation queries from using planner - Fixed React client-side errors in device detail and traces dashboard components ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Poller Status Updates"] -->|"Pass source IP"| B["storePollerStatus"] B -->|"Register with IP"| C["registerPollerAsDevice"] C -->|"Resolve IP from metadata"| D["Device Registry"] E["Stats Aggregator"] -->|"Fallback logic"| F["Service Components"] F -->|"Count when no canonical"| G["Device Stats"] H["Query Route"] -->|"Validate query type"| I["Device Planner"] I -->|"Skip aggregations"| J["SRQL Backend"] K["Device Detail"] -->|"Handle object arrays"| L["Discovery Sources"] M["Traces Dashboard"] -->|"Safe null handling"| N["Trace ID Display"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>pollers.go</strong><dd><code>Add source IP tracking to poller device registration</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> pkg/core/pollers.go <ul><li>Added <code>hostIP</code> parameter to <code>storePollerStatus</code> function to pass source IP <br>from requests<br> <li> Enhanced <code>registerPollerAsDevice</code> to accept and resolve host IP with <br>fallback logic checking ServiceRegistry metadata<br> <li> Removed redundant device registration from <code>storePollerStatus</code> to avoid <br>duplicate registrations<br> <li> Updated all callers to pass <code>req.SourceIp</code> or empty string when <br>registering pollers</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1937/files#diff-fe81e2a32f1ac64bcdc6f25f55c5fa918d17bad8c0546f2cf80c757ff4051816">+27/-20</a>&nbsp; </td> </tr> <tr> <td> <details> <summary><strong>stats_aggregator.go</strong><dd><code>Improve stats aggregator with service component fallback</code>&nbsp; </dd></summary> <hr> pkg/core/stats_aggregator.go <ul><li>Refactored record processing into <code>processRecord</code> closure for better <br>code organization<br> <li> Added nil checks before updating metadata counters to prevent nil <br>pointer dereferences<br> <li> Implemented fallback logic to include service components when no <br>canonical records exist<br> <li> Service components are now counted in stats only when they are the <br>sole available records</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1937/files#diff-09304e5f27f54a3a8fa2b57cc6a5485b9f7074d7554babde62b51a75753205e0">+49/-24</a>&nbsp; </td> </tr> <tr> <td> <details> <summary><strong>route.ts</strong><dd><code>Add query validation for device planner routing</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> web/src/app/api/query/route.ts <ul><li>Added <code>extractPrimaryStream</code> function to parse stream names from SRQL <br>queries<br> <li> Added <code>shouldUseDevicePlanner</code> function to validate query eligibility <br>for device planner<br> <li> Device planner is now skipped for aggregation queries (containing <br><code>stats:</code>) and non-device streams<br> <li> Prevents client-side React errors from invalid planner requests</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1937/files#diff-53eee72ee62c074b7ff0b7556920802805621e9e16bfa91f85675ec55b66bf32">+46/-1</a>&nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>DeviceDetail.tsx</strong><dd><code>Fix discovery sources handling for object arrays</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> web/src/components/Devices/DeviceDetail.tsx <ul><li>Added <code>DiscoverySource</code> interface to handle structured discovery source <br>objects<br> <li> Updated <code>DeviceRecord</code> type to support discovery sources as object <br>arrays<br> <li> Enhanced <code>ensureArray</code> function to handle both string and object array <br>formats<br> <li> Safely extracts <code>source</code> field from discovery source objects to prevent <br>rendering errors</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1937/files#diff-2dbb02c57d308332683ec3795ef41e65387b5d1c30c559f01b535b5e704ba872">+27/-3</a>&nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>TracesDashboard.tsx</strong><dd><code>Add null safety handling for trace ID display</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> web/src/components/Observability/TracesDashboard.tsx <ul><li>Added null/undefined safety checks for <code>traceId</code> parameter in <br><code>TraceIdCell</code> component<br> <li> Returns placeholder dash when trace ID is missing instead of rendering <br>undefined<br> <li> Prevents clipboard copy operations when trace ID is empty<br> <li> Fixed whitespace formatting issues in component</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1937/files#diff-2ef490885a0b6e53d3fa243395f8209914ac8d955dca43333cb1f59c6eeb3d48">+15/-7</a>&nbsp; &nbsp; </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>stats_aggregator_test.go</strong><dd><code>Add test for service component fallback logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> pkg/core/stats_aggregator_test.go <ul><li>Added new test <code>TestStatsAggregatorFallsBackToServiceComponents</code> to <br>verify service component handling<br> <li> Test validates that service components are counted when they are the <br>only available records<br> <li> Ensures proper device count aggregation for poller and agent service <br>components</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1937/files#diff-af6bb2ab9ebdde96e719773736d19f6c570755b8d2517917bb761cc9ec29bb2a">+40/-0</a>&nbsp; &nbsp; </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented 2025-11-12 18:40:05 +00:00 (Migrated from github.com)

Author

Owner

Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1937#issuecomment-3523395883
Original created: 2025-11-12T18:40:05Z

PR Compliance Guide 🔍

(Compliance updated until commit github.com/carverauto/serviceradar@00abea0042)

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Sensitive information exposure Description: Potential logging of host IP addresses in info-level logs and capability events may expose sensitive infrastructure details; consider redacting or lowering log level where IPs are included. pollers.go [418-456] Referred Code func (s *Server) storePollerStatus(ctx context.Context, pollerID string, isHealthy bool, hostIP string, now time.Time) error { normIP := normalizeHostIP(hostIP) pollerStatus := &models.PollerStatus{ PollerID: pollerID, IsHealthy: isHealthy, LastSeen: now, HostIP: normIP, } if err := s.DB.UpdatePollerStatus(ctx, pollerStatus); err != nil { return fmt.Errorf("failed to store poller status: %w", err) } // Register poller as a device for inventory tracking (best-effort) if err := s.registerPollerAsDevice(ctx, pollerID, normIP); err != nil { s.logger.Warn(). Err(err). Str("poller_id", pollerID). Msg("Failed to register poller as device") } ... (clipped 18 lines)
	Insecure transport usage Description: Downloading onboarding package over HTTP client without explicit TLS configuration or certificate pinning could allow MITM if Core API URL is non-TLS or insecure; ensure HTTPS and proper TLS verification are enforced in runtime configs. download.go [169-213] Referred Code req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(body)) if err != nil { return fmt.Errorf("create download request: %w", err) } req.Header.Set("Content-Type", "application/json") req.Header.Set("Accept", "application/json") resp, err := b.getHTTPClient().Do(req) if err != nil { return fmt.Errorf("request core deliver endpoint: %w", err) } defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { msg := readErrorBody(resp.Body) if msg != "" { return fmt.Errorf("%w (%s): %s", ErrCoreDeliverEndpoint, resp.Status, msg) } return fmt.Errorf("%w returned %s", ErrCoreDeliverEndpoint, resp.Status) } ... (clipped 24 lines)
Ticket Compliance
🟡	🎫 #1936 🟢 Ensure dashboard/device-related components render without crashing. Provide a robust fix so UI pages that show agent details and traces work with null/array/object variations. Bug reproducible in Docker environment; fix should apply there too. ⚪ Fix client-side React error shown on agent detail page (React error
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing Audit Logs: New critical actions like registering pollers as devices and updating poller status do not emit explicit audit trail logs identifying actor/context beyond generic info logs, and may require structured auditing. Referred Code func (s *Server) storePollerStatus(ctx context.Context, pollerID string, isHealthy bool, hostIP string, now time.Time) error { normIP := normalizeHostIP(hostIP) pollerStatus := &models.PollerStatus{ PollerID: pollerID, IsHealthy: isHealthy, LastSeen: now, HostIP: normIP, } if err := s.DB.UpdatePollerStatus(ctx, pollerStatus); err != nil { return fmt.Errorf("failed to store poller status: %w", err) } // Register poller as a device for inventory tracking (best-effort) if err := s.registerPollerAsDevice(ctx, pollerID, normIP); err != nil { s.logger.Warn(). Err(err). Str("poller_id", pollerID). Msg("Failed to register poller as device") } ... (clipped 18 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Verbose Errors: Returned errors propagate raw response messages from Core API to CLI output which may expose internal details to end users depending on usage context. Referred Code if resp.StatusCode != http.StatusOK { message := readErrorBody(resp.Body) if message == "" { message = resp.Status } return fmt.Errorf("%w: %s", errCoreAPIError, message) } var packages []edgePackageView if err := json.NewDecoder(resp.Body).Decode(&packages); err != nil { return fmt.Errorf("decode list response: %w", err) } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Token In Logs: Info logs after downloading or loading packages include identifiers and context, and code handles join and download tokens where care is needed to avoid logging secrets though no explicit redaction is shown. Referred Code b.tokenInfo = tokenInfo b.logger.Info(). Str("package_id", pkg.PackageID). Str("component_id", pkg.ComponentID). Str("core_api_url", coreURL). Msg("Downloaded edge onboarding package") return nil Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: SQL Safety Check: The getUserByField function builds a query with a dynamic field name which could be unsafe without strict whitelisting though the value is parameterized; ensure field is strictly controlled. Referred Code // getUserByField retrieves a user by a specific field (e.g., id or email). func (db *DB) getUserByField(ctx context.Context, field, value string) (*models.User, error) { user := &models.User{} query := ` SELECT id, email, username AS name, provider, created_at, updated_at FROM users WHERE ` + field + ` = $1 LIMIT 1` Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

---

Previous compliance checks

Compliance check up to commit 6d99691

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #1936 🟢 E v y P q ⚪ - F i x c l e n t s d R a r o h w g / b p ( M f # 3 1 ) U I u m .
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Action Logging: New critical actions like registering pollers as devices and updating poller status are performed without explicit additional audit log entries beyond existing info/warn logs, making it unclear if audit requirements are met. Referred Code pollerStatus.FirstSeen = existingStatus.FirstSeen } if err := s.DB.UpdatePollerStatus(ctx, pollerStatus); err != nil { return fmt.Errorf("failed to update poller status: %w", err) } // Register poller as a device for inventory tracking if err := s.registerPollerAsDevice(ctx, pollerID, ""); err != nil { // Log but don't fail - device registration is best-effort s.logger.Warn(). Err(err). Str("poller_id", pollerID). Msg("Failed to register poller as device") } return nil } func (s *Server) updatePollerState( ctx context.Context, ... (clipped 879 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error Exposure: The API route proxies queries and may pass backend error messages through the SRQL response; ensure user-facing errors remain generic and detailed errors are only logged internally. Referred Code // but the Go backend will likely do it if it's missing. } if (cookieHeader) { headersToSrql["Cookie"] = cookieHeader; headersToPlanner["Cookie"] = cookieHeader; } const plannerEnabled = isDeviceSearchPlannerEnabled(); if (plannerEnabled && shouldUseDevicePlanner(query)) { const plannerRequest = { query, mode: typeof body.mode === "string" ? body.mode : "auto", filters: typeof body.filters === "object" && body.filters !== null ? body.filters : {}, pagination: { limit: typeof body.limit === "number" ? body.limit : body.pagination?.limit, offset: typeof body.offset === "number" ? body.offset : body.pagination?.offset ?? 0, cursor: typeof body.cursor === "string" ? body.cursor : body.pagination?.cursor ?? "", direction: Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Sensitive Logging: New logs include host_ip for devices during registration which could be sensitive; confirm logging of IPs complies with privacy policies and log retention. Referred Code func (s *Server) copyPollerStatusCache() map[string]*models.PollerStatus { result := make(map[string]*models.PollerStatus, len(s.pollerStatusCache)) for k, v := range s.pollerStatusCache { result[k] = v } return result } // registerPollerAsDevice registers a poller as a device in the inventory func (s *Server) registerPollerAsDevice(ctx context.Context, pollerID, hostIP string) error { if s.DeviceRegistry == nil { s.logger.Debug(). Str("poller_id", pollerID). Msg("DeviceRegistry is nil, skipping poller device registration") return nil // Registry not available } resolvedIP := strings.TrimSpace(hostIP) ... (clipped 36 lines) Learn more about managing compliance generic rules or creating your own custom rules

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1937#issuecomment-3523395883 Original created: 2025-11-12T18:40:05Z --- ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/00abea00424a2ff892cd0dfe32147686a4f4707d --> #### (Compliance updated until commit https://github.com/carverauto/serviceradar/commit/00abea00424a2ff892cd0dfe32147686a4f4707d) Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=2>⚪</td> <td><details><summary><strong>Sensitive information exposure </strong></summary><br> <b>Description:</b> Potential logging of host IP addresses in info-level logs and capability events may expose <br>sensitive infrastructure details; consider redacting or lowering log level where IPs are <br>included.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1937/files#diff-fe81e2a32f1ac64bcdc6f25f55c5fa918d17bad8c0546f2cf80c757ff4051816R418-R456'>pollers.go [418-456]</a></strong><br> <details open><summary>Referred Code</summary> ```go func (s *Server) storePollerStatus(ctx context.Context, pollerID string, isHealthy bool, hostIP string, now time.Time) error { normIP := normalizeHostIP(hostIP) pollerStatus := &models.PollerStatus{ PollerID: pollerID, IsHealthy: isHealthy, LastSeen: now, HostIP: normIP, } if err := s.DB.UpdatePollerStatus(ctx, pollerStatus); err != nil { return fmt.Errorf("failed to store poller status: %w", err) } // Register poller as a device for inventory tracking (best-effort) if err := s.registerPollerAsDevice(ctx, pollerID, normIP); err != nil { s.logger.Warn(). Err(err). Str("poller_id", pollerID). Msg("Failed to register poller as device") } ... (clipped 18 lines) ``` </details></details></td></tr> <tr><td><details><summary><strong>Insecure transport usage</strong></summary><br> <b>Description:</b> Downloading onboarding package over HTTP client without explicit TLS configuration or <br>certificate pinning could allow MITM if Core API URL is non-TLS or insecure; ensure HTTPS <br>and proper TLS verification are enforced in runtime configs.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1937/files#diff-4502ef07f8a1b9a8363fb9a1685599655ecc0560dac96c9f174247b6900ac12bR169-R213'>download.go [169-213]</a></strong><br> <details open><summary>Referred Code</summary> ```go req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(body)) if err != nil { return fmt.Errorf("create download request: %w", err) } req.Header.Set("Content-Type", "application/json") req.Header.Set("Accept", "application/json") resp, err := b.getHTTPClient().Do(req) if err != nil { return fmt.Errorf("request core deliver endpoint: %w", err) } defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { msg := readErrorBody(resp.Body) if msg != "" { return fmt.Errorf("%w (%s): %s", ErrCoreDeliverEndpoint, resp.Status, msg) } return fmt.Errorf("%w returned %s", ErrCoreDeliverEndpoint, resp.Status) } ... (clipped 24 lines) ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/1936>#1936</a></summary> <table width='100%'><tbody> <tr><td rowspan=3>🟢</td> <td>Ensure dashboard/device-related components render without crashing.</td></tr> <tr><td>Provide a robust fix so UI pages that show agent details and traces work with <br>null/array/object variations.</td></tr> <tr><td>Bug reproducible in Docker environment; fix should apply there too.</td></tr> <tr><td rowspan=1>⚪</td> <td>Fix client-side React error shown on agent detail page (React error</td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=2>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=4>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1937/files#diff-fe81e2a32f1ac64bcdc6f25f55c5fa918d17bad8c0546f2cf80c757ff4051816R418-R456'><strong>Missing Audit Logs</strong></a>: New critical actions like registering pollers as devices and updating poller status do not <br>emit explicit audit trail logs identifying actor/context beyond generic info logs, and may <br>require structured auditing.<br> <details open><summary>Referred Code</summary> ```go func (s *Server) storePollerStatus(ctx context.Context, pollerID string, isHealthy bool, hostIP string, now time.Time) error { normIP := normalizeHostIP(hostIP) pollerStatus := &models.PollerStatus{ PollerID: pollerID, IsHealthy: isHealthy, LastSeen: now, HostIP: normIP, } if err := s.DB.UpdatePollerStatus(ctx, pollerStatus); err != nil { return fmt.Errorf("failed to store poller status: %w", err) } // Register poller as a device for inventory tracking (best-effort) if err := s.registerPollerAsDevice(ctx, pollerID, normIP); err != nil { s.logger.Warn(). Err(err). Str("poller_id", pollerID). Msg("Failed to register poller as device") } ... (clipped 18 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1937/files#diff-df917d3115d52d8e8eb0341e2b1538e93cc226823218d31241494bfdde1e349dR310-R321'><strong>Verbose Errors</strong></a>: Returned errors propagate raw response messages from Core API to CLI output which may <br>expose internal details to end users depending on usage context.<br> <details open><summary>Referred Code</summary> ```go if resp.StatusCode != http.StatusOK { message := readErrorBody(resp.Body) if message == "" { message = resp.Status } return fmt.Errorf("%w: %s", errCoreAPIError, message) } var packages []edgePackageView if err := json.NewDecoder(resp.Body).Decode(&packages); err != nil { return fmt.Errorf("decode list response: %w", err) } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1937/files#diff-4502ef07f8a1b9a8363fb9a1685599655ecc0560dac96c9f174247b6900ac12bR206-R214'><strong>Token In Logs</strong></a>: Info logs after downloading or loading packages include identifiers and context, and code <br>handles join and download tokens where care is needed to avoid logging secrets though no <br>explicit redaction is shown.<br> <details open><summary>Referred Code</summary> ```go b.tokenInfo = tokenInfo b.logger.Info(). Str("package_id", pkg.PackageID). Str("component_id", pkg.ComponentID). Str("core_api_url", coreURL). Msg("Downloaded edge onboarding package") return nil ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1937/files#diff-4d4e15c7e925bdd07a11ca5d2779a6f2acef74c2bebe286c4bbc9a7593cefdb7R29-R37'><strong>SQL Safety Check</strong></a>: The <code>getUserByField</code> function builds a query with a dynamic field name which could be unsafe <br>without strict whitelisting though the value is parameterized; ensure <code>field</code> is strictly <br>controlled.<br> <details open><summary>Referred Code</summary> ```go // getUserByField retrieves a user by a specific field (e.g., id or email). func (db *DB) getUserByField(ctx context.Context, field, value string) (*models.User, error) { user := &models.User{} query := ` SELECT id, email, username AS name, provider, created_at, updated_at FROM users WHERE ` + field + ` = $1 LIMIT 1` ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> <!-- placeholder --> <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details> ___ #### Previous compliance checks <details> <summary>Compliance check up to commit <a href='https://github.com/carverauto/serviceradar/commit/6d99691f3c04c2a208e5e4b299f295db4aeb4ff4'>6d99691</a></summary><br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/1936>#1936</a></summary> <table width='100%'><tbody> <tr><td rowspan=5>🟢</td> <td>E</td></tr> <tr><td>v</td></tr> <tr><td>y</td></tr> <tr><td>P</td></tr> <tr><td>q</td></tr> <tr><td rowspan=35>⚪</td> <td>-</td></tr> <tr><td> </td></tr> <tr><td>F</td></tr> <tr><td>i</td></tr> <tr><td>x</td></tr> <tr><td>c</td></tr> <tr><td>l</td></tr> <tr><td>e</td></tr> <tr><td>n</td></tr> <tr><td>t</td></tr> <tr><td>s</td></tr> <tr><td>d</td></tr> <tr><td>R</td></tr> <tr><td>a</td></tr> <tr><td>r</td></tr> <tr><td>o</td></tr> <tr><td>h</td></tr> <tr><td>w</td></tr> <tr><td>g</td></tr> <tr><td>/</td></tr> <tr><td>b</td></tr> <tr><td>p</td></tr> <tr><td>(</td></tr> <tr><td>M</td></tr> <tr><td>f</td></tr> <tr><td>#</td></tr> <tr><td>3</td></tr> <tr><td>1</td></tr> <tr><td>)</td></tr> <tr><td>U</td></tr> <tr><td>I</td></tr> <tr><td>u</td></tr> <tr><td>m</td></tr> <tr><td>.</td></tr> <tr><td> </td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=3>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=3>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1937/files#diff-fe81e2a32f1ac64bcdc6f25f55c5fa918d17bad8c0546f2cf80c757ff4051816R418-R1317'><strong>Action Logging</strong></a>: New critical actions like registering pollers as devices and updating poller status are <br>performed without explicit additional audit log entries beyond existing info/warn logs, <br>making it unclear if audit requirements are met.<br> <details open><summary>Referred Code</summary> ```go pollerStatus.FirstSeen = existingStatus.FirstSeen } if err := s.DB.UpdatePollerStatus(ctx, pollerStatus); err != nil { return fmt.Errorf("failed to update poller status: %w", err) } // Register poller as a device for inventory tracking if err := s.registerPollerAsDevice(ctx, pollerID, ""); err != nil { // Log but don't fail - device registration is best-effort s.logger.Warn(). Err(err). Str("poller_id", pollerID). Msg("Failed to register poller as device") } return nil } func (s *Server) updatePollerState( ctx context.Context, ... (clipped 879 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1937/files#diff-53eee72ee62c074b7ff0b7556920802805621e9e16bfa91f85675ec55b66bf32R93-R110'><strong>Error Exposure</strong></a>: The API route proxies queries and may pass backend error messages through the SRQL <br>response; ensure user-facing errors remain generic and detailed errors are only logged <br>internally.<br> <details open><summary>Referred Code</summary> ```typescript // but the Go backend will likely do it if it's missing. } if (cookieHeader) { headersToSrql["Cookie"] = cookieHeader; headersToPlanner["Cookie"] = cookieHeader; } const plannerEnabled = isDeviceSearchPlannerEnabled(); if (plannerEnabled && shouldUseDevicePlanner(query)) { const plannerRequest = { query, mode: typeof body.mode === "string" ? body.mode : "auto", filters: typeof body.filters === "object" && body.filters !== null ? body.filters : {}, pagination: { limit: typeof body.limit === "number" ? body.limit : body.pagination?.limit, offset: typeof body.offset === "number" ? body.offset : body.pagination?.offset ?? 0, cursor: typeof body.cursor === "string" ? body.cursor : body.pagination?.cursor ?? "", direction: ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1937/files#diff-fe81e2a32f1ac64bcdc6f25f55c5fa918d17bad8c0546f2cf80c757ff4051816R1261-R1317'><strong>Sensitive Logging</strong></a>: New logs include <code>host_ip</code> for devices during registration which could be sensitive; confirm <br>logging of IPs complies with privacy policies and log retention.<br> <details open><summary>Referred Code</summary> ```go func (s *Server) copyPollerStatusCache() map[string]*models.PollerStatus { result := make(map[string]*models.PollerStatus, len(s.pollerStatusCache)) for k, v := range s.pollerStatusCache { result[k] = v } return result } // registerPollerAsDevice registers a poller as a device in the inventory func (s *Server) registerPollerAsDevice(ctx context.Context, pollerID, hostIP string) error { if s.DeviceRegistry == nil { s.logger.Debug(). Str("poller_id", pollerID). Msg("DeviceRegistry is nil, skipping poller device registration") return nil // Registry not available } resolvedIP := strings.TrimSpace(hostIP) ... (clipped 36 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> </details>

qodo-code-review[bot] commented 2025-11-12 18:41:14 +00:00 (Migrated from github.com)

Author

Owner

Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1937#issuecomment-3523400131
Original created: 2025-11-12T18:41:14Z

PR Code Suggestions ✨

Latest suggestions up to 0022a97

Category	Suggestion	Impact
Incremental [*]	Fix incomplete INSERT statement Add the missing VALUES clause to the insertUserStatement constant to fix the SQL syntax for user creation and update operations. pkg/db/auth.go [27-98] -const insertUserStatement = "INSERT INTO users (id, username, email, provider, password_hash, created_at, updated_at, is_active, roles)" +const insertUserStatement = ` +INSERT INTO users ( + id, username, email, provider, password_hash, created_at, updated_at, is_active, roles +) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) +` ... batch, err := db.Conn.PrepareBatch(ctx, insertUserStatement) if err != nil { return fmt.Errorf("failed to prepare batch: %w", err) } if err := batch.Append( user.ID, user.Name, // username field user.Email, normalizeProvider(user.Provider), "", // password_hash (empty for OAuth users) user.CreatedAt, user.UpdatedAt, true, // is_active (default to true) normalizeRoles(user.Roles), // roles (default role if missing) ); err != nil { return fmt.Errorf("failed to append user: %w", err) } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 10 __ Why: This suggestion correctly identifies a critical bug where the insertUserStatement is missing the VALUES clause, which would cause all user creation and update operations to fail at runtime.	High
	Use upsert instead of insert In UpdateUser, replace the INSERT statement with an UPSERT (INSERT ... ON CONFLICT ... DO UPDATE) to correctly update existing user records instead of creating duplicates. pkg/db/auth.go [175-203] func (db *DB) UpdateUser(ctx context.Context, user *models.User) error { user.UpdatedAt = time.Now() - batch, err := db.Conn.PrepareBatch(ctx, insertUserStatement) + const upsertUserStatement = ` + INSERT INTO users ( + id, username, email, provider, password_hash, created_at, updated_at, is_active, roles + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT (id) DO UPDATE SET + username = excluded.username, + email = excluded.email, + provider = excluded.provider, + password_hash = excluded.password_hash, + updated_at = excluded.updated_at, + is_active = excluded.is_active, + roles = excluded.roles + ` + + batch, err := db.Conn.PrepareBatch(ctx, upsertUserStatement) if err != nil { return fmt.Errorf("failed to prepare batch: %w", err) } if err := batch.Append( user.ID, - user.Name, // username field + user.Name, user.Email, normalizeProvider(user.Provider), - "", // password_hash (empty for OAuth users) + "", user.CreatedAt, user.UpdatedAt, - true, // is_active (default to true) - normalizeRoles(user.Roles), // roles (default role if missing) + true, + normalizeRoles(user.Roles), ); err != nil { - return fmt.Errorf("failed to append user update: %w", err) + return fmt.Errorf("failed to append user upsert: %w", err) } if err := batch.Send(); err != nil { - return fmt.Errorf("failed to send update user batch: %w", err) + return fmt.Errorf("failed to send user upsert batch: %w", err) } - return nil } Apply / Chat Suggestion importance[1-10]: 9 __ Why: This suggestion correctly points out that using an INSERT statement in UpdateUser is a bug that would lead to duplicate data or errors, and it proposes a correct UPSERT pattern to fix it.	High
	Reinstate localhost bind safety Reinstate the listen_host: 127.0.0.1 setting in the configuration file to prevent services from binding to all network interfaces and being accidentally exposed externally. packaging/proton/config/config.yaml [31-45] node: # Roles that the current node has # Supported roles : Metadata, Data, Compute # `Data` role contains `Compute` roles by default. # `Compute` node can't have streams storing data on them. Can serve ingest or query roles: role: - Metadata - Data # Public protocols and ports http: port: 3218 # Maintained default, was http_port in old config is_tls_port: false +# Restrict server to localhost to avoid unintended external exposure in single-instance setups +listen_host: 127.0.0.1 + [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies the removal of listen_host: 127.0.0.1 and raises a valid security concern about unintentionally exposing services on all network interfaces, which is a significant regression.	High
	Prevent Stop deadlock on watcher In the Stop function, replace the blocking wait on s.watchDone with a select statement that includes a timeout. This prevents a potential deadlock during shutdown if the config watcher goroutine exits without closing the channel. pkg/sweeper/sweeper.go [523-562] func (s *NetworkSweeper) Stop() error { s.logger.Info().Msg("Stopping network sweeper") if s.done == nil { s.logger.Debug().Msg("Sweep service already stopped") return nil } close(s.done) + // Avoid indefinite block if watchDone was never set or already closed. if s.watchDone != nil { - <-s.watchDone // Wait for KV watching to stop + select { + case <-s.watchDone: + // watcher stopped + case <-time.After(5 * time.Second): + s.logger.Warn().Msg("Timeout waiting for KV watcher to stop") + } } s.done = nil s.watchDone = nil if s.icmpScanner != nil { if err := s.icmpScanner.Stop(); err != nil { s.logger.Error().Err(err).Msg("Failed to stop ICMP scanner") } s.icmpScanner = nil } if s.tcpScanner != nil { if err := s.tcpScanner.Stop(); err != nil { s.logger.Error().Err(err).Msg("Failed to stop TCP scanner") } s.tcpScanner = nil } if s.tcpConnectScanner != nil { if err := s.tcpConnectScanner.Stop(); err != nil { s.logger.Error().Err(err).Msg("Failed to stop TCP connect scanner") } s.tcpConnectScanner = nil } return nil } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential deadlock in the Stop function if the watchDone channel is not closed, and proposes a robust solution using a select with a timeout to prevent the service from hanging on shutdown.	Medium
	Validate mandatory deliver fields After decoding the deliverResponse in downloadPackage, add checks to ensure that deliver.JoinToken and deliver.BundlePEM are not empty. Return ErrJoinTokenEmpty or ErrBundlePEMEmpty if they are, to fail fast. pkg/edgeonboarding/download.go [190-207] -func (b *Bootstrapper) downloadPackage(ctx context.Context) error { - if strings.TrimSpace(b.cfg.PackagePath) != "" { - return b.loadPackageFromArchive(ctx, b.cfg.PackagePath) - } +var deliver deliverResponse +if err := json.NewDecoder(resp.Body).Decode(&deliver); err != nil { + return fmt.Errorf("decode deliver response: %w", err) +} - tokenInfo, err := parseOnboardingToken(b.cfg.Token, b.cfg.PackageID, b.cfg.CoreAPIURL) - if err != nil { - return err - } +if strings.TrimSpace(deliver.JoinToken) == "" { + return ErrJoinTokenEmpty +} +if strings.TrimSpace(deliver.BundlePEM) == "" { + return ErrBundlePEMEmpty +} - coreURL, err := b.resolveCoreAPIURL(tokenInfo) - if err != nil { - return err - } +pkg, err := deliver.Package.toModel() +if err != nil { + return fmt.Errorf("invalid package payload: %w", err) +} - endpoint := fmt.Sprintf("%s/api/admin/edge-packages/%s/download?format=json", coreURL, url.PathEscape(tokenInfo.PackageID)) - payload := map[string]string{"download_token": tokenInfo.DownloadToken} - body, err := json.Marshal(payload) - if err != nil { - return fmt.Errorf("encode download payload: %w", err) - } +b.pkg = pkg +b.downloadResult = &models.EdgeOnboardingDeliverResult{ + Package: pkg, + JoinToken: strings.TrimSpace(deliver.JoinToken), + BundlePEM: []byte(deliver.BundlePEM), +} +b.tokenInfo = tokenInfo - req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(body)) - if err != nil { - return fmt.Errorf("create download request: %w", err) - } - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Accept", "application/json") - - resp, err := b.getHTTPClient().Do(req) - if err != nil { - return fmt.Errorf("request core deliver endpoint: %w", err) - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusOK { - msg := readErrorBody(resp.Body) - if msg != "" { - return fmt.Errorf("%w (%s): %s", ErrCoreDeliverEndpoint, resp.Status, msg) - } - return fmt.Errorf("%w returned %s", ErrCoreDeliverEndpoint, resp.Status) - } - - var deliver deliverResponse - if err := json.NewDecoder(resp.Body).Decode(&deliver); err != nil { - return fmt.Errorf("decode deliver response: %w", err) - } - - pkg, err := deliver.Package.toModel() - if err != nil { - return fmt.Errorf("invalid package payload: %w", err) - } - - b.pkg = pkg - b.downloadResult = &models.EdgeOnboardingDeliverResult{ - Package: pkg, - JoinToken: deliver.JoinToken, - BundlePEM: []byte(deliver.BundlePEM), - } - b.tokenInfo = tokenInfo - [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion improves robustness by adding validation for essential fields from an API response, ensuring the program fails early with a clear error message instead of proceeding with invalid data.	Medium
	Prevent spurious initial reload In StartWatch, prevent an unnecessary reload on initial startup by setting the baseline snapshot and returning early if prevSnapshot is nil. pkg/config/kv_client.go [467-510] func (m *KVManager) StartWatch(ctx context.Context, kvKey string, cfg interface{}, logger logger.Logger, onReload func()) { if m == nil || m.client == nil { return } prevSnapshot, err := newConfigSnapshot(cfg) if err != nil && logger != nil { logger.Warn().Err(err).Str("key", kvKey).Msg("Failed to snapshot initial config for KV watch") } StartKVWatchOverlay(ctx, m.client, kvKey, cfg, logger, func() { currSnapshot, snapErr := newConfigSnapshot(cfg) if snapErr != nil { if logger != nil { logger.Warn().Err(snapErr).Str("key", kvKey).Msg("Failed to snapshot config after KV overlay") } return } - if prevSnapshot != nil && bytes.Equal(prevSnapshot.raw, currSnapshot.raw) { + // If this is the first overlay and we have no previous snapshot, set baseline and do not reload. + if prevSnapshot == nil { + prevSnapshot = currSnapshot + return + } + + if bytes.Equal(prevSnapshot.raw, currSnapshot.raw) { // No actual change after overlay; skip reload. return } triggers := map[string]bool{"reload": true, "rebuild": true} - var changed []string - if prevSnapshot != nil { - changed = FieldsChangedByTag(prevSnapshot.value, currSnapshot.value, "hot", triggers) - } + changed := FieldsChangedByTag(prevSnapshot.value, currSnapshot.value, "hot", triggers) if len(changed) == 0 { - changed = []string{"*"} + return } - if len(changed) > 0 { - if logger != nil { - logger.Info().Strs("changed_fields", changed).Msg("Applying hot-reload") - } - if onReload != nil { - onReload() - } - prevSnapshot = currSnapshot + if logger != nil { + logger.Info().Strs("changed_fields", changed).Msg("Applying hot-reload") } + if onReload != nil { + onReload() + } + prevSnapshot = currSnapshot }) } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the current logic triggers an unnecessary reload on startup and proposes a valid fix to prevent this behavior, improving the startup sequence.	Medium
	Restore correct engine arity The Stream engine signature was changed from Stream(1, 1, rand()) to Stream(1, rand()). Verify if this change in arity is correct and intended, as it might affect engine initialization or sharding semantics. pkg/db/migrations/00000000000001_consolidated_serviceradar_schema.up.sql [49-55] CREATE STREAM IF NOT EXISTS device_updates ( timestamp DateTime64(3), available boolean, metadata map(string, string) -) ENGINE = Stream(1, rand()) +) ENGINE = Stream(1, 1, rand()) PARTITION BY to_start_of_day(timestamp) ORDER BY (timestamp, device_id, poller_id) TTL to_start_of_day(coalesce(timestamp, _tp_time)) + INTERVAL 3 DAY [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a significant and repeated change to the Stream engine signature but incorrectly assumes it's an error; the PR seems to intentionally remove the replica parameter for single-instance setups.	Medium
	Let IP resolution occur centrally In storePollerStatus, call registerPollerAsDevice with an empty string for the IP address instead of normIP. This allows registerPollerAsDevice to use its more comprehensive internal logic to resolve the most reliable IP. pkg/core/pollers.go [418-438] func (s *Server) storePollerStatus(ctx context.Context, pollerID string, isHealthy bool, hostIP string, now time.Time) error { normIP := normalizeHostIP(hostIP) pollerStatus := &models.PollerStatus{ PollerID: pollerID, IsHealthy: isHealthy, LastSeen: now, HostIP: normIP, } if err := s.DB.UpdatePollerStatus(ctx, pollerStatus); err != nil { return fmt.Errorf("failed to store poller status: %w", err) } - // Register poller as a device for inventory tracking (best-effort) - if err := s.registerPollerAsDevice(ctx, pollerID, normIP); err != nil { + // Register poller as a device for inventory tracking (best-effort). + // Allow registerPollerAsDevice to resolve the most reliable IP. + if err := s.registerPollerAsDevice(ctx, pollerID, ""); err != nil { s.logger.Warn(). Err(err). Str("poller_id", pollerID). Msg("Failed to register poller as device") } return nil } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that registerPollerAsDevice has more robust IP resolution logic, and passing an empty string forces it to use its fallbacks, improving data quality.	Low
Possible issue	Fix missing VALUES in insert Add the missing VALUES clause with placeholders to the insertPollerStatusQuery and provide all corresponding arguments to batch.Append to fix the malformed SQL. pkg/db/pollers.go [391-412] const insertPollerStatusQuery = ` INSERT INTO pollers ( poller_id, component_id, registration_source, status, spiffe_identity, first_registered, first_seen, last_seen, metadata, created_by, is_healthy, agent_count, checker_count -)` +) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)` func (db *DB) insertPollerStatus(ctx context.Context, status *models.PollerStatus) error { return db.executeBatch(ctx, insertPollerStatusQuery, func(batch driver.Batch) error { return batch.Append( status.PollerID, // poller_id "", // component_id (empty for implicit registration) + status.RegistrationSource, // registration_source + status.Status, // status + status.SPIFFEID, // spiffe_identity + status.FirstRegistered, // first_registered + status.FirstSeen, // first_seen + status.LastSeen, // last_seen + status.Metadata, // metadata + status.CreatedBy, // created_by + status.IsHealthy, // is_healthy + status.AgentCount, // agent_count + status.CheckerCount, // checker_count + ) + }) +} [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a malformed SQL INSERT statement that is missing its VALUES clause and has an incomplete list of arguments, which would cause a runtime error.	High
	Remove Buffer dependency in browser Replace the Node.js Buffer dependency with browser-native APIs like TextEncoder and btoa for base64url encoding to ensure client-side compatibility and prevent runtime errors. web/src/app/admin/edge-packages/page.tsx [19-250] -import { Buffer } from 'buffer'; -... function base64UrlEncode(value: string): string { - return Buffer.from(value, 'utf-8') - .toString('base64') - .replace(/\+/g, '-') - .replace(/\//g, '_') - .replace(/=+$/g, ''); + const encoder = new TextEncoder(); + const bytes = encoder.encode(value); + let binary = ''; + for (let i = 0; i < bytes.length; i++) { + binary += String.fromCharCode(bytes[i]); + } + const b64 = btoa(binary); + return b64.replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/g, ''); } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that using Node's Buffer in a Next.js client component can cause runtime errors in browsers and provides a robust, browser-native alternative to prevent potential crashes.	Medium
	Support more config kinds Extend cloneConfigValue to support map and slice types in addition to pointer and struct to make the configuration snapshot logic more robust for hot-reloading. pkg/config/kv_client.go [540-562] func cloneConfigValue(cfg interface{}) (interface{}, error) { val := reflect.ValueOf(cfg) if !val.IsValid() { return nil, nil } - kind := val.Kind() - if kind == reflect.Ptr { + switch val.Kind() { + case reflect.Ptr: if val.IsNil() { return nil, nil } elem := val.Elem() clone := reflect.New(elem.Type()) return clone.Interface(), nil - } - - if kind == reflect.Struct { + case reflect.Struct: clone := reflect.New(val.Type()) return clone.Interface(), nil + case reflect.Map: + // Create a pointer to a new map of the same type + m := reflect.MakeMapWithSize(val.Type(), 0) + return m.Addr().Interface(), nil + case reflect.Slice: + // Create an empty slice of the same type + s := reflect.MakeSlice(val.Type(), 0, 0) + return reflect.New(s.Type()).Elem().Addr().Interface(), nil + default: + return nil, fmt.Errorf("%w: %T", errUnsupportedConfigType, cfg) } - - return nil, fmt.Errorf("%w: %T", errUnsupportedConfigType, cfg) } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that the cloneConfigValue function is incomplete and only handles struct and pointer types, which limits the hot-reloading feature.	Medium
	Preserve shard/replica semantics Explicitly set shards and replicas in the Stream engine SETTINGS to avoid potential misconfiguration due to the changed engine signature. pkg/db/migrations/00000000000001_consolidated_serviceradar_schema.up.sql [311-314] ) ENGINE = Stream(1, rand()) +SETTINGS shards = 1, replicas = 1 PARTITION BY int_div(to_unix_timestamp(timestamp), 3600) ORDER BY (timestamp, service_name, poller_id) TTL to_start_of_day(coalesce(timestamp, _tp_time)) + INTERVAL 3 DAY Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a semantic change in the Stream engine signature and proposes making the replica count explicit for robustness, which is a valid consideration during a version upgrade.	Low
General	Make Stop idempotent and thread-safe Implement thread-safe, idempotent shutdown logic in Stop() by using sync.Once to prevent panics from closing the s.done channel multiple times concurrently. pkg/sweeper/sweeper.go [523-538] +// Add to struct: shutdownOnce sync.Once func (s *NetworkSweeper) Stop() error { s.logger.Info().Msg("Stopping network sweeper") + var alreadyStopped bool + s.shutdownOnce.Do(func() { + if s.done != nil { + close(s.done) + } + alreadyStopped = false + }) if s.done == nil { + alreadyStopped = true + } + if alreadyStopped { s.logger.Debug().Msg("Sweep service already stopped") return nil } - - close(s.done) if s.watchDone != nil { <-s.watchDone // Wait for KV watching to stop } s.done = nil s.watchDone = nil + if s.icmpScanner != nil { + _ = s.icmpScanner.Stop() + s.icmpScanner = nil + } + if s.tcpScanner != nil { + _ = s.tcpScanner.Stop() + s.tcpScanner = nil + } + if s.tcpConnectScanner != nil { + _ = s.tcpConnectScanner.Stop() + s.tcpConnectScanner = nil + } + return nil +} + [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential panic from concurrent calls to Stop() and proposes a valid solution using sync.Once to ensure the shutdown logic is idempotent and thread-safe.	Medium
	Validate URL in structured token Validate that coreApiUrl is an absolute URL before including it in the structured token payload to prevent generating malformed tokens that would fail during client bootstrapping. web/src/app/admin/edge-packages/page.tsx [252-267] +function isAbsoluteUrl(url: string): boolean { + try { + const u = new URL(url); + return u.protocol === 'https:' || u.protocol === 'http:'; + } catch { + return false; + } +} function buildStructuredToken(packageId: string, downloadToken: string, coreApiUrl: string): string { - const pkg = packageId.trim(); - const token = downloadToken.trim(); + const pkg = packageId?.trim(); + const token = downloadToken?.trim(); if (!pkg || !token) { return ''; } - const payload: { pkg: string; dl: string; api?: string } = { - pkg, - dl: token, - }; - const api = coreApiUrl.trim(); - if (api) { + const payload: { pkg: string; dl: string; api?: string } = { pkg, dl: token }; + const api = coreApiUrl?.trim(); + if (api && isAbsoluteUrl(api)) { payload.api = api; } return `edgepkg-v1:${base64UrlEncode(JSON.stringify(payload))}`; } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out a potential issue where an invalid coreApiUrl could lead to a malformed token, and proposes adding validation to improve the robustness of the token generation logic.	Medium
	Safer JSON negotiation default Modify shouldReturnJSON to treat a missing or wildcard (/) Accept header as a request for JSON, improving compatibility with generic HTTP clients. pkg/core/api/edge_onboarding.go [642-658] func shouldReturnJSON(r *http.Request) bool { if r == nil { return false } - format := strings.ToLower(strings.TrimSpace(r.URL.Query().Get("format"))) - if format == "json" { + if strings.EqualFold(strings.TrimSpace(r.URL.Query().Get("format")), "json") { return true } accept := strings.ToLower(strings.TrimSpace(r.Header.Get("Accept"))) - if accept == "" { - return false + if accept == "" || accept == "*/*" { + // Default to JSON when client doesn't specify a concrete type + return true } - if strings.Contains(accept, "application/json") && !strings.Contains(accept, "application/gzip") { + // Prefer JSON when acceptable; only avoid JSON if gzip is explicitly preferred without JSON + if strings.Contains(accept, "application/json") { return true } return false } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion improves the content negotiation logic by correctly handling the */* Accept header and defaulting to JSON, which enhances compatibility with generic HTTP clients.	Low
Update

---

Previous suggestions

✅ Suggestions up to commit e2c4260

Category	Suggestion	Impact
Incremental [*]	Harden discovery source parsing Harden the parsing of discovery_sources by rejecting objects with unknown keys and adding limits on token length and total byte size to mitigate security risks from malicious input. web/src/components/Devices/DeviceDetail.tsx [60-253] -interface DiscoverySource { - source?: string; - agent_id?: string; - poller_id?: string; - first_seen?: string; - last_seen?: string; - confidence?: number | string; -} +const MAX_DISCOVERY_SOURCES = 50; +const MAX_TOKEN_LENGTH = 256; +const MAX_TOTAL_BYTES = 8 * 1024; // 8KB safety cap -interface DeviceRecord { - device_id: string; - agent_id?: string; - poller_id?: string; - discovery_sources?: string[] | string | DiscoverySource[]; - ip?: string; - mac?: string; - hostname?: string; - first_seen?: string; - last_seen?: string; - is_available?: boolean; - metadata?: Record<string, unknown>; - device_type?: string; - service_type?: string; - service_status?: string; - last_heartbeat?: string | null; - os_info?: string | null; - version_info?: string | null; -} - -const MAX_DISCOVERY_SOURCES = 50; const ALLOWED_DISCOVERY_SOURCE_KEYS = new Set([ "source", "agent_id", "poller_id", "first_seen", "last_seen", "confidence", ]); +const sanitizeToken = (s: string): string | null => { + const t = s.trim(); + if (!t) return null; + // Basic guard to prevent extremely long strings + return t.length <= MAX_TOKEN_LENGTH ? t : t.slice(0, MAX_TOKEN_LENGTH); +}; + const ensureArray = ( value: string[] | string | DiscoverySource[] | undefined, ): string[] => { if (value == null) return []; const out: string[] = []; + let totalBytes = 0; + const pushSafe = (s: string) => { - if (typeof s === "string" && s.length > 0 && out.length < MAX_DISCOVERY_SOURCES) { - out.push(s); + const sanitized = sanitizeToken(s); + if (!sanitized) return; + const bytes = new TextEncoder().encode(sanitized).length; + if ( + out.length < MAX_DISCOVERY_SOURCES && + totalBytes+bytes <= MAX_TOTAL_BYTES + ) { + out.push(sanitized); + totalBytes += bytes; } }; const isPlainObject = (v: unknown): v is Record<string, unknown> => { if (Object.prototype.toString.call(v) !== "[object Object]") return false; const proto = Object.getPrototypeOf(v); return proto === null || proto === Object.prototype; }; const fromDiscoverySource = (obj: unknown) => { if (!isPlainObject(obj)) return; const keys = Object.keys(obj); - if (!keys.every((key) => ALLOWED_DISCOVERY_SOURCE_KEYS.has(key))) return; + // Reject if any disallowed key present + if (keys.some((k) => !ALLOWED_DISCOVERY_SOURCE_KEYS.has(k))) return; const ds = obj as DiscoverySource; if (typeof ds.source === "string") { pushSafe(ds.source); } }; if (Array.isArray(value)) { for (const item of value) { if (typeof item === "string") { pushSafe(item); } else if (typeof item === "number" || typeof item === "boolean") { pushSafe(String(item)); } else { fromDiscoverySource(item); } - if (out.length >= MAX_DISCOVERY_SOURCES) break; + if (out.length >= MAX_DISCOVERY_SOURCES || totalBytes >= MAX_TOTAL_BYTES) break; } return out; } if (typeof value === "string") { for (const token of value.split(",")) { const t = token.trim(); if (t) pushSafe(t); - if (out.length >= MAX_DISCOVERY_SOURCES) break; + if (out.length >= MAX_DISCOVERY_SOURCES || totalBytes >= MAX_TOTAL_BYTES) break; } return out; } return out; }; Suggestion importance[1-10]: 7 __ Why: The suggestion provides valid security hardening for parsing discovery_sources by adding checks for disallowed keys and imposing byte limits, which helps prevent potential DoS or unexpected UI behavior from malicious data.	Medium
	✅ Fix aggregation detection and routing Suggestion Impact: The commit updated the AGGREGATION_PATTERN to explicitly list token boundary characters, aligning with the suggestion to improve aggregation detection. Other parts of the suggestion (comment stripping and multi-stream handling) were not implemented in this diff. code diff: -const AGGREGATION_PATTERN = /(^|[\s([{,;]))stats\s*:/i; +const AGGREGATION_PATTERN = /(^|\s|\(|\{|\[|,|;)stats\s*:/i; Improve the query routing logic by adding support for stripping comments and ensuring the device planner is not used for queries involving multiple streams. web/src/app/api/query/route.ts [13-76] -const AGGREGATION_PATTERN = /(^|[\s([{,;]))stats\s*:/i; +// Match stats at start or at token boundaries (e.g., start, whitespace, or punctuation) +const AGGREGATION_PATTERN = /(^|[\s([{,;])stats\s*:/i; + +function stripComments(input: string): string { + // Remove single-line // comments and /* block */ comments without disturbing quotes + // This is a best-effort; quoted regions are handled separately by isInsideQuotes. + return input + .replace(/\/\/[^\n\r]*/g, "") + .replace(/\/\*[\s\S]*?\*\//g, ""); +} function hasAggregationOutsideQuotes(query: string): boolean { + const cleaned = stripComments(query); const regex = new RegExp(AGGREGATION_PATTERN.source, "gi"); let match: RegExpExecArray | null; - while ((match = regex.exec(query)) !== null) { - const boundary = match[1] ?? ""; - const statsIndex = match.index + boundary.length; - if (!isInsideQuotes(query, statsIndex)) { + while ((match = regex.exec(cleaned)) !== null) { + const boundaryLen = (match[1] ?? "").length; + const statsIndex = match.index + boundaryLen; + if (!isInsideQuotes(cleaned, statsIndex)) { return true; } } + return false; +} - return false; +function extractPrimaryStream(rawQuery: unknown): string | null { + if (typeof rawQuery !== "string") return null; + + const inMatch = rawQuery.match(/\bin\s*:\s*([^\s]+)/i); + if (!inMatch) return null; + + const raw = inMatch[1] ?? ""; + const candidates = raw.split(/[,|]/).map(cleanToken).filter(Boolean); + // If multiple streams are specified, avoid planner + if (candidates.length !== 1) return null; + return candidates[0]!; } function shouldUseDevicePlanner(query: unknown): boolean { if (typeof query !== "string") return false; if (hasAggregationOutsideQuotes(query)) return false; const stream = extractPrimaryStream(query); return !!stream && DEVICE_PLANNER_STREAMS.has(stream); } [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the routing logic could be more robust by handling comments and multi-stream queries, which could prevent incorrect routing to the device planner.	Low
Possible issue	✅ Prevent service-device record coalescing Suggestion Impact: The commit changed the key selection logic to prioritize DeviceID for service devices (models.IsServiceDevice), matching the suggestion to avoid coalescing with hosts. code diff: + var key string + switch { + case models.IsServiceDevice(deviceID): + key = deviceID + case canonicalID != "": + key = canonicalID + default: key = deviceID } To prevent undercounting, modify the canonical record selection logic to use the unique DeviceID as the key for service devices, ensuring they are not merged with other device records. pkg/core/stats_aggregator.go [454-574] -func (a *StatsAggregator) selectCanonicalRecords(records []*registry.DeviceRecord, meta *models.DeviceStatsMeta) []*registry.DeviceRecord { //nolint:gocyclo // Complex device record selection logic, intentionally kept together for clarity +func (a *StatsAggregator) selectCanonicalRecords(records []*registry.DeviceRecord, meta *models.DeviceStatsMeta) []*registry.DeviceRecord { //nolint:gocyclo if len(records) == 0 { return nil } canonical := make(map[string]canonicalEntry) fallback := make(map[string]*registry.DeviceRecord) processRecord := func(record *registry.DeviceRecord) { if !shouldCountRecord(record) { if meta != nil { meta.SkippedSweepOnlyRecords++ } return } deviceID := strings.TrimSpace(record.DeviceID) canonicalID := canonicalDeviceID(record) - key := canonicalID - if key == "" { + var key string + // Ensure service devices are uniquely keyed by DeviceID to prevent coalescing with hosts. + if models.IsServiceDevice(deviceID) { + key = deviceID + } else if canonicalID != "" { + key = canonicalID + } else { key = deviceID } + key = strings.TrimSpace(key) if key == "" { if meta != nil { meta.SkippedNonCanonical++ } return } normalizedKey := strings.ToLower(key) if canonicalID != "" && strings.EqualFold(canonicalID, deviceID) { if entry, ok := canonical[normalizedKey]; ok { if entry.canonical { if shouldReplaceRecord(entry.record, record) { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } else if meta != nil { meta.SkippedNonCanonical++ } } else { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } } else { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } return } if entry, ok := canonical[normalizedKey]; ok { switch { case entry.canonical: if meta != nil { meta.SkippedNonCanonical++ } case shouldReplaceRecord(entry.record, record): canonical[normalizedKey] = canonicalEntry{record: record, canonical: false} default: if meta != nil { meta.SkippedNonCanonical++ } } return } if existing, ok := fallback[normalizedKey]; ok { if shouldReplaceRecord(existing, record) { fallback[normalizedKey] = record } if meta != nil { meta.SkippedNonCanonical++ } return } fallback[normalizedKey] = record } for _, record := range records { if record == nil { if meta != nil { meta.SkippedNilRecords++ } continue } if isTombstonedRecord(record) { if meta != nil { meta.SkippedTombstonedRecords++ } continue } - // All records (including pollers, agents, global services) are counted as devices. - // Even if service components share an IP with other devices, they maintain - // separate device records in inventory for tracking purposes. processRecord(record) } for key, record := range fallback { if _, ok := canonical[key]; ok { continue } canonical[key] = canonicalEntry{record: record, canonical: false} } out := make([]*registry.DeviceRecord, 0, len(canonical)) for _, entry := range canonical { out = append(out, entry.record) } return out } [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: This suggestion correctly identifies a potential edge case where service devices could be incorrectly merged with host devices if they share a canonicalID. The proposed change makes the new device counting logic more robust by explicitly using the unique DeviceID for service components, ensuring they are always counted separately.	Medium

✅ Suggestions up to commit 8a76746

Category	Suggestion	Impact
General	✅ Avoid misrouting when "stats:" appears in values Suggestion Impact: The commit updated the aggregation regex to require a token boundary and added logic to check that matches are not inside quotes, then used this check in shouldUseDevicePlanner—addressing the misrouting issue as suggested. code diff: -const AGGREGATION_PATTERN = /\bstats\s*:/i; +// Match stats aggregations at token boundaries (e.g., "stats:" or " stats :") +const AGGREGATION_PATTERN = /(^|[\s([{,;]))stats\s*:/i; function cleanToken(token: string): string { let t = token.trim(); - // remove surrounding quotes/brackets t = t.replace(/^[\s"'`(]+/, "").replace(/[\s"'`),]+$/, ""); return t.toLowerCase(); +} + +function isInsideQuotes(query: string, targetIndex: number): boolean { + let inSingle = false; + let inDouble = false; + let inBacktick = false; + + for (let i = 0; i < targetIndex; i++) { + const ch = query[i]; + if (ch === "\\" && i + 1 < targetIndex) { + i++; + continue; + } + if (!inDouble && !inBacktick && ch === "'") { + inSingle = !inSingle; + } else if (!inSingle && !inBacktick && ch === '"') { + inDouble = !inDouble; + } else if (!inSingle && !inDouble && ch === "`") { + inBacktick = !inBacktick; + } + } + + return inSingle || inDouble || inBacktick; } function extractPrimaryStream(rawQuery: unknown): string | null { @@ -29,9 +52,25 @@ return candidates.length > 0 ? candidates[0] : null; } +function hasAggregationOutsideQuotes(query: string): boolean { + const regex = new RegExp(AGGREGATION_PATTERN.source, "gi"); + let match: RegExpExecArray | null; + + while ((match = regex.exec(query)) !== null) { + const boundary = match[1] ?? ""; + const statsIndex = match.index + boundary.length; + if (!isInsideQuotes(query, statsIndex)) { + return true; + } + } + + return false; +} + function shouldUseDevicePlanner(query: unknown): boolean { if (typeof query !== "string") return false; - if (AGGREGATION_PATTERN.test(query)) return false; + if (hasAggregationOutsideQuotes(query)) return false; + const stream = extractPrimaryStream(query); return !!stream && DEVICE_PLANNER_STREAMS.has(stream); } Improve the regex for detecting aggregations in shouldUseDevicePlanner to avoid incorrectly identifying "stats:" within string values, ensuring queries are routed correctly. web/src/app/api/query/route.ts [11-37] const DEVICE_PLANNER_STREAMS = new Set(["devices", "device", "device_inventory"]); -const AGGREGATION_PATTERN = /\bstats\s*:/i; +// Match keys like stats: or stats : at token boundaries, not inside quotes +const AGGREGATION_PATTERN = /(?:^|\s)(stats)\s*:/i; function cleanToken(token: string): string { let t = token.trim(); - // remove surrounding quotes/brackets t = t.replace(/^[\s"'`(]+/, "").replace(/[\s"'`),]+$/, ""); return t.toLowerCase(); +} + +function isInsideQuotes(q: string, matchIndex: number): boolean { + let inSingle = false, inDouble = false, inBack = false; + for (let i = 0; i < matchIndex; i++) { + const ch = q[i]; + if (ch === "\\" && i + 1 < matchIndex) { i++; continue; } + if (!inDouble && !inBack && ch === "'") inSingle = !inSingle; + else if (!inSingle && !inBack && ch === '"') inDouble = !inDouble; + else if (!inSingle && !inDouble && ch === "`") inBack = !inBack; + } + return inSingle || inDouble || inBack; } function extractPrimaryStream(rawQuery: unknown): string | null { if (typeof rawQuery !== "string") return null; const inMatch = rawQuery.match(/\bin\s*:\s*([^\s]+)/i); if (!inMatch) return null; const raw = inMatch[1] ?? ""; const candidates = raw.split(/[,|]/).map(cleanToken).filter(Boolean); return candidates.length > 0 ? candidates[0] : null; } function shouldUseDevicePlanner(query: unknown): boolean { if (typeof query !== "string") return false; - if (AGGREGATION_PATTERN.test(query)) return false; + + const aggMatch = query.match(AGGREGATION_PATTERN); + if (aggMatch && aggMatch.index !== undefined && !isInsideQuotes(query, aggMatch.index)) { + return false; + } + const stream = extractPrimaryStream(query); return !!stream && DEVICE_PLANNER_STREAMS.has(stream); } [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential flaw in the regex and proposes a more robust solution to prevent false positives, improving the reliability of the query routing logic.	Low
Security	✅ Sanitize discovery_sources normalization Suggestion Impact: The commit implements a secure refactor of ensureArray: adds plain object checks, whitelists allowed keys, caps array size via MAX_DISCOVERY_SOURCES, and tokenizes strings—matching the suggestion’s intent. code diff: +const MAX_DISCOVERY_SOURCES = 50; +const ALLOWED_DISCOVERY_SOURCE_KEYS = new Set([ + "source", + "agent_id", + "poller_id", + "first_seen", + "last_seen", + "confidence", +]); const ensureArray = ( value: string[] | string | DiscoverySource[] | undefined, ): string[] => { if (value == null) return []; + const out: string[] = []; + const pushSafe = (s: string) => { + if (typeof s === "string" && s.length > 0 && out.length < MAX_DISCOVERY_SOURCES) { + out.push(s); + } + }; + + const isPlainObject = (v: unknown): v is Record<string, unknown> => { + if (Object.prototype.toString.call(v) !== "[object Object]") return false; + const proto = Object.getPrototypeOf(v); + return proto === null || proto === Object.prototype; + }; + + const fromDiscoverySource = (obj: unknown) => { + if (!isPlainObject(obj)) return; + const keys = Object.keys(obj); + if (!keys.every((key) => ALLOWED_DISCOVERY_SOURCE_KEYS.has(key))) return; + const ds = obj as DiscoverySource; + if (typeof ds.source === "string") { + pushSafe(ds.source); + } + }; + if (Array.isArray(value)) { - return value - .map((item) => { - if (typeof item === "string") return item; - if (typeof item === "object" && item !== null) { - const obj = item as DiscoverySource; - return typeof obj.source === "string" ? obj.source : ""; - } - if (typeof item === "number" || typeof item === "boolean") { - return String(item); - } - return ""; - }) - .filter((s): s is string => typeof s === "string" && s.length > 0); + for (const item of value) { + if (typeof item === "string") { + pushSafe(item); + } else if (typeof item === "number" || typeof item === "boolean") { + pushSafe(String(item)); + } else { + fromDiscoverySource(item); + } + if (out.length >= MAX_DISCOVERY_SOURCES) break; + } + return out; } - if (typeof value !== "string") { - return []; + if (typeof value === "string") { + for (const token of value.split(",")) { + const t = token.trim(); + if (t) pushSafe(t); + if (out.length >= MAX_DISCOVERY_SOURCES) break; + } + return out; } - return value - .split(",") - .map((item) => item.trim()) - .filter((s) => s.length > 0); + return out; }; Refactor the ensureArray function to more securely handle discovery_sources by validating object shapes, only accepting whitelisted keys, and capping the output array length. web/src/components/Devices/DeviceDetail.tsx [192-221] const ensureArray = ( value: string[] | string | DiscoverySource[] | undefined, ): string[] => { if (value == null) return []; + const out: string[] = []; + const pushSafe = (s: string) => { + if (typeof s === "string" && s.length > 0) out.push(s); + }; + + const isPlainObject = (v: unknown): v is Record<string, unknown> => { + if (Object.prototype.toString.call(v) !== "[object Object]") return false; + const proto = Object.getPrototypeOf(v); + return proto === null || proto === Object.prototype; + }; + + const fromDiscoverySource = (obj: unknown) => { + if (!isPlainObject(obj)) return; + const ds = obj as DiscoverySource; + if (typeof ds.source === "string") pushSafe(ds.source); + }; + if (Array.isArray(value)) { - return value - .map((item) => { - if (typeof item === "string") return item; - if (typeof item === "object" && item !== null) { - const obj = item as DiscoverySource; - return typeof obj.source === "string" ? obj.source : ""; - } - if (typeof item === "number" || typeof item === "boolean") { - return String(item); - } - return ""; - }) - .filter((s): s is string => typeof s === "string" && s.length > 0); + for (const item of value) { + if (typeof item === "string") { + pushSafe(item); + } else if (typeof item === "number" || typeof item === "boolean") { + pushSafe(String(item)); + } else { + fromDiscoverySource(item); + } + if (out.length >= 50) break; // cap to reasonable size + } + return out; } - if (typeof value !== "string") { - return []; + if (typeof value === "string") { + for (const token of value.split(",")) { + const t = token.trim(); + if (t) pushSafe(t); + if (out.length >= 50) break; + } + return out; } - return value - .split(",") - .map((item) => item.trim()) - .filter((s) => s.length > 0); + return out; }; [Suggestion processed] Suggestion importance[1-10]: 4 __ Why: The suggestion provides a more robust and secure implementation for the ensureArray function by adding stricter type checking and length capping, though the described risks are somewhat theoretical.	Low
General	✅ Keep service devices uniquely canonical Suggestion Impact: The commit changed the key selection logic to prioritize DeviceID for service devices, then canonicalID, matching the suggestion’s intent to uniquely key service devices. code diff: + var key string + switch { + case models.IsServiceDevice(deviceID): + key = deviceID + case canonicalID != "": + key = canonicalID + default: key = deviceID } In selectCanonicalRecords, modify the key selection logic to always use record.DeviceID as the canonical key for service devices. This prevents them from being incorrectly merged with other device records that might share the same canonical ID alias. pkg/core/stats_aggregator.go [454-572] -func (a *StatsAggregator) selectCanonicalRecords(records []*registry.DeviceRecord, meta *models.DeviceStatsMeta) []*registry.DeviceRecord { //nolint:gocyclo // Complex device record selection logic, intentionally kept together for clarity +func (a *StatsAggregator) selectCanonicalRecords(records []*registry.DeviceRecord, meta *models.DeviceStatsMeta) []*registry.DeviceRecord { //nolint:gocyclo if len(records) == 0 { return nil } canonical := make(map[string]canonicalEntry) fallback := make(map[string]*registry.DeviceRecord) processRecord := func(record *registry.DeviceRecord) { if !shouldCountRecord(record) { if meta != nil { meta.SkippedSweepOnlyRecords++ } return } deviceID := strings.TrimSpace(record.DeviceID) canonicalID := canonicalDeviceID(record) - key := canonicalID - if key == "" { + var key string + // Service devices should be uniquely keyed by their own DeviceID to avoid + // coalescing with host devices that may share IP/aliases. + if models.IsServiceDevice(deviceID) { + key = deviceID + } else if canonicalID != "" { + key = canonicalID + } else { key = deviceID } + key = strings.TrimSpace(key) if key == "" { if meta != nil { meta.SkippedNonCanonical++ } return } normalizedKey := strings.ToLower(key) if canonicalID != "" && strings.EqualFold(canonicalID, deviceID) { if entry, ok := canonical[normalizedKey]; ok { if entry.canonical { if shouldReplaceRecord(entry.record, record) { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } else if meta != nil { meta.SkippedNonCanonical++ } } else { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } } else { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } return } if entry, ok := canonical[normalizedKey]; ok { switch { case entry.canonical: if meta != nil { meta.SkippedNonCanonical++ } case shouldReplaceRecord(entry.record, record): canonical[normalizedKey] = canonicalEntry{record: record, canonical: false} default: if meta != nil { meta.SkippedNonCanonical++ } } return } if existing, ok := fallback[normalizedKey]; ok { if shouldReplaceRecord(existing, record) { fallback[normalizedKey] = record } if meta != nil { meta.SkippedNonCanonical++ } return } fallback[normalizedKey] = record } for _, record := range records { if record == nil { if meta != nil { meta.SkippedNilRecords++ } continue } if isTombstonedRecord(record) { if meta != nil { meta.SkippedTombstonedRecords++ } continue } - - // All records (including pollers, agents, global services) are counted as devices. - // Even if service components share an IP with other devices, they maintain - // separate device records in inventory for tracking purposes. processRecord(record) } for key, record := range fallback { if _, ok := canonical[key]; ok { + continue + } + canonical[key] = canonicalEntry{record: record, canonical: false} + } + out := make([]*registry.DeviceRecord, 0, len(canonical)) + for _, entry := range canonical { + out = append(out, entry.record) + } + return out +} + Suggestion importance[1-10]: 9 __ Why: This suggestion identifies a critical edge case where service devices could be incorrectly deduplicated against other devices if they share a common alias (like an IP address), leading to undercounting. Forcing service devices to use their unique DeviceID as the canonical key is a robust solution to ensure they are always counted separately, which aligns with the PR's intent.	High
Possible issue	✅ Validate and normalize host IPs Suggestion Impact: The commit added a normalizeHostIP function and applied it across the code: when storing poller status, when registering pollers and services, and when resolving host IPs, including metadata and fallback cases. It also replaced raw req.SourceIp usage with the normalized IP in multiple paths. code diff: +func normalizeHostIP(raw string) string { + ip := strings.TrimSpace(raw) + if ip == "" { + return "" + } + + if host, _, err := net.SplitHostPort(ip); err == nil && host != "" { + ip = host + } + + if strings.HasPrefix(ip, "[") && strings.Contains(ip, "]") { + if end := strings.Index(ip, "]"); end > 0 { + ip = ip[1:end] + } + } + + ip = strings.TrimSpace(ip) + if ip == "" { + return "" + } + + if net.ParseIP(ip) == nil { + return "" + } + + return ip +} + func (s *Server) storePollerStatus(ctx context.Context, pollerID string, isHealthy bool, hostIP string, now time.Time) error { + normIP := normalizeHostIP(hostIP) + pollerStatus := &models.PollerStatus{ PollerID: pollerID, IsHealthy: isHealthy, LastSeen: now, - HostIP: hostIP, + HostIP: normIP, } if err := s.DB.UpdatePollerStatus(ctx, pollerStatus); err != nil { @@ -399,7 +430,7 @@ } // Register poller as a device for inventory tracking (best-effort) - if err := s.registerPollerAsDevice(ctx, pollerID, hostIP); err != nil { + if err := s.registerPollerAsDevice(ctx, pollerID, normIP); err != nil { s.logger.Warn(). Err(err). Str("poller_id", pollerID). @@ -565,6 +596,7 @@ IsHealthy: true, LastSeen: now, } + normSourceIP := normalizeHostIP(req.SourceIp) existingStatus, err := s.DB.GetPollerStatus(ctx, req.PollerId) if err == nil { @@ -581,7 +613,7 @@ } // Register poller as a device for inventory tracking - if err := s.registerPollerAsDevice(ctx, req.PollerId, req.SourceIp); err != nil { + if err := s.registerPollerAsDevice(ctx, req.PollerId, normSourceIP); err != nil { // Log but don't fail - device registration is best-effort s.logger.Warn(). Err(err). @@ -602,10 +634,10 @@ } // Register the poller/agent as a device - go func() { + go func(normalizedIP string) { // Run in a separate goroutine to not block the main status report flow. // Skip registration if location data is missing. A warning is already logged in ReportStatus. - if req.Partition == "" || req.SourceIp == "" { + if req.Partition == "" || normalizedIP == "" { return } @@ -614,13 +646,13 @@ defer cancel() if err := s.registerServiceDevice(timeoutCtx, req.PollerId, s.findAgentID(req.Services), - req.Partition, req.SourceIp, now); err != nil { + req.Partition, normalizedIP, now); err != nil { s.logger.Warn(). Err(err). Str("poller_id", req.PollerId). Msg("Failed to register service device for poller") } - }() + }(normSourceIP) return apiStatus, nil } @@ -658,10 +690,10 @@ s.processServices(ctx, req.PollerId, req.Partition, req.SourceIp, apiStatus, req.Services, now) // Register the poller/agent as a device for new pollers too - go func() { + go func(normalizedIP string) { // Run in a separate goroutine to not block the main status report flow. // Skip registration if location data is missing. A warning is already logged in ReportStatus. - if req.Partition == "" || req.SourceIp == "" { + if req.Partition == "" || normalizedIP == "" { return } @@ -670,13 +702,13 @@ defer cancel() if err := s.registerServiceDevice(timeoutCtx, req.PollerId, s.findAgentID(req.Services), - req.Partition, req.SourceIp, now); err != nil { + req.Partition, normalizedIP, now); err != nil { s.logger.Warn(). Err(err). Str("poller_id", req.PollerId). Msg("Failed to register service device for poller") } - }() + }(normSourceIP) return apiStatus, nil } @@ -1286,14 +1318,14 @@ return nil // Registry not available } - resolvedIP := strings.TrimSpace(hostIP) + resolvedIP := normalizeHostIP(hostIP) if resolvedIP == "" && s.ServiceRegistry != nil { if poller, err := s.ServiceRegistry.GetPoller(ctx, pollerID); err == nil && poller != nil { if poller.Metadata != nil { - if ip := strings.TrimSpace(poller.Metadata["source_ip"]); ip != "" { + if ip := normalizeHostIP(poller.Metadata["source_ip"]); ip != "" { resolvedIP = ip - } else if ip := strings.TrimSpace(poller.Metadata["host_ip"]); ip != "" { + } else if ip := normalizeHostIP(poller.Metadata["host_ip"]); ip != "" { resolvedIP = ip } } @@ -1301,8 +1333,9 @@ } if resolvedIP == "" { - // Fall back to the local host IP if we cannot determine the poller's address - resolvedIP = s.getHostIP() + if fallback := normalizeHostIP(s.getHostIP()); fallback != "" { + resolvedIP = fallback + } } metadata := map[string]string{ @@ -1311,11 +1344,13 @@ deviceUpdate := models.CreatePollerDeviceUpdate(pollerID, resolvedIP, metadata) - s.logger.Info(). + logger := s.logger.Info(). Str("poller_id", pollerID). - Str("device_id", deviceUpdate.DeviceID). - Str("host_ip", resolvedIP). - Msg("Registering poller as device") + Str("device_id", deviceUpdate.DeviceID) + if resolvedIP != "" { + logger = logger.Str("host_ip", resolvedIP) + } + logger.Msg("Registering poller as device") if err := s.DeviceRegistry.ProcessBatchDeviceUpdates(ctx, []*models.DeviceUpdate{deviceUpdate}); err != nil { return err Add a function to normalize and validate the hostIP before it is stored or used for device registration. This function should handle trimming, stripping IPv6 brackets and ports, and use net.ParseIP for validation. [pkg/core/pollers.go [389-410]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-fe81e2a32f1ac64bcdc6f25f55c5fa918d17bad8c0546f2cf80c757ff40...

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1937#issuecomment-3523400131 Original created: 2025-11-12T18:41:14Z --- ## PR Code Suggestions ✨ <!-- 0022a97 --> Latest suggestions up to 0022a97 <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=8>Incremental ^{<a href='https://qodo-merge-docs.qodo.ai/core-abilities/incremental_update/'>[*]</a>}</td> <td> <details><summary>Fix incomplete INSERT statement</summary> ___ **Add the missing <code>VALUES</code> clause to the <code>insertUserStatement</code> constant to fix the SQL <br>syntax for user creation and update operations.** [pkg/db/auth.go [27-98]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-4d4e15c7e925bdd07a11ca5d2779a6f2acef74c2bebe286c4bbc9a7593cefdb7R27-R98) ```diff -const insertUserStatement = "INSERT INTO users (id, username, email, provider, password_hash, created_at, updated_at, is_active, roles)" +const insertUserStatement = ` +INSERT INTO users ( + id, username, email, provider, password_hash, created_at, updated_at, is_active, roles +) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) +` ... batch, err := db.Conn.PrepareBatch(ctx, insertUserStatement) if err != nil { return fmt.Errorf("failed to prepare batch: %w", err) } if err := batch.Append( user.ID, user.Name, // username field user.Email, normalizeProvider(user.Provider), "", // password_hash (empty for OAuth users) user.CreatedAt, user.UpdatedAt, true, // is_active (default to true) normalizeRoles(user.Roles), // roles (default role if missing) ); err != nil { return fmt.Errorf("failed to append user: %w", err) } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 10</summary> __ Why: This suggestion correctly identifies a critical bug where the `insertUserStatement` is missing the `VALUES` clause, which would cause all user creation and update operations to fail at runtime. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>Use upsert instead of insert</summary> ___ **In <code>UpdateUser</code>, replace the <code>INSERT</code> statement with an <code>UPSERT</code> (<code>INSERT ... ON </code><br><code>CONFLICT ... DO UPDATE</code>) to correctly update existing user records instead of <br>creating duplicates.** [pkg/db/auth.go [175-203]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-4d4e15c7e925bdd07a11ca5d2779a6f2acef74c2bebe286c4bbc9a7593cefdb7R175-R203) ```diff func (db *DB) UpdateUser(ctx context.Context, user *models.User) error { user.UpdatedAt = time.Now() - batch, err := db.Conn.PrepareBatch(ctx, insertUserStatement) + const upsertUserStatement = ` + INSERT INTO users ( + id, username, email, provider, password_hash, created_at, updated_at, is_active, roles + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT (id) DO UPDATE SET + username = excluded.username, + email = excluded.email, + provider = excluded.provider, + password_hash = excluded.password_hash, + updated_at = excluded.updated_at, + is_active = excluded.is_active, + roles = excluded.roles + ` + + batch, err := db.Conn.PrepareBatch(ctx, upsertUserStatement) if err != nil { return fmt.Errorf("failed to prepare batch: %w", err) } if err := batch.Append( user.ID, - user.Name, // username field + user.Name, user.Email, normalizeProvider(user.Provider), - "", // password_hash (empty for OAuth users) + "", user.CreatedAt, user.UpdatedAt, - true, // is_active (default to true) - normalizeRoles(user.Roles), // roles (default role if missing) + true, + normalizeRoles(user.Roles), ); err != nil { - return fmt.Errorf("failed to append user update: %w", err) + return fmt.Errorf("failed to append user upsert: %w", err) } if err := batch.Send(); err != nil { - return fmt.Errorf("failed to send update user batch: %w", err) + return fmt.Errorf("failed to send user upsert batch: %w", err) } - return nil } ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=1 --> <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This suggestion correctly points out that using an `INSERT` statement in `UpdateUser` is a bug that would lead to duplicate data or errors, and it proposes a correct `UPSERT` pattern to fix it. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>Reinstate localhost bind safety<!-- not_implemented --></summary> ___ **Reinstate the <code>listen_host: 127.0.0.1</code> setting in the configuration file to <br>prevent services from binding to all network interfaces and being accidentally <br>exposed externally.** [packaging/proton/config/config.yaml [31-45]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-457b4de30f735ca58a681ea2207984e42d2cd9bbbf1b34d7e4840dbbd299a5feR31-R45) ```diff node: # Roles that the current node has # Supported roles : Metadata, Data, Compute # `Data` role contains `Compute` roles by default. # `Compute` node can't have streams storing data on them. Can serve ingest or query roles: role: - Metadata - Data # Public protocols and ports http: port: 3218 # Maintained default, was http_port in old config is_tls_port: false +# Restrict server to localhost to avoid unintended external exposure in single-instance setups +listen_host: 127.0.0.1 + ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies the removal of `listen_host: 127.0.0.1` and raises a valid security concern about unintentionally exposing services on all network interfaces, which is a significant regression. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>Prevent Stop deadlock on watcher</summary> ___ **In the <code>Stop</code> function, replace the blocking wait on <code>s.watchDone</code> with a <code>select</code> <br>statement that includes a timeout. This prevents a potential deadlock during <br>shutdown if the config watcher goroutine exits without closing the channel.** [pkg/sweeper/sweeper.go [523-562]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-d11dee1504de681453c5a04e22ae44d64820221ac6b84a1630d3a3929dace47aR523-R562) ```diff func (s *NetworkSweeper) Stop() error { s.logger.Info().Msg("Stopping network sweeper") if s.done == nil { s.logger.Debug().Msg("Sweep service already stopped") return nil } close(s.done) + // Avoid indefinite block if watchDone was never set or already closed. if s.watchDone != nil { - <-s.watchDone // Wait for KV watching to stop + select { + case <-s.watchDone: + // watcher stopped + case <-time.After(5 * time.Second): + s.logger.Warn().Msg("Timeout waiting for KV watcher to stop") + } } s.done = nil s.watchDone = nil if s.icmpScanner != nil { if err := s.icmpScanner.Stop(); err != nil { s.logger.Error().Err(err).Msg("Failed to stop ICMP scanner") } s.icmpScanner = nil } if s.tcpScanner != nil { if err := s.tcpScanner.Stop(); err != nil { s.logger.Error().Err(err).Msg("Failed to stop TCP scanner") } s.tcpScanner = nil } if s.tcpConnectScanner != nil { if err := s.tcpConnectScanner.Stop(); err != nil { s.logger.Error().Err(err).Msg("Failed to stop TCP connect scanner") } s.tcpConnectScanner = nil } return nil } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies a potential deadlock in the `Stop` function if the `watchDone` channel is not closed, and proposes a robust solution using a `select` with a timeout to prevent the service from hanging on shutdown. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Validate mandatory deliver fields</summary> ___ **After decoding the <code>deliverResponse</code> in <code>downloadPackage</code>, add checks to ensure that <br><code>deliver.JoinToken</code> and <code>deliver.BundlePEM</code> are not empty. Return <code>ErrJoinTokenEmpty</code> <br>or <code>ErrBundlePEMEmpty</code> if they are, to fail fast.** [pkg/edgeonboarding/download.go [190-207]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-4502ef07f8a1b9a8363fb9a1685599655ecc0560dac96c9f174247b6900ac12bR190-R207) ```diff -func (b *Bootstrapper) downloadPackage(ctx context.Context) error { - if strings.TrimSpace(b.cfg.PackagePath) != "" { - return b.loadPackageFromArchive(ctx, b.cfg.PackagePath) - } +var deliver deliverResponse +if err := json.NewDecoder(resp.Body).Decode(&deliver); err != nil { + return fmt.Errorf("decode deliver response: %w", err) +} - tokenInfo, err := parseOnboardingToken(b.cfg.Token, b.cfg.PackageID, b.cfg.CoreAPIURL) - if err != nil { - return err - } +if strings.TrimSpace(deliver.JoinToken) == "" { + return ErrJoinTokenEmpty +} +if strings.TrimSpace(deliver.BundlePEM) == "" { + return ErrBundlePEMEmpty +} - coreURL, err := b.resolveCoreAPIURL(tokenInfo) - if err != nil { - return err - } +pkg, err := deliver.Package.toModel() +if err != nil { + return fmt.Errorf("invalid package payload: %w", err) +} - endpoint := fmt.Sprintf("%s/api/admin/edge-packages/%s/download?format=json", coreURL, url.PathEscape(tokenInfo.PackageID)) - payload := map[string]string{"download_token": tokenInfo.DownloadToken} - body, err := json.Marshal(payload) - if err != nil { - return fmt.Errorf("encode download payload: %w", err) - } +b.pkg = pkg +b.downloadResult = &models.EdgeOnboardingDeliverResult{ + Package: pkg, + JoinToken: strings.TrimSpace(deliver.JoinToken), + BundlePEM: []byte(deliver.BundlePEM), +} +b.tokenInfo = tokenInfo - req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(body)) - if err != nil { - return fmt.Errorf("create download request: %w", err) - } - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Accept", "application/json") - - resp, err := b.getHTTPClient().Do(req) - if err != nil { - return fmt.Errorf("request core deliver endpoint: %w", err) - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusOK { - msg := readErrorBody(resp.Body) - if msg != "" { - return fmt.Errorf("%w (%s): %s", ErrCoreDeliverEndpoint, resp.Status, msg) - } - return fmt.Errorf("%w returned %s", ErrCoreDeliverEndpoint, resp.Status) - } - - var deliver deliverResponse - if err := json.NewDecoder(resp.Body).Decode(&deliver); err != nil { - return fmt.Errorf("decode deliver response: %w", err) - } - - pkg, err := deliver.Package.toModel() - if err != nil { - return fmt.Errorf("invalid package payload: %w", err) - } - - b.pkg = pkg - b.downloadResult = &models.EdgeOnboardingDeliverResult{ - Package: pkg, - JoinToken: deliver.JoinToken, - BundlePEM: []byte(deliver.BundlePEM), - } - b.tokenInfo = tokenInfo - ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion improves robustness by adding validation for essential fields from an API response, ensuring the program fails early with a clear error message instead of proceeding with invalid data. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Prevent spurious initial reload</summary> ___ **In <code>StartWatch</code>, prevent an unnecessary reload on initial startup by setting the <br>baseline snapshot and returning early if <code>prevSnapshot</code> is <code>nil</code>.** [pkg/config/kv_client.go [467-510]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-0c911990aa32d43404f0f23dadc0363ec97ed4ca315592e6c9bd24510e82e06dR467-R510) ```diff func (m *KVManager) StartWatch(ctx context.Context, kvKey string, cfg interface{}, logger logger.Logger, onReload func()) { if m == nil || m.client == nil { return } prevSnapshot, err := newConfigSnapshot(cfg) if err != nil && logger != nil { logger.Warn().Err(err).Str("key", kvKey).Msg("Failed to snapshot initial config for KV watch") } StartKVWatchOverlay(ctx, m.client, kvKey, cfg, logger, func() { currSnapshot, snapErr := newConfigSnapshot(cfg) if snapErr != nil { if logger != nil { logger.Warn().Err(snapErr).Str("key", kvKey).Msg("Failed to snapshot config after KV overlay") } return } - if prevSnapshot != nil && bytes.Equal(prevSnapshot.raw, currSnapshot.raw) { + // If this is the first overlay and we have no previous snapshot, set baseline and do not reload. + if prevSnapshot == nil { + prevSnapshot = currSnapshot + return + } + + if bytes.Equal(prevSnapshot.raw, currSnapshot.raw) { // No actual change after overlay; skip reload. return } triggers := map[string]bool{"reload": true, "rebuild": true} - var changed []string - if prevSnapshot != nil { - changed = FieldsChangedByTag(prevSnapshot.value, currSnapshot.value, "hot", triggers) - } + changed := FieldsChangedByTag(prevSnapshot.value, currSnapshot.value, "hot", triggers) if len(changed) == 0 { - changed = []string{"*"} + return } - if len(changed) > 0 { - if logger != nil { - logger.Info().Strs("changed_fields", changed).Msg("Applying hot-reload") - } - if onReload != nil { - onReload() - } - prevSnapshot = currSnapshot + if logger != nil { + logger.Info().Strs("changed_fields", changed).Msg("Applying hot-reload") } + if onReload != nil { + onReload() + } + prevSnapshot = currSnapshot }) } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that the current logic triggers an unnecessary reload on startup and proposes a valid fix to prevent this behavior, improving the startup sequence. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Restore correct engine arity</summary> ___ **The <code>Stream</code> engine signature was changed from <code>Stream(1, 1, rand())</code> to <code>Stream(1, </code><br><code>rand())</code>. Verify if this change in arity is correct and intended, as it might <br>affect engine initialization or sharding semantics.** [pkg/db/migrations/00000000000001_consolidated_serviceradar_schema.up.sql [49-55]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-1e05de747238f2112bb2230aac8db388e4c80eebe84c071eb78e035d64e67eb6R49-R55) ```diff CREATE STREAM IF NOT EXISTS device_updates ( timestamp DateTime64(3), available boolean, metadata map(string, string) -) ENGINE = Stream(1, rand()) +) ENGINE = Stream(1, 1, rand()) PARTITION BY to_start_of_day(timestamp) ORDER BY (timestamp, device_id, poller_id) TTL to_start_of_day(coalesce(timestamp, _tp_time)) + INTERVAL 3 DAY ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a significant and repeated change to the `Stream` engine signature but incorrectly assumes it's an error; the PR seems to intentionally remove the replica parameter for single-instance setups. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Let IP resolution occur centrally</summary> ___ **In <code>storePollerStatus</code>, call <code>registerPollerAsDevice</code> with an empty string for the <br>IP address instead of <code>normIP</code>. This allows <code>registerPollerAsDevice</code> to use its more <br>comprehensive internal logic to resolve the most reliable IP.** [pkg/core/pollers.go [418-438]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-fe81e2a32f1ac64bcdc6f25f55c5fa918d17bad8c0546f2cf80c757ff4051816R418-R438) ```diff func (s *Server) storePollerStatus(ctx context.Context, pollerID string, isHealthy bool, hostIP string, now time.Time) error { normIP := normalizeHostIP(hostIP) pollerStatus := &models.PollerStatus{ PollerID: pollerID, IsHealthy: isHealthy, LastSeen: now, HostIP: normIP, } if err := s.DB.UpdatePollerStatus(ctx, pollerStatus); err != nil { return fmt.Errorf("failed to store poller status: %w", err) } - // Register poller as a device for inventory tracking (best-effort) - if err := s.registerPollerAsDevice(ctx, pollerID, normIP); err != nil { + // Register poller as a device for inventory tracking (best-effort). + // Allow registerPollerAsDevice to resolve the most reliable IP. + if err := s.registerPollerAsDevice(ctx, pollerID, ""); err != nil { s.logger.Warn(). Err(err). Str("poller_id", pollerID). Msg("Failed to register poller as device") } return nil } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that `registerPollerAsDevice` has more robust IP resolution logic, and passing an empty string forces it to use its fallbacks, improving data quality. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=4>Possible issue</td> <td> <details><summary>Fix missing VALUES in insert</summary> ___ **Add the missing <code>VALUES</code> clause with placeholders to the <code>insertPollerStatusQuery</code> <br>and provide all corresponding arguments to <code>batch.Append</code> to fix the malformed <br>SQL.** [pkg/db/pollers.go [391-412]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-4e4ff6d32240a5f2e8d053438abcc4a77959d521bc99028ed2bbcf1e07145631R391-R412) ```diff const insertPollerStatusQuery = ` INSERT INTO pollers ( poller_id, component_id, registration_source, status, spiffe_identity, first_registered, first_seen, last_seen, metadata, created_by, is_healthy, agent_count, checker_count -)` +) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)` func (db *DB) insertPollerStatus(ctx context.Context, status *models.PollerStatus) error { return db.executeBatch(ctx, insertPollerStatusQuery, func(batch driver.Batch) error { return batch.Append( status.PollerID, // poller_id "", // component_id (empty for implicit registration) + status.RegistrationSource, // registration_source + status.Status, // status + status.SPIFFEID, // spiffe_identity + status.FirstRegistered, // first_registered + status.FirstSeen, // first_seen + status.LastSeen, // last_seen + status.Metadata, // metadata + status.CreatedBy, // created_by + status.IsHealthy, // is_healthy + status.AgentCount, // agent_count + status.CheckerCount, // checker_count + ) + }) +} ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a malformed SQL `INSERT` statement that is missing its `VALUES` clause and has an incomplete list of arguments, which would cause a runtime error. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>Remove Buffer dependency in browser</summary> ___ **Replace the Node.js <code>Buffer</code> dependency with browser-native APIs like <code>TextEncoder</code> <br>and <code>btoa</code> for base64url encoding to ensure client-side compatibility and prevent <br>runtime errors.** [web/src/app/admin/edge-packages/page.tsx [19-250]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-d627c4ba6a102a46a28024061371276f01053b63f20c75986aef2631b967bafcR19-R250) ```diff -import { Buffer } from 'buffer'; -... function base64UrlEncode(value: string): string { - return Buffer.from(value, 'utf-8') - .toString('base64') - .replace(/\+/g, '-') - .replace(/\//g, '_') - .replace(/=+$/g, ''); + const encoder = new TextEncoder(); + const bytes = encoder.encode(value); + let binary = ''; + for (let i = 0; i < bytes.length; i++) { + binary += String.fromCharCode(bytes[i]); + } + const b64 = btoa(binary); + return b64.replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/g, ''); } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that using Node's `Buffer` in a Next.js client component can cause runtime errors in browsers and provides a robust, browser-native alternative to prevent potential crashes. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Support more config kinds</summary> ___ **Extend <code>cloneConfigValue</code> to support <code>map</code> and <code>slice</code> types in addition to <code>pointer</code> <br>and <code>struct</code> to make the configuration snapshot logic more robust for <br>hot-reloading.** [pkg/config/kv_client.go [540-562]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-0c911990aa32d43404f0f23dadc0363ec97ed4ca315592e6c9bd24510e82e06dR540-R562) ```diff func cloneConfigValue(cfg interface{}) (interface{}, error) { val := reflect.ValueOf(cfg) if !val.IsValid() { return nil, nil } - kind := val.Kind() - if kind == reflect.Ptr { + switch val.Kind() { + case reflect.Ptr: if val.IsNil() { return nil, nil } elem := val.Elem() clone := reflect.New(elem.Type()) return clone.Interface(), nil - } - - if kind == reflect.Struct { + case reflect.Struct: clone := reflect.New(val.Type()) return clone.Interface(), nil + case reflect.Map: + // Create a pointer to a new map of the same type + m := reflect.MakeMapWithSize(val.Type(), 0) + return m.Addr().Interface(), nil + case reflect.Slice: + // Create an empty slice of the same type + s := reflect.MakeSlice(val.Type(), 0, 0) + return reflect.New(s.Type()).Elem().Addr().Interface(), nil + default: + return nil, fmt.Errorf("%w: %T", errUnsupportedConfigType, cfg) } - - return nil, fmt.Errorf("%w: %T", errUnsupportedConfigType, cfg) } ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=10 --> <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that the `cloneConfigValue` function is incomplete and only handles `struct` and `pointer` types, which limits the hot-reloading feature. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Preserve shard/replica semantics</summary> ___ **Explicitly set <code>shards</code> and <code>replicas</code> in the <code>Stream</code> engine <code>SETTINGS</code> to avoid <br>potential misconfiguration due to the changed engine signature.** [pkg/db/migrations/00000000000001_consolidated_serviceradar_schema.up.sql [311-314]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-1e05de747238f2112bb2230aac8db388e4c80eebe84c071eb78e035d64e67eb6R311-R314) ```diff ) ENGINE = Stream(1, rand()) +SETTINGS shards = 1, replicas = 1 PARTITION BY int_div(to_unix_timestamp(timestamp), 3600) ORDER BY (timestamp, service_name, poller_id) TTL to_start_of_day(coalesce(timestamp, _tp_time)) + INTERVAL 3 DAY ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=11 --> <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly identifies a semantic change in the `Stream` engine signature and proposes making the replica count explicit for robustness, which is a valid consideration during a version upgrade. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=3>General</td> <td> <details><summary>Make Stop idempotent and thread-safe</summary> ___ **Implement thread-safe, idempotent shutdown logic in <code>Stop()</code> by using <code>sync.Once</code> to <br>prevent panics from closing the <code>s.done</code> channel multiple times concurrently.** [pkg/sweeper/sweeper.go [523-538]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-d11dee1504de681453c5a04e22ae44d64820221ac6b84a1630d3a3929dace47aR523-R538) ```diff +// Add to struct: shutdownOnce sync.Once func (s *NetworkSweeper) Stop() error { s.logger.Info().Msg("Stopping network sweeper") + var alreadyStopped bool + s.shutdownOnce.Do(func() { + if s.done != nil { + close(s.done) + } + alreadyStopped = false + }) if s.done == nil { + alreadyStopped = true + } + if alreadyStopped { s.logger.Debug().Msg("Sweep service already stopped") return nil } - - close(s.done) if s.watchDone != nil { <-s.watchDone // Wait for KV watching to stop } s.done = nil s.watchDone = nil + if s.icmpScanner != nil { + _ = s.icmpScanner.Stop() + s.icmpScanner = nil + } + if s.tcpScanner != nil { + _ = s.tcpScanner.Stop() + s.tcpScanner = nil + } + if s.tcpConnectScanner != nil { + _ = s.tcpConnectScanner.Stop() + s.tcpConnectScanner = nil + } + return nil +} + ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a potential panic from concurrent calls to `Stop()` and proposes a valid solution using `sync.Once` to ensure the shutdown logic is idempotent and thread-safe. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Validate URL in structured token</summary> ___ **Validate that <code>coreApiUrl</code> is an absolute URL before including it in the <br>structured token payload to prevent generating malformed tokens that would fail <br>during client bootstrapping.** [web/src/app/admin/edge-packages/page.tsx [252-267]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-d627c4ba6a102a46a28024061371276f01053b63f20c75986aef2631b967bafcR252-R267) ```diff +function isAbsoluteUrl(url: string): boolean { + try { + const u = new URL(url); + return u.protocol === 'https:' || u.protocol === 'http:'; + } catch { + return false; + } +} function buildStructuredToken(packageId: string, downloadToken: string, coreApiUrl: string): string { - const pkg = packageId.trim(); - const token = downloadToken.trim(); + const pkg = packageId?.trim(); + const token = downloadToken?.trim(); if (!pkg || !token) { return ''; } - const payload: { pkg: string; dl: string; api?: string } = { - pkg, - dl: token, - }; - const api = coreApiUrl.trim(); - if (api) { + const payload: { pkg: string; dl: string; api?: string } = { pkg, dl: token }; + const api = coreApiUrl?.trim(); + if (api && isAbsoluteUrl(api)) { payload.api = api; } return `edgepkg-v1:${base64UrlEncode(JSON.stringify(payload))}`; } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out a potential issue where an invalid `coreApiUrl` could lead to a malformed token, and proposes adding validation to improve the robustness of the token generation logic. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Safer JSON negotiation default</summary> ___ **Modify <code>shouldReturnJSON</code> to treat a missing or wildcard (<code>*/*</code>) <code>Accept</code> header as a <br>request for JSON, improving compatibility with generic HTTP clients.** [pkg/core/api/edge_onboarding.go [642-658]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-c494568149a0b526ca5349c1d78d5e5ee7ec4144ecfd324c44f25fef584292bcR642-R658) ```diff func shouldReturnJSON(r *http.Request) bool { if r == nil { return false } - format := strings.ToLower(strings.TrimSpace(r.URL.Query().Get("format"))) - if format == "json" { + if strings.EqualFold(strings.TrimSpace(r.URL.Query().Get("format")), "json") { return true } accept := strings.ToLower(strings.TrimSpace(r.Header.Get("Accept"))) - if accept == "" { - return false + if accept == "" || accept == "*/*" { + // Default to JSON when client doesn't specify a concrete type + return true } - if strings.Contains(accept, "application/json") && !strings.Contains(accept, "application/gzip") { + // Prefer JSON when acceptable; only avoid JSON if gzip is explicitly preferred without JSON + if strings.Contains(accept, "application/json") { return true } return false } ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=14 --> <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion improves the content negotiation logic by correctly handling the `*/*` `Accept` header and defaulting to JSON, which enhances compatibility with generic HTTP clients. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table> ___ #### Previous suggestions <details><summary>✅ Suggestions up to commit e2c4260</summary> <br><table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=2>Incremental ^{<a href='https://qodo-merge-docs.qodo.ai/core-abilities/incremental_update/'>[*]</a>}</td> <td> <details><summary>Harden discovery source parsing</summary> ___ **Harden the parsing of <code>discovery_sources</code> by rejecting objects with unknown keys <br>and adding limits on token length and total byte size to mitigate security risks <br>from malicious input.** [web/src/components/Devices/DeviceDetail.tsx [60-253]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-2dbb02c57d308332683ec3795ef41e65387b5d1c30c559f01b535b5e704ba872R60-R253) ```diff -interface DiscoverySource { - source?: string; - agent_id?: string; - poller_id?: string; - first_seen?: string; - last_seen?: string; - confidence?: number | string; -} +const MAX_DISCOVERY_SOURCES = 50; +const MAX_TOKEN_LENGTH = 256; +const MAX_TOTAL_BYTES = 8 * 1024; // 8KB safety cap -interface DeviceRecord { - device_id: string; - agent_id?: string; - poller_id?: string; - discovery_sources?: string[] | string | DiscoverySource[]; - ip?: string; - mac?: string; - hostname?: string; - first_seen?: string; - last_seen?: string; - is_available?: boolean; - metadata?: Record<string, unknown>; - device_type?: string; - service_type?: string; - service_status?: string; - last_heartbeat?: string | null; - os_info?: string | null; - version_info?: string | null; -} - -const MAX_DISCOVERY_SOURCES = 50; const ALLOWED_DISCOVERY_SOURCE_KEYS = new Set([ "source", "agent_id", "poller_id", "first_seen", "last_seen", "confidence", ]); +const sanitizeToken = (s: string): string | null => { + const t = s.trim(); + if (!t) return null; + // Basic guard to prevent extremely long strings + return t.length <= MAX_TOKEN_LENGTH ? t : t.slice(0, MAX_TOKEN_LENGTH); +}; + const ensureArray = ( value: string[] | string | DiscoverySource[] | undefined, ): string[] => { if (value == null) return []; const out: string[] = []; + let totalBytes = 0; + const pushSafe = (s: string) => { - if (typeof s === "string" && s.length > 0 && out.length < MAX_DISCOVERY_SOURCES) { - out.push(s); + const sanitized = sanitizeToken(s); + if (!sanitized) return; + const bytes = new TextEncoder().encode(sanitized).length; + if ( + out.length < MAX_DISCOVERY_SOURCES && + totalBytes+bytes <= MAX_TOTAL_BYTES + ) { + out.push(sanitized); + totalBytes += bytes; } }; const isPlainObject = (v: unknown): v is Record<string, unknown> => { if (Object.prototype.toString.call(v) !== "[object Object]") return false; const proto = Object.getPrototypeOf(v); return proto === null || proto === Object.prototype; }; const fromDiscoverySource = (obj: unknown) => { if (!isPlainObject(obj)) return; const keys = Object.keys(obj); - if (!keys.every((key) => ALLOWED_DISCOVERY_SOURCE_KEYS.has(key))) return; + // Reject if any disallowed key present + if (keys.some((k) => !ALLOWED_DISCOVERY_SOURCE_KEYS.has(k))) return; const ds = obj as DiscoverySource; if (typeof ds.source === "string") { pushSafe(ds.source); } }; if (Array.isArray(value)) { for (const item of value) { if (typeof item === "string") { pushSafe(item); } else if (typeof item === "number" || typeof item === "boolean") { pushSafe(String(item)); } else { fromDiscoverySource(item); } - if (out.length >= MAX_DISCOVERY_SOURCES) break; + if (out.length >= MAX_DISCOVERY_SOURCES || totalBytes >= MAX_TOTAL_BYTES) break; } return out; } if (typeof value === "string") { for (const token of value.split(",")) { const t = token.trim(); if (t) pushSafe(t); - if (out.length >= MAX_DISCOVERY_SOURCES) break; + if (out.length >= MAX_DISCOVERY_SOURCES || totalBytes >= MAX_TOTAL_BYTES) break; } return out; } return out; }; ``` <!-- /improve --apply_suggestion=0 --> <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion provides valid security hardening for parsing `discovery_sources` by adding checks for disallowed keys and imposing byte limits, which helps prevent potential DoS or unexpected UI behavior from malicious data. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>✅ <s>Fix aggregation detection and routing</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit updated the AGGREGATION_PATTERN to explicitly list token boundary characters, aligning with the suggestion to improve aggregation detection. Other parts of the suggestion (comment stripping and multi-stream handling) were not implemented in this diff. code diff: ```diff -const AGGREGATION_PATTERN = /(^|[\s([{,;]))stats\s*:/i; +const AGGREGATION_PATTERN = /(^|\s|\(|\{|\[|,|;)stats\s*:/i; ``` </details> ___ **Improve the query routing logic by adding support for stripping comments and <br>ensuring the device planner is not used for queries involving multiple streams.** [web/src/app/api/query/route.ts [13-76]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-53eee72ee62c074b7ff0b7556920802805621e9e16bfa91f85675ec55b66bf32R13-R76) ```diff -const AGGREGATION_PATTERN = /(^|[\s([{,;]))stats\s*:/i; +// Match stats at start or at token boundaries (e.g., start, whitespace, or punctuation) +const AGGREGATION_PATTERN = /(^|[\s([{,;])stats\s*:/i; + +function stripComments(input: string): string { + // Remove single-line // comments and /* block */ comments without disturbing quotes + // This is a best-effort; quoted regions are handled separately by isInsideQuotes. + return input + .replace(/\/\/[^\n\r]*/g, "") + .replace(/\/\*[\s\S]*?\*\//g, ""); +} function hasAggregationOutsideQuotes(query: string): boolean { + const cleaned = stripComments(query); const regex = new RegExp(AGGREGATION_PATTERN.source, "gi"); let match: RegExpExecArray | null; - while ((match = regex.exec(query)) !== null) { - const boundary = match[1] ?? ""; - const statsIndex = match.index + boundary.length; - if (!isInsideQuotes(query, statsIndex)) { + while ((match = regex.exec(cleaned)) !== null) { + const boundaryLen = (match[1] ?? "").length; + const statsIndex = match.index + boundaryLen; + if (!isInsideQuotes(cleaned, statsIndex)) { return true; } } + return false; +} - return false; +function extractPrimaryStream(rawQuery: unknown): string | null { + if (typeof rawQuery !== "string") return null; + + const inMatch = rawQuery.match(/\bin\s*:\s*([^\s]+)/i); + if (!inMatch) return null; + + const raw = inMatch[1] ?? ""; + const candidates = raw.split(/[,|]/).map(cleanToken).filter(Boolean); + // If multiple streams are specified, avoid planner + if (candidates.length !== 1) return null; + return candidates[0]!; } function shouldUseDevicePlanner(query: unknown): boolean { if (typeof query !== "string") return false; if (hasAggregationOutsideQuotes(query)) return false; const stream = extractPrimaryStream(query); return !!stream && DEVICE_PLANNER_STREAMS.has(stream); } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that the routing logic could be more robust by handling comments and multi-stream queries, which could prevent incorrect routing to the device planner. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>✅ <s>Prevent service-device record coalescing</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit changed the key selection logic to prioritize DeviceID for service devices (models.IsServiceDevice), matching the suggestion to avoid coalescing with hosts. code diff: ```diff + var key string + switch { + case models.IsServiceDevice(deviceID): + key = deviceID + case canonicalID != "": + key = canonicalID + default: key = deviceID } ``` </details> ___ **To prevent undercounting, modify the canonical record selection logic to use the <br>unique <code>DeviceID</code> as the key for service devices, ensuring they are not merged <br>with other device records.** [pkg/core/stats_aggregator.go [454-574]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-09304e5f27f54a3a8fa2b57cc6a5485b9f7074d7554babde62b51a75753205e0R454-R574) ```diff -func (a *StatsAggregator) selectCanonicalRecords(records []*registry.DeviceRecord, meta *models.DeviceStatsMeta) []*registry.DeviceRecord { //nolint:gocyclo // Complex device record selection logic, intentionally kept together for clarity +func (a *StatsAggregator) selectCanonicalRecords(records []*registry.DeviceRecord, meta *models.DeviceStatsMeta) []*registry.DeviceRecord { //nolint:gocyclo if len(records) == 0 { return nil } canonical := make(map[string]canonicalEntry) fallback := make(map[string]*registry.DeviceRecord) processRecord := func(record *registry.DeviceRecord) { if !shouldCountRecord(record) { if meta != nil { meta.SkippedSweepOnlyRecords++ } return } deviceID := strings.TrimSpace(record.DeviceID) canonicalID := canonicalDeviceID(record) - key := canonicalID - if key == "" { + var key string + // Ensure service devices are uniquely keyed by DeviceID to prevent coalescing with hosts. + if models.IsServiceDevice(deviceID) { + key = deviceID + } else if canonicalID != "" { + key = canonicalID + } else { key = deviceID } + key = strings.TrimSpace(key) if key == "" { if meta != nil { meta.SkippedNonCanonical++ } return } normalizedKey := strings.ToLower(key) if canonicalID != "" && strings.EqualFold(canonicalID, deviceID) { if entry, ok := canonical[normalizedKey]; ok { if entry.canonical { if shouldReplaceRecord(entry.record, record) { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } else if meta != nil { meta.SkippedNonCanonical++ } } else { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } } else { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } return } if entry, ok := canonical[normalizedKey]; ok { switch { case entry.canonical: if meta != nil { meta.SkippedNonCanonical++ } case shouldReplaceRecord(entry.record, record): canonical[normalizedKey] = canonicalEntry{record: record, canonical: false} default: if meta != nil { meta.SkippedNonCanonical++ } } return } if existing, ok := fallback[normalizedKey]; ok { if shouldReplaceRecord(existing, record) { fallback[normalizedKey] = record } if meta != nil { meta.SkippedNonCanonical++ } return } fallback[normalizedKey] = record } for _, record := range records { if record == nil { if meta != nil { meta.SkippedNilRecords++ } continue } if isTombstonedRecord(record) { if meta != nil { meta.SkippedTombstonedRecords++ } continue } - // All records (including pollers, agents, global services) are counted as devices. - // Even if service components share an IP with other devices, they maintain - // separate device records in inventory for tracking purposes. processRecord(record) } for key, record := range fallback { if _, ok := canonical[key]; ok { continue } canonical[key] = canonicalEntry{record: record, canonical: false} } out := make([]*registry.DeviceRecord, 0, len(canonical)) for _, entry := range canonical { out = append(out, entry.record) } return out } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: This suggestion correctly identifies a potential edge case where service devices could be incorrectly merged with host devices if they share a `canonicalID`. The proposed change makes the new device counting logic more robust by explicitly using the unique `DeviceID` for service components, ensuring they are always counted separately. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table> </details> <details><summary>✅ Suggestions up to commit 8a76746</summary> <br><table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>General</td> <td> <details><summary>✅ <s>Avoid misrouting when "stats:" appears in values</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit updated the aggregation regex to require a token boundary and added logic to check that matches are not inside quotes, then used this check in shouldUseDevicePlanner—addressing the misrouting issue as suggested. code diff: ```diff -const AGGREGATION_PATTERN = /\bstats\s*:/i; +// Match stats aggregations at token boundaries (e.g., "stats:" or " stats :") +const AGGREGATION_PATTERN = /(^|[\s([{,;]))stats\s*:/i; function cleanToken(token: string): string { let t = token.trim(); - // remove surrounding quotes/brackets t = t.replace(/^[\s"'`(]+/, "").replace(/[\s"'`),]+$/, ""); return t.toLowerCase(); +} + +function isInsideQuotes(query: string, targetIndex: number): boolean { + let inSingle = false; + let inDouble = false; + let inBacktick = false; + + for (let i = 0; i < targetIndex; i++) { + const ch = query[i]; + if (ch === "\\" && i + 1 < targetIndex) { + i++; + continue; + } + if (!inDouble && !inBacktick && ch === "'") { + inSingle = !inSingle; + } else if (!inSingle && !inBacktick && ch === '"') { + inDouble = !inDouble; + } else if (!inSingle && !inDouble && ch === "`") { + inBacktick = !inBacktick; + } + } + + return inSingle || inDouble || inBacktick; } function extractPrimaryStream(rawQuery: unknown): string | null { @@ -29,9 +52,25 @@ return candidates.length > 0 ? candidates[0] : null; } +function hasAggregationOutsideQuotes(query: string): boolean { + const regex = new RegExp(AGGREGATION_PATTERN.source, "gi"); + let match: RegExpExecArray | null; + + while ((match = regex.exec(query)) !== null) { + const boundary = match[1] ?? ""; + const statsIndex = match.index + boundary.length; + if (!isInsideQuotes(query, statsIndex)) { + return true; + } + } + + return false; +} + function shouldUseDevicePlanner(query: unknown): boolean { if (typeof query !== "string") return false; - if (AGGREGATION_PATTERN.test(query)) return false; + if (hasAggregationOutsideQuotes(query)) return false; + const stream = extractPrimaryStream(query); return !!stream && DEVICE_PLANNER_STREAMS.has(stream); } ``` </details> ___ **Improve the regex for detecting aggregations in <code>shouldUseDevicePlanner</code> to avoid <br>incorrectly identifying "stats:" within string values, ensuring queries are <br>routed correctly.** [web/src/app/api/query/route.ts [11-37]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-53eee72ee62c074b7ff0b7556920802805621e9e16bfa91f85675ec55b66bf32R11-R37) ```diff const DEVICE_PLANNER_STREAMS = new Set(["devices", "device", "device_inventory"]); -const AGGREGATION_PATTERN = /\bstats\s*:/i; +// Match keys like stats: or stats : at token boundaries, not inside quotes +const AGGREGATION_PATTERN = /(?:^|\s)(stats)\s*:/i; function cleanToken(token: string): string { let t = token.trim(); - // remove surrounding quotes/brackets t = t.replace(/^[\s"'`(]+/, "").replace(/[\s"'`),]+$/, ""); return t.toLowerCase(); +} + +function isInsideQuotes(q: string, matchIndex: number): boolean { + let inSingle = false, inDouble = false, inBack = false; + for (let i = 0; i < matchIndex; i++) { + const ch = q[i]; + if (ch === "\\" && i + 1 < matchIndex) { i++; continue; } + if (!inDouble && !inBack && ch === "'") inSingle = !inSingle; + else if (!inSingle && !inBack && ch === '"') inDouble = !inDouble; + else if (!inSingle && !inDouble && ch === "`") inBack = !inBack; + } + return inSingle || inDouble || inBack; } function extractPrimaryStream(rawQuery: unknown): string | null { if (typeof rawQuery !== "string") return null; const inMatch = rawQuery.match(/\bin\s*:\s*([^\s]+)/i); if (!inMatch) return null; const raw = inMatch[1] ?? ""; const candidates = raw.split(/[,|]/).map(cleanToken).filter(Boolean); return candidates.length > 0 ? candidates[0] : null; } function shouldUseDevicePlanner(query: unknown): boolean { if (typeof query !== "string") return false; - if (AGGREGATION_PATTERN.test(query)) return false; + + const aggMatch = query.match(AGGREGATION_PATTERN); + if (aggMatch && aggMatch.index !== undefined && !isInsideQuotes(query, aggMatch.index)) { + return false; + } + const stream = extractPrimaryStream(query); return !!stream && DEVICE_PLANNER_STREAMS.has(stream); } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies a potential flaw in the regex and proposes a more robust solution to prevent false positives, improving the reliability of the query routing logic. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>Security</td> <td> <details><summary>✅ <s>Sanitize discovery_sources normalization</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit implements a secure refactor of ensureArray: adds plain object checks, whitelists allowed keys, caps array size via MAX_DISCOVERY_SOURCES, and tokenizes strings—matching the suggestion’s intent. code diff: ```diff +const MAX_DISCOVERY_SOURCES = 50; +const ALLOWED_DISCOVERY_SOURCE_KEYS = new Set([ + "source", + "agent_id", + "poller_id", + "first_seen", + "last_seen", + "confidence", +]); const ensureArray = ( value: string[] | string | DiscoverySource[] | undefined, ): string[] => { if (value == null) return []; + const out: string[] = []; + const pushSafe = (s: string) => { + if (typeof s === "string" && s.length > 0 && out.length < MAX_DISCOVERY_SOURCES) { + out.push(s); + } + }; + + const isPlainObject = (v: unknown): v is Record<string, unknown> => { + if (Object.prototype.toString.call(v) !== "[object Object]") return false; + const proto = Object.getPrototypeOf(v); + return proto === null || proto === Object.prototype; + }; + + const fromDiscoverySource = (obj: unknown) => { + if (!isPlainObject(obj)) return; + const keys = Object.keys(obj); + if (!keys.every((key) => ALLOWED_DISCOVERY_SOURCE_KEYS.has(key))) return; + const ds = obj as DiscoverySource; + if (typeof ds.source === "string") { + pushSafe(ds.source); + } + }; + if (Array.isArray(value)) { - return value - .map((item) => { - if (typeof item === "string") return item; - if (typeof item === "object" && item !== null) { - const obj = item as DiscoverySource; - return typeof obj.source === "string" ? obj.source : ""; - } - if (typeof item === "number" || typeof item === "boolean") { - return String(item); - } - return ""; - }) - .filter((s): s is string => typeof s === "string" && s.length > 0); + for (const item of value) { + if (typeof item === "string") { + pushSafe(item); + } else if (typeof item === "number" || typeof item === "boolean") { + pushSafe(String(item)); + } else { + fromDiscoverySource(item); + } + if (out.length >= MAX_DISCOVERY_SOURCES) break; + } + return out; } - if (typeof value !== "string") { - return []; + if (typeof value === "string") { + for (const token of value.split(",")) { + const t = token.trim(); + if (t) pushSafe(t); + if (out.length >= MAX_DISCOVERY_SOURCES) break; + } + return out; } - return value - .split(",") - .map((item) => item.trim()) - .filter((s) => s.length > 0); + return out; }; ``` </details> ___ **Refactor the <code>ensureArray</code> function to more securely handle <code>discovery_sources</code> by <br>validating object shapes, only accepting whitelisted keys, and capping the <br>output array length.** [web/src/components/Devices/DeviceDetail.tsx [192-221]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-2dbb02c57d308332683ec3795ef41e65387b5d1c30c559f01b535b5e704ba872R192-R221) ```diff const ensureArray = ( value: string[] | string | DiscoverySource[] | undefined, ): string[] => { if (value == null) return []; + const out: string[] = []; + const pushSafe = (s: string) => { + if (typeof s === "string" && s.length > 0) out.push(s); + }; + + const isPlainObject = (v: unknown): v is Record<string, unknown> => { + if (Object.prototype.toString.call(v) !== "[object Object]") return false; + const proto = Object.getPrototypeOf(v); + return proto === null || proto === Object.prototype; + }; + + const fromDiscoverySource = (obj: unknown) => { + if (!isPlainObject(obj)) return; + const ds = obj as DiscoverySource; + if (typeof ds.source === "string") pushSafe(ds.source); + }; + if (Array.isArray(value)) { - return value - .map((item) => { - if (typeof item === "string") return item; - if (typeof item === "object" && item !== null) { - const obj = item as DiscoverySource; - return typeof obj.source === "string" ? obj.source : ""; - } - if (typeof item === "number" || typeof item === "boolean") { - return String(item); - } - return ""; - }) - .filter((s): s is string => typeof s === "string" && s.length > 0); + for (const item of value) { + if (typeof item === "string") { + pushSafe(item); + } else if (typeof item === "number" || typeof item === "boolean") { + pushSafe(String(item)); + } else { + fromDiscoverySource(item); + } + if (out.length >= 50) break; // cap to reasonable size + } + return out; } - if (typeof value !== "string") { - return []; + if (typeof value === "string") { + for (const token of value.split(",")) { + const t = token.trim(); + if (t) pushSafe(t); + if (out.length >= 50) break; + } + return out; } - return value - .split(",") - .map((item) => item.trim()) - .filter((s) => s.length > 0); + return out; }; ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion provides a more robust and secure implementation for the `ensureArray` function by adding stricter type checking and length capping, though the described risks are somewhat theoretical. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>✅ <s>Keep service devices uniquely canonical</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit changed the key selection logic to prioritize DeviceID for service devices, then canonicalID, matching the suggestion’s intent to uniquely key service devices. code diff: ```diff + var key string + switch { + case models.IsServiceDevice(deviceID): + key = deviceID + case canonicalID != "": + key = canonicalID + default: key = deviceID } ``` </details> ___ **In <code>selectCanonicalRecords</code>, modify the key selection logic to always use <br><code>record.DeviceID</code> as the canonical key for service devices. This prevents them <br>from being incorrectly merged with other device records that might share the <br>same canonical ID alias.** [pkg/core/stats_aggregator.go [454-572]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-09304e5f27f54a3a8fa2b57cc6a5485b9f7074d7554babde62b51a75753205e0R454-R572) ```diff -func (a *StatsAggregator) selectCanonicalRecords(records []*registry.DeviceRecord, meta *models.DeviceStatsMeta) []*registry.DeviceRecord { //nolint:gocyclo // Complex device record selection logic, intentionally kept together for clarity +func (a *StatsAggregator) selectCanonicalRecords(records []*registry.DeviceRecord, meta *models.DeviceStatsMeta) []*registry.DeviceRecord { //nolint:gocyclo if len(records) == 0 { return nil } canonical := make(map[string]canonicalEntry) fallback := make(map[string]*registry.DeviceRecord) processRecord := func(record *registry.DeviceRecord) { if !shouldCountRecord(record) { if meta != nil { meta.SkippedSweepOnlyRecords++ } return } deviceID := strings.TrimSpace(record.DeviceID) canonicalID := canonicalDeviceID(record) - key := canonicalID - if key == "" { + var key string + // Service devices should be uniquely keyed by their own DeviceID to avoid + // coalescing with host devices that may share IP/aliases. + if models.IsServiceDevice(deviceID) { + key = deviceID + } else if canonicalID != "" { + key = canonicalID + } else { key = deviceID } + key = strings.TrimSpace(key) if key == "" { if meta != nil { meta.SkippedNonCanonical++ } return } normalizedKey := strings.ToLower(key) if canonicalID != "" && strings.EqualFold(canonicalID, deviceID) { if entry, ok := canonical[normalizedKey]; ok { if entry.canonical { if shouldReplaceRecord(entry.record, record) { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } else if meta != nil { meta.SkippedNonCanonical++ } } else { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } } else { canonical[normalizedKey] = canonicalEntry{record: record, canonical: true} } return } if entry, ok := canonical[normalizedKey]; ok { switch { case entry.canonical: if meta != nil { meta.SkippedNonCanonical++ } case shouldReplaceRecord(entry.record, record): canonical[normalizedKey] = canonicalEntry{record: record, canonical: false} default: if meta != nil { meta.SkippedNonCanonical++ } } return } if existing, ok := fallback[normalizedKey]; ok { if shouldReplaceRecord(existing, record) { fallback[normalizedKey] = record } if meta != nil { meta.SkippedNonCanonical++ } return } fallback[normalizedKey] = record } for _, record := range records { if record == nil { if meta != nil { meta.SkippedNilRecords++ } continue } if isTombstonedRecord(record) { if meta != nil { meta.SkippedTombstonedRecords++ } continue } - - // All records (including pollers, agents, global services) are counted as devices. - // Even if service components share an IP with other devices, they maintain - // separate device records in inventory for tracking purposes. processRecord(record) } for key, record := range fallback { if _, ok := canonical[key]; ok { + continue + } + canonical[key] = canonicalEntry{record: record, canonical: false} + } + out := make([]*registry.DeviceRecord, 0, len(canonical)) + for _, entry := range canonical { + out = append(out, entry.record) + } + return out +} + ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This suggestion identifies a critical edge case where service devices could be incorrectly deduplicated against other devices if they share a common alias (like an IP address), leading to undercounting. Forcing service devices to use their unique `DeviceID` as the canonical key is a robust solution to ensure they are always counted separately, which aligns with the PR's intent. </details></details></td><td align=center>High </td></tr><tr><td rowspan=2>Possible issue</td> <td> <details><summary>✅ <s>Validate and normalize host IPs</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit added a normalizeHostIP function and applied it across the code: when storing poller status, when registering pollers and services, and when resolving host IPs, including metadata and fallback cases. It also replaced raw req.SourceIp usage with the normalized IP in multiple paths. code diff: ```diff +func normalizeHostIP(raw string) string { + ip := strings.TrimSpace(raw) + if ip == "" { + return "" + } + + if host, _, err := net.SplitHostPort(ip); err == nil && host != "" { + ip = host + } + + if strings.HasPrefix(ip, "[") && strings.Contains(ip, "]") { + if end := strings.Index(ip, "]"); end > 0 { + ip = ip[1:end] + } + } + + ip = strings.TrimSpace(ip) + if ip == "" { + return "" + } + + if net.ParseIP(ip) == nil { + return "" + } + + return ip +} + func (s *Server) storePollerStatus(ctx context.Context, pollerID string, isHealthy bool, hostIP string, now time.Time) error { + normIP := normalizeHostIP(hostIP) + pollerStatus := &models.PollerStatus{ PollerID: pollerID, IsHealthy: isHealthy, LastSeen: now, - HostIP: hostIP, + HostIP: normIP, } if err := s.DB.UpdatePollerStatus(ctx, pollerStatus); err != nil { @@ -399,7 +430,7 @@ } // Register poller as a device for inventory tracking (best-effort) - if err := s.registerPollerAsDevice(ctx, pollerID, hostIP); err != nil { + if err := s.registerPollerAsDevice(ctx, pollerID, normIP); err != nil { s.logger.Warn(). Err(err). Str("poller_id", pollerID). @@ -565,6 +596,7 @@ IsHealthy: true, LastSeen: now, } + normSourceIP := normalizeHostIP(req.SourceIp) existingStatus, err := s.DB.GetPollerStatus(ctx, req.PollerId) if err == nil { @@ -581,7 +613,7 @@ } // Register poller as a device for inventory tracking - if err := s.registerPollerAsDevice(ctx, req.PollerId, req.SourceIp); err != nil { + if err := s.registerPollerAsDevice(ctx, req.PollerId, normSourceIP); err != nil { // Log but don't fail - device registration is best-effort s.logger.Warn(). Err(err). @@ -602,10 +634,10 @@ } // Register the poller/agent as a device - go func() { + go func(normalizedIP string) { // Run in a separate goroutine to not block the main status report flow. // Skip registration if location data is missing. A warning is already logged in ReportStatus. - if req.Partition == "" || req.SourceIp == "" { + if req.Partition == "" || normalizedIP == "" { return } @@ -614,13 +646,13 @@ defer cancel() if err := s.registerServiceDevice(timeoutCtx, req.PollerId, s.findAgentID(req.Services), - req.Partition, req.SourceIp, now); err != nil { + req.Partition, normalizedIP, now); err != nil { s.logger.Warn(). Err(err). Str("poller_id", req.PollerId). Msg("Failed to register service device for poller") } - }() + }(normSourceIP) return apiStatus, nil } @@ -658,10 +690,10 @@ s.processServices(ctx, req.PollerId, req.Partition, req.SourceIp, apiStatus, req.Services, now) // Register the poller/agent as a device for new pollers too - go func() { + go func(normalizedIP string) { // Run in a separate goroutine to not block the main status report flow. // Skip registration if location data is missing. A warning is already logged in ReportStatus. - if req.Partition == "" || req.SourceIp == "" { + if req.Partition == "" || normalizedIP == "" { return } @@ -670,13 +702,13 @@ defer cancel() if err := s.registerServiceDevice(timeoutCtx, req.PollerId, s.findAgentID(req.Services), - req.Partition, req.SourceIp, now); err != nil { + req.Partition, normalizedIP, now); err != nil { s.logger.Warn(). Err(err). Str("poller_id", req.PollerId). Msg("Failed to register service device for poller") } - }() + }(normSourceIP) return apiStatus, nil } @@ -1286,14 +1318,14 @@ return nil // Registry not available } - resolvedIP := strings.TrimSpace(hostIP) + resolvedIP := normalizeHostIP(hostIP) if resolvedIP == "" && s.ServiceRegistry != nil { if poller, err := s.ServiceRegistry.GetPoller(ctx, pollerID); err == nil && poller != nil { if poller.Metadata != nil { - if ip := strings.TrimSpace(poller.Metadata["source_ip"]); ip != "" { + if ip := normalizeHostIP(poller.Metadata["source_ip"]); ip != "" { resolvedIP = ip - } else if ip := strings.TrimSpace(poller.Metadata["host_ip"]); ip != "" { + } else if ip := normalizeHostIP(poller.Metadata["host_ip"]); ip != "" { resolvedIP = ip } } @@ -1301,8 +1333,9 @@ } if resolvedIP == "" { - // Fall back to the local host IP if we cannot determine the poller's address - resolvedIP = s.getHostIP() + if fallback := normalizeHostIP(s.getHostIP()); fallback != "" { + resolvedIP = fallback + } } metadata := map[string]string{ @@ -1311,11 +1344,13 @@ deviceUpdate := models.CreatePollerDeviceUpdate(pollerID, resolvedIP, metadata) - s.logger.Info(). + logger := s.logger.Info(). Str("poller_id", pollerID). - Str("device_id", deviceUpdate.DeviceID). - Str("host_ip", resolvedIP). - Msg("Registering poller as device") + Str("device_id", deviceUpdate.DeviceID) + if resolvedIP != "" { + logger = logger.Str("host_ip", resolvedIP) + } + logger.Msg("Registering poller as device") if err := s.DeviceRegistry.ProcessBatchDeviceUpdates(ctx, []*models.DeviceUpdate{deviceUpdate}); err != nil { return err ``` </details> ___ **Add a function to normalize and validate the <code>hostIP</code> before it is stored or used <br>for device registration. This function should handle trimming, stripping IPv6 <br>brackets and ports, and use <code>net.ParseIP</code> for validation.** [pkg/core/pollers.go [389-410]](https://github.com/carverauto/serviceradar/pull/1937/files#diff-fe81e2a32f1ac64bcdc6f25f55c5fa918d17bad8c0546f2cf80c757ff40...

qodo-code-review[bot] commented 2025-11-12 20:13:08 +00:00 (Migrated from github.com)

Author

Owner

Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1937#issuecomment-3523729101
Original created: 2025-11-12T20:13:08Z

Persistent suggestions updated to latest commit 96ebe5a

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1937#issuecomment-3523729101 Original created: 2025-11-12T20:13:08Z --- **[Persistent suggestions](https://github.com/carverauto/serviceradar/pull/1937#issuecomment-3523400131)** updated to latest commit 96ebe5a

qodo-code-review[bot] commented 2025-11-12 20:23:16 +00:00 (Migrated from github.com)

Author

Owner

Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1937#issuecomment-3523760389
Original created: 2025-11-12T20:23:16Z

Persistent suggestions updated to latest commit 8a76746

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1937#issuecomment-3523760389 Original created: 2025-11-12T20:23:16Z --- **[Persistent suggestions](https://github.com/carverauto/serviceradar/pull/1937#issuecomment-3523400131)** updated to latest commit 8a76746

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

1week

  

2weeks

  

Failed compliance check

  

IP cameras

  

NATS

NATS JetStream

  

Possible security concern

  

Review effort 1/5

  

Review effort 2/5

  

Review effort 3/5

  

Review effort 4/5

  

Review effort 5/5

  

UI

  

aardvark

  

accessibility

  

amd64

  

api

  

arm64

  

auth

  

back-end

  

bgp

  

blog

  

bug

Something isn't working

  

build

  

checkers

  

ci-cd

continuous integration-continuous deployments

  

cleanup

  

cnpg

cloud-native postgres

  

codex

  

core

core service

  

dependencies

Pull requests that update a dependency file

  

device-management

  

documentation

Improvements or additions to documentation

  

duplicate

This issue or pull request already exists

  

dusk

  

ebpf

  

enhancement

New feature or request

  

eta 1d

  

eta 1hr

  

eta 3d

  

eta 3hr

  

feature

  

fieldsurvey

  

github_actions

Pull requests that update GitHub Actions code

  

go

Pull requests that update Go code

  

good first issue

Good for newcomers

  

help wanted

Extra attention is needed

  

invalid

This doesn't seem right

  

javascript

Pull requests that update Javascript code

  

k8s

  

log-collector

  

mapper

  

mtr

multi traceroute

  

needs-triage

  

netflow

  

network-sweep

  

observability

  

oracle

Oracle Linux related issues

  

otel

opentelemetry logs, traces, metrics

  

plug-in

  

proton

timeplus proton streaming database

  

python

  

question

Further information is requested

  

reddit

  

redhat

  

research

  

rperf

  

rperf-checker

  

rust

Pull requests that update rust code

  

sdk

  

security

  

serviceradar-agent

  

serviceradar-agent-gateway

  

serviceradar-web

  

serviceradar-web-ng

  

siem

  

snmp

  

sysmon

  

topology

  

ubiquiti

  

wasm

  

wontfix

This will not be worked on

  

zen-engine

No labels

1week

2weeks

Failed compliance check

IP cameras

NATS

Possible security concern

Review effort 1/5

Review effort 2/5

Review effort 3/5

Review effort 4/5

Review effort 5/5

UI

aardvark

accessibility

amd64

api

arm64

auth

back-end

bgp

blog

bug

build

checkers

ci-cd

cleanup

cnpg

codex

core

dependencies

device-management

documentation

duplicate

dusk

ebpf

enhancement

eta 1d

eta 1hr

eta 3d

eta 3hr

feature

fieldsurvey

github_actions

go

good first issue

help wanted

invalid

javascript

k8s

log-collector

mapper

mtr

needs-triage

netflow

network-sweep

observability

oracle

otel

plug-in

proton

python

question

reddit

redhat

research

rperf

rperf-checker

rust

sdk

security

serviceradar-agent

serviceradar-agent-gateway

serviceradar-web

serviceradar-web-ng

siem

snmp

sysmon

topology

ubiquiti

wasm

wontfix

zen-engine

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

carverauto/serviceradar!2411

No description provided.