Repository navigation
feat: port adk-python v0.5.0 changes to artifacts, auth, events (+7 more) - #1102
ScottMansfield wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the OpenAPI operation parser to correctly handle empty string descriptions and nullish 'required' fields, and adds comprehensive unit tests for these changes. Additionally, it introduces new unit tests for the LLM user content appending logic (maybeAppendUserContent) and the in-memory session service's timestamp-based event filtering. No review comments were provided, so there is no additional feedback.
15bbb33 to
dba1f2a
Compare
…ore) **Please ensure you have read the [contribution guide](https://google.github.io/adk-docs/contributing-guide/) before creating a pull request.** ### Link to Issue or Description of Change **1. Link to an existing issue (if applicable):** - Closes: N/A - Related: N/A **2. Or, if no issue exists, describe the change:** **Problem:** `OperationParser` copies a parameter description only when `schema.description` is `null` or `undefined`. It ignores the parameter description when `schema.description` is `""`, and lacks unit tests from `adk-python` `v0.5.0`. **Solution:** Update `OperationParser.processOperationParameters` to copy a non-empty parameter description whenever `schema.description` is falsy, and default `required` with `?? false`. Add unit tests for `OperationParser`, `BaseLlm.maybeAppendUserContent`, and `InMemorySessionService.getSession`. ### Files Changed 4 files changed, 230 insertions(+), 2 deletions(-): - `core/src/tools/openapi_tool/openapi_spec_parser/operation_parser.ts` (+2, -2): lines 117 and 121 updated in place to check `!schema.description && description !== ''` and `param.required ?? false`. - `core/test/models/base_llm_test.ts` (+106, -0) - `core/test/sessions/in_memory_session_service_test.ts` (+70, -0) - `core/test/tools/openapi_tool/operation_parser_test.ts` (+52, -0) ### Ported Behavior (`adk-python` `v0.4.0`..`v0.5.0`) - `tools/openapi_tool/openapi_spec_parser/operation_parser.py`: copy `param.description` to `paramSchema.description` when `schema.description` is empty, and default `param.required` to `false` when `null` or `undefined`. - `tools/openapi_tool/common/common.py`: `ApiParameter.required` defaults to `false` (already present in `adk-js`). - `artifacts/*` and `memory/*`: async `BaseArtifactService`, `GcsArtifactService`, `InMemoryArtifactService`, `BaseMemoryService`, `InMemoryMemoryService`, and `VertexAiRagMemoryService` methods (already present in `adk-js`). - `models/base_llm.py` and `models/google_llm.py`: `BaseLlm.maybeAppendUserContent` (already on `BaseLlm` in `adk-js`; added unit tests for empty, non-user, user-ending, and model-ending `contents`). - `models/gemini_llm_connection.py`: live `inputTranscription` support (already present in `adk-js`). - `sessions/in_memory_session_service.py`: inclusive `afterTimestamp` and combined `numRecentEvents` filtering in `getSession` (already present in `adk-js`; added unit tests for boundary, out-of-range, and combined filters). ### Interface discrepancies | Member | adk-python | adk-js | Why | | --- | --- | --- | --- | | `BaseLlm.maybeAppendUserContent` | `_maybe_append_user_content` (private helper) | `maybeAppendUserContent` (public method) | Preserves the existing `adk-js` public method signature. | | `GeminiLlmConnection` input transcription | Yields `LlmResponse(content=Content(role='user', parts=[...]))` | Yields `LlmResponse(inputTranscription=...)` | Preserves newer `adk-js` structured transcription output. | | `DatabaseSessionService.appendEvent` | Raises `ValueError` with formatted timestamps on stale `last_update_time` | Reloads stale session from storage | Preserves newer `adk-js` stale-session reload behavior. | | `AnthropicLlm` / `LiteLlm` | In `google.adk.models` | Omitted from `@google/adk` core | Third-party model SDKs and Python-only facades are excluded from `core`. | | Pydantic `model_config` docstrings | `ConfigDict` docstrings on models, events, sessions, and auth | TypeScript `interface` declarations | `adk-js` uses TypeScript interfaces instead of Pydantic models. | ### Reference Test Accounting | Reference test (`adk-python` `v0.4.0`..`v0.5.0`) | Status in `adk-js` | | --- | --- | | `test_operation_parser.py::test_get_json_schema` (`required` default and empty `schema.description`) | Ported in `core/test/tools/openapi_tool/operation_parser_test.ts` | | `test_litellm.py::test_maybe_append_user_content` (3 parametrized cases + empty `contents`) | Ported on `BaseLlm.maybeAppendUserContent` in `core/test/models/base_llm_test.ts` | | `test_session_service.py::test_get_session_with_config` (`after_timestamp` boundary, out-of-range, combined with `num_recent_events`) | Ported in `core/test/sessions/in_memory_session_service_test.ts` | | `test_artifact_service.py` (`test_load_empty`, `test_save_load_delete`, `test_list_keys`, `test_list_versions` async conversion) | Already covered in `core/test/artifacts/in_memory_artifact_service_test.ts` and `gcs_artifact_service_test.ts` | | `test_auth_handler.py` (`test_generate_auth_uri_oauth2`, `test_generate_auth_uri_openid`) | Not portable: patches `google.adk.auth.auth_handler.OAuth2Session` from Python `requests_oauthlib` | | `test_connections_client.py` / `test_integration_client.py` | Already covered in `core/test/tools/application_integration_tool/` | | `test_rest_api_tool.py` (`sample_endpiont` fixture typo rename) | Not portable: Python pytest fixture rename with no behavioral change | ### Testing Plan **Unit Tests:** - [x] I have added or updated unit tests for my change. - [x] All unit tests pass locally. Verification results on Linux (Windows and macOS CI matrix entries require forge runners): - `npx vitest run --project unit:core core/test/tools/openapi_tool/operation_parser_test.ts core/test/models/base_llm_test.ts core/test/sessions/in_memory_session_service_test.ts`: 3 test files passed, 94 of 94 tests passed (`operation_parser_test.ts`: 35 passed, `base_llm_test.ts`: 11 passed, `in_memory_session_service_test.ts`: 48 passed). - Mutation verification: reverting `OperationParser.processOperationParameters`, `BaseLlm.maybeAppendUserContent`, and `InMemorySessionService.getSession` slice indexing caused the corresponding new unit tests to fail before reverting. - `npm run build`: passed (`@google/adk`, `@google/adk-devtools`, `@google/adk-integrations`). - `npm run ts:check` and `npm run ts:check:samples`: passed with 0 errors. - `npm run lint`: passed with 0 errors. - `npm run format:check`: passed (`All matched files use Prettier code style!`). - `npm run docs:check`: passed with 0 warnings. - `npx secretlint`: passed with 0 findings. **Manual End-to-End (E2E) Tests:** Not applicable; covered by hermetic unit tests. ### Checklist - [x] I have read the [CONTRIBUTING.md](https://github.com/google/adk-js/blob/main/CONTRIBUTING.md) document. - [x] I have performed a self-review of my own code. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have added tests that prove my fix is effective or that my feature works. - [x] New and existing unit tests pass locally with my changes. - [ ] I have manually tested my changes end-to-end. - [ ] Any dependent changes have been merged and published in downstream modules. ### Additional context #### What the reviewer must decide - Whether to introduce an Anthropic model integration under `@google/adk-integrations` in a separate change (`models/anthropic_llm.py` and `LiteLlm` are excluded from `@google/adk` core). - Whether `DatabaseSessionService.appendEvent` should continue reloading stale sessions in `adk-js` instead of throwing an error on stale `lastUpdateTime` as `adk-python` `v0.5.0` did. - Whether `GeminiLlmConnection` should continue emitting structured `inputTranscription` on `LlmResponse` instead of a `role: 'user'` content event. Produced by Foundry (task 72489d7b-3a9c-4071-8f3d-aa1a22cdf462). Reviewed by an automated reviewer before upload; still needs human review.
49089d0 to
31f152a
Compare
|
Scored 2026-10-17 00:00 UTC at 31f152a, round 1. Earlier runs are in this comment's edit history. Foundry merge critic: APPROVE OperationParser now falls back to the parameter description when schema.description is empty, with new unit tests for OperationParser, BaseLlm.maybeAppendUserContent, and InMemorySessionService.getSession. Rubric — the default is approve; each rule is a reason a human should read this change first. [no ] public_api — only the private OperationParser.processOperationParameters helper changes; no exported symbol, type, or method signature is modified Posted by Foundry's merge critic as evidence rather than a request, so nothing here is waiting on you. Advisory only: Foundry has given no approval. |
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:
OperationParsercopies a parameter description only whenschema.descriptionisnullorundefined. It ignores the parameter description whenschema.descriptionis"", and lacks unit tests fromadk-pythonv0.5.0.Solution:
Update
OperationParser.processOperationParametersto copy a non-empty parameter description wheneverschema.descriptionis falsy, and defaultrequiredwith?? false. Add unit tests forOperationParser,BaseLlm.maybeAppendUserContent, andInMemorySessionService.getSession.Files Changed
4 files changed, 230 insertions(+), 2 deletions(-):
core/src/tools/openapi_tool/openapi_spec_parser/operation_parser.ts(+2, -2): lines 117 and 121 updated in place to check!schema.description && description !== ''andparam.required ?? false.core/test/models/base_llm_test.ts(+106, -0)core/test/sessions/in_memory_session_service_test.ts(+70, -0)core/test/tools/openapi_tool/operation_parser_test.ts(+52, -0)Ported Behavior (
adk-pythonv0.4.0..v0.5.0)tools/openapi_tool/openapi_spec_parser/operation_parser.py: copyparam.descriptiontoparamSchema.descriptionwhenschema.descriptionis empty, and defaultparam.requiredtofalsewhennullorundefined.tools/openapi_tool/common/common.py:ApiParameter.requireddefaults tofalse(already present inadk-js).artifacts/*andmemory/*: asyncBaseArtifactService,GcsArtifactService,InMemoryArtifactService,BaseMemoryService,InMemoryMemoryService, andVertexAiRagMemoryServicemethods (already present inadk-js).models/base_llm.pyandmodels/google_llm.py:BaseLlm.maybeAppendUserContent(already onBaseLlminadk-js; added unit tests for empty, non-user, user-ending, and model-endingcontents).models/gemini_llm_connection.py: liveinputTranscriptionsupport (already present inadk-js).sessions/in_memory_session_service.py: inclusiveafterTimestampand combinednumRecentEventsfiltering ingetSession(already present inadk-js; added unit tests for boundary, out-of-range, and combined filters).Interface discrepancies
BaseLlm.maybeAppendUserContent_maybe_append_user_content(private helper)maybeAppendUserContent(public method)adk-jspublic method signature.GeminiLlmConnectioninput transcriptionLlmResponse(content=Content(role='user', parts=[...]))LlmResponse(inputTranscription=...)adk-jsstructured transcription output.DatabaseSessionService.appendEventValueErrorwith formatted timestamps on stalelast_update_timeadk-jsstale-session reload behavior.AnthropicLlm/LiteLlmgoogle.adk.models@google/adkcorecore.model_configdocstringsConfigDictdocstrings on models, events, sessions, and authinterfacedeclarationsadk-jsuses TypeScript interfaces instead of Pydantic models.Reference Test Accounting
adk-pythonv0.4.0..v0.5.0)adk-jstest_operation_parser.py::test_get_json_schema(requireddefault and emptyschema.description)core/test/tools/openapi_tool/operation_parser_test.tstest_litellm.py::test_maybe_append_user_content(3 parametrized cases + emptycontents)BaseLlm.maybeAppendUserContentincore/test/models/base_llm_test.tstest_session_service.py::test_get_session_with_config(after_timestampboundary, out-of-range, combined withnum_recent_events)core/test/sessions/in_memory_session_service_test.tstest_artifact_service.py(test_load_empty,test_save_load_delete,test_list_keys,test_list_versionsasync conversion)core/test/artifacts/in_memory_artifact_service_test.tsandgcs_artifact_service_test.tstest_auth_handler.py(test_generate_auth_uri_oauth2,test_generate_auth_uri_openid)google.adk.auth.auth_handler.OAuth2Sessionfrom Pythonrequests_oauthlibtest_connections_client.py/test_integration_client.pycore/test/tools/application_integration_tool/test_rest_api_tool.py(sample_endpiontfixture typo rename)Testing Plan
Unit Tests:
Verification results on Linux (Windows and macOS CI matrix entries require forge runners):
npx vitest run --project unit:core core/test/tools/openapi_tool/operation_parser_test.ts core/test/models/base_llm_test.ts core/test/sessions/in_memory_session_service_test.ts: 3 test files passed, 94 of 94 tests passed (operation_parser_test.ts: 35 passed,base_llm_test.ts: 11 passed,in_memory_session_service_test.ts: 48 passed).OperationParser.processOperationParameters,BaseLlm.maybeAppendUserContent, andInMemorySessionService.getSessionslice indexing caused the corresponding new unit tests to fail before reverting.npm run build: passed (@google/adk,@google/adk-devtools,@google/adk-integrations).npm run ts:checkandnpm run ts:check:samples: passed with 0 errors.npm run lint: passed with 0 errors.npm run format:check: passed (All matched files use Prettier code style!).npm run docs:check: passed with 0 warnings.npx secretlint: passed with 0 findings.Manual End-to-End (E2E) Tests:
Not applicable; covered by hermetic unit tests.
Checklist
Additional context
What the reviewer must decide
@google/adk-integrationsin a separate change (models/anthropic_llm.pyandLiteLlmare excluded from@google/adkcore).DatabaseSessionService.appendEventshould continue reloading stale sessions inadk-jsinstead of throwing an error on stalelastUpdateTimeasadk-pythonv0.5.0did.GeminiLlmConnectionshould continue emitting structuredinputTranscriptiononLlmResponseinstead of arole: 'user'content event.Produced by Foundry (task 72489d7b-3a9c-4071-8f3d-aa1a22cdf462). Reviewed by an automated
reviewer before upload; still needs human review.