From e09bb3dbb06c875490d03059c69ae292735c3581 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Wed, 29 Jul 2026 10:34:30 +0200 Subject: [PATCH 1/3] ISSUE-355: Verify hook event and matcher against the declaring component - HookSettingsCheck now compares each pack hook command's registered event and matcher against the component's HookRegistration; absence fails, drift warns since the next sync rewrites it - Extract ComponentDefinition.hookCommand(prefix:) as the single source of truth for the command string, replacing four inline copies - Index hook placements in one pass via Settings.hookPlacementsByCommand() --- Sources/mcs/Core/Constants.swift | 15 ++ Sources/mcs/Core/Settings.swift | 26 +++ Sources/mcs/Doctor/CoreDoctorChecks.swift | 123 ++++++++-- Sources/mcs/Doctor/DoctorRunner.swift | 38 ++- Sources/mcs/Sync/Configurator.swift | 5 +- Sources/mcs/Sync/ConfiguratorSupport.swift | 6 +- Sources/mcs/Sync/GlobalSyncStrategy.swift | 6 +- Sources/mcs/Sync/ProjectSyncStrategy.swift | 6 +- Sources/mcs/Sync/SyncScope.swift | 4 +- Sources/mcs/TechPack/Component.swift | 18 ++ Tests/MCSTests/ComponentTests.swift | 84 +++++++ Tests/MCSTests/CoreDoctorCheckTests.swift | 216 ++++++++++++++++++ .../MCSTests/LifecycleIntegrationTests.swift | 49 ++++ docs/architecture.md | 14 ++ 14 files changed, 578 insertions(+), 32 deletions(-) create mode 100644 Tests/MCSTests/ComponentTests.swift diff --git a/Sources/mcs/Core/Constants.swift b/Sources/mcs/Core/Constants.swift index 048e6e0a..d2f6c6e1 100644 --- a/Sources/mcs/Core/Constants.swift +++ b/Sources/mcs/Core/Constants.swift @@ -114,6 +114,21 @@ enum Constants { static let validRawValues: Set = Set(allCases.map(\.rawValue)) } + // MARK: - Hook Commands + + /// Prefixes for the `command` string a hook component registers in settings. + /// + /// Sync writes these and doctor reads them back to match a settings entry against the + /// component that declared it, so both sides must build the string identically — see + /// `ComponentDefinition.hookCommand(prefix:)`. + enum HookCommand { + /// Project scope — hooks live in `/.claude/hooks/`. + static let projectPrefix = "bash .claude/hooks/" + + /// Global scope — hooks live in `~/.claude/hooks/`. + static let globalPrefix = "bash ~/.claude/hooks/" + } + // MARK: - MCP Scopes enum MCPScope { diff --git a/Sources/mcs/Core/Settings.swift b/Sources/mcs/Core/Settings.swift index 49e5b70b..c0b89993 100644 --- a/Sources/mcs/Core/Settings.swift +++ b/Sources/mcs/Core/Settings.swift @@ -151,6 +151,32 @@ struct Settings: Codable { removeHookEntry(event: event.rawValue, command: command) } + /// Where a hook command sits in the tree: the event it is registered under, and the matcher + /// of the group holding it. `matcher` lives on the group, not the entry. + struct HookPlacement { + let event: String + let matcher: String? + } + + /// Indexes every registered hook command by the placements it appears in, in a single pass. + /// + /// A command can legitimately appear more than once — `addHookEntry` keys on a group's first + /// entry and appends rather than replaces when it finds no match — so placements are collected + /// rather than collapsed. + func hookPlacementsByCommand() -> [String: [HookPlacement]] { + var index: [String: [HookPlacement]] = [:] + for (event, groups) in hooks ?? [:] { + for group in groups { + for entry in group.hooks ?? [] { + guard let command = entry.command else { continue } + index[command, default: []] + .append(HookPlacement(event: event, matcher: group.matcher)) + } + } + } + return index + } + // MARK: - Deep Merge /// Merge `other` into `self`, preserving existing user values. diff --git a/Sources/mcs/Doctor/CoreDoctorChecks.swift b/Sources/mcs/Doctor/CoreDoctorChecks.swift index 3a2fb349..12ed573f 100644 --- a/Sources/mcs/Doctor/CoreDoctorChecks.swift +++ b/Sources/mcs/Doctor/CoreDoctorChecks.swift @@ -366,12 +366,51 @@ struct ProjectIndexCheck: DoctorCheck { } } -/// Verifies that pack-contributed hook commands are still present in the settings file. +/// A hook command a pack contributed, paired with what the pack declared for it. +struct ExpectedHook { + let command: String + + /// Declared registration when the command traces back to a component with a + /// `HookRegistration`. nil when there is nothing to compare against — hooks a pack ships + /// through `settingsFile:` rather than a `hook:` component, or state files written before + /// registration verification existed. A nil registration means presence-only verification. + let registration: HookRegistration? +} + +/// Verifies that pack-contributed hook commands are still present in the settings file, and that +/// they are registered the way the declaring component said they should be. +/// +/// A hook whose matcher does not match any tool Claude Code emits installs cleanly, registers, and +/// fires for nothing — the symptom is an empty log, which reads as "no problems" rather than +/// "never ran". Comparing the installed `event` and `matcher` against the component's +/// `HookRegistration` turns that silence into a reported finding. +/// +/// Absence is a failure; a registration that differs from what was declared is a warning, because +/// `Settings.addHookEntry` rewrites a differing matcher on the next sync — so "run 'mcs sync'" is +/// a remedy that actually works, and a user who narrowed a matcher deliberately is not blocked. +/// +/// This proves the declared matcher reached settings, **not** that it matches any tool name Claude +/// Code actually emits. Only a real session transcript proves that. struct HookSettingsCheck: DoctorCheck { - let commands: [String] + let expectations: [ExpectedHook] let settingsPath: URL let packName: String + init(expectations: [ExpectedHook], settingsPath: URL, packName: String) { + self.expectations = expectations + self.settingsPath = settingsPath + self.packName = packName + } + + /// Presence-only verification, for callers with no declaration to compare against. + init(commands: [String], settingsPath: URL, packName: String) { + self.init( + expectations: commands.map { ExpectedHook(command: $0, registration: nil) }, + settingsPath: settingsPath, + packName: packName + ) + } + var name: String { "Hook entries (\(packName))" } @@ -390,21 +429,79 @@ struct HookSettingsCheck: DoctorCheck { } catch { return .fail("cannot read settings: \(error.localizedDescription)") } - let allCommands = (settings.hooks ?? [:]).values - .flatMap(\.self) - .compactMap(\.hooks) - .flatMap(\.self) - .compactMap(\.command) - let commandSet = Set(allCommands) - let missing = commands.filter { !commandSet.contains($0) } - if missing.isEmpty { - return .pass("all hook commands present") + + // One traversal of the hook tree, then a lookup per expectation. + let placementsByCommand = settings.hookPlacementsByCommand() + var missing: [String] = [] + var drift: [String] = [] + for expectation in expectations { + let placements = placementsByCommand[expectation.command] ?? [] + guard !placements.isEmpty else { + missing.append(expectation.command) + continue + } + if let registration = expectation.registration, + let finding = driftFinding( + command: expectation.command, + registration: registration, + placements: placements + ) { + drift.append(finding) + } + } + + if !missing.isEmpty { + // Append drift so a failure never swallows findings from other expectations. + let details = ["missing hook commands: \(missing.joined(separator: ", "))"] + drift + return .fail(details.joined(separator: "; ")) + } + if !drift.isEmpty { + return .warn("\(drift.joined(separator: "; ")) — run 'mcs sync'") } - return .fail("missing hook commands: \(missing.joined(separator: ", "))") + let verified = expectations.contains { $0.registration != nil } + return .pass(verified ? "all hook commands registered as declared" : "all hook commands present") } func fix() -> FixResult { - .notFixable("Run 'mcs sync' to restore hook entries") + .notFixable("Run 'mcs sync' to restore or update hook entries") + } + + // MARK: - Helpers + + /// Describes how `placements` differ from what was declared, or nil when they satisfy it. + /// + /// Extra registrations are not policed — a command may appear under events the pack never + /// declared, which is the user's business. Only the declared registration must be present. + private func driftFinding( + command: String, + registration: HookRegistration, + placements: [Settings.HookPlacement] + ) -> String? { + let expectedEvent = registration.event.rawValue + let underEvent = placements.filter { $0.event == expectedEvent } + guard !underEvent.isEmpty else { + let actual = Set(placements.map(\.event)).sorted().joined(separator: ", ") + return "'\(command)' is registered under \(actual), pack declares \(expectedEvent)" + } + // 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: ", ") + 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)'" } } diff --git a/Sources/mcs/Doctor/DoctorRunner.swift b/Sources/mcs/Doctor/DoctorRunner.swift index 72be5c2c..29017341 100644 --- a/Sources/mcs/Doctor/DoctorRunner.swift +++ b/Sources/mcs/Doctor/DoctorRunner.swift @@ -42,6 +42,18 @@ struct DoctorRunner { let label: String let artifactsByPack: [String: PackArtifactRecord] + /// Prefix the hook commands in `artifactsByPack` were recorded with. + /// + /// Relies on an invariant every `CheckScope` construction path upholds: a non-nil + /// `effectiveProjectRoot` means the records came from project state, which sync wrote + /// with the project prefix. Global-only packs get their own scope rather than being + /// folded into a project one, so the two never mix. + var hookCommandPrefix: String { + effectiveProjectRoot != nil + ? Constants.HookCommand.projectPrefix + : Constants.HookCommand.globalPrefix + } + func makeCollisionContext(environment: Environment) -> (any CollisionFilesystemContext)? { let trackedFiles = PackArtifactRecord.allTrackedFiles(from: artifactsByPack.values) if let root = effectiveProjectRoot { @@ -474,7 +486,7 @@ struct DoctorRunner { if !artifacts.hookCommands.isEmpty { checks.append(( check: HookSettingsCheck( - commands: artifacts.hookCommands, + expectations: hookExpectations(for: artifacts, pack: pack, scope: scope), settingsPath: settingsPath, packName: pack.displayName ), @@ -518,6 +530,30 @@ struct DoctorRunner { return checks } + /// Pairs each recorded hook command with the `HookRegistration` of the component that declared + /// it, so the check can verify *how* the hook was registered and not merely that it exists. + /// + /// The join key is the command string, rebuilt with `hookCommand(prefix:)` — the same helper + /// sync used to write it. `pack` is already collision-resolved here, so namespaced + /// destinations match what sync actually installed. Commands with no matching component + /// (hooks supplied via `settingsFile:`, or state files predating this check) get a nil + /// registration and keep presence-only semantics. + private func hookExpectations( + for artifacts: PackArtifactRecord, + pack: any TechPack, + scope: CheckScope + ) -> [ExpectedHook] { + var registrationsByCommand: [String: HookRegistration] = [:] + for component in pack.components { + if let command = component.hookCommand(prefix: scope.hookCommandPrefix) { + registrationsByCommand[command] = component.hookRegistration + } + } + return artifacts.hookCommands.map { + ExpectedHook(command: $0, registration: registrationsByCommand[$0]) + } + } + // MARK: - Standalone checks (not tied to any component) /// Checks that cannot be derived from any ComponentDefinition. diff --git a/Sources/mcs/Sync/Configurator.swift b/Sources/mcs/Sync/Configurator.swift index ee326d6d..8c64e0d4 100644 --- a/Sources/mcs/Sync/Configurator.swift +++ b/Sources/mcs/Sync/Configurator.swift @@ -578,10 +578,7 @@ struct Configurator { } else { output.warn(" Could not remove '\(relativePath)' — will retry on next sync") } - if component.type == .hookFile, - component.hookRegistration != nil, - fileType == .hook { - let hookCmd = "\(scope.hookCommandPrefix)\(destination)" + if let hookCmd = component.hookCommand(prefix: scope.hookCommandPrefix) { artifacts.hookCommands.removeAll { $0 == hookCmd } } diff --git a/Sources/mcs/Sync/ConfiguratorSupport.swift b/Sources/mcs/Sync/ConfiguratorSupport.swift index 9d79ae54..e1b52d96 100644 --- a/Sources/mcs/Sync/ConfiguratorSupport.swift +++ b/Sources/mcs/Sync/ConfiguratorSupport.swift @@ -195,10 +195,8 @@ enum ConfiguratorSupport { for component in pack.components { guard !excluded.contains(component.id) else { continue } - if component.type == .hookFile, - let reg = component.hookRegistration, - case let .copyPackFile(_, destination, .hook) = component.installAction { - let command = "\(hookCommandPrefix)\(destination)" + if let reg = component.hookRegistration, + let command = component.hookCommand(prefix: hookCommandPrefix) { if settings.addHookEntry( event: reg.event, command: command, diff --git a/Sources/mcs/Sync/GlobalSyncStrategy.swift b/Sources/mcs/Sync/GlobalSyncStrategy.swift index 93bec6d1..b75b1904 100644 --- a/Sources/mcs/Sync/GlobalSyncStrategy.swift +++ b/Sources/mcs/Sync/GlobalSyncStrategy.swift @@ -116,10 +116,8 @@ struct GlobalSyncStrategy: SyncStrategy { let relativePath = fileRelativePath(destination: destination, fileType: fileType) artifacts.files.append(relativePath) artifacts.fileHashes.merge(result.hashes) { _, new in new } - if component.type == .hookFile, - component.hookRegistration != nil, - fileType == .hook { - artifacts.hookCommands.append("\(scope.hookCommandPrefix)\(destination)") + if let hookCommand = component.hookCommand(prefix: scope.hookCommandPrefix) { + artifacts.hookCommands.append(hookCommand) } output.success(" \(component.displayName) installed") } diff --git a/Sources/mcs/Sync/ProjectSyncStrategy.swift b/Sources/mcs/Sync/ProjectSyncStrategy.swift index 959b8447..4262c12a 100644 --- a/Sources/mcs/Sync/ProjectSyncStrategy.swift +++ b/Sources/mcs/Sync/ProjectSyncStrategy.swift @@ -93,10 +93,8 @@ struct ProjectSyncStrategy: SyncStrategy { ) artifacts.files.append(contentsOf: result.paths) artifacts.fileHashes.merge(result.hashes) { _, new in new } - if component.type == .hookFile, - component.hookRegistration != nil, - fileType == .hook { - artifacts.hookCommands.append("\(scope.hookCommandPrefix)\(destination)") + if let hookCommand = component.hookCommand(prefix: scope.hookCommandPrefix) { + artifacts.hookCommands.append(hookCommand) } if !result.paths.isEmpty { output.success(" \(component.displayName) installed") diff --git a/Sources/mcs/Sync/SyncScope.swift b/Sources/mcs/Sync/SyncScope.swift index d80cdc58..bf508475 100644 --- a/Sources/mcs/Sync/SyncScope.swift +++ b/Sources/mcs/Sync/SyncScope.swift @@ -75,7 +75,7 @@ extension SyncScope { isGlobalScope: false, syncHint: "mcs sync", labelSuffix: "", - hookCommandPrefix: "bash .claude/hooks/", + hookCommandPrefix: Constants.HookCommand.projectPrefix, fileDisplayPrefix: ".claude/" ) } @@ -95,7 +95,7 @@ extension SyncScope { isGlobalScope: true, syncHint: "mcs sync --global", labelSuffix: " (global)", - hookCommandPrefix: "bash ~/.claude/hooks/", + hookCommandPrefix: Constants.HookCommand.globalPrefix, fileDisplayPrefix: "~/.claude/" ) } diff --git a/Sources/mcs/TechPack/Component.swift b/Sources/mcs/TechPack/Component.swift index d2861a34..8524cef2 100644 --- a/Sources/mcs/TechPack/Component.swift +++ b/Sources/mcs/TechPack/Component.swift @@ -118,6 +118,24 @@ struct ComponentDefinition: Identifiable { #endif } +extension ComponentDefinition { + /// The settings `command` string this component registers, or nil when it registers no hook. + /// + /// Single source of truth for the sync↔doctor join: sync writes this string into + /// `settings.local.json` and records it in `PackArtifactRecord.hookCommands`, and doctor + /// rebuilds it to match a recorded command back to the component that declared it. Both sides + /// must build it identically — a divergence makes every hook look missing. + /// + /// - Parameter prefix: Scope-dependent prefix, `Constants.HookCommand.projectPrefix` or + /// `.globalPrefix`. + func hookCommand(prefix: String) -> String? { + guard type == .hookFile, hookRegistration != nil, + case let .copyPackFile(_, destination, .hook) = installAction + else { return nil } + return prefix + destination + } +} + /// How to install a component enum ComponentInstallAction { case mcpServer(MCPServerConfig) diff --git a/Tests/MCSTests/ComponentTests.swift b/Tests/MCSTests/ComponentTests.swift new file mode 100644 index 00000000..b8bf8424 --- /dev/null +++ b/Tests/MCSTests/ComponentTests.swift @@ -0,0 +1,84 @@ +import Foundation +@testable import mcs +import Testing + +// MARK: - hookCommand(prefix:) + +struct ComponentHookCommandTests { + private func makeComponent( + type: ComponentType, + hookRegistration: HookRegistration?, + installAction: ComponentInstallAction + ) -> ComponentDefinition { + ComponentDefinition( + id: "test", + displayName: "Test", + description: "test", + type: type, + packIdentifier: nil, + dependencies: [], + isRequired: false, + hookRegistration: hookRegistration, + installAction: installAction + ) + } + + private func hookFile(destination: String) -> ComponentInstallAction { + .copyPackFile( + source: URL(fileURLWithPath: "/tmp/\(destination)"), + destination: destination, + fileType: .hook + ) + } + + @Test("joins prefix and destination for a registered hook component") + func buildsCommandForHookComponent() { + let component = makeComponent( + type: .hookFile, + hookRegistration: HookRegistration(event: .preToolUse, matcher: "Agent"), + installAction: hookFile(destination: "gate.sh") + ) + #expect( + component.hookCommand(prefix: Constants.HookCommand.projectPrefix) + == "bash .claude/hooks/gate.sh" + ) + #expect( + component.hookCommand(prefix: Constants.HookCommand.globalPrefix) + == "bash ~/.claude/hooks/gate.sh" + ) + } + + @Test("nil when the component declares no hook registration") + func nilWithoutRegistration() { + let component = makeComponent( + type: .hookFile, + hookRegistration: nil, + installAction: hookFile(destination: "gate.sh") + ) + #expect(component.hookCommand(prefix: Constants.HookCommand.projectPrefix) == nil) + } + + @Test("nil for a non-hook file type") + func nilForNonHookFileType() { + let component = makeComponent( + type: .hookFile, + hookRegistration: HookRegistration(event: .preToolUse, matcher: nil), + installAction: .copyPackFile( + source: URL(fileURLWithPath: "/tmp/gate.sh"), + destination: "gate.sh", + fileType: .command + ) + ) + #expect(component.hookCommand(prefix: Constants.HookCommand.projectPrefix) == nil) + } + + @Test("nil for an install action that copies no file") + func nilForNonCopyAction() { + let component = makeComponent( + type: .hookFile, + hookRegistration: HookRegistration(event: .sessionStart, matcher: nil), + installAction: .shellCommand(command: "echo hi") + ) + #expect(component.hookCommand(prefix: Constants.HookCommand.projectPrefix) == nil) + } +} diff --git a/Tests/MCSTests/CoreDoctorCheckTests.swift b/Tests/MCSTests/CoreDoctorCheckTests.swift index a312e9d1..d792ff13 100644 --- a/Tests/MCSTests/CoreDoctorCheckTests.swift +++ b/Tests/MCSTests/CoreDoctorCheckTests.swift @@ -102,6 +102,222 @@ struct HookSettingsCheckTests { Issue.record("Expected .pass, got \(result)") } } + + // MARK: - Registration verification + + /// Settings holding `gate.sh` under `PreToolUse` with the given matcher. + private func makeGateSettings(matcher: String) throws -> URL { + try makeTempSettings(content: """ + { + "hooks": { + "PreToolUse": [ + { \(matcher), "hooks": [{ "type": "command", "command": "bash .claude/hooks/gate.sh" }] } + ] + } + } + """) + } + + private func gateCheck(matcher: String?, settingsPath: URL) -> HookSettingsCheck { + HookSettingsCheck( + expectations: [ExpectedHook( + command: "bash .claude/hooks/gate.sh", + registration: HookRegistration(event: .preToolUse, matcher: matcher) + )], + settingsPath: settingsPath, + packName: "test-pack" + ) + } + + @Test("pass when declared event and matcher both match") + func passWhenRegistrationMatches() throws { + let url = try makeGateSettings(matcher: "\"matcher\": \"Agent|Task\"") + defer { try? FileManager.default.removeItem(at: url) } + + let result = gateCheck(matcher: "Agent|Task", settingsPath: url).check() + if case .pass = result { + // expected + } else { + Issue.record("Expected .pass, got \(result)") + } + } + + @Test("warn when matcher differs, naming both actual and declared") + func warnWhenMatcherDiffers() throws { + let url = try makeGateSettings(matcher: "\"matcher\": \"Task\"") + defer { try? FileManager.default.removeItem(at: url) } + + let result = gateCheck(matcher: "Agent|Task", settingsPath: url).check() + if case let .warn(msg) = result { + #expect(msg.contains("'Task'")) + #expect(msg.contains("'Agent|Task'")) + #expect(msg.contains("mcs sync")) + } else { + Issue.record("Expected .warn, got \(result)") + } + } + + @Test("warn when registered under a different event than declared") + func warnWhenEventDiffers() throws { + let url = try makeTempSettings(content: """ + { + "hooks": { + "PostToolUse": [ + { "matcher": "Agent", "hooks": [{ "type": "command", "command": "bash .claude/hooks/gate.sh" }] } + ] + } + } + """) + defer { try? FileManager.default.removeItem(at: url) } + + let result = gateCheck(matcher: "Agent", settingsPath: url).check() + if case let .warn(msg) = result { + #expect(msg.contains("PostToolUse")) + #expect(msg.contains("PreToolUse")) + } else { + Issue.record("Expected .warn, got \(result)") + } + } + + @Test("warn when no matcher was declared but settings has one") + func warnWhenUndeclaredMatcherPresent() throws { + let url = try makeGateSettings(matcher: "\"matcher\": \"Agent\"") + defer { try? FileManager.default.removeItem(at: url) } + + let result = gateCheck(matcher: nil, settingsPath: url).check() + if case let .warn(msg) = result { + #expect(msg.contains("no matcher")) + } else { + Issue.record("Expected .warn, got \(result)") + } + } + + @Test("pass when no matcher was declared and settings has none") + func passWhenNoMatcherEitherSide() throws { + let url = try makeTempSettings(content: """ + { + "hooks": { + "PreToolUse": [ + { "hooks": [{ "type": "command", "command": "bash .claude/hooks/gate.sh" }] } + ] + } + } + """) + defer { try? FileManager.default.removeItem(at: url) } + + let result = gateCheck(matcher: nil, settingsPath: url).check() + if case .pass = result { + // expected + } else { + Issue.record("Expected .pass, got \(result)") + } + } + + @Test("empty matcher in settings is equivalent to no matcher declared") + func passWhenEmptyMatcherNormalizes() throws { + let url = try makeGateSettings(matcher: "\"matcher\": \"\"") + defer { try? FileManager.default.removeItem(at: url) } + + let result = gateCheck(matcher: nil, settingsPath: url).check() + if case .pass = result { + // expected + } else { + Issue.record("Expected .pass, got \(result)") + } + } + + @Test("pass when one of several groups under the declared event matches") + func passWhenAnyGroupMatches() throws { + // addHookEntry appends rather than replaces when the command is not the group's first + // entry, so a correct registration can coexist with a stale one. + let url = try makeTempSettings(content: """ + { + "hooks": { + "PreToolUse": [ + { "matcher": "Task", "hooks": [ + { "type": "command", "command": "bash .claude/hooks/other.sh" }, + { "type": "command", "command": "bash .claude/hooks/gate.sh" } + ] }, + { "matcher": "Agent|Task", "hooks": [{ "type": "command", "command": "bash .claude/hooks/gate.sh" }] } + ] + } + } + """) + defer { try? FileManager.default.removeItem(at: url) } + + let result = gateCheck(matcher: "Agent|Task", settingsPath: url).check() + if case .pass = result { + // expected + } else { + Issue.record("Expected .pass, got \(result)") + } + } + + @Test("extra registration under an undeclared event is not policed") + func passWhenExtraEventPresent() throws { + let url = try makeTempSettings(content: """ + { + "hooks": { + "PreToolUse": [ + { "matcher": "Agent", "hooks": [{ "type": "command", "command": "bash .claude/hooks/gate.sh" }] } + ], + "PostToolUse": [ + { "matcher": "Bash", "hooks": [{ "type": "command", "command": "bash .claude/hooks/gate.sh" }] } + ] + } + } + """) + defer { try? FileManager.default.removeItem(at: url) } + + let result = gateCheck(matcher: "Agent", settingsPath: url).check() + if case .pass = result { + // expected + } else { + Issue.record("Expected .pass, got \(result)") + } + } + + @Test("a nil registration verifies presence only, whatever the matcher") + func passWhenNoRegistrationDeclared() throws { + let url = try makeGateSettings(matcher: "\"matcher\": \"anything\"") + defer { try? FileManager.default.removeItem(at: url) } + + let check = HookSettingsCheck( + expectations: [ExpectedHook(command: "bash .claude/hooks/gate.sh", registration: nil)], + settingsPath: url, + packName: "test-pack" + ) + if case .pass = check.check() { + // expected + } else { + Issue.record("Expected .pass for presence-only expectation") + } + } + + @Test("failure message reports drift from other expectations too") + func failMessageIncludesDrift() throws { + let url = try makeGateSettings(matcher: "\"matcher\": \"Task\"") + defer { try? FileManager.default.removeItem(at: url) } + + let check = HookSettingsCheck( + expectations: [ + ExpectedHook( + command: "bash .claude/hooks/gate.sh", + registration: HookRegistration(event: .preToolUse, matcher: "Agent|Task") + ), + ExpectedHook(command: "bash .claude/hooks/absent.sh", registration: nil), + ], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + if case let .fail(msg) = result { + #expect(msg.contains("absent.sh")) + #expect(msg.contains("'Agent|Task'")) + } else { + Issue.record("Expected .fail, got \(result)") + } + } } // MARK: - SettingsKeysCheck diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index f65a8829..c0e2c050 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -331,6 +331,55 @@ struct SinglePackLifecycleTests { #expect(!removedContent.contains("")) } } + + @Test("Hand-edited hook matcher is reported as drift and healed by re-sync") + func hookMatcherDriftLifecycle() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let hookSource = try bed.makeHookSource(name: "gate.sh") + let pack = MockTechPack( + identifier: "gate-pack", + displayName: "Gate Pack", + components: [ + bed.hookComponent( + pack: "gate-pack", id: "gate-hook", + source: hookSource, destination: "gate.sh", + hookRegistration: HookRegistration(event: .preToolUse, matcher: "Agent|Task") + ), + ] + ) + let registry = TechPackRegistry(packs: [pack]) + let configurator = bed.makeConfigurator(registry: registry) + + // === Step 1: Sync installs the hook with the declared matcher === + try configurator.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) + + // === Step 3: Doctor reports the drift the old presence-only check missed === + var driftRunner = bed.makeDoctorRunner(registry: registry) + let driftSummary = try driftRunner.run() + #expect(driftSummary.warnings > clean.warnings) + #expect(driftSummary.issues == clean.issues) + + // === Step 4: Re-sync heals it, which is what justifies warn over fail === + try configurator.configure(packs: [pack], confirmRemovals: false) + let healed = try Settings.load(from: bed.settingsLocalPath) + #expect(healed.hooks?["PreToolUse"]?.first?.matcher == "Agent|Task") + + var healedRunner = bed.makeDoctorRunner(registry: registry) + #expect(try healedRunner.run().warnings == clean.warnings) + } } // MARK: - Scenario 2: Multi-Pack Convergence diff --git a/docs/architecture.md b/docs/architecture.md index f4a398a1..f182918d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -278,6 +278,20 @@ MCP server checks follow the same pattern: project-scoped entries (`projects[pat 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. +### Hook Registration Verification + +A hook whose matcher names a tool Claude Code never emits installs cleanly, registers in settings, and fires for nothing. The symptom is an *empty* log, which reads as "no problems found" rather than "never ran" — so `HookSettingsCheck` verifies not just that each pack-contributed hook command is present, but that it is registered the way the declaring component said it should be. + +The check joins the commands recorded in `PackArtifactRecord.hookCommands` back to the components that declared them, keyed on the command string that `ComponentDefinition.hookCommand(prefix:)` builds for both sides. Where a component supplied a `HookRegistration`, its `event` and `matcher` are compared against what is installed: + +- **Command absent** — `✗ fail`. The hook is not registered at all. +- **Registered under a different event, or with a different matcher** — `⚠ warn`. `Settings.addHookEntry` rewrites a differing matcher on the next sync, so `mcs sync` is a remedy that actually works, and a user who narrowed a matcher deliberately is not blocked by a red doctor. +- **No declaration to compare against** — presence-only, as before. This covers hooks a pack ships through a merged settings file rather than a `hook:` component. + +Extra registrations under events the pack never declared are not policed, and an empty matcher string is treated as an absent one. `timeout`, `async`, and `statusMessage` stay unverified — a wrong value there is cosmetic, whereas `event` and `matcher` are the two whose wrong value silently disables the hook. + +**This proves the declared matcher reached settings, not that it matches any tool Claude Code actually emits.** Only a real session transcript proves that. Tool names are harness implementation details that can change between Claude Code releases, so any matcher naming a specific tool is worth re-checking after an upgrade. + ### Pack Resolution When determining which packs to check, doctor uses a priority chain: From 6dfc97f4571d0c9a8b7b2b30f10f94d9f7e3e185 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Wed, 29 Jul 2026 10:48:12 +0200 Subject: [PATCH 2/3] ISSUE-355: Address review feedback on hook registration verification - Only claim "registered as declared" when every expectation had a declaration, so a mixed set no longer overstates what was verified - Correct the ExpectedHook doc: a nil registration means the command no longer maps to a component, not that it came from a settings file - Bind the registration explicitly when building the join map --- Sources/mcs/Doctor/CoreDoctorChecks.swift | 18 ++++++++---- Sources/mcs/Doctor/DoctorRunner.swift | 7 +++-- Tests/MCSTests/CoreDoctorCheckTests.swift | 36 +++++++++++++++++++++++ 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/Sources/mcs/Doctor/CoreDoctorChecks.swift b/Sources/mcs/Doctor/CoreDoctorChecks.swift index 12ed573f..7398d612 100644 --- a/Sources/mcs/Doctor/CoreDoctorChecks.swift +++ b/Sources/mcs/Doctor/CoreDoctorChecks.swift @@ -370,10 +370,14 @@ struct ProjectIndexCheck: DoctorCheck { struct ExpectedHook { let command: String - /// Declared registration when the command traces back to a component with a - /// `HookRegistration`. nil when there is nothing to compare against — hooks a pack ships - /// through `settingsFile:` rather than a `hook:` component, or state files written before - /// registration verification existed. A nil registration means presence-only verification. + /// Declared registration when the command still traces back to a component, which is the + /// normal case — sync only records a hook command for a component that had a + /// `HookRegistration`, so every freshly written record has one. + /// + /// nil means the recorded command no longer maps to any component in the pack: the component + /// was removed or its destination renamed since the last sync. There is nothing to compare + /// against, so the command falls back to presence-only verification rather than being + /// reported as drift. let registration: HookRegistration? } @@ -458,8 +462,10 @@ struct HookSettingsCheck: DoctorCheck { if !drift.isEmpty { return .warn("\(drift.joined(separator: "; ")) — run 'mcs sync'") } - let verified = expectations.contains { $0.registration != nil } - return .pass(verified ? "all hook commands registered as declared" : "all hook commands present") + // Claim "as declared" only when every expectation actually had a declaration — a mixed set + // would otherwise assert verification the presence-only entries never got. + let allDeclared = !expectations.isEmpty && expectations.allSatisfy { $0.registration != nil } + return .pass(allDeclared ? "all hook commands registered as declared" : "all hook commands present") } func fix() -> FixResult { diff --git a/Sources/mcs/Doctor/DoctorRunner.swift b/Sources/mcs/Doctor/DoctorRunner.swift index 29017341..fabfca84 100644 --- a/Sources/mcs/Doctor/DoctorRunner.swift +++ b/Sources/mcs/Doctor/DoctorRunner.swift @@ -545,8 +545,11 @@ struct DoctorRunner { ) -> [ExpectedHook] { var registrationsByCommand: [String: HookRegistration] = [:] for component in pack.components { - if let command = component.hookCommand(prefix: scope.hookCommandPrefix) { - registrationsByCommand[command] = component.hookRegistration + // Bind the registration explicitly rather than relying on `hookCommand(prefix:)` + // already having required one — same shape the sync-side join uses. + if let registration = component.hookRegistration, + let command = component.hookCommand(prefix: scope.hookCommandPrefix) { + registrationsByCommand[command] = registration } } return artifacts.hookCommands.map { diff --git a/Tests/MCSTests/CoreDoctorCheckTests.swift b/Tests/MCSTests/CoreDoctorCheckTests.swift index d792ff13..ed31a500 100644 --- a/Tests/MCSTests/CoreDoctorCheckTests.swift +++ b/Tests/MCSTests/CoreDoctorCheckTests.swift @@ -294,6 +294,42 @@ struct HookSettingsCheckTests { } } + @Test("a mixed set does not claim every command was verified against a declaration") + func mixedSetDoesNotOverclaim() throws { + let url = try makeTempSettings(content: """ + { + "hooks": { + "PreToolUse": [ + { "matcher": "Agent", "hooks": [{ "type": "command", "command": "bash .claude/hooks/gate.sh" }] } + ], + "PostToolUse": [ + { "hooks": [{ "type": "command", "command": "bash .claude/hooks/orphan.sh" }] } + ] + } + } + """) + defer { try? FileManager.default.removeItem(at: url) } + + // The second command no longer maps to a component, so it can only be checked for presence. + let check = HookSettingsCheck( + expectations: [ + ExpectedHook( + command: "bash .claude/hooks/gate.sh", + registration: HookRegistration(event: .preToolUse, matcher: "Agent") + ), + ExpectedHook(command: "bash .claude/hooks/orphan.sh", registration: nil), + ], + settingsPath: url, + packName: "test-pack" + ) + let result = check.check() + if case let .pass(msg) = result { + #expect(!msg.contains("as declared")) + } else { + Issue.record("Expected .pass, got \(result)") + } + } + @Test("failure message reports drift from other expectations too") func failMessageIncludesDrift() throws { let url = try makeGateSettings(matcher: "\"matcher\": \"Task\"") From cc8b57d270be2213b516bc12cd1934a2269431d2 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Wed, 29 Jul 2026 10:49:44 +0200 Subject: [PATCH 3/3] ISSUE-355: Correct what a missing hook declaration actually means - The architecture doc claimed presence-only checking covers hooks shipped via a merged settings file; those never enter the artifact record at all, so the case is a stale component reference instead --- docs/architecture.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture.md b/docs/architecture.md index f182918d..ec4048e2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -286,7 +286,7 @@ The check joins the commands recorded in `PackArtifactRecord.hookCommands` back - **Command absent** — `✗ fail`. The hook is not registered at all. - **Registered under a different event, or with a different matcher** — `⚠ warn`. `Settings.addHookEntry` rewrites a differing matcher on the next sync, so `mcs sync` is a remedy that actually works, and a user who narrowed a matcher deliberately is not blocked by a red doctor. -- **No declaration to compare against** — presence-only, as before. This covers hooks a pack ships through a merged settings file rather than a `hook:` component. +- **No declaration to compare against** — presence-only, as before. This means the recorded command no longer maps to any component in the pack: it was removed, or its destination renamed, since the last sync. Extra registrations under events the pack never declared are not policed, and an empty matcher string is treated as an absent one. `timeout`, `async`, and `statusMessage` stay unverified — a wrong value there is cosmetic, whereas `event` and `matcher` are the two whose wrong value silently disables the hook.