diff --git a/docs/reviews/CODE-REVIEW-587-r1.md b/docs/reviews/CODE-REVIEW-587-r1.md new file mode 100644 index 00000000..aecae1ee --- /dev/null +++ b/docs/reviews/CODE-REVIEW-587-r1.md @@ -0,0 +1,171 @@ +# CODE-REVIEW-587-r1 + +**Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0** + +Материал: `978035c49193a7d367d3dba6a22bebef149e1eef` (совпадает с текущим `HEAD`, +рабочая копия детачнута на нём). Трек: инфраструктурный (метки `bug`, `P2`, +`infra`, `S7-code-review`; без продуктового `S*`). + +## Скоуп + +Один коммит, диапазон `origin/dev..HEAD` = сам коммит `978035c4`. Класс B +целиком: `.github/workflows/performance.yml`, `scripts/mutation-registry.mjs`, +`demo/performance/README.md`, плюс новые `scripts/performance-baseline.mjs` и +`test/performance-baseline.test.mjs`. Продуктовый код (`src/**`, +`custom_components/**`) не тронут — подтверждено `git diff origin/dev...HEAD +--stat`. + +Проблема из issue: гейт «Полных бенчмарков» для кандидата стабильного релиза +сравнивал линейку саму с собой, потому что `performance.yml` запускается +только на push в `main`, а базой относительного сравнения брался «предыдущий +push в main» — то есть первый коммит той же релизной линейки. Наблюдалось +вживую на v1.76.0: честный красный прогон `35097102695` на `c3d64789` и +зелёный `35109734519` на `9683a590` независимо от правки бюджетов. + +Решение: выбор базы вынесен из shell в `scripts/performance-baseline.mjs`; +стабильный кандидат (head несёт трейлер `Release: vX.Y.Z` без пре-релизного +суффикса) сравнивается с предыдущим стабильным тегом, остальные случаи +(бета, обычный push, ручной `comparison_ref`) — без изменений. + +## Как проверялось + +Дешёвые гейты для этого SHA уже подтверждены зелёным прогоном Validate +(системная метка ревью ссылается на `35135781823`; сам исполнитель в хендоффе +называет соседний прогон `35135276439` — оба независимо проверены мной, +`headSha` обоих совпадает с материалом ревью, `conclusion: success`): +`npx tsc --noEmit`, `npm test`, `npm run build` не перегонялись повторно. + +Что прогнал сам, потому что это точечные, дешёвые и по существу задачи проверки: + +| Гейт | Команда | Результат | +|---|---|---| +| Целевой тест AC2 | `node --test --test-name-pattern="AC2" test/performance-baseline.test.mjs` | зелёный, 1/1 | +| Мутационный гейт по задаче | `node scripts/mutation-gate.mjs --id=stable-candidate-compares-against-itself` | «заявленный тест покраснел на мутанте» — оба шага (чистый прогон, прогон с мутацией) зелёные в ожидаемом смысле | +| Реестр мутантов (целостность якорей) | `node scripts/mutation-gate.mjs --check` | прошёл, новый якорь `stable-candidate-compares-against-itself` в списке | +| Выбор смоков по дифу | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «Исполняемого frontend-диффа нет… браузерные смоки этим диффом не выбираются» — `src/**` не тронут, смоки не нужны | + +Не прогонял и почему: +- `npm test` целиком, `npx tsc --noEmit`, `npm run build` со сверкой бандла — + уже зелёные на этом SHA (см. выше), диф не в `src/**`/`custom_components/**`, + перегонять нет смысла; +- `node scripts/check-docs.mjs` — диф не трогает `src/**`, отпечаток + скриншотов документации им не задет; исполнитель называет тот же известный + устаревший отпечаток (#586), не относящийся к этой задаче; +- инварианты модели (`npm run invariants`) — диф не касается геометрии, стен, + `layout`, `marker.space`, `open_spans`; +- `npm run golden:verify` — диф не меняет рендер/геометрию/стили; +- `python -m pytest tests_backend -q` — `custom_components/**/*.py` не тронут; +- performance-профили (реальный прогон бенчмарков) — задача меняет только то, + *с чем* сравнивается прогон, а не измеряемый код; AC не требуют реального + запуска `performance.yml`, только доказательства выбора базы, что и + покрывают юнит-тесты с инжектированным `git`. + +Дополнительно прочитал код инвариантов, на которые опирается AC4 (см. ниже), и +сверил регекспы/веточную логику `scripts/performance-baseline.mjs` построчно +против старой shell-реализации, которую он заменяет. + +## Разбор по AC (issue #587) + +**AC1** — «Push в `main` коммита с трейлером `Release: vX.Y.Z` (стабильный) +берёт базой предыдущий стабильный тег; это видно в сводке прогона.» +Доказано тестом `AC1: кандидат стабильного релиза судится о предыдущий +стабильный тег` (`test/performance-baseline.test.mjs:43`) — прогнан лично, +зелёный. Видимость в сводке: `resolveComparisonBase` возвращает `source`, +запись в `$GITHUB_STEP_SUMMARY` не убрана (`scripts/performance-baseline.mjs:206-208`), +проверено чтением — идентична строке из прежней shell-версии. +Чем краснеет: тот же мутант, что и AC2 (see below) убирает ветку +`previous-stable` целиком — тест AC1 тоже упал бы на этом мутанте (не +зарегистрирован отдельно, но код-путь общий: см. «Защитный AC» ниже). + +**AC2** — «Два коммита подряд одной линейки не могут дать зелёный гейт "сам с +собой" — второй сравнивается с тем же стабильным тегом, что и первый.» +Доказано тестом `AC2` — прогнан лично, зелёный. Защитный AC доказан таблицей: + +| AC | чем доказан | чем краснеет | +|---|---|---| +| AC2 | `node --test --test-name-pattern="AC2" test/performance-baseline.test.mjs` | мутант `stable-candidate-compares-against-itself` (`scripts/mutation-registry.mjs`) убирает условие входа в ветку «предыдущий стабильный тег»; прогнал лично `node scripts/mutation-gate.mjs --id=stable-candidate-compares-against-itself` → «заявленный тест покраснел на мутанте» | + +Для чистого юнита (не дорогой гейт — смок/бэкенд/golden) прогона со снятой +защитой достаточно по §2.7; сделано. + +**AC3** — «Бета-кандидат и обычный push сохраняют текущую базу.» Доказано +тестом `AC3` (тот же файл) — три под-случая (бета с трейлером, push без +трейлера, `workflow_dispatch` без `comparison_ref`), прогнан лично, зелёный. + +**AC4** — «Заодно проверить, не опирается ли на "родителя" ещё какой-нибудь +релизный гейт.» Разобрано **чтением, не исполнением**: +- `validate.yml` использует `github.event.before` только как `FALLBACK` для + диапазона классифицируемых файлов (`base`/`range_base`, строки 240–295, + 606, 700); действующая база диапазона — «самый новый предок с успешно + завершённым Validate» (#387/#388), не сырой родитель — комментарий на + строке 249 прямо это фиксирует как урок предыдущего инцидента (#86 r5). + Циклическая проблема из #587 к этой логике неприменима: она не измеряет + релизную линейку саму собой, а определяет объём проверки файлов. +- `scripts/release-gate.mjs`, `release-contract.mjs`, `release-membership.mjs`, + `release-bookkeeping.mjs`, `e2e-gate.mjs` — grep на `parent|HEAD^|event.before` + ничего не находит; `docs/DEVELOPMENT.md:485-504` подтверждает, что + `release.yml` резолвит кандидата по точному тегу/SHA и требует полное + доказательство Validate для этого SHA, родителя не касаясь. +Вывод соответствует утверждению исполнителя; независимая проверка грепом и +чтением документации его подтверждает. + +## Что проверено и корректно + +- Регекспы `releaseTrailerTag`/`isStableTag` соответствуют существующей + конвенции трейлера `Release:` в `scripts/classify-changes.mjs` и + `scripts/process-gate.mjs` — это не новый придуманный контракт, а уже + используемый в кодовой базе (`docs/DEVELOPMENT.md:489`: стабильный релиз + обязан нести `Release: ` на кандидате). +- `previousStableTag` корректно исключает и тег кандидата (по номеру версии, + independent от текущего SHA — для случая «ремонт уже выпущенного релиза»), + и тег, стоящий на самой голове; оба случая покрыты тестом + «свой тег и тег на голове предыдущим не считаются». +- Порядок отказов (недоступная база → родитель → последний достижимый + релизный тег → ошибка) сохранён из старой shell-реализации построчно; + тесты «непригодная база уводит...», «без предыдущего тега...», «база + старше HP-PERF-01...» подтверждают эквивалентность поведения не-стабильных + путей. +- Тест `workflow берёт базу из скрипта...` (`test/performance-baseline.test.mjs:161`) + проверяет сам YAML-файл: вызов `scripts/performance-baseline.mjs` на + месте, `HEAD_MESSAGE` прокинут, старая shell-развилка + (`source="candidate parent"`) удалена целиком — защищает именно от риска + «два места принятия одного решения расходятся», названного исполнителем. +- Коммит несёт корректные трейлеры `Issue: #587`, `User-Visible: no` — + продуктовое поведение не меняется, changelog не требуется. +- `demo/performance/README.md` обновлён в том же коммите — документация + подсистемы синхронна с кодом. +- Одно число — один источник: изменение не вводит новых пользовательских + величин, дублирования нет (CI-внутренний артефакт, не UI). + +## Чего не проверял + +- Реальный прогон `performance.yml` на push в `main` с трейлером `Release:` — + недоступно из код-ревью (нет push-события), поведение доказано юнит-тестами + с инжектированным `git`, а не сквозным прогоном workflow. Риск невысокий: + единственная переменная, которую тесты не видят, — фактический формат + `github.event.head_commit.message`, но он прокидывается без обработки как + строка, и `releaseTrailerTag` уже проверен на многострочных сообщениях с + трейлерами после текста коммита (тест «трейлер релиза читается...»). +- `npm run mutation-gate` полный прогон всех ~760 мутантов (гонял только + `--check` для целостности реестра и точечный `--id` для новой записи) — + полный прогон не относится к дельте этой задачи и дорог. + +## Материал раунда + +- SHA: `978035c49193a7d367d3dba6a22bebef149e1eef` +- Дерево: `git rev-parse HEAD^{tree}` на момент ревью соответствует рабочей + копии (детач на материале, без локальных изменений — `git status` чист). + +--- + + + +## Материал раунда + +- Ветка: `issue/587-stable-perf-baseline`, коммит `978035c49193` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `8f80b742f032f854e5b54f5184adb4c8daa52cc1` + ``` + git log --all --format='%H %T' | grep 8f80b742f032 + ``` +- Тело issue: `ff876b4abd609814b1700c764a650cba2a3ccc262d9ae744e02c16f0d1cbd186` +- Вердикт конвейера: `green` · High 0