Skip to content

fix: load artifacts from workflow text responses#5969

Closed
he-yufeng wants to merge 4 commits into
google:mainfrom
he-yufeng:fix/load-artifacts-text-response
Closed

fix: load artifacts from workflow text responses#5969
he-yufeng wants to merge 4 commits into
google:mainfrom
he-yufeng:fix/load-artifacts-text-response

Conversation

@he-yufeng

@he-yufeng he-yufeng commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • support load_artifacts results that were converted into workflow context text
  • scan all parts in the latest content turn instead of assuming the first part is the function response
  • add regressions for workflow text responses and non-first function responses

Fixes #5967.

Verified

  • $env:PYTHONPATH=(Resolve-Path src).Path; python -m pytest tests\unittests\tools\test_load_artifacts_tool.py -q -> 9 passed, 5 warnings
  • python -m pyink --check src\google\adk\tools\load_artifacts_tool.py tests\unittests\tools\test_load_artifacts_tool.py -> 2 files would be left unchanged
  • $env:PYTHONPATH=(Resolve-Path src).Path; python -m py_compile src\google\adk\tools\load_artifacts_tool.py tests\unittests\tools\test_load_artifacts_tool.py -> passed
  • git diff --check upstream/main..HEAD -> passed

Regression exercised

The new tests cover a workflow text response such as [workflow_node] `load_artifacts` tool returned result: {'artifact_names': ['invoice.txt'], 'status': 'ok'} and a turn where the load_artifacts function response is not the first part. In both cases the request receives a new artifact content turn before the model answers.

@adk-bot adk-bot added the tools [Component] This issue is related to tools label Jun 4, 2026
@adk-bot

adk-bot commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Response from ADK Triaging Agent

Hello @he-yufeng, thank you for creating this PR!

We noticed that a few requirements from our contribution guidelines are missing:

  1. Summary of passed unit tests: Please run your tests and include a summary of the passed pytest results in your PR description.
  2. Logs or screenshots: Please provide execution logs or screenshots showing the tool in action after applying the fix to help reviewers better understand the change.

Providing these details will help the maintainers review and merge your PR more efficiently. Thank you!

@he-yufeng he-yufeng force-pushed the fix/load-artifacts-text-response branch from 1a40481 to 202acdc Compare June 5, 2026 05:24
@adk-bot

adk-bot commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

I have completed the read-only triage and analysis workflow for PR #5969 resolving workflow tool loops as requested.

The detailed findings, architectural audit, and compliance evaluation have been compiled into a premium pull request analysis report which you can access directly here:

📄 PR Analysis Report

  • Report Location: analysis_results.md

Key Summary & Next Steps

  1. Google CLA Verification: Verified successfully. The contributor's signature check has been completed and marked as SUCCESS.
  2. Technical Quality: Excellent code quality. The solution securely uses ast.literal_eval to safely parse workflow stringified representations without invoking code. Correctly handles multiple function response parts through recursive matching of isinstance.
  3. Recommendation: Approve.
  4. Action Needed: If you are satisfied with this analysis, you can proceed to merge the PR or let me know if you would like me to trigger integration tasks or further workspace steps.

@rohityan rohityan self-assigned this Jun 5, 2026
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Rechecked the branch locally on current main. The PR description already includes the regression and verification details; current local validation is:

  • python -m py_compile src/google/adk/tools/load_artifacts_tool.py tests/unittests/tools/test_load_artifacts_tool.py
  • PYTHONPATH=src C:\dev\GITHUB-clean\adk-python\.venv\Scripts\python.exe -m pytest -q tests/unittests/tools/test_load_artifacts_tool.py --basetemp .tmp\pytest-5969 -p no:cacheprovider (9 passed)
  • uv run pyink --check src/google/adk/tools/load_artifacts_tool.py tests/unittests/tools/test_load_artifacts_tool.py
  • git diff --check upstream/main..HEAD

@he-yufeng

Copy link
Copy Markdown
Contributor Author

I rechecked the current red checks on this branch.

They do not look caused by this PR diff:

  • pre-commit --all-files is touching upstream files outside this PR's diff. The same .github/workflows/release-update-adk-web.yaml on google:main currently has name: Release: Update ADk Web, which check-yaml rejects at line 1, column 14; the generated browser chunk whitespace is also present on google:main.
  • The failing unit tests are in tests/unittests/cli/test_fast_api.py and fail with missing Google Application Default Credentials, unrelated to the artifact loading fix.
  • pr-analyze, check-changes, check-file-contents, mypy-diff, CLA, and header-check are green.

I am leaving the PR code unchanged for now to avoid mixing unrelated main-branch CI cleanup into this focused fix.

@he-yufeng he-yufeng force-pushed the fix/load-artifacts-text-response branch from 202acdc to d2f49e6 Compare June 9, 2026 10:51
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Rebuilt the branch cleanly on current main, which includes the recent ADC test fix, and kept the PR diff to the two artifact-loading files. Focused tests pass (9 tests), plus ruff, pyink, py_compile, and diff check.

@adk-bot

adk-bot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

I will read the instruction file for the adk-pr-analyze skill before proceeding, to make sure I follow the exact steps and guidelines defined for read-only pull request analysis.I will run the CLA verification script for PR 5969 using the helper script to verify compliance before reviewing the code.I will fetch the details of GitHub Pull Request 5969 directly using the API tool.I will fetch details of the linked issue #5967 using the specific Custom API tool to understand the background and problem.I will fetch the line-by-line diff for PR 5969 using the gh tool command.I will view the load_artifacts_tool.py file in the local workspace to inspect its current implementation and verify the context of the PR changes.I will view the test_load_artifacts_tool.py test file in the workspace to verify the baseline tests and ensure the new test additions align with the existing style.I will view the rest of the test file to see if the new tests were added properly.I will write the comprehensive PR Analysis Report for PR #5969 into the artifact file analysis_results.md located in the artifacts directory.I will run a local dry-run of the artifact tool tests to verify test execution in the active workspace.I have successfully completed the read-only audit and comprehensive architectural analysis of PR #5969 addressing formatting loops inside multi-agent workflow nodes.

The complete premium report has been compiled and saved as an artifact which you can view here:

  • analysis_results.md

Key Decisions and Open Questions

  • Recommendation: Approve. The implementation is secure, well-typed, backward-compatible, and solves a high-priority bug safely using ast.literal_eval.
  • Merge and Cleanup: Since the contributor recently rebased the PR on google:main and verified that all 9 unit tests pass successfully, the patch is clean, fully isolated, and ready to be merged. Let me know if you would like me to assist with merging the PR or analyzing another workflow component.

Summary of Work Done

  1. Context Scoping: Retrieved details of both PR fix: load artifacts from workflow text responses #5969 and the linked Issue #5967.
  2. Diff Auditing: Analyzed the changes applied in load_artifacts_tool.py, identifying the implementation of the new private helper symbol _requested_artifact_names to scan all parts of a content turn.
  3. Local Workspace Verification: Inspected the baseline implementation of LoadArtifactsTool and test_load_artifacts_tool.py to map formatting discrepancies situations inside contents.py.
  4. Premium Reporting: Drafted and persisted the complete architectural alignment checklists and justification findings in analysis_results.md.

@he-yufeng

Copy link
Copy Markdown
Contributor Author

Closing this lower-signal artifact text-response PR for now. It has not picked up maintainer review signal, and the ADK queue is too crowded to keep every small isolated branch open.

@he-yufeng he-yufeng closed this Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tools [Component] This issue is related to tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

load_artifacts is not working as expected in adk 2.0

3 participants