-
Notifications
You must be signed in to change notification settings - Fork 88
Scope-awareness gaps: hook integrator, auto_create, integration tests #576
Description
Problem
Architecture audit (April 2026) revealed that while resolve_targets() + for_scope() correctly handle scope resolution at the dispatch layer, several internal integrator methods and test paths still have scope-awareness gaps. These cause silent failures when using apm install -g or apm uninstall -g.
Root cause pattern: The targeting refactor (#542, #562) unified the dispatch layer but did not reach into every integrator's internal path logic. The hook integrator is the primary offender -- it predates the data-driven model and still uses hardcoded paths and target.name string comparisons internally.
Findings
P1-1: Hook sync_integration() is scope-unaware
hook_integrator.py:621-715 hardcodes all cleanup paths (.github/hooks/, .claude/settings.json, .cursor/hooks.json, .codex/hooks.json). No targets= parameter. Uninstall at user scope fails to clean hook files deployed to .copilot/hooks/.
Fix: Add targets= parameter (parallel SkillIntegrator.sync_integration()), derive cleanup paths from resolved profiles. Update uninstall engine to pass _resolved_targets.
P1-2: Hook Copilot integration bypasses resolved root_dir
PR #566 partially addresses this by adding target= param to integrate_package_hooks(), but sync_integration() still adds .copilot/hooks/ as yet another hardcoded prefix instead of deriving from targets.
P1-3: _integrate_merged_hooks uses target_key not root_dir
f".{config.target_key}" constructs the deploy path instead of using target.root_dir. Latent bug if any merge-target gets a different user root.
P1-4: install.py creates dirs for auto_create=False targets
Lines 1434-1442 mkdir for all resolved targets. --target=all creates .codex/, .opencode/, .cursor/ when user hasn't opted in. Should gate on _t.auto_create.
P2-1: _dry_run_uninstall reads lockfile from Path(".") ignoring scope
Should accept apm_dir parameter.
P2-2: Hook display paths in install.py hardcoded
target.name == "claude" -> .claude/settings.json. Should derive from target profile.
P2-3: active_targets_user_scope() returns unresolved profiles
Public function that returns raw KNOWN_TARGETS profiles. Should be private or return resolved profiles.
Missing integration test coverage
Why these bugs didn't surface: There are NO integration tests that exercise the full install -> uninstall cycle at user scope with actual file deployment. Unit tests mock the integrators; CLI tests verify output strings. Neither catches "files went to wrong directory."
Tests needed:
- Install package at user scope, verify files land in
~/.copilot/not~/.github/ - Uninstall at user scope, verify files removed from
~/.copilot/ deps update -g, verify lockfile written to~/.apm/not./- OpenCode
.config/opencode/multi-level root at user scope (full cycle) - Codex excluded at user scope (
for_scopereturns None) - Trie routing with all 5 targets resolved simultaneously
--target=alldoes NOT create dirs for auto_create=False targets
Recommended approach
Phase 1: Hook integrator scope completion
- Make
sync_integration()target-driven (accepttargets=param) - Make
_integrate_merged_hooksusetarget.root_dirnottarget_key - Gate
mkdironauto_createin install.py - Fix
_dry_run_uninstalllockfile path
Phase 2: Integration test suite
- Create
tests/integration/test_scope_deploy.py - Parametrize across targets x scopes x commands
- Use real integrators (not mocks) against temp dirs
- Verify file existence at expected paths AND absence at wrong paths
Related: #542 (scope-resolved targets), #562 (dispatch unification), #566 (hook root_dir), #575 (deps update scope)