diff --git a/docs/reviews/CODE-REVIEW-695-r1.md b/docs/reviews/CODE-REVIEW-695-r1.md new file mode 100644 index 00000000..39ee2745 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-695-r1.md @@ -0,0 +1,210 @@ +# CODE-REVIEW #695 r1 + +Материал: `origin/dev..HEAD`, ровно один коммит +`57ce10721fc0e48cb48b20798d7fb861b4c75ba8` поверх `dev@7d4d75bd`. +Диапазон: `PROCESS.md`, `AGENTS.md`, `docs/process/AUTHOR.md`, +`docs/process/REVIEWER.md`, `.github/workflows/_process.yml`, +`scripts/mutation-registry.mjs`, `scripts/task-packet.mjs`, +`test/process-digests.test.mjs`, `test/task-packet.test.mjs`. + +## Скоуп + +Инфраструктурная задача (§1: ни одного файла класса A) — без S1–S6, вход сразу +на `S7-code-review`, что подтверждено веткой `issue/695-track-labels` и меткой +`process` на issue. Заявленный автором скоуп (комментарий «Взял»): канон +треков PROCESS §5/§5.1/§2/§4/§7/§9/§11, конспекты AUTHOR/REVIEWER, AGENTS, +`task-packet.mjs`, лимит циклов по `track:*` в `_process.yml`. Явно вне +скоупа — поведение конвейера по треку (мутанты, слияние `ship` без ревью +модели, ребейз) — это #696, отдельная задача; проверял только то, что заявлено +сделанным в #695. + +Проверял по строке кода/текста, а не по заявлению автора: каждый пункт +хендоффа сверен с диффом и/или исполнением ниже. + +## Как проверялось + +| Гейт | Команда | Результат | +|---|---|---| +| Validate на материале | CI run на `57ce1072` | зелёный (ссылка в задаче ревью), дешёвые гейты (`tsc`, `npm test`, `build`+`bundle-policy`) не перегонял — приняты по этой ссылке (#343) | +| Целевой юнит-набор | `node --test test/process-digests.test.mjs test/task-packet.test.mjs test/entry-cost.test.mjs` | 28 pass, 0 fail (прогнано мной точечно поверх зелёного Validate, т.к. это ядро правок) | +| `process-gate.mjs` (офлайн) | `node scripts/process-gate.mjs --range origin/dev..HEAD` | «гейт пройден, предупреждений 0» | +| `mutation-gate --check` | `node scripts/mutation-gate.mjs --check` | `browser guards: 200/200`; 3 предупреждения — все про `#650` (geometry-corpus/nightly-reuse), не связаны с этим диффом, baseline | +| Мутант `process-digest-dead-anchor` | вручную заменил якорь AUTHOR.md `#51-метки-тяжёлых-проверок-и-прежние-метки` → `#51-метки-тяжёлых-проверок`, прогнал `node --test --test-name-pattern="#634 конспект: каждая ссылка" test/process-digests.test.mjs`, вернул файл | **краснеет**: `AssertionError` — тест поймал мёртвый якорь | +| Мутант `process-digest-bullet-without-canon-link` | вручную снял `([§5.1](...))` у пункта про `track:show`/`ci:*`, прогнал `--test-name-pattern="#634 конспект: каждый пункт"`, вернул файл | **краснеет**: пункт без ссылки на канон пойман | +| Якоря §5/§5.1 в новых ссылках | `headings()`/`markdownLinks()` из `scripts/md-anchors.mjs` (настоящий алгоритм GitHub slug, не схлопывающий дефисы) — покрыто тестом выше | зелёный тест = якоря `5-треки-ship-show-ask--метка-владельца` и `51-метки-тяжёлых-проверок-и-прежние-метки` существуют и совпадают с реальными заголовками | +| Ветка/трейлеры | `git log`, `git show -s --format=full HEAD` | один коммит, `Issue: #695`, `User-Visible: no`, ветка `issue/695-track-labels` — соответствует §3 п.10 | + +Не прогонял (и почему): golden/скриншоты, браузерные смоки, `pytest tests_backend`, +инварианты модели, `check-docs.mjs`, performance-профили — диапазон не +затрагивает `src/**`, `custom_components/**/*.py` ни один файл класса A; +`smoke-select.mjs` не запускал по той же причине (нет продуктового диффа, +которому смок мог бы соответствовать). `no-new-any.mjs` не применим — TS не +менялся. + +## Находки + +### Medium (в скоупе) — «инфраструктура без метки трека» не читается как `track:show` нигде в коде + +**Файл:** `PROCESS.md:607` (§5.1) и отсутствие реализации в +`.github/workflows/_process.yml`, `scripts/task-packet.mjs`. + +Новый канон §5.1 утверждает: + +> «...продуктовая задача без трековой метки — как `track:ask`; **инфраструктурная +> задача (§1) без трековой метки — как `track:show`**. Новые задачи получают +> только `track:*`.» + +Это прямое, машинно-проверяемое правило (наравне с «trivial/small читаются как +show», которое реализовано). Скоуп задачи явно включает «лимит циклов по +`track:*` в `_process.yml`» и `task-packet.mjs`. Ни один из них правило не +реализует: + +- `.github/workflows/_process.yml:87-93` (`guard` job) считает `SMALL=true` + только по меткам `small`, `trivial`, `track:show`, `track:ship`. Нет ветки, + которая бы распознавала инфраструктурную задачу без трековой метки — сам + bash-скрипт вообще не читает признак «инфраструктура» (только `gh issue view + --json labels`, без диффа). Инфраструктурная задача без метки трека + (обычный случай: конспект автора **нигде** не просит поставить + `track:*` инфраструктурной задаче — ни в `docs/process/AUTHOR.md`, ни в + «Вход в процесс» этого файла) получает `limit=4`, а не `2`, как обещает §5.1. +- `scripts/task-packet.mjs:180-183` (`buildPacket`) для инфраструктурной ветки + никогда не вызывает `trackFromLabels()` — track всегда равен + `'инфраструктурный'` / `'инфраструктурный (предварительно...)'`, даже когда + меток трека нет вовсе. Докстрока `trackFromLabels` (строки 128-134) прямо + говорит «задача без трековой метки — как `ask`» без исключения для + инфраструктуры, то есть и здесь правило §5.1 не реализовано ни в основной, + ни в обходной ветке. + +**Воспроизведение (не гипотеза — существующий зелёный тест доказывает +обратное поведение):** `test/task-packet.test.mjs:171-183` +(`'#632: statusless or returned infra issue without spec keeps the class A +ban'`) собирает пакет с `labels: ['infra', 'S7-code-review']` (нет ни одной +`track:*`, `small`, `trivial`) и **утверждает** `packet.track === 'инфраструктурный'` +— то есть текущий код по конструкции не даёт `'show'` для этого случая, и тест +это фиксирует как ожидаемое поведение уже сейчас, при полностью зелёном прогоне. + +**Почему это не редакционная мелочь.** Практический эффект — не +косметический: лимит циклов управляет тем, когда конвейер обязан +эскалировать задачу владельцу (`review-4`, §4). Инфраструктурная задача без +метки трека (а таких, по конспекту автора, будет большинство — постановка +метки трека нигде не предписана для инфраструктурного входа) при 2 подряд +жёлтых/красных вердиктах должна была бы уже упереться в лимит и уйти на +решение владельца (разделить/отклонить/арбитраж), а по факту реализованного +кода получает бюджет 4 — вдвое больше заявленного в собственном канона этой +же задачи. Это ровно тот класс расхождения «канон говорит одно, автоматизация +делает другое», который сам документ требует не игнорировать, а заводить +находкой (шапка PROCESS.md: «Расхождение не игнорируется, а заводится issue с +меткой `process`»); поскольку расхождение целиком внутри диффа и заявленного +скоупа этой задачи — чинится здесь, не отдельным issue. + +**Чем закрыть:** либо реализовать правило (например, `_process.yml` мог бы +трактовать `S7-code-review` без каких-либо `track:*`/`small`/`trivial` меток +как `SMALL=true`, когда issue не несёт продуктовых `S1-S6`-признаков — но тогда +нужен признак, отличающий это от «продуктовая задача без метки = ask», которого +у bash-скрипта сейчас нет вовсе), либо явно понизить формулировку §5.1 +до «рекомендации», сняв слово «читается как» и обязанность машинного +соответствия, как это сделано для temporarily-not-implemented частей `ship` +(«пока #696 не влит...», PROCESS.md:562-564) — то есть либо код, либо честная +оговорка о недоделке, но не утверждение без покрытия. + +## Что проверено и корректно + +- **Таблица треков §5 и её колонки** (для чего, маршрут, ТЗ, ревью ТЗ, + локальный гейт, код-ревью, лимит циклов) — согласована между PROCESS.md, + AGENTS.md, `docs/process/AUTHOR.md`, `docs/process/REVIEWER.md`; сверено + построчно диффом каждого файла, расхождений в формулировках не нашёл. +- **`trackFromLabels()` для явно помеченных задач** (`track:ship` / + `track:show` / `track:ask` / легаси `trivial`/`small` / без меток → + продуктовая задача) — корректна и покрыта новым тестом + `test/task-packet.test.mjs` (`#695: трек по меткам`), прогнан, зелёный. + Проверено исполнением, не только чтением. + Приоритет `track:ship` → `track:show` → `track:ask` → legacy → default + реализован в объявленном порядке (§5: «метка владельца главнее критериев», + явный `track:ask` переопределяет legacy-метки) — сверил построчно с + `.github/workflows/_process.yml:87-93`: `if has track:ask; then SMALL=false; + TRIVIAL=false; fi` идёт после чтения legacy-меток, порядок совпадает. +- **Лимит циклов для явно помеченных `track:show`/`track:ship`/`track:ask`** — + `_process.yml` строки 87-99 корректно матчатся: `track:show`/`track:ship`/ + legacy `small`/`trivial` → `limit=2`, явный `track:ask` возвращает `limit=4` + даже поверх legacy-меток. Дешёвая, но реальная проверка: прогнал bash-логику + построчно на всех комбинациях меток из `has()`; для случаев, где метка трека + явно стоит, поведение соответствует таблице §5 и §4. +- **Якоря конспектов (AC2 #634)** — оба новых заголовка (`§5`, `§5.1`) и все + ссылки на них из `AUTHOR.md`/`REVIEWER.md` резолвятся настоящим алгоритмом + GitHub-слага (`scripts/md-anchors.mjs`, без схлопывания дефисов) — проверено + исполнением зелёного `test/process-digests.test.mjs` (5/5) и двумя ручными + мутациями, которые «умеют падать» (см. таблицу гейтов). + Оба перенацеленных мутанта в `scripts/mutation-registry.mjs` + (`process-digest-dead-anchor`, + `process-digest-bullet-without-canon-link`) действительно ловят регресс — + проверено воспроизведением, не по названию. +- **Ключевые формулировки конспекта** («Метка владельца главнее критериев», + «ожидаемое поведение уже зафиксировано») дословно совпадают в PROCESS.md §5 и + в `AUTHOR.md`, со ссылкой на верный раздел — проверено тестом + `test/process-digests.test.mjs` (правило KEY_RULES), зелёный. +- **Трейлеры и провенанс коммита** — один коммит, `Issue: #695`, + `User-Visible: no`, ветка `issue/695-track-labels` от `dev@7d4d75bd`; + `User-Visible: no` корректен — изменение не задевает ни одного + пользовательского поведения (правок в CHANGELOG нет и не требуется). + `process-gate.mjs --range origin/dev..HEAD` — 0 предупреждений. +- **Отсутствие продуктового кода** — диффом не задет ни один файл класса A + (`src/**`, `custom_components/houseplan/**/*.py`, i18n); инфраструктурный + маршрут применён верно. +- **Обратная совместимость легаси-меток** (`small`/`trivial` → `show`, + продукт без метки → `ask`) реализована и в `_process.yml`, и в + `task-packet.mjs`, и задокументирована в §5.1 — согласованно во всех трёх + местах. +- Мусора после диффа не осталось: `grep` по репозиторию не находит мёртвых + ссылок на старые заголовки `§5 лёгкий трек`/`§5.1 короткий трек` ни в одном + скрипте, тесте или документе вне `docs/reviews/`. + +## Чего не проверял + +- **Поведение конвейера, отличное от лимита циклов** (мутанты по диффу на + `ship`, слияние `ship` без ревью модели, ребейз-семантика треков) — заявлено + автором как вне скоупа (#696), не проверял вовсе. +- **Живой прогон `_process.yml` guard job** на реальном issue с `track:show`/ + `track:ship` — GitHub Actions с реальным `gh issue view` не воспроизводил; + проверка bash-логики — чтением и ручной подстановкой значений `has()`, не + исполнением job. Записываю явно: **проверено чтением, не исполнением** для + всей ветки guard job, кроме той части, что покрыта `process-gate.mjs` + (офлайн-подмножество). + Тесты, исполняющие именно этот bash-скрипт (`_process.yml`), в репозитории + не существуют ни для старой, ни для новой логики — это не новый пробел + этой задачи, а существующее свойство инфраструктуры тестирования workflow. +- **`docs/reviews/INDEX.md`, golden, скриншоты, смоки, perf, pytest, + инварианты модели** — не прогонял: диапазон не касается `src/**`, + `custom_components/**/*.py`, визуала или геометрии. +- Полный `npm test` (3223 теста) не перегонял целиком — принят по зелёному + Validate на этом SHA (#343); перегонял точечно только изменённые файлы + (`process-digests`, `task-packet`, `entry-cost`) и мутации по ним. +- Не проверял, действительно ли `process-labels/labels.tsv` (вне репозитория, + в «папке владельца» по словам автора) создаёт `track:ship`, `track:ask`, + `ci:full`, `ci:golden`, `ci:mutants` с корректными цветами/описаниями — это + файл вне git-дерева репозитория, недоступен ревью. + +## Вердикт + +Единственная находка — Medium, в скоупе задачи (реализация лимита циклов по +`track:*` в `_process.yml` и track в `task-packet.mjs` — часть заявленного +скоупа), не High: явных меток трека это не касается, дефект сужен до случая +«инфраструктурная задача без единой метки трека», расхождение делает конвейер +мягче объявленного (даёт больше циклов, а не меньше) — не блокирует +использование, но противоречит только что написанному собственному канону. +Без High это жёлтый вердикт, возврат автору на исправление в этом же issue. + +Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 → в задаче + +--- + + + +## Материал раунда + +- Ветка: `issue/695-track-labels`, коммит `57ce10721fc0` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `2faf4770f1ba6a32a599552db9c611bfb5cc5a18` + ``` + git log --all --format='%H %T' | grep 2faf4770f1ba + ``` +- Тело issue: `c74209278f8dd164514eda40d8841d6eefdeffd3b490a90f47b20e3ff2b667e7` +- Вердикт конвейера: `yellow` · High 0