feat(plugins): add ReflectAndRetryToolPlugin and ReflectAndRetryModelPlugin for self-healing error recovery - #631
Conversation
AmaadMartin
left a comment
There was a problem hiding this comment.
CI is green on ubuntu, macOS, and Windows, and the plugin logic and tests look sound. I am holding for two reasons. First, the PR bundles an unrelated agent_loader behavior change and integration-test edits; please split them out. Second, it uses avoidable type-escape casts. The rest are optional nits.
| fileOrDir.name === 'node_modules' || | ||
| fileOrDir.name.startsWith('.') |
There was a problem hiding this comment.
Not a nit. Split the unrelated changes into a separate PR.
This PR adds the reflect-and-retry plugins. These changes are not part of that feature:
agent_loader.tsskipsnode_modulesand dot directories.agent_dirname_test.tsandapp_loader_test.tschange timeouts and addpreloadAgents().
The loader change alters directory discovery for every user. A reviewer must judge it on its own. Please move it to its own PR.
There was a problem hiding this comment.
Done. I have split out the agent_loader changes and integration test edits into a separate PR: #674
| reflection_guidance: reflectionMessage, | ||
| }; | ||
|
|
||
| return response as unknown as Record<string, unknown>; |
There was a problem hiding this comment.
Not a nit. Remove the as unknown as double cast.
return response as unknown as Record<string, unknown>;The double cast turns off type checking. It appears here and at line 420. Add an index signature to ToolFailureResponse so it is a Record<string, unknown>:
export interface ToolFailureResponse {
// ...existing fields
[key: string]: unknown;
}Then return response with no cast. I did not run tsc on this.
There was a problem hiding this comment.
Done. Added [key: string]: unknown; to ToolFailureResponse and removed the as unknown as double casts. Returning response directly now.
| async extractErrorFromResult({ | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| tool, | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| toolArgs, | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| toolContext, | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| result, |
There was a problem hiding this comment.
Nit. Match the base_plugin stub pattern.
base_plugin.ts marks an unused-param stub with one eslint-disable on a single params object. This override destructures four params and repeats the disable four times. Take one params object and use one disable, like the base class.
There was a problem hiding this comment.
Done. Updated extractErrorFromResult to take a single params object with one disable comment matching base_plugin.ts.
| response_type: string; | ||
| error_type: string; | ||
| error_details: string; | ||
| retry_count: number; | ||
| reflection_guidance: string; |
There was a problem hiding this comment.
Nit, optional. These public fields use snake_case:
response_type: string;
// ...
reflection_guidance: string;The codebase uses camelCase for TypeScript interfaces. If this is the model-facing payload shape, keep it and add a comment. Otherwise rename the fields.
There was a problem hiding this comment.
Kept snake_case for model reflection payload parity with Python ADK, and added JSDoc documentation explaining the schema.
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| export * from './_reflect_retry_utils.js'; |
There was a problem hiding this comment.
Nit. This barrel file is unused. Delete it.
core/package.json exports only ., and index.ts re-exports common.ts, which imports each plugin file directly. No ./plugins subpath exists. The agents, tools, and models directories have no index.ts. This file adds an inconsistent, unreferenced entry point.
There was a problem hiding this comment.
Done. Deleted core/src/plugins/index.ts.
| * @param trackingScope - Optional tracking scope if positional arguments are used. | ||
| */ | ||
| constructor( | ||
| optionsOrName?: ReflectAndRetryToolPluginOptions | string, |
There was a problem hiding this comment.
Nit. Drop the positional-args constructor.
optionsOrName?: ReflectAndRetryToolPluginOptions | string,
maxRetries?: number,
// ...The options object already covers every field. The positional form adds a second API and extra branching with no caller that needs it. The same applies in reflect_retry_model_plugin.ts:74. Keep the options object only.
There was a problem hiding this comment.
Done. Removed the positional constructor arguments and kept only the options object form for both plugins.
…Plugin for self-healing error recovery
99114e6 to
5ee24d2
Compare
|
Thank you for the review @AmaadMartin. I have addressed all the feedback. Could you please take another look? |
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-review at 5ee24d2. All prior findings are resolved.
- Both "Not a nit" items are fixed: the unrelated agent_loader changes are gone, and the
as unknown asdouble casts in the tool plugin are replaced with an index signature onToolFailureResponseand a plain return. - The nits are addressed: the barrel
index.tsis removed, both constructors take an options object only, the stub uses oneparamsand one disable, and the snake_case payload fields carry a parity comment.
New public types are exported from common.ts. The remaining as unknown as casts are in test mocks, and instanceof is only instanceof Error. CI run-tests passes on ubuntu, macOS, and Windows at this head. The PR text contains no injection.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Description of Change:
Problem:
When an AI agent calls a tool with invalid or hallucinated parameters, or when a tool encounters a temporary network or runtime error, the agent throws an error and terminates the turn immediately. There was no built-in reflection mechanism to give the model a chance to analyze the error, adjust its arguments, and retry the operation safely.
Solution:
Implemented
ReflectAndRetryToolPluginand companionReflectAndRetryModelPluginin@google/adk:Testing Plan
Unit Tests:
Summary of test results (npm run test:unit):
core/test/plugins/reflect_retry_tool_plugin_test.tscore/test/plugins/reflect_retry_model_plugin_test.tscore/test/plugins/reflect_retry_integration_test.tsnpm run lintpassed with 0 errors and 0 warningsnpm run format:checkpassed with zero issuesnpm run docs:checkpassed with 0 TypeDoc warningsManual End-to-End (E2E) Tests:
Checklist
Additional context
This feature provides full parity with the Python ADK implementation and improves agent reliability against parameter hallucinations and transient tool errors without requiring external dependencies or databases.