git_admin/obsidian-vault
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
122872a677c2a784b24b8ce00985bfcda62e1c65
obsidian-vault/personal/plans/runtime-pr5-toolset-dedup-review-fixes.md
T

15 KiB
Executable File
Raw Blame History

Создан 2026-09-28. Статус: 📋 ПЛАН, ЖДЁТ ОДОБРЕНИЯ ALEX. Ветка: fix/hosted-toolsets-duplicate-endpoint (@ b01bdc6, 2 файла, +263) PR normahq/runtime#5 — OPEN, CHANGES_REQUESTED от metalagman (2026-09-28T09:04:10Z) Репо: ~/Developer/runtime-local. Связано: personal/plans/runtime-pr4-openai-toolcalls-fixes.

0. Что говорит ревью (дословно, 3 пункта)

# Severity Суть
1 P1 — blocking mcpEndpointKey выбрасывает все query-параметры, а группировка игнорирует Headers. И query, и headers могут выбирать тенанта / авторизационный контекст / режим сервера. /mcp?tenant=a и /mcp?tenant=b схлопываются в один произвольный конфиг → доступ к чужому tool-set / пересечение границы доступа. Ревьюер: «keep generic endpoint identity exact» и «resolve the Balda-specific duplicate in the Balda composition layer, or introduce explicit alias/replacement metadata».
2 P2 — test gap selectScopedOnly повторяет алгоритм продакшена внутри теста и вычисляет ожидаемого победителя сам. Не проверяет, что реально удержанный toolset/транспорт — именно scoped URL. Ассерт вакуумный.
3 P2 — test gap Локальный интерфейс ProcessRequest(ctx any, req any) error — Go требует точных типов параметров, поэтому реальный метод ProcessRequest(agent.Context, *model.LLMRequest) error всё равно не удовлетворит интерфейс. Тест не может поймать регрессию.

