fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics

[caohy1988]

Staging PR in the caohy1988/adk-python fork for blind review (Claude Code Opus 5.5 implementer, Codex Astra + Claude Fable reviewers). Upstream google/adk-python PR only after Haiyuan OKs it.

Suggested title for the upstream squash (release-please): feat(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics. This PR's commits are titled fix(plugins): …, but the change adds public API and changes which event types the plugin emits, so the squashed title should use feat to list it under Features.

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

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

• Closes: BigQueryAgentAnalyticsPlugin records error-bearing tool results as TOOL_COMPLETED with status OK google/adk-python#7112

2. Or, if no issue exists, describe the change:

Problem:

BigQueryAgentAnalyticsPlugin writes TOOL_ERROR only from on_tool_error_callback, and after_tool_callback writes TOOL_COMPLETED / status = 'OK' for whatever the tool returned. So two common failures are recorded as successes:

1. An MCP tool whose server answers isError: true. McpTool returns the CallToolResult as a result dict instead of raising.
2. A tool that raises while ReflectAndRetryToolPlugin is registered ahead of the analytics plugin. PluginManager stops at the first plugin that answers the error, so the retry plugin's response reaches after_tool_callback as the tool's result.

With the plugins in the opposite order, on_tool_error_callback records the TOOL_ERROR. The retry plugin's answer then still reaches after_tool_callback, which writes a second, spurious TOOL_COMPLETED row. Because the tool span is already closed, that row pops a span it does not own and carries the enclosing agent span's id and latency.

Separately, _get_tool_origin returns UNKNOWN for ToolboxToolset tools and for SkillToolset's own tools.

Solution:

after_tool_callback now decides whether a result reports a failure. Three checks run in order:

1. ReflectAndRetryToolPlugin answer. Identified by its response_type. It stands for a raised error and is recorded as TOOL_ERROR before any classifier runs, so a classifier can't hide a raised error in one plugin order only. The message is the answer's error_details, which is str(error), the same text on_tool_error_callback records. If that is empty the exception's type name is used, and if both are empty a fixed message.
2. Application classifier. The new optional BigQueryLoggerConfig.tool_result_classifier runs next. It follows the ToolResultClassifier protocol: keyword-only (*, tool, tool_args, tool_context, result), returning a ToolResultClassification(status="ERROR" | "OK", error_message=...) or None.

   • When the plugin is created, it rejects a classifier that:

     • is not callable;
     • is a coroutine or async-generator function, or has such a __call__, either directly or behind functools.partial;
     • has no signature inspect can read, including one whose __signature__ raises;
     • can't take those four keyword arguments.

     The signature is read without evaluating annotations: on Python 3.14 they are read as strings. So a classifier that annotates its parameters with names imported only under TYPE_CHECKING passes, where 3.14's default evaluation raises NameError.

   • At run time, a classifier that raises an Exception, or whose verdict can't be read, is skipped with a warning that names only the tool.

   • A returned awaitable is skipped the same way, after it is released without being awaited:

     • a coroutine, native or generator-based, is closed;
     • a pending asyncio future or task is cancelled, so a task that hasn't started never runs;
     • the exception a future holds, or ends with after the cancellation, is retrieved.

     So Python doesn't warn about the coroutine, and asyncio doesn't later log an exception the future or task holds. Otherwise asyncio logs a failed future's unretrieved exception, which can carry the result, when the future is collected. An async generator, or any other awaitable, runs nothing until iterated or awaited, so it is simply dropped, along with any future it wraps.

3. MCP error flag. Any dict result whose top-level isError (is_error in SDK 2.x dumps) is exactly True, from any tool, which covers a CallToolResult dump. Or a CallToolResult model itself, from mcp.types or the SDK 2.x mcp_types package, with that flag.

Reading the result. Fields are read without calling any method of the result, its keys, or its values:

• types are checked with type.__subclasscheck__ against the value's actual type, never through isinstance's __class__ fallback or a metaclass hook;
• a dict result is scanned for exact-str keys rather than looked up, so no key of the result is hashed or compared;
• a model's fields come through the SDK class's own __dict__ descriptor, so a subclass's __getattribute__ or __dict__ property can't intercept the read.

Each check runs on its own, so a malformed or hostile field can't hide a valid signal in another field. If classification still fails, the call falls back to its TOOL_COMPLETED row with a constant warning.

A closed span always gets its row.

• Serializing a TOOL_COMPLETED result does run the result's code. On main a result whose attribute reads or __class__ raise made the serializer raise, lost the row, and put the exception, which can carry the payload, in _safe_callback's traceback log.
• Serialization now has the same kind of boundary as classification. A result that raises there is recorded with the sentinel the serializer writes for a result it rejects itself, and a constant warning is logged.
• Every warning is logged after its except block. Logged inside it, a failing log handler would print the caught exception, which can carry the result.
• A tool result model is dumped with model_dump(warnings=False). Pydantic still dumps a field whose value no longer matches its type, such as an MCP result's content list appended to after validation. But by default it first emits a UserWarning that quotes the value, to stderr or the py.warnings logger, before any content_formatter, column denylist or log_multi_modal_content setting applies. The flag is per call, so no process-wide warning filter changes.
• A Pydantic model whose model_dump is an override isn't called, top-level or nested. Whatever its signature, an override may not pass the flag on, as super().model_dump(exclude=...) doesn't. So the row records the serializer's sentinel in its place, as it does for a model_dump that fails or doesn't take the flag. Reading the fields instead would record what the override or Pydantic's own serializers leave out.
• The function response part of the next LLM_REQUEST row is dumped with the same flag. Pydantic serializes models nested in it itself, without calling their overrides.
• BaseException subclasses that aren't Exceptions (cancellation, KeyboardInterrupt, SystemExit) still propagate, as they do from every callback of this plugin.

The TOOL_ERROR row.

• It has status = 'ERROR' and a non-null error_message.
• Its content is {tool, args, tool_origin}, the same shape on_tool_error_callback writes.
• Its span and latency pair with TOOL_STARTING.
• The result the model receives is unchanged.
• A failed tool listed in final_response_tool_names logs no AGENT_RESPONSE.

Privacy. error_message is redacted and truncated, but content_formatter and payload_column_denylist don't apply to it.

• For MCP errors the message is fixed ("Tool returned an MCP result with isError=true.").
• The result is left out of the TOOL_ERROR content, because a content_formatter written for TOOL_COMPLETED rows would not scrub it there.
• A later LLM_REQUEST row records the function response only in content_parts (as part_attributes.function_response), and only while log_multi_modal_content is on and content_parts isn't denied.
• An application can put its own text into error_message through a classifier.

One outcome row per call.

• on_tool_error_callback keeps a weak reference to the tool context of the call whose failure it recorded, keyed by plugin instance.
• after_tool_callback then skips that call, so it neither writes a second row nor pops another span.
• The error's message is read before the call is marked, and falls back to its type name if str(error) raises. Nothing that can fail runs between the mark and the row, so a marked call always has its TOOL_ERROR.
• Each parallel call runs in its own copy of the context, so the calls' entries never mix.
• A context that can't take a weak reference goes unmarked, and its row is still written.

Other changes.

• An exception raised without a message, or whose str() raises, now records its type name as error_message.
• _get_tool_origin returns TOOLBOX for toolbox_adk.ToolboxTool, and SKILL for SkillToolset's own tools, looking both up in sys.modules without importing them.

Plugin order, as now documented in the plugin's docstring. Register the analytics plugin before plugins that answer or re-raise tool callbacks.

• With ReflectAndRetryToolPlugin ahead, a call gets no outcome row when:
  • it re-raises after its retries run out;
  • it answers a call to a tool name that doesn't exist, whose after-tool callbacks ADK skips;
  • a subclass's extract_error_from_result answers after_tool_callback.
• Registered first, the analytics plugin records each raised error once as TOOL_ERROR. A failure that only a later plugin detects in a result is recorded as TOOL_COMPLETED, unless tool_result_classifier flags the same result.

Why the per-tool error hooks aren't used. The built-in rules don't consult the tools' _detect_error_in_response hooks, which set the OpenTelemetry error.type.

• Several hooks flag any dict with a truthy error key, which a function tool may also return as data.
• A result they flagged would move from a TOOL_COMPLETED row, which carries it, to a TOOL_ERROR row that leaves it out.
• Some hooks do match ADK's own error envelopes, such as GoogleTool's {"status": "ERROR", "error_details": ...}; see Follow-ups. A classifier can record those as failures today.

Release note (suggested)

• BigQueryAgentAnalyticsPlugin now records tool results that report a failure as TOOL_ERROR with status = 'ERROR', instead of TOOL_COMPLETED / OK. These are ReflectAndRetryToolPlugin answers, and any tool's dict result whose top-level isError or is_error is exactly True, such as an MCP CallToolResult, or a CallToolResult model itself.
• Dashboards that count TOOL_COMPLETED will see these calls move to TOOL_ERROR.
• With ReflectAndRetryToolPlugin registered ahead of the analytics plugin, the exception text moves. str(error) used to reach the TOOL_COMPLETED row as the answer's error_details in content, where content_formatter and payload_column_denylist apply. It now reaches the TOOL_ERROR row's error_message, where only the built-in redaction applies. That is the same text the TOOL_ERROR row already records when the analytics plugin is registered first.
• Configs whose event_allowlist lists TOOL_COMPLETED but not TOOL_ERROR stop recording these calls.
• The rows for these calls no longer carry the result, which is left out of the TOOL_ERROR row for privacy. Queries that read JSON_VALUE(content, '$.result…'), such as the MCP error text, will no longer find it. The function response is still in a later LLM_REQUEST row's content_parts when multimodal content logging is on.
• New API: BigQueryLoggerConfig.tool_result_classifier, ToolResultClassification, ToolResultClassifier.
• tool_origin reports TOOLBOX and SKILL for Toolbox and SkillToolset tools.
• Other:
  • TOOL_COMPLETED latency no longer includes the plugin's own truncation of the result.
  • An exception without a message, or whose message can't be read, records its type name. Before, a message-less exception recorded an empty string, and an unreadable message lost the row.
  • A result that raises when serialized keeps its TOOL_COMPLETED row, with a sentinel in place of the result, instead of losing the row and logging the exception.
  • When the plugin dumps a tool result model whose field no longer matches its type, Pydantic no longer warns with a message that quotes the value. That holds for the TOOL_COMPLETED row and for the function response in the next LLM_REQUEST row.
  • A tool result model whose model_dump is an override, or whose dump fails, is recorded as the sentinel ([UNPARSEABLE_JSON_BLOB] in the row). Before, the override's output was recorded, or for a failed dump the model's fields, including ones the dump leaves out.

Follow-ups (not in this PR)

Each item below is left as it is on main, or is outside google#7112; the reason follows the dash.

• Built-in rule for GoogleTool's {"status": "ERROR", "error_details": …} envelope, which the BigQuery, Spanner, Bigtable, Pub/Sub, GCS, Eventarc and Data Agent tools return. It widens which results count as failures and needs its own review, and a classifier records them today.
• Built-in rule for McpTool's graceful-error envelope, {"error": "MCP tool execution failed: …"} or {"error": "Unexpected error during MCP tool execution: …"}, which the default-on _MCP_GRACEFUL_ERROR_HANDLING returns for McpError and transport failures such as a gateway 403. A rule keyed on an {"error": …} shape must first rule out tools that return the same shape as data, McpTool's own _detect_error_in_response doesn't flag it either, and a classifier records it today.
• Built-in rule for FunctionTool's missing-argument envelope, {"error": "Invoking `<name>()` failed as the following mandatory input parameters are not present: …"}. Same reason as the McpTool envelope: the {"error": …} shape is also data, and the HITL confirmation pause, for one, uses it.
• An opt-in to keep the result, such as the MCP error text, in TOOL_ERROR rows. It is a privacy decision, and a content_formatter written for TOOL_COMPLETED rows would not scrub the result there.
• An opt-in classifier that mirrors the tools' OpenTelemetry error hooks. Several of them flag any truthy error key, including data and confirmation envelopes.
• _log_event's content-formatter and content-parser boundaries log inside their except blocks, so a log handler that fails at that moment prints the caught exception. That code predates this change and is shared by every event type; this PR's boundaries log after leaving the block.
• The Support for specifying GCP Project and Location per Vertex AI Model instance google/adk-python#485 item: a content_formatter failure writes [FORMATTER_FAILED] with a null error_message. It is a separate behavior of the formatter boundary, outside BigQueryAgentAnalyticsPlugin records error-bearing tool results as TOOL_COMPLETED with status OK google/adk-python#7112.
• A classifier whose signature takes any arguments, such as **kwargs, passes the construction check. inspect can't tell that its body rejects the keywords, and such a classifier fails at run time with the constant warning like any failing classifier. On Python 3.11 operator.itemgetter is one of them.
• An object that only looks like a model, not a Pydantic one, is still dumped by its model_dump, with warnings=False. One that takes the keyword but calls a Pydantic dump inside without passing it on can still warn, and so can a custom to_dict() that calls one. The plugin can't tell without calling them, and skipping every such object would drop ordinary results.
• Other event types' model dumps, such as the UI widgets copied into attributes and tool declaration schemas, can raise the same Pydantic warning. They are outside the tool-result path this PR covers; the same per-call flag would apply.
• google-genai's own request conversion emits the same Pydantic warning when it sends such a function response to a real model. That is outside this plugin, and happens with or without it.
• ADK core dumps the same response with Pydantic's warnings on: span content capture (ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS, on by default, with any recording tracer), SqliteSessionService, and the other session services' event dumps. These are outside this plugin; passing warnings=False there, as flows/llm_flows/tools/_caller.py already does, is an upstream change of its own.
• is_error is accepted in any dict, not only from MCP models or MCP tools. Narrowing it would be a behavior change of its own. It matters only for plain dicts that use the snake_case spelling, since ADK's McpTool dumps use camelCase.
• A plugin ahead of analytics that answers before_tool_callback leaves no TOOL_STARTING, and the outcome row pops the agent's span. The span handling predates this PR; pop_span(expected_kind="tool") could guard it.
• Skill tools are recognized by class name, so a new tool class in skill_toolset would need adding. The test that enumerates the module's tool classes fails when that happens.
• Pre-existing and unchanged:
  • A resumed long-running tool's TOOL_COMPLETED is not classified. That path doesn't go through after_tool_callback.
  • Live streaming tool chunks don't go through after_tool_callback either. That's the flow's design, not this plugin's.
  • With shared plugins, a nested AgentTool run clears the outer span stack, and two analytics instances share the module-level span stack. Both are span-stack design; each call still gets exactly one outcome row.
  • ReflectAndRetryToolPlugin calls str(error) itself, so an exception whose __str__ raises ends the run there. That plugin's code is outside this change. Registered first, the analytics plugin records the call's TOOL_ERROR before the run ends.
  • A result the serializer can't read reaches the row as [UNPARSEABLE_JSON_BLOB]. The content parser reads the bracketed [UNSUPPORTED_OBJECT] sentinel as malformed JSON, as it always has.
• Hostile-class edge cases left alone. None has a realistic trigger, since each needs code written to raise from introspection hooks or to misuse asyncio on purpose:
  • tool_args serialization has no new boundary. The model supplies them as JSON, so only a tool that plants such an object in its own argument dict reaches it.
  • A tool whose name or class hooks raise.
  • An exception class whose metaclass makes reading __name__ raise.
  • A classifier whose __signature__ returns a Signature subclass whose bind raises something other than TypeError. That raises in the application's own code at startup.
  • A classifier that returns a task which swallows cancellation and never finishes. asyncio reports "Task was destroyed but it is pending!" with the coroutine's name, but no exception text.
  • A classifier that returns an async generator it has already started by hand. Its cleanup runs on collection, and asyncio logs what that cleanup raises.
  • A classifier that returns a future bound to an event loop running in another thread. Cancelling it from this thread isn't thread-safe, though its exception is still retrieved.
  • A classifier that returns an awaitable of its own kind that wraps a failed future, for example with __await__ returning future.__await__(). The plugin doesn't run the wrapper's code to reach the future, so asyncio logs that future's exception when it's collected.

Testing Plan

Unit Tests:

• I have added or updated unit tests for my change.
• All unit tests pass locally, apart from the environment-only failures listed under Results.

The tests are in tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py, in TestToolResultErrorClassification and TestToolProvenance: 145 cases pass, and one more runs only on Python 3.14 and later.

• Hostile fields. A valid error signal survives a hostile sibling field: a value whose __class__ raises, a key that collides with a wanted key and raises when compared, a raising == or truth test. That holds for both the MCP flag and the retry answer.

• Hostile MCP models. A CallToolResult subclass whose attribute access raises still reports its flag, and model recognition runs no metaclass hook.

• Hostile results, contained by the serializer. Results with a raising ==, raising get or other methods, numpy or pandas values, or a raising __getattr__ keep their row without any fallback firing.

• Hostile results that raise when serialized. These keep their TOOL_COMPLETED row with the sentinel, and a constant warning is logged:

  • a dict subclass or plain object whose __getattribute__ raises;
  • an object whose __class__ raises;
  • a CallToolResult subclass whose attribute access raises, either successful or with an error the classifier records as OK.

• No payload in the log. In all the cases above, no payload reaches the log.

• Isolation. A failing rule doesn't hide another rule's signal. A boundary's warning doesn't carry the caught exception, even through a failing log handler, and that includes the serialization boundary.

• Pydantic warnings that would quote the result. The tests use an MCP result whose content list was appended to after validation, and an application model given a value of the wrong type after validation. Each keeps its TOOL_COMPLETED row with the dumped value, and neither stderr (Python's default warning display) nor the py.warnings logger (logging.captureWarnings(True)) carries the payload. The plugin's own dumps stay quiet across a real turn with log_multi_modal_content off, where the next LLM_REQUEST row dumps the function response too. Those turn tests switch off ADK's span content capture, which dumps the same response with the warnings on whenever a recording tracer is installed (see Follow-ups).

• Result models with their own model_dump. Three overrides: one that takes no keywords and calls super().model_dump(exclude=...), one that takes keywords but doesn't pass them on, and one that builds its own dict. Each is recorded as the sentinel whether it is the result or is nested in it, with no payload and no excluded field in the row, and no payload on stderr or py.warnings. The same holds across a real turn. A result model whose dump fails is recorded as the sentinel rather than as its fields, which would include one declared with Field(exclude=True). An object that only looks like a model is still dumped by its model_dump, which receives warnings=False.

• Unreadable error messages. An error whose str() raises gets exactly one TOOL_ERROR, with its type name. This holds when nothing answers it, when a later handler's answer reaches after_tool_callback, and end to end with an agent on_tool_error_callback that answers it.

• What a classifier returns, followed past garbage collection. Each of these gets the built-in rule's row, and a constant warning:

  • a failed future;
  • a task that fails after a yield, or once cancelled;
  • a failing asyncio.gather;
  • an async generator;
  • another kind of awaitable.

  The test yields to the loop and collects garbage. Neither asyncio's log nor a warning then carries the payload, and asyncio logs no error at all. A pending task is cancelled before it runs, and a pending future is cancelled.

• Classifier:

  • it can't hide a retry answer;
  • async, async-generator, signature-less and positional-only classifiers are rejected, including a functools.partial of an object with an async __call__ and a classifier whose __signature__ raises;
  • on Python 3.14, a classifier annotated with names imported only for type checking is accepted;
  • a coroutine or generator-based coroutine it returns is closed;
  • malformed verdicts fall back to the built-ins;
  • a BaseException propagates.

• Other units:

  • MCP models, including ones from the mcp_types package;
  • two analytics instances, and a context without weak references;
  • message-less errors and retry answers with no text;
  • span and latency pairing;
  • the final-response tool;
  • the tool origins.

• End to end:

  • a raising tool with ReflectAndRetryToolPlugin, in both plugin orders;
  • two parallel raising calls, in both orders;
  • an always-OK classifier, in both orders;
  • a workflow ToolNode, in both orders;
  • an MCP error result, which reaches the model unchanged;
  • a tool that returns an MCP model;
  • an agent on_tool_error_callback answering an error whose str() raises.

Results (Python 3.11.13, macOS), on this head:

• pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8: 685 passed, 1 skipped (the 3.14-only test). The same on Python 3.10.16 and 3.13.7.
• The same file plus test_reflect_retry_tool_plugin.py, test_reflect_retry_utils.py, test_reflect_retry_model_plugin.py and tools/mcp_tool/test_mcp_tool.py: 846 passed, 1 skipped.
• pytest tests/unittests/plugins tests/unittests/workflow/test_tool_node.py -n 8: 997 passed, 1 skipped.
• The real-turn tests still pass when tests that install a recording OpenTelemetry tracer run first in the same process. One run was cli/test_fast_api.py then the turn tests; another was cli/test_fast_api.py, telemetry/test_setup.py and flows/llm_flows/test_llm_callback_span_consistency.py then the whole plugin file (840 passed).
• pytest tests/unittests -n 8 --timeout=300 on this head, run in four parts: 16441 passed. The 9 failures and 4 collection errors fail on this machine without this change too:
  • three timing hangs under load: cli/utils/test_cli_create.py::test_handle_login_with_google_option_3 and two LiveKit tests;
  • four antigravity samples and the labs/antigravity collection errors (google-antigravity can't be installed on x86_64);
  • two test_import_loading allowlist cases (this Python loads sitecustomize).
• Python 3.14. The plugin's compiled dependencies don't load on the only 3.14 build available here, CPython 3.14.0a6. So the classifier validator was extracted from the plugin file and run with the same classifiers on its own:
  • it accepts a classifier annotated with a name imported only for type checking, evaluates none of the annotations, and gives the same verdict as before for every other classifier shape;
  • without this change it rejects that classifier with NameError;
  • the same script passes on 3.10 to 3.13.
• pre-commit run --files <changed files>: all hooks pass (isort 8.0.1, pyink 25.12.0, ruff, addlicense, compliance checks, codespell).
• mypy src/google/adk/plugins/bigquery_agent_analytics_plugin.py: no issues.

Mutation check. Each of 66 mutations removes one guard from a temporary copy of the plugin, and each makes at least one test fail. No mutation survives.

Three of them change code that only Python 3.14 runs. For those, mutate.py runs the check embedded in it with the interpreter PY314 names: it validates a classifier whose annotations name something undefined, and fails if the classifier is rejected or its annotations are evaluated. This report used CPython 3.14.0a6.

The per-mutation report and the script are below. Run the script from the worktree root with PY314=<python3.14> python mutate.py <first> <last>; this report ran 0–21, 22–43 and 44–65.

Per-mutation report (mutation → how many of the 145 passing cases it makes fail, and up to two of them; the script prints them all)

[0] R7 override of a Pydantic model's model_dump called with the flag: 4 failed
      caught by test_result_model_with_its_own_model_dump_is_recorded_as_a_sentinel[keyword_dropping-nested-py.warnings]
      caught by test_result_model_with_its_own_model_dump_is_recorded_as_a_sentinel[keyword_dropping-nested-stderr]
      … and 2 more
[1] R7 override of a Pydantic model's model_dump called without the flag: 16 failed
      caught by test_result_model_with_its_own_model_dump_is_not_quoted_in_a_turn[nested-py.warnings]
      caught by test_result_model_with_its_own_model_dump_is_not_quoted_in_a_turn[nested-stderr]
      … and 14 more
[2] R7 every object whose model_dump is not Pydantic's recorded as a sentinel: 1 failed
      caught by test_result_that_only_looks_like_a_model_is_dumped_quietly
[3] R7 failed model_dump falls through to reading the fields: 1 failed
      caught by test_result_model_whose_dump_fails_is_recorded_as_a_sentinel
[4] R6 result dumped with Pydantic's warnings on: 7 failed
      caught by test_result_field_that_no_longer_fits_is_not_quoted_in_a_turn[py.warnings]
      caught by test_result_field_that_no_longer_fits_is_not_quoted_in_a_turn[stderr]
      … and 5 more
[5] R6 function response part dumped with Pydantic's warnings on: 6 failed
      caught by test_result_field_that_no_longer_fits_is_not_quoted_in_a_turn[py.warnings]
      caught by test_result_field_that_no_longer_fits_is_not_quoted_in_a_turn[stderr]
      … and 4 more
[6] R5 asyncio futures not released: 5 failed
      caught by test_classifier_return_value_leaks_nothing_once_collected[failed_future]
      caught by test_classifier_return_value_leaks_nothing_once_collected[failing_gather]
      … and 3 more
[7] R5 exception of a done future not retrieved: 1 failed
      caught by test_classifier_return_value_leaks_nothing_once_collected[failed_future]
[8] R5 pending future not cancelled: 3 failed
      caught by test_classifier_return_value_leaks_nothing_once_collected[failing_gather]
      caught by test_classifier_return_value_leaks_nothing_once_collected[failing_task]
      … and 1 more
[9] R5 exception a pending future ends with not retrieved: 2 failed
      caught by test_classifier_return_value_leaks_nothing_once_collected[failing_gather]
      caught by test_classifier_return_value_leaks_nothing_once_collected[task_failing_once_cancelled]
[10] R5 exception of a cancelled future read: 1 failed
      caught by test_classifier_return_value_leaks_nothing_once_collected[failing_task]
[11] R5 returned awaitable not released: 7 failed
      caught by test_classifier_return_value_leaks_nothing_once_collected[failed_future]
      caught by test_classifier_return_value_leaks_nothing_once_collected[failing_gather]
      … and 5 more
[12] R5 classifier signature read with its annotations evaluated: 1 failed
      caught by test_classifier_annotated_with_names_it_imports_for_type_checking
[13] R5 annotations read as values on Python 3.14: 1 failed
      caught by test_classifier_annotated_with_names_it_imports_for_type_checking
[14] R5 annotations read as forward references on Python 3.14: 1 failed
      caught by test_classifier_annotated_with_names_it_imports_for_type_checking
[15] R4 no boundary around result serialization: 6 failed
      caught by test_boundary_warning_does_not_carry_the_failure[result_serialization]
      caught by test_mcp_model_that_cannot_be_serialized_keeps_its_row[error_classified_as_ok]
      … and 4 more
[16] R4 serialization warning logged inside except: 1 failed
      caught by test_boundary_warning_does_not_carry_the_failure[result_serialization]
[17] R4 serialization warning logged for every result: 7 failed
      caught by test_hostile_result_is_recorded_without_logging_it[numpy_response_type]
      caught by test_hostile_result_is_recorded_without_logging_it[pandas_response_type]
      … and 5 more
[18] R4 serialization failure not logged: 4 failed
      caught by test_boundary_warning_does_not_carry_the_failure[result_serialization]
      caught by test_result_that_cannot_be_serialized_keeps_its_row[raising_attributes]
      … and 2 more
[19] R4 unserializable result recorded as null: 5 failed
      caught by test_mcp_model_that_cannot_be_serialized_keeps_its_row[error_classified_as_ok]
      caught by test_mcp_model_that_cannot_be_serialized_keeps_its_row[success]
      … and 3 more
[20] R4 error message read after the call is marked: 3 failed
      caught by test_agent_answer_to_an_error_whose_message_cannot_be_read
      caught by test_error_whose_message_cannot_be_read_is_recorded_once[answered_later]
      … and 1 more
[21] R4 error message read without a fallback: 3 failed
      caught by test_agent_answer_to_an_error_whose_message_cannot_be_read
      caught by test_error_whose_message_cannot_be_read_is_recorded_once[answered_later]
      … and 1 more
[22] R4 unreadable error message recorded as empty: 3 failed
      caught by test_agent_answer_to_an_error_whose_message_cannot_be_read
      caught by test_error_whose_message_cannot_be_read_is_recorded_once[answered_later]
      … and 1 more
[23] R4 functools.partial not unwrapped: 2 failed
      caught by test_async_classifier_is_rejected[partial_async_call_method]
      caught by test_async_classifier_is_rejected[partial_async_generator_call_method]
[24] R4 wrapped callable's __call__ not checked: 2 failed
      caught by test_async_classifier_is_rejected[partial_async_call_method]
      caught by test_async_classifier_is_rejected[partial_async_generator_call_method]
[25] R4 only TypeError and ValueError from inspect.signature converted: 1 failed
      caught by test_classifier_without_an_inspectable_signature_is_rejected[raising_signature_attribute]
[26] R3 value type read through __class__: 2 failed
      caught by test_valid_error_signal_survives_a_hostile_sibling_field[error_flag_beside_raising_class]
      caught by test_valid_error_signal_survives_a_hostile_sibling_field[retry_answer_beside_raising_class]
[27] R3 dict result recognized with isinstance: 1 failed
      caught by test_mcp_model_subclass_with_hostile_attributes_is_classified[raising_every_attribute]
[28] R3 MCP model recognized with isinstance: 1 failed
      caught by test_mcp_model_recognition_runs_no_metaclass_hooks
[29] R3 model fields read through the instance: 2 failed
      caught by test_mcp_model_subclass_with_hostile_attributes_is_classified[raising_dict_attribute]
      caught by test_mcp_model_subclass_with_hostile_attributes_is_classified[raising_every_attribute]
[30] R3 entries looked up by key: 3 failed
      caught by test_hostile_result_is_recorded_without_logging_it[raising_methods_with_error_flag]
      caught by test_valid_error_signal_survives_a_hostile_sibling_field[error_flag_beside_colliding_key]
      … and 1 more
[31] R3 result's own items() used: 1 failed
      caught by test_hostile_result_is_recorded_without_logging_it[raising_methods_with_error_flag]
[32] R3 non-str keys kept: 2 failed
      caught by test_valid_error_signal_survives_a_hostile_sibling_field[error_flag_beside_colliding_key]
      caught by test_valid_error_signal_survives_a_hostile_sibling_field[retry_answer_beside_colliding_key]
[33] R3 rules not isolated: 1 failed
      caught by test_failing_rule_does_not_hide_another_rules_signal
[34] R3 rule warning logged inside except: 1 failed
      caught by test_boundary_warning_does_not_carry_the_failure[retry_rule]
[35] R3 classifier warning logged inside except: 1 failed
      caught by test_boundary_warning_does_not_carry_the_failure[classifier]
[36] R3 outer warning logged inside except: 1 failed
      caught by test_boundary_warning_does_not_carry_the_failure[result_fields]
[37] R3 classifier runs before the retry-answer rule: 2 failed
      caught by test_classifier_cannot_hide_a_retry_answer
      caught by test_raised_error_is_recorded_alike_whatever_the_classifier[retry_first]
[38] R3 async generator classifiers accepted: 4 failed
      caught by test_async_classifier_is_rejected[async_generator_call_method]
      caught by test_async_classifier_is_rejected[async_generator_function]
      … and 2 more
[39] R3 classifier without a readable signature accepted: 3 failed
      caught by test_classifier_without_an_inspectable_signature_is_rejected[builtin_without_signature]
      caught by test_classifier_without_an_inspectable_signature_is_rejected[invalid_signature_attribute]
      … and 1 more
[40] R3 generator-based coroutine not closed: 1 failed
      caught by test_classifier_returning_a_generator_coroutine_closes_it
[41] R3 weak reference failure not handled: 1 failed
      caught by test_error_row_is_kept_for_a_context_without_weak_references
[42] R2 no boundary around classification: 2 failed
      caught by test_boundary_warning_does_not_carry_the_failure[result_fields]
      caught by test_failure_to_classify_a_result_still_records_the_call
[43] R2 response_type compared without str guard: 5 failed
      caught by test_hostile_result_is_recorded_without_logging_it[numpy_response_type]
      caught by test_hostile_result_is_recorded_without_logging_it[numpy_response_type_with_error_flag]
      … and 3 more
[44] R2 retry details truth-tested without str guard: 2 failed
      caught by test_hostile_result_is_recorded_without_logging_it[retry_response_with_raising_details]
      caught by test_valid_error_signal_survives_a_hostile_sibling_field[retry_answer_beside_raising_class]
[45] R2 verdict read outside the classifier boundary: 6 failed
      caught by test_classifier_return_value_leaks_nothing_once_collected[async_generator]
      caught by test_malformed_classification_falls_back_to_the_builtin_rules[unreadable_fields]
      … and 4 more
[46] R2 verdict message used without str check: 2 failed
      caught by test_classifier_message_of_a_str_subclass_is_recorded_as_text
      caught by test_malformed_classification_falls_back_to_the_builtin_rules[unvalidated_int_message]
[47] R2 verdict status compared without str guard: 1 failed
      caught by test_malformed_classification_falls_back_to_the_builtin_rules[unvalidated_status_equal_to_everything]
[48] R2 no error_message validation in __post_init__: 1 failed
      caught by test_classification_rejects_a_message_that_is_not_text
[49] R2 coroutine function classifiers accepted: 4 failed
      caught by test_async_classifier_is_rejected[async_call_method]
      caught by test_async_classifier_is_rejected[coroutine_function]
      … and 2 more
[50] R2 async __call__ not checked: 4 failed
      caught by test_async_classifier_is_rejected[async_call_method]
      caught by test_async_classifier_is_rejected[async_generator_call_method]
      … and 2 more
[51] R2 no keyword-signature check: 1 failed
      caught by test_classifier_without_the_keyword_arguments_is_rejected
[52] R2 awaitable not special-cased: 7 failed
      caught by test_classifier_return_value_leaks_nothing_once_collected[failed_future]
      caught by test_classifier_return_value_leaks_nothing_once_collected[failing_gather]
      … and 5 more
[53] R2 coroutine not closed: 2 failed
      caught by test_classifier_returning_a_generator_coroutine_closes_it
      caught by test_classifier_returning_an_awaitable_is_ignored_and_closed
[54] R2 marker not keyed per plugin instance: 1 failed
      caught by test_each_analytics_plugin_records_a_raised_error_once
[55] R2 MCP models not recognized: 6 failed
      caught by test_mcp_call_tool_result_model_is_classified[True-TOOL_ERROR]
      caught by test_mcp_model_from_the_mcp_types_package_is_classified
      … and 4 more
[56] R2 mcp_types package not looked up: 2 failed
      caught by test_mcp_model_from_the_mcp_types_package_is_classified
      caught by test_mcp_model_recognition_runs_no_metaclass_hooks
[57] R2 message-less error records empty text: 1 failed
      caught by test_error_without_a_message_records_its_type
[58] R2 empty retry text accepted: 1 failed
      caught by test_retry_response_without_error_text_records_a_message
[59] R2 str subclass not copied to plain str: 1 failed
      caught by test_classifier_message_of_a_str_subclass_is_recorded_as_text
[60] R1 recorded error not marked: 8 failed
      caught by test_agent_answer_to_an_error_whose_message_cannot_be_read
      caught by test_answer_to_a_recorded_error_adds_no_row_and_pops_no_span
      … and 6 more
[61] R1 marker ignores call identity: 1 failed
      caught by test_recorded_error_of_one_call_does_not_skip_the_next_call
[62] R1 error flag checked for truthiness: 2 failed
      caught by test_hostile_result_is_recorded_without_logging_it[raising_bool_error_flag]
      caught by test_mcp_result_without_error_flag_is_recorded_as_tool_completed[non_bool_flag]
[63] R1 result copied into TOOL_ERROR content: 3 failed
      caught by test_mcp_error_text_is_not_recorded_in_the_tool_error_row
      caught by test_mcp_result_with_error_flag_is_recorded_as_tool_error[isError]
      … and 1 more
[64] R1 TOOLBOX origin dropped: 1 failed
      caught by test_toolbox_tool_returns_toolbox
[65] R1 SKILL origin dropped: 1 failed
      caught by test_skill_toolset_tools_return_skill
SURVIVING MUTATIONS: none (66 of 66 caught; run as 0-21, 22-43, 44-65; mutations 12-14 checked on Python 3.14.0a6 via PY314)

mutate.py

"""Mutation check for the BigQuery analytics tool-result classification.

Each mutation removes one guard from a copy of the plugin in a temporary
directory, then runs the plugin's classification and provenance tests against
that copy through PYTHONPATH. The worktree is never modified. A mutation that
no test catches is reported as surviving.

A guard that only Python 3.14 exercises is checked instead by PY314_CHECK, run
with the interpreter the PY314 environment variable names: it extracts the
classifier validator from the mutated copy and validates a classifier whose
annotations name something undefined. Without PY314 those mutations are
reported as not run.

Usage, from the adk-python worktree root:
  [PY314=/path/to/python3.14] python mutate.py [first_index] [last_index]
"""

import os
import pathlib
import shutil
import subprocess
import sys
import tempfile

WORKTREE = pathlib.Path.cwd()
RELATIVE = "google/adk/plugins/bigquery_agent_analytics_plugin.py"
ORIGINAL = (WORKTREE / "src" / RELATIVE).read_text()

OUTER_BOUNDARY = '''    try:
      error_message = self._tool_result_error_message(
          tool=tool,
          tool_args=tool_args,
          tool_context=tool_context,
          result=result,
      )
    except Exception:  # pylint: disable=broad-except
      error_message = None
      classification_failed = True
'''
RULE_BOUNDARY = '''  try:
    return rule(fields)
  except Exception:  # pylint: disable=broad-except
    pass
  logger.warning(
      "Built-in tool result rule %s failed; skipping it.", rule.__name__
  )
  return None
'''
CLASSIFIER_EXCEPT = '''      except Exception:  # pylint: disable=broad-except
        classifier_failed = True
'''
RESULT_FIELDS_SCAN = '''  return {
      key: value
      for key, value in dict.items(entries)
      if type(key) is str and key in _RESULT_FIELD_NAMES
  }
'''
RETRY_FIRST = '''    retry_answer_message = _apply_rule(_retry_answer_error_message, fields)
    if retry_answer_message is not None:
      return retry_answer_message
    classifier = self.config.tool_result_classifier
'''
VERDICT_IN_BOUNDARY = '''        elif classification is not None:
          return _classification_error_message(classification)
'''
FINAL_MCP_RULE = '''    return _apply_rule(_mcp_error_message, fields)
'''
SERIALIZATION_BOUNDARY = '''    try:
      resp_truncated, is_truncated = _recursive_smart_truncate(
          result, self.config.max_content_length
      )
    except Exception:  # pylint: disable=broad-except
      resp_truncated, is_truncated = "[UNSUPPORTED_OBJECT]", True
      serialization_failed = True
'''
SERIALIZATION_WARNING = '''    if serialization_failed:
      logger.warning(
          "Could not serialize the result of tool %s; recording a sentinel in"
'''
MESSAGE_BEFORE_MARK = '''    try:
      error_message = str(error) or type(error).__name__
    except Exception:  # pylint: disable=broad-except
      error_message = type(error).__name__
'''
MESSAGE_IN_ROW = '''            status="ERROR",
            error_message=error_message,
            latency_ms=duration,
'''
SIGNATURE_EXCEPT = '''  except Exception as e:  # pylint: disable=broad-except
    raise ValueError(
        "tool_result_classifier must have a signature that inspect can read,"
'''
CALL_CHECKS = '''          getattr(classifier, "__call__", None),
          getattr(wrapped, "__call__", None),
'''
FUTURE_RELEASE = '''  elif asyncio.isfuture(awaitable):
    if awaitable.done():
      _retrieve_exception(awaitable)
    else:
      awaitable.add_done_callback(_retrieve_exception)
      awaitable.cancel()
'''

PY314_CHECK = r'''
import ast, functools, inspect, pathlib, sys
from typing import Any

TEST = "check_py314::test_classifier_annotated_with_names_it_imports_for_type_checking"
path = pathlib.Path(sys.argv[1])
namespace = {"functools": functools, "inspect": inspect, "sys": sys, "Any": Any}
for node in ast.parse(path.read_text()).body:
  if isinstance(node, ast.FunctionDef) and node.name in (
      "_signature_without_annotations", "_validate_tool_result_classifier"):
    exec(compile(ast.Module([node], []), str(path), "exec"), namespace)
evaluated = []

class Probe:
  def __class_getitem__(cls, item):
    evaluated.append(item)
    return cls

scope = {"Probe": Probe}
exec(compile(
    "def classify(*, tool: Probe[BaseTool], tool_args, tool_context, result):\n"
    "  return None\n", "<classifier>", "exec", dont_inherit=True), scope)
try:
  namespace["_validate_tool_result_classifier"](scope["classify"])
  failure = "its annotations were evaluated" if evaluated else None
except ValueError as e:
  failure = "rejected with " + type(e.__cause__).__name__
print(f"FAILED {TEST} - {failure}" if failure else f"PASSED {TEST}")
print("1 failed" if failure else "1 passed")
'''

MUTATIONS = [
    # Round 7: a Pydantic model's own model_dump override, and failed dumps.
    ("R7 override of a Pydantic model's model_dump called with the flag", [
        ('          return "[UNSUPPORTED_OBJECT]", True\n'
         "        dumped = model_dump_fn(warnings=False)\n",
         "          pass\n"
         "        dumped = model_dump_fn(warnings=False)\n")]),
    ("R7 override of a Pydantic model's model_dump called without the flag", [
        ('          return "[UNSUPPORTED_OBJECT]", True\n'
         "        dumped = model_dump_fn(warnings=False)\n",
         "          dumped = model_dump_fn()\n"
         "        else:\n"
         "          dumped = model_dump_fn(warnings=False)\n")]),
    ("R7 every object whose model_dump is not Pydantic's recorded as a sentinel", [
        ("        if _is_instance(obj, pydantic.BaseModel) and (\n",
         "        if (\n")]),
    ("R7 failed model_dump falls through to reading the fields", [
        ('      except Exception:\n        return "[UNSUPPORTED_OBJECT]", True\n'
         "    elif dict_fn is not None and callable(dict_fn):\n",
         "      except Exception:\n        pass\n"
         "    elif dict_fn is not None and callable(dict_fn):\n")]),
    # Round 6: Pydantic warnings that quote a tool result's values.
    ("R6 result dumped with Pydantic's warnings on", [
        ("        dumped = model_dump_fn(warnings=False)\n",
         "        dumped = model_dump_fn()\n")]),
    ("R6 function response part dumped with Pydantic's warnings on", [
        ('    dumped = value.model_dump(exclude_none=True, mode="json", '
         "warnings=False)\n",
         '    dumped = value.model_dump(exclude_none=True, mode="json")\n')]),
    # Round 5: what a classifier returns, and Python 3.14 annotations.
    ("R5 asyncio futures not released", [(FUTURE_RELEASE, "")]),
    ("R5 exception of a done future not retrieved", [
        ("    if awaitable.done():\n      _retrieve_exception(awaitable)\n",
         "    if awaitable.done():\n      pass\n")]),
    ("R5 pending future not cancelled", [
        ("      awaitable.cancel()\n", "")]),
    ("R5 exception a pending future ends with not retrieved", [
        ("      awaitable.add_done_callback(_retrieve_exception)\n", "")]),
    ("R5 exception of a cancelled future read", [
        ("  if not future.cancelled():\n    future.exception()\n",
         "  future.exception()\n")]),
    ("R5 returned awaitable not released", [
        ("          _release_awaitable(classification)\n", "")]),
    ("R5 classifier signature read with its annotations evaluated", [
        ("    signature = _signature_without_annotations(classifier)\n",
         "    signature = inspect.signature(classifier)\n")], "py314"),
    ("R5 annotations read as values on Python 3.14", [
        ("annotation_format=annotationlib.Format.STRING",
         "annotation_format=annotationlib.Format.VALUE")], "py314"),
    ("R5 annotations read as forward references on Python 3.14", [
        ("annotation_format=annotationlib.Format.STRING",
         "annotation_format=annotationlib.Format.FORWARDREF")], "py314"),
    # Round 4: the completion path, the error message and classifier checks.
    ("R4 no boundary around result serialization", [
        (SERIALIZATION_BOUNDARY, '''    resp_truncated, is_truncated = _recursive_smart_truncate(
        result, self.config.max_content_length
    )
''')]),
    ("R4 serialization warning logged inside except", [
        (SERIALIZATION_BOUNDARY, SERIALIZATION_BOUNDARY.replace(
            "      serialization_failed = True\n",
            "      serialization_failed = True\n"
            '      logger.warning("Could not serialize.")\n'))]),
    ("R4 serialization warning logged for every result", [
        (SERIALIZATION_WARNING,
         SERIALIZATION_WARNING.replace("if serialization_failed:", "if True:"))]),
    ("R4 serialization failure not logged", [
        (SERIALIZATION_WARNING,
         SERIALIZATION_WARNING.replace("if serialization_failed:", "if False:"))]),
    ("R4 unserializable result recorded as null", [
        ('      resp_truncated, is_truncated = "[UNSUPPORTED_OBJECT]", True\n',
         "      resp_truncated, is_truncated = None, True\n")]),
    ("R4 error message read after the call is marked", [
        (MESSAGE_BEFORE_MARK, ""),
        (MESSAGE_IN_ROW, MESSAGE_IN_ROW.replace(
            "error_message=error_message,",
            "error_message=str(error) or type(error).__name__,"))]),
    ("R4 error message read without a fallback", [
        (MESSAGE_BEFORE_MARK,
         "    error_message = str(error) or type(error).__name__\n")]),
    ("R4 unreadable error message recorded as empty", [
        (MESSAGE_BEFORE_MARK, MESSAGE_BEFORE_MARK.replace(
            "      error_message = type(error).__name__\n",
            '      error_message = ""\n'))]),
    ("R4 functools.partial not unwrapped", [
        ("  while isinstance(wrapped, functools.partial):\n"
         "    wrapped = wrapped.func\n", "")]),
    ("R4 wrapped callable's __call__ not checked", [
        ('          getattr(wrapped, "__call__", None),\n', "")]),
    ("R4 only TypeError and ValueError from inspect.signature converted", [
        (SIGNATURE_EXCEPT, SIGNATURE_EXCEPT.replace(
            "  except Exception as e:  # pylint: disable=broad-except\n",
            "  except (TypeError, ValueError) as e:\n"))]),
    # Round 3: fields are read without running code of the result.
    ("R3 value type read through __class__", [
        ("  if _is_instance(value, str):", "  if isinstance(value, str):")]),
    ("R3 dict result recognized with isinstance", [
        ("entries = result if _is_instance(result, dict) else",
         "entries = result if isinstance(result, dict) else")]),
    ("R3 MCP model recognized with isinstance", [
        ("      if _is_instance(result, model_type):",
         "      if isinstance(result, model_type):")]),
    ("R3 model fields read through the instance", [
        ("      fields = descriptor.__get__(obj, type(obj))",
         "      fields = vars(obj)")]),
    ("R3 entries looked up by key", [
        (RESULT_FIELDS_SCAN,
         "  return {key: dict.get(entries, key) for key in _RESULT_FIELD_NAMES"
         " if key in entries}\n")]),
    ("R3 result's own items() used", [
        ("      for key, value in dict.items(entries)",
         "      for key, value in entries.items()")]),
    ("R3 non-str keys kept", [
        ("      if type(key) is str and key in _RESULT_FIELD_NAMES",
         "      if key in _RESULT_FIELD_NAMES")]),
    ("R3 rules not isolated", [
        (RULE_BOUNDARY, "  return rule(fields)\n")]),
    ("R3 rule warning logged inside except", [
        (RULE_BOUNDARY, '''  try:
    return rule(fields)
  except Exception:  # pylint: disable=broad-except
    logger.warning("Built-in tool result rule %s failed.", rule.__name__)
  return None
''')]),
    ("R3 classifier warning logged inside except", [
        (CLASSIFIER_EXCEPT, CLASSIFIER_EXCEPT.replace(
            "        classifier_failed = True\n",
            "        classifier_failed = True\n"
            '        logger.warning("tool_result_classifier failed.")\n'))]),
    ("R3 outer warning logged inside except", [
        (OUTER_BOUNDARY, OUTER_BOUNDARY.replace(
            "      classification_failed = True\n",
            "      classification_failed = True\n"
            '      logger.warning("Could not classify.")\n'))]),
    ("R3 classifier runs before the retry-answer rule", [
        (RETRY_FIRST, "    classifier = self.config.tool_result_classifier\n"),
        (FINAL_MCP_RULE,
         "    return _apply_rule(_retry_answer_error_message, fields) or"
         " _apply_rule(_mcp_error_message, fields)\n")]),
    ("R3 async generator classifiers accepted", [
        ("      or inspect.isasyncgenfunction(function)", "      or False")]),
    ("R3 classifier without a readable signature accepted", [
        (SIGNATURE_EXCEPT, SIGNATURE_EXCEPT.replace(
            "    raise ValueError(\n", "    return\n    raise ValueError(\n"))]),
    ("R3 generator-based coroutine not closed", [
        ("  if inspect.iscoroutine(awaitable) or inspect.isgenerator(awaitable):",
         "  if inspect.iscoroutine(awaitable):")]),
    ("R3 weak reference failure not handled", [
        ("    try:\n      recorded_call = weakref.ref(tool_context)\n"
         "    except TypeError:",
         "    if True:\n      recorded_call = weakref.ref(tool_context)\n"
         "    if False:")]),
    # Round 2.
    ("R2 no boundary around classification", [
        (OUTER_BOUNDARY, '''    error_message = self._tool_result_error_message(
        tool=tool,
        tool_args=tool_args,
        tool_context=tool_context,
        result=result,
    )
''')]),
    ("R2 response_type compared without str guard", [
        ('  if _exact_str(fields.get("response_type")) !=',
         '  if fields.get("response_type") !=')]),
    ("R2 retry details truth-tested without str guard", [
        ("    message = _exact_str(fields.get(key))",
         "    message = fields.get(key)")]),
    ("R2 verdict read outside the classifier boundary", [
        (VERDICT_IN_BOUNDARY,
         "        elif classification is not None:\n          pass\n"),
        (FINAL_MCP_RULE,
         "      if not classifier_failed and classification is not None and"
         " not inspect.isawaitable(classification):\n"
         "        return _classification_error_message(classification)\n"
         + FINAL_MCP_RULE)]),
    ("R2 verdict message used without str check", [
        ('''  text = _exact_str(message)
  if text is None:
    raise TypeError("ToolResultClassification.error_message is not a str.")
  return text or _CLASSIFIED_TOOL_ERROR_MESSAGE''',
         "  return message or _CLASSIFIED_TOOL_ERROR_MESSAGE")]),
    ("R2 verdict status compared without str guard", [
        ("  status = _exact_str(classification.status)",
         "  status = classification.status")]),
    ("R2 no error_message validation in __post_init__", [
        ('''    if self.error_message is not None and not _is_instance(
        self.error_message, str
    ):
      raise ValueError("error_message must be a str or None.")''',
         "    pass")]),
    ("R2 coroutine function classifiers accepted", [
        ("      inspect.iscoroutinefunction(function)\n"
         "      or inspect.isasyncgenfunction(function)",
         "      inspect.isasyncgenfunction(function)")]),
    ("R2 async __call__ not checked", [
        (CALL_CHECKS, "")]),
    ("R2 no keyword-signature check", [
        ("    signature.bind(tool=None, tool_args=None, tool_context=None,"
         " result=None)", "    pass")]),
    ("R2 awaitable not special-cased", [
        ("        if inspect.isawaitable(classification):",
         "        if False:")]),
    ("R2 coroutine not closed", [
        ("    awaitable.close()\n", "    pass\n")]),
    ("R2 marker not keyed per plugin instance", [
        ("    recorded_call = recorded.get(id(self))",
         "    recorded_call = recorded.get(0)"),
        ("recorded.items() if key != id(self)}",
         "recorded.items() if key != 0}"),
        ("          id(self): recorded_call,", "          0: recorded_call,")]),
    ("R2 MCP models not recognized", [
        ("      if _is_instance(result, model_type):\n"
         "        return _instance_dict(result, model_type)",
         "      pass")]),
    ("R2 mcp_types package not looked up", [
        ('_MCP_TYPES_MODULES = ("mcp.types", "mcp_types")',
         '_MCP_TYPES_MODULES = ("mcp.types",)')]),
    ("R2 message-less error records empty text", [
        ("      error_message = str(error) or type(error).__name__\n",
         "      error_message = str(error)\n")]),
    ("R2 empty retry text accepted", [
        ("    if message:\n      return message\n"
         "  return _REFLECT_AND_RETRY_TOOL_ERROR_MESSAGE",
         "    if message is not None:\n      return message\n"
         "  return _REFLECT_AND_RETRY_TOOL_ERROR_MESSAGE")]),
    ("R2 str subclass not copied to plain str", [
        ("    return str.__str__(value)", "    return value")]),
    # Round 1.
    ("R1 recorded error not marked", [
        ("    if recorded_call is not None:\n      _tool_errors_recorded_ctx.set",
         "    if False:\n      _tool_errors_recorded_ctx.set")]),
    ("R1 marker ignores call identity", [
        ("    if recorded_call is not None and recorded_call() is tool_context:",
         "    if recorded_call is not None:")]),
    ("R1 error flag checked for truthiness", [
        ('  if fields.get("isError") is True or fields.get("is_error") is True:',
         '  if fields.get("isError") or fields.get("is_error"):')]),
    ("R1 result copied into TOOL_ERROR content", [
        ('              "args": args_truncated,\n'
         '              "tool_origin": tool_origin,\n'
         '          },\n          is_truncated=args_is_truncated,',
         '              "args": args_truncated,\n'
         '              "result": result,\n'
         '              "tool_origin": tool_origin,\n'
         '          },\n          is_truncated=args_is_truncated,')]),
    ("R1 TOOLBOX origin dropped", [
        ('    return "TOOLBOX"', '    return "UNKNOWN"')]),
    ("R1 SKILL origin dropped", [
        ('    return "SKILL"', '    return "UNKNOWN"')]),
]


def main():
  first = int(sys.argv[1]) if len(sys.argv) > 1 else 0
  last = int(sys.argv[2]) if len(sys.argv) > 2 else len(MUTATIONS) - 1
  scratch = pathlib.Path(tempfile.mkdtemp(prefix="bqaa-mutation-"))
  shutil.copytree(WORKTREE / "src", scratch / "src")
  target = scratch / "src" / RELATIVE
  env = {**os.environ, "PYTHONPATH": str(scratch / "src")}
  py314_check = scratch / "check_py314.py"
  py314_check.write_text(PY314_CHECK)
  survivors = []
  not_run = []
  try:
    for index in range(first, last + 1):
      name, edits, *checker = MUTATIONS[index]
      mutated = ORIGINAL
      for old, new in edits:
        count = mutated.count(old)
        if count != 1:
          raise SystemExit(f"{name}: pattern found {count} times: {old!r}")
        mutated = mutated.replace(old, new)
      target.write_text(mutated)
      if checker == ["py314"]:
        if not os.environ.get("PY314"):
          print(f"[{index}] {name}: not run; set PY314 to a Python 3.14"
                " interpreter")
          not_run.append(name)
          continue
        command = [os.environ["PY314"], str(py314_check), str(target)]
      else:
        command = [
            sys.executable, "-m", "pytest",
            "tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py",
            "-k", "TestToolResultErrorClassification or TestToolProvenance",
            "-q", "-p", "no:cacheprovider", "--tb=no", "-rf",
        ]
      proc = subprocess.run(
          command, cwd=WORKTREE, capture_output=True, text=True, env=env
      )
      caught = sorted({
          line.split("::")[-1].split(" - ")[0]
          for line in proc.stdout.splitlines()
          if line.startswith("FAILED")
      })
      lines = proc.stdout.strip().splitlines()
      print(f"[{index}] {name}: {lines[-1] if lines else proc.stderr[-200:]}")
      for test in caught:
        print(f"      caught by {test}")
      if not caught:
        survivors.append(name)
  finally:
    target.write_text(ORIGINAL)
    shutil.rmtree(scratch, ignore_errors=True)
  print("SURVIVING MUTATIONS:", survivors or "none")
  if not_run:
    print("NOT RUN:", not_run)


if __name__ == "__main__":
  main()

Manual End-to-End (E2E) Tests:

I ran the reproduction script from google#7112 unchanged. It uses a scripted BaseLlm and captures rows by replacing _log_event, so it needs no BigQuery.

Before:

ReflectAndRetryToolPlugin ahead=False: [('TOOL_STARTING', 'OK', 'mcp_style_failure'), ('TOOL_COMPLETED', 'OK', 'mcp_style_failure')]
ReflectAndRetryToolPlugin ahead=True: [('TOOL_STARTING', 'OK', 'mcp_style_failure'), ('TOOL_COMPLETED', 'OK', 'mcp_style_failure'), ('TOOL_STARTING', 'OK', 'raising_tool'), ('TOOL_COMPLETED', 'OK', 'raising_tool')]

After:

ReflectAndRetryToolPlugin ahead=False: [('TOOL_STARTING', 'OK', 'mcp_style_failure'), ('TOOL_ERROR', 'ERROR', 'mcp_style_failure')]
ReflectAndRetryToolPlugin ahead=True: [('TOOL_STARTING', 'OK', 'mcp_style_failure'), ('TOOL_ERROR', 'ERROR', 'mcp_style_failure'), ('TOOL_STARTING', 'OK', 'raising_tool'), ('TOOL_ERROR', 'ERROR', 'raising_tool')]

With the analytics plugin registered before ReflectAndRetryToolPlugin, a raising tool used to produce TOOL_ERROR plus a spurious TOOL_COMPLETED that carried the agent's span. It now produces one TOOL_ERROR.

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, apart from the same environment-only failures.
• I have manually tested my changes end-to-end.
• Any dependent changes have been merged and published in downstream modules.

Additional context

This is independent of google#7114, which covers only part of the issue and treats any dict with an error_details key as an error. That rule would misclassify successful results from tools that report details in that field.

🤖 Generated with Claude Code

[caohy1988]

Independent blind review by Astra on Haiyuan's behalf, at 3fc3c951c6a130ce2a5d4ba4103d7ca4ba3a1352, compared with e4c0d946f6d602b0ac23804328473b8d9e597b3e. I read the PR description, full diff, implementation commit, repository code, and issues google#7112 and google#485. I did not read another reviewer's material. No tracked files were modified, and no branch was pushed or merged.

Rejection criteria, written before reading the diff

1. Error results produce missing, duplicate, or incorrectly classified rows in either plugin order or supported tool execution paths.
2. Hostile results or classifiers break the callback, drop a row, or bypass the existing formatting and redaction contract.
3. Origin detection changes unrelated tools, or verification contradicts the PR's correctness claims.

P0 findings: None.

P1 findings — changes required

1. Built-in classification introduces a row-loss and exception-text disclosure path. src/google/adk/plugins/bigquery_agent_analytics_plugin.py:804-805 calls payload-controlled get, equality, and truthiness before sanitizing the result. These operations can raise, after after_tool_callback has already popped the span. The outer _safe_callback then logs the exception and skips the outcome row; it does not provide the promised fallback.

   Reproductions against the actual serialized-row fixture:

   class BadGet(dict):
       def get(self, *args, **kwargs):
           raise RuntimeError("PROBE_PRIVATE_PAYLOAD_get")
   
   result = BadGet({"isError": True})
   # Or an ordinary dict:
   result = {"isError": True, "response_type": numpy.array(["a", "b"])}

   Both produce only TOOL_STARTING, with no outcome row. The first also puts the injected private-payload marker into the process log through the traceback. A matching Reflect response whose error_details.__bool__ raises has the same failure. All three inputs retained TOOL_STARTING + TOOL_COMPLETED on the base plugin, without that exception-text disclosure, so this is a regression rather than an existing sanitizer limitation.

   Suggested fix: guard the entire built-in inspection; avoid overridden dictionary methods and arbitrary equality/truthiness where possible; validate the response-type/message primitives before interpreting them. A malformed field must not prevent checking a valid isError=True or emitting one safely formatted outcome. Any diagnostic must exclude exception text/tracebacks. Add regression tests that assert both row count and absence of payload markers in logs.

2. The classifier's return-value handling is outside its exception boundary. src/google/adk/plugins/bigquery_agent_analytics_plugin.py:8493-8496 accesses status/error_message and evaluates error_message truthiness in the else block, which is not covered by the try around the classifier call. ToolResultClassification.__post_init__ validates only status (:2203-2205), so malformed messages are accepted.

   class BadMessage:
       def __bool__(self):
           raise RuntimeError("PROBE_PRIVATE_PAYLOAD_bool")
   
   def classify(tool, result):
       return ToolResultClassification("ERROR", error_message=BadMessage())

   With an otherwise ordinary {"isError": True} result, this drops the outcome row and logs the injected exception text. Returning a classification with a multi-element NumPy array as its message also drops the row; a classification subclass with raising attribute access does likewise. Ordinary classifiers that raise during the call, return an unrelated object, or return None do fall back correctly. The PR's claim that classifier failures cannot drop rows therefore holds only for the call itself, not interpretation of its return value.

   Suggested fix: validate and interpret the returned classification inside the guarded boundary, including its fields. Reject malformed statuses/messages or use the fixed fallback message without evaluating arbitrary truthiness. Fall back to built-ins with a constant warning on any interpretation failure. Cover these cases alongside the existing raising/unsupported-return tests.

P2 finding

3. A direct MCP CallToolResult still reports success. src/google/adk/plugins/bigquery_agent_analytics_plugin.py:802-803 rejects every non-dict result. With installed MCP 2.2.0, CallToolResult(content=[], isError=True) records TOOL_COMPLETED/OK. I reproduced this both through the callback/Arrow-row fixture and through a real Runner using a FunctionTool wrapper returning that object. The native McpTool path is covered because it dumps the model into a dict; this is a remaining result-shape gap, not a regression in that native path. Suggested fix: safely recognize supported MCP result models as well as their dict dumps, and add a Runner regression for a direct model result. Do not add unguarded duck-typed attribute access while doing so.

What I tried and verified

• Own uv environment in this worktree, Python 3.11.13. PYTHONPATH=$PWD/src python -m pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q: 580 passed.
• Reflect/retry tool, utility, and model suites; MCP tool suite; workflow ToolNode suite: 183 passed. Together these include the PR's five-file 741-test selection, plus 22 ToolNode tests.
• Pinned pyink 25.12.0 --check and isort 8.0.1 --check-only on both changed files: passed. git diff --check: passed.
• New classification tests loaded with the base plugin through an isolated import hook: 19 failed, 6 passed. This verifies detection of the original issue without replacing any tracked file. The three hostile-result baseline comparisons separately passed.
• Own adversarial suite: 8 failed, 13 passed. Six failures demonstrate the two P1 findings; two demonstrate the direct-model P2 finding. Truthy non-bool flags, raising ordinary object attributes, raising classifiers, and unrelated classifier return objects retained their rows safely in the passing cases.
• Four 32-call concurrent Runner probes mixed raised errors, MCP error results, and successes, with both plugin orders and both retry guidance and exhausted-retry responses. Each run produced 32 starts and exactly 32 outcomes (22 errors), with matching spans and no duplicate completion rows. Workflow ToolNode probes also passed in both orders for raised and returned errors.
• Another 17 path/privacy probes passed: live sync/async raising tools and live dict results in both plugin orders; nested AgentTool outcome counts; content formatter, failing formatter, and payload denylist behavior; allowlist filtering; and explicit characterization of existing callback bypasses. No live BigQuery service or external model was used.

I agree with omitting result content from the new error row and using a fixed MCP message. The normal rows honor the existing formatter/redaction boundary, and the Reflect response's error_details matches what the exception callback records. The six Skill origins and installed Toolbox class pass the provenance tests. The shared context marker survived the tested parallel, workflow, live, and nested call paths. An allowlist containing only TOOL_COMPLETED does intentionally exclude the new error rows, as the PR documents.

Coverage limits remain: live streaming chunks bypass the outcome callback and still leave TOOL_COMPLETED/OK; a resumed long-running isError=True response also remains TOOL_COMPLETED/OK; an earlier plugin returning from after_tool_callback prevents analytics from observing the result. The repository's unchanged dispatch paths explain these limits; I am not treating them as regressions introduced here. The nested test checked outcome counts, not a repair of the documented outer-span issue.

Environment/evidence limits: all-extras sync could not install google-antigravity on macOS x86_64. I omitted it, onnxruntime, and lancedb, and used a no-deps requirements install with wheel-compatible cryptography 48.0.1 / pyOpenSSL 25.3.0 after the latest cryptography source build failed. This is a targeted review environment, not proof of a coherent all-extras installation. I did not independently reproduce the full 16k-test run, all pre-commit hooks, mypy, or the exact eight mutation experiments claimed in the PR. The broader evidence is therefore unverified here; the concrete classifier robustness claim is disproved above.

Reproducers and logs are under /tmp/adk-pr15-dual/astra-probes/: test_adversarial.py, test_paths.py, test_base_safety.py, run_base.py, focused-tests.log, adjacent-tests.log, adversarial-final.log, adversarial.log (tracebacks), paths.log, base-new-tests.log, and base-safety.log. Rejection criterion 2 is met by both P1 findings.

VERDICT: REQUEST_CHANGES @ 3fc3c95

[caohy1988]

Blind review: Fable (Claude Opus 5.5), head 3fc3c951

Scope: the implementation commit is f187aef2. The head 3fc3c951 only merges upstream main: git diff f187aef2 3fc3c951 on the two files is empty, and the head's src/ differs from base only in the plugin file. I read only the PR description, the diff and code, google#7112, and GoogleCloudPlatform/BigQuery-Agent-Analytics-SDK#485. I read no other review or comment on this PR.

Rejection criteria (written before I read the diff)

• R1, row integrity. An error-bearing result (MCP isError=True, or a ReflectAndRetry response matched by response_type) produces exactly one TOOL_ERROR/ERROR row per tool call. That holds with analytics ahead of or behind the retry plugin, and with parallel calls. Zero rows, two rows, or a TOOL_COMPLETED/OK row for such a result rejects.
• R2, no crash, no drop, no regression. A raising or odd-returning tool_result_classifier, or a hostile result (truthy non-bool isError, an attribute or == that raises, dict vs pydantic shapes), must never raise out of after_tool_callback, drop the row, or change the result the model receives. Successful results stay TOOL_COMPLETED/OK, and a raising tool still gets exactly one TOOL_ERROR. Any violation rejects.
• R3, data contract. The new TOOL_ERROR rows carry no payload beyond what TOOL_ERROR already records. content_formatter, redaction and truncation still apply to them. tool_origin changes only for Toolbox and Skill tools. The new tests fail on the unfixed plugin.

Outcome: R1 holds for the supported single-instance setup. R2 is violated (P1 below). R3 holds.

What I tried

Environment: my own .venv in a detached worktree at the head. Python 3.11.13 (x86_64 under Rosetta), mcp 2.2.0, toolbox-adk 1.4.0, pyink 25.12.0 and isort 8.0.1 as pinned. I skipped google-antigravity, onnxruntime and lancedb, which have no wheels for this platform. My probes live outside the repo.

• pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8: 580 passed.

• The PR's 5-file set: 741 passed, which matches the claim. tests/unittests/plugins plus test_tool_node.py and test_optional_dependencies.py: 902 passed, 6 skipped.

• New tests against the base plugin (base src extracted with git archive e4c0d946 and put on PYTHONPATH):

  • 21 of the 28 new test cases (counting parametrizations) fail on base, including every headline test.
  • The 7 that pass on base are negative controls or guards: the no-flag cases, the unrelated error_details key, "next call not skipped", and "toolbox_adk not imported".

• Mutation spot-check, on a scratch copy of src. All 8 of my own mutations are caught by at least one new test:

  • no marker skip
  • marker never set
  • truthy isError
  • is_error rule dropped
  • result copied into TOOL_ERROR content
  • classifier ignored
  • SKILL origin dropped
  • reflect message fixed

• pyink --check and isort --check-only on both files pass. mypy on the plugin reports no issues.

• End-to-end probes with real InMemoryRunner runs, a scripted BaseLlm and rows captured from _log_event, run in both plugin orders unless noted, at the head and on base:

  • two raising tools called in parallel
  • three parallel calls: one raising, one returning an MCP isError result, one succeeding
  • no retry plugin; the agent's own on_tool_error_callback answers the error
  • ReflectAndRetryToolPlugin retry-exceeded final response, both with and without throw_exception_if_retry_exceeded
  • a nested AgentTool with shared plugins
  • a workflow ToolNode with a raising tool, and one returning an MCP result
  • a tool name the model made up
  • errors and successes alternating across turns
  • a ReflectAndRetryToolPlugin subclass using extract_error_from_result
  • two analytics plugin instances in one runner

  At the head, every single-instance case writes exactly one outcome row per call, and its span pairs with its TOOL_STARTING. On base the same runs write TOOL_ERROR plus a spurious TOOL_COMPLETED, or TOOL_COMPLETED/OK for the failure, so the fix is real.

• Unit probes through the real _log_event pipeline:

  • result shapes: isError of 1, "true" or 1.0; None; a str; a list; reflect dicts without details, with dict details, or with both fields empty
  • a real MCP 2.2.0 CallToolResult: dumped by McpTool's own _dump_mcp_model, dumped unaliased (is_error), and as a raw model
  • content_formatter observation, and a formatter that raises on TOOL_ERROR
  • event_allowlist, payload_column_denylist=["content"], and a 5000-char reflect error_details against max_content_length=200
  • classifiers returning an int, a dict, bytes, or a string holding a secret; an async classifier
  • hostile values: an == that raises, numpy, pandas, a dict subclass whose get raises, and a __bool__ that raises

  All pass except the P1 cases.

• Parallel calls get independent markers. _start_execute_task creates each call's task from a copy of the prepare-phase context (_batch_executor.py:186), and a workflow retry builds a fresh Context for each attempt (_node_runner.py:128). Live mode goes through the same _start_execute_task and _execute_single_prepared_call, so the same reasoning applies. I checked that by reading the code, not by running it.

P1: must fix

P1-1. Classifying a result has no fail-closed boundary, so a hostile result or an odd classifier return drops the outcome row, and a payload-bearing traceback reaches the logs. This is a regression from base.

after_tool_callback pops the tool span (bigquery_agent_analytics_plugin.py:8397) and then calls self._tool_result_error_message(tool, result) (:8405) with no try. Two places inside it can raise:

• The built-in rule compares an arbitrary value with == and tests the truth of the outcome (:804).
• The classifier's return value is used after the try has ended, in classification.error_message or ... (:8496).

When either raises, _safe_callback swallows the exception and writes the full traceback through logger.exception. No outcome row is written, even though the span was already popped. The plugin's own formatter and parser boundaries avoid exactly this: they fail closed and log constant messages.

Results of before_tool_callback then after_tool_callback, with no classifier unless stated:

Result or classifier	Base e4c0d946	Head 3fc3c951
{"response_type": HostileEq()} (__eq__ raises RuntimeError("SECRET…"))	STARTING, COMPLETED; secret not logged	STARTING only; secret in the traceback log
{"response_type": np.array([1, 2])}	STARTING, COMPLETED	STARTING only
{"response_type": pd.Series(["a", "b"])}, for example df.to_dict("series") on a table with a response_type column	STARTING, COMPLETED	STARTING only
dict subclass whose get raises	STARTING, COMPLETED	STARTING only
Classifier returns ToolResultClassification("ERROR", error_message=<__bool__ raises>)	n/a	STARTING only; payload in the log

This breaks the contract's "a raising/odd-returning classifier must never drop the row", and my R2. These shapes are unlikely in practice, but base recorded every one of them. Suggested fix, which I prototyped on a scratch copy: with it all 580 tests in the file and all 46 of my probes pass, and the rows come back without the traceback.

# _builtin_tool_result_error_message: compare a str, never an arbitrary object.
response_type = result.get("response_type")
if isinstance(response_type, str) and response_type == REFLECT_AND_RETRY_RESPONSE_TYPE:
  ...

# after_tool_callback: a fail-closed boundary, like the formatter and parser ones.
try:
  error_message = self._tool_result_error_message(tool, result)
except Exception:  # pylint: disable=broad-except
  # Constant message: the exception can carry the result.
  logger.warning(
      "Could not classify the result of tool %s; recording it as"
      " TOOL_COMPLETED.", tool.name)
  error_message = None

# _tool_result_error_message: never take the truth value of an arbitrary object.
message = classification.error_message
if isinstance(message, str) and message:
  return message
return _CLASSIFIED_TOOL_ERROR_MESSAGE

Add a parametrized regression test over those hostile results. It should assert [TOOL_STARTING, TOOL_COMPLETED] and that the payload text is absent from caplog.

P2: should fix or discuss

1. The marker is module-global, not per plugin instance. See _tool_error_recorded_ctx (:2418), set at :8534 and cleared at :8393. With two analytics instances in one runner plus the retry plugin (the second renamed so PluginManager accepts it), the first instance's after_tool_callback consumes the marker. The second then pops a span it doesn't own and writes a second TOOL_ERROR. Base wrote a spurious TOOL_COMPLETED in the same spot, so this isn't a regression, but that instance now counts each error twice.
   • Fix: key the marker by instance with copy-on-write, {**(ctx.get() or {}), id(self): weakref.ref(tool_context)}, and remove only your own key in after_tool_callback. I validated this on a scratch copy.
2. An async classifier is silently ignored. It passes the callable check at :1576, returns a coroutine, is warned about as an unsupported value, and produces a "coroutine was never awaited" RuntimeWarning; the row stays TOOL_COMPLETED.
   • Fix: reject coroutine functions at construction, as you already do for non-callables.
   • Also settle the public signature now. (tool, result) has no tool_args or tool_context, and adding them later is a breaking change. Keyword-only (*, tool, tool_args, tool_context, result) would be the safer choice.
3. The built-in rules diverge from ADK's own error detection. ADK already has per-tool hooks, _detect_error_in_response, which _tool_error_handler.detect_error_type_for_telemetry calls.
   • FunctionTool's own {"error": ...} results count as errors in OTel but stay TOOL_COMPLETED/OK in BigQuery. These cover validation errors, missing mandatory arguments and signature-less TypeErrors (function_tool.py:261, :389, :430).
   • The same goes for the other tools that define a detector (SkillToolset tools, BashTool, RestApiTool, and others): the plugin never consults them. I found this by reading the code, not by running it.
   • McpTool's hook tests isError for truth (mcp_tool.py:763-764), while this PR requires is True (:809).
   • Consider reusing detect_error_type_for_telemetry as a built-in rule. It already skips pending auth and confirmation. Otherwise, document the divergence so OTel and BigQuery error counts can be reconciled.
4. Plugin-order gaps the PR doesn't mention. Both are pre-existing and identical on base.
   • ReflectAndRetryToolPlugin's default throw_exception_if_retry_exceeded=True (reflect_retry_tool_plugin.py:106), with the retry plugin ahead of analytics: once retries run out it re-raises, analytics never sees the error, and the call has no outcome row.
   • A retry-plugin subclass using the documented extract_error_from_result, registered ahead of analytics: it answers after_tool_callback first, and again there is no outcome row.
   • With analytics first, both cases write exactly one TOOL_ERROR. Document "register BigQueryAgentAnalyticsPlugin before plugins that answer or re-raise tool callbacks", and qualify the "either plugin order" claim.
5. Minor message consistency.
   • A raise RuntimeError() records error_message='' with analytics first (from on_tool_error_callback) but 'RuntimeError' with the retry plugin first.
   • A reflect dict whose error_details and error_type are both empty records '' (:805).
   • Suggest or _REFLECT_AND_RETRY_TOOL_ERROR_MESSAGE for the empty case.
6. Release communication. The PR adds public API (ToolResultClassification, BigQueryLoggerConfig.tool_result_classifier) and changes which event types the plugin emits. I confirmed that event_allowlist=["TOOL_STARTING", "TOOL_COMPLETED"] now drops these failures entirely. That fits feat(plugins): better than fix: for release-please, and it deserves a release note for dashboards and allowlists.

The design points the implementer flagged

• Result left out of TOOL_ERROR content. Agreed, for privacy: the formatter sees {args, tool, tool_origin} for TOOL_ERROR, and the MCP text appears nowhere in the row. The claim that the result "is still recorded in the next LLM_REQUEST row" holds only through content_parts[].part_attributes.function_response, with the default log_multi_modal_content=True. It is not in the content column, which reads "Function response: t". It is also missing when log_multi_modal_content=False, when content_parts is denied, or when no further model call follows. Please say this precisely in the docstring and PR body.
• Reflect error_message taken from error_details. Fine. It equals str(error), so both plugin orders record the same text. It is redacted (api_key=[REDACTED]) and truncated (5000 chars becomes 214, with is_truncated).
• Toolbox and Skill detection. Correct:
  • Checked against toolbox-adk 1.4.0, which exports toolbox_adk.ToolboxTool(BaseTool) at the top level.
  • Checked against all six classes in skill_toolset; the test enumerates them, so drift gets caught.
  • The detection never imports toolbox_adk.
  • Importing skill_toolset cost 0.16 s here, against ~0.4 s claimed; the difference depends on the environment and doesn't matter.
• Per-call marker. Holds for parallel calls, nested AgentTool, workflow ToolNode, sequential calls, the unknown-tool path, and the agent-level error callback. Only the multi-instance case breaks (P2-1).
• event_allowlist and dashboard change. See P2-6.

Not verified: live and streaming tools (reasoned only); long-running tools resumed through on_user_message_callback, which the PR acknowledges it doesn't classify; the full tests/unittests suite, which I didn't run because the change touches one module; and real BigQuery writes (the write client was mocked).

VERDICT: REQUEST_CHANGES @ 3fc3c95

[caohy1988]

Blind review by Astra on Haiyuan's behalf, scoped to def458b609c2811d137b0332b2fc7b201dddd5c0..fa56c26f717dc0e1a3c15b50fd703983937191ca. I read the PR description, both implementation commit messages, the complete two-file diff, issues google#7112/google#485, and relevant repository code. I did not read another reviewer's review or artifacts.

Rejection criteria, written before reading the diff

1. Reject a reproducible lost, duplicated, or misclassified outcome row within the promised callback coverage, including MCP isError=True, ReflectAndRetry responses, and parallel/multiple-plugin execution.
2. Reject hostile results or classifier failures escaping the callback or leaking payload-derived diagnostics; require the advertised classifier construction and return-value handling.
3. Reject regressions in successful/raising tools, content formatting/redaction, or tool origins, and unsupported guarantees that conceal a coverage limit.

What I tried

• Created a separate uv environment in this worktree, Python 3.11.13, from the frozen lock. The all-extras install required excluding google-antigravity, onnxruntime, and lancedb; locked cryptography==50.0.1 also failed its local Rust/OpenSSL build, so this environment uses 48.0.1. Repository dependency files were unchanged.
• Ran the requested PYTHONPATH=$PWD/src python -m pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q: 613 passed.
• Ran all plugin tests plus workflow ToolNode, MCP tool, and streaming-tool-event tests: 1,064 passed. This includes the related suites named in the PR.
• Pinned pyink 25.12.0 and isort 8.0.1 checks passed on both changed files. Mypy passed on the plugin.
• Wrote 41 independent probe cases outside the repository: 35 passed, 6 failed. These exercised actual serialized rows for hostile results, direct MCP models, exact/non-bool flags, formatter/denylist behavior, and real Runner/Workflow callback ordering. The failures support findings 1 and 2 below.
• Two separately named analytics instances in one real Runner each emitted exactly one error per call across 36 parallel calls, three plugin orders (both before retry, both after retry, and sandwich), and both ordinary/message-less exceptions. No outcome rows were lost or duplicated in these probes.
• Workflow ToolNode passed for MCP errors, raised errors, and retry-exhausted returned guidance in both orders. Nested AgentTool outcome counts passed. Probed non-streaming live dispatch, streaming chunks, resumed long-running responses, retry re-raising, and plugins short-circuiting before/after callbacks. The documented existing exclusions remain real.
• Ran the issue google#7112 reproduction unchanged on the head and with the pinned base plugin loaded in an isolated process. Also ran three assertion-based regressions: all 3 pass on head and fail on base; the base exhibits both false successes and the analytics-first duplicate outcome. No tracked file was replaced for this comparison.

Probe sources/logs: /tmp/adk-pr15-dual/r2/astra-probes/ (test_adversarial.py, test_paths.py, adversarial-tests.log, path-tests.log, regression-{head,base}.log). BigQuery writes and model responses were mocked; I did not exercise a live BigQuery service or remote MCP server, or rerun the historical full-unit-suite totals.

P1 — findings requiring changes

1. A hostile sibling field can turn a valid MCP failure back into TOOL_COMPLETED/OK.

Locations: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:788, :839, :843, and the fallback at :8585.

For example:

class Unreadable:
    @property
    def __class__(self):
        raise RuntimeError("protected payload")

result = {"isError": True, "response_type": Unreadable()}

_exact_str calls isinstance(value, str), which accesses the object's __class__. That raises before the known isError=True flag is examined. The outer handler then emits TOOL_COMPLETED, status OK, null error_message. My full row-writing test test_valid_error_survives_hostile_fields[raising_class_field] fails on precisely that outcome. The row survives and the warning stays constant; the error classification does not survive.

Two further full-row reproductions fail the same way: a dictionary containing a key whose hash collides with "response_type" and whose __eq__ raises (dict.get still invokes key equality), and a CallToolResult subclass whose __getattribute__("__dict__") raises (vars(result) dispatches to it). These also disprove the code/commit/PR assertion that classification runs none of the result's own code. The existing test at tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py:6958 explicitly expects a success row after classification failure, so it does not protect a valid error flag from this loss.

Suggested fix: read recognized fields without dispatching to payload-controlled instance methods, normalize actual string keys before lookup, and isolate malformed fields so one cannot suppress another valid error indicator. Use type-based string checks and bypass model instance attribute overrides where appropriate. Add all three full-row regressions, asserting TOOL_ERROR/ERROR and constant diagnostics. This is a missing part of the requested hostile-result hardening, not a claim that base already classified these results correctly.

P2 — additional findings

2. Classifier validation and awaitable cleanup do not meet the advertised contract.

Locations: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:1678, :1687, :8691.

Three independent probes fail:

• An async def classifier(**kwargs): yield None is accepted at construction: only coroutine functions are checked, not async-generator functions/callable objects.
• operator.itemgetter("status") is accepted, although it cannot receive the four keyword arguments. inspect.signature raises ValueError and validation simply returns. Consequently this configuration can never classify a call and only warns at runtime.
• A generator-based coroutine produced with types.coroutine is recognized as awaitable but is not closed, because inspect.iscoroutine is false. Its gi_frame remains live after classification. The fallback outcome row is correct; the promised cleanup is not.

Suggested fix: reject async-generator callables too; require an inspectable compatible signature (or an explicit synchronous adapter) when construction-time validation is promised; close supported generator-based coroutines as well as native coroutines. Document a precise policy for other awaitable kinds instead of promising that every awaitable is closed. Add constructor and cleanup tests beyond the current coroutine-only cases.

3. Registering analytics first does not guarantee the documented TOOL_ERROR outcome.

Location: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:4996.

The docstring says that registered first, this plugin records each failure as TOOL_ERROR whatever later plugins do. I tested a tool returning {"failed": True} and a later ReflectAndRetry subclass whose extract_error_from_result recognizes that result. Analytics first records TOOL_COMPLETED before the retry plugin produces its reflection response. Retry first records no outcome, as already documented. test_extractor_plugin_order_documentation confirms both outcomes.

Suggested fix: qualify the first-position guarantee to raised exceptions and result shapes recognized by the analytics built-ins/classifier. Explicitly tell users of extract_error_from_result subclasses to supply equivalent tool_result_classifier logic when they need matching counts. This is a documentation correction for an existing architectural limit, not a request to redesign PluginManager in this PR.

4. The exact 19-guard mutation claim has no reproducible evidence in the permitted review inputs.

Location: PR description, Testing Plan → Mutation check. The description reports 19 killed mutations but supplies neither the mutations nor a command/per-mutation report; the two changed files contain the tests, not that mutation runner. I independently verified the normal suites and before/after regressions above, but those do not establish the exact reported mutation tally. Please attach the mutation patches/script and per-mutation failing-test output, or narrow the validation claim. This is an evidence gap, not an assertion that the author's mutation run failed.

Design judgments

Omitting result payloads from TOOL_ERROR, using ReflectAndRetry's error_details with existing error redaction, keyword invocation of the hook, and the current Toolbox/Skill class detection are reasonable for this scope. Conservative built-ins that deliberately differ from _detect_error_in_response are acceptable with the opt-in classifier and accurate coverage documentation. The event-allowlist/dashboard migration is explicitly disclosed. The per-instance/task-local marker passed the concurrency and row-accounting probes; the previously documented nested span-stack and bypass-path limitations should not be presented as newly fixed.

Finding 1 meets rejection criterion 1; findings 2–3 also contradict explicit contract/coverage claims. Request changes on this head. This is intentionally posted with review event COMMENT only.

VERDICT: REQUEST_CHANGES @ fa56c26

[caohy1988]

Fable blind review (round 2) of PR #15 @ fa56c26

This review is blind: I read no other review, review comment or PR comment. My sources were the PR description, the diff def458b6..fa56c26f, issues google#7112 and BigQuery-Agent-Analytics-SDK#485, and the repository code. The merge commits 3fc3c95 and fa56c26 touch only Redis files, so the plugin changes are entirely f187aef and 563d32c.

Rejection criteria (written before I read the diff)

• R1 Row integrity. For one tool call, a reachable path writes zero outcome rows, or more than one (TOOL_COMPLETED + TOOL_ERROR, or two TOOL_ERRORs), or lets an exception escape into the agent run. The paths: MCP isError as a dict and as a model, ReflectAndRetry answers in either plugin order, parallel calls, two analytics plugin instances, raising tools, and classifiers that raise or return junk. Results that succeed must stay TOOL_COMPLETED/OK.
• R2 Fail-closed and leakage. A hostile result or classifier drops the outcome row or crashes the callback. Or payload-derived text reaches the logs, or reaches the TOOL_ERROR row beyond what TOOL_ERROR already recorded. Or the new rows bypass content_formatter or redaction.
• R3 Unsupported claims. A claimed round-2 item fails under a probe, the new tests pass on the unfixed plugin, or existing tests regress.

I found no evidence that meets any of the three.

What I tried

Setup: Python 3.11.13, the repo's venv, PYTHONPATH=src. All probes lived outside the repo, and the worktree was never modified.

1. The PR's stated results.

   • Test counts, all reproduced exactly: the plugin test file gives 613 passed; plugins/ plus workflow/test_tool_node.py gives 925 passed; the plugin file plus the reflect-retry and mcp_tool suites gives 774 passed.
   • Wider runs: plugins/ plus the tool node and MCP tool suites, 1034 passed. test_optional_dependencies.py plus cli/test_fast_api.py, which also reference the plugin: 152 passed, 11 skipped, 1 xfailed.
   • Static checks: mypy on the plugin is clean. pyink --check 25.12.0 and isort --check-only 8.0.1 (the pre-commit pins) are clean on both files. Both files compile on Python 3.10.16.
   • The google#7112 reproduction script, run unchanged, prints exactly the "Before" output on def458b and exactly the "After" output on fa56c26.

2. Do the tests fail on unfixed code? The test module subclasses ToolResultClassification at import, so a plain base run fails at collection. I ran the tests instead against a shim: the base plugin plus only the new public names (the dataclass, the Protocol and the config field), with none of the behavior.

   • 43 of the 73 TestToolResultErrorClassification/TestToolProvenance tests fail on the shim.
   • The 30 that pass are the 12 existing provenance tests and the intended "stays TOOL_COMPLETED" and validation guards.

3. Mutations, applied to a /tmp copy of head. At least one test catches each one:

   • drop the marker skip: 3 fail
   • key the marker by a constant instead of per instance: 1 fails
   • use a truthy isError instead of is True: 1 fails
   • remove the outer classification boundary: 1 fails
   • drop the fallback for exceptions without a message: 1 fails
   • run the built-in rules before the classifier: 9 fail
   • remove the async-classifier validation: 3 fail
   • remove the SKILL origin: 1 fails

4. End to end through a real Runner, head against base. Rows were captured by replacing _log_event, and each outcome row was paired with its TOOL_STARTING by span.

   • Four parallel calls in one turn (raise, MCP isError, OK, raise), with the retry plugin in either order.
     • Head: exactly one correct outcome per call, and every span pairs.
     • Base, retry plugin first: all four are TOOL_COMPLETED.
     • Base, analytics first: duplicate TOOL_COMPLETED rows that carry the wrong span.
   • Two renamed analytics instances, arranged as [bq1,bq2,retry], [bq1,retry,bq2] and [retry,bq1,bq2]: each instance writes one outcome per call.
   • Retries exhausted with throw_exception_if_retry_exceeded=False: two TOOL_ERRORs in both orders.
   • A message-less RuntimeError(): error_message == "RuntimeError" in both orders.
   • Workflow ToolNode.
     • A raising tool with the retry plugin in either order: one TOOL_ERROR.
     • An MCP error result: TOOL_ERROR.
     • A node retry_config whose first attempt raises: TOOL_ERROR, then TOOL_COMPLETED. Each attempt gets a fresh Context, so the marker does not skip the retry.
   • A nested AgentTool whose inner tool returns an MCP error: one outcome per call.
   • Calls that get no outcome row behave exactly as on base. With the retry plugin first, that happens on a re-raise, with an extract_error_from_result subclass, and with an unknown tool name. Two span issues are also unchanged from base: the outer AgentTool's span mismatch, and the span swap between two plugin instances.

5. Hostile results. On head, each of these keeps its outcome row, and no log record contains the payload:

   • dict keys that share a lookup key's hash and whose == raises or always returns True
   • objects whose __class__ claims dict or str
   • numpy.bool_, numpy.str_ and int flags
   • a MappingProxyType
   • a model_construct result with a non-bool flag
   • a real CallToolResult(isError=True)

   The one exception is a result whose __class__ itself raises. It loses its row and logs the payload through _safe_callback. Base behaves the same, and the PR discloses it.

6. Hostile classifiers. Each of these keeps its row, logs only the constant warning, and triggers no "never awaited" warning:

   • one that raises an exception whose __str__ is the payload
   • ones that return an async generator or a Future
   • verdicts whose status or error_message was replaced via object.__setattr__ with an object whose __class__ claims str
   • a verdict with a fake __class__ whose status raises
   • a 100 KB error_message
   • an async classifier assigned to config after construction

   The construction check:

   • accepts keyword-only, positional-or-keyword and **kw functions;
   • rejects a missing or extra required argument, coroutine functions, an async __call__, partial(async), AsyncMock, print and non-callables;
   • accepts a sync wrapper that returns a coroutine, which the documented runtime path then closes.

7. The real write path, using the repo's fixtures.

   • content_formatter runs on the new TOOL_ERROR rows.
   • A raising formatter writes the sentinel, and the payload stays out of the logs.
   • A classifier's error_message is redacted (api_key=sk-live-123) and truncated.
   • An event_allowlist without TOOL_ERROR drops the outcome row, as the release note says.

P0

None.

P1

None.

P2 (suggestions, none blocking)

P2-1: A classifier can make a raised exception's row depend on plugin order.

• Where: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:8680-8706 runs the classifier before the built-in rules. With the retry plugin first, the classifier therefore sees the ReflectAndRetryToolPlugin answer.
• Probe: a classifier that returns ToolResultClassification(status="OK") for everything. A raising tool records TOOL_COMPLETED/OK with [retry, bq] but TOOL_ERROR with [bq, retry].
• Scope: any classifier that ends in "else OK" has the same problem. An ERROR verdict likewise changes error_message in one order only.
• Fix: check the response_type == REFLECT_AND_RETRY_RESPONSE_TYPE rule before calling the classifier. That answer stands in for on_tool_error_callback, which the classifier never sees in the other order. test_classifier_ok_overrides_the_builtin_rules (test file line 6736) uses an MCP result, so it is unaffected. If you keep the current order, the tool_result_classifier docs should say the classifier also sees retry-plugin answers and that an OK verdict hides a raised exception.

P2-2: The plugin-order docs claim more than holds.

• Where: the docstring at …plugin.py:4996 ("Registered first, this plugin records each failure once, as TOOL_ERROR, whatever later plugins do with it"), and the PR body ("With the analytics plugin first, a failure answered by ReflectAndRetryToolPlugin is recorded exactly once, as TOOL_ERROR").
• Counterexample: a ReflectAndRetryToolPlugin subclass whose extract_error_from_result flags {"status": "error"}, registered as [bq, retry]. The row is TOOL_COMPLETED/OK, while the model receives the retry plugin's error response.
• Fix: reword to "…records each raised error once, as TOOL_ERROR; a failure that only a later plugin detects in the result is recorded as TOOL_COMPLETED unless tool_result_classifier flags it."

P2-3: The list of plugin-order limits leaves out unknown tool names.

• What happens: with the retry plugin first, a hallucinated tool name is answered in on_tool_error_callback. ADK then skips the after-tool callbacks for lookup errors (src/google/adk/flows/llm_flows/tools/_caller.py:820-838), so the call gets TOOL_STARTING and no outcome row. Base does the same.
• Why it matters: this is a realistic case, since the retry plugin's own guidance covers "Wrong Function Name".
• Fix: list it next to re-raise and extract_error_from_result in the ### Plugin order docstring (:4988).

P2-4: Declining the _detect_error_in_response hooks is acceptable for this PR, but the stated reason is broader than it should be.

• "A tool may also return those as data" holds for FunctionTool's truthy error. It does not hold for ADK's own error envelopes.
• GoogleTool.run_async (src/google/adk/tools/google_tool.py:103) catches every exception and returns {"status": "ERROR", "error_details": str(ex)}. GoogleTool underlies BigQueryTool and the Spanner, Bigtable, Pub/Sub, GCS, Eventarc and Data Agent toolsets.
• Probe: a GoogleTool whose function raises writes TOOL_COMPLETED/OK, while the same call's OTel error.type is TOOL_ERROR. Base does the same.
• So a BigQuery analytics plugin still undercounts failures of ADK's own BigQuery tools.
• Suggested follow-up, either of:
  • a built-in rule for the GoogleTool envelope that takes error_message from error_details, as the retry-answer rule does (it is str(ex));
  • or an opt-in classifier that mirrors the telemetry hooks, with the GoogleTool case named in the release note.

P2-5: The release note leaves out a data change. For reclassified calls, the result is no longer in content, and that includes the MCP error text. Queries that read JSON_VALUE(content, '$.result…') for those calls will stop finding it. The Privacy section of the PR body says this, but the release note does not. Keeping the result out by default is defensible, and you might offer an opt-in to keep it for debugging MCP failures.

P2-6: Two claims need rewording.

• :818-821 and the PR body say classification "runs none of the result's own code". But dict.get calls a stored key's __eq__ on a hash collision. My colliding key that always compares equal turned {key: True} into a TOOL_ERROR row. A colliding key that raises is caught by the outer boundary, so the effect is harmless. Suggested caveat: "except dict keys' own __hash__/__eq__".
• :8690 and the PR body say a returned awaitable is closed. Only coroutines are closed. Other awaitables, such as a Future, are ignored with a warning.

P2-7: Optional hardening of the marker.

• Where: on_tool_error_callback creates weakref.ref(tool_context) (:8736-8739) after popping the span and before _log_event.
• Failure: a context that can't take a weak reference, such as a SimpleNamespace from a custom caller or a test, raises inside _safe_callback, and the TOOL_ERROR row is lost.
• Reachability: no ADK flow reaches this, since ToolContext = Context supports weak references.
• Fix: catch TypeError around the ref and skip the marker; the row is then kept.

P2-8: One test gap.

• Dependency: the marker is correct because each parallel call runs in its own copied context (src/google/adk/flows/llm_flows/tools/_batch_executor.py:171). If two errors interleaved in one shared context, the single slot per instance would be overwritten and a duplicate row written.
• Gap: no test pins this.
• Suggested test: a Runner test with two parallel raising calls and [bq, retry], expecting two TOOL_ERROR rows and no TOOL_COMPLETED, next to test_raised_error_answered_by_retry_plugin_is_recorded_once (test file line 7587). It would guard both the marker and the span stack. A ToolNode case would be cheap to add as well.

P2-9: Title. The PR title and the commits say fix(plugins), but the body recommends feat(plugins). Use the feat title for the upstream squash, so release-please lists the new API and the TOOL_COMPLETED→TOOL_ERROR change under Features.

The design points you flagged

• Leaving the result out of TOOL_ERROR content: acceptable. It matches the rows on_tool_error_callback writes and closes the formatter gap; see P2-5.
• Taking the retry-answer message from error_details: correct. It equals str(error), and both plugin orders agree in my runs, including exceptions with no message.
• The classifier signature (four keyword-only arguments): good.
• Toolbox and Skill detection through sys.modules and class names: correct for toolbox-adk 1.4.0. It exports ToolboxTool at the top level, and ToolboxTool's MRO has no FunctionTool. The enumeration test guards the skill tool names.
• The per-call marker: sound on every ADK path I exercised: the LLM flow, the live path (which shares _execute_single_prepared_call), ToolNode, node retry and nested AgentTool.
• The event_allowlist and dashboard change: documented.

VERDICT: APPROVE @ fa56c26

[caohy1988]

Independent Astra review of head 92ff4bb2233c8fae540bca37dcb09cd204f53ee9 against def458b609c2811d137b0332b2fc7b201dddd5c0. Submitted as COMMENT as requested.

Rejection criteria, written before reading the diff

1. A valid error signal is missed, or hostile result/classifier values drop or duplicate an outcome row.
2. New error rows leak payload text or bypass formatting/redaction.
3. Plugin ordering or execution paths violate the stated contract without a documented, acceptable limit.

P1 — The completion fallback still drops hostile outcomes and logs their payload-bearing exceptions

Location: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:8693 (completion serialization), with the new unconditional row guarantee at :8647; the escaping type check is at :1890, and _safe_callback logs its traceback at :568.

The classification boundary ends before _recursive_smart_truncate(result, ...). A result whose __class__ access raises passes the new classification safely, then raises from the sanitizer's unguarded isinstance before its own try. _safe_callback logs the payload-bearing exception and returns without an outcome row. The span has already been popped.

I reproduced all three cases through the real mocked BigQuery row-writing fixtures:

• An opaque result with a raising __class__ property.
• A valid CallToolResult(isError=False) subclass whose attribute access raises after construction.
• A CallToolResult(isError=True) of that subclass with the new classifier returning ToolResultClassification(status="OK").

For each, the actual assertion result is (['TOOL_STARTING'], True), where the boolean means the synthetic payload text appeared in captured logs. Required: (['TOOL_STARTING', 'TOOL_COMPLETED'], False). The third case exercises the new classifier's documented override path. The error-flagged model without that override correctly produces TOOL_ERROR.

Evidence: /tmp/adk-pr15-dual/r3/astra-probes/test_completion_boundary.py and completion-boundary.log — 3 failures. Reproduce with this worktree's .venv-astra/bin/python, PYTHONPATH=$PWD/src:$PWD:/tmp/adk-pr15-dual/r3/astra-probes, and -m pytest /tmp/adk-pr15-dual/r3/astra-probes/test_completion_boundary.py -q -p no:cacheprovider.

Scope distinction: I also reproduced the underlying opaque-result serializer defect with the base plugin loaded separately. It is inherited, not a newly introduced serializer regression, and the PR body acknowledges part of it. However, the requested round-2/round-3 acceptance contract explicitly requires hostile results to retain their outcome row without payload logging. Documentation alone does not meet that requirement; this is why I am treating it as blocking for this review.

Suggested fix: contain completion-payload preparation in a fail-closed boundary, substitute a constant sentinel and mark truncation on failure, then continue through the normal formatter/redaction and TOOL_COMPLETED writer. Log only constant text after leaving the exception handler. Add the successful hostile MCP and classifier-override cases alongside the existing error-flagged hostile model tests.

P0 / other P1 / P2 findings: None.

What I tried and verified

• Read the complete two-file diff, the three implementation commit messages, the PR description, issues google#7112/google#485, and relevant tool, workflow, plugin-manager, streaming, and AgentTool code. No branch modifications.
• Fresh uv environment in this worktree, Python 3.11.13. Full frozen all-extras setup hit the anticipated unsupported google-antigravity wheel and a locked cryptography source-build failure; used a scoped dependency install with BigQuery, MCP, Toolbox, A2A and test dependencies. This was not a full locked all-extras environment.
• Required plugin suite: 641 passed with -n 8 -q, no skips after installing A2A. Broader plugins + ToolNode + McpTool + AgentTool run: 1,115 passed (includes that suite). Separate live/long-running/parallel/streaming-flow selection: 65 passed.
• Own adversarial matrix: 106 passed under MCP 2.2.0, and 106 passed with MCP 1.30.0 selected through an isolated package directory. It covers dicts, dict subclasses, Pydantic dumps, actual MCP/Pydantic model subclasses, raising attribute/equality/truth hooks, colliding keys, numpy/pandas values, malformed classifier returns, awaitables, and successful controls. Valid built-in error signals survived all tested hostile sibling fields.
• The same probes verified actual row content, formatter success/failure, payload-column denial, and the documented TOOL_COMPLETED-only allowlist behavior. Classifier exceptions and malformed verdicts used content-free warnings. Native returned coroutines were closed.
• Two distinctly named analytics instances in a real runner, 24 mixed asynchronous calls per run, three plugin orders, retry guidance and final responses: each instance recorded exactly one outcome per call; message-less exceptions agreed. Six additional live terminal-result probes passed.
• Separate base/head comparisons confirmed the MCP dict/model fix, retry/final-response fix in both orders, removal of the duplicate completion, and nested AgentTool outcome counts. They also reproduced the documented inherited limits for retry re-raise, unknown tool names, short-circuiting after-tool plugins, and resumed long-running results.
• PR-body mutation script: 40/40 mutations caught; no survivors, rerun against temporary source copies. This confirms the reported mutation result, but does not cover the three additional failing completion cases above.
• Pinned pyink 25.12.0 --check and isort 8.0.1 --check-only both passed for both changed files. No live BigQuery service test, full all-extras suite, mypy, or complete pre-commit rerun is claimed.

Design judgments

Leaving the result out of TOOL_ERROR is appropriate for the existing formatter/privacy contract. Reflect retry messages use the same exception text as the raised-error path; redaction remains in the writer. Keyword arguments and construction-time classifier validation are appropriate. Toolbox and the six current Skill tool classes were correctly recognized without importing optional modules during classification.

The explicitly documented plugin-order limitations are acceptable framework constraints for this change. Keep analytics first. The strict boolean MCP rule and not invoking arbitrary per-tool detection hooks are defensible: broad truthy error rules would change successful application data and output retention. A built-in GoogleTool envelope rule and an opt-in to retain results are useful follow-ups, neither is a blocker here. Event-allowlist/dashboard migration is clearly described. The hostile completion fallback above is a separate, explicit acceptance-contract failure.

Blindness disclosure: no peer review text, review results, forbidden files, or PR review/comment bodies were read. An incidental process-status diagnostic exposed a peer launch command containing task instructions and another pytest command of unknown provenance; no review conclusions or test results were exposed.

VERDICT: REQUEST_CHANGES @ 92ff4bb

[caohy1988]

Fable blind review, round 3: PR #15 @ 92ff4bb

Independent blind review (Claude Fable) of head 92ff4bb2233c8fae540bca37dcb09cd204f53ee9 against base def458b609c2811d137b0332b2fc7b201dddd5c0. Code changes reviewed: f187aef, 563d32c, 92ff4bb (the two merges touch no plugin files). I read only the PR description, the diff, issues google#7112 and GoogleCloudPlatform/BigQuery-Agent-Analytics-SDK#485, the repository code, and CI check logs. I read no other review or comment.

Outcome: one narrow P1: a row-loss regression in the new per-call marker, with a five-line fix that I prototyped and tested. Everything else I threw at the change held up. Details below.

Rejection criteria (written before I read the diff or the PR body)

• R1 Row integrity. Each tool call gets exactly one outcome row (TOOL_COMPLETED or TOOL_ERROR). Reject on a lost row, a duplicate (TOOL_ERROR + TOOL_COMPLETED, or two TOOL_ERRORs), or an exception escaping a callback. This applies in either order relative to ReflectAndRetryToolPlugin, with two analytics instances, with parallel calls, and with hostile results or classifiers.
• R2 Classification correctness. Reject if a well-formed error signal is recorded as TOOL_COMPLETED/OK, particularly next to a hostile or malformed sibling field. The signals are MCP isError=True as a dict, a dict subclass or a CallToolResult model, and a reflect-and-retry answer. Also reject if a successful result becomes TOOL_ERROR, or if a Toolbox or Skill tool_origin is wrong.
• R3 Payload safety, API contract, evidence. Reject if a new row or warning carries payload-derived text, or if the new rows bypass content_formatter or redaction. Also reject if the classifier contract isn't what ships, or if the core claims have no test that fails on unfixed code.

Result. R1 is met, narrowly, by P1-1 below. R2 and R3 are not met.

What I tried

Environment: Python 3.11.13 (macOS x86_64), pydantic 2.13.5, mcp 2.2.0 (SDK 2.x, where mcp.types.CallToolResult is mcp_types.CallToolResult and the field is is_error), toolbox-adk 1.4.0.

• pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8: 641 passed.
• tests/unittests/plugins, workflow/test_tool_node.py and tools/mcp_tool/test_mcp_tool.py: 1062 passed. Wider set: every test file that references the plugin (cli/test_fast_api.py, test_optional_dependencies.py), plus plugins/, workflow/, tools/mcp_tool/, flows/llm_flows/, tools/test_agent_tool.py and tools/test_skill_toolset.py: 3689 passed, 12 skipped, 6 xfailed.
• pyink 25.12.0 and isort 8.0.1 (the pre-commit pins) report both files clean. ruff is clean, and mypy on the plugin reports no issues.
• Do the tests fail on unfixed code? The test module subclasses ToolResultClassification at import time, so I ran it against an API-only shim instead of reverting in place: base def458b6 plus only the new public names, with no behavior. 70 of the 89 new tests fail and no pre-existing test fails. The 19 that pass are guards that should pass on base: results without a failure signal stay TOOL_COMPLETED, the dataclass validates itself, toolbox_adk isn't imported, a context without weak references keeps its row, and the next call isn't skipped.
• Mutation check. I reran the PR's mutate.py (0–39) from a temporary copy. All 40 mutations are caught, with the same per-mutation failure counts as the PR body.
• CI. All five Unit Tests jobs on this head were canceled at the workflow's 10-minute timeout-minutes, around 94%, each showing one F in the progress dots and no summary. The fork's main push e4c0d946 shows the identical pattern (one F per job, then canceled), so this isn't attributable to the PR. It does mean CI gives no evidence here, and upstream CI will need to be green. Mypy, pre-commit, MCP v2 and A2A jobs pass.
• My probes (739 cases in /tmp/adk-pr15-dual/r3/fable-probes/test_fable_r3_probes.py, run under -p no:logging with a stock StreamHandler and a swapped sys.stderr, and hostile hooks armed only during the callback). Result at head: 732 passed, 3 failed (P1-1), 4 xfailed (identical on base, P2-1). With my P1-1 fix prototype: 735 passed, 4 xfailed.

  • Hostile-sibling matrix, 692 cases.

    • Result shapes:
      • a plain dict;
      • a dict subclass where every method raises, including __getattribute__;
      • an MCP CallToolResult;
      • a CallToolResult subclass with raising __getattribute__, __eq__, __bool__ and __iter__;
      • a CallToolResult subclass under a pydantic metaclass subclass with raising __instancecheck__, __subclasscheck__ and __getattribute__.
    • Signals: isError, is_error, a reflect answer, and a reflect answer with hostile error_details.
    • Hostile values in every other wanted slot and in unrelated slots (content, structured_content, meta):
      • an object whose every dunder raises;
      • str subclasses whose every hook raises;
      • metaclass-hostile classes, plain and str-derived;
      • a raising __class__ property;
      • a numpy array, np.True_, pd.NA, a pandas Series, NaN.
    • Keys that hash like each wanted name and raise on comparison.
    • Result: the valid signal always wins, with the correct message. str subclasses count by value, so one holding the retry marker reads as a retry answer. No payload reaches the log, stderr or the rows, and no rule-failure or "Could not classify" warning fires.

  • End-to-end ordering with real runners. Plugin orders: [bq, reflect], [reflect, bq], [bq1, reflect, bq2], [reflect, bq1, bq2] and [bq1, bq2, reflect]. One model turn makes 5 parallel calls: two raising, an MCP dict with isError, an MCP model with isError, and one success. Every analytics instance writes exactly 5 outcomes, with the right types and messages. These cases also passed:

    • retry exceeded with max_retries=0, throw_exception_if_retry_exceeded=False, in both orders;
    • an agent-level on_tool_error_callback answering;
    • the same tool raising and succeeding in parallel, in both orders;
    • a message-less exception in both orders (RuntimeError either way);
    • a workflow ToolNode with RetryConfig: it fails then succeeds, giving TOOL_ERROR then TOOL_COMPLETED, and the failed attempt's mark doesn't suppress the retry because each attempt gets a new Context;
    • a nested AgentTool with shared plugins in both orders: the inner TOOL_ERROR is written once and the outer TOOL_COMPLETED once.

    The documented "unknown tool name with reflect ahead" gap reproduces as stated.

  • Classifier:

    • swapped to async after construction: the row is kept, a warning is logged, and there's no "never awaited";
    • KeyboardInterrupt, SystemExit, CancelledError and GeneratorExit propagate, as documented;
    • signature variants behave as documented;
    • the verdict message is redacted.

  • New rows versus formatter, denylist and allowlist:

    • content_formatter receives TOOL_ERROR with {tool, args, tool_origin} and no result, and its output is used;
    • a raising formatter gives the [FORMATTER_FAILED] sentinel, keeps the row, and leaks nothing;
    • payload_column_denylist=["content"] nulls the content;
    • an allowlist without TOOL_ERROR drops just that row, with no error.

  • A real _dump_mcp_model(CallToolResult(isError=True)) is recorded as TOOL_ERROR, and the MCP text is absent.

Findings

P1-1: a raised error whose __str__ raises now leaves the call with no outcome row

src/google/adk/plugins/bigquery_agent_analytics_plugin.py:8845-8869. on_tool_error_callback pops the span (8845) and marks the call (8848, 8855). Only afterwards does it evaluate str(error) or type(error).__name__ (8869), inside the arguments of _log_event. If str(error) raises, _safe_callback swallows it and no TOOL_ERROR row is written, but the mark stays. When a later handler answers the error, after_tool_callback (8628-8636) sees the mark and skips. The call ends with TOOL_STARTING and no outcome, in the plugin order the PR recommends. This is the same class of hazard 92ff4bb already fixed for contexts that can't take a weak reference.

Analytics registered first; tool raises ValueError(obj) where obj.__str__ raises	base def458b6	head 92ff4bb2	head + fix below
no handler answers	TOOL_STARTING	TOOL_STARTING	TOOL_STARTING, TOOL_ERROR
a later handler answers (unit)	TOOL_STARTING, TOOL_COMPLETED/OK	TOOL_STARTING only	TOOL_STARTING, TOOL_ERROR
E2E: LlmAgent(on_tool_error_callback=...) answers	TOOL_STARTING, TOOL_COMPLETED/OK	TOOL_STARTING only	TOOL_STARTING, TOOL_ERROR ("ValueError")

ReflectAndRetryToolPlugin itself calls str(error), so with it the run raises RuntimeError and the call keeps only TOOL_STARTING, at base and head alike (probed). The regression shows with any other answerer, such as an agent callback or a custom plugin. This is narrow, but it breaks the PR's own invariants ("exactly one row per tool call", "nothing hostile ever drops the row") in code this PR changed. It also meets R1.

Suggested fix, prototyped in a copy of src. It passes my three probes and the PR's 641 tests. Compute the message before marking the call:

    span_id, duration = TraceManager.pop_span()
    parent_span_id, _ = TraceManager.get_current_span_and_parent()
    # Computed before the call is marked: once it is marked,
    # after_tool_callback skips the call, so nothing that can fail may run
    # between the mark and the row.
    try:
      error_message = str(error) or type(error).__name__
    except Exception:  # pylint: disable=broad-except
      error_message = type(error).__name__
    try:
      recorded_call = weakref.ref(tool_context)
    ...
        event_data=EventData(
            status="ERROR",
            error_message=error_message,

Please add a regression test (unit, plus E2E with an agent-level on_tool_error_callback) asserting exactly [TOOL_STARTING, TOOL_ERROR] with error_message == "ValueError".

P2-1 (pre-existing, identical on base): hostile results without an error signal still lose their row and log the payload

Four shapes lose the TOOL_COMPLETED row at both head and base:

• a dict subclass with raising methods and no flag, e.g. _RaisingMethodsDict({"rows": 1});
• an object with a raising __getattribute__;
• an object with a raising __class__;
• a CallToolResult subclass with a raising __getattribute__ and isError=False.

In each case _recursive_smart_truncate(result, ...) (8693) raises, and _safe_callback's logger.exception (568) logs a traceback that carries the payload text. The TOOL_ERROR path is safe because it never serializes the result. This isn't a regression. But the PR body's "Results with a raising ==, raising get or other methods … keep their row. No payload reaches the log" holds only when the result carries an error signal, and the "Pre-existing and unchanged" list names only the __class__ case. Please either narrow that wording or, as a follow-up, put the TOOL_COMPLETED serialization behind the same constant-warning boundary with a sentinel result.

P2-2: gaps in construction-time classifier validation (the runtime boundary still copes)

• functools.partial(obj) where obj.__call__ is async def passes _validate_tool_result_classifier (1732-1739). Neither inspect.iscoroutinefunction(partial) nor the partial's own __call__ detects it. At run time the coroutine is closed with a warning.
• A classifier whose __signature__ raises something other than TypeError/ValueError (e.g. a property raising RuntimeError) escapes inspect.signature (1742-1744) as that exception instead of the documented ValueError.

Suggested fixes: unwrap partials before the __call__ check, and catch Exception around inspect.signature so it re-raises as ValueError.

P2-3: design notes (non-blocking)

The MCP server's error text no longer appears anywhere in the analytics row. The message is fixed and the result is omitted, so MCP users lose some debuggability compared with main. The privacy rationale is sound, and the release note and the "keep-result opt-in" follow-up cover it.

ADK-generated envelopes still record TOOL_COMPLETED/OK: GoogleTool's {"status": "ERROR", "error_details": ...} and FunctionTool's own {"error": "Invoking X() failed ..."} for missing mandatory args. I'd list the FunctionTool case next to the GoogleTool follow-up.

On the design points the implementer flagged

• Result omitted from TOOL_ERROR: agree, with the P2-3 caveat.

• Reflect message from error_details: agree. ReflectAndRetryToolPlugin sets error_details=str(error) on both the retry and the retry-exceeded answers, which matches on_tool_error_callback, and redaction applies.

• Keyword-only classifier signature: agree, apart from the P2-2 nits.

• Toolbox/Skill detection through sys.modules and class names: acceptable. toolbox_adk.ToolboxTool subclasses BaseTool directly, so placing the check after FunctionTool is safe. The skill test enumerates every BaseTool subclass in skill_toolset, so a new tool class fails loudly.

• Marker relying on a shared context: sound across these flows:

  • _caller.py: on_tool_error and after_tool run in one _run_with_trace coroutine, and each parallel call is a task holding a copied context (_batch_executor.py:186);
  • live mode uses the same pipeline;
  • _tool_node.py uses one coroutine, with a new Context per retry attempt;
  • nested AgentTool.

  The one exception is P1-1's ordering.

• Dashboard/event_allowlist change: acceptable as a feat with the release note. Probes behave as the release note says.

• Not aligning with _detect_error_in_response: acceptable. FunctionTool's truthy-error rule would flag data and confirmation envelopes. For real McpTool dumps, isError is a JSON bool, so is True equals McpTool's truthiness.

• Deferred GoogleTool rule and keep-result opt-in: not blockers. Both are outside google#7112's stated scope, and a classifier covers the envelope today.

• BaseException from a classifier propagates: acceptable deviation from "never crash the callback". The span has already been popped, so the stack stays consistent.

Independence

I read no reviews or comments on this PR. My local session memory holds general test-harness notes from earlier sessions: the hook-free-read and probe techniques, plus a one-line mention of earlier rounds of this review. I read no earlier-round results, probes or peer material.

VERDICT: REQUEST_CHANGES @ 92ff4bb

[caohy1988]

Blind Astra review of d5b8ecbb1f846b5d920d6d151c693abf18433434, against def458b609c2811d137b0332b2fc7b201dddd5c0. This is a COMMENT review, as requested. No other review or earlier-round artifact was consulted.

Rejection criteria, set before reading the diff

1. A supported tool outcome reaching the callbacks is lost, duplicated, or assigned the wrong outcome/status; successful results or raised-error handling regress.
2. A new result/classifier/serialization path exposes payload text beyond the existing TOOL_ERROR privacy contract, including through process logs.
3. A claimed fail-closed or lifecycle guarantee fails a targeted probe. Under the final-round cap, hostile-class-only residuals are P2 follow-ups; REQUEST_CHANGES requires P0/P1.

P0 findings: None.

P1 — Consume failures from returned Future/Task objects before discarding them.

Location: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:8819–8828.

Classification (1): a real payload leak using ordinary Python functions, dictionaries, ValueError, asyncio.Future and asyncio.Task; no hostile/adversarial class is involved. It requires an incorrectly implemented synchronous classifier returning an awaitable, which is precisely a malformed return this new boundary attempts to contain.

Construction accepts the following synchronous classifier with the required keyword signature:

def classify(*, tool, tool_args, tool_context, result):
    future = asyncio.get_running_loop().create_future()
    future.set_exception(ValueError(result["private_text"]))
    return future

With result {"isError": True, "private_text": "ASTRA_R4_PRIVATE_PAYLOAD_2391"}, the callback correctly writes one TOOL_ERROR, but drops the Future without retrieving its exception. Python then emits:

Future exception was never retrieved
future: <Future finished exception=ValueError('ASTRA_R4_PRIVATE_PAYLOAD_2391')>
ValueError: ASTRA_R4_PRIVATE_PAYLOAD_2391

A sync wrapper returning asyncio.create_task(...), whose async computation raises ValueError(result["private_text"]) after yielding, similarly logs Task exception was never retrieved plus the payload and traceback. I reproduced both through the actual row pipeline with a scrubbing content_formatter and payload_column_denylist=["content", "content_parts"]: the rows contain no canary, but the process log does. A constant warning from this plugin does not contain the later asyncio diagnostic. The PR body's explicit decision to ignore Futures as-is leaves the malformed-classifier privacy guarantee unmet.

Suggested fix: explicitly handle standard asyncio Future/Task returns: retrieve a completed failure without logging its exception, and attach a cancellation-aware, exception-consuming completion callback for pending ones. Preserve the built-in classification fallback and constant warning; do not arbitrarily cancel caller-owned work. Add regression tests for both already-failed Futures and Tasks that fail after the callback returns, checking the asyncio log as well as the plugin log and row. This is one finding with two reproductions, not two separate blockers.

Reproduction artifacts: /tmp/adk-pr15-dual/r4/astra-probes/test_independent.py, tests test_failed_standard_awaitable_does_not_leak[future] and [task]; output in independent.log. A standalone reproduction is future_probe.py in the same directory.

P2 findings: No additional finding retained. The disclosed pre-existing hostile-class residuals are not promoted to blockers.

What I checked

• Read the PR description, all four implementation commit messages, the complete two-file base-to-head diff, issues google#7112 and BigQuery-Agent-Analytics-SDK#485 including their allowed issue comments, and the relevant runner, callback, workflow, tool, and logging code.
• Requested command: PYTHONPATH="$PWD/src" .venv/bin/python -m pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q: 654 passed.
• tests/unittests/plugins plus tests/unittests/workflow/test_tool_node.py, -n 8 -q: 966 passed, including the previous 654.
• MCP tool tests plus test_live_tool_callbacks.py and test_plugin_tool_callbacks.py, -n 8 -q: 150 passed.
• Independent suite: 86 passed, 2 failed, both failures being the P1 above. This includes a 48-case sibling-field matrix over dicts, hostile dict subclasses, Pydantic dumps, and MCP model subclasses; raising attribute/class/comparison/truth hooks; numpy/pandas fields; colliding keys; strict non-bool flags; unreadable success/error MCP models; malformed/raising classifiers; coroutine closure; partial async/async-generator callable rejection; signature failures; formatter failures, redaction, and payload denylists.
• Independent actual-runner probes exercised 12 async calls held at a barrier, mixing successful results, MCP errors, and message-less raised errors. Both plugin orders and one/two distinctly named analytics instances retained exactly one outcome per call per instance. max_retries=0, throw_exception_if_retry_exceeded=False also exercised final retry responses. The shipped tests additionally cover ToolNode, unreadable exception text answered by an agent callback, classifier-forced OK, span pairing, and final-response tools.
• Independent live terminal tools classified MCP errors correctly. Probes confirmed the documented limits for streaming chunks, resumed long-running responses, and a retry subclass short-circuiting after-tool callbacks. Nested AgentTool runs retained their inner/outer outcome rows in both orders; the separately documented shared span-stack limitation remains out of scope.
• An independent runner reproduction passed in both orders at head and failed in both with only the plugin replaced by the base version in a temporary source copy: base mislabels MCP/retry results and, with analytics first, emits the duplicate TOOL_COMPLETED after TOOL_ERROR. No tracked worktree file was replaced.
• Pinned pyink 25.12.0 and isort 8.0.1 checks passed on both changed files. mypy passed on the plugin.

Design/scope assessment

Omitting the result from TOOL_ERROR is acceptable given existing event-specific formatters; a keep-result opt-in is not a blocker. Reflect error_details matches the existing exception message path, including the tested message-less fallback. Keyword invocation, instance-specific weak markers, and lazy Toolbox/Skill type detection are reasonable for the current code; existing origin tests passed with the real installed Toolbox package. The documented recommendation to register analytics first is necessary because PluginManager stops callback dispatch; the limitations are not silently presented as universal order independence.

Keeping built-in rules narrower than _detect_error_in_response is acceptable for this scoped fix: arbitrary truthy error fields need not be failures, and the MCP contract uses a boolean flag. A GoogleTool envelope rule would be useful, but its pre-existing undercount and the keep-result opt-in can remain follow-ups. The dashboard/event_allowlist migration and lost result column are material behavior changes and are explicitly disclosed in the proposed release note.

Validation limits and workspace

Python 3.11.13; this worktree's uv environment. All-extras sync excluded the three permitted unavailable packages. Pinned cryptography 50.0.1 also failed its native build (missing x86_64 Rust target/OpenSSL); validation retained installed cryptography 48.0.1, so this was not a fully locked dependency environment. No live BigQuery, external MCP server, or remote model was contacted. I inspected the supplied mutation report/script but did not independently rerun all 51 mutations. No tracked files were modified; HEAD and the tracked worktree remain unchanged. No push or merge was performed.

Criterion 2 is met by the standard Future/Task leak; the other exercised behavior passed within the documented scope.

VERDICT: REQUEST_CHANGES @ d5b8ecb

[caohy1988]

Fable blind review: PR #15 round 4 @ d5b8ecb

This is a blind review. I read the PR description, the diff against def458b6, issues google#7112 and google#485, and the repository code. I read no other review or comment on this PR.

Rejection criteria (written before reading the diff)

• R1, row integrity. Reject if any tool call reachable with ordinary tools, results or errors ends with zero outcome rows (TOOL_COMPLETED / TOOL_ERROR), or with more than one, for any analytics plugin instance. That covers MCP isError results, ReflectAndRetryToolPlugin answers in either plugin order, raising tools, parallel calls, AgentTool nesting, and a failing or odd classifier. Also reject if the new code can raise out of a plugin callback into the agent run.
• R2, classification. Reject if a successful result becomes TOOL_ERROR. Also reject if an error the PR claims to handle stays TOOL_COMPLETED / OK, including when a sibling field is malformed. The claimed errors are an MCP result with isError True, as a dict or a CallToolResult model, and a retry answer. Reject too if raising-tool behavior changes beyond what is documented.
• R3, leak or false claim. Reject if a TOOL_ERROR row or a warning carries payload-derived text beyond what TOOL_ERROR already records, or if content_formatter and redaction stop holding for the new rows. Also reject if a concrete claim in the PR body, a docstring or a commit message is false or has no test behind it.

None of the three is met. Evidence follows.

What I ran

Environment: Python 3.11.13 (worktree venv) and 3.10.16 (a fresh dependencies-only venv in /tmp), MCP SDK 2.2.0, pydantic 2.13.5.

• tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8: 654 passed on 3.11 and 654 passed on 3.10. The PR body's other two counts reproduce exactly: 815 with the reflect-retry and MCP tool files, and 966 for tests/unittests/plugins plus workflow/test_tool_node.py.
• The new tests fail on unfixed code. I ran this head's test file against base def458b6 with only the new public names added: ToolResultClassification, ToolResultClassifier and the config field, with no behavior. 82 of the 114 TestToolResultErrorClassification / TestToolProvenance cases fail. The 32 that pass are the existing provenance tests, the dataclass validation tests, and the intended "unchanged" guards.
• The PR's mutation check (mutate.py, rerun from the PR body): 51 of 51 mutations caught, with the same failure counts the body reports.
• Static checks: pyink 25.12.0 --check, isort 8.0.1 --check-only, ruff 0.15.17, codespell 2.4.2 and mypy are clean on both files. The worktree was never modified.
• My own probes: 101, kept outside the repo. All pass at this head on 3.10 and 3.11. Against base, 16 of the 20 end-to-end probes fail, so they discriminate.

What I tried to break

1. Hostile sibling fields beside a valid error signal (19 cases).

   • Results: dicts and dict subclasses with isError / is_error True. Some subclasses have raising get, items, __iter__ or __getitem__, a raising __getattribute__, or a raising __class__.
   • Siblings: fields whose __class__, __eq__, __bool__, __str__ or metaclass hooks raise; numpy and pandas values; keys that collide with isError, response_type or error_details and raise when compared.
   • Retry answers whose response_type or error_details is hostile or a str subclass.
   • MCP 2.x CallToolResult built with model_construct around hostile content.
   • CallToolResult subclasses with a raising __getattribute__, a raising __class__, or a ModelMetaclass subclass whose __instancecheck__, __subclasscheck__ and __getattribute__ raise.

   Every case gave exactly TOOL_STARTING then TOOL_ERROR, with the expected message, a paired span and {tool, args, tool_origin} content. The marker text never reached the rows, a real StreamHandler (run with -p no:logging), or stderr.

2. Successful and odd results (25 shapes).

   • isError False beside the same hostile siblings.
   • isError set to np.True_, 1 or "true", and a (str, Enum) key.
   • Hostile dicts and objects that cannot be serialized.
   • An MCP model with is_error False, both plain and as a hostile subclass.
   • A non-MCP pydantic model with an isError field.
   • DataFrame, ndarray, set, generator, bytes, datetime, Decimal and None.
   • A list of MCP dumps, {"error": …} and {"error_details": …}.

   All kept one TOOL_COMPLETED / OK row. The unserializable ones got the sentinel and the constant warning, and nothing payload-derived was logged. By design, the flag must be exactly True under an exact-str key; McpTool's JSON dump always produces a bool.

3. Plugin orders, end to end. One model turn made six parallel calls: raising, raising without a message, MCP dict error, MCP model error, success, and MCP success.

   • Orders: [bq, reflect], [reflect, bq], [bq1, reflect, bq2], [reflect, bq1, bq2], [bq1, bq2, reflect], and [bq] and [bq1, bq2] with an agent on_tool_error_callback that answers. The second instance is renamed, because PluginManager rejects duplicate names.
   • Wrapping each instance's _log_event showed exactly one outcome row per call per instance.
   • The messages were the same in every order: boom, ValueError and the fixed MCP text.
   • Spans were paired in single-instance runs, and the model received the MCP result unchanged.
   • On base, the same probe shows TOOL_ERROR plus a spurious TOOL_COMPLETED with the analytics plugin first, whether the retry plugin or an agent callback answers.

4. Classifier in both orders. Always-OK and always-ERROR classifiers still leave raised errors as TOOL_ERROR, and the verdict decides every other call.

5. Retry cap.

   • With throw_exception_if_retry_exceeded=False, three calls give three TOOL_ERROR rows in either order.
   • With the analytics plugin first, a re-raise still leaves one TOOL_ERROR.
   • With the retry plugin first, a re-raise leaves no outcome row, which is the documented limitation.

6. Nesting and workflows.

   • AgentTool with shared plugins, in both orders.
   • A fan-out of three parallel ToolNodes, two of them failing, in both orders.
   • A ToolNode with retry_config whose first attempt fails unanswered and whose second succeeds. It records TOOL_ERROR then TOOL_COMPLETED: the leftover entry never skips the new attempt, which gets a fresh Context.

7. Real McpTool. With a mocked session returning CallToolResult(is_error=True), the row is TOOL_ERROR with tool_origin MCP, and the MCP text is absent from it.

8. The LLM_REQUEST privacy claim. By default the MCP text appears only in the later LLM_REQUEST's content_parts. With log_multi_modal_content=False, or with content_parts denied, it appears nowhere.

9. Classifier validation, at construction and at run time.

   • Rejected with ValueError (16 shapes):

     • asynchronous callables: a coroutine function, a partial or nested partial of an object with an async __call__, an async-generator __call__, a bound async method, AsyncMock, and a partialmethod async __call__;
     • wrong parameters: positional-only, missing or extra required keywords;
     • no readable signature: a raising __signature__, and max;
     • other: a non-callable, and the ToolResultClassification class itself.

     The same holds when the classifier is passed as a plugin keyword argument.

   • Accepted (6 shapes): a keyword-only function, a bound method, a **kwargs lambda, a partial that binds an extra keyword, MagicMock, and None.

   • At run time:

     • returned coroutines and generator-based coroutines are closed, with no "never awaited" warning;
     • str, dict and MagicMock verdicts and a raising classifier fall back to the built-in rules with the constant warning;
     • the classifier's error_message is recorded.

10. Marker and concurrency, from reading the flows.

    • Each LLM-flow call runs in its own task, created from a copy of its prepare snapshot (_batch_executor._start_execute_task).
    • PluginManager awaits callbacks inline.
    • Workflow retries build a new Context for each attempt (_node_runner._create_child_context).

    So a marker entry lives only in the failing call's task and matches only that call's context. I found no ADK flow in which a second failure in the same task could overwrite it before the first call's after_tool_callback.

11. Unreadable error messages. An exception whose __str__ returns a str subclass with raising __bool__ / __len__ still records exactly one TOOL_ERROR, with its type name.

Findings

P0: none. P1: none.

P2 follow-ups (none blocks this PR):

1. Python 3.14 rejects a correctly typed classifier at construction (src/google/adk/plugins/bigquery_agent_analytics_plugin.py:1752).
   • Category: compatibility. It is neither a leak nor a hostile class, and it fails loudly at startup rather than losing rows.
   • Cause: on 3.14, annotations are evaluated lazily, so a classifier can annotate tool: BaseTool with BaseTool imported only under TYPE_CHECKING. inspect.signature() then raises NameError, which the check turns into "must have a signature that inspect can read".
   • Evidence: I ran _validate_tool_result_classifier, copied verbatim from this head, on CPython 3.14.0a6. It rejects such a classifier with cause NameError: name 'BaseTool' is not defined. inspect.signature(..., annotation_format=annotationlib.Format.FORWARDREF) reads the same function.
   • Scope: on 3.10–3.13 such a function cannot even be defined, so only 3.14 is affected. Worth confirming on 3.14 final.
   • Fix: on 3.14 and later, call inspect.signature(classifier, annotation_format=annotationlib.Format.FORWARDREF).
2. McpTool's default error envelope is still TOOL_COMPLETED / OK.
   • Category: an undercount that predates this PR and lies outside its done-when list.
   • Cause: _MCP_GRACEFUL_ERROR_HANDLING is on by default (src/google/adk/features/_feature_registry.py:191). So McpTool.run_async returns {"error": "MCP tool execution failed: …"} or {"error": "Unexpected error during MCP tool execution: …"} (src/google/adk/tools/mcp_tool/mcp_tool.py:606, :611) instead of raising, for McpError and transport failures such as a gateway 403.
   • Evidence: a real McpTool whose session raises McpError(-32000, "403 denied") records TOOL_STARTING, then TOOL_COMPLETED / OK. McpTool._detect_error_in_response doesn't flag this envelope either, so the PR is consistent with the tool's own hook.
   • Why it matters: this is probably the most common MCP failure in production, yet the PR body's follow-up list names only the GoogleTool and FunctionTool envelopes.
   • Suggestion: add it to that list. A low-risk rule: tool origin MCP, and a plain dict whose only key is "error" with an exact-str value, becomes TOOL_ERROR with a fixed message.
3. Doc nit: the dict rule is not limited to MCP tools (bigquery_agent_analytics_plugin.py:873-883, :8782).
   • Any tool's dict whose top-level isError / is_error is exactly True is recorded as TOOL_ERROR with "Tool returned an MCP result with isError=true."
   • That is intended: the google#7112 reproduction uses a FunctionTool returning that shape.
   • The docstrings, though, call it "an MCP CallToolResult … as a dict dump". Suggest "any dict result whose top-level isError / is_error is True".

Not counted: google#485's ADK-side item is untouched here and is not in this PR's done-when list. That item is a content_formatter failure writing [FORMATTER_FAILED] with a null error_message.

Design points the implementer flagged

• Result left out of TOOL_ERROR: agree. A formatter written for TOOL_COMPLETED rows would not scrub it. The text is still in the next LLM_REQUEST's content_parts by default (probed).
• Retry-answer message taken from error_details: agree. It is the same str(error) that on_tool_error_callback records, and the type-name fallback makes both orders record the same text (probed in both orders).
• Classifier signature: fine. The keyword-only (tool, tool_args, tool_context, result) mirrors after_tool_callback.
• Toolbox and Skill detection through sys.modules: fine. It avoids importing an optional package, and test_skill_toolset_tools_return_skill enumerates every BaseTool subclass in skill_toolset, so a new skill tool cannot silently become UNKNOWN.
• Per-call marker in a contextvar: sound for every current flow (item 10 above).
• event_allowlist and dashboard change: documented in the release note, and the suggested feat squash title fits.
• Not reusing the _detect_error_in_response hooks: acceptable. FunctionTool's hook (function_tool.py:437) flags any truthy "error" key. That includes the HITL confirmation pause, {"error": "This tool call requires confirmation, …"} (function_tool.py:411), which is not a failure.
• Deferred GoogleTool envelope rule and keep-result opt-in: not blockers. Both lie outside this contract, a classifier covers the first today, and by default the MCP text is still in LLM_REQUEST content_parts.

VERDICT: APPROVE @ d5b8ecb

[caohy1988]

Blind Astra review of head 86d62bf0367ac68fe18570757dce46bce63971ad (round 5). I read the PR description, the complete specified base-to-head diff, the two source issues, and repository code; I did not read peer reviews or earlier-round artifacts.

Rejection criteria, written before reading the diff

1. A supported tool outcome is lost, duplicated, or misclassified, including MCP errors, retry responses, and parallel calls.
2. Ordinary results or classifier awaitables leak payload text through logging/warnings or bypass the redaction boundary.
3. A claimed final-round fix is false or lacks meaningful test evidence.

What I tried

• Required plugin suite, Python 3.11.13: 661 passed, 1 skipped. The skipped case requires Python 3.14.
• Adjacent reflect/retry, MCP, and workflow ToolNode suites: 183 passed.
• Independent probes: 101 passed, 3 failed; all three failures reproduce the single P1 below. Probes are under /tmp/adk-pr15-dual/r5/astra-probes/.
• Awaitable lifecycle: failed/pending futures; already-failed, newly-created, already-started, and cancellation-swallowing tasks; gather, shield, wrap_future, run_in_executor; coroutines, async generators, and custom awaitables. Tested successful and error-bearing tool results, yielded to the loop, collected garbage, and checked rows, captured logs, warnings, stderr, and asyncio ERROR records. These passed. A task created inside the classifier was cancelled before its body ran.
• Hostile sibling fields across dicts, dict subclasses, Pydantic dumps, direct MCP models, and MCP subclasses, including numpy/pandas values and raising attribute/comparison/truth hooks. Valid error signals survived, non-boolean flags remained successes, malformed/raising classifiers fell back, and hostile results explicitly classified OK retained a sentinel row.
• Real-runner probes with two distinctly named analytics instances and eight parallel asynchronous failures, with retry before/between/after them; both retry guidance and final responses; message-less exceptions. Each instance recorded one error per call. Also tested 24 interleaved callback calls, AgentTool nesting with inherited plugins, and unary live tools with/without the thread pool. Repository tests additionally cover workflow nodes, unreadable exception messages answered at agent level, provenance, span pairing, and classifier validation.
• Formatter, formatter-failure, payload-denylist, and event-allowlist behavior on the new TOOL_ERROR rows. Result omission and the fixed MCP error message preserved the intended row privacy.
• Independently extracted the unchanged validator functions from this head and ran them on available CPython 3.14.0a6: TYPE_CHECKING-only annotations were accepted without evaluation; the old signature read raised NameError; incompatible and partial-async classifiers were rejected. This is a validator test, not a full plugin run on stable 3.14.
• Pinned Pyink 25.12.0 and isort 8.0.1 checks passed for both PR files. The fresh uv environment omitted the three permitted unsupported packages; Cryptography 50.0.1 failed its native build, so testing used the compatible 46.0.5 wheel. Pydantic/MCP were the locked 2.13.5/2.2.0 versions.
• A separate source copy with the plugin reverted to the specified base reproduced MCP misclassification; head recorded TOOL_ERROR. No tracked worktree file was changed. streaming_utils.py has no base-to-head diff, so the reported CI Mypy issue is outside this PR. The four other changed files in the full diff come from the upstream merge.

I checked the callback short-circuit, streaming-chunk, resumed long-running-tool, and nested span-stack paths. Their documented limits are acceptable for this scope. The deferred GoogleTool/McpTool/FunctionTool envelopes and OTel-hook classifier need separate semantics; the existing classifier provides an extension point. Deferring keep-result is also reasonable given formatter compatibility. I did not rerun the entire reported 60-mutation campaign or perform a live BigQuery write.

P0

None.

P1 — category (1): ordinary Pydantic serializer warnings still expose tool payload before redaction

Location: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:8758, calling the existing Pydantic dumping path at line 2142.

The new serialization boundary catches exceptions, but Pydantic can successfully return a dump while emitting a UserWarning containing the original field value. That warning escapes before _log_event applies content_formatter. No hostile class, overridden method, or custom awaitable is needed. For example, return this ordinary mutable SDK model from a FunctionTool:

result = CallToolResult(content=[], isError=False)
result.content.append({"type": "text", "text": "ASTRA_R5_PRIVATE_71fd9"})
return result

The list mutation is not revalidated. model_dump_fn() emits PydanticSerializationUnexpectedValue(... input_value={'type': 'text', 'text': 'ASTRA_R5_PRIVATE_71fd9'}, input_type=dict) and still returns normally. A normal application BaseModel with a subsequently assigned mismatching field value reproduces the same leak.

Evidence: test_plain_model_serialization_warning_privacy[mcp] and [plain_pydantic] both fail even though a formatter replaces the entire row content and the row contains no private marker. test_warning_leak_in_actual_runner fails through an actual tool invocation. The standalone warning_stderr.py confirms payload_in_default_stderr: True and payload_in_py.warnings: True when standard logging.captureWarnings(True) is enabled. Outcome rows remain present.

Classification: category (1), a reproducible payload leak using ordinary SDK/Pydantic classes and built-in containers. I also reproduced it on base main: this is a pre-existing serializer leak left outside the new boundary, not a newly introduced regression. It nevertheless violates the requested final-round warning/privacy guarantee; it is not one of the deferred hostile-class cases. Rejection criterion 2 is met.

Suggested fix: suppress payload-bearing diagnostics at the recognized Pydantic serialization call (for example, native model_dump(warnings=False)), while preserving sanitized output or the sentinel and using only a constant diagnostic if needed. Add a regression using ordinary mutable MCP/Pydantic models that checks Python warnings/default stderr and py.warnings, alongside the row assertion. Do not rely solely on catching serialization exceptions or inspecting the plugin logger.

P2

No additional findings retained.

VERDICT: REQUEST_CHANGES @ 86d62bf

[caohy1988]

Fable blind review — PR #15, round 5 (final) @ 86d62bf

Blind review by Claude Fable on caohy1988's behalf. Sources: the PR description, the diff, issues google#7112 and GoogleCloudPlatform/BigQuery-Agent-Analytics-SDK#485, and the repository code. I did not read any other review, review comment or PR comment, and I changed nothing on the branch.

Rejection criteria (written before reading the diff)

• R1: lost or duplicated rows. Fail if a reachable path, needing no hostile classes, leaves a tool call with zero outcome rows or with two (TOOL_ERROR plus TOOL_COMPLETED, or two TOOL_ERRORs). The paths to cover: either order with ReflectAndRetryToolPlugin, parallel calls, two analytics instances, an agent-level error callback, and a raising or odd classifier. An outcome row that pops a span it doesn't own also fails.
• R2: leaked payload. Fail if result, argument or exception text reaches a BigQuery row outside the content_formatter/redaction contract, or reaches logs (warnings, tracebacks, asyncio "never retrieved" records, sys.unraisablehook).
• R3: wrong classification, or a false or untested claim. Fail if a well-formed error signal (MCP isError=True, a reflect-and-retry answer) is recorded as TOOL_COMPLETED, if a plain success is recorded as TOOL_ERROR, if a PR-body or docstring claim is false, or if a fix has no test that fails without it.

Outcome: none of R1–R3 is met at 86d62bf. There are no P0 or P1 findings. Below are three non-blocking P2 notes, each classified, and two NOT VERIFIED items (Python 3.14 final; real BigQuery and MCP servers).

What I ran (this session, on this head)

Environment: this worktree on Python 3.11.13 (x86_64, uv venv, pinned tools); dependency-only venvs for 3.10.16 and 3.13.7; CPython 3.14.0a6 for stdlib-only checks.

Check	Result
pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8	661 passed, 1 skipped (the 3.14-only test)
tests/unittests/plugins + tests/unittests/workflow/test_tool_node.py	973 passed, 1 skipped, as the body says
the same plus tests/unittests/tools/mcp_tool/test_mcp_tool.py	1082 passed, 1 skipped
-k "TestToolResultErrorClassification or TestToolProvenance" on 3.10.16 and 3.13.7	121 passed, 1 skipped on each
pyink 25.12.0 --check, isort 8.0.1 --check-only, ruff 0.15.17 on both files	clean
mypy src/google/adk/plugins/bigquery_agent_analytics_plugin.py	no issues
my 94 out-of-repo probes (described below) on 3.10, 3.11 and 3.13; on 3.11 both with pytest's log capture and with -p no:logging	94 passed on each
my 22 end-to-end probes against base def458b6	18 fail; the 4 that pass are intended "unchanged" guards
the head's classifier-return tests against the round-4 plugin (d5b8ecbb)	5 failed, 4 passed, 1 skipped. The failures are exactly the round-5 cases: failed future, failing task, task failing once cancelled, failing gather, pending future cancelled.
my classifier-return probes against d5b8ecbb	16 fail (8 kinds × 2: failed future, unstarted tasks, shield, both gathers, run_in_executor, wrap_future), 14 pass
the PR's own mutate.py, mutations 0–8 (round 5; 6–8 run with PY314 set to 3.14.0a6)	9 of 9 caught; the worktree stayed untouched
_signature_without_annotations + _validate_tool_result_classifier, AST-extracted and run on 3.14.0a6	Accepts TYPE_CHECKING-only annotations, including through functools.partial and a callable instance, and evaluates none of them. Rejects a missing keyword, async, positional-only, max and a raising __signature__ with ValueError. The pre-change inspect.signature wrongly rejects the 3 valid classifiers with NameError.
CI	Mypy (3.11 job): 858 errors on main and 858 on the branch. The one "new" error is in src/google/adk/utils/streaming_utils.py, which has no diff against def458b6 or against fork main e4c0d946, so it's out of scope. Unit Tests: all 5 jobs were cancelled at the 10-minute limit; the two I counted show one F in the progress dots. Fork main's run 36517507151 shows the same pattern (5 jobs cancelled, one F each), so CI gives no signal either way.
scope	Since d5b8ecbb, only 01ff6041 (the fix) and 86d62bf0 (a 4-line docstring change) touch the two PR files. The merge aa456a78 touches neither.

What I tried to break

The probes live in /tmp, not in the repo.

End to end. A real Runner with a scripted model; I read the rows the plugin actually writes to the mocked write client.

• Orders and parallel calls. A raising tool × {analytics first, retry first} × {1, 3 parallel calls}. Each call gets exactly one TOOL_ERROR with error_message == str(error). Every outcome row carries the span its TOOL_STARTING opened (same span_id and parent_span_id). The model still receives the retry plugin's answer.
• Stale markers. One batch holding an unknown tool name, a raising call and a successful call, followed by more turns. Successes are always TOOL_COMPLETED. With analytics first, the lookup error gets its TOOL_ERROR; with retry first it gets none, which is the documented limit.
• Two analytics instances in the orders bq1,retry,bq2, bq1,bq2,retry and retry,bq1,bq2, with 2 parallel raising calls. Each instance writes exactly 2 TOOL_STARTING and 2 TOOL_ERROR rows.
• AgentTool nesting with shared plugins, in both orders. The inner raising call gets one TOOL_ERROR and the outer AgentTool call one TOOL_COMPLETED.
• Workflow ToolNode with RetryConfig(max_attempts=2): attempt 1 raises and nothing answers it; attempt 2 succeeds. Rows: STARTING, ERROR, STARTING, COMPLETED, with spans paired.
• Agent-level on_tool_error_callback answering, with analytics as the only plugin, over 2 calls: one TOOL_ERROR each.
• A real MCPTool (mocked session) whose server returns isError=True. The row is TOOL_ERROR with the fixed message and tool_origin MCP, and carries no server text. The model receives the dump with isError and the server text. With isError=False the row stays TOOL_COMPLETED and carries the text.
• A sync tool on RunConfig.tool_thread_pool_config (upstream c50ade39), in both orders. The tool runs on worker threads, and each call gets one TOOL_ERROR.
• Where the omitted MCP error text still lands. With log_multi_modal_content on, it appears in exactly one place across all rows and columns: LLM_REQUEST.content_parts. With it off, it appears nowhere. This confirms the after_tool_callback docstring and the body's Privacy section.
• A plugin ahead of analytics that answers before_tool_callback. Head and base behave identically apart from the new classification. Neither writes a TOOL_STARTING, and the outcome row borrows the agent's span. That span handling predates this PR and is unchanged, so I don't count it as a finding. A possible follow-up outside google#7112: pop_span(expected_kind="tool") in the tool callbacks.

Unit level. Every case checks the rows and the root and plugin logger output (formatted with tracebacks), plus stderr, sys.unraisablehook and warnings, for a per-case payload marker. Checks run after gc.collect() and several loop turns.

• Classifier return values, each tried with a plain result and with an isError result: failed Future; unstarted failing Task (its body never ran); started Task that raises on cancel; Task that swallows the cancel and raises later; gather of a started task; shield of a task that fails on its own; gather of a failed future; gather of unstarted coroutines (they never ran); run_in_executor that raises; wrap_future of a failed concurrent future; a failed concurrent.futures.Future (not awaitable); a coroutine whose __qualname__ is the payload; an async generator. Every case gives the right row and a constant warning. No payload appears anywhere, and asyncio logs no ERROR record.

• Hostile siblings next to a valid signal, each tried with no classifier, a raising classifier and an OK classifier. The shapes:

  • a dict whose values raise from __eq__, __bool__ or __str__;
  • a value whose __class__ raises;
  • str-subclass values with raising hooks;
  • a key that hash-collides with a wanted key and raises when compared;
  • a dict subclass whose every method and __getattribute__ raise;
  • a CallToolResult subclass whose __getattribute__ raises;
  • model_construct with hostile content;
  • a hostile extra field;
  • a retry answer with hostile error_details.

  The valid signal always wins, and the retry answer wins even over OK. Otherwise OK gives TOOL_COMPLETED. No payload reaches the logs or any TOOL_ERROR row.

• Near misses stay TOOL_COMPLETED with no boundary warning: isError set to False, "true" or 1; is_error set to "True"; numpy True_; {"error": …}; {"status": "ERROR", "error_details": …}; another response_type; a nested flag; a list; a str; None. Separately, a non-MCP pydantic model with isError=True, run end to end, stays TOOL_COMPLETED as documented.

• Unserializable results (raising __getattribute__, raising __class__, hostile dict subclass) keep their TOOL_COMPLETED row with [UNPARSEABLE_JSON_BLOB] and a constant warning, and leak no payload.

The code the marker relies on.

• Every call, single or parallel, async or live, runs in its own task, created from a copy of its prepare-phase context (flows/llm_flows/tools/_batch_executor.py:171-186).
• ToolNode runs both callbacks in one coroutine (workflow/_tool_node.py).
• Each workflow retry gets a fresh child context (workflow/_node_runner.py:128).
• The marker check compares a weak reference's target by identity.

I found no path where one call's mark can skip another call's row.

Findings

P0: none. P1: none.

P2 (non-blocking)

1. A docstring overstates what releasing an awaitable guarantees. src/google/adk/plugins/bigquery_agent_analytics_plugin.py:8825-8828 says that after _release_awaitable, "neither Python nor asyncio later logs anything about it".

   • A started task that swallows the cancellation and keeps running is still reported by asyncio ("Task was destroyed but it is pending!"); the PR body lists that case itself.
   • A variant for the body's hostile-class list: a custom awaitable that wraps a failed future (its __await__ returns fut.__await__()). It's left alone, as designed, and asyncio then logs "Future exception was never retrieved", with the payload, when it's collected. My probe custom_awaitable_holding_failed_future shows the row is correct and confirms the leak.

   Classification: (2). It needs a classifier that builds such objects, and code can't fix it without running the wrapper's own code. Fix: reword to "so that asyncio does not later log an exception the returned future or task holds", and add the wrapper case to the body's hostile-class list.

2. The release note is narrower than the rule. The suggested note says "MCP results with isError set". But _mcp_error_message (plugin.py:873) applies to any tool's dict whose top-level isError/is_error is exactly True, and the docstrings now say so. A function tool that returns, say, {"is_error": True, "line": …} as data moves from TOOL_COMPLETED (with the result) to TOOL_ERROR (without it). Classification: not (1). The docstrings document it and an OK verdict overrides it; the release note is incomplete, not false. Fix: say "any tool's dict result whose top-level isError or is_error is exactly True, such as an MCP CallToolResult" in the release note. Optionally, accept the snake_case spelling only from MCP models or MCP-origin tools, since ADK's McpTool always restores camelCase (tools/mcp_tool/mcp_tool.py:125-150).

3. The release note should say where the exception text moves. With ReflectAndRetryToolPlugin ahead of analytics, str(error) used to land in TOOL_COMPLETED.content.result.error_details, which content_formatter and payload_column_denylist govern. It now lands in TOOL_ERROR.error_message, which only _sanitize_sensitive_text governs. TOOL_ERROR already records that same text for the same exception in the other order, so the contract holds, and the body's Privacy section covers it. But a user who relied on a formatter or a content denylist to scrub exception text in that order would only learn of the change from the release note. Classification: not (1), since nothing leaks beyond what TOOL_ERROR already records. Fix: add one release-note bullet.

Judgment on the flagged design points and deferrals

• Result left out of TOOL_ERROR: agree. The probe above shows where the text still lands.
• Reflect error_message taken from error_details: agree. It is str(error), with error_type (the type name) as the fallback (plugins/reflect_retry_tool_plugin.py:320-325, 358-363), so both plugin orders record the same text, message-less errors included.
• Keyword-only, four-argument classifier signature: fine.
• Toolbox and Skill detection via sys.modules and class names: fine. Both tests use the real classes, and the Skill test enumerates the module, so a new skill tool would fail it.
• Per-call marker: sound, per the flow analysis above.
• Allowlist and dashboard change: documented.
• Not reusing _detect_error_in_response: acceptable, and the stated reason is accurate. The FunctionTool, BashTool, RestApiTool and SkillToolset hooks flag any truthy error key (tools/function_tool.py:439, tools/bash_tool.py:293, tools/openapi_tool/openapi_spec_parser/rest_api_tool.py:742, tools/skill_toolset.py:723).
• Deferred items: none is a blocker; none loses a row or leaks. I'd take the McpTool graceful-error envelope first. _MCP_GRACEFUL_ERROR_HANDLING is on by default (features/_feature_registry.py:191-192), so an MCP transport failure is still recorded as TOOL_COMPLETED/OK by default. A rule scoped to McpTool and its two fixed message prefixes would sidestep the concern about {"error": …} being data.

NOT VERIFIED

• Python 3.14 final. The 3.14-only test is skipped on 3.10–3.13, CI's 3.14 unit job was cancelled before reaching a result, and this machine only has 3.14.0a6, where I checked the validator by extraction (above). I suggest running test_classifier_annotated_with_names_it_imports_for_type_checking once on 3.14 final before the upstream PR.
• Real BigQuery writes and a live MCP server. Every row came from the mocked write client, and the MCP session was mocked.
• Issue google#485 item A ([FORMATTER_FAILED] with a null error_message): the PR defers it and the done-when contract doesn't include it, so I didn't review it as a gap.

VERDICT: APPROVE @ 86d62bf

[caohy1988]

Independent blind review of head 8b05d3810f8493d63a86621f09ae9b9d45b339ac, against def458b609c2811d137b0332b2fc7b201dddd5c0. I read the permitted PR description, issues, diff and repository code; no other review or earlier-round artifact was read.

Rejection criteria (written before reading the diff)

1. Normal tool-result serialization or classifier awaitable cleanup leaks payload text into stderr or logging.
2. Supported MCP/reflect error results or raising tools lose or duplicate outcome rows, including either plugin order and separate analytics instances.
3. A claimed safety fix fails executable validation. Hostile-class-only edge cases are P2 under the final-round cap.

What I tried

• Required plugin suite: 668 passed, 1 skipped (Python 3.14-only test), using Python 3.11.13 and PYTHONPATH=src. Pinned pyink 25.12.0 and isort 8.0.1 checks pass on both files; tracked worktree remains clean.
• Environment: uv sync --all-extras could not install google-antigravity on macOS x86_64. Excluded the three permitted packages; locked cryptography 50.0.1 also failed to build, so used the available cryptography 48.0.1 wheel. Pydantic is 2.13.5. This is a validation limitation, not a PR finding.
• Independent probes, with repository fixtures and row-capture helpers but independent inputs/assertions: 44 passed and 2 failed; four additional full-turn reproductions fail on the same leak below. Successful probes cover mutable MCP and application models, nested models, both default-style stderr and logging.captureWarnings(True), multimodal logging on/off, a safe no-keyword dump override, and unrelated warnings remaining visible.
• Field isolation: valid MCP and retry signals survive raising sibling values in plain dicts, dict subclasses, MCP models and hostile MCP subclasses; non-boolean flags remain successes and retain their rows.
• Awaitable cleanup after loop turns and gc.collect(): failed Future, completed failing Task, newly created Task, cancellation-swallowing Task that then raises, gather, shield, wrap_future, run_in_executor, async generator and an inert custom awaitable. Outcome rows survive; no payload canary or asyncio ERROR record appears; the new task does not start.
• Real runners: four parallel async message-less failures in each plugin order; two analytics instances (distinct plugin names) each record the raised error once; AgentTool nesting retains the inner error and outer completion. The required suite also exercises workflow ToolNode, hostile result serialization, malformed classifiers, unreadable exception strings, agent error handlers, formatter/redaction behavior, and Toolbox/Skill origins.
• Extracted the actual signature-validation functions and ran them on the available CPython 3.14.0a6: undefined annotation names raise with default inspect.signature, but pass this implementation, including a partial. Full plugin execution on 3.14 was not run.
• Separate baseline source copy: the MCP classification control records TOOL_COMPLETED on base and TOOL_ERROR on head. A direct standalone subprocess using Python's untouched default warning display reproduces the remaining override leak on head and base. Simply reverting the plugin under all new tests fails collection because the new API is absent; I do not count that as a regression-test result.
• Inspected live/streaming, long-running resume, short-circuit callback, nested AgentTool and workflow call paths. Live chunks and resumed long-running outcomes bypass this classifier as documented; no real provider/live-service run was performed. The documented callback-order limits agree with dispatch code. Not mirroring OTel error hooks, deferring GoogleTool envelopes, and omitting results from TOOL_ERROR are acceptable scope/privacy choices here; event_allowlist/dashboard changes and the retry-first exception-text relocation are disclosed.
• git diff def458b6 HEAD -- src/google/adk/utils/streaming_utils.py is empty. The merge after 8688539 changes neither PR file. The reported unrelated CI typing failure is outside this review; I did not infer a green full CI run.

P0

None.

P1 — category (1): the compatibility fallback still leaks ordinary Pydantic tool results

Location: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:1891-1894, invoked for completion payloads at line 2161.

The TypeError fallback calls a legacy model_dump() override without warnings=False. A normal application override that delegates to Pydantic to exclude a field therefore reinstates the payload-quoting warning:

class Result(BaseModel):
    number: int = 1
    excluded: str = "omit this field"

    def model_dump(self):
        return super().model_dump(exclude={"excluded"})

result = Result()
result.number = "PRIVATE_PAYLOAD"
# Return result (or {"nested": result}) from an ordinary FunctionTool.

With content_formatter=lambda content, event: "scrubbed" and log_multi_modal_content=False, the plugin still emits PydanticSerializationUnexpectedValue(... input_value='PRIVATE_PAYLOAD' ...) before the formatter. I reproduced it through the actual TOOL_COMPLETED row capture and a full scripted runner turn, for both a top-level and nested result, to stderr and py.warnings. All affected turns still record one TOOL_COMPLETED and two LLM_REQUEST rows: this is a privacy leak, not a lost-row finding.

Evidence: /tmp/adk-pr15-dual/r6/astra-probes/test_independent.py (test_plain_override_leak_probe, test_override_leak_real_turn), independent.log, leak-real-turn.log, and default-head.stderr. The standalone default_warnings.py subprocess uses Python's actual default warning display, without pytest's warning recorder.

Classification: category (1), a real payload leak in the plugin's normal tool-result serialization path. No raising hooks, adversarial objects, custom warnings or asyncio misuse are involved. The PR body explicitly acknowledges this fallback as deferred, and it also exists on base; the final-round contract expressly permits retaining pre-existing normal tool-result payload leaks. Documentation does not close this privacy boundary.

Suggested fix: avoid the unchecked no-keyword retry for Pydantic overrides. Require an override to accept and forward warnings=False; otherwise retain the outcome row with a safe sentinel and constant diagnostic (or provide an explicit safe serialization adapter). Preserve field exclusions rather than bypassing the override, and add this delegating-override regression in both warning displays. A process-wide warning filter is unnecessary for the sentinel approach.

P2 (category 2)

No additional findings retained. Documented hostile-class-only and asynchronous cleanup limitations are not escalated.

The first rejection criterion is met by the P1 above. This is a COMMENT review; the verdict does not merge or modify the branch.

VERDICT: REQUEST_CHANGES @ 8b05d38

[caohy1988]

Fable (Claude) blind review — round 6 — PR #15 @ 8b05d38

Blind: I read no review, review comment or PR #15 comment before posting this. Disclosure: my local auto-memory was loaded at session start. It holds technique notes from earlier rounds of this PR, some written during the implementer's google#7112 sessions. No peer review text was read.

Rejection criteria (written before reading the diff)

• R1: lost or duplicate rows. Any non-hostile path where one tool call gets zero outcome rows (TOOL_COMPLETED / TOOL_ERROR), or more than one. Cases:
  • either order with ReflectAndRetryToolPlugin;
  • raised errors, including message-less ones;
  • agent-level on_tool_error_callback answers;
  • two analytics instances;
  • parallel calls;
  • workflow and AgentTool nesting.
• R2: payload leak. Any non-hostile path where tool result, argument or error payload reaches stderr, py.warnings or the logger. Or reaches a row beyond the existing TOOL_ERROR / TOOL_COMPLETED contract, or bypasses content_formatter or redaction for the new rows. For round 6 this includes:
  • a payload-quoting Pydantic warning still emitted by the plugin's tool-result serialization: the TOOL_COMPLETED row, the next LLM_REQUEST function_response, nested models, log_multi_modal_content off;
  • the fix hiding unrelated warnings process-wide.
• R3: misclassification, a false claim, or an untested fix.
  • A valid MCP isError=True (dict or CallToolResult) or a retry answer that isn't TOOL_ERROR / ERROR.
  • A success that isn't TOOL_COMPLETED / OK.
  • A wrong Toolbox or Skill origin, or a changed origin for other tools.
  • A false claim in the PR body, a docstring or the release notes.
  • A fix whose tests pass on unfixed code.

Result: R3 is met. There is one false PR-body claim, and two new tests fail under the repo's own CI command (P1-1). R1 and R2 are not met. The plugin code held up under everything below.

Findings

P1-1 (category 1: false PR-body claim; new tests fail under the repo's CI command): the round-6 turn test isn't hermetic

tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py:7271, test_result_field_that_no_longer_fits_is_not_quoted_in_a_turn[stderr] and [py.warnings], fail at their asserts (:7303 / :7304). They fail whenever an earlier test in the same process has installed a recording OpenTelemetry tracer provider.

Evidence

• Repro, no xdist: pytest tests/unittests/cli/test_fast_api.py 'tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py::TestToolResultErrorClassification::test_result_field_that_no_longer_fits_is_not_quoted_in_a_turn' → 2 failed. A sequential run of cli/test_fast_api.py, telemetry/test_setup.py, flows/llm_flows/test_llm_callback_span_consistency.py and the plugin file gives the same 2 failures.

• Full suite: pytest tests/unittests -n 8 on this head gives 16410 passed, 8 failed.

  • Six are env-only on this Mac: 4 antigravity samples and 2 test_import_loading allowlist cases.
  • The other two are these tests.

• CI matches. Every Unit Tests (3.10–3.14) job on this head shows 3 F in its progress lines. Each baseline shows 1 F:

  • upstream main 5a0421c (run 36613592212), all 5 jobs;
  • fork main e4c0d94, all 5 jobs;
  • the previous PR head 86d62bf (3.11 job).

  git diff --stat 5a0421cb 8b05d381 lists only this PR's two files. The jobs are still cancelled at the 10-minute timeout, which predates this PR.

• The cause is not the plugin. A stack recorder on warnings._showwarnmsg shows the payload warnings come from ADK core span content capture (ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS, default on):

  • telemetry/tracing.py:344: trace_tool_call → _serialization.safe_json_serialize → model_dump();
  • tracing.py:885: _build_llm_request_for_trace → Content.model_dump(...).

  A standalone app turn after trace.set_tracer_provider(TracerProvider()) prints the payload-quoting warning to stderr both with and without the analytics plugin.

Claims this falsifies

• The checklist's "[x] All unit tests pass locally" and "[x] New and existing unit tests pass locally".
• Testing Plan: "The same holds across a real turn with log_multi_modal_content off". That holds only for the plugin's own dumps. Any app with a recording tracer (adk web, adk api_server, Cloud Trace) still prints the payload in that turn.

Fix (verified on a /tmp copy of tests/)

• In that test, add monkeypatch.setenv("ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS", "false"), with a one-line comment that ADK's span content capture dumps the same response with Pydantic warnings on.
  • The patched copy passes after the three tracer-installing files (823 passed).
  • It still fails both parametrizations against the pre-round-6 plugin (86d62bf), so it still pins the fix.
• Alternative: a stack-scoped recorder that counts only warnings raised under bigquery_agent_analytics_plugin frames.
• In the PR body, scope the "real turn" sentence to the plugin's dumps, and add the ADK core sources in P2-1 to the follow-ups.

P2-1 (not category 1 under the rubric: pre-existing and outside the plugin's paths): ADK core still quotes the same payload in a real turn

• Telemetry, as above. By reading, tracing.py:545 does the same for merged tool calls.
• SqliteSessionService at sessions/sqlite_session_service.py:499 (event.model_dump_json(exclude_none=True)). A turn with no analytics plugin printed two payload-quoting warnings to stderr.
• Other session services, by reading: sessions/schemas/v0.py, v1.py and vertex_ai_session_service.py dump events the same way.
• The PR body's follow-ups name only google-genai. Suggest listing these too, and filing an upstream issue to pass warnings=False at those sites, as flows/llm_flows/tools/_caller.py:601 already does.

P2-2 (category 2, pre-existing on def458b): a result whose own model_dump raises falls back to __dict__

_model_dump_quietly retries only the missing-keyword case. When a present model_dump raises for any other reason, including a TypeError from its body, _recursive_smart_truncate (plugin :2161 branch) falls through to the instance-__dict__ walk. The row then records fields the override would have left out.

• Probe: an override that always raises TypeError recorded {'s': 'ok'}, identical on def458b.
• Nothing reaches the log, and content_formatter still applies.
• One-line fix: when model_dump_fn exists and raises, return ("[UNSUPPORTED_OBJECT]", True) instead of falling through.

P2-3 (category 2, already documented): a custom awaitable wrapping a failed future

It still leaks through asyncio's "Future exception was never retrieved" at GC. I reproduced it. It matches the PR body's hostile-class list. No action.

Scope note (no severity)

google#485's ADK-side item A is deferred and listed under follow-ups: a content_formatter failure writes [FORMATTER_FAILED] with a null error_message. The done-when list doesn't require it; I'm flagging it only so the deferral is a deliberate call.

What I tried

Everything ran on 8b05d38 unless noted. Probes live in /tmp; the worktree was not modified.

Environment and suites

• Worktree venv: Python 3.11.13, pydantic 2.13.5 / pydantic-core 2.46.5, mcp 2.2.0, google-genai 2.25.0. These are the versions CI's Unit Tests job lists.
• PR test file with -n 8: 668 passed, 1 skipped.
• The two new classes: 128 passed, plus 1 skipped (3.14-only).
• The PR body's tests/unittests/plugins tests/unittests/workflow/test_tool_node.py -n 8: 980 passed, 1 skipped.
• pyink --check 25.12.0, isort --check-only 8.0.1 and ruff are clean. mypy on the plugin reports no issues.

Round 6, run like a real app. A standalone script with default warning filters and stderr redirected to a file, plus a logging.captureWarnings(True) variant. Real Runner turns used a FunctionTool returning:

• a mutated MCP CallToolResult (content appended after validation);
• the same with isError=True;
• an app model with a wrong-typed field;
• that model nested in a dict, a list field or a dataclass.

Each case ran with log_multi_modal_content on and off.

• Head: 0 payload hits in stderr and py.warnings, and TOOL_COMPLETED rows keep the dumped value.
• 86d62bf and def458b both leak via after_tool_callback → _recursive_smart_truncate, so the fix closes a pre-existing plugin-path leak.

Round 6, in process

• No payload warning for models nested in a dict, list, tuple, dataclass, list field or Any field, a RootModel, or a **kw pass-through override.
• warnings.filters is unchanged.
• An unrelated warnings.warn inside a field_serializer is still shown.
• Overrides that take no keywords or fixed keywords get their own output recorded.
• Five own mutations, all caught by the new tests:
  • each dump site made loud;
  • warnings=False dropped;
  • the TypeError fallback removed;
  • the fallback returning None.

The PR's mutate.py, rerun: 64 of 64 caught, with mutations 10–12 checked on CPython 3.14.0a6.

Runner end to end. 34 probes, all passing on head; 19 of them fail on def458b.

• Raising tool with 1 and 3 parallel calls, with and without a message, in both plugin orders: exactly one TOOL_ERROR per call, with the expected error_message, and its span ids pair with TOOL_STARTING.
• A dict isError of True / "true" / 1 / False / None, in 3 orders: only True becomes TOOL_ERROR, and the model still receives the result unchanged.
• A CallToolResult model with isError True and False.
• Two analytics instances through a Runner in [bq, retry, bq], [bq, bq, retry] and [retry, bq, bq]: 2 TOOL_ERROR rows each.
• A classifier returning OK, returning ERROR, or raising: no payload in the log.
• content_formatter applies to the new TOOL_ERROR rows.
• AgentTool nesting in both orders.

Round 5. Awaitables a classifier returns, checked after gc.collect() for payload in caplog, stderr and warnings, and for asyncio ERROR records:

• a failed Future;
• an unstarted failing Task;
• a started task that raises on cancel;
• a task that swallows the cancel, then raises;
• a failing gather;
• shield, wrap_future, run_in_executor;
• an async generator and a coroutine.

All clean; only the documented custom awaitable leaks.

Round 4

• An error whose __str__ raises gets exactly one TOOL_ERROR, carrying its type name.
• Classifier validation:
  • accepted: a functools.partial, lambda **kw;
  • rejected: a nested partial of an object with an async __call__, a positional-only function, max, a non-callable.

Round 3, hostile siblings. The valid signal wins, a hostile-only result stays TOOL_COMPLETED, and nothing reaches the log. Cases:

• a raising __eq__ value;
• a colliding key;
• a dict subclass whose __getattribute__ raises;
• a CallToolResult subclass whose __getattribute__ raises, with and without an error;
• a retry answer next to a raising value.

Python 3.14 (3.14.0a6, validator extracted)

• A classifier annotated with names imported only under TYPE_CHECKING is accepted, with nothing evaluated.
• With Format.VALUE the same classifier is rejected with NameError.
• 3.10–3.13 keep the unchanged path.

Marker. In _caller.py the error and after-tool callbacks run in the same coroutine; the thread pool runs only the tool body. The same holds in workflow/_tool_node.py. So the ContextVar marker is visible where it's consumed.

Origins. ToolboxTool (toolbox_adk 1.4.0) and all six Skill tools subclass BaseTool directly, so the new checks are reachable after the FunctionTool check.

CI Mypy. The "new" errors are in dependencies/_mcp.py, from upstream e738c26 merged by 8b05d38, and in utils/streaming_utils.py, which has no diff against def458b. Both are out of scope.

Design points (no change requested)

• Leaving the result out of TOOL_ERROR content is fine.
• Taking the retry answer's error_message from error_details is fine. It equals str(error), which is what on_tool_error_callback records.
• The keyword-only classifier signature is fine.
• The deferred GoogleTool envelope rule and keep-result opt-in are not blockers; a classifier covers both today.
• Toolbox and Skill detection through sys.modules and hardcoded class names is accurate today. It will silently drift if skill_toolset adds a tool class; checking the class's module would be sturdier.

VERDICT: REQUEST_CHANGES @ 8b05d38

[caohy1988]

Astra R7 independent blind review

Reviewed head 380335e48c0bc2369d9054d5fe9ace8a29ee1684 against def458b609c2811d137b0332b2fc7b201dddd5c0, focusing on the two PR files and distinguishing upstream merge changes. No earlier-round or peer review material was read. No tracked files were modified.

Rejection criteria (written before reading the diff)

1. A supported error result or raised error loses or duplicates its outcome row, or ordinary successful results change classification.
2. Plugin serialization or classifier cleanup emits payload text through warnings/logging, or exposes excluded model fields, in a reachable ordinary model/async case.
3. A final-head contract/testing claim is false or a substantive fix lacks passing evidence, including real-turn isolation and model override handling.

What I tried

• Read the PR description, the two-file diff, issue google#7112 including its reproduction, and SDK issue google#485. Reviewed callback dispatch, parallel context copying, workflow ToolNode, AgentTool nesting, long-running handling, and live/streaming dispatch. Live and streaming paths were inspected, not exercised against an external service. The documented short-circuit limitations match PluginManager and the caller: analytics cannot record callbacks an earlier answering/re-raising plugin prevents it from seeing.
• PYTHONPATH=$PWD/src .venv/bin/python -m pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q: 685 passed, 1 skipped. This exercises both retry orders, parallel raised calls, instance-specific markers, direct MCP models, message-less/unprintable errors, classifier validation/fallbacks, formatter/redaction, origins, and hostile serialization.
• Independent probes outside the repository: 77 passed. Used dict/dict-subclass and MCP model/model-subclass results with hostile sibling fields beside valid MCP/retry signals, plus successful controls. Verified exactly one outcome row per call and no payload-bearing boundary logs.
• Probed model overrides top-level and nested: delegating with no keywords, swallowing keywords, returning a private dict, raising overrides, mixins, functools.wraps, and instance/class assignments. Probed a failing Pydantic serializer with an excluded field. Overrides/failing dumps retain a completed row with the sentinel and no excluded field leakage. RootModel, generic models, MCP CallToolResult/TextContent, genai Content/Part/FunctionResponse, ADK Event, a non-Pydantic look-alike, and MagicMock retain rows; common real models are not incorrectly treated as overrides.
• Ran real runner turns for the override variants with log_multi_modal_content=False, both default stderr warning display and py.warnings, and span content capture disabled. Each had one TOOL_COMPLETED and two LLM_REQUEST rows, with no payload warning from either plugin serialization path.
• Returned failed Future, failed Task, cancellation-swallowing-then-failing Task, gather, shield, wrap_future, run_in_executor future, async generator, and custom awaitable from synchronous classifiers. Yielded and called gc.collect() repeatedly, checking rows, captured logging/stderr and the loop exception handler: no leaked payload or asyncio error. The repository tests additionally verify cancellation before an unstarted task runs, pending-future cancellation, and native/generator coroutine closing.
• Installed an actual recording SDK OpenTelemetry tracer first, then ran a FastAPI telemetry test and the entire plugin file in the same process: 687 passed, 1 skipped. The new turn tests remain hermetic. The initially attempted full FastAPI-file ordering run was interrupted after 50 passing tests when it stalled; I do not count it as passing.
• Entire plugins directory plus workflow ToolNode tests, -n 8: 997 passed, 1 skipped.
• Python 3.14.0a6: extracted the two validator functions from this head and verified an unresolved type-only annotation fails ordinary inspect.signature but passes this validator. This is validator-only evidence, not a full 3.14 plugin run.
• Negative control in an isolated source copy using pre-R7 plugin 86885394: the 23 focused warning/override/failing-dump tests gave 17 failed, 6 passed, including the real-turn override tests. Trying the base plugin with the head test file cannot collect because the new classification API is absent; tracked source was never reverted.
• Pinned pyink 25.12.0 and isort 8.0.1 checks passed on both files; git diff --check passed. The final merge changes neither PR file. git diff BASE HEAD -- src/google/adk/utils/streaming_utils.py is empty.

Findings

• P0/P1 — category (1): none. No ordinary reachable leak/data-loss regression, false final-head claim, or untested substantive fix found in the reviewed scope.
• P2 — category (2): none retained. The disclosed hostile-class/custom-awaitable limitations and pre-existing core/session/OTel warning paths are not new findings in this review.

Design judgment and verification limits

The keyword-call classifier contract is practical; invalid signatures/async callables are rejected and unusable returns fall back safely. Strict literal-True flags and response_type matching avoid treating ordinary error data as failure. Not aligning all built-ins with OTel error hooks, and deferring GoogleTool/graceful-MCP/missing-argument envelopes and keep-result, are acceptable scoped choices with the application classifier available.

Replacing overriding Pydantic model payloads with a sentinel is an acceptable disclosed privacy trade-off: row/classification and the model's actual result survive, and excluded fields are not recovered through __dict__. The TOOL_ERROR result omission and error_details-to-error_message handling follow the existing raised-error row contract. Dashboard/event_allowlist migration and plugin-order limits are explicitly documented.

The relevant reported test totals were reproduced. I did not independently run all 16k unit tests, validate every reported environment-only failure, or reproduce hosted CI timeouts/Mypy. Full extras sync hit the disclosed google-antigravity macOS x86_64 wheel limitation; the usable Python 3.11 worktree environment ran the checks above. No live BigQuery/external MCP service was used.

VERDICT: APPROVE @ 380335e

[caohy1988]

Fable (Claude) blind review: PR #15 round 7 (final) @ 380335e

Reviewed blind. I read only the PR description, the diff and files at 380335e48c0bc2369d9054d5fe9ace8a29ee1684 (base def458b609c2811d137b0332b2fc7b201dddd5c0), and repo code. I read no review or comment on this PR.

Disclosure: my local Claude Code auto-memory was loaded into this session. It is a notes directory shared by Claude Code sessions on this machine, and it holds probing and env techniques. Some of those notes date from earlier rounds of this PR, including a one-paragraph note of the implementer's round-7 rule (only call model_dump when __func__ is BaseModel.model_dump). No peer review text was in it.

Rejection criteria (written before reading the diff)

• R1, row integrity. Every tool call gets exactly one outcome row (TOOL_COMPLETED or TOOL_ERROR). This covers success, raised errors, MCP isError, ReflectAndRetry answers, both plugin orders, parallel calls, two analytics instances, and any non-hostile result type: real Pydantic models (including ones that override model_dump), MCP/genai/ADK types, MagicMock, RootModel and generics. A dropped or duplicated row reachable without adversarial classes means REQUEST_CHANGES.

• R2, no payload leak through the plugin's own paths. No result or field text may reach default stderr, py.warnings or plugin logs through the plugin's serialization: the TOOL_COMPLETED row, or the next LLM_REQUEST function_response part. This must hold in a real runner turn, with log_multi_modal_content on or off, without hostile classes. A leak means REQUEST_CHANGES.

• R3, claims hold and are tested. Each round-7 claim must be true and pinned by a test that fails when the fix is reverted:

  • an override is never called and becomes the sentinel;
  • BaseModel and RootModel dumps use warnings=False;
  • a raising dump becomes the sentinel with no __dict__ fallback;
  • the real-turn tests are hermetic and fail on pre-fix code;
  • common types are not misclassified as overrides.

  A false claim, a common type regressed to the sentinel, or an untested fix means REQUEST_CHANGES.

Outcome: no criterion is met. The one judgement call, on R3's "common types", is spelled out under P2-1.

What I ran

Environment: Python 3.11.13, pydantic 2.13.5, mcp 2.2.0, google-genai 2.25.0. Probes live in /tmp and not in the repo. The worktree has no tracked changes.

Suites and hygiene

• Plugin file: pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 gives 685 passed, 1 skipped. The same holds on Python 3.10.16, using a dependency-only venv.

• Full suite: pytest tests/unittests -n 8 --timeout=300, run in four parts, gives 16,443 passed. The only failures are environment-only:

  • 4 labs/antigravity collection errors and 4 antigravity samples (no x86_64 wheel);
  • 2 test_import_loading allowlist cases (sitecustomize);
  • 1 LiveKit hang.

  So the PR body's "all unit tests pass apart from the environment-only failures" is true.

• Formatters: pinned pyink 25.12.0 --check, isort 8.0.1 --check-only, ruff 0.15.17 and codespell 2.4.2 are all clean on both files. mypy on the plugin file finds no issues.

• Mutation check: I reran the PR body's mutate.py (it writes only to a scratch copy) for all 66 mutations, with PY314 set to 3.14.0a6. None survive, and the per-mutation failure counts are identical to the report.

Claim 3: hermetic real-turn tests

• cli/test_fast_api.py then TestToolResultErrorClassification, in one process: pass.
• Negative control: the same run from a /tmp copy of the tests with the two monkeypatch.setenv("ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS", "false") lines removed. All 6 turn tests fail. So the tracer-installing file really does break them, and the fix is what makes them hermetic.
• test_fast_api.py, telemetry/test_setup.py, test_llm_callback_span_consistency.py, then the whole plugin file: 840 passed, as the PR body says.

Fail-before

• Round-7 tests on the round-6 plugin (86885394): 17 fail. The 7 passes are the round-6 tests and the look-alike test.
• The round-6 and round-7 dump tests on base def458b6, with an API-only shim so the module imports: 24 of 24 fail.
• Both new classes on the base shim: 113 of 145 fail.

Claim 1: overrides are never called, and nothing leaks

• Subprocess, Python's default warning display (no pytest). I ran _recursive_smart_truncate and _serialize_part_model over 20 shapes, each with a mistyped field carrying a unique marker. The shapes:

  • plain models top-level, and in a dict or list;
  • delegating, kw-swallowing, **kw-forwarding, own-dict and functools.wraps overrides, top-level and in a dict;
  • an override inside a plain model and inside an Any field;
  • RootModel[Plain], a generic, a computed_field;
  • a dataclass, an object __dict__ and to_dict holders;
  • MCP results appended to after validation.

  Head puts 0 markers on stderr and 0 in py.warnings. The round-6 plugin leaks the override markers, and base leaks all 20.

• Real Runner turn in a subprocess (no pytest). Tools return a kw-swallowing override, an appended MCP result, and a mistyped model nested in a dict. I ran stderr and py.warnings displays, log_multi_modal_content on and off, and span capture both off and at its default. Head puts 0 markers anywhere, with exactly 1 TOOL_STARTING, 1 TOOL_COMPLETED and 2 LLM_REQUEST per turn. The base shim leaks all 3 markers.

• pytest real-turn matrix. 3 overrides × top/dict/list × multimodal on/off × 2 displays, plus 3 parallel calls with mixed results (override, MCP isError appended, mistyped nested model): one outcome row per call and no marker. A spy shows the override is called 0 times during a whole turn, including the LLM_REQUEST part dump.

• Trying to break the override check.

  • Dumped as expected: subclass, mixin, Gen[T], RootModel[str] and RootModel[list], model_dump = BaseModel.model_dump.
  • Sentinel as expected: **kw-forwarding override, functools.wraps override, instance attribute set via object.__setattr__.
  • Not Pydantic models, so called with warnings=False and the row kept: MagicMock, MagicMock(spec=Model), pydantic dataclass, pydantic.v1 model.
  • Not misclassified: MCP CallToolResult and TextContent, genai Content, Part, FunctionResponse and GenerateContentResponse, ADK Event and LlmResponse. An MCP model with isError=True is still TOOL_ERROR.

Claim 2. A raising field_serializer, a raising computed_field and a raising model_serializer all record the sentinel. The Field(exclude=True) value is absent, and nothing reaches the logs.

Earlier-round claims, re-probed fresh

• Hostile sibling fields: raising __class__, raising __eq__/__bool__, a dict subclass whose __getattribute__ raises, a hostile CallToolResult subclass. Also a numpy True_ flag and a top-level raising __class__. Each got the correct row, and no payload appeared in rows, logs or stderr.
• What a classifier returns: a failed Future, a started failing Task, a task that swallows cancellation then raises, a failing gather, wrap_future, run_in_executor. After GC there was no asyncio ERROR record and no payload. The exception is shield (P2-4).
• Classifier validation: async, partial(async), partial(partial(async __call__)), a raising __signature__, positional-only and max are all rejected. Keyword and partial classifiers are accepted. On 3.14.0a6, a TYPE_CHECKING-annotated classifier is accepted with nothing evaluated.
• Exactly once: [bq, retry], [retry, bq], [bq1, retry, bq2] and [bq1, bq2, retry], each with 1 and 2 parallel raising calls, give one TOOL_ERROR per call per instance. AgentTool nesting with the retry plugin, in both orders: one row per call. An error whose __str__ raises, answered by an agent on_tool_error_callback: one TOOL_ERROR.
• Upstream merges since round 4: NodeTool now raises into on_tool_error, and the tool thread pool runs only the tool body off-loop. Both use the same callback path the marker relies on. The newer upstream 09601045 touches neither PR file, and doesn't change McpTool's isError dump.

CI (out of scope, verified)

• Mypy's "new" errors are in src/google/adk/dependencies/_mcp.py (from upstream e738c26f) and src/google/adk/utils/streaming_utils.py. No PR commit touches either file, git diff def458b6 380335e4 -- src/google/adk/utils/streaming_utils.py is empty, and local mypy on the plugin is clean.
• The Unit Tests jobs are cancelled at 10 min with F/E counts 1,1,1,1,2 (3.10 to 3.14). Upstream's own run for fd14aec2 (this tree minus the two PR files) shows 1,1,2,2,1, so the difference is noise.

Findings

P0: none. P1: none.

P2-1: the sentinel for override models hits popular library models whose override does forward warnings, and it applies plugin-wide.

Classification: non-hostile, but this is the disclosed trade-off, not a misclassification. No leak and no row loss; the payload is replaced by a sentinel. Recommended before the upstream PR.

bigquery_agent_analytics_plugin.py:2153-2157 records the sentinel for any Pydantic model whose model_dump.__func__ isn't pydantic.BaseModel.model_dump. In the installed dependencies that includes:

• sqlmodel.SQLModel (0.0.47);
• google-genai 2.25's public google.genai.interactions models (170 classes, e.g. Interaction, via google.genai._gaos.types.basemodel.BaseModel);
• LiteLLM ModelResponse;
• llama-index instrumentation events.

SQLModel and google-genai both pass warnings= on to Pydantic's own dump, so for them the sentinel prevents nothing.

Evidence:

• A SQLModel Hero table row returned by a tool: head records ['[UNSUPPORTED_OBJECT]'], where base recorded [{'id': 1, 'name': 'Deadpond', 'secret_name': 'Dive Wilson'}].
• Interaction(status="completed", id=...): head records [UNSUPPORTED_OBJECT]; base recorded {'status': 'completed', 'id': ..., 'output_text': ''}.
• The same happens for session state, since _recursive_smart_truncate is shared by state, workflow node outputs, agent state, custom metadata, A2A payloads and attributes. The release note only says "A tool result model".

Suggested fix:

• Trust audited forwarding overrides by identity, looked up through sys.modules like _imported_types:

  _FORWARDING_MODEL_DUMP_OWNERS = (("sqlmodel.main", "SQLModel"), ("google.genai._gaos.types.basemodel", "BaseModel"))
  trusted = (pydantic.BaseModel.model_dump, *(c.__dict__["model_dump"] for m, n in _FORWARDING_MODEL_DUMP_OWNERS for c in _imported_types(m, n)))

  and test __func__ against that tuple with is.

• I prototyped this in a /tmp copy. The plugin file still passes (685), all my override probes still pass, SQLModel rows and Interaction get their payload back, and a SQLModel row with a mistyped id puts 0 markers on stderr where base leaks it.

• At minimum, the release note should name SQLModel and google-genai Interactions models, and say the rule applies to every value the plugin serializes, not only tool results.

P2-2: Pydantic's own model_dump returning a non-container still falls through to __dict__ and records Field(exclude=True) values.

Classification: no hostile class needed (an ordinary @model_serializer returning Decimal, UUID, datetime, an Enum or a set), but pre-existing and byte-identical on base def458b6, and not claimed fixed. Claim 2 is scoped to raising dumps, and it holds. So this is not blocking under the final-round cap.

At :2158-2167 the branch returns only for containers and scalars. Anything else drops to the __dict__ fallback at :2225, which records every public field, with is_truncated=False. Round 7's own rationale ("reading the fields instead would record what it leaves out", :2150-2151) applies here too.

Evidence: a model with amount: Decimal, internal_note: str = Field(exclude=True) and @model_serializer returning self.amount. The TOOL_COMPLETED row records {"amount": "9.99", "internal_note": "EXCL-SCALAR-NOTE"} on head and on base. The same happens with UUID, datetime and frozenset serializers.

Fix: after the scalar check, add:

if _is_instance(obj, pydantic.BaseModel):
  return _recursive_smart_truncate(dumped, max_len, seen, depth + 1, budget)

I prototyped it in /tmp: 685 plugin tests pass, all 103 of my probes pass, and the rows become '9.99', the UUID string and so on.

P2-3: "Recorded as the sentinel whether it is the result or is nested in it" holds only for nesting in a dict, list or plain object.

Classification: doc precision; non-hostile, pre-existing and consistent with Pydantic.

An override model that is a field of another Pydantic model, or wrapped in RootModel[...], is dumped by Pydantic's schema serializer. The override isn't called, which is true as claimed, but what it leaves out is recorded:

• Outer(inner=Delegating()) gives {'inner': {'number': ..., 'excluded': 'OVERRIDE-OMITS'}}, on head and on base.
• The next LLM_REQUEST content_parts (with log_multi_modal_content on, the default) also carries the full schema dump of a top-level override model, including excluded. That is what the model receives, so the sentinel's privacy benefit only applies to TOOL_COMPLETED rows.

Fix: in the PR body's round-7 bullets (lines 60 and 170), say "top level or in a dict or list", and note the model-field case.

P2-4: a classifier returning asyncio.shield(task) still leaks at GC.

Classification: category 2 (deliberate asyncio misuse in a synchronous classifier).

_release_awaitable (:954) cancels the outer future that shield returns and retrieves its exception. But shield detaches the inner task the classifier created, which later fails. At collection, asyncio logs Task exception was never retrieved ... exception=RuntimeError('FBL-SECRET-task') at ERROR, and that text is payload. This is the same class as the disclosed "custom awaitable wrapping a failed future": the plugin can't reach the inner task.

Fix: docs only. Add "asyncio.shield, or any task the classifier creates and does not return" to the classifier docstring and the hostile-case list.

Judgement on the disclosed trade-off (override models become the sentinel)

It is acceptable as a default for unknown overrides: it is provably leak-free, and the rows are kept and flagged is_truncated.

It is under-disclosed and broader than it needs to be. It silently empties results from SQLModel, google-genai Interactions and LiteLLM, and it reaches every event type. The leak it prevents needs an override that drops the flag and a field mutated to the wrong type, which is not the case for the libraries above.

P2-1's allowlist keeps the leak-freedom and restores those payloads. I'd do it, or at least fix the release note, before upstreaming.

On R3: its "common types" clause targeted non-overriding types misclassified as overrides, and I found none. SQLModel and genai Interactions models genuinely override model_dump, so I've treated their sentinel as the disclosed trade-off, not as an R3 failure. If you read R3 literally, P2-1 is the item to reconsider.

VERDICT: APPROVE @ 380335e