fix(hooks): compose per-file routing with dependency targets#2259
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a hooks integration regression where specifying a per-dependency targets: subset disabled per-file hook routing, causing per-target hook files to be merged into every active target and cross-contaminate tool configs.
Changes:
- Always apply per-file hook routing in the Copilot hook integrator path (no longer gated by
dep_targets_active). - Always apply per-file hook routing in the merged-hook targets path (Claude/Cursor/Codex/Gemini/Windsurf/Antigravity).
- Always apply per-file hook routing in the Kiro hook integrator path, plus add unit regression tests for divergent per-target hook files under dep-level target subsets.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/unit/integration/test_dep_target_intersection.py | Adds parametrized regression tests ensuring divergent per-target hook files route only to their intended merge-based harness when dep_target_subset is set. |
| src/apm_cli/integration/kiro_hook_integrator.py | Removes the dep_targets_active gate so filename-based per-file routing always applies for Kiro. |
| src/apm_cli/integration/hook_integrator.py | Removes the dep_targets_active gate so filename-based per-file routing always applies for Copilot and merge-based targets. |
| CHANGELOG.md | Documents the bug fix under Fixed (needs an issue/PR reference suffix for consistency). |
| - A per-dependency `targets:` list no longer disables per-file hook target | ||
| routing. Previously, setting `targets:` flipped an internal | ||
| `dep_targets_active` gate that skipped filename-suffix routing entirely, so a | ||
| package shipping divergent per-target hook files (e.g. | ||
| `pkg-claude-hooks.json` with a `Skill` matcher and `pkg-codex-hooks.json` with | ||
| an `apply_patch` matcher) merged every file into every active target -- | ||
| cross-contaminating each tool's config and duplicating shared entries. Routing | ||
| now always runs per active target, so `targets:` only narrows which targets | ||
| are active while each divergent file still lands solely in its intended tool. |
| hook_files = self.find_hook_files(package_info.install_path) | ||
| package_name = self._get_package_name(package_info, project_root) | ||
| if not dep_targets_active: | ||
| hook_files = _filter_hook_files_for_target( | ||
| hook_files, | ||
| "copilot", | ||
| package_name=package_name, | ||
| warned_packages=self._deprecated_hook_routing_warnings, | ||
| package_identity=package_info.get_canonical_dependency_string(), | ||
| ) | ||
| # Per-file target routing always runs. A dep-level ``targets:`` list | ||
| # restricts WHICH targets are active (upstream in services.py); it must | ||
| # not disable per-file routing here, or divergent per-target files | ||
| # (e.g. ``pkg-claude-hooks.json`` vs ``pkg-codex-hooks.json``) would all | ||
| # merge into every active target -- cross-contaminating configs and | ||
| # duplicating shared entries (microsoft/apm#2020 regression class). | ||
| hook_files = _filter_hook_files_for_target( | ||
| hook_files, | ||
| "copilot", | ||
| package_name=package_name, | ||
| warned_packages=self._deprecated_hook_routing_warnings, | ||
| package_identity=package_info.get_canonical_dependency_string(), | ||
| ) |
| hook_files = integrator.find_hook_files(package_info.install_path) | ||
| package_name = integrator._get_package_name(package_info, project_root) | ||
| if not dep_targets_active: | ||
| hook_files = _filter_hook_files_for_target( | ||
| hook_files, | ||
| "kiro", | ||
| package_name=package_name, | ||
| warned_packages=integrator._deprecated_hook_routing_warnings, | ||
| package_identity=package_info.get_canonical_dependency_string(), | ||
| ) | ||
| # Per-file target routing always runs; a dep-level ``targets:`` list narrows | ||
| # the active target set upstream but must not disable per-file routing (see | ||
| # HookIntegrator.integrate_package_hooks for the full rationale). | ||
| hook_files = _filter_hook_files_for_target( | ||
| hook_files, | ||
| "kiro", | ||
| package_name=package_name, | ||
| warned_packages=integrator._deprecated_hook_routing_warnings, | ||
| package_identity=package_info.get_canonical_dependency_string(), | ||
| ) |
35060c1 to
549a641
Compare
|
Thanks for the review. Addressed all three points:
Both new tests fail on the pre-fix source and pass with the fix (verified). In |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Canonical owner composition is correct; only a later signature-cleanup nit remains. |
| CLI Logging Expert | 0 | 0 | 0 | No CLI output or diagnostic surface changed. |
| DevX UX Expert | 0 | 0 | 0 | Restores intuitive package-manager target semantics. |
| Supply Chain Security Expert | 0 | 0 | 0 | Always-on per-file routing improves hook-harness isolation. |
| OSS Growth Hacker | 0 | 0 | 0 | Community contribution is now credited in the changelog. |
| Doc Writer | 0 | 0 | 0 | apm-guide now reflects target plus filename routing intersection. |
| Test Coverage Expert | 0 | 0 | 0 | Exact-head regression coverage passed for merged harnesses, Copilot, and Kiro. |
| Performance Expert | 0 | 0 | 0 | The always-on filename filter is O(F) over tiny hook-file sets. |
B = blocking-severity findings, R = recommended, N = nits. Counts are signal strength, not gates. The maintainer ships.
Folded in this run
- (panel) Update apm-guide hook routing guidance so per-dependency targets compose with filename routing instead of bypassing it -- resolved in
48f56bbaacc76f208635217e5c7f73f33f465b99. - (panel) Credit community contributor @srobroek in the changelog entry for the fix -- resolved in
48f56bbaacc76f208635217e5c7f73f33f465b99. - (owner gate) Add static and architecture guardrails preventing
dep_targets_activefrom gating_filter_hook_files_for_targetagain -- resolved inceab8a96b9493be67f858cb7e4b29b31a4b1e65a.
Copilot signals reviewed
Copilot posted no inline comments to classify.
Regression-trap evidence (mutation-break gate)
tests/unit/integration/test_dep_target_intersection.py-- restored the olddep_targets_activegate at Copilot, merged-target, and Kiro sites; test FAILED as expected; guard restored.tests/integration/test_architecture_authorities.py::test_hook_file_routing_dep_targets_gate_has_static_guard-- deleted the static guard block; test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/, uv run --extra dev ruff format --check src/ tests/, pylint R0801, scripts/lint-auth-signals.sh, and scripts/lint-architecture-boundaries.sh all exited 0 before push.
CI
gh pr view 2259 --json statusCheckRollup at ceab8a96b9493be67f858cb7e4b29b31a4b1e65a reports license/cla SUCCESS; no other checks were reported for this fork PR (after 0 CI fix iterations).
Mergeability status
Captured from gh pr view 2259 --repo microsoft/apm --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2259 | ceab8a9 |
ship_now | 1 | 3 | 0 | 1 | green | MERGEABLE | BLOCKED | awaiting required review |
Convergence
1 outer iteration; 1 Copilot round. Final panel verdict: ship_now.
Ready for maintainer review.
Full per-persona findings
Python Architect
- [nit] Copilot helper still accepts
dep_targets_activeafter the gate removal atsrc/apm_cli/integration/hook_integrator.py:1135.
The parameter is vestigial inintegrate_package_hooksitself, but remains part of the broader dispatch contract and has no correctness impact.
All other active personas returned no findings. Auth Expert was inactive because no auth surface changed.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Address shepherd review follow-ups for PR microsoft#2259 by removing vestigial dep_targets_active plumbing from hook leaf integrations, replacing the fragile line-window architecture guard with an AST checker, and clarifying the user-facing hook-routing docs and changelog entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Clean owner extension: dep-level target gating and per-file routing stay separate, with behavioral plus static guardrails. |
| CLI Logging Expert | 0 | 0 | 0 | No CLI output regression; deprecation warnings now surface correctly for target-suffixed hook files. |
| DevX UX Expert | 0 | 0 | 0 | UX semantics match least-surprise intersection behavior; docs-site wording now says so. |
| Supply Chain Security Expert | 0 | 0 | 0 | Security-positive containment fix; hooks no longer leak into unintended target configs. |
| OSS Growth Hacker | 0 | 0 | 0 | Changelog is symptom-led, credits the community contributor, and links issue plus PR. |
| Doc Writer | 0 | 0 | 0 | Docs accurately state dependency targets narrow the active set while filename routing still applies. |
| Test Coverage Expert | 0 | 0 | 0 | Regression and architecture guard evidence passed on exact head. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
Code is converged. Maintainer action is needed to approve the contributor-fork GitHub Actions runs before this PR can show observed-green CI.
Folded in this run
- (panel) Replace fragile hook-routing line-window guard with AST owner checker and fast rejection test -- resolved in 0906037.
- (panel) Remove vestigial
dep_targets_activeplumbing from hook leaf integrations after the routing fix -- resolved in 0906037. - (panel) Clarify symptom-led changelog and docs-site intersection semantics for hook filename routing with dependency targets -- resolved in 0906037.
- (panel) Broaden Copilot per-file routing coverage across every foreign target token -- resolved in 0906037.
Regression-trap evidence (mutation-break gate)
tests/unit/integration/test_dep_target_intersection.py::test_divergent_hook_files_route_per_target_under_multi_dep_targets[claude-codex]and::test_divergent_hook_files_route_single_target_under_single_dep_target[claude-codex]-- reintroduced the_integrate_merged_hooksdep_targets_activegate; tests FAILED as expected; guard restored.tests/integration/test_architecture_authorities.py::test_hook_file_routing_dep_targets_gate_has_static_guard-- removed thescripts/check_hook_file_routing_owner.pyinvocation fromscripts/lint-architecture-boundaries.sh; test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/, uv run --extra dev ruff format --check src/ tests/, pylint R0801, bash scripts/lint-auth-signals.sh, and bash scripts/lint-architecture-boundaries.sh all exited 0 on 09060370d85c5e3ea072b0b0842b88473f1d5ffd.
CI
GitHub Actions runs for CI, CodeQL, Merge Gate, Deploy Docs, NOTICE Drift Check, and Spec conformance completed with conclusion action_required at head 09060370d85c5e3ea072b0b0842b88473f1d5ffd; license/cla is SUCCESS. No CI recovery iteration was possible because the workflows require maintainer approval.
Mergeability status
Captured from gh pr view 2259 --repo microsoft/apm --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2259 | 0906037 |
ship_now | 1 | 4 | 0 | 1 | red | MERGEABLE | BLOCKED | workflow approval required |
Convergence
1 outer iteration; 1 Copilot round. Final panel stance: ship_now.
Code is ready for maintainer review once workflow approval allows CI to run.
A per-dependency `targets:` list disabled per-file hook target routing. Any `targets:` value flipped the `dep_targets_active` gate that skipped `_filter_hook_files_for_target`, so a package shipping divergent per-target hook files (e.g. `pkg-claude-hooks.json` with a `Skill` matcher and `pkg-codex-hooks.json` with an `apply_patch` matcher) merged every file into every active target -- cross-contaminating each tool's config and duplicating shared entries. Even a single-target list leaked: `targets: [codex]` gave Codex the Claude file's hooks. Remove the gate at all three call sites (Copilot, the merged-target integrator, and Kiro) so per-file routing always runs. The `targets:` list still narrows which targets are active upstream in `filter_targets_for_dependency`; per-file routing then places each file in only its intended tool. Universal files and combined `claude-codex` manifests are unaffected, and the `dep_targets_active`/`allowed_targets` chokepoint assertion is unchanged. Tests parametrize divergent-file routing across every merge-based harness pair (claude, codex, cursor, gemini, antigravity, windsurf) for both multi-target and single-target `targets:` lists, proving no cross-harness leak or duplication.
The guard removal in the parent commit also affects the Copilot per-file integrator and the Kiro integrator, but the parametrized regression tests only exercised the merged-hook registry. Add two tests driving integrate_package_primitives with a dep-level targets: list for the Copilot (.github/hooks) and Kiro (.kiro/hooks) paths, asserting each deploys only its own per-target file and does not absorb a foreign target's hook manifest. Both fail on the pre-fix source.
Fold panel documentation follow-ups for the per-file hook routing fix: credit the community author in the changelog and update apm-guide so dependency targets compose with filename routing instead of bypassing it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add the canonical-owner guard requested by the shepherd gate so dep_targets_active cannot reintroduce a bypass around per-file hook routing. The architecture assertion keeps the static lint and test suite in sync. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address shepherd review follow-ups for PR microsoft#2259 by removing vestigial dep_targets_active plumbing from hook leaf integrations, replacing the fragile line-window architecture guard with an AST checker, and clarifying the user-facing hook-routing docs and changelog entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
apm-spec-waiver: Bug fix restores existing per-file hook routing under dependency targets; no new spec surface. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
0906037 to
2f3b15f
Compare
|
CI-green resolution update:
Post-push PR probe sees head 2f3b15f, mergeable MERGEABLE, mergeStateStatus BLOCKED while checks repopulate on the fork branch; Spec conformance has not reported yet. |
Preserve current-main Copilot user-scope path rewriting and dropped-target reconciliation while keeping per-file target routing unconditional at the HookIntegrator leaf owners. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab22e2ac-6064-4629-808d-183bc2129654
Description
A per-dependency
targets:list disabled per-file hook routing. Anytargets:value set
dep_targets_active = True, which gated_filter_hook_files_for_targetat three call sites, so a package shipping divergent per-target hook files (e.g.
pkg-claude-hooks.jsonwith aSkillmatcher andpkg-codex-hooks.jsonwith anapply_patchmatcher) had every file merged into every active target —cross-contaminating each tool's config and duplicating shared entries. Even a
single-target
targets: [codex]leaked: Codex absorbed the Claude file's hooks.This removes the gate at all three sites (Copilot, the merged-target integrator,
and Kiro) so per-file routing always runs. The
targets:list still narrowswhich targets are active upstream in
filter_targets_for_dependency; per-filerouting then places each file only in its intended tool. Universal files and
combined
claude-codexmanifests are unaffected, and thedep_targets_active/allowed_targetschokepoint assertion is unchanged.Fixes #2258
Type of change
Testing
New parametrized tests in
tests/unit/integration/test_dep_target_intersection.pycover divergent-file routing across every pair of merge-based harnesses (claude,
codex, cursor, gemini, antigravity, windsurf), for both multi-target and
single-target
targets:lists: each file lands only in its own harness, nocross-harness command leaks in, and shared entries are not duplicated. These fail
on the pre-fix code and pass with the fix. The existing per-target-selection and
combined-manifest tests remain green.
Spec conformance (OpenAPM v0.1)