refactor: decouple execution events from Copilot SDK#389
Closed
spboyer wants to merge 1 commit into
Closed
Conversation
Introduce provider-neutral session event and tool result types for ExecutionResponse and transcript consumers, with Copilot SDK conversion kept inside the Copilot event adapter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors execution/transcript event handling to be provider-neutral by introducing models.SessionEvent / models.ToolExecutionResult, updating ExecutionResponse.Events and downstream consumers to use those types, and confining Copilot SDK event parsing/conversion to the Copilot adapter layer (internal/copilotevents + SessionEventsCollector).
Changes:
- Added provider-neutral event/result models (
internal/models/events.go) and updated transcript JSON marshalling/unmarshalling to preserve the existing output contract. - Updated execution responses, transcript builders, graders, CLI suggestion trace generation, and web API mapping to consume
models.SessionEventinstead ofcopilot.SessionEvent. - Updated unit tests to construct and validate neutral event types.
Show a summary per file
| File | Description |
|---|---|
| internal/webapi/store.go | Maps neutral transcript events into the web API response shape. |
| internal/webapi/additional_test.go | Updates web API tests to use neutral tool-complete events/results. |
| internal/transcript/transcript.go | Changes transcript builder to accept neutral session events. |
| internal/transcript/transcript_test.go | Updates transcript tests to construct neutral events (adds string ptr helper). |
| internal/orchestration/runner_orchestration_test.go | Updates orchestration tests to use neutral events and types. |
| internal/models/events.go | Introduces SessionEventType, SessionEvent, ToolExecutionResult; updates transcript JSON (un)marshal + tool-call correlation. |
| internal/models/events_test.go | Updates transcript event JSON round-trip tests for neutral event model. |
| internal/graders/inline_script_grader.go | Adjusts grader stdin generation to pass neutral session events. |
| internal/graders/inline_script_grader_test.go | Updates grader tests/helpers to use neutral session events. |
| internal/execution/session_events_collector.go | Converts Copilot SDK events to neutral events at collection time. |
| internal/execution/session_events_collector_test.go | Updates collector tests for neutral tool result type. |
| internal/execution/mock.go | Updates mock engine response to use neutral events list. |
| internal/execution/engine.go | Switches ExecutionResponse.Events to neutral events and updates message extraction. |
| internal/execution/engine_response_test.go | Updates ExtractMessages tests for neutral events (adds string ptr helper). |
| internal/copilotevents/events.go | Adds conversion from Copilot SDK SessionEvent / tool results into neutral models. |
| cmd/waza/cmd_run_suggest.go | Updates suggestion trace extraction to read fields from neutral events. |
| cmd/waza/cmd_run_suggest_test.go | Updates suggestion command tests to use neutral events/results. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Low
Comment on lines
+352
to
+354
| r.Arguments = e.Arguments | ||
| r.Success = e.Success | ||
| r.ToolResult = e.ToolResult |
Member
Author
|
Closing in favor of #396 (smaller, green CI, latest iteration of this Phase-1 refactor). |
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
models.SessionEventandmodels.ToolExecutionResulttypes.execution.ExecutionResponse.Eventsand transcript/tool-call consumers to use neutral events instead ofcopilot.SessionEvent.internal/copilotevents/SessionEventsCollector.Closes #10
Scope
Phase 1 only: this does not add Phase 2 engine implementations.
Validation
go test ./...(fails on pre-existing baseline:internal/models TestLoadEvalSpec_MCPMocksRequireSchemaVersion11, reproduced before changes)go test ./internal/models -run 'TestTranscriptEvent'go test $(go list ./... | grep -v '/internal/models$')make lint(aftergolangci-lint cache cleanto clear stale worktree paths)make build