From 80098d711bcf4cf1b0f6de5e05680c92ee547868 Mon Sep 17 00:00:00 2001 From: settler-av Date: Tue, 12 May 2026 05:48:40 +0000 Subject: [PATCH 1/9] fix(llm_flows): injecting TransferToAgentTool after request confirmation --- .../flows/llm_flows/request_confirmation.py | 26 +- .../llm_flows/test_request_confirmation.py | 251 ++++++++++++++++++ 2 files changed, 271 insertions(+), 6 deletions(-) diff --git a/src/google/adk/flows/llm_flows/request_confirmation.py b/src/google/adk/flows/llm_flows/request_confirmation.py index d066db791d..442827e42f 100644 --- a/src/google/adk/flows/llm_flows/request_confirmation.py +++ b/src/google/adk/flows/llm_flows/request_confirmation.py @@ -30,6 +30,9 @@ from ...tools.tool_confirmation import ToolConfirmation from ._base_llm_processor import BaseLlmRequestProcessor from .functions import REQUEST_CONFIRMATION_FUNCTION_CALL_NAME +from .agent_transfer import _get_transfer_targets +from ...tools.transfer_to_agent_tool import TransferToAgentTool + if TYPE_CHECKING: from ...agents.llm_agent import LlmAgent @@ -166,15 +169,26 @@ async def run_async( return # Step 4: Re-execute the confirmed tools. + + tools_dict = { + tool.name: tool + for tool in await agent.canonical_tools( + ReadonlyContext(invocation_context) + ) + } + + if hasattr(agent, 'disallow_transfer_to_parent'): + transfer_targets = _get_transfer_targets(agent) + if transfer_targets: + transfer_tool = TransferToAgentTool( + agent_names=[a.name for a in transfer_targets] + ) + tools_dict[transfer_tool.name] = transfer_tool + if function_response_event := await functions.handle_function_call_list_async( invocation_context, tools_to_resume_with_args.values(), - { - tool.name: tool - for tool in await agent.canonical_tools( - ReadonlyContext(invocation_context) - ) - }, + tools_dict, tools_to_resume_with_confirmation.keys(), tools_to_resume_with_confirmation, ): diff --git a/tests/unittests/flows/llm_flows/test_request_confirmation.py b/tests/unittests/flows/llm_flows/test_request_confirmation.py index a4c9297f42..88638ed51e 100644 --- a/tests/unittests/flows/llm_flows/test_request_confirmation.py +++ b/tests/unittests/flows/llm_flows/test_request_confirmation.py @@ -302,6 +302,257 @@ async def test_request_confirmation_processor_tool_not_confirmed(): ) # tool_confirmation_dict +TRANSFER_TOOL_NAME = "transfer_to_agent" +TRANSFER_FC_ID = "transfer_fc_id" +TRANSFER_CONFIRMATION_FC_ID = "transfer_confirmation_fc_id" + + +def _build_transfer_confirmation_events( + confirmed: bool, +) -> tuple[Event, Event]: + """Helper to build the agent + user events for a transfer_to_agent confirmation.""" + original_fc = types.FunctionCall( + name=TRANSFER_TOOL_NAME, + args={"agent_name": "sub_agent"}, + id=TRANSFER_FC_ID, + ) + tool_confirmation = ToolConfirmation( + confirmed=False, hint="Approve transfer?" + ) + agent_event = Event( + author="agent", + content=types.Content( + parts=[ + types.Part( + function_call=types.FunctionCall( + name=functions.REQUEST_CONFIRMATION_FUNCTION_CALL_NAME, + args={ + "originalFunctionCall": original_fc.model_dump( + exclude_none=True, by_alias=True + ), + "toolConfirmation": tool_confirmation.model_dump( + by_alias=True, exclude_none=True + ), + }, + id=TRANSFER_CONFIRMATION_FC_ID, + ) + ) + ] + ), + ) + user_confirmation = ToolConfirmation(confirmed=confirmed) + user_event = Event( + author="user", + content=types.Content( + parts=[ + types.Part( + function_response=types.FunctionResponse( + name=functions.REQUEST_CONFIRMATION_FUNCTION_CALL_NAME, + id=TRANSFER_CONFIRMATION_FC_ID, + response={ + "response": user_confirmation.model_dump_json() + }, + ) + ) + ] + ), + ) + return agent_event, user_event + + +@pytest.mark.asyncio +async def test_request_confirmation_transfer_to_agent_approved(): + """Test that transfer_to_agent is injected into tools_dict when confirmed.""" + sub_agent = LlmAgent(name="sub_agent", model="gemini-2.0-flash") + agent = LlmAgent( + name="orchestrator", model="gemini-2.0-flash", sub_agents=[sub_agent] + ) + invocation_context = await testing_utils.create_invocation_context( + agent=agent + ) + llm_request = LlmRequest() + + agent_event, user_event = _build_transfer_confirmation_events(confirmed=True) + invocation_context.session.events.append(agent_event) + invocation_context.session.events.append(user_event) + + expected_event = Event( + author="agent", + content=types.Content( + parts=[ + types.Part( + function_response=types.FunctionResponse( + name=TRANSFER_TOOL_NAME, + id=TRANSFER_FC_ID, + response={}, + ) + ) + ] + ), + ) + + with patch( + "google.adk.flows.llm_flows.functions.handle_function_call_list_async" + ) as mock_handle: + mock_handle.return_value = expected_event + + events = [] + async for event in request_processor.run_async( + invocation_context, llm_request + ): + events.append(event) + + assert len(events) == 1 + mock_handle.assert_called_once() + args, _ = mock_handle.call_args + tools_dict = args[2] + assert TRANSFER_TOOL_NAME in tools_dict + + +@pytest.mark.asyncio +async def test_request_confirmation_transfer_to_agent_rejected(): + """Test that transfer_to_agent is injected even when rejected.""" + sub_agent = LlmAgent(name="sub_agent", model="gemini-2.0-flash") + agent = LlmAgent( + name="orchestrator", model="gemini-2.0-flash", sub_agents=[sub_agent] + ) + invocation_context = await testing_utils.create_invocation_context( + agent=agent + ) + llm_request = LlmRequest() + + agent_event, user_event = _build_transfer_confirmation_events( + confirmed=False + ) + invocation_context.session.events.append(agent_event) + invocation_context.session.events.append(user_event) + + expected_event = Event( + author="agent", + content=types.Content( + parts=[ + types.Part( + function_response=types.FunctionResponse( + name=TRANSFER_TOOL_NAME, + id=TRANSFER_FC_ID, + response={"error": "Tool execution not confirmed"}, + ) + ) + ] + ), + ) + + with patch( + "google.adk.flows.llm_flows.functions.handle_function_call_list_async" + ) as mock_handle: + mock_handle.return_value = expected_event + + events = [] + async for event in request_processor.run_async( + invocation_context, llm_request + ): + events.append(event) + + assert len(events) == 1 + mock_handle.assert_called_once() + args, _ = mock_handle.call_args + tools_dict = args[2] + assert TRANSFER_TOOL_NAME in tools_dict + + +@pytest.mark.asyncio +async def test_request_confirmation_no_sub_agents_no_transfer_tool(): + """Test that transfer_to_agent is NOT injected when agent has no sub_agents.""" + agent = LlmAgent( + name="test_agent", model="gemini-2.0-flash", tools=[mock_tool] + ) + invocation_context = await testing_utils.create_invocation_context( + agent=agent + ) + llm_request = LlmRequest() + + original_fc = types.FunctionCall( + name=MOCK_TOOL_NAME, args={"param1": "test"}, id=MOCK_FUNCTION_CALL_ID + ) + tool_confirmation = ToolConfirmation(confirmed=False, hint="test hint") + invocation_context.session.events.append( + Event( + author="agent", + content=types.Content( + parts=[ + types.Part( + function_call=types.FunctionCall( + name=functions.REQUEST_CONFIRMATION_FUNCTION_CALL_NAME, + args={ + "originalFunctionCall": original_fc.model_dump( + exclude_none=True, by_alias=True + ), + "toolConfirmation": tool_confirmation.model_dump( + by_alias=True, exclude_none=True + ), + }, + id=MOCK_CONFIRMATION_FUNCTION_CALL_ID, + ) + ) + ] + ), + ) + ) + + user_confirmation = ToolConfirmation(confirmed=True) + invocation_context.session.events.append( + Event( + author="user", + content=types.Content( + parts=[ + types.Part( + function_response=types.FunctionResponse( + name=functions.REQUEST_CONFIRMATION_FUNCTION_CALL_NAME, + id=MOCK_CONFIRMATION_FUNCTION_CALL_ID, + response={ + "response": user_confirmation.model_dump_json() + }, + ) + ) + ] + ), + ) + ) + + expected_event = Event( + author="agent", + content=types.Content( + parts=[ + types.Part( + function_response=types.FunctionResponse( + name=MOCK_TOOL_NAME, + id=MOCK_FUNCTION_CALL_ID, + response={"result": "Mock tool result with test"}, + ) + ) + ] + ), + ) + + with patch( + "google.adk.flows.llm_flows.functions.handle_function_call_list_async" + ) as mock_handle: + mock_handle.return_value = expected_event + + events = [] + async for event in request_processor.run_async( + invocation_context, llm_request + ): + events.append(event) + + assert len(events) == 1 + mock_handle.assert_called_once() + args, _ = mock_handle.call_args + tools_dict = args[2] + assert TRANSFER_TOOL_NAME not in tools_dict + assert MOCK_TOOL_NAME in tools_dict + + @pytest.mark.asyncio async def test_request_confirmation_processor_finds_user_confirmation_in_default_branch(): """Processor finds user confirmation in default branch when agent is in child branch. From 521b7b2b6b9e41a584d7449ae4aeda044cfda01d Mon Sep 17 00:00:00 2001 From: Bo Yang Date: Fri, 29 May 2026 11:12:42 -0700 Subject: [PATCH 2/9] feat(skills): Automate PR triage and CLA verification Introduce the adk-pr-triage skill and its supporting triage_pr.py helper script to guide AI coding assistants through conducting rigorous and automated triage of GitHub pull requests. The workflow script handles compliance verification, validates that the contributor's Google CLA signature status check run is SUCCESS, fetches remote branches, and automates PR updates via server-side rebase before local checkout. Provides an interactive 'Local Review' option that integrates branch checkout, preserves original PR commit metadata, and triggers /adk-review for rigorous quality control before squashing and pushing changes to Gerrit. Change-Id: If311b90b185636f4f737aa04a3c50a4a460b94db --- .agents/skills/adk-pr-triage/SKILL.md | 302 ++++++++++++++++++ .../skills/adk-pr-triage/scripts/triage_pr.py | 193 +++++++++++ .github/workflows/copybara-pr-handler.yml | 13 +- AGENTS.md | 3 + 4 files changed, 509 insertions(+), 2 deletions(-) create mode 100644 .agents/skills/adk-pr-triage/SKILL.md create mode 100755 .agents/skills/adk-pr-triage/scripts/triage_pr.py diff --git a/.agents/skills/adk-pr-triage/SKILL.md b/.agents/skills/adk-pr-triage/SKILL.md new file mode 100644 index 0000000000..9475a22036 --- /dev/null +++ b/.agents/skills/adk-pr-triage/SKILL.md @@ -0,0 +1,302 @@ +--- +name: adk-pr-triage +description: Analyze and triage GitHub pull requests for the adk-python repository. The user provides a PR number or URL, and the skill performs an evaluation on the PR's objectives, legitimacy, alignment with ADK's principles (including API stability, package self-containment, explicit exports, styling and naming conventions), and asks the user whether to push back on the PR or perform a local review (checking out, rebasing, and running adk-review before pushing to Gerrit). Triggers on "triage pr", "pr triage", "review pr", "pr review", "pull request", "github.com/google/adk-python/pull/". +--- + +# ADK Pull Request Triage (adk-pr-triage) + +This skill guides AI assistants in conducting a highly professional, rigorous, and constructive triage of GitHub pull requests (PRs) submitted to the `google/adk-python` repository. It parses the PR, retrieves its context, evaluates it against ADK's design, style, and testing principles, presents a premium analysis report, and authors tailored response feedback (such as structured push-back or approval comments) under direct user guidance. + +> [!IMPORTANT] +> ## CRITICAL EXECUTION RULE: STOP AND ASK DECISION GATE +> * **Phase 1 (PR Analysis) is strictly read-only**: Do NOT create branches, modify workspace files, or post any comments in your first response. +> * **STOP and ask**: You must present your full in-depth PR review report first, and explicitly ask the user: +> > "Would you like me to push back on this pull request? (If yes, select one of the push-back reasons or write custom feedback, and I will author a professional and precise review message for you to review. If no, I will draft an approval response highlighting the positive aspects of the implementation.)" +> * **Wait for Instructions**: Only author and present feedback drafts in a subsequent turn *after* the user provides their decision. + +--- + +## Phase 1: Retrieve and Parse the PR & Linked Context (Read-Only) + +### Step 1: Extract PR Identifier & Verify CLA Signature (Mandatory Entry Gate) +1. **Identify the PR identifier**: Parse the PR number or URL from the prompt (e.g., `https://github.com/google/adk-python/pull/5885` -> `5885`). +2. **CRITICAL COMPLIANCE GATE - Run CLA Verification Script**: + * **Rule**: BEFORE doing any further work, diff reading, or analysis, you MUST run the compliance helper script in read-only mode to verify the contributor's Contributor License Agreement (CLA) signature: + ```bash + .venv/bin/python .agents/skills/adk-pr-triage/scripts/triage_pr.py --skip-update + ``` + * **Inspect the Exit Status**: + * **Exit Code 2 (Refusal)**: The contributor HAS NOT signed the Google CLA. You **MUST absolutely refuse** to perform any analysis, triage, diff-fetching, checking out, or workspace operations. Stop calling tools immediately and print a clear compliance refusal message stating that the Google CLA is not signed. + * **Exit Code 0 (Success)**: The Google CLA is verified. Proceed with the triage steps. +3. **Fetch PR details**: Use the `gh` CLI tool to fetch pull request metadata in JSON format: + ```bash + gh pr view --repo google/adk-python --json number,title,body,state,url,author,additions,deletions,changedFiles,labels + ``` +4. **Locate linked issue(s)**: Review the PR body or closing references (e.g., `Fixes #5882`, `Resolves #5882`). If an issue is linked, fetch that issue's details to understand the original problem statement: + ```bash + gh issue view --repo google/adk-python --json number,title,body,state + ``` + +### Step 2: Retrieve the Complete Diff +1. **Fetch pull request changes**: Run the `gh pr diff` command to view the actual line-by-line diff of the PR: + ```bash + gh pr diff --repo google/adk-python + ``` +2. **Review files modified**: Match the diff segments against existing repository files to identify the target components under review. + +--- + +## Phase 2: Deep Code & Architectural Analysis (Read-Only) + +Conduct an extremely thorough review of the changes by examining the diff and analyzing the local codebase. You must address the following three critical dimensions and organize your findings in a premium **PR Review Report**: + +### 1. Objectives & Impact ("What issue does it fix, or feature does it introduce?") +- **Core Change Summary**: Define what the code modifications do, where they are applied (classes, methods, functions), and the execution flow involved. +- **Problem Resolution**: Confirm how the implementation fixes the linked issue or introduces the target feature. +- **Context Tracing**: Trace the execution flow in the active workspace and explain what modules are impacted by this patch. + +### 2. Legitimacy & Value ("Is it a legitimate issue or a useful feature?") +- **Codebase Verification**: Verify the bug/gap exists in the baseline code by searching the local workspace using `grep_search` and inspecting target files with `view_file`. +- **Aesthetic & Structural Value**: Analyze whether the problem represents a legitimate, high-priority bug (e.g., causing hangs, memory leaks, or incorrect API validation) or if the feature adds actual, tangible utility to ADK developers. +- **Alternatives Assessment**: Assess if the PR's solution is the most elegant one, or if there is a cleaner, less intrusive, or more robust alternative pattern (e.g., utilizing an existing helper instead of introducing duplicate logic). + +### 3. Architectural & Principle Alignment ("Does it align with ADK's principles?") +Evaluate the implementation against the established architectural, style, and testing guidelines. Use direct file links to code reference examples. + +#### A. Public API and Visibility Principles +- **API Stability**: Does the change introduce a breaking change to any public classes, methods, or CLI structures in the `google.adk` namespace? (Breaking changes are unacceptable under Semantic Versioning without an official deprecation cycle). +- **Module and File Naming**: Are new `.py` module files under `src/google/adk/` private by default (prefixed with a leading underscore, e.g., `_my_module.py`)? +- **Explicit Exports**: Are new public symbols explicitly exposed via the package's `__init__.py` using the `__all__` list? Are internal helper classes and on-wire objects kept internal by omitting them from `__all__`? +- **Self-Containment**: Does inside-framework code import from the subsystem's specific module directly, rather than importing from `__init__.py`? (Within ADK, framework-level imports from `__init__.py` are strictly prohibited to avoid circular dependencies and maintain clean encapsulation). +- **Intuitive Naming**: Are public methods and class names concise (e.g., `Runner.run`), while private/internal methods are descriptive (e.g., `_validate_chat_agent_wiring`)? + +#### B. Code Quality, Style & Pythonic Conventions +- **Future Annotations**: Does every new or heavily edited python source file include `from __future__ import annotations` immediately after the license header? +- **Strong Typing**: Are type hints used for all function arguments and return values? Is the use of `Any` avoided in favor of precise types, abstract interfaces, or generics? +- **Modern Types**: Is the modern union syntax `X | None` preferred for new code over the legacy `Optional[X]`? +- **Keyword-Only Arguments**: Are swaps and parameter mismatches prevented by enforcing keyword-only arguments using `*` for constructors with multiple attributes? +- **Mutable Defaults**: Are mutable defaults (like `list`, `dict`, `set`) avoided? (Use `None` and instantiate within the method body). +- **Runtime Discrimination**: Does type validation use `isinstance(obj, Type)` instead of `type(obj) is Type` to support subclasses, and is a fallback `else` raise handled? +- **Pydantic v2 Idioms**: For Pydantic models: + - Do they use `Field()` constraints for simple boundary checks? + - Do validation rules use `@field_validator` (with `mode='after'`) and `@model_validator`? + - Is `use_attribute_docstrings=True` configured in the model `ConfigDict` so that docstrings are utilized as field descriptions? + - Are internal mutable states declared with `PrivateAttr()` and constructor logic mapped in `model_post_init()`? +- **Lazy Logging**: Does logging utilize lazy-evaluated `%`-based templates rather than eager `f-strings`? (e.g., `logging.info("Completed in %s ms", duration)` is correct; `logging.info(f"Completed in {duration} ms")` is a violation). +- **Error Handling**: Are specific exceptions caught with context, avoiding bare `except:` constructs? + +#### C. Test Integrity & Verification Quality +- **Behavior-Focused Testing**: Do the new unit or integration tests under `tests/` target public boundaries rather than internal execution states? +- **No Mocking of Core Components**: Are real ADK modules (`BaseNode`, `Event`, `Context`) used, restricting mocking exclusively to external web or network dependencies? +- **Minimal Fixtures & Locality**: Are test helper classes and fixtures kept close to the test functions (defined inline inside the test function when utilized by a single test) to improve discoverability? +- **Structure**: Do tests follow the clean **Arrange-Act-Assert** pattern separated by clear logical blocks? + +--- + +## Phase 3: Stop and Ask for Push-Back or Local Review (Interactive Gate) + +Present the completed analysis report in your response. Follow the **PR Review Report Template** below for a highly premium, readable presentation. + +### The Interactive Gate Callout +At the end of your report, stop calling tools and output this explicit message: +> "### 🛑 Review Decision Gate +> I have completed my in-depth analysis of Pull Request #. Please review the findings above. +> +> **How would you like to proceed with this Pull Request?** +> - **[Option 1]**: **Push Back** (Draft a professional, constructive feedback response with recommendations for the author). +> - **[Option 2]**: **Local Review** (Checkout the PR locally, rebase onto the latest main, and run the `/adk-review` skill to thoroughly verify and polish before pushing to Gerrit)." + +--- + +## Phase 4: Action Execution (Subsequent Turn) + +Once the user provides their decision, perform the tailored operations in your subsequent turns: + +### Branch A: Push Back +1. **Analyze the Push-Back Focus**: Read the user's specific feedback or selected points of concern. +2. **Draft Constructive Feedback**: Author a highly structured, objective, and supportive response that teaches the contributor while insisting on quality. +3. **Include Concrete Recommendations**: Quote specific files/lines in their diff and provide complete, refactored code blocks in your comments so they can easily apply the fixes. Reference the relevant ADK style guides. +4. **Present the Draft**: Format your draft using the **GitHub Review Draft Template** below. + +### Branch B: Local Review (Checkout & Revise) +If the user selects **Local Review**, run the following structured sequence: + +1. **Step 0: Update the PR Head Branch on GitHub (Mandatory Sync)**: + * **Rule**: BEFORE downloading or checking out the pull request locally, you MUST trigger an update on the remote GitHub pull request to align it with the latest remote base branch (`main`). + * Run the verification & sync helper script to update the branch: + ```bash + .venv/bin/python .agents/skills/adk-pr-triage/scripts/triage_pr.py [pr_number] + ``` + * *What it does*: This script automatically checks the Google CLA signature status again, attempts to update the PR branch on GitHub by rebasing onto `main`, and if rebase-update is blocked, falls back to updating via a merge commit. It handles all outputs and fallbacks gracefully. + +2. **Step 1: Checkout the PR to a Local Branch**: + * Branch naming convention: `pr-triage-[pr_number]-[short_desc]` (e.g. `pr-triage-5875-parallelize-tool-union`). + * Fetch the pull request ref directly from the remote GitHub endpoint: + ```bash + git fetch https://github.com/google/adk-python.git pull/[pr_number]/head:pr-triage-[pr_number]-[short_desc] + ``` + * Checkout to the newly created local branch: + ```bash + git checkout pr-triage-[pr_number]-[short_desc] + ``` + +3. **Step 2: Preserve the Commit Message & Append Merge Reference**: + * **CRITICAL**: You MUST preserve the exact same commit message from the pull request! + * Determine if the PR contains a single commit or multiple commits: + * **Single Commit**: Retrieve the exact original commit message: + ```bash + git log -1 --pretty=%B + ``` + * **Multiple Commits**: Squash them into a single local commit first, keeping the overall PR Title and PR Body as the exact commit message. An elegant way to squash is: + ```bash + git reset --soft $(git merge-base HEAD origin/main) && git commit -m "" + ``` + * Append `"Merge "` to the very end of the commit message (separated by a blank line). Use this elegant shell command to do it in one-shot: + ```bash + git commit --amend -m "$(git log -1 --pretty=%B) + + Merge https://github.com/google/adk-python/pull/[pr_number]" + ``` + * *Note*: When you run git commit/amend, the Gerrit `commit-msg` hook will automatically execute and append the `Change-Id:` footprint if not already present. + +4. **Step 3: Rebase on top of Main**: + * Run the rebase command to place the CL commit on top of the latest local remote tracking `main` branch: + ```bash + git rebase origin/main + ``` + +5. **Step 4: Execute Code Verification & Polishing**: + * Trigger the local review process by invoking the **`/adk-review`** skill! + * Follow its comprehensive guidelines to audit edge cases, style compliance, dependencies, and test validation. Work in partnership with the user to revise the local changes as needed. + +6. **Step 5: Squash User Revisions & Push to Gerrit**: + * If the user requests to push to Gerrit, squash/amend all local workspace revisions into the single original commit: + * **CRITICAL**: You MUST preserve the exact same commit message, including the `Merge ` footer and the original `Change-Id:` footer. Do NOT change it. + * Command to squash all changes into the current commit without opening an editor: + ```bash + git commit -a --amend --no-edit + ``` + * Push the single finalized CL commit to Gerrit: + ```bash + git push origin HEAD:refs/for/main + ``` + +--- + +## PR Review Report Template + +Present the initial analysis using the following structured format: + +```markdown +# 🔍 ADK Pull Request Review: PR # +**Title**: +**Author**: @ +**Status**: `` +**Impact**: ` additions`, ` deletions` across ` files` + +## Detailed Findings & Analysis + +### 1. Objectives & Impact ("What does it do?") +- **Context & Background**: [Briefly explain the background and the problem it targets. Reference linked Issue # using markdown links if available] +- **Implementation Mechanism**: [Detail precisely which modules are modified and how the execution flow is altered] +- **Affected Surface**: [Highlight any changes to public classes, CLI interfaces, state models, or setup pipelines] + +### 2. Legitimacy & Value ("Is it a valid and useful change?") +- **Workspace Verification**: + - Investigated current workspace files: [file_name.py](file:///absolute/path/to/src/google/adk/...#L123-L145) (using `view_file` / `grep_search`). + - Found that: [Describe the baseline condition that proves the bug exists or the feature is missing] +- **Value Assessment**: [Explain why this is a good addition. Does it solve a genuine real-world developer problem, improve performance, or prevent resources leaks?] +- **Alternative Approaches**: [Evaluate if there is an alternative implementation path. Did the author choose the cleanest design?] + +### 3. Principle & Style Alignment Checklist ("Does it follow rules?") + +* **Public API & Visibility Boundaries**: + * *Status*: [Pass / Fail / N/A] + * *Analysis*: [Check for breaking changes, private module conventions `_`, and explicit exports in `__init__.py` using `__all__`] +* **Code Quality, Typing & Conventions**: + * *Status*: [Pass / Fail / Nits] + * *Analysis*: [Check for `from __future__ import annotations`, absence of `Any`, modern unions `X | None`, lazy logging `%`, specific exception catching, and Pydantic v2 structures] +* **Robustness & Edge Cases**: + * *Status*: [Pass / Fail] + * *Analysis*: [Check for type discrimination (`isinstance`), boundaries, null checks, fallback else routes, and thread/async safety] +* **Test Integrity & Quality**: + * *Status*: [Pass / Fail / N/A] + * *Analysis*: [Check coverage, testing through public interfaces, minimal inline fixtures, and Arrange-Act-Assert formatting] + +--- + +## Executive Summary +1. **Core Objective**: [Briefly summarize what issue is fixed or feature is introduced] +2. **Legitimacy & Value**: [Legitimate Fix / Valuable Feature / Duplicate / Redundant] - [1-sentence explanation] +3. **Alignment with Principles**: [Pass / Pass with Nits / Major Changes Required] - [1-sentence architecture alignment summary] +4. **Recommendation**: [Approve / Approve with Nits / Push Back (Request Changes)] + +--- + +### 🛑 Review Decision Gate +I have completed my in-depth analysis of Pull Request #. Please review the findings above. + +**How would you like to proceed with this Pull Request?** +- **[Option 1]**: **Push Back** (Draft a professional, constructive feedback response with recommendations for the author). +- **[Option 2]**: **Local Review** (Checkout the PR locally under `pr-triage-[pr_number]-[short_desc]`, rebase onto the latest main, and run the `/adk-review` skill to thoroughly verify and polish before pushing to Gerrit)." +``` + +--- + +## GitHub Review Draft Template + +Format the authored review response as a premium markdown snippet block: + +````markdown +# 💬 GitHub PR Review Draft Message +*Copy and paste this response directly into the GitHub review interface:* + +--- + +### PR Review: + +Hello @! Thank you very much for contributing this pull request to improve ADK. I've conducted a thorough architectural and style review of your implementation against our design guidelines and standards. + +Here is the feedback and a few suggested changes to align your patch with ADK's principles: + +#### 🔴 Major Concerns / Blocks +1. **[Concern 1 Title, e.g., Import from init.py is not allowed]** + - **Target Code**: [filename.py:L100-L105](file:///absolute/path/to/src/google/adk/file_name.py#L100-L105) + - **Issue**: [Detailed explanation of why this violates design/architectural rules, referencing the relevant ADK skill like `adk-architecture` or `adk-style`] + - **Suggested Correction**: + ```python + # Provide full, drop-in replacement code block + ``` + +2. **[Concern 2 Title, e.g., Missing Unit Tests for Edge Cases]** + - **Target Code**: [test_filename.py](file:///absolute/path/to/tests/unittests/test_filename.py) + - **Issue**: [Detail what is missing, e.g., "We need verification coverage of boundaries like empty string and negative values."] + +#### 🟡 Style & Quality Nits +1. **[Style Nit, e.g., Eager Logging formatting]** + - **Target Code**: [filename.py:L42](file:///absolute/path/to/src/google/adk/file_name.py#L42) + - **Suggestion**: Use lazy-evaluated `%` template syntax: + ```python + # Corrected: + logging.info("User registered: %s", user_id) + ``` + +2. **[Typing Nit, e.g., Optional[X] instead of X | None]** + - **Target Code**: [filename.py:L15](file:///absolute/path/to/src/google/adk/file_name.py#L15) + - **Suggestion**: Prefer more concise union type hint `X | None`. + +#### 🟢 Positive Aspects +- [Highlight stellar work, e.g., "Excellent Pydantic v2 validation logic!" or "Highly readable and clean docstrings!"] + +Please let me know if you have any questions on these suggestions, and let's work together to get this PR merged! +```` + +--- + +## Tips & Best Practices + +> [!TIP] +> Always verify the baseline behavior in your active workspace before claiming something is a bug or invalid. Reading the current source files using `view_file` gives you full context. + +> [!IMPORTANT] +> When referencing files and line numbers in your reports and draft reviews, always use clickable markdown file links of format `[filename.py](file:///absolute/path/to/file#L100-L120)` without surrounding backticks around the brackets. Ensure the links represent valid absolute file paths in the local workspace. diff --git a/.agents/skills/adk-pr-triage/scripts/triage_pr.py b/.agents/skills/adk-pr-triage/scripts/triage_pr.py new file mode 100755 index 0000000000..9e3d1425d1 --- /dev/null +++ b/.agents/skills/adk-pr-triage/scripts/triage_pr.py @@ -0,0 +1,193 @@ +#!/usr/bin/env python3 +# Copyright 2026 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Helper script for ADK PR Triage verification and remote update.""" + +from __future__ import annotations + +import argparse +import json +import subprocess +import sys + + +def run_command(args: list[str]) -> tuple[int, str, str]: + """Runs a shell command and returns its exit code, stdout, and stderr.""" + try: + res = subprocess.run(args, capture_output=True, text=True, check=False) + return res.returncode, res.stdout.strip(), res.stderr.strip() + except Exception as e: + return -1, "", str(e) + + +def verify_cla(pr_number: str) -> bool: + """Verifies if the Google CLA is signed for the given PR number.""" + print(f"[*] Fetching status checks for PR #{pr_number}...") + cmd = [ + "gh", + "pr", + "view", + pr_number, + "--repo", + "google/adk-python", + "--json", + "statusCheckRollup", + ] + code, stdout, stderr = run_command(cmd) + if code != 0: + print( + f"Error: Failed to fetch PR details from GitHub: {stderr}", + file=sys.stderr, + ) + sys.exit(1) + + try: + data = json.loads(stdout) + except json.JSONDecodeError: + print("Error: Failed to parse GitHub API JSON response.", file=sys.stderr) + sys.exit(1) + + status_checks = data.get("statusCheckRollup", []) + cla_check = None + for check in status_checks: + if check.get("name") == "cla/google": + cla_check = check + break + + if not cla_check: + print("\n" + "=" * 80) + print("🚨 CRITICAL COMPLIANCE REFUSAL: GOOGLE CLA NOT SIGNED/VERIFIED 🚨") + print("=" * 80) + print( + "Error: The mandatory 'cla/google' status check is completely missing" + " on GitHub." + ) + print( + "The contributor HAS NOT signed the Google Contributor License" + " Agreement." + ) + print( + "Legal policy strictly prohibits triaging, downloading, or reviewing" + " this PR." + ) + print("=" * 80 + "\n") + return False + + conclusion = cla_check.get("conclusion") + if conclusion != "SUCCESS": + print("\n" + "=" * 80) + print("🚨 CRITICAL COMPLIANCE REFUSAL: GOOGLE CLA NOT SIGNED/VERIFIED 🚨") + print("=" * 80) + print( + "Error: The 'cla/google' status check has the status:" + f" '{conclusion or 'UNKNOWN'}'." + ) + print( + "The contributor HAS NOT successfully signed or verified the Google" + " CLA." + ) + print( + "Legal policy strictly prohibits triaging, downloading, or reviewing" + " this PR." + ) + print("=" * 80 + "\n") + return False + + print("✅ Google CLA is verified and signed (status SUCCESS).") + return True + + +def update_pr_branch(pr_number: str) -> None: + """Updates the remote PR branch with the latest changes from the base branch.""" + print( + f"\n[*] Attempting to update PR #{pr_number} branch via remote REBASE..." + ) + rebase_cmd = [ + "gh", + "pr", + "update-branch", + pr_number, + "--rebase", + "--repo", + "google/adk-python", + ] + code, stdout, stderr = run_command(rebase_cmd) + if code == 0: + print( + "✅ Successfully updated PR branch on GitHub by rebasing onto base" + " branch!" + ) + if stdout: + print(stdout) + return + + print(f"Warning: Remote rebase-update failed: {stderr}") + print("[*] Falling back to standard remote MERGE commit update...") + + merge_cmd = [ + "gh", + "pr", + "update-branch", + pr_number, + "--repo", + "google/adk-python", + ] + code, stdout, stderr = run_command(merge_cmd) + if code == 0: + print( + "✅ Successfully updated PR branch on GitHub via standard merge commit!" + ) + if stdout: + print(stdout) + return + + print( + "\n[!] Warning: Remote branch update failed completely on GitHub server:" + f" {stderr}" + ) + print(" This is typical if edits are disabled on the contributor's fork.") + print( + " No worries! We will automatically rebase locally after checking out." + ) + + +def main() -> None: + parser = argparse.ArgumentParser( + description="Triage PR verification and sync helper." + ) + parser.add_argument( + "pr_number", help="The GitHub Pull Request number (e.g. 5875)." + ) + parser.add_argument( + "--skip-update", + action="store_true", + help="Skip updating the remote PR branch on GitHub.", + ) + args = parser.parse_args() + + # Step 1: Verify CLA + if not verify_cla(args.pr_number): + sys.exit(2) # Exit code 2 indicates compliance refusal + + # Step 2: Update branch + if not args.skip_update: + update_pr_branch(args.pr_number) + + print("\n[*] Verification complete. Safe to proceed with checkout.") + sys.exit(0) + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/copybara-pr-handler.yml b/.github/workflows/copybara-pr-handler.yml index c176d8746a..28c0f3cdcd 100644 --- a/.github/workflows/copybara-pr-handler.yml +++ b/.github/workflows/copybara-pr-handler.yml @@ -57,11 +57,12 @@ jobs: console.log(`\n--- Processing commit ${sha.substring(0, 7)} ---`); console.log(`Committer: ${committer}`); - // Check if this is a Copybara commit + // Check if this is a Copybara commit or has a pull request reference const isCopybara = committer === 'Copybara-Service' || commit.author?.email === 'genai-sdk-bot@google.com' || message.includes('GitOrigin-RevId:') || - message.includes('PiperOrigin-RevId:'); + message.includes('PiperOrigin-RevId:') || + /Merge:?\s+https:\/\/github\.com\/google\/adk-python\/pull\/(\d+)/.test(message); if (!isCopybara) { console.log('Not a Copybara commit, skipping'); @@ -118,6 +119,14 @@ jobs: body: `Thank you @${author} for your contribution! 🎉\n\nYour changes have been successfully imported and merged via Copybara in commit ${commitSha}.\n\nClosing this PR as the changes are now in the main branch.` }); + // Add 'merged' label to the PR + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + labels: ['merged'] + }); + // Close the PR await github.rest.pulls.update({ owner: context.repo.owner, diff --git a/AGENTS.md b/AGENTS.md index 66b101e679..efae707ea1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,6 +18,9 @@ For all matters regarding ADK development, please use the appropriate skill: - Read `.agents/skills/adk-review/SKILL.md` for full instructions. - **`adk-issue`**: Use this skill when analyzing and triaging GitHub issues for the adk-python repository to verify legitimacy, recommend fixes, and check for existing PRs. - Read `.agents/skills/adk-issue/SKILL.md` for full instructions. +- **`adk-pr-triage`**: Use this skill when triaging and analyzing GitHub pull requests (PRs) to evaluate their objectives, legitimacy, value, and alignment with ADK's architectural, styling, and testing principles. + - Read `.agents/skills/adk-pr-triage/SKILL.md` for full instructions. + ## Project Overview From 98d86446ab251c9139e9cfe3e10d23a3c23b60d9 Mon Sep 17 00:00:00 2001 From: Google Team Member Date: Fri, 29 May 2026 14:20:17 -0400 Subject: [PATCH 3/9] perf(flows): Resolve agent tool unions in parallel Merge 31eda4959b72761a5442446a4c0c15f023ec5baa into 6ce4b87858b1fca8d5f0609bb92f198678555c4a Merges https://github.com/google/adk-python/pull/5875 ORIGINAL_AUTHOR=wenzhaoy-google <141370433+wenzhaoy-google@users.noreply.github.com> GitOrigin-RevId: 436276658021d7127fddf1da77f17563b8e8d4df Change-Id: I2093e78fd71c20bb600b82a979cf4ae92d0b18ff --- .../adk/flows/llm_flows/base_llm_flow.py | 43 ++++-- .../flows/llm_flows/test_base_llm_flow.py | 125 ++++++++++++++++++ 2 files changed, 158 insertions(+), 10 deletions(-) diff --git a/src/google/adk/flows/llm_flows/base_llm_flow.py b/src/google/adk/flows/llm_flows/base_llm_flow.py index 497560628b..74f7c6a8db 100644 --- a/src/google/adk/flows/llm_flows/base_llm_flow.py +++ b/src/google/adk/flows/llm_flows/base_llm_flow.py @@ -420,6 +420,15 @@ async def _process_agent_tools( instances, and calls ``process_llm_request`` on each to register tool declarations in the request. + Tool-union resolution is dispatched concurrently via ``asyncio.gather`` + to overlap I/O-bound listings (e.g. MCP ``list_tools`` over the + network). The subsequent ``process_llm_request`` calls are kept + serial in the original ``agent.tools`` order: some tools read/write + ``llm_request`` state (e.g. ``GoogleSearchTool`` writes + ``llm_request.model``; ``ComputerUseToolset`` performs an idempotency + check on ``llm_request.config.tools``) and rely on observing the + post-state of earlier tools. + After this function returns, ``llm_request.tools_dict`` maps tool names to ``BaseTool`` instances ready for function call dispatch. @@ -429,12 +438,34 @@ async def _process_agent_tools( llm_request: The LLM request to populate with tool declarations. """ agent = invocation_context.agent - if not hasattr(agent, 'tools') or not agent.tools: + if agent is None or not hasattr(agent, 'tools') or not agent.tools: return multiple_tools = len(agent.tools) > 1 model = agent.canonical_model - for tool_union in agent.tools: + + from ...agents.llm_agent import _convert_tool_union_to_tools + + # Resolve tool_unions in parallel. ``asyncio.gather`` preserves + # input order in the returned list, so the serial commit phase below + # still observes ``agent.tools`` order. If any resolution raises, + # gather cancels the siblings and propagates -- same observable + # behavior as the previous serial loop, which would propagate the + # first exception and abandon the rest. + resolved_tools_per_union = await asyncio.gather(*( + _convert_tool_union_to_tools( + tool_union, + ReadonlyContext(invocation_context), + model, + multiple_tools, + ) + for tool_union in agent.tools + )) + + # Serial commit phase, in original ``agent.tools`` order. Mutations + # to ``llm_request`` and reads of its state (model, config.tools, + # tools_dict) preserve today's ordering semantics exactly. + for tool_union, tools in zip(agent.tools, resolved_tools_per_union): tool_context = ToolContext(invocation_context) # If it's a toolset, process it first @@ -443,15 +474,7 @@ async def _process_agent_tools( tool_context=tool_context, llm_request=llm_request ) - from ...agents.llm_agent import _convert_tool_union_to_tools - # Then process all tools from this tool union - tools = await _convert_tool_union_to_tools( - tool_union, - ReadonlyContext(invocation_context), - model, - multiple_tools, - ) for tool in tools: await tool.process_llm_request( tool_context=tool_context, llm_request=llm_request diff --git a/tests/unittests/flows/llm_flows/test_base_llm_flow.py b/tests/unittests/flows/llm_flows/test_base_llm_flow.py index fff1ec6f1a..abfb6e4c68 100644 --- a/tests/unittests/flows/llm_flows/test_base_llm_flow.py +++ b/tests/unittests/flows/llm_flows/test_base_llm_flow.py @@ -14,6 +14,7 @@ """Unit tests for BaseLlmFlow toolset integration.""" +import asyncio from unittest import mock from unittest.mock import AsyncMock @@ -243,6 +244,130 @@ def _my_tool(sides: int) -> int: ) +@pytest.mark.asyncio +async def test_process_agent_tools_resolves_unions_in_parallel(): + """``_convert_tool_union_to_tools`` is dispatched for every tool_union concurrently. + + Each mocked resolution blocks until ``all_started`` is set; the event + is only set once every call has been entered. If + ``_process_agent_tools`` were still serial, the first call would + block forever waiting for the event the second call hasn't yet + entered to set. + """ + num_tools = 5 + started_count = 0 + all_started = asyncio.Event() + release = asyncio.Event() + + async def blocking_convert(tool_union, *args, **kwargs): + del args, kwargs + nonlocal started_count + started_count += 1 + if started_count == num_tools: + all_started.set() + await release.wait() + return [_AsyncProcessLlmRequestTool(name=tool_union.__name__)] + + def _make_func(i): + def _f(): + """Test function.""" + return i + + _f.__name__ = f'fn_{i}' + return _f + + funcs = [_make_func(i) for i in range(num_tools)] + + with mock.patch( + 'google.adk.agents.llm_agent._convert_tool_union_to_tools', + side_effect=blocking_convert, + ): + agent = Agent(name='test_agent', tools=funcs) + invocation_context = await testing_utils.create_invocation_context( + agent=agent, user_content='test message' + ) + flow = BaseLlmFlowForTesting() + llm_request = LlmRequest() + + async def drive(): + async for _ in flow._preprocess_async(invocation_context, llm_request): + pass + + drive_task = asyncio.create_task(drive()) + try: + # If resolution were serial this would hang; release the gate as + # soon as every coroutine has entered. + await asyncio.wait_for(all_started.wait(), timeout=5.0) + finally: + release.set() + await asyncio.wait_for(drive_task, timeout=5.0) + + assert started_count == num_tools + + +@pytest.mark.asyncio +async def test_process_agent_tools_preserves_order_when_later_unions_resolve_first(): + """``process_llm_request`` is called in original ``agent.tools`` order even when later unions resolve first.""" + + resolution_started_evt = [asyncio.Event(), asyncio.Event()] + process_call_order: list[str] = [] + + async def staggered_convert(tool_union, *args, **kwargs): + del args, kwargs + if tool_union.__name__ == 'fn_slow': + # Resolve only after fn_fast's resolution has completed. + await resolution_started_evt[1].wait() + tool_name = 'slow_tool' + else: + tool_name = 'fast_tool' + resolution_started_evt[1].set() + return [ + _AsyncProcessLlmRequestTool( + name=tool_name, on_process=process_call_order.append + ) + ] + + def fn_slow(): + """Slow-resolving function.""" + return 0 + + def fn_fast(): + """Fast-resolving function.""" + return 0 + + with mock.patch( + 'google.adk.agents.llm_agent._convert_tool_union_to_tools', + side_effect=staggered_convert, + ): + # agent.tools order is [slow, fast]; resolution completes [fast, slow]. + agent = Agent(name='test_agent', tools=[fn_slow, fn_fast]) + invocation_context = await testing_utils.create_invocation_context( + agent=agent, user_content='test message' + ) + flow = BaseLlmFlowForTesting() + llm_request = LlmRequest() + + async for _ in flow._preprocess_async(invocation_context, llm_request): + pass + + # Even though fast_tool was resolved first, process_llm_request must + # be invoked in agent.tools order (slow_tool first). + assert process_call_order == ['slow_tool', 'fast_tool'] + + +class _AsyncProcessLlmRequestTool: + """Minimal stand-in for a BaseTool that records process_llm_request calls.""" + + def __init__(self, name: str, on_process=None): + self.name = name + self._on_process = on_process + + async def process_llm_request(self, *, tool_context, llm_request): + del tool_context, llm_request + if self._on_process is not None: + self._on_process(self.name) + + # TODO(b/448114567): Remove the following # test_handle_after_model_callback_grounding tests once the workaround # is no longer needed. From f33dbf85bf3b51faf28392aeeefffa75523e2d5f Mon Sep 17 00:00:00 2001 From: Luis Tomas Bolivar Date: Fri, 29 May 2026 07:54:15 +0200 Subject: [PATCH 4/9] fix(deps): bump starlette and fastapi to address CVE-2026-48710 Starlette prior to 1.0.1 did not validate the HTTP Host header before reconstructing request.url, allowing a malformed header to bypass security restrictions based on request.url.path. Bump starlette to >=1.0.1 and fastapi to >=0.133.0 (the minimum version compatible with starlette >=1.0.1). Fixes #5893 Merge https://github.com/google/adk-python/pull/5894 Change-Id: If7743e53d95740452c9e562e9bba98d132ae049e --- pyproject.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 657920e2c7..c8f8a86a16 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -36,7 +36,7 @@ dependencies = [ "aiosqlite>=0.21", "authlib>=1.6.6,<2", "click>=8.1.8,<9", - "fastapi>=0.124.1,<1", + "fastapi>=0.133,<1", "google-auth[pyopenssl]>=2.47", "google-genai>=2.4,<3", "graphviz>=0.20.2,<1", @@ -51,7 +51,7 @@ dependencies = [ # go/keep-sorted start "pyyaml>=6.0.2,<7", "requests>=2.32.4,<3", - "starlette>=0.49.1,<1", + "starlette>=1.0.1,<2", "tenacity>=9,<10", "typing-extensions>=4.5,<5", "tzlocal>=5.3,<6", From ec1b9ddd2297e67dff6f72eae302267ee633edc6 Mon Sep 17 00:00:00 2001 From: George Weale Date: Fri, 29 May 2026 20:15:09 +0000 Subject: [PATCH 5/9] fix(agents): restore abc.ABC base for BaseAgent and LlmAgent BaseAgent and LlmAgent re-inherit abc.ABC. v2 GA re-rooted them on BaseNode (plain BaseModel), so static checkers no longer see an ABCMeta base and flag @abc.abstractmethod on consumer subclasses as stray, cascading into bad-return-type. Runtime is unchanged because pydantic's ModelMetaclass already extends ABCMeta; this only makes the metaclass visible to type checkers. Change-Id: I2632a180b0616a244cba97a161c1845101acd998 --- src/google/adk/agents/base_agent.py | 5 ++++- src/google/adk/agents/llm_agent.py | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/google/adk/agents/base_agent.py b/src/google/adk/agents/base_agent.py index b4af1bd276..c21b7a8ee9 100644 --- a/src/google/adk/agents/base_agent.py +++ b/src/google/adk/agents/base_agent.py @@ -14,6 +14,7 @@ from __future__ import annotations +import abc import inspect import logging from typing import Any @@ -87,7 +88,9 @@ class BaseAgentState(BaseModel): AgentState = TypeVar('AgentState', bound=BaseAgentState) -class BaseAgent(BaseNode): +# TODO: drop the explicit abc.ABC base once BaseNode surfaces ABCMeta to +# static type checkers. +class BaseAgent(BaseNode, abc.ABC): """Base class for all agents in Agent Development Kit.""" model_config = ConfigDict( diff --git a/src/google/adk/agents/llm_agent.py b/src/google/adk/agents/llm_agent.py index 7e469b8adc..ee1b05c535 100644 --- a/src/google/adk/agents/llm_agent.py +++ b/src/google/adk/agents/llm_agent.py @@ -14,6 +14,7 @@ from __future__ import annotations +import abc import asyncio import importlib import inspect @@ -192,7 +193,9 @@ async def _convert_tool_union_to_tools( return [] -class LlmAgent(BaseAgent): +# TODO: drop the explicit abc.ABC base once BaseNode surfaces ABCMeta to +# static type checkers. +class LlmAgent(BaseAgent, abc.ABC): """LLM-based Agent.""" DEFAULT_MODEL: ClassVar[str] = 'gemini-3-flash-preview' From 668109ac494dea7126858708ea1cd3113934b937 Mon Sep 17 00:00:00 2001 From: George Weale Date: Fri, 29 May 2026 20:15:13 +0000 Subject: [PATCH 6/9] fix(sessions): guard None event.actions before reading state_delta BaseSessionService guards `event.actions` for None before reading state_delta in _apply_temp_state, _trim_temp_delta_state, and _update_session_state, matching prior behavior for callers that build events with actions unset. InMemorySessionService applies the same guard in append_event. Change-Id: I6598ac68711ebb360f8081c2f895e1dc17942ad5 --- src/google/adk/sessions/base_session_service.py | 6 +++--- src/google/adk/sessions/in_memory_session_service.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/google/adk/sessions/base_session_service.py b/src/google/adk/sessions/base_session_service.py index e3a259e22e..324abe230b 100644 --- a/src/google/adk/sessions/base_session_service.py +++ b/src/google/adk/sessions/base_session_service.py @@ -138,7 +138,7 @@ def _apply_temp_state(self, session: Session, event: Event) -> None: the duration of the current invocation but is NOT persisted to storage (the event delta is trimmed separately by _trim_temp_delta_state). """ - if not event.actions.state_delta: + if not event.actions or not event.actions.state_delta: return for key, value in event.actions.state_delta.items(): if key.startswith(State.TEMP_PREFIX): @@ -151,7 +151,7 @@ def _trim_temp_delta_state(self, event: Event) -> Event: in-memory session state (updated by _apply_temp_state) retains the values for the duration of the current invocation. """ - if not event.actions.state_delta: + if not event.actions or not event.actions.state_delta: return event event.actions.state_delta = { @@ -163,7 +163,7 @@ def _trim_temp_delta_state(self, event: Event) -> Event: def _update_session_state(self, session: Session, event: Event) -> None: """Updates the session state based on the event.""" - if not event.actions.state_delta: + if not event.actions or not event.actions.state_delta: return for key, value in event.actions.state_delta.items(): session.state.update({key: value}) diff --git a/src/google/adk/sessions/in_memory_session_service.py b/src/google/adk/sessions/in_memory_session_service.py index 934e712036..b8f6cfab46 100644 --- a/src/google/adk/sessions/in_memory_session_service.py +++ b/src/google/adk/sessions/in_memory_session_service.py @@ -346,7 +346,7 @@ def _warning(message: str) -> None: storage_session.events.append(event) storage_session.last_update_time = event.timestamp - if event.actions.state_delta: + if event.actions and event.actions.state_delta: state_deltas = _session_util.extract_state_delta( event.actions.state_delta ) From 1b242778ab99742df58d8844bfce1d7eebc84f48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kjell=20Reidar=20J=C3=B8ssang?= Date: Mon, 25 May 2026 23:54:20 +0200 Subject: [PATCH 7/9] fix: exclude temp: state keys from Firestore session writes FirestoreSessionService.append_event() writes the full session.state dict to Firestore, filtering app: and user: prefixed keys but not temp: prefixed keys. This causes crashes when temp state contains non-serializable objects (e.g. GroundingMetadata from GoogleSearchTool via AgentTool's propagate_grounding_metadata). The DatabaseSessionService and SqliteSessionService don't have this issue because they only persist state_deltas extracted via extract_state_delta(), which already filters temp: keys. Fix: add State.TEMP_PREFIX to the exclusion filter in session_only_state. Test: extend existing test_append_event_with_temp_state to assert temp keys are absent from the persisted session state dict. Merge https://github.com/google/adk-python/pull/5841 Change-Id: I3278c205bba7c7e287308c2625112789593bb7ef --- .../integrations/firestore/firestore_session_service.py | 1 + .../firestore/test_firestore_session_service.py | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/src/google/adk/integrations/firestore/firestore_session_service.py b/src/google/adk/integrations/firestore/firestore_session_service.py index 83b97c33c2..e90753be5c 100644 --- a/src/google/adk/integrations/firestore/firestore_session_service.py +++ b/src/google/adk/integrations/firestore/firestore_session_service.py @@ -551,6 +551,7 @@ async def _append_txn(transaction: firestore.AsyncTransaction) -> int: for k, v in session.state.items() if not k.startswith(State.APP_PREFIX) and not k.startswith(State.USER_PREFIX) + and not k.startswith(State.TEMP_PREFIX) } transaction.update( session_ref, diff --git a/tests/unittests/integrations/firestore/test_firestore_session_service.py b/tests/unittests/integrations/firestore/test_firestore_session_service.py index 1445bfe0ef..4d202e1500 100644 --- a/tests/unittests/integrations/firestore/test_firestore_session_service.py +++ b/tests/unittests/integrations/firestore/test_firestore_session_service.py @@ -386,6 +386,15 @@ async def test_append_event_with_temp_state(mock_firestore_client): assert "temp:k1" not in event_data["actions"]["state_delta"] assert event_data["actions"]["state_delta"]["session_key"] == "session_val" + # 3. Verify temp keys are NOT written to session state in Firestore + transaction.update.assert_called_once() + update_args, _ = transaction.update.call_args + persisted_state = update_args[1]["state"] + assert ( + "temp:k1" not in persisted_state + ), "temp: keys must not be persisted to Firestore session state" + assert "session_key" in persisted_state + @pytest.mark.asyncio async def test_list_sessions_with_user_id(mock_firestore_client): From aabc60caa66a1c89f41d9afbb0b788776bd62675 Mon Sep 17 00:00:00 2001 From: Bo Yang Date: Fri, 29 May 2026 14:32:27 -0700 Subject: [PATCH 8/9] feat(skills): Enforce PR assignment gates and stream metadata via stdout Verify pull request assignment against the active GitHub user to prevent concurrent triage overhead and restrict local checkout to the assigned owner. Additionally, stream the fetched PR metadata directly via stdout instead of writing to a local workspace cache file to prevent disk clutter and simplify assistant JSON parsing. Change-Id: I9408fba5c200cca5814afb7223302eb849c0b319 --- .agents/skills/adk-pr-triage/SKILL.md | 141 +++++++----------- .../skills/adk-pr-triage/scripts/triage_pr.py | 95 ++++++++++-- 2 files changed, 140 insertions(+), 96 deletions(-) diff --git a/.agents/skills/adk-pr-triage/SKILL.md b/.agents/skills/adk-pr-triage/SKILL.md index 9475a22036..30ef295e6e 100644 --- a/.agents/skills/adk-pr-triage/SKILL.md +++ b/.agents/skills/adk-pr-triage/SKILL.md @@ -2,74 +2,96 @@ name: adk-pr-triage description: Analyze and triage GitHub pull requests for the adk-python repository. The user provides a PR number or URL, and the skill performs an evaluation on the PR's objectives, legitimacy, alignment with ADK's principles (including API stability, package self-containment, explicit exports, styling and naming conventions), and asks the user whether to push back on the PR or perform a local review (checking out, rebasing, and running adk-review before pushing to Gerrit). Triggers on "triage pr", "pr triage", "review pr", "pr review", "pull request", "github.com/google/adk-python/pull/". --- - # ADK Pull Request Triage (adk-pr-triage) - This skill guides AI assistants in conducting a highly professional, rigorous, and constructive triage of GitHub pull requests (PRs) submitted to the `google/adk-python` repository. It parses the PR, retrieves its context, evaluates it against ADK's design, style, and testing principles, presents a premium analysis report, and authors tailored response feedback (such as structured push-back or approval comments) under direct user guidance. - > [!IMPORTANT] -> ## CRITICAL EXECUTION RULE: STOP AND ASK DECISION GATE -> * **Phase 1 (PR Analysis) is strictly read-only**: Do NOT create branches, modify workspace files, or post any comments in your first response. -> * **STOP and ask**: You must present your full in-depth PR review report first, and explicitly ask the user: -> > "Would you like me to push back on this pull request? (If yes, select one of the push-back reasons or write custom feedback, and I will author a professional and precise review message for you to review. If no, I will draft an approval response highlighting the positive aspects of the implementation.)" -> * **Wait for Instructions**: Only author and present feedback drafts in a subsequent turn *after* the user provides their decision. - +> ## CRITICAL EXECUTION RULES: STOP AND ASK DECISION GATES +> 1. **MANDATORY PR ASSIGNMENT BLOCK GATE**: +> * BEFORE doing any metadata reading, diff-fetching, issue-viewing, or code analysis (Phase 1, Step 1.3 onwards), you MUST verify if the pull request is assigned to you (via the verification script). + +> * If the PR is NOT assigned to you: +> * **STOP calling tools and ask immediately**: You must present the PR Assignment Block gate in your chat response: +> > "Pull Request # is NOT assigned to you. (Current assignees: ). Would you like to take over this Pull Request?" +> * **Wait for Instructions**: Do NOT perform any code analysis or diff-fetching in this turn. +> * **Action Paths**: +> * **Yes (Take Over)**: In your next turn, run the assignment command: +> ```bash +> gh pr edit --add-assignee "@me" --repo google/adk-python +> ``` +> **CRITICAL**: Immediately after assigning, you MUST re-run the verification script to refresh the PR metadata with the updated assignees: +> ```bash +> .venv/bin/python .agents/skills/adk-pr-triage/scripts/triage_pr.py --skip-update +> ``` +> Then parse the updated PR details from the script's stdout and proceed with the remaining triage steps. +> * **No (Decline)**: **Stop executing immediately** and do not run any further tools or operations. State that triage has terminated. +> 2. **PR Analysis is strictly read-only**: Do NOT create branches, modify workspace files, or post any comments in your first response (unless performing PR assignment under the takeover gate above). +> 3. **Triage Decision Gate**: You must present your full in-depth PR review report first, and explicitly ask the user: +> > "Would you like me to push back on this pull request? (If yes, select one of the push-back reasons or write custom feedback, and I will author a professional and precise review message for you to review. If no, I will draft an approval response highlighting the positive aspects of the implementation.)" +> Wait for instructions before performing any branch creation or Gerrit push. --- - ## Phase 1: Retrieve and Parse the PR & Linked Context (Read-Only) - -### Step 1: Extract PR Identifier & Verify CLA Signature (Mandatory Entry Gate) +### Step 1: Extract PR Identifier, Verify CLA Signature & PR Assignment (Mandatory Entry Gate) 1. **Identify the PR identifier**: Parse the PR number or URL from the prompt (e.g., `https://github.com/google/adk-python/pull/5885` -> `5885`). -2. **CRITICAL COMPLIANCE GATE - Run CLA Verification Script**: - * **Rule**: BEFORE doing any further work, diff reading, or analysis, you MUST run the compliance helper script in read-only mode to verify the contributor's Contributor License Agreement (CLA) signature: +2. **CRITICAL COMPLIANCE & ASSIGNMENT GATES - Run Verification Script**: + * **Rule**: BEFORE doing any further work, diff reading, or analysis, you MUST run the verification helper script in read-only mode to verify the contributor's Contributor License Agreement (CLA) signature and check PR assignment: ```bash .venv/bin/python .agents/skills/adk-pr-triage/scripts/triage_pr.py --skip-update ``` - * **Inspect the Exit Status**: + * **Inspect the Exit Status & Verification Output**: * **Exit Code 2 (Refusal)**: The contributor HAS NOT signed the Google CLA. You **MUST absolutely refuse** to perform any analysis, triage, diff-fetching, checking out, or workspace operations. Stop calling tools immediately and print a clear compliance refusal message stating that the Google CLA is not signed. - * **Exit Code 0 (Success)**: The Google CLA is verified. Proceed with the triage steps. -3. **Fetch PR details**: Use the `gh` CLI tool to fetch pull request metadata in JSON format: - ```bash - gh pr view --repo google/adk-python --json number,title,body,state,url,author,additions,deletions,changedFiles,labels - ``` -4. **Locate linked issue(s)**: Review the PR body or closing references (e.g., `Fixes #5882`, `Resolves #5882`). If an issue is linked, fetch that issue's details to understand the original problem statement: + * **Exit Code 0 (Success)**: The Google CLA is verified. Proceed. + * **Verify PR Assignment Status**: Parse the script output to check if the Pull Request is assigned to you (the user running the skill). + - **PR IS NOT ASSIGNED TO YOU**: You **MUST stop calling tools immediately**, present the following assignment block decision gate in your chat response, and wait for the user's input: + > "⚠️ **Pull Request Assignment Block** + > Pull Request # is NOT assigned to you. (Current assignees: ). + > + > **Would you like to take over this Pull Request?** + > - **[Option 1]**: **Yes, take over Pull Request #** (Assign the PR to myself and proceed with the triage analysis). + > - **[Option 2]**: **No, do not take over** (Stop executing)." + - **If the user chooses Option 1**: Run the assignment command: + ```bash + gh pr edit --add-assignee "@me" --repo google/adk-python + ``` + **CRITICAL**: Immediately after assigning, you MUST re-run the verification helper script to fetch the updated metadata from GitHub and refresh the cached details: + ```bash + .venv/bin/python .agents/skills/adk-pr-triage/scripts/triage_pr.py --skip-update + ``` + Then proceed to parse the updated PR details from the script's stdout in Step 1.3 and continue standard triage. + + + - **If the user chooses Option 2 (or declines)**: **Stop executing immediately** and do not run any further tools or operations. State that triage has terminated. + - **PR IS ALREADY ASSIGNED TO YOU**: Proceed directly with Step 1.3. +3. **Parse PR Details from Script Output**: The verification script in Step 2 outputs the complete PR details JSON directly to standard output, wrapped in `[PR_METADATA_JSON]` and `[/PR_METADATA_JSON]` tags. Do NOT write to or read from local cache files, and do NOT make separate network commands to fetch PR details. Parse the JSON metadata directly from the command's stdout: + * **Key JSON Attributes**: `number`, `title`, `body`, `state`, `url`, `author`, `additions`, `deletions`, `changedFiles`, `labels`, `assignees`, `closingIssuesReferences` (used to locate linked issues). +4. **Locate and Fetch Linked Issue(s)**: Extract linked closing issues directly from the `closingIssuesReferences` array in the parsed JSON metadata from the script's stdout. If any closing issues are linked, fetch their details to understand the original problem statement: ```bash gh issue view --repo google/adk-python --json number,title,body,state ``` - ### Step 2: Retrieve the Complete Diff 1. **Fetch pull request changes**: Run the `gh pr diff` command to view the actual line-by-line diff of the PR: ```bash gh pr diff --repo google/adk-python ``` 2. **Review files modified**: Match the diff segments against existing repository files to identify the target components under review. - --- - ## Phase 2: Deep Code & Architectural Analysis (Read-Only) - Conduct an extremely thorough review of the changes by examining the diff and analyzing the local codebase. You must address the following three critical dimensions and organize your findings in a premium **PR Review Report**: - ### 1. Objectives & Impact ("What issue does it fix, or feature does it introduce?") - **Core Change Summary**: Define what the code modifications do, where they are applied (classes, methods, functions), and the execution flow involved. - **Problem Resolution**: Confirm how the implementation fixes the linked issue or introduces the target feature. - **Context Tracing**: Trace the execution flow in the active workspace and explain what modules are impacted by this patch. - ### 2. Legitimacy & Value ("Is it a legitimate issue or a useful feature?") - **Codebase Verification**: Verify the bug/gap exists in the baseline code by searching the local workspace using `grep_search` and inspecting target files with `view_file`. - **Aesthetic & Structural Value**: Analyze whether the problem represents a legitimate, high-priority bug (e.g., causing hangs, memory leaks, or incorrect API validation) or if the feature adds actual, tangible utility to ADK developers. - **Alternatives Assessment**: Assess if the PR's solution is the most elegant one, or if there is a cleaner, less intrusive, or more robust alternative pattern (e.g., utilizing an existing helper instead of introducing duplicate logic). - ### 3. Architectural & Principle Alignment ("Does it align with ADK's principles?") Evaluate the implementation against the established architectural, style, and testing guidelines. Use direct file links to code reference examples. - #### A. Public API and Visibility Principles - **API Stability**: Does the change introduce a breaking change to any public classes, methods, or CLI structures in the `google.adk` namespace? (Breaking changes are unacceptable under Semantic Versioning without an official deprecation cycle). - **Module and File Naming**: Are new `.py` module files under `src/google/adk/` private by default (prefixed with a leading underscore, e.g., `_my_module.py`)? - **Explicit Exports**: Are new public symbols explicitly exposed via the package's `__init__.py` using the `__all__` list? Are internal helper classes and on-wire objects kept internal by omitting them from `__all__`? - **Self-Containment**: Does inside-framework code import from the subsystem's specific module directly, rather than importing from `__init__.py`? (Within ADK, framework-level imports from `__init__.py` are strictly prohibited to avoid circular dependencies and maintain clean encapsulation). - **Intuitive Naming**: Are public methods and class names concise (e.g., `Runner.run`), while private/internal methods are descriptive (e.g., `_validate_chat_agent_wiring`)? - #### B. Code Quality, Style & Pythonic Conventions - **Future Annotations**: Does every new or heavily edited python source file include `from __future__ import annotations` immediately after the license header? - **Strong Typing**: Are type hints used for all function arguments and return values? Is the use of `Any` avoided in favor of precise types, abstract interfaces, or generics? @@ -84,19 +106,14 @@ Evaluate the implementation against the established architectural, style, and te - Are internal mutable states declared with `PrivateAttr()` and constructor logic mapped in `model_post_init()`? - **Lazy Logging**: Does logging utilize lazy-evaluated `%`-based templates rather than eager `f-strings`? (e.g., `logging.info("Completed in %s ms", duration)` is correct; `logging.info(f"Completed in {duration} ms")` is a violation). - **Error Handling**: Are specific exceptions caught with context, avoiding bare `except:` constructs? - #### C. Test Integrity & Verification Quality - **Behavior-Focused Testing**: Do the new unit or integration tests under `tests/` target public boundaries rather than internal execution states? - **No Mocking of Core Components**: Are real ADK modules (`BaseNode`, `Event`, `Context`) used, restricting mocking exclusively to external web or network dependencies? - **Minimal Fixtures & Locality**: Are test helper classes and fixtures kept close to the test functions (defined inline inside the test function when utilized by a single test) to improve discoverability? - **Structure**: Do tests follow the clean **Arrange-Act-Assert** pattern separated by clear logical blocks? - --- - ## Phase 3: Stop and Ask for Push-Back or Local Review (Interactive Gate) - Present the completed analysis report in your response. Follow the **PR Review Report Template** below for a highly premium, readable presentation. - ### The Interactive Gate Callout At the end of your report, stop calling tools and output this explicit message: > "### 🛑 Review Decision Gate @@ -105,41 +122,33 @@ At the end of your report, stop calling tools and output this explicit message: > **How would you like to proceed with this Pull Request?** > - **[Option 1]**: **Push Back** (Draft a professional, constructive feedback response with recommendations for the author). > - **[Option 2]**: **Local Review** (Checkout the PR locally, rebase onto the latest main, and run the `/adk-review` skill to thoroughly verify and polish before pushing to Gerrit)." - --- - ## Phase 4: Action Execution (Subsequent Turn) - Once the user provides their decision, perform the tailored operations in your subsequent turns: - ### Branch A: Push Back 1. **Analyze the Push-Back Focus**: Read the user's specific feedback or selected points of concern. 2. **Draft Constructive Feedback**: Author a highly structured, objective, and supportive response that teaches the contributor while insisting on quality. 3. **Include Concrete Recommendations**: Quote specific files/lines in their diff and provide complete, refactored code blocks in your comments so they can easily apply the fixes. Reference the relevant ADK style guides. 4. **Present the Draft**: Format your draft using the **GitHub Review Draft Template** below. - ### Branch B: Local Review (Checkout & Revise) If the user selects **Local Review**, run the following structured sequence: - 1. **Step 0: Update the PR Head Branch on GitHub (Mandatory Sync)**: * **Rule**: BEFORE downloading or checking out the pull request locally, you MUST trigger an update on the remote GitHub pull request to align it with the latest remote base branch (`main`). * Run the verification & sync helper script to update the branch: ```bash - .venv/bin/python .agents/skills/adk-pr-triage/scripts/triage_pr.py [pr_number] + .venv/bin/python .agents/skills/adk-pr-triage/scripts/triage_pr.py ``` * *What it does*: This script automatically checks the Google CLA signature status again, attempts to update the PR branch on GitHub by rebasing onto `main`, and if rebase-update is blocked, falls back to updating via a merge commit. It handles all outputs and fallbacks gracefully. - 2. **Step 1: Checkout the PR to a Local Branch**: - * Branch naming convention: `pr-triage-[pr_number]-[short_desc]` (e.g. `pr-triage-5875-parallelize-tool-union`). + * Branch naming convention: `pr-triage--[short_desc]` (e.g. `pr-triage-5875-parallelize-tool-union`). * Fetch the pull request ref directly from the remote GitHub endpoint: ```bash - git fetch https://github.com/google/adk-python.git pull/[pr_number]/head:pr-triage-[pr_number]-[short_desc] + git fetch https://github.com/google/adk-python.git pull//head:pr-triage--[short_desc] ``` * Checkout to the newly created local branch: ```bash - git checkout pr-triage-[pr_number]-[short_desc] + git checkout pr-triage--[short_desc] ``` - 3. **Step 2: Preserve the Commit Message & Append Merge Reference**: * **CRITICAL**: You MUST preserve the exact same commit message from the pull request! * Determine if the PR contains a single commit or multiple commits: @@ -154,21 +163,17 @@ If the user selects **Local Review**, run the following structured sequence: * Append `"Merge "` to the very end of the commit message (separated by a blank line). Use this elegant shell command to do it in one-shot: ```bash git commit --amend -m "$(git log -1 --pretty=%B) - - Merge https://github.com/google/adk-python/pull/[pr_number]" + Merge https://github.com/google/adk-python/pull/" ``` * *Note*: When you run git commit/amend, the Gerrit `commit-msg` hook will automatically execute and append the `Change-Id:` footprint if not already present. - 4. **Step 3: Rebase on top of Main**: * Run the rebase command to place the CL commit on top of the latest local remote tracking `main` branch: ```bash git rebase origin/main ``` - 5. **Step 4: Execute Code Verification & Polishing**: * Trigger the local review process by invoking the **`/adk-review`** skill! * Follow its comprehensive guidelines to audit edge cases, style compliance, dependencies, and test validation. Work in partnership with the user to revise the local changes as needed. - 6. **Step 5: Squash User Revisions & Push to Gerrit**: * If the user requests to push to Gerrit, squash/amend all local workspace revisions into the single original commit: * **CRITICAL**: You MUST preserve the exact same commit message, including the `Merge ` footer and the original `Change-Id:` footer. Do NOT change it. @@ -180,36 +185,27 @@ If the user selects **Local Review**, run the following structured sequence: ```bash git push origin HEAD:refs/for/main ``` - --- - ## PR Review Report Template - Present the initial analysis using the following structured format: - ```markdown # 🔍 ADK Pull Request Review: PR # **Title**: **Author**: @ **Status**: `` **Impact**: ` additions`, ` deletions` across ` files` - ## Detailed Findings & Analysis - ### 1. Objectives & Impact ("What does it do?") - **Context & Background**: [Briefly explain the background and the problem it targets. Reference linked Issue # using markdown links if available] - **Implementation Mechanism**: [Detail precisely which modules are modified and how the execution flow is altered] - **Affected Surface**: [Highlight any changes to public classes, CLI interfaces, state models, or setup pipelines] - ### 2. Legitimacy & Value ("Is it a valid and useful change?") - **Workspace Verification**: - Investigated current workspace files: [file_name.py](file:///absolute/path/to/src/google/adk/...#L123-L145) (using `view_file` / `grep_search`). - Found that: [Describe the baseline condition that proves the bug exists or the feature is missing] - **Value Assessment**: [Explain why this is a good addition. Does it solve a genuine real-world developer problem, improve performance, or prevent resources leaks?] - **Alternative Approaches**: [Evaluate if there is an alternative implementation path. Did the author choose the cleanest design?] - ### 3. Principle & Style Alignment Checklist ("Does it follow rules?") - * **Public API & Visibility Boundaries**: * *Status*: [Pass / Fail / N/A] * *Analysis*: [Check for breaking changes, private module conventions `_`, and explicit exports in `__init__.py` using `__all__`] @@ -222,43 +218,29 @@ Present the initial analysis using the following structured format: * **Test Integrity & Quality**: * *Status*: [Pass / Fail / N/A] * *Analysis*: [Check coverage, testing through public interfaces, minimal inline fixtures, and Arrange-Act-Assert formatting] - --- - ## Executive Summary 1. **Core Objective**: [Briefly summarize what issue is fixed or feature is introduced] 2. **Legitimacy & Value**: [Legitimate Fix / Valuable Feature / Duplicate / Redundant] - [1-sentence explanation] 3. **Alignment with Principles**: [Pass / Pass with Nits / Major Changes Required] - [1-sentence architecture alignment summary] 4. **Recommendation**: [Approve / Approve with Nits / Push Back (Request Changes)] - --- - ### 🛑 Review Decision Gate I have completed my in-depth analysis of Pull Request #. Please review the findings above. - **How would you like to proceed with this Pull Request?** - **[Option 1]**: **Push Back** (Draft a professional, constructive feedback response with recommendations for the author). - **[Option 2]**: **Local Review** (Checkout the PR locally under `pr-triage-[pr_number]-[short_desc]`, rebase onto the latest main, and run the `/adk-review` skill to thoroughly verify and polish before pushing to Gerrit)." ``` - --- - ## GitHub Review Draft Template - Format the authored review response as a premium markdown snippet block: - ````markdown # 💬 GitHub PR Review Draft Message *Copy and paste this response directly into the GitHub review interface:* - --- - ### PR Review: - Hello @! Thank you very much for contributing this pull request to improve ADK. I've conducted a thorough architectural and style review of your implementation against our design guidelines and standards. - Here is the feedback and a few suggested changes to align your patch with ADK's principles: - #### 🔴 Major Concerns / Blocks 1. **[Concern 1 Title, e.g., Import from init.py is not allowed]** - **Target Code**: [filename.py:L100-L105](file:///absolute/path/to/src/google/adk/file_name.py#L100-L105) @@ -267,11 +249,9 @@ Here is the feedback and a few suggested changes to align your patch with ADK's ```python # Provide full, drop-in replacement code block ``` - 2. **[Concern 2 Title, e.g., Missing Unit Tests for Edge Cases]** - **Target Code**: [test_filename.py](file:///absolute/path/to/tests/unittests/test_filename.py) - **Issue**: [Detail what is missing, e.g., "We need verification coverage of boundaries like empty string and negative values."] - #### 🟡 Style & Quality Nits 1. **[Style Nit, e.g., Eager Logging formatting]** - **Target Code**: [filename.py:L42](file:///absolute/path/to/src/google/adk/file_name.py#L42) @@ -280,23 +260,16 @@ Here is the feedback and a few suggested changes to align your patch with ADK's # Corrected: logging.info("User registered: %s", user_id) ``` - 2. **[Typing Nit, e.g., Optional[X] instead of X | None]** - **Target Code**: [filename.py:L15](file:///absolute/path/to/src/google/adk/file_name.py#L15) - **Suggestion**: Prefer more concise union type hint `X | None`. - #### 🟢 Positive Aspects - [Highlight stellar work, e.g., "Excellent Pydantic v2 validation logic!" or "Highly readable and clean docstrings!"] - Please let me know if you have any questions on these suggestions, and let's work together to get this PR merged! ```` - --- - ## Tips & Best Practices - > [!TIP] > Always verify the baseline behavior in your active workspace before claiming something is a bug or invalid. Reading the current source files using `view_file` gives you full context. - > [!IMPORTANT] > When referencing files and line numbers in your reports and draft reviews, always use clickable markdown file links of format `[filename.py](file:///absolute/path/to/file#L100-L120)` without surrounding backticks around the brackets. Ensure the links represent valid absolute file paths in the local workspace. diff --git a/.agents/skills/adk-pr-triage/scripts/triage_pr.py b/.agents/skills/adk-pr-triage/scripts/triage_pr.py index 9e3d1425d1..f624eac69c 100755 --- a/.agents/skills/adk-pr-triage/scripts/triage_pr.py +++ b/.agents/skills/adk-pr-triage/scripts/triage_pr.py @@ -32,9 +32,9 @@ def run_command(args: list[str]) -> tuple[int, str, str]: return -1, "", str(e) -def verify_cla(pr_number: str) -> bool: - """Verifies if the Google CLA is signed for the given PR number.""" - print(f"[*] Fetching status checks for PR #{pr_number}...") +def fetch_pr_data(pr_number: str) -> dict | None: + """Fetches all PR metadata in one shot from GitHub.""" + print(f"[*] Fetching PR #{pr_number} metadata from GitHub...") cmd = [ "gh", "pr", @@ -43,7 +43,21 @@ def verify_cla(pr_number: str) -> bool: "--repo", "google/adk-python", "--json", - "statusCheckRollup", + ",".join([ + "number", + "title", + "body", + "state", + "url", + "author", + "additions", + "deletions", + "changedFiles", + "labels", + "statusCheckRollup", + "assignees", + "closingIssuesReferences", + ]), ] code, stdout, stderr = run_command(cmd) if code != 0: @@ -51,15 +65,17 @@ def verify_cla(pr_number: str) -> bool: f"Error: Failed to fetch PR details from GitHub: {stderr}", file=sys.stderr, ) - sys.exit(1) - + return None try: - data = json.loads(stdout) + return json.loads(stdout) except json.JSONDecodeError: print("Error: Failed to parse GitHub API JSON response.", file=sys.stderr) - sys.exit(1) + return None - status_checks = data.get("statusCheckRollup", []) + +def verify_cla(pr_data: dict) -> bool: + """Verifies if the Google CLA is signed using cached PR data.""" + status_checks = pr_data.get("statusCheckRollup") or [] cla_check = None for check in status_checks: if check.get("name") == "cla/google": @@ -109,6 +125,47 @@ def verify_cla(pr_number: str) -> bool: return True +def get_current_user() -> str | None: + """Fetches the login name of the current authenticated GitHub user.""" + cmd = ["gh", "api", "user", "-q", ".login"] + code, stdout, stderr = run_command(cmd) + if code != 0: + return None + return stdout.strip() + + +def verify_pr_assignment(pr_data: dict, pr_number: str) -> bool: + """Checks if the PR is assigned to the current user using cached PR data.""" + print(f"\n[*] Verifying assignment for PR #{pr_number}...") + + # Fetch the current logged in user + current_user = get_current_user() + if not current_user: + print( + "Warning: Could not determine current GitHub user. Skipping assignment" + " check." + ) + return True + + print(f"[*] Current GitHub user: {current_user}") + + assignees = pr_data.get("assignees") or [] + assignee_logins = [a.get("login") for a in assignees if a.get("login")] + + if current_user in assignee_logins: + print(f"✅ Pull Request #{pr_number} is assigned to you.") + return True + + assignees_str = ", ".join(assignee_logins) if assignee_logins else "None" + print( + f"⚠️ WARNING: Pull Request #{pr_number} is NOT assigned to you!" + f" Current assignees: {assignees_str}" + ) + print("\n[!] ACTION REQUIRED: The Pull Request is not assigned to you.") + print(" Please ask the user if they want to take over the PR.") + return False + + def update_pr_branch(pr_number: str) -> None: """Updates the remote PR branch with the latest changes from the base branch.""" print( @@ -177,11 +234,25 @@ def main() -> None: ) args = parser.parse_args() - # Step 1: Verify CLA - if not verify_cla(args.pr_number): + # Step 0: Fetch PR data in one-shot + pr_data = fetch_pr_data(args.pr_number) + if not pr_data: + sys.exit(1) + + # Step 1: Verify CLA using cached PR data + if not verify_cla(pr_data): sys.exit(2) # Exit code 2 indicates compliance refusal - # Step 2: Update branch + # Step 2: Output the PR metadata JSON directly to standard output + print("\n[PR_METADATA_JSON]") + print(json.dumps(pr_data, indent=2)) + print("[/PR_METADATA_JSON]") + + # Step 3: Verify PR Assignment using cached PR data + if not verify_pr_assignment(pr_data, args.pr_number): + sys.exit(3) # Exit code 3 indicates assignment block + + # Step 4: Update branch if not args.skip_update: update_pr_branch(args.pr_number) From 093bfa39dd83a7c989a32558614efb3f20ad3a93 Mon Sep 17 00:00:00 2001 From: settler-av Date: Mon, 1 Jun 2026 07:16:02 +0000 Subject: [PATCH 9/9] refactor(llm_flows): streamline agent transfer logic and clean up test formatting --- src/google/adk/flows/llm_flows/request_confirmation.py | 7 +++---- .../unittests/flows/llm_flows/test_request_confirmation.py | 4 +--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/google/adk/flows/llm_flows/request_confirmation.py b/src/google/adk/flows/llm_flows/request_confirmation.py index 442827e42f..dc7d4e2db6 100644 --- a/src/google/adk/flows/llm_flows/request_confirmation.py +++ b/src/google/adk/flows/llm_flows/request_confirmation.py @@ -28,11 +28,10 @@ from ...events.event import Event from ...models.llm_request import LlmRequest from ...tools.tool_confirmation import ToolConfirmation +from ...tools.transfer_to_agent_tool import TransferToAgentTool from ._base_llm_processor import BaseLlmRequestProcessor -from .functions import REQUEST_CONFIRMATION_FUNCTION_CALL_NAME from .agent_transfer import _get_transfer_targets -from ...tools.transfer_to_agent_tool import TransferToAgentTool - +from .functions import REQUEST_CONFIRMATION_FUNCTION_CALL_NAME if TYPE_CHECKING: from ...agents.llm_agent import LlmAgent @@ -177,7 +176,7 @@ async def run_async( ) } - if hasattr(agent, 'disallow_transfer_to_parent'): + if isinstance(agent, LlmAgent): transfer_targets = _get_transfer_targets(agent) if transfer_targets: transfer_tool = TransferToAgentTool( diff --git a/tests/unittests/flows/llm_flows/test_request_confirmation.py b/tests/unittests/flows/llm_flows/test_request_confirmation.py index 88638ed51e..557172f574 100644 --- a/tests/unittests/flows/llm_flows/test_request_confirmation.py +++ b/tests/unittests/flows/llm_flows/test_request_confirmation.py @@ -421,9 +421,7 @@ async def test_request_confirmation_transfer_to_agent_rejected(): ) llm_request = LlmRequest() - agent_event, user_event = _build_transfer_confirmation_events( - confirmed=False - ) + agent_event, user_event = _build_transfer_confirmation_events(confirmed=False) invocation_context.session.events.append(agent_event) invocation_context.session.events.append(user_event)