Validation ревьюера: go test -race ./..., go vet ./..., go tool golangci-lint run ./... — проходят. GitHub currently reports no check runs/statuses (тот же fork-блокер, что в PR #4).

1. Подтверждение фактами

P1 — ключ стирает query и не смотрит Headers

agentfactory/agentfactory.go (b01bdc6), функция mcpEndpointKey:

u.RawQuery = ""
u.Fragment = ""
return u.String()

→ http://h:1/mcp/balda?ctx=abc123 и http://h:1/mcp/balda?tenant=b дают один ключ. Headers в ключе не участвуют вообще: MCPServerConfig.Headers есть (toRuntimeMCPServers его клонирует, httpClientWithHeaders его применяет) — значит разные Authorization на одном URL сейчас считаются «тем же сервером». Это реальная дыра, ревью право.

Побочный эффект mcpURLIsScoped: «scoped» = «есть хоть какой-то query». Это тоже выводит семантику из формы хоста/пути/строки — ровно то, против чего ревьюер.

P2-1 — тест дублирует прод

duplicate_toolset_test.go:49,57-94: selectScopedOnly — копия цикла hostedToolsets (keeperOf/skip/mcpURLIsScoped). Тест проверяет копию алгоритма, а не результат работы hostedToolsets (который вернул непрозрачные tool.Toolset; тест лишь считает len(toolsets)). Замечание верное: если поменять приоритет в проде и в копии синхронно — тест останется зелёным.

P2-2 — вакуумный ассерт

type requestProcessor interface { ProcessRequest(ctx any, req any) error }

Продакшн-сигнатура (подтверждено в модульном кэше adk/v2@v2.4.0/internal/toolinternal/tool.go:41 и tool/tool.go:192):

ProcessRequest(ctx agent.Context, req *model.LLMRequest) error

any ≠ agent.Context → метод не реализует интерфейс ни в каком случае. if _, ok := ...; ok всегда false, Fatalf недостижим. Вакуумный.

2. План правок

Фикс 1 (P1) — точная идентичность на generic-слое + явный алиас на слое Balda

Разделяем «когда схлопывать» и «какой именно дубль схлопывать».

(a) agentfactory: убрать умозаключения из хоста/пути.

  • Удалить mcpEndpointKey (стирание query) и mcpURLIsScoped.
  • Ввести явное объявление эквивалентности, а не вывод из формы URL. Два кандидата:
    • MCPServerConfig.Aliases []string — «id этого конфига дублирует id X» (аддитивно к agentconfig; правка публичного API → документировать в README.md по правилу AGENTS.md);
    • либо поле DedupKey string — «схлопывать только конфиги с одинаковым непустым ключом».
  • Рекомендация: DedupKey string (omitempty). Почему: не требует знания id-ов, не создаёт транзитивных связей, неинвазивно для остальных потребителей (пусто → не схлопывается никогда). Generic-слой больше ничего не выводит — кто задал ключ, тот и отвечает за смысл.
  • Схлопывание: только key != "" && key == key → детерминированный победитель (см. ниже).

(b) balda (composition layer) — объявить дубль явно. В internal/apps/balda/agent/runtime_manager.go:344 (место registry.Set(binding.ID, binding.Config)) проставлять DedupKey = endpoint без query, но только для связки bundledBaldaServerID → session-scoped binding. Это то самое «resolve in the Balda composition layer», чего просит ревьюер. Разные ?tenant=/Authorization в общем слое остаются разными.

(c) Детерминированный победитель — без «scoped эвристики». Вместо mcpURLIsScoped (снова форма URL) — при DedupKey-группе размером >1 выбирать лексикографически меньший id и логировать остальные как skipped. В Balda это даст balda (стабильно, не зависит от случайного токена). 🔴 Следствие для Balda: session-токен перестанет попадать в URL активного тулсета — надо подтвердить, что context-broker доставляет контекст иначе (см. §3, риск 1). Если нет — победителя задавать явным полем DedupPreferred bool, а не эвристикой.

Обоснование смены победителя (было: scoped, стало: явное/lexicographic): старый выбор «scoped» — это тоже неявный вывод, и ревьюер его отвергает по духу P1. Явное поле/детерминированный порядок — предсказуемо и тестируемо.

Фикс 2 (P2-1) — тест на фактический результат

Вместо копии алгоритма проверять то, что реально вернул hostedToolsets:

  • поднять два httptest-HTTP-сервера (MCP handler) с разными tool-листами по разным путям; настоящий hostedToolsets → вызвать Tools()/Name() полученных тулсетов и убедиться, что виден tool-set сервера с ожидаемого URL;
  • либо, если поднять MCP-сервер дорого, — прокинуть наружу фактически использованный endpoint через инъектируемый конструктор транспорта (var mcpTransportForConfig = ... + подмена в тесте), и ассертить Transport.Endpoint. Тогда тест падает, если прод удержал не тот конфиг.
  • 🔴 Предпочтение: инъекция конструктора — дешевле, детерминированнее и не тянет сеть. Реализовать как тестовый хук в agentfactory (тонкая обёртка), без изменения публичного API.

Фикс 3 (P2-2) — точная сигнатура

Заменить вакуумный интерфейс:

import (
    "google.golang.org/adk/v2/agent"
    "google.golang.org/adk/v2/model"
)
type requestProcessor interface {
    ProcessRequest(ctx agent.Context, req *model.LLMRequest) error
}

Продакшн-типы (agent.Context, *model.LLMRequest) подтверждены в модульном кэше. Плюс позитивный контроль: если ассерт вакуумный, тест должен падать. Добавить в тот же тест проверку, что интерфейс реализуется заведомо подходящим типом (например, tool.SetRequestProcessor-совместимой обёрткой или *mcpTool из mcptoolset) — если сигнатура разъедется с продакшном, тест это покажет, а не будет молчать.

Тесты (контрактные)

Тест Что фиксирует
TestDistinctQueryParamsNeverCollapseWithoutAlias /mcp?tenant=a vs /mcp?tenant=b (без DedupKey) → 2 тулсета
TestDistinctHeadersNeverCollapseWithoutAlias один URL, разные Authorization → 2 тулсета
TestSameDedupKeyCollapses два конфига с одинаковым DedupKey → 1, и удержан детерминированный
TestEmptyDedupKeyNeverCollapses DedupKey:"" + одинаковые URL → 2 (generic-слой ничего не выводит)
TestHostedToolsetsKeepsRetainedEndpoint через хук транспорта: удержан именно ожидаемый endpoint/id (замена selectScopedOnly)
TestMcpToolsetRequestProcessorSignature точная сигнатура agent.Context/*model.LLMRequest + позитивный контроль

Гейты

gofmt -l agentfactory/ agentconfig/
go build ./...
go test ./... && go test -race ./...
go vet ./...
go tool golangci-lint run ./...

3. Риски / питфоллы

  1. 🔴 Смена победителя в Balda — поведенческий риск, не косметика. Сейчас в URL живёт ctx=<токен> (binding.Config.URL, ставит context-broker). Если удержать balda (без токена), session-memory/context может перестать доставляться. Перед реализацией (b/c) — проверить, как композируется binding.Config.URL и нужен ли токен в URL вообще. Развилка: (i) токен не нужен → lexicographic-победитель; (ii) токен нужен → явное DedupPreferred на scoped-конфиге. Решение за Alex.
  2. ⚠️ Правка agentconfig.MCPServerConfig = публичный API модуля. По AGENTS.md: сохранять публичные API, изменения документировать в README.md. DedupKey string — аддитивно, но документировать обязательно.
  3. ⚠️ Правки делать только дельта-апплаером (tools/run-project-deltas.sh, check → apply), не sed/in-place (правило Alex).
  4. 🔴 Ветка PR #5 ≠ текущая локальная ветка. Сейчас checked out feat/openai-tool-calls-v2 (PR #4). Нужен git switch fix/hosted-toolsets-duplicate-endpoint до правок; git status чистый (untracked tools/ — не коммитить).
  5. ⚠️ PR #4 и PR #5 оба открыты и оба форк-PR → CI не запускается без одобрения мейнтейнера (см. §0в в personal/plans/runtime-pr4-openai-toolcalls-fixes). Локальные гейты — эквивалент CI.
  6. ⚠️ Ревьюер — metalagman. Ревью опубликовано в PR (в отличие от PR #4, где работали по тексту Alex). Ответ ревьюеру писать в PR.

4. Порядок (после одобрения)

  1. ⛔ Ожидание решения Alex по §3.1 (нужен ли ctx-токен в URL удержанного тулсета)
  2. ⛔ Ожидание решения по §2-Фикс1(a): DedupKey vs Aliases
  3. git switch fix/hosted-toolsets-duplicate-endpoint; бэкап-коммит текущего состояния (проверить, что нет своих правок)
  4. agentconfig: добавить поле (+ документировать в README.md)
  5. agentfactory: убрать mcpEndpointKey/mcpURLIsScoped; схлопывание по явному ключу; детерминированный победитель; тестовый хук транспорта
  6. balda: проставить DedupKey в composition layer (runtime_manager.go:344)
  7. Переписать тесты: удалить selectScopedOnly, добавить 6 контрактных; фикс сигнатуры
  8. Гейты (§2) — все зелёные
  9. Коммит (Conventional Commits) → push → gh pr checks 5
  10. Ответ ревьюеру в PR + обновить Obsidian (этот файл + personal/tech/adk-hostedagent-openai-mcp)

Связанные заметки

Reference in new issue View Git Blame Copy Permalink