Skip to content

Commit fac218c

Browse files
MRayermannMSFTMRayermannMSFT
committed
Fix code quality review feedback and formatting issues
- Python: fix ruff formatting, add comments to empty except blocks, remove unused imports - .NET: simplify boolean expressions, combine nested ifs, narrow generic catch clause - Go: fix struct field alignment for go fmt compliance
1 parent 357a0b0 commit fac218c

8 files changed

Lines changed: 57 additions & 77 deletions

File tree

‎dotnet/src/Client.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -504,7 +504,7 @@ public async Task<CopilotSession> CreateSessionAsync(SessionConfig config, Cance
504504
config.DisabledSkills,
505505
config.InfiniteSessions,
506506
Commands: config.Commands?.Select(c => new CommandWireDefinition(c.Name, c.Description)).ToList(),
507-
RequestElicitation: config.OnElicitationRequest != null ? true : false,
507+
RequestElicitation: config.OnElicitationRequest != null,
508508
Traceparent: traceparent,
509509
Tracestate: tracestate);
510510

@@ -624,7 +624,7 @@ public async Task<CopilotSession> ResumeSessionAsync(string sessionId, ResumeSes
624624
config.DisabledSkills,
625625
config.InfiniteSessions,
626626
Commands: config.Commands?.Select(c => new CommandWireDefinition(c.Name, c.Description)).ToList(),
627-
RequestElicitation: config.OnElicitationRequest != null ? true : false,
627+
RequestElicitation: config.OnElicitationRequest != null,
628628
Traceparent: traceparent,
629629
Tracestate: tracestate);
630630

‎dotnet/src/Session.cs‎

Lines changed: 27 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -730,7 +730,7 @@ private async Task HandleElicitationRequestAsync(ElicitationRequest request, str
730730
Content = result.Content
731731
});
732732
}
733-
catch
733+
catch (Exception)
734734
{
735735
// Handler failed — attempt to cancel so the request doesn't hang
736736
try
@@ -740,7 +740,7 @@ private async Task HandleElicitationRequestAsync(ElicitationRequest request, str
740740
Action = SessionUiElicitationResultAction.Cancel
741741
});
742742
}
743-
catch (Exception ex) when (ex is IOException or ObjectDisposedException)
743+
catch (Exception innerEx) when (innerEx is IOException or ObjectDisposedException)
744744
{
745745
// Connection lost — nothing we can do
746746
}
@@ -791,18 +791,17 @@ public async Task<bool> ConfirmAsync(string message, CancellationToken cancellat
791791
Required = ["confirmed"]
792792
};
793793
var result = await session.Rpc.Ui.ElicitationAsync(message, schema, cancellationToken);
794-
if (result.Action == SessionUiElicitationResultAction.Accept && result.Content != null)
794+
if (result.Action == SessionUiElicitationResultAction.Accept
795+
&& result.Content != null
796+
&& result.Content.TryGetValue("confirmed", out var val))
795797
{
796-
if (result.Content.TryGetValue("confirmed", out var val))
798+
return val switch
797799
{
798-
return val switch
799-
{
800-
bool b => b,
801-
JsonElement { ValueKind: JsonValueKind.True } => true,
802-
JsonElement { ValueKind: JsonValueKind.False } => false,
803-
_ => false
804-
};
805-
}
800+
bool b => b,
801+
JsonElement { ValueKind: JsonValueKind.True } => true,
802+
JsonElement { ValueKind: JsonValueKind.False } => false,
803+
_ => false
804+
};
806805
}
807806
return false;
808807
}
@@ -820,17 +819,16 @@ public async Task<bool> ConfirmAsync(string message, CancellationToken cancellat
820819
Required = ["selection"]
821820
};
822821
var result = await session.Rpc.Ui.ElicitationAsync(message, schema, cancellationToken);
823-
if (result.Action == SessionUiElicitationResultAction.Accept && result.Content != null)
822+
if (result.Action == SessionUiElicitationResultAction.Accept
823+
&& result.Content != null
824+
&& result.Content.TryGetValue("selection", out var val))
824825
{
825-
if (result.Content.TryGetValue("selection", out var val))
826+
return val switch
826827
{
827-
return val switch
828-
{
829-
string s => s,
830-
JsonElement { ValueKind: JsonValueKind.String } je => je.GetString(),
831-
_ => val?.ToString()
832-
};
833-
}
828+
string s => s,
829+
JsonElement { ValueKind: JsonValueKind.String } je => je.GetString(),
830+
_ => val?.ToString()
831+
};
834832
}
835833
return null;
836834
}
@@ -853,17 +851,16 @@ public async Task<bool> ConfirmAsync(string message, CancellationToken cancellat
853851
Required = ["value"]
854852
};
855853
var result = await session.Rpc.Ui.ElicitationAsync(message, schema, cancellationToken);
856-
if (result.Action == SessionUiElicitationResultAction.Accept && result.Content != null)
854+
if (result.Action == SessionUiElicitationResultAction.Accept
855+
&& result.Content != null
856+
&& result.Content.TryGetValue("value", out var val))
857857
{
858-
if (result.Content.TryGetValue("value", out var val))
858+
return val switch
859859
{
860-
return val switch
861-
{
862-
string s => s,
863-
JsonElement { ValueKind: JsonValueKind.String } je => je.GetString(),
864-
_ => val?.ToString()
865-
};
866-
}
860+
string s => s,
861+
JsonElement { ValueKind: JsonValueKind.String } je => je.GetString(),
862+
_ => val?.ToString()
863+
};
867864
}
868865
return null;
869866
}

