diff --git a/docs/reviews/SPEC-REVIEW-44-r1.md b/docs/reviews/SPEC-REVIEW-44-r1.md new file mode 100644 index 00000000..bef0b5f5 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-44-r1.md @@ -0,0 +1,244 @@ +# SPEC-REVIEW-44-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/44 +- Этап: spec (ревью ТЗ, PROCESS.md §2.4) +- Заход: r1 · блокирующих циклов израсходовано 0/4 +- ТЗ: `docs/specs/044-filter-grouping-policy.md`, ревизия 2, зафиксирована коммитом + `3d4c5090` (только этот файл, `docs/specs/044-filter-grouping-policy.md`, + 180 добавлено / 56 удалено — см. `git show --stat 3d4c5090`) +- Вердикт: **красный** + +## Скоуп ревью + +Полный разбор ТЗ #44 ревизии 2 (первый заход, дельты нет). Сверялись: + +1. `docs/SCOPE.md` — задача закрывает пункт J6 issue-текста («третий вариант» — + скрытая настройка с видимым эффектом — запрещён), не конфликтует со SCOPE. +2. `AGENTS.md`, `PROCESS.md` §7.1, §5 — трек полный (не `small`), файл ТЗ + обязателен и существует; проверены обязательные разделы. +3. Тело issue #44 и три комментария (актуализация 2026-08-14, вход в работу и + ревизия 2 от 2026-08-30) — сверка фактов кода, на которые ссылается автор. +4. `docs/USER-GUIDE.ru.md` §10 «Устройства» — терминология вкладок инбокса. +5. Код, который ТЗ описывает как «факты»: `src/devices.ts`, `src/rules.ts`, + `src/houseplan-card.ts`, `src/space-render.ts`, `src/device-inbox.ts`, + `src/houseplan-editor-runtime.ts`, `src/i18n/*.json`, + `scripts/config-field-registry.mjs`. + +## Как проверялось + +Построчная сверка каждого числового/файлового факта из раздела «Проблема», +«Скоуп» и «Контракт поведения» ТЗ с текущим деревом (`git log` подтверждает, +что рабочая копия — это коммит `3d4c5090`, единственный докс-коммит #44 в этой +ветке). Инструменты: `Grep`/`Read` по указанным файлам и строкам, `git log -S` +для датировки происхождения кода, на который ссылается ТЗ как на «уже +существующее». + +## Находки + +### H1 — Сценарий и AC4 построены на неверном факте: причина «исключена +интеграция» и появление кандидата в инбоксе уже существуют сегодня, но не там, +куда их помещает ТЗ + +**Файл:** `docs/specs/044-filter-grouping-policy.md`, раздел «Что человек +увидит до и после» (строка 23) и «Скоуп → 2. Причина „Исключена интеграция X“» +(строки 69–75), AC4 (строка 128). + +**Утверждение ТЗ:** «До: … понять „почему этого устройства нет в списке“ +нельзя. […] Сегодня такие устройства не попадают в инбокс вовсе — они +появляются на «Скрытых» именно с этой причиной» и AC4: «скрытый фильтром +кандидат виден на «Скрытых» с причиной «Исключена интеграция X»». + +**Что показывает код:** +- Значение `excluded_integration` типа `DeviceInboxReason` и одноимённый i18n- + ключ `device_inbox.reason_excluded_integration` уже существуют и не новые: + добавлены коммитом `cab8d128 feat: add device lifecycle catalog` (issue #29), + задолго до ревизии 2 этого ТЗ (`git log -S excluded_integration` → + `cab8d128`). Тексты уже переведены на все 4 языка + (`src/i18n/en.json:484`, `de.json:484`, `fr.json:484`, `ru.json:484`), + сегодняшний текст — не с плейсхолдером: «Integration excluded by device + filters» / «Интеграция исключена фильтрами устройств» и т.д. — обобщённая + фраза без имени интеграции. +- Уже сегодня `src/houseplan-editor-runtime.ts:7635-7637` считает + `excluded = […].some((domain) => this.host._excluded.has(domain))` (тот же + резолвер `_excluded`, что ТЗ описывает как «единственный источник» в + Контракте №3) и присваивает `reasonByBinding[binding] = 'excluded_integration'` + ещё до всякой доработки — то есть кандидат УЖЕ размечен этой причиной. +- Эта причина уже рендерится пользователю безусловно для любой категории + строки (`src/houseplan-editor-runtime.ts:11848-11851`, + `${this.host._t(\`device_inbox.reason_${row.reason}\` as any)}` — вызов не + зависит от `row.category`). +- Категория строки для такого кандидата — не «Скрытые», а «Доступны»: + `buildDeviceInbox` (`src/device-inbox.ts:218-223`) присваивает + `category = 'hidden'` **только** когда есть маркер `live?.hidden === true`; + кандидат без runtime/live и без маркера получает `category = 'available'` + (строка 222), а `reasonByBinding[binding]` подставляется именно в ветке + «иначе» для *этой* категории (`src/device-inbox.ts:234-239`). Категория + `'hidden'` в принципе не рассматривает `reasonByBinding` для кандидатов без + маркера — код это структурно не допускает. + +**Воспроизведение (проверено чтением, не исполнением):** возьмите +конфигурацию без маркера на устройство с платформой из `EXCLUDED_DOMAINS` +(или из `settings.exclude_integrations`). Откройте диалог «Устройства» уже +на сегодняшнем `dev`. Кандидат уже виден на вкладке «Доступны» (не «Скрытые») +со строкой причины «Integration excluded by device filters» — это происходит +без единой строки нового кода, только из-за существующей связки +`houseplan-editor-runtime.ts:7637` → `device-inbox.ts:238` → +`houseplan-editor-runtime.ts:11849`. + +**Почему это блокирует:** ТЗ выдаёт неверное описание сегодняшнего поведения +за факт (не за предположение) и строит на нём и сценарий, и AC4. Раздел +«Принятые предположения» (строка 183: «причина показывается на «Скрытых», НЕ +в отдельной новой вкладке») маскирует ровно эту развилку решением, но не +называет техническое противоречие: сегодняшняя категоризация структурно не +кладёт такого кандидата на «Скрытые» без отдельного изменения контракта +`buildDeviceInbox`, которого раздел «Контракт поведения» не содержит (там +только 5 пунктов, ни один не про категорию строки). AC4 в текущей +формулировке невыполним без незаявленного изменения категоризации, а смок, +написанный по AC4 «как есть», либо провалится на реальной категории +(«available», не «hidden»), либо будет молча ослаблен под фактическое +поведение — оба исхода авторами не решены, а решение здесь ровно продуктовое +(«на какой вкладке пользователь видит причину») и должно быть либо переписано +по факту, либо вынесено владельцу одним вопросом с вариантом по умолчанию +(PROCESS.md §7.1). + +**Что нужно поправить:** переписать «Проблема»/сценарий по фактическому +сегодняшнему поведению (причина и её текст уже есть, только обобщённые и на +вкладке «Доступны»), решить и явно записать: 1) остаётся ли причина на +«Доступны» (тогда AC4 меняет текст на плейсхолдерный, но не трогает +`buildDeviceInbox`) или 2) вводится изменение категоризации, переносящее такие +строки на «Скрытые» (тогда это новый пункт Контракта поведения и отдельный +риск — категория `hidden` сегодня зарезервирована за маркерами `hidden: true`, +смешение с «нет маркера вовсе» меняет смысл вкладки для остальных находок). + +### H2 — Контракт №3 («оба значения читаются из одного источника всеми +потребителями») не выполняется уже сегодня для `exclude_integrations`, и ТЗ +не замечает единственного потребителя-исключения + +**Файл:** `docs/specs/044-filter-grouping-policy.md`, «Проблема» (строка 34: +«потребители `houseplan-card.ts:3924`, `space-render.ts:245`, discovery +`devices.ts:1491`») и «Контракт поведения» п.3 (строка 98). + +**Что показывает код:** `src/devices.ts:1491` — это НЕ потребитель +настраиваемого `ctx.excluded`/`_excluded`. Строка лежит внутри +`roomClimateMap` (объявление `src/devices.ts:1435`, сигнатура +`roomClimateMap(hass, rules?, markers?)` — параметра `settings`/`excluded` в +ней нет вовсе) и хардкодит продуктовый `EXCLUDED_DOMAINS`, импортированный +напрямую из `rules.ts` (`src/devices.ts:5`): +``` +if (EXCLUDED_DOMAINS.has(reg.platform)) continue; // filtered-out integrations +``` +Это единственное место в `src/**`, где `EXCLUDED_DOMAINS.has(` вызывается +напрямую, минуя настраиваемый резолвер (`grep -rn "EXCLUDED_DOMAINS\.has\(" +src` → одно вхождение). `lightGroups()` (второй ключ, `group_lights`) такой +проблемы не имеет — оба его вызова (`devices.ts:1035`, `:1100`) читают +`settings.group_lights` честно. + +**Почему это в скоупе, а не соседний баг:** issue #44 прямо требует «для +каждого ключа выбрать: 1) поддерживаемая настройка с понятным эффектом […] 2) +фиксированное поведение. Нельзя оставлять третий вариант — скрытую настройку, +которая влияет на результат, но не видна пользователю». `roomClimateMap` +(агрегирование температуры/влажности комнаты, J5) — это ровно третий вариант +для `exclude_integrations`: после того как пользователь через новый UI уберёт +интеграцию из исключений, устройство появится в списке комнаты, но его +температурные/влажностные показания по-прежнему будут молча исключаться (или +не исключаться) по продуктовому дефолту — независимо от выбора пользователя. +И наоборот: добавление интеграции в исключения через UI не уберёт её показания +из climate-агрегата. Это именно то расхождение, ради ликвидации которого +заведён #44, только оно остаётся необнаруженным самим ТЗ. + +**Воспроизведение (проверено чтением, не исполнением):** установить +`settings.exclude_integrations: []` (пользователь явно снял все исключения +через «Вернуть рекомендуемые» → нет, это удаляет ключ; для эффекта нужно явно +задать `[]` — «ничего не исключать», валидное значение по «Принятым +предположениям» ТЗ). После этого `_excluded` в `houseplan-card.ts:3925` вернёт +пустой `Set`, устройства всех интеграций попадут в `buildDevices`. Но +`roomClimateMap` по-прежнему исключит их показания температуры/влажности по +жёстко зашитому `EXCLUDED_DOMAINS`, потому что в её сигнатуре нет входа для +пользовательского значения. Пользователь увидит устройство на плане, но не +увидит его вклад в комнатный climate-показатель, и UI, который ТЗ обещает +(«честные представления», «понятный эффект»), не объяснит почему. + +**Почему это блокирует:** Контракт поведения — раздел, на который ссылаются +AC (AC7 — «отсутствие ключей → выдача байт-в-байт», проверяет только +`buildDevices`, не climate-агрегат) и release-текст «нет скрытых +discovery-ключей» (Release-артефакты, ARCHITECTURE.md). Ложное утверждение +«читается из одного источника всеми потребителями» — это ровно тот случай, +когда «утверждение о поведении, которого нет ни в одном документе, выдано за +факт»: инвентаризация (тот самый пункт «инвентаризация exclude_integrations… +на реальных конфигах», который issue требует первым делом) пропустила +единственного потребителя-нарушителя. ТЗ должно либо явно перевести +`roomClimateMap` на настраиваемый резолвер (тогда это новый пункт +контракта/AC и новая поверхность — `roomClimateMap`, J5), либо осознанно +задокументировать расхождение как принятое ограничение с явным «не-скоуп» и +предупреждением в UI/USER-GUIDE («фильтр не влияет на климат-агрегацию») — +сейчас не сделано ни то, ни другое. + +### L1 — Название вкладки инбокса не совпадает с интерфейсным словарём + +**Файл:** `docs/specs/044-filter-grouping-policy.md`, «Сценарий» (строка 13), +«Скоуп → 1» (строка 48) и везде далее — «вкладка «Доступные»». + +Канонический термин из `docs/USER-GUIDE.ru.md:785` и живого +`src/i18n/ru.json:457` — **«Доступны»** (`device_inbox.tab_available`), не +«Доступные». AGENTS.md требует брать терминологию интерфейса из +USER-GUIDE, а не изобретать. Не блокирует (Low, по решению ревьюера правится +автором при следующей правке текста ТЗ, отдельного цикла не требует). + +## Что проверено и корректно + +- Резолвер `group_lights` (`devices.ts:1033`, `:1098` — `!== false`, default + TRUE) и `exclude_integrations` (`houseplan-card.ts:3924-3925`, + `space-render.ts:245-246` — replace-семантика, не additive) описаны точно, + номера строк совпадают день в день. +- `EXCLUDED_DOMAINS` (`rules.ts:12`) — 13 доменов, совпадает с описанием. +- `scripts/config-field-registry.mjs:42-70` — оба ключа сегодня + `status: 'decision-required'`, `ui: 'hidden'`, паспорта `allow-extra` — ТЗ + переносит их в `current` с `ui`-путём в инбоксе; статус `current` + используется в реестре многократно (6 других полей), формат совпадает. +- `expected_rev` (оптимистическая блокировка, #340) реально существует в + `houseplan-card.ts`/`houseplan-editor-runtime.ts`; шаблон удаления ключа при + возврате к дефолту уже применён для `settings.weather_entity` + (`enforcedBy` в реестре) — техническое решение ТЗ опирается на существующий + прецедент, а не выдумано. +- `buildDevices(ctx: BuildCtx)` (`devices.ts:1092`) — чистая функция, принимает + `settings`/`excluded` через `ctx`, не делает I/O; план AC6 («превью — тот же + вход, что у боевого buildDevices, без копии логики фильтра») технически + реализуем как заявлено. +- `[]` как валидное «ничего не исключать», отличное от отсутствия ключа — + подтверждено кодом (`list ? new Set(list) : EXCLUDED_DOMAINS`, строка + разбора верна). +- Обязательные разделы §7.1 присутствуют все (сценарий, что увидит человек, + проблема, скоуп/не-скоуп, контракт, UX/i18n, модель данных и миграция, AC, + план автотестов, риски, откат, release-артефакты); «Принятые предположения» + оформлены отдельным блоком, как требует PROCESS.md, хотя (H1) не покрывают + реальное техническое противоречие. +- Трек (полный, не `small`) выбран правильно и соответствует решению + аналитики от 2026-08-15, зафиксированному в issue. + +## Чего не проверял + +- Гейты `typecheck`/`test`/`build` не запускались: на этом этапе (S4, + ревизия ТЗ) изменён только `docs/specs/044-filter-grouping-policy.md` + (коммит `3d4c5090`, `git show --stat` подтверждает единственный файл); + продуктовый код не тронут, гейты неприменимы к чистому докс-коммиту. + `node scripts/check-docs.mjs` не запускался по той же причине (diff не + касается `src/**`). +- Смоки/golden/backend/perf — неприменимо, кода нет. +- Не проверялась истинность утверждения «инфраструктура для UI появилась + #29 … паспорта уже выданы в #33» сверх того, что напрямую процитировано + выше (паспорта `allow-extra` подтверждены реестром; сам факт наличия + вкладок/`DeviceInboxReason` подтверждён и лёг в основу H1). +- Не оценивалась производительность превью-диффа (буст `buildDevices` на + больших registry) — вопрос реализации, не ТЗ; риск назван самим автором и + выглядит разумно (пересчёт по явному действию, не на тик). + +## Итог + +Вердикт: красный. Два High: ложное описание сегодняшнего поведения инбокса +(H1, делает AC4 невыполнимым как написано и маскирует нерешённый продуктовый +вопрос про вкладку) и незамеченный потребитель `EXCLUDED_DOMAINS` в +`roomClimateMap`, нарушающий заявленный контракт «единственный источник» +(H2, оставляет ровно тот «третий вариант», ради ликвидации которого заведён +issue). Оба — фактические ошибки инвентаризации кода, а не вкусовщина; +обе воспроизведены чтением конкретных строк, обе in-scope issue #44. Low +(L1) — терминология вкладки, правится попутно.