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

11 KiB
Executable File
Raw Blame History

Создан 2026-09-28. Статус: ✅ РЕАЛИЗОВАНО, ЗАПУШЕНО. Ре-ревью ожидается. Голова PR: d4ab923 (4 файла, +411/−1), ветка fix/hosted-toolsets-duplicate-endpoint. PR normahq/runtime#5 — OPEN. Ответ ревьюеру опубликован: https://github.com/normahq/runtime/pull/5#issuecomment-5867224210 Репо: ~/Developer/runtime-local. Связано: personal/plans/runtime-pr4-openai-toolcalls-fixes.

0. ИТОГ — что сделано

Коммит d4ab923 fix(agentfactory): dedup MCP toolsets by explicit key.

Гейты локально — все зелёные:

gofmt -l agentfactory/ agentconfig/   → чисто
go build ./...                        → OK
go vet ./...                          → OK
go test ./...                         → все 10 пакетов ok
go test -race ./...                   → все ok
go tool golangci-lint run ./...       → 0 issues
# Дефект ревью Фикс
1 P1 ключ стирал query и игнорировал Headers → слияние разных тенантов/авторизаций mcpEndpointKey + mcpURLIsScoped удалены. Инференс из URL убран. Схлопывание только по явному непустому DedupKey; пусто → никогда не схлопывается
2 P2-1 selectScopedOnly копировал алгоритм прода удалён. Тесты пишут endpoint каждого конфига, реально дошедшего до mcpTransportFactory (инъектируемая переменная), и ассертят на нём
3 P2-2 ProcessRequest(ctx any, req any) — вакуумный Точная продакшн-сигнатура (agent.Context, *model.LLMRequest) + позитивный контроль TestRequestProcessorInterfaceIsSatisfiable

Новые поля agentconfig.MCPServerConfig (аддитивно, по умолчанию выключено):

  • DedupKey — явный ключ эквивалентности (opaque, задаёт вызывающий);
  • DedupPreferred — кто выживает в группе, когда записи не взаимозаменяемы.

Приоритет победителя: помеченный DedupPreferred > лексикографически меньший id (детерминированно, не зависит от порядка map).

Balda — composition layer (как просил ревьюер):

  • sessionmemorymcp.BundledDedupKey = "balda-bundled-mcp" — общий ключ;
  • internal/apps/balda/internalmcp/manager.go:213 — bundled-запись получает DedupKey;
  • internal/apps/balda/sessionapp/module.go:130 — scoped-binding получает DedupKey + DedupPreferred: true.

Тесты (8 в agentfactory): TestHostedToolsetsCollapsesScopedEndpointDuplicate, TestDistinctQueryParamsNeverCollapse, TestDistinctHeadersNeverCollapse, TestSameDedupKeyCollapsesThreeEntries, TestDedupPreferredDecidesPlaceholderWinner, TestDedupWithoutPreferenceIsLexicographicallySmallest, TestEmptyDedupKeyNeverCollapses, TestHostedToolsetsKeepsDistinctEndpoints, TestMcpToolsetRequestProcessorSignature, TestRequestProcessorInterfaceIsSatisfiable.

🔴 Негативные контроли (доказано, что тесты не вакуумные)

Проверено скриптом /tmp/negctl.sh:

  1. Вернул инференс из URL (стрип query) → TestDistinctQueryParamsNeverCollapse FAIL: expected 2 toolsets for distinct tenants, got 1. Лог: skipping duplicate toolset "tenant-b": dedup key "http://h:1/mcp" already served by "tenant-a".
  2. Вернул ctx any, req any → build failed: *stubRequestProcessor does not implement requestProcessor (wrong type for method ProcessRequest).
  3. Файлы восстановлены байт-в-байт (diff чистый), ре-верификация зелёная.

1. 🔴 КЛЮЧЕВОЕ ОТКРЫТИЕ: scoped-запись ОБЯЗАНА выживать

Изначально планировался лексикографический победитель (balda < balda-session-memory-*), т.е. plain запись. При проверке кода брокера выяснилось — это сломало бы session-memory:

internal/apps/balda/sessionmemorymcp/broker.go:

  • contextQueryParameter = "balda_context" — токен читается из query (:124);
  • Wrap (:118-150): без токена запрос идёт дальше без инъекции HeaderSessionLocator/ HeaderSessionID/HeaderSessionBinding;
  • комментарий в коде: «session-memory calls still fail closed in HeaderSessionResolver».

