From a4b686a7fe3ad8728d2cb99e05e4a0083e91f779 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Mon, 31 Aug 2026 00:26:38 +0000 Subject: [PATCH] docs: review document for #398 Issue: #398 User-Visible: no --- docs/reviews/SPEC-REVIEW-398-r1.md | 183 +++++++++++++++++++++++++++++ 1 file changed, 183 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-398-r1.md diff --git a/docs/reviews/SPEC-REVIEW-398-r1.md b/docs/reviews/SPEC-REVIEW-398-r1.md new file mode 100644 index 00000000..df26bd3c --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-398-r1.md @@ -0,0 +1,183 @@ +# SPEC-REVIEW-398-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/398 +- Этап: ТЗ на ревью (PROCESS.md §2.4) +- Материал: `docs/specs/398-sysmodules-guard-scope.md`, коммит `69dd09a7` (тело + issue #398 без изменений) +- Заход: r1 · блокирующих циклов израсходовано до этого вердикта: 0 из 4 +- Комментариев в issue к моменту ревью: 0 (первый заход, полный разбор) + +## Скоуп проверки + +Issue #398 просит расширить гвард `test/backend-test-hygiene.test.mjs`, +который сегодня ловит запись стаба интеграции в `sys.modules` только по +строковому литералу и поэтому не видит `tests_backend/pure_imports.py:31` +(`sys.modules[name] = module`, запись через переменную). Изменение полностью +внутри класса B (`test/**`, `tests_backend/**`), не трогает `src/**` и +`custom_components/**/*.py`, `User-Visible: no`. ТЗ оформлено полным треком — +`docs/specs/398-sysmodules-guard-scope.md`, файл на месте, ссылка issue↔ТЗ +двусторонняя. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком. +2. Прочитано тело issue #398 и лента меток (`gh api .../timeline`) — задача + ушла `S1-new → S3-spec → S4-spec-review` без публичного комментария + аналитики (см. «Наблюдения», п.2). +3. Прочитан ТЗ `docs/specs/398-sysmodules-guard-scope.md` целиком. +4. Сверены с реальным кодом все цитаты из ТЗ: `test/backend-test-hygiene.test.mjs` + (регэксп на строке 39), `tests_backend/pure_imports.py` (строка 31), + `tests_backend/conftest.py` (условная подмена под `if not HAS_HA:`), + `tests_backend/test_junction_limits.py` (безусловный вызов `load_pure` с + каноническим именем `custom_components.houseplan.junction_limits`), + `tests_backend/test_validation.py` (собственный `_load_pure`, пишет в + `sys.modules` под префиксом `hp_pure.*`, вне описанного скоупа гварда). +5. Канонические документы подсистем (`SUN.md`, `LIGHT.md`, `CANVAS.md`, + `WALL-THICKNESS.md`, `UX-MODES.md`, `CONFIG-COMPATIBILITY.md`, + `TOUCH-SUPPORT.md`) — не применимы, задача не меняет видимое поведение + продукта. +6. `docs/USER-GUIDE.ru.md` — не применим, `User-Visible: no`, персона — + разработчик, а не пользователь продукта. +7. Гейты кода не гонялись: на этапе ревью ТЗ продуктового/тестового кода ещё + нет, есть только новый markdown-файл — `typecheck`/`test`/`build` к нему + неприменимы. + +## Находки + +### Medium (в скоупе задачи) — AC3 разрешает «путь 2», AC4 делает его невозможным + +`docs/specs/398-sysmodules-guard-scope.md:73-84` называет два одинаково +допустимых исхода для `load_pure`: + +1. снимать регистрацию за собой после `exec_module` (самоочистка); +2. объявить `pure_imports.py` вторым легальным исключением рядом с + `conftest.py`, с тестом, фиксирующим список ровно из двух файлов. + +Текст явно оставляет выбор реализации: «Предпочтителен (1)… — но завтра +станет прецедентом» (строка 83) описывает предпочтение, не запрет второго +пути, и **AC3** (строки 117-119) прямо предусматривает путь 2: «единственное +исключение (**либо два файла, если выбран путь 2** — тогда список +зафиксирован тестом и его рост краснеет)». + +Но **AC4** (строки 120-122) требует: «После прогона всего `tests_backend/` в +`sys.modules` нет ключей `custom_components*` **сверх тех, что положил +conftest**». Это не совместимо с путём 2 по конструкции: если `pure_imports.py` +объявлен вторым легальным исключением и не убирает за собой запись, то после +прогона `tests_backend/` в `sys.modules` останутся ключи +`custom_components.houseplan.junction_limits`, +`custom_components.houseplan.wall_segment_model`, +`custom_components.houseplan.coordinate_canonicalization` — ровно то, что +issue проверил исполнением как текущее наблюдаемое поведение — и они не +«положены conftest», а положены вторым разрешённым файлом. AC4 в буквальной +формулировке тогда красный при полностью корректной, явно допустимой +реализации. + +**Почему это находка ТЗ, а не мелочь для код-ревью.** DoR требует +«пронумерованные проверяемые критерии приёмки» — набор AC должен быть +одновременно выполним. Сейчас AC3 и AC4 совместно выполнимы только при выборе +пути (1); реализатор, прочитавший ТЗ и выбравший путь (2) как более простой +(сам документ признаёт его допустимым и даже кладёт под него отдельный +тест-пункт «план автотестов», п.4), гарантированно упрётся в красный AC4 при +исполняемой проверке — и потратит цикл код-ревью на то, что было видно уже на +этапе ТЗ. + +**Как править (для автора, не предписание):** либо явно исключить путь 2 из +допустимых исходов (тогда «выбор — за реализацией» в контракте лишний — есть +только путь 1), либо переформулировать AC4 так, чтобы он допускал оба исхода: +«…нет ключей `custom_components*` сверх положенных `conftest.py` **и**, если +выбран путь 2, кроме предъявленных явным списком исключений в +`pure_imports.py`». + +**Воспроизведение** (не гипотеза — уже исполнено issue-автором и +переподтверждено при разборе): `cd tests_backend && python3 -c "import +conftest, test_junction_limits, sys; print([k for k in sys.modules if +k.startswith('custom_components')])"` печатает три ключа +`junction_limits`/`wall_segment_model`/`coordinate_canonicalization` сверх +пустышек `custom_components`/`custom_components.houseplan`, поставленных +`conftest.py`. При выборе пути 2 эти три ключа остаются легальными по AC3, но +проваливают AC4 буквально. + +## Наблюдения (не блокируют, не находки к ТЗ) + +1. **Контракт vs риски о способе разбора.** Раздел «Проблема и контракт» + говорит «Форма проверки меняется с регулярного выражения на разбор + синтаксиса Python» (что совпадает с просьбой issue: «разбор AST вместо + регулярки»), а раздел «Риски» тут же ограничивает: «Полноценный парсер + тянуть не нужно и нельзя: достаточно узкого разбора строк… с fail-closed на + всё непонятое». Формулировки не запрещают друг друга буквально (узкий + разбор конкретно конструкции присваивания — тоже «разбор синтаксиса», не + обязательно generic-грамматика), но провисает связка: если ключевая + гарантия ТЗ — «отказ на всё недоказуемое» — реализована самим fail-closed + по умолчанию, то выбор между вызовом Python `ast` через subprocess (0 новых + зависимостей, репозиторий и так тянет Python 3.13) и узким текстовым + разбором в Node не влияет на корректность настолько, насколько намекает + контраст формулировок. Технический вопрос — не продуктовый, решается + автором/ревьюером по существу, не эскалируется владельцу; не блокирую, + но рекомендую снять формулировочное противоречие в следующей редакции. +2. **Маршрутизация issue мимо инфраструктурного пути.** Диапазон правки — + исключительно класс B (`test/**`, `tests_backend/**`), ни одного файла + класса A. По `AGENTS.md` («Infrastructure-only work runs outside this + flow… The test… is mechanical: not a single class A file») и + `PROCESS.md` §1 такая задача идёт «без ТЗ, ревью ТЗ, код-ревью и без + прохода по статусам». Лента меток issue (`gh api .../timeline`) показывает, + что все переходы `S1-new → S3-spec → S4-spec-review` проставил лично + владелец (`Matysh`) без публичного комментария аналитики S2 — то есть это + не сбой автоматики, а его собственное решение вести задачу полным треком. + Это не дефект ТЗ и не встаёт в оценку документа: владелец вправе выбрать + больше строгости для третьего по счёту случая одного класса дефектов. + Фиксирую как наблюдение для гигиены процесса, не как находку ревью. + +## Что проверено и корректно + +- Оба обязательных продуктовых раздела на месте: «Сценарий» называет персону + верно (разработчик, добавляющий backend-тест — не пользователь продукта по + `docs/SCOPE.md`, что для чисто инфраструктурной задачи ожидаемо) и + поверхность (backend test harness, `tests_backend/`); «Что человек увидит до + и после» отвечает без терминов реализации. +- Все обязательные разделы §7.1 присутствуют: проблема, скоуп/не-скоуп, + контракт, UX (Н/П), модель данных и миграция (Н/П), i18n (Н/П), AC1…AC6 с + указанием доказательства, план автотестов, риски, откат, release-артефакты. +- Цитаты кода в ТЗ (регэксп `test/backend-test-hygiene.test.mjs:39`, запись + `pure_imports.py:31`) дословно совпадают с текущим содержимым файлов — не + придуманы задним числом. +- AC1, AC2, AC5, AC6 однозначны и проверяемы: у каждого назван способ + доказательства (тест-контракт по форме записи, backend-прогон, мутант). + AC2 корректно перечисляет конкретные проверяемые формы (f-строка, + конкатенация, `setdefault`/`update`) вместо общего «и другие похожие». +- Скоуп/не-скоуп разграничены точно: `tests_backend/conftest.py` (контракт + #394) и правило про `sys.path` (#393) явно исключены из скоупа этой задачи — + граница, названная в issue #394, не ослабляется этим ТЗ. +- `docs/SCOPE.md` не нарушается: задача не претендует ни на одну строку Core + user jobs, это ожидаемо для чисто инфраструктурного изменения, и ТЗ не + делает вид, что решает продуктовую задачу. +- Риски названы по существу (разбор Python из Node, снятие регистрации ломает + относительные импорты, скрытая зависимость от повторного импорта) со + смягчениями, а не общими словами. +- Откат дешёвый и конкретный: «два коммита назад, продуктовый код не + затронут» — соответствует факту (диапазон правки — исключительно class B). +- Ни одна догадка о поведении не выдана за факт: утверждения о текущем + дефекте («переживает свой тест», перечень трёх модулей) сопровождаются + пометкой «проверено исполнением» и совпадают с тем, что показал сам issue. + +## Чего не проверял + +- Не гонял `npx tsc --noEmit` / `npm test` / `npm run build` — на этапе + ревью ТЗ нет ни строки продуктового или тестового кода, есть только новый + `docs/specs/*.md`; эти гейты неприменимы к чтению документа. +- Не проверял технической реализуемости «разбора синтаксиса Python из Node» + экспериментом (не писал прототип) — вопрос вынесен в наблюдение 1 как + формулировочный, а не как проверенный тупик. +- Не проверял `tests_backend/test_validation.py` построчно за пределами + найденного вызова `_load_pure`/`sys.modules[f"hp_pure...`] — этого было + достаточно, чтобы подтвердить корректность утверждения ТЗ «при + необходимости» про этот файл (префикс `hp_pure` вне заявленного скоупа + гварда `custom_components*`); полный разбор файла оставляю код-ревью, если + реализация всё же тронет этот файл. + +## Вердикт + +Жёлтый. Единственная блокирующая находка (Medium, в скоупе задачи) — +несовместимость AC3 и AC4 для явно допущенного документом «пути 2»: описанный +как разрешённый исход не может пройти собственный исполняемый критерий +приёмки. Правится в тексте ТЗ этим же автором, без выхода за рамки текущей +задачи.