From edf532e217f8180d2aee1174d213fe1d1faf4ccc Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 17:53:38 +0000 Subject: [PATCH] docs: review document for #205 Issue: #205 User-Visible: no --- docs/reviews/CODE-REVIEW-205-r1.md | 253 +++++++++++++++++++++++++++++ 1 file changed, 253 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-205-r1.md diff --git a/docs/reviews/CODE-REVIEW-205-r1.md b/docs/reviews/CODE-REVIEW-205-r1.md new file mode 100644 index 00000000..52294abf --- /dev/null +++ b/docs/reviews/CODE-REVIEW-205-r1.md @@ -0,0 +1,253 @@ +# CODE-REVIEW-205-r1 — продолжение следа пылесоса после короткой остановки + +- Issue: [#205](https://github.com/Matysh/houseplan-card/issues/205) +- Этап: `code` (PROCESS.md §2.7) +- Диапазон: `origin/dev...HEAD`, `origin/dev` = `ad8e7a5`, `HEAD` = `11b0283` + (ветка `issue/205-vacuum-trail-grace`, detached `HEAD`); merge-base — `f287bdd` +- ТЗ: [`docs/specs/205-vacuum-trail-resume-grace.md`](../specs/205-vacuum-trail-resume-grace.md), + ревью ТЗ зелёное — [`SPEC-REVIEW-205-r1.md`](SPEC-REVIEW-205-r1.md) +- Цикл: **r1/4** +- Ревьюер: Claude, свежая сессия, без переписки с автором реализации +- Вердикт: **зелёный** + +## Скоуп ревью + +Три коммита диапазона: + +1. `f3472de` «docs: specify vacuum trail resume grace» — ТЗ (класс C), уже + зелёное ревью ТЗ (`SPEC-REVIEW-205-r1.md`), не пересматриваю содержание + заново, только сверяю, что реализация ему соответствует. +2. `c47a6bc` «docs: review document for #205» — сам документ ревью ТЗ (класс C). +3. `11b0283` «fix: resume vacuum trails after short stops» — единственный + продуктовый коммит (класс A/B), `Issue: #205`, `User-Visible: yes`, 11 файлов: + `custom_components/houseplan/trails.py`, `demo/smoke_vacuum.mjs`, + `scripts/mutation-gate.mjs`, `scripts/trail-resume-test-guard.mjs` (новый), + `tests_backend/test_trails.py`, `tests_backend/test_trail_recorder.py`, + `docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`, `docs/STATUS.md`, + `docs/TESTING.md`, `docs/USER-GUIDE.ru.md`, `docs/VACUUM.md`. + +`src/**`, i18n и `dist/**`/сгенерированные копии бандла не тронуты — согласовано +со спекой («фронтенд не требует правок», подтверждено ещё на этапе spec-review +чтением `_renderVacuums`). Прочитано целиком: `docs/SCOPE.md`, `AGENTS.md`, +`PROCESS.md`, тело issue #205 и все 6 комментариев (аналитика Q1–Q4, +«ТЗ готово», вердикт spec-review, хендофф реализации, занятие слота S7), +`docs/specs/205-vacuum-trail-resume-grace.md`, `SPEC-REVIEW-205-r1.md`, +`docs/VACUUM.md` целиком, весь diff `custom_components/houseplan/trails.py` +(построчно, не выборочно), оба изменённых тестовых файла целиком, оба скрипта +mutation-gate/guard, все изменения в changelog/`STATUS.md`/`TESTING.md`/ +`USER-GUIDE.ru.md`/`VACUUM.md`. + +## Как проверялось + +Зависимости и Chromium уже установлены средой ревью; `npm ci` не запускался. +Backend Python в этой среде без `pytest`/`voluptuous` — оба установлены +`pip install` перед прогоном (сеть доступна), чтобы гейт был реальным +исполнением, а не пропуском. + +| Гейт | Команда | Результат | +|---|---|---| +| Типы | `npx tsc --noEmit` | green | +| Unit | `npm test` | **912/912** green | +| Сборка + синхронность бандлов | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js && cmp dist/houseplan-card.js demo/srv/assets/houseplan-card.js` | green, обе копии байт-в-байт совпадают (diff не трогал `src/**`, пересборка ничего не меняет) | +| Whitespace | `git diff --check origin/dev...HEAD` | чисто, без предупреждений | +| Backend — targeted | `python -m pytest tests_backend/test_trails.py tests_backend/test_trail_recorder.py -q` | **37 passed** | +| Backend — полный pure/stub набор | `python -m pytest tests_backend -q` (после установки `pytest`+`voluptuous`) | **172 passed**, совпадает с числом из хендоффа автора; `test_ha_*.py` собираются и включены в счёт (HA-пакет не установлен в среде ревью, но conftest не молчаливо их проигнорировал — они реально прошли в этом прогоне за счёт остального стека; полный HA-harness всё равно канон только на Linux CI, см. «Чего не проверял») | +| Целевой смок (назван в AC8) | `node demo/smoke_vacuum.mjs` | `OK` | +| Mutation guard (AC10), точечно | `node scripts/mutation-gate.mjs --id=vacuum-trail-resume-disabled` | `ok чистый прогон` → `ok vacuum-trail-resume-disabled: тест покраснел, как обязан` → `поймано 1 из 1` | +| Mutation gate, полный набор (дёшево, `~0.07s`, весь список уже мал) | `node scripts/mutation-gate.mjs --check` | все 36 мутантов пойманы, включая новый | + +Не прогонялись (соразмерность гейта, PROCESS.md §8): полный набор из 127 +браузерных смоков (diff не задевает рендер, canvas, редакторы — только один +targeted smoke, названный в AC8, и он прогнан), `npm run golden:verify` +(visual-diff не может измениться: diff не трогает ни один файл рендера, путь +проверяется численно по SVG `d`/points, как и оговорено в разделе 10 ТЗ), +performance-профили (в AC не названы, изменение O(1) на точку, асимптотика не +меняется). Полный HA-harness backend — канонически только Linux CI +(`AGENTS.md`: `.venv-backend` недоступен в среде ревью). + +### Проверка дисциплины «тест умеет падать» + +Для AC10 — не поверил заявлению автора «mutant caught (1/1)», а повторил сам: +`node scripts/mutation-gate.mjs --id=vacuum-trail-resume-disabled` действительно +патчит `resumed = bool(cur and can_resume_trail_run(cur, map_id, now))` → +`resumed = False` и заново гоняет `scripts/trail-resume-test-guard.mjs` +(таргетированный `pytest -k "resume or short_available"` по обоим backend-файлам); +гейт подтверждает, что при отключённом resume фокусный набор действительно +краснеет, а на чистом дереве — зелёный. Это прямое эмпирическое доказательство, +что новые тесты чувствительны к самому фиксу, а не тавтологичны. + +## Построчная проверка кода против контракта ТЗ (раздел 6) + +Читал `custom_components/houseplan/trails.py` целиком, не только diff. + +- **Пункт 1 (resume при совпадении map_id и `0 ≤ now-ended ≤ 1800`).** + `can_resume_trail_run` проверяет `run.get("map_id") == map_id` до всего + остального; при совпадении и `elapsed` в допустимом диапазоне `on_point` + ставит `cur["ended"] = None` и не создаёт `previous` (условие + `cur.get("ended") is not None or cur.get("map_id") != map_id` после этого + ложно) — точное соответствие. +- **Пункт 2 (граница 30:00 включительно, эпсилон выше — новый run).** + `elapsed <= TRAIL_RESUME_GRACE_S` — нестрогое неравенство, `1800.0` резолвится + в `True`; `1800.000001` — в `False`. Подтверждено исполнением + `test_resume_grace_is_inclusive_then_rotates_after_epsilon`. +- **Пункт 3 (смена карты — всегда жёсткая граница).** `can_resume_trail_run` + возвращает `False` при несовпадении `map_id` независимо от `elapsed`; + дальше сработает обычная ветка ротации. Подтверждено + `test_resume_never_crosses_maps_and_malformed_timestamps_fail_closed`. +- **Пункт 4 (дубликат первой точки после resume: `changed=True`, точка не + добавляется).** `pts and pts[-1] == [x,y]: return resumed` — если resume + произошёл, `resumed=True` возвращается даже без добавления точки; если нет — + `False`, как раньше. Подтверждено + `test_ended_run_resumes_within_grace_and_duplicate_is_a_change`. +- **Пункт 5 (malformed/отрицательное/nan/inf/bool не даёт resume, safely новый + run).** Явные проверки `isinstance(ended, bool)` (bool — подкласс int, + исключён явно), `not isinstance(ended, (int, float))`, тот же контроль для + `now`, и `math.isfinite(elapsed)` после вычитания — отсекает `nan`/`inf` + и переполнения. Подтверждено 4 кейсами (`"bad"`, `True`, `nan`, `inf`) в + `test_resume_never_crosses_maps_and_malformed_timestamps_fail_closed` плюс + отдельным clock-rollback кейсом в + `test_resume_fails_closed_on_clock_rollback_and_survives_restart_shape` + (`ended=1000.0`, `now=900.0` → `elapsed=-100` → `0 <= elapsed` ложно). +- **Пункт 6 (idempotent `end_run`, timestamp первого stop не сдвигается).** + `end_run` изменился с `if cur and not cur.get("ended")` на + `if cur and cur.get("ended") is None` — при повторном вызове на уже + завершённом run условие ложно, `ended` не перезаписывается. Подтверждено + `test_repeated_short_stops_keep_all_points_and_existing_previous` (три цикла + dock/pause, `ended` не двигается) и recorder-тестом + `test_short_available_stops_resume_one_run_and_neutral_states_do_not_shift_window`. +- **Пункт 7 (`unavailable`/`unknown`/missing нейтральны, не двигают окно).** + Код `_sample` здесь не менялся — ветка `continue` для этих состояний стоит + до вызова `end_run` (`if not st_vac or st_vac.state in ("unavailable", + "unknown"): continue`), заявление ТЗ «уже нейтрально» подтверждено чтением, а + не переоткрыто. Recorder-тест подтверждает и то, что после этих состояний + `ended` того же значения, что до них. +- **Пункт 8 (единый контракт для любого available non-moving state).** + `can_resume_trail_run` не смотрит на конкретное значение `state` вообще — + оно уже отфильтровано в `_sample` до вызова `end_run`/`on_point`, поэтому + единство контракта следует из самой структуры кода, а не из перечисления + случаев. Подтверждено параметризованным + `test_any_available_nonmoving_state_uses_the_same_grace_contract` по пяти + состояниям (`paused`, `idle`, `error`, `washing`, `docked`) — то есть даже + вендорские атрибуты типа `washing` (явно вне scope, раздел 5 ТЗ) корректно + идут по общему пути, а не игнорируются по имени. + +Побочная проверка: изменение `end_run`/`on_point` с truthy-проверок (`not +cur.get("ended")`, `cur.get("ended")`) на `is None`/`is not None` меняет +поведение только для гипотетического `ended == 0` (реальные epoch-timestamps +никогда не бывают нулевыми) — это исправление, не регресс: `0` — валидный, +хоть и нереалистичный, timestamp окончания, и `is None` корректно отличает +«run активен» от «run завершён в момент 0». + +## Проверка AC1–AC11 + +| AC | Статус | Как доказано | +|---|---|---| +| AC1 | Доказан | `test_ended_run_resumes_within_grace_and_duplicate_is_a_change`, `test_short_available_stops_resume_one_run_and_neutral_states_do_not_shift_window` — исполнены. | +| AC2 | Доказан | `test_resume_grace_is_inclusive_then_rotates_after_epsilon` — исполнен, граница явная (`+0`/`+0.001`). | +| AC3 | Доказан | `test_resume_never_crosses_maps_...` (хвост про `floor-a`/`floor-b`) — исполнен. | +| AC4 | Доказан | `test_ended_run_resumes_within_grace_and_duplicate_is_a_change` — исполнен, проверяет и `points`, и возвращаемое значение. | +| AC5 | Доказан | `test_repeated_short_stops_keep_all_points_and_existing_previous` — исполнен, три цикла, `previous is previous` (не тронут). | +| AC6 | Доказан | `test_short_available_stops_...` (unknown/missing) + `test_any_available_nonmoving_state_...` — исполнены. | +| AC7 | Доказан | `test_resume_fails_closed_on_clock_rollback_and_survives_restart_shape` — конструирует `TrailBook` из «сохранённого» словаря (форма Store после рестарта), проверяет resume и rollback. | +| AC8 | Доказан | `node demo/smoke_vacuum.mjs` — исполнен, зелёный; отдельно прочитан рендер-путь (`showCur`/`srvCur` не зависят от `ended`), поэтому `always`-часть AC8 достаточно покрыта уже существующим (не новым) сценарием в том же смоке — новый код не меняет эту ветку рендера вовсе (см. ниже). | +| AC9 | Доказан | Полный прогон `tests_backend/test_trails.py` + `test_trail_recorder.py` (37 тестов) включает нетронутые `test_map_switch_mid_run_starts_a_new_run`, `test_cap_decimates_but_keeps_the_freshest_point`, `test_junk_store_data_tolerated`, source-health/two-floor/decimation/delete-тесты в `test_trail_recorder.py` — все зелёные, без изменений в самих тестах. | +| AC10 | Доказан | `node scripts/mutation-gate.mjs --id=vacuum-trail-resume-disabled` — мутант пойман, гайд-скрипт исполнен, не просто заявлен. | +| AC11 | Доказан | Таблица гейтов выше — typecheck/unit/build/targeted smoke/pytest все зелёные, исполнены в этой сессии, не приняты на слово. | + +Проверка «AC8 не требует правки always-ветки» — не догадка: `_renderVacuums` +(`src/houseplan-card.ts:16664-16681`) вычисляет `showCur` только по +`trail_mode`/`moving`, а `srvCur` фильтрует `srv.current` только по `map_id` и +наличию `points`; поле `ended` нигде не читается фронтендом (`grep -n "ended" +src/houseplan-card.ts src/vacuum.ts` — единственные совпадения относятся к +локальному `endedTs` рантайм-буфера, не к серверному payload). Значит, стиль +current/previous в `always` не может отличаться между «обычным активным +current» и «только что возобновлённым current» — новый смок-сценарий +(`srvResumedCurrentKeepsEarlierPoints`) специально нацелен на `cleaning`-режим, +где риск реален (скрыть/показать переключается по `moving`), а не дублирует +уже существующий `always`-сценарий того же файла — читал оба сценария рядом, +пересечения по риску нет. + +## Находки + +Ни одной **High**. Ни одной **Medium**. Ни одной **Low**. + +Мелкое стилистическое наблюдение — `resumed = bool(cur and +can_resume_trail_run(...))`: обёртка `bool()` не меняет поведение +(`cur and X` уже даёт `None`/`bool`, и оба варианта корректно приводятся к +`False`/значению в `if`/`return`), просто фиксирует тип явно для функции, +объявленной как `-> bool`. Не поведенческий дефект, не блокирует и не стоит +отдельной записи как Low — это осознанный стиль, а не небрежность. + +## Что проверено и признано корректным + +- **Причина и контракт ТЗ реализованы буквально** — каждый из 8 пунктов + раздела 6 сверен построчно с кодом и подтверждён исполнением + соответствующего теста (см. таблицу выше), не принят на слово ни разу. +- **Scope/non-scope соблюдены.** Diff не трогает `MOVING_STATES`, `trail_mode`, + vendor-атрибуты (`washing`/`drying`/`emptying` использованы только как + тестовое значение состояния, а не как отдельная ветка кода), source health + monitor, calibration, decimation/cap механику, Store version/shape. Новая + константа `TRAIL_RESUME_GRACE_S` — module-level, без конфигурации, как и + зафиксировано в «принятых предположениях» ТЗ (раздел 13, пункт 2). +- **Тесты умеют падать, не только показывают зелёное.** Собственноручно + воспроизвёл mutation-guard (AC10) и убедился, что таргетированный набор + краснеет при отключённом resume и зеленеет на реальном коде — не поверил + комментарию автора «mutant caught (1/1)». +- **Гейты воспроизведены самостоятельно**, не переписаны из хендоффа: + typecheck, 912/912 unit, сборка+синхронные бандлы, 172 backend pure/stub + теста (после установки отсутствовавших в среде `pytest`/`voluptuous`), + целевой смок, полный (дешёвый) прогон mutation-gate. Совпадение чисел с + отчётом автора (912, 172, 1/1) — не причина не перепроверять, а + independent confirmation того же результата. +- **Документация и трейлеры.** `docs/CHANGELOG.md`/`docs/CHANGELOG.ru.md` + правлены в том же коммите `11b0283`, что и код (`User-Visible: yes`) — + соответствует правилу №10/PROCESS.md §2.6. `docs/VACUUM.md` и + `docs/USER-GUIDE.ru.md` описывают контракт словами, совпадающими с ТЗ, без + придуманной терминологии. `docs/TESTING.md` называет конкретные файлы + доказательства. Ветка `issue/205-vacuum-trail-grace` соответствует + трейлерам `Issue: #205`. `git diff --check` чист. +- **Не расширяет скоуп.** Единственный продуктовый файл — + `custom_components/houseplan/trails.py`; изменение минимально (одна новая + функция, три изменённых условия), никаких «раз уж я здесь» правок в + соседних областях (calibration, source health, decimation) не найдено при + построчном чтении всего файла. + +## Чего не проверял + +- **Полный набор из 127 браузерных смоков** — не запускал; diff не касается + canvas/редакторов/other rendering surfaces, единственный релевантный смок + (`smoke_vacuum.mjs`, назван в AC8) исполнен явно. Сужение гейта осознанное + (PROCESS.md §8), не молчаливый пропуск. +- **`npm run golden:verify`** — не запускал; diff не меняет ни один файл + рендера/геометрии/стилей, path continuity в AC8 доказывается численно через + SVG `d`/points, не пикселями, как и предписывает раздел 10 ТЗ. +- **Полный HA-harness backend (`test_ha_*.py` с реальным Home Assistant)** — + в среде ревью нет `.venv-backend`/пакета `homeassistant`; канонический + прогон — Linux CI на точном SHA (AGENTS.md, PROCESS.md §8). Косвенно эти + файлы всё же собрались и прошли в общем прогоне `pytest tests_backend -q` + этой сессии (172 passed), но не считаю это заменой CI-гейта — Home + Assistant в этой среде не установлен, только его тестовые фикстуры без + реального рантайма. +- **Perf-профили** — не названы в AC, изменение алгоритмически O(1) на точку + (одна дополнительная проверка условия), асимптотика решения не меняется. +- **Живой робот/симулятор демо-стенда** — вне цикла код-ревью без ручного + тестирования (PROCESS.md §2 — фазы тестирования нет по конструкции + процесса); задача доказывается автотестами, что и сделано. +- **Возможное визуальное склеивание двух реальных уборок в пределах 30 минут** + — явно принятый владельцем trade-off (issue, комментарий аналитики Q3, + раздел 6 ТЗ «неизбежный trade-off принят владельцем»), не дефект этой + реализации. + +## Вердикт + +**Зелёный · цикл r1/4 · High: 0 · Medium: 0 → нет новых issue.** + +Реализация — узкая, точная правка `TrailBook.on_point`/`end_run` плюс одна +новая чистая функция `can_resume_trail_run`, буквально соответствующая +восьмипунктовому контракту ТЗ раздела 6. Каждый пункт контракта и каждый из +AC1–AC11 подтверждён самостоятельным исполнением тестов и гейтов в этой +сессии (не принят на слово из хендоффа автора), включая прямую проверку, что +mutation-guard AC10 реально красный при отключённом фиксе. Документация и оба +changelog обновлены в том же коммите, что и код, трейлеры корректны, скоуп не +расширен. Следующий статус — очередь на пре-релиз (`S8-merged` после мёржа в +`dev`, PROCESS.md §2.7 / AGENTS.md «Do not merge into `dev` by hand»).