Repository navigation
Conversation
AmaadMartin
left a comment
There was a problem hiding this comment.
Verified the port against the Python reference (src/google/adk/plugins/context_filter_plugin.py) at the head SHA; the windowing and orphan-pair logic match. Two items block: the helpers are exported as public API but are private in the reference, and isContextFilterPlugin uses instanceof where this repo uses brand-symbol guards. CI note: run-tests (windows-latest) failed while macOS and the aggregate run-tests passed, which matches the known windows flake in files this PR does not touch.
| export function isContextFilterPlugin( | ||
| plugin: unknown, | ||
| ): plugin is ContextFilterPlugin { | ||
| return plugin instanceof ContextFilterPlugin; |
There was a problem hiding this comment.
Not a nit. isContextFilterPlugin uses instanceof. This check fails when two copies of the package share one runtime. This repo uses brand-symbol guards instead (see isBaseLlm in core/src/models/base_llm.ts:24).
This guard also has no caller in the repo and no counterpart in the Python reference. Please remove it. If a caller needs it later, add a brand symbol like isBaseLlm.
There was a problem hiding this comment.
Done. Completely removed isContextFilterPlugin (along with its exports and unit tests) since it has no callers in the codebase and no counterpart in the Python reference.
| export { | ||
| ContextFilterPlugin, | ||
| adjustSplitIndexToAvoidOrphanedFunctionResponses, | ||
| getInvocationStartIndices, | ||
| isContextFilterPlugin, | ||
| isFunctionResponseContent, | ||
| isHumanUserContent, | ||
| type ContextFilterPluginOptions, | ||
| type CustomFilterFunction, | ||
| } from './plugins/context_filter_plugin.js'; |
There was a problem hiding this comment.
Not a nit. Only ContextFilterPlugin and the two types belong in the public API. The four helpers and isContextFilterPlugin are private in the Python reference (_-prefixed).
Please drop these five names from common.ts:
adjustSplitIndexToAvoidOrphanedFunctionResponses,
getInvocationStartIndices,
isContextFilterPlugin,
isFunctionResponseContent,
isHumanUserContent,
The test imports them from the module path directly, so this does not break the test.
There was a problem hiding this comment.
Done. Dropped all five names from common.ts. Only ContextFilterPlugin, type ContextFilterPluginOptions, and type CustomFilterFunction remain exported in the public API.
| constructor( | ||
| options?: ContextFilterPluginOptions | number, | ||
| customFilter?: CustomFilterFunction, | ||
| name = 'context_filter_plugin', | ||
| removeAmount = 1, | ||
| ) { | ||
| if (typeof options === 'object' && options !== null) { | ||
| super(options.name ?? 'context_filter_plugin'); | ||
| this.numInvocationsToKeep = options.numInvocationsToKeep; | ||
| this.customFilter = options.customFilter; | ||
| this.removeAmount = options.removeAmount ?? 1; | ||
| } else { | ||
| super(name); | ||
| this.numInvocationsToKeep = options; | ||
| this.customFilter = customFilter; | ||
| this.removeAmount = removeAmount; | ||
| } | ||
|
|
There was a problem hiding this comment.
Not a nit. The constructor accepts an options object or four positional arguments. The Python reference has only keyword arguments, which map to the options object. The positional form has no caller except the test.
Please keep the options object only and drop the positional overload and its branch. This also removes the runtime typeof check.
There was a problem hiding this comment.
Done. Dropped the positional constructor overload and runtime typeof check. The constructor now takes only options: ContextFilterPluginOptions = {}.
| } | ||
|
|
||
| if (this.removeAmount < 1) { | ||
| throw new Error('remove_amount must be at least 1'); |
There was a problem hiding this comment.
Nit. The error says remove_amount, but the JS field is removeAmount. Use the JS name so the message matches the API.
There was a problem hiding this comment.
Done. Updated the error message to 'removeAmount must be at least 1' to match the JS property name.
| return ( | ||
| parts !== undefined && | ||
| parts !== null && | ||
| Array.isArray(parts) && | ||
| parts.some( | ||
| (part) => | ||
| part.functionResponse !== undefined && part.functionResponse !== null, | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Nit, optional. parts is typed Part[] | undefined, so the !== null and Array.isArray checks are dead. The Python reference checks only parts is not None.
return parts?.some((part) => part.functionResponse != null) ?? false;There was a problem hiding this comment.
Done. Simplified _isFunctionResponseContent using optional chaining to:
return content.parts?.some((part) => part.functionResponse != null) ?? false;|
All suggested changes have been updated and tested. Could you please take another look? @AmaadMartin |
AmaadMartin
left a comment
There was a problem hiding this comment.
Approve. All five earlier findings are fixed at the head source: the instanceof guard is removed, common.ts exports only ContextFilterPlugin and its two types, the constructor takes only the options object, the error message uses removeAmount, and the dead null checks are gone. The windowing and orphan-pairing logic match the adk-python reference. The tests exercise the behavior through the public plugin API. CI was still running at review time; I decided on the code.
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-review at c6bc608. All five earlier findings are fixed in the source: the instanceof guard is gone, common.ts exports only ContextFilterPlugin and the two types, the constructor takes an options object only, the error text uses removeAmount, and the dead parts checks are removed. No suppressions, no instanceof, no console.log. CI: the PR's own context_filter_plugin_test.ts passed 17/17; the red run-tests jobs failed only on known flakes in untouched files (unsafe_local_code_executor_test.ts, app_loader_test.ts, agent_dirname_test.ts, build_setup_test.ts, skills/script_js). Approving on the code.
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-review at c6bc608. All five earlier findings are fixed: the instanceof guard is gone, common.ts exports only the plugin and its two types, the constructor takes one options object, the error message uses removeAmount, and the dead parts checks are gone. The port matches the adk-python reference field for field. CI: run-tests failed on ubuntu and windows in integration tests this PR does not touch (app_loader, build_setup, skills); this is a known flake, macos passed, and I approve on the code.
|
|
||
| if ( | ||
| this.numInvocationsToKeep !== undefined && | ||
| this.numInvocationsToKeep !== null && |
There was a problem hiding this comment.
Nit, optional. numInvocationsToKeep is typed number | undefined, so !== null is dead code. The !== undefined and > 0 checks are enough. This is the same dead null check you removed from _isFunctionResponseContent.
There was a problem hiding this comment.
Done. Removed the redundant !== null check on numInvocationsToKeep so only !== undefined && > 0 remain. Could you please check it again? @AmaadMartin
AmaadMartin
left a comment
There was a problem hiding this comment.
Approve. I verified all earlier findings against the source at head 741f6aa:
- The
instanceofguard is removed. common.tsexports onlyContextFilterPluginand its two types.- The constructor takes the options object only.
- The error message uses
removeAmount. - The dead
!== nullchecks are gone.
I found no new type suppressions, no instanceof, and no missing exports. CI was green at review time.
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-review at head 741f6aa. All six prior findings are fixed in the source. The instanceof guard is gone, common.ts exports only ContextFilterPlugin, ContextFilterPluginOptions and CustomFilterFunction, the positional constructor overload is dropped, and the dead null checks and the error message are corrected. No new any, suppression, instanceof or console.log. CI was green at review time. Approving.
| * @param contents Full conversation contents in chronological order. | ||
| * @returns A list of indices where each index marks the beginning of an invocation. | ||
| */ | ||
| function _getInvocationStartIndices(contents: Content[]): number[] { |
There was a problem hiding this comment.
please avoid using _ prefix for private functions/methods/properties
| llmRequest: LlmRequest; | ||
| }): Promise<LlmResponse | undefined> { | ||
| try { | ||
| if (!params.llmRequest.contents) { |
There was a problem hiding this comment.
we should check on length as well
| const neededCallIds = new Set<string>(); | ||
| for (let i = contents.length - 1; i >= 0; i--) { | ||
| const parts = contents[i].parts; | ||
| if (parts) { |
There was a problem hiding this comment.
parts && parts.length
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. Or, if no issue exists, describe the change:
Problem:
In multi-turn conversations, the chat history sent to the LLM grows with every turn. This causes three issues:
ADK-Python already has
ContextFilterPluginto handle this, but ADK-JS was missing this feature.Solution:
We port
ContextFilterPluginto ADK-JS:Testing Plan
Unit Tests:
Summary of results:
context_filter_plugin_test.tscovering windowing, consecutive user turns, threshold bypass, sync/async custom filters, PII masking, orphan response protection, error resilience, and multi-turn runner integration.Manual End-to-End (E2E) Tests:
npm run ts:check,npm run lint,npm run format:check,npm run docs:check,npm run build, andbash scripts/check_license.sh. All passed with zero errors or warnings.Checklist
Additional context
This is a non-breaking, opt-in feature that ports
ContextFilterPluginfrom Python ADK to achieve parity in ADK-JS. Existing agents and runners that do not use the plugin remain completely unaffected.