Files
transcriber/openspec/changes/archive/2026-08-11-fix-http-handler-tests/review/triage.md
T
av 6c04c801c9 http: тесты приёма переписаны на подставные адаптеры
- проверки больше не зовут ffprobe и не меняют рабочий каталог процесса;
  добавлены случаи на отказ чтения метаданных и на отсутствие поля audio
- заведена спека intake на приём по HTTP, ADR о подставных адаптерах,
  запись в журнал ревью о проверке, которая не могла упасть
- go test снят из объявленных долгов CLAUDE.md, послабление errcheck
  для _test.go в .golangci.yml убрано
2026-08-11 08:35:11 +03:00

391 lines
34 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Триаж ревью: `fix-http-handler-tests`
## Сводка
- **Размер:** малое. **Сложность:** знакомое. **Метка:** `small`.
Обоснование разметки (`review-scope`): изменение трогает один тестовый файл и
одну строку конфига линтера, поведение сервиса не меняется, отрицательный тест
метки (миграция, формат файла на диске, публичный контракт API, имя ключа) не
срабатывает — контракт HTTP API спекой **фиксируется**, а не меняется.
- **Режим прогона:** по графу. Дизайн — одна стадия (`specs`), код — `autotests`
`specs` + `code` → триаж.
- **Сигнал о заниженной метке:** пришёл от `review-code` — возражений нет, метку
`small` проход счёл обоснованной. `review-basics` не запускался (своих тем у
проекта нет), второго независимого подтверждения метки нет.
- **Состояние гейта (проверено мной на этом прогоне):**
- `go test ./...`**зелёный** (`ok internal/controller/http 0.013s`); это и
есть цель изменения;
- `golangci-lint run`**4 замечания, ровно объявленный долг**
(`speechkit.go:55`, `main.go:124`, `worker.go:51`, `service/transcribe.go:394`).
Ни одного нового, в том числе после снятия исключения `errcheck` для `_test.go`;
- критерий приёмки «не зависит от `ffprobe`» подтверждён: собранный
`go test -c` бинарь проходит под `env -i PATH=<пустой каталог>`;
- критерий «`os.Chdir` не остаётся» подтверждён: `grep -rn 'os.Chdir' internal/` пуст.
- Итог: гейт красный **только унаследованным долгом**; новых красных шагов нет.
### План разметки с исходом по каждой теме
| тема | дом | глубина | закрывает | исход |
|---|---|---|---|---|
| requirements | `openspec/changes/fix-http-handler-tests/specs/intake/spec.md` | сверка | `specs` | **закрыта**, 3 находки (потолок 3/3 сработал), 2 из них починены и мной перепроверены мутацией |
| autotests | `CLAUDE.md` § Гейт | — | `autotests` | **закрыта**, 0 находок; отчёт о гейте + 1 строка в границы покрытия. О своём потолке проход не сообщил |
| conventions | `docs/conventions/README.md` (на `small` — только README) | сверка | `code` | **закрыта**, 1 находка (потолок 1/2), **не починена** — единственный блокер ниже |
| architecture | `CLAUDE.md` § Инварианты | сверка | `code` | **закрыта**, 0 находок (потолок инвариантов 0/1) |
| security | `CLAUDE.md` § Инварианты | сверка | `code` | **закрыта**, 0 находок — но см. предупреждение ниже |
| operations | `CLAUDE.md` § Инварианты | сверка | `code` | **закрыта**, 0 находок |
Тем без отчёта нет. Тем без дома нет.
**Предупреждение по теме `security`.** «0 находок» здесь не значит «чисто».
Нарушение `critical`-инварианта «содержимое записи остаётся приватным»
(`internal/service/transcribe.go:107` пишет `"file_name", fileName` — имя файла
пользователя — в журнал) найдено **на стадии дизайна** и **сознательно отложено
решением человека на чекпоинте**, поэтому проход кода вернул пустой итог: находка
уже известна и вынесена. Она в урожае, не в блокерах, и это решение человека, а не
моё. Тот же путь проходят записи из Telegram.
### Арифметика
- **На входе:** 14 позиций — 7 именованных находок (`specs` 3, `code` 4),
3 названные ниже потолка, 3 отложенные решением человека, 1 замечание о
непокрытом конвейере от `autotests`.
- **После дедупликации, добычи оракулов и отсева:** **2** позиции, требующие
действия (1 блокер + 1 развилка). Остальное — в урожай, гипотезы и promote,
ничего не выброшено молча.
- Починенное проверено мной независимо: обе `major`-находки `specs` закрыты,
мутации их роняют (оракулы ниже).
---
## Блокирует мердж
### `CLAUDE.md` продолжает объявлять долгом отказ, которого больше нет, — и следующий настоящий отказ тестов приёма спишут на него молча
- Файл: `CLAUDE.md:96-101`; сопутствующее: `tasks/BACKLOG.md:23`,
`tasks/items/http-handler-tests-never-green.md`
- Severity: major
- Confidence: high
- Действие: **инлайн**
- Оракул (мой, на этом прогоне):
- дословно `CLAUDE.md:98-101`: «`go test ./...` падает в
`internal/controller/http`: тесты требуют `testdata/sample.m4a`, которого в
репозитории нет и не было… Заведено задачей `http-handler-tests-never-green`»;
- `go test ./...``ok git.vakhrushev.me/av/transcriber/internal/controller/http 0.013s`;
- дословно `CLAUDE.md` § Работа: «Два объявленных долга из раздела „Гейт“
сломанным состоянием **не** считаются, пока их не закрыли задачами».
- Последствие: после мерджа проект будет письменно утверждать, что красный
`go test` в `internal/controller/http` — это норма. Ровно этот механизм записан в
журнале дефектов (`docs/review.md`, запись 2026-08-10): «тест, который никогда
не проходил, обнуляет сигнал всего пакета: настоящий отказ в нём становится
неотличим от привычного шума». Изменение восстанавливает сигнал в коде и
оставляет его выключенным в документе, по которому судят «сломано ли». Отказ
будет молчаливым: никто не станет разбираться в отказе, объявленном известным.
- Предложение: убрать первый из двух известных отказов в `CLAUDE.md` § Гейт
(остаётся только `golangci-lint`), закрыть задачу штатным путём каталога
(`tasks.py close` — реализованные в `REJECTED.md` не идут, у них есть коммит),
снять строку из `tasks/BACKLOG.md`. Задача `tasks.md` этого шага не содержит —
добавить его в чек-лист.
- Найдено проходом: `review-code`/конвенции; оракул и провенанс — триаж.
- Почему блокер, а не «стоит исправить»: `CLAUDE.md` § Работа — единственное
место, где записано, что считается сломанным. Пока оно врёт, определение
сделанного у следующей задачи опирается на неверный список. Правка
механическая, путь документирован, цена — минуты.
**Замечание о разделении обязанностей.** Общая согласованность документов между
собой и с кодом — работа скилла `av-dev-docs:healthcheck`, а не ревью
(`CLAUDE.md` § Гейт говорит это прямо). Здесь исключение узкое и обосновано: речь
не о дрейфе документации вообще, а о том, что **это самое изменение** закрывает
долг, поимённо перечисленный в `CLAUDE.md`, и без правки определение «сломано»
становится ложным в момент мерджа.
---
## Стоит исправить сейчас
### Переименование маршрута `POST /api/audio` в `main.go` уедет зелёным: тест ходит по своей копии регистрации
- Файл: `main.go:198-202` против `internal/controller/http/transcribe_test.go:140-144`
- Severity: minor
- Confidence: high
- Действие: **развилка**
- Оракул (мой, мутация в копии дерева, `/tmp/.../scratchpad/mut`):
`api.POST("/audio", …)``api.POST("/upload", …)` в `main.go`;
`go build ./...` проходит, `go test ./internal/controller/http/ -count=1`
`ok … 0.017s`. Тест зелёный при сломанном контракте.
Для сравнения — то, что теперь ловится: переименование тега
`json:"job_id"``json:"jobId"` роняет `TestCreateTranscribeJob_Success`
(«does not contain "job_id"»), удаление `s.jobRepo.Create(job)` роняет его же.
- Последствие: имена полей ответа изменение защитило (это и была починенная
`major`-находка), а путь маршрута — часть того же публичного контракта HTTP API,
объявленного в `CLAUDE.md` **необратимым**, — остался незащищённым. Внешняя
программа сломается молча, машина промолчит. Вероятность невысока (мутация
видна в диффе `main.go`), но класс тот же самый.
- Развилка для человека — правка трогает продуктовый код, а `design.md` объявил
это Non-Goal («код `internal/service` и `internal/controller/http` не правится»):
1. **Вынести регистрацию маршрутов** в экспортируемую функцию пакета
`internal/controller/http` (например, `RegisterRoutes(r gin.IRouter, h *TranscribeHandler)`),
звать её из `main.go` и из сборки теста. Цена: ~10 строк продуктового кода,
выход за объявленный Non-Goal задачи, зато контракт маршрута закрыт машиной.
2. **Оставить как есть**, записать в урожай отдельной задачей. Цена: контракт
маршрута остаётся на человеке до следующей задачи, которая и так трогает
`main.go` (например, `json-api-for-spa`).
3. **Оставить как есть и не заводить задачу**, приняв, что маршрут проверяется
глазами. Цена: класс дефекта известен, но не записан нигде — при следующем
промахе оракула не будет.
- Найдено проходом: `review-specs` (там — `minor`, «цена исправления выше цены
дефекта»); оракул мутацией — триаж.
Второго пункта в этой секции нет: остальное либо починено и перепроверено, либо
не имеет цены, оправдывающей правку (см. «Отсеяно» и «Урожай»).
---
## Гипотезы без доказательства
### Понижено оракулом: «зелёный прогон печатает ERROR-строки в stderr» — заявленного последствия нет
- Исходно: `review-code`/техника, `minor`. Часть починена (логгер сборки теста
уведён в `io.Discard`), остаток — `log.Printf("Err: %v", err)` в
`internal/controller/http/transcribe.go:45`.
- Мой оракул: `go test ./internal/controller/http/ -count=1 2>&1 | grep -c 'Err:'`
**0**. `go test` буферизует вывод пакета и на успехе его не печатает. Строка
видна только под `-v` или при прямом запуске собранного бинаря — то есть на
зелёном `task gate` признак ничем не размывается.
- Итог: последствие в формулировке находки не воспроизводится, находка снята.
Остаётся факт «продуктовый код пишет мимо `slog`» — он **уже записан** в
`docs/conventions/logging.md:161` как расхождение, новой находкой не является,
ушёл в promote (механизация правила).
### Понижено: «база `:memory:` без ограничения пула»
- Исходно: `review-code`/техника, `minor`, `Confidence: low`, «сегодня не
срабатывает». Оракула, показывающего отказ, нет ни у прохода, ни у меня.
Починка (`db.SetMaxOpenConns(1)`, одна строка, с комментарием почему) уже
внесена, безвредна и оставлена как есть. Действия не требует.
### Не понижалось, но перепроверено: две `major`-находки `specs`
Обе были заявлены с оракулом и обе починены. Я не поверил на слово и повторил
мутации в копии дерева — см. оракул в секции «Стоит исправить сейчас». Обе
мутации теперь роняют тест. Находки закрыты.
---
## Отсеяно
- **Мёртвая строка `router.MaxMultipartMemory` в сборке теста**
(`transcribe_test.go:138`, названо `review-code` ниже потолка). Проверено:
гиновский `MaxMultipartMemory` читается только в `c.FormFile`/`c.MultipartForm`,
а обработчик зовёт `c.Request.FormFile`, который использует собственный
`defaultMaxMemory` = 32 MiB — то же число. Поведение не меняется ни в тесте, ни
в `main.go`, стоимость следующего изменения не растёт, записанной конвенции нет.
Выброшено, а не смягчено.
- **Проектных ложноположительных (`docs/review.md` → «Типовые
ложноположительные», 4 пункта) в выводах не оказалось ни одного.** Ближайший
сосед — «Файлы и объекты не удаляются, диск растёт» — к отложенной находке про
файл-сироту **не относится**: та запись про отсутствие срока хранения, а
находка — про файл, на который нет ни задачи, ни записи в учёте. По этому
пункту ничего не отсеяно.
---
## Promote candidates
1. **Имена полей публичного ответа судятся по сырому JSON, а не по разобранной
структуре.** Приём в разобранную структуру переименовывает тег вместе с
ожиданием, и проверка теряет способность упасть — это ровно та `major`, что
нашлась здесь. Приём (`map[string]json.RawMessage` + `assert.Contains`)
сработал дважды в одном файле. Дома у правила пока нет: в `docs/conventions/`
файла про тесты нет. Кандидат в новый раздел конвенций.
2. **Стандартный `log` в продуктовом коде — механизировать, а не помнить.**
`docs/conventions/logging.md` уже пишет «**Механизировано:** ничего. Ни
`sloglint`, ни `forbidigo` в `.golangci.yml` не заведено» и поимённо называет
расхождение `internal/controller/http/transcribe.go`. Правило записано и
механизируемо, значит это не находка ревью, а `Promote candidate`: `forbidigo`
на `log.` в `.golangci.yml`.
3. **Регистрация маршрутов — одна на процесс и на тест** (производное от развилки
выше; актуально, если человек выберет вариант 2 или 3).
---
## Урожай
Формулировка → оракул → провенанс. Ничего из этого не чинится в этом изменении.
1. **Приём пишет имя файла пользователя в журнал — нарушение `critical`-инварианта.**
`internal/service/transcribe.go:107`:
`s.logger.Info("Creating transcribe job", "file_id", …, "file_name", fileName, …)`.
Оракул: дословно `CLAUDE.md` § Инварианты — «Содержимое записи остаётся
приватным. Текст расшифровки, **имя файла пользователя** и его сообщение в лог
не пишутся — только длина и идентификаторы. Нарушение необратимо: строки уже
уехали в журнал контейнера. **critical**». Плюс вопрос темы `security` в
`docs/review.md`. Путь общий с Telegram, то есть касается живых записей.
Провенанс: ревью дизайна; **отложено решением человека на чекпоинте**,
записано в `design.md` § Risks. Спека `intake` нормирует только хранилище и о
журнале молчит намеренно.
2. **Отказ чтения метаданных оставляет файл на диске без уборки.**
`internal/service/transcribe.go:129-133` (ветка `metaviewer.GetInfo`) против
`152-157` (ветка `fileRepo.Create`, где `os.Remove` есть). Оракул: чтение кода;
ни задачи, ни записи в учёте под такой файл нет — сопоставить его не с чем.
Провенанс: ревью дизайна; отложено решением человека (правка трогает три ветки
отказа и меняет поведение на диске).
3. **Настоящий `ffprobe` после этой правки не проверяется ничем.**
У `internal/adapter/metaviewer/ffmpeg` своего теста нет; разбор вывода
`ffprobe` не покрыт. Оракул: `go test ./...``? …/adapter/metaviewer/ffmpeg
[no test files]`. Провенанс: `design.md` § Risks, названо прямо. Формально
покрытие не потеряно — прежний тест проверял отказ `ffprobe` и выдавал его за
проверку приёма.
4. **Конвейер задач не покрыт ни одним тестом.**
`FindAndRunConversionJob`, `FindAndRunTranscribeJob`,
`FindAndRunTranscribeCheckJob`, `internal/controller/worker`. Три воркера
читают общий `*sql.DB`, теста с параллельным доступом нет. Оракул:
`go test ./...``[no test files]` у `internal/service` и
`internal/controller/worker`. Провенанс: `autotests`, вне scope задачи.
Совпадает со свойством, добавленным в `docs/review.md` («изменённое место
покрыто хоть одним **проходящим** тестом») и с вопросом темы `autotests`.
5. **Опечатка `transcibe` в тексте ошибки теперь закреплена проверкой.**
`internal/controller/http/transcribe.go:46` и
`transcribe_test.go:379``"Failed to create transcibe job"`. Оракул: обе
строки дословно. Исправление меняет тело ответа, то есть **публичный контракт
HTTP API**, объявленный в `CLAUDE.md` необратимым, — значит спрашивается у
человека и не делается походя. Провенанс: `review-specs`, ниже потолка.
Действия сейчас не требует: статус-кво зафиксирован сознательно.
6. **Требование «имя отправителя не попадает в хранилище» проверяется только
благополучными именами.** Проверено мной попутно (вопрос темы `security` из
`docs/review.md`: «не строится ли путь на диске из значения, пришедшего
снаружи»): обхода каталога нет **по построению**`filepath.Ext` не
пересекает разделитель пути, и на входах `../../../etc/passwd.m4a`,
`evil.m4a/../../x`, `/etc/passwd`, `a.b/../../c`, `..` результат всегда
`<uuid><ext>` внутри каталога хранения (оракул: прогон `filepath.Ext` +
`filepath.Join` на этих восьми входах, вывод снят на этом прогоне). Дефекта
нет; недостающее — сторожевой случай, который зафиксирует это свойство.
Провенанс: триаж.
---
## Границы покрытия
### План: темы, дома, глубины
Полностью воспроизведён в сводке выше вместе с исходом каждой темы. Тем без дома
нет, тем без отчёта нет. Дома тем `architecture`, `security` и `operations`
раздел «Инварианты» `CLAUDE.md`, глубина «сверка».
### Какие проходы запускались
- Ревью дизайна: `review-specs`, одна стадия, метка `small`.
- Ревью кода: `review-autotests``review-specs` + `review-code` → триаж. Режим —
по графу, метка `small`.
- **Не запускался `review-basics`**: своих тем у проекта нет — так сказал план.
Следствие названо ниже, в строке про корректор метки.
### Сработавшие потолки
- `review-specs` (код): **3 из 3**, потолок сработал. Ниже среза остались
названными: литералы сообщений об ошибке, включая опечатку `transcibe`;
расхождение сценария «Поля с записью нет» с тем, что делал тест (починено).
- `review-code`: **техника 3/3 — сработал**; **конвенции 1/2**; **инварианты 0/1**.
Ниже среза осталась названной мёртвая строка `router.MaxMultipartMemory`.
- `review-autotests`: **о своём потолке не сообщил**. Это находка о прогоне: по
контракту проход обязан сказать, сколько нашёл, каков был потолок и что
осталось за срезом. Судить, есть ли за его срезом что-то ещё, нечем.
- Триаж: потолок 3/4 не исчерпан (1 блокер, 1 в «стоит исправить»). Из-за потолка
**ничего не выброшено**.
### Что каждый запущенный проход не мог проверить в принципе
Ниже — по отчётам проходов; charter'ы агентов мне дословно не подавались, поэтому
это пересказ их собственных заявлений, а не цитата устава.
- `review-autotests`: судит наличие и зелёность проверок, а не правильность
нормы, которую они проверяют. Прогнал `go test` 5× подряд и с `-race` — флаки
не обнаружен; это отсутствие сигнала на пяти прогонах, а не доказательство
детерминированности.
- `review-specs`: судит соответствие кода дельта-спеке; правильность самой спеки
вне его входа. Приём из Telegram спекой `intake` не описан сознательно — значит,
и не проверялся.
- `review-code`: на метке `small` конвенции сверялись только с
`docs/conventions/README.md`, а `architecture`/`security`/`operations` — только
с записанными инвариантами `CLAUDE.md`.
- Триаж: **ничего нового не находит по определению**. Я не читаю код в поисках
дефектов, я работаю с чужими выводами. Пропуск любого прохода — мой пропуск
тоже; всё, что я могу, — назвать его поимённо, что и сделано выше.
### Что осталось целиком на человеке
Из `docs/review.md` → «Недоступно проверке», **двумя отдельными списками, как
записано**:
**Не проверит ни один проход:**
- `operations`: поведение внешних сервисов под нагрузкой и на границах — SpeechKit
и Object Storage поднять в тесте нечем;
- `operations`: реальный профиль нагрузки. Проект работает на единицах записей в
день, и утверждения о росте остаются условиями, а не замерами;
- `security`: стойкость `ffmpeg` к вредоносному входу — разбор чужого формата
отдан внешней программе, и она вне нашей границы.
**Перестали проверять сознательно:**
- по записи в `docs/review.md` — «Ничего не отключали: проверять пока и не
начинали». **Однако этим изменением список пополняется фактически**: приём
перестал проверяться сквозь настоящий `ffprobe` (решение записано в
`design.md` § Risks и обосновано — прежняя проверка проверяла отказ внешней
программы и выдавала его за проверку приёма). Раздел `docs/review.md` этого
ещё не знает; строку туда добавляет синк документации, не я.
Сверх записанного в проекте — общее, чего не видит ни один прогон: история
инцидентов, поведение под реальным потоком, поведение внешних систем в их
версиях, завязка потребителей на текущее поведение и вопрос «а нужна ли эта
функциональность вообще».
### Каких документов проекта не хватило
Строкой на каждый, с причиной — деградация поразрядная:
- `docs/conventions/` **про тесты файла нет**: конвенции покрывают конфиг, базу,
ошибки, журнал и веб-UI. Изменение целиком про тесты, и сверять его форму было
не с чем — отсюда promote-кандидат №1, а не находка.
- `docs/adr/` **пуст**: только `README.md` и `template.md`, ни одного решения. См.
обязательную строку 1 ниже.
- `docs/research/` — только `README.md`, записанных замеров нет. См. строку 2.
- `docs/review.md` § «Как настроен конвейер» **устарел с этого прогона**: там
написано «Конвейера ревью в проекте пока нет: плагин не подключён, ни одного
прогона не было». Прогон был — этот. Отсев ложноположительных при этом **не был
слепым**: раздел «Типовые ложноположительные» заполнен наперёд, четыре пункта, и
я им пользовался.
- `docs/security.md` в проекте **есть**, но на метке `small` план отправил тему
`security` в инварианты `CLAUDE.md`, и как дом темы `security.md` не
открывался. См. строку 5 ниже.
### Четыре строки, которых не принесёт ни один проход
1. **Решения проекта не сверялись.** `docs/adr/*` — процессный документ, прогон
его не открывает. Расхождение изменения с записанным решением ловит сверка
документации (скилл `av-dev-docs:healthcheck`), а не ревью. Здесь у этого есть
и вторая сторона: каталог решений пуст, сверять было бы не с чем.
2. **Записанные наблюдения проекта не использовались.** `docs/research/` — тоже
процессный. Всякое число в этом отчёте снято командой на этом прогоне; чисел
без приложенной команды в отчёте нет.
3. **Поимённая сверка с руководствами по стилю Go не задавалась ни одним
проходом.** Различение «идиоматично против просто распространено» на этом
прогоне не спрашивал никто.
4. **Альтернативной реализации, с которой можно сдиффить решения, у конвейера
нет.** Проход независимой реализации снят по стоимости, а не по замеру.
«Не знаю, чего не знаю» здесь никто не достаёт: например, вопрос «а верна ли
сама форма подстановки в сборке теста» не задал никто, кроме автора дизайна.
### Пятая строка — следствие метки `small`
Темы `security`, `operations` и `architecture` сверялись **только с записанными
инвариантами `CLAUDE.md`**; дома этих тем (`docs/security.md`,
`docs/architecture.md`, `docs/conventions/logging.md` как источник норм журнала)
не открывались. Свойство, которого нет в семи пунктах инвариантов, на этом
прогоне не проверил никто.
### Отдельно про корректор метки
`review-code` метку `small` подтвердил, сигнала о занижении не подал.
`review-basics` **не запускался**, поэтому второго, независимого от `review-code`
подтверждения метки нет. Согласия двух проходов здесь не было бы и при запуске:
несколько агентов — один источник, высказавшийся несколько раз; совпадение
подняло бы приоритет, но не `confidence`.