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

23 KiB
Executable File
Raw Blame History

Создан 2026-09-28. Статус: ✅ MERGED В АПСТРИМ 2026-09-28T13:22:13Z (head facb0d0). Финальная голова PR: facb0d0. Мейнтейнер добавил 3 фикса поверх моего d4ab923 — см. §0г. Образ Балды переведён на тег v2.1.0 (= c41939f, содержит PR #4 + #5). PR normahq/runtime#5 — MERGED. Репо: ~/Developer/runtime-local. Связано: personal/plans/runtime-pr4-openai-toolcalls-fixes, personal/plans/balda-runtime-rebuild-mm-validation.

🔴 2026-09-29 — УТОЧНЕНИЕ ПО РЕПОЗИТОРИЮ. PR #4/#5 (и #7) живут в normahq/runtime, НЕ в baldaworks/runtime — такого репо не существует (GraphQL: Could not resolve to a Repository with the name 'baldaworks/runtime'). Прежняя запись «репо baldaworks/runtime» — ошибка. baldaworks/ содержит только balda.

📌 Ветки PR снесены 2026-09-29. В ~/Developer/runtime-local удалены локально и в origin: feat/openai-tool-calls-v2, fix/hosted-toolsets-duplicate-endpoint, fix/openai-tool-call-roundtrip-v2 (PR #7). fetch --all --prune подтянул upstream/main до 5375072 (PR #7) и тег v2.1.1. Остались main + origin/main + upstream/main.

⚠️ ~/Developer/norma-local, norma-fork, norma-upstream-check — это форки norma, ДРУГОГО репозитория. К PR #4/#5/#7 отношения не имеют, хотя названия сбивают. runtime-local — единственный настоящий клон runtime.

🗑️ СНЕСЕНЫ 2026-09-29 (291 МБ, решение Alex). ⚠️ Вместе с norma-fork потерян непушенный коммит 70ff876 feat: support custom OpenAI base URL via OPENAI_BASE_URL env var (ветка feat/custom-openai-base-url, в remotes отсутствовал) + незакоммиченная правка norma-local/pkg/runtime/hostedagent/openai.go. Это тот самый OPENAI_BASE_URL, на котором держится запуск Балды (в YAML base_url мёртв — см. personal/tech/balda-go-fx-pitfalls). Восстанавливать неоткуда — при необходимости писать заново. 📌 Урок: перед rm -rf репо — git log --branches --not --remotes --oneline.

0г. 🔴 Мейнтейнер нашёл 2 реальных дефекта в моём фиксе (d4ab923 → facb0d0)

Перед мержем в ветку PR #5 добавлены 3 коммита: f8011c1 fix(agentfactory): preserve MCP dedup metadata · e888d5e fix(agentconfig): validate preferred MCP dedup · facb0d0 fix(agentfactory): keep MCP dedup silent. Diff: 5 файлов, +160/−10.

# Дефект в моём фиксе Фикс мейнтейнера
1 🔴 hydrateMCPServerConfig не переносил DedupKey/DedupPreferred — собирал новый MCPServerConfig поле за полем и забыл новые поля. Фикс был бы мёртв в проде на пути Build() добавлены DedupKey: cfg.DedupKey, DedupPreferred: cfg.DedupPreferred. Покрыто тестом TestFactoryBuildKeepsPreferredDedupEndpoint (сквозной путь registry → hydration → hostedToolsets)
2 strings.TrimSpace(key) нормализовал ключ → независимо настроенные namespace'ы с пробелами схлопывались (ровно тот cross-tenant риск, который API и предотвращает) сравнение по сырому ключу; проверка пустоты отдельно: key := resolvedMCP[id].DedupKey; if strings.TrimSpace(key) == "". Тест TestDedupKeyIsOpaque
3 Две записи с DedupPreferred: true в одной группе — молча выбиралась одна (зависит от порядка id) явная ошибка: mcp dedup key %q has multiple preferred configs. Тест TestHostedToolsetsRejectsMultipleDedupPreferredConfigs
4 fmt.Printf("[mcp] skipping …") в библиотеке — печать в stdout потребителя вывод убран (коммит «keep MCP dedup silent»)
5 DedupPreferred: true без DedupKey не валидировался ValidateMCPServerConfig: dedup_preferred requires dedup_key. Тест TestValidateMCPServerConfig_DedupPreferredRequiresDedupKey

📌 Уроки (durable):

  1. 🔴 Добавляя поле в конфиг — грепнуть ВСЕ места ручного копирования структуры. hydrateMCPServerConfig собирает MCPServerConfig поле за полем; компилятор такое не ловит (пропущенное поле = zero value, валидный код). Искать grep -n "MCPServerConfig{" во всём репо. Это ровно тот класс дефекта, что и «терпимый ассерт» / «копия логики в тесте»: выглядит рабочим, молчит в проде.
  2. ⚠️ Opaque = opaque. Назвал ключ «непрозрачным» у вызывающего — не нормализуй его сам. TrimSpace выглядел гигиеной, а был дырой безопасности.
  3. ⚠️ Библиотека не печатает в stdout. Логирование — дело потребителя.
  4. 📌 Тест сквозного пути ловит то, что юнит-тест уровня функции пропускает. Мой тест дёргал hostedToolsets напрямую и не проходил через hydration — дефект №1 был невидим. Правильный тест — через f.Build().

0а. 🔴 ПОСТ-МЕРЖ: мейнтейнер нашёл 2 дефекта в МОЁМ фиксе

После моего d4ab923 и до мержа в ветку прилетели 3 коммита мейнтейнера (f8011c1, e888d5e, facb0d0 — head PR стал facb0d0). Это code review моего кода, и два замечания — настоящие дефекты:

Коммит Что нашёл Почему это мой дефект
f8011c1 fix(agentfactory): preserve MCP dedup metadata hydrateMCPServerConfig не переносил DedupKey/DedupPreferred в гидратированную копию 🔴 Фикс был бы мёртвым в продовом пути Build(). hostedToolsets читает гидратированные конфиги → ключ всегда пуст → дедуп никогда не срабатывает. Мой юнит-тест звал hostedToolsets напрямую, минуя гидратацию, поэтому дефект не поймал
e888d5e fix(agentconfig): validate preferred MCP dedup DedupPreferred: true без DedupKey не отвергался Бессмысленная конфигурация проходила молча
facb0d0 fix(agentfactory): keep MCP dedup silent Убрал мой fmt.Printf из библиотеки; добавил ошибку при нескольких DedupPreferred в одной группе Библиотека не должна писать в stdout; молчаливый выбор победителя делал активную авторизацию зависимой от порядка id
— strings.TrimSpace(key) — нормализация ключа 🔴 Ключ обязан быть opaque: "tenant-a" и " tenant-a " — разные namespace'ы. Нормализация склеила бы независимо сконфигурированные namespace'ы и воссоздала бы cross-tenant merge, который этот API и должен предотвращать

Заменённые строки в моём коде:

// было (мой вариант)                    // стало (upstream/main)
key := strings.TrimSpace(cfg.DedupKey)   key := cfg.DedupKey
                                         if strings.TrimSpace(key) == "" { continue }
skip[id] = true                          skip[id] = true   // без fmt.Printf
  • hydrateMCPServerConfig теперь копирует DedupKey/DedupPreferred.

📌 ГЛАВНЫЙ УРОК (durable): юнит-тест, вызывающий внутреннюю функцию напрямую, не покрывает продовый путь, где конфиг проходит через гидратацию/нормализацию. Мейнтейнер добавил TestFactoryBuildKeepsPreferredDedupEndpoint — он идёт через f.Build() (registry lookup → hydration → hostedToolsets). Правило: тестировать сквозь композиционный путь, а не только целевую функцию. Этот дефект дал бы «зелёные гейты и неработающий фикс» — ровно тот класс ошибки, против которого был весь PR.

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. ✅ Ре-ревью пройдено, PR влит (6775d71).
  2. 🔴 Синхронизация копии рантайма в Balda — ОТКРЫТО, блокирует боевой стенд. ~/Developer/balda/runtime-local — не клон runtime, а снапшот кода эпохи PR #4 (3 файла отличаются от upstream/main, все три — в пользу апстрима; DedupKey — 0 вхождений). Каталог git-ignored в balda и подключён через replace github.com/normahq/runtime/v2 => ./runtime-local. Правки Balda-слоя из §0 не компилируются против него (unknown field DedupKey in struct literal of type agentconfig.MCPServerConfig). ✅ Потери при замене НЕТ — проверено diff -rq: отличаются только agentfactory/agentfactory.go, hostedagent/openai.go, hostedagent/openai_test.go (все — устаревшие версии того, что уже в апстриме). ⚠️ В апстриме мой код уже исправлен мейнтейнером (§0а) — синхронизировать надо с upstream/main (c41939f), а не с моим d4ab923. План и развилка (убрать replace совсем vs заменить копию) — personal/plans/balda-runtime-rebuild-mm-validation §1/§3.
  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/#5 больше не «утекли» — обе влиты.

6. ✅ РЕШЕНО 2026-09-28: синхронизация копии рантайма в Balda

Открытый вопрос из §5 закрыт в тот же день. Изложен и решён в personal/plans/balda-runtime-rebuild-mm-validation §1.

Что было: ~/Developer/balda/runtime-local — снапшот файлов runtime без .git, снятый руками на эпоху PR #4, подключённый через replace. DedupKey (PR #5) в нём отсутствовал, поэтому Balda-слой дедупа не компилировался.

Что сделано: после мержа PR #4/#5 в апстрим (тег v2.1.0 = c41939f = upstream/main):

  • go.mod → require github.com/normahq/runtime/v2 v2.1.0, директива replace УДАЛЕНА;
  • Dockerfile.release → удалён COPY runtime-local/ ./runtime-local/;
  • каталог runtime-local/ вынесен из дерева в /tmp (восстановимо).

📌 Проверка перед удалением (обязательна для любой такой копии):

diff -rq runtime-local <чистый upstream-чекаут>

Отличались ровно 3 файла (agentfactory/agentfactory.go, hostedagent/openai.go, hostedagent/openai_test.go) — и во всех трёх апстрим новее. Потери нет.

Результат: образ balda-mm:0.1.14 собран и задеплоен на Kraken. Приёмка пройдена: duplicate tool: = 0, function_response_part_count = 1 (инструмент исполнен), ответ от аккаунта Alex, settlement_stage=accepted, RestartCount=0.

⚠️ Побочный эффект: пока runtime-local/ лежал в корне, go-arch-lint давал 56 notices (осиротевший каталог чужого модуля). После выноса — OK - No warnings found.

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

Удалить лишнее дерево, созданное ошибочным write_file:

/Users/admin/Developer/balda/personal/

Содержит мой дубль plans/runtime-pr5-…md и чужой tech/music-broken-toload.md (не мой файл!). Удаление требует явного подтверждения — не трогал.

🔴 Найденный питфолл (durable): write_file с относительным путём пишет относительно cwd сессии, а НЕ в vault. Для vault — только mcp_obsidian_write_note (vault root /Users/admin/obsidian).

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