Skip to content

Commit 0c4ff33

Browse files
fix: address human review feedback
.NET: - Cache SessionUiApiImpl instance via Lazy<> instead of allocating on every .Ui access - Await command/elicitation handler calls instead of redundant fire-and-forget (outer caller already fire-and-forgets) - Use ElicitationRequestedDataMode enum for Mode instead of string Go: - Handle SessionEventTypeCapabilitiesChanged in handleBroadcastEvent to update session capabilities when other clients join/leave with elicitation handlers - Add test verifying capabilities.changed event updates session
1 parent 764d271 commit 0c4ff33

File tree

5 files changed

+60
-7
lines changed

5 files changed

+60
-7
lines changed

dotnet/src/Session.cs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ public sealed partial class CopilotSession : IAsyncDisposable
6363
private volatile PermissionRequestHandler? _permissionHandler;
6464
private volatile UserInputHandler? _userInputHandler;
6565
private volatile ElicitationHandler? _elicitationHandler;
66+
private readonly Lazy<ISessionUiApi> _ui;
6667
private ImmutableArray<SessionEventHandler> _eventHandlers = ImmutableArray<SessionEventHandler>.Empty;
6768

6869
private SessionHooks? _hooks;
@@ -122,7 +123,7 @@ public sealed partial class CopilotSession : IAsyncDisposable
122123
/// if the host does not report elicitation support via <see cref="Capabilities"/>.
123124
/// Check <c>session.Capabilities.Ui?.Elicitation == true</c> before calling.
124125
/// </remarks>
125-
public ISessionUiApi Ui => new SessionUiApiImpl(this);
126+
public ISessionUiApi Ui => _ui.Value;
126127

127128
/// <summary>
128129
/// Initializes a new instance of the <see cref="CopilotSession"/> class.
@@ -140,6 +141,7 @@ internal CopilotSession(string sessionId, JsonRpc rpc, ILogger logger, string? w
140141
_rpc = rpc;
141142
_logger = logger;
142143
WorkspacePath = workspacePath;
144+
_ui = new Lazy<ISessionUiApi>(() => new SessionUiApiImpl(this));
143145

144146
// Start the asynchronous processing loop.
145147
_ = ProcessEventsAsync();
@@ -469,7 +471,7 @@ private async Task HandleBroadcastEventAsync(SessionEvent sessionEvent)
469471
if (string.IsNullOrEmpty(data.RequestId))
470472
return;
471473

472-
_ = ExecuteCommandAndRespondAsync(data.RequestId, data.CommandName, data.Command, data.Args);
474+
await ExecuteCommandAndRespondAsync(data.RequestId, data.CommandName, data.Command, data.Args);
473475
break;
474476
}
475477

@@ -490,12 +492,12 @@ private async Task HandleBroadcastEventAsync(SessionEvent sessionEvent)
490492
}
491493
: null;
492494

493-
_ = HandleElicitationRequestAsync(
495+
await HandleElicitationRequestAsync(
494496
new ElicitationRequest
495497
{
496498
Message = data.Message,
497499
RequestedSchema = schema,
498-
Mode = data.Mode?.ToString().ToLowerInvariant(),
500+
Mode = data.Mode,
499501
ElicitationSource = data.ElicitationSource,
500502
Url = data.Url
501503
},

dotnet/src/Types.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -704,7 +704,7 @@ public class ElicitationRequest
704704
public ElicitationSchema? RequestedSchema { get; set; }
705705

706706
/// <summary>Elicitation mode: <c>"form"</c> for structured input, <c>"url"</c> for browser redirect.</summary>
707-
public string? Mode { get; set; }
707+
public ElicitationRequestedDataMode? Mode { get; set; }
708708

709709
/// <summary>The source that initiated the request (e.g., MCP server name).</summary>
710710
public string? ElicitationSource { get; set; }

dotnet/test/ElicitationTests.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -243,14 +243,14 @@ public void ElicitationRequest_Has_All_Properties()
243243
["color"] = new Dictionary<string, object> { ["type"] = "string", ["enum"] = new[] { "red", "blue" } },
244244
},
245245
},
246-
Mode = "form",
246+
Mode = ElicitationRequestedDataMode.Form,
247247
ElicitationSource = "mcp-server",
248248
Url = null,
249249
};
250250

251251
Assert.Equal("Pick a color", request.Message);
252252
Assert.NotNull(request.RequestedSchema);
253-
Assert.Equal("form", request.Mode);
253+
Assert.Equal(ElicitationRequestedDataMode.Form, request.Mode);
254254
Assert.Equal("mcp-server", request.ElicitationSource);
255255
Assert.Null(request.Url);
256256
}

go/session.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -983,6 +983,13 @@ func (s *Session) handleBroadcastEvent(event SessionEvent) {
983983
ElicitationSource: elicitationSource,
984984
URL: url,
985985
}, *requestID)
986+
987+
case SessionEventTypeCapabilitiesChanged:
988+
if event.Data.UI != nil && event.Data.UI.Elicitation != nil {
989+
s.setCapabilities(&SessionCapabilities{
990+
UI: &UICapabilities{Elicitation: *event.Data.UI.Elicitation},
991+
})
992+
}
986993
}
987994
}
988995

go/session_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,50 @@ func TestSession_Capabilities(t *testing.T) {
386386
t.Errorf("Expected UI to be nil after reset, got %+v", caps.UI)
387387
}
388388
})
389+
390+
t.Run("capabilities.changed event updates session capabilities", func(t *testing.T) {
391+
session, cleanup := newTestSession()
392+
defer cleanup()
393+
394+
// Initially no capabilities
395+
caps := session.Capabilities()
396+
if caps.UI != nil {
397+
t.Fatal("Expected UI to be nil initially")
398+
}
399+
400+
// Dispatch a capabilities.changed event with elicitation=true
401+
elicitTrue := true
402+
session.dispatchEvent(SessionEvent{
403+
Type: SessionEventTypeCapabilitiesChanged,
404+
Data: Data{
405+
UI: &UI{Elicitation: &elicitTrue},
406+
},
407+
})
408+
409+
// Give the broadcast handler time to process
410+
time.Sleep(50 * time.Millisecond)
411+
412+
caps = session.Capabilities()
413+
if caps.UI == nil || !caps.UI.Elicitation {
414+
t.Error("Expected UI.Elicitation to be true after capabilities.changed event")
415+
}
416+
417+
// Dispatch with elicitation=false
418+
elicitFalse := false
419+
session.dispatchEvent(SessionEvent{
420+
Type: SessionEventTypeCapabilitiesChanged,
421+
Data: Data{
422+
UI: &UI{Elicitation: &elicitFalse},
423+
},
424+
})
425+
426+
time.Sleep(50 * time.Millisecond)
427+
428+
caps = session.Capabilities()
429+
if caps.UI == nil || caps.UI.Elicitation {
430+
t.Error("Expected UI.Elicitation to be false after second capabilities.changed event")
431+
}
432+
})
389433
}
390434

391435
func TestSession_ElicitationCapabilityGating(t *testing.T) {

0 commit comments

Comments
 (0)