ISSUE-355: Assert hook matcher and command in hookEventExists - #360
Merged
Conversation
- 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
There was a problem hiding this comment.
Pull request overview
This PR improves doctor verification for hooks registered via pack-supplied settings files by enhancing hookEventExists to optionally assert the expected hook group matcher and a hook command substring, and by surfacing previously-silent hook group drops during settings composition.
Changes:
- Extend
hookEventExistsdoctor checks with optionalmatcherandcommand(substring) assertions, warning (not failing) on mismatches while still failing on missing registrations. - Make settings composition explicitly prefer derived (component-declared) hook registrations over settings-file copies, and warn when settings-merge dedup drops a group due to a command collision with a differing matcher.
- Add shared matcher normalization/formatting helpers, plus unit/integration test coverage and schema documentation for the new behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/MCSTests/SettingsMergeTests.swift | Adds tests covering hook-group dedup behavior and conflict reporting during settings merges. |
| Tests/MCSTests/PackHeuristicsTests.swift | Adds tests for warnings when matcher is declared on doctor-check types that ignore it. |
| Tests/MCSTests/LifecycleIntegrationTests.swift | Adds lifecycle coverage for settings-file hook matcher assertions and deterministic precedence vs component-derived hooks. |
| Tests/MCSTests/ExternalPackManifestTests.swift | Adds validation tests for hookEventExists matcher/command fields. |
| Tests/MCSTests/ExternalDoctorCheckTests.swift | Verifies factory wiring of matcher and command into the hookEventExists check. |
| Tests/MCSTests/CoreDoctorCheckSandboxTests.swift | Adds sandbox tests for matcher/command assertion semantics and precedence of project vs global settings. |
| Sources/mcs/Sync/ConfiguratorSupport.swift | Changes settings composition to two-pass merge and emits warnings for dropped conflicting hook groups. |
| Sources/mcs/ExternalPack/PackHeuristics.swift | Adds heuristic warning when matcher is declared on doctor checks that don’t consult it. |
| Sources/mcs/ExternalPack/ExternalPackManifest.swift | Validates non-empty matcher/command for hookEventExists and adds an explicit init with defaults to keep construction sites stable. |
| Sources/mcs/ExternalPack/ExternalDoctorCheck.swift | Implements matcher/command assertions for hookEventExists and updates the factory to forward the new fields. |
| Sources/mcs/Doctor/CoreDoctorChecks.swift | Switches matcher normalization/description to shared helper functions. |
| Sources/mcs/Core/Settings.swift | Makes merge(with:) return dropped hook-group conflicts and introduces shared matcher normalization/description helpers. |
| docs/techpack-schema.md | Documents hookEventExists matcher/command semantics and limitations. |
Comments suppressed due to low confidence (1)
Sources/mcs/ExternalPack/ExternalDoctorCheck.swift:350
isOptionalis not honored when no settings file exists: the check always returns.fail("no settings file found...")even ifisOptional == true. This contradicts the documented semantics that optional checks downgrade absence to a skip, and it makes optional hook assertions fail on fresh installs where settings haven’t been created yet.
guard probe.anyFileExisted else {
return .fail("no settings file found (searched \(searchedFileNames))")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A hook whose matcher matches nothing installs cleanly, registers, passes doctor, and never fires — the only symptom is an empty log, which reads as "nothing wrong" rather than "never ran." Doctor already derives event/matcher verification for hooks declared as components, but a pack can also ship hooks inside a settings file, and those are invisible to that mechanism: they never enter the artifact record and are excluded from the per-pack settings hash. This adds the one form of verification available for that shape, and fixes two silent behaviors in settings composition that a strict matcher assertion would otherwise trip over.
Closes #355.
Changes
hookEventExistsgains optionalmatcherandcommandassertions. Matchers compare as raw strings — the regex is not interpreted, since Claude Code evaluates it at runtime. When both are given they must be satisfied by the same hook group. A mismatch warns rather than fails: the registration exists, and a narrowed matcher may be deliberate. Absence still fails.techpack.yaml.Documented alongside the schema: these fields prove the declared matcher reached the settings file, not that it matches a tool Claude Code actually emits. Only a session transcript settles that.
Test plan
swift testpasses locallyswiftformat --lint .andswiftlintpass without violationssettingsFile:and declaresmatcher:on ahookEventExistscheck: runmcs sync, thenmcs doctor→ expect pass. Edit the matcher in.claude/settings.local.jsonby hand, re-runmcs doctor→ expect a warning naming both the declared and the found matcher, and no failure.hook:component and an entry in its settings file, under differing matchers: runmcs sync→ expect a warning that the settings-file group was not merged, and the component's matcher on disk.Checklist for engine changes
LifecycleIntegrationTestsorDoctorRunnerIntegrationTests)CLAUDE.md,docs/,techpack.yamlschema inExternalPackManifest.swift)