Register the Copilot event handler via SessionConfig.OnEvent so session-lifecycle events are not dropped - #746
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Copilot provider’s session creation/resume path so the run-scoped Copilot event handler is registered via SessionConfig.OnEvent at session construction time, preventing session-lifecycle events emitted during session.create/session.resume from being dropped.
Changes:
- Thread the run’s
eventHandlerintoopenSessionand set it onSessionConfig.OnEvent/ResumeSessionConfig.OnEventbefore the create/resume RPC. - Remove the post-return
session.On(eventHandler)subscription that introduced an event-loss window. - Add a regression test that emits a lifecycle event during
session.resume(before the RPC response) and asserts it is surfaced.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| provider/copilotprovider/copilot.go | Registers the run event handler through config at session construction time to avoid dropping early lifecycle events. |
| provider/copilotprovider/copilot_test.go | Adds a fake-runtime mechanism to emit pre-response resume events and a test ensuring the provider surfaces them. |
Comments suppressed due to low confidence (1)
provider/copilotprovider/copilot.go:252
- resumeSessionConfig unconditionally sets cfg.OnEvent = eventHandler, which prevents any handler configured on p.cfg.SessionConfig.OnEvent from ever running on resumed sessions (and also makes create/resume behavior inconsistent if callers rely on SessionConfig.OnEvent). Consider seeding cfg.OnEvent from p.cfg.SessionConfig.OnEvent and chaining it with the per-run handler.
func (p *provider) resumeSessionConfig(streaming bool, eventHandler copilot.SessionEventHandler, options []agent.Option) copilot.ResumeSessionConfig {
cfg := copyResumeSessionConfig(p.cfg.SessionConfig)
cfg.Streaming = copilot.Bool(streaming)
cfg.OnEvent = eventHandler
cfg.SystemMessage = systemMessageWithInstructions(cfg.SystemMessage, slices.Collect(agent.AllOptions(options, agent.WithInstructions)))
cfg.Tools = append(cfg.Tools, copilotTools(options)...)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
The Copilot provider opened the session and only afterward called copilotSession.On(eventHandler). Because the SDK runs the session read loop on a separate goroutine and registers the session before the session.create/session.resume RPC, any lifecycle events the CLI emits during session creation (e.g. session.start) were dispatched to a session with zero handlers and dropped in the window before the post-return On call. Thread the handler into openSession and set it on OnEvent in both sessionConfig and resumeSessionConfig so it is registered before the RPC, matching the SDK's no-missed-events guarantee and the .NET/Python behaviour of attaching the provider handler at session-construction time rather than after the create call returns. The redundant post-return On call is removed.
c0bbe44 to
2869077
Compare
Parity Review ✅This PR changes only unexported/internal methods in
No exported Go API surface was added or changed, so the Cross-repo parity: This is Copilot-specific session plumbing with no direct equivalent in the upstream .NET or Python No parity issues found. 🟢 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
What
The Copilot provider opened the session (
openSession→client.CreateSession/ResumeSession) and only afterward calledcopilotSession.On(eventHandler).sessionConfig()andresumeSessionConfig()never setOnEvent.This change threads the
eventHandlercreated inrun()intoopenSessionand setscfg.OnEvent = eventHandlerinside bothsessionConfig()andresumeSessionConfig(), then removes the now-redundant post-return.On(eventHandler).Why
The Copilot SDK registers the session (and any
config.OnEventhandler) synchronously before it issues thesession.create/session.resumeRPC, precisely so that lifecycle events the CLI emits during session creation (e.g.session.start) are not missed. The SDK read loop runs on a separate goroutine, so with the old post-returnOnthere is a window between the RPC returning and the.Oncall during which such events are dispatched to a session with zero handlers and silently dropped (the event channel's consumer snapshots the handler list at dequeue time, so an event dequeued before.Onruns is lost).Registering through
OnEventcloses that window and matches the cross-SDK behaviour: the .NET and Python Copilot integrations attach the provider's event handler at session-construction time (as part of the create/resume options) rather than after the create call returns, so no early events are missed.This is not a duplicate of #710 (SessionConfig field carry-over) or #595 (Tools clone).
Testing
Added
TestRun_SurfacesLifecycleEventEmittedDuringSessionResumeincopilot_test.go. The fake runtime now emits asession.startevent duringsession.resumehandling, before the RPC response, and the test asserts the provider surfaces it. The test flakily fails against the old code (the event is dropped in the race window — reproduced deterministically with-count) and passes reliably with the fix, including under-raceand-count=300.go build ./...go vet ./provider/copilotprovider/...go test ./provider/copilotprovider/... -race