diff --git a/docs/reviews/SPEC-REVIEW-500-r1.md b/docs/reviews/SPEC-REVIEW-500-r1.md new file mode 100644 index 00000000..c2f90d85 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-500-r1.md @@ -0,0 +1,233 @@ +# SPEC-REVIEW-500-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/500 +- **Этап:** ревью ТЗ (PROCESS.md §2.4) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 (лимит на полном треке — 4) +- **Материал:** ветка `issue/500-config-adoption-boundary`, коммит `ca07067386dee0ce2bfad64c169404c9284cd242` + (docs-only поверх `origin/dev` = `6b31e945`), файл ТЗ + `docs/specs/500-config-adoption-boundary.md`, дерево `183f1ddc6b6261a2345fcc84502883e6274a9b70`. +- **Ревьюер:** Claude (роль «ревьюер ТЗ», отдельно от автора). + +## Скоуп ревью + +Issue помечен `S4-spec-review`, не `small` — трек полный, файл ТЗ обязателен и +существует. Проверялись: §7.1 обязательные разделы, однозначность и +доказуемость каждого AC, соответствие `docs/SCOPE.md` (J6, техдолг), отсутствие +догадок, выданных за факт, и — отдельно — фактическая точность инвентаризации +кода, на которой стоит весь дизайн (проблема §3, скоуп §4, контракт §6, +критерий AC1/AC2/AC4). Полный текст ТЗ и все ссылки на строки кода +перепроверены чтением `src/**` на рабочей копии, соответствующей материалу. + +## Как проверялось + +Ревью текстовое (продукт кода не меняет). Дополнительно к чтению ТЗ выполнена +верификация количественных утверждений §3/§6.3/AC1/AC2 прямым чтением +источника (не догадка «похоже на правду», а построчный grep + просмотр тела +функций): + +``` +grep -rn "_adoptStructuralResponses" src/ # все вызовы и объявления +grep -rn "_serverCfg\s*=" src/ --include=*.ts # писатели тела config +grep -rn "_cfgRev\s*=" / "_layoutRev\s*=" src/ # писатели ревизии +wc -l src/houseplan-card.ts # бюджет файла +grep -n "13700" test/core-file-budget.test.mjs +grep -n "recoveryPreparesBackdropBeforeAdoption" demo/smoke_summary_panel.mjs +``` + +Гейты кода не запускались — этап ревью ТЗ, продуктовый код не меняется, что +проверять автотестами нечего (ТЗ — не код). Спецификация — единственный +артефакт этого раунда. + +## Находки + +### High-1 — Инвентаризация вызывающих `_adoptStructuralResponses` неполна: 4 названных против 7 фактических, включая дубликат самого дефекта, который чинит AC4 + +**Резюме.** §3.2 и §6.3 ТЗ утверждают ровно четыре вызывающих +`_adoptStructuralResponses`: `_loadFromServer`, `_reloadConfigOnly`, summary +recovery, onboarding `space/delete`. AC2 требует, чтобы после переноса метод +`_adoptStructuralResponses` не существовал как метод хоста вовсе. Чтение +`src/**` на материале ревью находит **семь** реальных мест вызова, три из них +нигде не названы в ТЗ: + +``` +src/houseplan-card.ts:4350 _loadFromServer — учтён +src/houseplan-card.ts:4527 _reloadConfigOnly — учтён +src/summary-panel-runtime-loaded.ts:625 summary recovery — учтён +src/houseplan-onboarding-runtime.ts:476 onboarding space/delete — учтён +src/houseplan-editor-runtime.ts:8693 ВТОРОЙ обработчик space/delete — НЕ учтён +src/houseplan-editor-runtime.ts:9577 _undoPlanOptimization (plan/optimize_undo) — НЕ учтён +src/houseplan-editor-runtime.ts:9736 _applyBackupImport (import/apply) — НЕ учтён +``` + +Хуже самого недосчёта: `houseplan-editor-runtime.ts:8684–8695` — байт-в-байт +повтор того же самого обработчика `houseplan/space/delete`, что и в +`houseplan-onboarding-runtime.ts:463–478`, с тем же дефектом класса M1 #490, +который ТЗ называет проблемой №3 и чинит через AC4: после +`_adoptStructuralResponses(configResponse, layoutResponse)` код на строке 8694 +делает `this.host._cfgRev = response?.config_rev ?? this.host._cfgRev` — то +есть ревизия снова берётся из ответа `delete`, а не из адоптированного `get` +(строка 8695 — то же для `_layoutRev`). Это ровно тот же баг, только во +втором, необъявленном месте. + +AC4 и его свидетель (`demo/smoke_space_delete_adoption.mjs`, «или расширение +существующего смока onboarding», §11) целятся только в +`houseplan-onboarding-runtime.ts`. После реализации по ТЗ как оно написано +`houseplan-editor-runtime.ts:8693` останется звать метод, который по AC2 +«не существует как метод хоста» — то есть либо AC2 фактически не выполнена +(метод остаётся под другим именем ради этого вызывающего), либо этот +вызывающий тихо не мигрирован и продолжает содержать исходный дефект, +который issue #500 прямо ссылается как мотивирующий пример (M1 #490) и +который данная задача заявляет закрытым. + +**Почему это не мелочь.** Смысл задачи — «одна граница владения», а не +частичный перенос. §4 (скоуп), §6.3 (единая гейт-последовательность), риски +§12 и план тестов §11 построены вокруг «четырёх путей»; проверка полноты +(AC2: «не существует как метод хоста») пройдёт тестом, который просто не +знает о трёх реальных вызывающих — это ложноположительный AC (##435 намекает +именно на такой класс: защитный AC зелёный, потому что не видит второй +писатель). А конкретно для `space/delete` — это не абстрактный риск, а +буквально второй экземпляр того самого дефекта, который задача написана +чинить. + +**Воспроизведение.** См. команды выше; строки процитированы дословно из +рабочей копии на материале ревью (`ca070673` докс-коммит поверх `dev` +`6b31e945`, идентичного коду, который описывает ТЗ). + +**Что нужно.** Либо (а) включить `houseplan-editor-runtime.ts:8693` (второй +`space/delete`), `:9577` (`optimize_undo`) и `:9736` (`import/apply`) в +инвентаризацию §3, охват §4/§6.3 и AC2/AC4 с соответствующими свидетелями — +особенно дубликат `space/delete`, раз его дефект и есть предмет AC4; либо +(б) явно и по имени вывести их «не входит» (§5) с обоснованием, почему метод +`_adoptStructuralResponses` для них не мигрирует и AC2 в этой части не +абсолютна. Второе не обязано быть неверным решением — но must быть решением, +а не пропуском. + +### High-2 — Инвентаризация писателей тела `_serverCfg =` тоже неполна: 6/16 названо, фактически 8 модулей/18 присваиваний, ровно тех, что нужны AC6 + +**Резюме.** §3.1 называет 16 присваиваний `_serverCfg =` в 6 модулях +(`houseplan-card.ts` 6, `houseplan-editor-runtime.ts` 6, +`plan-optimize-write.ts`, `serialized-write-queue.ts`, +`space-copy-runtime.ts`, `summary-panel-runtime-loaded.ts`). Grep на +материале ревью даёт **18** присваиваний в **8** файлах — сверх названных +ещё: + +``` +src/editors/vacuum-maps-section.ts:107 host._serverCfg = nextConfig; +src/vacuum-calibration-write.ts:108 host._serverCfg = candidate; +``` + +(`houseplan-editor-runtime.ts` фактически даёт 8 присваиваний, не 6 — +отдельное расхождение того же счёта.) + +Оба необъявленных места делают ровно то, ради чего задуман `beginOptimistic`/ +`rollbackOptimistic` (§6.2, AC6): читают `previous = host._serverCfg`, зовут +`optimisticAttempt(previous, next, host._cfgContentFingerprint, host._cfgRev, +contentFingerprint)` из `serialized-write-queue.ts` — той самой функции, +которую ТЗ называет мигрирующей в модуль, — а затем пишут +`host._serverCfg = next` напрямую в обход какого-либо централизованного шага. + +AC1 требует «ноль» присваиваний только в пяти явно названных файлах и +разрешает присваивания только в allowlist `{houseplan-editor-runtime.ts, +houseplan-card.ts}` (staging размещения устройств). Ни один из двух +vacuum-файлов не назван ни в allowlist, ни в списке «ноль» — то есть AC1 в +текущей формулировке не решает, что с ними происходит: если lint-тест +(«по образцу single-source-numbers», сканирует весь `src/**`) увидит их как +нарушение, реализация тихо расширится на два модуля, ни разу не упомянутых в +скоупе (§4), рисках (§12) или плане тестов (§11); если инструмент их не +заметит (например, из-за allowlist по счётчику файлов, а не по regex), то +заявление AC1 «идентичность пишет только модуль» после мержа будет +фактически неверным. + +**Почему High, а не Medium.** Тот же корень, что и High-1 — заявленная +инвентаризация кодовой базы, на которой построены критерии приёмки, неполна +именно там, где неполнота выключает саму гарантию задачи («одна точка +записи»). Разница с #500-мотивирующим примером (M1 #490) методологическая +одна и та же: новый путь (здесь — уже существующий, просто не увиденный) +подключается к состоянию мимо правил. + +**Что нужно.** Явно решить участь `vacuum-maps-section.ts` и +`vacuum-calibration-write.ts` — включить в перенос на `beginOptimistic` с +AC/тестом, либо аргументированно исключить (§5) с указанием, почему их +прямая запись не нарушает инвариант I1/AC1 (например, если признано, что они +пишут после `rollbackOptimistic`-подобной логики и это осознанно оставлено +отдельным техдолгом). Любой из исходов приемлем — отсутствие решения не +приемлемо. + +## Что проверено и корректно + +- Обязательные разделы §7.1 присутствуют все: сценарий, «что человек + увидит», проблема, скоуп/не-скоуп, контракт (§6), UX, данные/миграция, + i18n, AC1–AC8 с доказательством и «чем краснеет», план автотестов, риски, + откат, release-артефакты. +- Продуктовых вопросов владельцу нет — оправдано: пользователь не наблюдает + изменений (кроме одного явно названного и покрытого AC4 случая), решения + технические и вынесены отдельным блоком §15 «принято предположительно» с + обоснованием — это ровно формат, требуемый §7.1, а не догадки, выданные за + факт. +- Отказ от `small` обоснован названными нарушенными критериями §5 + (сложность/риск 7/10, 7 модулей, state-контракт revision+fingerprint) — не + голословное «обычный трек». +- Точечные фактические проверки, не связанные с находками выше, подтвердились + дословно: `houseplan-card.ts` — 13699 строк при бюджете `core-file-budget` + 13700 (§3.5/AC7); `_adoptStructuralResponses` объявлен в + `houseplan-card.ts:4231`, вызовы `_loadFromServer`/`_reloadConfigOnly` на + строках 4350/4527 соответствуют заявленным (4315/4496 — расхождение на + десяток строк от актуального дерева, не искажает факт); восемь методов + `SummaryPanelHost` (`summary-panel-host.ts:84–94`), которые AC2/§6.4 + предлагает сузить до одного, совпадают построчно с списком в ТЗ; смок + `demo/smoke_summary_panel.mjs` действительно содержит сценарий + `recoveryPreparesBackdropBeforeAdoption`, названный доказательством AC3. +- I2/I4 (ревизия только вместе со своим телом; ссылочная идентичность без + клонирования) — корректно отражают текущее поведение + (`houseplan-editor-runtime.ts:8099` сравнивает `host._serverCfg === cfg`). +- Скоуп по `docs/SCOPE.md`: техдолг в J6 без новой пользовательской + поверхности, без новых настроек и i18n — соответствует. +- §5 «не входит» корректно отсекает lifecycle registry (#493/#425) и + umbrella-неизменяемость (#34/#425) — не повторяет уже сделанное + предшественниками, как и требует комментарий S2. + +## Чего не проверял + +- Не проверялась достижимость самого рефакторинга (перенос кода) — на этом + этапе кода ещё нет, ревью ТЗ оценивает план, а не реализацию. +- Не запускались автотесты/typecheck/build — класс изменения этого раунда + докс-онли, продуктовый код не менялся; проверка гейтов относится к + код-ревью (§2.7), не к ревью ТЗ. +- Не проверялась область `plan-optimize-write.ts`/`space-copy-runtime.ts` на + предмет собственных недосчитанных вызывающих сверх того, что указано в + находках выше — фокус верификации был на счётчиках, явно используемых в + AC1/AC2/AC4, поскольку именно они образуют находки; полный построчный аудит + всех шести первоначально названных модулей на предмет иных пропусков не + проводился. + +## Вердикт + +High: 2 (обе — в скоупе задачи, чинятся тем же автором в текущем ТЗ, без +отдельного issue). Medium: 0. + +Обе High-находки — не гипотетические риски, а проверенные чтением +расхождения между тем, что ТЗ утверждает о коде, и тем, что в коде есть на +названном материале. Пока инвентаризация вызывающих/писателей не будет +исправлена (включением недостающих мест в объём с AC/тестами либо явным и +обоснованным исключением в §5), задача не может дать заявленную гарантию +«одна граница владения» — а по `space/delete` вторая копия дефекта, который +задача написана закрыть, попадёт в `dev` немигрированной. + +**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 2 · Medium: 0 → в задаче** + +--- + + + +## Материал раунда + +- Ветка: `issue/500-config-adoption-boundary`, коммит `ca07067386de` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `183f1ddc6b6261a2345fcc84502883e6274a9b70` + ``` + git log --all --format='%H %T' | grep 183f1ddc6b62 + ``` +- ТЗ `docs/specs/500-config-adoption-boundary.md`, блоб `303837bef0fea5996f5bf36df714e8b560f49f17` + ``` + git log --all --find-object=303837bef0fea5996f5bf36df714e8b560f49f17 -- docs/specs/500-config-adoption-boundary.md + ``` +- Вердикт конвейера: `yellow` · High 2