ревью: скилл review-pipeline — стадии, профили, контракт находок, храповик
Конвейер собран по типу проходов, а не по ролям: гейт (детерминированный, блокирующий) → сверка с дельта-спеками в обе стороны → generative-проходы → архитектура → враждебные постановки → обязательный триаж. Профили quick / standard / deep / design с правилом выбора по факту изменения. Контракт находок: заголовок через последствие, обязательное поле «Последствие», critical без оракула или построенного пути не существует, потолок 7 пунктов и разметка «инлайн | развилка» — отчёт читает оркестратор и молча реализует прочитанное, поэтому потолок защищает код от незаказанных правок. Храповик находка → конвенция → правило → удаление из прозы и промптов; журнал проскочивших дефектов и калибровка инъекцией с вердиктами keep/retune/drop. Секция границ покрытия обязательна: отчёт без неё потребляет ощущение проверенности, ничего не гарантируя. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,73 @@
|
||||
# Калибровка проходов
|
||||
|
||||
Без измерения набор проходов растёт монотонно и вырождается в театр: каждый
|
||||
кажется полезным, потому что иногда что-то говорит. Калибровка отвечает на
|
||||
единственный вопрос — **ловит ли проход дефект своего класса**.
|
||||
|
||||
## Процедура (инъекция дефекта)
|
||||
|
||||
1. Взять **реальный коммит** из истории (`git log --oneline`), лучше
|
||||
архивированный change с непустым диффом.
|
||||
2. Внести в него **один** дефект того класса, который проход обязан ловить по
|
||||
своему charter'у. Дефект должен быть правдоподобным — таким, какой реально
|
||||
пишет модель, а не карикатурой (`panic("TODO")` не считается).
|
||||
3. Прогнать **только этот проход** на подготовленном диффе — **три раза**,
|
||||
каждый в чистом контексте.
|
||||
4. Зафиксировать: нашёл `n/3`, число находок всего, число ложных.
|
||||
5. Вердикт:
|
||||
|
||||
| Результат | Вердикт | Что делаем |
|
||||
|---|---|---|
|
||||
| нашёл 3/3 или 2/3, ложных немного | `keep` | ничего |
|
||||
| нашёл 1/3 или 0/3 | `retune` | правим charter — сужаем вход, убираем чек-лист, добавляем оракул |
|
||||
| `retune` уже был дважды подряд | `drop` | удаляем проход |
|
||||
| находит, но ложных больше трети от всех находок | `retune` | триаж съедает больше, чем экономит проход |
|
||||
|
||||
**`retune` не более двух раз подряд.** Проход, не находящий дефект своего класса
|
||||
в 2 из 3 прогонов после двух правок промпта, — это театр. Удалять, а не
|
||||
бесконечно править формулировки: каждая итерация правки промпта стоит дороже,
|
||||
чем отсутствие прохода.
|
||||
|
||||
**Существующий проход не удаляется без замера.** Сначала калибровка, потом
|
||||
решение — иначе удаляется то, что работало, а остаётся то, что громче.
|
||||
|
||||
## Пробы дефектов по проходам
|
||||
|
||||
Проба — заготовка инъекции. Список пополняется из
|
||||
[журнала проскочивших дефектов](../../../../docs/review/journal.md): реальный
|
||||
проскочивший дефект — лучшая проба, какая вообще возможна, потому что
|
||||
синтетические смещены в сторону тех, которые уже умеешь придумывать.
|
||||
|
||||
| Проход | Класс дефекта для инъекции | Заготовка пробы |
|
||||
|---|---|---|
|
||||
| `jellybit-review-gate` | отсутствующая верификация | убрать тест на изменённую ветку, оставить код рабочим |
|
||||
| `jellybit-review-specs` | поведение вне спеки | добавить незаказанный фолбэк-дефолт при пустом ответе LLM |
|
||||
| `jellybit-review-rubric` | нарушенное свойство узла | в клиенте внешнего API убрать таймаут/протяжку `context` |
|
||||
| `jellybit-review-reimpl` | форма решения | размазать решение по трём слоям там, где хватало одной функции |
|
||||
| `jellybit-review-idiom` | «распространённое» вместо идиоматичного | завести интерфейс с одной реализацией ради мока |
|
||||
| `jellybit-review-negative` | отсутствующее | убрать `state transition` из новой стадии воркера |
|
||||
| `jellybit-review-architecture` | второй способ | завести вторую точку генерации id мимо `internal/ident` |
|
||||
| `jellybit-review-adversary` | построенный путь | принять внешний id без `ident.Parse` до запроса в БД |
|
||||
| `jellybit-review-ops` | деградация зависимости | убрать обработку недоступности qBittorrent в фоновом цикле |
|
||||
| `jellybit-review-triage` | шум | подать 20 находок, из них 15 вкусовщина и 3 дубля — проверить потолок и дедуп |
|
||||
|
||||
## Дополнительные метрики
|
||||
|
||||
Собираются попутно, в журнале прогонов (свободной формой, там же где вердикты):
|
||||
|
||||
- **precision** — доля находок, отвергнутых при ревью. Если отвергается меньше
|
||||
трети — хорошо; если больше — триаж съедает больше, чем экономит проход.
|
||||
- **корреляция между проходами** — стабильно одинаковые находки у двух проходов
|
||||
означают, что один лишний. Смотрится по полю `Найдено проходом` в
|
||||
триажированных отчётах.
|
||||
- **стоимость прогона** — токены и время. Профиль `deep` дороже `standard`
|
||||
примерно на порядок; если разница в найденном не видна, состав профиля
|
||||
пересматривается.
|
||||
|
||||
## Когда калибровать
|
||||
|
||||
- при заведении нового прохода — **до** включения в профиль по умолчанию;
|
||||
- при правке charter'а существующего — иначе непонятно, правка помогла или нет;
|
||||
- при появлении записи в журнале проскочивших дефектов — калибруем тот проход,
|
||||
который должен был поймать;
|
||||
- планово — нет. Календарная калибровка ради галочки сама превращается в театр.
|
||||
@@ -0,0 +1,83 @@
|
||||
# Контракт находок
|
||||
|
||||
Единый формат для всех проходов конвейера ревью. Проход, нарушивший контракт,
|
||||
считается сломанным — триаж вправе выбросить его вывод целиком.
|
||||
|
||||
## Форма находки
|
||||
|
||||
```
|
||||
### <краткая формулировка ПОСЛЕДСТВИЯ, не симптома>
|
||||
- Файл: internal/layout/link.go:120-134
|
||||
- Severity: critical | major | minor | nit
|
||||
- Confidence: high | medium | low
|
||||
- Оракул: <падающий тест / команда с выводом / положение гайда / нет>
|
||||
- Последствие: <что произойдёт и при каких условиях>
|
||||
- Предложение: <конкретное изменение>
|
||||
- Найдено проходом: <имя агента>
|
||||
```
|
||||
|
||||
## Правила
|
||||
|
||||
- **Заголовок через последствие.** Не «нет проверки владельца», а «пользователь
|
||||
может прочитать чужой заказ по id». Не «путь не санитизируется», а «архив с
|
||||
`../` в имени файла разложит хардлинк вне `paths.movies`». Симптом в
|
||||
заголовке — это заявка на то, что читатель сам достроит последствие; он не
|
||||
достроит, он просто починит симптом.
|
||||
- **`critical` без оракула или построенного пути не существует.** Оракул — это
|
||||
падающий тест, вывод выполненной команды или поимённое положение гайда. Не
|
||||
«вероятно, здесь гонка», а `CGO_ENABLED=1 go test -race` с выводом детектора.
|
||||
- **`confidence: low` — это «так обычно пишут».** Такие находки допустимы, но не
|
||||
поднимаются выше `minor`. Частотность конструкции в публичном Go — не аргумент.
|
||||
- **Находка без поля «Последствие» не выводится вовсе.** Пустое «Последствие:
|
||||
ухудшает читаемость» равносильно отсутствию поля.
|
||||
- **`nit` допустим только при нарушении записанной конвенции** — со ссылкой на
|
||||
файл и раздел `docs/conventions/*` либо на правило `.golangci.yml`. Если
|
||||
правило механизируемо, но не механизировано — это не находка ревью, это
|
||||
`Promote candidate` (см. [promote.md](promote.md)).
|
||||
- **Расхождение — не дефект, пока не названо последствие.** Особенно для
|
||||
`jellybit-review-reimpl`: «я бы сделал иначе» без последствия не выводится.
|
||||
|
||||
## Шкала severity
|
||||
|
||||
| Severity | Что это | Пример |
|
||||
|---|---|---|
|
||||
| `critical` | нарушение инварианта безопасности данных, потеря/порча данных, утечка секрета, построенный путь к отказу | хардлинк за пределы `paths.movies`, пароль qBittorrent в поле лога |
|
||||
| `major` | сломанное требование дельта-спеки, необрабатываемый отказ штатного сценария, флаки-тест, поведение вне спеки, меняющее исход | ретрай, которого нет в спеке, маскирует ошибку записи в БД |
|
||||
| `minor` | отступление от конвенции с реальной ценой, отсутствующая наблюдаемость, дублирование, которое разойдётся | стадия воркера не пишет `state transition`, разбор по логам невозможен |
|
||||
| `nit` | нарушение записанной конвенции без последствий за пределами чтения | `msg` с интерполяцией вместо константы |
|
||||
|
||||
## Блок границ покрытия
|
||||
|
||||
Каждый проход завершает вывод этим блоком. Он не сокращается и не заменяется
|
||||
фразой «всё проверено».
|
||||
|
||||
```
|
||||
## Coverage of this pass
|
||||
- проверено: <что реально прочитано/запущено, с путями и командами>
|
||||
- не проверялось и почему: <бюджет, недоступный инструмент, вне входа>
|
||||
- принципиально недоступно этому проходу: <из charter'а агента>
|
||||
```
|
||||
|
||||
## Финальный отчёт триажа
|
||||
|
||||
Секции строго в этом порядке, потолок — 7 пунктов в первых двух:
|
||||
|
||||
1. `Блокирует мердж` (≤3, каждая с оракулом);
|
||||
2. `Стоит исправить сейчас` (≤4);
|
||||
3. `Гипотезы без доказательства` — что понижено и почему;
|
||||
4. `Promote candidates` — кандидаты в конвенцию или правило линтера;
|
||||
5. `Границы покрытия` — сводная, обязательная.
|
||||
|
||||
Каждая находка в секциях 1–2 несёт дополнительное поле:
|
||||
|
||||
```
|
||||
- Действие: инлайн | развилка
|
||||
```
|
||||
|
||||
`инлайн` — оркестратор чинит сам, не спрашивая и не логируя. `развилка` — цена
|
||||
исправления сопоставима с переработкой, либо выбор меняет scope, либо решение
|
||||
трогает инвариант: идёт человеку через `AskUserQuestion` вопросом с вариантами.
|
||||
|
||||
Потребитель отчёта — оркестратор, который **реализует прочитанное**. Поэтому
|
||||
потолок в 7 пунктов — не забота о внимании читателя, а защита кодовой базы от
|
||||
правок, которых никто не заказывал.
|
||||
@@ -0,0 +1,77 @@
|
||||
# Отчёт о переработке конвейера ревью (2026-07-23)
|
||||
|
||||
Что было, что стало и на основании чего. Обоснование «почему» —
|
||||
[ADR-2026-07-23-review-pipeline-generative](../../../../docs/adr/ADR-2026-07-23-review-pipeline-generative.md).
|
||||
|
||||
## Было
|
||||
|
||||
Отдельного скилла ревью не существовало. Ревью — это два сабагента
|
||||
(`jellybit-review-specs`, `jellybit-review-code`), вызываемые из шагов 4 и 7
|
||||
`task-pipeline`, плюс дубль в финальной сверке `task-batch`. Детерминированные
|
||||
проверки жили отдельно и **после** опиниативных: `lefthook` срабатывал на
|
||||
коммите, `task test`/`task lint` — внутри apply.
|
||||
|
||||
Диагностика показала: все проходы applicative, generative нет ни одного; ни один
|
||||
проход не запускает инструменты (обоим это было прямо запрещено); сверка со
|
||||
спекой односторонняя; архитектурный угол судит по диффу; триажа нет; границ
|
||||
покрытия нет; измерения качества нет. Плюс два мелких долга: ссылка на
|
||||
несуществующий скилл `verify` и дублирующие проходы в `task-batch`.
|
||||
|
||||
## Стало
|
||||
|
||||
| Было | Стало | На основании чего |
|
||||
|---|---|---|
|
||||
| `task test`/`task lint` внутри apply, lefthook на коммите | **Stage 0** `jellybit-review-gate` + `task gate`: build, vet, lint, gofmt, тесты, повтор на флаки, `-race`, покрытие изменённых строк, миграции, ER-схема, gitleaks, govulncheck. Блокирует опиниативные проходы | детерминированный оракул надёжнее мнения; проверка, идущая после ревью, не защищает ревью |
|
||||
| `jellybit-review-specs`: spec → code | **Stage 1** он же + **code → spec** (тихие ветки, самодеятельные дефолты, проглоченные ошибки, незаказанные ретраи), границы спеки, право сказать «требование неверно» | системная болезнь агентского кода — тихо добавленное поведение; односторонняя сверка его не видит |
|
||||
| — | **Stage 2** `jellybit-review-rubric`, `jellybit-review-reimpl`, `jellybit-review-idiom`, `jellybit-review-negative` | recall чек-листа равен его длине; неявный слой достаётся только порождением критерия |
|
||||
| архитектура как один из 9 буллетов `review-code`, вход = дифф | **Stage 3** `jellybit-review-architecture`, вход = `task review:context` (пакеты, граф зависимостей, публичная поверхность, инвентарь концепций) + дифф. Потолок 3 находки | агент, видящий только дифф, не знает словаря проекта и потому не может судить о втором способе делать то же самое |
|
||||
| — | **Stage 4** `jellybit-review-adversary` (находка = построенный путь), `jellybit-review-ops` (условный постмортем) | враждебная постановка находит то, чего не находит перечисление свойств |
|
||||
| разгребал оркестратор вручную | **Stage 5** `jellybit-review-triage`: дедуп по причине, оракул для critical/major, понижение неподтверждённого, отсев вкусовщины, потолок 7, разметка `инлайн`/`развилка` | отчёт читает оркестратор и молча реализует прочитанное: без потолка узкое место переезжает в незаказанные правки кода |
|
||||
| `review-code`: 9 углов, включая механизируемое | `review-code` сжат до конвенций, **не выраженных правилом** | всё, что проверяет линтер, в промпте только отвлекает внимание |
|
||||
| ревью-проходы дублировались в `task-batch` | в `task-batch` осталось **только то, что появилось от слияния**: рассинхроны на стыках + вопрос о втором способе | те же проходы на тех же файлах дают те же находки и удорожают триаж |
|
||||
| ссылка на несуществующий скилл `verify` | Skill `run` | скилла `verify` нет ни в проекте, ни у пользователя — шаг молча не выполнялся |
|
||||
|
||||
## Что слито и что удалено
|
||||
|
||||
- **Слито:** архитектурный угол и стиль/дублирование выведены из
|
||||
`jellybit-review-code` в отдельные проходы с разными классами дефектов;
|
||||
per-capability прогон в `task-batch` сужен до стыков вместо повторного полного
|
||||
ревью.
|
||||
- **Удалено:** ничего. Существующий проход не удаляется без замера — сначала
|
||||
калибровка (`calibration.md`), потом решение. `jellybit-review-code` оставлен
|
||||
как дешёвый проход профиля `quick`.
|
||||
|
||||
## Конвенции → правила
|
||||
|
||||
Механизировано и вычеркнуто из прозы (`docs/conventions/*`) и из
|
||||
`openspec/config.yaml`:
|
||||
|
||||
| Правило | Инструмент | Откуда убрано |
|
||||
|---|---|---|
|
||||
| `msg` — константа, без интерполяции; стиль ключ-значение; ошибка полем | `sloglint` | logging.md |
|
||||
| `slog` вместо `fmt.Print*` | `forbidigo` | logging.md |
|
||||
| конфиг не из env | `forbidigo` (`os.Getenv`) | config.md |
|
||||
| время только через `store.Now()` | `forbidigo` (`time.Now`) | database.md |
|
||||
| `err == ErrX`, приведение типа ошибки | `errorlint` | errors.md |
|
||||
| сторонние пакеты ошибок | `depguard` | errors.md |
|
||||
| матчинг ошибки по тексту сообщения | `internal/archrules` | errors.md |
|
||||
| `AUTOINCREMENT`, `DEFAULT (datetime('now'))` в новых миграциях | `internal/archrules` | database.md |
|
||||
| транспорты не знают друг о друге, ядро не знает о транспортах | `internal/archrules` | CLAUDE.md (осталась одна строка принципа) |
|
||||
| ER-схема обновлена вместе с миграцией | `scripts/gate.sh` (по диффу) | — |
|
||||
|
||||
Правки кода под новые правила: `logging.StartCall` как единая точка отсчёта
|
||||
длительности внешних вызовов, `store.Now` вместо `time.Now` в `httpapi` и
|
||||
часах воркера, `slog.DiscardHandler` в тестах.
|
||||
|
||||
## Что осталось непокрытым намеренно
|
||||
|
||||
- **Ревьювер наименований** по словарю единого языка — глоссария нет, проверять
|
||||
не по чему. Заводится после задачи «Словарь единого языка».
|
||||
- **Дробление `review-code` на узкие оптики** — отклонено: декорреляция внимания
|
||||
без декорреляции суждения почти не добавляет recall, но линейно удорожает
|
||||
триаж.
|
||||
- **Профиль нагрузки, история инцидентов, завязка внешних потребителей** —
|
||||
недоступны ни одному проходу и остаются человеку. Перечислены в разделе
|
||||
«Честный предел» скилла.
|
||||
- **Калибровка проходов не проведена**: процедура заведена, первые прогоны — за
|
||||
пользователем (журнал проскочивших дефектов пока пуст).
|
||||
@@ -0,0 +1,89 @@
|
||||
# Промоут: находка → конвенция → правило → удаление
|
||||
|
||||
Механизм храповика. Без него конвейер выдаёт одни и те же находки бесконечно, а
|
||||
конвенции не растут — то есть внимание тратится повторно на уже решённое.
|
||||
|
||||
Роли уровней:
|
||||
|
||||
- **generative-проходы** — механизм *открытия* неявного (дорого, шумно, но
|
||||
только они достают то, чего нет в списках);
|
||||
- **конвенции** — дешёвая *регрессионная сетка* на уже открытое;
|
||||
- **правила линтера** — то же с детерминированным оракулом и нулевой ценой
|
||||
внимания.
|
||||
|
||||
## Шаг 1. Находка → конвенция
|
||||
|
||||
Условия: находка **принята** при ревью (не отвергнута, не понижена в гипотезу) и
|
||||
**не специфична для одного места**.
|
||||
|
||||
- Формулируется как **проверяемое свойство**, а не как совет: «уровень доменного
|
||||
отказа выбирает единственный логирующий чокпоинт», а не «внимательнее с
|
||||
уровнями логов».
|
||||
- Записывается источник — какой проход нашёл. Это единственные данные для
|
||||
калибровки: проход, чьи находки регулярно доезжают до конвенции, оправдан;
|
||||
проход, чьи находки не доезжают никогда, — кандидат на `drop`.
|
||||
- Место записи — соответствующий файл `docs/conventions/*.md`. Если тема
|
||||
относится к поведению системы, а не к тому, как мы пишем код, — это не
|
||||
конвенция, а требование: заводится дельта-спека OpenSpec обычным путём.
|
||||
|
||||
Промоут идёт **тем же путём, что change → spec**: правка попадает в тот же
|
||||
коммит, что и исправление кода, с пометкой в сообщении — история промоутов
|
||||
видна в `git log docs/conventions/`.
|
||||
|
||||
## Шаг 2. Конвенция → правило
|
||||
|
||||
Как только свойство выражается детерминированно, оно переезжает в инструмент.
|
||||
Порядок предпочтения — от дешёвого к дорогому:
|
||||
|
||||
1. **готовый линтер** в `.golangci.yml` (`sloglint`, `errorlint`, `depguard`,
|
||||
`forbidigo`, `misspell`, стандартный набор v2);
|
||||
2. **`forbidigo`/`depguard` с собственным паттерном** — запрет идентификатора или
|
||||
импорта;
|
||||
3. **`revive`/`gocritic` с настройкой** — когда нужна форма, а не имя;
|
||||
4. **тест-сканер исходников** `internal/arch_test.go` — когда правило про
|
||||
структуру проекта или SQL: направление зависимостей, `AUTOINCREMENT` в
|
||||
миграциях, матчинг ошибки по тексту, бизнес-логика в транспорте;
|
||||
5. **`go/analysis`-анализатор** — последний рубеж, заводим только если 1–4 не
|
||||
выражают правило.
|
||||
|
||||
Правило обязано быть **зелёным на текущем коде в момент включения**: иначе
|
||||
lefthook блокирует любой коммит, и правило снимут первым же раздражённым
|
||||
движением. Приводить код в соответствие — часть шага 2, отдельным коммитом.
|
||||
|
||||
## Шаг 3. Удаление из конвенций и из промптов
|
||||
|
||||
**Шаг, который пропускают чаще всего, и единственный, ради которого затевались
|
||||
первые два.**
|
||||
|
||||
Как только правило работает:
|
||||
|
||||
- из `docs/conventions/*.md` убирается формулировка правила; остаётся, если
|
||||
нужно, одна строка «проверяется линтером `<имя>`» — но только там, где без неё
|
||||
раздел теряет связность;
|
||||
- из charter'ов агентов (`.claude/agents/jellybit-review-*.md`) убирается
|
||||
соответствующий пункт;
|
||||
- из `openspec/config.yaml` → `context` убирается дубль, если он там был.
|
||||
|
||||
Практический критерий: **в прозаических конвенциях остаётся только то, что
|
||||
принципиально не выражается правилом.** Файл конвенций на несколько сотен строк
|
||||
размазывает внимание модели по тривиальному — она добросовестно проверит
|
||||
именование полей лога и не дойдёт до формы решения. Каждая строка конвенций,
|
||||
которую можно было бы проверить машиной, оплачивается непойманным дефектом
|
||||
где-то ещё.
|
||||
|
||||
## Обратное движение
|
||||
|
||||
Правило, которое даёт ложные срабатывания чаще, чем ловит (порядка трети от
|
||||
общего числа), снимается и возвращается в прозу — или удаляется совсем, если
|
||||
свойство перестало быть важным. Снятие фиксируется там же, где включалось, с
|
||||
одной строкой «почему».
|
||||
|
||||
## Что промоуту не подлежит
|
||||
|
||||
- Находка, специфичная для одного места (её лечит комментарий в коде).
|
||||
- Вкусовщина: не меняет поведения, не влияет на стоимость следующего изменения,
|
||||
не нарушает записанного. Такое выбрасывается на триаже и не хранится.
|
||||
- Свойство, требующее знания рантайма (профиль нагрузки, история инцидентов) —
|
||||
его нельзя проверить ни промптом, ни линтером; место такому — в
|
||||
[journal.md](../../../../docs/review/journal.md) как «признано
|
||||
неавтоматизируемым».
|
||||
Reference in New Issue
Block a user