‎go/types.go‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -873,11 +873,11 @@ type createSessionRequest struct {
873873
ConfigDir string `json:"configDir,omitempty"`
874874
SkillDirectories []string `json:"skillDirectories,omitempty"`
875875
DisabledSkills []string `json:"disabledSkills,omitempty"`
876-
InfiniteSessions *InfiniteSessionConfig `json:"infiniteSessions,omitempty"`
877-
Commands []wireCommand `json:"commands,omitempty"`
878-
RequestElicitation *bool `json:"requestElicitation,omitempty"`
879-
Traceparent string `json:"traceparent,omitempty"`
880-
Tracestate string `json:"tracestate,omitempty"`
876+
InfiniteSessions *InfiniteSessionConfig `json:"infiniteSessions,omitempty"`
877+
Commands []wireCommand `json:"commands,omitempty"`
878+
RequestElicitation *bool `json:"requestElicitation,omitempty"`
879+
Traceparent string `json:"traceparent,omitempty"`
880+
Tracestate string `json:"tracestate,omitempty"`
881881
}
882882

883883
// wireCommand is the wire representation of a command (name + description only, no handler).
@@ -888,9 +888,9 @@ type wireCommand struct {
888888

889889
// createSessionResponse is the response from session.create
890890
type createSessionResponse struct {
891-
SessionID string `json:"sessionId"`
892-
WorkspacePath string `json:"workspacePath"`
893-
Capabilities *SessionCapabilities `json:"capabilities,omitempty"`
891+
SessionID string `json:"sessionId"`
892+
WorkspacePath string `json:"workspacePath"`
893+
Capabilities *SessionCapabilities `json:"capabilities,omitempty"`
894894
}
895895

896896
// resumeSessionRequest is the request for session.resume
@@ -915,20 +915,20 @@ type resumeSessionRequest struct {
915915
EnvValueMode string `json:"envValueMode,omitempty"`
916916
CustomAgents []CustomAgentConfig `json:"customAgents,omitempty"`
917917
Agent string `json:"agent,omitempty"`
918-
SkillDirectories []string `json:"skillDirectories,omitempty"`
919-
DisabledSkills []string `json:"disabledSkills,omitempty"`
920-
InfiniteSessions *InfiniteSessionConfig `json:"infiniteSessions,omitempty"`
921-
Commands []wireCommand `json:"commands,omitempty"`
922-
RequestElicitation *bool `json:"requestElicitation,omitempty"`
923-
Traceparent string `json:"traceparent,omitempty"`
924-
Tracestate string `json:"tracestate,omitempty"`
918+
SkillDirectories []string `json:"skillDirectories,omitempty"`
919+
DisabledSkills []string `json:"disabledSkills,omitempty"`
920+
InfiniteSessions *InfiniteSessionConfig `json:"infiniteSessions,omitempty"`
921+
Commands []wireCommand `json:"commands,omitempty"`
922+
RequestElicitation *bool `json:"requestElicitation,omitempty"`
923+
Traceparent string `json:"traceparent,omitempty"`
924+
Tracestate string `json:"tracestate,omitempty"`
925925
}
926926

927927
// resumeSessionResponse is the response from session.resume
928928
type resumeSessionResponse struct {
929-
SessionID string `json:"sessionId"`
930-
WorkspacePath string `json:"workspacePath"`
931-
Capabilities *SessionCapabilities `json:"capabilities,omitempty"`
929+
SessionID string `json:"sessionId"`
930+
WorkspacePath string `json:"workspacePath"`
931+
Capabilities *SessionCapabilities `json:"capabilities,omitempty"`
932932
}
933933

934934
type hooksInvokeRequest struct {

‎python/copilot/session.py‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1260,9 +1260,7 @@ def _handle_broadcast_event(self, event: SessionEvent) -> None:
12601260
request["elicitationSource"] = event.data.elicitation_source
12611261
if event.data.url is not None:
12621262
request["url"] = event.data.url
1263-
asyncio.ensure_future(
1264-
self._handle_elicitation_request(request, request_id)
1265-
)
1263+
asyncio.ensure_future(self._handle_elicitation_request(request, request_id))
12661264

12671265
elif event.type == SessionEventType.CAPABILITIES_CHANGED:
12681266
cap: SessionCapabilities = {}
@@ -1403,7 +1401,7 @@ async def _execute_command_and_respond(
14031401
)
14041402
)
14051403
except (JsonRpcError, ProcessExitedError, OSError):
1406-
pass
1404+
pass # Connection lost — nothing we can do
14071405
return
14081406

14091407
try:
@@ -1429,7 +1427,7 @@ async def _execute_command_and_respond(
14291427
)
14301428
)
14311429
except (JsonRpcError, ProcessExitedError, OSError):
1432-
pass
1430+
pass # Connection lost — nothing we can do
14331431