→ Токен в URL функциональный, а не косметический. Поэтому:

  1. нужен был DedupPreferred (порядок id не подходит);
  2. заметка в тесте исправлена: ?ctx= → реальный ?balda_context=<token>.

⚠️ Вывод на будущее: старая эвристика mcpURLIsScoped («есть query → scoped») давала правильного победителя случайно. Замена на «лексикографически меньший» без DedupPreferred была бы регрессией. Проверка кода, а не симметрия с ревью, её поймала.

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

Ревьюер metalagman, CHANGES_REQUESTED, 2026-09-28T09:04:10Z (опубликовано в PR).

# Severity Суть
1 P1 — blocking mcpEndpointKey выбрасывает все query-параметры, группировка игнорирует Headers. /mcp?tenant=a vs ?tenant=b, или один URL с разными Authorization, схлопываются в один произвольный конфиг → доступ к чужому 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 selectScopedOnly повторяет алгоритм продакшена и вычисляет победителя сам — не проверяет фактически удержанный toolset
3 P2 ProcessRequest(ctx any, req any) — Go требует точных типов, реальный метод интерфейс не удовлетворит; ассерт не может сработать

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

3. ⛔ CI: внешний блокер (не наш дефект)

gh pr checks 5 → no checks reported on the 'fix/hosted-toolsets-duplicate-endpoint' branch. Причина та же, что в PR #4 (§0в в personal/plans/runtime-pr4-openai-toolcalls-fixes): fork-PR (mallexxx/runtime → normahq/runtime), workflows ждут одобрения мейнтейнера. Починить не можем. Локальные гейты эквивалентны CI (test.yml + lint.yml).

4. Питфоллы, найденные в работе

  1. 🔴 write_file с относительным путём пишет НЕ в vault. Первый write_file с путём personal/plans/runtime-pr5-...md создал файл в /Users/admin/Developer/balda/personal/.... Для vault — только mcp_obsidian_write_note (vault root /Users/admin/obsidian).
  2. ⚠️ rm -rf в терминале требует подтверждения — таймаут апрува заблокировал команду. Лишнее дерево /Users/admin/Developer/balda/personal/ не удалено (см. §6).
  3. ⚠️ LSP-диагностика после patch бывает устаревшей — показывала undefined: mcpEndpointKey после того, как go build уже был чист. Верить go build/go test, не LSP-снапшоту.
  4. ⚠️ Правки — только patch, не sed/in-place (правило Alex). Ни одного sed не использовано.
  5. ⚠️ Untracked tools/ в репо — не мой файл, не коммитил.

5. Осталось сделать

  1. ⏳ Ре-ревью metalagman — ждать.
  2. ⛔ Синхронизация копии рантайма в Balda (~/Developer/balda/runtime-local, HEAD 05342c5a, grep -c DedupKey agentconfig/config.go → 0). Правки Balda-слоя (§0) сейчас не компилируются против этой копии — ошибки unknown field DedupKey in struct literal of type agentconfig.MCPServerConfig. Это отдельная задача, как и в PR #4 (решать после мержа, чтобы копия не разъехалась снова). 🔴 Пока не синхронизировано — боевой стенд Балды фикс не получил.
  3. ⚠️ /tmp/negctl.sh, /tmp/control1.py, /tmp/control2.py, /tmp/af.bak, /tmp/dt.bak, /tmp/commitmsg.txt, /tmp/reply.md, /tmp/pr5_af.go, /tmp/pr5_test.go — временные.
  4. ⚠️ Проверить, не утекла ли ветка PR #4 (feat/openai-tool-calls-v2) — она тоже открыта.

6. Требует решения Alex

Удалить лишнее дерево, созданное ошибочным write_file (содержит только пустые plans/ + tech/, ничего ценного):

/Users/admin/Developer/balda/personal/

Удаление требует явного подтверждения — не трогал.

7. Контекст PR (исходная проблема)

hostedagent/agentfactory — ADK разворачивает каждый toolset в один плоский список инструментов, а упаковка не идемпотентна: два сервера с одинаковым именем инструмента аварийно завершают ход с duplicate tool: "balda.control.shutdown".

Balda регистрирует bundled MCP-сервер дважды: как "balda" и как session-scoped binding того же listener'а с токеном в query. Оба отдают один список инструментов → каждый tool-using ход падал. Исходный фикс (b01bdc6) схлопывал по host/path — что и открыло P1.

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

Reference in new issue View Git Blame Copy Permalink