Skip to content

feature: Add GitHub Copilot support (CLI and CHAT) ‼️‼️‼️#129

Open
npkriami18 wants to merge 1 commit intotirth8205:mainfrom
npkriami18:feature/github-copilot-support
Open

feature: Add GitHub Copilot support (CLI and CHAT) ‼️‼️‼️#129
npkriami18 wants to merge 1 commit intotirth8205:mainfrom
npkriami18:feature/github-copilot-support

Conversation

@npkriami18
Copy link
Copy Markdown

Adds MCP server integration for:

  • GitHub Copilot (VS Code Extension)
  • GitHub Copilot Chat (VS Code)
  • GitHub Copilot CLI

Uses --platform copilot for VS Code variant
Uses --platform copilot-cli for CLI variant

Platforms now: 10 (was 8)

@npkriami18 npkriami18 changed the title Add GitHub Copilot support feature : Add GitHub Copilot support ‼️‼️‼️ Apr 8, 2026
@npkriami18 npkriami18 changed the title feature : Add GitHub Copilot support ‼️‼️‼️ feature : Add GitHub Copilot support (CLI and CHAT) ‼️‼️‼️ Apr 8, 2026
@npkriami18 npkriami18 changed the title feature : Add GitHub Copilot support (CLI and CHAT) ‼️‼️‼️ feature: Add GitHub Copilot support (CLI and CHAT) ‼️‼️‼️ Apr 8, 2026
Copy link
Copy Markdown
Owner

@tirth8205 tirth8205 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding Copilot support! The structure is solid. A few issues to fix:

  1. CHANGELOG version regression. The PR adds a [2.1.1] entry, but we're already at [2.2.1]. Please remove the CHANGELOG entry — we'll add it when we cut the next release.

  2. Detection is too broad. _copilot_vscode_detected returns True if any VS Code directory exists — but that proves VS Code is installed, not Copilot. Every VS Code user would get Copilot MCP config injected. Other platforms check for platform-specific directories. Consider checking for the Copilot extension directory instead (e.g., ~/.vscode/extensions/github.copilot-*).

  3. Writes to VS Code's global settings.json. This is more invasive than other platforms which use dedicated config files. A misconfigured write could corrupt VS Code settings. Please add extra caution here (e.g., validate the JSON structure before writing).

  4. Tests are shallow. The existing TestPlatformInstall tests verify actual file writes and JSON structure. Please add similar tests for Copilot (not just that detect() returns a bool).

@npkriami18
Copy link
Copy Markdown
Author

npkriami18 commented Apr 9, 2026

Thanks for adding Copilot support! The structure is solid. A few issues to fix:

  1. CHANGELOG version regression. The PR adds a [2.1.1] entry, but we're already at [2.2.1]. Please remove the CHANGELOG entry — we'll add it when we cut the next release.
  2. Detection is too broad. _copilot_vscode_detected returns True if any VS Code directory exists — but that proves VS Code is installed, not Copilot. Every VS Code user would get Copilot MCP config injected. Other platforms check for platform-specific directories. Consider checking for the Copilot extension directory instead (e.g., ~/.vscode/extensions/github.copilot-*).
  3. Writes to VS Code's global settings.json. This is more invasive than other platforms which use dedicated config files. A misconfigured write could corrupt VS Code settings. Please add extra caution here (e.g., validate the JSON structure before writing).
  4. Tests are shallow. The existing TestPlatformInstall tests verify actual file writes and JSON structure. Please add similar tests for Copilot (not just that detect() returns a bool).

Thank you for the feedback! I've addressed all the points:

  • Removed the [2.1.1] entry from the CHANGELOG to avoid the version regression.
  • Updated _copilot_vscode_detected to specifically check for the Copilot extension directory (e.g., ~/.vscode/extensions/github.copilot-*), making detection more accurate.
  • Added extra validation before writing to VS Code's global settings.json to ensure the JSON structure is correct and prevent potential corruption.
  • Expanded the Copilot-related tests to verify file writes and JSON structure, similar to the TestPlatformInstall tests.
  • Please let me know if any further changes are needed!

@npkriami18 npkriami18 force-pushed the feature/github-copilot-support branch from aede802 to 95c4421 Compare April 9, 2026 09:11
@npkriami18 npkriami18 requested a review from tirth8205 April 10, 2026 02:17
@tirth8205
Copy link
Copy Markdown
Owner

Review: PR #129 — feature: Add GitHub Copilot support

The owner already requested changes and the author has addressed them in a follow-up commit. Looking at the current state of the diff:

