-
Notifications
You must be signed in to change notification settings - Fork 417
fix(core): retry invalid structured model responses #1433
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,9 +17,13 @@ | |
| package com.google.adk.tools; | ||
|
|
||
| import com.google.adk.SchemaUtils; | ||
| import com.google.common.collect.ImmutableMap; | ||
| import com.google.genai.types.FunctionDeclaration; | ||
| import com.google.genai.types.Schema; | ||
| import io.reactivex.rxjava3.core.Single; | ||
| import java.util.ArrayList; | ||
| import java.util.LinkedHashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
|
|
||
|
|
@@ -33,6 +37,12 @@ | |
| public class SetModelResponseTool extends BaseTool { | ||
| public static final String NAME = "set_model_response"; | ||
|
|
||
| // Prefix of the SchemaUtils validation message after which the full schema is appended. Used to | ||
| // strip the schema dump from feedback on a best-effort basis; if SchemaUtils changes its wording | ||
| // the feedback simply stays unstripped. runAsync_unknownArg_feedbackOmitsSchemaDump pins the | ||
| // current format. | ||
| private static final String OUTPUT_SCHEMA_DUMP_MARKER = " does not match agent output schema: "; | ||
|
|
||
| private final Schema outputSchema; | ||
|
|
||
| public SetModelResponseTool(Schema outputSchema) { | ||
|
|
@@ -56,12 +66,63 @@ public Optional<FunctionDeclaration> declaration() { | |
|
|
||
| @Override | ||
| public Single<Map<String, Object>> runAsync(Map<String, Object> args, ToolContext toolContext) { | ||
| // This tool is a marker for the final response, it doesn't do anything but return its arguments | ||
| // which will be captured as the final result. | ||
| // Record validated responses on the event actions; return validation feedback so the model can | ||
| // retry. | ||
| return Single.fromCallable( | ||
| () -> { | ||
| SchemaUtils.validateMapOnSchema(args, outputSchema, /* isInput= */ false); | ||
| return args; | ||
| try { | ||
| SchemaUtils.validateMapOnSchema(args, outputSchema, /* isInput= */ false); | ||
| } catch (IllegalArgumentException e) { | ||
| return ImmutableMap.of( | ||
| "error", | ||
| "Validation Error found:\n" | ||
| + sanitizeValidationMessage(e.getMessage()) | ||
| + "\nRecall the set_model_response function correctly, fix the errors, and" | ||
| + " call it again with all required fields using the correct types."); | ||
| } | ||
| // Match Python's model_dump(exclude_none=True) for Java's map-shaped response. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks — this is what I meant, and using the same map for the return value and the actions is right. One thing the loop does not cover: pydantic's Narrow in practice — it needs a nested nullable property that the model actually sends as null, since
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, you’re right — my first pass missed nested nulls. Sorry about that. Just pushed a recursive fix + tests for nested objects and objects inside arrays. |
||
| Map<String, Object> validatedResponse = excludeNullFields(args); | ||
| toolContext.actions().setSetModelResponse(validatedResponse); | ||
| return validatedResponse; | ||
| }); | ||
| } | ||
|
|
||
| private static Map<String, Object> excludeNullFields(Map<String, Object> values) { | ||
| Map<String, Object> result = new LinkedHashMap<>(); | ||
| for (Map.Entry<String, Object> entry : values.entrySet()) { | ||
| Object value = entry.getValue(); | ||
| if (value != null) { | ||
| result.put(entry.getKey(), excludeNullFields(value)); | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private static Object excludeNullFields(Object value) { | ||
| if (value instanceof Map<?, ?>) { | ||
| return excludeNullFields((Map<String, Object>) value); | ||
| } | ||
| if (value instanceof List<?>) { | ||
| List<Object> result = new ArrayList<>(); | ||
| for (Object item : (List<?>) value) { | ||
| result.add(excludeNullFields(item)); | ||
| } | ||
| return result; | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| private static String sanitizeValidationMessage(String message) { | ||
| if (message == null) { | ||
| return "Arguments do not match the output schema."; | ||
| } | ||
| // The model already knows the schema from the tool declaration, so the appended schema dump is | ||
| // redundant in feedback. | ||
| int schemaDumpIndex = message.indexOf(OUTPUT_SCHEMA_DUMP_MARKER); | ||
| if (schemaDumpIndex >= 0) { | ||
| message = message.substring(0, schemaDumpIndex) + " does not match agent output schema."; | ||
| } | ||
| return message; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches
_output_schema_processor.pyexactly — reading the recorded value rather than the raw function response is what stops feedback being promoted, so no change asked for here.Worth a test though: an event whose
set_model_responsenever succeeded should yieldOptional.empty()fromgetStructuredModelResponse, so the "feedback is never the final output" property is pinned rather than implied.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion.
getStructuredModelResponse_withValidationFeedback_returnsEmptycovers this case by constructing validation feedback without a recordedsetModelResponseand verifying thatgetStructuredModelResponsereturnsOptional.empty().