From 48308209c2952f4ee1e8ff8d5f26c102d1dfbe28 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 20 Aug 2026 20:27:01 +0000 Subject: [PATCH] docs: review document for #220 Issue: #220 User-Visible: no --- docs/reviews/SPEC-REVIEW-220-r2.md | 185 +++++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-220-r2.md diff --git a/docs/reviews/SPEC-REVIEW-220-r2.md b/docs/reviews/SPEC-REVIEW-220-r2.md new file mode 100644 index 00000000..cc895dd7 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-220-r2.md @@ -0,0 +1,185 @@ +# SPEC-REVIEW-220-r2 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/220 +- **ТЗ:** `docs/specs/220-space-tab-reorder.md` (текущий коммит `e57e1d9`, + «docs: close M1 and M2 from the spec review of #220») +- **Ревьюер:** Claude (роль «ревьюер ТЗ», PROCESS.md §2.4) +- **Заход:** r2 · блокирующих циклов 1/4 +- **Вердикт:** жёлтый · High: 0 · Medium: 1 (в скоупе задачи) + +## Скоуп ревью + +Разбор по дельте (PROCESS.md §2.9, #214), а не заново — с одним расширением. +Раунд r1 закрыл два Medium (M1, M2) точечной правкой текста ТЗ, но правка по +M2 не косметическая: §8.3 полностью переписан — вместо «хранить якорь в +`settings`» теперь «материализовать привязку в той же записи». Это смена +контракта поведения (новая нормативная процедура записи конфигурации), а не +переформулировка факта, поэтому именно этот кусок разобран полно — включая +сверку с кодом `devices.ts`/`houseplan-card.ts` заново, а не по памяти r1. +Остальное (§1–§7, §10–§16, AC1/2/4/5/6/7/8, мутационный гейт вне AC3, план +автотестов) дельта не касается — унаследовано из r1 без повторной проверки +(раздел ниже). + +## Как проверялось + +1. Найден вердикт r1 в комментариях issue #220 + (`https://github.com/Matysh/houseplan-card/issues/220#issuecomment-5361187761`, + `2026-08-20T20:17:49Z`) и документ `docs/reviews/SPEC-REVIEW-220-r1.md`, + зафиксировавший ТЗ на коммите `0fd2331`. +2. Объявлена дельта: `git diff 0fd2331..HEAD -- docs/specs/220-space-tab-reorder.md` + (HEAD = `e57e1d9`). Дельта — 58 вставок / 19 удалений, только §8.3, §9, AC3, + мутационная таблица, §17.3; остальные разделы файла побитово не менялись. +3. Комментарий владельца о закрытии r1 называет коммит `d0a5bfa` — такого + объекта в репозитории нет (`git cat-file -t d0a5bfa` → `fatal: Not a valid + object name`), вероятно переписан ребейзом веток issue. Не доверяю + названному SHA, проверка сделана по факту — прямым диффом файла между + `0fd2331` (зафиксирован в документе r1) и текущим `HEAD` (`e57e1d9`, + единственный коммит после `c3278dd`/review-doc r1, который трогает файл + ТЗ) — содержимое дословно совпадает с тем, что владелец описал в + комментарии, расхождение только в имени SHA в тексте комментария. +4. Каждая находка r1 (M1, M2) сверена не по заявлению автора, а по строке + текста ТЗ — см. таблицу «Закрытие раунда r1». +5. `docs/TOUCH-SUPPORT.md` (§153–165, «Documentation rule») прочитан повторно, + построчно сверена ровно та формулировка ярлыка, которую требует правило. +6. Новая нормативная процедура §8.3 (материализация в той же записи) сверена + с фактическим кодом разрешения `firstSpaceId`: + `src/devices.ts:1045-1065` (`resolveExplicitMarkerPlacement`, включая ветку + `manualRoomWithoutArea`, ранее не разбиравшуюся отдельно ни в issue, ни в + r1) и `src/devices.ts:1249` (виртуальный маркер). Оба случая подтверждают + общий критерий ТЗ «нет ни `area`, ведущей в пространство, ни собственного + `space`» — он корректно обобщает все три ветки кода, включая ветку + marker-без-HA-area из #3, которую ТЗ не называет по номеру, но покрывает + по содержанию. + Также проверено третье использование `firstSpaceId` + (`src/houseplan-card.ts:18818`, черновик маркера в диалоге) — это + непостоянный preview, не пишется в конфиг, к обязательству §8.3 + («ни один маркер не меняет `space`») не относится: диалог всегда + пересчитывает его заново по актуальной модели. +7. AC3 (единственный AC, задетый дельтой) перепроверен на однозначность и + доказуемость с учётом новой формулировки. AC1, AC2, AC4–AC8 дельтой не + задеты — унаследованы из r1 без повторной проверки. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **M1** — нет обязательной touch-классификации по `docs/TOUCH-SUPPORT.md` §153 | В §9 добавлена строка **`Touch editor: not exposed.`** с обоснованием (вкладки живут во View, где переключение — fully supported/release-blocking; жест на вкладке рискует съесть тап) и отдельно — что safety floor §69 соблюдён по построению | `docs/specs/220-space-tab-reorder.md:156-165`; формулировка ярлыка совпадает с требуемой буква в букву (сверено с `docs/TOUCH-SUPPORT.md:160-162`) | +| **M2** — §9 отрицал новое поле конфигурации, §8.3/§17.3 предполагали якорь в `settings` | §8.3 переписан: вместо якоря — материализация существующего поля `marker.space` в той же записи, что и порядок; §9 теперь говорит «новых полей конфигурации не появляется... см. §8.3», и это уже не противоречит остальному документу, а согласуется с ним; §17.3 переписан в терминах материализации, старое предположение про якорь явно помечено отвергнутым | `docs/specs/220-space-tab-reorder.md:107-139` (§8.3), `:167-170` (§9), `:268-271` (§17.3); AC3 (`:202-206`) и мутационная таблица (`:240-241`) синхронно обновлены под новую механику | + +Обе находки закрыты не декларативно: правка убирает саму причину +противоречия (M2) и добавляет предметное содержание, а не просто ярлык (M1). +Новых полей конфигурации в изменённом варианте действительно не появляется — +проверено по коду: материализация пишет уже существующее поле маркера +`space`, `CONFIG_SCHEMA` и `scripts/config-field-registry.mjs` в этой задаче +не упоминаются как изменяемые, и это утверждение больше не противоречит §9. + +## Унаследовано из r1 + +Принято без повторной проверки в этом раунде — проверено и подтверждено в +`docs/reviews/SPEC-REVIEW-220-r1.md` на коммите `0fd2331`, дельта эти разделы +не касается: + +- Персона, сценарий и job `docs/SCOPE.md` J6 (§1–§2 ТЗ). +- Продуктовые решения владельца §4 (тач/права/клавиатура) и их соответствие + owner-decision в комментариях issue. +- §8.1, §8.2, §8.4, §8.5 контракта (порог 4px, `.tabadd`, запись через + `_writeConfig`/`expected_rev`, `swipeTarget`, тост о числовом `floor`). +- AC1, AC2, AC4, AC5, AC6, AC7, AC8 — однозначность и способ доказательства. +- Мутанты `tab-reorder-not-persisted`, `tab-reorder-eats-click`, + `tab-reorder-ignores-pointer-type` (не переписаны в дельте). +- §11 (риски), §15 (release-артефакты), §16 (откат) кроме уже отражённых в + дельте формулировок. +- Low-находка r1 про `.tabedit`/`pointerdown` — оставлена без правки + экспертным решением r1 («реализационная деталь, накрываемая кодревью»), + дельта её не касается, пересматривать нет причины. + +## Находки + +### M3 — Medium, в скоупе. §17.3 маркирует нормативное требование как «свободно меняемое» + +Заголовок §17 — «Принятые предположения (**техническое, менять свободно**)». +Пункт 3 внутри него (`docs/specs/220-space-tab-reorder.md:268-271`) гласит: +«Материализация привязки (§8.3) выполняется в том же `config/set`, что и +порядок, а не отдельной записью: две записи дали бы окно, в котором порядок +уже новый, а привязка ещё старая». + +Но именно это же самое требование в §8.3 названо не предположением, а +**«Норматив»** (`:113`, буквально это слово стоит заголовком абзаца) — и оно +уже вошло в тестируемый контракт: AC3 требует «маркер получает явное `space` +**в той же записи**» (`:203-204`), а мутант `reorder-skips-materialization` +(`:240`) специально ловит его нарушение. + +**Почему это не формальность.** §17.3 своим же текстом объясняет, почему +атомарность нельзя менять свободно: если порядок и материализация уйдут +двумя разными записями `config/set`, то в промежутке между ними +`firstSpaceId = model[0]?.id` уже пересчитан по новому порядку — маркер, +ещё не материализованный, в это окно резолвится в **не то** пространство. +Если исполнитель прочитает заголовок §17 буквально («менять свободно») и +раздельными записями, а не заголовок §8.3 («Норматив»), он получит ровно тот +риск, ради которого писан весь раздел 8.3 и ради которого задача оценена в +сложность 4/10 (см. §11, риск №2 «Маркеры уезжают» — главный риск задачи). +Опасность усугубляется тем, что при таком (неверном) выборе реализации +собственный юнит-тест AC3, написанный тем же исполнителем под ту же +(неверную) модель, скорее всего будет проверять «согласованность после двух +записей», а не «атомарность одной записи» — то есть мутационный гейт +`reorder-skips-materialization` перестанет быть надёжным барьером именно в +том сценарии, для которого его писали. + +**Чем закрывается:** убрать пункт 3 из §17 (он не является свободным +предположением — норма уже сформулирована в §8.3 как обязательная и +проверяется AC3), либо явно пометить его как исключение из «менять свободно» +с отсылкой на §8.3/AC3. Правка текстовая, AC и скоуп не меняются. + +## AC — что перепроверено дельтой + +| AC | Задет дельтой | Однозначен | Доказательство | Комментарий | +|---|---|---|---|---| +| AC1, AC2, AC4, AC5, AC6, AC7, AC8 | нет | — | — | унаследованы из r1 без повторной проверки | +| AC3 | да | да | `unit` (`buildDevices` + запись) | формулировка усилена («маркеры с `area`/`space` — побитово прежними»), однозначна и доказуема; ровно вокруг него — находка M3 (не сам AC, а соседний раздел §17.3, который создаёт риск неверной трактовки реализации, проверяемой этим же AC) | + +## Что проверено и корректно + +- M1 и M2 закрыты предметно, а не декларативно — см. таблицу выше; §9 больше + не противоречит §8.3. +- Новая механика §8.3 (материализация) корректно обобщает все три реальных + пути резолюции `firstSpaceId` в `devices.ts`, включая ветку + `manualRoomWithoutArea` (маркер ручной комнаты без HA area, контракт #3), + которую ни issue, ни r1 не разбирали пофамильно — критерий «нет `area`, + ведущей в пространство, и нет собственного `space`» покрывает её без + исключений. +- Материализация не задевает preview-путь черновика маркера + (`houseplan-card.ts:18818`) — он не персистентный, обязательство §8.3 на + него не распространяется, и в ТЗ об этом ничего лишнего не заявлено. +- Новая пара мутантов (`reorder-skips-materialization`, + `materialization-touches-bound-markers`) корректно закрывает ровно два + направления ошибки материализации (пропуск и избыточность), заменив собой + устаревший `first-space-follows-order`. +- Владелец сам в комментарии честно отделил решение от подгонки: назвал две + альтернативы снятия противоречия M2 («признать поле» / «обойтись без + него») и выбрал вторую с объяснением — это ровно то, что PROCESS требует + от продуктовых/технических решений, выносимых в ревью. + +## Чего не проверял + +- Не проверял реализацию — кода ещё нет, это ревью ТЗ. +- Не запускал `tsc`/`npm test`/`npm run build` — класс изменения C + (документация), гейты неприменимы, как и в r1. +- Не пересматривал разделы, не тронутые дельтой (§1–§7, §10–§16, AC1/2/4-8, + три из пяти мутантов) — они унаследованы из r1 на коммите `0fd2331`, дельта + их не меняла ни байтом. +- Не проверял точность SHA `d0a5bfa`, названного владельцем в комментарии о + закрытии r1 — объект не существует в репозитории; вместо этого верификация + сделана по прямому диффу файла до `HEAD` (`e57e1d9`), см. «Как + проверялось», п.3. Расхождение SHA не влияет на вывод ревью, но зафиксировано + как наблюдение. + +## Итог + +Обе находки r1 закрыты предметно. Правка §8.3 (материализация вместо якоря) +— смена контракта поведения, а не косметика, и именно в ней найдена новая +находка M3: §17 маркирует как «свободно меняемое» требование, которое сам же +документ в §8.3 называет обязательным норматив и проверяет тестируемым AC3. +Находка в скоупе, чинится точечной правкой текста (перенос/каветирование +пункта 17.3), без изменения AC, скоупа или контракта поведения. High-находок +нет. Вердикт — жёлтый: правка проходит третий цикл ревью ТЗ (заход r3, лимит +циклов ревью ТЗ — 4).