fix(plugins): record content_formatter failure class in BigQuery analytics error_message

[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.

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):

• Related: docs: README Prerequisites link is 404 and Quick Start fails on non-default table_id/location GoogleCloudPlatform/BigQuery-Agent-Analytics-SDK#485 (item A in the first comment: "ADK plugin: content_formatter failure is undiagnosable")

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

Problem:

When BigQueryLoggerConfig.content_formatter raises (the reported case is a
plain ImportError from a missing import inside the formatter), or returns a
type the parser cannot store, BigQueryAgentAnalyticsPlugin writes the row
with content [FORMATTER_FAILED], logs a constant warning, bumps the
formatter_failed drop counter, and leaves error_message NULL. The developer
gets no signal about why the formatter failed.

The fail-closed behavior is intentional and stays: the formatter is a redaction
boundary, so its failure must never write or log the unformatted payload, the
exception message, or the traceback, all of which can embed that payload. The
plugin also deliberately stopped logging type(e).__name__, because
type(name, ...) can create a class named after the payload
(test_formatter_logs_never_carry_payload_derived_names). So the obvious fix,
error_message = type(e).__name__, would reintroduce that leak into a more
durable sink.

Solution:

1. error_message names the failure only by a trusted class label. Both
   failure paths set a fixed-shape message:

   Formatter behavior	error_message
   raises ImportError(...)	content_formatter raised ImportError
   raises google.api_core.exceptions.NotFound	content_formatter raised <subclass of GoogleAPICallError>
   raises a class it defines, or one created by type(name, ...), even if bound into a module	content_formatter raised <subclass of ValueError>
   raises a class that even type's own descriptors cannot read	content_formatter raised <unknown class>
   returns a tuple	content_formatter returned unsupported type tuple
   is an async def formatter, or returns a generator	content_formatter returned unsupported type coroutine (or generator)
   returns a local LlmRequest subclass	content_formatter returned unsupported type <subclass of LlmRequest>
   returns an object whose __class__ only claims to be str, dict, or list	content_formatter returned unsupported type <subclass of object>

   A class gets a label only if no runtime data can have chosen it. That is
   either:

   • a static type compiled into C (every built-in exception and type), whose
     name is fixed at build time; or
   • one of five allowlisted classes, matched by identity, whose labels are
     fixed strings in the plugin source: LlmRequest, types.Content,
     types.Part, pydantic BaseModel, and google.api_core
GoogleAPICallError.

   Every other class, including every Python-defined library exception and
   every class the developer defines, is described by its nearest labeled
   ancestor. Classes are read only through type's own descriptors, and
   allowlist matching uses identity, so no metaclass hook runs. The trade-off
   is fewer exact names; for example, json.JSONDecodeError reads
   <subclass of ValueError>. debug_content_formatter_errors shows the exact
   class locally.

2. One boundary settles everything after the formatter call. The
   formatter call keeps its own except Exception. After it,
   _settle_formatter_outcome does these steps, all behind one boundary:

   • Judge the result by its real type. It uses issubclass(type(x), ...)
     and identity, never isinstance, which would fall back to the object's
     own __class__ and run its code. A subclass of str, dict, or list
     is accepted, and a str subclass is normalized to the exact built-in
     string, so its hooks never run. An object whose __class__ merely claims
     to be one of these is now a counted failure rather than an
     [UNSUPPORTED_OBJECT] row.
   • Close a rejected coroutine or generator, inside its own guard. For an
     unstarted one, this runs no code and emits no "coroutine '' was
     never awaited" warning, whose name a formatter can set from the content.
   • Name the class.
   • Render the debug traceback.
   • Emit the warning (see 5 for how every plugin log call is emitted).

   What it guarantees, and where that stops. No failure while describing
   a failed or rejected result can drop the row, leave the sentinel out,
   change the counter, or escape the callback, apart from the interrupts
   described next, which are raised only after the row is written. Two inner
   fallbacks keep the warning logged when a step fails: labeling falls back
   to <unknown class>, and rendering falls back to
   [traceback could not be rendered]. A result the parser logs natively
   leaves the boundary unchanged, and the content parser's own boundary
   catches Exception only. So a deliberately hostile dict or list
   subclass whose __class__ raises an interrupt inside the parser still
   drops the row, exactly as on main (see the follow-ups).

   Interrupts. Python cannot tell a KeyboardInterrupt or SystemExit
   that a signal handler raised from one raised directly, and code can even
   signal its own process. So the boundary sorts interrupts by the code that
   raised them:

   • Code of the failed class or the rejected result runs in two steps only:
     rendering the debug traceback, and closing a rejected coroutine or
     generator. Each has its own guard, and whatever it raises, interrupts
     included, is contained, so the content under redaction cannot end the
     agent run by raising.
   • Every other step runs only plugin code and the application's own log
     filters and handlers. A KeyboardInterrupt or SystemExit there came
     from a signal or from the application. It is set aside, the row is
     handed to the writer, and _log_event then raises a new exception
     without text in its place; a SystemExit keeps an int exit code. Both
     the row and the signal survive. It is raised while a context-free
     stand-in is handled, so its __context__ is that stand-in: it is chained
     to no exception the caller is handling, even for code that walks
     __context__ and ignores __suppress_context__, which is all
     from None would have covered.
   • CancelledError is always contained, because nothing in the boundary
     awaits, so it cannot be a real cancellation.
   • The trade-off: a signal that lands while one of the two
     payload-controlled steps runs is still absorbed. Those steps are brief
     unless the payload-controlled code itself blocks.
   • KeyboardInterrupt, SystemExit, and CancelledError raised by the
     formatter call itself still propagate, as before.

3. Precedence: the event's own error_message stays first. A row that
   already carries a diagnostic (a TOOL_ERROR or LLM_ERROR) keeps it
   intact and first. The formatter failure is appended after "; ", as in
   upstream timed out after 30s; content_formatter raised ImportError. An
   empty existing message is treated as absent. The note bypasses the bounded
   error sanitizer by design, because it is fixed text plus a trusted label.

4. Opt-in BigQueryLoggerConfig.debug_content_formatter_errors: bool = False. When True, the plugin renders the formatter exception's
   traceback to text once and appends it to the warning; log handlers never
   receive the live exception. Whatever the exception's own code raises while
   rendering, a constant placeholder is logged instead, and the row is
   unaffected. The traceback is never written to BigQuery. The docstring
   carries the leakage caveat: the traceback can include the unformatted
   content, and it reaches every configured handler, including stderr when a
   handler fails, because handleError echoes the record's arguments. The
   default False keeps today's constant, traceback-free warning.

5. Every plugin log call runs while a stand-in exception is handled. The
   module logger's handle runs each record's filters and handlers while a
   constant stand-in exception is being handled, with its __context__
   cleared. A failing handler's handleError, or a handler that reports the
   current exception, therefore prints only the stand-in. It never prints an
   exception the caller is handling; ADK runs error callbacks inside its own
   except. This covers every plugin warning, not just the formatter's: the
   plugin logs only through this module logger. Records are unchanged,
   because Logger._log resolves exc_info and the calling function before
   handle runs, and the process-global logging.raiseExceptions is left
   alone. The class's handle is looked up on every call, so a patch applied
   to logging.Logger.handle after import, as instrumentation and test
   fixtures apply it, still reaches this logger, and still runs while the
   stand-in is handled.

6. Downstream effect on error predicates (ruled acceptable in review; no
   behavior change). The row's status is left as the event set it, so a
   formatter failure puts an error_message on a status = 'OK' row of any
   event type, and any query that counts a non-NULL error_message as an
   error counts that row. That includes the BigQuery Agent Analytics SDK's
   canonical error predicate, ENDS_WITH(event_type, '_ERROR') OR error_message IS NOT NULL OR status = 'ERROR':

   • in event_semantics.ERROR_SQL_PREDICATE and udf_kernels.is_error_event;
   • copied inline for session-level has_error in
     src/bigquery_agent_analytics/system_evaluator.py (about L662-666),
     insights.py (about L401-405), and trace.py (about L247-253 and
     L737-743).

   So the SDK dashboards and examples/continuous_query_alerting.sql count
   formatter-failed rows as errors, and one formatter failure marks its whole
   session has_error in evaluation and insights output. Both reviewers ruled
   this acceptable: item A asks for error_message, and a formatter failure
   is real content loss on that row, which should be loud rather than silent.
   The content_formatter docstring states the effect.

   Recommended SDK-side follow-up (BigQuery-Agent-Analytics-SDK, not this
   PR). Give formatter-failure rows their own predicate, for example a
   FORMATTER_FAILED_SQL_PREDICATE. Keep them out of ERROR_SQL_PREDICATE,
   is_error_event, and the inline has_error copies, so that an evaluation
   does not penalize an agent for a misconfigured logging pipeline. Key the
   new predicate, preferably, on a structured marker, such as an
   attributes key written next to the note, rather than on the note's
   text: the note follows event-controlled text, so a text match can be
   spoofed. The marker is a new data contract, to be designed with the SDK
   owners, so this PR does not add it.

The warning text (with the flag off), the [FORMATTER_FAILED] sentinel, and
the formatter_failed counter are unchanged. The error_message column
description now says it may be populated on status = 'OK' rows whose
formatter failed. That affects new tables only; _SCHEMA_VERSION is unchanged.

Testing Plan

Unit Tests:

• I have added or updated unit tests for my change.
• All unit tests pass locally. See the notes below: every failure seen is environmental and reproduces on unmodified main.

TestContentFormatterFailureDiagnostics in
tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py has 115 tests.
All of them go through the real Arrow write path.

• test_raised_interrupt_is_not_chained_to_the_callers_exception (2 cases,
  KeyboardInterrupt and SystemExit): an application handler raises the
  interrupt during the warning while the caller handles a payload exception.
  The row is written, and the interrupt that follows links, through
  __context__, to nothing but a constant stand-in that is itself chained to
  nothing.

• test_later_patches_of_logger_handle_see_plugin_records: a class-level
  patch of logging.Logger.handle applied after import sees the plugin's
  formatter warning, while the stand-in is handled.

• test_a_raise_anywhere_in_diagnosis_leaves_the_row_intact: 45 cases, the
  injection property.

  • It raises RuntimeError, CancelledError, KeyboardInterrupt,
    SystemExit, and another BaseException at every step after the call:
    judging the result, closing a rejected generator, naming the class,
    rendering, a raising log handler, and a raising log filter. Each step runs
    on both failure paths where it applies.
  • Each case requires the sentinel row, one drop count, the right
    error_message, and no payload in the row or the default log. A
    KeyboardInterrupt or SystemExit from any step but closing must be
    raised after the row, as a new exception without text; everything else
    must be contained. No step may leave an exception for the unraisable
    hook.

• test_genuine_signal_during_the_warning_is_raised_after_the_row (2 cases;
  skipped on Windows): a real SIGTERM, whose handler calls sys.exit(143),
  and a real SIGINT are sent with os.kill while a slow handler emits the
  warning. The row is written, and then SystemExit(143) or
  KeyboardInterrupt is raised.

• test_plugin_warnings_never_print_the_callers_exception (2 cases): the
  parser fails while the caller handles a payload exception, with a stock
  handler on a closed stream and with a handler that walks __context__.
  Neither prints the payload.

• test_plugin_error_logs_keep_their_own_exception: _safe_callback's error
  log still carries its own exc_info.

• test_supported_results_pass_through_unchanged (7 cases): None, a str
  subclass whose hooks raise, dict and list subclasses, and exact
  Content, Part, and LlmRequest are logged as before, uncounted.

• Round-4 tests:

  • a failing handler never printing the caller's active exception (2 cases);
  • a result whose __class__ lies or raises, rejected unrun (10 cases);
  • a rejected coroutine closed without a warning (2 cases).

• Earlier tests:

  • the trusted-label table (16 cases);
  • a renamed allowlisted class;
  • metaclass hooks that never run (6 cases);
  • an unreadable class on both paths;
  • unrenderable tracebacks (6 cases);
  • interrupts from the formatter call still propagating (3 cases);
  • a failing handler never printing the formatter exception;
  • exception text never reaching the row;
  • precedence (3 cases): appended after the event's own message; an empty
    message, which a model error with no text leaves, treated as absent; and
    a tool error with no text, which fork PR fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15 records by its type name,
    kept first;
  • no traceback by default;
  • the debug traceback appearing locally but not in the row.

Hostile hooks fire only while armed, and anything that escapes becomes an
ordinary test failure, so a regression never crashes pytest's own reporting.

Red/green. Against the plugin at the previous head 9f16d70b, the 3 new
round-6 cases fail and the other 111 pass. They fail because the re-raised
interrupt was chained to the caller's exception (2 cases) and because a
class-level patch of Logger.handle never saw the plugin's record (1 case).

In round 5, against 7fae9655, 25 of the then-111 cases failed and 86 passed.
Each failure matched a reviewed defect:

• 18 interrupts were absorbed instead of raised after the row: 16 injected
  and 2 real signals;
• 5 raises from a rejected generator's cleanup knocked the label down to
  <unknown class>;
• 2 non-formatter plugin warnings printed the caller's payload exception.

The 7 pass-through cases pass on 7fae9655 too, because they pin guards it
already had. The mutation check shows that they catch those guards' removal.

Mutation check. On the merged head, I applied 46 mutations one at a time,
and 45 of them make at least one test fail. New in round 6:

• raising the set-aside interrupt without the stand-in, or leaving the
  stand-in's own __context__ in place;
• binding Logger.handle at import again.

From round 5:

• not installing the logger stand-in, handling records after its except
  block, or keeping its __context__;
• never raising the set-aside interrupt, never setting it aside, setting
  CancelledError aside too, raising the original instead of a new one,
  raising it before the row, or keeping a non-int exit code;
• closing a rejected generator outside its own guard;
• not normalizing a str subclass, or not passing None through.

Every earlier guard is still caught, including closing a rejected generator
at all. That mutation needed the new unraisable-hook check, because the
inner close guard makes the row identical either way. The survivor reads a
static type's name through normal attribute access. It is equivalent, because
that read happens only after the trusted flags check.

$ pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q
799 passed, 2 skipped, 22 warnings in 11.11s
# 801 collected. Fork main 2c16d9dc has 686 and google main 552 in this file.
# The skips: toolbox_adk is not installed here, and one test needs Python 3.14.

On Python 3.10.16 and 3.13.7, with toolbox-adk installed, the diagnostics
class passes (115), and the whole plugin file gives 800 passed and 1 skipped,
the 3.14-only test.

Full suite, at the merged head f3d2ceb2. I ran pytest tests/unittests -n 8 --timeout=300 on Python 3.11 on an Intel macOS machine, in four parts to fit a
10-minute limit: 16662 passed. Every failure is environmental and
reproduces on unmodified main:

• google-antigravity has no x86_64 wheel: 4 sample loads fail and 4 labs
  modules fail to collect;
• test_import_loading (2 cases) picks up Homebrew's sitecustomize.

Three known hangs were deselected: two LiveKit tests and one cli login test.

pyink, isort, ruff, codespell, addlicense, and the ADK compliance hooks pass
(pre-commit run --files on both files). mypy (strict) reports no issues for
the plugin module.

Manual End-to-End (E2E) Tests:

Standalone scripts drive the public on_user_message_callback and
on_tool_error_callback with a real InvocationContext. They capture each
assembled row at the batch-processor boundary, with no live BigQuery.

Behavior on main, then with this PR:

A. formatter raises ImportError   -> main: error_message=None
                                     now:  'content_formatter raised ImportError'
B. TOOL_ERROR + failing formatter -> main: 'upstream timed out after 30s'
                                     now:  'upstream timed out after 30s; content_formatter raised ImportError'
C. formatter returns a tuple      -> main: None     now: 'content_formatter returned unsupported type tuple'
D. payload-named exception class  -> main: None     now: 'content_formatter raised <subclass of ValueError>'

The round-4 scenarios, at 974e4642 and then with this PR:

a1. caller handling a payload exception; closed handler; formatter re-raises it
    974e4642: payload printed to stderr        now: stderr payload-free, row written
a2. caller handling a distinct payload exception; closed handler
    974e4642: payload printed to stderr        now: stderr payload-free, row written
b.  returned object's __class__ raises CancelledError / SystemExit / KeyboardInterrupt
    974e4642: escaped the callback, 0 rows     now: 1 row, 'content_formatter returned unsupported type <subclass of object>'
    ... raises RuntimeError
    974e4642: 'content_formatter raised RuntimeError' (misattributed)   now: returned unsupported type <subclass of object>
c1. payload-named coroutine
    974e4642: "coroutine 'TOPSECRET_…' was never awaited"   now: no warning
