From 692e3b72f485e38492c2cab6658de736a17b42ab Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Mon, 31 Aug 2026 00:38:50 +0000 Subject: [PATCH] docs: review document for #398 Issue: #398 User-Visible: no --- docs/reviews/SPEC-REVIEW-398-r2.md | 204 +++++++++++++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-398-r2.md diff --git a/docs/reviews/SPEC-REVIEW-398-r2.md b/docs/reviews/SPEC-REVIEW-398-r2.md new file mode 100644 index 00000000..285c34b8 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-398-r2.md @@ -0,0 +1,204 @@ +# SPEC-REVIEW-398-r2 + +- Issue: https://github.com/Matysh/houseplan-card/issues/398 +- Этап: ТЗ на ревью (PROCESS.md §2.4) +- Материал: `docs/specs/398-sysmodules-guard-scope.md`, коммит `480d202f` + («#398 spec revision 2 per SPEC-REVIEW-398-r1») +- Заход: r2 · блокирующих циклов израсходовано до этого вердикта: 1 из 4 +- Предыдущий вердикт: `docs/reviews/SPEC-REVIEW-398-r1.md`, жёлтый, на коммите + `69dd09a7` (ревизия 1 ТЗ). SHA в вердикте r1 не назван явно текстом — найден + сопоставлением: `git show a4b686a7 --stat` (коммит с самим документом r1) + датирован позже `69dd09a7` и раньше `480d202f`, а «Материал» в r1 называет + файл без ревизии — правки после которой ещё не было, то есть r1 читал ровно + содержимое `69dd09a7`. + +## Разбор по дельте (PROCESS.md §2.9, #214) + +Дельта — `git diff 69dd09a7..480d202f -- docs/specs/398-sysmodules-guard-scope.md` +(28 добавленных / 22 удалённых строк). Правка не переезжает на другую подсистему, +не меняет продуктовый контракт (задача остаётся `User-Visible: no`, класс B) и по +объёму меньше исходной задачи — полный повторный разбор не требуется. Проверены +только: (а) закрытие находки r1 по цитате нового текста и (б) те AC, чьё +доказательство дельта задевает — это AC3, AC4 и, как показано ниже, обнаружилось, +что дельта задевает ещё и совместимость AC1/AC2 с AC3 через код `pure_imports.py`, +явно оставленный «в скоупе» правки. Разделы, которых дельта не касалась (Сценарий, +UX, i18n, AC5, AC6, откат, release-артефакты), унаследованы из r1 — см. раздел +ниже. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| Medium: AC3 явно допускал «путь 2» (объявить `pure_imports.py` вторым легальным исключением), а AC4 требовал отсутствия лишних ключей после прогона — путь 2 конструктивно не мог пройти AC4 | «Путь 2» полностью удалён из текста; раздел «Что делать с `load_pure`» переписан как «один путь: снимать за собой», с явной ссылкой на находку ревьюера | `docs/specs/398-sysmodules-guard-scope.md:71-87` («Ревизия 1 допускала второй исход… вариант снят»); AC3 (строки 120-121) больше не содержит альтернативы «либо два файла»; план автотестов, п.4 (строка 139) убрал условность «если выбран путь 2» | + +Текстовое закрытие корректно и однозначно: альтернативный путь убран, а не +спрятан («не рекомендуется», «как правило» и т.п. отсутствуют — использована +формулировка «вариант снят»). + +## Новая находка r2 + +### Medium (в скоупе задачи) — устранение «пути 2» открывает новый конфликт: AC1/AC2 не совместимы с AC3 для `tests_backend/pure_imports.py`, оставленного «в скоупе» + +Ревизия 2 фиксирует ровно одно исключение из-под гварда — +`tests_backend/conftest.py` (строка 87: «Единственное исключение остаётся одно»; +AC3, строки 120-121: «остаётся единственным исключением»; план автотестов, +п.4, строка 139: «попытка добавить **второй** файл в список исключений → +красный» — то есть механизм исключения зафиксирован именно как список файлов +мощности 1, а не как распознавание безопасного паттерна «запись + восстановление»). + +Одновременно раздел «Скоуп/не-скоуп» (строка 91) оставляет +`tests_backend/pure_imports.py` в скоупе как файл, который правка изменяет (а не +удаляет), и требует (через «Что делать с `load_pure`», строки 78-87, и AC5, +строки 125-127), чтобы после правки `load_pure` продолжал резолвить +относительные импорты для существующих вызовов — в частности, +`tests_backend/test_junction_limits.py:28-30`, который передаёт канонический +литерал `"custom_components.houseplan.junction_limits"` в качестве аргумента: + +```python +sys.modules[name] = module # tests_backend/pure_imports.py:31 +``` + +Это ровно образец, который AC1 называет напрямую: «Файл, записывающий +`sys.modules[<переменная>] = …`, где переменная может содержать имя пакета +интеграции, отклоняется гвардом» (строки 112-114). Значение `name` внутри +`pure_imports.py` — параметр функции, из текста самого файла статически не +определимо, что оно не начинается с `custom_components`; собственный +fail-closed принцип ТЗ («значение ключа может быть неизвестно статически — +тогда гвард обязан отказывать», строки 67-69) требует отклонить именно этот +файл. Проверка в `test/backend-test-hygiene.test.mjs` статическая, идёт +файл-за-файлом без исполнения (комментарий в коде, строки 11-14: «Проверка +статическая — по исходникам, без запуска») — она не может увидеть, что +`load_pure` восстанавливает `sys.modules` после `exec_module`; восстановление +— рантайм-факт, а гвард текст не исполняет. + +Итог: `pure_imports.py`, оставленный в скоупе и обязанный по AC5 продолжать +работать для `test_junction_limits.py`, **структурно не может** одновременно (а) +не быть исключением (по AC3, ровно один файл-исключение — `conftest.py`) и (б) +избежать отклонения по AC1 (поскольку пишет в `sys.modules` по переменной, +которая в вызовах из скоупа действительно принимает значения вида +`custom_components.houseplan.*`). Здесь нет обходного варианта на уровне формы +записи: регистрация модуля в `sys.modules` под точным каноническим именем — +обязательное условие резолва относительных импортов в Python +(`importlib.util.module_from_spec` + `exec_module`), других способов сделать +`from .wall_segment_model import …` рабочим для этого файла ТЗ не описывает и +предложить не может без изменения самого механизма загрузки. + +**Почему это находка ТЗ, а не код-ревью.** Это тот же класс дефекта, что и +закрытая находка r1: набор AC совместно невыполним для сценария, который сам +документ называет обязательным (пункт «в скоупе» + AC5). Разработчик, +реализующий ТЗ буквально — упрощённая по AC1/AC2 регулярка/AST-проверка плюс +очистка `sys.modules` в `load_pure` — получит перманентно красный +`test/backend-test-hygiene.test.mjs` на собственном `pure_imports.py`, то есть +красный `npm test` сразу после реализации, и упрётся в противоречие, которое +было видно уже на этапе чтения ТЗ. + +**Как править (для автора, не предписание).** Разрешить `pure_imports.py` +вторым явным исключением в AC3 (список из двух конкретных, поимённо +зафиксированных файлов — `conftest.py` и `pure_imports.py`, рост списка сверх +этого по-прежнему красный) — это НЕ реинкарнация снятого «пути 2»: там +исключение освобождало от очистки и потому ломало AC4; здесь очистка (уже +специфицированная в ревизии 2 как «снимать всё, что появилось под префиксом +`custom_components` за время `exec_module`») остаётся обязательной сама по +себе и закрывает AC4 независимо от статуса файла в списке исключений +статического гварда. Иными словами: исключение из AC1-проверки (статический +факт «этому файлу разрешено писать») и обязанность очистки по AC4 +(рантайм-факт «после прогона лишнего не осталось») — два независимых +требования, и revision 2 верно оставила второе, но по инерции убрала и первое, +хотя первое было не источником проблемы. Альтернатива — доказать в тексте, что +`name` в `pure_imports.py` статически ограничен паттерном, не совпадающим с +`custom_components*` (не просматривается, как это сделать без потери гибкости +вызова с каноническим именем). + +**Воспроизведение** (разбор по коду, не исполнено как эксперимент — гварда, +описанного ТЗ, ещё не существует в коде): применить контракт AC1 к +`tests_backend/pure_imports.py:31` буквально даёт «отклонить», а вызов из +`tests_backend/test_junction_limits.py:28` требует, чтобы файл продолжал +работать. Оба требования одновременно предъявлены этой же ревизией ТЗ (AC1 + +скоуп + AC5). + +## Унаследовано из r1 + +Без повторной проверки в этом раунде — дельта не касалась этих участков текста +и они не входят в цепочку доказательства новой находки: + +- Обязательные продуктовые разделы («Сценарий», «Что человек увидит до и + после») и корректность персоны — `docs/reviews/SPEC-REVIEW-398-r1.md`, + раздел «Что проверено и корректно», п.1, на SHA `69dd09a7` (текст этих + разделов дельтой не тронут). + Как проверить самостоятельно, что раздел не менялся: сравнить + `git diff 69dd09a7..480d202f -- docs/specs/398-sysmodules-guard-scope.md` + — «Сценарий» и «Что человек увидит» отсутствуют среди затронутых hunks. +- Полнота обязательных разделов §7.1 (проблема, скоуп/не-скоуп, UX Н/П, модель + данных и миграция Н/П, i18n Н/П, план автотестов, откат, release-артефакты) + — там же, п.2. Структура не менялась, менялось только содержимое трёх блоков + (проблема/контракт, AC3, план автотестов п.4), уже разобранных выше. +- Соответствие `docs/SCOPE.md`: задача не претендует на строку Core user jobs, + чисто инфраструктурная — там же, п.5. Не переоценивалось: дельта не меняет + ни продуктовую рамку, ни `User-Visible`. +- AC2, AC5, AC6 — однозначны и проверяемы каждый по отдельности — там же, п.4 + (частично; см. ниже уточнение по AC1). Текст этих критериев дельтой не + тронут (см. диф выше — правки только в AC3 и в тексте перед AC-блоком). + Уточнение к унаследованному: r1 также назвал AC1 «однозначным» — это верно + в изоляции (формулировка ясна), но r1 не проверял совместность AC1 с AC3 и + скоупом; это ровно то, что находка r2 закрывает как пробел прошлого раунда, + а не противоречие ему. +- Риски «разбор Python из Node» и «скрытая зависимость от повторного импорта» + — назывались по существу уже в r1, текст не менялся (третий риск, «снятие + регистрации ломает относительные импорты», получил только уточнение + «замер: модуль полностью рабочий после очистки» — редакционное, не меняет + сути) — там же, п.6. Новый риск ревизии 2 («соседи, подтянутые + относительными импортами») проверен заново в этом раунде, см. «Что + проверено и корректно» ниже. +- Откат и release-артефакты — не изменены дельтой, наследуются из п.7-8 того + же документа. +- Наблюдение 1 (напряжение формулировок «контракт vs риски» про способ + разбора) и наблюдение 2 (маршрутизация issue мимо инфраструктурного пути) — + не блокировали в r1 и не затронуты дельтой; остаются наблюдениями, не + находками. + +## Что проверено и корректно (в этом раунде) + +- Закрытие находки r1 текстуально полное: «путь 2» не просто ослаблен + формулировкой, а вычеркнут вместе со всеми его следами в AC3 и плане + автотестов (сверено построчно по диффу выше). +- Новый риск «Соседи, подтянутые относительными импортами» (строки 163-167) + назван конкретно, с перечислением реальных имён модулей + (`wall_segment_model`, `coordinate_canonicalization`) и с механизмом + смягчения (диф по префиксу вокруг `exec_module`, а не снятие одного имени) — + проверено сопоставлением с фактическим поведением, описанным в разделе + «Проблема и контракт» (те же три модуля названы там как реально + оставшиеся после `import test_junction_limits`). +- `docs/SCOPE.md` по-прежнему не нарушается: правка не расширяет заявленную + поверхность, остаётся полностью инфраструктурной. + +## Чего не проверял + +- Не гонял `npx tsc --noEmit` / `npm test` / `npm run build` — на этапе ревью + ТЗ нет продуктового или тестового кода, есть только правка `docs/specs/*.md`; + эти гейты неприменимы. (То же основание, что и в r1.) +- Не проверял эксперимента «разбор синтаксиса Python из Node» — вопрос остаётся + открытым наблюдением из r1, не заново поднятым в этом раунде. +- Не перечитывал `tests_backend/test_validation.py` целиком повторно — его + собственный `_load_pure` использует префикс `hp_pure.*`, статически не + совпадающий с `custom_components*` (литеральная часть f-строки/конкатенации + проверяема по AC2), и это уже подтверждено в r1; для новой находки этот файл + не релевантен, она про `pure_imports.py`. +- Не проверял, можно ли реализовать распознавание паттерна «запись сразу с + последующим восстановлением» как альтернативу пофайловому списку исключений + — это один из вариантов правки, adресованных автору, а не факт, который + нужно доказывать ревьюеру; сам документ фиксирует именно файловый список + (AC3 + план автотестов п.4), и находка оценивает именно этот, явно выбранный + автором механизм. + +## Вердикт + +Жёлтый. Единственная блокирующая находка (Medium, в скоупе задачи) — устраняя +находку r1 удалением «пути 2», ревизия 2 разорвала совместность AC1/AC2 (гвард +обязан отклонить запись `pure_imports.py:31` по правилу fail-closed) и AC3 +(единственное разрешённое исключение — только `conftest.py`) при том, что +`pure_imports.py` явно оставлен в скоупе и обязан по AC5 продолжать работать +для `test_junction_limits.py`. Правится в тексте ТЗ тем же автором, без выхода +за рамки текущей задачи — вероятный минимальный фикс: вернуть +`pure_imports.py` в список статических исключений (не путать со снятым «путём +2» — обязанность очистки `sys.modules` по AC4 остаётся в силе независимо от +статуса файла в списке исключений гварда).