Skip to content

Feat: add the detectErrorInResponse tool telemetry hook - #606

Open
AmaadMartin wants to merge 3 commits into
mainfrom
feat/tool-error-detection-telemetry-hook
Open

Feat: add the detectErrorInResponse tool telemetry hook#606
AmaadMartin wants to merge 3 commits into
mainfrom
feat/tool-error-detection-telemetry-hook

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 4, 2026

Copy link
Copy Markdown
Owner

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):
    N/A

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

Problem: Many tools never throw. They report failure in-band, resolving with
{status: 'ERROR', ...} or {error: '...'}, and the agent loop treats that as a
normal return. The execute_tool span therefore renders as a green, successful
span and the failure is invisible to operators. adk-python solves this with an
optional per-tool hook (_detect_error_in_response) whose result is put on the
tool span as error.type plus an ERROR span status; adk-js had no equivalent.

Solution: Port the framework side of that mechanism — the extension point and
its single reader — in three coordinated pieces:

  1. core/src/tools/base_tool.ts: declare an optional method
    detectErrorInResponse?(response: unknown): string | undefined. An optional
    method declaration with no body is erased at compile time, so BaseTool gains
    zero runtime surface and every existing subclass, in-repo and downstream,
    still typechecks.
  2. core/src/agents/functions.ts: a module-level
    detectErrorTypeForTelemetry(tool, toolContext, response) consulted from
    callToolAsync where the function-response event handed to traceToolCall is
    built. It returns undefined when the tool is requesting auth or confirmation
    (that response is a control signal, not a failure), when the tool declares no
    hook, and when the hook returns a non-string. It never throws: an exception
    from a detector is logged via logger.error and swallowed.
  3. core/src/telemetry/tracing.ts: TraceToolCallParams gains an optional
    errorType; when present, traceToolCall sets the error.type attribute and
    span.setStatus({code: SpanStatusCode.ERROR, message: errorType}). The status
    is what actually stops the span rendering as successful. The message repeats
    the type, never the response body, so no tool content escapes into an
    attribute the ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS toggle cannot gate.

Behaviour is unchanged for every tool that does not declare the hook: no new
attribute, no status change, no extra log line, and the emitted
function-response event is byte-for-byte what it is today.

Two deliberate divergences from adk-python, called out explicitly

1. setStatus is parity — but only against current adk-python main, so
check the revision.
trace_tool_call in
src/google/adk/telemetry/tracing.py
computes a failure_type and does:

if failure_type is not None:
    span.set_attribute(ERROR_TYPE, failure_type)
    # Without an explicit error status the span renders as successful, which
    # hides tools that reported a failure as a response dict instead of
    # raising. The description repeats the type rather than the error message
    # so no tool content lands in an attribute the content toggle cannot gate.
    span.set_status(Status(StatusCode.ERROR, failure_type))

An adk-python checkout from before that landed sets only the attribute and
contains no StatusCode at all, so a stale reference reads as though adk-js
invented the status. It did not — the comment in tracing.ts is a paraphrase of
the one above. The status is also the point of the change: the attribute alone
still leaves the span rendering green in a trace UI, which is the bug being
fixed.

Known asymmetry this PR does not fix: callToolAsync is a bare
try/finally with no catch, so a tool that genuinely throws skips
traceToolCall entirely and its span ends UNSET with no error.type — an
in-band failure is therefore reported more severely than a real exception.
adk-python has no such gap (it passes the exception to trace_tool_call, which
records it and sets the same status). Closing it means tracing the throw path,
which changes the telemetry of every currently-crashing tool, so it is left as
its own change rather than half-fixed here.

2. The hook is public SDK surface, where adk-python's is not. adk-python
reaches the detector with getattr(tool, '_detect_error_in_response', None)
specifically "to avoid adding a public API on BaseTool". TypeScript has no
equivalent: calling an undeclared member requires a cast, and as any /
as unknown as are exactly what the contribution guidelines forbid. Declaring
the member optional is the only cast-free option, so BaseTool gains six lines
of public, documented extension point. The name drops Python's leading
underscore per the adk-js style guide; the observable strings (error.type, and
error-type values such as TOOL_ERROR) match adk-python byte for byte.

