Skip to content

fix(plugins): return None, not the input, when a built-in plugin leaves a value unchanged - #7390

Open
AtulJoshi1206 wants to merge 2 commits into
google:mainfrom
AtulJoshi1206:fix/multimodal-plugin-after-tool-none
Open

AtulJoshi1206 wants to merge 2 commits into
google:mainfrom
AtulJoshi1206:fix/multimodal-plugin-after-tool-none

Conversation

@AtulJoshi1206

@AtulJoshi1206 AtulJoshi1206 commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Update (Oct 17): rebased onto current main, plus one more commit that fixes the same pattern in the other two built-in plugins that had it.

  • ReflectAndRetryModelPlugin: once the retry limit is exceeded with throw_exception_if_retry_exceeded=False, after_model_callback returned the error response itself. core/_finalizer.py treats any plugin result as an override, so the remaining plugins and the agent's canonical_after_model_callbacks were skipped for that response. It now returns None.
  • SaveFilesAsArtifactsPlugin: with no artifact service configured, it logged that it was disabled and then returned the user message itself, which stopped every later plugin's on_user_message_callback. It now returns None. That matches its other no-op paths (no parts, no inline data), which already return None.
  • Scope check: I scanned every class in src/google/adk/plugins for a method that returns one of its own parameters (result, llm_response, user_message, ...). After this change there are none left.
  • Tests:
    • Updated the two tests that asserted the input came back, and renamed one to match.
    • Added test_agent_after_model_callback_runs_when_retry_limit_is_exceeded, which runs through InMemoryRunner.
    • Added test_no_artifact_service_does_not_stop_later_plugins, at the PluginManager level.
    • Both fail on main.
    • tests/unittests/plugins, flows/llm_flows, agents and runners: 3375 passed.

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

No existing issue; described below following the bug-report structure.

Describe the Bug:

MultimodalToolResultsPlugin.after_tool_callback returns the tool result itself when the result is not a Part or a list of Parts:

if not (isinstance(result, types.Part) or ...):
  return result

PluginManager stops at the first non-None plugin result. In flows/llm_flows/tools/_caller.py, the agent's canonical_after_tool_callbacks only run when the plugin result is None. Under the BasePlugin contract, None means "leave the result unchanged".

Impact:

Registering this built-in plugin silently turns off every agent-level after_tool_callback (redaction, rewriting, state updates) for every tool that returns an ordinary dict. It also turns off any plugin registered after it.

Steps to Reproduce:

An LlmAgent with a dict-returning tool, an after_tool_callback that rewrites the response, and plugins=[MultimodalToolResultsPlugin()].

Observed Behavior: the callback is never called, and the original dict is sent to the model.

Expected Behavior: the callback runs and its return value replaces the response, the same as without the plugin.

Solution:

Return None for non-part results. The other return paths of this callback already return None.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.

  • All unit tests pass locally.

  • Added test_agent_after_tool_callback_runs_for_non_part_results, which runs an agent end to end through InMemoryRunner with the plugin.

  • Updated test_tool_returning_non_list_of_parts_is_unchanged to assert result is None. That matches its stated intent ("has this result unchanged") under the plugin contract.

  • Both fail on main.

pytest tests/unittests/plugins                                  # 1134 passed
pytest tests/unittests/flows/llm_flows tests/unittests/agents   # 1841 passed, 2 xfailed

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules. (none)

🤖 Generated with Claude Code

…rt results

For a tool result that is not a Part or a list of Parts, after_tool_callback
returned the result itself. The plugin manager stops at the first non-None
plugin result, and the agent's after_tool_callbacks only run when the plugin
result is None, so registering this plugin silently disabled every agent
after_tool_callback, and every later plugin, for ordinary dict results.
Returning None is the BasePlugin contract for leaving a result unchanged.
@AtulJoshi1206
AtulJoshi1206 force-pushed the fix/multimodal-plugin-after-tool-none branch from 4ebb7ff to 1b24c99 Compare October 17, 2026 03:58
… plugins

ReflectAndRetryModelPlugin and SaveFilesAsArtifactsPlugin had the same
pattern as MultimodalToolResultsPlugin, returning their input to mean
"unchanged". The plugin manager stops at the first non-None result, so:

- ReflectAndRetryModelPlugin, once the retry limit was exceeded with
  throw_exception_if_retry_exceeded=False, returned the error response
  itself, and the flow then skipped the remaining plugins and the agent's
  canonical after_model_callbacks for it.
- SaveFilesAsArtifactsPlugin, with no artifact service configured,
  returned the user message itself, which stopped every later plugin's
  on_user_message_callback.

Both now return None, the BasePlugin contract for leaving a value
unchanged.
@AtulJoshi1206
AtulJoshi1206 force-pushed the fix/multimodal-plugin-after-tool-none branch from 1b24c99 to aaeeab9 Compare October 17, 2026 04:01
@AtulJoshi1206 AtulJoshi1206 changed the title fix(plugins): return None from MultimodalToolResultsPlugin for non-part results fix(plugins): return None, not the input, when a built-in plugin leaves a value unchanged Oct 17, 2026
@AtulJoshi1206

Copy link
Copy Markdown
Contributor Author

Rebased on latest main. While re-checking this I found the same "return the input to mean unchanged" pattern in two more built-in plugins: ReflectAndRetryModelPlugin (once retries are exhausted without throwing, it skipped the agent's after_model_callback) and SaveFilesAsArtifactsPlugin (with no artifact service, it stopped later plugins). Fixed both in a second commit with tests, and updated the title and description. A scan of the plugins package shows nothing else left with this pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants