fix(hooks): emit env and stop picking the Copilot command field by platform - #2621
Merged
Merged
Conversation
…atform canonicalToCopilotHooks filtered every canonical key out of its passthrough, so an authored env never reached copilot-hooks.json even though Copilot supports it natively on command hooks. It also chose between the bash and powershell fields from process.platform. That file is committed to the repository, so the artifact differed by generating machine, and the cloud agent runs hooks in a Linux sandbox where a powershell entry is ignored outright - a Windows-generated file was unusable. The field now follows the canonical shell selector, falling back to the portable command field, mirroring copilotcli-hooks.ts. Import is made platform-independent for the same reason: an entry carrying both fields resolves to bash everywhere, and the shell selector and env now round-trip.
…lector Three spots still described the platform-based choice this branch removed: the Copilot format note, the per-tool event summary, and the canonical shell field, which now lists both Copilot targets among its consumers. The rationale is also restated: the load-bearing problem is that the Linux-sandboxed cloud agent ignores a powershell entry outright, so a Windows-generated hook never ran. Output stability is the secondary benefit, and only for users who check the file in - rulesync gitignores its own generated copy.
This was referenced Aug 7, 2026
Merged
Merged
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
2 participants
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.
Two bugs in
src/features/hooks/copilot-hooks.ts, both from the 2026-08-06 comment.envwas never emitted. The unknown-key passthrough filters out every key inHookDefinitionSchema, andenvis canonical, so an authoredenvwas dropped on the floor - even though Copilot supports it natively on command hooks. It is now emitted explicitly, the waycopilotcli-hooks.ts:239-248already does.The command field was chosen from
process.platform..github/hooks/copilot-hooks.jsonis committed to the repository, so the artifact differed depending on which machine ran generate. Worse, the docs are explicit that the cloud agent "runs hooks in a Linux sandbox. Only thebashfield is honored;powershellentries are ignored" - so a file generated on Windows was simply inert. The field now follows the canonicalshellselector, falling back to the portablecommandfield (which upstream copies to both), exactly as the Copilot CLI adapter does.Import made platform-independent too
Beyond the assigned scope, but the same bug class:
resolveImportCommandbroke a bash+powershell tie usingprocess.platform, so importing one file produced different canonical configs on different machines. It now always takesbash- the only field the cloud agent runs - and warns. Import also records theshellselector and readsenv, so both round-trip.Note on the test changes
Nine existing tests asserted the platform-dependent behavior directly (
should use powershell field on Windowsand friends). Those encode the bug being fixed, so they are rewritten rather than kept: two becameshell-selector tests, one asserts the generated file is byte-identical acrosswin32andlinux, and the rest had incidentalentry.bashassertions updated toentry.command. New coverage:envemission, and a round-trip of the shell selector plusenvthrough import and re-export.Verified against the hooks reference; checked locally with the hooks e2e spec.
Part of #2401
The issue's other items -
edits/urlsautoApprove permissions, MCPinputs/sandbox, and global hooks scope - are untouched and remain open.🤖 Generated with Claude Code