Other notes

  • Not dead code. The hook ships with no in-repo implementer, but its
    readerfunctions.ts — is added in this same change, and it is an
    extension point any user-defined tool can implement on day one. Landing a
    producer first (e.g. on McpTool) with nothing calling it would be the
    genuinely dead ordering. Implementing the hook on concrete tools changes the
    observable telemetry of shipping tools and is left to follow-ups.
  • Where detection runs. In adk-python the whole tool pipeline runs inside one
    instrumentation context, so detection happens on the post-after-callback
    response. In adk-js the execute_tool span is opened and closed inside
    callToolAsync, which wraps only tool.runAsync. Detection therefore runs on
    the tool's own return value — which is also the correct input, since the hook
    is defined as a tool classifying its own response, not a value an after-tool
    callback substituted.
  • No new dependencies. SpanStatusCode comes from @opentelemetry/api,
    already a core dependency; error.type is a local const alongside the
    existing attribute-key constants rather than pulling in
    @opentelemetry/semantic-conventions. No package.json / lockfile change.
  • No suppressions. No any, as any, as never, as unknown as,
    @ts-expect-error, eslint-disable or coverage-ignore directive is added
    anywhere in this diff (verified by grep over git diff).

Collision check (run before starting): I listed all 380 open PRs on the fork
(gh pr list --repo AmaadMartin/adk-js --state open --limit 1000) and read the
file lists of every plausibly adjacent one. No open PR implements
detectErrorInResponse or puts error.type on the tool span
(gh search code --owner AmaadMartin detectErrorInResponse returns nothing).
The nearest neighbours are #399/#400 (port resolveErrorType and the OTel
metrics contract — error.type on metric attributes in telemetry/metrics.ts,
not on the execute_tool span), #27 (implements traceToolCall at the merged
tool-call site), and #352/#441/#394 (other optional BaseTool members). These
overlap in file only, not in behaviour, so this branches from main rather than
stacking.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

All tests live in one new file, core/test/telemetry/tool_error_span_test.ts,
and use no mocks of the code under test: a real NodeTracerProvider +
InMemorySpanExporter + SimpleSpanProcessor records the actual spans and every
assertion reads the exported span. No existing test file is modified.

describe('traceToolCall error reporting') — 3 cases calling traceToolCall
inside a real active span: errorType present → error.type attribute +
{code: ERROR, message: 'TOOL_ERROR'} status; errorType absent → no attribute,
UNSET; errorType: '' → treated as absent.

describe('execute_tool span for a tool that classifies its own response') — 7
cases driven end to end through the public
functionsExportedForTestingOnly.handleFunctionCallList, each also asserting the
function-response event handed back to the agent is exactly what the tool
returned:

  1. {status: 'ERROR'} + hook → error.type = 'TOOL_ERROR' and ERROR status
  2. {status: 'OK'} + same hook → no attribute, UNSET status
  3. tool declaring no hook → no attribute, UNSET status
  4. tool calling toolContext.requestConfirmation(...) during runAsync → skipped
  5. tool calling toolContext.requestCredential(...) during runAsync → skipped
  6. hook throws → no attribute, the normal response event is still emitted, and
    logger.error is called exactly once with the tool name
  7. hook returns a non-string (untyped JS tool) → no attribute

An earlier revision put cases 1-7 in core/test/agents/functions_test.ts behind a
file-scope vi.mock of the tracing module. That stubbed traceToolCall for the
whole file, so all 30 pre-existing cases there would have run against a no-op
tracer — the harness equivalent of a file-scope eslint-disable. The cases were
moved here instead, where a real exporter proves strictly more, and
functions_test.ts is back to untouched.

Commands run (targeted only, never the whole suite), all green on the pushed
commit:

npx vitest run --project unit:core core/test/telemetry/tool_error_span_test.ts \
  core/test/telemetry/tracing_test.ts core/test/agents/functions_test.ts
#  Test Files 3 passed (3)   Tests 46 passed (46)
npm run build        # ok
npm run lint         # ok (0 problems)
npm run format:check # ok
npm run docs:check   # ok (typedoc --treatWarningsAsErrors)
bash scripts/check_license.sh  # ok

Coverage of the new code: 100% of new lines and branches. Measured with
--coverage.include limited to the touched sources; every uncovered line and
branch reported for functions.ts and tracing.ts is pre-existing code
elsewhere in those files (e.g. the tool.description || '' fallback and the
content-toggle ternary at tracing.ts:121,129-131), none is in the new helper,
the new call-site argument, or the new errorType block (tracing.ts:134-141).

Proof that each new test can fail. Every mutation below was applied to the
source, the targeted tests re-run, and the source restored:

