feat: better chec-updates logging

[paolomainardi]

PR Type

Enhancement

---

Description

• Add Homebrew package update checking to menu bar app

• Implement dynamic brew binary path detection using 'which'

• Add upgrade brew packages menu item with count display

• Improve update status display with combined indicators

---

Changes walkthrough 📝

	Relevant files
Enhancement	main.swift Add Homebrew integration with update checking                        src/menubar-app/Sources/SparkdockManager/main.swift Add upgradeBrew menu item tag and corresponding UI elements Implement findBrewPath() to dynamically locate brew binary Add runBrewOutdatedCheck() to count outdated packages Update UI to show combined Sparkdock and brew update status Add upgradeBrew() action to execute brew upgrade command +176/-7
Tests	SparkdockManagerTests.swift Add tests for Homebrew integration features                            src/menubar-app/Tests/SparkdockManagerTests/SparkdockManagerTests.swift Add test for new upgradeBrewTag menu item uniqueness Add validation test for /usr/bin/which command existence Add tests for brew command format validation +24/-0
Bug fix	sparkdock.macos Improve update availability logging output                              bin/sparkdock.macos Improve update logging by checking if diff output exists Add conditional check before displaying update information +5/-2

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

[Copilot]

Pull Request Overview

This PR enhances the check-updates functionality by adding Homebrew package update detection to the existing Sparkdock update checking. The menu bar app now monitors both Sparkdock updates and outdated Homebrew packages, providing a unified view of system update status.

Key changes:

• Added Homebrew package monitoring with dynamic brew path detection
• Enhanced UI to display combined update status for both Sparkdock and Homebrew
• Added new menu item for upgrading Homebrew packages when outdated packages are detected

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/menubar-app/Sources/SparkdockManager/main.swift	Added Homebrew update checking, new menu item for brew upgrades, and enhanced UI to show combined update status
src/menubar-app/Tests/SparkdockManagerTests/SparkdockManagerTests.swift	Added test coverage for new menu item tags and Homebrew command validation

[Copilot]

The findBrewPath() function is called on every update check, which involves spawning a process and executing which brew. Consider caching the brew path after the first successful lookup to avoid repeated process execution overhead.

[Copilot]

The brew upgrade command is executed without validating the brew path, potentially allowing execution of any brew binary found in PATH. Consider using the cached brew path from findBrewPath() to ensure you're executing the expected binary.

Suggested change

	executeTerminalCommand("brew upgrade")
	}
	guard let brewPath = findBrewPath() else {
	showErrorAlert("Brew Not Found", "Could not find the Homebrew binary. Please ensure Homebrew is installed.")
	return
	}
	executeTerminalCommand("\"\(brewPath)\" upgrade")
	}
	// Returns the path to the Homebrew binary, or nil if not found
	private func findBrewPath() -> String? {
	let possiblePaths = [
	"/usr/local/bin/brew",
	"/opt/homebrew/bin/brew"
	]
	for path in possiblePaths {
	if FileManager.default.isExecutableFile(atPath: path) {
	return path
	}
	}
	// Fallback: try to locate brew in PATH
	let whichProcess = Process()
	let pipe = Pipe()
	whichProcess.executableURL = URL(fileURLWithPath: "/usr/bin/which")
	whichProcess.arguments = ["brew"]
	whichProcess.standardOutput = pipe
	do {
	try whichProcess.run()
	whichProcess.waitUntilExit()
	let data = pipe.fileHandleForReading.readDataToEndOfFile()
	if let output = String(data: data, encoding: .utf8)?
	.trimmingCharacters(in: .whitespacesAndNewlines),
	!output.isEmpty,
	FileManager.default.isExecutableFile(atPath: output) {
	return output
	}
	} catch {
	// Ignore errors, return nil below
	}
	return nil
	}

[sparkfabrik-ai-bot]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 Security concerns Command injection: The dynamically obtained brew path from findBrewPath() is directly interpolated into a shell command string without validation or sanitization. If an attacker could manipulate the PATH or place a malicious 'brew' executable, this could lead to arbitrary command execution. The brew path should be validated to ensure it points to a legitimate executable location.

