jellybit/docs/review.md

# Ревью: настройка и журнал

Проектная часть конвейера ревью: чем jellybit отличается от абстрактного
Go-сервиса и что здесь уже проскакивало. Устройство самого конвейера (метки,
стадии, контракт находок) живёт в скилле, а не здесь.

## Как настроен конвейер

**Прежде чем задать вопрос, посмотри, не задан ли он уже машиной.** Перечень
механизированного — [conventions/README.md](conventions/README.md) →
«Механизировано»; чем безусловно краснеет гейт и чего в нём намеренно нет —
[CLAUDE.md](../CLAUDE.md) → «Гейт». Вопрос про уже проверенное вытесняет вопрос
про непроверенное — места в прогоне столько же.

**В worktree гейт краснеет ложно, и это не находка.** Ветки задач живут в
`tmp/wt-<задача>` — внутри самого репозитория. Два следствия, оба наблюдались:

- кэш `golangci-lint` переживает смену каталога и отдаёт результаты прошлого
  прогона из **основного** дерева. Признак — пути в `tmp/gate/lint.log`
  начинаются с `../../internal/`, то есть указывают наружу worktree, и жалобы
  приходят на файлы, которых дифф не касался. Лечится
  `golangci-lint cache clean` перед прогоном;
- пробы проходов ревью, оставленные в `tmp/`, линтуются вместе с проектом:
  `.go`-файл со `fmt.Printf` в `tmp/` краснит шаг `lint` через `forbidigo`.
  Проход обязан за собой убирать, а оркестратор — сверять `tmp/` перед гейтом.

**Severity не выводится проходом заново.** Она стоит рядом с формулировкой
инварианта в [CLAUDE.md](../CLAUDE.md) → «Инварианты», обратимость — там же в
«Работа» → «Необратимое». Шкала ущерба берётся оттуда, а порядок ценностей —
из [security.md](security.md) → «Что чувствительнее чего».

### Типовые узлы

Рода узлов проекта и проверяемые свойства к каждому. Род, а не инвентарь
пакетов: узел, которого ещё нет, но который проект заведёт, включён намеренно.

**Клиент внешнего HTTP-сервиса** (`qbt`, `llm`, `metadata`, `jellyfin`, `tgbot`)

- у каждого исходящего вызова свой таймаут из конфига, не дефолт транспорта;
- `context` доходит до запроса и отменяет его, а не игнорируется;
- ошибка зависимости отличима от ошибки нашей логики на приёме результата;
- секреты (пароль, ключ, токен) не попадают ни в лог, ни в текст ошибки;
- недоступность **опциональной** зависимости (метабаза, Jellyfin, Telegram) не
  двигает состояние загрузки и не краснеет ERROR-ом в фоновом цикле.

**Тик воркера и переход состояния**

- переход легален по декларативному графу, а не «просто присвоили `state`»;
- работа идёт под per-download блокировкой; два транспорта не гонятся;
- тик идемпотентен: повтор на том же состоянии не порождает второго эффекта;
- отмена контекста на середине не оставляет полуприменённого состояния;
- новое промежуточное состояние имеет выход **и** предохранитель по времени.

**Репозиторий `store`**

- запрос параметризован, время только через `store.Now()`, id через `ident`;
- многошаговое изменение — в одной write-транзакции (`_txlock=immediate`);
- инвариант, не выражаемый схемой («одна активная загрузка на infohash»),
  держится guarded-методом, а не проверкой в вызывающем коде;
- миграция forward-only и сопровождается правкой [database.md](database.md).

**Операция с файловой системой** (`layout`)

- целевой путь проверяется **после** `filepath.Clean`, на принадлежность
  библиотеке;
- существующая цель не перезаписывается ни при каком исходе;
- под `paths.downloads` нет ни одной операции записи или удаления;
- частичный сбой батча оставляет систему в состоянии, из которого повтор
  доводит начатое или откатывает целиком;
- удаление снимает только свои ссылки своего батча и не снимает последнюю копию.

**Парсер недоверенного входа** (`magnet`, `torrent`, парсер сообщения бота,
разбор ответа LLM)

- вход враждебный по умолчанию: длина, вложенность, мусорные байты, пустота;
- разбор не паникует и не аллоцирует по числу из самого входа;
- невалидный вход даёт доменную ошибку, а не тихий дефолт;
- результат нормализуется на границе (lowercase hex, trim, `ident.Parse`).

**Вызов LLM и разбор его ответа** (`recognize`, `naming.Derive`)

Самый специфичный род узла в проекте: единственный, чей выход недетерминирован,
недоверен и стоит денег одновременно.

- решение узла **не является гейтом безопасности**: инъекция в промпт считается
  состоявшейся, защита стоит ниже — на валидации целевого пути
  ([security.md](security.md));
- самооценка модели не заменяет матч в метабазе — порог `confidence` стоит
  **поверх** матча дополнительным условием, а не вместо него;
- разбор ответа устойчив к лишним и недостающим полям, обрезке и не-JSON;
  негодный ответ даёт доменную ошибку, а не тихий дефолт;
- попытка стоит лимита и денег: число ретраев берётся из конфига, а фоновый цикл
  не запускает распознавание сам по себе повторно;
- тест не ходит в живой эндпоинт — провайдер за интерфейсом, ответ фикстурой;
  живые прогоны только за env-гейтом.

**htmx-хендлер**

- один партиал обслуживает страницу и фрагмент, ветвление по `isHTMX`;
- ошибка на htmx-пути — 200 плюс фрагмент, а не 4xx/5xx;
- страница деградирует без JS;
- поллинг самозавершается, когда наблюдать больше нечего.

### Типовые ложноположительные

Находки, которые здесь выглядят убедительно и всегда неверны.

- **«Веб-UI и REST без авторизации».** Принятое решение под сегодняшний
  периметр — [security.md](security.md). Дефектом станет только вместе с путём
  снаружи LAN.
- **«Ошибка на htmx-пути возвращает 200».** Так и задумано —
  [conventions/web-ui.md](conventions/web-ui.md).
- **«Решение auto/review должно опираться на `confidence` модели».** Неверно в
  форме «вместо матча»: авто только при подтверждённом матче в базе —
  [ADR-2026-06-13-auto-link-requires-db-match](adr/ADR-2026-06-13-auto-link-requires-db-match.md).
  Самооценка LLM плохо откалибрована и поддаётся инъекции. Порог `confidence`
  при этом существует **дополнительным** блокирующим условием поверх матча —
  его наличие дефектом не является.
- **«Копировать надёжнее, чем хардлинк» / «взять симлинк».** Хардлинк —
  осознанный выбор ради неприкосновенности источника и недублирования диска,
  [ADR-2026-06-13-hardlinks](adr/ADR-2026-06-13-hardlinks.md); copy — только
  фолбэк.
- **«Целочисленный автоинкрементный ключ был бы проще».** ULID — требование
  capability `identity`; `AUTOINCREMENT` вдобавок краснит гейт.
- **«Не хватает метрик, трейсинга, health-эндпоинтов по каждой зависимости».**
  «Минимум компонентов» — принцип проекта; глубокий healthcheck заведён задачей
  и ждёт своей очереди, а не является упущением.
- **«Здесь нужен интерфейс, чтобы это можно было замокать».** Единственная
  реализация за интерфейсом — обычно лишний слой; см. «Честный предел» ниже.
- **«Нет ретрая у вызова в фоновом цикле».** Тик повторится сам через
  `poll_interval`; ретрай внутри тика чаще вреден.
- **«Оригинальное и локализованное названия дублируются — избыточность».**
  `original_title` заполняется всегда и при неуверенности дублирует `title` —
  это контракт capability `recognition`, а не недосмотр.

### Вопросы по темам

