diff --git a/docs/reviews/SPEC-REVIEW-262-r1.md b/docs/reviews/SPEC-REVIEW-262-r1.md new file mode 100644 index 00000000..c87c08e1 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-262-r1.md @@ -0,0 +1,174 @@ +# SPEC-REVIEW-262-r1 + +- Issue: [#262](https://github.com/Matysh/houseplan-card/issues/262) — Deleting entities prevents them from being added again later +- ТЗ: [`docs/specs/262-readd-child-entity-after-device-delete.md`](https://github.com/Matysh/houseplan-card/blob/issue/262-readd-child-entity/docs/specs/262-readd-child-entity-after-device-delete.md) +- Ветка: `issue/262-readd-child-entity`, коммит на ревью: `7b3ea37` +- Этап: `S4-spec-review` (ревью ТЗ, PROCESS.md §2.4) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 (зелёный вердикт бюджет не тратит, #227) +- Диф ревью: `git diff origin/dev...origin/issue/262-readd-child-entity` — только + `docs/specs/262-readd-child-entity-after-device-delete.md` (новый файл) и одна + строка в `docs/specs/README.md`. Продуктового кода нет — соответствует стадии + `S3→S4` (Rule #1: код трогают только с `S5-ready`). + +## Скоуп + +Bug-репорт: после удаления HA-**устройства** с плана его отдельные дочерние +**сущности** невозможно добавить обратно даже при включённом флаге «Показывать +сущности» — можно вернуть только всё устройство целиком. Аналитика владельца +(комментарий в issue) разложила репорт на три сценария и подтвердила, что +именно третий («удалить device, добавить одну entity того же device») — баг, +остальные два уже работают штатно. ТЗ фиксирует контракт на этот третий +сценарий: exact live `entity:X` должна перевешивать exact/parent tombstone +только для себя самой, не воскрешая ни родительское устройство, ни соседние +сущности. + +Соответствие `docs/SCOPE.md`: закрывает J6 («Keep the plan true as the home +evolves») — часть про «поддерживать план в актуальном состоянии» и явный выбор +нужной HA-привязки после удаления. Продуктовой рамке не противоречит, новых +поверхностей не открывает. + +## Как проверялось + +Ревью технической части ТЗ построено не на доверии тексту, а на сверке каждого +фактического утверждения (§3, §4, §6, §8) с кодом на `origin/dev` — здесь +именно тот случай, где «утверждение о поведении не в документах и не +помеченное как предположение» было бы находкой, если бы не подтвердилось. + +1. **§4.1 (picker разрешает только точный tombstone binding).** Прочитан + `_bindingCandidates()` в `src/houseplan-card.ts` (ветка «Individual entities + of devices — behind the show entities checkbox»): `removedBindings` — Set + точных строк binding с `m.removed`; исключение `!removedBindings.has(v)` + сравнивается с `v = 'entity:'+eid`, а не с `'device:'+parentId`. При + `device:D` tombstone `isRemovedPlanEntity(h, eid, removed)` возвращает + `true` (по `removed.devices.has(deviceId)`), `removedBindings.has('entity:'+eid)` + — `false` ⇒ строка `continue` отбрасывает X из списка даже при включённом + чекбоксе. Ровно то, что описывает ТЗ. Подтверждено чтением. +2. **§4.2 (фикса списка недостаточно).** Прочитан `_saveMarker()`: + `replacedRemovedIds = markers.filter(m => m.removed && m.binding === dlg.binding)` + — снимает tombstone только с тем же exact binding (`entity:X`), родительский + `device:D` tombstone переживает сохранение. Прочитан `buildDevices()`, ветка + `kind === 'entity'`: `if (isRemovedPlanEntity(fullHass, ref, removed)) continue;` + — уже сохранённый **живой** explicit marker `entity:X` всё равно отбрасывается, + потому что `removed` строится из того же `markers[]`, где всё ещё лежит + `device:D` tombstone. Оба утверждения ТЗ подтверждены чтением, не + домыслены. +3. **§8 (schema-valid combination).** Прочитан `validation.py`: схема маркера + не запрещает сосуществование двух записей с разными `binding` и независимым + `removed`; для `device:D removed:true` + живой `entity:X` нет constraint, + который бы это отклонил. Утверждение «уже schema-valid» — не гипотеза. +4. **§3 (воспроизведение).** Прочитан `demo/smoke_binding_picker.mjs` (добавлен + в #263): блок 3 буквально помечает + `o.knownDefect262ChildEntityBlocked = !(await offered(true)).includes('entity:'+childEntity)` + с комментарием «почитают #262, смок покраснеет, потребовав перевернуть + проверку» — соответствует плану автотестов ТЗ (AC1: «проверка known defect + перевёрнута в положительную»). +5. **Согласованность с #226.** Прочитан `docs/specs/226-entity-parent-dedup.md` + (решения 4–5: «явная конфигурация сильнее автоматической», «tombstone не + владеет сущностью») — контракт §6.4 ТЗ #262 («явная D не подавляет явную X») + этому не противоречит, использует уже принятую модель, не переопределяет её. +6. **Согласованность с #104.** `docs/specs/104-opening-ha-reference-after-marker-delete.md` + сам ссылается на `isRemovedPlanEntity`/`isRemovedPlanSource` — подтверждает + заявление ТЗ, что точные ссылки opening contact/lock уже независимы от + marker tombstones и не задеты этим исправлением. +7. **Регистрация smoke-связи.** `scripts/smoke-links.mjs` уже связывает символы + `removedPlanBindings`, `isRemovedPlanEntity`, `deletePlanMarkerRecords` со + `smoke_binding_picker.mjs` (внесено в #263) — заявление §11 ТЗ («имя + `_bindingCandidates` остаётся прямым совпадением, дублировать не требуется») + корректно, связь уже есть, ничего добавлять не нужно. +8. **Терминология UI.** Флаг называется в ТЗ «Показывать сущности» — + совпадает с формулировкой из `docs/USER-GUIDE.ru.md` (строки 657, 1587), а + не изобретён. (Сам i18n-ключ `marker.show_entities` в коде переведён как + «Отображать сущности» — расхождение между `USER-GUIDE.ru.md` и `ru.json` + существует независимо от #262, автор ТЗ корректно взял термин из + пользовательского гайда, как требует AGENTS.md; фиксирую это отдельно ниже + как находку вне скоупа, не блокирующую этот заход.) +9. **Обязательные разделы ТЗ (PROCESS.md §7.1).** Сверены построчно: сценарий + и персона (§1) — есть, включая поверхность и момент; что человек видит + до/после одной фразой без терминов реализации (§2) — есть, без упоминаний + функций/tombstone; проблема (§3–4); скоуп и не-скоуп (§5); контракт + поведения (§6); UX/touch/a11y (§7); данные/compatibility/миграция (§8); + i18n/security/performance (§9); AC1…AC6 с доказательством (§10); план + автотестов (§11); файлы (§12); release-артефакты (§13); риски (§14); + откат (§15); блок принятых предположений (§16). Ничего не пропущено. +10. **Проверка «не выдана ли догадка за факт».** Все технические утверждения о + текущем поведении (§3, §4, §8) проверены чтением реального кода (пункты + 1–3 выше), а не приняты на слово. Пять пунктов §16 корректно помечены как + предположения и являются техническими (форма реализации, позиция нового + маркера, отсутствие whitelist для registry-hidden), а не спрятанными + продуктовыми решениями — ни один не требовал вопроса владельцу. +11. **AC на проверяемость и доказательства.** AC1–AC6 однозначны, у каждого + указан способ доказательства (`smoke`/`unit`/«существующие + unit/smoke #104/#161/#226»/gates), совпадающий с §11 и с уже + зарегистрированными файлами/тестами. + +## Находки + +Блокирующих (High) и находок Medium в скоупе — нет. + +### Medium — вне скоупа (не блокирует, заводится отдельным issue по §12 PROCESS.md) + +Расхождение термина чекбокса между `docs/USER-GUIDE.ru.md` («Показывать +сущности», строки 657 и 1587) и фактическим i18n-ключом `marker.show_entities` +в `src/i18n/ru.json` («Отображать сущности»). Автор ТЗ #262 действовал верно — +взял формулировку из канонического пользовательского гайда, как требует +AGENTS.md, — но само расхождение живёт в репозитории независимо от #262 и не +имеет отношения к его сценарию. Правка в этой ветке была бы посторонним +скоупом (диалог маркера правит другой файл — `houseplan-card.ts`/`ru.json`, не +затронутый ТЗ #262). Заведён отдельным issue — см. решение ниже. + +Низких (Low) находок нет. + +## Что проверено и корректно + +- Диагноз двухчастного дефекта (picker exact-binding whitelist + runtime + `isRemovedPlanEntity` в `buildDevices`) — точное соответствие коду `dev`, не + гипотеза. +- Контракт §6 (матрица Add picker, транзакция сохранения, runtime-приоритет, + что остаётся удалённым) — самосогласован: список Add специально шире, чем + runtime-построение (все дети D становятся видимыми в Add, но + строится/рендерится только явно сохранённый marker) — это осмысленное + разделение, а не противоречие. +- Совместимость с уже принятыми контрактами #226, #161, #104 подтверждена по + их собственным спекам и коду, не только заявлена. +- Данные/миграция: комбинация `device:D removed:true` + живой `entity:X` уже + schema-valid, новых полей и model bump нет — подтверждено чтением + `validation.py`. +- Воспроизведение (§3) подтверждено существующим browser smoke + `demo/smoke_binding_picker.mjs` (#263), который уже фиксирует + `knownDefect262ChildEntityBlocked: true` и явно рассчитан на переворот + проверки этим исправлением. +- Все обязательные разделы ТЗ (PROCESS.md §7.1) присутствуют и по существу, не + формально. +- Продуктовых вопросов владельцу нет, и это оправдано: ни одна неопределённость + в ТЗ не задевает то, что видит или делает пользователь сверх уже описанного + в §1–§2; сценарий закрыт исходным репортом и аналитическим комментарием + владельца. +- i18n-раздел корректен: новых ключей нет, использованная терминология взята + из `docs/USER-GUIDE.ru.md`, а не изобретена. + +## Чего не проверял + +- Гейты `typecheck`/`test`/`build`/`check-docs` не гонялись: этап — ревью ТЗ + (`S3→S4`), продуктового и тестового кода в дифе нет (только новый + `docs/specs/*.md` и строка в `docs/specs/README.md`), эти гейты относятся к + этапу код-ревью (PROCESS.md §2.7, §8) и будут прогнаны там. +- Не проверялась работоспособность будущей реализации — её ещё нет; предмет + этого ревью — выполнимость и однозначность ТЗ, а не код. +- Не запускал browser smoke `demo/smoke_binding_picker.mjs` — код на этой + ветке не менялся относительно `dev`, поведение смока не могло измениться; + чтения исходника было достаточно, чтобы подтвердить, что смок уже + воспроизводит описанный в ТЗ дефект. +- Не проверял golden/performance/mutation-gate — на этой стадии нет ни + визуальных, ни мутационных изменений: диф ограничен документацией. + +## Решение + +Найдена ровно одна находка — Medium, вне скоупа задачи (терминологическое +расхождение чекбокса, не создано и не усугублено этим ТЗ). Она не блокирует +переход в `S5-ready` и заводится отдельным issue со ссылкой на #262, а не +чинится в этой ветке. + +**Вердикт: зелёный.** ТЗ выполнимо, однозначно, каждый AC проверяем и снабжён +способом доказательства, технические утверждения о текущем поведении +подтверждены чтением кода, а не приняты на веру, продуктовых вопросов +владельцу нет.