c2. async def formatter
    974e4642: "coroutine '…async_formatter' was never awaited"   now: no warning

The round-5 scenarios, at the previous head 7fae9655 and then with this PR:

ra1. real SIGTERM (handler calls sys.exit(143)) while a slow handler emits the warning
     7fae9655: signal absorbed, 1 row       now: 1 row, then SystemExit(143) raised
ra2. real SIGINT, same
     7fae9655: signal absorbed, 1 row       now: 1 row, then KeyboardInterrupt() raised
ra3. application log handler calls sys.exit(3)
     7fae9655: absorbed, 1 row              now: 1 row, then SystemExit(3) raised
ra4. formatter's exception raises KeyboardInterrupt while rendered (debug on)
     7fae9655: contained, 1 row             now: contained, 1 row
ra5. rejected generator's cleanup raises SystemExit
     7fae9655: contained, '... <unknown class>'   now: contained, '... unsupported type generator'
rb1. parse-failure warning; closed handler; caller handling a payload exception
     7fae9655: payload printed to stderr    now: stderr payload-free
rb2. parse-failure warning; handler walking __context__; caller handling a payload
     7fae9655: payload reached the handler  now: payload-free
p1.  formatter returns a str subclass whose hooks raise SystemExit
     7fae9655: logged as 'redacted text'    now: same

The round-6 scenarios, at the previous head 9f16d70b and then with this PR:

p21. an app log handler raises KeyboardInterrupt during the warning,
     while the caller handles a payload exception
     9f16d70b: KeyboardInterrupt() after 1 row; __context__ is the caller's ValueError (payload)
     now:      KeyboardInterrupt() after 1 row; __context__ is _LoggingStandIn, chained to nothing
p22. a class-level patch of logging.Logger.handle applied after import
     9f16d70b: it saw 0 plugin warnings     now: it saw 1, while _LoggingStandIn was handled

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. The plugin test file passes; the rest of the suite is covered in the notes above.
• I have manually tested my changes end-to-end. The repro captures rows before the BigQuery write; it was not run against a live table.
• Any dependent changes have been merged and published in downstream modules. There are none.

Additional context

• Review follow-up (round 6). Commit b8f50f8a:

  • raises the set-aside interrupt while a context-free stand-in is handled,
    so it is chained to no exception the caller is handling;
  • looks up Logger.handle on every call, so later class-level patches apply;
  • states the downstream effect on error predicates in the
    content_formatter docstring. No behavior change; see Solution 6.

• Merge with fork main, which now carries PR fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15 (2c16d9dc, "record
  error-bearing tool results as TOOL_ERROR"). The merge commit is
  f3d2ceb2, and google main d312c0ec was merged first, cleanly.

  • Conflicts. Only the plugin and its test file conflicted, and every
    conflict was additive, so both sides are kept:
    • fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15's import pydantic and this PR's from pydantic import BaseModel
      are both kept, because each side's code uses its own form.
    • In BigQueryLoggerConfig, fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15's tool_result_classifier field and
      docstring entry come first and debug_content_formatter_errors comes
      last. fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15's already-merged positional index is therefore unchanged,
      and the new field goes at the end, as the class's comment asks.
    • In the tests, the functools (fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15) and io imports are both kept.
  • One interaction. fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15 records a tool error that has no message by its
    type name, so on_tool_error_callback no longer yields an empty
    error_message. This PR's precedence rule keeps that message first, as
    in RuntimeError; content_formatter raised ImportError. The precedence
    test's empty-message case now comes from on_model_error_callback,
    which still records str(error) as is, so it keeps pinning that an empty
    message is treated as absent. A new case pins the combined behavior.
  • Other interactions (no change needed). fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15's TOOL_ERROR rows for
    error-bearing results set their own error_message: a fixed message for
    an MCP error flag, the retry answer's error text, or the classifier's
    message. They go through _log_event, so a formatter failure there is
    appended after that message. fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15's warnings go through the module
    logger, so they also run while the stand-in is handled.
  • Checks.
    • The merged plugin is exactly this PR plus fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15, line for line: the
      changed lines equal fix(plugins): record error-bearing tool results as TOOL_ERROR in BigQuery analytics #15's patch (693 lines) and this PR's patch (498
      lines).
    • Every file outside the plugin and its test file is identical to google
      main d312c0ec.
    • The merged test file collects 801 tests. They cover every test node of
      this branch (666) and of fork main (686), plus the one new precedence
      case.
    • All 801 pass, apart from the environment skips noted above.

• Downstream effect and SDK follow-up. See Solution 6.

• Docs follow-up for adk.dev (google/adk-docs, not this PR). Document the
  [FORMATTER_FAILED] sentinel, the formatter_failed counter, the
  error_message prefixes above, and debug_content_formatter_errors.

• Follow-ups (not fixed, hostile-class edge cases). Each also happens on
  main, and in each the row itself is either written with the sentinel or
  dropped exactly as on main:

  • A rejected, already-started async generator whose cleanup raises.
    Closing it means awaiting aclose() inside the callback, which has its
    own cancellation semantics. The row is correct, and the text reaches
    asyncio's default logger only after a formatter starts an async generator
    and returns it. Astra rates this reachable without a hostile class.
  • An admitted dict or list subclass whose __class__ raises an
    interrupt in the content parser. The fix is to dispatch the parser on
    real types and give its boundary the same interrupt policy, which is a
    parser change beyond this PR.
  • A payload-named coroutine nested inside a rejected container. Closing
    it needs a bounded walk of rejected built-in containers. The top-level
    case is fixed.
  • A rejected object's raising __del__, or a generator that yields again
    on GeneratorExit. Finalizers run whenever the last reference goes,
    outside any boundary the plugin holds, and no close() call can contain
    arbitrary finalization.
  • A metaclass __subclasscheck__ on the raised class that raises an
    interrupt. CPython runs it while raising, before the formatter call's
    except can match, and the result looks exactly like an interrupt from
    the formatter call, which deliberately propagates.

• Known pre-existing path, not changed here. When one of the plugin's
  own callbacks fails while the caller is handling an exception, as in an
  error callback, _safe_callback's logger.exception renders the plugin
  error with its implicit __context__ chain, which includes the caller's
  exception text. This needs no failing handler and no hostile class, and it
  is identical on main. The stand-in in 5 does not change records, so it
  does not cover this. Trimming that chain would change what the error log
  shows, so it is left as a follow-up.

• Cosmetic, not changed. A plain ExceptionGroup is a heap type, so it
  reads <subclass of BaseExceptionGroup>. A whitespace-only existing
  error_message gives " ; content_formatter ...".

🤖 Generated with Claude Code

[caohy1988]

Independent blind review by Astra (Codex), on Haiyuan's behalf, of head 48dd52cf989267a0fa675003d9c700caab84e14f against base 2c759e9e6e0a12e74e2d114a962559e1152d9f26. Read the PR description, full diff, implementation commit e13f9fba, relevant repository code, and issue google#485 item A. No peer reviews or excluded worktrees/vault files were read. This is a COMMENT review; the technical recommendation is REQUEST_CHANGES.

Rejection criteria — recorded before reading the diff

1. A formatter failure leaks payload-derived text, including dynamically named classes, into a BigQuery row or default logs, or changes the sentinel, counter, or constant warning.
2. Diagnostic handling escapes the fail-closed boundary, drops a row, erases an existing event error, or writes opt-in exception details to BigQuery.
3. Independent checks disprove claimed behavior or coverage for either failure path, precedence, debug logging, or adversarial formatter objects. An unresolved intent conflict over consumer error classification requires escalation.

P0: None found.

P1 — Module registration does not establish that a class name is safe.

Location: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:1037–1039, also used for ancestors at lines 1066–1069.

The PR's assertion that a runtime type() class is not bound under its name in its module is false. Dynamic class factories can register their products, including for pickling. This formatter is sufficient:

def formatter(content, event_type):
    name = content.parts[0].text
    cls = type(name, (ValueError,), {"__module__": __name__})
    globals()[name] = cls
    raise cls("irrelevant")

Using the synthetic input ASTRA_SYNTHETIC_PRIVATE_ACCOUNT_73951, the real Arrow write path produces:

content = [FORMATTER_FAILED]
error_message = content_formatter raised ASTRA_SYNTHETIC_PRIVATE_ACCOUNT_73951
formatter_failed = 1

Returning a registered unsupported object leaks the same payload; an unregistered subclass of a registered payload-named class leaks it through <subclass of ...>. All four cases retain constant default logs but persist the protected payload in the row. All four fail on this head and pass on the base plugin. This directly violates rejection criterion 1.

Suggested fix: Replace runtime module-binding/name inference with fixed labels for a small, explicitly trusted set of type identities, and a constant fallback or trusted ancestor label for everything else. Do not trust arbitrary heap-type names, including ancestors. Add raise/return regressions for registered payload-derived classes and their subclasses, checking every serialized column and default logs.

P1 — Diagnostic introspection can cancel the callback and drop the row.

Location: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:1031–1041 and :1065–1072; called before sentinel/counter assignment at lines 7313 and 7333.

Attribute reads on a class invoke its metaclass. For example:

class Meta(type):
    def __getattribute__(cls, name):
        if name == "__qualname__":
            raise asyncio.CancelledError("synthetic payload")
        return super().__getattribute__(name)

class FormatterError(ValueError, metaclass=Meta):
    pass

A formatter raising FormatterError() raises an ordinary ValueError subclass. The new helper then causes cancellation while inspecting it. Its except Exception does not catch CancelledError, nor does _safe_callback. Through on_user_message_callback, I observed escaped=CancelledError, zero Arrow rows, and no formatter_failed increment. This reproduces for both raise/unsupported-return paths and for each of __qualname__, __module__, and __mro__: six failures on head, six passes on base. These are synchronous metadata accesses, not actual task cancellation requests.

Suggested fix: Avoid executing user-defined metaclass hooks during diagnostic classification; obtain any needed structural metadata through trusted built-in descriptors and use fixed labels/fallbacks. Ensure diagnostic construction cannot bypass sentinel assignment, accounting, and row delivery. Add these regressions without globally swallowing genuine asynchronous cancellation.

P2 — Opt-in traceback logging can itself abort row delivery.

Location: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:7327–7335.

