From 563f5a702cef26a4d95761a4147f0b1e0f9efdb3 Mon Sep 17 00:00:00 2001 From: Alan Wizemann Date: Thu, 2 Apr 2026 03:15:03 -0400 Subject: [PATCH] Improve code quality: error logging, constants, path validation, safe defaults - Replace try? with do/catch and [Scarf] error logging in all service-layer JSON decoding, file writes, and directory creation - Extract sqliteTransient constant replacing raw unsafeBitCast(-1, ...) pattern - Add QueryDefaults and FileSizeUnit enums for all magic numbers - Guard HOME env var with NSHomeDirectory() fallback instead of force-unwrap - Add path traversal validation to loadSkillContent() - Add SessionStats.empty and use it across all initialization sites - Replace KnownPlatforms array indexing with named .cli constant Co-Authored-By: Claude Opus 4.6 (1M context) --- scarf/scarf/Core/Models/HermesConstants.swift | 34 +++++++++++++-- scarf/scarf/Core/Models/HermesTool.swift | 3 +- .../Core/Services/HermesDataService.swift | 42 +++++++++---------- .../Core/Services/HermesFileService.swift | 32 +++++++++++--- .../Core/Services/HermesLogService.swift | 8 +++- .../Services/ProjectDashboardService.swift | 22 ++++++++-- .../ViewModels/DashboardViewModel.swift | 5 +-- .../ViewModels/SessionsViewModel.swift | 6 +-- .../ViewModels/SettingsViewModel.swift | 7 +++- .../Tools/ViewModels/ToolsViewModel.swift | 17 +++++--- 10 files changed, 126 insertions(+), 50 deletions(-) diff --git a/scarf/scarf/Core/Models/HermesConstants.swift b/scarf/scarf/Core/Models/HermesConstants.swift index 7a19bbd..3d733e2 100644 --- a/scarf/scarf/Core/Models/HermesConstants.swift +++ b/scarf/scarf/Core/Models/HermesConstants.swift @@ -1,8 +1,11 @@ import Foundation +import SQLite3 enum HermesPaths: Sendable { - // Using ProcessInfo to avoid main-actor isolation issues with FileManager/NSHomeDirectory - nonisolated static let home: String = ProcessInfo.processInfo.environment["HOME"]! + "/.hermes" + private nonisolated static let userHome: String = ProcessInfo.processInfo.environment["HOME"] + ?? NSHomeDirectory() + + nonisolated static let home: String = userHome + "/.hermes" nonisolated static let stateDB: String = home + "/state.db" nonisolated static let configYAML: String = home + "/config.yaml" nonisolated static let memoriesDir: String = home + "/memories" @@ -15,7 +18,32 @@ enum HermesPaths: Sendable { nonisolated static let skillsDir: String = home + "/skills" nonisolated static let errorsLog: String = home + "/logs/errors.log" nonisolated static let gatewayLog: String = home + "/logs/gateway.log" - nonisolated static let hermesBinary: String = ProcessInfo.processInfo.environment["HOME"]! + "/.local/bin/hermes" + nonisolated static let hermesBinary: String = userHome + "/.local/bin/hermes" nonisolated static let scarfDir: String = home + "/scarf" nonisolated static let projectsRegistry: String = scarfDir + "/projects.json" } + +// MARK: - SQLite Constants + +/// SQLITE_TRANSIENT tells SQLite to make its own copy of bound string data. +/// The C macro is defined as ((sqlite3_destructor_type)-1) which can't be imported directly into Swift. +nonisolated let sqliteTransient = unsafeBitCast(-1, to: sqlite3_destructor_type.self) + +// MARK: - Query Defaults + +enum QueryDefaults: Sendable { + nonisolated static let sessionLimit = 100 + nonisolated static let messageSearchLimit = 50 + nonisolated static let toolCallLimit = 50 + nonisolated static let sessionPreviewLimit = 10 + nonisolated static let previewContentLength = 100 + nonisolated static let logLineLimit = 200 + nonisolated static let defaultSilenceThreshold = 200 +} + +// MARK: - File Size Formatting + +enum FileSizeUnit: Sendable { + nonisolated static let kilobyte = 1_024.0 + nonisolated static let megabyte = 1_048_576.0 +} diff --git a/scarf/scarf/Core/Models/HermesTool.swift b/scarf/scarf/Core/Models/HermesTool.swift index 21f6943..e240876 100644 --- a/scarf/scarf/Core/Models/HermesTool.swift +++ b/scarf/scarf/Core/Models/HermesTool.swift @@ -16,8 +16,9 @@ struct HermesToolPlatform: Identifiable, Sendable { } enum KnownPlatforms { + static let cli = HermesToolPlatform(name: "cli", displayName: "CLI", icon: "terminal") static let all: [HermesToolPlatform] = [ - HermesToolPlatform(name: "cli", displayName: "CLI", icon: "terminal"), + cli, HermesToolPlatform(name: "telegram", displayName: "Telegram", icon: "paperplane"), HermesToolPlatform(name: "discord", displayName: "Discord", icon: "bubble.left.and.bubble.right"), HermesToolPlatform(name: "slack", displayName: "Slack", icon: "number"), diff --git a/scarf/scarf/Core/Services/HermesDataService.swift b/scarf/scarf/Core/Services/HermesDataService.swift index fa5fbbb..a10a955 100644 --- a/scarf/scarf/Core/Services/HermesDataService.swift +++ b/scarf/scarf/Core/Services/HermesDataService.swift @@ -24,7 +24,7 @@ actor HermesDataService { db = nil } - func fetchSessions(limit: Int = 100) -> [HermesSession] { + func fetchSessions(limit: Int = QueryDefaults.sessionLimit) -> [HermesSession] { guard let db else { return [] } let sql = """ SELECT id, source, user_id, model, title, parent_session_id, @@ -59,7 +59,7 @@ actor HermesDataService { var stmt: OpaquePointer? guard sqlite3_prepare_v2(db, sql, -1, &stmt, nil) == SQLITE_OK else { return [] } defer { sqlite3_finalize(stmt) } - sqlite3_bind_text(stmt, 1, sessionId, -1, unsafeBitCast(-1, to: sqlite3_destructor_type.self)) + sqlite3_bind_text(stmt, 1, sessionId, -1, sqliteTransient) var messages: [HermesMessage] = [] while sqlite3_step(stmt) == SQLITE_ROW { @@ -68,7 +68,7 @@ actor HermesDataService { return messages } - func searchMessages(query: String, limit: Int = 50) -> [HermesMessage] { + func searchMessages(query: String, limit: Int = QueryDefaults.messageSearchLimit) -> [HermesMessage] { guard let db else { return [] } let sql = """ SELECT m.id, m.session_id, m.role, m.content, m.tool_call_id, m.tool_calls, @@ -82,7 +82,7 @@ actor HermesDataService { var stmt: OpaquePointer? guard sqlite3_prepare_v2(db, sql, -1, &stmt, nil) == SQLITE_OK else { return [] } defer { sqlite3_finalize(stmt) } - sqlite3_bind_text(stmt, 1, query, -1, unsafeBitCast(-1, to: sqlite3_destructor_type.self)) + sqlite3_bind_text(stmt, 1, query, -1, sqliteTransient) sqlite3_bind_int(stmt, 2, Int32(limit)) var messages: [HermesMessage] = [] @@ -92,7 +92,7 @@ actor HermesDataService { return messages } - func fetchRecentToolCalls(limit: Int = 50) -> [HermesMessage] { + func fetchRecentToolCalls(limit: Int = QueryDefaults.toolCallLimit) -> [HermesMessage] { guard let db else { return [] } let sql = """ SELECT id, session_id, role, content, tool_call_id, tool_calls, @@ -114,10 +114,10 @@ actor HermesDataService { return messages } - func fetchSessionPreviews(limit: Int = 10) -> [String: String] { + func fetchSessionPreviews(limit: Int = QueryDefaults.sessionPreviewLimit) -> [String: String] { guard let db else { return [:] } let sql = """ - SELECT m.session_id, substr(m.content, 1, 100) + SELECT m.session_id, substr(m.content, 1, \(QueryDefaults.previewContentLength)) FROM messages m INNER JOIN ( SELECT session_id, MIN(id) as min_id @@ -149,13 +149,15 @@ actor HermesDataService { let totalInputTokens: Int let totalOutputTokens: Int let totalCostUSD: Double + + static let empty = SessionStats( + totalSessions: 0, totalMessages: 0, totalToolCalls: 0, + totalInputTokens: 0, totalOutputTokens: 0, totalCostUSD: 0 + ) } func fetchStats() -> SessionStats { - guard let db else { - return SessionStats(totalSessions: 0, totalMessages: 0, totalToolCalls: 0, - totalInputTokens: 0, totalOutputTokens: 0, totalCostUSD: 0) - } + guard let db else { return .empty } let sql = """ SELECT COUNT(*), COALESCE(SUM(message_count),0), COALESCE(SUM(tool_call_count),0), COALESCE(SUM(input_tokens),0), COALESCE(SUM(output_tokens),0), @@ -163,16 +165,9 @@ actor HermesDataService { FROM sessions """ var stmt: OpaquePointer? - guard sqlite3_prepare_v2(db, sql, -1, &stmt, nil) == SQLITE_OK else { - return SessionStats(totalSessions: 0, totalMessages: 0, totalToolCalls: 0, - totalInputTokens: 0, totalOutputTokens: 0, totalCostUSD: 0) - } + guard sqlite3_prepare_v2(db, sql, -1, &stmt, nil) == SQLITE_OK else { return .empty } defer { sqlite3_finalize(stmt) } - - guard sqlite3_step(stmt) == SQLITE_ROW else { - return SessionStats(totalSessions: 0, totalMessages: 0, totalToolCalls: 0, - totalInputTokens: 0, totalOutputTokens: 0, totalCostUSD: 0) - } + guard sqlite3_step(stmt) == SQLITE_ROW else { return .empty } return SessionStats( totalSessions: Int(sqlite3_column_int(stmt, 0)), totalMessages: Int(sqlite3_column_int(stmt, 1)), @@ -344,7 +339,12 @@ actor HermesDataService { private func parseToolCalls(_ json: String?) -> [HermesToolCall] { guard let json, !json.isEmpty, let data = json.data(using: .utf8) else { return [] } - return (try? JSONDecoder().decode([HermesToolCall].self, from: data)) ?? [] + do { + return try JSONDecoder().decode([HermesToolCall].self, from: data) + } catch { + print("[Scarf] Failed to decode tool calls: \(error.localizedDescription)") + return [] + } } private func columnText(_ stmt: OpaquePointer, _ col: Int32) -> String { diff --git a/scarf/scarf/Core/Services/HermesFileService.swift b/scarf/scarf/Core/Services/HermesFileService.swift index 0f1d2ef..32be55c 100644 --- a/scarf/scarf/Core/Services/HermesFileService.swift +++ b/scarf/scarf/Core/Services/HermesFileService.swift @@ -44,7 +44,7 @@ struct HermesFileService: Sendable { showReasoning: values["display.show_reasoning"] == "true", verbose: values["agent.verbose"] == "true", autoTTS: values["voice.auto_tts"] != "false", - silenceThreshold: Int(values["voice.silence_threshold"] ?? "") ?? 200 + silenceThreshold: Int(values["voice.silence_threshold"] ?? "") ?? QueryDefaults.defaultSilenceThreshold ) } @@ -52,7 +52,12 @@ struct HermesFileService: Sendable { func loadGatewayState() -> GatewayState? { guard let data = readFileData(HermesPaths.gatewayStateJSON) else { return nil } - return try? JSONDecoder().decode(GatewayState.self, from: data) + do { + return try JSONDecoder().decode(GatewayState.self, from: data) + } catch { + print("[Scarf] Failed to decode gateway state: \(error.localizedDescription)") + return nil + } } // MARK: - Memory @@ -77,8 +82,13 @@ struct HermesFileService: Sendable { func loadCronJobs() -> [HermesCronJob] { guard let data = readFileData(HermesPaths.cronJobsJSON) else { return [] } - let file = try? JSONDecoder().decode(CronJobsFile.self, from: data) - return file?.jobs ?? [] + do { + let file = try JSONDecoder().decode(CronJobsFile.self, from: data) + return file.jobs + } catch { + print("[Scarf] Failed to decode cron jobs: \(error.localizedDescription)") + return [] + } } func loadCronOutput(jobId: String) -> String? { @@ -123,7 +133,13 @@ struct HermesFileService: Sendable { } func loadSkillContent(path: String) -> String { - readFile(path) ?? "" + // Validate path stays within the skills directory to prevent traversal + guard !path.contains(".."), + path.hasPrefix(HermesPaths.skillsDir) else { + print("[Scarf] Rejected skill path outside skills directory: \(path)") + return "" + } + return readFile(path) ?? "" } // MARK: - Hermes Process @@ -156,6 +172,10 @@ struct HermesFileService: Sendable { } private func writeFile(_ path: String, content: String) { - try? content.write(toFile: path, atomically: true, encoding: .utf8) + do { + try content.write(toFile: path, atomically: true, encoding: .utf8) + } catch { + print("[Scarf] Failed to write \(path): \(error.localizedDescription)") + } } } diff --git a/scarf/scarf/Core/Services/HermesLogService.swift b/scarf/scarf/Core/Services/HermesLogService.swift index b05199d..59887dd 100644 --- a/scarf/scarf/Core/Services/HermesLogService.swift +++ b/scarf/scarf/Core/Services/HermesLogService.swift @@ -39,12 +39,16 @@ actor HermesLogService { } func closeLog() { - try? fileHandle?.close() + do { + try fileHandle?.close() + } catch { + print("[Scarf] Failed to close log handle: \(error.localizedDescription)") + } fileHandle = nil currentPath = nil } - func readLastLines(count: Int = 200) -> [LogEntry] { + func readLastLines(count: Int = QueryDefaults.logLineLimit) -> [LogEntry] { guard let path = currentPath, let data = FileManager.default.contents(atPath: path) else { return [] } let content = String(data: data, encoding: .utf8) ?? "" diff --git a/scarf/scarf/Core/Services/ProjectDashboardService.swift b/scarf/scarf/Core/Services/ProjectDashboardService.swift index dbc3fe6..c17a50a 100644 --- a/scarf/scarf/Core/Services/ProjectDashboardService.swift +++ b/scarf/scarf/Core/Services/ProjectDashboardService.swift @@ -8,14 +8,23 @@ struct ProjectDashboardService: Sendable { guard let data = FileManager.default.contents(atPath: HermesPaths.projectsRegistry) else { return ProjectRegistry(projects: []) } - return (try? JSONDecoder().decode(ProjectRegistry.self, from: data)) - ?? ProjectRegistry(projects: []) + do { + return try JSONDecoder().decode(ProjectRegistry.self, from: data) + } catch { + print("[Scarf] Failed to decode project registry: \(error.localizedDescription)") + return ProjectRegistry(projects: []) + } } func saveRegistry(_ registry: ProjectRegistry) { let dir = HermesPaths.scarfDir if !FileManager.default.fileExists(atPath: dir) { - try? FileManager.default.createDirectory(atPath: dir, withIntermediateDirectories: true) + do { + try FileManager.default.createDirectory(atPath: dir, withIntermediateDirectories: true) + } catch { + print("[Scarf] Failed to create scarf directory: \(error.localizedDescription)") + return + } } guard let data = try? JSONEncoder().encode(registry) else { return } // Pretty-print for readability (agents may read this file) @@ -33,7 +42,12 @@ struct ProjectDashboardService: Sendable { guard let data = FileManager.default.contents(atPath: project.dashboardPath) else { return nil } - return try? JSONDecoder().decode(ProjectDashboard.self, from: data) + do { + return try JSONDecoder().decode(ProjectDashboard.self, from: data) + } catch { + print("[Scarf] Failed to decode dashboard for \(project.name): \(error.localizedDescription)") + return nil + } } func dashboardExists(for project: ProjectEntry) -> Bool { diff --git a/scarf/scarf/Features/Dashboard/ViewModels/DashboardViewModel.swift b/scarf/scarf/Features/Dashboard/ViewModels/DashboardViewModel.swift index fbaf879..9c15c3a 100644 --- a/scarf/scarf/Features/Dashboard/ViewModels/DashboardViewModel.swift +++ b/scarf/scarf/Features/Dashboard/ViewModels/DashboardViewModel.swift @@ -5,10 +5,7 @@ final class DashboardViewModel { private let dataService = HermesDataService() private let fileService = HermesFileService() - var stats = HermesDataService.SessionStats( - totalSessions: 0, totalMessages: 0, totalToolCalls: 0, - totalInputTokens: 0, totalOutputTokens: 0, totalCostUSD: 0 - ) + var stats = HermesDataService.SessionStats.empty var recentSessions: [HermesSession] = [] var sessionPreviews: [String: String] = [:] var config = HermesConfig.empty diff --git a/scarf/scarf/Features/Sessions/ViewModels/SessionsViewModel.swift b/scarf/scarf/Features/Sessions/ViewModels/SessionsViewModel.swift index 6aad57c..f76ede3 100644 --- a/scarf/scarf/Features/Sessions/ViewModels/SessionsViewModel.swift +++ b/scarf/scarf/Features/Sessions/ViewModels/SessionsViewModel.swift @@ -158,10 +158,10 @@ final class SessionsViewModel { let fileSize: String if let attrs = try? FileManager.default.attributesOfItem(atPath: dbPath), let size = attrs[.size] as? Int { - if size >= 1_048_576 { - fileSize = String(format: "%.1f MB", Double(size) / 1_048_576) + if Double(size) >= FileSizeUnit.megabyte { + fileSize = String(format: "%.1f MB", Double(size) / FileSizeUnit.megabyte) } else { - fileSize = String(format: "%.0f KB", Double(size) / 1_024) + fileSize = String(format: "%.0f KB", Double(size) / FileSizeUnit.kilobyte) } } else { fileSize = "unknown" diff --git a/scarf/scarf/Features/Settings/ViewModels/SettingsViewModel.swift b/scarf/scarf/Features/Settings/ViewModels/SettingsViewModel.swift index 83f71b1..0ecbf89 100644 --- a/scarf/scarf/Features/Settings/ViewModels/SettingsViewModel.swift +++ b/scarf/scarf/Features/Settings/ViewModels/SettingsViewModel.swift @@ -18,7 +18,12 @@ final class SettingsViewModel { config = fileService.loadConfig() gatewayState = fileService.loadGatewayState() hermesRunning = fileService.isHermesRunning() - rawConfigYAML = (try? String(contentsOfFile: HermesPaths.configYAML, encoding: .utf8)) ?? "" + do { + rawConfigYAML = try String(contentsOfFile: HermesPaths.configYAML, encoding: .utf8) + } catch { + print("[Scarf] Failed to read config.yaml: \(error.localizedDescription)") + rawConfigYAML = "" + } personalities = parsePersonalities() } diff --git a/scarf/scarf/Features/Tools/ViewModels/ToolsViewModel.swift b/scarf/scarf/Features/Tools/ViewModels/ToolsViewModel.swift index db31c22..95949e8 100644 --- a/scarf/scarf/Features/Tools/ViewModels/ToolsViewModel.swift +++ b/scarf/scarf/Features/Tools/ViewModels/ToolsViewModel.swift @@ -2,7 +2,7 @@ import Foundation @Observable final class ToolsViewModel { - var selectedPlatform: HermesToolPlatform = KnownPlatforms.all[0] + var selectedPlatform: HermesToolPlatform = KnownPlatforms.cli var toolsets: [HermesToolset] = [] var mcpStatus: String = "" var isLoading = false @@ -30,7 +30,13 @@ final class ToolsViewModel { } private func loadPlatforms() { - let config = (try? String(contentsOfFile: HermesPaths.configYAML, encoding: .utf8)) ?? "" + let config: String + do { + config = try String(contentsOfFile: HermesPaths.configYAML, encoding: .utf8) + } catch { + print("[Scarf] Failed to read config.yaml: \(error.localizedDescription)") + config = "" + } var platforms: [HermesToolPlatform] = [] var inSection = false for line in config.components(separatedBy: "\n") { @@ -54,9 +60,10 @@ final class ToolsViewModel { } } } - availablePlatforms = platforms.isEmpty ? [KnownPlatforms.all[0]] : platforms - if !availablePlatforms.contains(where: { $0.name == selectedPlatform.name }) { - selectedPlatform = availablePlatforms[0] + availablePlatforms = platforms.isEmpty ? [KnownPlatforms.cli] : platforms + if !availablePlatforms.contains(where: { $0.name == selectedPlatform.name }), + let first = availablePlatforms.first { + selectedPlatform = first } }