# Mutation Result
1 delete the isEmpty(...requestedAuthConfigs/requestedToolConfirmations) skip guard 2 failed — expected { …(11) } to not have property "error.type" (confirmation and auth cases)
2 remove the try/catch around the detector 1 failed — expected { error: 'detection exploded' } to deeply equal { result: 'tool executed' } (the detector's exception escapes into the tool's own response)
3 return the hook result unfiltered instead of typeof … === 'string' 1 failed — expected { …(11) } to not have property "error.type"
4 stop passing errorType at the traceToolCall call site 2 failed — expected undefined to be 'TOOL_ERROR' and expected "error" to be called 1 times, but got 0 times
5 drop span.setStatus(...) in traceToolCall 2 failed — expected { code: +0 } to deeply equal { code: 2, message: 'TOOL_ERROR' }
6 weaken the guard to if (errorType !== undefined) 1 failed — expected { …(11) } to not have property "error.type"
7 drop the error.type attribute 2 failed — expected undefined to be 'TOOL_ERROR'

One honest caveat: npm run ts:check (tsc --noEmit, not run by CI) reports 281
pre-existing errors on main, caused by @google/adk resolving to core/dist
declarations inside core/test. The new test file adds exactly one more error of
that same pre-existing class (a tool typed from @google/adk passed to the
src-imported traceToolCall, which is not part of the public surface). Several
open PRs are fixing that resolution duality repo-wide; nothing here works around
it with a cast.

Manual End-to-End (E2E) Tests:

No credentials, network or model access are needed — the real-span suite is the
manual E2E procedure, and it can be run directly:

npx vitest run --project unit:core core/test/telemetry/tool_error_span_test.ts

To observe it by hand in an application, implement the hook on a tool and export
spans anywhere:

class InventoryTool extends BaseTool {
  async runAsync(): Promise<unknown> {
    return {status: 'ERROR', detail: 'SKU not found'}; // never throws
  }
  override detectErrorInResponse(response: unknown): string | undefined {
    return isRecord(response) && response['status'] === 'ERROR'
      ? 'TOOL_ERROR'
      : undefined;
  }
}

The execute_tool InventoryTool span now carries error.type = "TOOL_ERROR" and
an ERROR status; a tool that requests OAuth or a confirmation gate still
reports clean.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 3 commits August 3, 2026 18:49
A tool that reports a failure in-band, by resolving with a response such as
{status: 'ERROR'} rather than throwing, currently renders as a successful
execute_tool span, so the failure is invisible to operators.

Declare an optional detectErrorInResponse hook on BaseTool, consult it in
callToolAsync once the tool resolves, and set error.type plus an ERROR span
status from the detected type. Detection is skipped while the tool is
requesting auth or a confirmation, and any exception it raises is logged and
swallowed so telemetry can never break tool execution.
Adds unit tests for detectErrorTypeForTelemetry through the public
handleFunctionCallList entry point (detected type, clean response, no hook,
auth and confirmation skips, throwing detector, non-string detector result)
and a real-span suite that records execute_tool spans with an in-memory
OpenTelemetry exporter, so error.type and the ERROR span status are asserted
on an exported span rather than on a stub.
…acer

Relocates the seven tool error-detection cases out of functions_test.ts and
into the real-span suite. The file-scope vi.mock they needed stubbed
traceToolCall for the whole file, so all 30 pre-existing cases in
functions_test.ts ran against a no-op tracer; that file is now untouched
again, and every relocated case asserts the error.type attribute on a span
recorded by an in-memory OpenTelemetry exporter.

Also drops the redundant empty-string check from detectErrorTypeForTelemetry:
traceToolCall already treats an empty error type as no error, and that
behaviour is pinned by its own test.
AmaadMartin pushed a commit that referenced this pull request Aug 4, 2026
createAdkEventFromMetadata() restored `branch` straight from a remote
A2A peer's own response metadata (adk_branch), unlike `author` which is
always force-set by the caller. getContents() (content_processor_utils.ts)
uses an event's branch to keep sibling sub-agent conversation contexts
isolated from each other (a branch is visible in a given context only if
it is an ancestor of, or equal to, that context's current branch).

A malicious or compromised remote peer delegated a sub-task therefore
had two ways to break that isolation and inject its response into an
unrelated sibling sub-agent's LLM context:
  - setting adk_branch to a shared ancestor branch (e.g. the parent
    coordinator's branch instead of its own), or
  - omitting adk_branch entirely, which the filter treats as "always
    visible, in every branch".

This is the same class of bug fixed in #596 for actions.transferToAgent
(peer-controlled metadata able to corrupt local orchestrator state), on
a field that fix's allowlist didn't cover.

Fix: stop restoring `branch` in createAdkEventFromMetadata at all, and
thread it as an explicit parameter through toAdkEvent and its internal
per-event-type helpers instead, mirroring how `author` is already
force-set by the caller rather than trusted from peer metadata. The one
caller, A2ARemoteAgent.runAsyncImpl, now passes its own
InvocationContext.branch.
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.

1 participant