carverauto/serviceradar

Fork 0

Bug/sysmon vm svc not starting #2496

Merged

mfreeman451 merged 7 commits from refs/pull/2496/head into main

2025-12-03 05:16:00 +00:00

mfreeman451 commented

2025-12-03 04:01:11 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2044
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2044
Original created: 2025-12-03T04:01:11Z
Original updated: 2025-12-03T05:16:04Z
Original head: carverauto/serviceradar:bug/sysmon-vm_svc_not_starting
Original base: main
Original merged: 2025-12-03T05:16:00Z 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

Add macOS package installer scripts for automatic service startup
- postinstall: Creates log directory, validates files, bootstraps launchd service
- preinstall: Gracefully stops existing service before upgrade
Auto-restart sysmon-vm service after mTLS bootstrap configuration
- Implements launchctl kickstart in main.go after --mtls-bootstrap-only
- Checks for root privileges and darwin OS before attempting restart
Update packaging build to include pre/postinstall scripts
- Modify package-host-macos.sh to validate and pass scripts to pkgbuild
- Add pkg_scripts filegroup to BUILD.bazel for dependency tracking
Document implementation in openspec change proposal

Diagram Walkthrough

flowchart LR
  A["macOS Package<br/>Installation"] -->|preinstall| B["Stop Existing<br/>Service"]
  B -->|postinstall| C["Create Log Dir<br/>& Validate Files"]
  C --> D["Bootstrap & Enable<br/>LaunchDaemon"]
  D --> E["Service Running<br/>at Boot"]
  F["mTLS Bootstrap<br/>--mtls-bootstrap-only"] -->|restartLaunchdService| G["launchctl kickstart<br/>-k system/com.serviceradar.sysmonvm"]
  G --> H["Service Applies<br/>New Config"]

File Walkthrough

Relevant files

Enhancement

main.go `Add auto-restart after mTLS bootstrap on macOS` cmd/checkers/sysmon-vm/main.go Added imports for `os/exec` and `runtime` to support service restart Added `launchdServiceTarget` constant for macOS launchd service identifier Implemented `restartLaunchdService()` function to restart service after mTLS bootstrap Function checks for darwin OS and root privileges before executing launchctl kickstart Integrated service restart call after `--mtls-bootstrap-only` configuration write	+37/-2
postinstall `New postinstall script for service bootstrap` packaging/sysmonvm_host/scripts/postinstall New bash script executed after package installation Creates `/var/log/serviceradar` directory with proper permissions Validates binary, config file, and plist existence before proceeding Stops any existing service using `launchctl bootout` Bootstraps new service with `launchctl bootstrap system` Enables service for auto-start and immediately starts it with `launchctl kickstart -k`	+89/-0
preinstall `New preinstall script for clean upgrades` packaging/sysmonvm_host/scripts/preinstall New bash script executed before package installation Gracefully stops existing sysmon-vm service if running Uses `launchctl bootout` to cleanly unload service before upgrade Handles case where service doesn't exist (fresh install)	+41/-0
package-host-macos.sh `Update packaging script to include installer hooks` scripts/sysmonvm/package-host-macos.sh Added `PKG_SCRIPTS_DIR` variable pointing to packaging scripts directory Added validation loop to ensure preinstall and postinstall scripts exist and are executable Updated `pkgbuild` command to include `--scripts` parameter with scripts directory Updated success message to indicate scripts are included in package	+11/-1

Configuration changes

BUILD.bazel

Add build dependency for installer scripts

packaging/sysmonvm_host/BUILD.bazel

Added new pkg_scripts filegroup to track preinstall/postinstall
scripts
Updated sysmonvm_host_pkg genrule srcs to depend on :pkg_scripts
Ensures scripts are available during package build process

+10/-1

Documentation

proposal.md

Document sysmon-vm macOS service startup fix

openspec/changes/fix-sysmon-vm-macos-service-startup/proposal.md

