From 0b5cc116d09611ad08c170481c634d4a7478aeb4 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Fri, 6 Mar 2026 09:46:47 +0100 Subject: [PATCH 1/2] [#235] Add doctor verification for hooks, settings keys, and gitignore entries - Add HookSettingsCheck, SettingsKeysCheck, and PackGitignoreCheck structs - Extract artifactChecks() helper in DoctorRunner for artifact-record-driven checks - Add GitignoreManager.readLines() shared helper, fix GitignoreCheck to use exact line matching --- Sources/mcs/Core/GitignoreManager.swift | 14 +- Sources/mcs/Doctor/CoreDoctorChecks.swift | 134 +++++++++++- Sources/mcs/Doctor/DoctorRunner.swift | 89 ++++++-- Tests/MCSTests/CoreDoctorCheckTests.swift | 245 +++++++++++++++++++++- 4 files changed, 455 insertions(+), 27 deletions(-) diff --git a/Sources/mcs/Core/GitignoreManager.swift b/Sources/mcs/Core/GitignoreManager.swift index 799961c..75bd925 100644 --- a/Sources/mcs/Core/GitignoreManager.swift +++ b/Sources/mcs/Core/GitignoreManager.swift @@ -2,7 +2,7 @@ import Foundation /// Manages the global gitignore file. Resolves the correct path, /// creates the file if needed, and adds entries idempotently. -struct GitignoreManager: Sendable { +struct GitignoreManager { let shell: ShellRunner /// Core entries managed by mcs (not pack-specific). @@ -81,6 +81,18 @@ struct GitignoreManager: Sendable { return true } + /// Read the global gitignore and return its lines as a set. + /// Returns `nil` if the file doesn't exist or can't be read. + func readLines() -> Set? { + let path = resolveGlobalGitignorePath() + guard FileManager.default.fileExists(atPath: path.path), + let content = try? String(contentsOf: path, encoding: .utf8) + else { + return nil + } + return Set(content.components(separatedBy: .newlines)) + } + /// Add all core gitignore entries. func addCoreEntries() throws { for entry in Self.coreEntries { diff --git a/Sources/mcs/Doctor/CoreDoctorChecks.swift b/Sources/mcs/Doctor/CoreDoctorChecks.swift index 59f3f93..5fe9cba 100644 --- a/Sources/mcs/Doctor/CoreDoctorChecks.swift +++ b/Sources/mcs/Doctor/CoreDoctorChecks.swift @@ -235,19 +235,11 @@ struct GitignoreCheck: DoctorCheck { } func check() -> CheckResult { - let shell = ShellRunner(environment: Environment()) - let gitignoreManager = GitignoreManager(shell: shell) - let gitignorePath = gitignoreManager.resolveGlobalGitignorePath() - guard FileManager.default.fileExists(atPath: gitignorePath.path), - let content = try? String(contentsOf: gitignorePath, encoding: .utf8) - else { + let gitignoreManager = GitignoreManager(shell: ShellRunner(environment: Environment())) + guard let lines = gitignoreManager.readLines() else { return .fail("global gitignore not found") } - let allEntries = GitignoreManager.coreEntries - var missing: [String] = [] - for entry in allEntries where !content.contains(entry) { - missing.append(entry) - } + let missing = GitignoreManager.coreEntries.filter { !lines.contains($0) } if missing.isEmpty { return .pass("all entries present") } @@ -326,6 +318,126 @@ struct ProjectIndexCheck: DoctorCheck { } } +/// Verifies that pack-contributed hook commands are still present in the settings file. +struct HookSettingsCheck: DoctorCheck { + let commands: [String] + let settingsPath: URL + let packName: String + + var name: String { + "Hook entries (\(packName))" + } + + var section: String { + "Hooks" + } + + func check() -> CheckResult { + let settings: Settings + do { + settings = try Settings.load(from: settingsPath) + } catch { + return .fail("cannot read settings: \(error.localizedDescription)") + } + let allCommands = (settings.hooks ?? [:]).values + .flatMap(\.self) + .compactMap(\.hooks) + .flatMap(\.self) + .compactMap(\.command) + let commandSet = Set(allCommands) + let missing = commands.filter { !commandSet.contains($0) } + if missing.isEmpty { + return .pass("all hook commands present") + } + return .fail("missing hook commands: \(missing.joined(separator: ", "))") + } + + func fix() -> FixResult { + .notFixable("Run 'mcs sync' to restore hook entries") + } +} + +/// Verifies that pack-contributed settings keys are still present in the settings file. +struct SettingsKeysCheck: DoctorCheck { + let keys: [String] + let settingsPath: URL + let packName: String + + var name: String { + "Settings keys (\(packName))" + } + + var section: String { + "Settings" + } + + func check() -> CheckResult { + let data: Data + do { + data = try Data(contentsOf: settingsPath) + } catch { + return .fail("settings file not found or unreadable") + } + guard let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any] else { + return .fail("settings file contains invalid JSON") + } + var missing: [String] = [] + for keyPath in keys where !keyExists(keyPath, in: json) { + missing.append(keyPath) + } + if missing.isEmpty { + return .pass("all settings keys present") + } + return .fail("missing settings keys: \(missing.joined(separator: ", "))") + } + + func fix() -> FixResult { + .notFixable("Run 'mcs sync' to restore settings keys") + } + + /// Check if a dot-notation key path exists in a JSON dictionary. + private func keyExists(_ keyPath: String, in json: [String: Any]) -> Bool { + let parts = keyPath.split(separator: ".", maxSplits: 1) + let topLevel = String(parts[0]) + if parts.count == 2 { + let subKey = String(parts[1]) + guard let nested = json[topLevel] as? [String: Any] else { return false } + return nested[subKey] != nil + } + return json[topLevel] != nil + } +} + +/// Verifies that pack-contributed gitignore entries are still present in the global gitignore. +struct PackGitignoreCheck: DoctorCheck { + let entries: [String] + let packName: String + + var name: String { + "Gitignore entries (\(packName))" + } + + var section: String { + "Gitignore" + } + + func check() -> CheckResult { + let gitignoreManager = GitignoreManager(shell: ShellRunner(environment: Environment())) + guard let lines = gitignoreManager.readLines() else { + return .fail("global gitignore not found") + } + let missing = entries.filter { !lines.contains($0) } + if missing.isEmpty { + return .pass("all entries present") + } + return .fail("missing entries: \(missing.joined(separator: ", "))") + } + + func fix() -> FixResult { + .notFixable("Run 'mcs sync' to restore gitignore entries") + } +} + struct CommandFileCheck: DoctorCheck { let name: String let section = "Commands" diff --git a/Sources/mcs/Doctor/DoctorRunner.swift b/Sources/mcs/Doctor/DoctorRunner.swift index 146c986..d4cb47a 100644 --- a/Sources/mcs/Doctor/DoctorRunner.swift +++ b/Sources/mcs/Doctor/DoctorRunner.swift @@ -152,21 +152,14 @@ struct DoctorRunner { allChecks += pack.supplementaryDoctorChecks(projectRoot: scope.effectiveProjectRoot) .map { (check: $0, isExcluded: false) } - // File content drift checks from stored hashes + // Artifact-record-driven checks from stored state if let artifacts = scope.artifactsByPack[pack.identifier] { - let baseURL = scope.effectiveProjectRoot ?? env.claudeDirectory - for (relativePath, expectedHash) in artifacts.fileHashes { - let fileURL = baseURL.appendingPathComponent(relativePath) - allChecks.append(( - check: FileContentCheck( - name: "File content: \(relativePath)", - section: "Installed Files", - path: fileURL, - expectedHash: expectedHash - ), - isExcluded: false - )) - } + allChecks += artifactChecks( + for: artifacts, + pack: pack, + scope: scope, + env: env + ) } } } @@ -403,6 +396,74 @@ struct DoctorRunner { ) } + // MARK: - Artifact-record checks + + /// Builds doctor checks derived from a pack's stored artifact record. + /// Covers file content hashes, hook commands, settings keys, and gitignore entries. + private func artifactChecks( + for artifacts: PackArtifactRecord, + pack: any TechPack, + scope: CheckScope, + env: Environment + ) -> [(check: any DoctorCheck, isExcluded: Bool)] { + var checks: [(check: any DoctorCheck, isExcluded: Bool)] = [] + + let baseURL = scope.effectiveProjectRoot ?? env.claudeDirectory + for (relativePath, expectedHash) in artifacts.fileHashes { + let fileURL = baseURL.appendingPathComponent(relativePath) + checks.append(( + check: FileContentCheck( + name: "File content: \(relativePath)", + section: "Installed Files", + path: fileURL, + expectedHash: expectedHash + ), + isExcluded: false + )) + } + + if !artifacts.hookCommands.isEmpty || !artifacts.settingsKeys.isEmpty { + let settingsPath: URL = if let root = scope.effectiveProjectRoot { + root.appendingPathComponent(Constants.FileNames.claudeDirectory) + .appendingPathComponent("settings.local.json") + } else { + env.claudeSettings + } + if !artifacts.hookCommands.isEmpty { + checks.append(( + check: HookSettingsCheck( + commands: artifacts.hookCommands, + settingsPath: settingsPath, + packName: pack.displayName + ), + isExcluded: false + )) + } + if !artifacts.settingsKeys.isEmpty { + checks.append(( + check: SettingsKeysCheck( + keys: artifacts.settingsKeys, + settingsPath: settingsPath, + packName: pack.displayName + ), + isExcluded: false + )) + } + } + + if !artifacts.gitignoreEntries.isEmpty { + checks.append(( + check: PackGitignoreCheck( + entries: artifacts.gitignoreEntries, + packName: pack.displayName + ), + isExcluded: false + )) + } + + return checks + } + // MARK: - Standalone checks (not tied to any component) /// Checks that cannot be derived from any ComponentDefinition. diff --git a/Tests/MCSTests/CoreDoctorCheckTests.swift b/Tests/MCSTests/CoreDoctorCheckTests.swift index 21703b3..2441e44 100644 --- a/Tests/MCSTests/CoreDoctorCheckTests.swift +++ b/Tests/MCSTests/CoreDoctorCheckTests.swift @@ -2,9 +2,252 @@ import Foundation @testable import mcs import Testing +// MARK: - HookSettingsCheck + +struct HookSettingsCheckTests { + private func makeTempSettings(content: String) throws -> URL { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent("mcs-test-\(UUID().uuidString).json") + try content.write(to: url, atomically: true, encoding: .utf8) + return url + } + + @Test("pass when all hook commands are present") + func passWhenAllPresent() throws { + let url = try makeTempSettings(content: """ + { + "hooks": { + "PostToolUse": [ + { "hooks": [{ "type": "command", "command": "bash .claude/hooks/lint.sh" }] } + ], + "PreToolUse": [ + { "hooks": [{ "type": "command", "command": "bash .claude/hooks/guard.sh" }] } + ] + } + } + """) + defer { try? FileManager.default.removeItem(at: url) } + + let check = HookSettingsCheck( + commands: ["bash .claude/hooks/lint.sh", "bash .claude/hooks/guard.sh"], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + if case .pass = result { + // expected + } else { + Issue.record("Expected .pass, got \(result)") + } + } + + @Test("fail when hook command is missing") + func failWhenMissing() throws { + let url = try makeTempSettings(content: """ + { + "hooks": { + "PostToolUse": [ + { "hooks": [{ "type": "command", "command": "bash .claude/hooks/lint.sh" }] } + ] + } + } + """) + defer { try? FileManager.default.removeItem(at: url) } + + let check = HookSettingsCheck( + commands: ["bash .claude/hooks/lint.sh", "bash .claude/hooks/missing.sh"], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + if case let .fail(msg) = result { + #expect(msg.contains("missing.sh")) + } else { + Issue.record("Expected .fail, got \(result)") + } + } + + @Test("fail when settings file does not exist") + func failWhenFileNotFound() { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent("nonexistent-\(UUID().uuidString).json") + let check = HookSettingsCheck( + commands: ["bash .claude/hooks/lint.sh"], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + // Settings.load returns empty Settings for missing file, so hooks will be nil + if case .fail = result { + // expected + } else { + Issue.record("Expected .fail, got \(result)") + } + } + + @Test("pass when hooks section is empty and no commands expected") + func passWhenEmpty() throws { + let url = try makeTempSettings(content: "{}") + defer { try? FileManager.default.removeItem(at: url) } + + let check = HookSettingsCheck( + commands: [], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + if case .pass = result { + // expected + } else { + Issue.record("Expected .pass, got \(result)") + } + } +} + +// MARK: - SettingsKeysCheck + +struct SettingsKeysCheckTests { + private func makeTempSettings(content: String) throws -> URL { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent("mcs-test-\(UUID().uuidString).json") + try content.write(to: url, atomically: true, encoding: .utf8) + return url + } + + @Test("pass when all keys are present") + func passWhenAllPresent() throws { + let url = try makeTempSettings(content: """ + { + "env": { "FOO": "bar" }, + "alwaysThinkingEnabled": true + } + """) + defer { try? FileManager.default.removeItem(at: url) } + + let check = SettingsKeysCheck( + keys: ["env.FOO", "alwaysThinkingEnabled"], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + if case .pass = result { + // expected + } else { + Issue.record("Expected .pass, got \(result)") + } + } + + @Test("fail when key is missing") + func failWhenMissing() throws { + let url = try makeTempSettings(content: """ + { + "env": { "FOO": "bar" } + } + """) + defer { try? FileManager.default.removeItem(at: url) } + + let check = SettingsKeysCheck( + keys: ["env.FOO", "missingKey"], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + if case let .fail(msg) = result { + #expect(msg.contains("missingKey")) + } else { + Issue.record("Expected .fail, got \(result)") + } + } + + @Test("fail when nested key is missing") + func failWhenNestedMissing() throws { + let url = try makeTempSettings(content: """ + { + "env": { "FOO": "bar" } + } + """) + defer { try? FileManager.default.removeItem(at: url) } + + let check = SettingsKeysCheck( + keys: ["env.MISSING"], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + if case let .fail(msg) = result { + #expect(msg.contains("env.MISSING")) + } else { + Issue.record("Expected .fail, got \(result)") + } + } + + @Test("fail when settings file is missing") + func failWhenFileNotFound() { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent("nonexistent-\(UUID().uuidString).json") + let check = SettingsKeysCheck( + keys: ["someKey"], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + if case .fail = result { + // expected + } else { + Issue.record("Expected .fail, got \(result)") + } + } +} + +// MARK: - PackGitignoreCheck + +struct PackGitignoreCheckTests { + @Test("pass when all entries are present") + func passWhenAllPresent() throws { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent("mcs-test-\(UUID().uuidString)") + try ".build\n.swiftpm\n".write(to: url, atomically: true, encoding: .utf8) + defer { try? FileManager.default.removeItem(at: url) } + + // PackGitignoreCheck uses GitignoreManager internally, so we test the struct's + // logic directly using a known gitignore path. Since we can't inject the path, + // we test the check struct's interface consistency instead. + let check = PackGitignoreCheck(entries: [".build", ".swiftpm"], packName: "test-pack") + // The actual result depends on the system's global gitignore — just verify the struct works + let result = check.check() + // Result will be either .pass or .fail depending on system state — no assertion on value + switch result { + case .pass, .fail: break + default: Issue.record("Expected .pass or .fail, got \(result)") + } + } + + @Test("name includes pack name") + func nameIncludesPackName() { + let check = PackGitignoreCheck(entries: [".build"], packName: "my-pack") + #expect(check.name == "Gitignore entries (my-pack)") + } + + @Test("section is Gitignore") + func sectionIsGitignore() { + let check = PackGitignoreCheck(entries: [".build"], packName: "my-pack") + #expect(check.section == "Gitignore") + } + + @Test("fix returns notFixable") + func fixReturnsNotFixable() { + let check = PackGitignoreCheck(entries: [".build"], packName: "my-pack") + let result = check.fix() + if case .notFixable = result { + // expected + } else { + Issue.record("Expected .notFixable, got \(result)") + } + } +} + // MARK: - CommandFileCheck -@Suite("CommandFileCheck") struct CommandFileCheckTests { private func makeTempFile(content: String) throws -> URL { let url = FileManager.default.temporaryDirectory From f889fa7e70141c931ec78f0259ddcb1af98c7ff8 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Fri, 6 Mar 2026 09:52:40 +0100 Subject: [PATCH 2/2] Address PR review findings - Replace try? with do/catch in readLines(), SettingsKeysCheck (CLAUDE.md compliance) - Differentiate "not found" vs "unreadable" errors in gitignore checks - Add file-existence check in HookSettingsCheck for clearer error messages - Add test for SettingsKeysCheck with invalid JSON content --- Sources/mcs/Core/GitignoreManager.swift | 9 +++--- Sources/mcs/Doctor/CoreDoctorChecks.swift | 36 ++++++++++++++++++----- Tests/MCSTests/CoreDoctorCheckTests.swift | 18 ++++++++++++ 3 files changed, 50 insertions(+), 13 deletions(-) diff --git a/Sources/mcs/Core/GitignoreManager.swift b/Sources/mcs/Core/GitignoreManager.swift index 75bd925..cfa4b01 100644 --- a/Sources/mcs/Core/GitignoreManager.swift +++ b/Sources/mcs/Core/GitignoreManager.swift @@ -82,14 +82,13 @@ struct GitignoreManager { } /// Read the global gitignore and return its lines as a set. - /// Returns `nil` if the file doesn't exist or can't be read. - func readLines() -> Set? { + /// Returns `nil` if the file doesn't exist. Throws if the file exists but can't be read. + func readLines() throws -> Set? { let path = resolveGlobalGitignorePath() - guard FileManager.default.fileExists(atPath: path.path), - let content = try? String(contentsOf: path, encoding: .utf8) - else { + guard FileManager.default.fileExists(atPath: path.path) else { return nil } + let content = try String(contentsOf: path, encoding: .utf8) return Set(content.components(separatedBy: .newlines)) } diff --git a/Sources/mcs/Doctor/CoreDoctorChecks.swift b/Sources/mcs/Doctor/CoreDoctorChecks.swift index 5fe9cba..c5508ee 100644 --- a/Sources/mcs/Doctor/CoreDoctorChecks.swift +++ b/Sources/mcs/Doctor/CoreDoctorChecks.swift @@ -236,8 +236,14 @@ struct GitignoreCheck: DoctorCheck { func check() -> CheckResult { let gitignoreManager = GitignoreManager(shell: ShellRunner(environment: Environment())) - guard let lines = gitignoreManager.readLines() else { - return .fail("global gitignore not found") + let lines: Set + do { + guard let result = try gitignoreManager.readLines() else { + return .fail("global gitignore not found") + } + lines = result + } catch { + return .fail("global gitignore unreadable: \(error.localizedDescription)") } let missing = GitignoreManager.coreEntries.filter { !lines.contains($0) } if missing.isEmpty { @@ -247,8 +253,7 @@ struct GitignoreCheck: DoctorCheck { } func fix() -> FixResult { - let shell = ShellRunner(environment: Environment()) - let gitignoreManager = GitignoreManager(shell: shell) + let gitignoreManager = GitignoreManager(shell: ShellRunner(environment: Environment())) do { try gitignoreManager.addCoreEntries() return .fixed("added missing entries") @@ -333,6 +338,9 @@ struct HookSettingsCheck: DoctorCheck { } func check() -> CheckResult { + guard FileManager.default.fileExists(atPath: settingsPath.path) else { + return .fail("settings file not found") + } let settings: Settings do { settings = try Settings.load(from: settingsPath) @@ -378,8 +386,14 @@ struct SettingsKeysCheck: DoctorCheck { } catch { return .fail("settings file not found or unreadable") } - guard let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any] else { - return .fail("settings file contains invalid JSON") + let json: [String: Any] + do { + guard let parsed = try JSONSerialization.jsonObject(with: data) as? [String: Any] else { + return .fail("settings file is not a JSON object") + } + json = parsed + } catch { + return .fail("settings file contains invalid JSON: \(error.localizedDescription)") } var missing: [String] = [] for keyPath in keys where !keyExists(keyPath, in: json) { @@ -423,8 +437,14 @@ struct PackGitignoreCheck: DoctorCheck { func check() -> CheckResult { let gitignoreManager = GitignoreManager(shell: ShellRunner(environment: Environment())) - guard let lines = gitignoreManager.readLines() else { - return .fail("global gitignore not found") + let lines: Set + do { + guard let result = try gitignoreManager.readLines() else { + return .fail("global gitignore not found") + } + lines = result + } catch { + return .fail("global gitignore unreadable: \(error.localizedDescription)") } let missing = entries.filter { !lines.contains($0) } if missing.isEmpty { diff --git a/Tests/MCSTests/CoreDoctorCheckTests.swift b/Tests/MCSTests/CoreDoctorCheckTests.swift index 2441e44..4b01e57 100644 --- a/Tests/MCSTests/CoreDoctorCheckTests.swift +++ b/Tests/MCSTests/CoreDoctorCheckTests.swift @@ -197,6 +197,24 @@ struct SettingsKeysCheckTests { Issue.record("Expected .fail, got \(result)") } } + + @Test("fail when settings file contains invalid JSON") + func failWhenInvalidJSON() throws { + let url = try makeTempSettings(content: "not valid json {{{") + defer { try? FileManager.default.removeItem(at: url) } + + let check = SettingsKeysCheck( + keys: ["someKey"], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + if case let .fail(msg) = result { + #expect(msg.contains("invalid JSON")) + } else { + Issue.record("Expected .fail, got \(result)") + } + } } // MARK: - PackGitignoreCheck