feat(vault): agentes externos e tools/MCPs resolvem pelo cofre (EVO-2250) - #47
Merged
Merged
Conversation
…om fallback inline O builder de agente externo passa a resolver o segredo pelo cofre quando a integração carrega `credential_id`, e a manter o valor inline quando não carrega. Nenhuma instalação precisa migrar nada para esta mudança entrar: o caminho antigo continua intacto até a 2.7 remover o fallback. A resolução aqui é POR ID, deliberadamente. Precedência entre escopos tem um dono só, o resolvedor do CRM (story 2.2); percorrer cadeia no runtime criaria uma segunda verdade sobre qual credencial vence. O que o processor faz é buscar valor, não decidir hierarquia. O mapa de campo por provedor fica num lugar só, porque errar um nome aqui produz auth vazia em silêncio, não erro: dify, flowise e openai leem `apiKey`; n8n lê o par `basicAuthUser`/`basicAuthPass`, que vem do envelope composto do cofre e precisa ser traduzido; typebot não tem credencial nenhuma e está registrado com tupla vazia, para a ausência ler como decisão e não como esquecimento que alguém tenta "consertar" depois. Ordem de precedência: referência resolvida vence, senão inline, senão erro explícito. O erro só dispara quando o usuário PEDIU o cofre e não há inline para usar: mandar chave vazia ao provedor falharia mais longe e com mensagem pior. A razão da falha viaja na mensagem, porque "é oauth" e "não existe" pedem correções diferentes de quem configurou o agente. O vault lê a tabela pela sessão que o builder já tem, com query parametrizada e escopada a uma linha. Não abre conexão própria: o padrão de psycopg2 cru que existe nas tools de Calendar ignora ORM e tenant, e a própria story manda não copiá-lo. Prova negativa verificada: removendo o fallback inline, três testes falham (referência não resolvida, referência oauth e valor indecifrável). Nota: o módulo é livre de imports pesados de propósito, e o teste o importa por caminho, porque `src.services.__init__` puxa a stack ADK inteira e o ambiente local não tem essas dependências instaladas. Refs EVO-2250 (story 2.3)
…amento do os.environ Custom tools, MCPs remotos, MCPs oficiais e o Knowledge Nexus passam a resolver seus segredos pelo cofre quando há referência, e a manter o valor inline quando não há. Nada quebra antes da migração 2.6. A referência é um MAPA (nome do header ou da env var → id da credencial): a regra do épico é uma credencial por segredo, então uma tool com dois headers de auth referencia duas credenciais. O Nexus tem um segredo só, então o credential_id escalar do diálogo é adaptado para o mesmo formato de mapa. Os DOIS caminhos de injeção de header foram tratados (custom_tools.py e tool_builder.py). Endereçar só um deixaria o hardening pela metade, que é o alerta explícito da story. DECISÃO REGISTRADA (o ou/ou que a story obriga a resolver): o vazamento do os.environ em mcp_context.py foi CORRIGIDO aqui, não adiado. Cada env var de MCP era gravada no os.environ do processo do processor além de ser passada ao filho pelo env= da linha seguinte. A escrita era redundante e nunca desfeita: token de um agente vazava para todo subprocesso MCP posterior e, no enterprise, entre tenants. Como este é exatamente o ponto por onde os segredos resolvidos pelo cofre passam agora, deixá-lo desfaria o ganho do cofre uma linha depois. Junto: os dois logs que despejavam o mapa de headers inteiro antes do helper de máscara passam a registrar só os NOMES. Bearer token não volta a aparecer em log. Provas negativas verificadas: reintroduzir a escrita no os.environ quebra test_mcp_context_no_longer_pollutes_os_environ, e remover o fallback inline quebra os testes de referência não resolvida. Nota de ambiente: os testes rodam isolados (importação por caminho), porque src.services.__init__ puxa a stack ADK e as dependências não estão instaladas aqui. A suíte do repo já não coletava antes deste trabalho. Pareia com o commit do core (schema credential_refs + redação de header) e o do CRM (bot de canal) da mesma story. Refs EVO-2250 (story 2.4)
Reviewer's GuideImplements vault-based resolution for external agent integrations, custom tools, and MCP headers/env, with graceful fallback to inline secrets, adds schema to store vault references, and hardens MCP logging and environment handling to avoid secret leakage and cross-tenant contamination. Sequence diagram for vault-based credential resolution with inline fallbacksequenceDiagram
participant ExternalAgentBuilder
participant DatabaseCredentialVault
participant decrypt_api_key
ExternalAgentBuilder->>ExternalAgentBuilder: apply_vault_credential(provider, integration_config, vault, decrypt_api_key)
alt credential_id present in integration_config
ExternalAgentBuilder->>DatabaseCredentialVault: fetch_active(credential_id)
DatabaseCredentialVault-->>ExternalAgentBuilder: credential_row or None
alt credential_row exists and kind != KIND_OAUTH
ExternalAgentBuilder->>decrypt_api_key: decrypt_api_key(encrypted_value)
decrypt_api_key-->>ExternalAgentBuilder: plaintext_secret or None
alt plaintext_secret available
ExternalAgentBuilder->>ExternalAgentBuilder: _merge_secret(config, provider, secret_fields, plaintext_secret, credential_id)
ExternalAgentBuilder-->>ExternalAgentBuilder: config with vault secret applied
else plaintext_secret missing
alt inline secret present in config
ExternalAgentBuilder-->>ExternalAgentBuilder: keep inline secret and log warning
else no inline secret
ExternalAgentBuilder-->>ExternalAgentBuilder: raise ValueError (no usable secret)
end
end
else credential_row missing or kind == KIND_OAUTH
alt inline secret present in config
ExternalAgentBuilder-->>ExternalAgentBuilder: keep inline secret and log warning
else no inline secret
ExternalAgentBuilder-->>ExternalAgentBuilder: raise ValueError (no usable secret)
end
end
else no credential_id
ExternalAgentBuilder-->>ExternalAgentBuilder: return original config (inline-only)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 3 issues, and left some high level feedback:
- The
_apply_vault_refshelper is duplicated in bothtool_builder.pyandcustom_tools.py; consider centralizing this logic (e.g., inintegration_credentials.pyor a shared helper) to keep the vault resolution behavior consistent and easier to maintain. - In
DatabaseCredentialVault.fetch_active, you’re using raw SQL with a tuple index; if possible, map the result to named fields or a lightweight dataclass to avoid magic indices and make future changes to the credential schema less error‑prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_apply_vault_refs` helper is duplicated in both `tool_builder.py` and `custom_tools.py`; consider centralizing this logic (e.g., in `integration_credentials.py` or a shared helper) to keep the vault resolution behavior consistent and easier to maintain.
- In `DatabaseCredentialVault.fetch_active`, you’re using raw SQL with a tuple index; if possible, map the result to named fields or a lightweight dataclass to avoid magic indices and make future changes to the credential schema less error‑prone.
## Individual Comments
### Comment 1
<location path="src/services/adk/tool_builder.py" line_range="51-60" />
<code_context>
+def _apply_vault_refs(tool_config, headers, _db=None):
</code_context>
<issue_to_address>
**suggestion:** Consider deduplicating _apply_vault_refs and clarifying the unused _db parameter
This helper is duplicated in both `tool_builder` and `custom_tools`, and the `_db` parameter is unused because the function always creates its own `SessionLocal`. Consider either removing `_db` from the signature or actually using the injected session, and moving this helper to a shared module (e.g. `integration_credentials`) so both call sites share a single implementation and session-handling strategy.
</issue_to_address>
### Comment 2
<location path="src/services/adk/custom_tools.py" line_range="65-74" />
<code_context>
+def _apply_vault_refs(tool_config, headers, _db=None):
</code_context>
<issue_to_address>
**suggestion:** Behaviour around unresolvable credential_refs can be surprising vs. docstring
`resolve_credential_refs` can raise `ValueError` when a credential ref is unresolvable and there is no inline header to fall back to, which changes behaviour from “no header” to “hard error” and doesn’t quite match the docstring. If this stricter behaviour is intended, please update the docstring to say something like “falls back when an inline value exists; otherwise raises” so it’s clear failures are not always non-fatal.
Suggested implementation:
```python
def _apply_vault_refs(tool_config, headers, _db=None):
"""Resolves headers that point at the credential vault.
Opens its own short-lived session: neither tool builder carries one, and
threading a session through every call site would be a wider change than
this story needs. Failures in resolving credential refs fall back to the
inline headers when an inline value exists; otherwise they may raise,
so a vault outage generally degrades to today's behaviour instead of
silently breaking the tool.
"""
credential_refs = tool_config.get("credential_refs") or {}
if not credential_refs:
return headers
```
If there are other references to this behaviour (e.g. in `resolve_credential_refs`'s own docstring or higher-level documentation/comments), they should be updated similarly to mention that unresolvable credential refs without inline headers can result in a raised error rather than a silent fallback.
</issue_to_address>
### Comment 3
<location path="src/services/adk/integration_credentials.py" line_range="27" />
<code_context>
+logger = logging.getLogger(__name__)
+
+KIND_OAUTH = "oauth"
+VALUE_FORMAT_COMPOSITE = "composite"
+
+# The composite envelope of story 2.1 keys the secret half as `password`.
</code_context>
<issue_to_address>
**nitpick:** Unused VALUE_FORMAT_COMPOSITE constant could either be wired in or removed
`VALUE_FORMAT_COMPOSITE` is defined but never used. If composite JSON handling is meant to be conditional on `value_format == VALUE_FORMAT_COMPOSITE`, add that check before parsing; otherwise, remove the constant to avoid implying multiple formats are currently supported.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+60
| def _apply_vault_refs(tool_config, headers, _db=None): | ||
| """Resolves headers that point at the credential vault. | ||
|
|
||
| Opens its own short-lived session: neither tool builder carries one, and | ||
| threading a session through every call site would be a wider change than | ||
| this story needs. Any failure falls back to the inline headers, so a vault | ||
| outage degrades to today's behaviour instead of breaking the tool. | ||
| """ | ||
| credential_refs = tool_config.get("credential_refs") or {} | ||
| if not credential_refs: |
There was a problem hiding this comment.
suggestion: Consider deduplicating _apply_vault_refs and clarifying the unused _db parameter
This helper is duplicated in both tool_builder and custom_tools, and the _db parameter is unused because the function always creates its own SessionLocal. Consider either removing _db from the signature or actually using the injected session, and moving this helper to a shared module (e.g. integration_credentials) so both call sites share a single implementation and session-handling strategy.
Comment on lines
+65
to
+74
| def _apply_vault_refs(tool_config, headers, _db=None): | ||
| """Resolves headers that point at the credential vault. | ||
|
|
||
| Opens its own short-lived session: neither tool builder carries one, and | ||
| threading a session through every call site would be a wider change than | ||
| this story needs. Any failure falls back to the inline headers, so a vault | ||
| outage degrades to today's behaviour instead of breaking the tool. | ||
| """ | ||
| credential_refs = tool_config.get("credential_refs") or {} | ||
| if not credential_refs: |
There was a problem hiding this comment.
suggestion: Behaviour around unresolvable credential_refs can be surprising vs. docstring
resolve_credential_refs can raise ValueError when a credential ref is unresolvable and there is no inline header to fall back to, which changes behaviour from “no header” to “hard error” and doesn’t quite match the docstring. If this stricter behaviour is intended, please update the docstring to say something like “falls back when an inline value exists; otherwise raises” so it’s clear failures are not always non-fatal.
Suggested implementation:
def _apply_vault_refs(tool_config, headers, _db=None):
"""Resolves headers that point at the credential vault.
Opens its own short-lived session: neither tool builder carries one, and
threading a session through every call site would be a wider change than
this story needs. Failures in resolving credential refs fall back to the
inline headers when an inline value exists; otherwise they may raise,
so a vault outage generally degrades to today's behaviour instead of
silently breaking the tool.
"""
credential_refs = tool_config.get("credential_refs") or {}
if not credential_refs:
return headersIf there are other references to this behaviour (e.g. in resolve_credential_refs's own docstring or higher-level documentation/comments), they should be updated similarly to mention that unresolvable credential refs without inline headers can result in a raised error rather than a silent fallback.
| logger = logging.getLogger(__name__) | ||
|
|
||
| KIND_OAUTH = "oauth" | ||
| VALUE_FORMAT_COMPOSITE = "composite" |
There was a problem hiding this comment.
nitpick: Unused VALUE_FORMAT_COMPOSITE constant could either be wired in or removed
VALUE_FORMAT_COMPOSITE is defined but never used. If composite JSON handling is meant to be conditional on value_format == VALUE_FORMAT_COMPOSITE, add that check before parsing; otherwise, remove the constant to avoid implying multiple formats are currently supported.
Corrige os bloqueadores 1 e 2 da reprovação (review de 2026-07-29) e o achado 13.
BLOQUEADOR 1 — a resolução de header de MCP remoto era CÓDIGO MORTO.
`_resolve_mcp_headers` existia, tinha teste unitário e ZERO chamadores: um
`git grep` devolvia só a definição. O ponto real de montagem seguia com
`"headers": custom_server.headers or {}` cru, então um MCP remoto configurado
com `credential_refs` e sem header inline saía SEM AUTENTICAÇÃO. Agora a
montagem chama o resolvedor.
BLOQUEADOR 2 — env var de MCP oficial nunca resolvia pelo cofre. O ponto de
montagem fazia `update(server.get("envs", {}))` verbatim. Entra
`_resolve_mcp_envs`, chamado no mesmo lugar, lendo `credential_refs` como mapa
(nome da env var → id da credencial) e reusando o mesmo `resolve_credential_refs`
de tools e MCPs remotos.
⚠️ A LEITURA está ligada, mas a AC7 NÃO fecha só com isso: nada persiste
`credential_refs` na entrada de MCP do agente (ponta de escrita, no front). Até
existir, todo install segue no caminho verbatim. O contrato foi enviado ao
Reviewer; o estado real está registrado no docstring, não escondido.
ACHADO 13 — valor de header ia para o log. Os dois sites de `mcp_context.py`
mascaravam SÓ `authorization`, então `X-API-Key` e qualquer header de auth
customizado saíam em claro. A máscara passa a ser derivada de uma ALLOWLIST de
nomes seguros, espelhando o `safeHeaderNames` do secretmerge no Go: denylist de
nomes que "parecem auth" deixa passar `X-Tenant-Auth` e afins.
OS TESTES SÃO DE CAMINHO, não da função. Foi exatamente teste de função isolada
que deixou o defeito passar: ele passava enquanto ninguém chamava. Os novos
afirmam sobre o ponto de montagem e falham se a chamada for removida — provado
removendo-a.
Prova de chamador (o que a reprovação pediu):
src/services/adk/mcp_service.py:754 "headers": _resolve_mcp_headers(...)
src/services/adk/mcp_service.py:393 ...update(_resolve_mcp_envs(server, db))
Refs EVO-2250
Achado pelo Reviewer ao revisar minha própria correção do bloqueador 2, e ele
está certo: eu tinha ligado a chamada no lugar certo lendo a chave ERRADA, então
a resolução continuava inerte mesmo com a chamada no ponto de montagem. Conferi
o pipeline inteiro antes de aceitar:
1. FRONT grava `environments` (MCPConfigDialog é o único escritor). `envs` tem
zero ocorrências reais no front.
2. CORE valida por `environments` (config_processor.go:266, com erro
"server environments must be a dictionary") e REESCREVE a entrada persistida
com exatamente {id, environments, tools} (:278-282).
Consequência dupla que o Reviewer nomeou: (a) o guard em `server.get("envs")`
nunca era verdadeiro para agente configurado pela tela, então
`_resolve_mcp_envs` não rodava; (b) mesmo que rodasse, a allowlist do core
descartaria `credential_refs` antes de chegar ao processor — essa metade é do
Reviewer e ele está fazendo nesta rodada.
Meu lado, os três pontos: o guard e o resolvedor passam a ler `environments`,
tolerando `envs` para entradas escritas antes da reconciliação, e o
`MCPServerConfig` do schema ganha `environments` e `credential_refs` (declarava
só `envs`, mesma divergência).
O teste novo afirma sobre a CHAVE QUE O PIPELINE ESCREVE, não sobre a existência
da chamada: uma chamada no lugar certo com a chave errada passava no teste
anterior. Prova negativa: voltar para `envs` quebra o teste.
Ressalva de escopo mantida: a coluna `evo_core_mcp_servers.environments` do
CATÁLOGO continua sendo schema de chaves obrigatórias, não valor. O cofre entra
só na ponta do agente.
Refs EVO-2250
O ajuste de 2 linhas que o Reviewer pediu já estava no commit e33de81 (guard e resolvedor lendo `environments`, com tolerância a `envs` para config antiga, e o `MCPServerConfig` do schema com os dois campos). O que faltava era guardar a cadeia INTEIRA agora que as três pontas existem. Cadeia conferida no código, não suposta: 1. front 9f63077 — grava credential_refs junto de environments 2. core 62830b7 — deixa credential_refs atravessar o processamento 3. processor — resolve na montagem (mcp_service.py:399) O teste novo afirma sobre a LINHA QUE LÊ os valores, não sobre a existência da função: a primeira versão que escrevi passava mesmo com o resolvedor voltando a ler só `envs`, ou seja, não protegia nada. Corrigi e a prova negativa agora falha de verdade quando a chave regride — que é o defeito exato desta rodada. Observação sobre o ramo OAuth do core: ele monta a entrada com {id, environments, tools} e descarta credential_refs. Conferi a lista de provedores (github, notion, stripe e afins) e isso está CERTO por desenho: são conexões OAuth, cujo token vive no store dono e entra no cofre por referência (2.5), não como valor. Não é uma segunda allowlist esquecida. Refs EVO-2250
O MCPConfigDialog passou a persistir credential_refs e o core carrega o mapa pelo processMCPServers. O aviso deixava o próximo leitor concluir que a AC7 seguia aberta.
Sai o que pertencia ao relatório de review e não ao fonte: reconstrução do defeito, citação de card e severidade, referência a quem revisou. Fica o porquê não-óbvio — o mapa em vez de escalar porque uma credencial é um segredo, o allowlist de header no log, e o `environments` vs `envs` que deixava a resolução inerte. Só comentário e docstring: a AST dos 11 arquivos .py, com docstring removida, é idêntica à de antes. Os 32 testes seguem verdes.
gomessguii
approved these changes
Jul 30, 2026
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.
Resolução da credencial da plataforma pelo cofre com fallback inline, headers/env pelo cofre, e o fim do vazamento cross-tenant do os.environ em mcp_context. 23 testes isolados (a suíte geral do repo já não rodava na baseline: venv quebrado). Ordem de merge da feature: auth → core → CRM + processor → vendor/crm (front) → frontend (bump) → superprojeto.
Guia de validação tela a tela:
_evo-output/planning-artifacts/config-ia-unificada/guia-teste-dev-evo-2250.md(no superprojeto). Card: EVO-2250.Summary by Sourcery
Route external agent, custom tool, and MCP credentials through the shared integration credential vault while tightening MCP logging and environment handling.
New Features:
Bug Fixes:
Enhancements:
Tests: