Upgrade copilot CLI library and compatability#28
Conversation
There was a problem hiding this comment.
Pull request overview
Upgrades the GitHub Copilot SDK dependency and adapts the internal Copilot CLI client wrapper to the updated SDK APIs so the project remains compatible with the latest Copilot CLI.
Changes:
- Bump
github.com/github/copilot-sdk/gotov0.2.1-preview.1. - Update Copilot client calls to the new context-aware SDK method signatures (e.g.,
Start,Ping,CreateSession,Send,Disconnect). - Simplify shutdown handling to the new
Stop()error contract.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/copilotcli/client.go | Updates SDK integration code to new context-based APIs and session lifecycle methods. |
| go.mod | Bumps Copilot SDK dependency version. |
| go.sum | Updates checksums for the new Copilot SDK version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := c.client.Start(context.Background()); err != nil { | ||
| return fmt.Errorf("failed to start Copilot client: %w", err) | ||
| } | ||
|
|
||
| // Verify connectivity with ping | ||
| _, err := c.client.Ping("health-check") | ||
| _, err := c.client.Ping(context.Background(), "health-check") |
There was a problem hiding this comment.
Start() and Ping() are called with context.Background(), so if the CLI blocks/hangs during startup or health-check this can hang indefinitely and cannot be cancelled by the caller. Consider threading a caller-provided context into Start(ctx) (preferred) or at least using a context.WithTimeout for both calls (and returning a timeout error) to avoid stuck orchestrations.
| if err != nil { | ||
| c.client.Stop() | ||
| _ = c.client.Stop() | ||
| return fmt.Errorf("Copilot client ping failed: %w", err) |
There was a problem hiding this comment.
On ping failure, the shutdown error from c.client.Stop() is discarded. Since Stop() now returns a single error, it would be better to capture and log (or wrap) it so we don’t silently leak a running CLI process when ping fails.
| session, err := c.client.CreateSession(ctx, &copilot.SessionConfig{ | ||
| Model: model, | ||
| Streaming: true, | ||
| OnPermissionRequest: copilot.PermissionHandler.ApproveAll, | ||
| }) |
There was a problem hiding this comment.
OnPermissionRequest: copilot.PermissionHandler.ApproveAll will automatically approve any permission prompts from the CLI. If permission requests can include filesystem/network/tool execution, this becomes a broad escalation surface. Consider making this configurable (e.g., default deny/interactive prompt, with an explicit --auto-approve flag for CI), and/or scoping approvals to the minimum set of permissions required for chunk execution.
| session, err := c.client.CreateSession(ctx, &copilot.SessionConfig{ | ||
| Model: model, | ||
| Streaming: true, | ||
| OnPermissionRequest: copilot.PermissionHandler.ApproveAll, | ||
| }) |
There was a problem hiding this comment.
OnPermissionRequest: copilot.PermissionHandler.ApproveAll auto-approves all permission prompts for the summary session as well. Even if chunk execution is trusted, summary generation likely doesn’t require extra permissions; consider using a more restrictive handler here or reusing a configurable policy to avoid unnecessary blanket approvals.
|
When I first tried this, I ran into this issue on starting a job: When I switched to |
Done