Fix compiler emission bug: mount-mcp-clis step reference out of scope in generated lock files - #47235
Conversation
… in activation job
Replace the out-of-scope step output reference
`${{ steps.mount-mcp-clis.outputs.mcp-cli-servers-list }}` with a
compile-time static list of CLI server names in `buildMCPCLIPromptSection`.
The `mount-mcp-clis` step exists only in the `agent` job, but
`GH_AW_MCP_CLI_SERVERS_LIST` was being emitted in the `activation` job's
env blocks (both "Create prompt" and "Substitute placeholders" steps),
triggering 520 actionlint `property "mount-mcp-clis" is not defined` errors
across all 260 lock files.
Since the server names are known at compile time from `getMCPCLIServerNames`,
build a static `- \`server\` — run \`server --help\` ...` list instead.
This also eliminates the unnecessary env entry from the "Create prompt" step.
Closes #47224
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes invalid cross-job MCP CLI step references in compiled workflows.
Changes:
- Emits compile-time MCP CLI server lists.
- Updates regression coverage.
- Regenerates 260 workflow lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_cli_mount.go |
Generates static server guidance. |
pkg/workflow/mcp_cli_mount_test.go |
Verifies static output. |
.github/workflows/*.lock.yml (260 files) |
Removes invalid references and embeds server lists. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 262/262 changed files
- Comments generated: 1
- Review effort level: Medium
| IsFile: true, | ||
| EnvVars: map[string]string{ | ||
| "GH_AW_MCP_CLI_SERVERS_LIST": "${{ steps.mount-mcp-clis.outputs.mcp-cli-servers-list }}", | ||
| "GH_AW_MCP_CLI_SERVERS_LIST": strings.Join(lines, "\n"), |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47235 does not have the 'implementation' label and has only 21 new lines of code in business logic directories (threshold: 100). |
|
✅ 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.
The fix is correct and well-executed. Removing the cross-job ${{ steps.mount-mcp-clis.outputs.mcp-cli-servers-list }} reference (which was always empty at runtime and triggered 520 actionlint errors) in favour of a compile-time static list is a strict improvement. Lock files are consistently regenerated across all 260 workflows. No new blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 24.5 AIC · ⌖ 7.28 AIC · ⊞ 5K
|
Test comment
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 86/100. 14% implementation tests (threshold: 30%). No violations. Tests in pkg/workflow/mcp_cli_mount_test.go cover key behavioral contracts: restricted-bash allowlist logic, CLI command injection rules for playwright/github modes, absence of GitHub Actions expressions in env vars, and MCP server name assembly per engine type. Build tag present, no forbidden mocks.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking issues, but two follow-up items are worth addressing.
📋 Key Themes & Highlights
Key Themes
- Correct root-cause fix: replacing the cross-job
${{ steps.mount-mcp-clis.outputs... }}reference with a compile-time static list directly addresses both the actionlint errors and the silent empty-string substitution at runtime. - Test coverage: the existing test in
mcp_cli_mount_test.govalidates the in-memoryEnvVarsstring, but the YAML serialization path (compiler_github_actions_steps.gomulti-line escape) is not covered end-to-end. - Static list vs runtime reality: the
continue-on-error: truemount step means the advertised list may not match what actually mounted. The prompt text could soften this with a brief fallback note for agents.
Positive Highlights
- ✅ Clean, well-commented fix with a clear explanation in the code comment and commit message
- ✅
getMCPCLIServerNamesreuse keeps a single source of truth for which servers are eligible - ✅ Good regression test assertions (
NotContains "${{",Contains "--help",Contains "safeoutputs") - ✅ All 260 lock files regenerated — zero actionlint errors
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 79.8 AIC · ⌖ 4.77 AIC · ⊞ 6.7K
Comment /matt to run again
| assert.NotEmpty(t, serversList, "server list must not be empty") | ||
| assert.NotContains(t, serversList, "${{", "server list must not use a GitHub Actions expression (step output reference is out of scope in activation job)") | ||
| assert.Contains(t, serversList, "safeoutputs", "server list must mention the safeoutputs server") | ||
| assert.Contains(t, serversList, "--help", "server list must guide agents to use --help for tool signatures") |
There was a problem hiding this comment.
[/tdd] The test checks the in-memory EnvVars string but does not compile a full workflow and assert the generated YAML env block — a regression in the multi-line escape path in compiler_github_actions_steps.go would go undetected.
💡 Suggested addition
Add a compiler-level test that compiles a minimal workflow with safeoutputs enabled and asserts the Substitute placeholders step YAML contains a correctly-escaped GH_AW_MCP_CLI_SERVERS_LIST (no ${{, contains safeoutputs, is valid YAML). The multi-line value traverses the single-quote wrapping → \n-escape path in generatePlaceholderSubstitutionStep — that path has no targeted coverage today.
@copilot please address this.
| IsFile: true, | ||
| EnvVars: map[string]string{ | ||
| "GH_AW_MCP_CLI_SERVERS_LIST": "${{ steps.mount-mcp-clis.outputs.mcp-cli-servers-list }}", | ||
| "GH_AW_MCP_CLI_SERVERS_LIST": strings.Join(lines, "\n"), |
There was a problem hiding this comment.
[/diagnosing-bugs] The continue-on-error: true on the mount step means the server list may advertise tools that were never actually mounted (e.g. gateway startup failure). Agents that try to call an unlisted server will get an unhelpful "command not found" error with no guidance.
💡 Options
This is the trade-off the existing PR comment (#3628789334) flags. The simplest mitigation is a note in the prompt text itself (e.g. "if a command is not found, the mount step may have failed — check the mount-mcp-clis step logs") so agents know how to triage the failure rather than retrying silently.
@copilot please address this.
There was a problem hiding this comment.
Review: Fix is correct, two maintainability concerns to address
The core bug fix is sound: replacing the out-of-scope ${{ steps.mount-mcp-clis.outputs.mcp-cli-servers-list }} reference with a compile-time static list is the right approach and eliminates 520 actionlint errors.
Findings
Medium — Hardcoded format string (mcp_cli_mount.go:358): the per-entry bullet format is inlined directly in Go code with no binding to the prompt markdown template. A format change requires updating Go source and regenerating 260 lock files, with no compile-time guard if the two drift.
Medium — Weak test assertions (mcp_cli_mount_test.go:230): the replacement assertions (NotEmpty, Contains "safeoutputs", Contains "--help") don't verify exact entry format or the multi-server newline-join path. A regression in bullet syntax would pass undetected.
The existing comment (id: 3628789334) already covers the continue-on-error runtime divergence concern.
These findings are non-blocking but should be addressed before further changes to the prompt format touch this code path.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 73.8 AIC · ⌖ 4.86 AIC · ⊞ 5.7K
Comment /review to run again
| for i, server := range servers { | ||
| lines[i] = fmt.Sprintf("- `%s` — run `%s --help` to see available tools", server, server) | ||
| } | ||
|
|
There was a problem hiding this comment.
Hardcoded format string baked into 260 lock files has no binding to the prompt template: if the bullet format in mcp_cli_tools_prompt.md ever changes, the Go format string and all 260 lock files silently drift.
💡 Details
The format string:
lines[i] = fmt.Sprintf("- \`%s\` — run \`%s --help\` to see available tools", server, server)is now inlined verbatim into every regenerated .lock.yml (visible across all 260 files in this diff). There is no shared constant or compile-time assertion binding this string to the markdown template's structural expectations.
Consequences:
- Any future change to the desired bullet format requires updating the Go format string and recompiling 260 lock files. Forgetting the recompile silently produces inconsistent agent prompts across workflows.
- It's invisible which format the prompt template actually requires.
Suggested mitigation: extract the per-entry format as a named package-level constant (e.g. mcpCLIServerListEntryFmt) and add a test that validates a rendered entry against that constant, similar to how the prompt file heading checks are already done below.
| assert.Contains(t, serversList, "--help", "server list must guide agents to use --help for tool signatures") | ||
|
|
||
| wd, err := os.Getwd() | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Weak test assertions won't catch format regressions: the three assert.Contains checks allow any string mentioning safeoutputs and --help to pass, including malformed output.
💡 Details
Current assertions:
assert.NotEmpty(t, serversList)
assert.NotContains(t, serversList, "${{"...)
assert.Contains(t, serversList, "safeoutputs")
assert.Contains(t, serversList, "--help")These would all pass for degenerate output like "safeoutputs --help" — no backtick quoting, no bullet prefix, no em-dash separator. The exact per-entry format - \safeoutputs` — run `safeoutputs --help` to see available tools` is never verified.
Additionally, the test only exercises a single-server case. The strings.Join(lines, "\n") path for multiple servers is untested — a bug that dropped the newline separator or merged entries would pass silently.
Suggested fixes:
// Verify exact format for known server
assert.Contains(t, serversList, "- \`safeoutputs\` — run \`safeoutputs --help\` to see available tools")
// Add a multi-server case (e.g. safeoutputs + mcpscripts) and verify two lines separated by \n
|
🎉 This pull request is included in a new release. Release: |
Every compiled
.lock.ymlemittedGH_AW_MCP_CLI_SERVERS_LIST: ${{ steps.mount-mcp-clis.outputs.mcp-cli-servers-list }}in theactivationjob's env blocks, butmount-mcp-clisonly exists in theagentjob — causing 520 actionlintproperty "mount-mcp-clis" is not definederrors across all 260 lock files. Additionally, the expression evaluated to empty string at runtime since the step was never in scope.Changes
pkg/workflow/mcp_cli_mount.go—buildMCPCLIPromptSectionnow builds a compile-time static server list fromgetMCPCLIServerNames(data)instead of referencing a cross-job step output:Because the value is no longer a
${{ }}expression,unified_prompt_step.gono longer emits it in the "Create prompt" step env block — only in "Substitute placeholders" as a static string. This is a strict behavioral improvement: agents now receive actual server names instead of an empty substitution.pkg/workflow/mcp_cli_mount_test.go— Updated assertions to verify the value is a non-empty static string with no${{expression syntax.All 260
.github/workflows/*.lock.yml— Regenerated viamake recompile; zero actionlint expression errors.