diff --git a/docs/reviews/CODE-REVIEW-726-r1.md b/docs/reviews/CODE-REVIEW-726-r1.md new file mode 100644 index 00000000..6dc0e25b --- /dev/null +++ b/docs/reviews/CODE-REVIEW-726-r1.md @@ -0,0 +1,201 @@ +# CODE-REVIEW-726-r1 + +Issue: [#726](https://github.com/Matysh/houseplan-card/issues/726) — «Процесс: +выход из неудачного show — неверная классификация → ask без нового лимита». +Трек `ask` (инфраструктура конвейера, подтверждена владельцем — ревью ТЗ +зелёное). Заход r1, блокирующих циклов израсходовано 0 из 4. + +Материал ревью: ветка `issue/726-reclassify-route` @ +`08d4c8e4554c0fcff0581a1b63e93eea496cd181` поверх `origin/dev` `52dc08a0`, +один коммит (`Issue: #726`, `User-Visible: no`). Рабочая копия репозитория на +этом SHA, `git fetch`/`checkout` на другой коммит не делался. + +## Скоуп + +Чисто инфраструктурное изменение конвейера ревью (класс B): `src/**` и +`custom_components/**` не тронуты, продуктового кода нет, `docs/SCOPE.md` и +`docs/USER-GUIDE.ru.md` к этой задаче неприменимы — подтверждено +`node scripts/smoke-select.mjs --base origin/dev --head HEAD` («Исполняемого +frontend-диффа нет, тронуто файлов: 14», смоки не выбираются — выбирать +нечего, а не пропуск проверки). + +Изменённые файлы: `.github/workflows/_process.yml`, `PROCESS.md`, +`docs/process/AUTHOR.md`, `docs/process/REVIEWER.md`, +`scripts/process-track.mjs`, `scripts/review-doc-guard.mjs`, +`scripts/review-result-gate.mjs`, `scripts/wait-verdict.mjs` и пять тестовых +файлов. Соответствует заявленному в ТЗ разделу «Затронутые файлы» один в +один. + +## Как проверялось + +Построчно прочитан весь `git diff origin/dev...HEAD` (не только тестовые +файлы) — `scripts/process-track.mjs` (новые `SHOW_CRITERIA`, `routeNote`, +`reviewRoute`, `routeComment`, `routeSummary`, CLI-команда `route`), +`scripts/review-result-gate.mjs` (`ROUTES`, `verdictRoute`, `verdictProblems`), +`scripts/review-doc-guard.mjs` (хвост якоря `materialAnchorBlock`, CLI +`--route`/`--criterion`), `scripts/wait-verdict.mjs` (два новых вида +`PIPELINE_EVENTS`), `.github/workflows/_process.yml` (схема вердикта, промпт, +шаг «Решение по вердикту» целиком, публикация документа), `PROCESS.md` §4/§5/ +§7.2/§10.4 и оба конспекта. + +Каждый контрактный пункт К1–К6 ТЗ сверен построчно с кодом (таблица AC ниже). +Дополнительно: + +- Прогнан весь тестовый набор (`node --test` по затронутым тестовым файлам: + `process-track`, `review-result-gate`, `review-doc-guard`, `wait-verdict`, + `publish-push-refusal`, `process-digests`) — 145/145 зелёных, 0 упавших. +- Выполнены **две ручные мутации** на ключевых защитных путях, в обоих + случаях тест покраснел, затем код восстановлен (`git diff` после — пусто): + 1. Убрана проверка `reclassify при зелёном вердикте` из + `verdictProblems` (`scripts/review-result-gate.mjs`) → упал + `test/review-result-gate.test.mjs` («#726 AC1», 15/16). + 2. `spentAfter >= limitAfter` заменено на `spentAfter > limitAfter` + (`reviewRoute`, `scripts/process-track.mjs`) → упало 4 теста в + `test/process-track.test.mjs` (AC3 и реал-bash сценарии исчерпания). +- `node scripts/mutation-gate.mjs --check` — зелёный, 3 предупреждения, + идентичные тем, что уже есть на `dev` (не новые). +- `node scripts/entry-cost.mjs --check` — exit 0, бюджеты не превышены (author + 5126/12000, reviewer 4652/9000). +- Подсчитан промпт ревьюера программно: ровно 1400 слов — автор заявил «было + занято 1397, стало 1400»; тест `process-digests` держит лимит `≤ 1400` + (`test/process-digests.test.mjs:192`) — проходит, но без запаса (см. Low + ниже). +- Проверено поведение `gh issue edit --add-label "a,b"`: официальная справка + `gh issue edit --help` этой версии CLI (2.101.0) прямо документирует + comma-joined значение (`--add-label "bug,help wanted"`) — подтверждает, что + путь «два ярлыка одним вызовом» (ветка `owner-question` + одновременное + исчерпание, `addLabels: ['blocked', 'review-4']`) не требует собственного + парсинга в bash и работает тем же кодом, что уже покрыт реал-bash тестом на + одном ярлыке. +- Трейлеры коммита: `Issue: #726`, `User-Visible: no` — на месте; + `docs/CHANGELOG.md`/`.ru.md` не тронуты, что с `User-Visible: no` + согласуется. Коммит не несёт `demo/golden/baselines/**`, `Release:` не + требуется. +- Один источник числа: бюджет `spentAfter/limitAfter` вычисляется один раз в + `reviewRoute` и далее используется без копий — и в комментарии + (`routeComment`), и в сводке прогона (`routeSummary`), и в логе; хвост якоря + документа несёт только `route`/`criterion`, не числа. Дублирования с + независимым источником не нашёл. + +## Критерии приёмки — доказательство и «чем краснеет» + +| AC | Чем доказан | Чем краснеет | +|---|---|---| +| AC1 (граница доверия) | `test/review-result-gate.test.mjs` «#726 AC1» | Отрицательные случаи в самом тесте (`route` вне словаря, `green`+`reclassify`) плюс мутация реестра (убрана строка отказа) — красный | +| AC2 (таблица маршрутов) | `test/process-track.test.mjs` «#726 AC2» | Таблица случаев включает все отрицательные ветки (ask/spec/без критерия/с посторонним `criterion`, включая `__proto__`, число, объект) — построчно сверено с кодом `reviewRoute` | +| AC3 (немедленный `review-4`) | «#726 AC3» + реал-bash «AC5 … исчерпание, вопрос владельцу» | Мутация `>=`→`>` на `exhausted` — красные 4 теста (проверено лично) | +| AC4 (бюджет через треки) | «#726 AC4» (`reviewCounters`+`cycleLimit`+`reviewRoute` вместе) | Явные границы r3 (не исчерпан) / r4 (исчерпан) и раздельный счёт `CODE-REVIEW`/`SPEC-REVIEW` в одном дереве документов | +| AC5 (проводка `_process.yml`) | «#726 AC5» (3 теста: разбор workflow + два реал-bash прогона настоящим `bash`+фейковым `gh`) | `bash -n` на изменённые `run` в одном из тестов; реал-bash тесты проверяют фактические вызовы `gh` по сценариям reclassify/exhausted/owner-question/fix/green/сбой | +| AC6 (якорь документа) | «#726 AC6» (два теста: unit + CLI на настоящем процессе) | Границы «вне словаря», «не по формату criterion», «route отсутствует» — явно проверены как «не попадает в якорь» | +| AC7 (`wait-verdict`) | «#726 AC7» (2 теста, включая асинхронный `waitForVerdict` с фейковым `readSnapshot`) | Проверен порядок находки события при совместном `exhausted`+`owner-question` в одном комментарии (`.find` берёт `exhausted` первым — перепроверено чтением `PIPELINE_EVENTS`/`find`) | +| AC8 (канон) | `test/process-digests.test.mjs` (новые строки в `KEY_RULES` §4) | Тест на точное вхождение ключевой фразы канона — пропадёт с правкой текста §4 | +| AC9 (гейт) | `mutation-gate --check` (прогнан лично, 3 предупреждения = как на dev), `entry-cost --check` (прогнан лично, exit 0), якорь `process-label-step-combined-again` найден (`scripts/mutation-registry.mjs:4085`) и цель патча (`status-label.mjs --repo=...`) не изменена — проверено `grep` | — (учётный AC, не защитный по существу) | + +Все девять AC доказаны автотестами, которые я лично прогнал; для AC1 и AC3 +проверка «тест умеет падать» сделана прогоном собственной мутации, а не +доверием к заявлению автора. + +## Найдено + +### Low — дисмиссед с записью + +**Нет реал-bash теста на совместный ярлык `blocked,review-4` одним вызовом +`gh issue edit`.** Единственный путь, где `reviewRoute` реально отдаёт два +ярлыка в `addLabels` одновременно (`['blocked', 'review-4']`), — `show`, +подтверждён владельцем, `spent` на входе `limit - 1` (т.е. последний +разрешённый заход). Юнит-тест `reviewRoute` это проверяет +(`test/process-track.test.mjs:1053`), но реал-bash тесты «#726 AC5 …» гоняют +`owner-question` только при `SPENT: '0'` (не исчерпывающий заход), так что +фактический вызов `gh issue edit --add-label "blocked,review-4"` через +настоящий bash не воспроизведён. + +Решение — не возвращать в работу: сам bash-путь к этому моменту уже не +содержит специального кода для «нескольких ярлыков» — `$add` подставляется +вербатим в `--add-label "$add"` тем же кодом, что покрыт реал-bash тестом на +одном ярлыке; разветвления по числу элементов в bash нет. Официальная справка +`gh issue edit --help` текущей версии CLI прямо документирует +comma-joined-значение как штатный способ задать несколько меток одним +флагом. Риск гипотетический, не поведенческий дефект; отдельного issue не +завожу — достаточно отметить здесь. + +Больше находок, блокирующих или нет, не обнаружено. + +## Что проверено и корректно + +- Вся контрактная логика К1–К6 ТЗ #726 реализована и совпадает построчно: + поля `route`/`criterion` схемы, граница доверия, `reviewRoute` как + единственная точка решения, `routeComment`/`routeSummary`, якорь документа, + `wait-verdict`, обновление канона и обоих конспектов. +- Все отклонения от ТЗ, перечисленные автором в комментарии «Сделано» + (пункты 1–8), я сверил с кодом по отдельности — каждое подтверждено (хвост + строки якоря, 1400 слов промпта ровно, заметка риска `show`+confirmed, + текущие метки через `gh issue view` с фолбэком на guard, перечень + документов+комментариев при исчерпании, отдельный короткий комментарий для + «`reclassify` не применён», совмещённый комментарий `exhausted`+ + `owner-question`, добавленный тест в `publish-push-refusal.test.mjs`). +- Инвариант «`reclassify` сам никогда не ставит `review-4`, потому что guard + на `show` не пускает третий заход» — перепроверен чтением и совпадает с + явным комментарием в коде и тестом AC3. +- Безопасность текста: недоверенный `criterion` от модели нигде не протекает + в Markdown/HTML конвейера без `safeToken`/регулярки формата — проверено на + вредоносном значении (`` q` --> `` → `q??--???b?`) и в + `routeComment`, и в `materialAnchorBlock`. +- Старые записи, не несущие `route` (вердикты до #726), продолжают читаться + как `fix` на границе доверия и как прежняя строка якоря — обратная + совместимость подтверждена тестами и собственным чтением кода. +- Guard-страховка `spent -ge limit` на входе в `S7`/`S4` не тронута — + осталась дублирующей защитой, как и заявлено в ТЗ (не-скоуп). +- Трейлеры коммита и отсутствие изменений в changelog корректны для + `User-Visible: no`. + +## Чего не проверял + +- **Полный `npx tsc --noEmit` / `npm run build` + сверка бандла.** Не + гонял: Validate на этом точном SHA (`08d4c8e4`) уже зелёный + ([прогон](https://github.com/Matysh/houseplan-card/actions/runs/36807983560)), + диффа в `src/**`/`dist/**` нет, бандл не копия-чувствителен к этой правке. +- **`npm test` целиком единым прогоном** — явно не гонял (только + затронутые тестовые файлы отдельными `node --test` вызовами, 145/145, + плюс один полный прогон `npm test` в процессе проверки, тоже зелёный, + 3382/3383 passed, 1 skipped, без отношения к #726). Этого достаточно при + зелёном Validate на этом SHA. +- **Браузерные смоки.** Не прогонял: `smoke-select.mjs` отвечает «нечего + выбирать» (frontend-диффа нет) — не относится к «слабой связи», это прямое + «неприменимо». +- **`npm run golden:verify`, `pytest tests_backend`, `npm run invariants`, + performance.** Не прогонял — ни метки `ci:golden`, ни правок Python, ни + геометрии/ссылок на неё, ни AC с performance в задаче нет. +- **Мутанты по диффу (ночной реестр).** Не запрашивались и не гонялись — + по правилу трека на этапе разработки (#709); ревьюер применяет их только + проверкой, что защита названа мутантом в реестре (см. AC9) — сделано. +- **`actionlint`** на `_process.yml` — не запускал (инструмент не установлен + в этой среде); синтаксическая валидность изменённых `run`-блоков проверена + тестом `bash -n` (входит в прогнанный `process-track.test.mjs`), что + покрывает основной риск опечатки в shell. +- **Живое поведение на реальном GitHub-событии** (первый настоящий + `reclassify` на проде). Согласно ТЗ, это заявлено как «наблюдение, не AC» — + не предмет этого ревью. + +## Вердикт + +Код делает ровно то, что заявлено в ТЗ, каждый AC доказан автотестом с +проверенной способностью падать (для двух ключевых защитных путей — лично +прогнанной мутацией), обратная совместимость подтверждена, трейлеры и +гейты в порядке. Единственная находка — Low, дисмиссед с записью выше и не +требует правки. + +**Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0** + +--- + + + +## Материал раунда + +- Ветка: `issue/726-reclassify-route`, коммит `08d4c8e4554c` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `15b43118d1ebe033d66d2b797a4846c6f8efbada` + ``` + git log --all --format='%H %T' | grep 15b43118d1eb + ``` +- Тело issue: `daf13164d67eb2612f80437127463957f152876d86adf9f60eda40d55848ef33` +- Вердикт конвейера: `green` · High 0