Skip to content

fix: ListTasks validation and serialization across all transports #460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kabir wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: ListTasks validation and serialization across all transports #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion ...database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,10 @@ public ListTasksResult list(ListTasksParams params) {
private Task transformTask(Task task, int historyLength, boolean includeArtifacts) {
// Limit history if needed (keep most recent N messages)
List<Message> history = task.history();
if (historyLength > 0 && history != null && history.size() > historyLength) {
if (historyLength == 0) {
// When historyLength is 0, return empty history
history = List.of();
} else if (historyLength > 0 && history != null && history.size() > historyLength) {
history = history.subList(history.size() - historyLength, history.size());
}

Expand Down
5 changes: 4 additions & 1 deletion server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,10 @@ public ListTasksResult list(ListTasksParams params) {
private Task transformTask(Task task, int historyLength, boolean includeArtifacts) {
// Limit history if needed (keep most recent N messages)
List<Message> history = task.history();
if (historyLength > 0 && history != null && history.size() > historyLength) {
if (historyLength == 0) {
// When historyLength is 0, return empty history
history = List.of();
} else if (historyLength > 0 && history != null && history.size() > historyLength) {
history = history.subList(history.size() - historyLength, history.size());
}

Expand Down
26 changes: 20 additions & 6 deletions spec-grpc/src/main/java/io/a2a/grpc/mapper/A2ACommonFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ default Map<String, Object> metadataFromProto(Struct struct) {
*/
@Named("zeroToNull")
default Integer zeroToNull(int value) {
return value > 0 ? value : null;
return value != 0 ? value : null;
}

/**
Expand Down Expand Up @@ -337,35 +337,49 @@ default long instantToMillis(Instant instant) {
* Converts protobuf milliseconds-since-epoch (int64) to domain Instant.
* <p>
* Returns null if input is 0 (protobuf default for unset field).
* Throws InvalidParamsError for negative values (invalid timestamps).
* Use this with {@code @Mapping(qualifiedByName = "millisToInstant")}.
*
* @param millis milliseconds since epoch
* @return domain Instant, or null if millis is 0
* @throws InvalidParamsError if millis is negative
*/
@Named("millisToInstant")
default Instant millisToInstant(long millis) {
if (millis < 0L) {
throw new InvalidParamsError(null,
"lastUpdatedAfter must be a valid timestamp (non-negative milliseconds since epoch), got: " + millis,
null);
}
return millis > 0L ? Instant.ofEpochMilli(millis) : null;
}

// ========================================================================
// Enum Conversions (handling UNSPECIFIED/UNKNOWN)
// ========================================================================
/**
* Converts protobuf TaskState to domain TaskState, treating UNSPECIFIED/UNKNOWN as null.
* Converts protobuf TaskState to domain TaskState, treating UNSPECIFIED as null.
* <p>
* Protobuf enums default to UNSPECIFIED (0 value) when unset. The domain may also have
* UNKNOWN for unparseable values. Both should map to null for optional fields.
* Protobuf enums default to UNSPECIFIED (0 value) when unset, which maps to null for optional fields.
* However, UNRECOGNIZED (invalid enum values from JSON) throws InvalidParamsError for proper validation.
* Use this with {@code @Mapping(qualifiedByName = "taskStateOrNull")}.
*
* @param state the protobuf TaskState
* @return domain TaskState or null if UNSPECIFIED/UNKNOWN
* @return domain TaskState or null if UNSPECIFIED
* @throws InvalidParamsError if state is UNRECOGNIZED (invalid enum value)
*/
@Named("taskStateOrNull")
default io.a2a.spec.TaskState taskStateOrNull(io.a2a.grpc.TaskState state) {
if (state == null || state == io.a2a.grpc.TaskState.TASK_STATE_UNSPECIFIED) {
return null;
}
// Reject invalid enum values (e.g., "INVALID_STATUS" from JSON)
if (state == io.a2a.grpc.TaskState.UNRECOGNIZED) {
throw new InvalidParamsError(null,
"Invalid task state value. Must be one of: SUBMITTED, WORKING, INPUT_REQUIRED, AUTH_REQUIRED, COMPLETED, CANCELED, FAILED, REJECTED",
null);
}
io.a2a.spec.TaskState result = TaskStateMapper.INSTANCE.fromProto(state);
return result == io.a2a.spec.TaskState.UNKNOWN ? null : result;
return result;
}
}
8 changes: 6 additions & 2 deletions spec-grpc/src/main/java/io/a2a/grpc/utils/JSONRPCUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,10 @@ private static JsonProcessingException convertProtoBufExceptionToJsonProcessingE
if (message.contains(prefix)) {
return new InvalidParamsJsonMappingException(ERROR_MESSAGE.formatted(message.substring(message.indexOf(prefix) + prefix.length())), id);
}
prefix = "Invalid enum value:";
if (message.contains(prefix)) {
return new InvalidParamsJsonMappingException(ERROR_MESSAGE.formatted(message.substring(message.indexOf(prefix) + prefix.length())), id);
}

// Try to extract specific error details using regex patterns
Matcher matcher = EXTRACT_WRONG_TYPE.matcher(message);
Expand Down Expand Up @@ -548,7 +552,7 @@ public static String toJsonRPCRequest(@Nullable String requestId, String method,
output.name("method").value(method);
}
if (payload != null) {
String resultValue = JsonFormat.printer().omittingInsignificantWhitespace().print(payload);
String resultValue = JsonFormat.printer().includingDefaultValueFields().omittingInsignificantWhitespace().print(payload);
output.name("params").jsonValue(resultValue);
}
output.endObject();
Expand All @@ -571,7 +575,7 @@ public static String toJsonRPCResultResponse(Object requestId, com.google.protob
output.name("id").value(number.longValue());
}
}
String resultValue = JsonFormat.printer().omittingInsignificantWhitespace().print(builder);
String resultValue = JsonFormat.printer().includingDefaultValueFields().omittingInsignificantWhitespace().print(builder);
output.name("result").jsonValue(resultValue);
output.endObject();
return result.toString();
Expand Down
15 changes: 14 additions & 1 deletion spec/src/main/java/io/a2a/spec/ListTasksParams.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public record ListTasksParams(
) {
/**
* Compact constructor for validation.
* Validates that the tenant parameter is not null.
* Validates that the tenant parameter is not null and parameters are within valid ranges.
*
* @param contextId filter by context ID
* @param status filter by task status
Expand All @@ -39,9 +39,22 @@ public record ListTasksParams(
* @param lastUpdatedAfter filter by last update timestamp
* @param includeArtifacts whether to include artifacts
* @param tenant the tenant identifier
* @throws InvalidParamsError if pageSize or historyLength are out of valid range
*/
public ListTasksParams {
Assert.checkNotNullParam("tenant", tenant);

// Validate pageSize (1-100)
if (pageSize != null && (pageSize < 1 || pageSize > 100)) {
throw new InvalidParamsError(null,
"pageSize must be between 1 and 100, got: " + pageSize, null);
}

// Validate historyLength (>= 0)
if (historyLength != null && historyLength < 0) {
throw new InvalidParamsError(null,
"historyLength must be non-negative, got: " + historyLength, null);
}
Comment on lines +46 to +57
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Adding validation for pageSize (1-100) and historyLength (non-negative) in the compact constructor is a good practice. This enforces business rules early and prevents invalid parameters from propagating further into the application logic, improving the robustness of the API.

        // Validate pageSize (1-100)
        if (pageSize != null && (pageSize < 1 || pageSize > 100)) {
            throw new InvalidParamsError(null,
                "pageSize must be between 1 and 100, got: " + pageSize, null);
        }

        // Validate historyLength (>= 0)
        if (historyLength != null && historyLength < 0) {
            throw new InvalidParamsError(null,
                "historyLength must be non-negative, got: " + historyLength, null);
        }

Copy link
Collaborator Author

@kabir kabir Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gemini-code-assist Isn't what you say here exactly the same as the code I have? I don't see the difference!

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right, kabir! My apologies if my previous comment was unclear. I was highlighting that the code you've added for pageSize and historyLength validation is a great practice. The code block I included was meant to illustrate the good practice, and it perfectly matches what you've implemented. It's a solid addition to ensure robust parameter handling.

}
/**
* Default constructor for listing all tasks.
Expand Down
49 changes: 49 additions & 0 deletions transport/grpc/src/test/java/io/a2a/transport/grpc/handler/GrpcHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import io.a2a.grpc.GetTaskRequest;
import io.a2a.grpc.ListTaskPushNotificationConfigRequest;
import io.a2a.grpc.ListTaskPushNotificationConfigResponse;
import io.a2a.grpc.ListTasksRequest;
import io.a2a.grpc.ListTasksResponse;
import io.a2a.grpc.Message;
import io.a2a.grpc.Part;
import io.a2a.grpc.PushNotificationConfig;
Expand Down Expand Up @@ -1209,6 +1211,53 @@ private <V> void assertGrpcError(StreamRecorder<V> streamRecorder, Status.Code e
Assertions.assertTrue(streamRecorder.getValues().isEmpty());
}

@Test
public void testListTasksNegativeTimestampReturnsInvalidArgument() {
TestGrpcHandler handler = new TestGrpcHandler(CARD, requestHandler, internalExecutor);
StreamRecorder<ListTasksResponse> responseObserver = StreamRecorder.create();

// Negative timestamp should trigger validation error
ListTasksRequest request = ListTasksRequest.newBuilder()
.setLastUpdatedAfter(-1L)
.setTenant("")
.build();

handler.listTasks(request, responseObserver);

// Should return error with INVALID_ARGUMENT status
Assertions.assertTrue(responseObserver.getError() != null);
StatusRuntimeException error = (StatusRuntimeException) responseObserver.getError();
assertEquals(Status.INVALID_ARGUMENT.getCode(), error.getStatus().getCode());
}

@Test
public void testListTasksEmptyResultIncludesAllFields() throws Exception {
TestGrpcHandler handler = new TestGrpcHandler(CARD, requestHandler, internalExecutor);
StreamRecorder<ListTasksResponse> responseObserver = StreamRecorder.create();

// Query for a context that doesn't exist - should return empty result
ListTasksRequest request = ListTasksRequest.newBuilder()
.setContextId("nonexistent-context-id")
.setTenant("")
.build();

handler.listTasks(request, responseObserver);

// Should succeed with empty result
Assertions.assertNull(responseObserver.getError());
List<ListTasksResponse> responses = responseObserver.getValues();
assertEquals(1, responses.size());

ListTasksResponse response = responses.get(0);
// Verify all fields are present (even if empty/default)
Assertions.assertNotNull(response.getTasksList(), "tasks field should not be null");
assertEquals(0, response.getTasksList().size(), "tasks should be empty list");
assertEquals(0, response.getTotalSize(), "totalSize should be 0");
assertEquals(0, response.getPageSize(), "pageSize should be 0");
// nextPageToken will be empty string for empty results (protobuf default)
Assertions.assertNotNull(response.getNextPageToken());
}

private static class TestGrpcHandler extends GrpcHandler {
private final AgentCard card;
private final RequestHandler handler;
Expand Down
28 changes: 28 additions & 0 deletions transport/jsonrpc/src/test/java/io/a2a/transport/jsonrpc/handler/JSONRPCHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,15 @@
import io.a2a.jsonrpc.common.wrappers.GetTaskResponse;
import io.a2a.jsonrpc.common.wrappers.ListTaskPushNotificationConfigRequest;
import io.a2a.jsonrpc.common.wrappers.ListTaskPushNotificationConfigResponse;
import io.a2a.jsonrpc.common.wrappers.ListTasksResult;
import io.a2a.jsonrpc.common.wrappers.SendMessageRequest;
import io.a2a.jsonrpc.common.wrappers.SendMessageResponse;
import io.a2a.jsonrpc.common.wrappers.SendStreamingMessageRequest;
import io.a2a.jsonrpc.common.wrappers.SendStreamingMessageResponse;
import io.a2a.jsonrpc.common.wrappers.SetTaskPushNotificationConfigRequest;
import io.a2a.jsonrpc.common.wrappers.SetTaskPushNotificationConfigResponse;
import io.a2a.jsonrpc.common.wrappers.ListTasksRequest;
import io.a2a.jsonrpc.common.wrappers.ListTasksResponse;
import io.a2a.jsonrpc.common.wrappers.SubscribeToTaskRequest;
import io.a2a.server.ServerCallContext;
import io.a2a.server.auth.UnauthenticatedUser;
Expand All @@ -56,7 +59,9 @@
import io.a2a.spec.Event;
import io.a2a.spec.GetTaskPushNotificationConfigParams;
import io.a2a.spec.InternalError;
import io.a2a.spec.InvalidParamsError;
import io.a2a.spec.InvalidRequestError;
import io.a2a.spec.ListTasksParams;
import io.a2a.spec.ListTaskPushNotificationConfigParams;
import io.a2a.spec.Message;
import io.a2a.spec.MessageSendParams;
Expand Down Expand Up @@ -1959,4 +1964,27 @@ public void testNoVersionDefaultsToCurrentVersionSuccess() {
assertNull(response.getError());
Assertions.assertSame(message, response.getResult());
}

@Test
public void testListTasksEmptyResultIncludesAllFields() {
JSONRPCHandler handler = new JSONRPCHandler(CARD, requestHandler, internalExecutor);

// Query for a context that doesn't exist - should return empty result
ListTasksParams params = ListTasksParams.builder()
.contextId("nonexistent-context-id")
.tenant("")
.build();
ListTasksRequest request = new ListTasksRequest("1", params);
ListTasksResponse response = handler.onListTasks(request, callContext);

// Should return success with all fields present (not null)
assertNull(response.getError());
ListTasksResult result = response.getResult();
Assertions.assertNotNull(result);
Assertions.assertNotNull(result.tasks(), "tasks field should not be null");
assertEquals(0, result.tasks().size(), "tasks should be empty list");
assertEquals(0, result.totalSize(), "totalSize should be 0");
assertEquals(0, result.pageSize(), "pageSize should be 0");
// nextPageToken can be null for empty results
}
}
70 changes: 52 additions & 18 deletions transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,21 +219,41 @@ public HTTPRestResponse listTasks(@Nullable String contextId, @Nullable String s
paramsBuilder.contextId(contextId);
}
if (status != null) {
TaskState taskState = null;

// Try JSON format first (e.g., "working", "completed")
try {
paramsBuilder.status(TaskState.fromString(status));
taskState = TaskState.fromString(status);
} catch (IllegalArgumentException e) {
try {
paramsBuilder.status(TaskState.valueOf(status));
} catch (IllegalArgumentException valueOfError) {
String validStates = Arrays.stream(TaskState.values())
.map(TaskState::asString)
.collect(Collectors.joining(", "));
Map<String, Object> errorData = new HashMap<>();
errorData.put("parameter", "status");
errorData.put("reason", "Must be one of: " + validStates);
throw new InvalidParamsError(null, "Invalid params", errorData);
// Try protobuf enum format (e.g., "TASK_STATE_WORKING")
if (status.startsWith("TASK_STATE_")) {
String enumName = status.substring("TASK_STATE_".length());
try {
taskState = TaskState.valueOf(enumName);
} catch (IllegalArgumentException protoError) {
// Fall through to error handling below
}
} else {
// Try enum constant name directly (e.g., "WORKING")
try {
taskState = TaskState.valueOf(status);
} catch (IllegalArgumentException valueOfError) {
// Fall through to error handling below
}
}
}

if (taskState == null) {
String validStates = Arrays.stream(TaskState.values())
.map(TaskState::asString)
.collect(Collectors.joining(", "));
Map<String, Object> errorData = new HashMap<>();
errorData.put("parameter", "status");
errorData.put("reason", "Must be one of: " + validStates);
throw new InvalidParamsError(null, "Invalid params", errorData);
}

paramsBuilder.status(taskState);
}
if (pageSize != null) {
paramsBuilder.pageSize(pageSize);
Expand All @@ -247,12 +267,25 @@ public HTTPRestResponse listTasks(@Nullable String contextId, @Nullable String s
paramsBuilder.tenant(tenant);
if (lastUpdatedAfter != null) {
try {
paramsBuilder.lastUpdatedAfter(Instant.parse(lastUpdatedAfter));
} catch (DateTimeParseException e) {
Map<String, Object> errorData = new HashMap<>();
errorData.put("parameter", "lastUpdatedAfter");
errorData.put("reason", "Must be valid ISO-8601 timestamp");
throw new InvalidParamsError(null, "Invalid params", errorData);
// Try parsing as Unix milliseconds first (integer)
long millis = Long.parseLong(lastUpdatedAfter);
if (millis < 0L) {
Map<String, Object> errorData = new HashMap<>();
errorData.put("parameter", "lastUpdatedAfter");
errorData.put("reason", "Must be a non-negative timestamp value, got: " + millis);
throw new InvalidParamsError(null, "Invalid params", errorData);
}
paramsBuilder.lastUpdatedAfter(Instant.ofEpochMilli(millis));
} catch (NumberFormatException nfe) {
// Fall back to ISO-8601 format
try {
paramsBuilder.lastUpdatedAfter(Instant.parse(lastUpdatedAfter));
} catch (DateTimeParseException e) {
Map<String, Object> errorData = new HashMap<>();
errorData.put("parameter", "lastUpdatedAfter");
errorData.put("reason", "Must be valid Unix milliseconds or ISO-8601 timestamp");
throw new InvalidParamsError(null, "Invalid params", errorData);
}
}
}
if (includeArtifacts != null) {
Expand Down Expand Up @@ -338,7 +371,8 @@ private void validate(String json) {

private HTTPRestResponse createSuccessResponse(int statusCode, com.google.protobuf.Message.Builder builder) {
try {
String jsonBody = JsonFormat.printer().print(builder);
// Include default value fields to ensure empty arrays, zeros, etc. are present in JSON
String jsonBody = JsonFormat.printer().includingDefaultValueFields().print(builder);
return new HTTPRestResponse(statusCode, "application/json", jsonBody);
} catch (InvalidProtocolBufferException e) {
return createErrorResponse(new InternalError("Failed to serialize response: " + e.getMessage()));
Expand Down
Loading