diff --git a/docs/reviews/CODE-REVIEW-655-r2.md b/docs/reviews/CODE-REVIEW-655-r2.md new file mode 100644 index 00000000..0fba4b8d --- /dev/null +++ b/docs/reviews/CODE-REVIEW-655-r2.md @@ -0,0 +1,180 @@ +# CODE-REVIEW-655-r2 + +Issue: #655 · Заход: r2 · Трек: light (`small`) · Блокирующих циклов израсходовано: 1/2 + +Материал раунда: `git log --oneline origin/dev..HEAD` = три коммита +(`075a2d3fb128` — реализация, `7acf5c74ada4` — документ ревью r1, +`f340157f59b7` — закрытие находки r1), рабочая копия на +`f340157f59b75cbef4dc20d6ac456a10f4053517`. Раунд r1 закончился жёлтым +вердиктом с единственной находкой (Medium, в скоупе AC3); повод для r2 — +дельта `git diff 075a2d3fb128..f340157f59b7`, а не задача целиком +(PROCESS.md §2.10). + +## Дельта r1 → r2 + +``` +git diff 075a2d3fb128..f340157f59b7 --stat + docs/reviews/CODE-REVIEW-655-r1.md | 179 ++++++++++++++++++++++ + scripts/mutation-registry.mjs | 13 ++ + tests_backend/test_virtual_lights.py | 35 +++++ +``` + +Дельта строго локальна: продуктовый код (`custom_components/houseplan/*.py`) +не тронут ни строкой — весь diff это один новый тест, один новый мутант и +служебный документ ревью r1. Это ровно объём, заявленный автором в качестве +исправления («один тест + мутант, без расширения скоупа»), и ровно то, что +находка r1 просила. Ребейза на ушедший вперёд `dev` нет +(`origin/dev..HEAD` по-прежнему три коммита, база не сдвинулась), новой +подсистемы или смены контракта нет — разбор по дельте оправдан, полный +повторный разбор всей задачи не требуется. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| Medium — защитный `_flush_lock` в `VirtualLightController.async_flush()` не имел собственного теста/мутанта на конкурентный двойной flush (AC3, `virtual_lights.py:163`) | Новый тест `test_concurrent_flushes_after_failed_delayed_save_write_once` (`tests_backend/test_virtual_lights.py:129-163`): запускает `controller.async_flush()` дважды конкурентно (`asyncio.gather`) поверх состояния, оставленного неудачной отложенной записью (`_dirty=True`, `_save_task=None`); `BlockingStore.async_save` считает входы и блокируется на `asyncio.Event` до явного `release.set()`, так что до релиза видно, сколько задач реально вошло в `Store.async_save` конкурентно. Плюс новый мутант `virtual-light-concurrent-flushes-bypass-lock` (`scripts/mutation-registry.mjs`), убирающий `async with self._flush_lock:` → `if True:` | Тест: `tests_backend/test_virtual_lights.py:129` (`entered == 1`, `writes == [...]` — один физический вход и одна запись). Мутант: `scripts/mutation-registry.mjs` (id `virtual-light-concurrent-flushes-bypass-lock`, guard `backend-test-guard.mjs concurrent_flushes_after_failed_delayed_save_write_once tests_backend/test_virtual_lights.py`). Исполнение в CI: Validate `f340157f`, job «Мутанты по диффу (3/6)» (id `108623233714`), лог: `ok virtual-light-concurrent-flushes-bypass-lock: заявленный тест покраснел на мутанте` — реальный прогон с полным HA, не заявление автора | + +Третий столбец таблицы AC3 для `VirtualLightController`, пустой в r1, теперь +заполнен тем же способом, каким уже была доказана идемпотентность +`TrailRecorder` — паритет между двумя сторами восстановлен. + +## Независимая перепроверка (не на слово автору и не только на лог CI) + +Не ограничился чтением диффа и логом одного CI-job — воспроизвёл оба +направления мутации локально в песочнице ревью: + +1. Прогнал все 5 тестов `tests_backend/test_virtual_lights.py` напрямую + (без `pytest` — недоступен в песочнице, файл нарочно грузится по пути, + так что достаточно вызвать `test_*` функции модуля): все 5 `OK`, + включая новый `test_concurrent_flushes_after_failed_delayed_save_write_once`. +2. Применил ровно патч мутанта (`async with self._flush_lock:` → + `if True: # mutant: ...`) к рабочей копии `virtual_lights.py`, повторно + вызвал только новый тест — тест упал: + `AssertionError: only one concurrent flush may reach durable storage` + (счётчик входов в `Store.async_save` стал 2 без лока, ровно как + предсказывает трасса конкуренции). Файл вернул в исходное состояние сразу + после проверки, `git status --porcelain` — пусто, рабочая копия чиста. + +Это подтверждает находку r1 закрытой доказательством, которое умеет падать +(«тест умеет падать» — не только по логу CI, но и по независимому повторению +здесь), а не только фактом присутствия нового теста в диффе. + +## Как проверялось (гейты) + +| Гейт | Результат | Как | +|---|---|---| +| Дешёвые гейты (`typecheck`, `test`, `build`/`bundle-policy --verify`) | подтверждены зелёным Validate на точном SHA | `run 36320469875`, `conclusion: success`, `head_sha: f340157f59b75cbef4dc20d6ac456a10f4053517` (ссылка из задания ревью) — не перегонял повторно | +| `node scripts/smoke-select.mjs --base 075a2d3fb128 --head HEAD` | «Исполняемого frontend-диффа нет … Browser-smoke этим диффом не выбираются» | прогнал сам; diff не трогает `src/**` | +| Новый тест `test_concurrent_flushes_after_failed_delayed_save_write_once` + весь модуль `test_virtual_lights.py` (пуре-python, без HA) | 5/5 «OK» | вызвал функции теста напрямую (см. раздел «Независимая перепроверка») — в песочнице нет `pytest`, обошёл прямым вызовом, это не хуже: код исполнялся, а не читался | +| Мутант `virtual-light-concurrent-flushes-bypass-lock` (отрицательная проба) | тест краснеет | (а) лог реального CI-job'а Validate `f340157f`, шард 3/6 (`ok … покраснел на мутанте`); (б) независимо воспроизвёл тот же патч локально и получил тот же красный результат | +| CI-мутанты диффа в целом (6 шардов) | все success | `gh api .../actions/runs/36320469875/jobs` — все шесть job'ов «Мутанты по диффу (N/6)» зелёные | + +Не прогонял: `npx tsc --noEmit`, `npm test`, `npm run build` (покрыты +Validate на этом SHA — фронтенд не тронут, а гейт всё равно исполнялся в +job'е «Фронтенд: типы, юниты, мутанты, синхрон бандла» с зелёным исходом); +`python -m pytest tests_backend -q` с реальным HA (909/910 тестов) — снова +недоступен в песочнице (нет `homeassistant`, нет WSL); закрыл это тем же +способом, что и в r1 — не поверил заявлению, а нашёл в логах CI-шардов +реальное исполнение целевого теста с полным `homeassistant` до и после +мутации. Полный канонический backend-набор для этого SHA нигде не +исполнялся, кроме advisory WSL-прогона автора (heavy-гейт, не гейт ревью — +`AGENTS.md`, «Heavy CI gates…»). `golden:verify`, `check-docs.mjs`, +`npm run invariants`, браузерные смоки, performance — не нужны: diff не +меняет `src/**`, геометрию, рендер или пользовательские числа. + +## AC → доказательство (только то, что задевает дельта) + +AC1, AC2, AC4, AC5 не в дельте r1→r2 — их доказательства не менялись, +наследуются из r1 без повторной проверки (см. раздел ниже). Единственный +пересматриваемый пункт: + +| AC | Чем доказан | Чем краснеет | +|---|---|---| +| AC3 (unload/идемпотентность, часть «повторный/конкурентный `async_flush()` не пишет дважды», `VirtualLightController`) | `test_concurrent_flushes_after_failed_delayed_save_write_once` — конкурентный `asyncio.gather` двух `async_flush()` поверх pending-состояния после неудачной записи, счётчик реальных входов в `Store.async_save` | мутант `virtual-light-concurrent-flushes-bypass-lock` — убит и в CI (лог шарда 3/6), и локально при независимом воспроизведении в этом ревью | + +## Находки + +Новых находок нет. Единственная находка r1 (Medium, в скоупе AC3) закрыта +доказательством, которое я перепроверил исполнением дважды (CI-лог + +локальное воспроизведение), а не одним лишь фактом появления нового теста +в диффе. High-находок не было и не появилось. + +## Что проверено и корректно + +- Коммит `f340157f`: трейлеры `Issue: #655`, `User-Visible: no` — + корректно для тестового/инфраструктурного изменения без видимого + поведения; changelog не тронут, что и требуется при `no`. +- Изменение не вводит и не меняет ни одного числа, видимого пользователю — + правка тестовая/инфраструктурная, продуктовый код не тронут. +- Новый тест использует уже существующий в файле паттерн (`FakeStore`, + `FakeHass`, приватный event loop `_run`) и приватную запись состояния + контроллера (`controller._state = …`) — это пуре-python backend-тест, + гейт `no-new-private-writes.mjs` на него не распространяется: его область + (`isGatedPath`) — только `demo/smoke_*.mjs` и `demo/helpers/**.mjs` + (браузерные смоки), не `tests_backend/**`; прямая работа с приватным + состоянием контроллера — устоявшийся паттерн уже существующих тестов + этого же файла. +- Мутант зарегистрирован с корректным `guard` (`backend-test-guard.mjs` + с именем ровно нового теста и путём к файлу) и `because`, ссылающимся на + #655 AC3 / review r1 — соответствует формату соседних записей реестра. +- Синтаксис обоих изменённых файлов проверен (`python3 -m py_compile`, + `node --check`) — ошибок нет. +- Рабочая копия после локального воспроизведения мутанта возвращена в + чистое состояние (`git status --porcelain` пусто) — эксперимент не + оставил следов в дереве материала. + +## Унаследовано из r1 + +Всё остальное содержимое диффа (`custom_components/houseplan/__init__.py`, +`custom_components/houseplan/store.py`, `custom_components/houseplan/virtual_lights.py` +кроме `_flush_lock`, `tests_backend/test_ha_virtual_lights.py`, +`tests_backend/test_trail_recorder.py`, первый мутант +`shutdown-skips-deferred-store-flush` и `virtual-light-save-bypasses-ha-task-tracking`, +доказательства AC1/AC2/AC4/AC5, модель жизненного цикла HA STOP vs unload, +обработка ошибок в `_async_flush_runtime`) принято без повторной проверки — +дельта r1→r2 их не касается ни строкой. Источник: +`docs/reviews/CODE-REVIEW-655-r1.md` (коммит `7acf5c74ada4714672dea585f7238e288493c066`), +материал того раунда — коммит `075a2d3fb1284d58055160663adbfba9a1440202`, +дерево `e04239085a2bdbe537f270df4bd9e6e4ef46a35f`, вердикт `жёлтый · High 0 · Medium 1`. + +## Чего не проверял + +- Полный `python -m pytest tests_backend -q` с реальным HA (канонический + набор) — недоступен в песочнице ревью (нет `homeassistant`, нет WSL). + Заменил точечным исполнением: лог CI-шарда 3/6 показывает реальный запуск + нового теста под полным HA до и после мутации, плюс независимое + локальное воспроизведение того же мутанта без HA (пуре-python путь того + же теста не зависит от HA-рантайма). +- Полное повторное ревью AC1/AC2/AC4/AC5 и остального продуктового кода — + сознательно не повторял: дельта r1→r2 их не трогает (см. «Унаследовано из + r1»), а полный разбор был выполнен и опубликован в r1. +- `golden:verify`, `npm run invariants`, браузерные смоки, performance — + не запускал: diff не меняет `src/**`, геометрию или рендер; + `smoke-select.mjs` подтвердил отсутствие кандидатов. +- WSL-полный HA прогон автора (заявлен в предыдущей передаче) — принят как + advisory, не как канон, не переисполнял. + +## Вердикт + +Единственная находка r1 закрыта доказательством, проверенным исполнением +дважды независимо от автора (лог CI-мутации + локальное воспроизведение в +этой сессии). Новых находок нет, High нет. Дельта локальна, продуктовый код +не тронут, трейлеры корректны. + +Вердикт: зелёный · заход r2 · блокирующих циклов 1/2 · High: 0 · Medium: 0 → в задаче + +--- + +--- + + + +## Материал раунда + +- Ветка: `issue/655-shutdown-flush`, коммит `f340157f59b7` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `f551607360b8f4ca652c065869a717735e2f2f5b` + ``` + git log --all --format='%H %T' | grep f551607360b8 + ``` +- Тело issue: `9592e83bda3f396fe43ee14eb6bc22b4860f8309d2c5e4e954dc1b90a293c348` +- Вердикт конвейера: `green` · High 0