Use actual mounted MCP CLI wrappers so AI Moderator safe outputs remain reliable#39243
Use actual mounted MCP CLI wrappers so AI Moderator safe outputs remain reliable#39243Copilot wants to merge 3 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot fix the ai moderator or the cli generator instead . |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in 67ce9d1. I moved the fix to the CLI generator path: the MCP CLI prompt list now comes from |
There was a problem hiding this comment.
Pull request overview
This PR aims to keep the MCP CLI tools prompt consistent with runtime reality by advertising only MCP CLI wrappers that were actually mounted, preventing “no safe outputs generated” failures caused by the model being told wrappers exist when they don’t.
Changes:
- Adds a new
mounted-servers-liststep output frommount_mcp_as_cli.cjsfor prompt-ready bullet formatting. - Updates MCP CLI prompt section wiring to use the mount step output instead of a compile-time generated list.
- Extends unit tests (Go + JS) to validate the new output binding and formatting.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/mcp_cli_mount.go | Switches prompt env var binding to reference the mount step output. |
| pkg/workflow/mcp_cli_mount_test.go | Updates expectations to assert the new env var wiring. |
| actions/setup/js/mount_mcp_as_cli.cjs | Adds prompt-list formatting helper and exports it; emits new mounted-servers-list output. |
| actions/setup/js/mount_mcp_as_cli.test.cjs | Adds test coverage for prompt list formatting. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| 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 }}", | ||
| }, |
| function formatMountedServersPromptList(mountedServers) { | ||
| return mountedServers.map(name => `- \`${name}\` — run \`${name} --help\` to see available tools`).join("\n"); | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39243 does not have the 'implementation' label and has only 7 new lines in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![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 /diagnose and /tdd — approving with a few non-blocking observations. The core correctness fix is sound and the data-flow shift from compile-time to runtime is the right approach.
📋 Key Themes & Highlights
Key Themes
- One edge case to consider: the nil-guard in
buildMCPCLIPromptSectionuses configured servers (compile-time), so an all-servers-fail-to-mount scenario at runtime will still generate the prompt section — but with an empty bullet list. Not a blocker, but worth a follow-up. - Magic step ID:
mount-mcp-clisis embedded as a string literal in the Go-generated expression — a constant would make renaming safe. - PR description vs. diff: The body mentions removing a fallback sentence from
mcp_cli_tools_prompt.md, but that file is not in the diff. Confirm whether this was done in an earlier commit on the branch or is still pending.
Positive Highlights
- ✅
formatMountedServersPromptListis a clean, pure function — easy to test and reason about - ✅ Empty-array edge case is explicitly covered in the new test
- ✅ Go simplification (removing the compile-time list-building loop) is a welcome side effect
- ✅ The fix is at the right layer — runtime output, not prompt wording
- ✅ Both JS and Go test layers updated consistently
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 232 AIC · ⌖ 14.2 AIC · ⊞ 27.9K
| func buildMCPCLIPromptSection(data *WorkflowData) *PromptSection { | ||
| servers := getMCPCLIServerNames(data) | ||
| if len(servers) == 0 { | ||
| return nil |
There was a problem hiding this comment.
[/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:
- Document it in the comment — note that a partial-mount failure may produce an empty server list in the rendered prompt.
- Template-level guard — if
mcp_cli_tools_prompt.mdcan wrap the bullet list in a conditional block ({% if GH_AW_MCP_CLI_SERVERS_LIST %}...{% endif %}), the empty-list case degrades cleanly. - Runtime guard — have the step skip setting the
mounted-servers-listoutput (or emit a sentinel value) when the list is empty, so downstream consumers can detect the empty case.
| 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 }}", | ||
| }, |
There was a problem hiding this comment.
[/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.
| }); | ||
| }); | ||
|
|
||
| it("formats mounted servers for prompt rendering", () => { |
There was a problem hiding this comment.
[/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("");
});
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both the new JS test (formatMountedServersPromptList with normal + empty-input edge case) and the Go assertion extension (verifying EnvVars["GH_AW_MCP_CLI_SERVERS_LIST"] contract) are design tests that enforce real behavioral contracts.
There was a problem hiding this comment.
Two blocking concerns; the fix does not achieve its stated goal in the common case.\n\n
### Blocking themes
\n\nActivation-job expression resolution (critical, already flagged inline) —GH_AW_MCP_CLI_SERVERS_LIST is wired to ${{ steps.mount-mcp-clis.outputs.mounted-servers-list }}, but that expression is evaluated inside the activation job, which has no mount-mcp-clis step. The env var resolves to an empty string there. The prompt section is still emitted (guarded only by the config-time server count, not the runtime count), so the model receives a prompt advertising CLI tools with a blank server list.\n\nFragile literal-string test — the new assertion in mcp_cli_mount_test.go validates only that the expression string is spelled as written today. A step or output rename causes a silent runtime regression that tests do not catch (see inline comment).\n\n🔎 Code quality review by PR Code Quality Reviewer · 227.5 AIC · ⌖ 13.3 AIC · ⊞ 16.7K
| 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"]) | ||
| } |
There was a problem hiding this comment.
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.
AI Moderator could complete without safe outputs when MCP CLI wrappers were advertised in the prompt but not actually mounted at runtime, causing a “no safe outputs generated” failure path in an otherwise successful run.
CLI generator/runtime alignment fix
mounted-servers-listoutput inmount_mcp_as_cli.cjsderived from successfully mounted wrappers.steps.mount-mcp-clis.outputs.mounted-servers-listforGH_AW_MCP_CLI_SERVERS_LIST, so only real, mounted wrappers are advertised to the model.Prompt/test updates
mcp_cli_tools_prompt.md(no longer needed as primary mitigation).This keeps prompt guidance consistent with runtime capability and prevents false missing-wrapper paths from steering successful runs into missing-safe-output handling.