[go] support bundling and auto installing the GitHub Copilot CLI #414
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Cross-SDK Consistency ReviewSummaryThis PR introduces CLI bundling/embedding capability exclusively to the Go SDK, creating a significant feature disparity across the SDK implementations. While this is an excellent feature for Go users, the same capability would be valuable for Node.js, Python, and .NET SDKs. Current StateGo SDK (this PR):
Node.js SDK:
Python SDK:
.NET SDK:
Consistency ImpactRequirements sections now differ: Go SDK (after this PR): ## Distributing your application with an embedded GitHub Copilot CLI
The SDK supports bundling, using Go's `embed` package, the Copilot CLI binary within your application's distribution.
This allows you to bundle a specific CLI version and avoid external dependencies on the user's system.All other SDKs: ## Requirements
- [Language runtime]
- GitHub Copilot CLI installed and in PATH (or provide custom path)RecommendationsOption 1: Track feature parity (Recommended)Accept this PR as-is since it's Go-specific infrastructure, but create tracking issues to implement equivalent functionality in other SDKs: Node.js: Could use npm's optional dependencies or postinstall scripts to download platform-specific binaries Option 2: Document the differenceIf bundling is intentionally Go-only (due to Go's superior embedding capabilities), explicitly document this as a Go-specific advantage in the main README to set user expectations. Why This MattersDistribution simplification: This feature allows Go SDK users to ship applications without external CLI dependencies, significantly improving the deployment experience. Users of other SDKs would benefit equally from this capability. Version pinning: Embedding specific CLI versions ensures consistent behavior across deployments, which is valuable regardless of language. Offline environments: Bundled CLIs work in airgapped/restricted environments where downloading dependencies isn't possible. Suggested Next Steps
Would you like me to help create those tracking issues or provide implementation guidance for the other languages?
|
Cross-SDK Consistency Review: CLI Bundling FeatureI've reviewed PR #414 which adds CLI embedding/bundling functionality to the Go SDK. Here's the consistency analysis across all four SDK implementations: ✅ Good News: Feature Parity Already Exists!All four SDKs already have CLI bundling/embedding functionality, though implemented using different approaches appropriate to each ecosystem:
📋 Implementation ComparisonGo SDK (this PR)
Node.js/TypeScript SDK
Python SDK
.NET SDK
🎯 Consistency AssessmentAPI Consistency: ✅ All SDKs follow the same priority order:
Naming Consistency: ✅ Follows language conventions:
💡 Suggestions for ImprovementWhile this PR maintains cross-SDK consistency, consider these non-blocking documentation improvements across all SDKs:
✅ ConclusionThis PR maintains excellent cross-SDK consistency. The Go implementation uses idiomatic Go patterns (embed package, build tools) while achieving the same user-facing behavior as other SDKs. The feature allows users across all platforms to:
No blocking issues found. The implementation differences are appropriate ecosystem adaptations rather than inconsistencies.
|
There was a problem hiding this comment.
Pull request overview
Adds Go SDK support for bundling and automatically installing a GitHub Copilot CLI binary embedded into an application, so users don’t need a separate CLI installation.
Changes:
- Introduces a small cross-platform file-locking helper (
internal/flock) with tests. - Adds an embedded CLI installer (
internal/embeddedcli) plus a small public wrapper package (embeddedcli) and hooksNewClientto prefer the embedded CLI when available. - Adds a
go/cmd/bundlertool to download the CLI from npm, zstd-compress it, and generate Go source that embeds the artifacts; updates Go module deps and README.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| go/internal/flock/flock.go | Adds Acquire API for lockfiles (currently has an open-flag bug). |
| go/internal/flock/flock_unix.go | Unix flock implementation. |
| go/internal/flock/flock_windows.go | Windows LockFileEx/UnlockFileEx implementation. |
| go/internal/flock/flock_other.go | Stub implementation for unsupported platforms. |
| go/internal/flock/flock_test.go | Concurrency + idempotent release tests for flock. |
| go/internal/embeddedcli/embeddedcli.go | Implements lazy install + lookup of embedded CLI (hashing/dir choice need hardening). |
| go/internal/embeddedcli/embeddedcli_test.go | Tests for setup constraints, install, version sanitization, and hash mismatch behavior. |
| go/embeddedcli/installer.go | Public wrapper exposing embeddedcli.Setup + config type alias. |
| go/cmd/bundler/main.go | New bundler tool to download, compress, and generate embed-ready Go code (has a panic bug + networking/resource concerns). |
| go/client.go | Uses embedded CLI path when COPILOT_CLI_PATH is unset and default CLIPath is used (needs guard for external-server mode). |
| go/README.md | Documents embedding flow via bundler tool. |
| go/go.mod | Adds zstd dependency for generated code. |
| go/go.sum | Adds checksums for new dependency. |
Comments suppressed due to low confidence (3)
go/internal/embeddedcli/embeddedcli.go:130
- After writing
config.Clito disk, the code never verifies the written file matchesconfig.CliHash. This defeats the purpose of shipping a hash and allows silent corruption (or unexpected reader contents). Compute/compare the SHA-256 while writing (or immediately after), and return an error on mismatch (ideally via an atomic temp-file+rename install).
f, err := os.OpenFile(finalPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0755)
if err != nil {
return "", fmt.Errorf("creating binary file: %w", err)
}
_, err = io.Copy(f, config.Cli)
go/internal/embeddedcli/embeddedcli.go:123
- If
finalPathexists but has the wrong hash,installAtreturns an error and gives up. A previous interrupted install (partial file) would then permanently break embedded CLI usage. Consider a safer install strategy (temp file + rename) and a recovery path for hash mismatches (e.g., delete+reinstall in a trusted per-user dir or surface a clearer remediation).
}
if !bytes.Equal(existingHash, config.CliHash) {
return "", fmt.Errorf("existing binary hash mismatch")
}
return finalPath, nil
go/cmd/bundler/main.go:527
- File handle may be writable as a result of data flow from a call to OpenFile and closing it may result in data loss upon failure, which is not handled explicitly.
outFile.Close()
| func Setup(cfg Config) { | ||
| if cfg.Cli == nil { | ||
| panic("Cli reader is required") | ||
| } |
There was a problem hiding this comment.
Config docs say CliHash is required, but Setup only validates Cli. If CliHash is empty/wrong length, later comparisons are meaningless and corrupted/tampered installs won't be detected. Validate CliHash in Setup (e.g., non-empty and expected SHA-256 length) and fail fast with a clear panic/message.
This issue also appears in the following locations of the same file:
- line 126
- line 119
| } | |
| } | |
| if len(cfg.CliHash) != sha256.Size { | |
| panic(fmt.Sprintf("CliHash must be a SHA-256 hash (%d bytes), got %d bytes", sha256.Size, len(cfg.CliHash))) | |
| } |
| resp, err := http.Get(url) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to fetch: %w", err) | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
This tool uses http.Get, which has no request timeout and can hang indefinitely on stalled connections. Prefer an http.Client{Timeout: ...} and/or context-aware requests, and apply that consistently to all downloads in this tool (package-lock fetch, CLI tarball, and license tarball).
| // Check environment variable for CLI path | ||
| if cliPath := os.Getenv("COPILOT_CLI_PATH"); cliPath != "" { | ||
| opts.CLIPath = cliPath | ||
| } else if embeddedPath := embeddedcli.Path(); embeddedPath != "" && opts.CLIPath == "copilot" { | ||
| // Use the unpacked embedded CLI if available and no custom path was set | ||
| opts.CLIPath = embeddedPath |
There was a problem hiding this comment.
embeddedcli.Path() is evaluated even when CLIUrl is set (external server mode). In that case the client won’t spawn a local CLI process, so this can trigger an unnecessary embedded CLI install/unpack. Consider guarding this branch with !client.isExternalServer (or opts.CLIUrl == "") so embedded installation only happens when CLIPath will actually be used.
| // Acquire opens (or creates) the lock file at path and blocks until the lock is acquired. | ||
| // It returns a release function to unlock and close the file. | ||
| func Acquire(path string) (func() error, error) { | ||
| f, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC, 0644) |
There was a problem hiding this comment.
Acquire opens the lock file with os.O_CREATE|os.O_TRUNC but without an access mode (O_RDWR/O_WRONLY). In Go this means read-only (O_RDONLY) and combining that with O_TRUNC can fail on many platforms, causing locking to never work. Open the file with a write-capable flag (typically O_RDWR|O_CREATE) and consider dropping O_TRUNC since lockfiles generally shouldn't be truncated on each acquisition.
| f, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC, 0644) | |
| f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0644) |
| installDir := config.Dir | ||
| if installDir == "" { | ||
| installDir = os.TempDir() | ||
| } |
There was a problem hiding this comment.
When Dir is empty, the binary is installed directly into os.TempDir() (often a shared, world-writable location like /tmp). Installing the executable directly there increases collision/DoS risk between different apps/users and makes it easier for other local users to interfere. Consider defaulting to a per-user directory (e.g. os.UserCacheDir()/os.UserConfigDir()) and/or a dedicated subdirectory with restrictive permissions.
| stat, _ := os.Stat(binaryPath) | ||
| sizeMB := float64(stat.Size()) / 1024 / 1024 | ||
| fmt.Printf("Downloaded %s (%.1f MB)\n", binaryName, sizeMB) |
There was a problem hiding this comment.
downloadCLIBinary ignores the error from os.Stat(binaryPath) and then unconditionally uses stat.Size(). If os.Stat fails, stat is nil and this will panic. Handle the error (or reuse the earlier successful os.Stat result) before computing size.
| r, err := zstd.NewReader(bytes.NewReader(localEmbeddedCopilotCLI)) | ||
| if err != nil { | ||
| panic("failed to create zstd reader: " + err.Error()) | ||
| } | ||
| return r |
There was a problem hiding this comment.
The generated cliReader() creates a zstd decoder via zstd.NewReader(...) but never closes it. Since embeddedcli.Config only accepts an io.Reader, there’s currently no way for the installer to call Close, which can leak resources. Consider changing the config to accept an io.ReadCloser (or closing Cli when it implements io.Closer) and update the generated code accordingly.
| return fmt.Errorf("failed to create output file: %w", err) | ||
| } | ||
| if _, err := io.Copy(outFile, r); err != nil { | ||
| outFile.Close() |
There was a problem hiding this comment.
File handle may be writable as a result of data flow from a call to OpenFile and closing it may result in data loss upon failure, which is not handled explicitly.
This issue also appears on line 527 of the same file.
| // | ||
| // --platform: Target platform using Go conventions (linux/amd64, linux/arm64, darwin/amd64, darwin/arm64, windows/amd64, windows/arm64). Defaults to current platform. | ||
| // --output: Output directory for embedded artifacts. Defaults to the current directory. | ||
| // --cli-version: CLI version to download. If not specified, automatically detects from the copilot-sdk version in go.mod. |
There was a problem hiding this comment.
Is there something that forces developers to realise if their local bundled version is out of date?
2 participants
This pull request introduces support for embedding the GitHub Copilot CLI binary directly within Go applications using the SDK, removing the need for users to have the CLI installed separately. It adds a robust mechanism for embedding, unpacking, and managing the Copilot CLI binary, including versioning, file locking for concurrency, and hash validation. The changes also include comprehensive tests and documentation updates.