Copilot: make MCP CLI prompt list reflect actual mounted servers under --disable-builtin-mcps#46778
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
--disable-builtin-mcps
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
Thanks for the fix, What's working well:
To get this across the finish line:
The implementation looks ready for final validation. Once that step completes and the WIP is cleared, this should be good to merge! 🎉 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (70 additions detected, threshold is 100). |
There was a problem hiding this comment.
Pull request overview
Aligns Copilot prompts with MCP servers mounted as CLI wrappers.
Changes:
- Adds GitHub and custom MCP servers to Copilot’s CLI server list.
- Adds regression tests for Copilot and non-Copilot behavior.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_cli_mount.go |
Expands Copilot MCP CLI server discovery. |
pkg/workflow/mcp_cli_mount_test.go |
Tests server-list behavior by engine. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/mcp_cli_mount.go:130
- This pass does not actually mirror every server mounted from the manifest: it only adds GitHub and map-shaped custom MCP tools. For example, with Copilot + safe outputs + Playwright MCP and no
cli-proxy,safeoutputsactivates the mount step, the mount script creates both wrappers, but this loop omitsplaywrightfrom the prompt (the same applies toagentic-workflows). The agent is therefore still forced to guess some mounted command names. Build this augmentation from the same engine-filtered server set used to create the gateway manifest, including the manifest ID normalization for built-in servers.
for toolName, toolValue := range data.Tools {
if internalMCPServerNames[toolName] {
continue
}
mcpConfig, ok := toolValue.(map[string]any)
if !ok {
continue
}
if hasMcp, _ := hasMCPConfig(mcpConfig); hasMcp && !slices.Contains(servers, toolName) {
servers = append(servers, toolName)
}
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| // custom servers such as azure-devops) and exposes wrappers on PATH. Reflect | ||
| // that runtime reality in the generated CLI server list so agents can call | ||
| // these wrappers deterministically instead of guessing command names. | ||
| if len(servers) > 0 && data.EngineConfig != nil && data.EngineConfig.ID == string(constants.CopilotEngine) { |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (4 tests)
Verdict
Test highlights:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /codebase-design, and /tdd — commenting (no blocking issues, but three improvements worth addressing).
📋 Key Themes
Issues Found
- Hardcoded
"github"string (mcp_cli_mount.go:116) — inconsistent with constants-based naming elsewhere; typo risk. - Duplicated custom-server iteration (
mcp_cli_mount.go:120–131) — samedata.Toolsscan already exists in thecli-proxyblock; a shared helper would prevent drift. - Missing boundary-condition test (
mcp_cli_mount_test.go:256) — thelen(servers) > 0guard silently skips the new block for Copilot workflows withoutsafeoutputs/mcpscripts; a test should assert the intended behaviour.
Positive Highlights
- ✅ Clear PR description with an inline code snippet explaining the fix.
- ✅ Non-Copilot path untouched; regression test verifies this.
- ✅
internalMCPServerNamesskip guard correctly carried over to the new loop. - ✅
isGitHubCLIModeEnabledcheck prevents double-listinggithubingh-proxymode.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 39.7 AIC · ⌖ 4.75 AIC · ⊞ 6.7K
Comment /matt to run again
| // that runtime reality in the generated CLI server list so agents can call | ||
| // these wrappers deterministically instead of guessing command names. | ||
| if len(servers) > 0 && data.EngineConfig != nil && data.EngineConfig.ID == string(constants.CopilotEngine) { | ||
| if hasGitHubTool(data.ParsedTools) && !isGitHubCLIModeEnabled(data) && !slices.Contains(servers, "github") { |
There was a problem hiding this comment.
[/diagnosing-bugs] "github" is hardcoded rather than using a named constant, while the rest of the file uses constants.SafeOutputsMCPServerID.String() etc. A typo here would silently produce a wrong server name in the generated prompt.
💡 Suggested fix
Define a constant such as constants.GitHubMCPServerID and use .String() everywhere, so a future rename cannot diverge silently.
@copilot please address this.
| servers = append(servers, "github") | ||
| } | ||
|
|
||
| for toolName, toolValue := range data.Tools { |
There was a problem hiding this comment.
[/codebase-design] The new Copilot loop (lines 120–131) duplicates the custom-MCP-server logic already in the cli-proxy loop (lines 55–94). For a Copilot workflow that also has cli-proxy: true, custom servers will be appended twice (deduplicated by slices.Contains, but the scan happens twice). This adds fragility: future changes to the earlier loop may not be mirrored here.
💡 Suggested refactor
Extract the "collect custom MCP servers from data.Tools" logic into a shared helper, and call it from both places. This keeps a single source of truth and makes the invariants obvious.
@copilot please address this.
| assert.True(t, slices.IsSorted(servers), "server list should remain sorted") | ||
| }) | ||
|
|
||
| t.Run("non-copilot keeps existing behavior", func(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] slices.IsSorted(servers) passes because sort.Strings is called unconditionally at the end of getMCPCLIServerNames, but this test does not document that invariant explicitly. More importantly, there is no test for the boundary condition where len(servers) == 0 before the Copilot block — i.e., a Copilot workflow with the GitHub tool enabled but no safeoutputs/mcpscripts configured. In that case the new block is skipped entirely and github is silently omitted.
💡 Suggested test
t.Run("copilot without safeoutputs does not add github", func(t *testing.T) {
data := &WorkflowData{
EngineConfig: &EngineConfig{ID: string(constants.CopilotEngine)},
Tools: map[string]any{"github": true},
ParsedTools: NewTools(map[string]any{"github": true}),
// SafeOutputs intentionally nil
}
servers := getMCPCLIServerNames(data)
assert.Nil(t, servers, "no mounts active → guard prevents github from being added")
})If this is the intended behaviour, a test documents it explicitly. If it is a gap, the guard condition needs adjusting.
@copilot please address this.
There was a problem hiding this comment.
The implementation is correct and focused. The Copilot-specific block is logically sound: it uses len(servers) > 0 as a guard to ensure infrastructure mounts are active, correctly skips internalMCPServerNames (which includes github), and handles the GitHub server separately via hasGitHubTool. The test coverage validates both the new behavior and the non-regression case for other engines.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 33.5 AIC · ⌖ 4.32 AIC · ⊞ 5K
There was a problem hiding this comment.
Overall
The logic is sound and the intent is well-documented. One non-blocking issue found.
What the PR does: Adds a Copilot-specific block in getMCPCLIServerNames to reflect that --disable-builtin-mcps is always active for Copilot; the mount script exposes github and custom MCP servers on PATH, so they appear in the generated <mcp-clis> prompt section.
What looks good:
- The
internalMCPServerNamesguard and!slices.Containsduplicate-prevention are present. - Non-Copilot path is untouched.
- Tests cover both branches.
Non-blocking: The slices.IsSorted assertion in the new test is tautological — see inline comment.
🔎 Code quality review by PR Code Quality Reviewer · 41.9 AIC · ⌖ 4.47 AIC · ⊞ 5.6K
Comment /review to run again
| assert.Contains(t, servers, constants.SafeOutputsMCPServerID.String(), "safeoutputs remains present") | ||
| assert.Contains(t, servers, "github", "github CLI wrapper should be listed for Copilot") | ||
| assert.Contains(t, servers, "azure-devops", "custom MCP server wrappers should be listed for Copilot") | ||
| assert.True(t, slices.IsSorted(servers), "server list should remain sorted") |
There was a problem hiding this comment.
Tautological assertion: slices.IsSorted can never fail here because sort.Strings(servers) is called unconditionally at the end of getMCPCLIServerNames before returning. This assertion tests an implementation detail, not observable behavior, and gives false confidence that sort order is being maintained correctly.
💡 Suggested fix
Replace with an assertion that would actually catch a regression — e.g., verify the full sorted output or use assert.Equal with the expected ordered slice:
assert.Equal(t, []string{"azure-devops", "github", constants.SafeOutputsMCPServerID.String()}, servers,
"server list order should be deterministic and sorted")This pins both presence AND order, so a future change that skips the sort would actually fail the test.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ation Fixes the case where a Copilot workflow with cli-proxy: true and only a GitHub MCP tool (no safeoutputs) would have len(servers)==0 before the Copilot augmentation block, causing it to be skipped and `github` to never be advertised in the CLI server list. Include ParsedTools.CLIProxy in the activation condition alongside len(servers) > 0, so the block runs whenever a CLI-mount trigger is active regardless of whether any non-GitHub servers were collected first. Also adds a regression test covering this specific case. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the reviewer's feedback in commit isCLIMountActive := len(servers) > 0 || (data.ParsedTools != nil && data.ParsedTools.CLIProxy)
if isCLIMountActive && data.EngineConfig != nil && data.EngineConfig.ID == string(constants.CopilotEngine) {This ensures a Copilot workflow with |
|
[pr-sous-chef] @copilot please run the Unresolved review feedback:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
…est assertions - Add constants.GitHubMCPServerID = "github" to avoid hard-coded strings - Extract appendCustomMCPServerIfEligible() shared helper used by both the cli-proxy default case and the Copilot augmentation sweep, eliminating the duplicated custom-MCP-server detection logic - Use constants.GitHubMCPServerID.String() throughout mcp_cli_mount.go - Replace tautological slices.IsSorted() assertions with assert.Equal() pinning the full expected sorted slice in both Copilot test cases - Add boundary-condition test: Copilot with no CLI mount trigger active returns nil (github is not inadvertently advertised) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit
|
…rIfEligible Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Copilot runs with
--disable-builtin-mcps, but the generated<mcp-clis>section only listedsafeoutputs, even whengithub/custom MCP wrappers were mounted onPATH. This created nondeterministic GitHub access: runs succeeded only when the agent guessed CLI server binaries existed.Prompt/runtime alignment for Copilot CLI mounts
github(when GitHub MCP is configured and not ingh-proxymode)azure-devops)Regression coverage
safeoutputs,github, and custom MCP servers in the generated CLI server list.run: https://github.com/github/gh-aw/actions/runs/29735453773
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.