Skip to content

Commit 2ff2844

Browse files
Copilotedburns
Copilot
and
edburns
authored
Address PR review: fix EMPTY mode validation, hook null-checks, and Javadoc
- Update EMPTY mode error message to mention both CopilotHome and CliUrl - Add availableTools validation in resumeSession() for EMPTY mode - Add null-checks for hook handler return values to prevent NPE - Fix misleading Javadoc in SessionConfig/ResumeSessionConfig for EMPTY-mode options that are not yet wired to the protocol - Fix Javadoc formatting in ResumeSessionConfig clear* methods - Update CopilotClientMode.EMPTY Javadoc to match actual validation Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
1 parent d9f6c85 commit 2ff2844

6 files changed

Lines changed: 88 additions & 38 deletions

File tree

‎src/main/java/com/github/copilot/CopilotClient.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ public CopilotClient(CopilotClientOptions options) {
151151
|| (this.options.getCliUrl() != null && !this.options.getCliUrl().isEmpty());
152152
if (!hasPersistence) {
153153
throw new IllegalArgumentException(
154-
"CopilotClient was created with Mode = EMPTY but CopilotHome was not set. "
154+
"CopilotClient was created with Mode = EMPTY but neither CopilotHome nor CliUrl was set. "
155155
+ "Empty mode requires an explicit per-session persistence location.");
156156
}
157157
}
@@ -568,8 +568,15 @@ public CompletableFuture<CopilotSession> resumeSession(String sessionId, ResumeS
568568
request.setSystemMessage(extracted.wireSystemMessage());
569569
}
570570

571-
// Empty mode: set toolFilterPrecedence for resume path
571+
// Empty mode: validate availableTools and set toolFilterPrecedence for resume
572+
// path
572573
if (options.getMode() == CopilotClientMode.EMPTY) {
574+
if (config.getAvailableTools() == null) {
575+
throw new IllegalArgumentException(
576+
"CopilotClient is in Mode = EMPTY but the resume session config did not specify "
577+
+ "availableTools. Empty mode requires every session to explicitly opt into "
578+
+ "the tools it wants — e.g. setAvailableTools(new ToolSet().addBuiltIn(BuiltInTools.ISOLATED)).");
579+
}
573580
request.setToolFilterPrecedence("excluded");
574581
}
575582

