Skip to content

feat: implement hooks for copilot#1191

Open
r1bilski wants to merge 1 commit intodyoshikawa:mainfrom
r1bilski:copilot-add-hooks
Open

feat: implement hooks for copilot#1191
r1bilski wants to merge 1 commit intodyoshikawa:mainfrom
r1bilski:copilot-add-hooks

Conversation

@r1bilski
Copy link
Collaborator

This:

  • adds hooks feature for copilot

Notes:
Copilot has an unique hooks format, where it allows for defining the command through bash or powershell fields. Depending on the platform you're on, either powershell or bash are required. There are different ways in which this mismatch could be resolved. This chooses to read the user's platform and generate bash or powershell accordingly. If the user wants to maintain both, then they'll need to use copilot-specific overrides.

Closes #1006

This:
- adds hooks feature for copilot

Notes:
Copilot has an unique hooks format, where it allows for defining the
command through `bash` or `powershell` fields. Depending on the platform
you're on, either `powershell` or `bash` are required.
There are different ways in which this mismatch could be resolved. This
chooses to read the user's platform and generate bash or powershell
accordingly. If the user wants to maintain both, then they'll need to
use copilot-specific overrides.
@r1bilski
Copy link
Collaborator Author

@dyoshikawa This one is a little bit more complicated due to copilot's unusual format.

I was considering different approaches, but in the end landed on the one implemented, as described in the commit message.

One thing worth noting is that I decided to pass through unknown fields when generating, instead of skipping them, to allow users to define Copilot's bash and powershell fields in config.copilot.hooks.

It seems that currently handling of translation between rulesync's format and agent's format is inconsistent: depending on the feature or agent, we're either passing these fields through or ignoring them when exporting. When importing we're mostly ignoring unknown fields, except for rules, where we put them under agent specific configuration - this approach is not possible to achieve with other features because of how overrides work there.

I think it'd be worthwhile to consolidate these approaches, but that'd probably require rethinking how we do agent-specific overrides for all agents. I do have some ideas, so I might write up an issue if you want, but at the same time, changing this now would be a big breaking change.

Let me know if you find anything in this PR or even would like to approach Copilot's format differently. Thanks!

@dyoshikawa
Copy link
Owner

Thank you!

@dyoshikawa
Copy link
Owner

/oc review

@github-actions
Copy link
Contributor

PR #1191 Review Summary: feat: implement hooks for copilot

Code Review Results ✅

Summary: Well-implemented PR adding hooks feature for GitHub Copilot with platform-specific bash/powershell command handling.

Minor Issues:

  1. Unused global parameter in fromRulesyncHooks (src/features/hooks/copilot-hooks.ts:170-171)
  2. Missing test for toRulesyncHooks with neither bash nor powershell fields

Positive Aspects:

  • Excellent code consistency with existing patterns
  • Robust platform detection for Windows/Unix
  • Comprehensive test coverage (620 lines)
  • Proper error handling with formatError utility
  • Clean supportsMatcher integration

Verdict: ✅ Approve


Security Review Results ✅

No vulnerabilities detected. The implementation is secure:

  • No command injection risk (generates config files, doesn't execute commands)
  • Input validation via safeString schema filters control characters (\n, \r, \0)
  • Safe path construction using join() with fixed components
  • Proper JSON parsing with error handling

Security Best Practices Followed:

  • No direct command execution
  • Zod schema validation for type safety
  • No eval() or Function() usage
  • Consistent with existing secure patterns

Overall Assessment

✅ PR #1191 is ready for merge. Both code quality and security reviews pass with only minor, non-blocking recommendations for future improvement.

github run

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.

[Feature request]: Github Copilot Hooks

2 participants