Skip to content

Commit 2a3e7a5

Browse files
edburnsedburns
committed
Centralize requestId parsing to fix NumberFormatException risks across all RPC handlers
Prompted by github-code-quality review comments on PR #185 (#185 (comment)). The bot flagged four instances of uncaught `NumberFormatException` from `Long.parseLong(requestId)` in the new `handleExitPlanModeRequest` and `handleAutoModeSwitchRequest` handlers. The recommended fix was to parse `requestId` once, catch `NumberFormatException`, and reuse the parsed `long`. Assessment: The `NumberFormatException` comments (r3221146107, r3221146111, r3221146120, r3221146133) are fully addressed and exceeded — the fix applies the pattern to ALL seven handlers in the class, not just the two new ones. A shared `parseRequestId(String, String)` utility method replaces both the flagged inline calls and an existing ad-hoc try/catch in `handleSystemMessageTransform`. Two additional comments (r3221146142, r3221146149) flagged the `invocation` parameter as unused in `AutoModeSwitchHandler` and `ExitPlanModeHandler`. These are intentionally not addressed: the parameter is part of the consistent two-arg handler API contract shared by all handler functional interfaces in the SDK. --- Per-file manifest --- `src/main/java/com/github/copilot/sdk/RpcHandlerDispatcher.java` - Add `private static parseRequestId(String, String)` utility that wraps `Long.parseLong` in a try/catch for `NumberFormatException`, logs on failure, and returns `-1` as a sentinel. - `handleToolCall`: parse `requestId` upfront via `parseRequestId`; replace five `Long.parseLong(requestId)` call sites with `requestIdLong`. - `handlePermissionRequest`: same pattern; replace three call sites. - `handleUserInputRequest`: same pattern; replace three call sites. - `handleExitPlanModeRequest`: same pattern; replace three call sites. (Directly addresses the linked review comment.) - `handleAutoModeSwitchRequest`: same pattern; replace three call sites. - `handleHooksInvoke`: same pattern; replace three call sites. - `handleSystemMessageTransform`: replace existing inline try/catch NFE block with the shared `parseRequestId` call, removing duplicated logic. `src/site/markdown/advanced.md` - Add `.setOnPermissionRequest(PermissionHandler.APPROVE_ALL)` to the exit-plan-mode and auto-mode-switch code examples so they compile and run without a missing-handler error. `src/test/java/com/github/copilot/sdk/ModeHandlersTest.java` - Parameterize `configureAuthenticatedUser(String testName)` to call `ctx.configureForTest("mode_handlers", testName)` with per-test snapshot names. - `shouldInvokeAutoModeSwitchHandlerWhenRateLimited`: switch from `sendAndWait` to `send`, add assertions on the returned `messageId`. `src/test/java/com/github/copilot/sdk/SessionEventHandlingTest.java` - Update `SessionStartEventData` constructor calls (arity 10 -> 11) for new `detachedFromSpawningParentSessionId` field. - Update `AssistantMessageEventData` constructor calls (arity 12 -> 15) for new `anthropicAdvisorModel`, `turnId`, `parentToolCallId` fields; adjust positional `null` arguments accordingly. Signed-off-by: Ed Burns <edburns@microsoft.com>
1 parent a427c7f commit 2a3e7a5

4 files changed

Lines changed: 81 additions & 38 deletions

File tree

‎src/main/java/com/github/copilot/sdk/RpcHandlerDispatcher.java‎

