fix: tools api missmatch between acp and jaeger gateway#8608
Merged
Conversation
Signed-off-by: Nabil-Salah <nabil.salah203@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates how contextual (frontend-supplied) AG-UI tools are extracted from request metadata and adjusts contextual tool call streaming so the browser executes tools locally without the server emitting a tool “result” event.
Changes:
- Adjust contextual tool extraction to read from handler
**kwargsrather than afield_metawrapper. - Expand documentation around
_meta/kwargsbehavior and the AG-UI vs LLM-loop separation. - Stream tool call args to the browser (
raw_input) while suppressing result payloads on completion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/ai-sidecar/gemini/sidecar_helpers.py | Renames/reshapes contextual tool extraction to accept handler kwargs and documents ACP router behavior. |
| scripts/ai-sidecar/gemini/sidecar.py | Updates session initialization to pass kwargs into extraction; changes tool call streaming to send args and avoid sending tool results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+37
to
+48
| def _extract_contextual_tools(meta: Any) -> list[dict[str, Any]]: | ||
| """Pull AG-UI tools out of NewSessionRequest._meta. Returns an empty | ||
| list if the meta is absent, the namespaced key is missing, or the | ||
| payload is malformed — the gateway populates this only when the | ||
| frontend actually attached tools to the chat request.""" | ||
| if not isinstance(field_meta, dict): | ||
| frontend actually attached tools to the chat request. | ||
|
|
||
| The Python ACP router spreads ``_meta`` into the handler's ``**kwargs`` | ||
| by inner key, so callers pass the handler ``kwargs`` dict directly | ||
| rather than a separate ``field_meta`` argument.""" | ||
| if not isinstance(meta, dict): | ||
| return [] | ||
| payload = field_meta.get(CONTEXTUAL_TOOLS_META_KEY) | ||
| payload = meta.get(CONTEXTUAL_TOOLS_META_KEY) |
| NewSessionRequest._meta and stashes it per-session so the agentic loop | ||
| can merge those tools into the Gemini chat config. The Python ACP | ||
| router spreads ``_meta``'s inner keys into this handler's ``**kwargs``, | ||
| so ``kwargs`` itself is the meta dict to look up the namespaced key in. |
Comment on lines
198
to
+200
| """Dispatch a contextual (frontend-supplied) tool call back to the | ||
| gateway via the ACP extension method. The gateway either logs | ||
| (PR1 placeholder) or forwards to the AG-UI client (PR2+) and | ||
| returns the result, which we feed back into Gemini's chat. | ||
| gateway via the ACP extension method, fire-and-forget. | ||
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8608 +/- ##
=======================================
Coverage 96.59% 96.59%
=======================================
Files 334 334
Lines 17782 17789 +7
=======================================
+ Hits 17177 17184 +7
Misses 455 455
Partials 150 150
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CI Summary ReportMetrics Comparison✅ No significant metric changes Code Coverage✅ Coverage 97.3% (baseline 97.3%) ➡️ View CI run | View publish logs |
Signed-off-by: Nabil-Salah <nabil.salah203@gmail.com>
|
|
||
|
|
||
| def _extract_contextual_tools(field_meta: Any) -> list[dict[str, Any]]: | ||
| def _extract_contextual_tools(meta: Any) -> list[dict[str, Any]]: |
Comment on lines
206
to
+207
| """Dispatch a contextual (frontend-supplied) tool call back to the | ||
| gateway via the ACP extension method. The gateway either logs | ||
| (PR1 placeholder) or forwards to the AG-UI client (PR2+) and | ||
| returns the result, which we feed back into Gemini's chat. | ||
| gateway via the ACP extension method, fire-and-forget. |
Comment on lines
+423
to
+424
| for _, blank := range []string{" ", " \t ", "\n", "\r\n"} { | ||
| t.Run(strings.ReplaceAll(strings.ReplaceAll(blank, "\n", "\\n"), "\t", "\\t"), func(t *testing.T) { |
yurishkuro
reviewed
May 30, 2026
| // Claude Agent SDK, which routes every tool call through canUseTool / a | ||
| // session/request_permission RPC; the cancelled outcome aborted every call | ||
| // as "Tool use aborted". The Gemini sidecar never hit this because Gemini's | ||
| // tool path does not issue ACP permission requests. |
Member
There was a problem hiding this comment.
because Gemini's tool path does not issue ACP permission requests.
Why is that? Is it a bug in GEmini's SDK implementation of the ACP protocol?
Contributor
Author
There was a problem hiding this comment.
no, it's because gemini wasn't asking for permissions before running a tool or making an mcp call.
But now with this change, it'll work whether with acp agent that ask (like claude sdk one) and one that doesn't ask
yurishkuro
reviewed
May 30, 2026
| // we fall through to cancelled so a misconfigured agent cannot be coerced | ||
| // into a forbidden action. | ||
| func (*streamingClient) RequestPermission(_ context.Context, req acp.RequestPermissionRequest) (acp.RequestPermissionResponse, error) { | ||
| for _, opt := range req.Options { |
Member
There was a problem hiding this comment.
this permission request swaps always-cancel for always-allow, regardless of the tool name. Is that intentional?
Contributor
Author
There was a problem hiding this comment.
yes bacause currently we allow all tool calls there is no auth for the tool calls
yurishkuro
approved these changes
May 30, 2026
vic-comm
pushed a commit
to vic-comm/jaeger
that referenced
this pull request
Jun 11, 2026
…g#8608) ## Which problem is this PR solving? - jaegertracing#8607 ## Description of the changes Fixes two bugs in `scripts/ai-sidecar/gemini` that together made every frontend-supplied AG-UI tool a no-op (e.g. `ui_highlight_span` would appear in the chat but never actually highlight a span). **1. Read the contextual-tools snapshot from the right place in `new_session`.** The Python ACP router (`acp.router._make_func`) spreads `NewSessionRequest._meta`'s inner keys into the handler's `**kwargs` — it does **not** pass a `field_meta=` kwarg. The sidecar was reading `kwargs.get("field_meta")` and therefore always got `None`, dropping the gateway-supplied contextual tools and never advertising them to Gemini. Pass `kwargs` directly into `_extract_contextual_tools` so the namespaced `jaegertracing.io/contextual-tools` key is found where it actually lands. **2. Emit a correct AG-UI tool-call lifecycle for contextual tools in `_execute_contextual_tool`.** For client-side AG-UI tools the browser is what executes the side effect, so the gateway must stream `TOOL_CALL_START → TOOL_CALL_ARGS → TOOL_CALL_END` and must **not** emit `TOOL_CALL_RESULT` — assistant-ui treats a server-emitted result as "the tool already ran" and skips the local `execute()`. The previous code did the opposite of both halves: - `start_tool_call(...)` was called without `raw_input`, so the gateway's streaming client (`streaming_client.go:201-206`) never fired `TOOL_CALL_ARGS` — the browser received no arguments to act on. - `update_tool_call(..., raw_output={"content": response})` populated `RawOutput` with the gateway dispatcher's synthetic `{"acknowledged":true}` ack, which the streaming client (`streaming_client.go:215-220`) emitted as `TOOL_CALL_RESULT`. assistant-ui then marked the call complete without calling the local `execute()`. The fix: add `raw_input=args` to `start_tool_call`, and drop `content` / `raw_output` from `update_tool_call` so only `TOOL_CALL_END` fires on completion. The method still returns the dispatcher's ack so Gemini's agentic loop receives a function response and produces a final assistant text — preserving the fire-and-forget design, just keeping it on the LLM surface instead of leaking onto the AG-UI wire. The gateway dispatcher (`dispatcher.go`) is unchanged; its contract was already correct. Also tightened the supporting code: - Renamed the helper parameter `field_meta` → `meta` and expanded the docstring of `_extract_contextual_tools` to explain the router's spread behavior, so the next reader doesn't reintroduce the same lookup. - Corrected the misleading "parsed by the Python ACP runtime as ``field_meta``" comment in `new_session`. ## How was this change tested? - run pytest on the current implemented test flow ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully: `make lint test` ## AI Usage in this PR (choose one) See [AI Usage Policy](https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md#ai-usage-policy). - [ ] **None**: No AI tools were used in creating this PR - [ ] **Light**: AI provided minor assistance (formatting, simple suggestions) - [x] **Moderate**: AI helped with code generation or debugging specific parts - [ ] **Heavy**: AI generated most or all of the code changes --------- Signed-off-by: Nabil-Salah <nabil.salah203@gmail.com> Signed-off-by: Victor Chidera Obiezue <obiezuechidera@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Description of the changes
Fixes two bugs in
scripts/ai-sidecar/geminithat together made every frontend-supplied AG-UI tool a no-op (e.g.ui_highlight_spanwould appear in the chat but never actually highlight a span).1. Read the contextual-tools snapshot from the right place in
new_session.The Python ACP router (
acp.router._make_func) spreadsNewSessionRequest._meta's inner keys into the handler's**kwargs— it does not pass afield_meta=kwarg. The sidecar was readingkwargs.get("field_meta")and therefore always gotNone, dropping the gateway-supplied contextual tools and never advertising them to Gemini. Passkwargsdirectly into_extract_contextual_toolsso the namespacedjaegertracing.io/contextual-toolskey is found where it actually lands.2. Emit a correct AG-UI tool-call lifecycle for contextual tools in
_execute_contextual_tool.For client-side AG-UI tools the browser is what executes the side effect, so the gateway must stream
TOOL_CALL_START → TOOL_CALL_ARGS → TOOL_CALL_ENDand must not emitTOOL_CALL_RESULT— assistant-ui treats a server-emitted result as "the tool already ran" and skips the localexecute(). The previous code did the opposite of both halves:start_tool_call(...)was called withoutraw_input, so the gateway's streaming client (streaming_client.go:201-206) never firedTOOL_CALL_ARGS— the browser received no arguments to act on.update_tool_call(..., raw_output={"content": response})populatedRawOutputwith the gateway dispatcher's synthetic{"acknowledged":true}ack, which the streaming client (streaming_client.go:215-220) emitted asTOOL_CALL_RESULT. assistant-ui then marked the call complete without calling the localexecute().The fix: add
raw_input=argstostart_tool_call, and dropcontent/raw_outputfromupdate_tool_callso onlyTOOL_CALL_ENDfires on completion. The method still returns the dispatcher's ack so Gemini's agentic loop receives a function response and produces a final assistant text — preserving the fire-and-forget design, just keeping it on the LLM surface instead of leaking onto the AG-UI wire. The gateway dispatcher (dispatcher.go) is unchanged; its contract was already correct.Also tightened the supporting code:
field_meta→metaand expanded the docstring of_extract_contextual_toolsto explain the router's spread behavior, so the next reader doesn't reintroduce the same lookup.field_meta" comment innew_session.How was this change tested?
Checklist
make lint testAI Usage in this PR (choose one)
See AI Usage Policy.