Skip to content

Commit 88eefee

Browse files
author
Mark Pollack
committed
Null-mark the spec package
AcpSchema's record components follow the ACP schema (v1, unstable superset): 217 optional or nullable components are @nullable, required ones stay non-null, and the convenience constructors that forward to optional components take @nullable arguments. The JSON-RPC envelope's id, params, result, error and data are @nullable. Marking the sessions surfaced four wire bugs, each with a test: - a request or notification that omits params (legal JSON-RPC) reached its handler as null; it now reads as an empty object; - a request handler that completed empty sent no response at all; it is now answered with INTERNAL_ERROR; - an exception without a message produced a JSON-RPC error without the required message; the exception's type name is used instead; - a success response without a result failed with an opaque NullPointerException; it now fails saying so. A terminal command killed by a signal has no exit code, and execute() unboxed it into a NullPointerException. CommandResult now carries a nullable exit code and the signal (with a test).
1 parent 96c0946 commit 88eefee

13 files changed

Lines changed: 487 additions & 256 deletions

File tree

‎acp-agent-support/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ PromptResponse handle(PromptRequest req, SyncPromptContext ctx) {
171171

172172
// Execute terminal commands (requires client capabilities)
173173
CommandResult result = ctx.execute("ls", "-la");
174-
if (result.exitCode() == 0) {
174+
if (result.success()) {
175175
ctx.sendMessage("Output: " + result.output());
176176
}
177177

‎acp-core/src/main/java/com/agentclientprotocol/sdk/agent/CommandResult.java‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
package com.agentclientprotocol.sdk.agent;
66

7+
import org.jspecify.annotations.Nullable;
8+
79
/**
810
* Result of executing a terminal command via the convenience API.
911
*
@@ -16,15 +18,17 @@
1618
* Example usage:
1719
* <pre>{@code
1820
* CommandResult result = context.execute("make", "build");
19-
* if (result.exitCode() == 0) {
21+
* if (result.success()) {
2022
* context.sendMessage("Build succeeded!");
2123
* } else {
2224
* context.sendMessage("Build failed: " + result.output());
2325
* }
2426
* }</pre>
2527
*
2628
* @param output The combined stdout/stderr output from the command
27-
* @param exitCode The exit code (0 typically means success)
29+
* @param exitCode The exit code (0 typically means success), or null if the process was
30+
* terminated by a signal
31+
* @param signal The signal that terminated the process, or null if it exited normally
2832
* @param timedOut Whether the command was terminated due to timeout
2933
* @author Mark Pollack
3034
* @since 0.9.2
@@ -33,7 +37,8 @@
3337
*/
3438
public record CommandResult(
3539
String output,
36-
int exitCode,
40+
@Nullable Integer exitCode,
41+
@Nullable String signal,
3742
boolean timedOut
3843
) {
3944

@@ -44,15 +49,25 @@ public record CommandResult(
4449
* @param exitCode The exit code
4550
*/
4651
public CommandResult(String output, int exitCode) {
47-
this(output, exitCode, false);
52+
this(output, exitCode, null, false);
53+
}
54+
55+
/**
56+
* Creates a CommandResult for a process that exited with the given code.
57+
* @param output The command output
58+
* @param exitCode The exit code
59+
* @param timedOut Whether the command was terminated due to timeout
60+
*/
61+
public CommandResult(String output, int exitCode, boolean timedOut) {
62+
this(output, exitCode, null, timedOut);
4863
}
4964

5065
/**
5166
* Returns true if the command completed successfully (exit code 0).
52-
* @return true if exit code is 0 and command did not time out
67+
* @return true if the process exited with code 0 and did not time out
5368
*/
5469
public boolean success() {
55-
return exitCode == 0 && !timedOut;
70+
return exitCode != null && exitCode == 0 && !timedOut;
5671
}
5772

5873
}

‎acp-core/src/main/java/com/agentclientprotocol/sdk/agent/DefaultPromptContext.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,8 @@ public Mono<CommandResult> execute(Command command) {
223223

224224
return waitForTerminalExit(new WaitForTerminalExitRequest(sessionId, terminalId))
225225
.flatMap(exitResp -> getTerminalOutput(new TerminalOutputRequest(sessionId, terminalId))
226-
.map(outputResp -> new CommandResult(outputResp.output(), exitResp.exitCode(), false)))
226+
.map(outputResp -> new CommandResult(outputResp.output(), exitResp.exitCode(),
227+
exitResp.signal(), false)))
227228
// Release terminal after getting result, then return result
228229
.flatMap(result -> releaseTerminal(releaseReq).thenReturn(result))
229230
// On error, still release terminal before propagating error

‎acp-core/src/main/java/com/agentclientprotocol/sdk/client/transport/StreamableHttpAcpClientTransport.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -702,9 +702,13 @@ private Mono<Void> processNewSessionResponse(AcpSchema.JSONRPCResponse response)
702702
return emitInbound(response);
703703
}
704704

705+
Object result = response.result();
706+
if (result == null) {
707+
return emitInbound(errorResponse(response.id(), "session/new response carried no result", null));
708+
}
705709
String sessionId;
706710
try {
707-
AcpSchema.NewSessionResponse sessionResponse = jsonMapper.convertValue(response.result(),
711+
AcpSchema.NewSessionResponse sessionResponse = jsonMapper.convertValue(result,
708712
new TypeRef<AcpSchema.NewSessionResponse>() {
709713
});
710714
sessionId = sessionResponse.sessionId();
@@ -721,7 +725,7 @@ private Mono<Void> processNewSessionResponse(AcpSchema.JSONRPCResponse response)
721725
"Failed to open session SSE stream for session " + sessionId, error)));
722726
}
723727