New comprehensive change proposal documenting the fix
Explains root causes: missing postinstall script, no launchctl
bootstrap, missing log directory
Details all changes across packaging, build, and binary components
Includes build instructions, test procedures, and manual service
control commands
Documents impact on edge-onboarding and sysmon-telemetry specs

+125/-0

Imported from GitHub pull request. Original GitHub pull request: #2044 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2044 Original created: 2025-12-03T04:01:11Z Original updated: 2025-12-03T05:16:04Z Original head: carverauto/serviceradar:bug/sysmon-vm_svc_not_starting Original base: main Original merged: 2025-12-03T05:16:00Z 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** - Add macOS package installer scripts for automatic service startup - postinstall: Creates log directory, validates files, bootstraps launchd service - preinstall: Gracefully stops existing service before upgrade - Auto-restart sysmon-vm service after mTLS bootstrap configuration - Implements launchctl kickstart in main.go after --mtls-bootstrap-only - Checks for root privileges and darwin OS before attempting restart - Update packaging build to include pre/postinstall scripts - Modify package-host-macos.sh to validate and pass scripts to pkgbuild - Add pkg_scripts filegroup to BUILD.bazel for dependency tracking - Document implementation in openspec change proposal ___ ### Diagram Walkthrough ```mermaid flowchart LR A["macOS Package Installation"] -->|preinstall| B["Stop Existing Service"] B -->|postinstall| C["Create Log Dir & Validate Files"] C --> D["Bootstrap & Enable LaunchDaemon"] D --> E["Service Running at Boot"] F["mTLS Bootstrap --mtls-bootstrap-only"] -->|restartLaunchdService| G["launchctl kickstart -k system/com.serviceradar.sysmonvm"] G --> H["Service Applies New Config"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Enhancement</td><td><table> <tr> <td> <details> <summary>main.go<dd><code>Add auto-restart after mTLS bootstrap on macOS</code>                      </dd></summary> <hr> cmd/checkers/sysmon-vm/main.go <ul><li>Added imports for <code>os/exec</code> and <code>runtime</code> to support service restart <li> Added <code>launchdServiceTarget</code> constant for macOS launchd service identifier <li> Implemented <code>restartLaunchdService()</code> function to restart service after mTLS bootstrap <li> Function checks for darwin OS and root privileges before executing launchctl kickstart <li> Integrated service restart call after <code>--mtls-bootstrap-only</code> configuration write</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2044/files#diff-b1ab49590901666d7536778838bbf3eda1cac7c33257e8e89cb3ab74df8ae376">+37/-2</a>    </td> </tr> <tr> <td> <details> <summary>postinstall<dd><code>New postinstall script for service bootstrap</code>                          </dd></summary> <hr> packaging/sysmonvm_host/scripts/postinstall <ul><li>New bash script executed after package installation <li> Creates <code>/var/log/serviceradar</code> directory with proper permissions <li> Validates binary, config file, and plist existence before proceeding <li> Stops any existing service using <code>launchctl bootout</code> <li> Bootstraps new service with <code>launchctl bootstrap system</code> <li> Enables service for auto-start and immediately starts it with <code>launchctl kickstart -k</code></ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2044/files#diff-5fc12bd241c8f59b0a4244b6ce1984ca31f77f659e2c7e7add37cbe9856d3efc">+89/-0</a>    </td> </tr> <tr> <td> <details> <summary>preinstall<dd><code>New preinstall script for clean upgrades</code>                                  </dd></summary> <hr> packaging/sysmonvm_host/scripts/preinstall <ul><li>New bash script executed before package installation <li> Gracefully stops existing sysmon-vm service if running <li> Uses <code>launchctl bootout</code> to cleanly unload service before upgrade <li> Handles case where service doesn't exist (fresh install)</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2044/files#diff-385ef3c537d1d6adfacc34984c69889cc4e2b142f62e82da6f8a72ee1d13c230">+41/-0</a>    </td> </tr> <tr> <td> <details> <summary>package-host-macos.sh<dd><code>Update packaging script to include installer hooks</code>              </dd></summary> <hr> scripts/sysmonvm/package-host-macos.sh <ul><li>Added <code>PKG_SCRIPTS_DIR</code> variable pointing to packaging scripts directory <li> Added validation loop to ensure preinstall and postinstall scripts exist and are executable <li> Updated <code>pkgbuild</code> command to include <code>--scripts</code> parameter with scripts directory <li> Updated success message to indicate scripts are included in package</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2044/files#diff-5b027d22a6b110520421c7011cf90654b8fe427a5def3fae7b48c749c9b0402e">+11/-1</a>    </td> </tr> </table></td></tr><tr><td>Configuration changes</td><td><table> <tr> <td> <details> <summary>BUILD.bazel<dd><code>Add build dependency for installer scripts</code>                              </dd></summary> <hr> packaging/sysmonvm_host/BUILD.bazel <ul><li>Added new <code>pkg_scripts</code> filegroup to track preinstall/postinstall scripts <li> Updated <code>sysmonvm_host_pkg</code> genrule srcs to depend on <code>:pkg_scripts</code> <li> Ensures scripts are available during package build process</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2044/files#diff-61bb5e1d8b1e798d3a834a32ab46845f70bd9bd4b7d20bbfd76a8016e34f9754">+10/-1</a>    </td> </tr> </table></td></tr><tr><td>Documentation</td><td><table> <tr> <td> <details> <summary>proposal.md<dd><code>Document sysmon-vm macOS service startup fix</code>                          </dd></summary> <hr> openspec/changes/fix-sysmon-vm-macos-service-startup/proposal.md <ul><li>New comprehensive change proposal documenting the fix <li> Explains root causes: missing postinstall script, no launchctl bootstrap, missing log directory <li> Details all changes across packaging, build, and binary components <li> Includes build instructions, test procedures, and manual service control commands <li> Documents impact on edge-onboarding and sysmon-telemetry specs</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2044/files#diff-23212a3c01b653c3006efbbf6ca9782f767e74ce7aa85d7b71805dbb36d562ae">+125/-0</a>  </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-03 04:01:50 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2044#issuecomment-3604995839
Original created: 2025-12-03T04:01:50Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Command execution path hijack Description: The `restartLaunchdService` function executes `launchctl kickstart -k` `system/com.serviceradar.sysmonvm` via `exec.Command` without specifying an absolute path to the binary or sanitizing the environment, which can allow PATH hijacking if the process runs with elevated privileges or a manipulated environment; mitigate by using an absolute path (e.g., `/bin/launchctl` or `/usr/bin/launchctl`) and a controlled environment. main.go [254-266] Referred Code `log.Printf("restarting launchd service %s to apply new configuration...", launchdServiceTarget) // Use launchctl kickstart -k to restart the service // The -k flag kills the running service before restarting cmd := exec.Command("launchctl", "kickstart", "-k", launchdServiceTarget) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr if err := cmd.Run(); err != nil { return fmt.Errorf("launchctl kickstart failed: %w", err) } log.Printf("service restart initiated successfully")`
	Script PATH hijacking Description: The postinstall script runs `launchctl bootstrap/enable/kickstart` using unqualified `launchctl` and does not set a minimal PATH, which can be exploited via PATH hijacking during package installation; invoke `launchctl` with an absolute path (e.g., `/bin/launchctl` or `/usr/bin/launchctl`) and set `PATH` to a safe value. postinstall [68-78] Referred Code `# Load the new service log "Loading service from ${PLIST}..." launchctl bootstrap system "${PLIST}" # Enable the service to start at boot log "Enabling service for auto-start..." launchctl enable "${SERVICE_TARGET}" # Start the service immediately log "Starting service..." launchctl kickstart -k "${SERVICE_TARGET}"`
	Sensitive information exposure Description: The script creates `/var/log/serviceradar` and sets permissions to `755`, but does not set ownership explicitly; if ownership defaults are unexpected, logs could be readable to all users and may leak sensitive information—ensure correct ownership (e.g., root:wheel) and consider stricter file permissions for log files. postinstall [33-40] Referred Code `if [ ! -d "${LOG_DIR}" ]; then log "Creating log directory: ${LOG_DIR}" mkdir -p "${LOG_DIR}" fi # Ensure proper permissions on log directory chmod 755 "${LOG_DIR}"`
	Script PATH hijacking Description: The preinstall script invokes `launchctl` without an absolute path and without exporting a safe PATH, enabling PATH hijacking if an attacker can influence environment during install; use absolute path to `launchctl` and sanitize PATH. preinstall [30-34] Referred Code `# Stop existing service if running (ignore errors if not present) if launchctl print "${SERVICE_TARGET}" >/dev/null 2>&1; then log "Stopping existing sysmon-vm service..." launchctl bootout "${SERVICE_TARGET}" 2>/dev/null \|\| true sleep 1`
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
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: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed 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: 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: The new service restart path logs high-level messages but does not record structured audit details (user ID, timestamp, action outcome) for the critical action of restarting a system service. Referred Code `log.Printf("restarting launchd service %s to apply new configuration...", launchdServiceTarget) // Use launchctl kickstart -k to restart the service // The -k flag kills the running service before restarting cmd := exec.Command("launchctl", "kickstart", "-k", launchdServiceTarget) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr if err := cmd.Run(); err != nil { return fmt.Errorf("launchctl kickstart failed: %w", err) } log.Printf("service restart initiated successfully")` 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: Error fallback: On failure to restart the launchd service, the code logs a note and manual instruction but does not expose an alternative automated fallback or retry strategy, which may be acceptable yet warrants review. Referred Code `if err := restartLaunchdService(); err != nil { log.Printf("note: could not restart launchd service: %v", err) log.Printf("you may need to manually restart: sudo launchctl kickstart -k %s", launchdServiceTarget) }` Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

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

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2044#issuecomment-3604995839 Original created: 2025-12-03T04:01:50Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td rowspan=4>⚪</td> <td><details><summary>Command execution path hijack </summary> Description: The <code>restartLaunchdService</code> function executes <code>launchctl kickstart -k </code> <code>system/com.serviceradar.sysmonvm</code> via <code>exec.Command</code> without specifying an absolute path to the binary or sanitizing the environment, which can allow PATH hijacking if the process runs with elevated privileges or a manipulated environment; mitigate by using an absolute path (e.g., <code>/bin/launchctl</code> or <code>/usr/bin/launchctl</code>) and a controlled environment. <a href='https://github.com/carverauto/serviceradar/pull/2044/files#diff-b1ab49590901666d7536778838bbf3eda1cac7c33257e8e89cb3ab74df8ae376R254-R266'>main.go [254-266]</a> <details open><summary>Referred Code</summary> ```go log.Printf("restarting launchd service %s to apply new configuration...", launchdServiceTarget) // Use launchctl kickstart -k to restart the service // The -k flag kills the running service before restarting cmd := exec.Command("launchctl", "kickstart", "-k", launchdServiceTarget) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr if err := cmd.Run(); err != nil { return fmt.Errorf("launchctl kickstart failed: %w", err) } log.Printf("service restart initiated successfully") ``` </details></details></td></tr> <tr><td><details><summary>Script PATH hijacking </summary> Description: The postinstall script runs <code>launchctl bootstrap/enable/kickstart</code> using unqualified <code>launchctl</code> and does not set a minimal PATH, which can be exploited via PATH hijacking during package installation; invoke <code>launchctl</code> with an absolute path (e.g., <code>/bin/launchctl</code> or <code>/usr/bin/launchctl</code>) and set <code>PATH</code> to a safe value. <a href='https://github.com/carverauto/serviceradar/pull/2044/files#diff-5fc12bd241c8f59b0a4244b6ce1984ca31f77f659e2c7e7add37cbe9856d3efcR68-R78'>postinstall [68-78]</a> <details open><summary>Referred Code</summary> ```txt # Load the new service log "Loading service from ${PLIST}..." launchctl bootstrap system "${PLIST}" # Enable the service to start at boot log "Enabling service for auto-start..." launchctl enable "${SERVICE_TARGET}" # Start the service immediately log "Starting service..." launchctl kickstart -k "${SERVICE_TARGET}" ``` </details></details></td></tr> <tr><td><details><summary>Sensitive information exposure </summary> Description: The script creates <code>/var/log/serviceradar</code> and sets permissions to <code>755</code>, but does not set ownership explicitly; if ownership defaults are unexpected, logs could be readable to all users and may leak sensitive information—ensure correct ownership (e.g., root:wheel) and consider stricter file permissions for log files. <a href='https://github.com/carverauto/serviceradar/pull/2044/files#diff-5fc12bd241c8f59b0a4244b6ce1984ca31f77f659e2c7e7add37cbe9856d3efcR33-R40'>postinstall [33-40]</a> <details open><summary>Referred Code</summary> ```txt if [ ! -d "${LOG_DIR}" ]; then log "Creating log directory: ${LOG_DIR}" mkdir -p "${LOG_DIR}" fi # Ensure proper permissions on log directory chmod 755 "${LOG_DIR}" ``` </details></details></td></tr> <tr><td><details><summary>Script PATH hijacking </summary> Description: The preinstall script invokes <code>launchctl</code> without an absolute path and without exporting a safe PATH, enabling PATH hijacking if an attacker can influence environment during install; use absolute path to <code>launchctl</code> and sanitize PATH. <a href='https://github.com/carverauto/serviceradar/pull/2044/files#diff-385ef3c537d1d6adfacc34984c69889cc4e2b142f62e82da6f8a72ee1d13c230R30-R34'>preinstall [30-34]</a> <details open><summary>Referred Code</summary> ```txt # Stop existing service if running (ignore errors if not present) if launchctl print "${SERVICE_TARGET}" >/dev/null 2>&1; then log "Stopping existing sysmon-vm service..." launchctl bootout "${SERVICE_TARGET}" 2>/dev/null || true sleep 1 ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </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'>Custom Compliance</td></tr> <tr><td rowspan=4>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > 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>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > 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>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** Passed > 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>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** Passed > 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=2>⚪</td> <td><details> <summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2044/files#diff-b1ab49590901666d7536778838bbf3eda1cac7c33257e8e89cb3ab74df8ae376R254-R266'>Action logging</a>: The new service restart path logs high-level messages but does not record structured audit details (user ID, timestamp, action outcome) for the critical action of restarting a system service. <details open><summary>Referred Code</summary> ```go log.Printf("restarting launchd service %s to apply new configuration...", launchdServiceTarget) // Use launchctl kickstart -k to restart the service // The -k flag kills the running service before restarting cmd := exec.Command("launchctl", "kickstart", "-k", launchdServiceTarget) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr if err := cmd.Run(); err != nil { return fmt.Errorf("launchctl kickstart failed: %w", err) } log.Printf("service restart initiated successfully") ``` </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>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2044/files#diff-b1ab49590901666d7536778838bbf3eda1cac7c33257e8e89cb3ab74df8ae376R83-R86'>Error fallback</a>: On failure to restart the launchd service, the code logs a note and manual instruction but does not expose an alternative automated fallback or retry strategy, which may be acceptable yet warrants review. <details open><summary>Referred Code</summary> ```go if err := restartLaunchdService(); err != nil { log.Printf("note: could not restart launchd service: %v", err) log.Printf("you may need to manually restart: sudo launchctl kickstart -k %s", launchdServiceTarget) } ``` </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"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details>

