diff --git a/docs/reviews/SPEC-REVIEW-406-r1.md b/docs/reviews/SPEC-REVIEW-406-r1.md new file mode 100644 index 00000000..f341a2f7 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-406-r1.md @@ -0,0 +1,198 @@ +# SPEC-REVIEW-406-r1 + +- Issue: #406 +- ТЗ: `docs/specs/406-beta2-polish.md`, ветка `issue/406-beta2-polish`, SHA `8248f9e4` +- Этап: spec (PROCESS.md §2.4) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 +- Вердикт: **жёлтый** + +## Скоуп + +Пять независимых мелочей (полный трек, критерий «одна поверхность» для `small` +не выполняется — согласен, обоснование корректно и подкреплено прецедентами +#385/#400): + +- (а) мёртвые ключи `confirm.*` и смежных семейств в 4 словарях + гейт; +- (б) роль `alertdialog`/`aria-describedby` в `hp-dialog`/`hp-confirm`; +- (в) смок HA-ветки диалога подтверждения; +- (г) уборка `marker_area_snapshot` для исчезнувших устройств + порядок + обрезки при лимите; +- (д) сохранение `acceptance.declared` при пустой замене в `docs-accept.mjs`. + +## Как проверялось + +Ревью аналитическое (этап spec, кода ещё нет). Читал тело issue #406, оба +комментария владельца (S2-разбор, ссылка на ТЗ), сам файл ТЗ на SHA `8248f9e4`, +`PROCESS.md` (§2.4, §7.1, §7.2), `docs/SCOPE.md`. Затем **перепроверил +фактические утверждения ТЗ по текущему коду** (`git archive` того же SHA, +`src/i18n/en.json`, `src/hp-dialog.ts`, `src/hp-confirm.ts`, +`src/danger-confirm.ts`, `src/device-area-relocation.ts`, +`scripts/docs-acceptance.mjs`, `demo/smoke_*`), поскольку ТЗ строит контракт +(а) на конкретных числах и списке ключей, а разошедшееся число здесь — не +стилистика, а входные данные для гейта, который эта же задача пишет. + +Обязательные разделы §7.1 присутствуют все: сценарий · что человек увидит до/ +после · проблема+контракт (по каждому из пяти пунктов) · скоуп/не-скоуп · UX · +модель данных и миграция · i18n · AC1…AC14 с доказательством · план автотестов +· риски · откат · release-артефакты. + +## Находки + +### [Medium, в скоупе] (а) Инвентарь мёртвых ключей `*.help.aria` неверен — все 19, а не 6 из 19, используются + +**Файл**: `docs/specs/406-beta2-polish.md`, раздел «(а) Мёртвые строки в +словарях», строка со списком «13 ключей `*.help.aria`». + +**Что не так**: ТЗ утверждает, что из ключей семейства `*.help.aria` 13 (из 19 +в словаре) — «задел, не подключённый ни одним потребителем». Проверка кода на +том же SHA это опровергает: **все 19** ключей `*.help.aria` в `en.json` +используются — не литералом, а через паттерн `` `${key}.aria` `` в +`_help(key)` (`houseplan-editor-runtime.ts:1194`: `const ariaKey = +\`${key}.aria\``). Для каждого из 19 базовых `*.help` ключей нашёлся ровно один +литеральный вызов `this._help('xxx.help')` в `houseplan-editor-runtime.ts` +и/или `houseplan-onboarding-runtime.ts` (`space.cell_cm.help`, +`space.zero_wall_style.help`, `space.fill_mode.help`, +`device_inbox.show_hidden.help`, `gs.glow_radius.help`, `marker.controls.help`, +`marker.glow_radius.help`, `marker.value_badge.help`, +`marker.value_badge_source.help`, `marker.value_badge_position.help`, +`marker.value_source.help`, `marker.light_role.help`, +`marker.light_entity.help`, `marker.toggle_entity.help`, +`marker.glow_mode.help`, `gs.bg_mode.help`, `gs.north.help`, +`space.bg_mode.help`, `space.north.help`) — то есть ровно тот самый паттерн +«динамический суффикс от литерального ключа», который ТЗ признаёт законным +классом (`furn.cat_${id}` и «48 семейств»), но почему-то не признал для этого +конкретного случая. + +Отдельно: список «Остальные двадцать три» в этом же разделе перечисляет только +19 позиций (13 `.help.aria` + `marker.display_hint` + `marker.display_hint_icon` ++ `history.delete_room` + `markup.delete` + `title.markup` + +`history.partition_add` = 19), а не 23 — четыре ключа из заявленных 23 нигде не +названы. + +**Сценарий отказа**: если реализация будет опираться на нарратив ТЗ (список +«30 доказанно мёртвых ключей», явно включающий 13 `.help.aria`) вместо +самостоятельной перепроверки, из словарей будут удалены 13 реально +показываемых строк подсказки-для-скринридера в панели настроек — то есть +задача, соседний пункт которой (б) *чинит* доступность для скринридера, +пунктом (а) её же и *поломает*. Если же реализация полагается только на +механический гейт (AC1) — риск переносится на сам гейт: правило распознавания +«динамических семейств» (AC3) не называет паттерн `${key}.help → ${key}.aria` +явно (назван только пример `furn.cat_${id}`), и создатель гейта имеет +основания не покрыть его — а именно это и произошло у автора ТЗ при ручном +аудите. + +**Ожидаемо**: пересчитать список действительно мёртвых ключей раздела (а) +(проверено выше: 6 из 19 `confirm.*`-ключей подтверждаются, `marker. +display_hint*`, `history.delete_room`, `markup.delete`, `title.markup`, +`history.partition_add` подтверждаются — эти 12 факт-чекнуты и мертвы; ни один +`*.help.aria` мёртвым не подтверждён при ручной проверке) и явно включить +правило «литеральный вызов `_help('x.help')` делает живыми оба ключа, `x.help` +и `x.help.aria`» в контракт AC3 как отдельно названное семейство, а не +подразумевать его в «и прочие 48». + +### [Medium, в скоупе] (б) AC6/AC7 не решают роль для `HpConfirmKind.warning` (диалог разблокировки) + +**Файл**: `docs/specs/406-beta2-polish.md`, раздел «(б) Роль диалога и текст +последствий», AC6/AC7. + +**Что не так**: `hp-confirm` — общий компонент для двух видов запросов, +`kind: 'destructive'` (используется 7 раз, все — удаления) и `kind: 'warning'` +(используется ровно один раз — подтверждение разблокировки двери, +`houseplan-card.ts:13001-13006`, с собственным текстом последствий +`confirm.unlock_body`). Контракт ТЗ формулирует дихотомию «destructive → +alertdialog» / «обычный → dialog», а в качестве примера «обычных» диалогов +называет только диалоги, вообще не проходящие через `hp-confirm` (маркер, +калибровка пылесоса). Про `kind: 'warning'` контракт молчит: буквальное +прочтение AC6 («Подтверждение **разрушающего** действия объявляется как +alertdialog») позволяет реализовать признак как `destructive = +request.kind === 'destructive'` и оставить диалог разблокировки на +`role="dialog"` без `aria-describedby` — то есть ровно тот же дефект, ради +починки которого заведён пункт (б), сохранится на диалоге, который +`docs/SCOPE.md` называет единственной санкционированной поверхностью опасного +действия (лок/разлок). + +**Сценарий отказа**: AC6 и AC7 оба зелёные (пройдены буквально по тексту), но +пользователь со скринридером по-прежнему не услышит «Устройство разблокируется» +при нажатии Unlock — потому что это `warning`, а не `destructive`, и контракт +не сказал, что с ним делать. + +**Ожидаемо**: явное решение в контракте (б) — получает ли `kind: 'warning'` +тот же `alertdialog` + `aria-describedby`, что и `destructive` (агент вправе +решить сам, это не видимая пользователю форма, а объявляемая роль ARIA, но +решение должно быть явным и внесено в AC6/AC7, а не оставлено читателю +угадывать по бинарной формулировке), и соответствующий пункт в AC8/плане +автотестов, если ветка `warning` должна тоже проверяться смоком. + +## Что проверено и корректно + +- **Числа по словарю в целом**: 1201 ключ в `en.json` — подтверждено точным + подсчётом. 7 ключей `confirm.*` (delete_draft, delete_draft_segment, + delete_plan, delete_room, delete_space, remove_marker, unlock) — подтверждено + отсутствием любого литерального использования в `src/*.ts` на SHA + `8248f9e4`. `marker.display_hint`, `marker.display_hint_icon`, + `history.delete_room`, `markup.delete`, `title.markup`, + `history.partition_add` — подтверждено тем же способом, действительно мертвы. +- **(а) коллизия с `test/unified-wall-tool-source.test.mjs`** — реальна и + корректно описана как открытая: тест требует `history.partition_add`, ключ + в `src/` не используется; ТЗ явно требует разрешить конфликт, а не тихо + оставить. +- **(в) HA-ветка не покрыта** — подтверждено: `smoke_free_walls.mjs` — + единственный смок, определяющий `ha-dialog`, и он стабит `_confirmDanger`, + так что сам диалог не рисуется; в `smoke_danger_confirmation.mjs` и + `smoke_danger_confirm_branches.mjs` слова `ha-dialog` нет. Прецедент стаба + (`smoke_free_walls.mjs:20-22`), на который ссылается AC8, реален и рабочий. +- **(г) утечка снапшота** — подтверждено чтением `device-area-relocation.ts`: + `resolveDeviceAreaRelocations` строит `decisions` только по + `options.devices`; устройство, отсутствующее в этом списке целиком, никогда + не попадёт ни в `decisions`, ни, соответственно, в `removeSnapshot` — запись + `marker_area_snapshot[id]` переживёт его исчезновение бессрочно. + `removeMarkerAreaSnapshots` действительно вызывается только из ручного + редактирования/удаления маркера (`houseplan-editor-runtime.ts:8326,8448`) — + номера строк точны. Термин «авторитетный реестр» — не изобретение ТЗ, это + существующее в коде понятие (`device-area-relocation.ts:50,133,142`, + `ha-binding-status.ts`), контракт AC10 (не трогать при неавторитетном + реестре) корректно продолжает уже действующий ранний `return` в + `resolveDeviceAreaRelocations` при `!authoritative`. + `.slice(0, MARKER_AREA_SNAPSHOT_LIMIT)` при чтении — подтверждено + (`device-area-relocation.ts:64-75`), обрезка по вставке действительно режет + новые записи, а не старые; контракт на переворот правила (AC11) реализуем. +- **(д) `acceptance.declared`** — подтверждено чтением + `scripts/docs-acceptance.mjs` и `scripts/docs-accept.mjs:141-145`: + `docsAcceptancePlan` при пустом `declared` не отказывает (не тот класс + ошибок, что «объявлены, но не изменились»), и текущий код действительно + безусловно пишет `declared: [...decision.replace]` — при `replace: []` это + стирает предыдущий список. AC12/AC13 реализуемы без правки существующих + тестовых утверждений — `docsAcceptancePlan` не меняется, меняется только + сборка `accepted.acceptance` в `docs-accept.mjs`. +- **Скоуп/не-скоуп** — границы названы явно и по делу (например, отказ от + подключения `*.help.aria`-задела к интерфейсу — не в этой задаче), пересечение + с #403/#405 разведено по разным контрактам того же файла. +- **Продуктовые вопросы владельцу** — ни одного технического вопроса не + вынесено владельцу; UX-раздел корректно ссылается на терминологию + `USER-GUIDE.ru.md` вместо изобретения новой. +- Release-артефакты: изменение User-Visible (скринридер объявляет + подтверждение как alertdialog) корректно привязано к обоим CHANGELOG, + остальные четыре пункта верно помечены внутренними. + +## Чего не проверял + +- Не пересчитывал вручную полный список из «48 динамических семейств» и + итоговые «937 использовано / 30 не используется» построчно — точечно + перепроверил категории, названные в тексте (`confirm.*`, `*.help.aria`, + единичные ключи), этого достаточно, чтобы найти расхождение; исчерпывающий + пересчёт всех 1201 ключей — задача самого гейта (AC1), не ревью ТЗ. +- Не проверял, останется ли запись `marker_area_snapshot` живой, если + устройство пропало из `_devices`, но по тому же id всё ещё существует + маркер (контракт (г) говорит «пока жив маркер ИЛИ устройство», AC9 + формулирует доказательство только через отсутствие в `_devices`, без явной + проверки маркера) — не смог дёшево установить, входят ли такие маркеры в + `options.devices` по построению; если нет — AC9 в текущей формулировке может + требовать удаления записи, которую контракт просит сохранить. Не поднимаю + до отдельной находки: не смог воспроизвести конкретный сценарий отказа за + разумное время, это кандидат на внимание в код-ревью, а не блокер ТЗ. +- Гейты (`tsc`/`test`/`build`) не гонял — на этапе spec кода ещё нет, дерево + ветки состоит из одного doc-коммита. + +## Вердикт + +Вердикт: жёлтый · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 2 → в задаче