fix: ListTasks validation and serialization across all transports

[kabir]

These fixes are needed for the TCK tests in a2aproject/a2a-tck#93

• Fix pageSize=0 validation using hasPageSize() instead of zeroToNull
• Add includingDefaultValueFields() for complete JSON responses
• Validate enum UNRECOGNIZED, negative timestamps across transports
• Support multiple timestamp/status formats in REST
• Ensure consistent InvalidParamsError behavior across JSON-RPC, gRPC, REST

[gemini-code-assist]

Summary of Changes

Hello @kabir, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses necessary changes to enable the successful execution of new TCK tests for the tasks/list endpoint. The core of these changes involves a significant overhaul of the task listing and pagination mechanism, moving from a simple ID-based cursor to a more robust keyset pagination approach utilizing task timestamps. Additionally, a new lastUpdatedAfter filter has been introduced to enhance query capabilities. These updates ensure compliance with the latest specification requirements, including a temporary protobuf schema divergence to align with an anticipated upstream change.

Highlights

• Enhanced Task Listing Pagination: Implemented keyset pagination for ListTasks endpoints, sorting tasks by statusTimestamp (descending) and then id (ascending) for consistent and efficient retrieval. This replaces the previous ID-based cursor pagination.
• New lastUpdatedAfter Filter: Added a new parameter to the ListTasks API to filter tasks based on their statusTimestamp, allowing retrieval of tasks updated after a specific point in time. This includes validation to prevent future timestamps.
• Page Token Format Update: The pageToken now uses a composite format of 'timestamp_millis:taskId' to support keyset pagination, with robust validation for correct format and error handling for malformed tokens.
• Denormalized statusTimestamp: Introduced a statusTimestamp field in JpaTask to denormalize the task's last update timestamp, enabling efficient database indexing and querying for the new sorting and filtering mechanisms.
• Improved History Limiting Logic: Refined the historyLength parameter logic to explicitly handle historyLength=0 by returning an empty history list, ensuring clearer behavior.
• Protobuf Schema Alignment: Updated the ListTasksResponse protobuf definition to include a page_size field and reordered total_size to align with an upcoming upstream PR (#1160). A PROTO_DIVERGENCE.md file has been added to document this temporary divergence.
• Comprehensive Test Coverage: Added new unit tests to JpaDatabaseTaskStoreTest to validate the new keyset pagination logic, including scenarios with different timestamps and invalid page token formats, ensuring robustness.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces significant changes to the task listing functionality to align with new TCK tests. The key changes include implementing keyset pagination based on a composite sort key (timestamp and ID), adding a lastUpdatedAfter filter, and ensuring consistent behavior across both JPA and in-memory task stores. The protobuf specification has also been updated to include pageSize in the ListTasksResponse.

The changes are well-tested, with new unit tests covering the new pagination and filtering logic. My review focuses on improving code maintainability by reducing duplication, fixing a potential null pointer exception in the in-memory implementation, and removing temporary debug logging.

[fjuma]

This one now has some conflicts.

[kabir]

/gemini review

[gemini-code-assist]

Code Review

The pull request introduces several important fixes and improvements related to validation and serialization for ListTasks functionality across gRPC, JSON-RPC, and REST transports. Key changes include correctly handling historyLength = 0 to return an empty history list, enhanced parameter validation for pageSize, historyLength, lastUpdatedAfter (negative timestamps), and TaskState (unrecognized enum values). The REST API now offers greater flexibility by accepting multiple formats for status and lastUpdatedAfter. Crucially, includingDefaultValueFields() is now used in JSON-RPC and REST responses to ensure that fields with default values are always present in the JSON output, which is vital for client-side parsing. Comprehensive unit tests have been added to cover these new behaviors, significantly improving the robustness, usability, and correctness of the ListTasks API across all transports.

[kabir]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several important fixes and improvements related to validation and serialization for ListTasks across gRPC, JSON-RPC, and REST transports. The changes enhance API consistency and robustness by adding stricter validation for parameters like pageSize, historyLength, and timestamps, while also providing more flexibility in the REST transport for status and timestamp formats. The JSON serialization has been improved to include default value fields, ensuring more complete responses. The changes are well-supported by new tests. My review focuses on opportunities to improve maintainability by reducing code duplication and avoiding hardcoded values, and also highlights the importance of consistent application-level error handling.

[kabir]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several important fixes for validation and serialization in ListTasks across different transports, enhancing consistency and robustness. Key improvements include centralized validation for ListTasksParams, consistent error handling for invalid enum values and timestamps, refactoring of pageToken parsing, more flexible parsing in the REST handler, and ensuring complete JSON responses.

I've identified a critical bug that could lead to a NullPointerException and a medium-severity issue regarding redundant code that could be simplified. Please see my detailed comments below.

[kabir]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a series of valuable fixes and improvements focused on enhancing validation and serialization consistency for the ListTasks operation across gRPC, JSON-RPC, and REST transports. Key changes include robust validation for pageSize, historyLength, and timestamps, as well as more flexible parsing for status and timestamp formats in the REST API. The introduction of includingDefaultValueFields() ensures complete and consistent JSON responses, which is crucial for client compatibility and testing. The refactoring of validation logic into the ListTasksParams domain object is a commendable design choice that improves code clarity and maintainability. The added tests are thorough and effectively verify the new behaviors.

I have one suggestion regarding code duplication in page token parsing logic that could further improve maintainability.

[gemini-code-assist]

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.

[kabir]

I opened #597