refactor: decouple ExecutionResponse from Copilot SDK events (Phase 1, #10)#396
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 1 of issue #10 by introducing an engine-neutral event abstraction so ExecutionResponse no longer exposes Copilot SDK event types directly, enabling additional agent engines to integrate in a follow-up phase.
Changes:
- Added
internal/agenteventas an engine-neutral event wrapper with aKindenum and opaqueRawpayload. - Added
internal/copiloteventsbridging helpers to wrap/unwrap Copilot SDKSessionEventvalues at engine/consumer boundaries. - Updated
ExecutionResponse.Eventsand migrated key call sites + fixtures/tests to the new abstraction.
Show a summary per file
| File | Description |
|---|---|
| internal/agentevent/agentevent.go | Introduces engine-neutral Event + Kind model. |
| internal/copilotevents/bridge.go | Adds wrap/unwrap bridge between SDK events and agentevent.Event. |
| internal/execution/engine.go | Switches ExecutionResponse.Events to []agentevent.Event and updates ExtractMessages. |
| internal/execution/copilot.go | Wraps SDK events at the Copilot engine boundary via FromSDK. |
| internal/execution/mock.go | Removes Copilot import and emits empty []agentevent.Event. |
| internal/orchestration/runner.go | Unwraps to SDK events at legacy consumer boundaries. |
| internal/trigger/runner.go | Unwraps to SDK events when building transcripts. |
| cmd/waza/cmd_run_suggest.go | Converts to SDK events for transcript/error tracing paths. |
| cmd/waza/cmd_run_suggest_test.go | Updates fixtures to wrap SDK events via FromSDK. |
| internal/execution/engine_response_test.go | Updates fixtures to wrap SDK events via FromSDK. |
| internal/orchestration/runner_orchestration_test.go | Updates fixtures to wrap SDK events via FromSDK. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Low
This was referenced Jun 30, 2026
Phase 1 of issue #10: introduce an engine-neutral event abstraction so additional engines (Claude Code, Codex, generic CLI) can plug in in Phase 2 without coupling ExecutionResponse to the Copilot SDK. - Add internal/agentevent: engine-neutral Event with Kind + opaque Raw payload (KindAssistantMessage, KindToolStart, etc.). - Add internal/copilotevents.{FromSDK,ToSDK,AsSDKEvent,WrapSDKEvent}: bridge between SDK SessionEvent and the generic Event. - Change ExecutionResponse.Events from []copilot.SessionEvent to []agentevent.Event; rewrite ExtractMessages against the generic Kind and unwrap via copilotevents.AsSDKEvent. - Copilot engine wraps SDK events at the boundary with copilotevents.FromSDK; MockEngine emits []agentevent.Event{}. - Consumers (orchestration/runner, trigger/runner, cmd_run_suggest) call copilotevents.ToSDK at call sites that still consume SDK events. Tests updated to wrap fixtures with copilotevents.FromSDK. Pure refactor; no behavioral change. All existing tests pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Three fixes from the PR #396 review: 1. internal/orchestration/runner.go: convert resp.Events to SDK form once per executeRun result-building section and reuse the slice across buildGraderContext, BuildFromSessionEvents (transcript), and buildToolEvents. Previously executeRun walked the event slice three times after the responder/follow-up loop. checkpointRunner still converts on its own because it runs at each turn boundary. 2. internal/orchestration/runner.go: buildGraderContext now delegates to transcript.BuildFromSessionEvents instead of appending to a nil slice, so the result slice is preallocated to len(events) in one place. 3. internal/execution/engine.go: ExtractMessages no longer silently drops assistant messages from non-Copilot engines. agentevent gains an optional TextProvider interface; ExtractMessages falls back to Event.Text() when AsSDKEvent fails so future engines (Claude Code, Codex, generic CLI) can surface assistant content with no further plumbing in this method. Adds unit tests for the new TextProvider abstraction and the ExtractMessages fallback (covers Copilot-only, non-Copilot-only, mixed, and opaque-payload cases). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
fee2020 to
3d57f4a
Compare
This was referenced Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of #10 — introduces an engine-neutral event abstraction so additional agent engines (Claude Code, Codex, generic CLI) can plug in for Phase 2 without coupling
ExecutionResponseto the Copilot SDK.This is a pure refactor with no behavioral change. All existing tests pass unchanged.
Changes
New abstraction layer
internal/agentevent— engine-neutralEventcarrying aKindenum (KindAssistantMessage,KindToolStart, …) and an opaqueRaw anypayload. AccessorsKind()/Raw().internal/copilotevents/bridge.go— bridge between SDKcopilot.SessionEventand the genericagentevent.Event:FromSDK([]copilot.SessionEvent) []agentevent.Event— wrap at engine boundaryToSDK([]agentevent.Event) []copilot.SessionEvent— unwrap for legacy consumersAsSDKEvent(agentevent.Event) (copilot.SessionEvent, bool)— single-item unwrapWrapSDKEvent(copilot.SessionEvent) agentevent.EventExecutionResponseEvents []copilot.SessionEvent→Events []agentevent.Event.ExtractMessagesrewritten against the genericKind, unwrapping viacopilotevents.AsSDKEventto readAssistantMessageData.Engines
internal/execution/copilot.gowraps SDK events withcopilotevents.FromSDK(...)at the boundary when building the response.internal/execution/mock.goemits[]agentevent.Event{}(dropped thecopilotimport).Consumers (minimal-churn migration)
Call sites that still need SDK events use
copilotevents.ToSDK(resp.Events):internal/orchestration/runner.go—buildToolEvents, transcript construction,buildTranscriptinternal/trigger/runner.go—transcript.BuildFromSessionEventscmd/waza/cmd_run_suggest.go—buildNoSuggestionsError,writeSuggestionTranscriptTests
Fixture event slices now wrap with
copilotevents.FromSDK(...):cmd/waza/cmd_run_suggest_test.gointernal/execution/engine_response_test.gointernal/orchestration/runner_orchestration_test.goOut of scope (Phase 1)
ExecutionRequest.Tools,.MCPServers,.PermissionHandlerstill use SDK types. These are engine-input types, not response types, and are outside Phase 1's acceptance criteria. Phase 2 will revisit them.models.TranscriptEvent(on-disk transcript format) still embedscopilot.SessionEvent— persistence format, not an engine-neutral type.Validation
go build ./...— cleango test ./...— all packages passgolangci-lint run— 0 issuesPhase 2 hand-off
Phase 2 (multi-agent engine support, owned by @richardpark-msft) will extend the
agentevent.Eventmodel with more kinds and add per-engine adapters parallel tocopilotevents. Consumers can then migrate fromcopilotevents.ToSDK(...)to direct generic accessors.Partial progress on #10 — issue stays open for Phase 2.