14341432
async def _handle_elicitation_request(
14351433
self,

‎python/e2e/test_commands.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ async def teardown(self, test_failed: bool = False):
8181
try:
8282
await c.stop()
8383
except Exception:
84-
pass
84+
pass # Best-effort cleanup during teardown
8585
self._client1 = self._client2 = None
8686

8787
if self._proxy:

‎python/e2e/test_ui_elicitation.py‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,7 @@ async def test_elicitation_methods_throw_in_headless_mode(self, ctx: E2ETestCont
3232
with pytest.raises(RuntimeError, match="not supported"):
3333
await session.ui.confirm("test")
3434

35-
async def test_session_with_elicitation_handler_reports_capability(
36-
self, ctx: E2ETestContext
37-
):
35+
async def test_session_with_elicitation_handler_reports_capability(self, ctx: E2ETestContext):
3836
"""Session created with onElicitationRequest reports elicitation capability."""
3937

4038
async def handler(

‎python/e2e/test_ui_elicitation_multi_client.py‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,17 +79,15 @@ async def setup(self):
7979
self._actual_port = self._client1.actual_port
8080
assert self._actual_port is not None
8181

82-
self._client2 = CopilotClient(
83-
ExternalServerConfig(url=f"localhost:{self._actual_port}")
84-
)
82+
self._client2 = CopilotClient(ExternalServerConfig(url=f"localhost:{self._actual_port}"))
8583

8684
async def teardown(self, test_failed: bool = False):
8785
for c in (self._client2, self._client1):
8886
if c:
8987
try:
9088
await c.stop()
9189
except Exception:
92-
pass
90+
pass # Best-effort cleanup during teardown
9391
self._client1 = self._client2 = None
9492

9593
if self._proxy:

‎python/test_commands_and_elicitation.py‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
"""
77

88
import asyncio
9-
from unittest.mock import AsyncMock
109

1110
import pytest
1211

@@ -15,8 +14,6 @@
1514
from copilot.session import (
1615
CommandContext,
1716
CommandDefinition,
18-
CopilotSession,
19-
ElicitationHandler,
2017
ElicitationRequest,
2118
ElicitationResult,
2219
PermissionHandler,
@@ -245,9 +242,7 @@ async def test_sends_error_for_unknown_command(self):
245242
session = await client.create_session(
246243
on_permission_request=PermissionHandler.approve_all,
247244
commands=[
248-
CommandDefinition(
249-
name="deploy", handler=lambda ctx: None
250-
),
245+
CommandDefinition(name="deploy", handler=lambda ctx: None),
251246
],
252247
)
253248

@@ -478,17 +473,11 @@ async def mock_request(method, params):
478473
client._client.request = mock_request
479474

480475
# Call _handle_elicitation_request directly (as Node.js test does)
481-
await session._handle_elicitation_request(
482-
{"message": "Pick a color"}, "req-123"
483-
)
476+
await session._handle_elicitation_request({"message": "Pick a color"}, "req-123")
484477

485478
assert len(rpc_calls) >= 1
486479
cancel_call = next(
487-
(
488-
call
489-
for call in rpc_calls
490-
if call[1].get("result", {}).get("action") == "cancel"
491-
),
480+
(call for call in rpc_calls if call[1].get("result", {}).get("action") == "cancel"),
492481
None,
493482
)
494483
assert cancel_call is not None

0 commit comments

Comments
 (0)