Working around an issue in the copilot SDK, Start() and contexts#53
Conversation
…sed, even after the call completes, can kill the copilot CLI. Issue noted in the comments. - Updating to the latest copilot SDK. - Skipping some tests for now that are impacted by Start() not being able to take a context.
There was a problem hiding this comment.
Pull request overview
This PR works around a bug in the copilot SDK v0.1.30 where Start() uses exec.CommandContext(), causing the copilot CLI to be killed when its context is cancelled — even after Start() has completed. The workaround moves Start() to a dedicated Initialize() method that always calls e.client.Start(context.Background()), bypassing the cancellable context issue.
Changes:
CopilotEngine.Initialize()now callse.client.Start(context.Background())instead of the original context, andExecute()no longer handlesStart().copilot-sdk/goupgraded from v0.1.25 to v0.1.30, withOnPermissionRequest: copilot.PermissionHandler.ApproveAlladded to all session configs, andPermissionHandlerfield type updated tocopilot.PermissionHandlerFunc.- Tests that previously relied on context-cancellation to stop
Start()are skipped, and tests that previously relied onExecute()to callStart()are updated to callInitialize()first.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/execution/copilot.go |
Core change: moves startOnce.Do(Start) from Execute() into Initialize(), uses context.Background(), sets AutoStart: false, downgrades slog.Warn to slog.Debug for delete-session failures |
internal/execution/engine.go |
Changes PermissionHandler field type from copilot.PermissionHandler to copilot.PermissionHandlerFunc (SDK v0.1.30 rename) |
internal/execution/copilot_engine_test.go |
Skips two tests that relied on context-cancelling Start(), adds Initialize() calls to tests that now require it |
internal/execution/copilot_test.go |
Adds Initialize() calls before Execute() in parallel test |
internal/execution/skill_injection_test.go |
Adds Initialize() calls to two tests that need it |
internal/graders/prompt_grader.go |
Adds OnPermissionRequest: copilot.PermissionHandler.ApproveAll to session configs in grader |
internal/graders/prompt_grader_test.go |
Same OnPermissionRequest addition to live test's session config |
cmd/waza/cmd_run.go |
Adds slog import, changes fmt.Fprintf(os.Stderr) warnings to slog.Warn |
go.mod |
Upgrades copilot SDK, Azure SDK, charmbracelet libraries, golang.org/x packages |
go.sum |
Updated checksums for all dependency changes |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
=======================================
Coverage ? 72.25%
=======================================
Files ? 127
Lines ? 14242
Branches ? 0
=======================================
Hits ? 10291
Misses ? 3189
Partials ? 762
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… Updated tests and code paths to make sure we call Initialize!
The copilot SDK (as of 0.1.30) uses exec.CommandContext in Start(), which has a side effect of killing the copilot CLI if we cancel the context to Start(), even after the call to Start is completed. For now, we won't pass a cancellable context.
Filed as github/copilot-sdk#668