qodo-code-review[bot] commented

2025-12-03 04:02:53 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2044#issuecomment-3604997542
Original created: 2025-12-03T04:02:53Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Improve service startup verification logic Replace the unreliable `sleep` and `launchctl print` check with a robust polling loop using `launchctl list` to verify that the service has a running PID, avoiding race conditions and false warnings. packaging/sysmonvm_host/scripts/postinstall [80-86] # Verify service is running -sleep 2 -if launchctl print "${SERVICE_TARGET}" >/dev/null 2>&1; then - log "Service started successfully" -else - log "WARNING: Service may not have started. Check logs at ${LOG_DIR}/sysmon-vm.log" -fi +log "Verifying service status..." +for i in {1..5}; do + # The PID is the first column. If it's not 0 or -, the process is running. + pid=$(launchctl list "${SERVICE_LABEL}" \| awk 'NR>1 {print $1}') + if [[ -n "${pid}" && "${pid}" -ne 0 && "${pid}" != "-" ]]; then + log "Service started successfully with PID ${pid}" + log "sysmon-vm service setup complete" + exit 0 + fi + log "Waiting for service to start... (${i}/5)" + sleep 1 +done +log "WARNING: Service did not start in time. Check logs at ${LOG_DIR}/sysmon-vm.log" +log "You can check status with: sudo launchctl list ${SERVICE_LABEL}" +exit 1 + Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a race condition in the service verification logic and proposes a more robust polling mechanism to check if the service is actually running, improving the reliability of the post-install script.	Medium
High-level	Decouple application from OS service manager Instead of having the Go binary call macOS's `launchctl` to restart the service, the application should be decoupled from the OS service manager. A better approach is for the running daemon to monitor its configuration file for changes and automatically reload itself. Examples: cmd/checkers/sysmon-vm/main.go [83-86] `if err := restartLaunchdService(); err != nil { log.Printf("note: could not restart launchd service: %v", err) log.Printf("you may need to manually restart: sudo launchctl kickstart -k %s", launchdServiceTarget) }` cmd/checkers/sysmon-vm/main.go [242-268] `// restartLaunchdService restarts the sysmon-vm launchd service on macOS. // This is called after mTLS bootstrap to apply the new configuration. func restartLaunchdService() error { if runtime.GOOS != "darwin" { return nil } // Check if we're running with sufficient privileges if os.Geteuid() != 0 { return fmt.Errorf("root privileges required to restart launchd service") ... (clipped 17 lines)` Solution Walkthrough: Before: `// main.go func run() error { // ... if mtlsBootstrapOnly { log.Printf("mTLS bootstrap-only mode enabled; exiting after writing config") // Call out to OS service manager to restart the service if err := restartLaunchdService(); err != nil { log.Printf("note: could not restart launchd service: %v", err) } return nil } // ... start the actual service } func restartLaunchdService() error { // ... cmd := exec.Command("launchctl", "kickstart", ...) return cmd.Run() }` After: // main.go (bootstrap process) func run() error { // ... if mtlsBootstrapOnly { log.Printf("mTLS bootstrap-only mode enabled; exiting after writing config") // The bootstrap process just writes the config and exits. // The running daemon will detect the change and reload itself. return nil } // ... start the actual service and config watcher go watchConfigAndReload(configPath) // ... } // main.go (running daemon) func watchConfigAndReload(path string) { // Use fsnotify to watch for writes to the config file // On change, re-read config and re-initialize components // e.g., re-initialize mTLS transport } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that calling `launchctl` from Go creates undesirable coupling, but the impact is moderate as the logic is confined to a specific bootstrap flag (`--mtls-bootstrap-only`) and guarded by an OS check.	Low
General	✅ ~~Add timeout to external command~~ Suggestion Impact: The commit changed exec.Command to exec.CommandContext and threaded a context into restartLaunchdService. However, it did not add a timeout or capture output; it kept Stdout/Stderr redirection. code diff: -func restartLaunchdService() error { +func restartLaunchdService(ctx context.Context) error { if runtime.GOOS != "darwin" { return nil } // Check if we're running with sufficient privileges if os.Geteuid() != 0 { - return fmt.Errorf("root privileges required to restart launchd service") + return errRootPrivilegesRequired } log.Printf("restarting launchd service %s to apply new configuration...", launchdServiceTarget) // Use launchctl kickstart -k to restart the service // The -k flag kills the running service before restarting - cmd := exec.Command("launchctl", "kickstart", "-k", launchdServiceTarget) + cmd := exec.CommandContext(ctx, "launchctl", "kickstart", "-k", launchdServiceTarget) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr Replace `exec.Command` with `exec.CommandContext` and add a timeout to prevent the `launchctl` command from hanging. Also, capture the command's output to include it in the error message for better diagnostics. cmd/checkers/sysmon-vm/main.go [256-264] // Use launchctl kickstart -k to restart the service // The -k flag kills the running service before restarting - cmd := exec.Command("launchctl", "kickstart", "-k", launchdServiceTarget) - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() - if err := cmd.Run(); err != nil { - return fmt.Errorf("launchctl kickstart failed: %w", err) + cmd := exec.CommandContext(ctx, "launchctl", "kickstart", "-k", launchdServiceTarget) + output, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("launchctl kickstart failed: %w. Output: %s", err, string(output)) } `[Suggestion processed]` Suggestion importance[1-10]: 6 __ Why: The suggestion improves robustness by adding a timeout to an external command, preventing potential hangs, and enhances error diagnostics by capturing output, which is a good practice.	Low
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2044#issuecomment-3604997542 Original created: 2025-12-03T04:02:53Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td>Category</td><td align=left>Suggestion                                                                                                                                    </td><td align=center>Impact</td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Improve service startup verification logic</summary> ___ **Replace the unreliable <code>sleep</code> and <code>launchctl print</code> check with a robust polling loop using <code>launchctl list</code> to verify that the service has a running PID, avoiding race conditions and false warnings.** [packaging/sysmonvm_host/scripts/postinstall [80-86]](https://github.com/carverauto/serviceradar/pull/2044/files#diff-5fc12bd241c8f59b0a4244b6ce1984ca31f77f659e2c7e7add37cbe9856d3efcR80-R86) ```diff # Verify service is running -sleep 2 -if launchctl print "${SERVICE_TARGET}" >/dev/null 2>&1; then - log "Service started successfully" -else - log "WARNING: Service may not have started. Check logs at ${LOG_DIR}/sysmon-vm.log" -fi +log "Verifying service status..." +for i in {1..5}; do + # The PID is the first column. If it's not 0 or -, the process is running. + pid=$(launchctl list "${SERVICE_LABEL}" | awk 'NR>1 {print $1}') + if [[ -n "${pid}" && "${pid}" -ne 0 && "${pid}" != "-" ]]; then + log "Service started successfully with PID ${pid}" + log "sysmon-vm service setup complete" + exit 0 + fi + log "Waiting for service to start... (${i}/5)" + sleep 1 +done +log "WARNING: Service did not start in time. Check logs at ${LOG_DIR}/sysmon-vm.log" +log "You can check status with: sudo launchctl list ${SERVICE_LABEL}" +exit 1 + ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a race condition in the service verification logic and proposes a more robust polling mechanism to check if the service is actually running, improving the reliability of the post-install script. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>High-level</td> <td> <details><summary>Decouple application from OS service manager</summary> ___ **Instead of having the Go binary call macOS's <code>launchctl</code> to restart the service, the application should be decoupled from the OS service manager. A better approach is for the running daemon to monitor its configuration file for changes and automatically reload itself.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2044/files#diff-b1ab49590901666d7536778838bbf3eda1cac7c33257e8e89cb3ab74df8ae376R83-R86">cmd/checkers/sysmon-vm/main.go [83-86]</a> </summary> ```go if err := restartLaunchdService(); err != nil { log.Printf("note: could not restart launchd service: %v", err) log.Printf("you may need to manually restart: sudo launchctl kickstart -k %s", launchdServiceTarget) } ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2044/files#diff-b1ab49590901666d7536778838bbf3eda1cac7c33257e8e89cb3ab74df8ae376R242-R268">cmd/checkers/sysmon-vm/main.go [242-268]</a> </summary> ```go // restartLaunchdService restarts the sysmon-vm launchd service on macOS. // This is called after mTLS bootstrap to apply the new configuration. func restartLaunchdService() error { if runtime.GOOS != "darwin" { return nil } // Check if we're running with sufficient privileges if os.Geteuid() != 0 { return fmt.Errorf("root privileges required to restart launchd service") ... (clipped 17 lines) ``` </details> ### Solution Walkthrough: #### Before: ```go // main.go func run() error { // ... if *mtlsBootstrapOnly { log.Printf("mTLS bootstrap-only mode enabled; exiting after writing config") // Call out to OS service manager to restart the service if err := restartLaunchdService(); err != nil { log.Printf("note: could not restart launchd service: %v", err) } return nil } // ... start the actual service } func restartLaunchdService() error { // ... cmd := exec.Command("launchctl", "kickstart", ...) return cmd.Run() } ``` #### After: ```go // main.go (bootstrap process) func run() error { // ... if *mtlsBootstrapOnly { log.Printf("mTLS bootstrap-only mode enabled; exiting after writing config") // The bootstrap process just writes the config and exits. // The running daemon will detect the change and reload itself. return nil } // ... start the actual service and config watcher go watchConfigAndReload(configPath) // ... } // main.go (running daemon) func watchConfigAndReload(path string) { // Use fsnotify to watch for writes to the config file // On change, re-read config and re-initialize components // e.g., re-initialize mTLS transport } ``` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that calling `launchctl` from Go creates undesirable coupling, but the impact is moderate as the logic is confined to a specific bootstrap flag (`--mtls-bootstrap-only`) and guarded by an OS check. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>✅ <s>Add timeout to external command</s></summary> ___ <details><summary>Suggestion Impact:</summary>The commit changed exec.Command to exec.CommandContext and threaded a context into restartLaunchdService. However, it did not add a timeout or capture output; it kept Stdout/Stderr redirection. code diff: ```diff -func restartLaunchdService() error { +func restartLaunchdService(ctx context.Context) error { if runtime.GOOS != "darwin" { return nil } // Check if we're running with sufficient privileges if os.Geteuid() != 0 { - return fmt.Errorf("root privileges required to restart launchd service") + return errRootPrivilegesRequired } log.Printf("restarting launchd service %s to apply new configuration...", launchdServiceTarget) // Use launchctl kickstart -k to restart the service // The -k flag kills the running service before restarting - cmd := exec.Command("launchctl", "kickstart", "-k", launchdServiceTarget) + cmd := exec.CommandContext(ctx, "launchctl", "kickstart", "-k", launchdServiceTarget) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr ``` </details> ___ **Replace <code>exec.Command</code> with <code>exec.CommandContext</code> and add a timeout to prevent the <code>launchctl</code> command from hanging. Also, capture the command's output to include it in the error message for better diagnostics.** [cmd/checkers/sysmon-vm/main.go [256-264]](https://github.com/carverauto/serviceradar/pull/2044/files#diff-b1ab49590901666d7536778838bbf3eda1cac7c33257e8e89cb3ab74df8ae376R256-R264) ```diff // Use launchctl kickstart -k to restart the service // The -k flag kills the running service before restarting - cmd := exec.Command("launchctl", "kickstart", "-k", launchdServiceTarget) - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() - if err := cmd.Run(); err != nil { - return fmt.Errorf("launchctl kickstart failed: %w", err) + cmd := exec.CommandContext(ctx, "launchctl", "kickstart", "-k", launchdServiceTarget) + output, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("launchctl kickstart failed: %w. Output: %s", err, string(output)) } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion improves robustness by adding a timeout to an external command, preventing potential hangs, and enhances error diagnostics by capturing output, which is a good practice. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>