ISSUE-355: Assert hook matcher and command in hookEventExists

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)'"
+}

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))"
     }
 }
 

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<Bool> = 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<Registration> = 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

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 {

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`"
+                )
+            }
+    }
 }