Skip to content

Working around an issue in the copilot SDK, Start() and contexts#53

Merged
github-actions[bot] merged 7 commits into
microsoft:mainfrom
richardpark-msft:wz-showcase-support
Mar 4, 2026
Merged

Working around an issue in the copilot SDK, Start() and contexts#53
github-actions[bot] merged 7 commits into
microsoft:mainfrom
richardpark-msft:wz-showcase-support

Conversation

@richardpark-msft

Copy link
Copy Markdown
Member

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

…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.
Copilot AI review requested due to automatic review settings March 4, 2026 20:30
@github-actions github-actions Bot enabled auto-merge (squash) March 4, 2026 20:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 calls e.client.Start(context.Background()) instead of the original context, and Execute() no longer handles Start().
  • copilot-sdk/go upgraded from v0.1.25 to v0.1.30, with OnPermissionRequest: copilot.PermissionHandler.ApproveAll added to all session configs, and PermissionHandler field type updated to copilot.PermissionHandlerFunc.
  • Tests that previously relied on context-cancellation to stop Start() are skipped, and tests that previously relied on Execute() to call Start() are updated to call Initialize() 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

Comment thread internal/execution/skill_injection_test.go
Comment thread internal/execution/copilot_engine_test.go
Comment thread internal/execution/skill_injection_test.go
Comment thread internal/execution/copilot.go
Comment thread internal/execution/copilot.go
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 4, 2026 20:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.

Comment thread internal/execution/copilot.go
Comment thread internal/execution/copilot.go
Comment thread internal/execution/copilot.go
Comment thread internal/execution/skill_injection_test.go
@codecov-commenter

codecov-commenter commented Mar 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.00000% with 27 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@952c783). Learn more about missing BASE report.

Files with missing lines Patch % Lines
internal/graders/prompt_grader.go 0.00% 12 Missing ⚠️
internal/execution/copilot.go 72.72% 2 Missing and 1 partial ⚠️
cmd/waza/cmd_run.go 0.00% 2 Missing ⚠️
cmd/waza/cmd_suggest.go 0.00% 1 Missing and 1 partial ⚠️
cmd/waza/dev/copilot.go 0.00% 1 Missing and 1 partial ⚠️
cmd/waza/tokens/suggest.go 0.00% 1 Missing and 1 partial ⚠️
internal/execution/mock.go 33.33% 1 Missing and 1 partial ⚠️
internal/orchestration/runner.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #53   +/-   ##
=======================================
  Coverage        ?   72.25%           
=======================================
  Files           ?      127           
  Lines           ?    14242           
  Branches        ?        0           
=======================================
  Hits            ?    10291           
  Misses          ?     3189           
  Partials        ?      762           
Flag Coverage Δ
go-implementation 72.25% <25.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI review requested due to automatic review settings March 4, 2026 21:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated no new comments.

@github-actions github-actions Bot merged commit 3460e25 into microsoft:main Mar 4, 2026
10 checks passed
@richardpark-msft richardpark-msft deleted the wz-showcase-support branch March 4, 2026 22:07
chlowell pushed a commit to chlowell/waza that referenced this pull request Mar 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants