mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-02 12:49:56 +00:00
claude[bot]
@@ -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 → в задаче**
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `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
|
||||
Reference in New Issue
Block a user