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..7398d612 100644 --- a/Sources/mcs/Doctor/CoreDoctorChecks.swift +++ b/Sources/mcs/Doctor/CoreDoctorChecks.swift @@ -366,12 +366,55 @@ 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 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? +} + +/// 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 +433,81 @@ 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: ", "))") + // 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 { - .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..fabfca84 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,33 @@ 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 { + // 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 { + 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..ed31a500 100644 --- a/Tests/MCSTests/CoreDoctorCheckTests.swift +++ b/Tests/MCSTests/CoreDoctorCheckTests.swift @@ -102,6 +102,258 @@ 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("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\"") + 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..ec4048e2 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 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. + +**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: