297 lines
18 KiB
Markdown
Executable File
297 lines
18 KiB
Markdown
Executable File
# Balda — PR #8 review fixes: slash commands, /locator, group DM session
|
||
|
||
> Создан 2026-09-28. Статус: **✅ РЕАЛИЗОВАНО, ЗАПУШЕНО, CI 8/8 PASS.**
|
||
> Голова PR: **`05342c5a`** (9 коммитов). Основная правка — `cd4ce4bc`, тесты — `05342c5a`.
|
||
> PR **baldaworks/balda#8**: 46 файлов, +6850/−27, MERGEABLE.
|
||
> **CI 8/8 PASS:** architecture-lint · ingress-e2e · jetstream-integration · lint ·
|
||
> postgres-integration · projection-replay · sqlite-integration · test.
|
||
> Источник: ревью **`metalagman`** (опубликовано в PR 2026-09-28T06:02:48Z, state COMMENTED,
|
||
> **inline-комментариев 0** — всё в теле ревью) + **четвёртый дефект, найденный мной** (§5).
|
||
> Часть [[personal/plans/balda-mm-pr-plan]] (PR #8).
|
||
|
||
---
|
||
|
||
## 0a. Сверка с текстом ревью — построчно
|
||
|
||
Ревью `metalagman` **опубликовано в PR** (не «оставлено локально»). Inline-комментариев нет:
|
||
`gh api repos/baldaworks/balda/pulls/8/comments` → `length: 0`. Три пункта в теле ревью:
|
||
|
||
| # | Дословное требование ревью | Статус |
|
||
|---|---|---|
|
||
| 1 | «Please add a supported command ingress (and its setup), or change the user-facing invocation to one that arrives as a normal post» | ✅ HTTP-приёмник + раздел «Slash command setup» в `docs/mattermost.md` |
|
||
| 1b | «needs a real Mattermost integration check because the current command test injects a synthetic `posted` frame» | ✅ тесты идут через `httptest.NewRequest` (реальный HTTP), `processPosted`/`PostedData` — **0 вхождений** |
|
||
| 2 | «Please add transport-neutral Mattermost parsing **and a round-trip test**» | ✅ `locatorref` + `TestMattermostLocatorSatisfiesCommandGuard` — воспроизводит **точный** guard handler'а: `Parse(Format(l))` и `Format(parsed) == ref` |
|
||
| 3 | «give group DMs a stable channel-based session identity **and cover posts from two senders in one group channel**» | ✅ `TestGroupChannelPostsFromTwoSendersShareOneSession` + 2 теста изоляции сессий |
|
||
|
||
📌 **Важно:** требование 3 изначально закрыто **не было**. Существовавший
|
||
`TestLocatorForPostUsesDirectLocatorForDMAndGroupChannels` брал **одного** `user-1` для `D` и
|
||
`G` и не проверял двух отправителей — то есть именно тот случай, о котором писал ревьюер.
|
||
Добавлены 3 теста в `inbound_test.go` + 1 в `locatorref/mattermost_test.go`.
|
||
📌 Требование 2: тест был, но проверял `Parse → Format`, а handler требует
|
||
`Parse(Format(l))` + `Format(parsed) == ref` — другой порядок. Добавлен точный.
|
||
|
||
---
|
||
|
||
## 0. Итог
|
||
|
||
Ревью Alex подтверждено в коде — все три пункта. Плюс найден **четвёртый дефект**,
|
||
которого в ревью не было (§5). Все четыре закрыты одним коммитом.
|
||
|
||
| # | Дефект | Статус |
|
||
|---|---|---|
|
||
| Б1 | Slash-команды не доходят (только WS `posted`) | ✅ HTTP-приёмник `command_server.go` |
|
||
| Б2 | `/locator` падает: `locatorref` не знает `mattermost` | ✅ ветка + декодер + тесты |
|
||
| В3 | Групповой DM расщепляется по авторам | ✅ свой тип/префикс/сессия + scope |
|
||
| **Б4** | **Регистрация в `ScopeResolver` отсутствовала В PR** | ✅ `app.go` + `preflight.go` |
|
||
|
||
---
|
||
|
||
## 1. Гейты — все зелёные (локально, на закоммиченном дереве)
|
||
|
||
```
|
||
gofmt (мои файлы) → чисто
|
||
go build ./... → exit 0
|
||
go vet ./... → exit 0
|
||
go tool golangci-lint run → 0 issues
|
||
go tool go-arch-lint check --project-path . → OK - No warnings found (без runtime-local)
|
||
go test -race (7 затронутых пакетов) → все ok
|
||
go test ./cmd/balda/ (fx-граф) → ok
|
||
```
|
||
|
||
**Новые тесты: 19** (`command_server_test.go` 15 + `locatorref/mattermost_test.go` 4).
|
||
|
||
⚠️ **`go-arch-lint`:** 28 замечаний «not attached» — **все** из `runtime-local/`
|
||
(наша локальная папка с форком runtime, в PR её нет). Проверено `mv runtime-local` →
|
||
`OK - No warnings found`. Это не наши правки.
|
||
|
||
⚠️ **gofmt:** в репо 80 файлов не отформатированы — **унаследовано**, проверено на `HEAD`
|
||
(`git show HEAD:<file> | gofmt -l /dev/stdin`). Мои файлы чисты. Свои gofmt-правки в
|
||
`adapter.go`/`ingress_test.go` **откатил**, чтобы не раздувать diff PR-ом чужих файлов.
|
||
|
||
---
|
||
|
||
## 2. Б1 — slash-команды: HTTP-приёмник (вариант A)
|
||
|
||
📌 **Факт из доки MM:** «if you type in a slash command it will execute **without posting
|
||
a message**» — WS-события `posted` для slash не будет. Ревью право на 100%.
|
||
|
||
**Что уже было и что добавлено:**
|
||
|
||
| Компонент | Статус |
|
||
|---|---|
|
||
| `mattermostInboundHandler.HandleCommand` — публикация в `commandcmd` | ✅ уже был, корректен |
|
||
| `ParseCommand` — разбор `/name args` | ✅ уже был |
|
||
| вызов из `processPosted` (mention-путь: `@balda /locator`) | ✅ работает |
|
||
| **HTTP-путь для slash** | ❌ **добавлен** `channel/mattermost/command_server.go` |
|
||
|
||
🔴 **Правка Alex (важный урок):** «в твоей ММ реализации взят существующий slack транспорт
|
||
за основу? Они должны быть практически идентичны». Первая версия разошлась со Slack в
|
||
пяти местах **без причины** — переписал по образцу `slackagent/server.go`:
|
||
|
||
| Что | Slack | Было у меня | Стало |
|
||
|---|---|---|---|
|
||
| `invocationID` | хеш от тела | из `post_id` | `commandInvocationID(body)` |
|
||
| таймаут команды | `2500ms` | `30s` | `2500ms` |
|
||
| семафор | `processSem` 16 | отсутствовал | 16 |
|
||
| `webhookMaxBodyBytes` | `1 << 20` | `64 << 10` | `1 << 20` |
|
||
| `ReadTimeout`/`Idle` | 10s / 30s | 15s / 60s | 10s / 30s |
|
||
|
||
**Осталось ровно два отличия, и оба диктует провайдер (не наш выбор):**
|
||
1. **Верификация:** у MM `token` в теле запроса (сравнение constant-time), у Slack —
|
||
HMAC-подпись `X-Slack-Signature`. Копировать `verifySignature` нельзя.
|
||
2. **Форма ответа:** MM `{"response_type":"ephemeral","text":"..."}`, Slack `{"text":...}`.
|
||
|
||
**Поведение:**
|
||
- принимает обе конфигурации команды: `/locator` (аргументы в `text`) и `/balda locator`
|
||
(первое слово `text` = подкоманда) — как у Slack с `/balda`;
|
||
- неизвестная команда → **200 + usage** (не 400, не тишина) — тоже как у Slack;
|
||
- тип канала (`D`/`G`/`O`/`P`) резолвится через `GetChannel`, т.к. slash его не несёт;
|
||
при ошибке резолва — трактуем как **не-DM** (строгая авторизация);
|
||
- отвечает **сразу**, результат уходит постом (MM показывает timeout автору за ~3 с).
|
||
|
||
**Новые ключи конфига** (`balda.mattermost.*`): `commands_enabled`,
|
||
`commands_listen_addr` (`:8093`), `commands_path` (`/mattermost/commands`),
|
||
`commands_token`. Все 4 + провайдеры `name:`-тегов в `app.go`.
|
||
|
||
---
|
||
|
||
## 3. Б2 — `/locator`: `mattermost` в `locatorref`
|
||
|
||
Механически, по образцу `slackagentLocatorFromAddressKey`.
|
||
|
||
🔴 **Питфолл архитектуры ЗАКРЫТ правильно:** `AGENTS.md` требует независимости `locatorref`
|
||
от транспортов. Не импортировал `channel/mattermost` — использовал
|
||
`string(deliverycmd.ChannelTypeMattermost)`, ровно как для Slack
|
||
(`channelTypeSlackAgent = string(deliverycmd.ChannelTypeSlackAgent)`).
|
||
`deliverycmd` уже в allow-list `locatorref`. **`go-arch-lint` зелёный.**
|
||
|
||
🔴 **Ошибка, которую поймал тестами:** первая версия декодера использовала типы
|
||
`"channel"`/`"direct"`, а транспорт определяет `"channel"`/`"dm"`/`"group"`. Несовпадение
|
||
строк = локатор декодируется, но транспорт его отвергает. **Сверить имена типов с
|
||
`locator_codec.go`, не выдумывать.**
|
||
|
||
**Хвост Б2 (закрыт тем же изменением):**
|
||
- `internalmcp/manager.go:409` — `locatorref.Parse` теперь работает для MM;
|
||
- `envelopetarget/resolve.go:62` — `balda.scheduler.jobs` `target: locator` для MM теперь
|
||
работает. Это вторая половина фразы ревью «исключает Mattermost из config targets».
|
||
|
||
**Тесты:** `locatorref/mattermost_test.go` — 4 теста (канал/тред, DM/группа,
|
||
Format-round-trip, отказ на кривом ключе).
|
||
|
||
---
|
||
|
||
## 4. В3 — групповой DM
|
||
|
||
**Было (дефект):**
|
||
```go
|
||
dmSessionID: if a.UserID != "" { return "mm-dm-<hash(UserID)>" } // АВТОР, не чат
|
||
```
|
||
`G`-канал общий, авторов много → у каждого своя сессия.
|
||
|
||
**Стало:**
|
||
|
||
| Тип MM | AddressKey | SessionID | Scope |
|
||
|---|---|---|---|
|
||
| `D` (1:1) | `d:<channel_id>` | `mm-dm-<hash(channel_id)>` | personal |
|
||
| `G` (групповой) | **`g:<channel_id>`** | **`mm-gdm-<hash(channel_id)>`** | personal |
|
||
|
||
🔴 **Отдельный префикс ключа `g:` — не косметика.** Общий `d:` не различим при
|
||
`LocatorFromAddressKey`: round-trip `d:` восстанавливал **любой** групповой DM как 1:1.
|
||
Свой префикс сохраняет тип.
|
||
|
||
**Закрыто в 8 местах** (§«Тип канала» — самый коварный):
|
||
`addressTypeGroupDM` · `NewGroupDMLocator` · `dmAddressKey` · `dmSessionID` ·
|
||
`IsDirectLocator` · **`validateLocatorAddress`** · **`ClassifyLocatorScope`** ·
|
||
`LocatorForPost` + `IsGroupChannelType`.
|
||
|
||
⚠️ **`validateLocatorAddress` и `ClassifyLocatorScope` — я их пропустил в первой версии.**
|
||
`DecodeLocator` возвращает `ok=false` для неизвестного типа, и тогда **все**下游-хелперы
|
||
fail-closed: `IsDirectLocator`, `ChannelIDFromLocator`, `ClassifyLocatorScope`.
|
||
Тест `TestLocatorForPostUsesDirectLocatorForDMAndGroupChannels/G` это и поймал.
|
||
|
||
⚠️ **Обратная совместимость:** старая форма `D`-сессии была `mm-dm-<hash(UserID)>`, стала
|
||
`mm-dm-<hash(channel_id)>` → существующие 1:1 DM-сессии на Kraken **осиротеют** (новые
|
||
создадутся, старые останутся мёртвыми). Тестовая инсталляция — принято.
|
||
|
||
---
|
||
|
||
## 5. 🔴 Б4 — четвёртый дефект, которого НЕ было в ревью
|
||
|
||
**Регистрация MM в `ScopeResolver` отсутствовала в PR #8.**
|
||
|
||
Диагностика: `grep ClassifyLocatorScope internal/apps/balda/app.go` — строка **есть** в
|
||
рабочем дереве, но `git show HEAD:...app.go | grep ClassifyLocatorScope` — строка
|
||
**отсутствует**. Значит она была WIP-правкой на диске и в коммиты не уехала.
|
||
В `preflight.go` её не было вообще (обе ветки).
|
||
|
||
**Почему это критично (питфолл #11 навыка):** `sessionmemoryapp.ScopeResolver` строит карту
|
||
классификаторов по `channelType`; резолвер **fail-closed** (`capture.go`:
|
||
`classifier == nil` → `CodeUnsupportedScope`). Симптом при `session_memory.enabled: true`:
|
||
```
|
||
warn … "no session-memory classifier is registered for \"mattermost\""
|
||
→ в чате: "Could not reset this session."
|
||
```
|
||
+ **MCP-инструменты не подключаются** (`function_call_part_count: 0`).
|
||
Компилируется, `go build`/`vet`/линтеры зелёные, отказ **только в рантайме**.
|
||
|
||
Зарегистрировано в **обоих** файлах (`app.go` и `preflight.go`) — карт **две**,
|
||
правка одной даёт расхождение «preflight прошёл, рантайм упал».
|
||
|
||
📌 **Урок метода:** «зелёные гейты» ≠ «рабочий транспорт». Эта строка проходила все
|
||
статическиe проверки, отсутствуя в PR. Сверять `git show HEAD:<file>` с рабочим деревом
|
||
для критичных регистраций, а не доверять `git status`.
|
||
|
||
---
|
||
|
||
## 6. Что понадобилось дополнительно (для будущих транспортов)
|
||
|
||
1. 🔴 **Каждый `name:`-тег в `mattermostfx` = обязательный провайдер в `app.go`.**
|
||
Добавлены 4: `balda_mattermost_commands_{enabled,listen_addr,path,token}`.
|
||
Иначе fx: `missing types: string[name="balda_mattermost_commands_enabled"]`.
|
||
2. ⚠️ **Ключ обязан быть в `cmd/balda/balda.yaml`** — env-override
|
||
(`BALDA_MATTERMOST_COMMANDS_*`) применяется только к leaf-ключам, **уже присутствующим**
|
||
в YAML (`appconfig/loader.go`). Молчаливое игнорирование иначе.
|
||
3. ✅ **DI-граф проверен реально** — `TestValidateCommandRunsValidationWithMattermostTransport`
|
||
прогоняет `loadBaldaCommandConfig(true)` + `validateBaldaApplication(prepared)` с
|
||
включённым MM. В логе видно оба провайдера: `NewCommandServer` + `NewIngress`.
|
||
**`go build` этого не проверяет** — только `fx.ValidateApp`.
|
||
|
||
### Файлы (новые)
|
||
| Файл | Строк |
|
||
|---|---|
|
||
| `channel/mattermost/command_server.go` | ~409 |
|
||
| `channel/mattermost/command_server_test.go` | ~361 |
|
||
| `locatorref/mattermost_test.go` | ~94 |
|
||
|
||
### Дельты (все применены, `tools/`)
|
||
`deltas-pr8-b2-locatorref.json` · `-b2-locatorref-types.json` · `-p3-groupdm.json` ·
|
||
`-p3-groupdm-keys.json` · `-b1-wiring.json` · `-b1-config-keys.json` · `-lint-fixes.json` ·
|
||
`-docs-commands.json` · `-docs-mattermost.json`
|
||
|
||
---
|
||
|
||
## 7. Доки
|
||
|
||
- `docs/commands.md` — MM-колонка приведена к **реальности**: `/start`, `/topic`,
|
||
`/goalkeeper`, `/auto`, `/usage`, `/close`, `/cancel`, `/user`, `/locator` = **yes**
|
||
(было `no` — дока врала). Абзац про HTTP-природу slash + обе конфигурации команды.
|
||
- `docs/mattermost.md` — новый раздел **Slash command setup** (Request URL, token, шаги),
|
||
4 ключа в таблице, env-образец, slash-специфичный troubleshooting.
|
||
|
||
---
|
||
|
||
## 8. Дальше
|
||
|
||
| # | Шаг | Статус |
|
||
|---|---|---|
|
||
| 1 | CI на `05342c5a` (8 джобов) | ✅ **8/8 PASS** |
|
||
| 2 | **Ре-ревью PR #8** — `reviewDecision` пустой, ревью было `COMMENTED` ≠ APPROVED | ⛔ внешний блокер |
|
||
| 3 | Живая приёмка slash на Kraken (публичный HTTPS для Request URL!) | ⏳ следующий шаг |
|
||
| 4 | Фаза 4 — исполнитель (делегирование) | 🎯 основной фронт |
|
||
|
||
⚠️ **Б1 требует публичного HTTPS** для Request URL slash-команды. Стенд Kraken —
|
||
проверить достижимость эндпоинта **до** приёмки; иначе нужен туннель.
|
||
⚠️ **Команды PR (воспроизводимо):**
|
||
```bash
|
||
gh pr checks 8 --repo baldaworks/balda
|
||
gh api repos/baldaworks/balda/pulls/8/reviews | jq -r '.[] | "\(.user.login) \(.state)"'
|
||
gh api repos/baldaworks/balda/pulls/8/comments | jq 'length' # inline-комменты; 0 = всё в теле ревью
|
||
```
|
||
|
||
---
|
||
|
||
## 9. Конфиг — точные значения (для Фазы 3/приёмки)
|
||
|
||
`cmd/balda/balda.yaml`, блок `balda.mattermost` (все 4 ключа **обязаны** быть в файле,
|
||
иначе env молча игнорируется):
|
||
```yaml
|
||
balda:
|
||
mattermost:
|
||
enabled: true
|
||
server_url: "http://mattermost:8065"
|
||
token: "${BALDA_MATTERMOST_TOKEN}"
|
||
bot_user_id: "${BALDA_MATTERMOST_BOT_USER_ID}"
|
||
bot_username: "balda"
|
||
commands_enabled: true # ← HTTP-приёмник slash, НЕЗАВИСИМ от enabled
|
||
commands_listen_addr: ":8093"
|
||
commands_path: "/mattermost/commands"
|
||
commands_token: "${BALDA_MATTERMOST_COMMANDS_TOKEN}"
|
||
```
|
||
Env-имена (проверено, `appconfig/loader.go` `SetEnvPrefix("BALDA")` + `"." → "_"`):
|
||
```
|
||
BALDA_MATTERMOST_COMMANDS_ENABLED / _LISTEN_ADDR / _PATH / _TOKEN
|
||
```
|
||
DI-провайдеры в `internal/apps/balda/app.go` (`name:`-теги, иначе fx `missing types`):
|
||
`balda_mattermost_commands_{enabled,listen_addr,path,token}`.
|
||
⚠️ `commands_enabled` и `enabled` — **разные переключатели**: `enabled` = websocket-поток
|
||
(разговоры), `commands_enabled` = HTTP-приёмник (slash). Без второго команды не работают.
|
||
|
||
---
|
||
|
||
## Связанные заметки
|
||
|
||
- [[personal/plans/balda-mm-pr-plan]] — PR #8, состояние, деплой, приёмка
|
||
- [[personal/projects/balda/balda-mattermost-connector]] — Фаза 2 (код коннектора)
|
||
- [[personal/tech/balda-go-fx-pitfalls]] — fx-питфоллы, дельта-апплаер, arch-lint
|
||
- [[personal/projects/balda/balda-mm-delegation-plan]] — общий план
|