-
Notifications
You must be signed in to change notification settings - Fork 815
[go] support bundling and auto installing the GitHub Copilot CLI #414
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
base: main
Are you sure you want to change the base?
Changes from all commits
3b043c9
9fd038d
4587441
eb0db18
8647722
ba321bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ import ( | |
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/github/copilot-sdk/go/internal/embeddedcli" | ||
| "github.com/github/copilot-sdk/go/internal/jsonrpc2" | ||
| ) | ||
|
|
||
|
|
@@ -102,7 +103,7 @@ type Client struct { | |
| // }) | ||
| func NewClient(options *ClientOptions) *Client { | ||
| opts := ClientOptions{ | ||
| CLIPath: "copilot", | ||
| CLIPath: "", | ||
| Cwd: "", | ||
| Port: 0, | ||
| LogLevel: "info", | ||
|
|
@@ -1313,6 +1314,15 @@ func (c *Client) verifyProtocolVersion(ctx context.Context) error { | |
| // This spawns the CLI server as a subprocess using the configured transport | ||
| // mode (stdio or TCP). | ||
| func (c *Client) startCLIServer(ctx context.Context) error { | ||
| cliPath := c.options.CLIPath | ||
| if cliPath == "" { | ||
| // If no CLI path is provided, attempt to use the embedded CLI if available | ||
| cliPath = embeddedcli.Path() | ||
| } | ||
| if cliPath == "" { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good consistency: This fallback logic (embedded → "copilot" in PATH) matches the behavior in other SDKs nicely. However, when the CLI is not found, consider if the error message should mention running
A similar helpful message for Go might be:
|
||
| // Default to "copilot" in PATH if no embedded CLI is available and no custom path is set | ||
| cliPath = "copilot" | ||
| } | ||
| args := []string{"--headless", "--no-auto-update", "--log-level", c.options.LogLevel} | ||
|
|
||
| // Choose transport mode | ||
|
|
@@ -1339,10 +1349,10 @@ func (c *Client) startCLIServer(ctx context.Context) error { | |
|
|
||
| // If CLIPath is a .js file, run it with node | ||
| // Note we can't rely on the shebang as Windows doesn't support it | ||
| command := c.options.CLIPath | ||
| if strings.HasSuffix(c.options.CLIPath, ".js") { | ||
| command := cliPath | ||
| if strings.HasSuffix(cliPath, ".js") { | ||
| command = "node" | ||
| args = append([]string{c.options.CLIPath}, args...) | ||
| args = append([]string{cliPath}, args...) | ||
| } | ||
|
|
||
| c.process = exec.CommandContext(ctx, command, args...) | ||
|
|
||
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.
Cross-SDK consistency note: Consider adding a comparison section here explaining why Go requires manual bundling while Node.js, Python, and .NET SDKs have the CLI automatically bundled.
For example:
> **Note**: Unlike the Node.js, Python, and .NET SDKs where the CLI is automatically bundled with the package, the Go SDK requires you to explicitly run the bundler tool. This keeps the Go module size small and gives you control over which CLI version to embed.This would help users understand the different approach and set appropriate expectations.