SK-687: Support remote MCP servers for Copilot

[detkin]

Summary

• Remove the install-time skip that reported "Remote MCP (sse/http) not supported" for GitHub Copilot (fixes Copilot support for remote mcp servers #201)
• Write remote MCP assets to .vscode/mcp.json as {"type": "http"|"sse", "url": ...} per the VS Code MCP configuration reference; no extraction since nothing runs locally
• Mirror all MCP assets (stdio and remote) into the Copilot CLI config so they work in copilot sessions too: ~/.copilot/mcp-config.json for global scope, {repoRoot}/.github/mcp.json for repo/path scopes (both use the mcpServers key; CLI entries carry an explicit type)
• Root .mcp.json is deliberately not used for the CLI mirror — the Claude Code client manages that file
• Env vars are intentionally not written on remote entries — VS Code only supports env on stdio servers; remote servers authenticate via headers, which sx metadata does not model yet
• Fix VerifyInstalled for config-only and remote MCP servers: they have no extracted mcp-servers/{name}/ directory, so verification now falls back to checking the mcp.json entry (and detects packaged entries pointing at a deleted directory so --repair still re-extracts them)

Security implications of changes have been considered

[linear-code]

SK-687

[claude]

Claude finished @detkin's task in 2m 29s —— View job

---

Clean, well-tested change that correctly follows the existing multi-client remote-MCP pattern (matching opencode/cursor/kiro); one narrow verification regression is worth a look.

[LOW] `VerifyInstalled` can falsely report a packaged server as installed when its directory is deleted

Location: internal/clients/github_copilot/handlers/mcp.go:194-208

Issue: Verification now branches on directory presence rather than asset kind. For a packaged MCP server whose mcp-servers/{name}/ directory has been deleted but whose mcp.json entry remains, utils.IsDirectory(serverDir) returns false, so the code falls through to the mcp.json check and returns true, "registered in mcp.json". Previously mcpOps.VerifyInstalled returned false, "directory not found", which is what drives --repair to re-extract the server files. So a broken packaged install (config entry present, files gone — the entry still points at absolute paths under the missing dir) is now reported healthy and won't be repaired. The version-mismatch repair path is unaffected (that only triggers while the directory exists).

Suggestion: Gate the mcp.json fallback on cases that legitimately have no directory rather than on directory absence — e.g. take the fallback only when h.metadata.MCP != nil && h.metadata.MCP.IsRemote() (metadata-known) or config-only, and otherwise let mcpOps.VerifyInstalled report the missing directory. If config-only vs. packaged truly can't be distinguished from metadata alone here, a short comment noting the accepted trade-off would help the next reader. Fix this →

[NIT] `utils.HasContentFiles(zipData)` is computed even on the remote path where it's unused

Location: internal/clients/github_copilot/handlers/mcp.go:39-48

Issue: hasContent is evaluated before the switch, but the remote branch (checked first) never uses it — it reads/inspects the whole zip unnecessarily for remote servers.

Suggestion: Minor; optionally move the HasContentFiles call into the case hasContent/default arms (e.g. compute it lazily after the remote check) so remote installs skip the zip inspection. Not blocking.

Notes (no action needed): dropping env on remote entries is a reasonable, documented scoping decision consistent with opencode, and "type" correctly maps to the sse/http transport per the VS Code MCP config reference. I was not able to run go test/make prepush in this environment (command approval), so I relied on reading the tests — they look thorough (http, sse, env-not-written, remove, and verify for both remote and config-only).
• detkin/SK-687-copilot-remote-mcp

[claude]

Claude finished @detkin's task in 2m 13s —— View job

---

Both findings from the prior review are now resolved — the HasContentFiles call is scoped to the non-remote branch (so remote installs skip zip inspection), and VerifyInstalled no longer falsely reports a packaged server as healthy when its directory is deleted (entryReferencesDir now detects the dangling entry, backed by a new PackagedDirDeleted test); one residual heuristic gap is worth a heads-up but is already documented in-code.

[NIT] Verify heuristic can still miss a broken packaged server that doesn't reference its own directory

Location: internal/clients/github_copilot/handlers/mcp.go:215-236

Issue: entryReferencesDir only returns true when command or an arg is an absolute path prefixed by the (now-missing) serverDir. ResolveArgs only rewrites an arg to an absolute path when the file existed under serverDir at install time (os.Stat check in paths.go:177). So a packaged server that launches purely via a bare command + subcommands/flags (no file-path arg, e.g. a server invoked through a wrapper that cds into the dir) would leave no path reference in mcp.json; if its directory were later deleted, VerifyInstalled would still return true, "registered in mcp.json" and --repair wouldn't re-extract it. In practice genuine packaged servers almost always reference an extracted file, so this is unlikely — and the trade-off is already called out in the code comment.

Suggestion: No change required for merge. If you want to close the gap fully later, persisting the install kind (packaged vs config-only vs remote) into the lockfile so VerifyInstalled can branch on kind rather than on a path heuristic would be more robust than string-matching entries. A one-line note on this residual case in the existing comment would also suffice.

[NIT] Config-only entries are no longer content-verified against current metadata

Location: internal/clients/github_copilot/handlers/mcp.go:206-218

Issue: For config-only (and remote) servers the fallback returns true as soon as the named entry exists in mcp.json; it doesn't compare the stored command/args/url to current metadata, so a stale entry after a version bump won't be flagged for repair. Note this is still a net improvement — before this PR config-only servers had no extracted directory and mcpOps.VerifyInstalled would have reported them broken outright — so it's a limitation, not a regression.

Suggestion: Optional. If desired, compare the resolved entry against generateConfigOnlyMCPEntry()/generateRemoteMCPEntry() when metadata is available; otherwise leave as-is and treat presence-check as the intended contract.

Note: I couldn't run go build/go test/make prepush in this environment (command approval), so this is a read-only review. The new tests (Remote_HTTPInstall, Remote_SSEInstall, Remote_EnvNotWritten, Remote_Remove, VerifyInstalled_Remote, VerifyInstalled_ConfigOnly, VerifyInstalled_PackagedDirDeleted) read as thorough and cover the changed paths.
• detkin/SK-687-copilot-remote-mcp

[claude]

Claude finished @detkin's task in 2m 25s —— View job

---

The Copilot CLI mirror is a clean, well-tested addition that correctly writes VS Code (servers) and CLI (mcpServers) configs to their distinct schemas and preserves existing entries; two verification/scoping gaps around the new mirror are worth a look before merge.

[LOW] VerifyInstalled doesn't check the CLI-mirrored config, so --repair can't restore a broken CLI entry

Location: internal/clients/github_copilot/handlers/mcp.go:253-273 (verify) vs :80-86 (install writes two files)

Issue: Install now writes the entry to two places — VS Code's mcp.json and the Copilot CLI config (~/.copilot/mcp-config.json or {repoRoot}/.github/mcp.json). VerifyInstalled only inspects the VS Code side (serverDir / mcp.json). If the CLI mirror entry is later deleted or corrupted while the VS Code entry stays intact, VerifyInstalled returns true and --repair never re-runs Install, so the CLI config is left broken. The two write targets are now asymmetric with respect to verification.

Suggestion: When CLIConfigPath != "", also confirm the named entry exists in the CLI config before reporting healthy (read via readCopilotCLIMCPConfig and check MCPServers[name]); otherwise return false so repair rewrites both files. A one-line comment noting the accepted trade-off would suffice if you'd rather defer. Fix this →

[LOW] Packaged repo/path-scoped servers mirror machine-absolute paths into the shared .github/mcp.json

Location: internal/clients/github_copilot/handlers/mcp.go:82-86 + client.go:328-341 (repo/path → {repoRoot}/.github/mcp.json)

Issue: For a packaged server at repo/path scope, generateMCPEntry rewrites command/args to absolute paths under {repoRoot}/.vscode/mcp-servers/{name}/…. cliMCPEntry copies those verbatim into {repoRoot}/.github/mcp.json — a file typically committed and read by teammates' copilot CLI. Those absolute paths (e.g. /home/alice/proj/.vscode/…) won't resolve on another machine and leak the local home path. Remote (this PR's focus) and config-only entries are unaffected since they carry no paths; this only bites packaged servers. It mirrors a pre-existing characteristic of repo-scoped .vscode/mcp.json, but the mirror now propagates it into the CLI's shared surface.

Suggestion: No change strictly required for the remote use case. If packaged repo-scope installs are expected, consider limiting the .github/mcp.json mirror to remote/config-only entries (which are portable), or at least note the constraint near determineCLIMCPConfigPath.

[NIT] Residual verify heuristic gaps from prior reviews remain (documented, no action needed)

Location: internal/clients/github_copilot/handlers/mcp.go:262-286

Issue: Carrying forward from the earlier reviews: entryReferencesDir can miss a packaged server that launches via a bare command with no path arg, and config-only/remote entries are presence-checked rather than compared against current metadata (a stale entry after a version bump isn't flagged). Both are already called out in the in-code comment and are net improvements over the pre-PR behavior.

Suggestion: Optional follow-up — persist the install kind (packaged/config-only/remote) into the lockfile so VerifyInstalled can branch on kind rather than a path heuristic. Fine to leave as-is for this PR.

Note: I couldn't run go test/go build/make prepush in this environment (command approval), so this is a read-only review. The new tests (Remote_CLIMirror, Stdio_CLIMirrorAddsType, Remove_CLIMirror, Remove_NoCLIConfigNoop, CLIMirror_PreservesOtherEntries, plus the earlier remote/verify cases) read as thorough and cover the changed paths.
• detkin/SK-687-copilot-remote-mcp

[claude]

Claude finished @detkin's task in 2m 13s —— View job

---

The latest commit (d8d008c) cleanly resolves both prior LOW findings — VerifyInstalled now checks the CLI mirror entry (cliEntryExists) so --repair restores a broken CLI config, and packaged servers are kept out of the shared .github/mcp.json via CLIConfigShared/shouldMirrorToCLI — with matching tests; the install/verify mirror logic is now symmetric and correct, and only previously-documented heuristic gaps remain.

[NIT] Residual verify heuristics from earlier reviews remain (documented, no action needed)

Location: internal/clients/github_copilot/handlers/mcp.go:296-307

Issue: Two limitations carried over from the prior reviews are still present and already called out in the in-code comment: (1) entryReferencesDir only detects a broken packaged install when the entry contains an absolute path under the missing serverDir, so a packaged server launched via a bare command with no path arg wouldn't be flagged for --repair; and (2) config-only/remote entries are presence-checked rather than compared against current metadata, so a stale command/args/url after a version bump isn't re-repaired. Both are net improvements over pre-PR behavior (config-only/remote had no directory and would have failed verification outright).

Suggestion: No change required for merge. If you want to close these fully later, persisting the install kind (packaged/config-only/remote) into the lockfile so VerifyInstalled can branch on kind rather than a path heuristic would be more robust than string-matching. Fix this →

[NIT] Path-scoped MCP installs ignore the sub-path (pre-existing, not introduced here)

Location: internal/clients/github_copilot/client.go:302-317 and :326-342

Issue: For ScopePath, determineMCPTargetBase returns {repoRoot}/.vscode and determineCLIMCPConfig returns {repoRoot}/.github/mcp.json — both drop scope.Path, unlike determineTargetBase (which uses {repoRoot}/{path}/.github). So two different paths installing an MCP with the same name would collide at the repo root for both the VS Code and CLI configs. This predates the PR on the VS Code side; the new CLI mirror simply follows the same convention.

Suggestion: Not blocking and likely intentional (VS Code mcp.json is workspace-level). Worth a one-line note near determineCLIMCPConfig that path scope resolves to repo-root config, so the collision behavior is explicit for the next reader.

Note: I couldn't run go test/make prepush in this environment (command approval), so this is a read-only review. The new tests (Packaged_NotMirroredToSharedCLIConfig, VerifyInstalled_MissingCLIMirror, plus the earlier remote/CLI-mirror/verify cases) read as thorough and cover the changed paths, including the install↔verify symmetry for shared vs. non-shared configs.
• detkin/SK-687-copilot-remote-mcp

[claude]

Claude finished @detkin's task in 1m 58s —— View job

---

The Copilot remote-MCP + CLI-mirror change is clean and thoroughly tested, and the four prior review rounds' findings are all resolved; one config-serialization robustness gap and a couple of documented residual heuristics are worth a look, none blocking.

[LOW] Rewriting mcp.json / CLI config silently drops any sibling top-level keys (e.g. VS Code inputs)

Location: internal/clients/github_copilot/handlers/mcp.go:230-232 (mcpConfig) and :391-393 (copilotCLIMCPConfig)

Issue: Both config structs model a single top-level key (servers / mcpServers). readMCPConfig/readCopilotCLIMCPConfig unmarshal into them and writeMCPConfig/writeCopilotCLIMCPConfig MarshalIndent them back, so any other top-level key a user or another tool put in the file is discarded on the next write. VS Code's .vscode/mcp.json and repo .github/mcp.json support a top-level inputs block (for prompted variables/secrets); if a user has one, installing/removing an MCP entry will strip it. The CLIMirror_PreservesOtherEntries test covers preservation of other servers, but not other top-level keys. The VS Code side is pre-existing behavior, but this PR extends the same round-trip to the two new CLI mirror files (~/.copilot/mcp-config.json, {repoRoot}/.github/mcp.json).

Suggestion: Preserve unknown top-level fields — e.g. add Extra map[string]json.RawMessage with json:"-" populated via a raw-map decode, or decode into map[string]any, mutate only the servers key, and re-marshal. Otherwise, a short comment noting that only the servers key is preserved would set expectations. Fix this →

[NIT] Residual verify heuristics from earlier rounds remain (documented, no action needed)

Location: internal/clients/github_copilot/handlers/mcp.go:296-307

Issue: As noted in prior reviews and already called out in the in-code comment: (1) entryReferencesDir only flags a broken packaged install when the entry contains an absolute path under the missing serverDir, so a packaged server launched via a bare command with no path arg wouldn't be re-repaired; and (2) config-only/remote entries are presence-checked rather than compared against current metadata, so a stale command/args/url after a version bump isn't flagged. Both are net improvements over pre-PR behavior.

Suggestion: Optional follow-up — persist the install kind (packaged/config-only/remote) into the lockfile so VerifyInstalled branches on kind rather than a path heuristic. Fine to leave as-is for this PR.

Note: command approval is disabled in this environment, so I couldn't run go test/make prepush — this is a read-only review. The added tests (Remote_*, VerifyInstalled_*, CLIMirror_*, Packaged_NotMirroredToSharedCLIConfig) read as thorough and cover the changed install↔verify paths, including shared-vs-nonshared symmetry.
• detkin/SK-687-copilot-remote-mcp

[claude]

Claude finished @detkin's task in 1m 43s —— View job

---

The latest commit (bf947c5) cleanly resolves the outstanding LOW finding — readMCPConfig/readCopilotCLIMCPConfig now decode into a generic map, stash non-servers/mcpServers keys in Extra, and re-emit them on write (covered by PreservesUnknownTopLevelKeys) — so both the VS Code and CLI mirror files retain sibling keys like inputs; only the two previously-documented, in-code-commented heuristic nits remain, and nothing here is blocking.

[NIT] Config rewrites still drop JSONC comments (round-trip through plain JSON)

Location: internal/clients/github_copilot/handlers/mcp.go:237-262 and :466-492

Issue: readMCPConfig/readCopilotCLIMCPConfig parse via utils.UnmarshalJSONC (comment-tolerant) but writeMCPConfig/writeCopilotCLIMCPConfig emit with json.MarshalIndent, so any // comments a user placed in .vscode/mcp.json (VS Code explicitly documents JSONC there) are stripped on the next install/remove. This is pre-existing on the VS Code side, but Extra-preservation might create the expectation that user content survives — comments don't. Lower-impact than the (now-fixed) key loss.

Suggestion: Not blocking. If worth closing later, note in the writeMCPConfig comment that comments are not preserved, or move to a comment-preserving edit. Otherwise fine as-is.

[NIT] Residual verify heuristics from earlier rounds remain (documented, no action needed)

Location: internal/clients/github_copilot/handlers/mcp.go:309-320

Issue: Carried forward and already called out in the in-code comment: (1) entryReferencesDir only flags a broken packaged install when the entry contains an absolute path under the missing serverDir, so a packaged server launched via a bare command with no path arg wouldn't be re-repaired; and (2) config-only/remote entries are presence-checked rather than compared against current metadata, so a stale command/args/url after a version bump isn't flagged. Both are net improvements over pre-PR behavior.

Suggestion: Optional follow-up — persist the install kind (packaged/config-only/remote) into the lockfile so VerifyInstalled branches on kind rather than a path heuristic. Fine to leave as-is for this PR. Fix this →

Note: command approval is disabled in this environment, so I couldn't run go test/make prepush — this is a read-only review. The install↔verify mirror logic is symmetric (shouldMirrorToCLI gates both sides identically), and the new tests (PreservesUnknownTopLevelKeys, CLIMirror_PreservesOtherEntries, Packaged_NotMirroredToSharedCLIConfig, VerifyInstalled_MissingCLIMirror, plus the earlier Remote_*/VerifyInstalled_* cases) read as thorough and cover the changed paths.
• detkin/SK-687-copilot-remote-mcp