Lines changed: 67 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,10 @@ private void handleLifecycleEvent(JsonNode params) {
131131

132132
private void handleToolCall(JsonRpcClient rpc, String requestId, JsonNode params) {
133133
runAsync(() -> {
134+
final long requestIdLong = parseRequestId(requestId, "tool.call");
135+
if (requestIdLong == -1) {
136+
return;
137+
}
134138
try {
135139
String sessionId = params.get("sessionId").asText();
136140
String toolCallId = params.get("toolCallId").asText();
@@ -139,15 +143,15 @@ private void handleToolCall(JsonRpcClient rpc, String requestId, JsonNode params
139143

140144
CopilotSession session = sessions.get(sessionId);
141145
if (session == null) {
142-
rpc.sendErrorResponse(Long.parseLong(requestId), -32602, "Unknown session " + sessionId);
146+
rpc.sendErrorResponse(requestIdLong, -32602, "Unknown session " + sessionId);
143147
return;
144148
}
145149

146150
ToolDefinition tool = session.getTool(toolName);
147151
if (tool == null || tool.handler() == null) {
148152
var result = ToolResultObject.failure("Tool '" + toolName + "' is not supported.",
149153
"tool '" + toolName + "' not supported");
150-
rpc.sendResponse(Long.parseLong(requestId), Map.of("result", result));
154+
rpc.sendResponse(requestIdLong, Map.of("result", result));
151155
return;
152156
}
153157

@@ -163,7 +167,7 @@ private void handleToolCall(JsonRpcClient rpc, String requestId, JsonNode params
163167
toolResult = ToolResultObject
164168
.success(result instanceof String s ? s : MAPPER.writeValueAsString(result));
165169
}
166-
rpc.sendResponse(Long.parseLong(requestId), Map.of("result", toolResult));
170+
rpc.sendResponse(requestIdLong, Map.of("result", toolResult));
167171
} catch (Exception e) {
168172
LOG.log(Level.SEVERE, "Error sending tool result", e);
169173
}
@@ -172,7 +176,7 @@ private void handleToolCall(JsonRpcClient rpc, String requestId, JsonNode params
172176
var result = ToolResultObject.failure(
173177
"Invoking this tool produced an error. Detailed information is not available.",
174178
ex.getMessage());
175-
rpc.sendResponse(Long.parseLong(requestId), Map.of("result", result));
179+
rpc.sendResponse(requestIdLong, Map.of("result", result));
176180
} catch (Exception e) {
177181
LOG.log(Level.SEVERE, "Error sending tool error", e);
178182
}
@@ -181,7 +185,7 @@ private void handleToolCall(JsonRpcClient rpc, String requestId, JsonNode params
181185
} catch (Exception e) {
182186
LOG.log(Level.SEVERE, "Error handling tool call", e);
183187
try {
184-
rpc.sendErrorResponse(Long.parseLong(requestId), -32603, e.getMessage());
188+
rpc.sendErrorResponse(requestIdLong, -32603, e.getMessage());
185189
} catch (IOException ioe) {
186190
LOG.log(Level.SEVERE, "Failed to send error response", ioe);
187191
}
@@ -191,6 +195,10 @@ private void handleToolCall(JsonRpcClient rpc, String requestId, JsonNode params
191195

192196
private void handlePermissionRequest(JsonRpcClient rpc, String requestId, JsonNode params) {
193197
runAsync(() -> {
198+
final long requestIdLong = parseRequestId(requestId, "permission.request");
199+
if (requestIdLong == -1) {
200+
return;
201+
}
194202
try {
195203
String sessionId = params.get("sessionId").asText();
196204
JsonNode permissionRequest = params.get("permissionRequest");
@@ -199,7 +207,7 @@ private void handlePermissionRequest(JsonRpcClient rpc, String requestId, JsonNo
199207
if (session == null) {
200208
var result = new PermissionRequestResult()
201209
.setKind(PermissionRequestResultKind.DENIED_COULD_NOT_REQUEST_FROM_USER);
202-
rpc.sendResponse(Long.parseLong(requestId), Map.of("result", result));
210+
rpc.sendResponse(requestIdLong, Map.of("result", result));
203211
return;
204212
}
205213

@@ -212,15 +220,15 @@ private void handlePermissionRequest(JsonRpcClient rpc, String requestId, JsonNo
212220
throw new IllegalStateException(
213221
"Permission handlers cannot return 'no-result' when connected to a protocol v2 server.");
214222
}
215-
rpc.sendResponse(Long.parseLong(requestId), Map.of("result", result));
223+
rpc.sendResponse(requestIdLong, Map.of("result", result));
216224
} catch (IOException e) {
217225
LOG.log(Level.SEVERE, "Error sending permission result", e);
218226
}
219227
}).exceptionally(ex -> {
220228
try {
221229
var result = new PermissionRequestResult()
222230
.setKind(PermissionRequestResultKind.DENIED_COULD_NOT_REQUEST_FROM_USER);
223-
rpc.sendResponse(Long.parseLong(requestId), Map.of("result", result));
231+
rpc.sendResponse(requestIdLong, Map.of("result", result));
224232
} catch (IOException e) {
225233
LOG.log(Level.SEVERE, "Error sending permission denied", e);
226234
}
@@ -235,6 +243,10 @@ private void handlePermissionRequest(JsonRpcClient rpc, String requestId, JsonNo
235243
private void handleUserInputRequest(JsonRpcClient rpc, String requestId, JsonNode params) {
236244
LOG.fine("Received userInput.request: " + params);
237245
runAsync(() -> {
246+
final long requestIdLong = parseRequestId(requestId, "userInput.request");
247+
if (requestIdLong == -1) {
248+
return;
249+
}
238250
try {
239251
String sessionId = params.get("sessionId").asText();
240252
String question = params.get("question").asText();
@@ -246,7 +258,7 @@ private void handleUserInputRequest(JsonRpcClient rpc, String requestId, JsonNod
246258
LOG.fine("Found session: " + (session != null));
247259
if (session == null) {
248260
LOG.fine("Session not found, sending error");
249-
rpc.sendErrorResponse(Long.parseLong(requestId), -32602, "Unknown session " + sessionId);
261+
rpc.sendErrorResponse(requestIdLong, -32602, "Unknown session " + sessionId);
250262
return;
251263
}
252264

@@ -268,16 +280,15 @@ private void handleUserInputRequest(JsonRpcClient rpc, String requestId, JsonNod
268280
String answer = response.getAnswer() != null ? response.getAnswer() : "";
269281
LOG.fine("Sending userInput response: answer=" + answer + ", wasFreeform="
270282
+ response.isWasFreeform());
271-
rpc.sendResponse(Long.parseLong(requestId),
283+
rpc.sendResponse(requestIdLong,
272284
Map.of("answer", answer, "wasFreeform", response.isWasFreeform()));
273285
} catch (IOException e) {
274286
LOG.log(Level.SEVERE, "Error sending user input response", e);
275287
}
276288
}).exceptionally(ex -> {
277289
LOG.log(Level.WARNING, "User input handler exception", ex);
278290
try {
279-
rpc.sendErrorResponse(Long.parseLong(requestId), -32603,
280-
"User input handler error: " + ex.getMessage());
291+
rpc.sendErrorResponse(requestIdLong, -32603, "User input handler error: " + ex.getMessage());
281292
} catch (IOException e) {
282293
LOG.log(Level.SEVERE, "Error sending user input error", e);
283294
}
@@ -291,12 +302,16 @@ private void handleUserInputRequest(JsonRpcClient rpc, String requestId, JsonNod
291302

292303
private void handleExitPlanModeRequest(JsonRpcClient rpc, String requestId, JsonNode params) {
293304
runAsync(() -> {
305+
final long requestIdLong = parseRequestId(requestId, "exitPlanMode.request");
306+
if (requestIdLong == -1) {
307+
return;
308+
}
294309
try {
295310
String sessionId = params.get("sessionId").asText();
296311

297312
CopilotSession session = sessions.get(sessionId);
298313
if (session == null) {
299-
rpc.sendErrorResponse(Long.parseLong(requestId), -32602, "Unknown session " + sessionId);
314+
rpc.sendErrorResponse(requestIdLong, -32602, "Unknown session " + sessionId);
300315
return;
301316
}
302317

@@ -320,13 +335,13 @@ private void handleExitPlanModeRequest(JsonRpcClient rpc, String requestId, Json
320335

321336
session.handleExitPlanModeRequest(request).thenAccept(result -> {
322337
try {
323-
rpc.sendResponse(Long.parseLong(requestId), MAPPER.valueToTree(result));
338+
rpc.sendResponse(requestIdLong, MAPPER.valueToTree(result));
324339
} catch (IOException e) {
325340
LOG.log(Level.SEVERE, "Error sending exit plan mode response", e);
326341
}
327342
}).exceptionally(ex -> {
328343
try {
329-
rpc.sendErrorResponse(Long.parseLong(requestId), -32603,
344+
rpc.sendErrorResponse(requestIdLong, -32603,
330345
"Exit plan mode handler error: " + ex.getMessage());
331346
} catch (IOException e) {
332347
LOG.log(Level.SEVERE, "Error sending exit plan mode error", e);
@@ -341,12 +356,16 @@ private void handleExitPlanModeRequest(JsonRpcClient rpc, String requestId, Json
341356

342357
private void handleAutoModeSwitchRequest(JsonRpcClient rpc, String requestId, JsonNode params) {
343358
runAsync(() -> {
359+
final long requestIdLong = parseRequestId(requestId, "autoModeSwitch.request");
360+
if (requestIdLong == -1) {
361+
return;
362+
}
344363
try {
345364
String sessionId = params.get("sessionId").asText();
346365

347366
CopilotSession session = sessions.get(sessionId);
348367
if (session == null) {
349-
rpc.sendErrorResponse(Long.parseLong(requestId), -32602, "Unknown session " + sessionId);
368+
rpc.sendErrorResponse(requestIdLong, -32602, "Unknown session " + sessionId);
350369
return;
351370
}
352371

@@ -360,13 +379,13 @@ private void handleAutoModeSwitchRequest(JsonRpcClient rpc, String requestId, Js
360379

361380
session.handleAutoModeSwitchRequest(request).thenAccept(response -> {
362381
try {
363-
rpc.sendResponse(Long.parseLong(requestId), Map.of("response", response));
382+
rpc.sendResponse(requestIdLong, Map.of("response", response));
364383
} catch (IOException e) {
365384
LOG.log(Level.SEVERE, "Error sending auto mode switch response", e);
366385
}
367386
}).exceptionally(ex -> {
368387
try {
369-
rpc.sendErrorResponse(Long.parseLong(requestId), -32603,
388+
rpc.sendErrorResponse(requestIdLong, -32603,
370389
"Auto mode switch handler error: " + ex.getMessage());
371390
} catch (IOException e) {
372391
LOG.log(Level.SEVERE, "Error sending auto mode switch error", e);
@@ -381,27 +400,30 @@ private void handleAutoModeSwitchRequest(JsonRpcClient rpc, String requestId, Js
381400

382401
private void handleHooksInvoke(JsonRpcClient rpc, String requestId, JsonNode params) {
383402
runAsync(() -> {
403+
final long requestIdLong = parseRequestId(requestId, "hooks.invoke");
404+
if (requestIdLong == -1) {
405+
return;
406+
}
384407
try {
385408
String sessionId = params.get("sessionId").asText();
386409
String hookType = params.get("hookType").asText();
387410
JsonNode input = params.get("input");
388411

389412
CopilotSession session = sessions.get(sessionId);
390413
if (session == null) {
391-
rpc.sendErrorResponse(Long.parseLong(requestId), -32602, "Unknown session " + sessionId);
414+
rpc.sendErrorResponse(requestIdLong, -32602, "Unknown session " + sessionId);
392415
return;
393416
}
394417

395418
session.handleHooksInvoke(hookType, input).thenAccept(output -> {
396419
try {
397-
rpc.sendResponse(Long.parseLong(requestId), Collections.singletonMap("output", output));
420+
rpc.sendResponse(requestIdLong, Collections.singletonMap("output", output));
398421
} catch (IOException e) {
399422
LOG.log(Level.SEVERE, "Error sending hooks response", e);
400423
}
401424
}).exceptionally(ex -> {
402425
try {
403-
rpc.sendErrorResponse(Long.parseLong(requestId), -32603,
404-
"Hooks handler error: " + ex.getMessage());
426+
rpc.sendErrorResponse(requestIdLong, -32603, "Hooks handler error: " + ex.getMessage());
405427
} catch (IOException e) {
406428
LOG.log(Level.SEVERE, "Error sending hooks error", e);
407429
}
@@ -424,15 +446,11 @@ interface LifecycleEventDispatcher {
424446

425447
private void handleSystemMessageTransform(JsonRpcClient rpc, String requestId, JsonNode params) {
426448
runAsync(() -> {
449+
final long requestIdLong = parseRequestId(requestId, "systemMessage.transform");
450+
if (requestIdLong == -1) {
451+
return;
452+
}
427453
try {
428-
final long requestIdLong;
429-
try {
430-
requestIdLong = Long.parseLong(requestId);
431-
} catch (NumberFormatException nfe) {
432-
LOG.log(Level.SEVERE, "Invalid requestId for systemMessage.transform: " + requestId, nfe);
433-
return;
434-
}
435-
436454
String sessionId = params.has("sessionId") ? params.get("sessionId").asText() : null;
437455
JsonNode sections = params.get("sections");
438456

@@ -462,6 +480,25 @@ private void handleSystemMessageTransform(JsonRpcClient rpc, String requestId, J
462480
});
463481
}
464482

483+
/**
484+
* Parses a JSON-RPC request ID string into a {@code long}.
485+
*
486+
* @param requestId
487+
* the request ID string received from the JSON-RPC layer
488+
* @param methodName
489+
* the RPC method name, used in the log message on failure
490+
* @return the parsed request ID, or {@code -1} if the string is not a valid
491+
* long
492+
*/
493+
private static long parseRequestId(String requestId, String methodName) {
494+
try {
495+
return Long.parseLong(requestId);
496+
} catch (NumberFormatException nfe) {
497+
LOG.log(Level.SEVERE, "Invalid requestId for " + methodName + ": " + requestId, nfe);
498+
return -1;
499+
}
500+
}
501+
465502
private void runAsync(Runnable task) {
466503
try {
467504
if (executor != null) {

‎src/site/markdown/advanced.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1280,6 +1280,7 @@ When the model finishes creating a plan and wants to transition out of plan mode
12801280

12811281
```java
12821282
var session = client.createSession(new SessionConfig()
1283+
.setOnPermissionRequest(PermissionHandler.APPROVE_ALL)
12831284
.setOnExitPlanMode((request, invocation) -> {
12841285
System.out.println("Plan summary: " + request.getSummary());
12851286
System.out.println("Available actions: " + request.getActions());
@@ -1308,6 +1309,7 @@ When the model encounters a rate limit or similar constraint, it may request to
13081309

13091310
```java
13101311
var session = client.createSession(new SessionConfig()
1312+
.setOnPermissionRequest(PermissionHandler.APPROVE_ALL)
13111313
.setOnAutoModeSwitch((request, invocation) -> {
13121314
System.out.println("Error: " + request.getErrorCode());
13131315
System.out.println("Retry after: " + request.getRetryAfterSeconds() + "s");

‎src/test/java/com/github/copilot/sdk/ModeHandlersTest.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,15 +59,16 @@ private CopilotClient createAuthenticatedClient() {
5959
return ctx.createClient(new CopilotClientOptions().setEnvironment(env));
6060
}
6161

62-
private void configureAuthenticatedUser() throws Exception {
62+
private void configureAuthenticatedUser(String testName) throws Exception {
63+
ctx.configureForTest("mode_handlers", testName);
6364
ctx.setCopilotUserByToken(TOKEN, "mode-handler-user", "individual_pro", ctx.getProxyUrl(),
6465
"https://localhost:1/telemetry", "mode-handler-tracking-id");
6566
}
6667

6768
@Test
6869
void shouldInvokeExitPlanModeHandlerWhenModelUsesTool() throws Exception {
6970
final String summary = "Greeting file implementation plan";
70-
configureAuthenticatedUser();
71+
configureAuthenticatedUser("should_invoke_exit_plan_mode_handler_when_model_uses_tool");
7172

7273
var handlerCalled = new CompletableFuture<ExitPlanModeRequest>();
7374

@@ -118,7 +119,7 @@ void shouldInvokeExitPlanModeHandlerWhenModelUsesTool() throws Exception {
118119

119120
@Test
120121
void shouldInvokeAutoModeSwitchHandlerWhenRateLimited() throws Exception {
121-
configureAuthenticatedUser();
122+
configureAuthenticatedUser("should_invoke_auto_mode_switch_handler_when_rate_limited");
122123

123124
var handlerCalled = new CompletableFuture<AutoModeSwitchRequest>();
124125

@@ -131,10 +132,13 @@ void shouldInvokeAutoModeSwitchHandlerWhenRateLimited() throws Exception {
131132
}))
132133
.get(30, TimeUnit.SECONDS);
133134

134-
session.sendAndWait(new MessageOptions()
135+
var messageId = session.send(new MessageOptions()
135136
.setPrompt("Explain that auto mode recovered from a rate limit in one short sentence."))
136137
.get(30, TimeUnit.SECONDS);
137138

139+
assertNotNull(messageId);
140+
assertFalse(messageId.isEmpty());
141+
138142
var request = handlerCalled.get(30, TimeUnit.SECONDS);
139143
assertEquals("user_weekly_rate_limited", request.getErrorCode());
140144
assertEquals(1.0, request.getRetryAfterSeconds());

‎src/test/java/com/github/copilot/sdk/SessionEventHandlingTest.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ void testHandlerReceivesCorrectEventData() {
180180

181181
SessionStartEvent startEvent = createSessionStartEvent();
182182
startEvent.setData(new SessionStartEvent.SessionStartEventData("my-session-123", null, null, null, null, null,
183-
null, null, null, null));
183+
null, null, null, null, null));
184184
dispatchEvent(startEvent);
185185

186186
AssistantMessageEvent msgEvent = createAssistantMessageEvent("Test content");
@@ -857,15 +857,15 @@ private SessionStartEvent createSessionStartEvent() {
857857
private SessionStartEvent createSessionStartEvent(String sessionId) {
858858
var event = new SessionStartEvent();
859859
var data = new SessionStartEvent.SessionStartEventData(sessionId, null, null, null, null, null, null, null,
860-
null, null);
860+
null, null, null);
861861
event.setData(data);
862862
return event;
863863
}
864864

865865
private AssistantMessageEvent createAssistantMessageEvent(String content) {
866866
var event = new AssistantMessageEvent();
867-
var data = new AssistantMessageEvent.AssistantMessageEventData(null, content, null, null, null, null, null,
868-
null, null, null, null, null);
867+
var data = new AssistantMessageEvent.AssistantMessageEventData(null, null, content, null, null, null, null,
868+
null, null, null, null, null, null, null, null);
869869
event.setData(data);
870870
return event;
871871
}

0 commit comments

Comments
 (0)