⚡ Recommended focus areas for review Process Timeout The brew path detection and outdated check processes use async operations with timeout handling, but the timeout constant AppConstants.processTimeout is referenced without being visible in the diff. Need to verify this constant exists and has appropriate value for external command execution. terminationStatus = try await withTimeout(seconds: AppConstants.processTimeout) { await withCheckedContinuation { continuation in process.terminationHandler = { proc in continuation.resume(returning: proc.terminationStatus) } } } return true } catch { return false } }, onCancel: { process.terminate() } ) if !finished { AppConstants.logger.error("Which brew process timed out after \(AppConstants.processTimeout) seconds") return nil } if terminationStatus == 0 { let data = pipe.fileHandleForReading.readDataToEndOfFile() let path = String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) AppConstants.logger.info("Found brew at: \(path ?? "unknown")") return path } else { AppConstants.logger.warning("Which brew command failed with status: \(terminationStatus)") return nil } } catch { AppConstants.logger.error("Failed to run which brew: \(error)") return nil } } private func runBrewOutdatedCheck() async -> Int { // Use 'which' to find the brew binary dynamically guard let brewPath = await findBrewPath() else { AppConstants.logger.warning("Homebrew not found in PATH") return 0 } let process = Process() process.executableURL = URL(fileURLWithPath: "/bin/sh") process.arguments = ["-c", "\(brewPath) outdated --quiet | wc -l"] let pipe = Pipe() process.standardOutput = pipe process.standardError = Pipe() var terminationStatus: Int32 = -1 do { try process.run() // Await process termination with timeout let finished: Bool = await withTaskCancellationHandler( operation: { do { terminationStatus = try await withTimeout(seconds: AppConstants.processTimeout) { await withCheckedContinuation { continuation in process.terminationHandler = { proc in continuation.resume(returning: proc.terminationStatus) } } } return true } catch { return false } }, onCancel: { // If cancelled, terminate the process process.terminate() } ) if !finished { AppConstants.logger.error("Brew outdated check process timed out after \(AppConstants.processTimeout) seconds") return 0 Shell Injection The brew path is dynamically obtained and then used in shell command construction without proper validation or sanitization. This could potentially allow command injection if the brew path contains malicious characters. process.executableURL = URL(fileURLWithPath: "/bin/sh") process.arguments = ["-c", "\(brewPath) outdated --quiet | wc -l"] Error Handling The findBrewPath() method returns nil on various failure conditions, but the calling code in runBrewOutdatedCheck() treats this as a normal case and returns 0. This might mask legitimate errors and make debugging difficult. guard let brewPath = await findBrewPath() else { AppConstants.logger.warning("Homebrew not found in PATH") return 0 }

[sparkfabrik-ai-bot]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix process termination handler race condition The termination handler is set after process.run(), creating a race condition where the process might complete before the handler is assigned. Set the termination handler before calling process.run() to ensure it's always triggered. src/menubar-app/Sources/SparkdockManager/main.swift [316-368] private func findBrewPath() async -> String? { let process = Process() process.executableURL = URL(fileURLWithPath: "/usr/bin/which") process.arguments = ["brew"] let pipe = Pipe() process.standardOutput = pipe process.standardError = Pipe() var terminationStatus: Int32 = -1 do { - try process.run() - // Await process termination with timeout let finished: Bool = await withTaskCancellationHandler( operation: { do { terminationStatus = try await withTimeout(seconds: AppConstants.processTimeout) { await withCheckedContinuation { continuation in process.terminationHandler = { proc in continuation.resume(returning: proc.terminationStatus) } + try? process.run() } } return true } catch { return false } }, onCancel: { process.terminate() } ) if !finished { AppConstants.logger.error("Which brew process timed out after \(AppConstants.processTimeout) seconds") return nil } if terminationStatus == 0 { let data = pipe.fileHandleForReading.readDataToEndOfFile() let path = String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) AppConstants.logger.info("Found brew at: \(path ?? "unknown")") return path } else { AppConstants.logger.warning("Which brew command failed with status: \(terminationStatus)") return nil } } catch { AppConstants.logger.error("Failed to run which brew: \(error)") return nil } } Suggestion importance[1-10]: 8 __ Why: This addresses a critical race condition where the process might complete before the termination handler is set, which could cause the continuation to never resume. The fix ensures proper async handling.	Medium
General	Remove redundant condition check The condition [ -n "$DIFF" ] is redundant since we already know updates are available when $LOCAL != $REMOTE. This creates unnecessary complexity and potential inconsistency in the return value. bin/sparkdock.macos [47-53] if [ "$LOCAL" != "$REMOTE" ]; then - DIFF=$(git --no-pager log --oneline HEAD..origin/${DEFAULT_BRANCH}) - if [ -n "$DIFF" ]; then - echo "Updates available:" - echo "$DIFF" - fi + echo "Updates available:" + git --no-pager log --oneline HEAD..origin/${DEFAULT_BRANCH} return 0 fi Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies redundant logic since $LOCAL != $REMOTE already guarantees updates exist. However, the original code may handle edge cases where git log returns empty output, so the improvement is moderate.	Low