docs: review document for #397

Issue: #397
User-Visible: no

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 не требуется. Находок в
+скоупе задачи, требующих новой правки, нет.
+
+**Вердикт: зелёный.**