fix(custom-tools): custom HTTP tools now advertise their configured parameters to the LLM - #59
Open
VictorCano wants to merge 4 commits into
Conversation
…arameters to the LLM Both HTTP tool builders returned a `def http_tool(**kwargs)` closure. ADK derives a tool's FunctionDeclaration by walking `inspect.signature(func)` and it skips VAR_KEYWORD outright, so every custom HTTP tool was published with `declaration.parameters is None` — not one of its configured path, query or body parameters was visible to the model. The same signature locks a second door: `FunctionTool.run_async` filters the incoming arguments against it, and the only valid parameter name was `kwargs`. Even a forced tool call therefore delivered nothing to the request builder, so only the static `values` ever reached the endpoint. That is why a tool with a required `title` body param would happily POST an empty body. Both halves are fixed by attaching a real keyword-only `inspect.Signature` to the closure, derived from the tool's own configuration. A parameter is only mandatory when the configuration has no value for it, so tools that today rely entirely on their canned `values` keep firing with zero model arguments. An array parameter always declares an element type, because Gemini rejects an ARRAY schema with no `items`. A configured name that Python cannot declare, or that collides with a name ADK fills itself, costs that one parameter instead of the whole tool. The existing tests all invoked `built.func(...)` directly, which bypasses both broken links; the new ones go through `_get_declaration()` and `run_async()` and fail on the unfixed code. Continues EVO-2125 (evolution-foundation#34) on src/services/adk/custom_tools.py. Co-authored-by: Claude <noreply@anthropic.com>
…d malformed configs A configured parameter whose name is not a Python identifier — `user-id`, a keyword, or one of the names ADK fills itself — used to be left out of the published schema. For a required parameter that meant the model was never asked for it and the request went out without it. Such a parameter is now declared under a deterministic stand-in identifier and translated back to its configured name before the request is built, so the endpoint still sees the name it expects. Names that are already usable are untouched, and two names that sanitise alike no longer collapse into one. An array body marked required was always advertised as optional, so ADK fired the call with `[]` instead of asking for it; it now follows the same rule as an object body. A nested array declared ARRAY items with no items of their own, the very constraint Gemini rejects, and now names its innermost element type. Parameter configuration is unvalidated JSON, so a type could arrive as a list and a parameter name as a number. Either one raised, and inline http_tools are built in an unguarded loop, so a single bad row took the whole agent build with it. Unusable types now fall back to string and unusable names are skipped. Finally, the pass that injects the static default values read the raw defaults rather than the effective merge, so a value the model overrode travelled as the canned one in the query string and as the override in the body — one field, one request, two values. Both now carry the override. Co-authored-by: Claude <noreply@anthropic.com>
…realmente recebe Um parâmetro cujo nome configurado o Python não aceita — `user-id`, uma palavra reservada, o `tool_context` que o ADK preenche — é declarado sob um identificador substituto, e é esse nome que vai para o schema. A docstring gerada, porém, continuava listando o nome configurado: como o ADK descarta as descrições por parâmetro do schema, a docstring é o único canal onde o modelo lê o significado de cada campo, e ele lia um nome ali e outro no schema — convite a mandar o argumento errado, que o ADK então filtra em silêncio. A docstring passa a ser composta depois da assinatura, a partir do mapeamento de apelidos: lidera com o nome que o modelo pode usar e mantém o nome configurado entre colchetes, para quem escreveu a configuração reconhecer o próprio parâmetro. As linhas de documentação também indexavam `type` e `description` diretamente. Configuração de parâmetro é JSON não validado, então um corpo sem `description`, sem `type`, ou que nem é um dicionário, estourava ali — fora do trecho já endurecido contra exatamente isso. Como os http_tools inline são construídos em laço sem proteção, uma única linha incompleta derrubava o build do agente inteiro; agora ela é lida pelos mesmos coercers da assinatura. Vale também para uma lista de query params com elementos que não são texto. Co-authored-by: Claude <noreply@anthropic.com>
Reviewer's GuideCustom HTTP tools now expose configured dynamic parameters through real keyword-only signatures, allowing ADK to advertise and forward them while preserving static defaults. The shared implementation also handles arrays, invalid parameter names, malformed configuration, documentation consistency, and override semantics, with comprehensive runtime-path tests for both builders. Sequence diagram for dynamic custom HTTP tool argumentssequenceDiagram
participant LLM
participant FunctionTool
participant http_tool
participant HTTP_API
LLM->>FunctionTool: run_async(args)
FunctionTool->>FunctionTool: inspect.signature(http_tool)
FunctionTool->>http_tool: **kwargs with declared parameters
http_tool->>http_tool: apply alias mapping and merge values
http_tool->>HTTP_API: requests.request(...)
HTTP_API-->>http_tool: HTTP response
http_tool-->>FunctionTool: tool result
FunctionTool-->>LLM: response
Flow diagram for custom HTTP parameter publicationflowchart TD
A[HTTP tool configuration] --> B[apply_http_tool_signature]
B --> C{Parameter category}
C -->|path| D[Declare string parameter]
C -->|query scalar| E[Declare optional string parameter]
C -->|query list| F[Keep configured list literal]
C -->|body| G[Declare mapped JSON type]
C -->|array body| H[Declare typed array parameter]
D --> I[Set keyword-only __signature__]
E --> I
G --> I
H --> I
I --> J[ADK advertises properties and filters valid arguments]
J --> K[http_tool remaps aliases and merges model values]
F --> K
K --> L[Build path, query, and body request]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
VictorCano
marked this pull request as ready for review
August 31, 2026 01:50
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/services/adk/custom_tools.py" line_range="210" />
<code_context>
# Adds query parameters
for param, value in query_params.items():
if isinstance(value, list):
- param_docs.append(f"{param}: List[{', '.join(value)}]")
+ # The configured list is sent verbatim, and it is unvalidated
+ # JSON: joining it raw breaks the build on the first number.
</code_context>
<issue_to_address>
**issue (bug_risk):** A list-valued query parameter containing a non-string item still raises `TypeError` in `",".join(value)`, so the tool returns its generic execution error instead of sending the configured list. The new coercion only converts items while generating the docstring and does not fix request construction.
**Triggers:** When a configured query list contains an integer, boolean, null, or another non-string JSON value.
**Suggested fix:** Convert each item with `str(item)` before joining in the runtime query-building loops as well as in the docstring loops.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and model-supplied values will now reach arbitrary configured HTTP endpoints, including POST or other mutating requests, so a signature or aliasing mistake could send unintended data or trigger an unintended external action. Reverting prevents future calls but cannot undo requests that were already made.
Blocking findings: src/services/adk/custom_tools.py:210
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…esposta de fallback
O commit anterior desta branch ensinou o laço da docstring a converter cada
elemento antes de juntar, porque `", ".join(value)` estourava na construção
do agente no primeiro item que não fosse texto. A correção parou ali: o
caminho da requisição, nos dois builders, continuava com o `",".join(value)`
cru.
`query_params` é declarado como `Dict[str, Union[str, List[str]]]`, mas é
persistido em coluna JSON sem validação e a API em Go aceita
`map[string]interface{}` — uma lista com número, booleano ou null chega
mesmo aqui. O `TypeError` acontecia dentro da closure da tool, onde o
`except Exception` largo o engolia: a tool devolvia o `fallback_response`
configurado sem nunca ter feito a requisição, e o modelo recebia uma
mensagem de erro genérica no lugar da lista que estava configurada. Falha
silenciosa: nada no comportamento externo dizia que o problema era o tipo de
um item da lista.
Os dois laços de runtime passam a converter cada item, mantendo o separador
de cada lugar — `","` no fio, `", "` na docstring.
Co-authored-by: Claude <noreply@anthropic.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
2 participants
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.
Problema
Toda tool HTTP customizada é publicada para o LLM sem schema de entrada. O modelo nunca consegue passar argumento nenhum, então toda requisição sai com body vazio e sem valor dinâmico de path ou query. Só os
valuesestáticos configurados chegam na rede — o que deixa a funcionalidade inútil para qualquer dado dinâmico.Os dois builders definem a tool como uma closure
**kwargspura e a entregam direto aoFunctionTool, ajustando só__doc__e__name__. O ADK monta aFunctionDeclarationpercorrendoinspect.signature, e esse percurso aceita apenasPOSITIONAL_ONLY,POSITIONAL_OR_KEYWORDeKEYWORD_ONLY.**kwargséVAR_KEYWORD— descartado em silêncio, sem erro e sem warning.A trava é dupla. O
FunctionTool.run_asyncreusa a mesma assinatura para filtrar os argumentos que chegam:Para uma função só com
**kwargs, esse conjunto é literalmente{'kwargs'}. Nada é anunciado e nada seria aceito. Medido contra ogoogle-adk==1.19.0pinado emrequirements.txt:33:A docstring que lista os parâmetros é só documentação — o ADK não a converte em schema. É esse o sintoma: o agente conhece a tool e a chama vazia.
O defeito existe idêntico em
src/services/adk/custom_tools.pye emsrc/services/adk/tool_builder.py, e é o segundo que está ligado noLlmAgent(llm_agent_builder.py:472). Corrigir só um não mudaria nada em produção.Correção
Anexar uma
inspect.Signaturekeyword-only real à closure antes de embrulhá-la. Uma mudança resolve as duas metades:from_function_with_optionsrespeita__signature__, então as properties aparecem; e ovalid_paramsdorun_asyncpassa a conter os nomes reais, então os argumentos fluem para dentro do**kwargs. O corpo que monta a requisição não precisou de nenhuma alteração.O helper
apply_http_tool_signaturefica emcustom_tools.pyao lado destrip_modes_meta, que otool_builder.pyjá importa — sem módulo novo e sem segunda convenção.path_paramsstrvaluesjá respondequery_paramsescalarstrquery_paramslistabody_paramstype/element_typerequirede o nome não está emvaluesbody_type: array+array_paramrequireddeleReusa a tabela de tipos que já existe:
_map_json_type_to_pythonfoi promovido amap_json_type_to_python(único chamador atualizado) em vez de criar um terceiro mapeamento.Compatibilidade retroativa: um parâmetro que a configuração já responde é declarado opcional, então uma tool totalmente estática continua disparando com zero argumento do modelo e continua enviando seus valores fixos. Há teste dedicado a isso.
Continua o EVO-2125 (#34), que mexeu nos mesmos arquivos.
Os três commits
valuescru enquanto o body lia o merge. Parâmetro obrigatório cujo nome o Python não expressa (user-id) sendo descartado em silêncio e a tool disparando incompleta — agora vira alias, com o mapaalias -> nome reale remapeamento na entrada da closure.requiredignorado em body do tipo array. Array aninhado emitindoARRAYsemitems, que é exatamente a restrição do Gemini que o helper existe para satisfazer. Config malformada (type=['object'],array_param=123) abortando a construção do agente inteiro.user-idenquanto a property erauser_id— e como o ADK descarta descrição por parâmetro, a docstring é o único canal de descrição, então o modelo lia dois nomes diferentes. E os laços da docstring indexavamparam_config['type']e['description']direto, quebrando na mesma config não validada que o commit 2 tinha blindado um bloco adiante.Testes
A suíte existente não tinha como pegar isso: todo teste chama
built.func(**kwargs)direto, batendo na closure crua e pulando tanto aFunctionDeclarationquanto o filtro de argumentos. Otest_metadata_is_not_sent_in_the_bodychega a afirmar quetitle="hello"chega no body — provando que a closure funciona, enquanto o caminho real via ADK nunca conseguiria entregartitle.Os testes novos passam por
built._get_declaration()eawait built.run_async(args=..., tool_context=None), parametrizados sobre os dois builders.O último é o comando exato do job
unitem.github/workflows/ci.yml:33-34. Todo teste novo falha contra o código que ele protege — a única exceção deliberada é o guard de "nome comum nunca é reescrito", verde dos dois lados por construção.Cada commit é verde sozinho (45 → 67 → 81), verificado extraindo cada árvore com
git archive.Sem regressão de lint.
flake8nos quatro arquivos alterados: 11 achados antes, os mesmos 11 depois (mesmos códigos, só os números de linha deslocados).black --check: os dois arquivos que ele reformataria já reformatariam emorigin/develop, e a interseção entre os trechos do black e nossas linhas adicionadas é zero nos quatro arquivos. Maior linha adicionada: 87 caracteres, dentro doline-length = 88. Omake lintdo repo falha com 2236 achados emdevelop; não pioramos em um único.Smoke test numa tool real do
ToolBuildercom pathnote-id, querylang+tags: ['a', 1], bodytitle/user-id/broken: None:Todo nome documentado corresponde a uma property publicada.
Dois comportamentos fixados por sonda, não por suposição
declaration.parameters.requiredé sempreNoneno ADK 1.19.0 — o valor é calculado mas não sobrevive até o modelo. A obrigatoriedade é fixada pelo comportamento dorun_async: argumento obrigatório ausente não chega ao endpoint e retorna o erro de retry do ADK nomeando o parâmetro.None; orun_asyncrepassa só o que foi enviado, então oif param in all_valuesdo corpo continua correto.Mudança de comportamento a observar
Um
query_paramsescalar é documentado como descrição mas usado em runtime como fallback. Expô-lo como argumento opcional significa que o modelo pode passar um onde antes ia sempre o valor configurado. É a correção pretendida, mas é mudança real de tráfego para qualquer tool que tenha escrito esses escalares como valores e não como descrições — vale um smoke test numa tool de tenant existente antes do rollout.Notas de processo
CHANGELOG.mdnão foi tocado de propósito. O arquivo foi editado por 6 commits em toda a história do repo, todos de corte de release, e não existe seção[Unreleased]. Desde o último (c3cb21c), 58 commits entraram emdevelope nenhum o tocou. Adicionar entrada seria o desvio.@importantnão foram atualizados. A cláusula pede registrar quem alterou e quando, mas nenhum dos 40 commits recentes — nem nenhum commit desde a importação inicial — jamais atualizou um. Mexer tornaria este o único diff da história a fazê-lo. Faço com prazer se for a preferência de vocês.build-prse auto-pula aqui pela própria condição (docker-publish.yml:25,head.repo.full_name == github.repository), então a ausência do checkBuild PR imageé por desenho, não falha. Feliz em reabrir a partir de um branch interno se preferirem.Produzido com apoio de assistente de IA e revisado de forma independente por um segundo modelo; os commits carregam
Co-authored-by. Toda afirmação acima foi verificada executando código — os RED/GREEN são reproduzíveis a partir dos SHAs.Summary by Sourcery
Advertise custom HTTP tool parameters to the LLM and ensure supplied values are correctly forwarded to their endpoints.
Bug Fixes:
Enhancements:
Tests: