feat: Decouple ExecutionResponse from Copilot SDK event types (#10)#390
Closed
spboyer wants to merge 1 commit into
Closed
feat: Decouple ExecutionResponse from Copilot SDK event types (#10)#390spboyer wants to merge 1 commit into
spboyer wants to merge 1 commit into
Conversation
Phase 1 of #10: introduce a provider-neutral event model so waza's execution surface (ExecutionResponse.Events, transcripts, grader contexts, web API payloads) no longer depends on copilot-sdk's SessionEvent types. Changes: - New package internal/agentevents with neutral Event, EventType constants, and 21 typed payload structs that mirror the SDK shape. - New adapter internal/execution/agentevents_adapter.go (FromCopilotEvent) that converts copilot.SessionEvent into the neutral type at the SessionEventsCollector.On boundary. - ExecutionResponse.Events now []agentevents.Event; collector.SessionEvents() returns neutral events. - models.TranscriptEvent embeds agentevents.Event (field renamed SessionEvent -> Event). JSON wire format preserved: EventType constants are string-literal-identical to the SDK values. - Refactored consumers to neutral types: transcript builder, orchestration runner, inline-script grader, webapi store, and the cmd/waza run/suggest path. - Tests updated to construct neutral events and assert against neutral EventType constants. Phase 1 residual coupling (documented; addressed in Phase 2): agentevents.ToolExecutionCompleteData.Result and models.ToolCall.Result remain *copilot.ToolExecutionCompleteResult to keep the dashboard payload wire format bit-for-bit identical. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 1 of issue #10 by decoupling execution.ExecutionResponse.Events (and downstream transcript/web consumers) from Copilot SDK copilot.SessionEvent types. It introduces a provider-neutral event model (internal/agentevents) and adapts Copilot SDK events at the engine boundary, keeping existing JSON transcript/dashboard wire formats consistent while enabling future non-Copilot engines.
Changes:
- Introduces
internal/agenteventsas the neutral event model (event type constants + typed payloads + accessor helpers). - Adapts Copilot SDK events to neutral events at the boundary via
execution.FromCopilotEvent(...), and migratesExecutionResponse.Events+ transcript plumbing to use neutral events. - Updates key consumers (orchestration, graders, web API, suggest flow) and adjusts tests to construct/assert against neutral event types.
Show a summary per file
| File | Description |
|---|---|
| internal/agentevents/events.go | Adds provider-neutral Event/EventType model, payload structs, and accessor helpers. |
| internal/execution/agentevents_adapter.go | Converts Copilot SDK SessionEvent payloads into neutral agentevents.Event at the boundary. |
| internal/execution/session_events_collector.go | Stores neutral events internally while preserving the SDK callback signature for On(...). |
| internal/execution/engine.go | Switches ExecutionResponse.Events and message extraction to neutral event types/helpers. |
| internal/execution/mock.go | Updates mock engine response to return neutral events. |
| internal/models/events.go | Embeds neutral events in TranscriptEvent and updates transcript marshal/unmarshal + tool-call correlation. |
| internal/transcript/transcript.go | Updates transcript builder to wrap neutral events into models.TranscriptEvent. |
| internal/orchestration/runner.go | Updates transcript building to embed neutral events. |
| internal/webapi/store.go | Migrates transcript event mapping to use agentevents helpers. |
| cmd/waza/cmd_run_suggest.go | Migrates suggestion trace extraction to use neutral event types/helpers. |
| internal/webapi/additional_test.go | Updates tests to construct neutral events in transcripts. |
| internal/transcript/transcript_test.go | Updates transcript tests to use neutral events. |
| internal/orchestration/runner_orchestration_test.go | Updates orchestration tests to use neutral event types/constants. |
| internal/models/events_test.go | Updates transcript JSON round-trip tests for neutral event payload types. |
| internal/graders/inline_script_grader.go | Updates grader transcript/tool-call extraction to operate on neutral events. |
| internal/graders/inline_script_grader_test.go | Updates grader tests to use neutral events and collector output. |
| internal/execution/engine_response_test.go | Updates message-extraction test to use neutral events. |
| cmd/waza/cmd_run_suggest_test.go | Updates suggest tests to use neutral events. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 2
- Review effort level: Low
Comment on lines
+127
to
+130
| case agentevents.EventTypeAssistantMessage: | ||
| return &agentevents.AssistantMessageData{Content: derefString(content)} | ||
| case agentevents.EventTypeAssistantMessageDelta: | ||
| return &agentevents.AssistantMessageDeltaData{DeltaContent: derefString(content)} |
Comment on lines
+145
to
+146
| case agentevents.EventTypeSessionError: | ||
| return &agentevents.SessionErrorData{Message: derefString(message)} |
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
Phase 1 of #10. Decouples waza's execution surface from
copilot.SessionEventso future engines can populateExecutionResponse.Eventswithout depending on the Copilot SDK's event types. Phase 2 (multi-engine implementations, fully neutralToolExecutionCompleteResult) is intentionally out of scope.Changes
internal/agentevents— neutralEventstruct,EventTypeconstants, and 21 typed payload structs (AssistantMessageData,ToolExecutionStartData,ToolExecutionCompleteData, etc.) that mirror the SDK shape.internal/execution/agentevents_adapter.go—FromCopilotEvent(copilot.SessionEvent) agentevents.Eventis applied insideSessionEventsCollector.Onso the SDK callback contract is preserved at the engine boundary while everything downstream is neutral.ExecutionResponse.Events []agentevents.EventandSessionEventsCollector.SessionEvents()now return neutral events.models.TranscriptEventembedsagentevents.Event(the embedded field was renamedSessionEvent→Event). JSON wire format is preserved: eachEventTypeconstant has the same string value as itscopilot.SessionEventTypecounterpart (e.g."assistant.message","tool.execution_complete").internal/transcript,internal/orchestrationrunner,internal/graders/inline_script_grader,internal/webapi/store, andcmd/waza/cmd_run_suggest.EventTypeconstants. SDK-boundary tests still feedcopilot.SessionEventintoOn(...)as that's the SDK callback contract.Phase 1 residual coupling (documented; Phase 2)
agentevents.ToolExecutionCompleteData.Resultandmodels.ToolCall.Resultremain*copilot.ToolExecutionCompleteResultso the dashboard payload (Content,DetailedContent,Contents,BinaryResultsForLlm) is wire-compatible bit-for-bit.internal/copiloteventsandinternal/utils/logging.gokeep their SDK imports — they sit on the SDK side of the boundary.Verification
go build ./...— cleango test ./...— passes (the only failure isinternal/models/TestLoadEvalSpec_MCPMocksRequireSchemaVersion11, which is pre-existing onmainec8cb62 and unrelated to this change).golangci-lint run— 0 issues.Closes #10 (Phase 1 only)