-
Notifications
You must be signed in to change notification settings - Fork 51
Add binary path detection for MCP server self-invocation #14140
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.
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
Changes from all commits
0ee5325
0f2a946
3631c9f
2ab7b39
d6aa499
8c2360f
56eb2d4
35b8e5c
56d50f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -108,6 +108,18 @@ func runMCPServer(port int, cmdPath string) error { | |||||||||||||
| mcpLog.Print("Starting MCP server with stdio transport") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // 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 == "" { | ||||||||||||||
|
||||||||||||||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| //go:build !integration | ||
|
|
||
| package cli | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestGetBinaryPath(t *testing.T) { | ||
| t.Run("returns non-empty path", func(t *testing.T) { | ||
| path, err := GetBinaryPath() | ||
| require.NoError(t, err, "Should get binary path without error") | ||
| assert.NotEmpty(t, path, "Binary path should not be empty") | ||
| }) | ||
|
|
||
| t.Run("returns absolute path", func(t *testing.T) { | ||
| path, err := GetBinaryPath() | ||
| require.NoError(t, err, "Should get binary path without error") | ||
| assert.True(t, filepath.IsAbs(path), "Binary path should be absolute") | ||
| }) | ||
|
|
||
| t.Run("returned path exists", func(t *testing.T) { | ||
| path, err := GetBinaryPath() | ||
| require.NoError(t, err, "Should get binary path without error") | ||
|
|
||
| // Check if the file exists | ||
| info, err := os.Stat(path) | ||
| require.NoError(t, err, "Binary file should exist at the returned path") | ||
| assert.False(t, info.IsDir(), "Binary path should not be a directory") | ||
| }) | ||
|
|
||
| t.Run("path ends with executable name", func(t *testing.T) { | ||
| path, err := GetBinaryPath() | ||
| require.NoError(t, err, "Should get binary path without error") | ||
|
|
||
| // The path should end with a reasonable executable name | ||
| // During tests, it might be a test binary name | ||
| base := filepath.Base(path) | ||
| assert.NotEmpty(t, base, "Binary path should have a base name") | ||
| // Don't check for specific name as it could be the test binary | ||
| }) | ||
|
|
||
| t.Run("resolves symlinks", func(t *testing.T) { | ||
| path, err := GetBinaryPath() | ||
| require.NoError(t, err, "Should get binary path without error") | ||
|
|
||
| // The path should be the resolved path, not a symlink | ||
| // We can verify this by checking that EvalSymlinks returns the same path | ||
| resolved, err := filepath.EvalSymlinks(path) | ||
| if err == nil { | ||
| // If we can resolve symlinks, the path should already be resolved | ||
| assert.Equal(t, path, resolved, "Path should already be resolved (no symlinks)") | ||
| } | ||
| // If EvalSymlinks fails, that's OK - the original path is still valid | ||
| }) | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -185,7 +185,8 @@ func renderAgenticWorkflowsMCPConfigWithOptions(yaml *strings.Builder, isLast bo | |||||||||||
|
|
||||||||||||
| if actionMode.IsDev() { | ||||||||||||
| // Dev mode: Use locally built Docker image which includes gh-aw binary and gh CLI | ||||||||||||
| // The Dockerfile sets ENTRYPOINT ["gh-aw"] and CMD ["mcp-server", "--cmd", "gh-aw"] | ||||||||||||
| // The Dockerfile sets ENTRYPOINT ["gh-aw"] and CMD ["mcp-server"] | ||||||||||||
| // Binary path is automatically detected via os.Executable() | ||||||||||||
| // So we don't need to specify entrypoint or entrypointArgs | ||||||||||||
|
Comment on lines
+189
to
190
|
||||||||||||
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 35b8e5c. The comment is now accurate - the binary path detection is fully wired up to use the detected path via the
cmdPathvariable.