Skip to content

Breaking: refuse Google ADK method references whose class was rebound - #363

Open
campoy wants to merge 2 commits into
mainfrom
fix/google-adk-python-rebound-method-class
Open

campoy wants to merge 2 commits into
mainfrom
fix/google-adk-python-rebound-method-class

Conversation

@campoy

@campoy campoy commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

What was changed

activity_as_tool in google_adk tells a method passed as Class.method from a staticmethod by finding the class that the function's __qualname__ names, in the function's globals, and reading that attribute (#312). It now also checks that the attribute is the function it was given, or a staticmethod of it. A functools.wraps wrapper of that function, such as decorate(Class.method), still counts as that function. If the class's name or that attribute now holds another object, as after importlib.reload of a changed module, activity_as_tool can't tell what the reference is, so it raises ValueError, as it does for a class it can't reach. The message says to pass a bound method for a method, or to rename a staticmethod's parameter.

The activity_as_tool docstring names the new case.

Breaking change: compared with 0.0.1, a staticmethod taking a parameter named self, referenced after its class's name or attribute was rebound, now fails with ValueError when the tool is built, and its runs stop replaying. On 0.0.1 its tool offered self. #312 already does the same for a class that can't be reached. A method referenced this way didn't work on 0.0.1 either. #312 isn't released yet, so both land in the same release, and the release notes can list this case with #312's. No version gate.

Why?

See #362. On main since #312, such a reference is classified by the object now under the name. A staticmethod echo(self, value), kept before its class's name was rebound to a class with a method echo, gets a tool without self. The model's value reaches self, and the call returns a wrong result with no error. In the reverse case, a method keeps self in its tool.

Checklist

  1. Closes [Bug] google_adk: activity_as_tool classifies a method reference by its class's rebound attribute (main only) #362. Follow-up to Breaking: support method and callable-class Activities in Google ADK activity_as_tool #312.

  2. How was this tested:

    • test_activity_as_tool_rejects_method_of_unreachable_class now also rebinds the class's module-level name to a class with the opposite kinds. It checks that a kept staticmethod reference and a kept method reference both raise ValueError, bare and wrapped with a functools.wraps decorator. With main's workflow.py (88745dc) installed, it fails with DID NOT RAISE ValueError.
    • test_activity_as_tool_preserves_metadata now passes a functools.wraps wrapper of an unchanged Class.staticmethod, of a Class.method, and of an already decorated staticmethod. It checks their parameters, the wrapped staticmethod's ADK schema, and that calling its tool returns the model's self. It passes on main too.
    • [Bug] google_adk: activity_as_tool classifies a method reference by its class's rebound attribute (main only) #362's reproduction exits 0 on 0.0.1 (the tool offers ['self', 'value'] and returns self=model-self value=model-value), 1 on main (['value'], self=model-value value=default), and 0 with this change, where activity_as_tool raises ValueError.
    • Locally, in Docker with Python 3.13: make lint passes. make test gives 175 passed and 5 skipped without tests/test_optional_mcp.py, and 5 passed for that file on its own.
    • Replay: the 16 histories of Breaking: support method and callable-class Activities in Google ADK activity_as_tool #312's replay check (function, callable-instance, method and class tools, recorded before Breaking: support method and callable-class Activities in Google ADK activity_as_tool #312 and on its first revision, finished and held) give the same results on this branch as on main, and so does the control. No new histories were recorded: the change only adds a refusal when the tool is built, and changes no command of a tool it builds.
  3. Any docs updates needed?
    The activity_as_tool docstring is updated here. The release notes should list this case with Breaking: support method and callable-class Activities in Google ADK activity_as_tool #312's breaking changes.

activity_as_tool tells a method from a staticmethod by reading the
attribute that the function's __qualname__ names in its class. If the
class's name or that attribute was rebound after the reference was taken,
the attribute is another object, and the reference was classified by it: a
staticmethod could lose its self, so the model's value reached the wrong
parameter. Check that the attribute is the function, or a staticmethod of
it, and raise ValueError otherwise, as for a class that can't be reached.
@campoy
campoy marked this pull request as ready for review October 31, 2026 18:26
@campoy
campoy requested a review from a team as a code owner October 31, 2026 18:26

@DABH DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at head 53ba069 (two commits on 88745dc, current main). On a worktree at the head: make lint is clean and make test gives 180 passed, 5 skipped (Python 3.14.3, temporalio 1.34.0, google-adk 2.12.0); CI is green. Both findings were reproduced with probes against installed builds of 0.0.1, main, the head and a one-line fix; the first also end to end with real Workers on the pinned dev server under the sandboxed runner, with histories recorded on 0.0.1 and on main replayed across the builds. The body's claims hold: #362's reproduction exits 1 on main and 0 on the head; the rejection test fails on main with DID NOT RAISE ValueError and the metadata test passes there; four single-line mutants of the new check (no check, the first commit's is not fn, ignoring a staticmethod's __func__, unwrapping without stop) are each caught by the new tests. Checked and refuted: false refusals for functools.wraps-style decorators (tenacity's @retry, warnings.deprecated, pydantic's validate_call), for dataclass(slots=True) and attrs @define classes, which rebuild the class, and inside the Workflow sandbox (the suite's integration test builds the method, staticmethod, decorated and private-class forms in Workflow code); any change to a tool the head still builds (_is_method returns main's value whenever it does not raise). No other Python plugin has this classification, and Go and TypeScript take function values or registered names, so the languages stay consistent. Nothing material was left unverified.

Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

Requesting changes for one item: the new check also refuses wrapt-decorated methods that nothing rebound, so 0.0.1 runs that build such a tool stop replaying on upgrade, a break the body does not disclose; the one-line fix in the inline comment removes it and keeps every refusal this PR adds.

Replay, at the head: for every tool the head still builds, nothing changes. The 60 replays of the #312 review's histories (function, bound-method, staticmethod, method, callable-instance, class, tool_context and module-placement forms, recorded on #312's base and head, sandboxed and unsandboxed, finished and held) give identical outcomes on main and on the head: 49 replay, and the same 11 fail on #312's own changes. That agrees with the body's 16-history check, and the disclosed break holds as described (a staticmethod taking self, referenced after its class was rebound, now raises when the tool is built). The undisclosed break is the P2: histories recorded on 0.0.1 with a wrapt-decorated Class.method tool, including two runs that completed without calling it, replay on 0.0.1, on main and on the patched copy, and fail on the head. Those replays ran with debug_mode=True, because 0.0.1's own slow first activation intermittently trips the deadlock detector.

target = attr.__func__ if static else attr
# fn may wrap the attribute's function, as a functools.wraps decorator
# applied to Class.method does.
if inspect.unwrap(fn, stop=lambda f: f is target) is not target:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: the identity check also refuses methods decorated with a wrapt-based decorator, which nothing rebound, so 0.0.1 runs that build such a tool stop replaying on upgrade, and the body does not disclose it. wrapt keeps a FunctionWrapper in the class __dict__, and class access returns a new BoundFunctionWrapper whose __wrapped__ is the plain function, so the attribute is never on fn's unwrap chain and this line raises. Probe, wrapt 2.4.1, with class WraptHolder: @activity.defn @traced async def meth(self, trip: str) (traced a @wrapt.decorator): main offers ['trip']; the head raises Cannot tell whether 'WraptHolder.meth' is a method or a staticmethod: 'WraptHolder.meth' reachable from the function's globals is another object, in either decorator order. The Deprecated package's @deprecated(reason=...) (3.0.1, a wrapt.FunctionWrapper subclass) does the same. Replay, with real Workers on the pinned dev server: 0.0.1 offers the tool as ['self', 'trip'] and completes a run whose tools are track and WraptHolder.meth when the model calls only track, and a run with only WraptHolder.meth that calls nothing. Both histories replay on 0.0.1 and on main, and fail on the head with that ValueError, so a run still open at the upgrade fails every Workflow Task until the code changes. On main the same tool runs end to end (the Worker's WraptHolder("worker").meth returns wrapt-meth:worker:meth.trip); on the head a new run fails its first Workflow Task. A wrapt staticmethod works on all three. wrapt is already in this plugin's lockfile through the dev group's OpenInference instrumentation. Fix: compare the innermost functions, if inspect.unwrap(fn) is not inspect.unwrap(target):, and reword the comments here and the _is_method docstring to match. On a patched copy the wrapt and Deprecated methods offer ['trip'] again, the 0.0.1 and main histories replay, #362's reproduction still exits 0, references kept across a reload are still refused, and make lint and the full suite pass (180 passed, 5 skipped). To pin it without depending on wrapt, a 12-line decorator class in tests/helpers/decorators.py whose __get__ returns a fresh functools.wraps wrapper of self.__wrapped__.__get__(obj, cls) reproduces the refusal (main and the patched copy offer ['passengers'], the head raises) and fits as a row in the metadata loop. If you keep the stricter check instead, the Breaking change paragraph and the docstring need to name wrapt-style decorated methods and their 0.0.1 runs.

raise ValueError(
f"Cannot tell whether '{fn.__qualname__}' is a method or a"
f" staticmethod: '{'.'.join(qualname)}' reachable from the"
" function's globals is another object. Pass a bound method for a"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: the refusal fires after any in-process reload, not only one that changed the class, and neither remedy in the message fits that case. importlib.reload re-executes the module into the same globals, so every reference taken before it fails the check even when the file is unchanged. Probe: from acts import Holder, keep Holder.count (method) and Holder.echo (staticmethod taking self), then reload the unchanged acts: main offers ['passengers'] and ['self']; the head raises for both, and also for Holder.count read again through the caller's Holder, which is still the old class (a re-run notebook cell does exactly this). Refusing is the trade the body chooses, but 'as after importlib.reload of a changed module' understates it for the release notes, and that user needs to take the reference again, not pass a bound method or rename a parameter. Suggested: say 'after a reload' in the body, and end the message with 'Take the reference again if its module was reloaded, pass a bound method for a method, or rename a staticmethod's parameter.'

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] google_adk: activity_as_tool classifies a method reference by its class's rebound attribute (main only)

2 participants