‎src/main/java/com/github/copilot/CopilotSession.java‎

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1546,52 +1546,73 @@ CompletableFuture<Object> handleHooksInvoke(String hookType, JsonNode input) {
15461546
case "preToolUse" :
15471547
if (hooks.getOnPreToolUse() != null) {
15481548
PreToolUseHookInput preInput = MAPPER.treeToValue(input, PreToolUseHookInput.class);
1549-
return hooks.getOnPreToolUse().handle(preInput, invocation)
1550-
.thenApply(output -> (Object) output);
1549+
var preResult = hooks.getOnPreToolUse().handle(preInput, invocation);
1550+
if (preResult == null) {
1551+
return CompletableFuture.completedFuture(null);
1552+
}
1553+
return preResult.thenApply(output -> (Object) output);
15511554
}
15521555
break;
15531556
case "preMcpToolCall" :
15541557
if (hooks.getOnPreMcpToolCall() != null) {
15551558
PreMcpToolCallHookInput mcpInput = MAPPER.treeToValue(input, PreMcpToolCallHookInput.class);
1556-
return hooks.getOnPreMcpToolCall().handle(mcpInput, invocation)
1557-
.thenApply(output -> (Object) output);
1559+
var mcpResult = hooks.getOnPreMcpToolCall().handle(mcpInput, invocation);
1560+
if (mcpResult == null) {
1561+
return CompletableFuture.completedFuture(null);
1562+
}
1563+
return mcpResult.thenApply(output -> (Object) output);
15581564
}
15591565
break;
15601566
case "postToolUse" :
15611567
if (hooks.getOnPostToolUse() != null) {
15621568
PostToolUseHookInput postInput = MAPPER.treeToValue(input, PostToolUseHookInput.class);
1563-
return hooks.getOnPostToolUse().handle(postInput, invocation)
1564-
.thenApply(output -> (Object) output);
1569+
var postResult = hooks.getOnPostToolUse().handle(postInput, invocation);
1570+
if (postResult == null) {
1571+
return CompletableFuture.completedFuture(null);
1572+
}
1573+
return postResult.thenApply(output -> (Object) output);
15651574
}
15661575
break;
15671576
case "postToolUseFailure" :
15681577
if (hooks.getOnPostToolUseFailure() != null) {
15691578
PostToolUseFailureHookInput failureInput = MAPPER.treeToValue(input,
15701579
PostToolUseFailureHookInput.class);
1571-
return hooks.getOnPostToolUseFailure().handle(failureInput, invocation)
1572-
.thenApply(output -> (Object) output);
1580+
var failureResult = hooks.getOnPostToolUseFailure().handle(failureInput, invocation);
1581+
if (failureResult == null) {
1582+
return CompletableFuture.completedFuture(null);
1583+
}
1584+
return failureResult.thenApply(output -> (Object) output);
15731585
}
15741586
break;
15751587
case "userPromptSubmitted" :
15761588
if (hooks.getOnUserPromptSubmitted() != null) {
15771589
UserPromptSubmittedHookInput promptInput = MAPPER.treeToValue(input,
15781590
UserPromptSubmittedHookInput.class);
1579-
return hooks.getOnUserPromptSubmitted().handle(promptInput, invocation)
1580-
.thenApply(output -> (Object) output);
1591+
var promptResult = hooks.getOnUserPromptSubmitted().handle(promptInput, invocation);
1592+
if (promptResult == null) {
1593+
return CompletableFuture.completedFuture(null);
1594+
}
1595+
return promptResult.thenApply(output -> (Object) output);
15811596
}
15821597
break;
15831598
case "sessionStart" :
15841599
if (hooks.getOnSessionStart() != null) {
15851600
SessionStartHookInput startInput = MAPPER.treeToValue(input, SessionStartHookInput.class);
1586-
return hooks.getOnSessionStart().handle(startInput, invocation)
1587-
.thenApply(output -> (Object) output);
1601+
var startResult = hooks.getOnSessionStart().handle(startInput, invocation);
1602+
if (startResult == null) {
1603+
return CompletableFuture.completedFuture(null);
1604+
}
1605+
return startResult.thenApply(output -> (Object) output);
15881606
}
15891607
break;
15901608
case "sessionEnd" :
15911609
if (hooks.getOnSessionEnd() != null) {
15921610
SessionEndHookInput endInput = MAPPER.treeToValue(input, SessionEndHookInput.class);
1593-
return hooks.getOnSessionEnd().handle(endInput, invocation)
1594-
.thenApply(output -> (Object) output);
1611+
var endResult = hooks.getOnSessionEnd().handle(endInput, invocation);
1612+
if (endResult == null) {
1613+
return CompletableFuture.completedFuture(null);
1614+
}
1615+
return endResult.thenApply(output -> (Object) output);
15951616
}
15961617
break;
15971618
default :

‎src/main/java/com/github/copilot/rpc/CopilotClientMode.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ public enum CopilotClientMode {
2020
* When this mode is selected:
2121
* <ul>
2222
* <li>The client constructor requires
23-
* {@link CopilotClientOptions#getCopilotHome()} or a session filesystem to be
24-
* set.</li>
23+
* {@link CopilotClientOptions#getCopilotHome()} or
24+
* {@link CopilotClientOptions#getCliUrl()} to be set.</li>
2525
* <li>{@link SessionConfig#getAvailableTools()} must be supplied on every
2626
* session — no tools are exposed by default.</li>
2727
* <li>{@code session.create} always sets

‎src/main/java/com/github/copilot/rpc/ResumeSessionConfig.java‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,10 @@ public Optional<Boolean> getSkipCustomInstructions() {
296296

297297
/**
298298
* Sets whether to suppress loading of custom instruction files.
299+
* <p>
300+
* <b>Note:</b> This option is not yet propagated to the wire protocol. It is
301+
* reserved for future SDK defaulting behavior. Setting it currently has no
302+
* effect on the session resume request.
299303
*
300304
* @param skipCustomInstructions
301305
* whether to skip custom instructions
@@ -308,8 +312,9 @@ public ResumeSessionConfig setSkipCustomInstructions(boolean skipCustomInstructi
308312
}
309313

310314
/**
311-
* Clears the skipCustomInstructions setting. @return this instance for method
312-
* chaining
315+
* Clears the skipCustomInstructions setting.
316+
*
317+
* @return this instance for method chaining
313318
*/
314319
public ResumeSessionConfig clearSkipCustomInstructions() {
315320
this.skipCustomInstructions = null;
@@ -330,6 +335,10 @@ public Optional<Boolean> getCustomAgentsLocalOnly() {
330335
/**
331336
* Sets whether custom-agent discovery is restricted to the session's local
332337
* working directory.
338+
* <p>
339+
* <b>Note:</b> This option is not yet propagated to the wire protocol. It is
340+
* reserved for future SDK defaulting behavior. Setting it currently has no
341+
* effect on the session resume request.
333342
*
334343
* @param customAgentsLocalOnly
335344
* whether to restrict to local agents
@@ -342,8 +351,9 @@ public ResumeSessionConfig setCustomAgentsLocalOnly(boolean customAgentsLocalOnl
342351
}
343352

344353
/**
345-
* Clears the customAgentsLocalOnly setting. @return this instance for method
346-
* chaining
354+
* Clears the customAgentsLocalOnly setting.
355+
*
356+
* @return this instance for method chaining
347357
*/
348358
public ResumeSessionConfig clearCustomAgentsLocalOnly() {
349359
this.customAgentsLocalOnly = null;
@@ -364,6 +374,10 @@ public Optional<Boolean> getCoauthorEnabled() {
364374
/**
365375
* Sets whether the runtime is allowed to append a {@code Co-authored-by}
366376
* trailer.
377+
* <p>
378+
* <b>Note:</b> This option is not yet propagated to the wire protocol. It is
379+
* reserved for future SDK defaulting behavior. Setting it currently has no
380+
* effect on the session resume request.
367381
*
368382
* @param coauthorEnabled
369383
* whether coauthor is enabled
@@ -376,7 +390,9 @@ public ResumeSessionConfig setCoauthorEnabled(boolean coauthorEnabled) {
376390
}
377391

378392
/**
379-
* Clears the coauthorEnabled setting. @return this instance for method chaining
393+
* Clears the coauthorEnabled setting.
394+
*
395+
* @return this instance for method chaining
380396
*/
381397
public ResumeSessionConfig clearCoauthorEnabled() {
382398
this.coauthorEnabled = null;
@@ -396,6 +412,10 @@ public Optional<Boolean> getManageScheduleEnabled() {
396412

397413
/**
398414
* Sets whether to enable the {@code manage_schedule} tool.
415+
* <p>
416+
* <b>Note:</b> This option is not yet propagated to the wire protocol. It is
417+
* reserved for future SDK defaulting behavior. Setting it currently has no
418+
* effect on the session resume request.
399419
*
400420
* @param manageScheduleEnabled
401421
* whether manage schedule is enabled
@@ -408,8 +428,9 @@ public ResumeSessionConfig setManageScheduleEnabled(boolean manageScheduleEnable
408428
}
409429

410430
/**
411-
* Clears the manageScheduleEnabled setting. @return this instance for method
412-
* chaining
431+
* Clears the manageScheduleEnabled setting.
432+
*
433+
* @return this instance for method chaining
413434
*/
414435
public ResumeSessionConfig clearManageScheduleEnabled() {
415436
this.manageScheduleEnabled = null;

‎src/main/java/com/github/copilot/rpc/SessionConfig.java‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -354,9 +354,9 @@ public Optional<Boolean> getSkipCustomInstructions() {
354354
* {@code .github/copilot-instructions.md}, {@code AGENTS.md}) from the working
355355
* directory.
356356
* <p>
357-
* When {@code null}, the SDK chooses based on
358-
* {@link CopilotClientOptions#getMode()}: {@code true} under
359-
* {@link CopilotClientMode#EMPTY}, {@code null} otherwise.
357+
* <b>Note:</b> This option is not yet propagated to the wire protocol. It is
358+
* reserved for future SDK defaulting behavior. Setting it currently has no
359+
* effect on the session creation request.
360360
*
361361
* @param skipCustomInstructions
362362
* whether to skip custom instructions
@@ -394,9 +394,9 @@ public Optional<Boolean> getCustomAgentsLocalOnly() {
394394
* Sets whether custom-agent discovery is restricted to the session's local
395395
* working directory (no organisation-level discovery).
396396
* <p>
397-
* When {@code null}, the SDK chooses based on
398-
* {@link CopilotClientOptions#getMode()}: {@code true} under
399-
* {@link CopilotClientMode#EMPTY}, {@code null} otherwise.
397+
* <b>Note:</b> This option is not yet propagated to the wire protocol. It is
398+
* reserved for future SDK defaulting behavior. Setting it currently has no
399+
* effect on the session creation request.
400400
*
401401
* @param customAgentsLocalOnly
402402
* whether to restrict to local agents
@@ -434,9 +434,9 @@ public Optional<Boolean> getCoauthorEnabled() {
434434
* Sets whether the runtime is allowed to append a {@code Co-authored-by}
435435
* trailer when it commits on behalf of the user.
436436
* <p>
437-
* When {@code null}, the SDK chooses based on
438-
* {@link CopilotClientOptions#getMode()}: {@code false} under
439-
* {@link CopilotClientMode#EMPTY}, {@code null} otherwise.
437+
* <b>Note:</b> This option is not yet propagated to the wire protocol. It is
438+
* reserved for future SDK defaulting behavior. Setting it currently has no
439+
* effect on the session creation request.
440440
*
441441
* @param coauthorEnabled
442442
* whether coauthor is enabled
@@ -474,9 +474,9 @@ public Optional<Boolean> getManageScheduleEnabled() {
474474
* Sets whether to enable the {@code manage_schedule} tool (host scheduler
475475
* integration).
476476
* <p>
477-
* When {@code null}, the SDK chooses based on
478-
* {@link CopilotClientOptions#getMode()}: {@code false} under
479-
* {@link CopilotClientMode#EMPTY}, {@code null} otherwise.
477+
* <b>Note:</b> This option is not yet propagated to the wire protocol. It is
478+
* reserved for future SDK defaulting behavior. Setting it currently has no
479+
* effect on the session creation request.
480480
*
481481
* @param manageScheduleEnabled
482482
* whether manage schedule is enabled

‎src/test/java/com/github/copilot/SessionEventDeserializationTest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2399,7 +2399,8 @@ void testParseCapabilitiesChangedEvent() throws Exception {
23992399
assertTrue(castedEvent.getData().ui().elicitation());
24002400

24012401
// Verify setData round-trip
2402-
var newData = new CapabilitiesChangedEvent.CapabilitiesChangedEventData(new CapabilitiesChangedUI(false, null, null));
2402+
var newData = new CapabilitiesChangedEvent.CapabilitiesChangedEventData(
2403+
new CapabilitiesChangedUI(false, null, null));
24032404
castedEvent.setData(newData);
24042405
assertFalse(castedEvent.getData().ui().elicitation());
24052406
}

0 commit comments

Comments
 (0)