Repository navigation
feat(agents): port adk-python v0.5.0 changes to agents - #1104
Open
ScottMansfield wants to merge 2 commits into
Open
ScottMansfield wants to merge 2 commits into
ScottMansfield wants to merge 2 commits into
Conversation
…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.
`BaseAgent` did not expose `canonicalBeforeAgentCallbacks` or `canonicalAfterAgentCallbacks` getters to match `LlmAgent`, and `Context.loadArtifact` threw synchronously instead of returning a rejected promise when `artifactService` was missing. This change adds the canonical agent callback getters on `BaseAgent`, marks `Context.loadArtifact` as `async`, and expands unit tests for agent callbacks, model callbacks, context artifact operations, and `RunConfig.inputAudioTranscription`. - Add `canonicalBeforeAgentCallbacks` and `canonicalAfterAgentCallbacks` getters to `BaseAgent` and use them in `handleBeforeAgentCallback` and `handleAfterAgentCallback`. - Mark `Context.loadArtifact` as `async` so uninitialized `artifactService` errors reject asynchronously like `Context.saveArtifact`. - Add unit tests in `core/test/agents/base_agent_test.ts`, `core/test/agents/llm_agent_test.ts`, and `core/test/agents/run_config_test.ts` covering canonical callback getters, single and chained sync/async agent and model callbacks, async artifact operations in `Context`, and `inputAudioTranscription` on `RunConfig`. Testing: - `npx vitest run --project unit:core core/test/agents/base_agent_test.ts core/test/agents/llm_agent_test.ts core/test/agents/run_config_test.ts` (3 files, 127 tests passed) - `npx vitest run --project unit:core core/test/agents` (36 files passed, 1 skipped, 606 tests passed) - `npm run build && npm run ts:check` (passed) - `npm run lint && npm run format:check` (passed) Produced by Foundry (task 671469f3-9119-4e23-83a5-32be6b655726). Reviewed by an automated reviewer before upload; still needs human review.
ScottMansfield
added
foundry
parity
Member
Author
|
Scored 2026-10-17 00:37 UTC at eea5f64, round 1. Earlier runs are in this comment's edit history. Foundry merge critic: APPROVE Adds canonical agent callback getters to BaseAgent, marks Context.loadArtifact as async, and adds unit tests for callbacks and RunConfig transcription. Rubric — the default is approve; each rule is a reason a human should read this change first. [no ] public_api — Adds canonicalBeforeAgentCallbacks and canonicalAfterAgentCallbacks getters on BaseAgent without changing existing exported signatures. 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. |
kalenkevich
approved these changes
Oct 19, 2026
kalenkevich
approved these changes
Oct 19, 2026
Comment on lines
+94
to
+97
| async loadArtifact( | ||
| filename: string, | ||
| version?: number, | ||
| ): Promise<Part | undefined> { |
Collaborator
There was a problem hiding this comment.
why was formatting changed? Can we avoid changing that in the prs?
kalenkevich
added this pull request to stack #1107
October 19, 2026 17:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BaseAgentdid not exposecanonicalBeforeAgentCallbacksorcanonicalAfterAgentCallbacksgetters to matchLlmAgent, andContext.loadArtifactthrew synchronously instead of returning a rejected promise whenartifactServicewas missing. This change adds the canonical agent callback getters onBaseAgent, marksContext.loadArtifactasasync, and expands unit tests for agent callbacks, model callbacks, context artifact operations, andRunConfig.inputAudioTranscription.canonicalBeforeAgentCallbacksandcanonicalAfterAgentCallbacksgetters toBaseAgentand use them inhandleBeforeAgentCallbackandhandleAfterAgentCallback.Context.loadArtifactasasyncso uninitializedartifactServiceerrors reject asynchronously likeContext.saveArtifact.core/test/agents/base_agent_test.ts,core/test/agents/llm_agent_test.ts, andcore/test/agents/run_config_test.tscovering canonical callback getters, single and chained sync/async agent and model callbacks, async artifact operations inContext, andinputAudioTranscriptiononRunConfig.Testing:
npx vitest run --project unit:core core/test/agents/base_agent_test.ts core/test/agents/llm_agent_test.ts core/test/agents/run_config_test.ts(3 files, 127 tests passed)npx vitest run --project unit:core core/test/agents(36 files passed, 1 skipped, 606 tests passed)npm run build && npm run ts:check(passed)npm run lint && npm run format:check(passed)Produced by Foundry (task 671469f3-9119-4e23-83a5-32be6b655726). Reviewed by an automated
reviewer before upload; still needs human review.