Add binary path detection for MCP server self-invocation#14140
Add binary path detection for MCP server self-invocation#14140
Conversation
- Added GetBinaryPath() helper to determine gh-aw binary path using os.Executable() - Added runtime file existence check in startup path - Added logging of binary path in runMCPServer() and validateMCPServerConfiguration() - Added comprehensive tests for binary path detection - Binary path is now logged at MCP server startup for debugging Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Changed assert.NoError to require.NoError for error assertions - Complies with testifylint requirements Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Added test to verify symlinks are properly resolved - Documents behavior for gh CLI extension symlinks Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Removed redundant error check in test (require.NoError already ensures no error) - Extracted binary path validation logic into logAndValidateBinaryPath() helper - Eliminates code duplication between runMCPServer and validateMCPServerConfiguration - Improves maintainability with single source of truth for validation logic Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Removed redundant filepath.Abs() call (EvalSymlinks already returns absolute path) - Removed duplicate error logging (logAndValidateBinaryPath handles all logging internally) - Improved code clarity with better comments - All tests pass, linting passes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
- Only call logAndValidateBinaryPath() when cmdPath is empty (not provided by user) - Updated both runMCPServer() and validateMCPServerConfiguration() to skip detection - Removed --cmd gh-aw from Dockerfile CMD line (binary path now auto-detected) - Updated comment in mcp_config_builtin.go to reflect new CMD without --cmd arg - When --cmd is provided, user explicitly specifies binary path, so auto-detection is unnecessary Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit 56eb2d4:
Behavior:
|
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
❌ Changeset Generator failed. Please review the logs for details. |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
Agent Container Tool Check
Result: 10/12 tools available ❌ Missing tools:
|
There was a problem hiding this comment.
Pull request overview
This pull request adds binary path detection for the MCP server to support Docker container mounts and gh CLI extension symlinks. However, there is a critical implementation issue: while the binary path is successfully detected and logged, it is never actually used to invoke subprocesses, rendering the feature non-functional.
Changes:
- Added
GetBinaryPath()function to detect the currently running binary path with symlink resolution - Added
logAndValidateBinaryPath()helper for validation and logging - Integrated binary path detection into
runMCPServer()andvalidateMCPServerConfiguration() - Removed
--cmd gh-awfrom Dockerfile CMD to rely on automatic detection - Added comprehensive unit tests for
GetBinaryPath()
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/mcp_validation.go | Implements GetBinaryPath() and logAndValidateBinaryPath() functions, but doesn't return the detected path for use |
| pkg/cli/mcp_validation_test.go | Comprehensive unit tests for GetBinaryPath() covering symlink resolution, absolute paths, and file existence |
| pkg/cli/mcp_server.go | Calls logAndValidateBinaryPath() when cmdPath is empty but doesn't assign the detected path to cmdPath |
| Dockerfile | Removes --cmd flag from CMD, expecting automatic detection (but detection isn't actually used) |
| pkg/workflow/mcp_config_builtin.go | Updates comment claiming automatic detection works (but it doesn't) |
Comments suppressed due to low confidence (1)
pkg/cli/mcp_validation.go:75
- The function should return the binary path in addition to the error, so that callers can actually use the detected path. Currently, the function only logs and validates the path but doesn't return it, making it impossible for callers to use the detected value.
Consider changing the function signature to:
func logAndValidateBinaryPath() (string, error)And updating the implementation to:
return binaryPath, nil // at the end instead of return nil// logAndValidateBinaryPath determines the binary path, logs it, and validates it exists.
// Returns an error if the binary path cannot be determined or if the file doesn't exist.
// This is a helper used by both runMCPServer and validateMCPServerConfiguration.
func logAndValidateBinaryPath() error {
binaryPath, err := GetBinaryPath()
if err != nil {
mcpValidationLog.Printf("Warning: failed to get binary path: %v", err)
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Warning: failed to get binary path: %v", err)))
return err
}
// Check if the binary file exists
if _, err := os.Stat(binaryPath); err != nil {
if os.IsNotExist(err) {
mcpValidationLog.Printf("ERROR: binary file does not exist at path: %s", binaryPath)
fmt.Fprintln(os.Stderr, console.FormatErrorMessage(fmt.Sprintf("ERROR: binary file does not exist at path: %s", binaryPath)))
return fmt.Errorf("binary file does not exist at path: %s", binaryPath)
}
mcpValidationLog.Printf("Warning: failed to stat binary file at %s: %v", binaryPath, err)
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Warning: failed to stat binary file at %s: %v", binaryPath, err)))
return err
}
// Log the binary path for debugging
mcpValidationLog.Printf("gh-aw binary path: %s", binaryPath)
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("gh-aw binary path: %s", binaryPath)))
return nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Determine, log, and validate the binary path only if --cmd flag is not provided | ||
| // When --cmd is provided, the user explicitly specified the binary path to use | ||
| if cmdPath == "" { |
There was a problem hiding this comment.
The binary path is determined and logged, but never actually used. When cmdPath is empty, createMCPServer(cmdPath) receives an empty string and falls back to calling "gh aw" via workflow.ExecGHContext(), which won't work in Docker containers where gh-aw isn't installed as a gh extension.
The detected binary path should be assigned to cmdPath when it's empty. Consider modifying the code to:
if cmdPath == "" {
detectedPath, err := GetBinaryPath()
if err == nil {
cmdPath = detectedPath
}
// Note: logAndValidateBinaryPath handles all logging internally
_ = logAndValidateBinaryPath()
}This would ensure that createMCPServer(cmdPath) receives the actual binary path and uses it in exec.CommandContext(ctx, cmdPath, args...) instead of falling back to "gh aw".
| if cmdPath == "" { | |
| if cmdPath == "" { | |
| // Attempt to detect the binary path and assign it to cmdPath | |
| if detectedPath, err := GetBinaryPath(); err == nil && detectedPath != "" { | |
| cmdPath = detectedPath | |
| } |
There was a problem hiding this comment.
Fixed in commit 35b8e5c. The detected binary path is now assigned to cmdPath when detection succeeds, ensuring createMCPServer receives and uses the actual binary path instead of falling back to "gh aw".
pkg/cli/mcp_validation.go
Outdated
| // Note: logAndValidateBinaryPath handles all logging internally | ||
| _ = logAndValidateBinaryPath() |
There was a problem hiding this comment.
Similar to the issue in runMCPServer, the binary path is logged but never used. When cmdPath is empty, the validateMCPServerConfiguration function should use the detected binary path for validation instead of falling back to "gh aw" which won't work in environments where gh-aw isn't installed as a gh extension.
Consider returning the detected path from logAndValidateBinaryPath() and using it:
if cmdPath == "" {
detectedPath, err := logAndValidateBinaryPath()
if err == nil {
cmdPath = detectedPath
}
}Then update the function signature:
func logAndValidateBinaryPath() (string, error)| // Note: logAndValidateBinaryPath handles all logging internally | |
| _ = logAndValidateBinaryPath() | |
| // Note: logAndValidateBinaryPath handles all logging internally and returns the detected path | |
| detectedPath, err := logAndValidateBinaryPath() | |
| if err == nil && detectedPath != "" { | |
| cmdPath = detectedPath | |
| } |
There was a problem hiding this comment.
Fixed in commit 35b8e5c. Updated logAndValidateBinaryPath() to return (string, error) and assign the detected path to cmdPath for validation.
| // Binary path is automatically detected via os.Executable() | ||
| // So we don't need to specify entrypoint or entrypointArgs |
There was a problem hiding this comment.
This comment is misleading because the binary path is currently only logged but not actually used. The MCP server still falls back to calling "gh aw" when cmdPath is empty (see createMCPServer function in mcp_server.go line 159-160), which means the automatic detection doesn't actually work as described.
The comment should be updated to reflect the actual behavior once the binary path detection is properly wired up to be used by the MCP server.
| // Binary path is automatically detected via os.Executable() | |
| // So we don't need to specify entrypoint or entrypointArgs | |
| // Note: while the gh-aw binary path may be detected (e.g., via os.Executable()), | |
| // it is currently only logged and not used to override the MCP server command. | |
| // In dev mode we rely on the container's default ENTRYPOINT and CMD instead. |
There was a problem hiding this comment.
Fixed in commit 35b8e5c. The comment is now accurate - the binary path is actually used by the MCP server via the cmdPath variable passed to createMCPServer.
| # Note: Binary path detection is automatic via os.Executable() | ||
| CMD ["mcp-server"] |
There was a problem hiding this comment.
This comment is misleading for the same reason as the comment in mcp_config_builtin.go - the binary path is only logged but not actually used. The MCP server still needs the --cmd flag to be explicitly set, or it will fall back to calling "gh aw" which won't work in Docker.
The comment should be updated once the binary path detection is properly implemented to actually use the detected path.
See below for a potential fix:
# Default command runs MCP server. Note: the MCP server still requires an explicit
# --cmd flag to be set; otherwise it falls back to calling "gh aw", which will not
# work correctly inside this Docker image. Update this comment once binary path
# detection is fully wired to use the detected path.
There was a problem hiding this comment.
Fixed in commit 35b8e5c. The comment is now accurate - the binary path detection is fully wired up to use the detected path via the cmdPath variable.
|
🎭 The smoke test agent was here! 🎭 Testing, testing... 1, 2, 3! 🎤 Just your friendly neighborhood automation bot stopping by to make sure all the MCP servers are playing nicely together. Everything's looking good from my corner of the digital universe! ✨ Beep boop 🤖 Back to the testing mines I go!
|
Smoke Test ResultsPR #14150: [WIP] Remove .github/aw/github-agentic-workflows.md from init command ✅ GitHub MCP Testing Overall Status: PASS cc @pelikhan
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Smoke Test: Claude - PASS ✅
Overall: PASS
|
|
@copilot review comments carefully and fix code |
- Changed logAndValidateBinaryPath() to return (string, error) instead of error - Assign detected binary path to cmdPath variable when detection succeeds - This ensures createMCPServer receives the actual binary path instead of falling back to "gh aw" - Updated both runMCPServer() and validateMCPServerConfiguration() to use returned path - Added debug log when using detected path - Fixes the issue where binary path was logged but never actually used in subprocess invocations Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✅ Changeset Generator completed successfully! |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
Agent Container Tool Check
Result: 10/12 tools available Critical Issues FoundJava and .NET runtimes are experiencing severe system-level execution failures:
This suggests a fundamental system-level process execution problem, possibly:
Recommendation: This requires immediate investigation of the container image and runner environment. The 10 working tools demonstrate the container is functional, but Java and .NET execution paths are critically broken.
|
|
✅ Smoke Test PASS - Run 21754229163 PRs Tested: #14154, #14151 @pelikhan All tests passed!
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Add Dynamic Binary Path Detection in MCP Server ✅
Implementation Complete
This PR implements dynamic binary path detection in the MCP server, addressing the requirement to determine the path to the
gh-awbinary from the currently running Go binary.Changes Made
Added
GetBinaryPath()function inpkg/cli/mcp_validation.go:os.Executable()to get current binary pathfilepath.EvalSymlinks()(important for gh extensions)Added
logAndValidateBinaryPath()helper inpkg/cli/mcp_validation.go:Updated
runMCPServer()inpkg/cli/mcp_server.go:logAndValidateBinaryPath()at startup only when--cmdflag is not provided--cmdis provided, skips auto-detection (user explicitly specified path)Updated
validateMCPServerConfiguration()inpkg/cli/mcp_validation.go:logAndValidateBinaryPath()only when--cmdflag is not providedrunMCPServer()behaviorUpdated Dockerfile:
--cmd gh-awfrom CMD lineCMD ["mcp-server"](binary path auto-detected viaos.Executable())Updated
mcp_config_builtin.go:--cmdargumentos.Executable()Comprehensive test coverage in
pkg/cli/mcp_validation_test.go:Critical Fix Applied
Previous Issue: Binary path was detected and logged but never actually used. The
createMCPServerfunction would still fall back to calling "gh aw" viaworkflow.ExecGHContext(), which doesn't work in Docker containers where gh-aw isn't installed as a gh extension.Fix: The detected binary path is now assigned to the
cmdPathvariable, ensuringcreateMCPServerreceives and uses the actual binary path in subprocess invocations (exec.CommandContext(ctx, cmdPath, args...)).Manual Testing
Without
--cmdflag (auto-detection enabled and used):The detected path (
/home/runner/work/gh-aw/gh-aw/gh-aw) is now used bycreateMCPServer.With
--cmdflag (auto-detection skipped):Binary path detection is skipped, user-provided path is used.
Why These Changes
The MCP server needs to know where the gh-aw binary is located when launching itself with different arguments. This is particularly important for:
When the user provides
--cmd, they explicitly specify the binary path, so automatic detection is unnecessary and should be skipped.Code Quality
Ready for Merge
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Changeset
add_commentas warnings instead of retrying the request as a discussion when the target is gone.