From ab2f37aa4dab648ff04246d8c9e840fd9dcffe85 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:01:14 +0000 Subject: [PATCH] docs: review document for #265 Issue: #265 User-Visible: no --- docs/reviews/SPEC-REVIEW-265-r1.md | 251 +++++++++++++++++++++++++++++ 1 file changed, 251 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-265-r1.md diff --git a/docs/reviews/SPEC-REVIEW-265-r1.md b/docs/reviews/SPEC-REVIEW-265-r1.md new file mode 100644 index 00000000..1c7b88e2 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-265-r1.md @@ -0,0 +1,251 @@ +# SPEC-REVIEW-265-r1 + +- Issue: [#265](https://github.com/Matysh/houseplan-card/issues/265) — «Рефакторинг 2/5: один контракт шва импорта — ремап ссылок за собой и идемпотентная уборка» +- Этап: `S4-spec-review` (ревью ТЗ, PROCESS.md §2.4) +- Заход: r1 · блокирующих циклов израсходовано 0/4 (до вердикта) +- ТЗ: `docs/specs/265-import-reference-seam.md`, ветка `issue/265-import-seam-contract`, SHA `2cf47fc1` +- Ревьюер: Claude (свежая сессия, без переписки с автором) + +## Скоуп ревью + +Первый заход — разбор полный, дельта не применяется (§2.10 относится со второго +цикла). Материал: тело issue #265, оба комментария аналитика/автора, файл ТЗ +целиком (`docs/specs/265-import-reference-seam.md`, 454 строки) и правка +`docs/specs/README.md` (+1 строка — таблица ссылок). + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `PROCESS.md` (целиком, включая §2.4/§2.5/§2.9/§7.1), + `AGENTS.md`. +2. Прочитано тело issue #265 и оба комментария (`Взял: …`, аналитика, + «ТЗ готово»). +3. Проверены утверждения §2 «Подтверждённое состояние кода» против реального + `custom_components/houseplan/import_export.py`: `_fresh()` (строка 804), + `_repair_target_space_refs()` (825), `build_space_merge()` (875), + `create_preview()`/`revalidate_candidate()`/`prepare_apply()` (1214, 1330, + 1381). Все пять пунктов §2 подтверждены построчно — это не догадки: + `_fresh()` действительно строит stem из текущего `old` (без канонизации), + `_repair_target_space_refs()` действительно ремонтирует только exact-map + пяти перечисленных полей, preview/apply действительно каждый раз вызывают + `build_space_merge()` заново со случайным `secrets.token_hex(4)`. +4. Проверено утверждение §2.4 о `scripts/model-invariants.mjs`: функция + `checkReferences()` (строки 118–194) действительно не проверяет + `marker.room_id`, `vacuum.segment_map`, `rooms[].open_to`, `marker:*` + controls/value badge и partition opening host — совпадает с текстом ТЗ. +5. Проверены поля матрицы §7 против кода: `controls[]` с префиксом `marker:`, + `value_badge.ref`, `vacuum.segment_map`, `rooms[].open_to`, + `openings[].host.id` при `host.kind == "partition"` — все существуют в + `import_export.py` (строки 482–497, 750–780, 848–853, 916–927, 970–1014). + Матрица не изобретает несуществующие поля и не молчит про существующие, + которых коснулась выборка. +6. Проверены три «унаследованных решения владельца» (§3) против статуса и + текста связанных issue/спеков: #244, #248, #252, #258, #262 — все + `CLOSED` (`gh issue view`), тексты их ТЗ (`docs/specs/244-*.md`, + `252-*.md`, `262-*.md`) подтверждают ровно те формулировки, которые #265 + им приписывает (tombstone не собирается по сроку — #262 §6.2; отчёт живёт + внутри «Оптимизировать планы» — #252 §1–2; неразрешимая ссылка не + угадывается — #244 AC3/AC5/AC6). Ни один из трёх вопросов, которые исходный + текст issue адресовал владельцу, не решён произвольно: все три закрыты уже + принятыми (закрытыми) задачами, а не мнением автора. +7. Сверена терминология: «Оптимизировать планы», «Общие настройки» — + совпадает с `docs/USER-GUIDE.ru.md` (строки 402, 1425 и др.). «carrier» — + совпадает с `docs/WALL-THICKNESS.md`. Новый текст отчёта (§9) — это + черновик новой строки, не существующей ранее ни в одном каноне; это + ожидаемо для новой фичи и не является нарушением («термин из + USER-GUIDE» требование касается уже существующей терминологии интерфейса). +8. Сверена структура документа с двумя прямыми «братьями» той же серии + рефакторинга — `docs/specs/244-orphan-space-references.md` и + `docs/specs/252-optimize-orphan-layout-report.md` (тот же автор, тот же + класс задачи, ссылаются друг на друга) — чтобы отличить «этот документ так + устроен потому что задача другая» от реального отступления от конвенции. +9. Технически проверена состоятельность алгоритма канонизации lineage (§5.1, + edge case 1): смоделирован случай, когда пользовательский id случайно + принимает форму `__<8 lowercase hex>` и совпадает root'ом + с реальным импортным id. Риск ложного совпадения существует, но заперт + пятью одновременными условиями §5.2 (мертва исходная ссылка + ровно один + живой кандидат + совместимый тип + отсутствие exact id + отсутствие второго + кандидата с тем же root) и прямо назван эвристикой, а не идентичностью + (§17). Блокирующим не считаю. +10. `node scripts/check-docs.mjs` не запускался: diff не касается `src/**` + (только `docs/specs/**`), гейт неприменим по своему собственному условию + («любая правка фронтенда» — здесь правки фронтенда нет). Остальные гейты + (`typecheck`/`test`/`build`/смоки/инварианты) относятся к этапу код-ревью + (S7), а не спек-ревью (S4); на этом этапе кода еще нет. + +## Находки + +Все находки — **Medium, в скоупе задачи**: чинятся тем же автором в этом же +документе ТЗ, без нового цикла обсуждения продукта и без отдельного issue. +High-находок нет. + +### M1 — «что человек увидит» слито со сценарием и написано языком реализации; поверхность/touch не названы + +**Файл:** `docs/specs/265-import-reference-seam.md`, §1 (строки 19–51). + +PROCESS.md §7.1 требует два отдельных обязательных раздела: сценарий (какая +персона, на какой поверхности, в какой момент) и «что человек увидит» +**одной фразой, без терминов реализации** — и прямо предупреждает: «ТЗ, которое +не может ответить на эти два вопроса, описывает работу, а не изменение +продукта». Оба прямых родственника этой серии соблюдают разделение и +дают термин поверхности явно: `244-orphan-space-references.md` §1–2 и +`252-optimize-orphan-layout-report.md` §1–2 оба содержат фразу вида «Optimize — +desktop-first административная поверхность; View и kiosk только читают уже +сохранённый результат» и отдельное «До/После» на бытовом языке («видит список +внутренних id и не понимает, безопасно ли нажимать Apply»). + +В §265 «До»/«После» — это фактически список внутренних инвариантов: «id +строится от канонического корня lineage», «матрица plan-id ссылок», +«неизменяемый candidate», «Apply пишет ровно тот кандидат». Ни одна из этих +пяти строк не отвечает на вопрос, что увидит администратор в диалоге импорта. +Единственная строка, которая касается видимого экрана («Optimize остаётся +единственным инспектором») — про архитектурное решение, не про экран импорта. +Заодно нигде в документе не упомянут `docs/TOUCH-SUPPORT.md` и статус +поверхности (desktop-only админ-функция) — обязательный пункт DoR §2.5 +(«влияние на touch… названо»). + +**Почему это не мелочь:** без явного «что человек увидит» ревьюер не может +проверить AC10 на полноту (см. M2) — граница «это то же самое ТЗ, просто +описано плохо» и «здесь спрятано непродуманное продуктовое решение» стирается +именно тем, что раздел не выполняет свою функцию. + +**Как чинится:** вынести из §9 уже готовые пользовательские формулировки +(«сохранены без изменений…», «Убрано забытых записей: N») в отдельное «После» +одним-двумя предложениями без «lineage»/«canonical root»/«candidate»; добавить +фразу про desktop-only поверхность и отсутствие touch-влияния, как в +#244/#252. + +### M2 — AC10 хеджирует «если отчёт станет виден пользователю», хотя §9 безусловно описывает новый видимый текст + +**Файл:** `docs/specs/265-import-reference-seam.md`, §9 (264–291) против AC10 +(405–412). + +§9 не оставляет пространства для сомнения: «Import preview показывает: +количество восстановленных target-ссылок; количество сохранённых неразрешимых +ссылок с формулировкой «сохранены без изменений; после импорта запустите +"Оптимизировать планы"»; количество отброшенных incoming links…». Это +конкретный, безусловный список новых пользовательских строк. + +AC10 при этом формулирует i18n/changelog/screenshot обязательства условно: +«Если новый report виден пользователю... Если итоговая реализация не меняет +видимый UI, changelog фиксирует только "small fixes"». Это внутреннее +противоречие: собственный §9 уже утверждает, что UI меняется, а AC10 +оставляет автору лазейку объявить это невидимым. DoR §2.5 требует, чтобы +«i18n: ключи en + ru перечислены» было решено **до** перехода в +«Готово к разработке», а не отложено условием, которое своим же текстом +документа уже снято. + +**Как чинится:** снять условность из AC10 (текст отчёта уже зафиксирован в +§9 — значит UI меняется точно) и либо перечислить конкретные i18n-ключи +(даже черновые: `import.report.remapped`, `import.report.preserved_unresolved`, +`import.report.dropped_incoming` и т.п.), либо явно сказать, что ключи +называет реализация, а ТЗ фиксирует только текст — но без слова «если» там, +где сам документ уже решил вопрос. + +### M3 — AC1–AC9 не называют способ доказательства (в отличие от требования DoR и от прямого соседа #244) + +**Файл:** `docs/specs/265-import-reference-seam.md`, §13 (352–412), ср. +`docs/specs/244-orphan-space-references.md` §14 (355–369). + +PROCESS.md §2.5 требует: «AC1…ACn — пронумерованные проверяемые критерии +приёмки; у каждого указано, чем он доказывается: `unit`/`backend`/`smoke`/ +`golden`/«ревью кода»». `#244` (тот же автор, та же неделя, тот же кластер +задач) соблюдает это буквально — таблица АС там имеет отдельный столбец +«доказательство» для каждой строки («Table-driven optimizer unit + idempotence +unit.», «Backend import/export tests preview/revalidate/apply.» и т.д.). + +В §265 из десяти AC явный метод доказательства называет только AC10 +(«canonical-doc screenshots/golden review»). AC1–AC9 — чистый текст критерия +без указания, каким тестом он проверяется; §14/§15 дают только общий, +не привязанный к номеру AC список («Python import/export unit/backend tests; +frontend unit tests…»). Из этого списка не восстанавливается однозначно, что, +например, AC2 (cross-generation repair) проверяется backend unit, а AC7 +(идемпотентность) — какой конкретно раз запускаемым тестом. + +**Как чинится:** добавить к каждому AC1–AC9 короткую пометку способа +доказательства по образцу §14 из #244 — минимальная правка формата, не +меняющая ни один критерий по содержанию. + +### M4 — нет отдельного обязательного раздела «Риски» + +**Файл:** весь документ; ср. `docs/specs/244-orphan-space-references.md` §16 +(«Риски, производительность и security»). + +PROCESS.md §7.1 перечисляет «риски» как самостоятельный обязательный раздел +ТЗ, отдельно от AC и от edge cases. В §265 нет раздела с таким содержанием: +§12 «Edge cases» — про корректность алгоритма на конкретных входах, не про +риски проекта (например: риск ложного совпадения lineage при совпадающей +пользовательской строке — уже частично обсуждён в edge case 1, но не назван +явно как принятый риск с последствием; риск расхождения Python/TS реализаций +общего lineage-алгоритма, который §5.3 признаёт возможным и лечит только +conformance fixture — сам факт «может расходиться, лечим тестом» это и есть +риск, который нигде не сформулирован как риск). + +**Как чинится:** добавить раздел «Риски» (можно короткий, 3–5 пунктов): +ложноположительное совпадение lineage у пользовательского id; расхождение +Python/TS реализации до появления conformance fixture; неполнота матрицы §7, +если найдётся ещё не учтённое plan-id поле; стоимость независимого прогона +инвариантов на больших конфигурациях (уже частично закрыто AC9, достаточно +сослаться). + +## Что проверено и корректно + +- Диагноз §2 (five claims про `_fresh`/`_repair_target_space_refs`/preview↔apply + desync/`model-invariants.mjs` gaps) — точен, построчно сверен с кодом, не + является догадкой. +- Матрица внутренних ссылок §7 — все перечисленные поля существуют в коде; + не нашёл полей, которые код трогает, а матрица замалчивает бы, в пределах + выборки, которую успел свести (`marker.space`, `marker.room_id`, + `vacuum.segment_map`, `rooms[].open_to`, `markers[].controls[]`, + `value_badge.ref`, `openings[].host.id`). +- Три «унаследованных решения владельца» (§3) действительно наследуются от + закрытых issue #244/#248/#252/#262, а не выданы автором за решение владельца + без основания — все три вопроса, которые исходный текст #265 адресовал + владельцу, закрыты предыдущими, уже принятыми задачами. +- Скоуп/не-скоуп (§4) явный и не расширяет задачу дальше контракта шва импорта; + явно исключены full-import semantics, merge пространств, HA-registry remap, + удаление записей — совпадает с §SCOPE.md «never delete a user's file on an + inference» и с принципом lossless Optimize. +- Алгоритм канонизации lineage (§5) внутренне согласован, ограничен по глубине, + не путает namespace при разборе (проверено на примере + `marker_room_x_deadbeef`), и явно называет себя эвристикой, а не + криптографическим доказательством происхождения — риск ложного совпадения + признан и заперт пятью условиями §5.2. +- Терминология («Оптимизировать планы», «carrier») соответствует + `docs/USER-GUIDE.ru.md` и `docs/WALL-THICKNESS.md`, не изобретена. +- Открытых продуктовых вопросов владельцу нет; вопросов, которые стоило бы + туда вынести, я не нашёл — то, что можно было решить самостоятельно + (алгоритм lineage, формат id, порядок remap), решено автором и явно помечено + как допущение в §17, подлежащее спору с ревьюером, а не эскалации. +- AC5/AC8 корректно защищают инвариант «lossless по умолчанию» и явно + перечисляют regression-контракты пяти связанных задач, что закрывает риск + повторной регрессии одной из них при рефакторинге общего шва. +- Изменение файла `docs/specs/README.md` — верно оформленная строка таблицы, + без добавления второго источника статуса (колонка «Статус ТЗ» не трогается). + +## Чего не проверял + +- Не запускал `typecheck`/`test`/`build`/смоки/инварианты — на этапе спек-ревью + кода еще нет, эти гейты относятся к S7 (код-ревью) и будут релевантны при + следующем заходе после реализации. +- Не проверял `node scripts/check-docs.mjs` — diff не касается `src/**` + (только `docs/specs/**`), условие гейта не выполнено, прогон не даёт + дополнительной информации. +- Не оценивал производительность реального lineage-индекса на больших + инсталляциях эмпирически — только прочитал заявление AC9 о линейности и не + нашёл в описанном алгоритме (§8) шагов, требующих декартова сравнения; + оценка «читал, не исполнял». +- Не проверял конкретные формулировки будущих i18n-строк на корректность + русского/английского — они ещё не существуют, будет предметом код-ревью. + +## Вердикт + +Технический диагноз и дизайн решения обоснованы, привязаны к реальному коду и +к уже принятым решениям владельца по связанным задачам; ни одна находка не +касается корректности алгоритма или заявленного продуктового поведения. +Все четыре находки — про полноту обязательных разделов ТЗ по PROCESS.md §7.1 и +DoR §2.5 (пользовательское «до/после» без языка реализации, безусловность +i18n-обязательств, способ доказательства на каждый AC, отдельный раздел +рисков), и все устраняются правкой текста того же документа без пересмотра +дизайна. + +**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 4 → в задаче**