Skip to content

ISSUE-355: Verify hook event and matcher against the declaring component - #359

Merged
bguidolim merged 3 commits into
mainfrom
bruno/ISSUE-355-hook-matcher-verification
Jul 29, 2026
Merged

ISSUE-355: Verify hook event and matcher against the declaring component#359
bguidolim merged 3 commits into
mainfrom
bruno/ISSUE-355-hook-matcher-verification

Conversation

@bguidolim

@bguidolim bguidolim commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

A hook whose matcher names a tool Claude Code never emits installs cleanly, registers in settings, passes doctor, and then fires for nothing. The symptom is an empty log, which reads as "no problems found" rather than "never ran". Doctor previously checked only that a pack's hook command string appeared somewhere in the settings file, so it could not see this class of failure; it now also checks that the hook is registered under the event and matcher the pack declared.

This deliberately does not implement what issue #355 proposed (optional matcher: / command: fields on the hookEventExists doctor check). Deriving the assertion from the registration a pack already declares covers every pack with no manifest change and no second copy of the matcher to drift. The issue stays open, rescoped to the case this cannot reach — hooks a pack supplies through a merged settings file, where there is no declaration to compare against. Reasoning is recorded in a comment on #355.

Changes

  • Doctor warns when a pack's hook is registered under a different event or matcher than the pack declared, naming both the installed and the declared value. A hook that is missing outright still fails.
  • Drift warns rather than fails because the next sync rewrites a differing matcher — so the suggested remedy actually resolves it, and a deliberately narrowed matcher does not turn doctor red.
  • A recorded hook command whose component was removed or renamed since the last sync falls back to presence-only checking rather than being reported as drift.
  • A command may sit in more than one group or under extra undeclared events without being reported; only the declared registration has to be present.

Test plan

  • swift test passes locally
  • swiftformat --lint . and swiftlint pass without violations
  • Affected commands verified with a real pack (e.g. mcs sync, mcs doctor)

To verify manually with a pack that declares hookMatcher on a hook component:

  1. mcs sync the pack, then mcs doctor → expect the hook entries check to pass.
  2. Edit the matcher in the project's settings.local.json to a different value, then mcs doctor → expect a warning naming both the installed and declared matcher, and the summary to count it.
  3. mcs sync again, then mcs doctor → expect the warning to clear.
Checklist for engine changes
  • Integration tests updated for new features (LifecycleIntegrationTests or DoctorRunnerIntegrationTests)
  • Docs updated if behavior changed (CLAUDE.md, docs/, techpack.yaml schema in ExternalPackManifest.swift)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens mcs doctor’s hook validation by verifying that hook entries are not only present in settings, but also registered under the event and matcher declared by the component that installed them, addressing the “silent no-op hook” failure mode described in ISSUE-355.

Changes:

  • Adds a single-source-of-truth ComponentDefinition.hookCommand(prefix:) and centralizes hook command prefixes in Constants.
  • Enhances HookSettingsCheck to detect drift in hook event/matcher (warn) while still failing for missing hook commands.
  • Adds unit + integration coverage for matcher drift detection and healing via re-sync.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
Tests/MCSTests/LifecycleIntegrationTests.swift Adds lifecycle integration coverage for matcher drift → doctor warning → re-sync healing.
Tests/MCSTests/CoreDoctorCheckTests.swift Adds focused unit tests for event/matcher verification semantics and messaging.
Tests/MCSTests/ComponentTests.swift Adds unit tests for ComponentDefinition.hookCommand(prefix:) behavior.
Sources/mcs/TechPack/Component.swift Introduces hookCommand(prefix:) helper to unify sync↔doctor joining on hook command strings.
Sources/mcs/Sync/SyncScope.swift Replaces inline hook command prefixes with Constants.HookCommand values.
Sources/mcs/Sync/ProjectSyncStrategy.swift Uses hookCommand(prefix:) to record hook commands into artifacts.
Sources/mcs/Sync/GlobalSyncStrategy.swift Uses hookCommand(prefix:) to record hook commands into artifacts (global scope).
Sources/mcs/Sync/ConfiguratorSupport.swift Uses hookCommand(prefix:) when writing hook registrations into settings.
Sources/mcs/Sync/Configurator.swift Uses hookCommand(prefix:) when removing hook commands from artifact records.
Sources/mcs/Doctor/DoctorRunner.swift Builds hook expectations by joining stored hook commands back to declaring components.
Sources/mcs/Doctor/CoreDoctorChecks.swift Implements event/matcher drift detection based on declared HookRegistration.
Sources/mcs/Core/Settings.swift Adds hookPlacementsByCommand() indexing to enable efficient drift checks.
Sources/mcs/Core/Constants.swift Adds HookCommand prefix constants for project/global scope.
docs/architecture.md Documents the new hook registration verification behavior and its intended limitations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Sources/mcs/Doctor/DoctorRunner.swift
Comment thread Sources/mcs/Doctor/CoreDoctorChecks.swift Outdated
- 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()
- 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
- 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
@bguidolim
bguidolim force-pushed the bruno/ISSUE-355-hook-matcher-verification branch from be6eb4a to cc8b57d Compare July 29, 2026 09:01
@bguidolim
bguidolim merged commit ac96d0f into main Jul 29, 2026
7 of 8 checks passed
@bguidolim
bguidolim deleted the bruno/ISSUE-355-hook-matcher-verification branch July 29, 2026 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants