[fix] use target.root_dir for copilot hook deployment at user scope (#565)#570
Closed
edenfunf wants to merge 1 commit into
Closed
[fix] use target.root_dir for copilot hook deployment at user scope (#565)#570edenfunf wants to merge 1 commit into
edenfunf wants to merge 1 commit into
Conversation
When apm install --global is run, the Copilot target is scope-resolved to root_dir=".copilot" via TargetProfile.for_scope(). However, integrate_package_hooks ignored the resolved target and hardcoded ".github" for both the hooks directory and the script paths written inside hook JSON files. Hooks therefore always landed in ~/.github/hooks/ instead of ~/.copilot/hooks/ at user scope. Pass the scope-resolved target through integrate_hooks_for_target into integrate_package_hooks, which now derives hook_root from target.root_dir (defaulting to ".github" for callers that don't supply a target). Thread hook_root through _rewrite_hooks_data and _rewrite_command_for_target so that script paths embedded in the deployed JSON also reference the correct root directory. Fixes microsoft#565
Contributor
Author
|
Closing in favor of #566 which fixes the same bug with a more complete implementation — it also patches |
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
1 participant
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.
What
integrate_package_hookshardcoded".github"as the hooks root directory, ignoring the scope-resolvedtarget.root_dir. At user scope the Copilot target resolves toroot_dir=".copilot"(viaTargetProfile.for_scope()), but hooks always landed in~/.github/hooks/instead of~/.copilot/hooks/. The same bug applied to the script paths written inside the deployed hook JSON files.Fixes #565.
Why
TargetProfile.for_scope(user_scope=True)returns a copy of the profile withroot_diralready overridden touser_root_dir(".copilot"for Copilot). All other integrators (InstructionIntegrator,AgentIntegrator, etc.) readtarget.root_dirdirectly after scope resolution.HookIntegrator.integrate_package_hookswas the only one that ignored it.How
target=Noneparameter tointegrate_package_hooks. When provided,target.root_diris used ashook_root; otherwise falls back to".github"(backward-compatible, all existing call sites withouttargetcontinue to work).hook_rootthrough_rewrite_hooks_data→_rewrite_command_for_targetso that script paths embedded in the deployed JSON also reference the correct root.integrate_hooks_for_targetnow passestarget=targettointegrate_package_hooksfor the"copilot"branch, matching the pattern used by every otherintegrate_*_for_targetdispatcher.No changes outside
hook_integrator.py. No new abstractions.Test
Added
test_integrate_package_hooks_user_scope_uses_copilot_roottoTestVSCodeIntegration:root_dir=".copilot"(mirrors whatfor_scope(user_scope=True)returns for Copilot).copilot/hooks/, not.github/hooks/.copilot/, not.github/.copilot/hooks/scripts/All 71 existing tests continue to pass.