diff --git a/docs/reviews/SPEC-REVIEW-440-r1.md b/docs/reviews/SPEC-REVIEW-440-r1.md new file mode 100644 index 00000000..47323106 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-440-r1.md @@ -0,0 +1,151 @@ +# SPEC-REVIEW #440 — r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/440 +- Этап: spec (PROCESS.md §2.4) +- ТЗ: `docs/specs/440-v171-beta2-polish.md`, коммит `735710f1` +- Заход r1 · блокирующих циклов израсходовано 0/4 +- Вердикт: **зелёный** + +## Скоуп ревью + +Полный разбор — заход первый, дельты нет. ТЗ покрывает семь пунктов аудита +v1.71.0-beta.2 (§3.3): (а) зависание verifier на не-обычном файле, (б) потеря +pointer-modality на room pointermove при выключенном tooltip, (в) getter с +побочным эффектом `_dangerConfirmLocaleGate`, (г) TOCTOU в физической +инвентаризации ассетов + неверный HTTP 507, (д) source-regex свидетель AC5 +#434, (е) отсутствие ретроактивных review-документов для #429/#430, (ж) +`importorskip("homeassistant")`, скрывающий чистые Python-тесты канонизации. + +Читал в заданном порядке: `docs/SCOPE.md`, `PROCESS.md` §1–§8, тело issue +#440 и оба комментария (аналитика + сдача ТЗ), само ТЗ целиком, +`docs/TOUCH-SUPPORT.md` §«Pointer modality and hover ownership». + +## Как проверялось + +Ревью ТЗ — не код-ревью, но каждое фактическое утверждение ТЗ о текущем +поведении кода я сверил с реальным деревом на `origin/issue/440-audit-polish` +(= `735710f1`), чтобы отличить обоснованный контракт от догадки, выданной за +факт: + +| Пункт ТЗ | Файл:строка | Что проверено | +|---|---|---| +| (а) verifier/FIFO | `custom_components/houseplan/asset_integrity.py:42-129` | `_signature()` = голый `path.stat()` (проходит на FIFO/device), `_stream_sha256()` открывает `open("rb")` без таймаута, followers ждут `event.wait()` без границы. Совпадает с описанием ТЗ дословно | +| (б) pointer modality | `src/houseplan-card.ts:11589-11675`, `:7186-7193`, `:7291-7304` | `tip(e)` (bound на `@pointermove`) при `!showRoomTooltipOf(...)` возвращается на 11592-11593 **до** вызова `_notePointer`; `enterRoom` (bound на `@pointerenter`) вызывает `_notePointer` всегда. Подтверждает узкую щель: смена pointer type внутри уже наведённой комнаты не долетает до `_notePointer`, когда tooltip выключен | +| (в) locale getter | `src/houseplan-card.ts:2160-2166,2188,4193`, `src/i18n/language-runtime.ts:105-131` | Геттер `_dangerConfirmLocaleGate` вызывает `languageRenderGate()`, которая мутирует `host.inert`, `aria-busy`, `lang`, два `WeakSet`, планирует `requestUpdate()` — вызывается из `_confirmDanger()` и `updated()`, вне рендера. Подтверждено | +| (г) TOCTOU/507 | `custom_components/houseplan/decor_assets.py:376-394`, `http_api.py:262-347` | `physical_asset_blobs` не оборачивает `path.stat(follow_symlinks=False)` внутри цикла `iterdir()` — исчезновение даёт необработанный `OSError`, который поднимается через `physical_asset_usage` → `_store()` → перехватывается только на `except OSError` (500). Отдельно `except DecorAssetError: … status=507` на строке 344 ловит **любой** `DecorAssetError` из `_store()`, включая `invalid_image` (273, 289) — сейчас всегда 507. Оба факта подтверждены | +| (д) AC5 regex | `test/space-card-audit-lows.test.mjs:28-39` | Ровно пять `assert.match` по тексту `src/space-card.ts`/`src/config-store.ts`, как описано в ТЗ | +| (ж) importorskip | `tests_backend/test_coordinate_canonicalization.py:1-40` | Модульный `pytest.importorskip("homeassistant")` стоит выше импорта `coordinate_canonicalization` и `DECOR_BOX_KINDS` — чистые тесты (`test_decor_box_catalog_matches_shared_contract`, `test_all_4801_lattice_nodes_and_nine_decimal_forms_share_exact_bits`) реально скипаются без HA | +| AC6 инфраструктура | `scripts/mutation-gate.mjs:2402-2450` | Мутанты `image-box-python-canonicalization-omitted` и `all_4801_lattice_nodes...` уже указывают на `tests_backend/test_coordinate_canonicalization.py` через `backend-test-guard.mjs` — перенаправление на новый модуль механически осуществимо, путь не изобретён | +| TOUCH-SUPPORT.md | `docs/TOUCH-SUPPORT.md:43-57` | «Touch and pen input immediately clear transient room and device hover, including tooltips» — контракт п.2 ТЗ («независимо от настройки») не изобретён, а прямая цитата канона | +| i18n | `src/i18n/{en,ru,de,fr}.json` | Все четыре словаря существуют, ТЗ верно перечисляет «не меняются» | +| Смоки в «Затронутых модулях» | `ls demo/smoke_danger_confirm_branches.mjs demo/smoke_room_tooltip_toggle.mjs demo/smoke_space_card_decor_capability.mjs` | Все три файла существуют | +| `docs/specs/README.md` | `git show HEAD -- docs/specs/README.md` | Строка на #440 добавлена в том же коммите, ссылка на файл корректна | + +Проверка кодом не подтверждает и не опровергает пункт (е) — это решение +процесса, а не факт кода; ТЗ фиксирует его как аналитический вывод без +кодовых изменений, что соответствует PROCESS.md §1 (инфраструктурные задачи +без файлов класса A идут без ТЗ/код-ревью). + +## §7.1 — обязательные разделы + +Все присутствуют: Сценарий · Что человек увидит до и после · Проблема и +подтверждённые причины · Скоуп/Не-скоуп · Контракт поведения (7 пунктов) · +Touch и доступность (UX) · Модель данных, совместимость и i18n · Производительность +и безопасность · Затронутые модули · Критерии приёмки AC1–AC8 с доказательством +· таблица «чем краснеет» для AC1–AC6 · План автотестов · Риски · Откат · +Release-артефакты · блок принятых технических предположений. Трек (`full`) +обоснован явно названным нарушенным критерием лёгкого трека (несколько +поверхностей, TS+Python, touch-контракт) — соответствует PROCESS.md §5. + +## Догадки, выданные за факт + +Не найдены. Каждое утверждение о текущем поведении сверено с кодом (таблица +выше) и совпадает буквально. Технические решения, не вытекающие однозначно из +существующего контракта (точное значение follower-таймаута, точное имя +command-метода locale-gate, единство/раздельность test harness), явно +вынесены в раздел «Принятые технические предположения» с пометкой «может быть +свободно скорректировано ревьюером» — я эту пометку принимаю без правок: ни +одно из шести предположений не меняет продуктовый контракт и не требует +продуктового решения владельца. + +## Продуктовые вопросы владельцу + +Нет. Ни один AC не требует решения о том, что человек видит или делает — +задача исключительно про hardening существующих контрактов (пункты 1–4, 6) и +про качество тестового свидетеля (пункты 5, 7), без нового UX. Раздел +«Аналитика» в issue уже верно это фиксирует. + +## Находки + +Ни одной блокирующей (High) или требующей возврата (Medium) находки. + +**Low (снимается без правки, с записью).** Формулировка контракта AC4/§4 +«Если root либо отдельная entry исчезла… этот кандидат пропускается» лексически +объединяет два разных случая: исчезновение одной записи между `iterdir()` и +`stat()` (ровно то, что описывает аудит и что покрывает план автотестов п.4) и +исчезновение самого каталога `root` целиком (у которого нет «кандидата» — +пропадать нечему, весь скан просто должен дать `count=0, bytes=0` без +исключения). План автотестов раздела «План автотестов» п.4 явно строит только +первый случай (подмена stat одного кандидата), про второй не говорит. Это не +блокирует: реализация свободна закрыть оба случая одним try/except вокруг +`iterdir()`-цикла, а второй случай на порядок более гипотетичен (нужно, чтобы +директория ассетов исчезла во время активного апдейта HA) и не был частью +исходной находки аудита (г), которая специально про TOCTOU на отдельной +записи. Снимаю без возврата автору — реализация и ревьюер кода в состоянии +решить механику по контексту; если разработчик по факту не покроет +root-vanishing веткой теста, это не расхождение с ТЗ, а вопрос полноты теста +для код-ревью. + +## Что проверено и корректно + +- Все семь пунктов аудита в ТЗ имеют прямое соответствие в коде на текущем + SHA — ни один не является пересказом чужих слов без проверки. +- AC1–AC8 однозначны, у каждого указан способ доказательства; AC1–AC6 + дополнительно снабжены таблицей «чем краснеет» — это избыточно для этапа + spec (обязательно только для код-ревью, §2.7), но облегчает будущий цикл. +- Скоуп/не-скоуп разделены чётко, включая явный отказ трогать API/схему/ + golden/performance-baseline. +- Откат описан как atomic revert с оговоркой про частичный откат по + компонентам — соответствует стилю других ТЗ пакетных полишей. +- Touch-контракт п.2 — прямая цитата `docs/TOUCH-SUPPORT.md`, не изобретение. +- i18n/release-артефакты корректны по факту дерева (все 4 локали существуют, + правок нет; `docs/specs/README.md` обновлён в том же коммите). +- Названные для редактирования тестовые/смок-файлы существуют в дереве. + +## Чего не проверял + +- Не запускал автотесты/гейты — ревью ТЗ не требует прогона (нет кода для + тестирования, реализация ещё не начата). +- Не проверял `custom_components/houseplan/http_api.py` целиком построчно — + только участки, относящиеся к пунктам (а) и (г). +- Не проверял `docs/ARCHITECTURE.md`/`docs/TESTING.md` на точное текущее + содержание тех разделов, которые ТЗ предлагает обновить «если требуют» — + условная формулировка допустима на этапе ТЗ (детали редактирования + документации решает исполнитель). +- Не оценивал реализуемость точного числового значения follower-таймаута — + ТЗ прямо помечает это как техническое предположение, свободное для + корректировки, поэтому численное значение не является предметом ревью ТЗ. + +## Материал раунда + +- SHA ТЗ: `735710f1` (ветка `origin/issue/440-audit-polish`, HEAD на момент + ревью). +- Дерево материала: рабочая копия на `735710f1`, `git status` чист. +- Файл ТЗ: `docs/specs/440-v171-beta2-polish.md` (422 строки, добавлен этим + коммитом вместе с записью в `docs/specs/README.md`). + +--- + + + +## Материал раунда + +- Ветка: `issue/440-audit-polish`, коммит `735710f16b8f` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `927a83664db31061974ea9103581848a0cc8470e` + ``` + git log --all --format='%H %T' | grep 927a83664db3 + ``` +- ТЗ `docs/specs/440-v171-beta2-polish.md`, блоб `0de292e2f2dc734f08a9f3eb4d37d2a6734da3e4` + ``` + git log --all --find-object=0de292e2f2dc734f08a9f3eb4d37d2a6734da3e4 -- docs/specs/440-v171-beta2-polish.md + ```