Форма: `<тема>: <вопрос> (<провенанс>)`. Адресуется теме, а не имени прохода:
проход переезжает между метками и упраздняется, тема переезд переживает. Задаёт
вопрос тот, кто закрывает тему на текущем прогоне.

- `security`: можно ли, управляя только именами файлов в раздаче и текстом
  контекста, добиться целевого пути вне `paths.movies`/`series` — включая путь
  через юникод, длину сверх лимита ФС и коллизию после нормализации?
  (инвариант «целевой путь строго под библиотекой», [security.md](security.md))
- `security`: есть ли последовательность команд, после которой снимается
  **последняя** копия данных — с учётом `superseded`-ссылок и гонки со сверкой?
  (инвариант «источник неприкосновенен»)
- `security`: что даёт крафт-магнет с чужим или подставным инфохэшем —
  присоединение к чужой активной загрузке, отравление владения?
  (открытая задача про идентичность инфохэшей)
- `security`: где признак «это наше» снимается с одной сущности, а действие
  применяется к другой — присутствие раздачи в qBittorrent против байтов на
  диске, запись в БД против файла, инфохэш против содержимого? (журнал,
  2026-08-06: уборка своего торрента сносила чужие файлы)
- `operations`: что делает эта ветка, когда qBittorrent недоступен несколько
  минут подряд — сколько ERROR-строк в секунду и меняется ли состояние задач?
  (задача про ERROR-шторм фоновых циклов)
- `operations`: как это ведёт себя при сотне загрузок в базе и десятках тысяч
  `file_link` — есть ли запрос без индекса и полный проход по таблице?
  (задача про масштаб 100/1000, [database.md](database.md) → «Настройки»)
- `operations`: что остаётся на диске и в базе, если процесс убит посреди
  раскладки батча? (состояние `linking` и его восстановление)
- `autotests`: изменённые строки не просто исполнены тестом, а **проверены** —
  есть ли тест, который упал бы без этой правки? (diff-coverage в гейте меряет
  исполнение, отличить его от проверки машина не может)
- `autotests`: конкурентная часть правки проверена тестом, а не рассуждением?
  (`-race` без gcc уходит в `SKIP`, и тогда гонки не проверял никто —
  [CLAUDE.md](../CLAUDE.md) → «Гейт»)
- `autotests`: новый тест не ходит в живой qBittorrent, LLM, метабазу или
  Telegram — а если ходит, он за env-гейтом в `*_integration_test.go`?
  ([CLAUDE.md](../CLAUDE.md) → «Запреты»)
- `conventions`: логирующий чекпоинт один на операцию — или ошибка залогирована
  и возвращена вверх, где залогирована снова?
  ([conventions/logging.md](conventions/logging.md))
- `conventions`: новое поле конфига появилось в `config.example.toml` с
  описанием назначения, диапазона и единиц?
  ([conventions/config.md](conventions/config.md))
- `requirements`: не завелось ли поведение, которого спека не заказывала — тихий
  дефолт, проглоченная ошибка, ретрай «на всякий случай», отброшенное поле?
- `security`: читается ли тело ответа внешнего сервиса целиком без предела —
  лимита на размер ответа LLM в проекте нет, и это единственный недоверенный
  канал, где предел не стоит ([security.md](security.md) → «Что вне модели»)
- `operations`: не удваивает ли новая ветка расход лимита метабаз и платного
  LLM — повтор, ретрай, «распознать заново» на том же входе? (кэша ответов нет,
  задача `metadata-cache`)
- `operations`: копится ли то, что заводит это изменение, без срока хранения —
  строки терминальных задач, сырые ответы, файлы? (авточистки в проекте нет,
  задача `db-retention-cleanup`)
- `architecture`: не появился ли второй способ делать то, что уже делается —
  второе место, где генерится время или id, второй парсер источника, вторая
  логика целевых имён мимо `naming`?
  ([architecture.md](architecture.md) → «Единые точки проекта»)

