From cb48790a13ca57611f5ac349acfeb21e1b6969c8 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Tue, 28 Jul 2026 23:40:15 +0200 Subject: [PATCH 1/2] Resolve declarative settings doctor checks from the project scope - hookEventExists and settingsKeyEquals now read project settings.local.json before global settings.json, so project-scoped packs are verified against the file their artifacts were actually written to - Report settings files that exist but cannot be parsed instead of discarding the error and falling through silently - Warn from mcs pack validate when a check declares scope on a type that ignores it --- CLAUDE.md | 2 +- .../ExternalPack/ExternalDoctorCheck.swift | 160 +++++++-- .../ExternalPack/ExternalPackManifest.swift | 22 +- Sources/mcs/ExternalPack/PackHeuristics.swift | 31 ++ .../CoreDoctorCheckSandboxTests.swift | 331 ++++++++++++++++++ .../DoctorRunnerIntegrationTests.swift | 93 +++++ Tests/MCSTests/ExternalDoctorCheckTests.swift | 103 ++++++ Tests/MCSTests/PackHeuristicsTests.swift | 96 ++++- docs/architecture.md | 2 + docs/techpack-schema.md | 31 +- docs/troubleshooting.md | 5 +- 11 files changed, 840 insertions(+), 36 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 13b17e3b..ba9c7d59 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -108,7 +108,7 @@ mcs config set # Set a configuration value (true/false) - `PromptExecutor.swift` — executes pack prompts (interactive value resolution during sync) - `ScriptRunner.swift` — sandboxed script execution for pack scripts - `ExternalDoctorCheck.swift` — factory for converting YAML doctor check definitions to `DoctorCheck` instances -- `PackHeuristics.swift` — heuristic validation checks for `mcs pack validate` (empty pack, root source copy, missing files, unreferenced files, MCP dependency gaps, python module paths) +- `PackHeuristics.swift` — heuristic validation checks for `mcs pack validate` (empty pack, root source copy, missing files, unreferenced files, MCP dependency gaps, python module paths, `scope` declared on doctor check types that ignore it) ### Doctor (`Sources/mcs/Doctor/`) - `DoctorRunner.swift` — 5-layer check orchestration with project-aware pack resolution diff --git a/Sources/mcs/ExternalPack/ExternalDoctorCheck.swift b/Sources/mcs/ExternalPack/ExternalDoctorCheck.swift index e053f68d..e47990de 100644 --- a/Sources/mcs/ExternalPack/ExternalDoctorCheck.swift +++ b/Sources/mcs/ExternalPack/ExternalDoctorCheck.swift @@ -287,32 +287,38 @@ struct ExternalShellScriptCheck: DoctorCheck { // MARK: - Hook Event Exists Check -/// Checks that a hook event is registered in settings.json. +/// Checks that a hook event is registered in the Claude settings. /// Pack-contributed replacement for the engine-level HookEventCheck. -struct ExternalHookEventExistsCheck: DoctorCheck { +/// +/// Resolves project `settings.local.json` before global `settings.json` — see +/// `SettingsReadingCheck`. +struct ExternalHookEventExistsCheck: SettingsReadingCheck { let name: String let section: String let event: String let isOptional: Bool + var projectRoot: URL? var environment: Environment = .init() func check() -> CheckResult { - let settingsURL = environment.claudeSettings - guard FileManager.default.fileExists(atPath: settingsURL.path) else { - return .fail("settings.json not found") + let probe: SettingsProbe = probeSettings { url in + try Settings.load(from: url).hooks?[event] != nil ? true : nil } - let settings: Settings - do { - settings = try Settings.load(from: settingsURL) - } catch { - return .fail("settings.json is invalid: \(error.localizedDescription)") + + if let match = probe.match { + return probe.readErrors.isEmpty + ? .pass("registered in \(match.fileName)") + : .warn("registered in \(match.fileName)\(probe.errorSuffix)") + } + guard probe.anyFileExisted else { + return .fail("no settings file found (searched \(searchedFileNames))") } - guard let hooks = settings.hooks, hooks[event] != nil else { - return isOptional - ? .skip("\(event) not registered (optional)") - : .fail("\(event) not registered in settings.json") + if !probe.readErrors.isEmpty { + return .fail("\(event) not registered\(probe.errorSuffix)") } - return .pass("registered in settings.json") + return isOptional + ? .skip("\(event) not registered (optional)") + : .fail("\(event) not registered in \(searchedFileNames)") } func fix() -> FixResult { @@ -322,34 +328,42 @@ struct ExternalHookEventExistsCheck: DoctorCheck { // MARK: - Settings Key Equals Check -/// Checks that a specific key in settings.json has an expected value. +/// Checks that a specific settings key has an expected value. /// Uses dot-notation keyPath to navigate the raw JSON, ensuring forward compatibility /// with any key — not just those modeled by the Settings struct. -struct ExternalSettingsKeyEqualsCheck: DoctorCheck { +/// +/// Resolves project `settings.local.json` before global `settings.json` — see +/// `SettingsReadingCheck`. This matches Claude Code's own precedence, so the check reports +/// the value actually in effect. +struct ExternalSettingsKeyEqualsCheck: SettingsReadingCheck { let name: String let section: String let keyPath: String let expectedValue: String + var projectRoot: URL? var environment: Environment = .init() func check() -> CheckResult { - let settingsURL = environment.claudeSettings - guard FileManager.default.fileExists(atPath: settingsURL.path) else { - return .fail("settings.json not found") - } - guard let data = try? Data(contentsOf: settingsURL), - let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any] - else { - return .fail("settings.json is invalid") + let probe: SettingsProbe = probeSettings { url in + let data = try Data(contentsOf: url) + guard let json = try JSONSerialization.jsonObject(with: data) as? [String: Any] else { + throw MCSError.invalidConfiguration("not a JSON object") + } + return resolveKeyPath(keyPath, in: json) } - guard let actual = resolveKeyPath(keyPath, in: json) else { - return .warn("\(keyPath) not set") + guard probe.anyFileExisted else { + return .fail("no settings file found (searched \(searchedFileNames))") + } + guard let match = probe.match else { + return .warn("\(keyPath) not set\(probe.errorSuffix)") } - if actual == expectedValue { - return .pass("\(keyPath) = \(expectedValue)") + if match.value == expectedValue { + return .pass("\(keyPath) = \(expectedValue) (\(match.fileName))\(probe.errorSuffix)") } - return .warn("\(keyPath) is '\(actual)', expected '\(expectedValue)'") + return .warn( + "\(keyPath) is '\(match.value)' in \(match.fileName), expected '\(expectedValue)'\(probe.errorSuffix)" + ) } func fix() -> FixResult { @@ -501,11 +515,15 @@ enum ExternalDoctorCheckFactory { reason: "hookEventExists requires non-empty 'event'" ) } + // `scope` is deliberately not forwarded: it selects a base directory for an + // author-supplied `path`, and this check has none. The settings file is implied by + // the check type, so `projectRoot` alone drives resolution. return ExternalHookEventExistsCheck( name: definition.name, section: section, event: event, isOptional: definition.isOptional ?? false, + projectRoot: projectRoot, environment: environment ) @@ -518,11 +536,13 @@ enum ExternalDoctorCheckFactory { reason: "settingsKeyEquals requires non-empty 'keyPath' and 'expectedValue'" ) } + // See the note on `.hookEventExists` above — `scope` does not apply here either. return ExternalSettingsKeyEqualsCheck( name: definition.name, section: section, keyPath: keyPath, expectedValue: expectedValue, + projectRoot: projectRoot, environment: environment ) } @@ -564,6 +584,86 @@ extension ScopedPathCheck { } } +// MARK: - Settings Reading Protocol + +/// Shared settings-file resolution for doctor checks that read Claude settings. +/// +/// Unlike `ScopedPathCheck`, these checks have no author-supplied path — the file is implied +/// by the check type — so resolution is driven entirely by `projectRoot`, never by the +/// manifest's `scope` field (hence the name: these checks read settings, they are not scoped). +/// Candidates are tried most-specific-first: project `settings.local.json`, then global +/// `settings.json`. That order matches Claude Code's own precedence, so a check reports on the +/// settings actually in effect. +protocol SettingsReadingCheck: DoctorCheck { + var projectRoot: URL? { get } + var environment: Environment { get } +} + +/// Outcome of walking the candidate settings files. +struct SettingsProbe { + /// First candidate that produced a value, with the name of the file that answered. + let match: (value: Value, fileName: String)? + /// Files that exist on disk but could not be read or parsed. + let readErrors: [String] + /// Whether any candidate file was present on disk. + let anyFileExisted: Bool + + /// Read failures rendered for appending to a check message, empty when there were none. + /// Single definition so every result string reports unreadable files the same way. + var errorSuffix: String { + readErrors.isEmpty ? "" : " — \(readErrors.joined(separator: "; "))" + } +} + +extension SettingsReadingCheck { + /// Candidate settings files, most specific first. + var settingsCandidates: [URL] { + var urls: [URL] = [] + if let projectRoot { + urls.append( + projectRoot + .appendingPathComponent(Constants.FileNames.claudeDirectory) + .appendingPathComponent(Constants.FileNames.settingsLocal) + ) + } + urls.append(environment.claudeSettings) + return urls + } + + /// Human-readable list of the files this check consults, for diagnostics. + var searchedFileNames: String { + settingsCandidates.map(\.lastPathComponent).joined(separator: ", ") + } + + /// Walk the candidates in order and return the first non-nil `read` result. + /// + /// Absent files are skipped rather than treated as errors — `Settings.load(from:)` returns an + /// empty `Settings` for a missing file, so existence must be checked here to distinguish + /// "no such file" from "file present but key absent". Read and parse failures are collected + /// rather than discarded, so a corrupt file is always surfaced to the user. + func probeSettings(_ read: (URL) throws -> Value?) -> SettingsProbe { + var readErrors: [String] = [] + var anyFileExisted = false + + for url in settingsCandidates { + guard FileManager.default.fileExists(atPath: url.path) else { continue } + anyFileExisted = true + do { + if let value = try read(url) { + return SettingsProbe( + match: (value: value, fileName: url.lastPathComponent), + readErrors: readErrors, + anyFileExisted: true + ) + } + } catch { + readErrors.append("\(url.lastPathComponent) is unreadable: \(error.localizedDescription)") + } + } + return SettingsProbe(match: nil, readErrors: readErrors, anyFileExisted: anyFileExisted) + } +} + // MARK: - Helpers /// Expand `~` at the start of a path to the user's home directory. diff --git a/Sources/mcs/ExternalPack/ExternalPackManifest.swift b/Sources/mcs/ExternalPack/ExternalPackManifest.swift index 09bae7c0..20d09093 100644 --- a/Sources/mcs/ExternalPack/ExternalPackManifest.swift +++ b/Sources/mcs/ExternalPack/ExternalPackManifest.swift @@ -919,7 +919,7 @@ struct ExternalDoctorCheckDefinition: Codable { let isOptional: Bool? } -enum ExternalDoctorCheckType: String, Codable { +enum ExternalDoctorCheckType: String, Codable, CaseIterable { case commandExists case fileExists case directoryExists @@ -928,6 +928,26 @@ enum ExternalDoctorCheckType: String, Codable { case shellScript case hookEventExists case settingsKeyEquals + + /// Whether `scope` affects this check. + /// + /// `scope` selects the base directory for an author-supplied `path`, so it only means + /// something for the path-based checks. The others either take no path (`hookEventExists`, + /// `settingsKeyEquals` — the settings file is implied and resolved from the project root) or + /// run a command (`commandExists`, `shellScript`). `mcs pack validate` warns when a pack + /// declares `scope` on a type that ignores it. + /// + /// `ExternalDoctorCheckFactory.makeCheck` is the ground truth — only the checks it builds as + /// `ScopedPathCheck` consume `scope`. `ExternalDoctorCheckTests.honorsScopeMatchesFactory` + /// asserts this property agrees with the factory for every case, so the two cannot drift. + var honorsScope: Bool { + switch self { + case .fileExists, .directoryExists, .fileContains, .fileNotContains: + true + case .commandExists, .shellScript, .hookEventExists, .settingsKeyEquals: + false + } + } } enum ExternalDoctorCheckScope: String, Codable { diff --git a/Sources/mcs/ExternalPack/PackHeuristics.swift b/Sources/mcs/ExternalPack/PackHeuristics.swift index fe2b913b..4ac55b66 100644 --- a/Sources/mcs/ExternalPack/PackHeuristics.swift +++ b/Sources/mcs/ExternalPack/PackHeuristics.swift @@ -24,6 +24,7 @@ enum PackHeuristics { findings += unreferenced findings += checkMCPDependencyGaps(components: components) findings += checkPythonModulePaths(components: components, packPath: packPath) + findings += checkDoctorCheckScopeUsage(manifest: manifest, components: components) // Surface the `ignore:` hint only when an actual unreferenced-file warning was emitted // (not for the IO-failure warnings that share the same severity). @@ -331,4 +332,34 @@ enum PackHeuristics { return findings } + + /// Warns when a doctor check declares `scope` on a type that ignores it. + /// + /// A silently-ignored field reads as configuration but has no effect, which is how + /// `hookEventExists` and `settingsKeyEquals` came to look scope-aware while always reading + /// the global settings file. + private static func checkDoctorCheckScopeUsage( + manifest: ExternalPackManifest, + components: [ExternalComponentDefinition] + ) -> [Finding] { + let allChecks = (manifest.supplementaryDoctorChecks ?? []) + + components.flatMap { $0.doctorChecks ?? [] } + + return allChecks + .filter { $0.scope != nil && !$0.type.honorsScope } + .map { check in + let detail = switch check.type { + case .hookEventExists, .settingsKeyEquals: + "settings are resolved from the project root automatically" + + " (project settings.local.json, then global settings.json)" + default: + "`scope` only applies to checks with a `path`" + } + return Finding( + severity: .warning, + message: "Doctor check '\(check.name)' declares `scope` but type" + + " `\(check.type.rawValue)` ignores it — \(detail)" + ) + } + } } diff --git a/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift b/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift index 393e108a..4ec863cd 100644 --- a/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift +++ b/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift @@ -1128,3 +1128,334 @@ struct ExternalSettingsKeyEqualsCheckSandboxTests { } } } + +// MARK: - Scoped Settings Resolution Helpers + +/// Creates `/my-project/.claude/` and writes `settings.local.json` into it. +private func makeProjectSettings(in home: URL, contents: String) throws -> URL { + let projectRoot = home.appendingPathComponent("my-project") + let claudeDir = projectRoot.appendingPathComponent(".claude") + try FileManager.default.createDirectory(at: claudeDir, withIntermediateDirectories: true) + try contents.write( + to: claudeDir.appendingPathComponent("settings.local.json"), + atomically: true, encoding: .utf8 + ) + return projectRoot +} + +private func hookSettings(event: String) -> String { + """ + { + "hooks": { + "\(event)": [ + { "hooks": [{ "type": "command", "command": "bash .claude/hooks/run.sh" }] } + ] + } + } + """ +} + +// MARK: - ExternalHookEventExistsCheck Project Scope (issue #354) + +extension ExternalHookEventExistsCheckSandboxTests { + @Test("pass from project settings.local.json when global settings.json lacks the event") + func passFromProjectSettings() throws { + let home = try makeGlobalTmpDir(label: "hook-event-project") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings(in: home, contents: hookSettings(event: "PostToolUse")) + try hookSettings(event: "PreToolUse").write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + var check = ExternalHookEventExistsCheck( + name: "PostToolUse hook", section: "Hooks", + event: "PostToolUse", isOptional: false, projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case let .pass(msg) = result else { + Issue.record("Expected .pass, got \(result)") + return + } + #expect(msg == "registered in settings.local.json") + } + + @Test("pass via global fallback when project settings.local.json lacks the event") + func passViaGlobalFallback() throws { + let home = try makeGlobalTmpDir(label: "hook-event-fallback") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings(in: home, contents: hookSettings(event: "PreToolUse")) + try hookSettings(event: "PostToolUse").write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + var check = ExternalHookEventExistsCheck( + name: "PostToolUse hook", section: "Hooks", + event: "PostToolUse", isOptional: false, projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case let .pass(msg) = result else { + Issue.record("Expected .pass, got \(result)") + return + } + #expect(msg == "registered in settings.json") + } + + @Test("pass from project settings when no global settings.json exists") + func passFromProjectWithoutGlobalFile() throws { + let home = try makeGlobalTmpDir(label: "hook-event-noglobal") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings(in: home, contents: hookSettings(event: "SessionStart")) + + var check = ExternalHookEventExistsCheck( + name: "SessionStart hook", section: "Hooks", + event: "SessionStart", isOptional: false, projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case let .pass(msg) = result else { + Issue.record("Expected .pass, got \(result)") + return + } + #expect(msg == "registered in settings.local.json") + } + + @Test("fail when event is absent from both scopes") + func failWhenAbsentFromBothScopes() throws { + let home = try makeGlobalTmpDir(label: "hook-event-neither") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings(in: home, contents: hookSettings(event: "PreToolUse")) + try "{}".write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + var check = ExternalHookEventExistsCheck( + name: "PostToolUse hook", section: "Hooks", + event: "PostToolUse", isOptional: false, projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case let .fail(msg) = result else { + Issue.record("Expected .fail, got \(result)") + return + } + #expect(msg.contains("settings.local.json")) + #expect(msg.contains("settings.json")) + } + + @Test("ignores project settings when no projectRoot is given") + func ignoresProjectSettingsWithoutProjectRoot() throws { + let home = try makeGlobalTmpDir(label: "hook-event-noroot") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + _ = try makeProjectSettings(in: home, contents: hookSettings(event: "PostToolUse")) + try "{}".write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + // A globally-configured pack has no project root — the project file must not be consulted. + var check = ExternalHookEventExistsCheck( + name: "PostToolUse hook", section: "Hooks", + event: "PostToolUse", isOptional: false + ) + check.environment = env + let result = check.check() + guard case .fail = result else { + Issue.record("Expected .fail, got \(result)") + return + } + } + + @Test("warn naming the unreadable project file when the event is found globally") + func warnWhenProjectSettingsCorruptButFoundGlobally() throws { + let home = try makeGlobalTmpDir(label: "hook-event-corrupt-fallback") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings(in: home, contents: "{ not json") + try hookSettings(event: "PostToolUse").write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + var check = ExternalHookEventExistsCheck( + name: "PostToolUse hook", section: "Hooks", + event: "PostToolUse", isOptional: false, projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case let .warn(msg) = result else { + Issue.record("Expected .warn, got \(result)") + return + } + #expect(msg.contains("registered in settings.json")) + #expect(msg.contains("settings.local.json is unreadable")) + } + + @Test("fail surfaces the corrupt project file when the event is absent everywhere") + func failSurfacesCorruptProjectSettings() throws { + let home = try makeGlobalTmpDir(label: "hook-event-corrupt-fail") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings(in: home, contents: "{ not json") + try "{}".write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + var check = ExternalHookEventExistsCheck( + name: "PostToolUse hook", section: "Hooks", + event: "PostToolUse", isOptional: false, projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case let .fail(msg) = result else { + Issue.record("Expected .fail, got \(result)") + return + } + #expect(msg.contains("settings.local.json is unreadable")) + } + + @Test("skip when an optional event is absent from both scopes") + func skipWhenOptionalAbsentFromBothScopes() throws { + let home = try makeGlobalTmpDir(label: "hook-event-optional-project") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings(in: home, contents: "{}") + try "{}".write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + var check = ExternalHookEventExistsCheck( + name: "SessionStart hook", section: "Hooks", + event: "SessionStart", isOptional: true, projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case .skip = result else { + Issue.record("Expected .skip, got \(result)") + return + } + } + + @Test("fail names both candidates when neither settings file exists") + func failWhenNoSettingsFileAnywhere() throws { + let home = try makeGlobalTmpDir(label: "hook-event-nofiles") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = home.appendingPathComponent("my-project") + try FileManager.default.createDirectory(at: projectRoot, withIntermediateDirectories: true) + + var check = ExternalHookEventExistsCheck( + name: "PostToolUse hook", section: "Hooks", + event: "PostToolUse", isOptional: false, projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case let .fail(msg) = result else { + Issue.record("Expected .fail, got \(result)") + return + } + #expect(msg.contains("no settings file found")) + #expect(msg.contains("settings.local.json, settings.json")) + } +} + +// MARK: - ExternalSettingsKeyEqualsCheck Project Scope (issue #354) + +extension ExternalSettingsKeyEqualsCheckSandboxTests { + @Test("project value takes precedence over a differing global value") + func projectValueOverridesGlobal() throws { + let home = try makeGlobalTmpDir(label: "settings-key-override") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings( + in: home, contents: #"{ "permissions": { "defaultMode": "deny" } }"# + ) + try #"{ "permissions": { "defaultMode": "allowEdits" } }"# + .write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + var check = ExternalSettingsKeyEqualsCheck( + name: "Default mode", section: "Settings", + keyPath: "permissions.defaultMode", expectedValue: "allowEdits", + projectRoot: projectRoot + ) + check.environment = env + // Claude Code applies the project value, so the check must report on that one. + let result = check.check() + guard case let .warn(msg) = result else { + Issue.record("Expected .warn, got \(result)") + return + } + #expect(msg.contains("'deny' in settings.local.json")) + } + + @Test("pass from project settings.local.json") + func passFromProjectSettings() throws { + let home = try makeGlobalTmpDir(label: "settings-key-project") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings( + in: home, contents: #"{ "permissions": { "defaultMode": "allowEdits" } }"# + ) + + var check = ExternalSettingsKeyEqualsCheck( + name: "Default mode", section: "Settings", + keyPath: "permissions.defaultMode", expectedValue: "allowEdits", + projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case let .pass(msg) = result else { + Issue.record("Expected .pass, got \(result)") + return + } + #expect(msg.contains("settings.local.json")) + } + + @Test("pass via global fallback when the key is absent from project settings") + func passViaGlobalFallback() throws { + let home = try makeGlobalTmpDir(label: "settings-key-fallback") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings(in: home, contents: #"{ "env": { "FOO": "bar" } }"#) + try #"{ "permissions": { "defaultMode": "allowEdits" } }"# + .write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + var check = ExternalSettingsKeyEqualsCheck( + name: "Default mode", section: "Settings", + keyPath: "permissions.defaultMode", expectedValue: "allowEdits", + projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case let .pass(msg) = result else { + Issue.record("Expected .pass, got \(result)") + return + } + #expect(msg.contains("(settings.json)")) + } + + @Test("warn surfaces a corrupt project settings file instead of discarding it") + func warnSurfacesCorruptProjectSettings() throws { + let home = try makeGlobalTmpDir(label: "settings-key-corrupt") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectRoot = try makeProjectSettings(in: home, contents: "{ not json") + try "{}".write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + var check = ExternalSettingsKeyEqualsCheck( + name: "Default mode", section: "Settings", + keyPath: "permissions.defaultMode", expectedValue: "allowEdits", + projectRoot: projectRoot + ) + check.environment = env + let result = check.check() + guard case let .warn(msg) = result else { + Issue.record("Expected .warn, got \(result)") + return + } + #expect(msg.contains("settings.local.json is unreadable")) + } +} diff --git a/Tests/MCSTests/DoctorRunnerIntegrationTests.swift b/Tests/MCSTests/DoctorRunnerIntegrationTests.swift index 7dddbed1..0ae38269 100644 --- a/Tests/MCSTests/DoctorRunnerIntegrationTests.swift +++ b/Tests/MCSTests/DoctorRunnerIntegrationTests.swift @@ -211,6 +211,99 @@ struct DoctorRunnerIntegrationTests { try runner.run() } + // MARK: - External settings checks resolve project scope (issue #354) + + /// Builds an external pack whose only check is a declarative `hookEventExists`. + /// + /// The adapter must be constructed with a sandboxed shell: `convertDoctorCheck` passes + /// `shell.environment` to the factory, not the runner's environment, so a default shell would + /// make the check read the real `~/.claude/settings.json`. + private func externalHookCheckPack(home: URL, packPath: URL) -> ExternalPackAdapter { + let manifest = ExternalPackManifest( + schemaVersion: 1, + identifier: "external-pack", + displayName: "External Pack", + description: "Pack with a declarative hook event check", + author: nil, + minMCSVersion: nil, + components: nil, + templates: nil, + prompts: nil, + configureProject: nil, + supplementaryDoctorChecks: [ + ExternalDoctorCheckDefinition( + type: .hookEventExists, + name: "SessionStart hook", + section: "Hooks", + command: nil, args: nil, path: nil, pattern: nil, + scope: nil, fixCommand: nil, fixScript: nil, + event: "SessionStart", + keyPath: nil, expectedValue: nil, isOptional: false + ), + ], + ignore: nil + ) + return ExternalPackAdapter( + manifest: manifest, + packPath: packPath, + shell: ShellRunner(environment: Environment(home: home)), + output: CLIOutput(colorsEnabled: false) + ) + } + + @Test("declarative hookEventExists resolves the project settings.local.json") + func externalHookCheckFindsProjectScopedEvent() throws { + let (home, project) = try makeSandboxProject(label: "runner-ext-hook-project") + defer { try? FileManager.default.removeItem(at: home) } + + let registry = TechPackRegistry(packs: [externalHookCheckPack(home: home, packPath: home)]) + var state = try ProjectState(projectRoot: project) + state.recordPack("external-pack") + try state.save() + + // Project-scoped sync writes hook entries here — and nowhere else. + let projectSettings = """ + { + "hooks": { + "SessionStart": [ + { "hooks": [{ "type": "command", "command": "mcs check-updates --hook" }] } + ] + } + } + """ + try projectSettings.write( + to: project.appendingPathComponent(Constants.FileNames.claudeDirectory) + .appendingPathComponent(Constants.FileNames.settingsLocal), + atomically: true, encoding: .utf8 + ) + // No global settings.json — before this fix the check read only that file and failed. + + var runner = makeRunner(home: home, projectRoot: project, registry: registry) + let summary = try runner.run() + #expect(summary.issues == 0) + } + + @Test("declarative hookEventExists still fails when the event is registered nowhere") + func externalHookCheckFailsWhenEventMissing() throws { + let (home, project) = try makeSandboxProject(label: "runner-ext-hook-missing") + defer { try? FileManager.default.removeItem(at: home) } + + let registry = TechPackRegistry(packs: [externalHookCheckPack(home: home, packPath: home)]) + var state = try ProjectState(projectRoot: project) + state.recordPack("external-pack") + try state.save() + + try "{}".write( + to: project.appendingPathComponent(Constants.FileNames.claudeDirectory) + .appendingPathComponent(Constants.FileNames.settingsLocal), + atomically: true, encoding: .utf8 + ) + + var runner = makeRunner(home: home, projectRoot: project, registry: registry) + let summary = try runner.run() + #expect(summary.issues > 0) + } + @Test("runner resolves colliding hook destinations via collision resolver") func collidingHookDestinationsResolvedByDoctor() throws { let (home, project) = try makeSandboxProject(label: "runner-collision") diff --git a/Tests/MCSTests/ExternalDoctorCheckTests.swift b/Tests/MCSTests/ExternalDoctorCheckTests.swift index 7f88c2bf..6d269711 100644 --- a/Tests/MCSTests/ExternalDoctorCheckTests.swift +++ b/Tests/MCSTests/ExternalDoctorCheckTests.swift @@ -572,6 +572,109 @@ struct ExternalDoctorCheckTests { #expect(check.section == "Settings") } + // MARK: - Settings check scope resolution (issue #354) + + /// Builds a settings-reading check definition, optionally declaring a `scope`. + private func settingsCheckDefinition( + type: ExternalDoctorCheckType, + scope: ExternalDoctorCheckScope? + ) -> ExternalDoctorCheckDefinition { + ExternalDoctorCheckDefinition( + type: type, + name: "Settings check", + section: "Settings", + command: nil, + args: nil, + path: nil, + pattern: nil, + scope: scope, + fixCommand: nil, + fixScript: nil, + event: type == .hookEventExists ? "SessionStart" : nil, + keyPath: type == .settingsKeyEquals ? "permissions.defaultMode" : nil, + expectedValue: type == .settingsKeyEquals ? "plan" : nil, + isOptional: false + ) + } + + @Test("Factory threads projectRoot into settings-reading checks") + func factoryThreadsProjectRootIntoSettingsChecks() throws { + let tmpDir = try makeTmpDir() + defer { try? FileManager.default.removeItem(at: tmpDir) } + let projectRoot = tmpDir.appendingPathComponent("project") + + let hookCheck = ExternalDoctorCheckFactory.makeCheck( + from: settingsCheckDefinition(type: .hookEventExists, scope: nil), + packPath: tmpDir, projectRoot: projectRoot, scriptRunner: makeScriptRunner() + ) + let keyCheck = ExternalDoctorCheckFactory.makeCheck( + from: settingsCheckDefinition(type: .settingsKeyEquals, scope: nil), + packPath: tmpDir, projectRoot: projectRoot, scriptRunner: makeScriptRunner() + ) + + #expect((hookCheck as? ExternalHookEventExistsCheck)?.projectRoot == projectRoot) + #expect((keyCheck as? ExternalSettingsKeyEqualsCheck)?.projectRoot == projectRoot) + } + + @Test("honorsScope agrees with the factory for every check type") + func honorsScopeMatchesFactory() throws { + let tmpDir = try makeTmpDir() + defer { try? FileManager.default.removeItem(at: tmpDir) } + + // The factory decides which checks consume `scope` by building them as `ScopedPathCheck`. + // `honorsScope` mirrors that decision for `mcs pack validate`; this keeps the two in step + // when a ninth check type is added. + for type in ExternalDoctorCheckType.allCases { + let definition = ExternalDoctorCheckDefinition( + type: type, + name: "Check", + section: nil, + command: "true", + args: nil, + path: "some/path", + pattern: "pattern", + scope: .project, + fixCommand: nil, + fixScript: nil, + event: "SessionStart", + keyPath: "permissions.defaultMode", + expectedValue: "plan", + isOptional: nil + ) + let check = ExternalDoctorCheckFactory.makeCheck( + from: definition, packPath: tmpDir, projectRoot: tmpDir, + scriptRunner: makeScriptRunner() + ) + #expect( + (check is any ScopedPathCheck) == type.honorsScope, + "\(type.rawValue): honorsScope is \(type.honorsScope) but factory built \(Swift.type(of: check))" + ) + } + } + + @Test("Declared scope does not alter settings-check resolution") + func factoryIgnoresScopeForSettingsChecks() throws { + let tmpDir = try makeTmpDir() + defer { try? FileManager.default.removeItem(at: tmpDir) } + let projectRoot = tmpDir.appendingPathComponent("project") + + // `scope` selects a base directory for an author-supplied `path`; these checks have none, + // so it must not change which settings files are consulted. + for scope in [ExternalDoctorCheckScope.global, .project] { + let hookCheck = ExternalDoctorCheckFactory.makeCheck( + from: settingsCheckDefinition(type: .hookEventExists, scope: scope), + packPath: tmpDir, projectRoot: projectRoot, scriptRunner: makeScriptRunner() + ) + let keyCheck = ExternalDoctorCheckFactory.makeCheck( + from: settingsCheckDefinition(type: .settingsKeyEquals, scope: scope), + packPath: tmpDir, projectRoot: projectRoot, scriptRunner: makeScriptRunner() + ) + + #expect((hookCheck as? ExternalHookEventExistsCheck)?.projectRoot == projectRoot) + #expect((keyCheck as? ExternalSettingsKeyEqualsCheck)?.projectRoot == projectRoot) + } + } + @Test("Factory returns misconfigured for hookEventExists without event") func factoryHookEventExistsMisconfigured() throws { let tmpDir = try makeTmpDir() diff --git a/Tests/MCSTests/PackHeuristicsTests.swift b/Tests/MCSTests/PackHeuristicsTests.swift index d88c34d5..60e27522 100644 --- a/Tests/MCSTests/PackHeuristicsTests.swift +++ b/Tests/MCSTests/PackHeuristicsTests.swift @@ -6,6 +6,7 @@ struct PackHeuristicsTests { private func minimalManifest( identifier: String = "test-pack", components: [ExternalComponentDefinition]? = nil, + supplementaryDoctorChecks: [ExternalDoctorCheckDefinition]? = nil, ignore: [String]? = nil ) -> ExternalPackManifest { ExternalPackManifest( @@ -19,7 +20,7 @@ struct PackHeuristicsTests { templates: nil, prompts: nil, configureProject: nil, - supplementaryDoctorChecks: nil, + supplementaryDoctorChecks: supplementaryDoctorChecks, ignore: ignore ) } @@ -909,4 +910,97 @@ struct PackHeuristicsTests { let findings = PackHeuristics.check(manifest: manifest, packPath: tmpDir) #expect(!findings.contains { $0.message.contains("screenshot.png") && $0.message.contains(PackHeuristics.unreferencedMarker) }) } + + // MARK: - Doctor Check Scope Usage (issue #354) + + private func doctorCheck( + type: ExternalDoctorCheckType, + name: String, + scope: ExternalDoctorCheckScope? + ) -> ExternalDoctorCheckDefinition { + ExternalDoctorCheckDefinition( + type: type, + name: name, + section: nil, + command: nil, + args: nil, + path: type == .fileExists ? ".claude/hooks/run.sh" : nil, + pattern: nil, + scope: scope, + fixCommand: nil, + fixScript: nil, + event: type == .hookEventExists ? "SessionStart" : nil, + keyPath: type == .settingsKeyEquals ? "permissions.defaultMode" : nil, + expectedValue: type == .settingsKeyEquals ? "plan" : nil, + isOptional: nil + ) + } + + /// A pack with one inert component, so the empty-pack heuristic stays quiet. + private func manifestWithDoctorChecks( + supplementary: [ExternalDoctorCheckDefinition]? = nil, + componentChecks: [ExternalDoctorCheckDefinition]? = nil + ) -> ExternalPackManifest { + minimalManifest( + components: [ + ExternalComponentDefinition( + id: "test-pack.brew", + displayName: "Brew", + description: "package", + type: .brewPackage, + installAction: .brewInstall(package: "git"), + doctorChecks: componentChecks + ), + ], + supplementaryDoctorChecks: supplementary + ) + } + + @Test("Warns when scope is declared on a check type that ignores it") + func warnsOnIgnoredScope() throws { + let tmpDir = try makeTmpDir(label: "heuristics-scope") + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let manifest = manifestWithDoctorChecks(supplementary: [ + doctorCheck(type: .hookEventExists, name: "SessionStart hook", scope: .project), + doctorCheck(type: .settingsKeyEquals, name: "Plan mode", scope: .global), + ]) + let findings = PackHeuristics.check(manifest: manifest, packPath: tmpDir) + + #expect(findings.contains { + $0.severity == .warning && $0.message.contains("'SessionStart hook' declares `scope`") + }) + #expect(findings.contains { + $0.severity == .warning && $0.message.contains("'Plan mode' declares `scope`") + }) + } + + @Test("Does not warn about scope on path-based checks or when scope is omitted") + func noWarningForValidScopeUsage() throws { + let tmpDir = try makeTmpDir(label: "heuristics-scope-ok") + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let manifest = manifestWithDoctorChecks(supplementary: [ + doctorCheck(type: .fileExists, name: "Hook file", scope: .project), + doctorCheck(type: .hookEventExists, name: "SessionStart hook", scope: nil), + ]) + let findings = PackHeuristics.check(manifest: manifest, packPath: tmpDir) + + #expect(!findings.contains { $0.message.contains("declares `scope`") }) + } + + @Test("Warns for scope on a component-level doctor check") + func warnsOnIgnoredScopeInComponentCheck() throws { + let tmpDir = try makeTmpDir(label: "heuristics-scope-component") + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let manifest = manifestWithDoctorChecks(componentChecks: [ + doctorCheck(type: .settingsKeyEquals, name: "Plan mode", scope: .project), + ]) + let findings = PackHeuristics.check(manifest: manifest, packPath: tmpDir) + + #expect(findings.contains { + $0.severity == .warning && $0.message.contains("'Plan mode' declares `scope`") + }) + } } diff --git a/docs/architecture.md b/docs/architecture.md index f3c141b3..f4a398a1 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -276,6 +276,8 @@ Individual checks resolve component presence through three tiers: MCP server checks follow the same pattern: project-scoped entries (`projects[path].mcpServers` in `~/.claude.json`) are checked before global entries (`mcpServers`). +Settings-reading checks do too. `PluginCheck` and the pack-declared `hookEventExists` / `settingsKeyEquals` checks read `/.claude/settings.local.json` before `~/.claude/settings.json`, which mirrors Claude Code's own precedence — so they report on the configuration actually in effect rather than on one file in isolation. Doctor output names the file that answered, and a settings file that exists but cannot be parsed is always surfaced rather than skipped silently. + ### Pack Resolution When determining which packs to check, doctor uses a priority chain: diff --git a/docs/techpack-schema.md b/docs/techpack-schema.md index 6689444b..39bcf1cd 100644 --- a/docs/techpack-schema.md +++ b/docs/techpack-schema.md @@ -421,7 +421,6 @@ Doctor checks verify pack health. They can be defined at two levels: | `section` | `String` | No | Grouping label in output | | `fixCommand` | `String` | No | Shell command for `mcs doctor --fix` | | `fixScript` | `String` | No | Path to fix script (for complex fixes) | -| `scope` | `String` | No | `global` or `project` | | `isOptional` | `Boolean` | No | If `true`, failure is a warning, not an error | ### Check Types @@ -437,6 +436,36 @@ Doctor checks verify pack health. They can be defined at two levels: | `hookEventExists` | `event` | Is a hook event registered in settings? | | `settingsKeyEquals` | `keyPath`, `expectedValue` | Does a settings JSON key equal a specific value? | +### `scope` — path-based checks only + +| Field | Type | Required | Description | +|-------|------|----------|-------------| +| `scope` | `String` | No | `global` (default) or `project` | + +`scope` answers *"what is `path` relative to?"*, so it applies only to the four checks that take +a `path`: `fileExists`, `directoryExists`, `fileContains`, `fileNotContains`. + +- `global` — the path is used as written, with `~` expanded. +- `project` — the path is resolved against the project root and confined to it. A path that + escapes the project fails the check. Outside a project, the check is skipped. + +The other check types ignore `scope`; `mcs pack validate` warns if you set it on them. + +### Settings resolution + +`hookEventExists` and `settingsKeyEquals` don't take a `path` — the settings file is implied. They +resolve it automatically, most specific first: + +1. `/.claude/settings.local.json` — where `mcs sync` writes project-scoped hooks and + settings keys +2. `~/.claude/settings.json` — the global file + +Globally-configured packs have no project root and read only the global file. The order mirrors +Claude Code's own precedence (project settings override global), so these checks report on the +configuration actually in effect. Doctor output names the file that answered — `registered in +settings.local.json` vs `registered in settings.json` — and a settings file that exists but can't +be parsed is always reported rather than skipped silently. + ### Auto-Derived Checks Most components get free doctor checks from their install action — no need to define them manually: diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 5a25e76c..d0805bf5 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -263,8 +263,9 @@ If `mcs doctor` doesn't identify the problem: 1. Check that your PATH includes the necessary binaries (`brew`, `node`, `claude`) 2. Verify `~/.claude.json` is valid JSON: `python3 -m json.tool ~/.claude.json` 3. Verify `~/.claude/settings.json` is valid JSON: `python3 -m json.tool ~/.claude/settings.json` -4. Check `.claude/.mcs-project` in your project for state corruption -5. Open an issue at the project repository with the output of `mcs doctor` +4. Verify your project's `.claude/settings.local.json` is valid JSON — doctor reports it as unreadable when it can't be parsed, and checks then fall back to the global file +5. Check `.claude/.mcs-project` in your project for state corruption +6. Open an issue at the project repository with the output of `mcs doctor` --- From 66eaeaca871efdbf06469a92fc5f9a8d061bdef2 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Tue, 28 Jul 2026 23:47:19 +0200 Subject: [PATCH 2/2] Disable wrapIfStatementBodies to match the repo's inline style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Newer SwiftFormat releases wrap single-line `if x { return y }` bodies, which this codebase writes inline throughout — a clean main fails `swiftformat --lint --strict .` on 20 files under 0.62.1 - Declares the existing style in config rather than pinning the tool version, so the check is stable across SwiftFormat upgrades --- .swiftformat | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.swiftformat b/.swiftformat index 37c83427..c6a8ca08 100644 --- a/.swiftformat +++ b/.swiftformat @@ -2,4 +2,4 @@ --indent 4 --ifdef noindent --exclude .build,.swiftpm ---disable wrapMultilineStatementBraces +--disable wrapMultilineStatementBraces,wrapIfStatementBodies