diff --git a/docs/reviews/CODE-REVIEW-397-r2.md b/docs/reviews/CODE-REVIEW-397-r2.md new file mode 100644 index 00000000..05e03e07 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-397-r2.md @@ -0,0 +1,252 @@ +# CODE-REVIEW-397-r2 + +- Issue: #397 — «Undo позиций #74: своё же эхо сбрасывает историю, а + доказывающий это смок подстроен» +- ТЗ: `docs/specs/397-device-position-echo.md` (ревизия 2, принята + SPEC-REVIEW-397-r1/r2 → зелёным) +- Этап: code, заход r2, блокирующих циклов израсходовано 1 из 4 +- r1: `docs/reviews/CODE-REVIEW-397-r1.md`, вердикт жёлтый, SHA `08d56122` + (Medium-1: AC3 не имел теста; Low-1, Low-2 — не блокировали) +- Диапазон этого раунда: `git diff 08d56122..HEAD` (HEAD `f4c66bed`). + Единственный продуктовый коммит раунда — `f4c66bed` («test: prove AC3 on + the reconnect path, with a probe that can fail»); `b17f4167` — сам документ + r1, артефактов не меняет. + +## Почему разбор сужен до дельты, а не полный + +Дельта раунда — 26 добавленных / 3 удалённые строки в одном файле +(`demo/smoke_device_position_history.mjs`), только фикстура. Продуктовый код +(`src/houseplan-card.ts`) в этом раунде не менялся — фикс `d87cc298` тот же, +что проверялся в r1. Ребейза на dev не было (`git merge-base HEAD origin/dev` +не менялся между раундами — диапазон `origin/dev..HEAD` идентичен по базе), +контракт поведения не менялся, новая подсистема не задета. Условия §2.9 для +полного разбора не выполняются → разбор по AC ограничен AC3 (предмет +Medium-1) и тем, до чего дотягивается сама дельта — общими для нескольких AC +частями фикстуры, которые дельта тоже трогает (см. ниже). + +## Скоуп проверки дельты + +`demo/smoke_device_position_history.mjs`: + +1. Новый обработчик `houseplan/config/get` в фейковом `callWS` — эхо + `c._serverCfg`/`c._cfgRev`, чтобы `_adoptStructuralResponses` не отличил + `configChanged` по посторонней причине и проверка отвечала только за + layout-вопрос AC3. +2. `echoProbe`-позиция сменена с канонической `{x:0.42, y:0.42}` на + заведомо неканоническую `{x:0.024999999999999942, y:0.7/7}` — та же пара, + что уже использовалась в ТЗ как пример расхождения канонизации (39 из 115 + вида `px/800*0.997+0.0013` расходятся; `0.024999999999999942 → 0.025`). +3. Новая проверка `out.reconnectKeepsHistory`: после перемещения маркера — + `await c._loadFromServer()` (полный reconnect-путь, читает ОБА ответа) — + `canUndo` должен остаться `true`. + +Это ровно то, что просило Medium-1 из r1: доказательство для +`_adoptStructuralResponses` через `_loadFromServer`, а не только через +`_reloadLayoutOnly`. + +Поскольку `echoProbe`-позиция теперь общая для трёх проверок в одном блоке +(AC3 `reconnectKeepsHistory`, AC5б `deleteEchoKeepsHistory`, AC7 +`inFlightPositionWinsTheMerge`), дельта фактически затрагивает доказательства +всех троих — разбор по коду и прогон распространён на них тоже, а не только +на новую строку. + +## Как проверялось + +Зелёного Validate на `f4c66bed` нет — все гейты прогнаны локально. + +1. **Чтение диффа.** `git diff 08d56122..HEAD -- demo/smoke_device_position_history.mjs` + построчно сверен с текстом Medium-1 и с планом автотестов ТЗ (пункт 5). +2. **Чтение вызываемого пути.** `_loadFromServer` (`src/houseplan-card.ts:4283-4369`) + действительно шлёт `Promise.all([houseplan/config/get, houseplan/layout/get])` + и передаёт оба ответа в `_adoptStructuralResponses(cfgResp, layResp)` + (:4320) — то есть новая проверка идёт через второй путь, который в r1 был + не покрыт, а не через `_reloadLayoutOnly` ещё раз. Прочитан также + `_adoptStructuralResponses` (:4203-4269): `configChanged` считается по + `contentFingerprint(nextConfig)` vs `_cfgContentFingerprint` — при эхе + `c._serverCfg` без изменений это `false`, значит очистка истории по + `configChanged` (реальная, но не относящаяся к AC3 причина) исключена, и + единственная переменная в проверке — расхождение `_layout` из B3. +3. **Дешёвые гейты**, все на `HEAD` (`f4c66bed`), после `npm run bundle:sync`: + - `npx tsc --noEmit` — чисто. + - `npm test` — **1657 pass / 0 fail / 1 skip** (совпадает с r1; продуктовый + код не менялся, расхождения не ожидалось). + - `npm run build` — зелёный; `git status --short` после сборки пуст → три + копии бандла (`dist/`, `custom_components/houseplan/frontend/`, + `demo/srv/assets`) совпадают с закоммиченными. + - `node scripts/check-docs.mjs` — не прогонял: дельта не трогает `src/**` + (только `demo/**`), условие обязательности гейта не выполняется. + - `npm run invariants` / `python -m pytest tests_backend` — не применимы: + геометрия, рёбра, `layout`-ключи решётки и `custom_components/**/*.py` + дельтой не затронуты. + - `npm run golden:verify` — не применим: диф не меняет рендер/геометрию/ + стили/слои, только служебную фикстуру теста. +4. **`node scripts/smoke-select.mjs --base 08d56122 --head HEAD`** — вывод: + «Исполняемого frontend-диффа нет (`src/**/*.ts` не тронут). Browser-smoke + этим диффом не выбираются — это не "пропустить проверки", а "выбирать + нечего": смоки проверяют собранную карточку. Тронуто файлов: 2.» Полную + матрицу (209 файлов) не гонял: инструмент прямо говорит, что выбирать + нечего, диф — только фикстура одного уже названного АС3 смока, продуктовый + код не менялся. +5. **`node demo/smoke_device_position_history.mjs`** — прогнан явно на + `HEAD`: все 31 проверка зелёные, включая новую `reconnectKeepsHistory: + true`. +6. **Дисциплина «тест умеет падать» — проверена вручную, не принята на + слово автора.** В `_persistDevicePlacement` (`src/houseplan-card.ts:5246-5250`) + временно заменил + ``` + const pos = canonicalizePosition(this._layout[deviceId]); + if (contentFingerprint(pos) !== contentFingerprint(this._layout[deviceId])) { + this._layout = { ...this._layout, [deviceId]: pos }; + } + pending = pos; + ``` + на + ``` + const pos = canonicalizePosition(this._layout[deviceId]); void pos; + pending = pos; + ``` + (тот же мутант, что описан в `device-echo-keeps-local-noncanonical`), + пересобрал (`npm run bundle:sync`) и перезапустил смок. Результат: + ``` + FAILED (5): + - localCopyEqualsTheWire: expected true, got false + - sameContentReloadKeepsHistory: expected true, got false + - ownEchoMatchesWhatWentOverTheWire: expected true, got false + - reconnectKeepsHistory: expected true, got false + - deleteEchoKeepsHistory: expected true, got false + ``` + Совпадает буквально с заявлением автора в тексте коммита («five checks + red… including reconnectKeepsHistory and deleteEchoKeepsHistory»). + `deleteEchoKeepsHistory` краснеет не потому, что ветка удаления сама + сломана — а потому, что предшествующий `_loadFromServer()` в этом же + прогоне уже обнулил `_devicePositionHistory` (расхождение реальное, + `canUndo` остаётся `false` до конца блока); при исправленном коде тот же + вызов ничего не чистит, и `deleteEchoKeepsHistory` проверяет свою + собственную ветку независимо — это подтверждено зелёным прогоном на + восстановленном коде (см. п.5). Каскад ожидаем и не обесценивает + проверку. После теста откатил файл (`git checkout -- src/houseplan-card.ts`, + `git status --short` снова пуст) и пересобрал (`npm run bundle:sync`), + вернув дерево в состояние `HEAD`; финальный прогон смока — снова все 31 + зелёные (см. «Что проверено и корректно»). +7. **Трейлеры.** `git show f4c66bed` — `Issue: #397`, `User-Visible: no`. + Корректно: изменение затрагивает только тестовую фикстуру, видимого + поведения не меняет (сам видимый эффект уже описан в CHANGELOG коммитом + `d87cc298` из r1). Отдельной правки CHANGELOG не требуется и не добавлено. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **Medium-1**: AC3 не имел теста — `_adoptStructuralResponses`/`_loadFromServer` не упоминались ни в одном изменённом файле | `demo/smoke_device_position_history.mjs` теперь вызывает `await c._loadFromServer()` сразу после записи неканонической позиции и пишет `out.reconnectKeepsHistory`; серверный конфиг в фейковом WS зеркалит `c._serverCfg`, чтобы исключить `configChanged` как постороннюю причину очистки | `demo/smoke_device_position_history.mjs:296-304` (проверка), `:37-42` (мок `config/get`); падение подтверждено вручную (раздел «Как проверялось», п.6) — 5 красных, включая `reconnectKeepsHistory` | +| **Low-1**: AC1 доказан не тем способом, что назвало ТЗ (браузерный смок + текстовый юнит вместо юнита с перехватом `callWS`) | Не тронуто в этом раунде | Оставлено на усмотрение автора, как и предлагал r1; не блокирует | +| **Low-2**: план ТЗ (риски, п.3) просил отдельный юнит на «каноническая позиция не создаёт лишней записи» — теста нет, только код-гвард | Не тронуто в этом раунде | То же; гвард в коде не менялся и остаётся корректным (см. «Унаследовано») | + +Low-1 и Low-2 не были предметом Medium и не требовали правки в этом раунде; +формально они остаются открытыми, но не блокируют — решение об их закрытии +(комментарием или правкой) остаётся за автором, как и было сказано в r1. + +## Разбор по AC (только то, чего касается дельта) + +- **AC3** (тот же инвариант для `_adoptStructuralResponses`, reconnect-путь) + — **выполнено**. Доказано смоком `reconnectKeepsHistory` через реальный + `_loadFromServer()`; падает без фикса, зеленеет с фиксом (проверено + вручную, не на слово). Пункт 5 плана автотестов ТЗ закрыт. +- **AC5б** (эхо удаления не чистит историю) — уже был «Выполнено» в r1; + затронут дельтой (та же `echoProbe`-позиция, тот же блок). Перепроверено: + зелёный на `HEAD`, независимо подтверждает свою ветку (без искусственной + зависимости от AC3 при исправленном коде — см. п.6 «Как проверялось»). + Остаётся выполненным. +- **AC7** (запись в полёте побеждает ответ сервера) — уже был «Выполнено» в + r1; использует тот же `echoProbe`, но собственные значения позиции + (`inFlightPos = {x:0.63,y:0.21}`), не изменённые этой дельтой. Перепроверено + прогоном — зелёный, `inFlightPositionWinsTheMerge: true`. Остаётся + выполненным. + +## Унаследовано из r1 + +Документ: `docs/reviews/CODE-REVIEW-397-r1.md`, SHA `08d56122`. Принято без +повторной проверки в этом раунде (продуктовый код не менялся): + +- **AC1** — порядок операций в `_persistDevicePlacement`, локальная запись + канонического значения раньше `_sentPos.set` и раньше `callWS`. Юнит + `test/device-position-echo.test.mjs` и смок `localCopyEqualsTheWire` не + тронуты дельтой, продуктовый код (`src/houseplan-card.ts:5226-5261`) тоже — + оснований перепроверять нет. +- **AC2** (`sameContentReloadKeepsHistory` + `ownEchoMatchesWhatWentOverTheWire`) + — файл-источник (`_reloadLayoutOnly`, `:4904-4948`) не менялся, фикстура в + этой части (использует `deviceId`, не `echoProbe`) дельтой не затронута. +- **AC4** (`remoteContentClearsHistory`) — то же самое, вне блока `echoProbe`, + код и фикстура в этой части не менялись. +- **AC5а** (`applyDevicePlacement(layout, id, null)` удаляет ключ до + фиксации отпечатка) — юнит-доказательство, продуктовый код не менялся. +- **AC6** (смок краснеет на коде до фикса) — доказательство r1 относилось к + прежней версии смока (3 красных проверки); в этом раунде переисполнено + заново на новой версии смока (см. «Как проверялось», п.6, 5 красных + проверок) — это не наследование, а повторное независимое доказательство, + что фиксирую отдельно, чтобы не создавать видимость слепого доверия там, + где дельта как раз и меняет предмет AC6. +- **Мутант `device-echo-keeps-local-noncanonical`**, регистрация в + `scripts/mutation-gate.mjs`, согласованность с `test/mutation-gate.test.mjs` + — файлы не в дельте, не перепроверялись. +- **Трейлеры и CHANGELOG коммита `d87cc298`** — не в дельте, r1 подтвердил. +- **Бандл-артефакты, актуальность скриншотов на момент `08d56122`** — вне + дельты по скриншотам; актуальность на `HEAD` подтверждена заново в этом + раунде (см. «Как проверялось», п.3 — `git status --short` пуст после + пересборки), а не унаследована слепо, так как сборка в любом случае + перезапускалась для falsification-теста. + +## Что проверено и корректно + +- `houseplan/config/get`-мок зеркалит `c._serverCfg`/`c._cfgRev` без + изменений → `configChanged` в `_adoptStructuralResponses` остаётся `false` + на протяжении всего сценария AC3, значит `reconnectKeepsHistory` отвечает + ровно на layout-вопрос, а не на посторонний config-вопрос. +- Проба `{x:0.024999999999999942, y:0.7/7}` — действительно неканоническая + (снапается на `0.025`/`0.1`), то есть чек не мог бы проходить «по случаю», + как это было с прежним `{0.42,0.42}` (M1-класс дефекта, но для AC3, а не + только для AC2, — и он же теперь закрыт заодно). +- Финальный прогон смока на восстановленном `HEAD` — 31/31 зелёных, включая + `reconnectKeepsHistory`, `deleteEchoKeepsHistory`, `inFlightPositionWinsTheMerge`. +- Дерево репозитория после ручного falsification-теста восстановлено: + `git status --short` пуст, `git diff` пуст. +- Трейлеры `f4c66bed`: `Issue: #397`, `User-Visible: no` — корректно для + теста-only коммита. + +## Чего не проверял и почему + +- **GitHub-комментарии к issue #397** — MCP-инструмент `get_issue_comments` + отклонён средой (permission not granted) на трёх попытках подряд; вердикт + и контекст r1 восстановлены по коммитам и по самому документу + `CODE-REVIEW-397-r1.md`, чего достаточно для этого раунда — SHA и находки + там названы явно. Если в комментариях есть более свежее решение по Low-1/ + Low-2 (например, автор их уже закрыл словом), эта информация не попала в + документ; на вердикт это не влияет, так как Low не блокирует независимо от + того, закрыт он явно или нет. +- **`node scripts/check-docs.mjs`** — дельта не трогает `src/**`, условие + обязательности гейта не выполняется. +- **`npm run invariants`, `python -m pytest tests_backend`** — геометрия, + `layout`-ключи решётки и `custom_components/**/*.py` не затронуты ни этой + дельтой, ни диапазоном в целом (r1 это тоже отметил). +- **`npm run golden:verify`** — диф не меняет рендер/геометрию/стили/слои. +- **Полная матрица `demo/smoke_*.mjs` (209 файлов)** — `smoke-select.mjs` + прямо ответил «выбирать нечего» (диф вне `src/**`); диф — фикстура одного + уже названного АС смока, поверхность не расширена. +- **`scripts/mutation-gate.mjs` (полный/точечный)** — не в дельте, не + перезапускал; r1 уже подтвердил регистрацию и согласованность + (`test/mutation-gate.test.mjs`, 10/10) на неизменном с тех пор файле. +- **HA-харнесс** — в песочнице нет Home Assistant; предрелизный гейт, не + гейт ревью. + +## Вывод + +Medium-1 закрыт доказательно (не на слово автора — falsification +воспроизведена и совпадает буквально: 5 красных проверок, включая +`reconnectKeepsHistory`). AC3 выполнен. Побочно исправлена скрытая слабость +доказательства AC5б (прежняя каноническая проба `0.42` не могла бы поймать +регрессию своей же ветки — теперь может, за счёт общей неканонической +`echoProbe`-позиции). Low-1 и Low-2 остаются открытыми, но не блокирующими, +как и в r1. Продуктовый код не менялся, три копии бандла синхронны, +трейлеры корректны, User-Visible: no — CHANGELOG не требуется. Находок в +скоупе задачи, требующих новой правки, нет. + +**Вердикт: зелёный.**