[2026-09-26] eagle: personal/plans/balda-mm-pr-plan.md personal/projects/balda/balda-mattermost-connector.md personal/tech/balda-go-fx-pitfalls.md
This commit is contained in:
Alexey Martemyanov committed
1 parent
4ab53a2a63
commit
519f7c443c
3 files changed
+301
-47
No files matched your search
@@ -68,6 +68,41 @@ case transportMattermost:
|
||||
📌 **Правило: дефект, найденный в данных, искать и в коде.** Правка данных без правки
|
||||
кода = дефект вернётся при следующей записи. Проверять **оба слоя** и фиксить источник.
|
||||
|
||||
### ✅ ИСПРАВЛЕНО 2026-09-26 (сессия PR-подготовки) — фикс в хелпере
|
||||
|
||||
Вместо костыля `TrimPrefix` в вызывающем коде сделана **идемпотентной сама `MattermostSubject`**:
|
||||
|
||||
```go
|
||||
// MattermostSubject returns the channel-qualified subject for a Mattermost user.
|
||||
// It is idempotent: a value that is already channel-qualified ("mattermost:<id>")
|
||||
// yields the same subject, so callers may pass either a raw user ID or a subject
|
||||
// without risking a doubled prefix ("mattermost:mattermost:<id>").
|
||||
func MattermostSubject(userID string) string {
|
||||
trimmed := strings.TrimSpace(userID)
|
||||
trimmed = strings.TrimPrefix(trimmed, ChannelMattermost+":")
|
||||
return fmt.Sprintf("%s:%s", ChannelMattermost, trimmed)
|
||||
}
|
||||
```
|
||||
|
||||
`userSubject` вернулся к симметричному с другими транспортами виду:
|
||||
```go
|
||||
case transportMattermost:
|
||||
return auth.MattermostSubject(trimmed)
|
||||
```
|
||||
|
||||
📌 **Почему фикс в хелпере, а не в вызывающем:** `MattermostSubject` вызывается из **трёх**
|
||||
мест (`userSubject`, `mattermost_ingress.go:147`, `authorizeInbound`). Идемпотентность
|
||||
закрывает все пути сразу и защищает будущие вызовы поверх готового subject'а.
|
||||
Дельта `tools/deltas-mattermost-23-prefix-idempotent.json`.
|
||||
|
||||
**Общий приём для нового транспорта:** любой `<X>Subject(id)`-билдер делать
|
||||
**идемпотентным** — срезать уже существующий `<x>:`-префикс. Тогда вызывающий код может
|
||||
передавать и raw id, и готовый subject без риска двойного префикса. Это дешевле, чем
|
||||
искать все call-site'ы, и переживает добавление новых.
|
||||
|
||||
**Регресс-тесты:** `TestMattermostSubjectIsIdempotent` (`handlersfx/mattermost_ingress_test.go`),
|
||||
`TestMattermostHandlerSendsChannelQualifiedPrincipalExactlyOnce`.
|
||||
|
||||
⚠️ Тот же класс ошибки — двойное применение helper'а, который сам добавляет префикс.
|
||||
Признак: строка вида `<x>:<x>:<id>` в БД, в `display_name`, в `created_by`, в логах.
|
||||
Искать надо `MattermostSubject`/`TelegramSubject`-подобные билдеры, вызванные поверх
|
||||
@@ -79,6 +114,70 @@ case transportMattermost:
|
||||
|
||||
---
|
||||
|
||||
## 🔴 Прогресс-политика должна совпадать на command- и turn-пути одного транспорта
|
||||
|
||||
Найдено 2026-09-26 (сессия PR-подготовки). `handlersfx/mattermost_ingress.go` в
|
||||
`HandleCommand` ставил `ProgressPolicy{Typing: true, PlanUpdates: true}`, а inbound-путь
|
||||
(`channel/mattermost/inbound.go:135`) и `Adapter.SendTyping` — `Typing: false`.
|
||||
|
||||
**Почему это дефект:** у ботов Mattermost **нет typing API**, `SendTyping` — осознанный
|
||||
no-op (`return nil`). При `Typing:true` `SendProgress` вызывает `SendTyping`, тот молча
|
||||
ничего не делает → мёртвая ветка. Она не падает, поэтому компилятор и линтер её не видят.
|
||||
|
||||
📌 **Правило:** command-путь и turn-путь **одного и того же** транспорта обязаны давать
|
||||
**идентичную** `ProgressPolicy`. Расхождение — невидимый мёртвый код. Проверять оба места:
|
||||
в `handlersfx/*_ingress.go` (публикация команды) и в `channel/*/inbound.go` (normalized turn).
|
||||
|
||||
📌 **Метод поиска:** сравнить `ProgressPolicy` в обоих файлах транспорта. Если транспорт
|
||||
не поддерживает typing — `Typing: false` **везде**.
|
||||
|
||||
**Фикс:** дельта `tools/deltas-mattermost-24-typing.json`; тесты
|
||||
`TestMattermostHandlerUsesMarkdownWithoutTypingProgress`,
|
||||
`TestNormalizeInboundDisablesTypingProgress`.
|
||||
|
||||
## 🔴 Тесты нового транспорта: REST через `httptest`, WS через `Upgrader`, хендлер напрямую
|
||||
|
||||
Метод, отработанный 2026-09-26 на MM (опубликован в PR — воспроизводить для любого
|
||||
нового транспорта):
|
||||
|
||||
| Слой | Как тестировать |
|
||||
|---|---|
|
||||
| **REST-клиент/адаптер** | `httptest.NewServer` + fake-handler на `POST /api/v4/posts`, запоминающий запросы; инъецируемый `respond(attempt int, req) (status int, body string)` для проверки fallback (1-й → 400, 2-й → 201) |
|
||||
| **WebSocket-ингресс** | реальный `websocket.Upgrader` в `httptest` + `websocket.DefaultDialer` — **единственный** способ проверить, что `writeEvent` всегда шлёт `seq` |
|
||||
| **Хендлер** | тестировать **структуру напрямую** (`&xInboundHandler{...}`), без fx — изолирует авторизацию, префикс principal, nil-safety |
|
||||
| **Фикстуры** | `fake...OwnerKVStore` + `recording...CommandIngress` по образцу `zulip_ingress_test.go` |
|
||||
|
||||
📌 **Обязательные регресс-тесты для нового транспорта:**
|
||||
1. `writeEvent` всегда шлёт `seq` (регрессия `bad_seq`).
|
||||
2. `seq` уникален под конкуренцией (16 горутин) — иначе MM увидит переупорядоченный кадр.
|
||||
3. `<X>Subject` идемпотентен (регрессия двойного префикса).
|
||||
4. `ProgressPolicy` совпадает на command- и turn-пути.
|
||||
5. Fallback только на content-rejection (400), **не** на 5xx.
|
||||
6. `/start` доступен до авторизации.
|
||||
7. `Start` отказывается без обязательных конфигов (bot user id и т.п.).
|
||||
|
||||
### ⚠️ Метод: падающий тест — сначала перечитать контракт, а не «чинить» код
|
||||
|
||||
Три первых теста MM падали, и **все три** оказались неверными предположениями теста,
|
||||
а не дефектами кода:
|
||||
|
||||
| Я предположил | Реальность |
|
||||
|---|---|
|
||||
| тред-сессия == сессия root-поста | тред — **отдельная** сессия от канала (`mm-t-…` vs `mm-c-…`), это правильный дизайн |
|
||||
| `NewIngress` подставляет `bot_user_id` из клиента | не подставляет; гард — в `onStart` («refuses to start without bot user id») |
|
||||
| отказ неавторизованному возвращает `nil` | возвращает `runtime is unavailable` (нет dispatcher'а), но команда **не публикуется** — это и есть контракт |
|
||||
|
||||
📌 **Правило:** когда новый тест падает — сначала перечитать код и понять, **чей** это
|
||||
контракт. «Привести код к тесту» — неверный рефлекс: тест мог быть написан по неверному
|
||||
предположению. Из трёх падений ноль были дефектами.
|
||||
|
||||
📌 **Побочная находка (дизайн MM-локатора):** root-пост (`RootID=""`) → `mm-c-<hash(channel)>`,
|
||||
ответ в треде (`RootID=<root>`) → `mm-t-<hash(channel|root)>`. **Тред — отдельная беседа
|
||||
от канала.** Соответствует комментарию в `locator_codec.go`, но означает: бот в треде ведёт
|
||||
сессию, отдельную от канала. Зафиксировано тестом.
|
||||
|
||||
---
|
||||
|
||||
## 🔴🔴 Дефект 8: замкнутый круг привязки, когда binding занят чужим аккаунтом
|
||||
|
||||
Ситуация: владельцем привязан **не тот** аккаунт, а нужно привязать правильный.
|
||||
|
||||
Reference in new issue
Block a user