Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new validation layer for A2UI JSON payloads, which is a great addition to ensure data integrity beyond basic schema validation. The new validation.py module is well-structured and includes checks for component integrity, topology (cycles, orphans), recursion depth, and path syntax. The accompanying test suite in test_validation.py is comprehensive and covers many important edge cases.
My feedback includes a critical point about silent error handling that could lead to validation being skipped, and a suggestion to improve maintainability by refactoring hardcoded values into constants. Overall, this is a solid contribution that significantly improves the robustness of A2UI message processing.
| except Exception: | ||
| # If schema traversal fails, return empty map | ||
| pass |
There was a problem hiding this comment.
Using a broad except Exception: with a pass is dangerous. It will silently swallow any errors that occur during schema parsing (e.g., due to an unexpected schema structure), resulting in an empty ref_map. This effectively disables all component reference validation without any warning, which could allow invalid JSON to pass through. At a minimum, this should log a warning to indicate that reference validation was skipped. A better approach would be to handle more specific exceptions related to key/index errors during dictionary traversal.
| if global_depth > 50: | ||
| raise ValueError("Global recursion limit exceeded: Depth > 50") | ||
|
|
||
| if isinstance(item, list): | ||
| for x in item: | ||
| traverse(x, global_depth + 1, func_depth) | ||
| return | ||
|
|
||
| if isinstance(item, dict): | ||
| # Check for path | ||
| if PATH in item and isinstance(item[PATH], str): | ||
| path = item[PATH] | ||
| if not re.fullmatch(JSON_POINTER_PATTERN, path): | ||
| raise ValueError(f"Invalid JSON Pointer syntax: '{path}'") | ||
|
|
||
| # Check for FunctionCall | ||
| is_func = CALL in item and ARGS in item | ||
|
|
||
| if is_func: | ||
| if func_depth >= 5: | ||
| raise ValueError(f"Recursion limit exceeded: {FUNCTION_CALL} depth > 5") |
There was a problem hiding this comment.
The recursion depth limits (50 for global, 5 for function calls) are hardcoded as magic numbers. To improve readability and maintainability, it's better to define these as module-level constants (e.g., MAX_GLOBAL_DEPTH = 50, MAX_FUNC_CALL_DEPTH = 5). This makes their purpose clear and allows them to be easily located and modified in one place.
| 1. **JSON Schema Validation**: Ensures payload adheres to the A2UI schema. | ||
| 2. **Component Integrity**: | ||
| - All component IDs are unique. | ||
| - A 'root' component exists. |
There was a problem hiding this comment.
If the updateComponents message only include a partial list of components to update an existing surface, does it also require a root component?
There was a problem hiding this comment.
Should we only enforce the root requirement if there is a CreateSurface message along with the UpdateComponents message?
|
|
||
| a2ui_schema = await self.get_a2ui_schema(tool_context) | ||
| jsonschema.validate(instance=a2ui_json_payload, schema=a2ui_schema) | ||
| validate_a2ui_json(a2ui_json_payload, a2ui_schema) |
There was a problem hiding this comment.
What is the expected format of the a2ui_schema?
v0.9 is a modularized version with multiple schemas. Do we need to bundle them into a unified one?
#557 adds an A2uiValidator that builds referencing registry to work with multiple schemas: https://github.com/nan-yu/A2UI/blob/836b2858833516ec532fdaa5366e0c4f386c72ad/a2a_agents/python/a2ui_agent/src/a2ui/inference/schema/validator.py. I think we can move these additional validations into the A2uiValidator.
The send_a2ui_to_client_toolset.py will extract the A2uiCatalog instance stored in the session and invoke the validator to validate the json payload, for example, f350823#diff-5f858e097e91850cde41d939245f10a3ef616ec3875236afcb4cfa4105f4384aR279.
|
The build was failing because of the format check. |
2 participants
To cover common errors not covered by the json schema