mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -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) — терминология вкладки, правится попутно.
|
||||
Reference in New Issue
Block a user