Skip to content

Use actual mounted MCP CLI wrappers so AI Moderator safe outputs remain reliable #39243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Copilot wants to merge 3 commits into from
+25 −14
Closed

Use actual mounted MCP CLI wrappers so AI Moderator safe outputs remain reliable #39243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a82cc3
Initial plan
Copilot Jun 14, 2026
06f1970
Clarify MCP CLI prompt fallback to MCP tools
Copilot Jun 14, 2026
67ce9d1
Fix MCP CLI wrapper list to reflect actual mounted servers
Copilot Jun 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion actions/setup/js/mount_mcp_as_cli.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,16 @@ exec node "${safeBridge}" \\
`;
}

/**
* Format mounted server names into prompt-ready bullet lines.
*
* @param {string[]} mountedServers - Successfully mounted server names
* @returns {string}
*/
function formatMountedServersPromptList(mountedServers) {
return mountedServers.map(name => `- \`${name}\` — run \`${name} --help\` to see available tools`).join("\n");
}
Comment on lines +320 to +322

/**
* Mount MCP servers as CLI tools by reading the manifest and generating wrapper scripts.
*
Expand Down Expand Up @@ -445,6 +455,7 @@ async function main() {
}
core.info(`CLI bin directory added to PATH: ${CLI_BIN_DIR}`);
core.setOutput("mounted-servers", mountedServers.join(","));
core.setOutput("mounted-servers-list", formatMountedServersPromptList(mountedServers));
}

module.exports = { main, fetchMCPTools, generateCLIWrapperScript, isValidServerName, shellEscapeDoubleQuoted, parseMCPResponseBody };
module.exports = { main, fetchMCPTools, generateCLIWrapperScript, isValidServerName, shellEscapeDoubleQuoted, parseMCPResponseBody, formatMountedServersPromptList };
7 changes: 6 additions & 1 deletion actions/setup/js/mount_mcp_as_cli.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// @ts-check
import { describe, expect, it } from "vitest";

import { parseMCPResponseBody } from "./mount_mcp_as_cli.cjs";
import { formatMountedServersPromptList, parseMCPResponseBody } from "./mount_mcp_as_cli.cjs";

describe("mount_mcp_as_cli.cjs", () => {
it("parses JSON object responses unchanged", () => {
Expand Down Expand Up @@ -38,4 +38,9 @@ describe("mount_mcp_as_cli.cjs", () => {
},
});
});

it("formats mounted servers for prompt rendering", () => {

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The single it block combines a happy-path assertion and the empty-array regression case. Splitting them into two named it blocks means failure messages directly identify the broken scenario, and the regression intent is self-documenting.

💡 Suggested split 
it("formats multiple mounted servers into prompt bullet lines", () => {
  expect(formatMountedServersPromptList(["safeoutputs", "playwright"]))
    .toBe("- `safeoutputs` — run `safeoutputs --help` to see available tools\n- `playwright` — run `playwright --help` to see available tools");
});

it("returns empty string when no servers are mounted (regression: partial/full mount failure must not advertise absent wrappers)", () => {
  expect(formatMountedServersPromptList([])).toBe("");
});

expect(formatMountedServersPromptList(["safeoutputs", "playwright"])).toBe("- `safeoutputs` — run `safeoutputs --help` to see available tools\n- `playwright` — run `playwright --help` to see available tools");
expect(formatMountedServersPromptList([])).toBe("");
});
});
18 changes: 6 additions & 12 deletions pkg/workflow/mcp_cli_mount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,28 +297,22 @@ func GetMCPCLIPathSetup(data *WorkflowData) string {
return `export PATH="${RUNNER_TEMP}/gh-aw/mcp-cli/bin:$PATH"`
}

// buildMCPCLIPromptSection returns a PromptSection describing the CLI tools available
// to the agent, or nil if there are no servers to mount.
// The prompt is loaded from actions/setup/md/mcp_cli_tools_prompt.md at runtime,
// with the __GH_AW_MCP_CLI_SERVERS_LIST__ placeholder substituted by the substitution step.
// buildMCPCLIPromptSection returns a PromptSection describing CLI wrappers available
// to the agent, or nil if no servers are configured for mounting.
// The prompt content is loaded from actions/setup/md/mcp_cli_tools_prompt.md at runtime.
// The server list comes from the mount step output so only successfully mounted wrappers
// are advertised to the model.
func buildMCPCLIPromptSection(data *WorkflowData) *PromptSection {
servers := getMCPCLIServerNames(data)
if len(servers) == 0 {
return nil

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnose] The nil-guard uses getMCPCLIServerNames(data) — configured servers — not the runtime mount result. In the partial-failure scenario where all configured servers fail to mount, this prompt section is still generated; at runtime ${{ steps.mount-mcp-clis.outputs.mounted-servers-list }} resolves to "", so the model receives the "CLI servers available on PATH" preamble with an empty bullet list.

💡 Suggested mitigation

This is an edge case, but could produce confusing model behaviour. Options:

  1. Document it in the comment — note that a partial-mount failure may produce an empty server list in the rendered prompt.
  2. Template-level guard — if mcp_cli_tools_prompt.md can wrap the bullet list in a conditional block ({% if GH_AW_MCP_CLI_SERVERS_LIST %}...{% endif %}), the empty-list case degrades cleanly.
  3. Runtime guard — have the step skip setting the mounted-servers-list output (or emit a sentinel value) when the list is empty, so downstream consumers can detect the empty case.

}

// Build the human-readable list of servers with example usage
var listLines []string
for _, name := range servers {
listLines = append(listLines, fmt.Sprintf("- `%s` — run `%s --help` to see available tools", name, name))
}
serversList := strings.Join(listLines, "\n")

return &PromptSection{
Content: mcpCLIToolsPromptFile,
IsFile: true,
EnvVars: map[string]string{
"GH_AW_MCP_CLI_SERVERS_LIST": serversList,
"GH_AW_MCP_CLI_SERVERS_LIST": "${{ steps.mount-mcp-clis.outputs.mounted-servers-list }}",
},
Comment on lines 312 to 316

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnose] The step ID mount-mcp-clis is a magic string embedded in this Go-generated runtime expression. If the step is renamed in the workflow definition, the binding breaks silently — no compile-time check catches the mismatch.

💡 Suggested improvement

Define a Go constant that holds the step ID and reference it when building the expression:

const mcpMountStepID = "mount-mcp-clis"

// ...
"GH_AW_MCP_CLI_SERVERS_LIST": fmt.Sprintf("${{ steps.%s.outputs.mounted-servers-list }}", mcpMountStepID),

This also makes it easier to verify consistency with the step definition elsewhere in the Go layer.

}
}
1 change: 1 addition & 0 deletions pkg/workflow/mcp_cli_mount_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,5 @@ func TestBuildMCPCLIPromptSection_PromptFileUsesNonHeadingLabels(t *testing.T) {
assert.NotRegexp(t, `(?m)^\s*(>\s*)?##\s+`, prompt, "prompt must not contain H2 Markdown headings")
assert.NotRegexp(t, `(?m)^\s*(>\s*)?###\s+`, prompt, "prompt must not contain H3 Markdown headings")
assert.Contains(t, prompt, "Use `<server> --help` for tool names, parameters, and examples before calling any command.")
assert.Equal(t, "${{ steps.mount-mcp-clis.outputs.mounted-servers-list }}", section.EnvVars["GH_AW_MCP_CLI_SERVERS_LIST"])
}

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fragile magic-string assertion will not catch a step/output rename: this only validates the expression is spelled correctly today, not that the env var will be populated at runtime.

💡 Details

The new assertion:

assert.Equal(t, "${{ steps.mount-mcp-clis.outputs.mounted-servers-list }}", section.EnvVars["GH_AW_MCP_CLI_SERVERS_LIST"])

has no compile-time or test-time link to the step ID mount-mcp-clis or the output key mounted-servers-list generated elsewhere in the compiler. If either name drifts (e.g., step is renamed to mount-mcp-cli-tools), the generated YAML silently produces an empty server list and this test continues to pass with exit 0.

Suggested fix: extract the expression to a named constant in the package (e.g., const mcpCLIMountedServersListExpr = "${{ steps.mount-mcp-clis.outputs.mounted-servers-list }}"), use it in both the production code and the test, and add a TestMain or init-time assertion that the step ID constant matches what the compiler actually emits for the mount step. That way a rename breaks in exactly one place.

Loading