docs: review document for #398

Issue: #398
User-Visible: no

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 нет. Вердикт — зелёный.