feat(cli): add GitHub Copilot CLI setup target - #3092
Conversation
enesgules
left a comment
There was a problem hiding this comment.
The functional config is correct per the Copilot CLI docs (~/.copilot/mcp-config.json + mcpServers, project .mcp.json, type: "stdio" is a valid alias for local, skills in .agents/skills / ~/.agents/skills). But two things should change before merge:
-
Dead abstraction.
projectConfigKey,buildProjectEntry, andgetMcpConfigKeyare added andscopeis threaded throughsetup.ts/remove.ts, but no agent sets the optional fields — both scopes resolve to"mcpServers"(the test even asserts this). This looks like a leftover from the abandoned.vscode/mcp.jsondesign. Please delete it (~40 lines across 3 files) and keep usingagent.mcp.configKey; reintroduce the indirection when an agent actually needs different keys per scope. -
Stale PR body. The body says project
.vscode/mcp.jsonwith theserverskey. The code uses.mcp.json+mcpServers— which is right, Copilot CLI does not read.vscode/mcp.json. Update the body.
Judgment calls to confirm:
tools: ["query-docs", "resolve-library-id"]hard-codes the allowlist. If the MCP server adds or renames a tool, Copilot silently blocks it.tools: ["*"](the documented default) avoids that — keep the explicit list only if intentional.- Copilot shares
.mcp.json+mcpServerswith theclaudeagent, soctx7 remove --copilot --projectalso removes Claude Code's project entry, and any repo with.mcp.jsondetects Copilot as installed. Probably fine since it's the same entry, but confirm it's intentional.
|
Addressed:
Explicit project removal still removes the shared Native |
enesgules
left a comment
There was a problem hiding this comment.
All four points from my previous review are addressed:
- The unused
projectConfigKey/buildProjectEntry/getMcpConfigKeyabstraction is gone — back toagent.mcp.configKey. - PR body now matches the code (
.mcp.json+mcpServers). tools: ["*"]replaces the hard-coded allowlist.detect.projectPathsis now empty with a clear comment, so a Claude-only.mcp.jsonno longer auto-detects as Copilot; project setup stays available via explicit--copilot. There's a regression test for exactly this.
Remaining known caveat (fine to ship): remove --copilot --project and remove --claude --project operate on the same .mcp.json entry, since both tools read the same file and key — inherent to the shared format, not worth special-casing.
Approved.
3e30d1e to
e43dab7
Compare
e43dab7 to
eca692f
Compare
Summary
--copilotsetup/remove flags~/.copilot/mcp-config.jsonand project.mcp.json, both usingmcpServerstools: ["*"]default--copilotselection for project setup because.mcp.jsonis shared with Claude Code and cannot identify ownership safelyValidation
pnpm --dir packages/cli typecheckpnpm --dir packages/cli lint:checkpnpm --dir packages/cli test— 325 tests passedpnpm --dir packages/cli buildcopilot mcp getreports the generated entry as an enabled workspace server with all toolsLinear: CTX7-2532