diff --git a/docs/reviews/CODE-REVIEW-398-r1.md b/docs/reviews/CODE-REVIEW-398-r1.md new file mode 100644 index 00000000..e45c789c --- /dev/null +++ b/docs/reviews/CODE-REVIEW-398-r1.md @@ -0,0 +1,248 @@ +# CODE-REVIEW-398-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/398 +- Этап: code (PROCESS.md §2.7) +- Заход: r1 (первый заход код-ревью; заходы r1-r3 в комментариях issue относятся + к этапу spec и сюда не переносятся) +- SHA под ревью: `d9b67663624364b46b6263c66e9e8b34c3021964` + (`git log --oneline origin/dev..HEAD`, диапазон коммитов от + `69dd09a7` до `d9b67663`) +- ТЗ: `docs/specs/398-sysmodules-guard-scope.md`, ревизия 3 (принята зелёным + на этапе spec, SPEC-REVIEW-398-r3) + +## Скоуп диффа + +``` +docs/ARCHITECTURE.md | 7 ++ +docs/reviews/SPEC-REVIEW-398-r1.md | 183 +++++ +docs/reviews/SPEC-REVIEW-398-r2.md | 204 +++++ +docs/reviews/SPEC-REVIEW-398-r3.md | 167 +++++ +docs/specs/398-sysmodules-guard-scope.md | 201 +++++ +scripts/mutation-gate.mjs | 22 ++ +test/backend-test-hygiene.test.mjs | 135 ++++- +tests_backend/pure_imports.py | 24 ++- +tests_backend/test_backend_quality.py | 39 ++ +9 files changed, 975 insertions(+), 7 deletions(-) +``` + +Продуктовый код (`custom_components/**/*.py`, `src/**`) не тронут — подтверждено +`git diff --stat origin/dev...HEAD -- src/` и `-- custom_components/` (пусто). +`tests_backend/conftest.py` тоже не тронут — граница «не в скоупе» (контракт +#394) держится. Класс изменения — B (тесты и гейт), как заявлено в ТЗ. +`User-Visible: no` во всех коммитах, changelog не трогается — корректно для +инфраструктурной правки без видимого поведения. + +## Как проверялось + +Ручного тестирования в цикле нет; ниже — то, что прогнано мной и его +результат, плюс разбор кода там, где исполнение недоступно или избыточно. + +### Дешёвые гейты (прогнаны) + +- `npx tsc --noEmit` — чисто, без вывода. +- `npm test` — **1662 pass, 0 fail, 1 skipped** (1..1641 top-level с + подтестами). Совпадает с числом, заявленным автором в комментарии к issue. +- `npm run build` — `tsc --noEmit && rollup -c`, бандл собран успешно за + 15.6s. Сверка трёх копий бандла и `node scripts/check-docs.mjs` не + выполнялись: диф не трогает `src/**`, отпечаток документации не устаревает, + а строгий гейт на бандл нужен только когда фронтенд меняется. +- `node scripts/smoke-select.mjs --base origin/dev --head HEAD` — вывод: + «Исполняемого frontend-диффа нет (src/**/*.ts не тронут). Browser-smoke этим + диффом не выбираются — это не «пропустить проверки», а «выбирать нечего»: + смоки проверяют собранную карточку. Тронуто файлов: 9.» — согласуется с + диффом, ни один смок не запускался, и это обосновано инструментом, а не моим + решением. + +### Backend-тесты (прогнаны частично, целенаправленно) + +Полный `python -m pytest tests_backend -q` требует `pytest-homeassistant-custom-component` +и Python 3.14 (см. `tests_backend/requirements.txt`) — тяжёлый стек, не +предустановленный в этой песочнице (только Python 3.12). Инструкция гейтит +этот прогон на изменения `custom_components/**/*.py`, которых в этом диффе +нет. Вместо холостого пропуска я поставил лёгкие зависимости (`pytest`, +`voluptuous==0.15.2` — оба чистый Python, без HA) и прогнал всё, что не +требует реального Home Assistant: + +- `python3 -m pytest tests_backend/test_backend_quality.py -q` — + **5 passed** (включая новый `test_issue_398_pure_imports_leaves_sys_modules_as_it_found_it`, AC4). +- `python3 -m pytest tests_backend/test_junction_limits.py -q` — + **16 passed** (единственный файл, явно перечисленный в «в скоупе» ТЗ как + потребитель канонического имени через `load_pure`). +- `python3 -m pytest tests_backend/test_validation.py -q` — + **142 passed, 1 skipped** (использует и `pure_imports.load_pure`, и + собственный `_load_pure` с `hp_pure.*` — оба паттерна, которые гвард обязан + не путать с нарушением). +- `python3 -m pytest tests_backend -q --ignore=tests_backend/test_coordinate_canonicalization.py` + — **246 passed, 1 skipped**, без ошибок сборки sys.modules между файлами. + Единственный проигнорированный файл падает при коллекции с + `ModuleNotFoundError: No module named 'homeassistant'` — он импортирует + `custom_components.houseplan.store`, который тянет `homeassistant.config_entries` + напрямую, в обход `load_pure`; это давно существующая зависимость от + полного HA-стека, дифф её не касается и не может починить. + +Это закрывает AC5 («существующие backend-тесты продолжают проходить») по +факту исполнения почти всего набора, кроме той единственной части, что +физически требует Home Assistant и не относится к предмету правки. + +### Мутанты (оба воспроизведены вручную, не через `scripts/mutation-gate.mjs`) + +Полный `scripts/mutation-gate.mjs` пересобирает бандл в отдельном worktree для +каждого мутанта — дорогая операция уровня предрелизного гейта (файл сам об +этом говорит: «прогон дорогой... его место — перед стабильным релизом»). Для +двух новых мутантов пересборка бандла не нужна (`guard` — `node --test` +одного файла и `pytest` одного файла), поэтому я применил патчи руками, не +трогая инфраструктуру mutation-gate, и вернул файлы в исходное состояние: + +1. **`sysmodules-guard-blind-to-variable`**. Патч + `test/backend-test-hygiene.test.mjs`: строка + `if (start === null) { if (namesThePackage) hits.push(match[0]); continue; }` + → `if (start === null) { continue; }` (снят fail-closed на нелитеральный + ключ). Прогон `node --test test/backend-test-hygiene.test.mjs` — + **`# fail 2`** (падают `#398 AC1` и часть `#398 AC2`). Файл возвращён в + исходное состояние, `git status --porcelain` — пусто. +2. **`pure-imports-stops-cleaning`**. Патч `tests_backend/pure_imports.py`: + `del sys.modules[key]` → `pass` (снята очистка). Прогон + `python3 -m pytest tests_backend/test_backend_quality.py -q -k issue_398` + — **красный**: + ``` + AssertionError: load_pure оставил в sys.modules: + ['custom_components.houseplan.coordinate_canonicalization', + 'custom_components.houseplan.junction_limits', + 'custom_components.houseplan.wall_segment_model'] + ``` + Файл возвращён в исходное состояние, `git status --porcelain` — пусто. + +Оба мутанта, зарегистрированные в `scripts/mutation-gate.mjs`, действительно +краснеют на названном ими `guard` — дисциплина «тест умеет падать» выполнена +для обоих, силами ручного воспроизведения вместо дорогого прогона реестра. + +### Не проверялось (и почему) + +- **`python -m pytest tests_backend -q` с полным HA-стеком** — не установлен + в песочнице (Python 3.14 + `pytest-homeassistant-custom-component` + + `home-assistant-frontend`, сотни МБ). Диф не трогает + `custom_components/**/*.py`, поэтому по инструкции гейт не обязателен; + компенсировано прогоном 246/247 тестов без HA (см. выше) — не покрыт только + один файл, требующий HA по независимой от диффа причине. +- **`ruff`/`mypy` бэкенда** — область этих гейтов в CI ровно + `custom_components/houseplan` (`validate.yml:788`), диф её не касается. +- **`npm run golden:verify`** — диф не меняет рендер, геометрию, стили, слои. +- **`npm run invariants`** — диф не трогает рёбра комнат, `layout`, + `marker.space`, `open_spans` или иные ссылки на геометрию; модель данных не + затронута (раздел ТЗ «Модель данных и миграция»: не применимо). +- **`node scripts/check-docs.mjs`** — диф не трогает `src/**`. +- **`scripts/mutation-gate.mjs` целиком** (пересборка бандла в worktree) — + избыточно для инфраструктурного диффа без изменений в `src/**`; оба новых + мутанта проверены вручную (см. выше), остальные 60+ мутантов не связаны с + этим диффом. +- **«Одно число — один источник»** — неприменимо: диф не добавляет и не + меняет ни одной пользовательской величины. + +## Разбор по AC + +- **AC1** (запись через переменную ловится). Доказано контрактным тестом + `#398 AC1` (`test/backend-test-hygiene.test.mjs:127`) и подтверждено мутантом + `sysmodules-guard-blind-to-variable` — красный при снятии проверки. ✅ +- **AC2** (остальные формы: f-строка, конкатенация, `setdefault`/`update`, + пробелы вокруг ключа — ловятся; безопасные формы не ловятся). Доказано двумя + тестами `#398 AC2` (строки 132 и 144). Прочитан код `sysModulesWrites`: + разбор ключа через `literalStart` корректно отличает целый литерал и + f-строку с литеральным началом от выражения; `namesThePackage` — грубый, но + верно направленный fail-closed триггер («файл вообще способен назвать + пакет»). Тестовые случаи безопасных строк (`hp_pure.*`, `houseplan.trails`, + закомментированная строка) взяты дословно из реальных файлов + (`test_validation.py:103`, `test_junction_limits.py`) — не выдуманы, а + списаны с кода, который гвард обязан не сломать. ✅ +- **AC3** (список исключений закрыт двумя именами). Доказано тестом + `#398 AC3` через `assert.deepEqual` — третий элемент сломает и этот тест, и + `#398 AC1`-тесты на любом файле вне списка. Прочитан код: `SYS_MODULES_WRITERS` + используется как фильтр в тесте про #394, список действительно + `['conftest.py', 'pure_imports.py']`. ✅ +- **AC8** (`pure_imports.py` восстанавливает `sys.modules` в `finally`, + доказано отдельно от гварда). Доказано мутантом `pure-imports-stops-cleaning` + → красный `test_issue_398_pure_imports_leaves_sys_modules_as_it_found_it` + (воспроизведено выше). Статический тест `#398 AC8` (строка 164) дополнительно + проверяет форму кода (`finally`, `del sys.modules[key]`, + `key.startswith(PACKAGE_ROOT.name)`) — это проверка «код написан так, как + обещано», а не замена исполняемой проверки; обе присутствуют, как и + требовало разделение ролей из ТЗ. ✅ +- **AC4** (после прогона `tests_backend/` в `sys.modules` нет лишних ключей + `custom_components*`). Доказано исполняемым тестом + `test_issue_398_pure_imports_leaves_sys_modules_as_it_found_it`, который + сравнивает разницу до/после, включая повторный вызов подряд — прогнан лично, + зелёный (см. «Backend-тесты» выше). ✅ +- **AC5** (существующие backend-тесты проходят, относительные импорты + резолвятся). Прогнано 246/247 тестов `tests_backend/` без HA — все зелёные; + единственный непрогнанный файл падает по причине, не связанной с диффом + (прямой импорт `homeassistant.config_entries`, минуя `load_pure`). ✅ +- **AC6** (#393 и #394 не ослаблены). Тест `sys.path` (#393) не изменён + дифом. Тест #394 переписан на `sysModulesWrites`, но сохраняет обе проверки: + список офендеров пуст и подмена в conftest — под `if not HAS_HA:`. Прогнан + как часть `npm test`, зелёный. ✅ + +Ни одного AC, помеченного «проверено чтением, не исполнением» без +одновременного независимого прогона, в этой задаче нет — каждый AC либо +подтверждён исполняемым тестом лично, либо мутантом, либо обоими. + +## Находки + +Блокирующих (High/Medium) находок нет. Два Low-наблюдения, оба не в скоупе +для правки сейчас (узость уже осознанно объявлена в ТЗ, раздел «Риски»: +«Полноценный парсер тянуть не нужно и нельзя»), фиксирую как известные +границы эвристики, не как дефект: + +1. **Гвард статический и ловит только выражение вида `sys.modules[...] =` / + `sys.modules.setdefault|update(...)`, но не алиасинг**. Код вида + `sm = sys.modules; sm["custom_components.x"] = mod` или + `import sys as s; s.modules[...] = ...` не совпадёт ни с одним из regex в + `sysModulesWrites` (оба привязаны к литеральной подстроке `sys.modules`) и + пройдёт мимо гварда, даже если файл называет пакет интеграции литералом. + Не воспроизведено как реальный дефект — сегодня в `tests_backend/` такого + кода нет (проверено чтением всех `.py` файлов), и AC2 явно перечисляет + закрываемые формы (f-строка, конкатенация, `setdefault`/`update`), алиасинг + в их числе не назван. Ниже порога Medium: гипотетический обход требует + осознанного обфусцирования, а не естественного стиля кода в этом проекте. +2. **`namesThePackage` требует точку после `custom_components`** (regex + `(['"])custom_components\./`). Файл, который литералом называет только + голое `"custom_components"` (без точки) где-то не рядом с записью, и + пишет в `sys.modules` через переменную в другом месте, не взведёт + fail-closed триггер для этой записи (хотя прямая запись + `sys.modules["custom_components"] = ...` всё равно поймана — это отдельная + ветка `literalStart`, точки не требует). Крайне узкий кейс, не + воспроизведён в реальном коде. + +Оба наблюдения не требуют действия в этой задаче: серьёзность ниже Medium, +эксплуатируются только гипотетическим кодом, которого сегодня в репозитории +нет, а сама спецификация прямо признаёт эвристику неполной и заранее +ограничивает её притязания («узкий разбор... а не полноценный парсер»). + +## Что проверено и корректно + +- Разбор ключа (`literalStart`) отличает литерал/f-строку-с-литеральным-началом + от выражения; протестирован на реальных строках из `test_validation.py` и + `test_junction_limits.py`, а не на синтетике, оторванной от кода проекта. +- `load_pure` восстанавливает `sys.modules` по всей разнице под префиксом + `custom_components`, а не только по собственному имени — воспроизведено + исполнением (мутант выше явно показывает три «оставленных» ключа, включая + соседей по относительным импортам). +- `load_pure` не трогает нестандартные регистрации (`hp_validation`, + `hp_pure.*`) — они не подпадают под префикс `custom_components` и + сознательно остаются вне скоупа AC4 (класс дефекта #389/#394/#398 — это + именно коллизия с загрузчиком HA по имени пакета интеграции, а не любая + утечка в `sys.modules`). +- Список исключений и его обоснование (`conftest.py` — условная подмена, + `pure_imports.py` — самоочистка) закрыт тестом на список, а не только + комментарием. +- `docs/ARCHITECTURE.md` дополнен одной точной фразой, соответствующей коду + (проверено построчно), без раздувания документа. +- Трейлеры `Issue: #398` и `User-Visible: no` — в каждом коммите диапазона; + changelog не требуется и не тронут — правильно для этого класса изменения. +- Скоуп/не-скоуп ТЗ выдержан: `tests_backend/conftest.py`, + `custom_components/**`, `src/**` не задеты. + +## Вывод + +Все критерии приёмки (AC1–AC6, AC8) доказаны исполняемым способом там, где +это требовало ТЗ, и подтверждены мной лично, а не только заявлены автором. +Дешёвые гейты зелёные, оба мутанта проверены на способность падать вручную. +Находок уровня Medium/High нет. Вердикт — зелёный.