Skip to content

Commit 3fe2908

Browse files
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 2a3e7a5 commit 3fe2908

1 file changed

Lines changed: 3 additions & 2 deletions

File tree

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,9 @@ void shouldInvokeAutoModeSwitchHandlerWhenRateLimited() throws Exception {
132132
}))
133133
.get(30, TimeUnit.SECONDS);
134134

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

139140
assertNotNull(messageId);

0 commit comments

Comments
 (0)