### Триггеры метки

Уточняет умолчания конвейера, не отменяет их. Рабочее умолчание — `medium`;
миграция схемы и публичный контракт метку **не** поднимают: их проверяют
проходы, которые в `medium` и так есть. Списка три: два поднимают до `large`,
по одному на ось, третий опускает до `small`.

**Крупное здесь** — про объём, сколько узлов и слоёв трогает изменение:

- перенос ответственности между `worker`, `recognition`, `layout` и `store`;
- новое состояние в графе переходов загрузки: оно тянет за собой воркер, спеку,
  отображение в веб-UI и боте и восстановление после рестарта;
- правка, идущая насквозь по цепочке приём → распознавание → раскладка;
- новый провайдер метабазы за существующим интерфейсом: клиент, поле конфига с
  образцом, слияние полей кандидата, ветка «провайдера нет».

**Незнакомое здесь** — про форму решения, которую предстоит нащупать по ходу:

- новый пакет `internal/*` или новая capability в `openspec/specs/`;
- новый транспорт приёма или уведомлений рядом с REST, веб-UI, ботом и CLI;
- заводится или меняется **правило идентичности, слияния или разбора**: ключ
  владения раздачей и сверка с qBittorrent (`ident`, `internal/store`,
  `state-reconciliation`); построение целевых путей и санитизация имён
  (`internal/layout`, `naming`); новый вид входа или новая ветка неоднозначности
  у разбора недоверенного — bencode, magnet, текст контекста, ответ LLM
  (`internal/torrent`, `internal/magnet`, `internal/tgbot/parse.go`); новый
  источник или новый победитель при конфликте в слиянии кандидата метабазы
  (`internal/metadata`, `metadata-match`); merge-раскладка при повторном
  добавлении раздачи.

- «Поведение, видимое снаружи» здесь включает **тексты и карточки Telegram** —
  для единственного пользователя это и есть интерфейс.

**Место из перечня метку не поднимает — поднимает правило.** Перечни выше
отвечают «здесь такие правила водятся», а не «любая правка здесь идёт в
`large`». Метку поднимает то, что даёт работу новому проходу: заводится ключ
сравнения или меняется его состав; у разбора появляется новый вид входа или
новая ветка неоднозначности; в слияние добавляется источник или меняется
победитель при конфликте.

**Мелкое здесь** — опускает до `small`. Перечень закрытый, каждый пункт
проверяется взглядом на дифф и дельта-спеку, любое сработавшее держит метку
внизу:

- **дельта-спека называет исход поимённо** — сценарий уже говорит, что даёт
  вырожденный вход, и решать в коде нечего;
- **новых сценариев в дельта-спеке нет** — изменение уточняет уже описанное
  поведение, а не заказывает новое;
- **правка сообщения, комментария, записи журнала, имени или теста** в узле из
  перечней выше;
- **сужение уже существующей нормализации** без нового вида входа: вход остался
  тот же, изменился исход на одном его значении.

Отрицательный тест поверх перечня: что после мерджа не откатывается обратной
правкой — миграция, формат на диске, публичный контракт, имя, — **не** `small`,
каким бы маленьким ни был дифф.

**Ориентир частоты.** `medium` закрывает большинство задач, `large` рассчитана
на 5–10% и приходится на крупную функциональность, а не на уборку: задача типа
`chore` или `fix`, собранная из нитей прошлого ревью, идёт в `small` или
`medium`, даже когда трогает файл из перечней выше. `large` чаще одной задачи из
десяти означает ошибку в критерии, а не полосу сложных задач подряд.

### Недоступно проверке

**Не проверит ни один проход** — принципиальная граница, по факту промаха не
пересматривается.

- `operations`: история инцидентов на umbar и то, что уже ломалось в проде.
- `operations`: поведение таблицы SQLite под реальным объёмом и профилем
  нагрузки — реального профиля нет ни у кого, кроме сервера.
