⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
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

Merged
kabir merged 3 commits into from
Jan 19, 2026
Merged

fix: ListTasks validation and serialization across all transports #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b1ada3
fix: ListTasks parameter validation and serialization
kabir Jan 16, 2026
a9db41b
refactor: Address Gemini code review feedback
kabir Jan 16, 2026
4a21608
Gemini fixes
kabir Jan 16, 2026
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ pom.xml.releaseBackup
pom.xml.versionsBackup
release.properties
.flattened-pom.xml
*.args

# Eclipse
.project
Expand Down
61 changes: 29 additions & 32 deletions ...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 @@ -226,6 +226,25 @@ public ListTasksResult list(ListTasksParams params) {
LOGGER.debug("Listing tasks with params: contextId={}, status={}, pageSize={}, pageToken={}",
params.contextId(), params.status(), params.pageSize(), params.pageToken());

// Parse pageToken once at the beginning
Instant tokenTimestamp = null;
String tokenId = null;
if (params.pageToken() != null && !params.pageToken().isEmpty()) {
String[] tokenParts = params.pageToken().split(":", 2);
if (tokenParts.length == 2) {
try {
long timestampMillis = Long.parseLong(tokenParts[0]);
tokenId = tokenParts[1];
tokenTimestamp = Instant.ofEpochMilli(timestampMillis);
} catch (NumberFormatException e) {
throw new io.a2a.spec.InvalidParamsError(null,
"Invalid pageToken format: timestamp must be numeric milliseconds", null);
}
} else {
throw new io.a2a.spec.InvalidParamsError(null, "Invalid pageToken format: expected 'timestamp:id'", null);
}
}
Comment on lines +232 to +246
Copy link
Contributor

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

Choose a reason for hiding this comment

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

medium

The logic for parsing the pageToken is duplicated in InMemoryTaskStore.java (lines 76-124). This duplication can make future changes to the page token format error-prone and harder to maintain.

To improve maintainability and reduce code duplication, consider extracting this parsing logic into a dedicated helper method or a new class/record, for example, a PageToken record. This would encapsulate the parsing and validation logic in a single place.

Example of a PageToken record:

public record PageToken(Instant timestamp, String id) {
    public static @Nullable PageToken fromString(@Nullable String tokenStr) {
        if (tokenStr == null || tokenStr.isEmpty()) {
            return null;
        }
        String[] tokenParts = tokenStr.split(":", 2);
        if (tokenParts.length != 2) {
            throw new InvalidParamsError(null, "Invalid pageToken format: expected 'timestamp:id'", null);
        }
        try {
            long timestampMillis = Long.parseLong(tokenParts[0]);
            String id = tokenParts[1];
            return new PageToken(Instant.ofEpochMilli(timestampMillis), id);
        } catch (NumberFormatException e) {
            throw new InvalidParamsError(null, "Invalid pageToken format: timestamp must be numeric milliseconds", null);
        }
    }
}

Both JpaDatabaseTaskStore and InMemoryTaskStore could then use PageToken.fromString(params.pageToken()) to parse the token, simplifying the code in both classes and ensuring consistent behavior.

Copy link
Collaborator Author

@kabir kabir Jan 16, 2026

Choose a reason for hiding this comment

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

I opened #597


// Build dynamic JPQL query with WHERE clauses for filtering
StringBuilder queryBuilder = new StringBuilder("SELECT t FROM JpaTask t WHERE 1=1");
StringBuilder countQueryBuilder = new StringBuilder("SELECT COUNT(t) FROM JpaTask t WHERE 1=1");
Expand All @@ -249,18 +268,9 @@ public ListTasksResult list(ListTasksParams params) {
}

// Apply pagination cursor using keyset pagination for composite sort (timestamp DESC, id ASC)
// PageToken format: "timestamp_millis:taskId" (e.g., "1699999999000:task-123")
if (params.pageToken() != null && !params.pageToken().isEmpty()) {
String[] tokenParts = params.pageToken().split(":", 2);
if (tokenParts.length == 2) {
// Keyset pagination: get tasks where timestamp < tokenTimestamp OR (timestamp = tokenTimestamp AND id > tokenId)
// All tasks have timestamps (TaskStatus canonical constructor ensures this)
queryBuilder.append(" AND (t.statusTimestamp < :tokenTimestamp OR (t.statusTimestamp = :tokenTimestamp AND t.id > :tokenId))");
} else {
// Legacy ID-only pageToken format is not supported with timestamp-based sorting
// Throw error to prevent incorrect pagination results
throw new io.a2a.spec.InvalidParamsError(null, "Invalid pageToken format: expected 'timestamp:id'", null);
}
if (tokenTimestamp != null) {
// Keyset pagination: get tasks where timestamp < tokenTimestamp OR (timestamp = tokenTimestamp AND id > tokenId)
queryBuilder.append(" AND (t.statusTimestamp < :tokenTimestamp OR (t.statusTimestamp = :tokenTimestamp AND t.id > :tokenId))");
}

// Sort by status timestamp descending (most recent first), then by ID for stable ordering
Expand All @@ -279,25 +289,9 @@ public ListTasksResult list(ListTasksParams params) {
if (params.lastUpdatedAfter() != null) {
query.setParameter("lastUpdatedAfter", params.lastUpdatedAfter());
}
if (params.pageToken() != null && !params.pageToken().isEmpty()) {
String[] tokenParts = params.pageToken().split(":", 2);
if (tokenParts.length == 2) {
// Parse keyset pagination parameters
try {
long timestampMillis = Long.parseLong(tokenParts[0]);
String tokenId = tokenParts[1];

// All tasks have timestamps (TaskStatus canonical constructor ensures this)
Instant tokenTimestamp = Instant.ofEpochMilli(timestampMillis);
query.setParameter("tokenTimestamp", tokenTimestamp);
query.setParameter("tokenId", tokenId);
} catch (NumberFormatException e) {
// Malformed timestamp in pageToken
throw new io.a2a.spec.InvalidParamsError(null,
"Invalid pageToken format: timestamp must be numeric milliseconds", null);
}
}
// Note: Legacy ID-only format already rejected in query building phase
if (tokenTimestamp != null) {
query.setParameter("tokenTimestamp", tokenTimestamp);
query.setParameter("tokenId", tokenId);
}

// Apply page size limit (+1 to check for next page)
Expand Down Expand Up @@ -362,7 +356,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
33 changes: 21 additions & 12 deletions 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 @@ -90,18 +90,24 @@ public ListTasksResult list(ListTasksParams params) {
int mid = left + (right - left) / 2;
Task task = allFilteredTasks.get(mid);

// All tasks have timestamps (TaskStatus canonical constructor ensures this)
// Truncate to milliseconds for consistency with pageToken precision
java.time.Instant taskTimestamp = task.status().timestamp().toInstant()
.truncatedTo(java.time.temporal.ChronoUnit.MILLIS);
int timestampCompare = taskTimestamp.compareTo(tokenTimestamp);

if (timestampCompare < 0 || (timestampCompare == 0 && task.id().compareTo(tokenId) > 0)) {
// This task is after the token, search left half
right = mid;
} else {
// This task is before or equal to token, search right half
java.time.Instant taskTimestamp = (task.status() != null && task.status().timestamp() != null)
? task.status().timestamp().toInstant().truncatedTo(java.time.temporal.ChronoUnit.MILLIS)
: null;

if (taskTimestamp == null) {
// Task with null timestamp is always "before" a token with a timestamp, as they are sorted last.
// So, we search in the right half.
left = mid + 1;
} else {
int timestampCompare = taskTimestamp.compareTo(tokenTimestamp);

if (timestampCompare < 0 || (timestampCompare == 0 && task.id().compareTo(tokenId) > 0)) {
// This task is after the token, search left half
right = mid;
} else {
// This task is before or equal to token, search right half
left = mid + 1;
}
}
}
startIndex = left;
Expand Down Expand Up @@ -144,7 +150,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
46 changes: 40 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,23 @@ default Map<String, Object> metadataFromProto(Struct struct) {
*/
@Named("zeroToNull")
default Integer zeroToNull(int value) {
return value > 0 ? value : null;
return value != 0 ? value : null;
}

/**
* Converts protobuf int to Integer, preserving all values including 0.
* <p>
* Unlike zeroToNull, this method preserves 0 values, allowing compact constructor
* validation to catch invalid values (e.g., pageSize=0 must fail validation).
* For truly optional fields where 0 means "unset", use zeroToNull instead.
* Use this with {@code @Mapping(qualifiedByName = "intToIntegerOrNull")}.
*
* @param value the protobuf int value
* @return Integer (never null for primitive int input)
*/
@Named("intToIntegerOrNull")
default Integer intToIntegerOrNull(int value) {
return value;
}

/**
Expand Down Expand Up @@ -337,35 +353,53 @@ 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,
"Timestamp must be a non-negative number of milliseconds since epoch, but 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) {
String validStates = java.util.Arrays.stream(io.a2a.spec.TaskState.values())
.filter(s -> s != io.a2a.spec.TaskState.UNKNOWN)
.map(Enum::name)
.collect(java.util.stream.Collectors.joining(", "));
throw new InvalidParamsError(null,
"Invalid task state value. Must be one of: " + validStates,
null);
}
io.a2a.spec.TaskState result = TaskStateMapper.INSTANCE.fromProto(state);
return result == io.a2a.spec.TaskState.UNKNOWN ? null : result;
return result;
}
}
3 changes: 2 additions & 1 deletion spec-grpc/src/main/java/io/a2a/grpc/mapper/ListTasksParamsMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ public interface ListTasksParamsMapper {
*/
@Mapping(target = "contextId", source = "contextId", qualifiedByName = "emptyToNull")
@Mapping(target = "status", source = "status", qualifiedByName = "taskStateOrNull")
@Mapping(target = "pageSize", source = "pageSize", qualifiedByName = "zeroToNull")
// pageSize: Check if field is set using hasPageSize() to distinguish unset (null) from explicit 0 (validation error)
@Mapping(target = "pageSize", expression = "java(request.hasPageSize() ? request.getPageSize() : null)")
@Mapping(target = "pageToken", source = "pageToken", qualifiedByName = "emptyToNull")
@Mapping(target = "historyLength", source = "historyLength", qualifiedByName = "zeroToNull")
Copy link
Collaborator

@ehsavoie ehsavoie Jan 19, 2026

Choose a reason for hiding this comment

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

Shouldn't the mapping be: request.hasHistoryLength()){ return request.gethistoryLength();} return null;

@Mapping(target = "lastUpdatedAfter", source = "lastUpdatedAfter", qualifiedByName = "millisToInstant")
Expand Down
18 changes: 8 additions & 10 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 @@ -458,14 +458,12 @@ private static JsonProcessingException convertProtoBufExceptionToJsonProcessingE
return new JsonProcessingException(ERROR_MESSAGE.formatted("unknown parsing error"));
}

// Extract field name if present in error message
String prefix = "Cannot find field: ";
if (message.contains(prefix)) {
return new InvalidParamsJsonMappingException(ERROR_MESSAGE.formatted(message.substring(message.indexOf(prefix) + prefix.length())), id);
}
prefix = "Invalid value for";
if (message.contains(prefix)) {
return new InvalidParamsJsonMappingException(ERROR_MESSAGE.formatted(message.substring(message.indexOf(prefix) + prefix.length())), id);
// Extract field name if present in error message - check common prefixes
String[] prefixes = {"Cannot find field: ", "Invalid value for", "Invalid enum value:"};
for (String prefix : prefixes) {
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
Expand Down Expand Up @@ -548,7 +546,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 +569,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
45 changes: 26 additions & 19 deletions 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 @@ -2,7 +2,6 @@

import java.time.Instant;

import io.a2a.util.Assert;
import org.jspecify.annotations.Nullable;

/**
Expand All @@ -27,9 +26,12 @@ public record ListTasksParams(
@Nullable Boolean includeArtifacts,
String tenant
) {
private static final int MIN_PAGE_SIZE = 1;
private static final int MAX_PAGE_SIZE = 100;
private static final int DEFAULT_PAGE_SIZE = 50;
/**
* 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 +41,24 @@ public record ListTasksParams(
* @param lastUpdatedAfter filter by last update timestamp
* @param includeArtifacts whether to include artifacts
* @param tenant the tenant identifier
* @throws InvalidParamsError if tenant is null or if pageSize or historyLength are out of valid range
*/
public ListTasksParams {
Assert.checkNotNullParam("tenant", tenant);
if (tenant == null) {
throw new InvalidParamsError(null, "Parameter 'tenant' may not be null", null);
}

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

// Validate historyLength (>= 0)
if (historyLength != null && historyLength < 0) {
throw new InvalidParamsError(null,
"historyLength must be non-negative, got: " + historyLength, null);
}
}
/**
* Default constructor for listing all tasks.
Expand All @@ -61,33 +78,23 @@ public ListTasksParams(Integer pageSize, String pageToken) {
}

/**
* Validates and returns the effective page size (between 1 and 100, defaults to 50).
* Returns the effective page size (defaults to 50 if not specified).
* Values are validated in the constructor to be within the range [1, 100].
*
* @return the effective page size
*/
public int getEffectivePageSize() {
if (pageSize == null) {
return 50;
}
if (pageSize < 1) {
return 1;
}
if (pageSize > 100) {
return 100;
}
return pageSize;
return pageSize != null ? pageSize : DEFAULT_PAGE_SIZE;
}

/**
* Returns the effective history length (non-negative, defaults to 0).
* Returns the effective history length (defaults to 0 if not specified).
* Values are validated in the constructor to be non-negative.
*
* @return the effective history length
*/
public int getEffectiveHistoryLength() {
if (historyLength == null || historyLength < 0) {
return 0;
}
return historyLength;
return historyLength != null ? historyLength : 0;
}

/**
Expand Down
Loading