From 2ce97b3a8fb3d576a1799067c6d86d4859283d56 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Thu, 30 Jul 2026 22:59:13 +0200 Subject: [PATCH] ISSUE-355: Assert hook matcher and command in hookEventExists - Add optional `matcher` and `command` fields so packs can verify hooks they ship through a settings file, which no derived check can see - Report hook groups dropped by settings merge dedup instead of discarding them silently - Compose derived hook entries before pack settings files, so precedence no longer depends on component order in techpack.yaml --- Sources/mcs/Core/Settings.swift | 79 ++++++- Sources/mcs/Doctor/CoreDoctorChecks.swift | 19 +- .../ExternalPack/ExternalDoctorCheck.swift | 90 +++++++- .../ExternalPack/ExternalPackManifest.swift | 55 +++++ Sources/mcs/ExternalPack/PackHeuristics.swift | 24 ++ Sources/mcs/Sync/ConfiguratorSupport.swift | 94 ++++---- .../CoreDoctorCheckSandboxTests.swift | 207 ++++++++++++++++++ Tests/MCSTests/ExternalDoctorCheckTests.swift | 28 +++ .../MCSTests/ExternalPackManifestTests.swift | 61 ++++++ .../MCSTests/LifecycleIntegrationTests.swift | 105 +++++++++ Tests/MCSTests/PackHeuristicsTests.swift | 33 ++- Tests/MCSTests/SettingsMergeTests.swift | 69 ++++++ docs/techpack-schema.md | 41 +++- 13 files changed, 838 insertions(+), 67 deletions(-) diff --git a/Sources/mcs/Core/Settings.swift b/Sources/mcs/Core/Settings.swift index c0b89993..e8f75bd5 100644 --- a/Sources/mcs/Core/Settings.swift +++ b/Sources/mcs/Core/Settings.swift @@ -22,6 +22,27 @@ struct Settings: Codable { var hooks: [HookEntry]? } + /// A hook group `merge(with:)` discarded because an entry with the same command was already + /// registered under that event, and the two disagreed on the matcher. + /// + /// Dedup by command is deliberate — without it the same hook would register twice and fire + /// twice. Reporting the drop is what keeps it from being silent: the surviving matcher is not + /// the one the incoming settings asked for, and a matcher that never matches disables a hook + /// without any other symptom. + /// + /// The case that bites is the global scope, where composition starts from the user's existing + /// `~/.claude/settings.json`: a hand-written hook there wins over a pack shipping the same + /// command with a different matcher. Project scope composes from empty, so collisions are + /// limited to two packs' settings files, or a settings file naming a derived hook's path. + struct DroppedHookGroup { + let event: String + let command: String + /// The matcher on the group that lost. + let incomingMatcher: String? + /// The matcher on the group already present, which stays. + let installedMatcher: String? + } + struct HookEntry: Codable { var type: String? var command: String? @@ -184,19 +205,44 @@ struct Settings: Codable { /// - Plugin dict: merged at key level (existing keys win). /// - Extra JSON: generic merge — JSON objects get key-level merge, /// scalars/arrays use "existing wins" semantics. - mutating func merge(with other: Settings) { + /// + /// - Returns: The hook groups dropped by deduplication whose matcher disagreed with the group + /// that survived. An identical duplicate is the ordinary case and is not reported. Callers + /// with somewhere to report to should surface these; the result is discardable because most + /// callers merge settings that cannot collide. + @discardableResult + mutating func merge(with other: Settings) -> [DroppedHookGroup] { + var dropped: [DroppedHookGroup] = [] + // Hooks: deduplicate by command if let otherHooks = other.hooks { var merged = hooks ?? [:] for (event, otherGroups) in otherHooks { var existing = merged[event] ?? [] - let existingCommands = Set( - existing.compactMap { $0.hooks?.first?.command } - ) + // First group wins per command, matching the append-only behavior below. Keyed to + // the whole group rather than its matcher, so a group with no matcher stays + // distinguishable from a command that has not been seen at all. + var installedByCommand: [String: HookGroup] = [:] + for group in existing { + guard let command = group.hooks?.first?.command else { continue } + if installedByCommand[command] == nil { installedByCommand[command] = group } + } for group in otherGroups { - if let command = group.hooks?.first?.command, - !existingCommands.contains(command) { + guard let command = group.hooks?.first?.command else { continue } + guard let installed = installedByCommand[command] else { existing.append(group) + installedByCommand[command] = group + continue + } + if normalizedMatcher(group.matcher) != normalizedMatcher(installed.matcher) { + dropped.append( + DroppedHookGroup( + event: event, + command: command, + incomingMatcher: group.matcher, + installedMatcher: installed.matcher + ) + ) } } merged[event] = existing @@ -232,6 +278,8 @@ struct Settings: Codable { extraJSON[key] = valueData } } + + return dropped } // MARK: - Stale key removal @@ -412,3 +460,22 @@ struct Settings: Codable { // Re-serialization failure: keep existing value (no-op) } } + +// MARK: - Hook Matcher Semantics + +/// Treats an empty matcher as an absent one. +/// +/// A `HookGroup` with `matcher: ""` and one with no `matcher` key select the same tools, so +/// everything that compares matchers must agree on that or it reports drift that does not exist. +/// Shared by `Settings.merge`, `HookSettingsCheck`, and `ExternalHookEventExistsCheck` — the three +/// places that reason about matcher equality — so their verdicts cannot diverge. +func normalizedMatcher(_ matcher: String?) -> String? { + guard let matcher, !matcher.isEmpty else { return nil } + return matcher +} + +/// Renders a matcher for user-facing output, so all three call sites phrase it identically. +func describeMatcher(_ matcher: String?) -> String { + guard let matcher = normalizedMatcher(matcher) else { return "no matcher" } + return "'\(matcher)'" +} diff --git a/Sources/mcs/Doctor/CoreDoctorChecks.swift b/Sources/mcs/Doctor/CoreDoctorChecks.swift index 7398d612..5b7078a0 100644 --- a/Sources/mcs/Doctor/CoreDoctorChecks.swift +++ b/Sources/mcs/Doctor/CoreDoctorChecks.swift @@ -492,22 +492,11 @@ struct HookSettingsCheck: DoctorCheck { // Any group under the declared event carrying the declared matcher satisfies this: a // command can legitimately sit in more than one group, since `addHookEntry` matches on the // group's *first* entry and appends rather than replaces when it finds no match. - let expected = normalized(registration.matcher) - guard !underEvent.contains(where: { normalized($0.matcher) == expected }) else { return nil } - let actual = underEvent.map { describe($0.matcher) }.joined(separator: ", ") + let expected = normalizedMatcher(registration.matcher) + guard !underEvent.contains(where: { normalizedMatcher($0.matcher) == expected }) else { return nil } + let actual = underEvent.map { describeMatcher($0.matcher) }.joined(separator: ", ") return "'\(command)' has matcher \(actual) under \(expectedEvent)," - + " pack declares \(describe(registration.matcher))" - } - - /// Treats an empty matcher as an absent one, so a written `""` is not reported as drift. - private func normalized(_ matcher: String?) -> String? { - guard let matcher, !matcher.isEmpty else { return nil } - return matcher - } - - private func describe(_ matcher: String?) -> String { - guard let matcher = normalized(matcher) else { return "no matcher" } - return "'\(matcher)'" + + " pack declares \(describeMatcher(registration.matcher))" } } diff --git a/Sources/mcs/ExternalPack/ExternalDoctorCheck.swift b/Sources/mcs/ExternalPack/ExternalDoctorCheck.swift index e47990de..0da5b286 100644 --- a/Sources/mcs/ExternalPack/ExternalDoctorCheck.swift +++ b/Sources/mcs/ExternalPack/ExternalDoctorCheck.swift @@ -287,28 +287,63 @@ struct ExternalShellScriptCheck: DoctorCheck { // MARK: - Hook Event Exists Check -/// Checks that a hook event is registered in the Claude settings. +/// Checks that a hook event is registered in the Claude settings, optionally asserting the +/// matcher and command of the group that registers it. /// Pack-contributed replacement for the engine-level HookEventCheck. /// /// Resolves project `settings.local.json` before global `settings.json` — see /// `SettingsReadingCheck`. +/// +/// `matcher` and `command` exist for hooks a pack ships through a settings file. Those never +/// reach `PackArtifactRecord.hookCommands` — only `hook:` components do — so `HookSettingsCheck`, +/// which derives its expectations from `ComponentDefinition.hookRegistration`, cannot see them. +/// For a `hook:` component, prefer the derived check and leave these fields off: restating the +/// matcher gives a second copy that can drift from the component's. +/// +/// This proves the declared matcher reached settings, not that it matches a tool Claude Code +/// actually emits. Only a real session transcript proves that. struct ExternalHookEventExistsCheck: SettingsReadingCheck { + /// Whether the registration found under `event` satisfied the declared assertions. + private enum Registration { + case satisfied + /// The event is registered, but not the way the check declared. Carries the detail. + case mismatch(String) + } + let name: String let section: String let event: String + /// Exact matcher a `HookGroup` under `event` must carry. Compared as a raw string — the regex + /// is deliberately not interpreted, since Claude Code is what evaluates it at runtime. + var matcher: String? + /// Substring a hook command must contain. When `matcher` is also given, both must be satisfied + /// by the *same* group — that is what proves the registration belongs to this pack. + var commandSubstring: String? let isOptional: Bool var projectRoot: URL? var environment: Environment = .init() func check() -> CheckResult { - let probe: SettingsProbe = probeSettings { url in - try Settings.load(from: url).hooks?[event] != nil ? true : nil + // Any file that registers the event answers the probe, mismatch or not; otherwise a + // wrongly-registered project hook would fall through to a correct global one and pass. + let probe: SettingsProbe = probeSettings { url in + guard let groups = try Settings.load(from: url).hooks?[event] else { return nil } + return evaluate(groups) } if let match = probe.match { - return probe.readErrors.isEmpty - ? .pass("registered in \(match.fileName)") - : .warn("registered in \(match.fileName)\(probe.errorSuffix)") + switch match.value { + case .satisfied: + return probe.readErrors.isEmpty + ? .pass("registered in \(match.fileName)") + : .warn("registered in \(match.fileName)\(probe.errorSuffix)") + case let .mismatch(detail): + // Advisory, not fatal: the registration is present, and a user who narrowed a + // matcher on purpose should not be blocked by a red doctor. + return isOptional + ? .skip("\(detail) in \(match.fileName) (optional)") + : .warn("\(detail) in \(match.fileName)\(probe.errorSuffix)") + } } guard probe.anyFileExisted else { return .fail("no settings file found (searched \(searchedFileNames))") @@ -324,6 +359,45 @@ struct ExternalHookEventExistsCheck: SettingsReadingCheck { func fix() -> FixResult { .notFixable("Run 'mcs sync' to merge settings") } + + // MARK: - Helpers + + /// Judge the groups registered under `event` against the declared matcher and command. + private func evaluate(_ groups: [Settings.HookGroup]) -> Registration { + guard matcher != nil || commandSubstring != nil else { return .satisfied } + + // Narrow to the groups the matcher admits, then require the command inside one of those — + // checking them independently would let two unrelated groups satisfy the pair between them. + let candidates: [Settings.HookGroup] + if let matcher { + let expected = normalizedMatcher(matcher) + candidates = groups.filter { normalizedMatcher($0.matcher) == expected } + guard !candidates.isEmpty else { + let actual = groups.isEmpty + ? "none" + : groups.map { describeMatcher($0.matcher) }.joined(separator: ", ") + return .mismatch( + "\(event) has no hook group with matcher \(describeMatcher(matcher)) — found \(actual)" + ) + } + } else { + candidates = groups + } + + if let commandSubstring { + let found = candidates.contains { group in + (group.hooks ?? []).contains { $0.command?.contains(commandSubstring) == true } + } + guard found else { + let scope = matcher.map { " with matcher \(describeMatcher($0))" } ?? "" + return .mismatch( + "\(event) group\(scope) has no hook command containing '\(commandSubstring)'" + ) + } + } + + return .satisfied + } } // MARK: - Settings Key Equals Check @@ -522,6 +596,10 @@ enum ExternalDoctorCheckFactory { name: definition.name, section: section, event: event, + matcher: definition.matcher, + // `command` carries a per-type meaning: a script path for `shellScript`, a binary + // for `commandExists`, a hook-command substring here. + commandSubstring: definition.command, isOptional: definition.isOptional ?? false, projectRoot: projectRoot, environment: environment diff --git a/Sources/mcs/ExternalPack/ExternalPackManifest.swift b/Sources/mcs/ExternalPack/ExternalPackManifest.swift index 20d09093..2515fb67 100644 --- a/Sources/mcs/ExternalPack/ExternalPackManifest.swift +++ b/Sources/mcs/ExternalPack/ExternalPackManifest.swift @@ -269,6 +269,20 @@ extension ExternalPackManifest { guard Constants.HookEvent.validRawValues.contains(event) else { throw ManifestError.invalidDoctorCheck(name: check.name, reason: "hookEventExists has unknown event '\(event)'") } + // An empty matcher is ambiguous: it reads as an assertion but compares equal to a + // group with no matcher at all. Omit the key to skip the assertion. + if let matcher = check.matcher, matcher.isEmpty { + throw ManifestError.invalidDoctorCheck( + name: check.name, + reason: "hookEventExists 'matcher' must be non-empty — omit it to skip the assertion" + ) + } + if let command = check.command, command.isEmpty { + throw ManifestError.invalidDoctorCheck( + name: check.name, + reason: "hookEventExists 'command' must be non-empty — omit it to skip the assertion" + ) + } case .settingsKeyEquals: guard let keyPath = check.keyPath, !keyPath.isEmpty else { throw ManifestError.invalidDoctorCheck(name: check.name, reason: "settingsKeyEquals requires non-empty 'keyPath'") @@ -906,6 +920,8 @@ struct ExternalDoctorCheckDefinition: Codable { let type: ExternalDoctorCheckType let name: String let section: String? + /// Meaning depends on `type`: the binary for `commandExists`, the script path for + /// `shellScript`, a hook-command substring for `hookEventExists`. let command: String? let args: [String]? let path: String? @@ -914,9 +930,48 @@ struct ExternalDoctorCheckDefinition: Codable { let fixCommand: String? let fixScript: String? let event: String? + /// `hookEventExists` only — the exact matcher a hook group under `event` must carry. + let matcher: String? let keyPath: String? let expectedValue: String? let isOptional: Bool? + + /// Written out rather than synthesized so optional fields carry `nil` defaults. Swift gives a + /// `let` optional no default in the memberwise init, so every added field would otherwise have + /// to be threaded through all 12 construction sites. + init( + type: ExternalDoctorCheckType, + name: String, + section: String? = nil, + command: String? = nil, + args: [String]? = nil, + path: String? = nil, + pattern: String? = nil, + scope: ExternalDoctorCheckScope? = nil, + fixCommand: String? = nil, + fixScript: String? = nil, + event: String? = nil, + matcher: String? = nil, + keyPath: String? = nil, + expectedValue: String? = nil, + isOptional: Bool? = nil + ) { + self.type = type + self.name = name + self.section = section + self.command = command + self.args = args + self.path = path + self.pattern = pattern + self.scope = scope + self.fixCommand = fixCommand + self.fixScript = fixScript + self.event = event + self.matcher = matcher + self.keyPath = keyPath + self.expectedValue = expectedValue + self.isOptional = isOptional + } } enum ExternalDoctorCheckType: String, Codable, CaseIterable { diff --git a/Sources/mcs/ExternalPack/PackHeuristics.swift b/Sources/mcs/ExternalPack/PackHeuristics.swift index 4ac55b66..e3a3098d 100644 --- a/Sources/mcs/ExternalPack/PackHeuristics.swift +++ b/Sources/mcs/ExternalPack/PackHeuristics.swift @@ -25,6 +25,7 @@ enum PackHeuristics { findings += checkMCPDependencyGaps(components: components) findings += checkPythonModulePaths(components: components, packPath: packPath) findings += checkDoctorCheckScopeUsage(manifest: manifest, components: components) + findings += checkDoctorCheckMatcherUsage(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). @@ -362,4 +363,27 @@ enum PackHeuristics { ) } } + + /// Warns when a doctor check declares `matcher` on a type that ignores it. + /// + /// Same failure mode as `checkDoctorCheckScopeUsage`: a field that reads as an assertion but + /// is never consulted. There is no equivalent check for `command` — every type that accepts it + /// consumes it, only with a different meaning per type. + private static func checkDoctorCheckMatcherUsage( + manifest: ExternalPackManifest, + components: [ExternalComponentDefinition] + ) -> [Finding] { + let allChecks = (manifest.supplementaryDoctorChecks ?? []) + + components.flatMap { $0.doctorChecks ?? [] } + + return allChecks + .filter { $0.matcher != nil && $0.type != .hookEventExists } + .map { check in + Finding( + severity: .warning, + message: "Doctor check '\(check.name)' declares `matcher` but type" + + " `\(check.type.rawValue)` ignores it — `matcher` applies only to `hookEventExists`" + ) + } + } } diff --git a/Sources/mcs/Sync/ConfiguratorSupport.swift b/Sources/mcs/Sync/ConfiguratorSupport.swift index e1b52d96..61fbac09 100644 --- a/Sources/mcs/Sync/ConfiguratorSupport.swift +++ b/Sources/mcs/Sync/ConfiguratorSupport.swift @@ -178,6 +178,12 @@ enum ConfiguratorSupport { /// Shared by both project and global `composeSettings` — the inner loop is identical. /// The hook command prefix is parameterized via `hookCommandPrefix`. /// + /// Runs in two passes on purpose. `Settings.merge` deduplicates hook groups by command and + /// keeps the entry already present, so whichever source lands first wins. Derived entries must + /// be that source: they carry the engine's own scope-correct command path, and they are the + /// only ones doctor can verify against a `HookRegistration`. A single pass would decide it by + /// the order components happen to appear in `techpack.yaml`. + /// /// - Returns: Whether any content was added and the per-pack contributed settings keys. static func mergePackComponentsIntoSettings( packs: [any TechPack], @@ -190,50 +196,62 @@ enum ConfiguratorSupport { var hasContent = false var contributedKeys: [String: [String]] = [:] - for pack in packs { + let included: [(pack: any TechPack, component: ComponentDefinition)] = packs.flatMap { pack in let excluded = excludedComponents[pack.identifier] ?? [] - for component in pack.components { - guard !excluded.contains(component.id) else { continue } - - if let reg = component.hookRegistration, - let command = component.hookCommand(prefix: hookCommandPrefix) { - if settings.addHookEntry( - event: reg.event, - command: command, - matcher: reg.matcher, - timeout: reg.timeout, - isAsync: reg.isAsync, - statusMessage: reg.statusMessage - ) { - hasContent = true - } - } + return pack.components + .filter { !excluded.contains($0.id) } + .map { (pack, $0) } + } - if case let .plugin(name) = component.installAction { - let ref = PluginRef(name) - var plugins = settings.enabledPlugins ?? [:] - if plugins[ref.bareName] == nil { - plugins[ref.bareName] = true - } - settings.enabledPlugins = plugins + // Pass 1: entries derived from component definitions. + for (pack, component) in included { + if let reg = component.hookRegistration, + let command = component.hookCommand(prefix: hookCommandPrefix) { + if settings.addHookEntry( + event: reg.event, + command: command, + matcher: reg.matcher, + timeout: reg.timeout, + isAsync: reg.isAsync, + statusMessage: reg.statusMessage + ) { hasContent = true - contributedKeys[pack.identifier, default: []].append("enabledPlugins.\(ref.bareName)") } + } - if case let .settingsMerge(source) = component.installAction, let source { - do { - let packSettings = try Settings.load(from: source, substituting: resolvedValues) - if !packSettings.extraJSON.isEmpty { - contributedKeys[pack.identifier, default: []].append(contentsOf: packSettings.extraJSON.keys) - } - settings.merge(with: packSettings) - hasContent = true - } catch { - output.warn( - "Could not load settings from \(pack.displayName)/\(source.lastPathComponent): \(error.localizedDescription)" - ) - } + if case let .plugin(name) = component.installAction { + let ref = PluginRef(name) + var plugins = settings.enabledPlugins ?? [:] + if plugins[ref.bareName] == nil { + plugins[ref.bareName] = true } + settings.enabledPlugins = plugins + hasContent = true + contributedKeys[pack.identifier, default: []].append("enabledPlugins.\(ref.bareName)") + } + } + + // Pass 2: pack-supplied settings files, merged on top of the derived entries. + for (pack, component) in included { + guard case let .settingsMerge(source) = component.installAction, let source else { continue } + do { + let packSettings = try Settings.load(from: source, substituting: resolvedValues) + if !packSettings.extraJSON.isEmpty { + contributedKeys[pack.identifier, default: []].append(contentsOf: packSettings.extraJSON.keys) + } + for dropped in settings.merge(with: packSettings) { + output.warn( + "\(pack.displayName): hook group for '\(dropped.command)' under \(dropped.event)" + + " was not merged — \(source.lastPathComponent) declares matcher" + + " \(describeMatcher(dropped.incomingMatcher)) but" + + " \(describeMatcher(dropped.installedMatcher)) is already registered" + ) + } + hasContent = true + } catch { + output.warn( + "Could not load settings from \(pack.displayName)/\(source.lastPathComponent): \(error.localizedDescription)" + ) } } diff --git a/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift b/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift index 4ec863cd..e24b43fe 100644 --- a/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift +++ b/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift @@ -1459,3 +1459,210 @@ extension ExternalSettingsKeyEqualsCheckSandboxTests { #expect(msg.contains("settings.local.json is unreadable")) } } + +// MARK: - ExternalHookEventExistsCheck Matcher / Command Assertions (issue #355) + +extension ExternalHookEventExistsCheckSandboxTests { + /// Two groups under PreToolUse: one matching `Agent|Task` running the gate, one matching + /// `Bash` running something else. Enough shape to tell "same group" from "any group". + private static let twoGroupSettings = """ + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Agent|Task", + "hooks": [{ "type": "command", "command": "bash .claude/hooks/kb-gate.sh" }] + }, + { + "matcher": "Bash", + "hooks": [{ "type": "command", "command": "bash .claude/hooks/audit.sh" }] + } + ] + } + } + """ + + private func makeCheck( + in home: URL, + settings: String, + matcher: String? = nil, + command: String? = nil, + isOptional: Bool = false + ) throws -> ExternalHookEventExistsCheck { + let env = Environment(home: home) + try settings.write(to: env.claudeSettings, atomically: true, encoding: .utf8) + var check = ExternalHookEventExistsCheck( + name: "PreToolUse hook", section: "Hooks", + event: "PreToolUse", matcher: matcher, commandSubstring: command, + isOptional: isOptional + ) + check.environment = env + return check + } + + @Test("pass when the declared matcher is registered") + func passWhenMatcherMatches() throws { + let home = try makeGlobalTmpDir(label: "hook-matcher-pass") + defer { try? FileManager.default.removeItem(at: home) } + + let check = try makeCheck(in: home, settings: Self.twoGroupSettings, matcher: "Agent|Task") + let result = check.check() + guard case .pass = result else { + Issue.record("Expected .pass, got \(result)") + return + } + } + + @Test("warn when the event is registered but the matcher differs") + func warnWhenMatcherDiffers() throws { + let home = try makeGlobalTmpDir(label: "hook-matcher-differs") + defer { try? FileManager.default.removeItem(at: home) } + + // The motivating shape: settings carry `Agent|Task`, the check declares only `Task`. + let check = try makeCheck(in: home, settings: Self.twoGroupSettings, matcher: "Task") + let result = check.check() + guard case let .warn(msg) = result else { + Issue.record("Expected .warn, got \(result)") + return + } + // Both the expected and the actual matchers must be named, or the reader cannot tell a + // mismatch from an absent registration. + #expect(msg.contains("'Task'")) + #expect(msg.contains("'Agent|Task'")) + #expect(msg.contains("'Bash'")) + } + + @Test("warn when the matcher matches but the command substring is absent") + func warnWhenCommandAbsent() throws { + let home = try makeGlobalTmpDir(label: "hook-command-absent") + defer { try? FileManager.default.removeItem(at: home) } + + let check = try makeCheck( + in: home, settings: Self.twoGroupSettings, + matcher: "Agent|Task", command: "missing.sh" + ) + let result = check.check() + guard case let .warn(msg) = result else { + Issue.record("Expected .warn, got \(result)") + return + } + #expect(msg.contains("missing.sh")) + } + + @Test("warn when matcher and command are satisfied by different groups") + func warnWhenSatisfiedByDifferentGroups() throws { + let home = try makeGlobalTmpDir(label: "hook-different-groups") + defer { try? FileManager.default.removeItem(at: home) } + + // `Agent|Task` exists and `audit.sh` exists, but not together — the pair must be proven by + // one group or it proves nothing about which registration belongs to the pack. + let check = try makeCheck( + in: home, settings: Self.twoGroupSettings, + matcher: "Agent|Task", command: "audit.sh" + ) + let result = check.check() + guard case .warn = result else { + Issue.record("Expected .warn, got \(result)") + return + } + } + + @Test("pass on command substring alone, matched in any group") + func passWhenCommandOnlyMatches() throws { + let home = try makeGlobalTmpDir(label: "hook-command-only") + defer { try? FileManager.default.removeItem(at: home) } + + let check = try makeCheck(in: home, settings: Self.twoGroupSettings, command: "audit.sh") + let result = check.check() + guard case .pass = result else { + Issue.record("Expected .pass, got \(result)") + return + } + } + + @Test("pass on event presence alone when neither field is declared") + func passWhenNoAssertionsDeclared() throws { + let home = try makeGlobalTmpDir(label: "hook-no-assertions") + defer { try? FileManager.default.removeItem(at: home) } + + let check = try makeCheck(in: home, settings: Self.twoGroupSettings) + let result = check.check() + guard case .pass = result else { + Issue.record("Expected .pass, got \(result)") + return + } + } + + @Test("empty matcher in settings satisfies an undeclared matcher") + func emptyMatcherNormalizesToAbsent() throws { + let home = try makeGlobalTmpDir(label: "hook-empty-matcher") + defer { try? FileManager.default.removeItem(at: home) } + + let settings = """ + { + "hooks": { + "PreToolUse": [ + { "matcher": "", "hooks": [{ "type": "command", "command": "bash run.sh" }] } + ] + } + } + """ + // A written "" and an absent key select the same tools, so this must not read as drift. + let check = try makeCheck(in: home, settings: settings, command: "run.sh") + let result = check.check() + guard case .pass = result else { + Issue.record("Expected .pass, got \(result)") + return + } + } + + @Test("skip when optional and the matcher differs") + func skipWhenOptionalMismatch() throws { + let home = try makeGlobalTmpDir(label: "hook-optional-mismatch") + defer { try? FileManager.default.removeItem(at: home) } + + let check = try makeCheck( + in: home, settings: Self.twoGroupSettings, + matcher: "Task", isOptional: true + ) + let result = check.check() + guard case .skip = result else { + Issue.record("Expected .skip, got \(result)") + return + } + } + + @Test("a mismatched project registration is not rescued by a correct global one") + func projectMismatchWinsOverGlobal() throws { + let home = try makeGlobalTmpDir(label: "hook-project-mismatch") + defer { try? FileManager.default.removeItem(at: home) } + let env = Environment(home: home) + + let projectSettings = """ + { + "hooks": { + "PreToolUse": [ + { "matcher": "Task", "hooks": [{ "type": "command", "command": "bash gate.sh" }] } + ] + } + } + """ + let projectRoot = try makeProjectSettings(in: home, contents: projectSettings) + try Self.twoGroupSettings.write(to: env.claudeSettings, atomically: true, encoding: .utf8) + + var check = ExternalHookEventExistsCheck( + name: "PreToolUse hook", section: "Hooks", + event: "PreToolUse", matcher: "Agent|Task", + isOptional: false, projectRoot: projectRoot + ) + check.environment = env + // settings.local.json takes precedence and is what Claude Code will actually run, so its + // wrong matcher must be reported rather than masked by the correct global file. + let result = check.check() + guard case let .warn(msg) = result else { + Issue.record("Expected .warn, got \(result)") + return + } + #expect(msg.contains("settings.local.json")) + } +} diff --git a/Tests/MCSTests/ExternalDoctorCheckTests.swift b/Tests/MCSTests/ExternalDoctorCheckTests.swift index 6d269711..0dc5badd 100644 --- a/Tests/MCSTests/ExternalDoctorCheckTests.swift +++ b/Tests/MCSTests/ExternalDoctorCheckTests.swift @@ -538,6 +538,34 @@ struct ExternalDoctorCheckTests { #expect(check.section == "Hooks") } + @Test("Factory forwards matcher and command to hookEventExists check") + func factoryForwardsHookMatcherAndCommand() throws { + let tmpDir = try makeTmpDir() + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let definition = ExternalDoctorCheckDefinition( + type: .hookEventExists, + name: "Gate hook", + section: "Hooks", + command: "kb-gate.sh", + event: "PreToolUse", + matcher: "Agent|Task" + ) + + let check = ExternalDoctorCheckFactory.makeCheck( + from: definition, + packPath: tmpDir, + projectRoot: nil, + scriptRunner: makeScriptRunner() + ) + + // `command` means something different per check type; this asserts it lands on the hook + // check as a command substring rather than being ignored or read as a script path. + let hookCheck = try #require(check as? ExternalHookEventExistsCheck) + #expect(hookCheck.matcher == "Agent|Task") + #expect(hookCheck.commandSubstring == "kb-gate.sh") + } + @Test("Factory creates settingsKeyEquals check from definition") func factoryCreatesSettingsKeyEqualsCheck() throws { let tmpDir = try makeTmpDir() diff --git a/Tests/MCSTests/ExternalPackManifestTests.swift b/Tests/MCSTests/ExternalPackManifestTests.swift index 651defde..91441944 100644 --- a/Tests/MCSTests/ExternalPackManifestTests.swift +++ b/Tests/MCSTests/ExternalPackManifestTests.swift @@ -671,6 +671,67 @@ struct ExternalPackManifestTests { } } + @Test("Validation rejects hookEventExists with an empty matcher") + func rejectHookEventExistsEmptyMatcher() throws { + let yaml = """ + schemaVersion: 1 + identifier: test + displayName: Test + description: Test + version: "1.0.0" + supplementaryDoctorChecks: + - type: hookEventExists + name: Bad hook check + section: Hooks + event: PreToolUse + matcher: "" + """ + + let tmpDir = try makeTmpDir() + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let file = tmpDir.appendingPathComponent("techpack.yaml") + try yaml.write(to: file, atomically: true, encoding: .utf8) + + let manifest = try ExternalPackManifest.load(from: file) + // An empty matcher compares equal to a group with no matcher at all, so it reads as an + // assertion while asserting nothing. Omitting the key is the way to skip it. + #expect(throws: ManifestError.self) { + try manifest.validate() + } + } + + @Test("Validation accepts hookEventExists with matcher and command") + func acceptHookEventExistsWithMatcherAndCommand() throws { + let yaml = """ + schemaVersion: 1 + identifier: test + displayName: Test + description: Test + version: "1.0.0" + supplementaryDoctorChecks: + - type: hookEventExists + name: Gate hook + section: Hooks + event: PreToolUse + matcher: "Agent|Task" + command: kb-gate.sh + """ + + let tmpDir = try makeTmpDir() + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let file = tmpDir.appendingPathComponent("techpack.yaml") + try yaml.write(to: file, atomically: true, encoding: .utf8) + + let manifest = try ExternalPackManifest.load(from: file) + try manifest.validate() + + let check = manifest.supplementaryDoctorChecks?.first + #expect(check?.matcher == "Agent|Task") + #expect(check?.command == "kb-gate.sh") + } + @Test("Validation rejects settingsKeyEquals without keyPath") func rejectSettingsKeyEqualsNoKeyPath() throws { let yaml = """ diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index c0e2c050..bfd6ebf9 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -380,6 +380,111 @@ struct SinglePackLifecycleTests { var healedRunner = bed.makeDoctorRunner(registry: registry) #expect(try healedRunner.run().warnings == clean.warnings) } + + @Test("Declarative matcher check covers a hook shipped through a settings file") + func settingsFileHookMatcherLifecycle() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + // A hook that arrives via `settingsFile:` rather than a `hook:` component never reaches + // PackArtifactRecord.hookCommands, so HookSettingsCheck cannot see it at all. The + // declarative assertion is the only verification available for this shape. + let settingsSource = try bed.makeSettingsSource(content: """ + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Agent|Task", + "hooks": [{ "type": "command", "command": "bash .claude/hooks/gate.sh" }] + } + ] + } + } + """) + var matcherCheck = ExternalHookEventExistsCheck( + name: "Gate hook registered", section: "Hooks", + event: "PreToolUse", matcher: "Agent|Task", commandSubstring: "gate.sh", + isOptional: false, projectRoot: bed.project + ) + matcherCheck.environment = bed.env + + let pack = MockTechPack( + identifier: "settings-hook-pack", + displayName: "Settings Hook Pack", + components: [ + bed.settingsComponent(pack: "settings-hook-pack", id: "settings", source: settingsSource), + ], + supplementaryDoctorChecks: [matcherCheck] + ) + let registry = TechPackRegistry(packs: [pack]) + + // === Step 1: Sync merges the settings file, doctor confirms the matcher landed === + try bed.makeConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + let installed = try Settings.load(from: bed.settingsLocalPath) + #expect(installed.hooks?["PreToolUse"]?.first?.matcher == "Agent|Task") + + var runner = bed.makeDoctorRunner(registry: registry) + let clean = try runner.run() + #expect(clean.warnings == 0) + + // === Step 2: Hand-edit the matcher to one that matches nothing === + var drifted = installed + drifted.hooks?["PreToolUse"]?[0].matcher = "Task" + try drifted.save(to: bed.settingsLocalPath) + + var driftRunner = bed.makeDoctorRunner(registry: registry) + let driftSummary = try driftRunner.run() + #expect(driftSummary.warnings > clean.warnings) + // Advisory, not fatal — the registration is present, just not as declared. + #expect(driftSummary.issues == clean.issues) + } + + @Test("Derived hook entry wins over a settings-file copy regardless of component order") + func derivedHookWinsOverSettingsFileInEitherOrder() throws { + // Precedence rationale lives on `ConfiguratorSupport.mergePackComponentsIntoSettings`. + // Specific to this test: hook destinations are always namespaced under /, so a + // settings file collides with a derived entry only by spelling out that same namespaced + // path — which is what this pack does, to force the collision rather than hope for it. + for settingsFirst in [false, true] { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let hookSource = try bed.makeHookSource(name: "gate.sh") + let settingsSource = try bed.makeSettingsSource(content: """ + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Task", + "hooks": [{ "type": "command", "command": "bash .claude/hooks/order-pack/gate.sh" }] + } + ] + } + } + """) + + let hook = bed.hookComponent( + pack: "order-pack", id: "gate-hook", + source: hookSource, destination: "gate.sh", + hookRegistration: HookRegistration(event: .preToolUse, matcher: "Agent|Task") + ) + let settings = bed.settingsComponent(pack: "order-pack", id: "settings", source: settingsSource) + + let pack = MockTechPack( + identifier: "order-pack", + displayName: "Order Pack", + components: settingsFirst ? [settings, hook] : [hook, settings] + ) + try bed.makeConfigurator(registry: TechPackRegistry(packs: [pack])) + .configure(packs: [pack], confirmRemovals: false) + + let composed = try Settings.load(from: bed.settingsLocalPath) + let groups = composed.hooks?["PreToolUse"] ?? [] + // The component's matcher is the one doctor can verify, so it must be the survivor. + #expect(groups.count == 1, "settingsFirst=\(settingsFirst)") + #expect(groups.first?.matcher == "Agent|Task", "settingsFirst=\(settingsFirst)") + } + } } // MARK: - Scenario 2: Multi-Pack Convergence diff --git a/Tests/MCSTests/PackHeuristicsTests.swift b/Tests/MCSTests/PackHeuristicsTests.swift index 60e27522..36faba2a 100644 --- a/Tests/MCSTests/PackHeuristicsTests.swift +++ b/Tests/MCSTests/PackHeuristicsTests.swift @@ -916,7 +916,8 @@ struct PackHeuristicsTests { private func doctorCheck( type: ExternalDoctorCheckType, name: String, - scope: ExternalDoctorCheckScope? + scope: ExternalDoctorCheckScope? = nil, + matcher: String? = nil ) -> ExternalDoctorCheckDefinition { ExternalDoctorCheckDefinition( type: type, @@ -930,6 +931,7 @@ struct PackHeuristicsTests { fixCommand: nil, fixScript: nil, event: type == .hookEventExists ? "SessionStart" : nil, + matcher: matcher, keyPath: type == .settingsKeyEquals ? "permissions.defaultMode" : nil, expectedValue: type == .settingsKeyEquals ? "plan" : nil, isOptional: nil @@ -1003,4 +1005,33 @@ struct PackHeuristicsTests { $0.severity == .warning && $0.message.contains("'Plan mode' declares `scope`") }) } + + @Test("Warns when matcher is declared on a check type that ignores it") + func warnsOnIgnoredMatcher() throws { + let tmpDir = try makeTmpDir(label: "heuristics-matcher") + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let manifest = manifestWithDoctorChecks(supplementary: [ + doctorCheck(type: .fileExists, name: "Hook file", matcher: "Agent"), + ]) + let findings = PackHeuristics.check(manifest: manifest, packPath: tmpDir) + + #expect(findings.contains { + $0.severity == .warning && $0.message.contains("'Hook file' declares `matcher`") + }) + } + + @Test("Does not warn about matcher on hookEventExists or when matcher is omitted") + func noWarningForValidMatcherUsage() throws { + let tmpDir = try makeTmpDir(label: "heuristics-matcher-ok") + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let manifest = manifestWithDoctorChecks(supplementary: [ + doctorCheck(type: .hookEventExists, name: "SessionStart hook", matcher: "Agent"), + doctorCheck(type: .fileExists, name: "Hook file"), + ]) + let findings = PackHeuristics.check(manifest: manifest, packPath: tmpDir) + + #expect(!findings.contains { $0.message.contains("declares `matcher`") }) + } } diff --git a/Tests/MCSTests/SettingsMergeTests.swift b/Tests/MCSTests/SettingsMergeTests.swift index 7256658f..aad05f9d 100644 --- a/Tests/MCSTests/SettingsMergeTests.swift +++ b/Tests/MCSTests/SettingsMergeTests.swift @@ -116,6 +116,75 @@ struct SettingsMergeTests { #expect(commands.contains("echo new")) } + @Test("An identical duplicate is dropped without being reported") + func identicalDuplicateNotReported() { + let group = Settings.HookGroup( + matcher: "Edit", + hooks: [Settings.HookEntry(type: "command", command: "echo hi")] + ) + var base = Settings(hooks: ["PreToolUse": [group]]) + + let dropped = base.merge(with: Settings(hooks: ["PreToolUse": [group]])) + + // The ordinary case — a pack repeating a hook it already declared as a component. Nothing + // is lost, so warning about it would be noise. + #expect(dropped.isEmpty) + #expect(base.hooks?["PreToolUse"]?.count == 1) + } + + @Test("A dropped group with a differing matcher is reported") + func droppedGroupWithDifferingMatcherIsReported() { + var base = Settings(hooks: [ + "PreToolUse": [ + Settings.HookGroup( + matcher: "Agent|Task", + hooks: [Settings.HookEntry(type: "command", command: "bash gate.sh")] + ), + ], + ]) + let other = Settings(hooks: [ + "PreToolUse": [ + Settings.HookGroup( + matcher: "Task", + hooks: [Settings.HookEntry(type: "command", command: "bash gate.sh")] + ), + ], + ]) + + let dropped = base.merge(with: other) + + // Dedup by command keeps the installed group, so the incoming matcher never takes effect. + // Silently discarding it is how a hook ends up registered under a matcher nobody asked for. + #expect(dropped.count == 1) + #expect(dropped.first?.event == "PreToolUse") + #expect(dropped.first?.command == "bash gate.sh") + #expect(dropped.first?.incomingMatcher == "Task") + #expect(dropped.first?.installedMatcher == "Agent|Task") + #expect(base.hooks?["PreToolUse"]?.first?.matcher == "Agent|Task") + } + + @Test("An empty matcher and an absent matcher are not reported as a conflict") + func emptyAndAbsentMatcherAreEquivalent() { + var base = Settings(hooks: [ + "PreToolUse": [ + Settings.HookGroup( + matcher: nil, + hooks: [Settings.HookEntry(type: "command", command: "bash run.sh")] + ), + ], + ]) + let other = Settings(hooks: [ + "PreToolUse": [ + Settings.HookGroup( + matcher: "", + hooks: [Settings.HookEntry(type: "command", command: "bash run.sh")] + ), + ], + ]) + + #expect(base.merge(with: other).isEmpty) + } + @Test("Hooks merge across different events") func hooksMergeDifferentEvents() { var base = Settings(hooks: [ diff --git a/docs/techpack-schema.md b/docs/techpack-schema.md index 39bcf1cd..bab9127e 100644 --- a/docs/techpack-schema.md +++ b/docs/techpack-schema.md @@ -433,9 +433,48 @@ Doctor checks verify pack health. They can be defined at two levels: | `fileContains` | `path`, `pattern` | Does a file match a regex pattern? | | `fileNotContains` | `path`, `pattern` | Does a file NOT match a regex pattern? | | `shellScript` | `command` | Run a command. Exit codes: `0`=pass, `1`=fail, `2`=warn, `3`=skip | -| `hookEventExists` | `event` | Is a hook event registered in settings? | +| `hookEventExists` | `event` | Is a hook event registered in settings? Optionally asserts `matcher` and `command` — see below | | `settingsKeyEquals` | `keyPath`, `expectedValue` | Does a settings JSON key equal a specific value? | +### `matcher` and `command` — `hookEventExists` only + +By default `hookEventExists` asks only whether the event key is present. A hook group whose +matcher matches nothing satisfies that, so the hook can be registered, green in doctor, and never +fire. Two optional fields tighten it: + +```yaml +- type: hookEventExists + name: "Gate hook registered" + section: Hooks + event: PreToolUse + matcher: "Agent|Task" # a hook group with exactly this matcher must exist + command: kb-gate.sh # that group must run a command containing this substring +``` + +| Field | Type | Required | Description | +|-------|------|----------|-------------| +| `matcher` | `String` | No | Exact matcher a hook group under `event` must carry | +| `command` | `String` | No | Substring a hook command must contain | + +- `matcher` is compared as a **raw string**. The regex is not interpreted — Claude Code evaluates + it at runtime, and mcs does not second-guess what it will match. A written `""` and an absent + matcher are treated as the same thing; declaring `matcher: ""` is rejected at validation. +- When both are given they must be satisfied by the **same** group. Two unrelated groups + satisfying one field each does not prove the registration belongs to your pack. +- A mismatch is a **warning**, not a failure — the registration exists, and a user who narrowed a + matcher deliberately should not get a red doctor. An absent event still fails. +- `isOptional: true` downgrades both the mismatch and the absence to a skip. + +**Use these for hooks you ship through a settings file.** A hook declared as a component +(`hook:` with `hookEvent`) already gets event and matcher verification automatically, derived from +the component itself — restating the matcher here just creates a second copy that can drift from +the first. + +**What this does not prove:** that the matcher matches a tool Claude Code actually emits. It proves +the string you declared reached the settings file. Tool names are harness details that change +between releases — the sub-agent spawn tool is `Agent` in current Claude Code, and a matcher of +`Task` alone matches nothing. Only a real session transcript settles that. + ### `scope` — path-based checks only | Field | Type | Required | Description |