What was fixed (per author's comment):

  • CHANGELOG version regression removed
  • _copilot_vscode_detected now checks for the Copilot extension directory (~/.vscode/extensions/github.copilot-*) instead of just VS Code existing
  • Added validation before writing to VS Code's global settings.json
  • Added comprehensive Copilot-specific tests

Remaining issues:

  1. Config path is Linux/macOS only: ~/.config/Code/User/settings.json is the Linux path. On macOS it is ~/Library/Application Support/Code/User/settings.json. On Windows it is %APPDATA%\Code\User\settings.json. The current implementation will silently write nothing on macOS and Windows. This is a functional bug.

  2. _validate_copilot_vscode_settings skips when file missing: The validation returns False (skip) when settings.json doesn't exist yet. But a fresh VS Code install with no settings.json is a valid case — other platforms create their config files. This is overly conservative but safe. If this is intentional (avoid creating VS Code settings from scratch), it should be documented in the validation function's docstring.

  3. Double blank line in skills.py: There is a double blank line after _validate_copilot_vscode_settings (before _zed_settings_path). Minor style issue.

The owner's original CHANGES_REQUESTED review has not been formally re-reviewed (status is still CHANGES_REQUESTED). Please address item 1 (cross-platform config path) and re-request review. The macOS path bug in particular would affect most Mac users of VS Code.

@tirth8205
Copy link
Copy Markdown
Owner

Merge conflict detected. This PR has merge conflicts with main (mergeStateStatus: DIRTY). Please rebase on main and also address the cross-platform config path issue noted above (macOS uses ~/Library/Application Support/Code/User/settings.json, not ~/.config/Code/User/settings.json). Both issues need to be resolved before this can merge.

@npkriami18 npkriami18 force-pushed the feature/github-copilot-support branch from 95c4421 to 570c6f3 Compare April 11, 2026 09:24
@npkriami18
Copy link
Copy Markdown
Author

Merge conflict detected. This PR has merge conflicts with main (mergeStateStatus: DIRTY). Please rebase on main and also address the cross-platform config path issue noted above (macOS uses ~/Library/Application Support/Code/User/settings.json, not ~/.config/Code/User/settings.json). Both issues need to be resolved before this can merge.

@tirth8205
Thanks again for the detailed review. Everything called out in the thread should now be addressed on feature/github-copilot-support (rebased on current main, conflicts resolved).

Done

  • CHANGELOG: No ad-hoc version bump; release notes left for you when you cut the next release.
  • Copilot detection: Only when the Copilot VS Code extension is present (~/.vscode/extensions/github.copilot-*), not merely “VS Code installed.”
  • VS Code settings.json: Validates JSON shape before writing; skips if the file is missing or not a JSON object (documented in the validator — intentional that we don’t create a new global settings file from this tool).
  • Cross-platform paths: User settings.json uses the correct location on macOS (~/Library/Application Support/...), Windows (%APPDATA%\...), and Linux (~/.config/...).
  • Tests: Copilot flows covered similarly to other platforms (writes, JSON shape, idempotency, missing/corrupt settings, dry-run, CLI path, detection with mocked home).
  • Docs: docs/USAGE.md updated for OS-specific paths and the “open VS Code once” behavior.
  • CLI: install / init include copilot, copilot-cli, and codex in --platform choices after the rebase.

Full test suite was run locally: 673 passed (5 skipped, 2 xpassed).

Please re-review when you can and merge if this looks good. Happy to adjust anything else you want changed.

@tirth8205
Copy link
Copy Markdown
Owner

Skipping auto-merge: after #142 (platform target filters) landed on main, this branch now conflicts on code_review_graph/cli.py and code_review_graph/skills.py. Please rebase onto current main and resolve the conflicts in the platform-registration code.

Adds MCP server integration for:
- GitHub Copilot (VS Code Extension)
- GitHub Copilot Chat (VS Code)
- GitHub Copilot CLI

Uses `--platform copilot` for VS Code variant
Uses `--platform copilot-cli` for CLI variant

Platforms now: 10 (was 8)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Made-with: Cursor
@npkriami18 npkriami18 force-pushed the feature/github-copilot-support branch from 570c6f3 to dc1340f Compare April 12, 2026 01:10
@npkriami18
Copy link
Copy Markdown
Author

Skipping auto-merge: after #142 (platform target filters) landed on main, this branch now conflicts on code_review_graph/cli.py and code_review_graph/skills.py. Please rebase onto current main and resolve the conflicts in the platform-registration code.

Hi @tirth8205 - could you merge this when you have a moment? main is moving quickly and several other PRs landing after mine have already forced rebases. Earlier merges are fine, but each time main advances I need to resolve conflicts again in the platform-registration paths (cli.py / skills.py). Merging soon would save churn on both sides. Thanks for the reviews and for maintaining the queue.

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.

2 participants