- `architecture`: завязка внешних потребителей (Jellyfin, закладки, чужие
  ссылки) на текущее поведение.
- `architecture`: суждение «этой функциональности не должно существовать».
- `requirements`: качество распознавания как таковое — правильно ли LLM
  определил фильм. Это вопрос тюнинга модели и промпта, а не ревью кода;
  размеченный корпус, по которому это можно было бы судить числом, решено не
  собирать (`tasks/REJECTED.md`, 2026-08-06).

**Перестали проверять сознательно** — пересматривается первым, как только
что-то проскочило.

- `conventions`: **идиоматичность Go — с 2026-08-04.** Проектный проход `idiom`
  (поимённая сверка с положениями Effective Go, Go Code Review Comments,
  стайлгайдов Uber и Google) упразднён вместе с переездом конвейера в плагин
  ([ADR-2026-08-04-review-pipeline-to-plugin](adr/ADR-2026-08-04-review-pipeline-to-plugin.md));
  способные части переселены — в тему `operations` (эксперимент против
  поведения библиотеки и драйвера) и в `architecture` («не изобретаем ли то,
  что уже есть в библиотеке»). **Различение «идиоматично против
  распространено» теперь не спрашивает никто.** Класс обратимый: портит форму
  кода, не данные. Пересмотр — задача `quality-review-agents`.
- `security`, `operations`, `architecture`: на метках `small` и `medium` не
  проверяется ничто, требующее запуска, — построенных путей атаки, замеров и
  эксплуатационного постмортема там нет по устройству конвейера. Их даёт только
  `large`, а она приходится на 5–10% задач.

## Журнал дефектов

Запись на каждый воспроизведённый дефект, **сразу**, а не ретроспективно: со
временем теряется не факт, а причина непоймания. Проскочившие — эвал-сет для
калибровки конвейера, выборка по пометке.

Форма:

<!-- копия: журнал-дефектов-форма из av-dev-pipeline/skills/review-pipeline/references/review-journal.md -->
## ГГГГ-ММ-ДД — <краткое последствие> [проскочил|пойман]

- **Где:** путь:строка либо «конвейер, а не код»
- **Симптом:** как обнаружилось, кем и когда
- **Причина:** что на самом деле было не так
- **Чем воспроизведён:** тест, команда, замер — с числами
- **Почему не поймали:** только для проскочивших — какой проход обязан был найти
  и что ему помешало
- **Что меняем:** правило прохода, шаг гейта, конвенция, факт в документе
  проекта — либо «ничего, цена поимки выше цены дефекта»
<!-- /копия: журнал-дефектов-форма -->

### Записи

Журнал заведён 2026-07-23 вместе с переработкой конвейера
([ADR-2026-07-23-review-pipeline-generative](adr/ADR-2026-07-23-review-pipeline-generative.md));
случаи до этой даты не восстанавливались — восстановленная постфактум причина
непоймания недостоверна, а именно она и нужна.

## 2026-08-06 — уборка своего торрента после отмены сносит чужие файлы [проскочил]

- **Где:** `internal/worker/worker.go:501-556` — гард `:501-509`, удаление
  `:550`. Норма — `openspec/specs/download-tracking/spec.md`, требование
  «Добавление пойманной загрузки в qBittorrent».
- **Симптом:** найден проходом `adversary` на ревью задачи
  `dismiss-marker-lost` (2026-08-06), не в эксплуатации. В проде не всплывал.
- **Причина:** гард «подтверждённое отсутствие непосредственно перед `add`»
  подтверждает отсутствие **записи торрента** в qBittorrent, но не отсутствие
  **данных** на диске. Пользователь, снявший раздачу из qBittorrent с
  сохранением файлов (`download-tracking` сама предписывает это как способ
  восстановления зависшей magnet-раздачи), и подавший тот же торрент заново,
  получает `add`, подхватывающий пред-существующие файлы. Отмена в окне между
  re-read и `PromoteCatched` даёт `torrents/delete` с `deleteFiles=true` по
  этим файлам. Исключение инварианта «источник неприкосновенен» покрывает
  «собственный торрент», а признак «своё» подменён на «торрента не было».