724-
private AcpSchema.JSONRPCResponse errorResponse(Object id, String message, @Nullable Throwable error) {
728+
private AcpSchema.JSONRPCResponse errorResponse(@Nullable Object id, String message, @Nullable Throwable error) {
725729
Object data = error == null ? null : error.getMessage();
726730
return new AcpSchema.JSONRPCResponse(AcpSchema.JSONRPC_VERSION, id, null,
727731
new AcpSchema.JSONRPCError(AcpErrorCodes.INTERNAL_ERROR, message, data));

‎acp-core/src/main/java/com/agentclientprotocol/sdk/spec/AcpAgentSession.java‎

Lines changed: 47 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import com.agentclientprotocol.sdk.util.AcpSchedulers;
2020
import com.agentclientprotocol.sdk.util.Assert;
2121
import com.agentclientprotocol.sdk.json.TypeRef;
22+
import org.jspecify.annotations.Nullable;
2223
import org.slf4j.Logger;
2324
import org.slf4j.LoggerFactory;
2425
import reactor.core.publisher.Mono;
@@ -94,12 +95,12 @@ public class AcpAgentSession implements AcpSession {
9495
* Set when the transport's {@code start()} fails (already started, port in use, ...).
9596
* Requests to the client are then failed immediately with the cause.
9697
*/
97-
private volatile Throwable startFailure;
98+
private volatile @Nullable Throwable startFailure;
9899

99100
/**
100101
* Represents an active prompt session for single-turn enforcement.
101102
*/
102-
private record ActivePrompt(String sessionId, Object requestId) {
103+
private record ActivePrompt(String sessionId, @Nullable Object requestId) {
103104
}
104105

105106
/**
@@ -113,8 +114,9 @@ public interface RequestHandler<T> {
113114

114115
/**
115116
* Handles an incoming request with the given parameters.
116-
* @param params The request parameters
117-
* @return A Mono containing the response object
117+
* @param params The request parameters; an omitted params arrives as an empty
118+
* object
119+
* @return A Mono containing the response object; it must not complete empty
118120
*/
119121
Mono<T> handle(Object params);
120122

@@ -129,7 +131,8 @@ public interface NotificationHandler {
129131

130132
/**
131133
* Handles an incoming notification with the given parameters.
132-
* @param params The notification parameters
134+
* @param params The notification parameters; an omitted params arrives as an empty
135+
* object
133136
* @return A Mono that completes when the notification is processed
134137
*/
135138
Mono<Void> handle(Object params);
@@ -185,7 +188,7 @@ private void dismissPendingResponses() {
185188
dismissPendingResponses(null);
186189
}
187190

188-
private void dismissPendingResponses(Throwable cause) {
191+
private void dismissPendingResponses(@Nullable Throwable cause) {
189192
this.pendingResponses.forEach((id, sink) -> {
190193
logger.warn("Abruptly terminating exchange for request {}", id);
191194
sink.error(new RuntimeException("ACP session with client terminated", cause));
@@ -231,7 +234,7 @@ else if (message instanceof AcpSchema.JSONRPCRequest request) {
231234
errorCode = AcpErrorCodes.INTERNAL_ERROR;
232235
}
233236
var errorResponse = new AcpSchema.JSONRPCResponse(AcpSchema.JSONRPC_VERSION, request.id(), null,
234-
new AcpSchema.JSONRPCError(errorCode, error.getMessage(), errorData));
237+
new AcpSchema.JSONRPCError(errorCode, errorMessage(error), errorData));
235238
return Mono.just(errorResponse);
236239
}).map(response -> (AcpSchema.JSONRPCMessage) response);
237240
}
@@ -285,14 +288,14 @@ private Mono<AcpSchema.JSONRPCResponse> handleIncomingRequest(AcpSchema.JSONRPCR
285288
// ordered before publication on every path; doFinally covers cancellation.
286289
// Mono.defer keeps a handler that throws synchronously from holding the lock
287290
// forever: the throw becomes an error signal that passes through the release.
288-
return Mono.defer(() -> handler.handle(request.params()))
291+
return requireResult(Mono.defer(() -> handler.handle(paramsOrEmpty(request.params()))), request.method())
289292
.map(result -> new AcpSchema.JSONRPCResponse(AcpSchema.JSONRPC_VERSION, request.id(), result, null))
290293
.doOnNext(response -> releasePrompt(sessionId, newPrompt, "response"))
291294
.doOnError(error -> releasePrompt(sessionId, newPrompt, "error"))
292295
.doFinally(signal -> releasePrompt(sessionId, newPrompt, signal.toString()));
293296
}
294297

295-
return Mono.defer(() -> handler.handle(request.params()))
298+
return requireResult(Mono.defer(() -> handler.handle(paramsOrEmpty(request.params()))), request.method())
296299
.map(result -> new AcpSchema.JSONRPCResponse(AcpSchema.JSONRPC_VERSION, request.id(), result, null));
297300
});
298301
}
@@ -313,7 +316,7 @@ private void releasePrompt(String sessionId, ActivePrompt prompt, String reason)
313316
* Extracts the sessionId from request parameters, which arrive as a {@code Map} from
314317
* the JSON transports and as the typed record from in-process transports.
315318
*/
316-
private String extractSessionId(Object params) {
319+
private String extractSessionId(@Nullable Object params) {
317320
if (params instanceof AcpSchema.PromptRequest promptRequest) {
318321
return promptRequest.sessionId() != null ? promptRequest.sessionId() : "unknown";
319322
}
@@ -327,7 +330,7 @@ private String extractSessionId(Object params) {
327330
return "unknown";
328331
}
329332

330-
record MethodNotFoundError(String method, String message, Object data) {
333+
record MethodNotFoundError(String method, String message, @Nullable Object data) {
331334
}
332335

333336
private MethodNotFoundError getMethodNotFoundError(String method) {
@@ -356,7 +359,7 @@ private Mono<Void> handleIncomingNotification(AcpSchema.JSONRPCNotification noti
356359
logger.warn("No handler registered for notification method: {}", notification.method());
357360
return Mono.empty();
358361
}
359-
return handler.handle(notification.params());
362+
return handler.handle(paramsOrEmpty(notification.params()));
360363
});
361364
}
362365

@@ -415,7 +418,14 @@ public <T> Mono<T> sendRequest(String method, Object requestParams, TypeRef<T> t
415418
deliveredResponseSink.complete();
416419
}
417420
else {
418-
deliveredResponseSink.next(this.transport.unmarshalFrom(jsonRpcResponse.result(), typeRef));
421+
Object result = jsonRpcResponse.result();
422+
if (result == null) {
423+
deliveredResponseSink.error(new AcpProtocolException(AcpErrorCodes.INTERNAL_ERROR,
424+
"The response to " + method + " carried no result"));
425+
}
426+
else {
427+
deliveredResponseSink.next(this.transport.unmarshalFrom(result, typeRef));
428+
}
419429
}
420430
}
421431
});
@@ -429,7 +439,7 @@ public <T> Mono<T> sendRequest(String method, Object requestParams, TypeRef<T> t
429439
* @return a Mono that completes when the notification is sent
430440
*/
431441
@Override
432-
public Mono<Void> sendNotification(String method, Object params) {
442+
public Mono<Void> sendNotification(String method, @Nullable Object params) {
433443
return Mono.defer(() -> {
434444
Throwable failure = this.startFailure;
435445
if (failure != null) {
@@ -469,7 +479,7 @@ public boolean hasActivePrompt(String sessionId) {
469479
* </p>
470480
* @return one active session ID or null if no prompt is active
471481
*/
472-
public String getActivePromptSessionId() {
482+
public @Nullable String getActivePromptSessionId() {
473483
return activePrompts.keySet().stream().findFirst().orElse(null);
474484
}
475485

@@ -503,6 +513,27 @@ public void close() {
503513
transport.close();
504514
}
505515

516+
/**
517+
* The params a handler receives. JSON-RPC lets a request or notification omit them;
518+
* an omitted params reads as an empty object, which is what a peer sending {@code {}}
519+
* would deliver, so handlers never see null.
520+
*/
521+
private static Object paramsOrEmpty(@Nullable Object params) {
522+
return (params != null) ? params : Map.of();
523+
}
524+
525+
/** JSON-RPC requires an error message; an exception without one is named by its type. */
526+
private static String errorMessage(Throwable error) {
527+
String message = error.getMessage();
528+
return (message != null) ? message : error.getClass().getName();
529+
}
530+
531+
/** A request always gets a response: a handler that completes empty is answered with an error. */
532+
private static <T> Mono<T> requireResult(Mono<T> result, String method) {
533+
return result.switchIfEmpty(Mono.error(() -> new AcpProtocolException(AcpErrorCodes.INTERNAL_ERROR,
534+
"The " + method + " handler produced no response")));
535+
}
536+
506537
/**
507538
* ACP-specific error wrapper for JSON-RPC errors.
508539
* Provides detailed error information including code, message, and data.
@@ -549,7 +580,7 @@ public int getCode() {
549580
return error.code();
550581
}
551582

552-
public Object getData() {
583+
public @Nullable Object getData() {
553584
return error.data();
554585
}
555586

0 commit comments

Comments
 (0)