With debug_content_formatter_errors=True, an exception whose __getattribute__("__class__") raises RuntimeError causes traceback formatting to fail. This reproduces with a standard Python logging.StreamHandler, a standard formatter, and logging.raiseExceptions=True (Python's default), not just pytest's capture handler. The warning and subsequent _safe_callback traceback logging fail; the callback escapes with RuntimeError, zero rows, and no formatter-failure count. The same standard-handler probe retains its sentinel row on the base plugin. Ordinary hostile __str__/__repr__/__class__ exceptions with debug disabled remain safe.

Suggested fix: Make opt-in diagnostic logging best effort within the failure boundary. A failure to render or emit the debug traceback must fall back to safe constant logging and must not prevent sentinel content, the failure counter, or row delivery. Test an actual standard handler with a hostile exception and debug both off/on.

What I tried and verified

• Isolated .venv, CPython 3.11.13. All-extras installation hit unavailable Intel macOS wheels (google-antigravity, then unrelated onnxruntime/lancedb); installed the plugin/dev/test/A2A/MCP dependencies instead. Binary-only cryptography resolution selected 48.0.1 after 50.0.1 failed to build. No tracked dependency changes.
• python -m pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q: 565 passed (after installing A2A/MCP; the initial run had six optional-dependency skips).
• The PR's 13 new tests against the base plugin: 11 failed, 2 passed, matching the red/green claim. Independently reran all seven described mutations: exception text, unconditional exc_info, omitted metaclass guard, arbitrary class names, replacing the prior error, formatter note first, and omitted static-type handling. Every mutation was detected by the new tests.
• 36 independent adversarial cases: 24 passed, 12 failed. The failures are four payload-name leaks, six metaclass-induced cancellations, and two debug-log row-loss reproductions (capture handler and standard handler). The ten privacy/metaclass regression cases all pass on base; the two standard-handler settings also pass on base.
• Passing attempts included unregistered payload-named exceptions/results, false static-type flags, ordinary exceptions from metaclass attributes, invalid/string-subclass qualified names, exceptions with hostile __str__/__repr__/__class__, tuple/generator/coroutine/async-generator/awaitable returns, recursive formatters, re-entrant callbacks, NotFound, existing error precedence on both failure paths, and ordinary opt-in traceback logging. Checked real Arrow rows, counters, and default log text.
• Pinned pyink 25.12.0 --check and isort 8.0.1 --check-only pass on both files. Ruff, codespell, and the compliance script pass. Strict mypy with --follow-imports=silent reports no issues in the plugin module.
• Validation limits: no live BigQuery write, whole-repository suite, or independent reproduction of the PR's whole-suite environmental diagnoses. addlicense is unavailable. The PR's class-provenance and non-raising-helper claims are disproven above; its targeted test/mutation claims reproduced.

Consumer error classification decision: acceptable for the stated intent.

I checked the SDK's ERROR_SQL_PREDICATE and continuous-query example. A failing formatter on a status='OK' user-message row does count as an error and as a medium-severity alert. Item A explicitly requests error_message, and the PR discloses this effect. A redaction/telemetry failure is a meaningful error even when agent execution succeeds; I do not consider this an additional finding or unresolved intent conflict. It should not be represented to consumers as an agent-execution failure rate.

Probe sources and logs: /tmp/adk-pr14-dual/astra-probes/. Temporary base/mutation substitutions were restored byte-for-byte; tracked worktree and index are clean. No branch push or merge.

VERDICT: REQUEST_CHANGES @ 48dd52c

[caohy1988]

Fable (Claude) blind disproof review: PR #14 @ 48dd52c

Verdict: approve. None of my three rejection criteria is met. There are no P0 or P1 findings. Three P2s follow, all non-blocking.

Scope. The real change is e13f9fb. 48dd52c only merges upstream main, and git diff 2c759e9e..48dd52cf touches only the plugin and its test file.

Sources. Blind review: I read the PR body, the diff, issue google#485 item A, and the repository code. For the design question only, I also read the public BQAA SDK source (event_semantics.py, udf_kernels.py). I read no other review or comment on this PR.

Rejection criteria (written before reading the PR body or diff)

• RC1 Leak. Any formatter-failure path (raise, unsupported return, or the new helper) lets payload-derived text reach any BigQuery column, or the default Python logs with the flag off. Payload-derived text means str(e), repr(e), args, traceback text, or a class name the payload controls: a runtime-built class, or one whose __name__, __qualname__, or __module__ comes from data.
• RC2 Fail-closed break. The new logic can raise out of the fail-closed block (a metaclass with a raising __name__ or __qualname__, a raising __class__, a __getattr__ trap), drops the row, writes unredacted content, or changes the [FORMATTER_FAILED] sentinel, the formatter_failed counter, or the constant warning text.
• RC3 Contract completeness. Any one of these:
  • an existing error_message (TOOL_ERROR, LLM_ERROR) is overwritten or lost;
  • the precedence is undocumented or untested;
  • debug_content_formatter_errors isn't opt-in (default False);
  • its exc_info reaches BigQuery or changes default logging;
  • the new tests pass on the unfixed code.

Result: none met.

What I ran

Environment: CPython 3.11.13, uv venv in a worktree detached at 48dd52c.

Check	Result
pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q	565 passed (matches PR body)
New test class with the plugin reverted to 2c759e9	11 failed, 2 passed (the two guard tests), matching the PR body; file restored
11 mutations of the new code, each run alone against the new class	all caught (list below)
pyink 25.12.0 --check, isort 8.0.1 --check-only, ruff 0.15.17, codespell, scripts/compliance_checks.py on both files	clean
mypy with the repo config (strict = true) on the plugin module	Success: no issues found in 1 source file
My probes: 40 unit-level cases on _code_defined_class_name and _formatter_failure_message, plus 21 e2e probes through on_user_message_callback and on_tool_error_callback that decode the real Arrow rows	no leaks, no Exception raised, row always written (exceptions noted below)

The 11 caught mutations:

• drop the type(cls) is type guard on __flags__;
• trust any __qualname__;
• always attach exc_info, or never attach it under the flag;
• replace the existing message instead of appending;
• put the note first;
• write str(e) into error_message;
• the issue's naive type(e).__name__;
• drop the static-type path;
• treat "" as present;
• no note on the unsupported-return path.

Not re-run:

• the full tests/unittests suite, since only these two files change;
• addlicense, which isn't installed here (both files keep their Copyright 2026 Google LLC header);
• the PR's manual script, since my e2e probes drive the same callbacks.

Attacks tried

I checked every case in the row and in WARNING+ logs with the flag off. Unless noted otherwise, each row had:

• [FORMATTER_FAILED] content and formatter_failed == 1;

• a class-only error_message with no payload;

• one constant warning with no exc_info.

• Payload-named classes. type(payload, (ValueError,), {}), alone and with these spoofs:

  • __module__='builtins';
  • __qualname__='ValueError' plus __module__='builtins';
  • the qualname and module of google.api_core.exceptions.NotFound.

  All come out as <subclass of …>, because the vars(module).get(qualname) is cls identity check holds.

• Hostile names. Payload only in __module__; __module__ unhashable, or a str subclass whose __hash__ and __eq__ raise; __qualname__ a hostile str subclass. All come out as the ancestor's name.

• Metaclass tricks. All come out as <subclass of …> or <unknown class>:

  • lying about __flags__, via a property and via __getattribute__;
  • __getattribute__ returning the payload or an int for __qualname__;
  • __getattribute__ raising RuntimeError(payload) for __qualname__, __module__, or __name__;
  • __mro__ raising, returning a non-sequence, or returning a payload-named fake base;
  • every attribute raising.

  A metaclass __qualname__ property can't be built at all. CPython rejects it with type __qualname__ must be a str.

• Instance __class__ spoof. No effect, because the plugin passes type(e) and type(formatted).

• Hostile exception text. An exception whose __str__ and __repr__ raise payload text, and ExceptionGroup(payload, [ValueError(payload)]). Both give a class-only row and a clean default log.

• Unsupported returns. All come out class-only:

  • a payload-named instance;
  • an object with a payload __str__ and __repr__ and a spoofed __class__;
  • an async def formatter (coroutine), and a generator;
  • a namedtuple or functional-API Enum named after the payload;
  • a class object, and a builtin function.

  The never-awaited coroutine's RuntimeWarning carries no payload.

• Non-module sys.modules entry. A sys.modules entry that isn't a module and whose __dict__ raises is handled.

• Concurrency. 4 concurrent events, 2 of them failing. Each row carries only its own note, and formatter_failed == 2.

• Precedence.

  • A TOOL_ERROR with max_content_length=20 produces upstream timed out a...[TRUNCATED]; content_formatter raised ImportError.
  • A TOOL_ERROR carrying Authorization: Bearer … produces call failed: Authorization: [REDACTED]; content_formatter raised ImportError. The existing message is sanitized first and kept intact.

• Debug flag on. exc_info is attached to the single WARNING. The payload appears only in the local traceback, as documented, and the row doesn't change.

• Real library classes keep useful names. JSONDecodeError, SSLError, sqlite3 OperationalError, NotFound, pydantic ValidationError, TimeoutError, ModuleNotFoundError, ExceptionGroup.

Three results are outside the threat model; no change requested:

• BaseException from a metaclass hook. Suppose a metaclass __getattribute__ raises a BaseException that isn't an Exception when __qualname__ is read. That escapes the helper and the callback, so the row is dropped where base code wrote the sentinel. This needs deliberately hostile code, and a formatter could raise that BaseException directly with the same effect on base. The plugin's boundaries are except Exception throughout, so "the helper cannot raise" holds for Exception.
• Heuristic residual. Code can bind a payload-named class into its module under that same name (globals()[name] = type(name, ...)). That class passes the module check, and its name is written. The helper docstring states this rule. I found no library that does this with names derived from content, and the rule is strictly safer than the issue's type(e).__name__.
• Pre-existing misattribution, not introduced here. A result whose __class__ claims str, such as unittest.mock.Mock(spec=str), passes isinstance(formatted, str) at plugin.py:7282. The plugin's own str.__str__ then raises TypeError. The row reads content_formatter raised TypeError although the formatter returned a value. Nothing leaks. Optional fix: issubclass(type(formatted), str) would send it to the unsupported-type branch.

Design question: error_message on status='OK' rows and the SDK error predicate

Judgment: acceptable. Not ESCALATE, not P1.

I checked the SDK source. event_semantics.ERROR_SQL_PREDICATE and udf_kernels.is_error_event (ENDS_WITH(event_type,'_ERROR') OR error_message IS NOT NULL OR status='ERROR') will count formatter-failed rows as error events. NO_ERROR_SQL_PREDICATE will exclude them.

Why it's acceptable:

1. It is the stated intent. Issue google#485 item A.1 names error_message as the sink, and no conflicting intent is on record, so there is nothing to escalate.
2. It extends existing behavior. The column already carries diagnostics on status='OK' LLM_RESPONSE rows, and the SDK already counts those as errors.
3. A formatter failure is a real failure. The content is lost and replaced by the sentinel. Surfacing it through the predicate where users look for failures is the signal the issue says is missing.
4. The impact is limited. udf_kernels.tool_outcome keys only on event_type and status, so tool success and error outcomes don't change.
5. The PR discloses it. The PR body names the effect and the attributes alternative.

The cost is real but visible. A formatter that always fails flags every row, so error-rate and alerting views will count a pipeline fault as an agent error. P2-2 covers the follow-up.

Findings

P0: none.

P1: none.

P2-1 (test coverage): no test pins the guarantee that the helper can never raise or drop the row.

With each of these mutations, the new class still reports 13 passed:

• remove the try/except in _code_defined_class_name (plugin.py:1031-1041);
• remove the try/except around the __mro__ walk (plugin.py:1065-1072);
• drop the str.__str__(...) normalisation (plugin.py:1032).

With the first mutation, an exception class whose __qualname__ lookup raises makes the helper raise inside the except block at plugin.py:7333. _safe_callback then logs the chained traceback, which includes the formatter exception's text, and the row is dropped. That is exactly the failure the contract rules out.

Suggested fix: add this case to test_failed_row_names_the_formatter_failure_by_class (test file line 3971):

def _raise_class_whose_name_lookup_raises(content, event_type):
  class _Meta(type):
    def __getattribute__(cls, name):
      if name in ("__qualname__", "__module__"):
        raise RuntimeError(content.parts[0].text)
      return super().__getattribute__(name)
  raise _Meta("X", (ValueError,), {})()

• Expect content_formatter raised <subclass of ValueError>.
• Add a variant that also raises on __mro__, and expect content_formatter raised <unknown class>.
• For both, also assert self.SECRET not in caplog.text.

P2-2 (design follow-up, non-blocking; SDK repo, not this PR). Keep this PR as it is. When upstreaming:

1. Teach event_semantics and udf_kernels.is_error_event to tell this plugin diagnostic apart from an agent error. One option: a status='OK' row whose only diagnostic matches (^|; )content_formatter (raised|returned unsupported type) counts as a data-quality signal. Another: expose a FORMATTER_FAILED_SQL_PREDICATE so dashboards can split it out.
2. List these exact prefixes in the adk-docs follow-up, next to the [FORMATTER_FAILED] sentinel and the formatter_failed counter.

If Haiyuan prefers error rates to stay agent-only, the fallback is the PR's own alternative, attributes. That is a small change at plugin.py:7340-7346, plus the tests.

P2-3 (nit, comment accuracy). The block comment at plugin.py:7244-7248 says every error producer passes the bounded, fail-closed sanitizer "before formatter/parser row assembly". The formatter note is a new producer, appended after that pass at plugin.py:7337-7346, so error_message can exceed max_content_length.

With max_content_length=20 I observed a 72-char error_message. The sanitizer's own output was already 34 chars: 20 plus ...[TRUNCATED]. This is safe only because the note is fixed text plus a class name chosen by code. Suggest one comment line at the append site that states this invariant, so a later producer doesn't copy the pattern with free text.

Claims in the PR body

Verified:

• 565 passed.
• 11 of the 13 new tests fail on base.
• The listed mutations are caught.
• pyink, isort, ruff, codespell, and the compliance hooks pass.
• mypy (strict) is clean.
• The schema-description change affects new tables only. _maybe_upgrade_schema adds columns and records and never rewrites descriptions.
• The SDK predicate impact. I checked the predicate but didn't open the alerting example.

Not re-verified: the full-suite numbers and addlicense.

VERDICT: APPROVE @ 48dd52c

[caohy1988]

Independent review of head 3d94259a75a6e9fe08f8fea1b3f46e957cecc9b2 against def458b609c2811d137b0332b2fc7b201dddd5c0, on caohy1988's behalf. This is a COMMENT review; the verdict below concerns readiness, not a GitHub approval event.

Rejection criteria, recorded before reading the diff

1. Formatter diagnostics expose payload-derived text in BigQuery or default logs, or overwrite an existing event error.
2. Diagnostic introspection or debug logging lets a caught formatter failure escape, drop the row, or change the sentinel/counter behavior.
3. Claimed safety guards lack effective tests, or consumer error classification creates an unresolved conflict with the stated intent.

P0: None found.

P1 — Debug rendering can turn an already-caught formatter exception into an agent/process exit.

Location: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:1115-1116, called at :7388-7391; the test at tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py:4464-4488 currently enforces this escape.

With debug_content_formatter_errors=True, a formatter can raise an ordinary ValueError subclass whose __getattribute__ raises SystemExit only when rendering reads __traceback__ (also reproducible with __class__). The formatter's original exception is successfully caught, but the rendering hook's exception is re-raised. Through the public on_user_message_callback, I observed 0 rows, formatter_failed=0, and SystemExit escaping. KeyboardInterrupt gives the same result. Debug off writes the sentinel and increments the counter. No OS signal was sent in these probes.

Minimal trigger:

class RenderExit(ValueError):
    def __getattribute__(self, name):
        if name == "__traceback__":
            raise SystemExit("raised by rendering hook")
        return super().__getattribute__(name)

def formatter(content, event_type):
    raise RenderExit()

The policy is explicit, but the possibility of a genuine signal does not establish that an exception raised by this hook is genuine, or satisfy the round-2 requirement that optional traceback rendering never abort/drop the row. The public config docstring's assertion that the row is unaffected is also false for this case.

Suggested fix: contain exceptions from the optional diagnostic operation, including these hook-originated BaseExceptions, or render from a safe snapshot that does not execute the hostile hooks. Preserve the existing propagation of exceptions/cancellation from the formatter invocation itself. Change the synthetic-hook tests to require one sentinel row and one counter increment; test genuine interruption policy separately.

P2 — Warning emission remains able to drop the row and misidentify the formatter failure.

Location: src/google/adk/plugins/bigquery_agent_analytics_plugin.py:7403-7408, and the unsupported-result warning at :7366-7372. Test coverage at tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py:4496-4536 exercises a closed StreamHandler, which handles its ordinary write exception internally; it does not test a handler whose emit raises out of logging.

I attached a logging.Handler whose emit raises RuntimeError only for a multiline WARNING. For the same formatter raising ImportError, debug off writes 1 row; debug on writes 0 rows, with formatter_failed=1. _safe_callback catches the logger failure and logs “skipping”; it cannot recover the lost row. This directly disproves the new option's best-effort guarantee. Handlers raising CancelledError or SystemExit instead escape the public callback. The generic unguarded-handler weakness also exists on base; the new multiline debug record introduces a new way to trigger it.

There is a related correctness error on the return path: a formatter returning a tuple plus a handler that fails only on the unsupported-result warning produces content_formatter raised RuntimeError, instead of content_formatter returned unsupported type tuple. The formatter did not raise; the handler did.

Suggested fix: establish the sentinel, counter, and class diagnostic before warning emission on both paths, and contain logging failures in a narrowly scoped best-effort helper, without trying to report the failure through the same broken logger. Add tests for handlers/filters that propagate exceptions, including a handler accepting the default record but rejecting the debug record, and assert the original unsupported-type diagnostic is preserved.

What I tried and verified

• Read issue google#485 item A, the PR description, both implementation commit messages, the complete two-file base-to-head diff, the surrounding callbacks/parser, and the existing payload-name regression test. No branch changes, pushes, or merges.
• Created this worktree's own uv/Python 3.11.13 environment. The full extras install was blocked by Intel macOS wheel availability (google-antigravity, onnxruntime, lancedb) and the locked cryptography==50.0.1 source build. Used the locked plugin/dev dependencies plus A2A/MCP and pinned pytest tools, with an available cryptography==48.0.1 wheel. This is an explicit environment deviation, not a clean all-extras setup.
• python -m pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q: 587 passed, no skips, 8.17s.
• Pinned pyink 25.12.0 and isort 8.0.1 checks passed on both files. Ruff passed; repository-configured strict mypy reported no issues for the plugin.
• Independent public-callback/real-parser probes captured rows at the batch append boundary: 78 cases, 58 passed and 20 failed. They covered separately armed metaclass __getattribute__/__eq__/__hash__ raising RuntimeError, CancelledError, or SystemExit on both paths; payload-named registered classes; exception __str__, __repr__, __class__, and traceback hooks; tuple/generator/coroutine/async-generator/class/memoryview/custom-object returns; re-entrant class diagnosis from exception rendering; repeated calls; and failing handlers. The failures are the diagnostic failures above plus baseline edge cases described below, not 20 separate findings.
• No payload-derived name reached the new row diagnostic in the successful adversarial class-label cases. Default logs remained constant in those cases. The repository tests verified original-error precedence, empty-message handling, debug traceback isolation from the row, all five trusted labels, and renaming of an allowlisted class.
• Loaded alternate plugin source in memory, leaving tracked files unchanged: the new 35-test class produced 33 failures / 2 passes on base, and 18 failures / 17 passes on 48dd52cf, reproducing the PR's prior-head claim.
• Independently ran 26 source mutations against those 35 tests: 25 were rejected. These included descriptor bypasses, equality/hash hooks, runtime labels, every allowlist entry, rendering containment, live exc_info, warning placement, payload messages, precedence, and the return diagnostic. The sole survivor was ordinary name access after static-type validation, matching the described equivalent mutation. Thus the implemented guards have useful tests; the two findings concern a missing logging guard and a test that deliberately pins behavior contrary to the requested rendering contract.
• Baseline comparisons also reproduced an unsupported object's __class__ raising CancelledError during the existing isinstance gate, and an ambient exception leaking through a closed stock handler's handleError on stderr. These predate this PR; I am not presenting them as new regressions. The review does not claim the plugin is a sandbox against arbitrary formatter side effects.

Design judgments and limits

The trusted-label trade-off is sound for the tested CPython threat model: direct descriptors and identity checks avoid Python metaclass hooks; fixed strings for heap classes avoid module-registration/renaming attacks. Losing exact names for Python-defined library exceptions is preferable to trusting payload-chosen names.

I judge the consumer error-predicate effect acceptable for this intent, not an escalation or an additional finding. A formatter failure is an actual analytics/redaction failure, and the explicit contract puts its diagnostic in error_message. I verified a USER_MESSAGE_RECEIVED, status='OK' sentinel row satisfies the supplied predicate through its non-NULL diagnostic. A formatter failing on every event will therefore inflate error counts; distinguishing analytics quality from agent execution errors remains a worthwhile SDK follow-up, not a reason to silently change this PR's requested storage contract.

I did not run against live BigQuery or independently reproduce the historical whole-repository suite. Probe sources, logs, mutation results, and structured row observations are in /tmp/adk-pr14-dual/r2/astra-probes/. No tracked file was modified.

Blindness disclosure: I opened no reviewer review, PR discussion/review comments, peer result files, restricted vault files, or implementer worktree. The explicitly allowed PR description itself contains implementer summaries of previous review scenarios; I encountered those summaries and independently tested the claims rather than treating them as review evidence.

VERDICT: REQUEST_CHANGES @ 3d94259

[caohy1988]

Fable (Claude) blind review, round 2 @ 3d94259a

Blind: I read only the PR description, the diff/commits, issue google#485 (item A), the repo code, and the SDK's error-predicate source. I read no other review or PR comment. One disclosure: my local agent memory held a technique note from the implementer's round-2 session (tripwire tests; reading classes through type's descriptors). It contained no review findings.

Line numbers refer to src/google/adk/plugins/bigquery_agent_analytics_plugin.py at head unless noted.

Rejection criteria (written before I read the diff)

• RC1 (leak): payload-derived text reaches a row's error_message or the default (non-debug) local log. That covers str(e), args, the traceback, and payload-derived class names, including classes built with type() and bound into a module.
• RC2 (drop, raise, or drift): any formatter-failure path drops the row, raises out of the fail-closed block, or changes the sentinel, the formatter_failed counter, or the constant warning. That includes hostile exception or metaclass constructs, debug rendering, and logging failures.
• RC3 (precedence, opt-in, pinning): an existing error_message is lost; precedence is undocumented or untested; the debug flag is not off by default or reaches BigQuery; or a claimed guard can be removed without a test failing.

Result: one metaclass construct meets RC2, and RC1 follows from it (P1 below). Everything else I tried held.

P1 — classification can raise: a hostile class drops the row, leaks the payload into the default log, and can abort the agent run

_formatter_failure_message (:1084) and _trusted_class_label (:1059) read type.__dict__["__mro__"] and type.__dict__["__flags__"] through .__get__(cls). Those descriptors run no Python hooks. But CPython's descr_check first checks PyType_IsSubtype(type(cls), type), and that check walks the metaclass's own __mro__. A meta-metaclass mro() can drop type from that MRO after the class already exists.

except Exception still catches the instance, because the class's own MRO is intact. Classification then raises TypeError inside the handler at :7387, which is the one step with no handler of its own. Commit 2b9fbd0 says "Classification now cannot raise at all, so it needs no BaseException handler"; this construct disproves that.

state = {"broken": False}

class MetaMeta(type):
  def mro(cls):
    return [cls, object] if state["broken"] else super().mro()

def formatter(content, event_type):
  M = MetaMeta("M", (type,), {})
  err = M("AnyName", (ValueError,), {})(f"cannot redact {content}")
  state["broken"] = True
  M.__bases__ = (type,)   # re-runs MetaMeta.mro(M) -> (M, object)
  raise err               # still caught by `except Exception`
# -> TypeError: descriptor '__mro__' for 'type' objects doesn't apply to a 'M' object

The descriptor behavior reproduces on CPython 3.10.16, 3.11.13, 3.12.9, 3.13.7 and 3.14.0a6.

I also ran it end to end through on_user_message_callback, with a stock StreamHandler, logging.raiseExceptions=True (Python's default), debug off, and SECRET = "TOPSECRET-4111-1111-1111-1111". The probe had two variants: in the "rendering" variant the metaclass keeps __qualname__ readable; in the "plain" variant it doesn't.

	base def458b6	head 3d94259a	head + fix below
Row written	yes, with [FORMATTER_FAILED]	no	yes
formatter_failed	1	0	1
error_message	NULL	(no row)	content_formatter raised <unknown class>
Default log ("rendering" variant)	constant warning	_safe_callback (:566) logs an ERROR whose traceback prints the formatter exception as __context__: …TOPSECRET_4111_1111_1111_1111: cannot redact parts=[Part(text='TOPSECRET-4111-1111-1111-1111')] role=None. When the metaclass is payload-named, the TypeError text itself says doesn't apply to a 'TOPSECRET_4111_1111_1111_1111_Meta' object.	constant warning
PluginManager.run_on_user_message_callback ("plain" variant)	returns	raises AttributeError: handleError re-renders the chain, fails again, and the error escapes _safe_callback. PluginManager (plugin_manager.py:316-322) doesn't swallow plugin errors, and here its own logger.error(exc_info=True) hits the same failure. Runner awaits the call with no handler, so the agent run aborts.	returns

The return path (:7371) runs inside the try, so the same TypeError is caught and the row survives. But it is mislabeled content_formatter raised TypeError, and two warnings are logged.

This needs deliberately hostile code, so the practical risk is low. It is still the same threat class as the PR's hostile-metaclass-hook tests. It breaks the contract ("the helper can never raise out of the fail-closed block or drop the row"), and base handles it correctly.

Suggested fix (verified): make the helper total.

  outcome = "raised" if raised else "returned unsupported type"
  try:
    for depth, ancestor in enumerate(_TYPE_MRO.__get__(cls)):
      ...  # unchanged
  except Exception:  # descr_check walks the metaclass MRO, which the class controls
    pass
  return f"content_formatter {outcome} <unknown class>"

Also update the "Unreachable" comment at :1090, and the commit and PR text that says classification cannot raise. KeyboardInterrupt and SystemExit still propagate, which matches your stated policy.

With this change applied to a /tmp copy of head:

• The PR's test file passes 587/587.
• My 8 variants (plain or rendering metaclass × M or payload-named metaclass × debug off or on) each write the row with counter 1 and content_formatter raised <unknown class>. With debug off, nothing payload-derived is logged.
• The PluginManager probe returns normally.
• The return path reads content_formatter returned unsupported type <unknown class>.

Please add a regression test on both paths. Restore the metaclass MRO in finally before asserting: pytest's reporter reads the chained exception's __qualname__, the same trap your tripwire avoids.

P2 (non-blocking)

1. A raising log handler still drops the row, and the counter now over-reports.
   • A handler whose emit() raises instead of calling handleError makes the post-except warning (:7392 onward) raise out of _log_event in both modes. Base behaves the same way. Such handlers break the logging contract, but custom log shippers do this.
   • The regression: head has already run _count_local_drop("formatter_failed") (:7391), so it reports formatter_failed=1 for a row that was never written. The docstring at :5603 says such a row "WAS written", and base didn't count it.
   • Fix: those warnings now run outside the except, so a try/except Exception: pass around them can't reach the formatter's exception. It would also make "a failing logging handler can never drop the row" true beyond stock handlers. Today's test only covers a stock StreamHandler on a closed stream.
2. Open design question (error_message on status='OK' rows): acceptable; no escalation needed.
   • The issue's own proposal (item A.1) writes event_data.error_message.
   • The column already documents non-NULL values on OK LLM_RESPONSE rows.
   • A formatter failure is real content loss, and flagging it where SDK users already look for errors is reasonable.
   • I checked the SDK's event_semantics.ERROR_SQL_PREDICATE and udf_kernels.is_error_event; both match the PR body. NO_ERROR_SQL_PREDICATE flips too.
   • Handle it as the proposed SDK follow-up. The note is appended after event-controlled text, so a text predicate like (^|; )content_formatter … can be spoofed by a model or tool error_message. A structured marker (for example an attributes key) is sturdier if the SDK needs to separate these rows.
3. Nits (cosmetic; no leak).
   • ExceptionGroup is a heap type in CPython, so it renders as <subclass of BaseExceptionGroup>.
   • A result whose __class__ property claims str fails str.__str__ normalization inside the try, and the row reads content_formatter raised TypeError even though the formatter returned normally.

Judgments the contract asked for

• Trusted-set trade-off: sound. A static type's name is compiled in and immutable. A heap class's name can come from type(name, …), a factory's module binding, or a runtime rename. Matching a fixed allowlist by identity and falling back to the nearest trusted ancestor is the right rule. The cost is fewer exact names (for example, JSONDecodeError and pydantic ValidationError read <subclass of ValueError>). The motivating ImportError and ModuleNotFoundError stay exact, and the debug flag shows exact classes locally.
• BaseException policy: justified. Rendering contains everything except KeyboardInterrupt and SystemExit, which a real signal can deliver at any point. Rendering never awaits, so any CancelledError there must come from the exception being rendered. The policy is documented and pinned by tests.

What I tried that held at head (probes in /tmp, none in the repo)

• Unusual formatter results. Every case wrote the row with the right sentinel and counter, with no payload in the row or the logs:
  • bytes, bytearray, memoryview, set, int, float, bool;
  • a module, a function, a class object (type), NotImplemented, Ellipsis;
  • a payload tuple and a payload-named exception instance;
  • async (coroutine) and generator formatters;
  • payload-named str and dict subclasses, accepted as on base.
• Hostile instances. __getattribute__ raising CancelledError on every attribute, with __str__ and __repr__ raising SystemExit: row written, labeled <subclass of ValueError>.
• Re-entrancy. A formatter that schedules a nested event on the same plugin: both rows labeled correctly, counter 2.
• Default-log parity. Base and head log identical records on the raise and return paths, with no exc_info.
• Red/green. The new tests fail 33/35 on base and 18/35 on 48dd52cf, as claimed. All 552 existing tests pass on base; 587 pass at head.
• Mutations. The new tests kill all 24 single mutations I applied:
  • descriptor reads replaced by metaclass reads;
  • identity replaced by equality or in;
  • labels taken from the class's current name;
  • static-type naming removed, and each of the 5 allowlist entries dropped;
  • render containment narrowed, removed, or swallowing interrupts;
  • always rendering; the debug flag defaulting to True; the warning moved inside except;
  • str(e), and the naive type(e).__name__;
  • replace, prepend, or keep-empty precedence;
  • no return-path note; no counter increment.
• Tooling. pyink 25.12 and isort 8.0.1 --check are clean on both files; ruff is clean; mypy (the repo's strict config) is clean.
• Schema. _schema_fields_match compares only name, type and mode, so the description change can't churn existing tables.

Not verified: the full tests/unittests suite (I ran the plugin test file and my probes only), a live BigQuery write, and the plugin test file on Pythons other than 3.11. The P1 root cause itself was checked on 3.10 through 3.14.

VERDICT: REQUEST_CHANGES @ 3d94259

[caohy1988]

Blind Astra review, round 3, of head 974e4642d6ac38d3b31067cb10ab83e9367e8e4e against def458b609c2811d137b0332b2fc7b201dddd5c0. I read no other review or PR review comment.

Rejection criteria, recorded before reading the diff:

1. Formatter-controlled payload text reaches the row or default diagnostic output, including through dynamically chosen names.
2. Failure handling or diagnosis can escape, lose the row, change the sentinel/count, or discard the event's existing diagnostic. Genuine interrupts from the formatter call remain a separate, explicit policy.
3. Claimed guards lack tests that detect their removal, or downstream error classification conflicts with the requested intent.

P1 findings — acceptance-contract failures. These reproduce on the pinned base too; I am not presenting them as newly introduced regressions. They nevertheless prevent the requested blanket fail-closed/never-drop acceptance. The new diagnosis helper survives the direct exception-injection tests; the remaining gaps are in its execution context and the surrounding result handling.

1. The warning can still print the formatter exception through an active caller exception. Plugin lines 1177–1186, and the claimed isolation at lines 7457–7459.

   Attach a stock logging.StreamHandler whose StringIO stream is closed, with Python's default logging.raiseExceptions=True. Call the public callback inside an except block, and let the formatter rethrow that active exception:

   failure = ImportError("ASTRA_SECRET_PAYLOAD_92014")
   def formatter(content, event_type):
       raise failure
   try:
       raise failure
   except ImportError:
       await plugin.on_user_message_callback(...)

   With debug off, stderr contains ImportError: ASTRA_SECRET_PAYLOAD_92014, followed by the closed-stream exception. The row and count survive, but default diagnostic output leaks. Leaving the formatter's inner except does not clear the caller's active exception. StreamHandler.handleError() prints internally, so the outer catch cannot undo it. A distinct payload-bearing caller exception also leaks.

   Fix: emit the best-effort warning in an execution context with no inherited handled exception, retaining containment around emission; add regressions for both caller-context cases. Do not rely on changing the process-global logging.raiseExceptions setting. Probe: test_closed_handler_with_formatter_rethrowing_active_exception.

2. Invalid return-object introspection still escapes before the new boundary. Plugin line 7432.

   class Odd:
       @property
       def __class__(self):
           raise asyncio.CancelledError("synthetic payload")
   # formatter returns Odd(); the formatter call itself does not raise

   isinstance(formatted, (dict, list)) invokes that property. CancelledError, SystemExit, and KeyboardInterrupt each escape the public callback; each probe records 0 writes, formatter_failed=0. With RuntimeError, a row survives but the new note incorrectly says content_formatter raised RuntimeError instead of identifying the unsupported result.

   Fix: use the actual type for this check, e.g. issubclass(type(formatted), (dict, list)), as the PR already does for strings. This preserves genuine subclasses without running the object's __class__ hook. Keep genuine formatter-call interrupts propagating. Add these cases to the result-validation tests. Probes: test_returned_object_class_access, test_return_introspection_drop_receipt.

3. Discarding a coroutine result can publish a payload-derived name before diagnosis. Plugin lines 7439–7440.

   Return a fresh native coroutine whose __qualname__ is set from the protected content. Replacing formatted with the sentinel releases it and emits RuntimeWarning: coroutine 'ASTRA_SECRET_PAYLOAD_92014' was never awaited. The row safely says ... unsupported type coroutine and the counter is one, but the default warning sink leaks the name with debug off. This requires no custom finalizer or logging handler; the probe captures Python's warning machinery.

   Fix: close rejected native coroutines through a trusted operation under fail-closed containment before releasing them, and test warnings/stderr as well as caplog. Add an actual async formatter case and a payload-named coroutine case. Probe: test_payload_named_coroutine_return.

P0: none found. P2: no separate finding; the ordinary-exception misclassification is included in finding 2.

What I verified:

• Read issue google#485 item A, the PR description, all three implementation commit messages, and the full two-file diff.
• Own Python 3.11.13/uv environment: python -m pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q → 627 passed. Pinned pyink 25.12.0 and isort 8.0.1 checks pass on both files.
• 46 independent adversarial cases: 36 passed, 10 failed, with the failures covering the three findings (including multiple exception types and caller-context variants). Passing attempts cover payload-named exceptions with hostile __str__, __repr__, __class__, and traceback access; ordinary async formatters; re-entrant diagnosis; actual SIGINT delivery; and real task cancellation. Existing tests additionally cover module-bound payload-named classes, forged flags, hostile metaclass hooks, broken meta-MROs, precedence, handlers, and filters through the Arrow write path.
• 15 guard mutations, all detected: outer boundary removal/narrowing, inner fallback removal/narrowing, restored interrupt rethrow, ordinary flags/MRO access, equality matching, dynamic trusted labels, count/precedence removal, fake-string handling, result-class equality, and live-exception logging.
• In fresh processes, without changing tracked files: new diagnostic tests against the pinned base → 73 failed, 2 passed; against 3d94259a → 40 failed, 35 passed, confirming the PR's latter claim. Separate base probes confirm the findings above predate this PR.

Design judgments: fixed identity labels plus static-type names are a sound trade-off for the tested Python runtime; losing exact names of custom/library classes is preferable to trusting module registration. Containing BaseException during synchronous best-effort diagnosis is consistent with the stated priority: my real SIGINT inside diagnosis was absorbed, another outside propagated, and Task.cancel() still raised at the next await. The loss of a genuine interrupt in that window must remain documented.

The SDK predicate counting these rows as errors is acceptable for this requested change: a formatter failure is an operational failure, and populating error_message is explicit in item A. It does broaden agent-error metrics; retain the documented downstream follow-up for a structured formatter-failure marker. I do not infer an agent-only metric requirement or escalate this design choice.

Limitations: no live BigQuery or full-suite rerun. Frozen all-extras setup hit unavailable Intel macOS wheels for Antigravity, ONNX Runtime, and LanceDB; those packages were omitted. Locked cryptography 50.0.1 failed to build, so this environment uses available-wheel 48.0.1. Repository and lockfile remain unchanged. Evidence and reproducible probes: /tmp/adk-pr14-dual/r3/astra-probes/ (adversarial-final.log, mutation-results.json, base/previous-head logs, and drop receipts).

VERDICT: REQUEST_CHANGES @ 974e464

[caohy1988]

Fable (Claude) — blind disproof review, round 3 @ 974e464

Blind review on caohy1988's behalf. Sources: the PR description, git diff def458b6..974e4642, issue GoogleCloudPlatform/BigQuery-Agent-Analytics-SDK#485 (item A), and the repository code. I read no other review or comment on this PR. All probes ran outside the repo (/tmp/adk-pr14-dual/r3/fable-probes/), and the worktree is unmodified.

Rejection criteria (written before reading the diff)

• R1, leak. Payload-derived text reaches any BigQuery column or the default (flag off) Python log. That covers str(e), args, the traceback, or a payload-derived class name, including classes made with type() and bound into a module, and metaclasses that lie about __name__/__qualname__/__module__/flags.
• R2, drop or alter. On either failure path, anything raised while diagnosing drops the row, escapes the callback, or changes the sentinel, the formatter_failed count, or the constant warning. "Anything" means any type, including BaseException raised by metaclass or exception hooks, traceback rendering, or a failing handler or filter. R2 also covers losing a pre-existing error_message, or leaving the precedence undocumented or untested.
• R3, evidence. The new tests pass on the unfixed code, removing a guard fails no test, the default logging changes with the flag off, or a PR or commit claim does not reproduce.

Judgement items: the trusted-set trade-off, the BaseException policy for genuine interrupts, and error_message on status='OK' rows versus the SDK error predicate.

What I ran

• Environment. The worktree .venv, CPython 3.11.13 (x86_64), with pyink 25.12.0 and isort 8.0.1, matching the pre-commit pins.

• Test file. pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q gives 627 passed. I ran it 19 times: 13 from the worktree and 6 from an identical /tmp copy, 3 of them concurrently under load. There were 0 failures.

• Red/green. I ran the 75 new tests against other plugin copies through PYTHONPATH, leaving the worktree untouched.

  • Base def458b6: 73 fail, 2 pass. The two passes pin behavior that was already correct: exception text never reaches the row, and there is no traceback by default.
  • Previous head 3d94259a: 40 fail, 35 pass, exactly as the PR body says.

• Mutation spot-check. 17 mutants on a /tmp copy; 16 are killed. Failing-test counts are in parentheses.

  • Diagnosis boundary narrowed to Exception (28).
  • Diagnosis boundary removed (35).
  • Render fallback narrowed to Exception (4).
  • Label fallback removed (2).
  • MRO read as cls.__mro__, through the metaclass (6).
  • Flags read as cls.__flags__, through the metaclass (7).
  • Allowlist matched with == (6).
  • Allowlist label read from the class (1).
  • Heap-type names trusted (26).
  • isinstance(formatted, str) restored (1).
  • if error_message is not None (1).
  • Precedence swapped (1).
  • Live exc_info handed to handlers (11).
  • Diagnosis run inside an except context (1).
  • BaseModel dropped from the allowlist (1).
  • No note on the return path (70).

  The one survivor moves the drop count after diagnosis. That order can't be observed while the boundary holds, so the survivor is fine.

• Lint and types. pyink --check, isort --check-only, ruff check, and codespell are clean on both files. mypy with the repo's strict config reports no issues for the plugin module.

• Cross-version. Running the labeling helpers standalone gives identical labels on 3.11.13 and 3.13.7.

• 42 adversarial probes. The user message carries a payload SECRET, and the payload-named classes take their names from it.

  • No leak in 24 cases (12 formatters, each with debug off and on):

    • a namedtuple, an enum.Enum, and a pydantic.create_model class named after the payload;
    • a payload-named class bound into builtins, into a fresh module in sys.modules, and into google.api_core.exceptions;
    • a metaclass __getattribute__ that lies about __name__/__qualname__/__module__;
    • the payload in __notes__ and in a chained __cause__;
    • __str__ and __repr__ that raise;
    • an exception whose instance __class__ property raises SystemExit;
    • a payload-named int subclass as the result;
    • the issue's own case, import missing_module, which gives content_formatter raised ModuleNotFoundError.

    Every case wrote the sentinel row with formatter_failed=1 and the expected label. The payload never appeared in the row or in the default log. With debug on it appears only in the local warning, which is by design.

  • The row survived each of these:

    • debug on with a 3000-deep __cause__ chain;
    • a handler whose emit logs again to the same logger;
    • a closed-stream handler;
    • an async def formatter. The row is labeled returned unsupported type coroutine, and the only warning is "coroutine '…' was never awaited", which carries no payload.

  • Failures: the classification-stage hooks (P2-1) and the genuine-signal cases (P2-2), both below.

Findings

P0: none. P1: none.

P2-1: the result check still runs the returned object's own hooks (src/google/adk/plugins/bigquery_agent_analytics_plugin.py:7432)

This round made two checks in this elif hook-free: the exact-model check now compares identity (:7428-7431), and the str check uses issubclass(type(...), str) (:7411). isinstance(formatted, (dict, list)) does not. For any object that isn't a dict or list, CPython's object_isinstance falls back to formatted.__class__. That runs the returned object's __getattribute__ or __class__ property inside the fail-closed try.

What my probes saw, with both a property and a __getattribute__ variant of each hook:

Hook on the returned object	base	head
__class__ raises SystemExit / KeyboardInterrupt / CancelledError	escapes the callback, no row, not counted	same (pre-existing)
__class__ raises RuntimeError	row written, error_message=None	row written with content_formatter raised RuntimeError, although the formatter returned
__class__ returns dict	parser writes [UNSUPPORTED_OBJECT], not counted	same, and no error_message

This is not a regression, and no contract bullet names it: diagnosis itself is hook-free. It is the same kind of hazard the PR fixes two lines up, though, and the misreported "raised" is new with this feature.

Suggested fix, one line:

            or issubclass(type(formatted), (dict, list))

I checked it on a /tmp copy. All 9 cases then become sentinel rows with returned unsupported type <subclass of object> and formatter_failed=1. With the fix, the full plugin test file passed 627/627 in 14 of 16 runs. The other 2 runs each had exactly one failure. The one I captured is TestExactlyOnceDelivery::test_finalization_respects_remaining_close_budget[shutdown], an elapsed < 0.2 timing assert on a BatchProcessor that never reaches _log_event. The other run's failing test was not captured.

One side effect: objects that only claim to be a dict or list through __class__, such as mocks and proxies, become counted formatter failures instead of [UNSUPPORTED_OBJECT]. That is arguably more accurate. Please also add a tripwire test for instance-level hooks on a returned object.

P2-2: the genuine-interrupt policy is acceptable, but its justification overclaims (:1139-1146, boundary at :1182)

The probe sends a real os.kill from a timer thread while a synchronous handler's emit() blocks for 0.5 s on the formatter warning. I ran it with two handlers: a SIGTERM handler that calls sys.exit(143), and a SIGINT handler that raises KeyboardInterrupt.

• Head: the signal is delivered, but the exit is silently absorbed. The callback returns normally and the row is written.
• Base: the exit propagates, and the row is lost.

The containment itself satisfies the round-3 contract, and common asyncio setups never hit this. On 3.11+, asyncio.run turns the first SIGINT into a task cancel, and loop.add_signal_handler doesn't raise inside the frame. Two phrases in the justification still overclaim:

• "Short window": the window includes every configured handler's emit, and a network log shipper can block.
• "The next is delivered normally": an orchestrator usually follows SIGTERM only with SIGKILL, and SIGKILL then loses every buffered row.

Two options:

• (a) Keep the row and the interrupt. Catch (KeyboardInterrupt, SystemExit) separately; the except match runs no hooks. Remember a fresh, payload-free KeyboardInterrupt(), or a SystemExit that keeps code only when it is an int or None, and falls back to 1 otherwise. Read code through SystemExit.__dict__["code"]. Re-raise it in _log_event after await state.batch_processor.append(row) (:7614).
• (b) At minimum, reword the docstring and the PR text to say that such a signal is lost.

Not blocking.

P2-3: error_message on status='OK' rows: acceptable, not ESCALATE

Probe: a USER_MESSAGE_RECEIVED row with status='OK' and error_message='content_formatter raised ImportError' satisfies ENDS_WITH(event_type,'_ERROR') OR error_message IS NOT NULL OR status='ERROR'.

I judge this acceptable:

• Issue item A itself proposes error_message = "content_formatter raised …", so this is not an intent conflict.
• A formatter failure destroys the row's content. That is a real data-quality error, and it should be loud.
• LLM_RESPONSE rows already carry error_message with status='OK'.

The PR documents the downstream effect. An optional follow-up, cheap enough to do here: write a structured marker, such as an attributes key, next to the note. The SDK could then separate formatter failures from agent errors without matching on text, which the PR itself notes is spoofable.

P2-4: documentation nits

• The content_formatter docstring (:2276) says that if the formatter "raises", the row is written with [FORMATTER_FAILED]. But a KeyboardInterrupt, SystemExit, or CancelledError raised by the formatter propagates and no row is written, as the PR's own test_interrupts_raised_by_the_formatter_call_still_propagate pins. Suggest "raises an Exception".
• :2358 is 84 columns, over the 80-column limit. pyink doesn't reflow docstrings, so the check doesn't catch it.
• With debug on and a failing handler, logging.Handler.handleError echoes the record's args to stderr, and those args are the rendered traceback with the payload. That fits the docstring's "reaches the console" caveat; consider naming it explicitly.

Judgement: the trusted-set trade-off is sound

• Static C types have compile-time names, and CPython refuses to set __name__ on a non-heap type.
• The five allowlisted classes are matched by identity and carry fixed labels. Renaming them, look-alike classes, and type() clones can't spoof them; the tests and my probes confirm this.
• Classes are read through type's own descriptors, so no metaclass hook runs, and <unknown class> is the fallback.

The cost is coarser labels for Python-defined and heap C-extension exceptions: JSONDecodeError, pydantic ValidationError, and the sqlite3/ssl/zlib/decimal errors, on both 3.11 and 3.13. That is acceptable because the issue's own case (ImportError/ModuleNotFoundError) keeps its exact name and the debug flag shows the exact class locally. The identity allowlist can grow later if needed.

VERDICT: APPROVE @ 974e464

[caohy1988]

Astra blind review of 7fae96552a4f032e7d5ca0e25c3ee9a0e315ea7e, against the requested base def458b609c2811d137b0332b2fc7b201dddd5c0. I read the PR description, full supplied-base diff, repository code, and issue google#485 item A. I did not read any other review or any prohibited review/vault/worktree files.

Rejection criteria — recorded before reading the diff

1. Payload-derived text reaches BigQuery diagnostics or default logs, including through class names or failing handlers.
2. Formatter-result validation or diagnostics drop a row, change the sentinel/counter, or lose an existing event error.
3. Claimed guards lack effective tests, or the new error classification creates an unresolved conflict with the stated intent.

The core diagnostics work, but I cannot affirm the universal Round-4 never-drop/never-leak claim. The counterexamples below are residual paths also present on the supplied base, not newly introduced vulnerabilities. My requested changes follow the explicit acceptance criteria; the severity/classification below distinguishes production-relevant cleanup from deliberately hostile objects so the owner can decide whether to accept narrower scope.

What I verified

• Fresh worktree-local uv/Python 3.11.13 environment. Final requested command: python -m pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q -rs: 651 passed, no skips. This includes all 99 new diagnostic cases. Initial six optional-dependency skips were resolved by installing the locked A2A/MCP extras.
• Pinned pyink 25.12 --check and isort 8.0.1 --check-only: both changed files pass.
• 46 independent passing probes through the public user-message callback and real Arrow serialization, with the write client mocked: payload-named/module-bound exception and result classes; raising metaclass attribute/equality/hash hooks; raising instance __class__, __str__, and __repr__; dict/list/str class spoofing; debug traceback hooks raising five Exception/BaseException variants; async formatters; named coroutines and unstarted/started generators; closed and broken-pipe StreamHandlers while a caller exception is active, including re-raising it; re-entrant warning handling; and a real SIGINT delivered during warning emission.
• 12 independent guard mutations were all detected by the added tests: narrowing the outer boundary; removing warning isolation or from None; omitting coroutine/generator closure; restoring isinstance admission; dynamic allowlist labels; ordinary MRO/flags lookup; equality-based allowlist matching; narrowing rendering containment; and removing the label fallback. Removing isolation while retaining warning emission specifically fails both active-caller tests.
• Loaded the complete base plugin source into an isolated pytest process without editing tracked files: all 16 new class-label table cases fail on base. Separately compared the residuals below on head and base.
• The two failure paths produce the expected fixed labels, preserve the sentinel and one counter increment, preserve an existing event error first, and keep debug traceback text out of the row. The trusted-class trade-off is sound for the tested CPython/Python-class threat model: identity-bound fixed labels and static built-in ancestors avoid trusting dynamically registered names. Reduced specificity for library exceptions is documented and acceptable.

Artifacts and runnable probes: /tmp/adk-pr14-dual/r4/astra-probes/; full suite logs: /tmp/adk-pr14-dual/r4/astra-suite-final.log.

P0 / P1

None found. I did not demonstrate a newly introduced ordinary formatter failure that drops a row or leaks payload.

P2 findings

1. Rejected async-generator cleanup can still leak exception text to default logging. Classification (1): a payload-leak path requiring no hostile class/metaclass/name manipulation; pre-existing. At src/google/adk/plugins/bigquery_agent_analytics_plugin.py:1222–1225, cleanup covers only synchronous generators and coroutines. A started async generator is rejected, then released at :7508; its asynchronous finalizer can raise after the boundary, and asyncio logs the unconsumed exception. With debug disabled, my probe writes the sentinel row with content_formatter returned unsupported type async_generator and counter 1, then logs Task exception was never retrieved and ValueError: ASTRA_PRIVATE_PAYLOAD_782419 through the default asyncio logger. The async generator uses an ordinary fixed function name, not a payload-derived name.

   Minimal shape: create an async generator with try: yield ...; finally: raise ValueError(private_text), advance it once, keep it in a one-element list, and have the formatter return holder.pop(). Release/GC and an event-loop turn trigger the leak. A streaming resource cleanup exception can have this shape without malicious hooks; the test injects that failure and does not claim a particular SDK currently does so.

   Suggested fix: give rejected async generators an explicit cleanup path that consumes cleanup failures, with documented cancellation semantics and an end-to-end default-log test. If async-generator lifetime handling is intentionally excluded, obtain explicit acceptance of that exclusion and narrow the “everything after the call”/no-payload-in-logs contract accordingly. Evidence: test_astra_lifetimes.py::test_characterize_lifetime[async_generator], lifetime-head-async_generator.json; the corresponding base artifact reproduces the same leak.

2. Admitted dict/list subclasses can still run __class__ outside the new boundary and drop the row. Classification (2): deliberately hostile class edge, no realistic trigger demonstrated; pre-existing. The new gate at src/google/adk/plugins/bigquery_agent_analytics_plugin.py:1152 and early return at :1212–1215 admit these subclasses unchanged. The parser then executes isinstance(content, LlmRequest) at :4217, which consults their __class__. A property that raises CancelledError, SystemExit, or KeyboardInterrupt escapes both the parser's except Exception and _safe_callback; no row is submitted. All six dict/list × interrupt combinations reproduced. The new test at tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py:4790 tests plain objects that claim another class, not real subclasses accepted by this gate.

   Suggested fix: extend real-type dispatch/normalization through the accepted-container parser path, protecting its synchronous protocol accesses without swallowing ordinary async cancellation across parser awaits; add these six cases. Alternatively, explicitly narrow the claimed protection to rejected results, with owner acceptance. Evidence: test_astra_residuals.py::test_accepted_container_subclass_class_hook_must_not_drop_row, residuals.log, base-residuals.log.

3. Rejected-object lifetime warnings remain outside containment. Classification (2) for the demonstrated payload-naming/hostile-finalizer cases; pre-existing. At src/google/adk/plugins/bigquery_agent_analytics_plugin.py:1222–1225 / :7508, a tuple containing a payload-named coroutine produces coroutine 'ASTRA_PRIVATE_PAYLOAD_782419' was never awaited; a started generator that yields on GeneratorExit survives the first close and later prints a payload-bearing unraisable exception; an unsupported object's raising __del__ also prints its exception to stderr. In each probe the row itself remains a safe sentinel with counter 1. Direct, normally behaving coroutine/generator rejection passes.

   Suggested fix: add explicit lifetime regression cases; bounded traversal of exact built-in rejected containers can address nested coroutines. Do not claim that catching exceptions from .close() contains arbitrary finalization or __del__ output. Document and obtain acceptance of the remaining exclusions rather than advertising a universal guarantee. Evidence: the two remaining failing assertions in test_astra_residuals.py, plus test_astra_lifetimes.py and its head/base JSON artifacts, which capture actual default stderr/logging as well as warnings.

These findings are not an objection to the safer fixed-label diagnostics. They are concrete counterexamples to the review's stated acceptance envelope; the admitted-container case is also a missing case in the new result-hook tests.

Design judgments and limits

• Consumer SDK predicate: acceptable for item A, not an escalation. With the supplied predicate, a populated error_message necessarily counts a formatter failure as an error even when event status remains OK. Item A explicitly requests this column, and a redaction/telemetry failure is a diagnosable failure. Do not change status merely to make it agree. A structured formatter-failure marker is a useful SDK follow-up for separating telemetry failures from agent failures; text matching is not a trustworthy discriminator after event-controlled error text. I evaluated the supplied predicate, not a live downstream dashboard.
• Genuine interrupts: the BaseException policy is explicit and internally consistent with the specified diagnostic containment. My real SIGINT probe confirms it is absorbed during the warning, while the added tests confirm formatter-call interrupts propagate. This is a real operational trade-off, not just a hostile-class scenario: a Python signal handler that raises during blocking diagnostic I/O can lose its shutdown request. I accept it for this stated contract, with the documented buffered-row/shutdown risk; it should not be described as preserving all interrupts.
• Other plugin logging during an error callback with a failing handler: outside the new warning-isolation boundary and potentially a realistic pre-existing leak path, not evidence that this new isolated warning leaks. I did not independently validate every other plugin log site and make no plugin-wide safety claim.
• Full all-extras sync is not reproducible on this Intel Mac: locked google-antigravity, onnxruntime, and lancedb lack compatible wheels; cryptography 50.0.1's source build also failed. Relevant locked extras/test tools were installed, with cryptography 46.0.3 substituted. I did not rerun the full repository suite, the author's complete 33-mutation campaign, or live BigQuery writes. Those historical/full-environment claims are not independently certified here.
• Git HEAD is the requested SHA and tracked files are unchanged. The live PR base had advanced to e4c0d946f6d602b0ac23804328473b8d9e597b3e when checked; this review uses the explicitly requested def458b6 comparison.

VERDICT: REQUEST_CHANGES @ 7fae965

[caohy1988]

Fable (Claude) blind review: PR #14 round 4 at 7fae965

Blind review. Sources: the PR description, the diff def458b6..7fae9655, issue google#485 item A, and repo code. I read no other review or comment on this PR (see the note at the end). "plugin" below means src/google/adk/plugins/bigquery_agent_analytics_plugin.py at the head commit.

Rejection criteria (written before I read the diff)

• RC1, payload leak. Payload-derived text reaches the row (error_message, content, attributes) or the default-level logs or stderr on any formatter failure path. "Payload-derived" covers exception text, args and traceback; class names derived from the payload, including type()-made classes bound into modules; and the repr or qualname of a returned object. "Any path" includes hostile returned objects, payload-named coroutines and generators, and failing handlers that print the handled exception. With the debug flag on, the local logger may carry payload; nothing else may.

• RC2, row drop or contract drift. An exception of any type, including CancelledError, SystemExit, KeyboardInterrupt, GeneratorExit and RecursionError, raised while diagnosing an already-caught failure (naming the class, metaclass hooks, judging the result, closing, rendering, the logger or a handler), does any of these:

  • drops the row or escapes the callback;
  • changes the sentinel;
  • skips or double-counts formatter_failed;
  • changes the constant warning.

  RC2 is also met if default logging or rows for supported results differ from base.

• RC3, semantics, tests and claims. Any of these:

  • a pre-existing error_message is lost;
  • the precedence is undocumented or untested;
  • the note is not fixed-shape;
  • the new tests pass on the unfixed plugin;
  • removing a claimed never-raise or never-drop guard fails no test;
  • a PR or commit claim is contradicted by evidence I can reproduce.

What I ran

Setup: Python 3.11.13 x86_64 and pytest 9.1.1. Every probe lives outside the repo and ran against both head src and base def458b6 src, extracted with git archive.

• pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 gives 651 passed at head.

  • The 99 new tests against the base plugin: 97 failed, 2 passed. The two passes are the invariants "exception text never in the row" and "no traceback by default".
  • Against 974e4642: 24 failed, 75 passed, matching the PR body.
  • The 552 pre-existing tests pass on base.

• pyink --check (25.12) and isort --check-only (8.0.1), using the repo config on both files: clean.

• P1, names and class hooks (40 cases, debug on and off). Payload-named exception classes in these forms:

  • bound into builtins;
  • bound into google.api_core.exceptions as a NotFound subclass;
  • created by C PyErr_NewException;
  • inside an ExceptionGroup.

  Also a lying instance __class__ property; payload in __qualname__, __module__ and __doc__; a raising __str__ or __repr__; and a metaclass mro() that puts a static type first. Finally, every one of these metaclass hooks: __getattribute__, __eq__, __hash__, __instancecheck__, __subclasscheck__, __repr__ and mro. Each hook raised CancelledError, KeyboardInterrupt, SystemExit, GeneratorExit, RecursionError or RuntimeError, on both the raise path and the return path.

• P2, returned objects (45 cases):

  • admitted dict/list subclasses whose __class__ or __getattribute__ raises;
  • a hostile value nested in a plain dict;
  • payload-named dict, list and str subclasses;
  • a str subclass with every hook raising;
  • a started coroutine whose finally raises SystemExit;
  • a started generator whose finally raises a payload;
  • the running task's own coroutine;
  • a payload-named async generator;
  • a types.coroutine generator;
  • a payload-named coroutine nested in a tuple or list;
  • MagicMock(spec=dict) and a dict proxy with a lying __class__;
  • exact Content, Part and LlmRequest;
  • OrderedDict, None, bytes, a returned exception instance, a returned class;
  • an object with a raising __del__.

• P3, logging (62 cases, 12 skipped). Real handlers, run with -p no:logging, logging.raiseExceptions=True and stderr captured:

  • a stock StreamHandler on a closed stream;
  • a handler whose Formatter raises;
  • a handler that prints sys.exc_info();
  • a handler that walks __context__;
  • a handler that raises RecursionError;
  • a handler that calls sys.exit().

  Each ran with and without an active caller exception carrying a payload. The formatter raised, returned, or re-raised the caller's exception, with debug on and off.

  I also sent genuine signals with os.kill while a slow handler emitted the warning: a SIGINT (default_int_handler) and a SIGTERM whose handler calls sys.exit.

• P4, end to end. A real InMemoryRunner with MockModel and a raising FunctionTool, using the issue's formatter (a failing from json import does_not_exist), with a healthy handler and with a closed one. Precedence edges:

  • a long message (truncated);
  • a credential (redacted);
  • whitespace;
  • the literal "None";
  • an LLM_RESPONSE with status OK and an error_message;
  • payload_column_denylist=[content, content_parts].

• 26 mutations of the new helpers, run against the new tests. The two survivors were also run against the full file.

• CPython primitives on 3.10.16, 3.11.13, 3.12.9, 3.13.7 and 3.14.0a6:

  • the type descriptors and issubclass(t, (str, dict, list)) run no hooks;
  • CoroutineType.close suppresses the "never awaited" warning;
  • raise … from None keeps the caller's chain out of handleError;
  • single-argument format_exception works.

What holds (verified)

• The note is fixed-shape. error_message is always content_formatter raised or content_formatter returned unsupported type, followed by a static type name, a fixed allowlist label, <subclass of …> or <unknown class>.
• No payload leak by default. In every P1–P4 case with debug off, no payload text reached the row, the plugin's log lines, or stderr through the formatter path.
• Debug output stays local. With debug on, the traceback appeared only in the local log, never in the row.
• Diagnosis failures are contained. The PR's 45-case injection test passes, and my handler probes agree: every raise was contained, leaving one sentinel row and formatter_failed == 1. Head is better than base in three places:
  • Hostile metaclass hooks raising interrupts on the return path now keep the row; base dropped it.
  • RecursionError and sys.exit() handlers now keep the row; base dropped it.
  • A closed handler with an active caller exception no longer prints the caller's or the formatter's payload; base did.
• End to end, 10 of 10 rows are written. formatter_failed == 10, and the user payload never reaches rows, logs or stderr, with a healthy handler or a closed one. TOOL_ERROR, AGENT_ERROR, NODE_ERROR and INVOCATION_ERROR keep the tool text first, followed by ; content_formatter raised ImportError. Truncated, redacted and LLM_RESPONSE/OK precedence also hold. The tool text that does appear on stderr comes from ADK core's own "Node execution failed" and "Root node … failed" logs, identically on base, not from the plugin.
• Nothing else changed. The warning text, the sentinel and the counter are unchanged. Supported shapes produce identical rows on head and base: None, str/dict/list, a str subclass, exact Content/Part/LlmRequest, and OrderedDict. Objects whose __class__ claims to be a dict now get the sentinel, as intended.
• The trusted-label design is sound. Heap types are never named. Static names are fixed when CPython or the extension is compiled. Allowlist matching uses identity and fixed labels. Every naming trick above produced fixed text. The cost is that library exceptions lose their exact names. That is reasonable for a redaction boundary, and the debug flag recovers the names locally.

Findings

Each finding is classified as one of:

• (1) a data-loss or payload-leak path reachable without deliberately hostile code;
• (2) a hostile-class edge case with no realistic trigger.

I found no category (1) leak or data loss, and head is never worse than base in any probe. REQUEST_CHANGES is driven by RC3: two claims are contradicted by reproducible evidence. Both are cheap to fix.

P1-1 (RC3, category 2): a never-drop guard in the new helper has no test.

• Mutation: replace formatted = str.__str__(formatted) (plugin :1214) with pass.
• Result: all 651 tests still pass. Yet a str-subclass result whose __getattribute__ or __class__ raises CancelledError, SystemExit or KeyboardInterrupt now drops the row: the exception escapes from the parser. A RuntimeError there produces [CONTENT_PARSE_FAILED].
• Why it matters: this contradicts 7fae965 ("Removing any guard fails a test") and the contract ("removing a guard must fail a test").
• Fix: add a test whose formatter returns a str subclass with tripwired __class__, __getattribute__, __str__, __len__, __hash__ and __format__, armed after creation. Assert:
  • exactly one row, with content equal to the exact string;
  • formatter_failed == 0;
  • nothing escapes.
• My new-test runs killed 24 of 26 mutations. The other survivor is P2-4.

P1-2 (RC3, category 2): "nothing after the call can drop the row, … or escape the callback" is false for admitted results.

• Where the claim appears: the _settle_formatter_outcome docstring (:1168 "Everything after the call happens behind this one boundary", and :1177-1178) and PR body §2.
• Evidence: a formatter that returns a dict or list subclass whose __class__ property or __getattribute__ raises CancelledError, SystemExit or KeyboardInterrupt gives 0 rows and escapes the callback in all 12 such cases on head. Base does the same. A plain dict holding a hostile nested value does the same (3 of 3). A RuntimeError hook keeps the row: [CONTENT_PARSE_FAILED] on head, [FORMATTER_FAILED] on base.
• Mechanism: admission itself runs no hooks. But HybridContentParser.parse then calls isinstance(content, LlmRequest) (:4217), and pydantic's ABCMeta.__instancecheck__ reads instance.__class__. The parse boundary (:7570) and _safe_callback (:567) only catch Exception.
• The round-4 check fails for these objects. The round-4 check asked me to confirm that a result object with a raising __class__ never drops the row. That is true for rejected results and false for admitted container subclasses.
• Fix (either option works for this PR):
  • Scope the docstring and the PR text to failed or rejected results, and list "admitted str/dict/list subclasses and nested values go to the parser, whose boundary catches Exception only" among the residuals.
  • Or harden the parser: dispatch on type(content) in parse (:4217, :4263, :4272, :4276) and apply the same BaseException policy at :7570. That is follow-up-sized work.

P2-1 (category 2, pre-existing, every CPython from 3.10 to 3.14): a metaclass __subclasscheck__ on the raised class still aborts the run.

• Mechanism: when the formatter raises, CPython itself calls the metaclass __subclasscheck__(cls, cls) while raising and normalizing the exception. That happens before the plugin's except Exception at :7484.
• Evidence: a hook raising CancelledError, SystemExit, KeyboardInterrupt or GeneratorExit gives 0 rows and an escape (8 of 8 cases, head and base). A RuntimeError there only mislabels the note.
• Test gap: the PR's hook tests (test_naming_the_failure_runs_none_of_the_class_hooks, _metaclass_whose_hooks_raise at test :3877) cover __getattribute__, __eq__ and __hash__ only.
• Suggestion: document this next to "interrupts raised by the formatter call itself".

P2-2 (genuine-interrupt policy; reachable without hostile code; documented): acceptable but could be better.

• Verified: a real SIGINT, and a SIGTERM whose handler calls sys.exit, are absorbed on head when delivered while a slow handler emits the warning. The row is written and the process keeps running. On base both signals propagated. The docstring says this honestly.
• Why reconsider: P2-1 and P1-2 show that hostile content can still abort a run despite interrupts being contained in diagnosis. Meanwhile, containment does swallow real signals.
• Suggestion: record an absorbed KeyboardInterrupt or SystemExit and re-raise it at the end of _log_event, after batch_processor.append(row). The row is still never lost, and the signal is honored. CancelledError can stay absorbed, because it cannot be genuine in synchronous code.
• Not blocking.

P2-3 (the open design question about the SDK error predicate): acceptable, not ESCALATE.

• Why no escalation: issue google#485 item A itself asks for error_message, and status-OK rows with error_message already exist (LLM_RESPONSE termination details). There is no intent conflict.

• The PR's list of affected SDK code is incomplete. On the BigQuery-Agent-Analytics-SDK default branch today, the predicate lives in more than ERROR_SQL_PREDICATE and udf_kernels.is_error_event. It is copied inline for session-level has_error in:

  • src/bigquery_agent_analytics/system_evaluator.py (~L662-666);
  • insights.py (~L401-405);
  • trace.py (~L247-253, L737-743).

  So a single formatter failure marks the whole session has_error in evaluation and insights output. In my end-to-end run, 10 of 10 rows match the predicate, against 4 of 10 on base.

• Follow-up: the SDK fix must patch those inline copies as well. The structured attributes marker the PR proposes is the right way to tell these rows apart.

P2-4 (test gap; no current defect): passing None through is not pinned by any test.

• Removing formatted is None or (:1151) still passes all 651 tests.
• That regression would turn every formatter that returns None into a counted failure. Returning None is a documented, plausible way to omit content.
• Suggestion: one parametrized "supported results pass through unchanged" test covering None, a str subclass, a dict/list subclass, and exact Content/Part/LlmRequest. Together with P1-1, it kills both mutation survivors.

P2-5 (category 2; disclosed residual; meets the wording of RC1): a nested payload-named coroutine still warns.

• Returning (coro,) or [coro], where coro.__qualname__ comes from the content, still emits RuntimeWarning: coroutine '<payload>' was never awaited, on head and base.
• A coroutine returned at the top level is fixed.
• I accept this as a documented residual.

P2-6 (category 2; one-line hardening): the stand-in exception still links to the caller's exception.

• raise _WarningIsolation from None (:1228) only suppresses __context__; the link to the caller's exception remains.
• A handler that reads sys.exc_info() and walks __context__ without honoring __suppress_context__ therefore prints the caller's payload exception. My walk_context probe shows this on head and base. Stock logging and traceback do honor the flag.
• Fix, prototyped: except _WarningIsolation as stand_in: stand_in.__context__ = None. With it, the probe is clean and all 651 tests pass.

P2-7 (cosmetic):

• A plain ExceptionGroup is a heap type in CPython, so its note reads <subclass of BaseExceptionGroup>.
• A whitespace-only existing message yields " ; content_formatter …".

Implementer's listed residuals, classified:

• A genuine signal absorbed during the boundary: reachable without hostile code, but causes no direct loss or leak (P2-2).
• Other plugin log calls during an error callback, with a failing handler: pre-existing. My end-to-end run found no payload printed to stderr by the plugin.
• Nested coroutine: category 2 (P2-5).
• Async generator: an unstarted, payload-named one emits nothing (verified). The finalizer path is category 2.
• Raising __del__: category 2, pre-existing.
• Deferred attributes marker: agreed (P2-3).

Blindness note: my own automatically loaded tool memory held technique notes from earlier rounds of this PR. It contained no reviewer verdicts and nothing from Astra. I read no reviews or comments on this PR.

VERDICT: REQUEST_CHANGES @ 7fae965

[caohy1988]

Blind attempted-disproof review by Astra (Codex), for caohy1988, of head 9f16d70b9df2da5b4ebc375f2a2953713c14b2b0 against def458b609c2811d137b0332b2fc7b201dddd5c0. This is a COMMENT review, not a merge authorization.

Rejection criteria, stated before reading the diff:

1. Payload-derived text reaches BigQuery diagnostics or default logs/stderr through a realistic failure path.
2. Formatter diagnostics drop a row, alter the sentinel/counter, or lose a genuine interrupt contrary to the documented policy.
3. A material PR claim is false or a required guard lacks a test that detects its removal.

Applied with the final-round classification: category (1) means a realistic data-loss/payload-leak path, a material false claim, or an untested fix; category (2) means a hostile-class edge case without a realistic trigger. Named, disclosed follow-ups are not new findings unless an additional category (1) variant is established.

What I checked and tried:

• Read the PR description, issue google#485 item A, the complete two-file base-to-head diff, the surrounding callback/parser/writer code, and the final-round changes. The remote head still matched the requested SHA before posting.
• In a separate uv environment in this worktree, Python 3.11.13: PYTHONPATH=src python -m pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q -rs → 663 passed, 19 warnings, no skips. This includes all 111 diagnostics cases, precedence, opt-in debug output, hostile hooks, injected failures, and real SIGINT/SIGTERM during logging.
• Wrote and ran 35 independent probes, using repository transport fixtures and real Arrow serialization. All passed. These covered payload-named classes bound into a module; metaclass attribute/equality/hash/mro hooks; instance attribute/str/repr hooks; RuntimeError, CancelledError, KeyboardInterrupt, SystemExit and another BaseException; unrenderable debug tracebacks; async formatters; payload-named coroutines and generators; started-generator cleanup; re-entrant formatter diagnostics and logging; closed and failing StreamHandlers; and an actual tool-error callback while the caller handled a separate payload-bearing exception. I checked sentinel, counter, row text, logs, stderr, warnings and unraisable exceptions as applicable. Real signals delivered during labeling produced the row and then a fresh interrupt, preserving SIGTERM's integer exit code.
• Independently removed 12 guards, one mutation at a time, in temporary module copies: string normalization, logger installation, stand-in context clearing, traceback containment, generator-close containment, the outer BaseException boundary, deferred re-raise, fresh-interrupt copying, trusted-class identity matching, the inner label fallback, real-type judging, and coroutine closing. Every mutation failed its targeted tests. In particular, removing string normalization makes test_supported_results_pass_through_unchanged[hostile_str_subclass] fail because CancelledError escapes.
• Re-ran the current diagnostics class against the plugin from 7fae9655: 25 failed / 86 passed, matching the PR's red/green claim. Against the base plugin, all 16 class-diagnostic cases failed. Alternate plugins were loaded from temporary files; tracked repository files were never changed.
• Repository-pinned pyink 25.12.0 and isort 8.0.1 both passed checks on the two changed files.

Contract judgments:

• The trusted-label trade-off is sound for the reviewed CPython environment: direct type descriptors and identity-matched fixed labels avoid payload-controlled names and ordinary metaclass hooks (src/google/adk/plugins/bigquery_agent_analytics_plugin.py:1054–1135). Less-specific labels for Python-defined library exceptions are a reasonable privacy trade-off.
• Both failure paths preserve the sentinel and one formatter-failure count. Existing event diagnostics stay first; the formatter note is appended (:7582–7627). Default warnings remain constant and traceback-free. Debug rendering is expressly opt-in, local-logger only, best effort, and absent from the BigQuery row.
• The interrupt policy is explicit and matches the tested behavior (:1216–1310, :7484–7500). Payload-controlled traceback rendering and coroutine/generator closing contain interrupts; hook-free diagnosis and application logging defer fresh KeyboardInterrupt/SystemExit until writer handoff. CancelledError stays contained in that synchronous boundary. Absorbing a genuine signal during the two payload-controlled steps is a documented trade-off, not an undisclosed claim of perfect interrupt provenance. Writer handoff is not a guarantee of durable BigQuery delivery before process exit.
• The module logger stand-in isolates the implicit current-exception chain for all records (:126–152). Independent failing-handler probes covered both formatter warnings and another plugin warning during an ADK tool-error callback. The PR correctly discloses that explicit exc_info on a pre-existing _safe_callback failure can still carry the caller's context; this change does not claim to sanitize that record.
• The narrowed guarantee for failed/rejected results is honest: accepted dict/list subclasses still enter the parser's pre-existing boundary. I am not re-reporting the named parser/finalizer/async-generator follow-ups as newly discovered defects.
• SDK error predicate: acceptable for this PR, no escalation. With the supplied predicate, the new non-NULL diagnostic necessarily counts formatter-failed rows and their sessions as having an error, even when the event status remains OK. That is a real observability failure, and retaining OK preserves the agent outcome. The impact is documented. A structured marker and a separate SDK predicate remain useful follow-ups; matching an appended message string would not be a reliable distinction.

Findings by priority:

• P0/P1: None. No new category (1) defect or unsupported material claim established in the reviewed scope.
• P2: None. No additional category (2) finding beyond the already named follow-ups. No code changes requested.

Evidence and limits: Independent probes and logs are under /tmp/adk-pr14-dual/r5/astra-probes/, including test_astra_probes.py, plugin-tests.log, independent-tests.log, mutation-results.json, previous-head-tests.log, and base-tests.log. The all-extras install was blocked by Intel-macOS wheels for Antigravity, ONNX Runtime and LanceDB, followed by the locked cryptography source build. I used scoped locked runtime/dev dependencies, the required test and A2A/MCP packages, and cryptography 48.0.1 in the separate environment. I did not rerun the full repository suite, Python 3.10, live BigQuery, or all 43 claimed historical mutations. The worktree has no tracked changes.

Blindness disclosure: the permitted PR description itself contains historical reviewer attributions. I did not open any PR reviews/comments, peer results/probes, earlier-round artifacts, the implementer worktree, or vault files. The conclusions above are based on source inspection and the runs described here.

VERDICT: APPROVE @ 9f16d70

[caohy1988]

Fable blind review, round 5 (final): PR #14 @ 9f16d70

Reviewer: Fable (Claude Opus 5.5). This review was written blind: I read no other review or comment on this PR. Base is def458b6. The code under review is e13f9fba, 2b9fbd0f, 974e4642, 7fae9655, f64500aa and 9f16d70b; the merge commits touch no plugin files.

Summary. No P0 or P1. I found no category-1 path: no realistic formatter outcome drops a row or changes the sentinel or counter, and none puts payload into error_message, the default logs, stderr, warnings or the unraisable hook. I found no false claim in the PR body and no untested guard. There are two small category-2 hardening nits. I rule on the status='OK' design question below: it is acceptable and does not need escalation.

Rejection criteria (written before I read the diff)

• R1, row loss or contract drift. Some formatter outcome, or something the diagnosis does afterwards, drops the row, escapes the callback, or changes the [FORMATTER_FAILED] sentinel, the formatter_failed counter or the constant warning text. This covers raising, unsupported returns, coroutines and generators, str/dict/list subclasses, and failing or closed handlers with or without an active caller exception. It also covers a genuine KeyboardInterrupt or SystemExit being lost, or the row being lost when it is re-raised.
• R2, payload leak. Payload-derived text reaches the row (error_message, content, attributes), the default logs or stderr. Payload-derived text includes str(e), args, the traceback, reprs, and payload-derived class or module names. Routes include handleError or lastResort, and the debug path escaping to BigQuery.
• R3, claim or test gap. The precedence of an existing error_message is lost or untested, the flag is not default-off, a PR-body or docstring claim is false when probed, a claimed guard can be removed without any test failing, or the new tests pass on the unfixed code.

Result: no criterion is met. The evidence follows.

What I ran and tried

1. Repo tests. pytest tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py -n 8 -q gives 663 passed on 3.11.13. TestContentFormatterFailureDiagnostics gives 111 passed on each of 3.10.16, 3.11.13, 3.12.9 and 3.13.7.

2. Red/green. I ran the head's test file against /tmp copies of the plugin, so the worktree was untouched.

   • Against base def458b6: 101 failed, 10 passed. The 10 that pass are guards for unchanged behavior: the 7 pass-through cases, the constant default warning, exception text not reaching the row, and test_plugin_error_logs_keep_their_own_exception.
   • Against 7fae9655: 25 failed, 86 passed. These are exactly the 25 the body lists: 16 injected interrupts, 2 real signals, 5 close-step cases and 2 stand-in cases.

3. Mutation run. I applied 27 mutants of my own to a /tmp copy and ran the new class against each. 26 were killed, including all of these:

   • skipping the str normalization (killed only by [hostile_str_subclass], so that round-5 claim holds);
   • not installing the stand-in, leaving its __context__ set, handling records after its except, or applying it only to formatter warnings;
   • never raising the set-aside interrupt, raising it before the row, raising the original instead of a fresh one, keeping a non-int exit code, or keeping the KeyboardInterrupt's args;
   • setting CancelledError aside too;
   • narrowing the outer boundary, the close guard or the render guard to Exception;
   • not closing coroutines, or not closing generators;
   • reading the MRO through the metaclass, or removing the label fallback;
   • admitting results with isinstance, naming heap types, taking the allowlist label from the class, or matching the allowlist with == instead of is;
   • reversing the precedence, forcing debug on, not passing None through, or dropping the counter.

   The one survivor, _TYPE_NAME.__get__(cls) changed to cls.__name__, is equivalent: that line only runs for static types, and a static type cannot have a Python metaclass. The PR discloses the same survivor.

4. Adversarial probes. The probes are 158 cases in /tmp/adk-pr14-dual/r5/fable-probes/test_fable_r5_probes.py, and all pass on 3.10, 3.11, 3.12 and 3.13. Each drives a public callback through the real Arrow write path. It then checks the row, the drop counter, a stock app StreamHandler, stderr, warnings and sys.unraisablehook.

   • Realistic raises, debug off and on.
     • Cases: ImportError and KeyError carrying the payload; a user module-level class with a payload message plus add_note; raise … from; a json.loads JSONDecodeError, whose doc is the payload; a pydantic ValidationError, whose input is the payload; UnicodeDecodeError; ExceptionGroup; RecursionError; AttributeError and NameError, where 3.12+ adds suggestions.
     • Outcome: fixed labels such as ImportError, <subclass of ValueError> and <subclass of Exception>. No payload reached any sink with debug off. With debug on, the payload appears only in the local log, as documented.
   • Realistic unsupported returns.
     • Cases: tuple, set, int, float, bool, bytes, bytearray, memoryview, a generator expression, a started generator, map, filter, range, a dataclass, a Content subclass, MagicMock, Mock(spec=dict), UserDict, MappingProxyType, an async def formatter, functools.partial of an async function, an unstarted async generator, an asyncio.Future and a bare object.
     • Outcome: each gives the sentinel, one count and a label, with no "never awaited" warning.
   • Native results. A str subclass, OrderedDict, defaultdict, dict and list subclasses, None, Content, Part and LlmRequest all pass through unchanged and uncounted.
   • A TOOL_ERROR raised inside the caller's except, logged through a closed StreamHandler.
     • Row: error_message is "<tool error>; content_formatter raised ImportError".
     • With debug off, stderr is payload-free.
     • With debug on, handleError echoes the rendered traceback, as the docstring warns.
   • App handlers and filters that raise. App handlers' emit, format and handle, and filters, raise KeyboardInterrupt, SystemExit("<payload>"), CancelledError, RuntimeError or GeneratorExit, with debug off and on.
     • The row is always intact.
     • Interrupts come back as KeyboardInterrupt() or SystemExit(1), with no payload in args. Everything else is contained.
     • A SystemExit subclass whose code and __str__ raise gives SystemExit(143).
   • Real SIGINT.
     • During a slow app handler inside an error callback, the row is written and then KeyboardInterrupt() is raised.
     • During debug rendering (a slow __str__), the interrupt is absorbed and the row is intact. This is the trade-off as documented.
   • Hostile metaclasses. I tried __getattribute__ on every name, on __mro__ and __name__ only, __eq__ and __hash__, __instancecheck__ and __subclasscheck__. Each was crossed with raised and returned results, and with KI, SE, CancelledError and RuntimeError.
     • None of these hooks is run by the boundary.
     • Only __subclasscheck__ on a raised class escapes, with 0 rows. CPython 3.10–3.13 runs it inside the formatter's own raise, which is the residual the body discloses.
   • Other cases.
     • A hostile str subclass whose __getattribute__ and __len__ raise KeyboardInterrupt is normalized without running any hook.
     • Two events ran back to back, and only the first hit a handler that raises SystemExit: 2 rows, a count of 2, and only the first event raised.
     • A formatter that logs through the plugin logger and then raises is handled cleanly.
   • Stand-in. The record's funcName and the own exc_info of logger.exception are preserved, and handlers see _LoggingStandIn as the handled exception.

5. Disclosed residual: an admitted dict subclass whose __class__ raises.

   • With KeyboardInterrupt: 0 rows on both head and base, exactly as the body says.
   • With RuntimeError: head writes [CONTENT_PARSE_FAILED] (counted as content_parse_failed), where base wrote [FORMATTER_FAILED]. Only a hostile class triggers this, and the row still fails closed. Not a finding.

6. 3.14. Only 3.14.0a6 is available locally, and importing the plugin segfaults there against wheels built for 3.14 final. I therefore ran a stdlib-only copy of the primitives on 3.14.0a6 and 3.10.16, and all were OK:

   • closing an unstarted coroutine suppresses the warning;
   • the type descriptors run no hooks;
   • issubclass(type(x), …) runs no hooks;
   • single-argument format_exception works;
   • the instance-level handle stand-in works;
   • the SystemExit.code descriptor reads the code.

7. Format checks. Using the repo's pins, pyink 25.12.0 --check and isort 8.0.1 --check-only are clean on both files.

8. CI. All five Unit Tests jobs are cancelled at the 10-minute timeout, each with one F at 93–96%. The fork's own main push e4c0d946 shows the identical pattern on all five versions, so this red is not from this PR. The MCP, A2A, mypy and pre-commit jobs pass.

Contract check

• Fixed-shape, class-only error_message on both failure paths: PASS. Neither str(e), args nor the traceback is ever used.
  • Payload-derived names never appear. I tried type()-created classes, classes registered or bound into a module, a renamed allowlisted class, a metaclass that lies about __flags__, and an unreadable MRO.
  • The trusted-set rule is sound. It names only static C types, whose names are immutable and fixed at build time, plus five classes matched by identity whose labels are fixed strings. Every runtime-created class falls back to its nearest trusted ancestor.
  • The trade-off is acceptable given the local debug flag. JSONDecodeError, pydantic ValidationError and a developer's own classes lose their exact names and read as <subclass of ValueError> or <subclass of Exception>.
• Precedence: PASS. The event's own message stays first and is followed by "; ". An empty message is treated as absent. Two tests cover this, and mutant M19 was killed.
• debug_content_formatter_errors: PASS. It defaults to False.
  • Handlers only ever get rendered text, never live exc_info, and nothing reaches BigQuery.
  • The default warning strings are byte-identical to base.
• Sentinel, counter and warning text unchanged, and the helper never raises out or drops the row: PASS. Only set-aside interrupts propagate, and only after batch_processor.append(row).
• Tests fail on the unfixed code: PASS. 101 of 111 fail on base, and the 10 that pass are guards for unchanged behavior.
• Round 5: PASS on each claim.
  • The interrupt policy is right. Where an interrupt came from can't be observed, so sorting by the code that ran is the correct design:
    • Steps that run payload code (render, close) are contained, so the content cannot end the run.
    • Interrupts raised in plugin or app code are set aside and then re-raised fresh after the row, so neither the row nor the signal is lost.
    • Containing CancelledError is correct, because nothing in the boundary awaits.
  • The trade-off wording is accurate.
  • All plugin log calls go through the stand-in. I checked this with a closed StreamHandler plus a caller payload exception, for both the formatter warning and the parse-failure warning.
  • The one exception is pre-existing. _safe_callback's logger.exception still renders its own exc_info chain, which includes the caller's exception. This is unchanged from main, the body discloses it, and it is out of scope. I agree it should be filed as a follow-up.

Design question: error_message on status='OK' rows

Ruling: acceptable. I do not escalate this.

• The intent source, issue google#485 item A, explicitly prescribes event_data.error_message = …. Making the SDK predicate's error_message IS NOT NULL true is the direct consequence of the requested change, not a conflict with the intent.
• A formatter failure is real content loss on that row. Surfacing it in error dashboards and alerts turns the reported failure mode (an ImportError on every call) from silent into loud.
• The PR documents the effect in the schema description (src/google/adk/plugins/bigquery_agent_analytics_plugin.py:4587) and in the body, which lists the predicate and its inline copies.

It is not category 1 (no data loss, no leak, and it is disclosed). It is listed as P2-3 below.

Findings

P0: none.

P1: none.

P2-1 (category 2): the re-raised interrupt still chains to the caller's exception (src/google/adk/plugins/bigquery_agent_analytics_plugin.py:7497).

• Evidence. I sent a real SIGINT during the warning inside on_tool_error_callback, while the caller handled RuntimeError("caller <payload>"). The row was written, and KeyboardInterrupt() was raised with no args and __suppress_context__ set. But __context__ is the caller's RuntimeError('caller PAYLOADMARK-…').
• Why it matters. from None only sets __suppress_context__. The PR's own _ContextWalkingHandler models a consumer that ignores that flag, and that is the reason _LoggingStandIn clears __context__ rather than relying on from None.
• Why it isn't category 1. Default printers honor the flag, and any other interrupt raised during an error callback is chained the same way, without suppression. A leak needs a genuine interrupt during the warning, inside an error callback, plus a chain walker that ignores suppression.
• Suggested fix (I verified it in isolation: __context__ becomes the context-free stand-in):

      finally:
        if interrupts:
          try:
            raise _LoggingStandIn
          except _LoggingStandIn as stand_in:
            stand_in.__context__ = None
            raise interrupts[0] from None

P2-2 (category 2): the stand-in binds Logger.handle at import time (src/google/adk/plugins/bigquery_agent_analytics_plugin.py:138, :145).

• Effect. A class-level patch of logging.Logger.handle applied after the plugin is imported, such as instrumentation or a test fixture, is silently bypassed for this one logger. In my probe, mock.patch.object(logging.Logger, "handle", …) saw google_adk.other but never the plugin logger.
• Sentry's callHandlers patch still works, because Logger.handle resolves callHandlers on each call.
• Suggested fix. Resolve handle on each call: call type(target).handle(target, record) inside handle_with_stand_in. I verified that the patch then sees the record.
• This is interop only, with no data loss and no leak. I know of no mainstream library that patches Logger.handle.

P2-3 (design ruling, not a defect): the SDK follow-up for the error predicate.

• Before or with the upstream PR, file the BigQuery-Agent-Analytics-SDK issue the body proposes. It should separate formatter-failure rows from agent errors in ERROR_SQL_PREDICATE, is_error_event and the inline has_error copies, so that evaluations don't penalize an agent for a misconfigured logging pipeline.
• Prefer a structured attributes marker to text matching. The note is appended after event-controlled text, so a text match can be spoofed.

Residuals confirmed as disclosed (not findings, category 2)

I reproduced these and found no category-1 variant:

• __subclasscheck__ on the raised class escapes with 0 rows on 3.10–3.13.
• A payload-renamed coroutine nested in a rejected tuple still warns with its name. A normally named nested coroutine warns with its code-defined name only.
• An unstarted async generator is clean, and starting one from a sync formatter takes deliberate driving.
• The admitted dict/list subclass __class__ interrupt behaves exactly as on main.
• I did not probe a raising __del__.

VERIFIED / NOT VERIFIED

• Verified: everything listed above, with the commands and results stated.
• Not verified:
  • the full tests/unittests suite (I ran only the plugin file and the new class);
  • Python 3.14 final (primitives on 3.14.0a6 only);
  • a live BigQuery write (mocked write client, but the real Arrow path);
  • Windows (the signal tests are skipped there by design);
  • a raising __del__ on a rejected result.

VERDICT: APPROVE @ 9f16d70

[caohy1988]

Independent scoped round-6 review of f3d2ceb26eca99378193274fe8844a4238f59e51 against fork main 2c16d9dcb556ae464eae83aab2d10bdf2f815c15.

Rejection criteria, written before reading the diff:

1. Deferred interrupts retain the payload-bearing caller exception through __context__, or plugin warning handling exposes it to logs/stderr.
2. The round-6 fixes have ineffective regression coverage, or merge resolution loses PR #15 behavior/tests or PR #14 diagnostics.
3. The scoped plugin delta introduces a test, typing, or formatting regression absent from the base.

What I verified:

• Read issue google#485 item A, the PR description, both plugin/test deltas, b8f50f8a, and the merge against each parent. No peer review or PR comments/reviews were opened. Blindness disclosure: the allowed PR description itself states earlier reviewers' conclusions and attributes some residual assessments to reviewers; I saw those statements, but did not use them as evidence.
• Full plugin file on head: 800 passed, 1 skipped (801 cases; Python 3.11.13, real Arrow capture path). Full plugin file on an isolated archive of the requested base: 685 passed, 1 skipped (686 cases). The skip requires Python 3.14. toolbox-adk is installed here.
• AST comparison: every base test is unchanged; every pre-merge-head test remains, with only the documented error-message precedence test adapted. Base plugin functions are unchanged except _log_event, config, and schema description. The merge retains PR #15 classification, error deduplication, serialization protection, and tool-error handling. Outside these two files, the head is identical to google main d312c0ec.
• Independent probes in /tmp/adk-pr14-dual/r6/astra-probes/: 14 passed. Tried a module-registered payload-named exception; raising metaclass attribute/equality/hash hooks; returned object attribute hooks raising KeyboardInterrupt; payload-named coroutine and generator; traceback hooks raising interrupts; re-entrant settlement; a closed StreamHandler with an active payload-bearing caller exception; a later class-level Logger.handle patch handling a non-formatter warning; combined MCP/classifier TOOL_ERROR results plus a failing formatter; and handlers raising payload-bearing KeyboardInterrupt, SystemExit (text or integer code), and CancelledError. Sentinel rows and counter survive, default logs/stderr and formatter notes stay payload-free, cancellation is contained, and deferred fresh interrupts have only a context-free stand-in in their context chain.
• Mutation checks on the merged plugin: removing deferred-interrupt stand-in isolation fails 2 tests; restoring import-time handle binding fails 1 test; removing str-subclass normalization fails 1 test. All temporary tracked changes were restored.
• Pinned pyink 25.12.0 and isort 8.0.1 checks pass on both files. Strict mypy of the plugin reports no issues.

Contract judgments:

• Context isolation at src/google/adk/plugins/bigquery_agent_analytics_plugin.py:7980 works; clearing the stand-in context is necessary in addition to from None. Dynamic handle lookup at line 151 works and its new test detects reverting it.
• The status/error-predicate effect is accurately documented at lines 2845–2848. It is acceptable for item A: formatter failure represents lost content and the requested non-NULL diagnostic intentionally makes it visible to the consumer's error predicate. A separate SDK marker/predicate remains a reasonable owner-controlled follow-up.
• The conservative trusted-label set is sound: it sacrifices exact Python-defined class names to avoid runtime/payload-selected names. Interrupt containment during traceback rendering/result cleanup, versus deferred text-free propagation from hook-free diagnostics/application logging, is an explicit and reasonable trade-off within the bounded contract. The existing documented residuals are not new findings in this scoped review.

CI attribution (not a plugin regression):

• All four failing Mypy 3.10–3.13 jobs report the same src/google/adk/utils/streaming_utils.py:581 assignment error. That file is byte-identical to base 2c16d9dc. Running mypy without incremental cache reproduces that exact assignment error on both isolated base and head with the same environment. Thus the underlying error is pre-existing on the requested base, despite CI's baseline comparator calling it new. I did not establish why that comparator's baseline missed it.
• Pre-commit's failure is new relative to the requested base, but originates in merged upstream files, not the plugin/test changes: missing fork-required unit guides for _url_validator.py and model_consult _advisor.py, _context.py, _prompts.py. The explicit base-to-head guide check reproduces it; those files are absent on base. Suggested integration follow-up: supply the guides or apply justified guide exemptions through the fork's documented mechanism. Do not describe this failure as already present on base.
• Setup limitation: all-extras resolution hit unavailable Intel macOS wheels (antigravity, onnxruntime, lancedb) and a cryptography source-build failure. Validation used runtime/test tooling with compatible constraints and cryptography 46.0.5. No full repository suite or live BigQuery write was performed by this review.

Findings grouped by priority:

• P0/P1: none. No new category (1) data-loss/payload-leak path, false scoped implementation claim, or untested scoped fix found.
• P2: none. No new category (2) hostile-class issue found; the named residuals remain follow-ups as requested.

VERDICT: APPROVE @ f3d2ceb

[caohy1988]

Fable (Claude) blind review, round 6 (scoped), head f3d2ceb

Scope, as requested: (a) do the three round-6 P2 fixes (b8f50f8a) work, with tests that fail without them; (b) is the merge f3d2ceb2 (fork main 2c16d9dc = PR #15) correct and lossless for both PRs; (c) did anything regress; (d) are the red Mypy / Pre-commit checks caused by this PR. Everything below was re-run in this session at f3d2ceb26eca99378193274fe8844a4238f59e51.

Rejection criteria (written before reading the diff)

• R1, merge loss: the merge drops, weakens, or shadows either side. That means a #15 or #14 test is missing, renamed, weakened, or failing on the merged tree; a duplicate name silently shadows one side; or a conflict region loses one side's behavior (a TOOL_ERROR error_message overwritten by the formatter label, the label lost against the documented precedence, or #15's classifier bypassed).
• R2, P2 fixes unproven: one of the three fixes doesn't do what it claims, or no test fails when it is reverted.
  • P2-1: the re-raised interrupt still reaches the caller's exception via __context__ or __cause__.
  • P2-2: handle is still bound at import, so a later class-level Logger.handle patch is bypassed.
  • P2-3: the docstring about status is missing or wrong.
• R3, regression: on the merged tree, a row is dropped, the sentinel or counter changes, or payload reaches error_message, logs, or stderr through a path reachable without deliberately hostile code. That includes the #15 error-bearing-result path combined with a failing formatter, or CI red caused by this PR's files.

None of R1–R3 is met.

What I tried

Merge (R1)

• The merge base is e4c0d946. For the plugin and its test file, it is identical to google main: def458b6..d312c0ec touches neither file.
• git diff d312c0ec f3d2ceb2, excluding the two files, is empty. Every other file equals google main.
• Changed-line multisets, plugin: the lines the merge adds onto ours equal #15's patch exactly (693 lines). The lines it adds onto theirs equal #14's patch exactly (498 lines). There are no extra or missing lines.
• Changed-line multisets, test file: identical except for two things. One is the adapted precedence test. The other is import gc / import warnings, which both sides added and the merge keeps once.
• None of #15's plugin hunks falls inside the _log_event region (base lines 7105–7369) that #14 split into _log_event and _log_event_row, so no hunk was misplaced.
• AST check: no duplicate top-level names and no duplicate class members in either file at ours, theirs, or merged.
• --collect-only: merged (801) ⊇ ours (666) ∪ theirs (686). The only extra node is test_formatter_failure_follows_the_events_own_error_message[message_less_tool_error].
• Adapted test, before and after (test file:4505-4575):
  • It keeps the same exact-equality assertion.
  • The empty-message case now comes from on_model_error_callback, which still records str(error) verbatim. That move is needed because #15's on_tool_error_callback now records str(error) or type(error).__name__ (plugin:9353).
  • The new case pins the composition of the two PRs.
  • Mutation M4 below proves the empty case is still pinned.

Tests and red/green (R2, R3)

• Full plugin file: 800 passed, 1 skipped (the 3.14-only test) on each of Python 3.11.13, 3.10.16, 3.12.9, and 3.13.7.
• Red/green: I ran the b8f50f8a test file against the 9f16d70b plugin.
  • Exactly the three new cases fail: test_later_patches_of_logger_handle_see_plugin_records and both test_raised_interrupt_is_not_chained_to_the_callers_exception cases. The other 111 pass.
  • With its own plugin, all 114 pass. This matches the PR body.
• Mutations, applied to a /tmp copy of the merged plugin; the worktree was never touched. Each is killed by the repo's own diagnostics class:
  • M1, re-raise without the stand-in (reverts P2-1): the 2 chaining cases fail.
  • M2, stand-in keeps its __context__: the same 2 fail.
  • M3, bind Logger.handle at import (reverts P2-2): test_later_patches… fails.
  • M4, if error_message is not None instead of truthiness: [empty_existing_message] fails.
  • M5, formatter note first: 2 precedence cases fail.
  • M6, no note at all: 95 fail.

My own probes (17 tests outside the repo, all passing on 3.10–3.13; M1 and M2 each break 9 of them, M3 breaks 2)

• P2-1: the interrupt comes from an application log handler and, separately, from a logger filter. Variants: KeyboardInterrupt, SystemExit(3), and SystemExit("<payload text>"), each raised while the caller handles a payload-bearing exception.
  • Each time, the row gets the sentinel, the note, and one count.
  • The escaping exception has the exact type, with code 3 or 1.
  • A BFS over __context__ and __cause__ reaches only [_LoggingStandIn].
  • A hostile printer that ignores __suppress_context__ finds no payload.
  • The same holds for a realistic caller (the tool's exception handled, then on_tool_error_callback) and for a genuine SIGINT sent with os.kill during a slow handler.
• P2-2: a class-level logging.Logger.handle patch applied after import sees the plugin's records, including #15's classifier-failure warning. Each record is handled while _LoggingStandIn is current, with __context__ None. A later patch that drops plugin records is honored.
• #15 × #14:
  • MCP isError and classifier TOOL_ERROR rows, with a formatter that raises or returns a tuple, give status ERROR, sentinel content, and <#15 message>; content_formatter …. formatter_failed is 2 (TOOL_STARTING and TOOL_ERROR), and no payload appears in the rows.
  • #15's classifier warning, with a closed StreamHandler while the caller handles a payload exception, prints "Logging error" but no payload.

End to end with a real Runner (a subprocess, no pytest logging capture)

• Setup: a FunctionTool raises ValueError(<payload>), and ADK calls the error callback inside its own except. The formatter raises. An application handler raises KeyboardInterrupt, or SystemExit(7), on the TOOL_ERROR formatter warning.
• The TOOL_ERROR row reaches BatchProcessor.append with the sentinel and <str(error)>; content_formatter raised ImportError. The escaped chain is [KeyboardInterrupt | SystemExit(7), _LoggingStandIn]. Neither the chain, the in-process stderr, nor the process stderr contains the payload.
• formatter_failed was 8 with 7 rows. The eighth was AGENT_COMPLETED, cancelled by asyncio.run teardown after the interrupt. The counter already comes before the append on base, so this is not a finding.

Other checks

• 3.14: the plugin can't be imported on 3.14.0a6 here, so I ran a stdlib-only replica of the re-raise and the per-call handle lookup. It gives the chain [KeyboardInterrupt, StandIn] with __suppress_context__ True, and a later class patch sees the record under the stand-in.
• 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 (strict): 0 errors in the plugin module, at both base and head.
• Regression sweep: every other test file that imports the plugin (cli/test_fast_api.py, test_optional_dependencies.py), plus plugins/, utils/, and telemetry/: 2621 passed, 0 failed. I did not rerun the full 16k-test suite.

Round-6 verdicts per item

• P2-1 works and is pinned. _log_event (plugin:7973-7984) raises the fresh interrupt while a context-free _LoggingStandIn is handled. __context__ is the stand-in, the stand-in's own __context__ is None, and __suppress_context__ stays True.
• P2-2 works and is pinned. type(target).handle is resolved on each call (plugin:151).
• P2-3 is accurate. The docstring (plugin:2845-2848) says status is left as the event set it, usually 'OK'. My E2E rows show exactly that: status 'OK' with a non-NULL error_message on every formatter-failed non-error row. I accept the downstream error-predicate effect as documented; it doesn't block. Item A asks for error_message, and the SDK-side FORMATTER_FAILED predicate in Solution 6 is the right follow-up.
• The merge is correct and lossless. Both sides' behavior and tests are preserved (see R1 above). The import pydantic and from pydantic import BaseModel pair and the field order (tool_result_classifier then debug_content_formatter_errors, plugin:3021/3025) are right.

Findings

P0: none.

P1: none.

P2 (non-blocking):

• P2-a, category (2): cosmetic, no data loss or leak. #15's fixed MCP message _MCP_TOOL_ERROR_MESSAGE ends with a period (plugin:810), so the composed text (plugin:8111) reads Tool returned an MCP result with isError=true.; content_formatter raised ImportError. Changing the event's own text would conflict with "keep it intact", so no change is needed. If you care, drop the period from #15's constant in a later cleanup.

CI

• Pre-commit Linter: not caused by this PR's files.
  • Only check-new-py-prefix fails. It flags four files: src/google/adk/tools/_url_validator.py and tools/model_consult/{_advisor,_context,_prompts}.py.
  • All four were added by google main. They are absent from the fork base 2c16d9dc and identical to google main d312c0ec at head.
  • The hook diffs HEAD~1..HEAD of the CI merge ref a5752fe3 (f3d2ceb2 into 2c16d9dc), so any upstream file the fork lacks counts as new.
  • All other hooks pass in CI: ruff, isort, pyink, addlicense, ADK compliance, codespell, mdformat.
  • Syncing fork main with google main clears it.
• Mypy 3.10–3.13: not caused by this PR.
  • Totals are equal: 851/851 on 3.10 and 860/860 on the other versions.
  • The only "new" line is src/google/adk/utils/streaming_utils.py:581, a file that is byte-identical at base and head.
  • I reproduced it locally with mypy 2.3.1. With a cold cache, the full base and head trees give identical 860-error sets.
  • The job's second run reuses the base run's .mypy_cache. Replaying that locally (base run, then head overlaid in the same directory) produces CI's "new" line byte for byte. It is the same pre-existing error with the same 17 FinishReason members, listed alphabetically instead of in declaration order.
  • So this is a text-diff artifact of the warm cache.
• Unit Tests, all 5 jobs: cancelled at the 10-minute job timeout at 95–96%, not failed.
  • The base push run 36665568458 cancels the same way, with 1 F per job. Upstream google main runs show 1–2 F per job: d312c0ec 1 F, f44d5124 2 F on 3.10 and 3.12.
  • This PR's run has 2 F on 3.10 and 3.13 and 1 F elsewhere, within upstream's variance. Under -n auto those F can't be mapped to tests.
  • Locally, every test above passes on 3.10–3.13.

Independence note: I read no review and no review or issue comment on this PR. My local persistent memory, which Claude sessions on this machine share, did contain notes from earlier rounds of this PR. Those include implementer-side notes on this round's merge and CI analysis and my own earlier-round observations. I relied on none of them: every conclusion above comes from checks re-run in this session.

VERDICT: APPROVE @ f3d2ceb