- **Чем воспроизведён:** тестом на фейковом клиенте qBittorrent во временном
  каталоге прогона (`tmp/`, не сохранён): `Cancel` в окне после `add` вызывает
  `Delete(hashes, deleteFiles=true)` при живом файле под `paths.downloads`,
  созданном до `add`. Тот же путь достижим через `Dismiss` — гейт `Dismiss`
  шире, а уборка срабатывает по состоянию (`after.State != catched`), а не по
  команде. **Не прогонялся** последний шаг — что боевой qBittorrent по
  `deleteFiles=true` физически сносит пред-существующий контент: в бой ходить
  запрещено, отсюда `Confidence: medium`.
- **Почему не поймали:** окно после `add` разбиралось как **гонка** (кто
  успел — отмена или промоушен) и проверялось на «не удалим ли чужой торрент».
  Вопрос «а если торрента нет, но данные есть» не задавал никто: ни один
  проход не спрашивал про **асимметрию признака владения** — признак снимается
  с одной сущности (запись в qBittorrent), а действие применяется к другой
  (байты на диске). Враждебный проход до этой задачи на данном коде не гонялся.
- **Что меняем:** в «Вопросы по темам» добавлен вопрос темы `security` про
  асимметрию признака владения. Сам дефект — задачей в беклоге, кандидат
  `critical`; спека `state-reconciliation` в том же изменении перестала
  утверждать, что уборка «данных пользователя не касается».

## 2026-08-06 — нормализация имени раздачи сама производила сентинел, который отбрасывала [пойман]

- **Где:** `internal/torrent/torrent.go`, функция `displayName` (введена
  изменением `ingest-nits`). Норма — `openspec/specs/ingest/spec.md`, требование
  «Вырожденное имя раздачи не считается именем».
- **Симптом:** найден на ревью изменения, до мерджа. В эксплуатации не был.
- **Причина:** сравнение с `metainfo.NoName` стояло **до** схлопывания
  пробельного. Имя `" - "` сравнение не проходило, а `oneLine` превращал его
  ровно в `-`, и вырожденное значение уезжало вниз по потоку всеми тремя
  путями: строкой названия в контексте распознавания, в `source_ref` (фолбек на
  имя присланного файла не срабатывал — строка непуста) и подсказкой вывода
  отображаемого имени. Против `master` это **регрессия**: там стояло
  `strings.TrimSpace(...)` перед сравнением с `-`, и фолбек работал.
- **Чем воспроизведён:** двумя независимыми падающими тестами — `adversary`
  прогнал приём на входах `" - "`, `"-\n"`, `"\t-"`, `" -"` и получил
  `source_ref = "-"` вместо `Dune.torrent`; `reimpl` принёс свой тест на разбор.
  В дереве остался табличный `TestParseNoNameSentinelDropped`.
- **Почему не поймали раньше:** ловить было нечему — дефект внесён этим же
  изменением и пойман тем же прогоном. Отмечено потому, что это **эвал-сет
  наоборот**: случай, где старшая метка окупилась. Три прохода из семи
  (`specs`, `adversary`, `reimpl`) нашли его независимо, и двое принесли оракул;
  проход `code` (конвенции) и гейт его не видели — порядок двух операций внутри
  функции не выражается ни правилом линтера, ни конвенцией.
- **Что меняем:** ничего в конвейере. Класс «нормализация и сравнение с
  константой идут в неверном порядке» дешевле ловить тестом на границе разбора,
  чем правилом; такой тест заведён. Наблюдение о самой библиотеке (что
  `BestName()` сентинел **не** синтезирует — исходное основание нити было
  неверным) записано в `research/torrent-bencode-limits.md`.